diff --git a/.changeset/calm-herons-wake.md b/.changeset/calm-herons-wake.md new file mode 100644 index 000000000..ede0fe87f --- /dev/null +++ b/.changeset/calm-herons-wake.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3272 +--- +**`gsd-sdk query milestone.complete --help` (and all mutating query handlers) no longer execute mutations** — the dispatcher now short-circuits to a non-mutating help stub when `--help`/`-h` appears in args for any native mutating handler (dispatcher-level guard, fail-closed by default). `milestoneComplete` also rejects `--help`/`-h` as a version value before any disk write (handler-level defense-in-depth). diff --git a/sdk/src/query/phase-lifecycle.test.ts b/sdk/src/query/phase-lifecycle.test.ts index 92eb01222..7e85d0d10 100644 --- a/sdk/src/query/phase-lifecycle.test.ts +++ b/sdk/src/query/phase-lifecycle.test.ts @@ -1505,6 +1505,68 @@ describe('phasesArchive', () => { }); }); +// ─── milestoneComplete help-flag defense (#3259) ──────────────────────────── + +describe('milestoneComplete help-flag defense', () => { + it('rejects --help as a version value with GSDError before any disk write', async () => { + const { milestoneComplete } = await import('./phase-lifecycle.js'); + const { GSDError, ErrorClassification } = await import('../errors.js'); + await setupTestProject(tmpDir); + + // Capture pre-invocation filesystem state + const planningDir = join(tmpDir, '.planning'); + const milestonesPath = join(planningDir, 'MILESTONES.md'); + const statePath = join(planningDir, 'STATE.md'); + const preStateStat = await import('node:fs').then((m) => m.statSync(statePath)); + const milestonesExistedBefore = existsSync(milestonesPath); + + let thrown: unknown; + try { + await milestoneComplete(['--help'], tmpDir); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(GSDError); + const err = thrown as InstanceType; + expect(err.classification).toBe(ErrorClassification.Validation); + expect(err.message).toContain('--help'); + + // Assert no files were written + const postStateStat = await import('node:fs').then((m) => m.statSync(statePath)); + expect(postStateStat.mtimeMs).toBe(preStateStat.mtimeMs); + expect(existsSync(milestonesPath)).toBe(milestonesExistedBefore); + }); + + it('rejects -h as a version value with GSDError before any disk write', async () => { + const { milestoneComplete } = await import('./phase-lifecycle.js'); + const { GSDError, ErrorClassification } = await import('../errors.js'); + await setupTestProject(tmpDir); + + const statePath = join(tmpDir, '.planning', 'STATE.md'); + const preStateStat = await import('node:fs').then((m) => m.statSync(statePath)); + const milestonesPath = join(tmpDir, '.planning', 'MILESTONES.md'); + const milestonesExistedBefore = existsSync(milestonesPath); + + let thrown: unknown; + try { + await milestoneComplete(['-h'], tmpDir); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(GSDError); + const err = thrown as InstanceType; + expect(err.classification).toBe(ErrorClassification.Validation); + expect(err.message).toContain('-h'); + + // Assert no files were written + const postStateStat = await import('node:fs').then((m) => m.statSync(statePath)); + expect(postStateStat.mtimeMs).toBe(preStateStat.mtimeMs); + expect(existsSync(milestonesPath)).toBe(milestonesExistedBefore); + }); +}); + // ─── Registry integration ────────────────────────────────────────────────── describe('lifecycle handlers in registry', () => { diff --git a/sdk/src/query/phase-lifecycle.ts b/sdk/src/query/phase-lifecycle.ts index b07bbe196..cae5c81d9 100644 --- a/sdk/src/query/phase-lifecycle.ts +++ b/sdk/src/query/phase-lifecycle.ts @@ -1781,6 +1781,14 @@ export const milestoneComplete: QueryHandler = async (args, projectDir, workstre if (!version) { throw new GSDError('version required for milestone complete (e.g., v1.0)', ErrorClassification.Validation); } + // #3259: defense-in-depth — reject --help / -h as a version value before + // any disk write, regardless of whether the dispatcher guard intercepted first. + if (version === '--help' || version === '-h') { + throw new GSDError( + `"${version}" is not a valid milestone version; see \`gsd-sdk query --help\` for command list`, + ErrorClassification.Validation, + ); + } assertNoNullBytes(version, 'version'); const nameOpt = parseMultiwordArg(args, 'name'); diff --git a/sdk/src/query/query-dispatch.test.ts b/sdk/src/query/query-dispatch.test.ts index 01ccbf9a1..70b22d72e 100644 --- a/sdk/src/query/query-dispatch.test.ts +++ b/sdk/src/query/query-dispatch.test.ts @@ -1,11 +1,13 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { mkdir, rm, writeFile, readdir, stat } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; +import { existsSync } from 'node:fs'; import { createRegistry } from './index.js'; import { GSDToolsError } from '../gsd-tools-error.js'; import { runQueryDispatch } from './query-dispatch.js'; import { createCommandTopology } from './command-topology.js'; +import { COMMAND_MUTATION_SET } from './command-definition.js'; describe('runQueryDispatch', () => { let tmpDir: string; let fixtureDir: string; @@ -188,3 +190,210 @@ describe('runQueryDispatch', () => { expect(out.error.details).toMatchObject({ command: 'state.json', args: [] }); }); }); + +// ─── #3259 help-flag non-mutating guard ────────────────────────────────────── + +describe('--help guard: dispatcher short-circuits mutating native handlers', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = join(tmpdir(), `qdispatch-help-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await mkdir(join(tmpDir, '.planning', 'phases'), { recursive: true }); + // Minimal fixture required for most handlers to not crash on fs reads + await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n\n## Current Milestone: v1.0\n', 'utf-8'); + await writeFile( + join(tmpDir, '.planning', 'STATE.md'), + '---\ngsd_state_version: 1.0\nmilestone: v1.0\nstatus: executing\n---\n\n# Project State\n', + 'utf-8', + ); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'balanced', phase_naming: 'sequential' }), + 'utf-8', + ); + }); + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + }); + + /** + * Collect a digest of all file mtimes under .planning/ so we can compare + * pre- and post-invocation state without reading file content. + */ + async function collectPlanningDigest(projectDir: string): Promise> { + const planningDir = join(projectDir, '.planning'); + const digest = new Map(); + async function walk(dir: string): Promise { + let entries; + try { + entries = await readdir(dir, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + const full = join(dir, entry.name); + if (entry.isDirectory()) { + await walk(full); + } else { + try { + const s = await stat(full); + digest.set(full, s.mtimeMs); + } catch { + /* ignore */ + } + } + } + } + await walk(planningDir); + return digest; + } + + it('milestone.complete --help returns non-mutating help stub without writing to .planning/', async () => { + const registry = createRegistry(); + const topology = createCommandTopology(registry); + + const preDig = await collectPlanningDigest(tmpDir); + + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: false, + resolveGsdToolsPath: () => '', + topology, + }, ['milestone.complete', '--help']); + + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); + + // Response must contain help stub, not a milestone record + const parsed = JSON.parse(out.stdout) as Record; + expect(typeof parsed.help).toBe('string'); + expect(parsed.help).toContain('milestone.complete'); + + // .planning/ directory must be byte-identical (no new or modified files) + const postDig = await collectPlanningDigest(tmpDir); + expect(postDig.size).toBe(preDig.size); + for (const [path, mtime] of preDig) { + expect(postDig.get(path)).toBe(mtime); + } + // MILESTONES.md must not have been created + expect(existsSync(join(tmpDir, '.planning', 'MILESTONES.md'))).toBe(false); + }); + + it('milestone.complete -h returns non-mutating help stub without writing to .planning/', async () => { + const registry = createRegistry(); + const topology = createCommandTopology(registry); + + const preDig = await collectPlanningDigest(tmpDir); + + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: false, + resolveGsdToolsPath: () => '', + topology, + }, ['milestone.complete', '-h']); + + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); + + const parsed = JSON.parse(out.stdout) as Record; + expect(typeof parsed.help).toBe('string'); + + const postDig = await collectPlanningDigest(tmpDir); + expect(postDig.size).toBe(preDig.size); + for (const [path, mtime] of preDig) { + expect(postDig.get(path)).toBe(mtime); + } + }); + + it('registry-driven: all native mutating handlers with --help do not modify .planning/', async () => { + const registry = createRegistry(); + const topology = createCommandTopology(registry); + + // Collect all registered mutating commands from the manifest + const mutatingCommands = Array.from(COMMAND_MUTATION_SET).filter((cmd) => { + // Only canonical forms that are registered in the registry (not aliases) + return registry.has(cmd); + }); + + for (const cmd of mutatingCommands) { + // Reset fixture between each command to ensure isolation + await rm(join(tmpDir, '.planning'), { recursive: true, force: true }); + await mkdir(join(tmpDir, '.planning', 'phases'), { recursive: true }); + await writeFile(join(tmpDir, '.planning', 'ROADMAP.md'), '# Roadmap\n\n## Current Milestone: v1.0\n', 'utf-8'); + await writeFile( + join(tmpDir, '.planning', 'STATE.md'), + '---\ngsd_state_version: 1.0\nmilestone: v1.0\nstatus: executing\n---\n\n# Project State\n', + 'utf-8', + ); + await writeFile( + join(tmpDir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'balanced', phase_naming: 'sequential' }), + 'utf-8', + ); + + const preDig = await collectPlanningDigest(tmpDir); + + // Invoke via dispatcher with --help in args (after the command token) + // argv format: [cmd, '--help'] where cmd may be dotted or spaced + const argv = [...cmd.split(' '), '--help']; + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: false, + resolveGsdToolsPath: () => '', + topology, + }, argv); + + // Must succeed (help stub) or fail for validation reasons (e.g. arg rewriting + // that produces a non-mutating command) — the invariant is no disk mutation. + const postDig = await collectPlanningDigest(tmpDir); + expect(postDig.size, `${cmd} --help created new .planning files`).toBe(preDig.size); + for (const [path, mtime] of preDig) { + expect(postDig.get(path), `${cmd} --help modified ${path}`).toBe(mtime); + } + } + }); + + it('preserves #3019 contract: unknown-cmd --help falls through to cjs (not intercepted by guard)', async () => { + // The guard only fires when a NATIVE MUTATING handler is matched. + // Unknown commands with --help must still fall through to CJS fallback. + const script = await (async () => { + const scriptPath = join(tmpDir, 'text.cjs'); + await writeFile(scriptPath, "process.stdout.write('USAGE: help text');", { mode: 0o755 }); + return scriptPath; + })(); + + const registry = createRegistry(); + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: true, + resolveGsdToolsPath: () => script, + topology: createCommandTopology(registry), + }, ['unknown-cmd', '--help']); + + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); + expect(out.stdout).toBe('USAGE: help text\n'); + }); + + it('non-mutating native handlers are unaffected when --help is in args', async () => { + // E.g. state.json is non-mutating; --help in args should still dispatch normally. + const registry = createRegistry(); + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: true, + resolveGsdToolsPath: () => '', + dispatchNative: async () => ({ data: { ok: true } }), + topology: createCommandTopology(registry), + }, ['state', 'json', '--help']); + + // state.json is non-mutating, so --help should pass through to the handler + // The mock handler returns successfully, so we get a success result. + expect(out.ok).toBe(true); + }); +}); diff --git a/sdk/src/query/query-dispatch.ts b/sdk/src/query/query-dispatch.ts index 5f38a6ec9..42d7a0384 100644 --- a/sdk/src/query/query-dispatch.ts +++ b/sdk/src/query/query-dispatch.ts @@ -213,6 +213,21 @@ export async function runQueryDispatch(deps: QueryDispatchDeps, queryArgv: strin return toDispatchFailure(mapFallbackDispatchError(new Error('No native match in dispatch plan'), normCmd, normArgs)); } + // #3259: guard — if the invocation contains --help / -h AND the matched + // handler is a mutating command (mutation: true in the command manifest), + // short-circuit to a non-mutating stub. Mutating handlers are not help-aware + // by default (fail-closed). This prevents e.g. `milestone.complete --help` + // from writing milestone artifacts to disk. + const helpFlagPresent = matched.args.some((a) => a === '--help' || a === '-h'); + if (helpFlagPresent && matched.mutation) { + return dispatchSuccess( + formatSuccess( + { help: `Usage: gsd-sdk query ${matched.canonical} [args...]` }, + undefined, + ), + ); + } + const dispatchNative = deps.nativeAdapter ? (cmd: string, args: string[]) => deps.nativeAdapter!.dispatch(cmd, args) : deps.dispatchNative;