From 3b8e4f3e5ae80e2ebbba4912cfdddb147bffa737 Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 25 Aug 2026 08:16:50 -0400 Subject: [PATCH] fix(#3841): parse the identity payload before consulting the probe exit code `classifyIdentityProbe` short-circuited on a non-zero exit BEFORE it looked at stdout, so a tool that had already proved its identity and merely exited non-zero was classified `no_identity_verb`. The launcher preamble that this module speaks for reads stdout only -- its command substitution discards the status -- so the two surfaces disagreed on exactly that input: shell `ok`, classifier `no_identity_verb`. Measured against the real snippet, not inferred. That matters because the classifier is the engine for the announced hard-fail phase and has no production caller yet. An install verified by today's warn phase would have been refused the moment hard-fail landed, in the phase where that stops the run rather than printing a line. Nothing recorded or tested the difference. The classifier moves rather than the shell: the shell is the shipped path with observable dependents, the classifier has none. The predecessor defence is untouched -- a usage screen yields no usable payload, so it still falls through to the exit-code branch. Adds the cross-surface parity test the gauntlet requires for two surfaces implementing one decision: ten probe behaviors driven through both the real snippet and the classifier, asserting they agree with each other. Surfaced by re-running the feature-implementation directive's design and QA steps against the code merged in #3848, which shipped without them. Refs #3841 Co-Authored-By: Claude Opus 5 --- .changeset/sturdy-jaguars-munch.md | 5 + src/runtime-identity.cts | 87 ++++++++++++------ tests/runtime-identity.test.cjs | 143 +++++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 28 deletions(-) create mode 100644 .changeset/sturdy-jaguars-munch.md diff --git a/.changeset/sturdy-jaguars-munch.md b/.changeset/sturdy-jaguars-munch.md new file mode 100644 index 000000000..08edb8f8c --- /dev/null +++ b/.changeset/sturdy-jaguars-munch.md @@ -0,0 +1,5 @@ +--- +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) diff --git a/src/runtime-identity.cts b/src/runtime-identity.cts index ecda62a54..a6fbf20db 100644 --- a/src/runtime-identity.cts +++ b/src/runtime-identity.cts @@ -30,6 +30,11 @@ * close-enough would reproduce the exact silent success #3129 already produced. * Liberality is spent on visibility instead — distinct reason codes, each * naming what actually happened. + * + * The classifier and the shell preamble are held in agreement by a + * cross-surface parity test (#3841): both read stdout only and must reach the + * same verdict for the same payload, or the preamble's warning and this + * module's diagnostic would silently diverge on the same input. */ import { packageName } from './package-identity.cjs'; @@ -123,6 +128,24 @@ function excerpt(text: string): string { return flat.length > EVIDENCE_MAX_CHARS ? `${flat.slice(0, EVIDENCE_MAX_CHARS)}…` : flat; } +/** + * `stdout` as a plain JSON object, or `null` when it is not one. + * + * `JSON.parse` accepts `0`, `"str"`, `[]`, `null` and `true`; arrays and `null` + * are also `object` to `typeof`. All are rejected here so no downstream + * truthiness check can mistake `[]` for a verified identity. + */ +function parseIdentityRecord(stdout: string): Record | null { + let parsed: unknown; + try { + parsed = JSON.parse(stdout) as unknown; + } catch { + return null; + } + if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) return null; + return parsed as Record; +} + /** * Pure classifier: probe result -> verdict. Total — never throws, for any input. * @@ -142,9 +165,37 @@ export function classifyIdentityProbe( }; } - // A non-zero exit is what a binary without this verb does. The predecessor - // prints its usage screen and exits 1; treat that as "something else - // answered", never as a parse problem. + // PARSE BEFORE CONSULTING THE EXIT CODE (#3841). + // + // The exit-code rule below is right for its stated reason -- the predecessor + // prints a usage screen and exits 1, and that is "something else answered", + // not a parse problem -- but it used to run FIRST, so it also caught a tool + // that had already proved its identity and merely exited non-zero afterwards. + // The shell preamble this module speaks for reads stdout ONLY (its command + // substitution discards the status), so the two surfaces disagreed on exactly + // that input: shell `ok`, classifier `no_identity_verb`. Measured against the + // real snippet, not inferred. Since this classifier is the engine for the + // announced hard-fail phase, such an install would have flipped from verified + // to refused at rollout, in the phase where that stops the run. + // + // The predecessor defence is unaffected: a usage screen yields no usable + // payload, so it still falls through to the exit-code branch below. + const record = parseIdentityRecord(probe.stdout); + if (record !== null) { + const actual = record.packageName; + if (typeof actual === 'string' && actual.length > 0) { + // Unknown keys are ignored so a future payload addition cannot fail an + // 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 }; + } + } + + // Nothing was proved. NOW the exit status is the discriminator: a non-zero + // exit means a binary that does not implement this verb answered. if (probe.exitCode !== 0) { return { reason: 'no_identity_verb', @@ -153,31 +204,11 @@ export function classifyIdentityProbe( }; } - let parsed: unknown; - try { - parsed = JSON.parse(probe.stdout) as unknown; - } catch { - return { reason: 'unparseable', expected, detail: excerpt(probe.stdout) }; - } - - // Arrays are objects to typeof; null is too. Both must be rejected. - if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) { - return { reason: 'unparseable', expected, detail: excerpt(probe.stdout) }; - } - - const record = parsed as Record; - const actual = record.packageName; - if (typeof actual !== 'string' || actual.length === 0) { - return { reason: 'unparseable', expected, detail: 'payload has no usable packageName' }; - } - - const version = typeof record.version === 'string' ? record.version : undefined; - if (actual !== expected) { - return { reason: 'identity_mismatch', expected, actual, version }; - } - - // Unknown keys are ignored so a future payload addition cannot fail an older check. - return { reason: 'ok', expected, actual, version }; + return { + reason: 'unparseable', + expected, + detail: record === null ? excerpt(probe.stdout) : 'payload has no usable packageName', + }; } /** diff --git a/tests/runtime-identity.test.cjs b/tests/runtime-identity.test.cjs index 1fcac1731..a9e1ad91b 100644 --- a/tests/runtime-identity.test.cjs +++ b/tests/runtime-identity.test.cjs @@ -170,6 +170,59 @@ describe('classifyIdentityProbe', () => { const stdout = JSON.stringify({ packageName: 'get-shit-done-cc', note: EXPECTED_PACKAGE_NAME }); assert.equal(classifyIdentityProbe({ stdout, exitCode: 0 }).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 + // verdict a tool that already proved its identity and then exited non-zero + // for an unrelated reason (a later verb failing, a warning exit, etc.). + + test('a valid payload verifies even when the probe exits non-zero', () => { + const v = classifyIdentityProbe({ stdout: okStdout(), exitCode: 1 }); + assert.equal(v.reason, 'ok'); + assert.equal(v.actual, EXPECTED_PACKAGE_NAME); + }); + + test('a non-zero exit with a foreign payload is a mismatch, not a missing verb', () => { + const v = classifyIdentityProbe({ + stdout: '{"packageName":"get-shit-done-cc","version":"1.0.0"}', + exitCode: 1, + }); + assert.equal(v.reason, 'identity_mismatch'); + assert.equal(v.actual, 'get-shit-done-cc'); + }); + + // REGRESSION PIN: these pass today and must keep passing — the predecessor + // and any answer that proves nothing still fall through to no_identity_verb. + for (const stdout of [ + 'usage: gsd-tools ...', + '[]', + '0', + 'null', + '{"version":"1"}', + ]) { + test(`a non-zero exit that proved nothing stays no_identity_verb (${JSON.stringify(stdout)})`, () => { + const v = classifyIdentityProbe({ stdout, exitCode: 1 }); + assert.equal(v.reason, 'no_identity_verb'); + }); + } + + test('a signal-killed probe that still proved identity verifies', () => { + const v = classifyIdentityProbe({ stdout: okStdout(), exitCode: null }); + assert.equal(v.reason, 'ok'); + }); + + test('an empty stdout is unparseable', () => { + const v = classifyIdentityProbe({ stdout: '', exitCode: 0 }); + assert.equal(v.reason, 'unparseable'); + }); + + test('spawn failure short-circuits ahead of the payload', () => { + const spawnFailed = classifyIdentityProbe({ stdout: okStdout(), exitCode: 0, spawnFailed: true }); + assert.equal(spawnFailed.reason, 'probe_failed'); + const timedOut = classifyIdentityProbe({ stdout: okStdout(), exitCode: 0, timedOut: true }); + assert.equal(timedOut.reason, 'probe_failed'); + }); }); describe('buildIdentityPayload', () => { @@ -625,3 +678,93 @@ describe('launcher preamble: identity assertion on a path-based branch (#3841)', assert.equal(r.stdout.includes(`CHILD=${IDENTITY_STATUS.OK}`), true, r.stdout); }); }); + +describe('shell preamble and classifier agree (cross-surface parity, #3841)', () => { + let dir; + let binDir; + let toolsDir; + + 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 }, + ); + }; + + 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 matching = (extra = '') => `${IDENTITY_RAW_PREFIX},"version":"1.0.0"${extra}}`; + const foreign = '{"packageName":"get-shit-done-cc","version":"1.0.0"}'; + + const CASES = [ + ['P1 payload matches, exit 0', matching(), 0, true], + ['P2 payload matches, exit 1', matching(), 1, true], + ['P3 foreign payload, exit 0', foreign, 0, false], + ['P4 foreign payload, exit 1', foreign, 1, false], + ['P5 predecessor usage text, exit 1', 'usage: gsd-tools ', 1, false], + [ + 'P6 decoy embeds expected name in another field, exit 0', + JSON.stringify({ packageName: 'get-shit-done-cc', note: EXPECTED_PACKAGE_NAME }), + 0, + false, + ], + ['P7 truncated payload (no closing brace), exit 0', matching().slice(0, -1), 0, false], + ['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], + ]; + + for (const [label, stdout, exitCode, verified] of CASES) { + test(label, (t) => { + if (skipOnWindows(t)) return; + + installFakeTool(`process.stdout.write(${JSON.stringify(stdout)} + '\\n'); process.exit(${exitCode});`); + const r = sourceAndReport(); + const shellStatus = r.stdout.split('\n').find((l) => l.startsWith('STATUS='))?.slice('STATUS='.length); + + const verdict = classifyIdentityProbe({ stdout: `${stdout}\n`, exitCode }); + const jsStatus = statusForVerdict(verdict); + + const expectedStatus = verified ? IDENTITY_STATUS.OK : IDENTITY_STATUS.UNVERIFIED; + assert.equal(jsStatus, shellStatus, `classifier/shell disagree for: ${label}`); + assert.equal(jsStatus, expectedStatus, `classifier status wrong for: ${label}`); + assert.equal(shellStatus, expectedStatus, `shell status wrong for: ${label}`); + }); + } +});