test(ci): take the live registry reading out of a REQUIRED context (objectui#9562) - #9690
Conversation
`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
ACCEPT — in-seat review,
|
| 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 #9562 — no 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 --test → NOT 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 PATH → 6 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
- Commit trailer → A, and you chose correctly.
.claude/agents/os-dev.md:282requires 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. - 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 #9562rather thanFixesis the correct spelling for that. Clause-②nuance → A,nostands, 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
Part of #9562
The card reports that
scripts/check-lockfile-dedupe.mjsreturns different verdicts on a byte-identicalpnpm-lock.yamlwhile sitting in the blockingTest (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.tsrannode scripts/check-lockfile-dedupe.mjstwice — apnpm dedupe --checkthat resolves againstregistry.npmjs.org. That file is in theunitvitest project, whichvitest.config.mtsincludes asscripts/**/*.test.tsandci.ymlshards four ways asTest (shard N/4). All four shards are inREQUIRED_CONTEXTSinscripts/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.ymlwas built to prevent. That workflow is path-filtered topnpm-lock.yamlplus 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 —
sha256identical 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.VERDICT dedupedVERDICT dedupedVERDICT dedupedVERDICT dedupedVERDICT could not take a readingExit 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 atpnpm dedupeand 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 printsVERDICT 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 isb57d9644and the card's was4e954308, 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 Checkcontext, still classified BLOCKING inOPTIONAL_CONTEXTS, untouched.scripts/dependabot-merge-gate.mjs,lockfile-dedupe.ymland the checker itself are all unmodified.The test file now drives the shipped script through a stubbed
pnpmonPATH, following the conventioncheck-doc-snippet-types.test.tsalready 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:::error title=annotation, and that a finding never reaches stdout;dedupe --checkargv. This replaces the olddoes not rewrite the lockfile it judgesleg with its cause: the old leg could only catch a dropped--checkif 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
HEADblob before the suite ran, each restore proved by blob-hash equality plus an emptygit diff HEAD.PATHso the real pnpm serves the run:6 failed | 8 passed, control messagethe stub did not serve this runfired. Every hermetic test reds on the control rather than silently going live.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
perlno-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.ts—14 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 skippedtests.tsc -p tsconfig.scripts.json --noEmit— 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 --teston the changed path:NOT GOVERNED.eslint --no-inline-config --format jsonon the one changed file, 0 errors / 0 warnings, run at94d803cb8. Population read fromeslint.config.jsitself, which configures noproject/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 count1read from the--format jsonoutput, not assumed. The repo-wideeslint .run belongs to CI.Changeset
None. The gate's own verdict line on this branch:
Acceptance notes
What is left of #9562, and why it is not in this pull request. The live reading in
Lockfile Dedupe Checkis still registry-dependent: on a pull request that movespnpm-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 arepnpm 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 touchpnpm-lock.yamlcannot 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 Dedupeworkflow's own timeout derivation, which already flags itself for re-derivation once the workflow has run history.noted, not filed:
pnpm-lock.yamlis currently held by #8941 and was deliberately not touched.Constraints honoured
pnpm-lock.yamlcommitted; the lockfile is not in this diff..github/workflows/ci.yml,lint.ymlandscripts/dependabot-merge-gate.mjsare untouched and stay with ci: oneTestaggregator becomes the required test context, shards 4 -> 8, dist pins get their own job #9584.Generated by Claude Code