From 7ff196c505c5c941ee64f6bbc0d1fff163c52477 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 5 Sep 2026 13:49:58 -0400 Subject: [PATCH] fix(#4096): honor --dry-run in todo complete and write completion keys inside the frontmatter fence (#4325) * fix(#4096): honor --dry-run in todo complete and upsert completion keys inside the frontmatter fence * review(#4096): tighten todo complete flag rejection to any dash-prefixed token * chore(#4096): backfill PR number in changeset --------- Co-authored-by: sim --- .changeset/calm-pumas-frolic.md | 5 ++ docs/CLI-TOOLS.md | 11 ++- docs/ja-JP/CLI-TOOLS.md | 2 +- docs/ko-KR/CLI-TOOLS.md | 2 +- docs/pt-BR/CLI-TOOLS.md | 2 +- docs/zh-CN/CLI-TOOLS.md | 2 +- gsd-core/bin/gsd-tools.cjs | 15 +++- src/commands.cts | 67 ++++++++++++++-- tests/commands.test.cjs | 134 +++++++++++++++++++++++++++++++- 9 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 .changeset/calm-pumas-frolic.md diff --git a/.changeset/calm-pumas-frolic.md b/.changeset/calm-pumas-frolic.md new file mode 100644 index 000000000..19bfc4170 --- /dev/null +++ b/.changeset/calm-pumas-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4325 +--- +**`todo complete` honors `--dry-run` and stops corrupting frontmatter** — the flag was accepted and silently ignored (the todo was moved, exit 0, `completed: true` reported), and the `completed:` stamp was written above the opening `---` fence so no fence-locating reader could parse the archived file. `--dry-run` now prints a preview-shaped payload (`dry_run`/`would_*`) and touches nothing; a real completion upserts `completed:` and `status: completed` inside the frontmatter block, and unknown flags fail loudly instead of being dropped. (#4096) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 25e1a0d7e..960fa56f5 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1161,8 +1161,17 @@ active window are still outstanding. ```bash # Complete a todo -node gsd-tools.cjs todo complete +node gsd-tools.cjs todo complete [--dry-run] +``` +`--dry-run` previews the completion (a `dry_run`/`would_*` JSON payload naming +the source, the destination, and the frontmatter keys it would set) without +moving the file or touching anything on disk. A real completion moves the todo +from `todos/pending/` to `todos/completed/` and upserts `completed:` and +`status: completed` inside the file's frontmatter block. Unknown flags are +rejected loudly. + +```bash # UAT audit — scan all phases for unresolved items node gsd-tools.cjs audit-uat diff --git a/docs/ja-JP/CLI-TOOLS.md b/docs/ja-JP/CLI-TOOLS.md index 59d640da2..a600b623c 100644 --- a/docs/ja-JP/CLI-TOOLS.md +++ b/docs/ja-JP/CLI-TOOLS.md @@ -396,7 +396,7 @@ node gsd-tools.cjs progress [json|table|bar] node gsd-tools.cjs progress --json # TODO を完了にする -node gsd-tools.cjs todo complete +node gsd-tools.cjs todo complete [--dry-run] # UAT 監査 — 全フェーズの未解決項目をスキャン node gsd-tools.cjs audit-uat diff --git a/docs/ko-KR/CLI-TOOLS.md b/docs/ko-KR/CLI-TOOLS.md index d3c57aa96..991c7b05f 100644 --- a/docs/ko-KR/CLI-TOOLS.md +++ b/docs/ko-KR/CLI-TOOLS.md @@ -396,7 +396,7 @@ node gsd-tools.cjs progress [json|table|bar] node gsd-tools.cjs progress --json # 할 일 완료 -node gsd-tools.cjs todo complete +node gsd-tools.cjs todo complete [--dry-run] # UAT 감사 — 모든 단계에서 미해결 항목 스캔 node gsd-tools.cjs audit-uat diff --git a/docs/pt-BR/CLI-TOOLS.md b/docs/pt-BR/CLI-TOOLS.md index bcfaf0192..d0445eee3 100644 --- a/docs/pt-BR/CLI-TOOLS.md +++ b/docs/pt-BR/CLI-TOOLS.md @@ -398,7 +398,7 @@ node gsd-tools.cjs progress [json|table|bar] node gsd-tools.cjs progress --json # Conclui uma tarefa -node gsd-tools.cjs todo complete +node gsd-tools.cjs todo complete [--dry-run] # Auditoria UAT — verifica todas as fases em busca de itens não resolvidos node gsd-tools.cjs audit-uat diff --git a/docs/zh-CN/CLI-TOOLS.md b/docs/zh-CN/CLI-TOOLS.md index 31903097d..e77fb61e5 100644 --- a/docs/zh-CN/CLI-TOOLS.md +++ b/docs/zh-CN/CLI-TOOLS.md @@ -396,7 +396,7 @@ node gsd-tools.cjs progress [json|table|bar] node gsd-tools.cjs progress --json # 完成待办事项 -node gsd-tools.cjs todo complete +node gsd-tools.cjs todo complete [--dry-run] # UAT 审计——扫描所有阶段的未解决事项 node gsd-tools.cjs audit-uat diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index ab9a5601a..d8977c376 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -2699,7 +2699,20 @@ function dispatchOverlayCapabilityCommand({ command, args, cwd, raw, error, load function routeTodo({ args, cwd, raw, error }) { const subcommand = args[1]; if (subcommand === 'complete') { - commands.cmdTodoComplete(cwd, args[2], raw); + // #4096: this verb's flag surface is closed. `--dry-run` is parsed + // and plumbed (mirroring routeMilestone / #2118), and any OTHER + // flag fails loudly instead of being silently ignored — an + // accepted-but-ignored safety flag converts a deliberate preview + // into the mutation it was meant to avoid. (`--raw` is spliced by + // the dispatcher before routing; it stays in the set as a guard + // against that splice ever moving.) + const TODO_COMPLETE_KNOWN_FLAGS = new Set(['--dry-run', '--raw']); + for (const a of args.slice(3)) { + if (typeof a === 'string' && a.startsWith('-') && !TODO_COMPLETE_KNOWN_FLAGS.has(a)) { + error(`Unknown flag for todo complete: ${a}`, ERROR_REASON.USAGE); + } + } + commands.cmdTodoComplete(cwd, args[2], { dryRun: args.includes('--dry-run') }, raw); } else if (subcommand === 'match-phase') { commands.cmdTodoMatchPhase(cwd, args[2], raw); } else { diff --git a/src/commands.cts b/src/commands.cts index fb746f75c..1230b678f 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -2906,7 +2906,41 @@ function cmdTodoMatchPhase(cwd: string, phase: string | undefined, raw: boolean) output({ phase, matches, todo_count: todos.length }, raw, undefined); } -function cmdTodoComplete(cwd: string, filename: string | undefined, raw: boolean): void { +// #4096: upsert completion keys INSIDE the leading frontmatter block. Never a +// bare prefix line above the opening `---` (that displaces the fence to line 2 +// and breaks every fence-locating reader). A file with no well-formed block +// (absent, or an unterminated opening fence) gains a complete block. +function upsertTodoCompletionFields(content: string, today: string): string { + const lines = content.split('\n'); + const fields = [`completed: ${today}`, 'status: completed']; + + const hasOpeningFence = lines[0] !== undefined && lines[0].trim() === '---'; + const closeIdx = hasOpeningFence ? lines.findIndex((l, i) => i > 0 && l.trim() === '---') : -1; + + if (!hasOpeningFence || closeIdx === -1) { + // No parseable frontmatter: wrap the whole content in a complete block + // rather than prefixing bare keys (#4096 fix 2). + return `---\n${fields.join('\n')}\n---\n\n${content}`; + } + + const block = lines.slice(1, closeIdx); + for (const field of fields) { + const key = `${field.slice(0, field.indexOf(':'))}:`; + const idx = block.findIndex(l => l.startsWith(key)); + if (idx === -1) { + block.push(field); + } else { + block[idx] = field; + } + } + return [...lines.slice(0, 1), ...block, ...lines.slice(closeIdx)].join('\n'); +} + +interface TodoCompleteOptions { + dryRun?: boolean; +} + +function cmdTodoComplete(cwd: string, filename: string | undefined, options: TodoCompleteOptions, raw: boolean): void { if (!filename) { error('filename required for todo complete'); } @@ -2919,15 +2953,34 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, raw: boolean error(`Todo not found: ${filename as string}`); } - // Ensure completed directory exists + const content = fs.readFileSync(sourcePath, 'utf-8'); + const today = realClock.localToday(); + + // #4096: --dry-run mirrors `milestone complete --dry-run` (#2118) — every + // existence check above still runs, nothing below mutates, and the payload + // is preview-shaped (`dry_run`/`would_*`), never `completed: true`. + if (options.dryRun) { + output({ + dry_run: true, + would_complete: true, + file: filename, + date: today, + would_move: { + source: path.relative(cwd, sourcePath).split(path.sep).join('/'), + target: path.relative(cwd, path.join(completedDir, filename as string)).split(path.sep).join('/'), + }, + would_set: { completed: today, status: 'completed' }, + }, raw); + return; + } + + // Ensure completed directory exists (only on the real run — a dry run + // creates nothing). platformEnsureDir(completedDir); - // Read, add completion timestamp, move - let content = fs.readFileSync(sourcePath, 'utf-8'); - const today = realClock.localToday(); - content = `completed: ${today}\n` + content; + const completedContent = upsertTodoCompletionFields(content, today); - platformWriteSync(path.join(completedDir, filename as string), content); + platformWriteSync(path.join(completedDir, filename as string), completedContent); fs.unlinkSync(sourcePath); output({ completed: true, file: filename, date: today }, raw, 'completed'); diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index ed7dab856..a299cdc1a 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -14,6 +14,7 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const { runGsdTools, createTempProject, createTempDir, cleanup } = require('./helpers.cjs'); +const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); @@ -652,12 +653,15 @@ describe('todo complete command', () => { 'should be in completed' ); - // Verify completion timestamp added + // Verify completion timestamp added — #4096: the completed/status keys must + // live INSIDE a well-formed frontmatter block (line 1 is the opening fence), + // never above it. const content = fs.readFileSync( path.join(tmpDir, '.planning', 'todos', 'completed', 'add-dark-mode.md'), 'utf-8' ); - assert.ok(content.startsWith('completed:'), 'should have completed timestamp'); + assert.ok(content.startsWith('---\n'), 'should open with a frontmatter fence'); + assert.match(content, /^completed: \d{4}-\d{2}-\d{2}$/m); }); test('fails for nonexistent todo', () => { @@ -665,6 +669,132 @@ describe('todo complete command', () => { assert.ok(!result.success, 'should fail'); assert.ok(result.error.includes('not found'), 'error mentions not found'); }); + + // #4096 regressions — --dry-run must not mutate, and completion keys must be + // written inside the frontmatter fence. + test('--dry-run previews without moving the file or mutating anything', () => { + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + const source = path.join(pendingDir, 'dry-run-probe.md'); + fs.writeFileSync(source, '---\ntitle: probe\nstatus: pending\n---\n\n# body\n'); + + const result = runGsdTools('todo complete dry-run-probe.md --dry-run', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.dry_run, true, 'payload must be preview-shaped (dry_run)'); + assert.strictEqual(output.would_complete, true, 'payload must say would_complete'); + assert.strictEqual('completed' in output, false, 'preview must never report completed:true'); + assert.strictEqual(output.file, 'dry-run-probe.md'); + assert.match(output.date, /^\d{4}-\d{2}-\d{2}$/); + + // File untouched, in place; nothing written to completed/. + assert.ok(fs.existsSync(source), 'pending file must still exist under --dry-run'); + const completedPath = path.join(tmpDir, '.planning', 'todos', 'completed', 'dry-run-probe.md'); + assert.ok(!fs.existsSync(completedPath), 'no completed copy under --dry-run'); + assert.strictEqual( + fs.readFileSync(source, 'utf-8'), + '---\ntitle: probe\nstatus: pending\n---\n\n# body\n', + 'source bytes must be unchanged under --dry-run' + ); + }); + + test('--dry-run still fails for nonexistent todo', () => { + const result = runGsdTools('todo complete nonexistent.md --dry-run', tmpDir); + assert.ok(!result.success, 'dry-run must still run existence checks'); + assert.ok(result.error.includes('not found'), 'error mentions not found'); + }); + + test('writes completed and status: completed inside the frontmatter fence', () => { + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + fs.writeFileSync( + path.join(pendingDir, 'fence-probe.md'), + '---\ntitle: fence probe\nstatus: pending\ncreated: 2025-01-01\n---\n\n# body\n' + ); + + const result = runGsdTools('todo complete fence-probe.md', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const content = fs.readFileSync( + path.join(tmpDir, '.planning', 'todos', 'completed', 'fence-probe.md'), + 'utf-8' + ); + // Line 1 must remain the opening fence (#4096 defect 2). + assert.ok(content.startsWith('---\n'), 'opening fence must stay on line 1'); + const lines = splitLines(content); + const closeIdx = lines.indexOf('---', 1); + assert.ok(closeIdx > 0, 'closing fence must exist'); + const fm = lines.slice(1, closeIdx); + assert.ok(fm.includes('status: completed'), 'status: completed must be inside the fence'); + assert.strictEqual(fm.filter(l => /^completed: /.test(l)).length, 1, + 'exactly one completed: line, inside the fence'); + // Other frontmatter keys survive. + assert.ok(fm.some(l => l.startsWith('title:')), 'existing keys preserved'); + // Body preserved after the block. + assert.ok(content.includes('# body'), 'body preserved'); + }); + + test('upserts an existing completed field instead of duplicating it', () => { + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + fs.writeFileSync( + path.join(pendingDir, 'recomplete-probe.md'), + '---\ntitle: recomplete\nstatus: pending\ncompleted: 2020-01-01\n---\n\n# body\n' + ); + + const result = runGsdTools('todo complete recomplete-probe.md', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const content = fs.readFileSync( + path.join(tmpDir, '.planning', 'todos', 'completed', 'recomplete-probe.md'), + 'utf-8' + ); + assert.ok(content.startsWith('---\n'), 'opening fence must stay on line 1'); + assert.ok(!content.includes('completed: 2020-01-01'), 'stale completed value replaced'); + assert.strictEqual( + (content.match(/^completed: /gm) || []).length, 1, + 'exactly one completed: occurrence' + ); + }); + + test('wraps a frontmatter-less todo in a complete frontmatter block', () => { + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + fs.writeFileSync(path.join(pendingDir, 'bare.md'), '# just a body\n'); + + const result = runGsdTools('todo complete bare.md', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const content = fs.readFileSync( + path.join(tmpDir, '.planning', 'todos', 'completed', 'bare.md'), + 'utf-8' + ); + // #4096 fix-2: no bare prefix line — a complete frontmatter block instead. + assert.ok(content.startsWith('---\n'), 'frontmatter block must open the file'); + const lines = splitLines(content); + const closeIdx = lines.indexOf('---', 1); + assert.ok(closeIdx > 0, 'closing fence must exist'); + const fm = lines.slice(1, closeIdx); + assert.ok(fm.some(l => /^completed: \d{4}-\d{2}-\d{2}$/.test(l)), 'completed inside block'); + assert.ok(fm.includes('status: completed'), 'status: completed inside block'); + assert.ok(content.includes('# just a body'), 'body preserved'); + }); + + test('rejects unknown flags instead of silently completing', () => { + const pendingDir = path.join(tmpDir, '.planning', 'todos', 'pending'); + fs.mkdirSync(pendingDir, { recursive: true }); + fs.writeFileSync(path.join(pendingDir, 'flag-probe.md'), '---\nstatus: pending\n---\n'); + + const result = runGsdTools('todo complete flag-probe.md --bogus-flag', tmpDir); + assert.ok(!result.success, 'unknown flag must fail loudly'); + assert.ok(result.error.includes('Unknown flag'), 'error mentions Unknown flag'); + // And crucially: nothing moved. + assert.ok( + fs.existsSync(path.join(pendingDir, 'flag-probe.md')), + 'file must not move when a flag is rejected' + ); + }); }); // ─────────────────────────────────────────────────────────────────────────────