From 55e347b7ea96e016c4789045d83d34f82e4917e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 20:47:51 +0000 Subject: [PATCH] test(core): resolve the `__` operation-private-key pin's scan surface from git, not a readdirSync denylist (#7706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_015oe9MbWXhHmppcqxQv9Hh5 --- .../operation-private-keys.pin.test.ts | 309 ++++++++++++++---- 1 file changed, 241 insertions(+), 68 deletions(-) diff --git a/packages/core/src/security/operation-private-keys.pin.test.ts b/packages/core/src/security/operation-private-keys.pin.test.ts index 965d01fb9b..23f7a950af 100644 --- a/packages/core/src/security/operation-private-keys.pin.test.ts +++ b/packages/core/src/security/operation-private-keys.pin.test.ts @@ -28,10 +28,57 @@ * * Reworded freely: the pin matches DECLARATIONS, not mentions. Documentation, * comments and tests may name either symbol as much as they like. + * + * ── [#7706] Why the scan surface is git's and not `readdirSync`'s ───────────── + * + * This pin used to crawl `packages/` with `readdirSync`/`statSync` and skip a + * hand-maintained denylist (`node_modules`, `dist`, `build`, `.turbo`, + * `coverage`, `.next`). That crawl was wrong in two ways that only showed up + * inside merge-queue builds, where the suite runs concurrently with a full + * monorepo build rather than against a quiet checkout: + * + * 1. NONDETERMINISM. The denylist was never exhaustive against what a build + * actually deposits. `check:skill-examples` extracts every marked prose code + * block verbatim into `packages/spec/.examples-build/*.ts` — under + * `packages/`, matching `.ts`, and in none of the six skipped names. Any + * docs or skill example that *shows* the convention therefore became a + * transient offender, and whether the pin saw it depended purely on how the + * vitest shard interleaved with that gate. Same tree, different verdict. + * + * 2. COST. The crawl read all ~3,600 `.ts` files under `packages/` and ran the + * regex on each — ~355ms on an idle machine, under vitest's DEFAULT 5s + * timeout. On a queue runner busy with a full build that overran, and the + * observed failure was `Error: Test timed out in 5000ms` with NO offender + * list: the assertion never ran at all, so the pin reported nothing about + * the property it guards while taking unrelated PRs out of the batch. + * + * Both are fixed by asking git for the candidate set instead of the filesystem. + * The surface is now tracked files PLUS untracked ones, with ignored paths + * excluded (`--others --exclude-standard` / `--untracked`) — i.e. exactly the + * files a human authored, never build output, because build output is ignored + * by construction. That deliberately does NOT narrow to tracked-only: the + * docblock above promises a fourth author goes red "the first time they run the + * suite", and a file they have 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, the most expensive place to find out. `--untracked` keeps the + * local-loop promise intact while still excluding the build output that caused + * (1). + * + * `git grep` is a PREFILTER, never the verdict. It searches for the two + * identifiers as FIXED STRINGS, which is a provably exact superset of what + * `declarationMatcher()` can match (the regex cannot match text that lacks the literal + * identifier), so no judgement moves into git's regex dialect — the JS regex + * below remains the sole authority on what counts as a declaration. Reading only + * the handful of files that mention a symbol, rather than all ~3,600, is also + * what removes the timeout exposure: one `git grep` process instead of ~3,600 + * sequential `readFileSync` round-trips, which is the part that degrades + * non-linearly when a runner is I/O-saturated. */ -import { readFileSync, readdirSync, statSync } from 'node:fs'; -import { dirname, join, relative, resolve } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { dirname, join, relative, resolve, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, it, expect } from 'vitest'; @@ -39,10 +86,16 @@ import { describe, it, expect } from 'vitest'; const HERE = dirname(fileURLToPath(import.meta.url)); /** …/packages/core/src/security → repo root */ const REPO_ROOT = resolve(HERE, '../../../..'); -const PACKAGES = join(REPO_ROOT, 'packages'); + +/** git speaks repo-relative POSIX paths; match it so the two sets compare. */ +const toRepoPath = (absolute: string) => relative(REPO_ROOT, absolute).split(sep).join('/'); /** The one file allowed to declare the convention. */ -const HOME = join(HERE, 'operation-private-keys.ts'); +const HOME = toRepoPath(join(HERE, 'operation-private-keys.ts')); + +/** The two guarded identifiers, as data — naming them cannot declare them. */ +const PREFIX_SYMBOL = 'OPERATION_PRIVATE_KEY_PREFIX'; +const HELPER_SYMBOL = 'withoutOperationPrivateKeys'; /** * A DECLARATION of either symbol — `const OPERATION_PRIVATE_KEY_PREFIX =` or @@ -52,76 +105,196 @@ const HOME = join(HERE, 'operation-private-keys.ts'); * (`export { … } from`), calls and prose never match. Both spellings a copy * could plausibly take are covered: a `function` declaration is what all three * copies used, and `const … =` catches the arrow-function rewrite. + * + * The statement anchor is also why this very file — which quotes both spellings + * in its docblock and passes both to `git grep` below — is not its own offender: + * a docblock line begins with ` * `, and a string-literal or regex-source line + * begins with something other than `const`/`let`/`var`/`function` followed by a + * guarded name. That property is load-bearing, so it is asserted rather than + * trusted — see the last test. + * + * Returned as a FRESH regex per call, never shared. A `/g` regex carries + * `lastIndex` across calls, so one shared instance makes the result depend on + * the order files happen to come back in: after a successful `.test()` on the + * home file, a later `matchAll` on the same pattern silently resumes from the + * middle of the text and reports one declaration instead of two. That is the + * same species of order-dependent verdict this file exists to eliminate, so it + * is designed out rather than reset around. */ -const DECLARATION = +const declarationMatcher = (): RegExp => /^\s*(?:export\s+)?(?:const|let|var|function)\s+(OPERATION_PRIVATE_KEY_PREFIX|withoutOperationPrivateKeys)\b\s*[=(<]/gm; -const SKIP_DIRS = new Set(['node_modules', 'dist', 'build', '.turbo', 'coverage', '.next']); +/** + * Generous on purpose (#7706). The scan costs tens of milliseconds, so this is + * not a budget — it is headroom against a merge-queue runner that is doing a + * full monorepo build at the same time. The default 5s was the whole failure: + * the pin timed out before the assertion ran, and a pin that reports nothing is + * indistinguishable from a pin that found nothing. + */ +const SCAN_TIMEOUT_MS = 60_000; -/** Every `.ts`/`.tsx` file under `packages/`, excluding build output. */ -function sourceFiles(dir: string, out: string[] = []): string[] { - for (const entry of readdirSync(dir)) { - if (SKIP_DIRS.has(entry)) continue; - const full = join(dir, entry); - if (statSync(full).isDirectory()) sourceFiles(full, out); - else if (/\.tsx?$/.test(entry) && !entry.endsWith('.d.ts')) out.push(full); +function git(args: string[]): string[] { + let stdout: string; + try { + stdout = execFileSync('git', args, { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 1 << 28, + }); + } catch (error) { + const failure = error as { status?: number; stderr?: string }; + // `git grep` exits 1 for "found nothing", which is data. Anything else is a + // BROKEN scan, and must not be allowed to read as "no offenders" — throwing + // here, plus the anti-vacuity tests below, is what keeps a green result + // meaning "looked and found nothing" rather than "never looked". + if (failure.status === 1) return []; + throw new Error( + `git ${args.join(' ')} failed with status ${String(failure.status)}: ${failure.stderr ?? ''}`, + ); } - return out; + return stdout.split('\0').filter((line) => line.length > 0); +} + +const isScannedSource = (path: string) => /\.tsx?$/.test(path) && !path.endsWith('.d.ts'); + +/** + * The scan surface: every `.ts`/`.tsx` file under `packages/` that a human + * authored — tracked plus untracked, ignored paths (build output) excluded. + */ +function scannedFiles(): string[] { + return git([ + 'ls-files', + '-z', + '--cached', + '--others', + '--exclude-standard', + '--', + 'packages', + ]).filter(isScannedSource); +} + +/** + * The subset of the scan surface that so much as mentions either identifier. + * Fixed-string, so it is an exact superset of `declarationMatcher()`'s matches; the regex + * still decides which of these — if any — is an actual declaration. + */ +function filesMentioningASymbol(): string[] { + return git([ + 'grep', + '--files-with-matches', + '-z', + '--untracked', + '--text', + '--fixed-strings', + '-e', + PREFIX_SYMBOL, + '-e', + HELPER_SYMBOL, + '--', + 'packages', + ]).filter(isScannedSource); +} + +function declaringFiles(): string[] { + return filesMentioningASymbol().filter((file) => + declarationMatcher().test(readFileSync(join(REPO_ROOT, file), 'utf8')), + ); } describe('the `__` operation-private-key convention has one owner (#7284)', () => { - it('is declared in exactly one file, and that file is the shared home', () => { - const offenders: string[] = []; - - for (const file of sourceFiles(PACKAGES)) { - if (file === HOME) continue; - const text = readFileSync(file, 'utf8'); - DECLARATION.lastIndex = 0; - if (DECLARATION.test(text)) offenders.push(relative(REPO_ROOT, file)); - } - - expect( - offenders, - offenders.length === 0 - ? '' - : [ - 'These files declare their own copy of the `__` operation-private-key convention:', - ...offenders.map((f) => ` - ${f}`), - '', - 'That rule has a single owner since #7284. Import it instead:', - '', - " import { withoutOperationPrivateKeys } from '@objectstack/core';", - '', - 'The reasoning — why a consumer must drop these keys, why by PREFIX and', - 'never by a name list, and why the copy is load-bearing in both', - 'directions — lives at packages/core/src/security/operation-private-keys.ts.', - 'If you are adding a consumer, add it to that header\'s "Known consumers"', - 'list rather than re-deriving the argument locally.', - ].join('\n'), - ).toEqual([]); - }); - - it('the home really does declare both symbols — the scan cannot pass vacuously', () => { - // #4690: a check that finds nothing because it is looking in the wrong place - // reads exactly like a check that found no violations. Anchor it. - const text = readFileSync(HOME, 'utf8'); - const found = [...text.matchAll(DECLARATION)].map((m) => m[1]).sort(); - - expect(found).toEqual(['OPERATION_PRIVATE_KEY_PREFIX', 'withoutOperationPrivateKeys']); - }); - - it('the scan reaches the packages that used to hold the copies', () => { - // The second half of the same anti-vacuity guard: prove the walker actually - // descends into the three consumer packages, so a future refactor of - // SKIP_DIRS or the walk cannot silently narrow the scan to `packages/core`. - const scanned = sourceFiles(PACKAGES).map((f) => relative(REPO_ROOT, f)); - - for (const consumer of [ - 'packages/plugins/plugin-audit/src/comment-access-hooks.ts', - 'packages/services/service-storage/src/attachment-access-hooks.ts', - 'packages/plugins/plugin-reports/src/report-service.ts', - ]) { - expect(scanned).toContain(consumer); - } - }); + it( + 'is declared in exactly one file, and that file is the shared home', + () => { + const offenders = declaringFiles().filter((file) => file !== HOME); + + expect( + offenders, + offenders.length === 0 + ? '' + : [ + 'These files declare their own copy of the `__` operation-private-key convention:', + ...offenders.map((f) => ` - ${f}`), + '', + 'That rule has a single owner since #7284. Import it instead:', + '', + " import { withoutOperationPrivateKeys } from '@objectstack/core';", + '', + 'The reasoning — why a consumer must drop these keys, why by PREFIX and', + 'never by a name list, and why the copy is load-bearing in both', + 'directions — lives at packages/core/src/security/operation-private-keys.ts.', + 'If you are adding a consumer, add it to that header\'s "Known consumers"', + 'list rather than re-deriving the argument locally.', + ].join('\n'), + ).toEqual([]); + }, + SCAN_TIMEOUT_MS, + ); + + it( + 'the home really does declare both symbols — the scan cannot pass vacuously', + () => { + // #4690: a check that finds nothing because it is looking in the wrong + // place reads exactly like a check that found no violations. Anchor it at + // both stages, because since #7706 there are two places the scan can go + // silently blind: the git prefilter can fail to reach the home file at + // all, and the regex can fail to match it. + expect(filesMentioningASymbol()).toContain(HOME); + + // Deliberately AFTER a full scan, not before: this is the regression pin + // for the shared-`lastIndex` trap described on `declarationMatcher`. With + // one shared `/g` regex, whether this sees two declarations or one depends + // on where HOME fell in the scan order — green today, red the day a + // package is added after it. It must be indifferent to that. + declaringFiles(); + + const text = readFileSync(join(REPO_ROOT, HOME), 'utf8'); + const found = [...text.matchAll(declarationMatcher())].map((m) => m[1]).sort(); + + expect(found).toEqual([PREFIX_SYMBOL, HELPER_SYMBOL].sort()); + }, + SCAN_TIMEOUT_MS, + ); + + it( + 'the scan reaches the packages that used to hold the copies', + () => { + // The second half of the same anti-vacuity guard: prove the scan actually + // descends into the three consumer packages, so a future refactor of the + // scan surface cannot silently narrow it to `packages/core`. + const scanned = scannedFiles(); + + for (const consumer of [ + 'packages/plugins/plugin-audit/src/comment-access-hooks.ts', + 'packages/services/service-storage/src/attachment-access-hooks.ts', + 'packages/plugins/plugin-reports/src/report-service.ts', + ]) { + expect(scanned).toContain(consumer); + } + + // Every file the prefilter surfaces must be one the enumerated surface + // covers. If the two ever disagree — different pathspec, different ignore + // rules — the pin would be judging a set it does not enumerate, and this + // is the only place that would say so. + const surface = new Set(scanned); + expect(filesMentioningASymbol().filter((f) => !surface.has(f))).toEqual([]); + }, + SCAN_TIMEOUT_MS, + ); + + it( + 'this pin quotes both symbols without declaring them', + () => { + // The docblock names both spellings in prose and passes both to `git grep` + // as string literals, so this file is always in the prefilter's output. It + // must never be in the offender list. Asserting it makes the "statement + // anchor" claim above executable: loosen the matcher into something that + // matches mentions and this goes red immediately, rather than the pin + // quietly reporting itself and training everyone to ignore it. + const self = toRepoPath(fileURLToPath(import.meta.url)); + + expect(filesMentioningASymbol()).toContain(self); + expect(declaringFiles()).not.toContain(self); + }, + SCAN_TIMEOUT_MS, + ); });