From 5e997de5f0f8afed5db2ad009f264df343f29656 Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 25 Aug 2026 08:34:51 -0400 Subject: [PATCH] fix(#3841): honor the payload anchor in the classifier, dedup the fixture Three review findings, all fixed. The isolated security review found a SECOND divergence the design missed. The classifier parses structurally, so it accepted `packageName` at any key position; the shell's `case` is anchored at the start of stdout. For `{"note":"x","packageName":"",...}` the classifier said ok and the shell said unverified -- a fail-open disagreement, and none of the original ten parity rows caught it because every one put `packageName` first. Certifying agreement that does not hold would have been worse than shipping no parity suite. The classifier now honors the anchor on its ok arm, which is what the module already claimed to do: IDENTITY_RAW_PREFIX is documented as "ANCHORED, never a substring search". A foreign packageName stays identity_mismatch wherever it appears, so that arm is untouched. Parity rows P11-P13 added. The spec review found the test matrix marked the empty-string `packageName` row as already covered. It was not -- the `.length > 0` guard is a distinct path from "no packageName key at all", which is tested. Added, and the matrix corrected to say it was wrong. The standards review flagged ~45 lines of fixture helpers duplicated verbatim between the two preamble describes. Extracted to one `makeIdentityFixture()`. Changeset rewritten: it described only the exit-code half of the diff. Refs #3841 Co-Authored-By: Claude Opus 5 --- .changeset/sturdy-jaguars-munch.md | 2 +- src/runtime-identity.cts | 23 ++++- tests/runtime-identity.test.cjs | 131 +++++++++++++++-------------- 3 files changed, 91 insertions(+), 65 deletions(-) diff --git a/.changeset/sturdy-jaguars-munch.md b/.changeset/sturdy-jaguars-munch.md index 08edb8f8c..1cb56a89d 100644 --- a/.changeset/sturdy-jaguars-munch.md +++ b/.changeset/sturdy-jaguars-munch.md @@ -2,4 +2,4 @@ type: Fixed pr: 0 --- -**The identity classifier now agrees with the launcher preamble about a tool that proves itself and then exits non-zero** — the two surfaces implement one decision and disagreed on exactly that input, so the announced hard-fail rollout would have refused an install the current warn phase verifies. (#3841) +**The identity classifier and the launcher preamble now reach the same verdict for the same probe** — the two surfaces implement one decision and disagreed on two inputs, a tool that proves itself and then exits non-zero and a payload naming this package outside the anchored wire shape, so the announced hard-fail rollout would have refused installs the warn phase verifies and accepted ones it warns about. (#3841) diff --git a/src/runtime-identity.cts b/src/runtime-identity.cts index a6fbf20db..5abea8bb5 100644 --- a/src/runtime-identity.cts +++ b/src/runtime-identity.cts @@ -188,9 +188,26 @@ export function classifyIdentityProbe( // older check. The shell anchor is deliberately open in the middle for the // same reason, and the parity suite pins both halves of that (case P10). const version = typeof record.version === 'string' ? record.version : undefined; - return actual === expected - ? { reason: 'ok', expected, actual, version } - : { reason: 'identity_mismatch', expected, actual, version }; + if (actual !== expected) return { reason: 'identity_mismatch', expected, actual, version }; + + // ANCHOR PARITY (#3841). The shell preamble cannot parse JSON; it matches + // a `case` pattern anchored at the START of stdout, so `packageName` must + // serialize first. A structural parse alone would accept + // `{"note":"x","packageName":""}` — a shape this package never emits — + // while the shell rejected it, so the two surfaces would disagree in the + // FAIL-OPEN direction. Honoring the anchor here is also what the module + // already claims to do: IDENTITY_RAW_PREFIX is documented as "ANCHORED, + // never a substring search", and buildIdentityPayload inserts packageName + // first precisely so a genuine payload always satisfies it. + if (!probe.stdout.startsWith(IDENTITY_RAW_PREFIX)) { + return { + reason: 'unparseable', + expected, + detail: 'payload names this package but is not in the anchored wire shape', + }; + } + + return { reason: 'ok', expected, actual, version }; } } diff --git a/tests/runtime-identity.test.cjs b/tests/runtime-identity.test.cjs index a9e1ad91b..d1f56452b 100644 --- a/tests/runtime-identity.test.cjs +++ b/tests/runtime-identity.test.cjs @@ -74,6 +74,13 @@ describe('classifyIdentityProbe', () => { assert.equal(v.reason, 'unparseable'); }); + test('an empty-string packageName is unparseable, not a match', () => { + // Distinct path from "no packageName key at all": the key is present and is + // a string, so only the `.length > 0` guard rejects it. + const v = classifyIdentityProbe({ stdout: '{"packageName":"","version":"1.0.0"}', exitCode: 0 }); + assert.equal(v.reason, 'unparseable'); + }); + // JSON.parse accepts all of these. A naive truthiness check would let `[]` // through as a verified identity. for (const [label, raw] of [ @@ -171,6 +178,24 @@ describe('classifyIdentityProbe', () => { assert.equal(classifyIdentityProbe({ stdout, exitCode: 0 }).reason, 'identity_mismatch'); }); + // ── Anchor parity on the classifier itself (#3841) ────────────────────── + test('a payload that names this package but is not anchored does not verify', () => { + const stdout = `{"note":"x","packageName":"${EXPECTED_PACKAGE_NAME}","version":"1.0.0"}`; + const v = classifyIdentityProbe({ stdout, exitCode: 0 }); + assert.equal(v.reason, 'unparseable'); + }); + + test('leading whitespace breaks the anchor', () => { + const v = classifyIdentityProbe({ stdout: ` ${okStdout()}`, exitCode: 0 }); + assert.equal(v.reason, 'unparseable'); + }); + + test('a foreign packageName is a mismatch wherever it appears in the object', () => { + const stdout = '{"note":"x","packageName":"get-shit-done-cc"}'; + const v = classifyIdentityProbe({ stdout, exitCode: 0 }); + assert.equal(v.reason, 'identity_mismatch'); + }); + // ── Parse-before-exit-code (#3841) ────────────────────────────────────── // The shell preamble reads stdout only — its command substitution discards // the child's exit status entirely. So the classifier must reach the same @@ -470,11 +495,38 @@ describe('IDENTITY_RAW_PREFIX — the anchor the shell matches on', () => { }); }); -describe('launcher preamble: identity assertion on a path-based branch (#3841)', () => { +const skipOnWindows = (t) => { + if (process.platform !== 'win32') return false; + t.skip('POSIX shell preamble is not executed on Windows runtimes'); + return true; +}; + +/** + * Per-describe fixture for driving the REAL launcher preamble against a fake + * `gsd-tools`. Shared by the two describes below rather than retyped: they need + * the same temp tree (a `bin/` holding only a `node` symlink, plus + * `gsd-core/bin/gsd-tools.cjs`) and the same restricted-PATH invocation, and a + * second copy is a place for the two to drift apart. + * + * Returns handles rather than installing its own beforeEach/afterEach, so each + * describe keeps its own fresh state and its own lifecycle hooks. + */ +function makeIdentityFixture() { let dir; let binDir; let toolsDir; + const setup = () => { + dir = createTempDir('gsd-3841-identity-'); + binDir = path.join(dir, 'bin'); + toolsDir = path.join(dir, 'gsd-core', 'bin'); + fs.mkdirSync(binDir, { recursive: true }); + fs.mkdirSync(toolsDir, { recursive: true }); + fs.symlinkSync(process.execPath, path.join(binDir, 'node')); + }; + + const teardown = () => cleanup(dir); + // Resolution here goes through the RUNTIME_DIR-local branch — the branch // #3831 could NOT make safe structurally, and therefore the one this // assertion exists for. @@ -512,22 +564,17 @@ describe('launcher preamble: identity assertion on a path-based branch (#3841)', }); }; - const skipOnWindows = (t) => { - if (process.platform !== 'win32') return false; - t.skip('POSIX shell preamble is not executed on Windows runtimes'); - return true; - }; + return { setup, teardown, installFakeTool, sourceAndReport }; +} - beforeEach(() => { - dir = createTempDir('gsd-3841-identity-'); - binDir = path.join(dir, 'bin'); - toolsDir = path.join(dir, 'gsd-core', 'bin'); - fs.mkdirSync(binDir, { recursive: true }); - fs.mkdirSync(toolsDir, { recursive: true }); - fs.symlinkSync(process.execPath, path.join(binDir, 'node')); - }); +describe('launcher preamble: identity assertion on a path-based branch (#3841)', () => { + const fx = makeIdentityFixture(); - afterEach(() => cleanup(dir)); + beforeEach(() => fx.setup()); + afterEach(() => fx.teardown()); + + const installFakeTool = (identityBody) => fx.installFakeTool(identityBody); + const sourceAndReport = (extra = '') => fx.sourceAndReport(extra); const emit = (json) => `process.stdout.write(${JSON.stringify(json)} + '\\n'); process.exit(0);`; @@ -680,54 +727,13 @@ describe('launcher preamble: identity assertion on a path-based branch (#3841)', }); describe('shell preamble and classifier agree (cross-surface parity, #3841)', () => { - let dir; - let binDir; - let toolsDir; + const fx = makeIdentityFixture(); - const installFakeTool = (identityBody) => { - fs.writeFileSync( - path.join(toolsDir, 'gsd-tools.cjs'), - '#!/usr/bin/env node\n' + - 'const a = process.argv.slice(2);\n' + - "if (a[0] === 'runtime-identity') { " + identityBody + ' }\n' + - "process.stdout.write('RAN:' + a.join(',') + '\\n');\n", - { mode: 0o755 }, - ); - }; + beforeEach(() => fx.setup()); + afterEach(() => fx.teardown()); - const sourceAndReport = () => { - const harness = path.join(dir, 'harness.sh'); - fs.writeFileSync( - harness, - '#!/bin/sh\n' + - `. "${SNIPPET}"\n` + - "printf 'STATUS=%s\\n' \"$GSD_IDENTITY_STATUS\"\n", - { mode: 0o755 }, - ); - return runHook(harness, [], { - interpreter: '/bin/sh', - cwd: dir, - timeoutMs: PROBE_TIMEOUT_MS, - env: { PATH: binDir, HOME: dir, RUNTIME_DIR: dir }, - }); - }; - - const skipOnWindows = (t) => { - if (process.platform !== 'win32') return false; - t.skip('POSIX shell preamble is not executed on Windows runtimes'); - return true; - }; - - beforeEach(() => { - dir = createTempDir('gsd-3841-parity-'); - binDir = path.join(dir, 'bin'); - toolsDir = path.join(dir, 'gsd-core', 'bin'); - fs.mkdirSync(binDir, { recursive: true }); - fs.mkdirSync(toolsDir, { recursive: true }); - fs.symlinkSync(process.execPath, path.join(binDir, 'node')); - }); - - afterEach(() => cleanup(dir)); + const installFakeTool = (identityBody) => fx.installFakeTool(identityBody); + const sourceAndReport = () => fx.sourceAndReport(); const matching = (extra = '') => `${IDENTITY_RAW_PREFIX},"version":"1.0.0"${extra}}`; const foreign = '{"packageName":"get-shit-done-cc","version":"1.0.0"}'; @@ -748,6 +754,9 @@ describe('shell preamble and classifier agree (cross-surface parity, #3841)', () ['P8 payload followed by trailing garbage, exit 0', `${matching()}\nextra`, 0, false], ['P9 empty stdout, exit 0', '', 0, false], ['P10 additive unknown key, exit 0', matching(',"extra":true'), 0, true], + ['P11 packageName present but not first, exit 0', `{"note":"x","packageName":"${EXPECTED_PACKAGE_NAME}","version":"1.0.0"}`, 0, false], + ['P12 leading whitespace before the payload, exit 0', ` ${matching()}`, 0, false], + ['P13 foreign packageName not first, exit 0', `{"note":"x","packageName":"get-shit-done-cc"}`, 0, false], ]; for (const [label, stdout, exitCode, verified] of CASES) {