fix: pre-landing review fixes for the v2 port wave

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 <noreply@anthropic.com>
This commit is contained in:
Garry Tan 2026-08-12 16:02:11 -07:00
parent ea7ba921ce
commit ccb91c3afb
No known key found for this signature in database
GPG Key ID: C1F69E85C74EFE1D
9 changed files with 156 additions and 27 deletions

View File

@ -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

View File

@ -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`);

View File

@ -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);
}

View File

@ -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');
}

View File

@ -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",

View File

@ -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<number> {
}
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 },

View File

@ -34,7 +34,11 @@ function run(argv: string[], opts: { env?: Record<string, string>; 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 {}
}
});
// ---------------------------------------------------------------

View File

@ -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. */

View File

@ -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);
});
});