diff --git a/.changeset/help-passthrough.md b/.changeset/help-passthrough.md new file mode 100644 index 000000000..f6d82376b --- /dev/null +++ b/.changeset/help-passthrough.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3026 +--- +**`gsd-sdk query --help` now reaches the handler instead of returning top-level usage.** The query argv parser harvested `--help` as a global flag and `main()` short-circuited dispatch — there was no path to discover what arguments a query subcommand accepts. The parser now leaves `--help` in `queryArgv` so the handler/fallback can render contextual help. The `gsd-tools.cjs` fallback now renders top-level usage on `--help` (instead of erroring), preserving #1818's anti-hallucination invariant by NOT executing the destructive command. See #3019. diff --git a/get-shit-done/bin/gsd-tools.cjs b/get-shit-done/bin/gsd-tools.cjs index 6e1c057d8..19cc1dfaf 100755 --- a/get-shit-done/bin/gsd-tools.cjs +++ b/get-shit-done/bin/gsd-tools.cjs @@ -341,17 +341,50 @@ async function main() { const command = args[0]; + // Top-level usage string — emitted by `gsd-tools` (no args) and by + // `gsd-tools --help` / any `--help` request below. + // CR feedback: the command list must enumerate every top-level command + // supported by the dispatcher so `--help` is actually useful for + // discovery; previously it was a partial subset that didn't include + // phase / roadmap / milestone / progress / etc. + const TOP_LEVEL_USAGE = 'Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--ws ]\n' + + 'Commands: agent-skills, audit-open, audit-uat, check-commit, commit, commit-to-subrepo, ' + + 'config-ensure-section, config-get, config-new-project, config-path, config-set, ' + + 'current-timestamp, detect-custom-files, docs-init, extract-messages, find-phase, ' + + 'from-gsd2, frontmatter, gap-analysis, generate-claude-md, generate-claude-profile, ' + + 'generate-dev-preferences, generate-slug, graphify, history-digest, init, intel, ' + + 'learnings, list-todos, milestone, phase, phase-plan-index, phases, profile-questionnaire, ' + + 'profile-sample, progress, requirements, resolve-model, roadmap, scaffold, state, ' + + 'template, validate, verify, verify-path-exists, verify-summary, workstream\n\n' + + 'For command-specific argument requirements, invoke the command without args ' + + '(e.g. `gsd-tools phase add`) — the resulting error lists what is required.'; + if (!command) { - error('Usage: gsd-tools [args] [--raw] [--pick ] [--cwd ] [--ws ]\nCommands: state, resolve-model, find-phase, commit, verify-summary, verify, frontmatter, template, generate-slug, current-timestamp, list-todos, verify-path-exists, config-ensure-section, config-new-project, init, workstream, docs-init'); + error(TOP_LEVEL_USAGE); } - // Reject flags that are never valid for any gsd-tools command. AI agents - // sometimes hallucinate --help or --version on tool invocations; silently - // ignoring them can cause destructive operations to proceed unchecked. - const NEVER_VALID_FLAGS = new Set(['-h', '--help', '-?', '--h', '--version', '-v', '--usage']); + // #3019: a `--help` / `-h` flag in argv must render the top-level usage + // and exit 0 — not error out with "Unknown flag". The previous shape + // erred on agent-hallucinated flags, but it also blocked humans from + // discovering the command surface via subcommand help requests routed + // here from the SDK CLI's query dispatcher (after the cli.ts fix that + // stops harvesting --help as a global flag). Rendering top-level usage + // on --help is strictly better UX than the old short-circuit, which + // printed the SDK-level usage that doesn't mention any of these + // subcommands. + const HELP_FLAGS = new Set(['-h', '--help', '-?', '--h', '--usage']); + if (args.some((a) => HELP_FLAGS.has(a))) { + process.stdout.write(TOP_LEVEL_USAGE + '\n'); + return; + } + + // Reject version flags. AI agents sometimes hallucinate --version on tool + // invocations; silently ignoring it can cause destructive operations to + // proceed unchecked. (Help flags are handled above.) + const NEVER_VALID_FLAGS = new Set(['--version', '-v']); for (const arg of args) { if (NEVER_VALID_FLAGS.has(arg)) { - error(`Unknown flag: ${arg}\ngsd-tools does not accept help or version flags. Run "gsd-tools" with no arguments for usage.`); + error(`Unknown flag: ${arg}\ngsd-tools does not accept version flags. Run "gsd-tools" with no arguments for usage.`); } } diff --git a/sdk/src/cli.test.ts b/sdk/src/cli.test.ts index c2587f421..ee66a5aa9 100644 --- a/sdk/src/cli.test.ts +++ b/sdk/src/cli.test.ts @@ -126,6 +126,49 @@ describe('parseCliArgs', () => { expect(result.queryArgv).toEqual(['audit-open', '--json']); }); + // ─── #3019: --help inside `query ` reaches the handler ──── + + it('forwards --help to queryArgv when a subcommand precedes it (#3019)', () => { + // gsd-sdk query phase add --help + // Previously: --help was harvested as global, queryArgv = ['phase', 'add'], + // help: true → main() short-circuits to top-level USAGE, never dispatching. + // Now: --help travels with the rest of queryArgv so the registry handler + // (or the gsd-tools.cjs fallback) can render contextual subcommand help. + const result = parseCliArgs(['query', 'phase', 'add', '--help']); + expect(result.command).toBe('query'); + expect(result.queryArgv).toEqual(['phase', 'add', '--help']); + // The global help flag must NOT short-circuit dispatch when there is a + // subcommand to dispatch to. + expect(result.help).toBe(false); + }); + + it('forwards -h to queryArgv when a subcommand precedes it (#3019)', () => { + const result = parseCliArgs(['query', 'init', '-h']); + expect(result.queryArgv).toEqual(['init', '-h']); + expect(result.help).toBe(false); + }); + + it('treats bare `query --help` as a top-level help request (no subcommand to dispatch to)', () => { + // gsd-sdk query --help + // No subcommand follows, so the only useful response is the top-level + // USAGE. Preserve existing behavior: help: true. + const result = parseCliArgs(['query', '--help']); + expect(result.command).toBe('query'); + expect(result.help).toBe(true); + // queryArgv may be empty or carry just the lone --help; either is fine + // because main() short-circuits on help when there is no subcommand. + expect((result.queryArgv ?? []).filter((x) => x !== '--help' && x !== '-h')).toEqual([]); + }); + + it('preserves --help position when intermixed with other query flags (#3019)', () => { + // gsd-sdk query phase --help --pick name + // The handler/fallback should see --help in argv so it can render help + // even when other flags are present. + const result = parseCliArgs(['query', 'phase', '--help', '--pick', 'name']); + expect(result.queryArgv).toEqual(['phase', '--help', '--pick', 'name']); + expect(result.help).toBe(false); + }); + // ─── Init command parsing ────────────────────────────────────────────── it('parses init with @file input', () => { diff --git a/sdk/src/cli.ts b/sdk/src/cli.ts index 0bcfff519..d373dbaa7 100644 --- a/sdk/src/cli.ts +++ b/sdk/src/cli.ts @@ -85,8 +85,17 @@ function parseCliArgsQueryPermissive(argv: string[]): ParsedCliArgs { i += 2; continue; } + // #3019: do NOT consume -h / --help here unconditionally. Pushing the + // flag onto queryArgv lets the registered handler (or the gsd-tools.cjs + // fallback) render contextual subcommand help. We still set the global + // `help` flag when the flag appears, but only short-circuit dispatch in + // main() when there is no real subcommand to dispatch to (i.e. the only + // tokens in queryArgv are the help flags themselves). That preserves + // `gsd-sdk query --help` → top-level USAGE while letting + // `gsd-sdk query phase add --help` reach the handler. if (a === '-h' || a === '--help') { help = true; + queryArgv.push(a); i += 1; continue; } @@ -99,6 +108,14 @@ function parseCliArgsQueryPermissive(argv: string[]): ParsedCliArgs { i += 1; } + // If the user typed a real subcommand (anything other than help flags + // alone in queryArgv), do NOT short-circuit to top-level USAGE on help. + // The handler/fallback will render contextual help. + const nonHelpTokens = queryArgv.filter((t) => t !== '-h' && t !== '--help'); + if (help && nonHelpTokens.length > 0) { + help = false; + } + return { command: 'query', prompt: undefined, @@ -430,7 +447,22 @@ export async function main(argv: string[] = process.argv.slice(2)): Promise --help`. + * + * The tests therefore assert two things: + * 1. The destructive command did NOT run (anti-hallucination invariant). + * 2. The output contains the top-level usage (#3019 discoverability). + * + * --version remains rejected — it's never a valid gsd-tools flag and has + * no discovery use-case. */ 'use strict'; const { describe, test, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); -const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const fs = require('node:fs'); +const path = require('node:path'); +const { runGsdTools, createTempProject, cleanup, isUsageOutput } = require('./helpers.cjs'); -describe('unknown flag guard (bug #1818)', () => { +describe('unknown flag guard (bug #1818, updated for #3019)', () => { let tmpDir; beforeEach(() => { @@ -24,51 +40,68 @@ describe('unknown flag guard (bug #1818)', () => { cleanup(tmpDir); }); - // ── --help flag ──────────────────────────────────────────────────────────── + // ── --help renders usage and does NOT run the destructive command ──────── + + test('phases clear --help renders usage and does NOT clear phase dirs', () => { + // Create a sentinel phase dir so we can assert it survives. + const phaseDir = path.join(tmpDir, '.planning', 'phases', 'phase-99'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, 'PLAN.md'), 'sentinel'); - test('phases clear --help is rejected with non-zero exit', () => { const result = runGsdTools(['phases', 'clear', '--help'], tmpDir); - assert.strictEqual(result.success, false, 'should fail, not run destructive clear'); - assert.match(result.error, /--help/); + assert.strictEqual(result.success, true, 'help renders, no error exit'); + assert.ok(isUsageOutput(result.output), `expected top-level usage, got: ${result.output}`); + // Anti-hallucination invariant: the destructive command did NOT run. + assert.ok(fs.existsSync(phaseDir), 'phase dir must survive — clear must not have executed'); + assert.ok(fs.existsSync(path.join(phaseDir, 'PLAN.md'))); }); - test('generate-slug hello --help is rejected', () => { - // Non-destructive baseline: generate-slug hello succeeds without --help + test('generate-slug hello --help renders usage and does NOT emit a slug', () => { const ok = runGsdTools(['generate-slug', 'hello'], tmpDir); - assert.strictEqual(ok.success, true, 'control: generate-slug without --help must succeed'); + assert.strictEqual(ok.success, true, 'control: generate-slug works without --help'); + // The control output is just the slug; the help output is the usage. + const slugOut = ok.output; + assert.ok(slugOut && !isUsageOutput(slugOut), `control should not be usage: ${slugOut}`); const result = runGsdTools(['generate-slug', 'hello', '--help'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /--help/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output), 'help renders top-level usage'); + assert.notEqual(result.output, slugOut, 'help output must differ from the slug — generate-slug must not have run'); }); - test('phase complete --help is rejected', () => { + test('phase complete --help renders usage and does NOT mark a phase complete', () => { const result = runGsdTools(['phase', 'complete', '--help'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /--help/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); + // success:true + isUsageOutput is sufficient: if the destructive path + // had executed it would have emitted a phase-resolution error to stderr + // (success:false), not the usage to stdout (success:true). }); - test('state load --help is rejected', () => { + test('state load --help renders usage', () => { const result = runGsdTools(['state', 'load', '--help'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /--help/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); }); - // ── -h shorthand ────────────────────────────────────────────────────────── + // ── -h shorthand: same shape ───────────────────────────────────────────── - test('phases clear -h is rejected', () => { + test('phases clear -h renders usage and does NOT clear phase dirs', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', 'phase-42'); + fs.mkdirSync(phaseDir, { recursive: true }); const result = runGsdTools(['phases', 'clear', '-h'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /-h/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); + assert.ok(fs.existsSync(phaseDir), 'phase dir must survive'); }); - test('generate-slug hello -h is rejected', () => { + test('generate-slug hello -h renders usage', () => { const result = runGsdTools(['generate-slug', 'hello', '-h'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /-h/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); }); - // ── other common hallucinated flags ─────────────────────────────────────── + // ── --version is still rejected — no discovery use-case ────────────────── test('generate-slug hello --version is rejected', () => { const result = runGsdTools(['generate-slug', 'hello', '--version'], tmpDir); @@ -76,9 +109,11 @@ describe('unknown flag guard (bug #1818)', () => { assert.match(result.error, /--version/); }); - test('current-timestamp --help is rejected', () => { + // ── current-timestamp --help: same as the others ───────────────────────── + + test('current-timestamp --help renders usage', () => { const result = runGsdTools(['current-timestamp', '--help'], tmpDir); - assert.strictEqual(result.success, false); - assert.match(result.error, /--help/); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); }); }); diff --git a/tests/bug-3019-help-passthrough.test.cjs b/tests/bug-3019-help-passthrough.test.cjs new file mode 100644 index 000000000..2b3da53fa --- /dev/null +++ b/tests/bug-3019-help-passthrough.test.cjs @@ -0,0 +1,120 @@ +/** + * Regression test for bug #3019. + * + * `gsd-sdk query --help` returned the top-level SDK USAGE + * instead of contextual help for the subcommand. The query argv parser + * harvested --help as a global flag and main() short-circuited dispatch + * before the registry handler / gsd-tools.cjs fallback could render + * useful help. + * + * Two-layer fix: + * 1. sdk/src/cli.ts — leave --help in queryArgv so it travels to the + * handler/fallback. Only honor the global help flag when there is + * no subcommand to dispatch to. + * 2. get-shit-done/bin/gsd-tools.cjs — render the top-level usage on + * --help instead of erroring. Anti-hallucination invariant from + * #1818 is preserved (the destructive command never executes). + * + * Tests the integration: invoke gsd-tools.cjs the same way the SDK + * dispatcher does and assert structured-IR (success flag + usage shape) + * rather than raw substring matches. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const { runGsdTools, isUsageOutput } = require('./helpers.cjs'); + +// #3026 CR (Major outside-diff): the SDK fallback wraps gsd-tools.cjs. +// When gsd-tools emits plain-text help (exit 0), the SDK previously +// JSON.parsed stdout and threw "Unexpected token 'U'". Verify the fix +// by invoking the built SDK end-to-end and asserting: +// - exit 0 +// - stdout contains the gsd-tools usage +// - stderr does NOT contain a JSON parse error +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const SDK_CLI = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js'); +const fs = require('node:fs'); + +describe('bug #3026 (CR Major outside-diff): SDK forwards plain-text help from gsd-tools fallback', () => { + test('gsd-sdk query phase --help (fallback path) returns usage, not a JSON parse error', (t) => { + if (!fs.existsSync(SDK_CLI)) { + // CR feedback (#3026): a bare `return` here silent-passes the test + // when sdk/dist/cli.js is absent (CI checkouts that haven't run + // `npm run build`), giving no signal that the integration check + // was skipped. Use t.skip() so the omission is visible in the + // test report. The unit-level fix is covered by vitest on + // sdk/src/cli.ts; this integration test only runs when the + // built SDK is on disk. + t.skip('sdk/dist/cli.js not built — run `npm run build` in sdk/ to enable this integration test'); + return; + } + // `query phase --help` (no further subcommand) is NOT in the native + // registry, so it routes through the gsd-tools.cjs fallback. That is + // the path that JSON.parsed the help text and threw before this fix. + const result = spawnSync(process.execPath, [SDK_CLI, 'query', 'phase', '--help'], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 10000, + }); + // The fallback gsd-tools.cjs emits exit 0 with usage on stdout. + assert.strictEqual(result.status, 0, + `must exit 0 — got ${result.status}\nstdout: ${result.stdout}\nstderr: ${result.stderr}`); + // Negative: must NOT see the JSON parse error that was the regression. + assert.ok(!/Unexpected token|not valid JSON/i.test(result.stderr), + `must NOT JSON.parse the help text (stderr): ${result.stderr}`); + // Positive: the usage should reach the user via stdout. + assert.ok(/Usage:\s*gsd-tools/.test(result.stdout) && /Commands:/.test(result.stdout), + `usage must reach stdout: ${result.stdout}`); + }); +}); + +describe('bug #3019: gsd-tools renders usage on --help instead of erroring', () => { + test('bare gsd-tools (no args) renders usage', () => { + const result = runGsdTools([]); + // No args path: error() helper emits to stderr and exits non-zero, + // but the message body is the usage. + assert.strictEqual(result.success, false); + assert.ok(/Usage:\s*gsd-tools/.test(result.error)); + assert.ok(/Commands:/.test(result.error)); + }); + + test('gsd-tools --help renders usage on stdout, exits 0', () => { + const result = runGsdTools(['--help']); + assert.strictEqual(result.success, true, '--help should not be an error'); + assert.ok(isUsageOutput(result.output), `expected usage on stdout, got: ${result.output}`); + }); + + test('gsd-tools -h renders usage on stdout, exits 0', () => { + const result = runGsdTools(['-h']); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); + }); + + test('gsd-tools --help renders usage (does not run subcommand)', () => { + // The classic #3019 surface: the user types a subcommand expecting + // contextual help. We render the top-level usage — strictly better + // than the previous unhelpful "Unknown flag --help" error. + const result = runGsdTools(['phase', 'add', '--help']); + assert.strictEqual(result.success, true); + assert.ok(isUsageOutput(result.output)); + }); + + test('usage hint mentions how to discover argument requirements', () => { + // The usage now points users at the discovery method that actually works + // (run without args → error message names required arguments). Asserting + // on the parsed shape of the usage rather than substring-matching prose: + const result = runGsdTools(['--help']); + assert.strictEqual(result.success, true); + // Structural check: split into sections. + const lines = result.output.split('\n'); + const hasUsageLine = lines.some((l) => l.startsWith('Usage:')); + const hasCommandsLine = lines.some((l) => l.startsWith('Commands:')); + const hasDiscoveryHint = lines.some((l) => /argument requirements|without args|invoke the command/i.test(l)); + assert.ok(hasUsageLine, 'first section: Usage'); + assert.ok(hasCommandsLine, 'second section: Commands'); + assert.ok(hasDiscoveryHint, 'third section: how to discover per-command args'); + }); +}); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 0289252a2..7bb14585c 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -163,4 +163,11 @@ function parseFrontmatter(content) { return fields; } -module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, parseFrontmatter, TOOLS_PATH }; +// #3026 CR: shared `--help` output check used by bug-1818 + bug-3019 tests. +// Render-on-help shape is `Usage: gsd-tools …\nCommands: …` — both lines +// must be present; structural test, not prose substring matching. +function isUsageOutput(text) { + return /Usage:\s*gsd-tools/.test(text) && /Commands:/.test(text); +} + +module.exports = { runGsdTools, createTempDir, createTempProject, createTempGitProject, cleanup, parseFrontmatter, isUsageOutput, TOOLS_PATH };