test(#2126): resolve review findings — fold dedup, harness fidelity, malformed-parity lock

Adversarial review of the Phase 3 branch surfaced three verified defects; fix
all three in place (no defer):

- install-runtime-artifacts.test.cjs: finish the fold-triplication dedup started
  earlier (only enh-1511 had been collapsed). 11 B1-batch __foldDescribe blocks
  were byte-identical triplicates (~5.9k lines, ~49% of the file), tripling the
  subprocess-spawning installer suites under --test-concurrency — the same
  starvation that produced the temp-dir races this branch fixes. Byte-identity
  verified per block before removal; 230 distinct test/it titles preserved
  (origin/next: 230 -> 230), interleaved B3/B5/B6 singletons untouched.
- config-get-default.test.cjs: make runExpectError faithful to production. The
  throwing process.exit seam was caught by cmdConfigGet's "No config.json"
  guard and reclassified into a spurious 2nd error() with the wrong reason
  (CONFIG_PARSE_FAILED). Drive io.setJsonErrorMode + carry the original message
  on the sentinel so the guard re-throws (single fire), assert exitCount===1,
  and strengthen both probes to assert the typed reason (CONFIG_NO_FILE /
  CONFIG_KEY_NOT_FOUND).
- roadmap.test.cjs: lock the #2121/#2114 malformed_roadmap parity — a
  project-code-prefixed query against a checklist-only roadmap now surfaces the
  same diagnostic a bare query always did (fails on prior silent-empty behavior).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-10 00:53:00 -04:00
parent 6e7e3111fb
commit 16b61d437f
3 changed files with 85 additions and 5931 deletions

View File

@@ -28,6 +28,10 @@ const { cleanup } = require('./helpers.cjs');
// calling cmdConfigGet directly removes the subprocess-spawn cost and the
// wall-clock race entirely: no timeout of any size can flake this.
const config = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'config.cjs'));
// io.cjs owns error()/output() and the JSON-error-mode toggle. cmdConfigGet's `error`
// is bound to io.error at load, so we drive io directly to (a) get structured stderr
// we can assert a typed `reason` on, and (b) restore the mode after each error probe.
const io = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'io.cjs'));
/**
* cmdConfigGet's error() path (gsd-core/bin/lib/io.cjs) calls process.exit(1)
@@ -35,10 +39,18 @@ const config = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'con
* entrypoint's non-error paths). Intercepting process.exit with a throwable
* sentinel lets the error path be exercised in-process without killing the
* test worker.
*
* The sentinel carries the ORIGINAL error message (not a generic "process.exit(1)").
* That matters for cmdConfigGet's "no config.json" branch, whose `error()` sits inside
* a try/catch that reclassifies any throw NOT starting with "No config.json" as a parse
* failure (a guard that is dead in production, where process.exit terminates first, but
* becomes live once process.exit is a throwing seam). Carrying the real message makes
* that guard re-throw — modeling the single, faithful production termination instead of
* a spurious second error() call with the wrong reason.
*/
class _ExitSignal extends Error {
constructor(code) {
super(`process.exit(${code})`);
constructor(code, message) {
super(message ?? `process.exit(${code})`);
this.code = code;
}
}
@@ -120,30 +132,53 @@ describe('config-get --default flag (#1893)', () => {
function runExpectError(...args) {
const { keyPath, raw, defaultValue } = parseConfigGetArgs(args);
const origExit = process.exit;
const origWriteSync = fs.writeSync;
io.setJsonErrorMode(true); // structured stderr line lets the sentinel carry the message + assert reason
let exitCount = 0;
let exitCode;
process.exit = (code) => {
exitCode = code;
throw new _ExitSignal(code);
let stderr = '';
fs.writeSync = (fd, ...rest) => {
if (fd !== 2) return origWriteSync.call(fs, fd, ...rest);
const [data, offset = 0, length] = rest;
const chunk = Buffer.isBuffer(data)
? data.subarray(offset, offset + (length ?? data.length - offset)).toString('utf8')
: String(data);
stderr += chunk;
return Buffer.byteLength(chunk);
};
const lastError = () => {
const parts = stderr.split('\n').filter(Boolean);
try { return JSON.parse(parts[parts.length - 1]); } catch { return {}; }
};
process.exit = (code) => {
exitCount++;
exitCode = code;
// Carry the just-emitted error message so cmdConfigGet's seam guard re-throws
// (single, faithful fire) instead of catching + reclassifying into a 2nd error().
throw new _ExitSignal(code, lastError().message);
};
let stderr;
try {
stderr = captureFdWrite(2, () => {
try {
config.cmdConfigGet(tmpDir, keyPath, raw, defaultValue);
} catch (e) {
if (!(e instanceof _ExitSignal)) throw e;
}
});
config.cmdConfigGet(tmpDir, keyPath, raw, defaultValue);
} catch (e) {
if (!(e instanceof _ExitSignal)) throw e;
} finally {
process.exit = origExit;
fs.writeSync = origWriteSync;
io.setJsonErrorMode(false);
}
assert.ok(exitCode !== 0 && exitCode !== undefined, 'Expected non-zero exit code');
return { status: exitCode, stderr };
// Faithfulness guard: production process.exit terminates, so error() fires exactly
// once. A count of 2 means the throwing-exit seam was caught + reclassified (the bug
// this harness redesign fixes) — fail loudly rather than report a wrong reason.
assert.equal(exitCount, 1, 'error() must fire exactly once (production process.exit terminates)');
const payload = lastError();
return { status: exitCode, reason: payload.reason, message: payload.message, stderr };
}
test('absent key without --default errors', () => {
fs.writeFileSync(path.join(planningDir, 'config.json'), '{}');
runExpectError('config-get', 'nonexistent.key', '--raw');
const { reason } = runExpectError('config-get', 'nonexistent.key', '--raw');
assert.equal(reason, io.ERROR_REASON.CONFIG_KEY_NOT_FOUND, 'absent key must report CONFIG_KEY_NOT_FOUND');
});
test('absent key with --default returns default value', () => {
@@ -182,7 +217,8 @@ describe('config-get --default flag (#1893)', () => {
test('missing config.json without --default errors', () => {
// No config.json written
runExpectError('config-get', 'any.key', '--raw');
const { reason } = runExpectError('config-get', 'any.key', '--raw');
assert.equal(reason, io.ERROR_REASON.CONFIG_NO_FILE, 'missing config.json must report CONFIG_NO_FILE');
});
test('--default works with JSON output (no --raw)', () => {

File diff suppressed because it is too large Load Diff

View File

@@ -2121,6 +2121,39 @@ describe('bug #2114: roadmap get-phase resolves drifted prefixed headings by bar
assert.strictEqual(payload30.found, true);
assert.strictEqual(payload30.phase_name, 'Plain');
});
test('prefixed query surfaces malformed_roadmap when only a checklist entry exists (parity with bare)', () => {
// #2121/#2114 route all three resolvers through the shared 3-source lookup, so a
// project-code-prefixed query now surfaces the SAME `malformed_roadmap` diagnostic a
// bare numeric query always did: a `**Phase PROJ-42:**` summary line with no matching
// `### Phase PROJ-42:` detail heading is malformed for BOTH query forms. Before the
// consolidation the prefixed form silently returned `{found:false}` with no diagnostic
// (the exact-prefix pass discarded its malformed candidate) — this test fails on that
// prior behavior and locks the unified, more-informative result.
fs.writeFileSync(
path.join(tmpDir, '.planning', 'ROADMAP.md'),
[
'# Roadmap v1.0',
'',
'## Phases',
'',
'- [ ] **Phase PROJ-42: Checklist only, no header**',
'',
].join('\n'),
);
const prefixed = runGsdTools('roadmap get-phase PROJ-42 --json', tmpDir);
assert.ok(prefixed.success, `command failed: ${prefixed.error || prefixed.output}`);
const pPayload = JSON.parse(prefixed.output);
assert.strictEqual(pPayload.found, false, 'malformed roadmap: phase must not be found');
assert.strictEqual(pPayload.error, 'malformed_roadmap', 'prefixed query must surface malformed_roadmap');
assert.ok(pPayload.message.includes('missing'), 'message must explain the missing detail section');
// Parity: the bare numeric form yields the same diagnostic against the same fixture.
const bare = runGsdTools('roadmap get-phase 42 --json', tmpDir);
assert.ok(bare.success, `command failed: ${bare.error || bare.output}`);
assert.strictEqual(JSON.parse(bare.output).error, 'malformed_roadmap', 'bare query surfaces the same diagnostic');
});
});
describe('roadmap annotate-dependencies', () => {