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 <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/calm-pumas-frolic.md
Normal file
5
.changeset/calm-pumas-frolic.md
Normal file
@@ -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)
|
||||
@@ -1161,8 +1161,17 @@ active window are still outstanding.
|
||||
|
||||
```bash
|
||||
# Complete a todo
|
||||
node gsd-tools.cjs todo complete <filename>
|
||||
node gsd-tools.cjs todo complete <filename> [--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
|
||||
|
||||
|
||||
@@ -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 <filename>
|
||||
node gsd-tools.cjs todo complete <filename> [--dry-run]
|
||||
|
||||
# UAT 監査 — 全フェーズの未解決項目をスキャン
|
||||
node gsd-tools.cjs audit-uat
|
||||
|
||||
@@ -396,7 +396,7 @@ node gsd-tools.cjs progress [json|table|bar]
|
||||
node gsd-tools.cjs progress --json
|
||||
|
||||
# 할 일 완료
|
||||
node gsd-tools.cjs todo complete <filename>
|
||||
node gsd-tools.cjs todo complete <filename> [--dry-run]
|
||||
|
||||
# UAT 감사 — 모든 단계에서 미해결 항목 스캔
|
||||
node gsd-tools.cjs audit-uat
|
||||
|
||||
@@ -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 <filename>
|
||||
node gsd-tools.cjs todo complete <filename> [--dry-run]
|
||||
|
||||
# Auditoria UAT — verifica todas as fases em busca de itens não resolvidos
|
||||
node gsd-tools.cjs audit-uat
|
||||
|
||||
@@ -396,7 +396,7 @@ node gsd-tools.cjs progress [json|table|bar]
|
||||
node gsd-tools.cjs progress --json
|
||||
|
||||
# 完成待办事项
|
||||
node gsd-tools.cjs todo complete <filename>
|
||||
node gsd-tools.cjs todo complete <filename> [--dry-run]
|
||||
|
||||
# UAT 审计——扫描所有阶段的未解决事项
|
||||
node gsd-tools.cjs audit-uat
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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');
|
||||
|
||||
@@ -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'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user