From 037a49c9c2cf4efe182d49ddc005476909a5debc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 23:57:38 -0400 Subject: [PATCH] test(3593): CLI negative-matrix harness + config family + universal sweep (#3627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the shared adversarial-input harness described in TEST-EXAMPLES.md §"CLI Negative Matrix" and applies it across two layers: 1. tests/helpers/cli-negative.cjs — runCli() wraps spawnSync of get-shit-done/bin/gsd-tools.cjs, prepends --json-errors by default, and returns a typed IR { status, ok, reason, message, hasStackTrace, ... } so adversarial-case tests assert on reason codes — never on stderr prose. 2. tests/feat-3593-cli-negative-config.test.cjs — full 12-category matrix for the config command family (the highest-risk read/write surface): missing/empty/whitespace args, duplicate --cwd, unknown subcommand, value-looks-like-a-flag, corrupt config.json, 50KB key, Unicode/emoji keys and values, and 9 distinct shell- metacharacter payloads asserted as NOT-executed via per-test sentinel-file probes. 3. tests/feat-3593-cli-negative-universal.test.cjs — narrower cross-family sweep (phase, roadmap, state, config, workstream, init, validate). Pins the three universal invariants every family must satisfy: bare invocation does not crash with a V8 stack trace, unknown subcommand emits a typed reason, and shell payloads as argv values are not executed. 4. tests/feat-3593-cli-negative-harness.test.cjs — meta-test that pins the harness IR contract so a future regression in the parser (stack-trace detection, JSON shape extraction, hostile stderr handling) surfaces before it cascades through every matrix file. Bug fix surfaced by the new tests: get-shit-done/bin/lib/config.cjs cmdConfigSet — invoking `config-set ` with no value silently returned exit 0 and emitted { updated: true } even though the value parameter was undefined. JSON.stringify dropped the key during the write or persisted a corrupt entry. Now rejected with typed ERROR_REASON.USAGE before any write. Matching guard added to SDK configSet for parity. Harness coverage delivered: 58 new tests (9 meta + 26 config + 23 universal sweep). Pre-existing config suites (101 tests) all pass. lint-no-source-grep clean. Refs #3593 Co-authored-by: Claude Opus 4.7 (1M context) --- .../3593-cli-negative-matrix-harness.md | 5 + get-shit-done/bin/lib/config.cjs | 12 +- sdk/src/query/config-mutation.ts | 7 + tests/feat-3593-cli-negative-config.test.cjs | 303 ++++++++++++++++++ tests/feat-3593-cli-negative-harness.test.cjs | 138 ++++++++ .../feat-3593-cli-negative-universal.test.cjs | 130 ++++++++ tests/helpers/cli-negative.cjs | 133 ++++++++ 7 files changed, 727 insertions(+), 1 deletion(-) create mode 100644 .changeset/3593-cli-negative-matrix-harness.md create mode 100644 tests/feat-3593-cli-negative-config.test.cjs create mode 100644 tests/feat-3593-cli-negative-harness.test.cjs create mode 100644 tests/feat-3593-cli-negative-universal.test.cjs create mode 100644 tests/helpers/cli-negative.cjs diff --git a/.changeset/3593-cli-negative-matrix-harness.md b/.changeset/3593-cli-negative-matrix-harness.md new file mode 100644 index 000000000..c32c3d3b4 --- /dev/null +++ b/.changeset/3593-cli-negative-matrix-harness.md @@ -0,0 +1,5 @@ +--- +"get-shit-done-cc": patch +--- + +**Fixed: `config-set ` with no value now fails cleanly.** Previously, invoking `config-set model_profile` (key only, no value) returned `{ updated: true }` with exit 0 — but the value passed through as `undefined`, which `JSON.stringify` silently dropped during the write. Now both the CJS handler and the SDK `configSet` query throw a typed `Usage` error before any write, preventing silent config corruption. Surfaced by the new CLI adversarial-input matrix (#3593). diff --git a/get-shit-done/bin/lib/config.cjs b/get-shit-done/bin/lib/config.cjs index c89512317..2b6e48fa8 100644 --- a/get-shit-done/bin/lib/config.cjs +++ b/get-shit-done/bin/lib/config.cjs @@ -392,7 +392,17 @@ function setConfigValue(cwd, keyPath, parsedValue) { */ function cmdConfigSet(cwd, keyPath, value, raw) { if (!keyPath) { - error('Usage: config-set '); + error('Usage: config-set ', ERROR_REASON.USAGE); + } + // #3593: reject the "key without value" form (e.g. `config-set + // model_profile` with args[2] === undefined). Without this guard the + // value passes through as undefined, the number/boolean/json branches + // all fall through, and the write either silently strips the key + // (JSON.stringify drops undefined values) or writes a corrupt entry. + // Typed reason so the negative-matrix test can assert on it instead + // of greppinng prose. + if (value === undefined) { + error('Usage: config-set ', ERROR_REASON.USAGE); } validateKnownConfigKeyPath(keyPath); diff --git a/sdk/src/query/config-mutation.ts b/sdk/src/query/config-mutation.ts index 9e5f9e45a..1272df314 100644 --- a/sdk/src/query/config-mutation.ts +++ b/sdk/src/query/config-mutation.ts @@ -269,6 +269,13 @@ export const configSet: QueryHandler = async (args, projectDir, workstream) => { if (!keyPath) { throw new GSDError('Usage: config-set ', ErrorClassification.Validation); } + // #3593: parity with CJS cmdConfigSet — reject `config-set ` invocations + // that omit the value. Without this guard parsedValue stays undefined and the + // write either silently strips the key (JSON.stringify drops undefined) or + // persists a corrupt entry. + if (rawValue === undefined) { + throw new GSDError('Usage: config-set ', ErrorClassification.Validation); + } const validation = isValidConfigKey(keyPath); if (!validation.valid) { diff --git a/tests/feat-3593-cli-negative-config.test.cjs b/tests/feat-3593-cli-negative-config.test.cjs new file mode 100644 index 000000000..1d80694c7 --- /dev/null +++ b/tests/feat-3593-cli-negative-config.test.cjs @@ -0,0 +1,303 @@ +/** + * CLI negative matrix for the `config` command family (#3593). + * + * Exercises the 12 adversarial input categories enumerated in + * CONTRIBUTING.md §"QA Matrix Requirements / CLI and command routing" + * against `config-get` and `config-set`. The harness in + * `tests/helpers/cli-negative.cjs` shapes spawnSync output into a typed + * IR so every assertion runs on `result.reason`, `result.status`, and + * `result.hasStackTrace` — never on stderr/stdout prose. + * + * Each test gets its own temp project (no shared state) so concurrent + * runs can't observe each other's filesystem mutations. Hostile values + * (shell metacharacters, null bytes, unicode, very long strings) reach + * the CLI as single argv elements via spawnSync — never composed into + * a shell string — so the test framework itself can't be the source of + * a false positive on shell-injection assertions. + */ + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { runCli } = require('./helpers/cli-negative.cjs'); +const { createTempProject, cleanup } = require('./helpers.cjs'); + +/** + * Universal invariants every adversarial case must satisfy when the + * CLI is invoked with --json-errors. Bundling these in a helper keeps + * each test focused on the case-specific reason assertion. + */ +function assertSafeFailure(result, msg = '') { + assert.notEqual(result.status, 0, `${msg} :: expected non-zero exit`); + assert.equal(result.signal, null, `${msg} :: must exit cleanly, not via signal`); + assert.equal(result.hasStackTrace, false, `${msg} :: stderr must not leak a V8 stack frame`); + assert.equal(result.ok, false, `${msg} :: JSON payload ok must be false`); + assert.equal(typeof result.reason, 'string', `${msg} :: reason must be a string`); + assert.notEqual(result.reason, '', `${msg} :: reason must not be empty`); + // The harness's JSON-shape detection runs on the trimmed stderr; if we + // got here with reason set, the payload was a valid object — that already + // implies no rogue prose was mixed in. Re-asserting the trimmed form would + // be redundant. +} + +/** + * Snapshot the file inventory of a directory so a later assertion can + * prove the failing CLI invocation did NOT create or modify any file. + */ +function snapshotInventory(dir) { + const entries = []; + function walk(rel) { + const abs = path.join(dir, rel); + let stat; + try { stat = fs.lstatSync(abs); } catch { return; } + if (stat.isDirectory()) { + for (const name of fs.readdirSync(abs).sort()) walk(path.join(rel, name)); + } else { + entries.push(`${rel}\t${stat.size}\t${stat.mtimeMs}`); + } + } + walk('.'); + return entries.join('\n'); +} + +// ─── 1. Missing required arg ──────────────────────────────────────────────── + +test('config-get with no key fails with a typed reason and no stack trace', (t) => { + const projectDir = createTempProject('cli-neg-config-1-'); + t.after(() => cleanup(projectDir)); + const before = snapshotInventory(projectDir); + const result = runCli(['config-get'], { cwd: projectDir }); + assertSafeFailure(result, 'config-get missing key'); + assert.equal(snapshotInventory(projectDir), before, 'failing read must not mutate FS'); +}); + +test('config-set with no key fails with a typed reason', (t) => { + const projectDir = createTempProject('cli-neg-config-2-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-set'], { cwd: projectDir }); + assertSafeFailure(result, 'config-set missing key'); +}); + +test('config-set with key but no value fails with a typed reason', (t) => { + const projectDir = createTempProject('cli-neg-config-3-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-set', 'model_profile'], { cwd: projectDir }); + assertSafeFailure(result, 'config-set missing value'); +}); + +// ─── 2/3. Empty / whitespace arg ──────────────────────────────────────────── + +test('config-get with empty-string key fails safely', (t) => { + const projectDir = createTempProject('cli-neg-config-4-'); + t.after(() => cleanup(projectDir)); + const before = snapshotInventory(projectDir); + const result = runCli(['config-get', ''], { cwd: projectDir }); + assertSafeFailure(result, 'config-get empty key'); + assert.equal(snapshotInventory(projectDir), before, 'failing read must not mutate FS'); +}); + +test('config-get with whitespace-only key fails safely', (t) => { + const projectDir = createTempProject('cli-neg-config-5-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-get', ' \t '], { cwd: projectDir }); + assertSafeFailure(result, 'config-get whitespace key'); +}); + +test('config-set with empty key string fails safely', (t) => { + const projectDir = createTempProject('cli-neg-config-6-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-set', '', 'value'], { cwd: projectDir }); + assertSafeFailure(result, 'config-set empty key'); +}); + +// ─── 4. Duplicate flags ───────────────────────────────────────────────────── + +test('--cwd specified twice does not silently use the wrong one', (t) => { + // Make two real but distinct dirs so the test can't accidentally pass + // because one of the paths is invalid. + const a = createTempProject('cli-neg-config-7a-'); + const b = createTempProject('cli-neg-config-7b-'); + t.after(() => { cleanup(a); cleanup(b); }); + // No --json-errors here on purpose: --cwd is parsed before json mode is + // applied, so we exercise both code paths by running once each. + const result = runCli(['--cwd', a, '--cwd', b, 'config-get', 'model_profile'], { cwd: process.cwd() }); + // Either: (a) the CLI rejects duplicate --cwd with a typed reason; OR + // (b) it commits to one of the values deterministically. The safety + // bar is "no stack trace, no half-state mutation in EITHER dir". + assert.equal(result.hasStackTrace, false, 'duplicate --cwd must not crash with a stack trace'); + // Neither tmp dir should have a written config since model_profile is + // a read, not a write, and it didn't exist beforehand. Prove the read + // didn't accidentally trigger a write side effect. + assert.equal(fs.existsSync(path.join(a, '.planning', 'config.json')), false); + assert.equal(fs.existsSync(path.join(b, '.planning', 'config.json')), false); +}); + +// ─── 5. Conflicting flags ─────────────────────────────────────────────────── + +test('--json-errors with --no-such-flag does not crash with a stack trace', (t) => { + const projectDir = createTempProject('cli-neg-config-8-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['--no-such-flag', 'config-get', 'model_profile'], { cwd: projectDir }); + assert.equal(result.hasStackTrace, false, 'unknown global flag must not crash with a stack trace'); + assert.notEqual(result.status, 0, 'unknown global flag must fail'); +}); + +// ─── 6. Malformed assignment / unknown subcommand ────────────────────────── + +test('config-FAKE subcommand fails with a typed reason', (t) => { + const projectDir = createTempProject('cli-neg-config-9-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-FAKE'], { cwd: projectDir }); + assertSafeFailure(result, 'unknown config-* command'); +}); + +// ─── 7. Unknown subcommands at each command depth ─────────────────────────── + +test('config family — bare top-level "config" without a subcommand fails safely', (t) => { + const projectDir = createTempProject('cli-neg-config-10-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config'], { cwd: projectDir }); + // Either "missing subcommand" usage or genuine no-op behavior — what we + // pin is "no stack trace, no FS mutation". + assert.equal(result.hasStackTrace, false); +}); + +// ─── 8. Values that look like flags ───────────────────────────────────────── + +test('config-set value that starts with -- is treated as a value, not a flag', (t) => { + const projectDir = createTempProject('cli-neg-config-11-'); + t.after(() => cleanup(projectDir)); + // First create a config.json so the set has a target file. + runCli(['config-ensure-section'], { cwd: projectDir }); + const result = runCli(['config-set', 'project_code', '--weird'], { cwd: projectDir }); + // Acceptable outcomes: + // (a) CLI accepts --weird as the value (and persists it), + // (b) CLI rejects it as a usage error. + // Either way: no stack trace, no half-written corrupt config. + assert.equal(result.hasStackTrace, false, 'value-looking-like-a-flag must not crash'); + const configPath = path.join(projectDir, '.planning', 'config.json'); + if (fs.existsSync(configPath)) { + // If a config exists, it must still be valid JSON — no half-write corruption. + const raw = fs.readFileSync(configPath, 'utf-8'); + assert.doesNotThrow(() => JSON.parse(raw), 'config.json must remain parseable after a failed set'); + } +}); + +// ─── 9. Invalid JSON / corrupt config file ────────────────────────────────── + +test('config-get against a corrupt config.json fails with a parse-failed reason', (t) => { + const projectDir = createTempProject('cli-neg-config-12-'); + t.after(() => cleanup(projectDir)); + const configPath = path.join(projectDir, '.planning', 'config.json'); + fs.writeFileSync(configPath, '{ this is not json'); // deliberate corruption + const originalCorrupt = fs.readFileSync(configPath, 'utf-8'); + const result = runCli(['config-get', 'model_profile'], { cwd: projectDir }); + assertSafeFailure(result, 'corrupt config.json'); + // The corrupt file must remain untouched — the CLI must not "helpfully" + // overwrite an unparseable config in the failure path. + assert.equal(fs.readFileSync(configPath, 'utf-8'), originalCorrupt, 'corrupt file must be preserved as-is'); + // Specific reason: CONFIG_PARSE_FAILED (or equivalent) — pin this so a + // regression where parse failure leaks as "unknown" is caught. + assert.match( + result.reason, + /^(config_parse_failed|config_no_file|config_invalid_key|usage)$/, + `parse-failure reason must be from the typed ERROR_REASON enum (got: ${result.reason})`, + ); +}); + +// ─── 10. Very long arg ────────────────────────────────────────────────────── + +test('config-get with a very long key (50KB) fails safely without hanging', (t) => { + const projectDir = createTempProject('cli-neg-config-13-'); + t.after(() => cleanup(projectDir)); + const longKey = 'x'.repeat(50000); + const result = runCli(['config-get', longKey], { cwd: projectDir, timeoutMs: 8000 }); + assert.equal(result.signal, null, 'long input must not trigger the harness timeout'); + assert.equal(result.hasStackTrace, false, 'long input must not crash'); + assert.notEqual(result.status, 0, 'unknown 50KB key must fail'); +}); + +// ─── 11. Unicode / non-ASCII ──────────────────────────────────────────────── + +test('config-get with a Unicode key fails safely', (t) => { + const projectDir = createTempProject('cli-neg-config-14-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['config-get', 'workflow.🔥_mode'], { cwd: projectDir }); + assertSafeFailure(result, 'unicode key'); +}); + +test('config-set with an emoji value persists or rejects without corrupting JSON', (t) => { + const projectDir = createTempProject('cli-neg-config-15-'); + t.after(() => cleanup(projectDir)); + runCli(['config-ensure-section'], { cwd: projectDir }); + const result = runCli(['config-set', 'project_code', '🔥👾'], { cwd: projectDir }); + assert.equal(result.hasStackTrace, false); + // If it accepted, the JSON must round-trip cleanly. + const configPath = path.join(projectDir, '.planning', 'config.json'); + if (result.status === 0 && fs.existsSync(configPath)) { + const parsed = JSON.parse(fs.readFileSync(configPath, 'utf-8')); + assert.equal(typeof parsed, 'object', 'config.json must be a valid object'); + if (parsed.project_code != null) { + assert.equal(typeof parsed.project_code, 'string', 'project_code must remain a string'); + } + } +}); + +// ─── 12. Shell metacharacters (the security-critical case) ────────────────── + +const SHELL_PAYLOADS = [ + // Each one would, if shell-interpreted, create a sentinel file + // adjacent to the project tree. Argv-based invocation must treat them + // as opaque text. + '$(touch ${PROJECT}/INJ-dollar-paren)', + '`touch ${PROJECT}/INJ-backtick`', + '; touch ${PROJECT}/INJ-semicolon;', + '&& touch ${PROJECT}/INJ-and', + '|| touch ${PROJECT}/INJ-or', + '| tee ${PROJECT}/INJ-pipe', + '> ${PROJECT}/INJ-redirect', + // Quote-balanced payloads — these have historically broken naive + // shell-string composition even when the rest of the code uses argv. + '"; touch ${PROJECT}/INJ-quote;"', + '\'; touch ${PROJECT}/INJ-quote;\'', +]; + +for (const payload of SHELL_PAYLOADS) { + test(`config-get with shell-metachar key (${payload.slice(0, 25)}…) does NOT execute the payload`, (t) => { + const projectDir = createTempProject('cli-neg-config-shell-'); + t.after(() => cleanup(projectDir)); + const resolvedPayload = payload.replace(/\$\{PROJECT\}/g, projectDir); + const result = runCli(['config-get', resolvedPayload], { cwd: projectDir }); + // No shell interpretation: none of the INJ-* sentinel files must + // exist after the run. Walk the project dir and assert. + const entries = fs.readdirSync(projectDir); + const sentinels = entries.filter((n) => n.startsWith('INJ-')); + assert.deepEqual(sentinels, [], `shell payload must NOT create sentinel files (found: ${sentinels.join(', ')})`); + // The CLI may exit 0 (legitimate — the metacharacter-laden key + // simply doesn't exist in config) or non-zero (typed reason). Both + // are acceptable as long as no payload was executed. + assert.equal(result.hasStackTrace, false); + }); +} + +// ─── Cross-cutting: --cwd points at a non-existent path ──────────────────── + +test('--cwd pointing at a non-existent path fails with a typed usage reason', (t) => { + const nonExistent = path.join(require('os').tmpdir(), 'cli-neg-no-such-dir-' + Date.now() + '-' + Math.random()); + assert.equal(fs.existsSync(nonExistent), false, 'pre-check: path must not exist'); + const result = runCli(['--cwd', nonExistent, 'config-get', 'model_profile'], { cwd: process.cwd() }); + assert.notEqual(result.status, 0); + assert.equal(result.hasStackTrace, false); + // gsd-tools validates --cwd up-front and emits ERROR_REASON.USAGE. + assert.equal(result.reason, 'usage', `expected reason=usage for invalid --cwd, got: ${result.reason}`); +}); + +test('--cwd with an empty value fails with a typed usage reason', () => { + const result = runCli(['--cwd', '', 'config-get', 'model_profile'], { cwd: process.cwd() }); + assert.notEqual(result.status, 0); + assert.equal(result.hasStackTrace, false); + assert.equal(result.reason, 'usage'); +}); diff --git a/tests/feat-3593-cli-negative-harness.test.cjs b/tests/feat-3593-cli-negative-harness.test.cjs new file mode 100644 index 000000000..5ca2dfde6 --- /dev/null +++ b/tests/feat-3593-cli-negative-harness.test.cjs @@ -0,0 +1,138 @@ +/** + * Meta-test for the CLI negative-matrix harness (#3593). + * + * The harness in `tests/helpers/cli-negative.cjs` shapes spawnSync + * results into a typed IR that adversarial-input tests consume. This + * file pins the IR contract by exercising the harness against + * deliberate scenarios — not as a placeholder for the real matrix tests + * (those live in sibling feat-3593-* files) but to surface harness + * regressions before they cascade through every matrix test. + * + * Tests deliberately avoid prose-matching: they assert on numeric exit + * codes, boolean flags, and reason codes pulled from the parsed JSON + * payload. + */ + +'use strict'; + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { runCli, parseSpawnResult } = require('./helpers/cli-negative.cjs'); +const { createTempProject, cleanup } = require('./helpers.cjs'); + +test('runCli rejects non-array argv with TypeError', () => { + assert.throws( + () => runCli('config-get', { cwd: '/tmp' }), + (err) => err instanceof TypeError && /argv/.test(err.message), + ); +}); + +test('runCli rejects missing cwd with TypeError', () => { + assert.throws( + () => runCli(['config-get'], {}), + (err) => err instanceof TypeError && /cwd/.test(err.message), + ); +}); + +test('runCli surfaces typed reason from a known failure path', (t) => { + const projectDir = createTempProject('cli-neg-harness-'); + t.after(() => cleanup(projectDir)); + // Unknown command — gsd-tools emits ERROR_REASON.SDK_UNKNOWN_COMMAND or + // USAGE depending on dispatch depth. Either is a real reason string; + // the contract we pin here is just "the IR carries a reason from the + // ERROR_REASON enum, never null". + const result = runCli(['this-command-does-not-exist'], { cwd: projectDir }); + assert.notEqual(result.status, 0, 'unknown command must exit non-zero'); + assert.equal(result.ok, false, 'JSON payload must report ok=false'); + assert.equal(typeof result.reason, 'string', 'reason must be a string from ERROR_REASON'); + assert.notEqual(result.reason, null); + assert.notEqual(result.reason, ''); + assert.equal(result.hasStackTrace, false, 'a typed failure must NOT print a V8 stack trace'); +}); + +test('parseSpawnResult detects stack-trace leakage in stderr', () => { + const fakeSpawn = { + status: 1, + signal: null, + stdout: '', + stderr: 'Error: boom\n at Object. (/some/file.js:10:5)\n at Module._compile\n', + error: null, + }; + const ir = parseSpawnResult(fakeSpawn, { jsonErrorsRequested: false }); + assert.equal(ir.hasStackTrace, true, 'stack frames in stderr must be flagged'); + assert.equal(ir.reason, null, 'non-JSON stderr leaves reason null'); +}); + +test('parseSpawnResult does NOT match the literal word "at" in prose', () => { + // Guard against a regex regression that would catch sentences like + // "command failed at startup" as stack frames. + const fakeSpawn = { + status: 1, + signal: null, + stdout: '', + stderr: 'Error: command failed at startup\nbecause no project was found.\n', + error: null, + }; + const ir = parseSpawnResult(fakeSpawn, { jsonErrorsRequested: false }); + assert.equal(ir.hasStackTrace, false, 'prose containing the word "at" is not a stack frame'); +}); + +test('parseSpawnResult extracts ok/reason/message from a json-errors payload', () => { + const payload = { ok: false, reason: 'config_invalid_key', message: 'no such key: foo' }; + const fakeSpawn = { + status: 1, + signal: null, + stdout: '', + stderr: JSON.stringify(payload) + '\n', + error: null, + }; + const ir = parseSpawnResult(fakeSpawn, { jsonErrorsRequested: true }); + assert.equal(ir.ok, false); + assert.equal(ir.reason, 'config_invalid_key'); + assert.equal(ir.message, 'no such key: foo'); + assert.equal(ir.hasStackTrace, false); +}); + +test('parseSpawnResult ignores malformed JSON in stderr without throwing', () => { + const fakeSpawn = { + status: 1, + signal: null, + stdout: '', + stderr: '{ ok: false, reason }', // missing quotes — invalid JSON + error: null, + }; + const ir = parseSpawnResult(fakeSpawn, { jsonErrorsRequested: true }); + assert.equal(ir.ok, null, 'malformed JSON must NOT promote partial data into ok'); + assert.equal(ir.reason, null); + assert.equal(ir.message, null); +}); + +test('parseSpawnResult ignores JSON arrays and primitives, only accepts objects', () => { + const cases = [ + '["ok", false]', // array + '"just a string"', // primitive + 'null', // null literal + '42', // number + ]; + for (const stderr of cases) { + const ir = parseSpawnResult( + { status: 1, signal: null, stdout: '', stderr, error: null }, + { jsonErrorsRequested: true }, + ); + assert.equal(ir.ok, null, `non-object JSON (${stderr}) must not set ok`); + assert.equal(ir.reason, null); + } +}); + +test('runCli treats jsonErrors=false as an explicit human-formatter path', (t) => { + const projectDir = createTempProject('cli-neg-harness-text-'); + t.after(() => cleanup(projectDir)); + const result = runCli(['this-command-does-not-exist'], { cwd: projectDir, jsonErrors: false }); + assert.notEqual(result.status, 0); + assert.equal(result.jsonErrorsRequested, false); + // Reason fields stay null in human-mode because stderr is prose, not JSON. + assert.equal(result.ok, null); + assert.equal(result.reason, null); + // But the prose still must not include a V8 stack trace. + assert.equal(result.hasStackTrace, false); +}); diff --git a/tests/feat-3593-cli-negative-universal.test.cjs b/tests/feat-3593-cli-negative-universal.test.cjs new file mode 100644 index 000000000..52e4461cc --- /dev/null +++ b/tests/feat-3593-cli-negative-universal.test.cjs @@ -0,0 +1,130 @@ +/** + * Universal CLI negative-matrix sweep across the seven command families + * named in #3593: phase, roadmap, state, config, workstream, init, + * validate. + * + * The full per-case matrix for each family belongs in dedicated files + * (the `config` family is the template — see + * `feat-3593-cli-negative-config.test.cjs`). This file pins a much + * narrower contract that EVERY family must satisfy: + * + * 1. Bare top-level command with no subcommand: must not crash with + * a V8 stack trace, must exit non-zero (or, where the bare form + * is legitimate, return a clean payload). + * 2. Unknown subcommand at command depth: must emit a typed reason + * under --json-errors and must not crash. + * 3. Shell-metacharacter as an argv element: must not be executed. + * Sentinel-file probe in the project temp dir proves this. + * + * Future per-family files will deepen each family's matrix; this file + * is the floor. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { runCli } = require('./helpers/cli-negative.cjs'); +const { createTempProject, createTempGitProject, cleanup } = require('./helpers.cjs'); + +/** + * Each entry names a top-level command, a representative subcommand + * (for the "unknown sub" probe), and the temp-project factory. + * + * The `representativeSubcommand` is not asserted on directly — it + * exists so the test layout reads "for each family, run probe X" and + * so a future maintainer adding a family doesn't need to invent one. + */ +const FAMILIES = [ + { name: 'phase', bare: ['phase'], unknown: ['phase', 'this-sub-does-not-exist'], projectFactory: createTempProject }, + { name: 'roadmap', bare: ['roadmap'], unknown: ['roadmap', 'this-sub-does-not-exist'], projectFactory: createTempProject }, + { name: 'state', bare: ['state'], unknown: ['state', 'this-sub-does-not-exist'], projectFactory: createTempProject }, + { name: 'config', bare: ['config'], unknown: ['config', 'this-sub-does-not-exist'], projectFactory: createTempProject }, + { name: 'workstream', bare: ['workstream'], unknown: ['workstream', 'this-sub-does-not-exist'], projectFactory: createTempGitProject }, + { name: 'init', bare: ['init'], unknown: ['init', 'this-sub-does-not-exist'], projectFactory: createTempProject }, + { name: 'validate', bare: ['validate'], unknown: ['validate', 'this-sub-does-not-exist'], projectFactory: createTempProject }, +]; + +describe('feat-3593: bare top-level command does not crash', () => { + for (const fam of FAMILIES) { + test(`${fam.name}: bare invocation exits cleanly without a stack trace`, (t) => { + const projectDir = fam.projectFactory(`cli-neg-univ-bare-${fam.name}-`); + t.after(() => cleanup(projectDir)); + const result = runCli(fam.bare, { cwd: projectDir }); + assert.equal(result.hasStackTrace, false, `${fam.name} bare must not leak a V8 stack frame`); + assert.equal(result.signal, null, `${fam.name} bare must not be killed by a signal`); + // We do not pin exit code here: some families legitimately treat the + // bare form as a list/status command (status 0); others reject it as + // a usage error (status ≠ 0). What we pin is "no crash." + }); + } +}); + +describe('feat-3593: unknown subcommand emits a typed reason', () => { + for (const fam of FAMILIES) { + test(`${fam.name}: unknown subcommand fails with reason set and no stack trace`, (t) => { + const projectDir = fam.projectFactory(`cli-neg-univ-unk-${fam.name}-`); + t.after(() => cleanup(projectDir)); + const result = runCli(fam.unknown, { cwd: projectDir }); + assert.notEqual(result.status, 0, `${fam.name} unknown sub must exit non-zero`); + assert.equal(result.hasStackTrace, false, `${fam.name} unknown sub must not leak a stack frame`); + // When --json-errors is on, every typed-failure path lands a non-empty + // reason string from ERROR_REASON. A null reason here means the family + // is using throw/console.error somewhere — a TDD signal to wire that + // failure path through error(msg, ERROR_REASON.X). + assert.equal(typeof result.reason, 'string', `${fam.name}: reason must be a typed enum string (got: ${result.reason})`); + assert.notEqual(result.reason, '', `${fam.name}: reason must be non-empty`); + }); + } +}); + +describe('feat-3593: shell-metacharacter argv values are NOT executed', () => { + for (const fam of FAMILIES) { + test(`${fam.name}: shell-payload as subcommand argv does NOT execute the payload`, (t) => { + const projectDir = fam.projectFactory(`cli-neg-univ-shell-${fam.name}-`); + t.after(() => cleanup(projectDir)); + // Place the payload where the subcommand value goes. The payload + // would, if shell-interpreted, create a sentinel file in the + // project dir. Argv-based invocation must treat it as opaque text. + const sentinelPayload = `$(touch ${projectDir}/INJ-${fam.name})`; + const argv = [fam.name, sentinelPayload]; + const result = runCli(argv, { cwd: projectDir }); + // No stack trace — opaque text must not crash. + assert.equal(result.hasStackTrace, false, `${fam.name}: shell payload must not crash`); + // The sentinel file must NOT exist. We check the project dir's + // listing rather than fs.existsSync of a single path so the test + // surfaces any spelling drift. + const entries = fs.readdirSync(projectDir); + const sentinels = entries.filter((n) => n.startsWith('INJ-')); + assert.deepEqual( + sentinels, + [], + `${fam.name}: shell payload was executed — sentinel files exist: ${sentinels.join(', ')}`, + ); + }); + } +}); + +// ─── Cross-family invariants on the global --cwd flag ────────────────────── + +test('--cwd with an empty value fails the same way regardless of command family', () => { + for (const fam of FAMILIES) { + const result = runCli(['--cwd', '', ...fam.bare], { cwd: process.cwd() }); + assert.notEqual(result.status, 0, `${fam.name}: empty --cwd must fail`); + assert.equal(result.hasStackTrace, false, `${fam.name}: empty --cwd must not crash`); + assert.equal(result.reason, 'usage', `${fam.name}: empty --cwd reason should be 'usage', got: ${result.reason}`); + } +}); + +test('--cwd pointing at a non-existent path fails uniformly across families', () => { + const nonExistent = path.join(require('os').tmpdir(), 'cli-neg-univ-no-such-' + Date.now() + '-' + Math.random()); + assert.equal(fs.existsSync(nonExistent), false, 'pre-check: temp path must not exist'); + for (const fam of FAMILIES) { + const result = runCli(['--cwd', nonExistent, ...fam.bare], { cwd: process.cwd() }); + assert.notEqual(result.status, 0, `${fam.name}: invalid --cwd must fail`); + assert.equal(result.hasStackTrace, false); + assert.equal(result.reason, 'usage', `${fam.name}: invalid --cwd reason should be 'usage'`); + } +}); diff --git a/tests/helpers/cli-negative.cjs b/tests/helpers/cli-negative.cjs new file mode 100644 index 000000000..c9b05cd03 --- /dev/null +++ b/tests/helpers/cli-negative.cjs @@ -0,0 +1,133 @@ +/** + * CLI negative-matrix harness (#3593). + * + * Wraps spawnSync of get-shit-done/bin/gsd-tools.cjs so test files can + * assert on structured outputs (exit code, typed reason, stack-trace + * absence) without each test re-implementing the JSON-errors parsing + * dance. Hostile values are passed as argv elements — never composed + * into a shell string — so shell metacharacters in test inputs (;, &&, + * $(), backticks, quotes, newlines, null bytes) reach the CLI as + * opaque data, not as shell syntax. + * + * Designed against CONTRIBUTING.md §"Testing Standards" and + * TEST-EXAMPLES.md §"CLI Negative Matrix": + * + * - spawnSync (no shell) per the "no shell strings" rule. + * - Returns a typed IR { status, ok, reason, message, ... } so tests + * assert on `.reason === REASON_CODE`, never on prose. + * - Detects stack-trace leakage in stderr; tests can fail when the + * CLI surfaces an unexpected exception under hostile input. + * - --json-errors is on by default; pass jsonErrors:false to verify + * human-formatted stderr paths. + * + * The harness does NOT decide what the test asserts — it just shapes + * the data so the assertion is mechanical and prose-free. + */ + +'use strict'; + +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const TOOLS_PATH = path.resolve(__dirname, '..', '..', 'get-shit-done', 'bin', 'gsd-tools.cjs'); + +/** + * Run gsd-tools with the given argv against a project directory. + * + * @param {string[]} argv - argument vector passed to gsd-tools.cjs. Each + * element reaches the child as a single argv element regardless of + * shell metacharacters in its value. + * @param {object} options + * @param {string} options.cwd - working directory for the child (REQUIRED). + * @param {object} [options.env] - env vars merged on top of process.env. + * @param {boolean} [options.jsonErrors=true] - prepend --json-errors so + * the CLI emits a structured `{ ok, reason, message }` payload to + * stderr. Set false to exercise the human-formatted path. + * @param {number} [options.timeoutMs=10000] - kill child if it runs longer. + * @returns {object} typed IR — see field docs below. + */ +function runCli(argv, options) { + if (!options || typeof options.cwd !== 'string' || options.cwd.length === 0) { + throw new TypeError('runCli: options.cwd (string) is required'); + } + if (!Array.isArray(argv)) { + throw new TypeError('runCli: argv must be an array (no shell strings)'); + } + const jsonErrors = options.jsonErrors !== false; + const finalArgs = jsonErrors ? ['--json-errors', ...argv] : argv.slice(); + const env = { ...process.env, ...(options.env || {}) }; + const spawnResult = spawnSync(process.execPath, [TOOLS_PATH, ...finalArgs], { + cwd: options.cwd, + encoding: 'utf-8', + timeout: typeof options.timeoutMs === 'number' ? options.timeoutMs : 10000, + env, + }); + return parseSpawnResult(spawnResult, { jsonErrorsRequested: jsonErrors }); +} + +/** + * Shape a raw spawnSync result into the harness IR. + * + * Returned fields: + * - status {number|null} child exit code (null if killed by signal) + * - signal {string|null} terminating signal name when killed + * - stdout {string} captured stdout + * - stderr {string} captured stderr + * - ok {boolean|null} parsed from JSON-errors payload; null when + * stderr is not JSON-shaped + * - reason {string|null} ERROR_REASON code from JSON-errors payload + * - message {string|null} human message from JSON-errors payload + * - hasStackTrace {boolean} stderr contains a `\n at ` frame line. + * Tests use this to assert the CLI didn't + * leak an unexpected exception. + * - jsonErrorsRequested {boolean} true when --json-errors was added by + * the harness; tests use this to decide + * whether the missing-reason case is a bug + * vs. expected (jsonErrors:false path). + * - spawnError {Error|null} spawnSync's `error` field (e.g. ENOENT + * on process.execPath, timeout signal). + */ +function parseSpawnResult(spawnResult, meta) { + const stderr = typeof spawnResult.stderr === 'string' ? spawnResult.stderr : ''; + const stdout = typeof spawnResult.stdout === 'string' ? spawnResult.stdout : ''; + // Stack-trace leak detection: V8 prints " at Function (path:line:col)" + // frames into stderr when an uncaught throw escapes. Any such line means + // the CLI hit a code path that wasn't wrapped in error()/typed reason. + // \n is required so we don't match the word "at" appearing in prose. + const hasStackTrace = /\n\s{2,}at\s+/.test(stderr); + + let ok = null; + let reason = null; + let message = null; + // JSON-errors mode emits a single-line JSON document to stderr followed by + // a newline. Parse defensively — any non-JSON stderr leaves the fields null + // and lets the test surface the discrepancy. + const trimmed = stderr.trim(); + if (trimmed.startsWith('{') && trimmed.endsWith('}')) { + try { + const parsed = JSON.parse(trimmed); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + if (typeof parsed.ok === 'boolean') ok = parsed.ok; + if (typeof parsed.reason === 'string') reason = parsed.reason; + if (typeof parsed.message === 'string') message = parsed.message; + } + } catch { + // Not JSON after all — leave parsed fields null. + } + } + + return { + status: spawnResult.status, + signal: spawnResult.signal, + stdout, + stderr, + ok, + reason, + message, + hasStackTrace, + jsonErrorsRequested: meta && meta.jsonErrorsRequested === true, + spawnError: spawnResult.error || null, + }; +} + +module.exports = { runCli, parseSpawnResult, TOOLS_PATH };