From ccb91c3afb2942a1dbd76546441c30697272bf52 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 12 Aug 2026 16:02:11 -0700 Subject: [PATCH] fix: pre-landing review fixes for the v2 port wave MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review army (checklist + 5 specialists) + coverage/plan audits on the assembled branch. Genuine correctness/security/hygiene fixes: - test-paid-shards: strictTestExitCode now receives expectedFiles on the real bun path, so a shard that runs fewer files than planned (harness crash, nothing loaded) with exit 0 is no longer recorded 'passed' — the invisible-non-execution class the runner exists to kill. Pinned by the new test/strict-output.test.ts (also covers the chunk-boundary classifier). - test-paid-shards: EVALS_TIER env is validated (gate|periodic) like the --tier flag, so a typo can't self-skip every test and exit 0 green. - package.json: test:periodic:sharded sets EVALS_ALL=1, restoring the full-tier semantics the pre-shard script had (CI already set it; local eval:bg:periodic silently under-measured without it). - brain-sync.test: run() pins HOME to the temp home so gstack-artifacts-init stops writing/clobbering the operator's real ~/.gstack-artifacts-remote.txt every free-suite run; afterEach now also scrubs the current filename. - egress-receipt: cap each receipt field at 512B so a serialized line always fits the 4KB tail-read window — a longer line would make the next append hash a truncated prior line and verifyLedger report a permanent false TAMPER. warnLedgerSize short-circuits before statSync once fired (append hot path). - gstack-egress: import.meta.dir (Windows-safe) instead of new URL().pathname so grants doesn't silently report defaults on Windows; strip control chars from ledger-derived fields on render so a crafted receipt can't spoof the auditor's view. - extension/background.js + CLAUDE.md: renumber the identity-pin migration refs v1.62 -> v1.63 (main claimed 1.62.0.0; this wave queue-advances). - egress-receipt-wiring: pin lib/context-bill.ts unconditionally (both land together now); drop the dead RunShardsOptions.tier field. All fix-affected test files green; gate failures triaged as external-env (codex/gemini CLI drift) or pre-existing (hermetic-canary fails identically on base). Deferred polish tracked in the PR body + decision store. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 2 +- bin/gstack-egress | 27 +++++++++++-- extension/background.js | 14 +++---- lib/egress-receipt.ts | 18 ++++++++- package.json | 2 +- scripts/test-paid-shards.ts | 28 ++++++++++++-- test/brain-sync.test.ts | 25 ++++++++---- test/egress-receipt-wiring.test.ts | 6 ++- test/strict-output.test.ts | 61 ++++++++++++++++++++++++++++++ 9 files changed, 156 insertions(+), 27 deletions(-) create mode 100644 test/strict-output.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 7777a0967..f6339fd66 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -282,7 +282,7 @@ PTY via `window.gstackInjectToTerminal(text)`, exposed by `sidepanel-terminal.js`. No `/sidebar-command` POST — the live REPL is the only execution surface in the sidebar now. -**`/health` MUST NOT surface any token — and it no longer does** (v1.62+). +**`/health` MUST NOT surface any token — and it no longer does** (v1.63+). The historical headed-mode leak of `AUTH_TOKEN` is fixed: `GET /health` is liveness/status only in every mode. Token bootstrap is `POST /extension-token`, which validates the caller's Origin against the pinned extension identity diff --git a/bin/gstack-egress b/bin/gstack-egress index a1443b8b3..246448ef0 100755 --- a/bin/gstack-egress +++ b/bin/gstack-egress @@ -25,7 +25,23 @@ import { verifyLedger, } from '../lib/egress-receipt'; -const BIN_DIR = path.dirname(new URL(import.meta.url).pathname); +// import.meta.dir is Windows-safe; new URL(import.meta.url).pathname yields +// '/C:/...' with percent-encoded spaces there, which would make the +// gstack-config spawn silently fail and grants report every default. Matches +// the sibling bin/*.ts convention. +const BIN_DIR = import.meta.dir; + +/** + * Strip control characters (incl. ANSI escapes) from ledger-derived strings + * before printing. A receipt's host/payloadClass can derive from semi-trusted + * input (a URL argument, a git remote); JSON.parse restores \u001b escapes to + * live ESC bytes, so an attacker-shaped receipt could hide or spoof rows in + * the very output an auditor reads. The hash chain is unaffected; this only + * sanitizes the human render. + */ +function sanitizeForDisplay(value: unknown): string { + return String(value).replace(/[\u0000-\u001F\u007F]/g, ''); +} function usage(message: string): never { process.stderr.write(`gstack-egress: ${message}\n`); @@ -76,9 +92,14 @@ function egressList(args: string[], home: string): number { return 0; } for (const r of receipts) { + // sink/host/payload_class/consent can carry semi-trusted content — strip + // control chars so a crafted receipt can't spoof the auditor's view. ts, + // bytes, and sha256 are format-constrained at write time. process.stdout.write( - `${r.ts} ${r.sink} -> ${r.host} ${r.payload_class} ${r.bytes}B ` + - `sha256=${r.sha256 ?? '(subprocess-owned)'} consent=${r.consent} status=${r.status ?? '-'}\n`, + `${r.ts} ${sanitizeForDisplay(r.sink)} -> ${sanitizeForDisplay(r.host)} ` + + `${sanitizeForDisplay(r.payload_class)} ${r.bytes}B ` + + `sha256=${r.sha256 ?? '(subprocess-owned)'} consent=${sanitizeForDisplay(r.consent)} ` + + `status=${r.status ? sanitizeForDisplay(r.status) : '-'}\n`, ); } process.stdout.write(`${receipts.length} receipt(s) ledger: ${egressLedgerPath(home)}\n`); diff --git a/extension/background.js b/extension/background.js index 51a33c92e..a1e4cfa29 100644 --- a/extension/background.js +++ b/extension/background.js @@ -588,24 +588,24 @@ chrome.tabs.onUpdated.addListener((_id, changeInfo) => { } }); -// ─── v1.62 identity-pin migration notice ──────────────────────── +// ─── v1.63 identity-pin migration notice ──────────────────────── // -// The manifest "key" added in v1.62 pins the extension ID, which changes +// The manifest "key" added in v1.63 pins the extension ID, which changes // the ID for existing installs — chrome.storage.local is keyed by // extension ID, so panel-local state (saved port, snoozes) resets once. // Explain that in-product, one time. async function announceIdentityPinOnce() { try { - const data = await chrome.storage.local.get('gstack_id_migrated_v162'); - if (data.gstack_id_migrated_v162) return; - console.log('[gstack] gstack sidebar: extension identity pinned in v1.62 — panel state reset once.'); + const data = await chrome.storage.local.get('gstack_id_migrated_v163'); + if (data.gstack_id_migrated_v163) return; + console.log('[gstack] gstack sidebar: extension identity pinned in v1.63 — panel state reset once.'); chrome.runtime.sendMessage({ type: 'gstack-migration-notice', - message: 'gstack sidebar: extension identity pinned in v1.62 — panel state reset once.', + message: 'gstack sidebar: extension identity pinned in v1.63 — panel state reset once.', }).catch(() => { // Expected: panel not open. The console line above still lands. }); - await chrome.storage.local.set({ gstack_id_migrated_v162: true }); + await chrome.storage.local.set({ gstack_id_migrated_v163: true }); } catch (err) { console.debug('[gstack] identity-pin notice failed (non-fatal):', err.message); } diff --git a/lib/egress-receipt.ts b/lib/egress-receipt.ts index 93e5a774f..61b374430 100644 --- a/lib/egress-receipt.ts +++ b/lib/egress-receipt.ts @@ -121,8 +121,20 @@ function receiptError(message: string, cause?: unknown): Error & { code: string return error; } +// A serialized receipt line must stay under TAIL_READ_BYTES so the O(1) +// tail-read always captures the FULL previous line before hashing it into the +// chain. A caller-controlled field (sink/host/payloadClass/consent — e.g. +// context-bill builds payloadClass dynamically) long enough to push the line +// past the tail window would make the next append hash a truncated prior line, +// and verifyLedger would then report a permanent false TAMPER. Cap each field +// well under the window so the invariant holds by construction. +const MAX_FIELD_BYTES = 512; + function requireString(value: unknown, name: string): string { if (typeof value !== 'string' || !value) throw receiptError(`Egress receipt requires a non-empty ${name}`); + if (Buffer.byteLength(value) > MAX_FIELD_BYTES) { + throw receiptError(`Egress receipt ${name} exceeds ${MAX_FIELD_BYTES} bytes (${Buffer.byteLength(value)})`); + } return value; } @@ -199,13 +211,17 @@ export function resetLedgerSizeWarningForTests(): void { } function warnLedgerSizeOnce(ledger: string): void { + // Short-circuit BEFORE the stat: the warning fires at most once per process, + // so after it has fired there is no reason to stat the ledger on every + // subsequent writeReceipt (this runs on the append hot path). + if (warnedLedgerSize) return; let size: number; try { size = fs.statSync(ledger).size; } catch { return; // no file yet — nothing to warn about } - if (size <= LEDGER_WARN_BYTES || warnedLedgerSize) return; + if (size <= LEDGER_WARN_BYTES) return; warnedLedgerSize = true; process.stderr.write(ledgerSizeWarning(ledger, size) + '\n'); } diff --git a/package.json b/package.json index cc84d3d0b..a44803911 100644 --- a/package.json +++ b/package.json @@ -28,7 +28,7 @@ "test:gate": "EVALS=1 EVALS_TIER=gate bun test --retry 2 --concurrent --max-concurrency ${EVALS_CONCURRENCY:-15} test/skill-llm-eval.test.ts test/skill-e2e-*.test.ts test/skill-routing-e2e.test.ts test/codex-e2e.test.ts test/gemini-e2e.test.ts", "test:periodic": "EVALS=1 EVALS_TIER=periodic EVALS_ALL=1 bun test --retry 2 --concurrent --max-concurrency ${EVALS_CONCURRENCY:-15} test/skill-e2e-*.test.ts test/skill-routing-e2e.test.ts test/codex-e2e.test.ts test/gemini-e2e.test.ts", "test:gate:sharded": "bun run scripts/test-paid-shards.ts --tier gate", - "test:periodic:sharded": "bun run scripts/test-paid-shards.ts --tier periodic", + "test:periodic:sharded": "EVALS_ALL=1 bun run scripts/test-paid-shards.ts --tier periodic", "test:codex": "EVALS=1 bun test test/codex-e2e.test.ts", "test:codex:all": "EVALS=1 EVALS_ALL=1 bun test test/codex-e2e.test.ts", "test:gemini": "EVALS=1 bun test test/gemini-e2e.test.ts", diff --git a/scripts/test-paid-shards.ts b/scripts/test-paid-shards.ts index 975ec5267..fad74e868 100644 --- a/scripts/test-paid-shards.ts +++ b/scripts/test-paid-shards.ts @@ -167,7 +167,6 @@ export interface ShardCommand { } export interface RunShardsOptions { - tier?: PaidTier; timeoutMs?: number; jobs?: number; rootDir?: string; @@ -284,9 +283,17 @@ export async function runPaidShard( const summary = classifier.end(); if (!streamLive && buffered.length > 0) process.stdout.write(Buffer.concat(buffered)); + // Pass expectedFiles so a shard whose bun child ran fewer files than planned + // (or zero, all self-skipped) with exit 0 is NOT recorded 'passed' — the + // invisible-non-execution class this runner exists to kill. bun prints + // "Ran N tests across M files" with M = selected files even when every test + // self-skips, so terminalFileCounts must include files.length. Only enforced + // on the real bun path: an injected commandFor (tests) isn't bun and emits no + // terminal summary, so there's no file count to check against. + const expectedFiles = options.commandFor ? undefined : files.length; const status: ShardStatus = timedOut ? 'timed-out' - : strictTestExitCode(exitCode ?? 1, summary) === 0 ? 'passed' : 'failed'; + : strictTestExitCode(exitCode ?? 1, summary, expectedFiles) === 0 ? 'passed' : 'failed'; const elapsedMs = Date.now() - startedAt; log(`${label} ${status.toUpperCase()} in ${Math.round(elapsedMs / 1000)}s (exit ${exitCode ?? 'signal'})`); @@ -387,9 +394,21 @@ function parsePositiveInt(value: string | undefined, flag: string): number { return parsed; } +function validatedTier(value: string | undefined, source: string): PaidTier { + if (value === undefined || value === '') return DEFAULT_TIER; + // A typo'd EVALS_TIER (e.g. 'e2e', the tier string eval-store uses) would + // otherwise cast through unchecked, match nothing in the runtime E2E_TIERS + // filter, self-skip every test, and exit 0 with all shards 'passed' — the + // exact 0%-execution-looks-like-a-pass class this runner exists to kill. + if (value !== 'gate' && value !== 'periodic') { + throw new Error(`${source} must be gate or periodic. Received: ${value}`); + } + return value; +} + export function parseCliOptions(argv: string[], env: NodeJS.ProcessEnv = process.env): CliOptions { const options: CliOptions = { - tier: (env.EVALS_TIER as PaidTier) || DEFAULT_TIER, + tier: validatedTier(env.EVALS_TIER, 'EVALS_TIER'), listOnly: false, timeoutMs: env.EVALS_SHARD_TIMEOUT_MS ? parsePositiveInt(env.EVALS_SHARD_TIMEOUT_MS, 'EVALS_SHARD_TIMEOUT_MS') @@ -439,7 +458,8 @@ async function main(): Promise { } const summary = await runPaidShards(shards, { - tier: options.tier, + // Tier reaches the children only via EVALS_TIER below; the runtime + // E2E_TIERS filter inside each child is the real selection mechanism. timeoutMs: options.timeoutMs, jobs: options.jobs, env: { ...process.env, EVALS: '1', EVALS_TIER: options.tier }, diff --git a/test/brain-sync.test.ts b/test/brain-sync.test.ts index 140b008e5..a4587f806 100644 --- a/test/brain-sync.test.ts +++ b/test/brain-sync.test.ts @@ -34,7 +34,11 @@ function run(argv: string[], opts: { env?: Record; input?: strin const bin = argv[0]; const full = bin.startsWith('/') ? bin : path.join(BIN, bin); const res = spawnSync(full, argv.slice(1), { - env: { ...process.env, GSTACK_HOME: tmpHome, ...(opts.env || {}) }, + // HOME is overridden too: gstack-artifacts-init writes + // $HOME/.gstack-artifacts-remote.txt (plain $HOME, not GSTACK_HOME), so + // without this every free-suite run clobbers the operator's real + // artifacts-remote pointer. Keep it inside tmpHome, which afterEach removes. + env: { ...process.env, HOME: tmpHome, GSTACK_HOME: tmpHome, ...(opts.env || {}) }, encoding: 'utf-8', input: opts.input, cwd: ROOT, @@ -56,13 +60,18 @@ beforeEach(() => { afterEach(() => { fs.rmSync(tmpHome, { recursive: true, force: true }); fs.rmSync(bareRemote, { recursive: true, force: true }); - // Clean up any remote-helper file init may have written. - const remoteFile = path.join(os.homedir(), '.gstack-brain-remote.txt'); - // Only remove if it points at OUR bare remote (don't clobber a real user file). - try { - const contents = fs.readFileSync(remoteFile, 'utf-8').trim(); - if (contents === bareRemote) fs.unlinkSync(remoteFile); - } catch {} + // Clean up any remote-helper file init may have written. run() now pins + // HOME to tmpHome so these land inside the removed temp dir, but scrub the + // real home too as defense in depth — and cover BOTH the legacy brain-remote + // name and the current artifacts-remote name (init writes the latter). + for (const name of ['.gstack-brain-remote.txt', '.gstack-artifacts-remote.txt']) { + const remoteFile = path.join(os.homedir(), name); + // Only remove if it points at OUR bare remote (don't clobber a real user file). + try { + const contents = fs.readFileSync(remoteFile, 'utf-8').trim(); + if (contents === bareRemote) fs.unlinkSync(remoteFile); + } catch {} + } }); // --------------------------------------------------------------- diff --git a/test/egress-receipt-wiring.test.ts b/test/egress-receipt-wiring.test.ts index ea34f6601..82dafa7de 100644 --- a/test/egress-receipt-wiring.test.ts +++ b/test/egress-receipt-wiring.test.ts @@ -67,8 +67,10 @@ const MODULE_SINKS = [ 'bin/gstack-gbrain-sync.ts', 'bin/gstack-memory-ingest.ts', 'browse/src/server.ts', - // context-bill lands after this tripwire in the same wave; assert once present. - ...(exists('lib/context-bill.ts') ? ['lib/context-bill.ts'] : []), + // Unconditional: context-bill ships in the same tree as this tripwire. A + // missing file must fail loudly (a rename/move that drops its receipt wiring + // is exactly what this pins), not silently soften the assertion. + 'lib/context-bill.ts', ]; /** Shell sinks: must source the shared lib; every network op receipted. */ diff --git a/test/strict-output.test.ts b/test/strict-output.test.ts new file mode 100644 index 000000000..52f79fd48 --- /dev/null +++ b/test/strict-output.test.ts @@ -0,0 +1,61 @@ +/** + * Pins scripts/test-strict-output.ts — the verdict-integrity layer of the + * sharded paid runner. Its whole reason to exist is refusing to trust a zero + * exit when failures were printed OR fewer files ran than planned; paid-shards' + * fake commands never emit real Bun result lines, so without this file that + * core was exercised nowhere and a regex regression would silently revert the + * paid tier to trusting exit codes. + */ + +import { describe, expect, it } from 'bun:test'; +import { BunTestOutputClassifier, strictTestExitCode } from '../scripts/test-strict-output'; + +describe('strictTestExitCode', () => { + it('trusts a clean zero exit when the expected file count ran', () => { + const summary = { failedTests: 0, unhandledBetweenTests: 0, terminalFileCounts: [1] }; + expect(strictTestExitCode(0, summary, 1)).toBe(0); + }); + + it('refuses a zero exit when fewer files ran than expected (invisible non-execution)', () => { + const summary = { failedTests: 0, unhandledBetweenTests: 0, terminalFileCounts: [1] }; + expect(strictTestExitCode(0, summary, 2)).toBe(1); + }); + + it('refuses a zero exit when failure lines were printed', () => { + const summary = { failedTests: 1, unhandledBetweenTests: 0, terminalFileCounts: [1] }; + expect(strictTestExitCode(0, summary, 1)).toBe(1); + }); + + it('refuses a zero exit on an unhandled error between tests', () => { + const summary = { failedTests: 0, unhandledBetweenTests: 1, terminalFileCounts: [1] }; + expect(strictTestExitCode(0, summary, 1)).toBe(1); + }); + + it('propagates a non-zero child exit regardless of expectedFiles', () => { + const summary = { failedTests: 0, unhandledBetweenTests: 0, terminalFileCounts: [1] }; + expect(strictTestExitCode(1, summary, 1)).toBe(1); + }); +}); + +describe('BunTestOutputClassifier', () => { + it('counts a (fail) line split across write chunks', () => { + const c = new BunTestOutputClassifier(); + c.write('(fail) my te'); + c.write('st [3.42ms]\nRan 4 tests across 1 files. [2.10s]\n'); + const summary = c.end(); + expect(summary.failedTests).toBe(1); + expect(summary.terminalFileCounts).toEqual([1]); + // exit 0 + a printed failure must not be trusted + expect(strictTestExitCode(0, summary, 1)).toBe(1); + }); + + it('records the terminal file count from the summary line', () => { + const c = new BunTestOutputClassifier(); + c.write('Ran 0 tests across 1 files. [0.01s]\n'); + const summary = c.end(); + expect(summary.terminalFileCounts).toEqual([1]); + // a fully diff-skipped single-file shard (0 tests, 1 file loaded) still + // passes: 1 file ran, which is what was expected + expect(strictTestExitCode(0, summary, 1)).toBe(0); + }); +});