Skip to content

fix(cli): refuse a present non-array packages in the stack-collection and docs readers - #20231

Merged
objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-19925-cli-non-array-packages-refusal
Sep 27, 2026
Merged

objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/issue-19925-cli-non-array-packages-refusal

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #19925

Clause-②: no (narrowing)

What changed

Ruling A on #15293 (5634034754) says that a packages which is present but is not an array ({}, 0, 'x') is malformed, not absent, and that every reader refuses it. The runtime, @objectstack/core and the plugin readers already refused it. The four @objectstack/cli readers answered "no packages" instead. They now refuse it with core's own INVALID_ARTIFACT_PACKAGES refusal (ADR-0112, status: 422).

  • stack-collections.ts gets ONE exported helper, declaredPackageEntries(packages):

    The helper spells neither the rule nor the refusal, and no new code is minted.

  • packageBodies routes through the helper. So do docsPackageRefs, bodyDocsOf and attachPackageDocs in collect-docs.ts. None of the four keeps its own Array.isArray guard.

  • New pins in packages/cli/test/non-array-packages-readers.test.ts (16 tests). Each refusal asserts code + status.

  • New pins in packages/cli/test/null-packages-follows-resolver.test.ts (16 tests), added in round 1:

    A private null branch in the CLI never asks the resolver, so today only leg 2 can see one.

  • The existing row docsPackageRefs › "is empty for anything that is not an array" (src/utils/collect-docs.package-docs.test.ts) pinned [] for {}, which is the retired answer. It now asserts the refusal envelope and keeps its absent control.

  • Changeset .changeset/19925-cli-non-array-packages-refusal.md: @objectstack/cli minor, Clause-②: no (narrowing), a BREAKING paragraph, and the ADR-0087 disposition not-required (no-migration-prescription). Round 1 replaced its null sentence with a paragraph saying that null follows core's resolver (ruling 5805260775).

Reach, measured: why the changeset narrows

The built CLI (packages/cli/bin/run.js) was run on ce70876e4 (before) and on this branch (after). Each fixture is a hand-written objectstack.config.ts whose default export is a plain object carrying a manifest and the packages value shown.

packages command before after
{} / 0 / 'x' os info --json exit 0, stats.objects: 0 exit 1, INVALID_ARTIFACT_PACKAGES
{} / 0 / 'x' os lint --json exit 0, passed: true exit 1, INVALID_ARTIFACT_PACKAGES
{} / 0 / 'x' os validate --json exit 1, schema invalid_type at packages unchanged
{} os build --json exit 1, schema invalid_type at packages unchanged
{} os serve exit 1, core's INVALID_ARTIFACT_PACKAGES sentence raised during boot unchanged
well-formed array os info / os lint exit 0: 1 object / passed: true unchanged
absent os info / os lint exit 0: 1 object / passed: true unchanged
inlined entry (lit control) os info / os lint exit 1, INVALID_ARTIFACT_PACKAGE_ENTRY unchanged

With its default strict parse, defineStack(…) refuses packages: {} at config load (STACK_SCHEMA_INVALID). Only two spellings reach os info / os lint with the shape: a plain-object export, and defineStack(…, { strict: false }). Both were measured, and both behave as the table says.

So two public doors ACCEPTED the shape and answered success. The claim carried Clause-②: no. This PR follows the same-door precedents (.changeset/19120-install-door-parses-manifest-version.md, .changeset/19417-install-door-parses-manifest-id.md) and declares Clause-②: no (narrowing) with a minor bump instead.

The four readers, before and after

The readers were called from source with a well-formed control and an absent control.

reader {} / 0 / 'x' before after well-formed absent
packageBodies (via resolveStackCollection) [] INVALID_ARTIFACT_PACKAGES, 422 the body's objects, unchanged [], unchanged
docsPackageRefs [] INVALID_ARTIFACT_PACKAGES, 422 the package ids, unchanged [], unchanged
bodyDocsOf [] refuses through the helper the body's docs, unchanged [], unchanged
attachPackageDocs the same reference back INVALID_ARTIFACT_PACKAGES, 422 attaches, unchanged the same reference back, unchanged

bodyDocsOf has one caller, collectAndLintDocs, which calls it only for refs that docsPackageRefs produced from the same value. So a non-array never reached it, before or after. It uses the helper anyway, so that no reader keeps a guard of its own. The ablation below confirms that no pin can observe it.

Ablation

The fix was committed first (67d5b4829). The pin file imports src/ relatively, so no dist/ leg is involved. Each leg went through scripts/ablation-replace.mjs: the anchor went 1 → 0, the injected text 0 → 1, and the blob changed. The leg then ran the pin file, and the tool restored the file (blob equal to HEAD, git diff HEAD empty). At the end, git hash-object of both files matched their HEAD blobs.

leg mutation pin file
A the helper's non-array branch answers [] 12 failed / 4 passed
B packageBodies gets its old Array.isArray guard back 6 failed / 10 passed
C docsPackageRefs gets its old guard back 3 failed / 13 passed
D attachPackageDocs gets its old guard back 3 failed / 13 passed
E bodyDocsOf gets its old guard back 16 passed: unreachable, the expected direction

Round 1: the fix was committed first (5bac82635). The leg restored the private null branch, if (packages === undefined || packages === null) return [];, through scripts/ablation-replace.mjs: the anchor went 1 → 0, the injected text 0 → 1, and the blob c8c830d2e25b → 78739bfea3c8.

  • On the two pin files, the unmutated HEAD gave 32 passed.
  • The mutation gave 7 failed / 25 passed. All 7 leg-2 rows went red, and leg 1 stayed green, which is expected today.
  • The restore was proven: blob == HEAD (c8c830d2e25b), git diff HEAD empty, status clean.

Verification

Round 1, all on HEAD ae8e3e83f, after merging origin/main at 3cb84d084.

  • pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2: 229 files, 3250 tests passed. The integration tier is left to CI, because the diff touches no spawn entry and no integration-tier file.
  • pnpm --filter @objectstack/cli typecheck: exit 0. check:test-typecheck compiles both pin files under tsconfig.test.json.
  • pnpm lint over the whole repo: exit 0.
  • node scripts/pm/dispatch-gates.mjs --commands derived 62 commands. All 62 exited 0, and the --ran reconciliation reports 62 derived, 62 run, 0 NOT-MEASURED, 0 UNRUN. As in round 0, check:dual-build-cjs-loads and check:i18n-coverage first exited 3 (PREREQUISITE NOT MET, some dist/ missing). Both were re-run on the same HEAD once the builds were present, and exited 0.
  • node scripts/check-adr-0087-registration.mjs --base origin/main and node scripts/check-changeset-no-major.mjs --base origin/main: exit 0 each.
  • node scripts/check-issue-citations.mjs --base origin/main: exit 0, with 6 citations resolving.
  • packages: null on the built CLI, with a plain-object config:
    • On this HEAD, os info --json exits 0 (stats.objects: 0) and os lint --json exits 0 (passed: true). Both are unchanged.
    • A throwaway worktree of this HEAD was given PR fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228's packages/core/src/artifact-packages.ts (0706ffef7), and core was rebuilt. There, both commands exit 1 with INVALID_ARTIFACT_PACKAGES, and all four readers refuse null, with no CLI edit. Both pin files pass there too (32 of 32), and leg 1 takes the refusal branch.
    • The worktree was removed afterwards, and nothing from it was committed.

Round 0, on a89a8e528: 228 files / 3234 tests passed, and typecheck, lint, all 62 gates and the citation check each exited 0.

Not in this PR

Acceptance notes

  • File surface. The claim named "their tests in packages/cli/test/". The new pins live there. The existing docsPackageRefs row sits beside its source in packages/cli/src/utils/collect-docs.package-docs.test.ts, and it pinned the retired answer, so it had to change in this PR.

  • artifactPackages (packages/cli/src/utils/artifact-packages.ts) keeps its own !Array.isArray guard. That file is outside the claimed surface, so it is untouched. Its docblock says it reads the PARSED stack. Every caller reaches it only after the schema parse, or after one of the four readers has judged the same value:

    • compile.ts, validate.ts, nav-contribution-groups and permission-set-name-collisions pass parsed data;
    • lint.ts, sdui-manifest.ts and lintDocNavTargets run after authoringRuleUnionStack or docsPackageRefs.

    So no public door reaches it with a non-array now. Its carrier is whichever PR next touches that file.

  • os serve. serve.ts wraps collectDocsFromSrc in a catch-all ("docs are additive — never block boot"). A docsPackageRefs refusal is swallowed there. The same boot then refuses the stack through shouldAutoRegisterObjectQL and the manifest service. Measured: os serve still exits 1 with the same sentence.


Generated by Claude Code

…on and docs readers

`packageBodies`, `docsPackageRefs`, `bodyDocsOf` and `attachPackageDocs`
answered "no packages" for `packages: {}` / `0` / `'x'`. They now judge the
value through one helper, `declaredPackageEntries`, which hands a present
non-array to `resolveArtifactPackageOrder` so the refusal is core's own
`INVALID_ARTIFACT_PACKAGES` (ADR-0112, 422). Absent (`undefined` / `null`)
still reads as no packages, and an array is walked exactly as before.

Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP
Co-authored-by: Claude <noreply@anthropic.com>
Clause-② no (narrowing): `os info` and `os lint` exited 0 for a hand-written
stack carrying `packages: {}` / `0` / `'x'`, and now refuse it with
`INVALID_ARTIFACT_PACKAGES`.

Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP
Co-authored-by: Claude <noreply@anthropic.com>
…s: {}`

That row held the silent "no packages" answer the reader no longer gives. It
now asserts the `INVALID_ARTIFACT_PACKAGES` envelope, and keeps the absent
control.

Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 27, 2026
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

5 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 54 of 206 client-bound route-ledger rows — the other 152 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 152: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 97 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 25 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 3cb84d084efa5095089de71899fe7b304f8f8993 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 467f7faa3904e91f8abd8fc9095bc49b54e8e01e — the merge of head ae8e3e83f734dc33a4937af5221c1a41b7dce6f2 into base 3cb84d084efa5095089de71899fe7b304f8f8993, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 467f7faa3904e91f8abd8fc9095bc49b54e8e01e && git checkout 467f7faa3904e91f8abd8fc9095bc49b54e8e01e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 3cb84d084efa5095089de71899fe7b304f8f8993 ae8e3e83f734dc33a4937af5221c1a41b7dce6f2 && git checkout -B drift-repro 3cb84d084efa5095089de71899fe7b304f8f8993 && git merge --no-ff ae8e3e83f734dc33a4937af5221c1a41b7dce6f2

node scripts/docs-audit/affected-docs.mjs --json 3cb84d084efa5095089de71899fe7b304f8f8993

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: a89a8e5288aac60c260da2fdf88cdb9097a73961

① Derived judgments

  • Four readers refuse per ruling A (present non-array → INVALID_ARTIFACT_PACKAGES, absent → no packages): correct. packages/cli/src/utils/stack-collections.ts:122-135 declaredPackageEntries: undefined/null → []; array → same reference; else resolveArtifactPackageOrder({ packages }), whose first check (packages/core/src/artifact-packages.ts, !Array.isArray(declared)) throws refuse('INVALID_ARTIFACT_PACKAGES', …) with status: 422. packageBodies (stack-collections.ts:149-150), docsPackageRefs (collect-docs.ts:389-390), bodyDocsOf (collect-docs.ts:1171-1172) and attachPackageDocs (collect-docs.ts:1376-1378) all read through it; no Array.isArray fall-through remains in any of the four (git grep on head: the only Array.isArray(...packages left in cli src are the helper's own shape branch and lower-callables.ts:315, a transformer that passes non-arrays through to the schema parse, not a "no packages" reader). attachPackageDocs still returns the argument by identity for undefined/null/[]/no-op sets (entries.length === 0 || sets.length === 0, changed ? out : packages), as on main.
  • null: the helper treats null as absent by a PRIVATE copy of core's absent branch, and that copy pre-empts packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 — wrong. stack-collections.ts:123 if (packages === undefined || packages === null) return []; (docblock: "spelled exactly as the resolver's own absent branch"). packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 is not an open decision: ruling 5805260775 (2026-09-24, letter A) says null is malformed everywhere and resolveArtifactPackageOrder drops its declared === null branch; PR fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228 (open draft, head ec9402ad0, REWORK round 1 at 5856007483) executes it over packages/core, packages/runtime, plugin-dev, plugin-security, packages/spec, and touches no packages/cli file. Because the helper answers null itself and never hands it to the resolver, fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228 will not reach the CLI: the day it lands, core refuses packages: null while os info/os lint and the docs readers keep reading it as absent — the reader split these rulings exist to close, re-opened on this seam. The PR body ("packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 remains open … a separate decision"), the changeset ("packages: null is still read as absent; whether it should be is a separate decision") and the test header (test/non-array-packages-readers.test.ts, "that disagreement is its own decision (packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926)") all state the pre-ruling position three days after the ruling.
  • One source of the non-array rule: correct; one copy of the absent rule slipped in (above). The non-array → refusal leg is delegated, not copied. packages/cli/src/utils/artifact-packages.ts:118 artifactPackages keeps if (!Array.isArray(declared)) return [];; every caller is post-judgment: compile.ts:512, validate.ts:455 (result.data, after safeParse success at compile.ts:356-358 / validate.ts:293-295); lint.ts:546 (stack from authoringRuleUnionStack at :398); collect-docs.ts:390 (after the helper); collect-docs.ts:1070 lintDocNavTargets, called only at :1333 inside collectAndLintDocs after docsPackageRefs(stack.packages) at :1304; sdui-manifest.ts:172, evaluated after authoringRuleUnionStack(stack) at :171 in the same array literal; nav-contribution-groups.ts:198 and permission-set-name-collisions.ts:128/:177, whose callers (compile.ts:579/609, validate.ts:507/528) pass result.data; artifact-packages.ts:292 (run.parsed). No public door reaches it with a non-array.
  • No new side effects or refusals for well-formed configs: correct. Only non-arrays reach the resolver through the helper, and the resolver throws before any entry parse, id check, duplicate check or resolvePluginOrder. Arrays come back by reference; packageBodies already called resolveArtifactPackageOrder(stack) for arrays on main (stack-collections.ts:101 on main), so entry parsing on that path is unchanged, and the docs readers still do not parse entries.
  • os info / os lint user-facing refusal: correct, same path as a malformed entry. Both commands have one catch-all: info.ts:146-158 and lint.ts:1138-1160 — --json emits { error: message, ...errorCodeFields(error) } then process.exit(1); text prints printError(message) then process.exit(1). errorCodeFields (format.ts:278-287) copies code (and httpStatus, which the core envelope does not carry — it carries status, so the JSON face shows code without 422; pre-existing, identical for INVALID_ARTIFACT_PACKAGE_ENTRY). In os lint the throw originates in resolveJsxGateManifest → countJsxGatePages → jsxGateStacks → authoringRuleUnionStack (lint.ts:971, before lintConfig at :972) unless a project SDUI manifest resolves, then in lintConfig:398; neither has a catch in between (sdui-manifest.ts:266-310). Both codes are thrown by the same resolver call inside packageBodies, so they share the path. os serve: declaresObjects (stack-collections.ts:204-206) now throws from the CLI gate at serve.ts:3103 when the top level has no objects, instead of from the runtime manifest service; both land in the outer catch serve.ts:5384-5393 (printError + this.exit(1)) with core's sentence, so the measured "unchanged" holds. serve.ts:2666 collectDocsFromSrc is inside a swallowing catch (:2679); dev.ts:818-822 wraps artifactObjectNames in a try → null.
  • Flipped row in collect-docs.package-docs.test.ts: right to flip. The old row asserted docsPackageRefs({}) → [], the exact silent answer the card retires; the new row keeps the undefined control and asserts code + status on {}.

② Semver level

minor + Clause-②: no (narrowing) + BREAKING + adr-0087: not-required (no-migration-prescription) is the correct level and form. AGENTS.md:1075: "(narrowing) is BREAKING"; scripts/check-changeset-no-major.mjs header: during the launch window a breaking change ships as minor and major is refused, with the BREAKING banner and the ADR-0087 disposition as the carriers; scripts/pm/clause2-line.mjs:95 names no (narrowing) as "NOT a widening, but breaking". The form is identical to .changeset/19120-… and .changeset/19417-…. Nothing authorable is removed or renamed, so no FROM → TO migration mapping is owed (AGENTS.md rule 3 binds that to removals/renames); the marker matches the precedents; CI Check Changeset is green on the head. The BREAKING paragraph is complete for the commands that narrow: os info (exit 0 with empty package-owned collections → exit 1 INVALID_ARTIFACT_PACKAGES) and os lint (exit 0 passed: true → exit 1). The only other pre-parse doors are os serve (already exit 1 via the runtime, still exit 1) and os dev (build first, reader guarded); os validate, os build and os init schema-parse first (compile.ts:356, validate.ts:293, scaffold-validate.ts:77). The one defect is the paragraph's null sentence, which asserts an open decision that ruling 5805260775 has closed and that PR #20228 is executing (see ①).

③ Boundary flags

  • packages/lint, packages/core, packages/spec untouched: git diff --stat origin/main...head -- packages/lint packages/core packages/spec is empty. PR files (5): .changeset/19925-cli-non-array-packages-refusal.md, packages/cli/src/utils/stack-collections.ts, packages/cli/src/utils/collect-docs.ts, packages/cli/src/utils/collect-docs.package-docs.test.ts, packages/cli/test/non-array-packages-readers.test.ts. Within the claim 5855387241 surface (the colocated test flip is declared in the PR body).
  • PR body claims: the reach table and the reader table are consistent with the code read above; "16 tests" matches the file (3 spellings × 4 rows + 4 controls); the artifactPackages caller list is accurate; the os serve account is accurate. The claim that packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 "remains open … a separate decision" is stale (ruled 2026-09-24; PR fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228 in flight, surface excludes packages/cli).
  • Clause-② deviation from the claim (no → no (narrowing)) is declared in both the PR body and the changeset, in the spelling clause2-line.mjs reads.
  • Out-of-scope guard artifact-packages.ts:118 is correctly left alone and is noted in the dev report's out-of-scope findings.
  • Commits on the PR: 4 (fix 67d5b4829, changeset e7cbb6197, merge of origin/main ed72ac453, test flip a89a8e528); no rewrite evident. PR is draft, mergeable_state: blocked.
  • CI on a89a8e528 at read time (check-runs API, 31 runs): 15 success, 3 skipped, 13 in_progress, 0 failure. Green: Check Changeset, Type Check · source gates, Test Core 2/5/6, Governed Surface Queue Guard, closes/claims guards, Check PR Size, Check Documentation Links. Still running: Build Core, Test Core 1/3/4, Lint & Repo Gates, Type Check · workspace/consumer gates/debt ledger, Dogfood Verify CLI, Dogfood Regression Gate 1-3, Temporal Conformance. Not green yet; no red.

Implemented-by: claude/issue-19925-cli-non-array-packages-refusal
Reviewed-by: session_01UYBdGBzWSrAMzpW8ah3GbP

Independence: INDEPENDENT AGENT (fed the card, the rulings and the PR only; not the dispatch order or the seat's conclusions)

VERDICT: FAIL

…ng it as absent

`declaredPackageEntries` answered `null` through a private copy of the
resolver's absent branch, so the resolver's `null` refusal (ruling
5805260775 on #19926) could never reach the CLI readers. Only `undefined`
(key absent) is answered locally now. Every other non-array, `null`
included, goes to `resolveArtifactPackageOrder`, and its absent answer is
read by identity, so today's `null` answer is unchanged.

Pins: `null-packages-follows-resolver.test.ts` holds each reader to the real
resolver's verdict on `null`, and to a resolver double that refuses `null`.
The changeset and the pin-file header drop the "separate decision" wording
and cite the ruling.

Claude-Session: https://claude.ai/code/session_01UYBdGBzWSrAMzpW8ah3GbP
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l and removed size/m labels Sep 27, 2026
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: ae8e3e83f734dc33a4937af5221c1a41b7dce6f2

Delta of: 5856061282 (FAIL on a89a8e52)

① Derived judgments

  • Required change met: [] locally only for undefined; everything else, null included, goes to the resolver — correct. packages/cli/src/utils/stack-collections.ts:141-157 declaredPackageEntries: if (packages === undefined) return []; → if (Array.isArray(packages)) return packages; → const probe = { packages }; const answer = resolveArtifactPackageOrder(probe); if (answer.length === 1 && answer[0] === probe) return []; → throw new Error(...). The delta (a89a8e52..ae8e3e83, own paths) is the changeset, stack-collections.ts, docblocks only in collect-docs.ts, the header of non-array-packages-readers.test.ts, and the new pin file; collect-docs.package-docs.test.ts is untouched in the delta. Round-0 commits are retained, no rewrite (67d5b48, e7cbb61, ed72ac4, a89a8e5, 384c821, 5bac826, ae8e3e8).
  • Identity recognition is sound on main — correct. packages/core/src/artifact-packages.ts:208 (origin/main): if (declared === undefined || declared === null) return [artifact]; — the passed object by reference, and it is the only branch that returns [artifact]; every other non-array throws refuse('INVALID_ARTIFACT_PACKAGES', …) with status: 422. The probe is a fresh object per call, so answer[0] === probe can only be satisfied by that branch. Arrays and undefined return before the resolver is called, so the check cannot misfire for a well-formed array or an absent key. Today's null answers are byte-identical to main: packageBodies → [], docsPackageRefs(null) → [], attachPackageDocs(null, sets) → entries.length === 0 → returns the argument (collect-docs.ts:1381-1382).
  • With PR fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228's resolver, null refuses through all four readers with no CLI edit — correct. 0706ffef7:packages/core/src/artifact-packages.ts:216: if (declared === undefined) return [artifact]; then !Array.isArray(null) → INVALID_ARTIFACT_PACKAGES / 422 (message names null). The helper propagates the throw before the identity check. Reach: packageBodies:176 (→ resolveStackCollection, authoringRuleUnionStack:432 → collectMetadataStats format.ts:638, lintConfig lint.ts:398), docsPackageRefs:391 (→ collectDocsFromSrc, first statement), bodyDocsOf:1174, attachPackageDocs:1381. PR fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228's 12 files touch no packages/cli path.
  • Other resolver answers handled loudly — correct, with one note. A one-element answer that is not the probe, or any other length, throws a bare Error naming the type ('null' special-cased). It is loud, but it is not an ADR-0112 envelope; it is unreachable with main's and fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228's resolver, and it is the same construct round 0 accepted.
  • Leg 2's mock is file-scoped — correct. null-packages-follows-resolver.test.ts:41-44 vi.mock('@objectstack/core', importOriginal) spreads actual and wraps only resolveArtifactPackageOrder in vi.fn(actual.…). vitest is 4.1.11 (packages/cli/package.json), packages/cli/vitest.config.ts sets neither pool nor isolate, and no workflow passes --no-isolate, so the mock registry is per test file. afterEach (:127-130) resets to realResolver. The double is armed inside each leg-2 it only.
  • Leg 2 goes red on a private null branch — correct. With if (packages === undefined || packages === null) return []; restored, the helper never calls the double for null, so all seven reader rows answer instead of refusing; every row reaches the helper (paths above). The PR body's 7 failed / 25 passed (7 leg-2 rows red, leg-2 control and 8 leg-1 rows green) is consistent. Blind spot, noted only: a private null REFUSAL copy with the same envelope would pass leg 2 and, once fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228 lands, leg 1 too; today leg 1 catches it.
  • Leg 1 is non-vacuous — correct. :135-136 takes the live verdict from realResolver({ packages: null }); :138-146 pins that verdict to one of exactly two shapes ([probe] by identity, or INVALID_ARTIFACT_PACKAGES/422); :148-155 compares each reader's null outcome ({kind, value} / {kind, code, status}) to its absent outcome. A reader refusing null today, or answering it differently from absent (for example attachPackageDocs returning [] instead of the argument), goes red. Count: 1 + 7 + 7 + 1 = 16 tests, matching the PR body; 229 files / 3250 tests = round 0 + 1 file / + 16.
  • Wording — correct. grep on head over the six PR files: no "separate decision" / "its own decision" remains; ruling 5805260775 is cited in the changeset (:35), stack-collections.ts:117, both test headers (non-array…:37, null-…:6) and the PR body. packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 is open (pm:dispatched, domain:spec), so "packages: null on a release artifact: the schema and composeStacks refuse it, while every reader reads it as absent #19926 remains open, but it is not a separate decision" is true. Two nits, not required: non-array-packages-readers.test.ts:38 "it is refused by core's resolver" is present tense while main's resolver still answers null as absent (reads as the ruling's allocation of the refusal); changeset :39 "Today that is its absent answer" is time-relative and will read stale in a release that also carries .changeset/19926-packages-null-refused.md, though the mechanism sentence ("once the resolver refuses null, these commands refuse it too, with no change to the CLI") stays true.
  • PR body measurements — consistent. Blob c8c830d2e25b = git rev-parse ae8e3e83:packages/cli/src/utils/stack-collections.ts; round-1 fix 5bac82635 precedes the merge ae8e3e83f (merge-base 3cb84d084); the forward check on fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228's resolver was not re-run here but follows from reading both resolvers (above).

② Semver level

Unchanged by the delta: '@objectstack/cli': minor + Clause-②: no (narrowing) + the BREAKING paragraph + <!-- adr-0087: not-required (no-migration-prescription) … --> (changeset :1-3, :8, :10, :46). The delta narrows nothing today (null answers are byte-identical at all four readers), and the forward null narrowing is carried by PR #20228's own changeset for core/runtime/plugins; the 19925 changeset's new paragraph discloses the CLI consequence. AGENTS.md:1075 makes (narrowing) BREAKING; the form matches the .changeset/19120-… / 19417-… precedents; Check Changeset is green on both runs for this head.

③ Boundary flags

  • File surface vs merge-base 3cb84d084: 6 files, all in the claim (5855387241); git diff --stat … -- packages/core packages/spec packages/lint is empty. The origin/main..head stat shows other packages only because main has moved to ab820016; judged on the PR's own files.
  • The new pin file has no integration signals (vitest-tiers.ts tierSignals) and is not a nightly-tier file, so it runs in the unit project; tsconfig.test.json include: ["test/**/*"] compiles it (the dev's "2 hits" is loose phrasing; it is a glob).
  • The fallthrough throw new Error in the helper is not an ADR-0112 envelope; unreachable today, same as round 0.
  • Leg-2 cannot see a private null refusal copy after fix(core,runtime,plugin-dev,plugin-security): refuse packages: null as malformed, never absent #20228 (noted above); not required.
  • No worktree was created; no GitHub write; reads were unauthenticated GETs and git on origin refs.
  • CI on ae8e3e83 (check-runs API, 41 runs): 36 success, 5 skipped (Auto Label and Check PR Size in the 14:06 re-run batch, Build Docs, Console Pin Gate, Packed-tarball smoke opt-in), 0 failure, 0 in progress. Green: Build Core, Test Core 1-6, Lint & Repo Gates, Type Check (source, workspace, consumer gates, debt ledger), Check Changeset (13:23 and 14:06), Dogfood Verify CLI, Dogfood Regression Gate 1-3, Temporal Conformance, Governed Surface Queue Guard, claims/closes guards, Check Documentation Links. mergeable_state: clean; the PR is still a draft.

Implemented-by: claude/issue-19925-cli-non-array-packages-refusal
Reviewed-by: session_01UYBdGBzWSrAMzpW8ah3GbP

Independence: INDEPENDENT AGENT (fed the card, the prior review, and the PR only; not the dispatch order or the seat's conclusions)

VERDICT: PASS

veigajoao pushed a commit to veigajoao/objectstack that referenced this pull request Sep 29, 2026
…s group, refusing the rest (objectstack-ai#20497) (objectstack-ai#20517)

Fixes objectstack-ai#20497
Clause-②: no

## What changes

`parseNumberCell` in `packages/rest/src/import-coerce.ts` removed every
comma before parsing, as if every comma grouped thousands. So `POST
/api/v1/data/:object/import` stored a decimal-comma cell as a different
number and reported success.

A comma is now accepted only in a well-formed thousands group: 1 to 3
leading digits, then groups of exactly three, and only before any `.`
(`1,000`, `12,345.67`). One anchored pattern,
`THOUSANDS_GROUPED_INTEGER`, is tested before the strip, and the strip
now runs only for that form. Every other comma makes the cell
unparseable, so the row gets the importer's existing `invalid_number`
error. That is the same code the plain write doors already answer for
these cells. No locale is guessed, and there is no new error code and no
decimal-separator option, as triage directed (`5877481993`). The rule is
stated in the `parseNumberCell` docblock, where the reader's tolerances
are listed.

Measured through the real route (JSON rows, `writeMode: 'insert'`) on
`InMemoryDriver` and on `SqlDriver` (better-sqlite3). Both drivers
answered the same on every cell, at base `9449512a31` and at head
`c76a3c95f2`:

| cell | base: stored · import answer | head | plain `POST` (engine
insert), both trees |
|:--|:--|:--|:--|
| `'3,14'` | `314` · ok 1, errors 0 | row refused, `invalid_number`,
nothing stored | `VALIDATION_FAILED` / `invalid_number` |
| `'1,5'` | `15` · ok 1, errors 0 | row refused, `invalid_number`,
nothing stored | same |
| `'1.000,5'` | `1.0005` · ok 1, errors 0 | row refused,
`invalid_number`, nothing stored | same |
| `'1,2,3'` | `123` · ok 1, errors 0 | row refused, `invalid_number`,
nothing stored | same |
| `'1,000'` | `1000` | `1000` (unchanged) | refused (the import-only
tolerance stays) |
| `'12,345.67'` | `12345.67` | `12345.67` (unchanged) | refused |
| `'(1,234)'` | `-1234` | `-1234` (unchanged) | refused |

## PM hypotheses, measured

- **H0 holds.** On current `main` (`9449512a31`), the four cells
imported as `314`, `15`, `1.0005` and `123` with `ok 1, errors 0`, on
`InMemoryDriver` and on SQLite. The table above has the readings.
- **H1 holds.** The one line `s.replace(/,/g, '')` is the whole cause.
Before/after census through `coerceRow`: before is rest's built `dist`
at the base, after is the head's `src`. It covers 79 rows: the spec
grammar's 41 `NUMERIC_STRING_GRAMMAR_CASES` rows, 18 documented or
control forms, and 20 comma probes.
- **Grammar rows.** 1 of 41 changed: `'1.000,5'` went from `1.0005` to
refused. The other 7 grammar-refused rows the reader admits keep their
reading: `' 12 '`, `'12\n'`, `'\t-3'`, `'1,000'`, `'+5'`, `'.5'` and
`'007'`.
- **Documented and control forms.** 0 of 18 changed: `1,234`, `$1,000`,
`¥2,500.75`, `€1,000`, `£1,000`, `¥1,000`, `25%`, `(1,234)`, `(100)`,
`1,234.5`, `12,345.67`, `1,000`, `-1,234`, `+1,234`, `1,234,567.89`, `$
1,000`, `1,234%` and `1,000e3`.
- **Comma probes.** 18 of 20 changed, each from a stripped number to a
refusal: `3,14`, `1,5`, `1.000,5`, `1,2,3`, `0,5`, `1,23`, `1234,567`,
`1,0000`, `,123`, `-,123`, `.5,000`, `1,000,`, `12,345.6,7`,
`12,34,567`, `1,00,000`, `(3,14)`, `$1,5` and `1,5%`. The other 2
(`1,000.` and `1 ,000`) were already refused.
- **Overall.** 19 rows changed, every one from a stored number to a
refusal. No row moved the other way, and no admitted value changed.
- **H2 holds.** A refused cell's row carries `code: 'invalid_number'`,
the code the importer already uses for `abc`, with the importer's
existing sentence (`Amount: "3,14" is not a number`, from the catalog's
`import_invalid_number` key). The plain create door answers `400
VALIDATION_FAILED` with field code `invalid_number` for the same cells,
and that is pinned beside the import pins. No new code.
- **H3 holds, in the direction expected.** The ablation removed the
grouping guard, which restores the unconditional strip. It ran via
`scripts/ablation-replace.mjs` in wrap mode, at head `c76a3c95f2`. The
anchor went 1 to 0 and the blob went `afaf602da192` to `a06963b8c44c`,
so the mutation landed. Result: `Tests 24 failed | 46 passed (70)`.
- **Red: every refused-cell assertion and nothing else.** That is 18
`parseNumberCell` refusal cases, the 4 per-cell import pins, the CSV leg
and the dry-run leg.
- **Green: every admitted control.** That is 3 import pins, 8
`parseNumberCell` admitted cases, and the plain-door parity pin.
- **Restore proven.** Blob after restore equals HEAD (`afaf602da192`),
`git diff HEAD` is empty, and the marker count is 0. No build was
needed: the pins reach `import-coerce.ts` through relative imports
(`./rest-server`, `./import-coerce`), never through a `dist/`.

## Tests

- `packages/rest/src/import-number-thousands-group.test.ts` (new) goes
through the real `/import` route over `SqlDriver` (better-sqlite3
`:memory:`). It has 10 cases:
- each of the four cells is refused as its own row's `invalid_number`,
with a sibling row still written;
- each admitted control (`1,000`, `12,345.67`, `(1,234)`) is stored as
its number;
  - the same verdicts hold for quoted CSV cells;
  - the dry run predicts the refusals and persists nothing;
  - the plain create door answers the same code.
- `packages/rest/src/import-coerce.test.ts` gets the `parseNumberCell`
case table: 8 admitted groupings and 18 refused comma forms.
- At `c76a3c95f2`, run as `pnpm --filter @objectstack/rest test
--maxWorkers=2`: `Test Files 220 passed (220)` and `Tests 4211 passed |
40 skipped (4251)`. `test:repo` passed 1 file with 8 tests.
- `pnpm --filter @objectstack/rest typecheck` exits 0: `tsc --noEmit`,
then `check:test-typecheck: OK`, with 0 errors. Both test files are in
the `tsconfig.test.json` program (`--listFilesOnly`).

## Gates

- **Build.** `turbo run build` ran first for `@objectstack/rest...`,
then for all of `./packages/*` and `./packages/*/*`, because two gates
read the whole built tree. Result: 71/71 tasks.
- **Derived gates.** `node scripts/pm/dispatch-gates.mjs --repo
objectstack-ai/objectstack --commands` derived 61 commands at
`c76a3c95f2`. All 61 ran with exit 0. Reconciled with `--ran`: `61
derived, 61 run, 0 NOT-MEASURED, 0 UNRUN`.
- **Other gates, all exit 0.** `pnpm lint` (the whole repository, 29 s,
no narrowing) and `node scripts/check-issue-citations.mjs --base
origin/main` (`origin/main` is still `9449512a31`, the branch point, so
there was nothing to merge).
- **Changeset gates.** `check-adr-0087-registration` names this PR's
changeset as `[BREAKING+clause-②-narrowing] not-required
(no-migration-prescription)`. `check-changeset-no-major` reports no
`major`.

**Declared narrowing — verification ran UNLOCKED.**
`scripts/pm/os-verify-lock.sh`
could not take the shared verify lock on this host: no usable `flock`.
The shared
verify lock is declared Linux-only (`flock` is util-linux, and a stock
macOS does
not ship it), so the command below was run directly, without the lock —
a declared narrowing, not a silent one. No serialization guarantee held
for this
run, nor for any sibling agent in this container while it ran.

pnpm turbo run build --filter='@objectstack/rest...' --concurrency=2
--output-logs=errors-only
pnpm exec turbo run build --filter='./packages/*'
--filter='./packages/*/*' --concurrency=2 --output-logs=errors-only
    pnpm --filter @objectstack/rest test --maxWorkers=2
    pnpm --filter @objectstack/rest test:repo --maxWorkers=2
    pnpm --filter @objectstack/rest typecheck
pnpm --filter @objectstack/rest exec vitest run --project local
--maxWorkers=2 (the targeted pin files, and the ablation's wrapped run)

(The entry point printed this wording once for each command above. It is
pasted once here, with every command it covered.)

## Changeset

`.changeset/20497-import-number-thousands-group.md`: `@objectstack/rest`
`minor`, with a line-initial `Clause-②: no (narrowing)`, a BREAKING
paragraph giving each refused cell shape FROM → TO, and the ADR-0087
disposition `not-required (no-migration-prescription)`. The shape
follows PR objectstack-ai#20218 and PR objectstack-ai#20231.

## Acceptance notes

- **The `InMemoryDriver` leg is measured, not pinned. This departs from
triage's pin list.** Triage asked for `/import` pins on memory and
SQLite. `@objectstack/driver-memory`'s test consumers are a ruled,
ledgered set (`scripts/driver-memory-census.ledger.json`, gated by `pnpm
check:driver-memory-census`), and the gate says a new consumer is a
maintainer ruling. A first cut added the driver as a `packages/rest`
devDependency with a source alias. The census gate refused it by name:
`x LEDGERED: packages/rest/src/import-number-thousands-group.test.ts:31
binds @objectstack/driver-memory (import) and the ledger does not cover
it`. That cut was withdrawn, so the diff is back to the claim's file
surface. The cell is judged by the importer's reader before any driver
is reached, so one verdict holds on every driver. The memory readings at
base and head are recorded in the table above and in the pin's header.
If a permanent memory arm is wanted, it goes through the census ruling
first.
- **Grouping by twos is now refused (`12,34,567`, `1,00,000`).** These
used to import as the number they denote. The changeset names this as
the one case where the narrowing refuses a cell that was read correctly
before, because a two-digit group cannot be told apart from a decimal
comma.
- **The import template card objectstack-ai#18386 should follow this wording.** Its
value-domain row for `number` lists `1,234` among the tolerated forms.
It should say that a comma is read only as a thousands group (1 to 3
leading digits, then groups of exactly three, only before any `.`), and
that a decimal comma such as `3,14` is refused, never guessed. That card
is assigned elsewhere and is not edited here.
- **A spec comment now overstates the reader.** The module header of
`packages/spec/src/data/filter-number-comparand-declared-type.ts` says,
in the parenthetical under its refused digit-separator forms, that the
CSV import route's cell reader strips such punctuation before it parses.
For `1.000,5` that is no longer true. It was already untrue for `1_000`
and `1 000`, which the reader refused before this change too. This is a
comment in the spec seat's surface. There is no carrier, so it is noted
here only.
- **objectui's Import Wizard preview disagrees with the server on
grouped numbers.** The preview judges numeric cells with a bare
`Number()` (`packages/plugin-grid/src/ImportWizard.tsx` in objectui, the
number/currency/percent case). So it flags `1,000` as invalid while the
server admits it. That disagreement predates this PR and is unchanged by
it. For the four cells here, preview and server now agree: both refuse.
I read this in objectui's source and did not measure it through the UI.
There is no carrier, so it is noted here only.

---
_Generated by [Claude
Code](https://claude.ai/code/session_local_1d2a197c-c20e-4e90-9be8-413d4d432289)_

---------

Co-authored-by: Jack Zhuang <50353452+hotlong@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

2 participants