From 78f661fcaca4a8a3afbb194afa826d182ddf2a02 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 06:40:57 -0700 Subject: [PATCH] fix(bench): close two provenance-gate bypasses and guard the no-op run git status --porcelain quotes any path with a space and prints renames as 'old -> new', so slicing the status prefix and prefix-testing the result missed both: a daemon source renamed IN, or one with a space in its name, read as a clean tree. Let git match instead -- -z (never quotes) with a pathspec -- and cover config/scripts/build-orcad.mjs, which link 5 executes but link 1 did not watch. --cycles 0 and --output-bytes 0 are the siblings of the --sample-every 0 fix: each leaves the churn loop doing no work while the summary still reports a provenance-stamped zero-growth series. Reject both. Also realpath repoRoot before the containment check (a symlinked worktree path failed it closed) and add the bench:daemon-pty-churn-rss script entry its siblings all have. --- package.json | 1 + .../daemon-build-provenance-gate.mjs | 32 +++++++++++++------ .../benchmarks/daemon-pty-churn-rss-bench.mjs | 10 ++++++ 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index c8e59299c70..f304a4a78b7 100644 --- a/package.json +++ b/package.json @@ -128,6 +128,7 @@ "bench:macos-computer-helper-owner-loss": "node config/scripts/macos-computer-helper-owner-loss-benchmark.mjs", "bench:startup": "pnpm run ensure:electron-runtime && node tests/tools/benchmarks/startup-time-bench.mjs", "bench:daemon-coldstart": "pnpm run ensure:electron-runtime && node tests/tools/benchmarks/daemon-coldstart-bench.mjs", + "bench:daemon-pty-churn-rss": "node tests/tools/benchmarks/daemon-pty-churn-rss-bench.mjs", "bench:wsl-hook-relay-reattach": "pnpm run build:relay && node config/scripts/wsl-hook-relay-reattach-benchmark.mjs", "bench:wsl-git-shell": "node config/scripts/wsl-git-shell-benchmark.mjs", "bench:hang-watchdog-memory": "pnpm run ensure:electron-runtime && node config/scripts/hang-watchdog-memory-benchmark.mjs", diff --git a/tests/tools/benchmarks/daemon-build-provenance-gate.mjs b/tests/tools/benchmarks/daemon-build-provenance-gate.mjs index e4a7cfd125c..f801238b9fe 100644 --- a/tests/tools/benchmarks/daemon-build-provenance-gate.mjs +++ b/tests/tools/benchmarks/daemon-build-provenance-gate.mjs @@ -31,14 +31,28 @@ export function assertDaemonBuildProvenance(repoRoot) { const headSha = git('rev-parse', 'HEAD') const branch = git('rev-parse', '--abbrev-ref', 'HEAD') - const relevant = ['src/main/', 'src/shared/', 'config/patches/', 'pnpm-lock.yaml', 'package.json'] - // NOTE: no .trim() on the whole output — porcelain lines start with a - // two-character status whose first column may be a meaningful space. - const dirty = execFileSync('git', ['status', '--porcelain'], { cwd: repoRoot, encoding: 'utf8' }) - .split('\n') - .filter((line) => line.length > 3) - .map((line) => line.slice(3)) - .filter((file) => relevant.some((prefix) => file.startsWith(prefix))) + const relevant = [ + 'src/main', + 'src/shared', + 'config/patches', + 'config/scripts/build-orcad.mjs', + 'pnpm-lock.yaml', + 'package.json' + ] + // Why -z and a pathspec rather than parsing paths out of the default format: the + // default quotes any path with a space or non-ASCII byte (core.quotePath), and a + // rename prints `old -> new`, so a prefix test on the sliced line misses both — a + // daemon source moved in, or one with a space in its name, would read as clean. + // With -z git never quotes, and letting git do the matching removes the parse. + const dirty = execFileSync('git', ['status', '--porcelain', '-z', '--', ...relevant], { + cwd: repoRoot, + encoding: 'utf8' + }) + .split('\0') + // -z emits a bare `new\0old\0` pair for R/C; the status record is the one with + // the two-column prefix, and the trailing source path carries none. + .filter((record) => record.length > 3 && record[2] === ' ') + .map((record) => record.slice(3)) if (dirty.length > 0) { throw new Error( `PROVENANCE FAILURE: daemon-relevant paths are dirty; results would not be attributable to HEAD ${headSha}:\n ${dirty.join('\n ')}` @@ -63,7 +77,7 @@ export function assertDaemonBuildProvenance(repoRoot) { `PROVENANCE FAILURE: installed node-pty at ${installedRealRoot} was not built from the checked-out patch (expected patch_hash=${patchSha256}). Run pnpm install.` ) } - if (!installedRealRoot.startsWith(repoRoot + sep)) { + if (!installedRealRoot.startsWith(realpathSync(repoRoot) + sep)) { throw new Error( `PROVENANCE FAILURE: node_modules/node-pty resolves outside this worktree (${installedRealRoot}); a sibling worktree's install cannot prove this HEAD` ) diff --git a/tests/tools/benchmarks/daemon-pty-churn-rss-bench.mjs b/tests/tools/benchmarks/daemon-pty-churn-rss-bench.mjs index 600d729671c..24f4cc02f34 100644 --- a/tests/tools/benchmarks/daemon-pty-churn-rss-bench.mjs +++ b/tests/tools/benchmarks/daemon-pty-churn-rss-bench.mjs @@ -72,6 +72,11 @@ function parseArgs(argv) { break case '--cycles': args.cycles = num('--cycles') + // Zero cycles churns nothing: the loop never runs, `samples` holds only the + // baseline, and the summary reports a flat zero-growth series from no work. + if (args.cycles === 0) { + throw new Error('--cycles requires a positive integer') + } break case '--sample-every': args.sampleEvery = num('--sample-every') @@ -83,6 +88,11 @@ function parseArgs(argv) { break case '--output-bytes': args.outputBytes = num('--output-bytes') + // Zero bytes leaves `command` null, so every cycle degenerates to bare + // create+kill and the RSS series flattens for lack of churn, not for lack of a leak. + if (args.outputBytes === 0) { + throw new Error('--output-bytes requires a positive integer') + } break case '--settle-ms': args.settleMs = num('--settle-ms')