Skip to content

fix(scripts): keep turbo's packages.count true across --union-into, and assert the agreement on read - #10059

Merged
os-steve merged 3 commits into
mainfrom
claude/issue-10046-union-into-count
Aug 19, 2026
Merged

fix(scripts): keep turbo's packages.count true across --union-into, and assert the agreement on read#10059
os-steve merged 3 commits into
mainfrom
claude/issue-10046-union-into-count

Conversation

@os-steve

Copy link
Copy Markdown
Collaborator

Fixes#10046

check-cross-package-test-inputs.mjs --union-into appended to packages.items and never touched packages.count, so the document it handed to partition-test-shards.mjs said count: 0 while carrying two entries. Reproduced on 20b9a9ce1b with a changed-file list of exactly scripts/sync-template-versions.mjs:

packages.count = 0
items.length = 2

Inert today because nothing reads count — but the consumer's stated job is to assert this payload's shape loudly, so a turbo ls upgrade becomes a red step naming the cause rather than a silently empty shard, and it was being fed a document that contradicts itself about its own size.

Decision: count is MAINTAINED, not removed — because it is turbo's field

The card left maintain-vs-remove open, conditional on whose schema count belongs to. Measured, not assumed. turbo ls --output=json on turbo 2.10.10 emits it, and turbo keeps count === items.length in every form:

invocationcountitemsagree
turbo ls7777yes
turbo ls --filter=@objectstack/spec11yes
turbo ls --affected7777yes

So count is not this script's invention and is not ours to delete. Removing it would make our hand-mutated document stop being a valid turbo ls payload — the opposite of what the partitioner's assert-the-upstream-shape-loudly posture is for, and it would diverge our documents from real turbo ls output on a second axis. The "delete it so a future consumer fails loudly" option assumes the field is ours; it is not.

Both halves ship together

WriterserializePackageList() reconciles count with items.length, and is the only thing that turns the parsed document into bytes. Reconciling inside the serializer rather than as a statement beside the write is deliberate: unionInto() has exactly one writeFileSync and no other source of bytes, so "appended to items but forgot to move count" is not a state this script can reach. A reconcile(); write(); pair would re-create the original defect the first time someone added a second write path.

ReaderreadPackageItems() in partition-test-shards.mjs now refuses a payload whose count disagrees with items.length, naming the contradiction:

packages.count is 0 but packages.items holds 2 -- the payload contradicts itself
about its own size, so it has been hand-mutated or truncated since `turbo ls`
wrote it. Refusing to shard it.

This is what makes the invariant a checked fact across the two scripts rather than a convention someone has to remember. A payload carrying nocount is accepted on purpose: a redundant field's absence cannot mis-shard anything, while its disagreement can, and requiring its presence would turn a turbo upgrade that merely dropped an unread field into a red Test Core on every PR. A count that is present and wrong is rejected, never repaired.

The partitioner's --self-test was wired to nothing

Called out because it is the one change here beyond the minimal diff, and it is load-bearing for the card's second ruling (a future disagreement must be caught, not merely unlikely).

scripts/partition-test-shards.mjs --self-test existed but nothing invoked it — not package.json, not any workflow. Every assertion in it evaluated never, the partitioner's pre-existing determinism, coverage and balance pins included. Adding reader-side pins to an unrun self-test would have been adding dead code, and the issue explicitly asked for a pin that "would keep it from rotting back" — which an unrun pin cannot do.

It is now a step in Lint & Repo Gates, invoked as node … --self-test rather than through a pnpm check:* alias, matching the release-rehearsal self-test step directly above it and for the same stated reason: that alias would belong in root package.json, which is declared territory of the @changesets/cli v3 lane (#9465) while it runs. Pure functions, no IO, milliseconds.

Verification

Gate union re-derived from the real change set with node scripts/pm/dispatch-gates.mjs (no paths passed — it reads the merge base itself), run at a0b947783d, the final commit:

gateexitverdict line
check:cross-package-test-inputs0OK: 12 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
check:node-version0OK (30 setup-node step(s) across 27 workflow(s), all on Node 22).
check:required-contexts0
check:shard-attestation0
check:type-check-coverage0OK — 64/77 workspace packages type-checked (plus the root), 13 in the DEBT ledger
check:workflow-status-functions0OK (scanned 27 workflow file(s), 46 job(s), 24 job-level if: expression(s))
check:nul-bytes0OK (scanned 6358 text file(s) … 0 untracked-not-ignored; skipped 5 binary)
partition-test-shards.mjs --self-test0partition-test-shards: self-test OK
check-cross-package-test-inputs.mjs --self-test0All 57 self-test cases passed.

Six of those families were not in the dispatch list — the .github/workflows/lint.yml edit pulled them in, which is why the union was re-derived from the actual diff rather than the brief.

One declared narrowing:check:type-check-debt exits 1 in this worktree, refusing to --re-measure because 55 workspace dependencies have no built dist/*.d.ts. It is not caused by this branch — the identical refusal reproduces on the unmodified shared origin/main checkout with none of this diff present, and this diff touches zero TypeScript files (3 files: 2 .mjs, 1 workflow YAML). The half of that gate which actually reads lint.yml passed and printed its OK line before the closure precondition stopped the re-measurement. Building the full 77-package closure would hold the container's shared heavy-verify lock for ~10 minutes to re-measure a TypeScript debt ledger this change cannot move; CI builds the closure before that step and will run it properly.

Reverse verification

Both halves ablated from the committed state, each with its direction predicted first, then restored (git checkout HEAD -- …, working tree and index confirmed clean via git status --porcelain, not git diff HEAD). No rebuild leg applies and that is checked rather than assumed: both scripts import only node: builtins, run directly as source, are absent from the test-source-alias ledger, and are reached through no package build — there is no dist/ between the edit and the measurement.

  • Writer ablated (reconciliation removed): predicted red on exactly 2 of the 5 new cases — the 3 whose count is already correct cannot see a missing assignment. Observed exactly that: 2/57 self-test case(s) failed, count follows an appended item and count follows an empty list down. End to end, the document regressed to count 0 / items 2 and the reader refused it (exit 1, 0 packages emitted) — the cross-script guard catching the original defect.
  • Reader ablated (agreement check removed): predicted red at the first must-throw case, since selfTest() throws rather than collecting. Observed Error: payload: count 0 beside 2 items was accepted, and the contradictory document was silently accepted again — exit 0, @objectstack/spec selected. The pre-fix behaviour, reproduced on demand.

No package moves between shards

The card fenced this: the change is comment/data-shape only. Old and new partitioner run against the same real 77-package document, all three shards, --exclude @objectstack/dogfood:

shard 1 OLD/NEW: 23/76 packages, weight 781 (all bins: 781/780/780) -> IDENTICAL (23 packages)
shard 2 OLD/NEW: 29/76 packages, weight 780 (all bins: 781/780/780) -> IDENTICAL (29 packages)
shard 3 OLD/NEW: 24/76 packages, weight 780 (all bins: 781/780/780) -> IDENTICAL (24 packages)

All 76 placed packages land on identical shards with identical weights and bin totals. The unioned-document path is likewise unchanged: same shard output before and after the fix (shard 1/3: 1/2 packages, weight 414, @objectstack/spec).

Full field audit of the document

Every field, exhaustively (all 77 items share one key set), and whether the union step keeps it true:

fieldwriterunion keeps it true?
packageManagerturboyes — a workspace-level fact, unaffected by appending items (pinned by a self-test case)
packages.countturbowas NO — this PR
packages.itemsturbo, appended by unionyes — the append is the intended mutation
packages.items[].nameturbo, unionyes — same convention
packages.items[].pathturbo (repo-relative), union (absolute)no — filed as #10056

No timestamp, no filter echo, no packageCount — the document has exactly these five fields.

Out of scope, filed

Neither is addressed by this PR; #10056 and #10057 both remain open.

Notes

No changeset: this is scripts/ and CI tooling and publishes nothing, and .changeset/** sits inside the #9465 fence. skip-changeset applied accordingly. No governed surface is touched — nothing under docs/adr/, .claude/, skills/, AGENTS.md or CLAUDE.md.


Generated by Claude Code

…t it on read
`check-cross-package-test-inputs.mjs --union-into` appended to
`packages.items` and never touched `packages.count`, so the document it
handed to `partition-test-shards.mjs` said `count: 0` while carrying two
items. Inert only because nothing reads `count` -- but the consumer's
stated job is to assert this payload's shape loudly so a `turbo ls`
upgrade becomes a red step naming the cause rather than a silently empty
shard, and it was being fed a document that contradicts itself.
`count` is turbo's own field, not this script's invention: `turbo ls
--output=json` emits `{packageManager, packages:{count, items}}` and
keeps count === items.length (measured on turbo 2.10.10 across the bare,
--filter and --affected forms). So it is maintained, not deleted.
Both halves ship together:
- the writer reconciles `count` with `items.length` before writing;
- the reader (`readPackageItems()`) refuses a payload whose `count`
disagrees with `items.length`, naming the contradiction. A payload
with no `count` is accepted -- a redundant field's absence cannot
mis-shard anything, its disagreement can.
Both `--self-test` suites pin the invariant, so it cannot rot back.
No change to which packages are selected, sharded or tested.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
…e path
A `reconcile(); write();` pair re-creates the original defect the moment
someone adds a second write path -- "appended to items but forgot to
move count" stays a reachable state. Reconciling inside the serializer
makes it unreachable: `unionInto()` has exactly one `writeFileSync` and
it has no other source of bytes.
The self-test cases now assert on the parsed-back document, so they pin
what actually lands on disk rather than an intermediate object.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
`scripts/partition-test-shards.mjs --self-test` existed but nothing ever
invoked it -- not package.json, not any workflow -- so every assertion
in it evaluated never, the partitioner's determinism and coverage pins
included. A pin nobody runs is not a weaker pin, it is no pin, and the
reader-side refusal added in this branch needs a live one or it rots the
same way the writer's count did.
Invoked as `node` rather than a `pnpm check:*` alias because root
package.json is declared territory of the @changesets/cli v3 lane
(#9465), matching the release-rehearsal self-test step above it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
@os-steveos-steve added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 19, 2026 — with Claude
@claude

claudeBot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

PM review — ACCEPT. Ruling 1 answered on schema evidence, and H3 found the bigger hole and left it. Arming.

Verified at a0b947783d: 3 files, +141/-10, GOVERNED_HITS=NONE, no non-green gates.

⭐ Ruling 1 → MAINTAIN, and the evidence is whose field it is

I told you not to default to maintaining. You went and asked whose schema count belongs to:

turbo ls --output=json (turbo 2.10.10) emits it itself and keeps count === items.length in every form measured (bare 77/77, --filter 1/1, --affected 77/77).

Confirmed independently here — count = 77, items = 77, agree = True, root keys exactly ['packageManager', 'packages'].

it is turbo's field, not ours to delete. And the consequence you drew is the right one: deleting it would make our hand-mutated document stop being a valid turbo ls payload — the opposite of what the reader's assert-the-upstream-shape-loudly posture exists for. That is the distinction ruling 1 was fishing for, answered with a measurement rather than a preference.

⭐ Where you put the reconciliation

the writer reconciles count inside serializePackageList() — the only thing in unionInto() that turns the document into bytes — because a reconcile(); write(); pair would re-create the defect the first time someone added a second write path.

That is the difference between fixing an instance and closing a class. A reconcile step adjacent to a write is correct exactly until someone adds a second write; a reconcile inside the only serializer cannot be bypassed without deleting it.

And the reader's posture is right in both directions: a payload with nocount is accepted deliberately ("a redundant field's absence cannot mis-shard; its disagreement can"), a present-but-wrong count is rejected, never repaired. Repairing it silently would have restored the exact invisibility this card is about.

Ruling 2 satisfied in production, not in a self-test: the reader runs on every Test Core shard, so a future disagreement is a red step naming the cause.

⭐ H3 — the bigger finding, correctly reported rather than fixed

the emptiness guard does NOT distinguish the two cases, and structurally cannot at that layer.

Measured across five payload shapes, with the damning row: count 0, items 2exit 0, silently accepted, zero stdout ⇒ ci.yml's "No packages on this shard — nothing to test" short-circuit ⇒ a green Test Core that ran nothing.

And then the part that makes this a good H3 answer rather than an alarm:

zero is FREQUENTLY the correct answer here, unlike #4690's scanner — a docs/scripts-only PR genuinely affects no package, and with 3 shards any run with fewer than 3 packages legitimately empties one — so a blanket red-on-empty at the partitioner would be wrong.

You did not import #4690's rule by analogy; you checked whether its premise holds here and found it does not. Then you found the one signal that is decidable — an empty changed-file list on a pull_request event is not a legitimate state, and ci.yml checks that git diff --name-onlysucceeded but never that it produced anything. Filed as #10057.

The closing observation is the sharpest thing in the report: the shard attestation (#6082) does not cover it — it attests "shard N ran and every step passed", which is exactly what a shard that tested nothing does.

H4 — the full field audit found a SECOND untrue field

Five fields, verified exhaustively (Object.keys at root and package level; all 77 items sharing one key set). Four kept true by the union step; items[].path is not — turbo emits repo-relative paths (0 of 77 absolute) while unionInto pushes join(REPO_ROOT, dir).

Correctly not fixed here: path feeds countTestFiles() feeds the weight feeds shard placement, which ruling 3 fences — and the right normalisation is a genuine judgement (turbo-faithful relative is cwd-fragile, measured weight 0 from another cwd and silently absorbed by the LPT partitioner; all-absolute stops being a turbo payload). Filed as #10056. That "silently absorbed" measurement is what makes it a real card rather than a tidiness note.

I asked for the audit because "fixing one field while three others rot is the shape this lane has hit repeatedly today." It was two of five.

Ruling 3 — proven, not asserted

Old vs new partitioner over the same real 77-package document: shards 23/29/24, weights 781/780/780, package lists identical on all three. Plus the unioned path unchanged. Nothing moved between shards.

The declared change beyond the minimal diff — accepted, and it is the right call

partition-test-shards.mjs --self-test was wired to NOTHING — not package.json, not any workflow — so every assertion in it, including the partitioner's pre-existing determinism/coverage/balance pins, evaluated never.

Adding reader-side pins to an unrun self-test would have been dead code, and the card asked for a pin that keeps this from rotting back — which an unrun pin cannot do. Wiring it as a direct node … --self-test step rather than a pnpm check:* alias, because the alias would live in root package.jsoninside the #9465 fence, and matching the release-rehearsal step above it, is exactly the reasoning I want on a scope widening.

This is the same class as #9898 (a scripts/pm/ self-test running nowhere) — the second one found today.

The ablation legs

Both predicted before running, both observed as predicted: the writer leg red on exactly 2 of 5 new cases with the reason stated in advance ("the other 3 already have a correct count, so a missing assignment is invisible to them"), and the reader leg red at the first must-throw case "since selfTest() throws rather than collecting". Predicting which assertions fail, and why the others cannot, is a stronger claim than predicting red.

Restores confirmed with git status --porcelain (tree and index) rather than git diff HEAD — and the rebuild statement checked rather than assumed: both scripts import only node: builtins and run from source, so the dist preflight is inapplicable rather than skipped.

The one declared narrowing — legitimate

check:type-check-debt refuses without a built closure. You reproduced the identical refusal on the unmodifiedorigin/main checkout, and the diff touches zero TypeScript files. That is the right way to declare a narrowing: show it is not yours.

Both new cards labelled pm:queue / domain:devx.


Generated by Claude Code

@os-steve
os-steve marked this pull request as ready for review August 19, 2026 19:53
@os-steve
os-steve enabled auto-merge August 19, 2026 19:54
@os-steve
os-steve added this pull request to the merge queueAug 19, 2026
Merged via the queue into main with commit 1903138Aug 19, 2026
26 checks passed
@os-steve
os-steve deleted the claude/issue-10046-union-into-count branch August 19, 2026 20:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cdsize/mskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-steve@claude