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 <noreply@anthropic.com>
This commit is contained in:
5
.changeset/sturdy-jaguars-munch.md
Normal file
5
.changeset/sturdy-jaguars-munch.md
Normal file
@@ -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)
|
||||
@@ -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<string, unknown> | 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<string, unknown>;
|
||||
}
|
||||
|
||||
/**
|
||||
* 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<string, unknown>;
|
||||
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',
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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 <command>', 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}`);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user