Skip to content

test(ci): take the live registry reading out of a REQUIRED context (objectui#9562) - #9690

Merged
os-try-charles merged 1 commit into
mainfrom
claude/issue-9562-lockfile-dedupe-determinism
Sep 17, 2026
Merged

os-try-charles merged 1 commit into
mainfrom
claude/issue-9562-lockfile-dedupe-determinism

Conversation

@os-try-charles

Copy link
Copy Markdown
Collaborator

Part of #9562

The card reports that scripts/check-lockfile-dedupe.mjs returns different verdicts on a byte-identical pnpm-lock.yaml while sitting in the blocking Test (shard 1/4). This lands the half that is dev work: the required lane stops carrying a reading that is not a function of the repository's bytes. It does not close the card — the residual is named at the bottom and is a maintainer decision.

Clause-②: no

What was actually wrong

scripts/__tests__/check-lockfile-dedupe.test.ts ran node scripts/check-lockfile-dedupe.mjs twice — a pnpm dedupe --check that resolves against registry.npmjs.org. That file is in the unit vitest project, which vitest.config.mts includes as scripts/**/*.test.ts and ci.yml shards four ways as Test (shard N/4). All four shards are in REQUIRED_CONTEXTS in scripts/dependabot-merge-gate.mjs.

So a live network resolution decided a required verdict on every pull request — including pull requests that touch nothing the reading is about.

That is exactly what lockfile-dedupe.yml was built to prevent. That workflow is path-filtered to pnpm-lock.yaml plus the gate's runtime closure, and its own header says the filter is what keeps the live reading out of the required set under #3523's rule. The unit test walked around the filter.

The measurement

Lockfile blob held fixed as the control — sha256 identical before and after every leg, checked each time. Only terms outside the repository were varied. pnpm keeps abbreviated and full packuments in two separate caches, so "partial cache" is a real state and both halves were tested.

leg what varied exit verdict
A warm shared metadata cache 0 VERDICT deduped
B cold private cache (both halves empty) 0 VERDICT deduped
C abbreviated warm, full-metadata evicted 0 VERDICT deduped
D full-metadata warm, abbreviated evicted 0 VERDICT deduped
E same bytes, registry unreachable 2 VERDICT could not take a reading

Exit 2 is what the checker documents as "never a pass", and the old assertion was a bare expect(status).toBe(0) — so leg E reds a required context, and the failure the reader sees points at pnpm dedupe and tells them to commit the lockfile. That advice is wrong there, and the lockfile is shared by every open pull request.

Honest limits on this. Legs A–D did not reproduce the card's red, and I did not identify the card's mechanism — the card's red printed VERDICT not deduped, mine prints VERDICT could not take a reading. What is reproduced is the class: byte-identical repository input, two different verdicts, decided by a term the repository does not contain. Also note the current lockfile blob is b57d9644 and the card's was 4e954308, so this is the same property measured on a later tree, not a replay of the card's run.

The change

One file. The live reading stays where the repository already put it — the path-filtered Lockfile Dedupe Check context, still classified BLOCKING in OPTIONAL_CONTEXTS, untouched. scripts/dependabot-merge-gate.mjs, lockfile-dedupe.yml and the checker itself are all unmodified.

The test file now drives the shipped script through a stubbed pnpm on PATH, following the convention check-doc-snippet-types.test.ts already uses. The stub records its argv, and every run asserts that recording exists — so a spawn that reached a live pnpm fails on the control instead of quietly going to the network.

Coverage goes up, not down. The two live legs only ever exercised the GREEN path through main(). Replacing them covers:

  • the finding path — exit 1, package names, the ::error title= annotation, and that a finding never reaches stdout;
  • the could-not-run path from a real captured registry failure — exit 2, and explicitly not 1, so a crash can never be reported as a lockfile finding;
  • a non-zero exit with no output at all;
  • the dedupe --check argv. This replaces the old does not rewrite the lockfile it judges leg with its cause: the old leg could only catch a dropped --check if the run happened to rewrite something, while this catches it always.

Ablation — both legs mutated on disk, both restored and proven

Run from the committed state; each mutation proved on disk by blob-hash inequality against its HEAD blob before the suite ran, each restore proved by blob-hash equality plus an empty git diff HEAD.

  • Remove the stub from PATH so the real pnpm serves the run: 6 failed | 8 passed, control message the stub did not serve this run fired. Every hermetic test reds on the control rather than silently going live.
  • Collapse classify() so a crash reads as a finding (return 'findings';): 3 failed | 11 passed — the checker's own --self-test, the registry-unreachable leg and the signal-killed leg. The new assertions discriminate the exact defect class.

A first attempt at the mutation was a perl no-op; the on-disk hash check caught it and refused to run the suite, which is why it is there.

Verification

  • vitest run --project unit scripts/__tests__/check-lockfile-dedupe.test.ts14 passed, 1.03s. The two live legs alone cost 21–31s each warm here, and 71s with pnpm's retry backoff when the registry was unreachable.
  • vitest run --project unit scripts/__tests__/170 passed | 2 skipped (172) files, 4891 passed | 2 skipped tests.
  • tsc -p tsconfig.scripts.json --noEmit — exit 0.
  • Gates, each exit 0: check:control-bytes, check:new-line-citations, check:test-path-roots, check:shell-escape-residue, check:entry-guard, check:pre-install-import-graph, check:comment-mask-corpus.
  • check-governed-queue-guard.mjs --test on the changed path: NOT GOVERNED.
  • ESLint, narrowed and declared: eslint --no-inline-config --format json on the one changed file, 0 errors / 0 warnings, run at 94d803cb8. Population read from eslint.config.js itself, which configures no project / projectService — type-aware linting is not enabled, so a rule's verdict on a file is a function of that file alone and this one-file diff cannot move the verdict on any untouched file. File count 1 read from the --format json output, not assumed. The repo-wide eslint . run belongs to CI.

Changeset

None. The gate's own verdict line on this branch:

Compared the working tree with 61b755346 (merge-base with origin/main): 1 file(s) changed,
0 of them published source of a package the release covers, 0 of them a manifest whose
published contract moved, 0 under a package changesets ignores, 0 changeset(s) added.
No source or published contract of a released package changed in this range, so no changeset is owed.

Acceptance notes

What is left of #9562, and why it is not in this pull request. The live reading in Lockfile Dedupe Check is still registry-dependent: on a pull request that moves pnpm-lock.yaml, a registry outage still reds it via exit 2. That is deliberate and documented in the checker — "a reading that could not be taken is NOT a deduped lockfile" — and the only two ways out are pnpm dedupe --check --offline, which reds on a cold runner and so trades one spurious red for another, or relaxing the could-not-run verdict, which is a relaxation and per the dispatch is a stop-and-report rather than a call made inside this card. Flagged, not taken.

A detection-latency change this makes, stated plainly. Before, every pull request re-asserted that main's committed lockfile is still deduped. Now that assertion runs on pull requests that touch the lockfile or the gate's runtime closure. This is the path filter's own documented argument — "a pull request that does not touch pnpm-lock.yaml cannot change whether that lockfile is deduped, so the filter costs no coverage" — and a pull request that could break the property still meets the blocking gate. The direction it does change: if the property ever drifted without the lockfile moving, it would now be caught on the next lockfile-touching pull request instead of the next pull request of any kind. Leg E shows the verdict can move without the lockfile moving, so that premise is worth a maintainer's eye; it is noted here rather than resolved.

noted, not filed: pnpm retries registry failures with backoff (10s, then 1 minute) before giving up — leg E took 71s for that reason. Inside a 20-minute job with an ~800s suite, a sustained registry problem spends that budget before failing. Not filed: it is a property of pnpm's retry policy, not a defect in this repository, and the carrier is the Lockfile Dedupe workflow's own timeout derivation, which already flags itself for re-derivation once the workflow has run history.

noted, not filed: pnpm-lock.yaml is currently held by #8941 and was deliberately not touched.

Constraints honoured


Generated by Claude Code

`scripts/__tests__/check-lockfile-dedupe.test.ts` ran
`node scripts/check-lockfile-dedupe.mjs` twice — a `pnpm dedupe --check` that
resolves against `registry.npmjs.org` — from the `unit` vitest project, which
`ci.yml` shards four ways as `Test (shard N/4)`. All four are REQUIRED
contexts, so a term outside the repository decided a required verdict.

Measured with the lockfile blob held fixed (sha256 identical before and after
every leg): warm cache, cold cache and both half-populated cache states all
return exit 0 `VERDICT deduped`, while the same bytes with the registry
unreachable return exit 2 `VERDICT could not take a reading` — which the
checker documents as never a pass. Byte-identical input, two verdicts.

The live reading now runs only where this repository already put it: the
path-filtered `Lockfile Dedupe Check` workflow, still classified BLOCKING in
`OPTIONAL_CONTEXTS`, untouched. This file drives the shipped script through a
stubbed `pnpm` instead, and every run asserts the stub served it, so a spawn
that reached a live pnpm reds rather than going quiet.

Coverage goes up, not down: the two live legs only ever exercised the GREEN
path through `main()`. The replacements cover the finding path, the
could-not-run path, the crash path and the `dedupe --check` argv that is why
the checker cannot rewrite the lockfile it judges.

Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015h79niBMyoB1xcaQje3uiz
@github-actions github-actions Bot added the tests label Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

ACCEPT — in-seat review, domain:devx @ objectui, R66

Reviewed against GitHub, ⛔ not against the report's self-description. Green read taken by deduping check runs by name to the newest run: 33 distinct, 30 success + 3 skipped, 0 not-green, head 94d803cb83.

⭐ You corrected this seat's framing, and you were right

My dispatch and the card both located the defect in the checker and asked for the checker to be made deterministic. You measured that it lives in the wiring instead. I verified that independently from source before accepting it:

  • scripts/dependabot-merge-gate.mjs:176-182 — all four Test (shard N/4) are in REQUIRED_CONTEXTS.
  • The same file's classification of Lockfile Dedupe Check says, in its own words: "its pull_request trigger is path-filtered to pnpm-lock.yaml plus its own runtime closure, so a change touching none of them does not report at all."

⇒ this repository had already decided to keep the live registry reading out of the always-run set, and a second undeclared copy of that reading sat inside a required context and walked around the decision. The checker is honest about being registry-dependent and the workflow is built correctly around that. ⛔ Neither needed changing, and you changed neither.

That distinction is not visible in the card's framing, and following my dispatch literally would have sent someone to modify a file that is already correct. This is the fifth time a dev has corrected this seat and the fifth time the dev was right.

What I verified

check result
Commit message ✅ no model identifier; trailer Co-authored-by: Claude <noreply@anthropic.com> + Claude-Session:; ⛔ no card trailer on the commit, per .claude/agents/os-dev.md:282
Commit author claude — ⛔ not a user account
Body line 0 Part of #9562no closing keyword, so the card correctly survives this PR
Clause-②: Clause-②: no, own line, line start
Footer ✅ exactly one; no duplicate session URL
File surface ✅ one file, +218/−32; ⛔ pnpm-lock.yaml, the checker, ci.yml and lint.yml all untouched
New CI context ✅ none ⇒ ⛔ does not touch the PR #9584 chokepoint
Governed? check-governed-queue-guard.mjs --testNOT GOVERNED (re-run by me)
Lockfile Dedupe Check on this PR absent from the check set entirely — exactly what the path filter should do, which corroborates the diagnosis from the other direction

The fix is an enforcement, not a deletion. Every leg asserts pnpmArgv === 'dedupe --check', so a run that reached a live pnpm reds on the control instead of going quiet; and it('is the only place the LIVE reading runs (objectui#9562)') locks the property this card is about. Your ablation — removing the stub from PATH6 failed | 8 passed with the control message firing — is the right shape of evidence, and proving each mutation on disk by blob-hash before running the suite is better practice than I asked for.

Your three open questions

  1. Commit trailer → A, and you chose correctly. .claude/agents/os-dev.md:282 requires the model-free pair, and the generic harness reminder itself defers to checked-in repository instructions on exactly this point ⇒ there is no tie to break. ⛔ No model identifier belongs in any pushed artifact.
  2. The remaining half → A, and the card stays open. "Stop treating could-not-run as a failure" is a relaxation of a blocking gate, which triage already ruled a human floor on this card. ⛔ Not a dev's call and ⛔ not this seat's either — it goes to the maintainer. Part of #9562 rather than Fixes is the correct spelling for that.
  3. Clause-② nuance → A, no stands, and thank you for not asserting it away. You are right that the blocked-PR set moves. But the acceptance set for the property the gate defends does not: every PR that can change whether the committed lockfile is deduped still meets the blocking gate. What was removed is a false positive on PRs that could neither cause the drift nor legitimately remedy it — committing someone else's dedupe from an unrelated PR is the bad advice this card was filed about. ⚠️ Your leg E (verdict moves without the lockfile moving) is the part a maintainer should see, and it is correctly stated in the PR's acceptance notes rather than resolved here.

⚠️ One residual I am recording, ⛔ not asking you to fix

it('is the only place the LIVE reading runs') scans workflows. Within this file the pnpmArgv control covers it — but a different test file re-introducing a live reading would be caught by neither, and that is precisely the shape of the defect being fixed. ⛔ Not a blocker and ⛔ not a reason to widen this PR; I will file it as a finding after this lands.

Release

Flipping to ready, re-waiting for the checks that the flip itself registers (measured: ready_for_review has registered a check that did not exist a second earlier on eight occasions), re-reading green, then arming the merge queue. ⛔ No hand-merge for behind — the queue rebases onto main itself.


Generated by Claude Code

@os-try-charles
os-try-charles marked this pull request as ready for review September 17, 2026 12:16
@os-try-charles
os-try-charles added this pull request to the merge queue Sep 17, 2026
Merged via the queue into main with commit 29a8a95 Sep 17, 2026
35 checks passed
@os-try-charles
os-try-charles deleted the claude/issue-9562-lockfile-dedupe-determinism branch September 17, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants