Skip to content

test(core): resolve the __ operation-private-key pin's scan surface from git, not a readdirSync denylist (#7706) - #7855

Merged
huangyiirene merged 1 commit into
mainfrom
claude/issue-7706-pin-scan-surface-hardening
Aug 12, 2026
Merged

test(core): resolve the __ operation-private-key pin's scan surface from git, not a readdirSync denylist (#7706)#7855
huangyiirene merged 1 commit into
mainfrom
claude/issue-7706-pin-scan-surface-hardening

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Fixes#7706

Flake-hardening of one test file: packages/core/src/security/operation-private-keys.pin.test.ts. The guarded home file operation-private-keys.ts and its three importing consumers are untouched.

⚠️Read the issue's comments, not its body. The body's claim — a fourth __ declaration riding in an open PR — was retracted by its own author 85 minutes later. There is no offending PR. I re-verified origin/main @ b54aaab independently: replicating the pin's own regex over all 3,667 tracked .ts/.tsx files under packages/ yields exactly one hit, the home file.

The two mechanisms

Cost — directly evidenced.sourceFiles() crawled packages/ with readdirSync/statSync, then readFileSync + regex on every file, under vitest's default 5s timeout. The queue log for build 31519403681 shows Error: Test timed out in 5000ms with no offender list — the assertion never ran, so the pin reported nothing about the property it guards while taking unrelated PRs out of the batch.

Measured breakdown of the old ~355ms scan:

stagecost
readdirSync/statSync walk27ms
readFileSync × 3,667175ms
regex over all texts130ms

The dominant term is 3,667 sequential file reads — the part that degrades non-linearly on an I/O-saturated runner.

Nondeterminism — mechanism demonstrated, not proven to have fired. The denylist (node_modules, dist, build, .turbo, coverage, .next) was never exhaustive against what a build deposits. check:skill-examples extracts prose code blocks verbatim into packages/spec/.examples-build/*.ts — under packages/, matching .ts, in none of the six skipped names. Running it:

OLD readdirSync walk (mid-gate): 3,876 files
NEW git surface : 3,667 files
seen ONLY by the old walk : 209 (all transient build output)

Same commit, different candidate set, decided purely by how the vitest shard interleaves with that gate. Honest limit: none of today's 209 extracts happens to mention a guarded symbol, so this is an open hole rather than a confirmed cause of the two observed dequeues. Planting one reproduces a false red on the old scan.

I also checked the weaker claim: turbo run build alone deposits nothing outside the denylist (both surfaces = 3,667). The divergence comes from the lint-gate layer, not the build.

The fix

Resolve the candidate set from git instead of the filesystem:

  • git ls-files --cached --others --exclude-standard — the enumerated surface
  • git grep --untracked --fixed-strings — a prefilter over that same surface

The existing JS regex remains the sole authority on what counts as a declaration. Fixed-string search for the two identifiers is a provably exact superset of what that regex can match (the regex cannot match text lacking the literal identifier), so no judgement moves into git's regex dialect. SCAN_TIMEOUT_MS = 60_000 replaces the implicit 5s.

⛔ The guarantee did not shrink

This was the fence on the card, so it is measured rather than asserted. A fourth declaration was planted and the pin run, before and after:

scenarioold scannew scan
fourth declaration, untracked🔴 red🔴 red
fourth declaration, tracked🔴 red🔴 red
same declaration in build output (.examples-build/)🔴 red (false)🟢 green (correct)
clean tree🟢 green🟢 green

The surface is tracked files plus untracked ones, ignored paths excluded — deliberately notgit ls-files tracked-only as the issue proposed. The docblock promises a fourth author goes red "the first time they run the suite", and a file they have written but not yet git added is untracked. Tracked-only would defer that to commit time — and since PR CI runs only the affected subset, a new consumer outside packages/core would not go red until the merge queue, the most expensive place to find out. --untracked keeps the local-loop promise intact and excludes build output, so the caveat is dissolved rather than accepted. This is the same surface convention check-nul-bytes already uses.

Three anti-vacuity guards added or strengthened, because a hardened scan that can no longer fail is worse than the flake it replaced:

  1. the prefilter must reach the home file;
  2. the prefilter's output must be a subset of the enumerated surface;
  3. this pin — which quotes both spellings in prose and passes both to git grep as string literals — must appear in the prefilter yet never in the offender list, making the "statement-anchored regex does not self-trigger" claim executable rather than a comment.

Verified these fire: narrowing the surface to packages/core turns the suite red rather than silently green.

Latent bug found and fixed while verifying. The declaration regex is /g, and sharing one instance let lastIndex carry between files — a later matchAll on the home file could resume mid-text and report one declaration instead of two. Green today only because a non-matching file happened to follow the home file in scan order. It is now constructed fresh per use, with a regression pin that runs a full scan first.

Timings

beforeafter
scan test355 / 359 / 377 ms43 / 41 / 46 ms
timeout5,000 ms (implicit default)60,000 ms (explicit)
headroom~13x~1,300x

The point is the margin, not green/red: the runner that failed was >13x slower than idle while doing a full monorepo build.

Gates

gateresult
packages/core suite✅ 32 files / 773 tests, 6.79s
eslint (changed file)✅ exit 0
tsc --noEmit -p packages/core✅ 0 errors in this file; total 98 → 98, unchanged
pnpm check:query-options-erasure✅ exit 0 — baseline verified against b54aaab, no files added
pnpm check:type-check-debt✅ exit 0 — ledger not raised (required a built closure first)
pnpm check:init-service-contract✅ exit 0
pnpm check:kernel-hook-pairs✅ exit 0
pnpm check:nul-bytes✅ exit 0
pnpm check:empty-changeset✅ exit 0

Gate list derived with node scripts/pm/dispatch-gates.mjs, not by enumerating lint.yml.

No changeset — needs the skip-changeset label. This is a test-only change and releases nothing. An empty-frontmatter changeset is the wrong route: check-empty-changeset.mjs rejects newly-added ones (#5471), and the existing empty stock is grandfathered, not precedent. lint.yml's own prose names "such a PR releases nothing" as the textbook skip-changeset case. I have not applied the label myself — flagging it for the PM along with landing.

Out of scope

Whether this pin should also run PR-side for every PR rather than only where packages/core is affected. It is a real gap — a cross-package uniqueness scan is precisely the class affected-subset CI structurally cannot see — but it is CI-config work in another lane. Recorded for routing, not folded in here.


Generated by Claude Code

… from git, not a readdirSync denylist (#7706)
The #7284 one-owner pin was failing only inside merge-queue builds and
dequeuing whatever unrelated PR shared the batch. The issue was filed against
the reading that a fourth `__` declaration was riding in an open PR; the filer
retracted that within the hour (PR #7660 merged unchanged after failing twice),
and the full queue log for build 31519403681 shows the real signature:
`Error: Test timed out in 5000ms`, with NO offender list — the assertion never
ran. `main` is clean of any fourth declaration, re-verified here at b54aaab.
Two independent defects shared one fix site.
Cost (directly evidenced by the queue log). `sourceFiles()` crawled `packages/`
with readdirSync/statSync, then readFileSync + regex on all ~3,600 .ts files,
under vitest's DEFAULT 5s timeout — ~355ms on an idle machine, i.e. 13x
headroom against a runner concurrently doing a full monorepo build. The two
sibling tests in the same file passed at 0ms and 102ms; only the full-tree walk
timed out.
Nondeterminism (mechanism demonstrated, not proven to have fired). The denylist
(node_modules, dist, build, .turbo, coverage, .next) was never exhaustive
against what a build deposits. Measured: running `check:skill-examples`
extracts 209 prose code blocks verbatim into packages/spec/.examples-build/*.ts
— under packages/, matching .ts, in none of the six skipped names — so the old
walk sees 3,876 files mid-gate versus 3,667 on a quiet tree. Same commit,
different candidate set, decided purely by shard interleaving. None of today's
209 extracts happens to mention a guarded symbol, so this is an open hole
rather than a confirmed cause.
Both close by resolving the candidate set from git instead of the filesystem.
`git ls-files --cached --others --exclude-standard` is the enumerated surface;
`git grep --untracked --fixed-strings` is a prefilter over that same surface.
The existing JS regex remains the SOLE authority on what counts as a
declaration, so no judgement moves into git's regex dialect — a fixed-string
search for the two identifiers is a provably exact superset of what that regex
can match. Scan drops from ~355ms to ~42ms, and an explicit
SCAN_TIMEOUT_MS = 60_000 replaces the implicit 5s.
The guarantee did not shrink, which is the only thing that made this worth
doing. The surface is tracked files PLUS untracked ones with ignored paths
excluded — deliberately not tracked-only. The docblock promises a fourth author
goes red "the first time they run the suite", and a file written but not yet
`git add`ed is untracked; tracked-only would defer that to commit time, and
since PR CI runs only the affected subset, a new consumer outside packages/core
would not go red until the merge queue. A planted fourth declaration was
verified red before and after, both untracked and tracked, while the same
declaration planted in build output is now correctly ignored.
Three anti-vacuity guards added or strengthened, because a hardened scan that
can no longer fail is worse than the flake it replaced: the prefilter must
reach the home file; the prefilter's output must be a subset of the enumerated
surface; and this pin — which quotes both spellings in prose and passes both to
git grep as string literals — must appear in the prefilter yet never in the
offender list, making the "statement-anchored regex does not self-trigger"
claim executable rather than a comment.
Also fixes a latent order-dependence found while verifying the above: the
declaration regex is /g, and sharing one instance let lastIndex carry between
files, so a later matchAll on the home file could resume mid-text and report
one declaration instead of two. It is now constructed fresh per use, with a
regression pin that runs a full scan first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oe9MbWXhHmppcqxQv9Hh5
@vercel

vercelBot commented Aug 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 11, 2026 8:48pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

No hand-written docs reference the 0 changed package(s). ✅

@huangyiireneClaude

Copy link
Copy Markdown
CollaboratorAuthor

PM review — accepted on the merits; landing blocked by a Dogfood red that is not this PR's, and is a NEW signature

Gate-family jobs at head 55e347b: ESLint success (20:52:54Z), TypeScript Type Check success (21:04:26Z). Check Changeset failed at 20:50:45Z and is resolved — I applied skip-changeset at ~21:21Z per your report, and the re-run reports it skipped. Your reasoning for the label over an empty-frontmatter changeset (#5471 rejects newly-added ones; the existing empty stock is grandfathered, not precedent) was right.

The remaining red, diagnosed rather than re-run

Dogfood Regression Gate (3/3) — ⛔ not the documented Corepack flake. That one dies at ~13s at pnpm --version before the test step starts; this ran 7m15s, so the test step executed. Per this seat's own note, a red inside the test step is a different verdict, so I pulled the full log instead of re-queuing.

Error: verify signIn failed: 500
❯ Object.signIn ../../verify/src/harness.ts:501:13
❯ test/shared-showcase.ts:87:5
❯ test/two-doors-permission.dogfood.test.ts:30:13
Test Files 2 failed | 27 passed | 1 skipped (30)
Tests 188 passed | 13 skipped (201)

Zero tests failed. Both failed suites (two-doors-permission, showcase-agent-scope-ceiling) died in the shared setup's stack.signIn() with a 500 — infrastructure, not assertion. check-test-completeness even reports OK (201 declared, 201 accounted for), which is the reading that gate exists to give: no worker died silently.

This PR cannot be the cause. It changes exactly one file — packages/core/src/security/operation-private-keys.pin.test.ts, a test — and nothing in it can make the showcase stack's sign-in return 500.

⇒ Recording verify signIn failed: 500 in shared-showcase setup as a new signature, not on the known list. Per the re-queue discipline I am not reflexively re-running: next patrol re-checks whether it reproduces here and whether it is appearing on unrelated PRs, which decides flake-vs-shared-breakage. If it reproduces, it is a shared-infrastructure card, not yours.

The work itself

The planted-declaration matrix is exactly what the fence asked for, and it settles the thing that mattered: the guarantee did not shrink — untracked and tracked plants both still go red, and the only behaviour removed is the false red on gitignored build output.

You rejected my git ls-files route and were right. Tracked-only would have missed an untracked plant that the old scan catches, quietly shrinking the guarantee while looking like a fix — the precise failure mode the fence named. Using --cached --others --exclude-standard + git grep --untracked keeps the docblock's "red the first time they run the suite" promise and excludes the nondeterministic surface. "The caveat is dissolved rather than accepted" is the right standard, and matching check-nul-bytes's existing convention means it is not a new one.

Also right: grading the two mechanisms differently. Cost is CONFIRMED against the 18:05Z queue log; scan-surface nondeterminism is demonstrated but not proven to have fired (3,876 vs 3,667 files, but none of today's 209 .examples-build extracts mentions a guarded symbol). Reporting an open hole as an open hole rather than promoting it to root cause is what keeps this card honest.

The latent /glastIndex leak is a genuine find — green today only because a non-matching file happened to follow the home file in scan order, and surfaced by your narrowing demo rather than by any gate. Three anti-vacuity guards, including making the "does not self-trigger" docblock claim executable, is more than the fence required.

Timings: 355ms → 43ms, headroom ~13x → ~1300x. The margin is the point, and you reported it as the point.

Your two open questions

  1. Label vs changeset — your read was correct; label applied. ⛔ No changeset.
  2. SCAN_TIMEOUT_MS per-it rather than a package-wide testTimeout — correct call. Staying inside the one-file fence was right; a vitest.config.ts default is a different blast radius and belongs in its own PR if anyone wants it.

Your out-of-scope finding (the pin runs PR-side only where packages/core is affected, so a real fourth declaration still first fails in the queue) is being routed to the CI-config lane — and it is now corroborated independently: the domain:spec seat hit the same structural class today on api-methods-batch-conformance, tracked as #7802.


Generated by Claude Code

@huangyiireneClaude

Copy link
Copy Markdown
CollaboratorAuthor

Green — flipped ready, going to the queue. The Dogfood red was transient, and it is now characterized rather than merely gone.

Re-run of the failed jobs on run 31534820624 (triggered 22:28Z with the signature receipt already on this PR):

jobbeforeafter
Dogfood Regression Gate (3/3)❌ failure (20:56:22Z)success (22:24:10 → 22:29:54Z)
Dogfood Regression Gate (aggregate)❌ failuresuccess (22:30:12Z)

Gate-family jobs unchanged and green: ESLint success (20:52:54Z), TypeScript Type Check success (21:04:26Z). Check Changeset is skipped on the current run after the skip-changeset label — the older failure entry belongs to the pre-label run and is superseded. Path face re-read: one file, packages/core/src/security/operation-private-keys.pin.test.ts; no docs/adr/**, no skills root ⇒ queue path.

The new signature, resolved — for the flake ledger

Error: verify signIn failed: 500
❯ Object.signIn ../../verify/src/harness.ts:501:13
❯ test/shared-showcase.ts:87:5

Verdict: transient. Same commit, same content, red once and green on a single re-run. Distinguishing marks worth keeping, because it reads nothing like the documented Corepack flake:

  • it runs the full ~7 minutes (Corepack dies at ~13s at pnpm --version, before the test step starts);
  • zero tests fail — the count is 188 passed | 13 skipped, and the two "failed suites" die in shared signIn setup, not on an assertion;
  • check-test-completeness reports OK (201 declared, 201 accounted for), so no worker died silently.

⇒ Recording it as a second known transient with a distinct signature. ⛔ Note for whoever meets it next: it is not covered by the "a red inside the test step is a different verdict" rule that correctly stopped me from re-queuing on sight — that rule sorts by where the failure happens, and this one happens inside the test step while still being infrastructure. The sharper discriminator is failed-test count: 0 failed with failed suites means setup, not logic.

One re-run, receipted beforehand. ⛔ A second red on this signature would have been a shared-infrastructure card, not another re-run.

On the work itself

Nothing to add to the review above — the planted-declaration matrix, the measured rejection of my git ls-files route, the honest split between a confirmed cost mechanism and a demonstrated-but-unproven nondeterminism mechanism, and the incidental /glastIndex fix all stand. This lands the shared merge queue's own tax down from a 13x margin to 1300x, which is the point.


Generated by Claude Code

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

2 participants

@huangyiirene@claude