diff --git a/.changeset/derive-status-heals-stale-draft-pin.md b/.changeset/derive-status-heals-stale-draft-pin.md new file mode 100644 index 0000000..c4ede40 --- /dev/null +++ b/.changeset/derive-status-heals-stale-draft-pin.md @@ -0,0 +1,5 @@ +--- +'@zettelgeist/core': patch +--- + +`deriveStatus` now ignores a `status: draft` frontmatter override when the spec has at least one counted task and every counted task is checked. The override is provably stale in that case (the board's "+" button writes it on new-card creation and nothing clears it afterwards). The card transparently moves to `in-review` (or `done` if merged) on the next render — no file mutation, no user action. Other override values (`planned`, `in-progress`, `in-review`, `done`, `blocked`, `cancelled`) and `draft` overrides with partial or zero progress are unchanged. diff --git a/.changeset/hook-noop-when-config-missing.md b/.changeset/hook-noop-when-config-missing.md new file mode 100644 index 0000000..1b7c3a7 --- /dev/null +++ b/.changeset/hook-noop-when-config-missing.md @@ -0,0 +1,5 @@ +--- +'@zettelgeist/cli': patch +--- + +Pre-commit hook now self-disables in repos that aren't zettelgeist repos. Previously, a stale install left over from a removed config (or a partial init) would block every commit with `error: not a zettelgeist repo`. The installed hook block now exits 0 silently when `.zettelgeist.yaml` is missing. Re-run `zettelgeist install-hook` (or `zettelgeist init`) to update existing hooks. diff --git a/.changeset/tick-task-clears-draft-override.md b/.changeset/tick-task-clears-draft-override.md new file mode 100644 index 0000000..3a99fe4 --- /dev/null +++ b/.changeset/tick-task-clears-draft-override.md @@ -0,0 +1,5 @@ +--- +'@zettelgeist/mcp-server': patch +--- + +`tick_task` now clears a stale `status: draft` frontmatter override on `requirements.md` as part of the same commit. This unblocks specs created from the board's "+" column button (which pins `status: draft`) from staying stuck in the draft column after every task is ticked. Other override values (`blocked`, `cancelled`, `planned`, `in-progress`, `in-review`, `done`) are left untouched — they may reflect explicit user intent. `untick_task` does not clear the override, so the draft column reset use-case still works. diff --git a/packages/core/src/status.ts b/packages/core/src/status.ts index b5c2272..a1bd8a4 100644 --- a/packages/core/src/status.ts +++ b/packages/core/src/status.ts @@ -19,14 +19,26 @@ export function deriveStatus(spec: Spec, repo: RepoState): Status { // blocked/cancelled. Board drag-to-column writes this field; ignoring the // override here would render those drags invisible (the card snaps back). const fm = spec.frontmatter.status; - if (typeof fm === 'string' && VALID_STATUSES.has(fm as Status)) { - return fm as Status; - } - const counted = spec.tasks.filter(isCounted); const claimed = repo.claimedSpecs.has(spec.name); const merged = repo.mergedSpecs.has(spec.name); + if (typeof fm === 'string' && VALID_STATUSES.has(fm as Status)) { + // Self-heal a stale `status: draft` override. The board's "+" button + // writes `status: draft` so a new card lands in the draft column; + // nothing clears it once the user starts (and finishes) the work, so + // before this rule, a spec with every task ticked could remain pinned + // to "draft" indefinitely. We ignore the override only when it is + // provably wrong — `draft` AND at least one counted task exists AND + // every counted task is checked — so partial-progress pins (which + // may reflect deliberate user intent) are still honoured. Other + // override values (planned/in-progress/in-review/done/blocked/ + // cancelled) are never auto-cleared here. + const isStaleDraftPin = + fm === 'draft' && counted.length > 0 && counted.every((t) => t.checked); + if (!isStaleDraftPin) return fm as Status; + } + if (counted.length === 0) { // No counted tasks. A live claim still bumps to in-progress. return claimed ? 'in-progress' : 'draft'; diff --git a/packages/core/tests/status.test.ts b/packages/core/tests/status.test.ts index 7f841bb..bea5096 100644 --- a/packages/core/tests/status.test.ts +++ b/packages/core/tests/status.test.ts @@ -133,6 +133,78 @@ describe('deriveStatus', () => { ).toBe('in-review'); }); + it('self-heals a `status: draft` override when every counted task is checked (unmerged)', () => { + // The board's "+" button pins `status: draft` so new cards appear in + // the draft column. Without a self-heal, the override beats the + // derived status forever — even after the user ticks every task. + expect( + deriveStatus( + spec({ + frontmatter: { status: 'draft' }, + tasks: [ + { index: 1, checked: true, text: 'a', tags: [] }, + { index: 2, checked: true, text: 'b', tags: [] }, + ], + }), + emptyRepoState, + ), + ).toBe('in-review'); + }); + + it('self-healed draft override goes to "done" when also merged', () => { + expect( + deriveStatus( + spec({ + name: 'foo', + frontmatter: { status: 'draft' }, + tasks: [{ index: 1, checked: true, text: 'a', tags: [] }], + }), + { claimedSpecs: new Set(), mergedSpecs: new Set(['foo']) }, + ), + ).toBe('done'); + }); + + it('respects a `status: draft` override when only SOME tasks are checked', () => { + // Partial progress might be deliberate ("I want to rethink this; keep + // it on the draft column"), so we only self-heal at all-done. + expect( + deriveStatus( + spec({ + frontmatter: { status: 'draft' }, + tasks: [ + { index: 1, checked: true, text: 'a', tags: [] }, + { index: 2, checked: false, text: 'b', tags: [] }, + ], + }), + emptyRepoState, + ), + ).toBe('draft'); + }); + + it('respects a `status: draft` override when there are no counted tasks', () => { + // Matches conformance fixture 19-all-statuses/a-draft: status:draft + // override on a spec with no tasks at all stays "draft". + expect( + deriveStatus(spec({ frontmatter: { status: 'draft' } }), emptyRepoState), + ).toBe('draft'); + }); + + it('does NOT self-heal other override values even when all tasks are checked', () => { + // The self-heal is narrowly for `draft` — other overrides may reflect + // intentional user state and must not be silently re-derived. + for (const s of ['planned', 'in-progress', 'in-review', 'done', 'blocked', 'cancelled'] as const) { + expect( + deriveStatus( + spec({ + frontmatter: { status: s }, + tasks: [{ index: 1, checked: true, text: 'a', tags: [] }], + }), + emptyRepoState, + ), + ).toBe(s); + } + }); + it('returns "done" when all non-#skip tasks ticked and merged', () => { expect( deriveStatus( diff --git a/packages/git-hook/src/install-hook.ts b/packages/git-hook/src/install-hook.ts index 74f9eaf..32791cc 100644 --- a/packages/git-hook/src/install-hook.ts +++ b/packages/git-hook/src/install-hook.ts @@ -8,8 +8,14 @@ export const HOOK_MARKER_END = '# <<< zettelgeist <<<'; // Resolve the zettelgeist binary at hook execution time. Pre-commit hooks // run with the user's login PATH, which won't include ./node_modules/.bin — // so we fall back to the workspace-local binary if PATH lookup misses. +// +// The leading `.zettelgeist.yaml` guard makes the hook self-disabling in +// repos that aren't zettelgeist repos: a stale install left over from a +// removed config, or a partial init, would otherwise block every commit +// with `error: not a zettelgeist repo`. export const HOOK_BLOCK = HOOK_MARKER_BEGIN + '\n' + + '[ -f .zettelgeist.yaml ] || exit 0\n' + 'if command -v zettelgeist >/dev/null 2>&1; then\n' + ' zettelgeist regen --check\n' + 'elif [ -x ./node_modules/.bin/zettelgeist ]; then\n' + diff --git a/packages/git-hook/tests/install-hook.test.ts b/packages/git-hook/tests/install-hook.test.ts index 71d59d4..98beab0 100644 --- a/packages/git-hook/tests/install-hook.test.ts +++ b/packages/git-hook/tests/install-hook.test.ts @@ -50,6 +50,35 @@ describe('mergeHookContent', () => { expect(HOOK_BLOCK).toContain('command -v zettelgeist'); expect(HOOK_BLOCK).toContain('./node_modules/.bin/zettelgeist'); }); + + it('HOOK_BLOCK self-disables when .zettelgeist.yaml is missing', () => { + expect(HOOK_BLOCK).toContain('[ -f .zettelgeist.yaml ] || exit 0'); + }); +}); + +describe('HOOK_BLOCK execution', () => { + let tmp: string; + + beforeEach(async () => { + tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'zg-hook-exec-')); + }); + + afterEach(async () => { + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it('exits 0 silently in a directory without .zettelgeist.yaml', async () => { + // We run the block in sh from inside `tmp`, with PATH cleared so the + // command-v fallback would otherwise fail. The pre-flight check must + // short-circuit before any zettelgeist lookup happens. + const { stdout, stderr } = await execFileP( + 'sh', + ['-c', HOOK_BLOCK], + { cwd: tmp, env: { PATH: '/usr/bin:/bin' } }, + ); + expect(stderr).toBe(''); + expect(stdout).toBe(''); + }); }); describe('installPreCommitHook', () => { diff --git a/packages/mcp-server/src/tools/write.ts b/packages/mcp-server/src/tools/write.ts index 9acb653..6260d0b 100644 --- a/packages/mcp-server/src/tools/write.ts +++ b/packages/mcp-server/src/tools/write.ts @@ -61,6 +61,44 @@ export const writeHandoffTool: ToolDef, { comm const TASK_LINE = /^([\s>]*[-*+]\s+\[)([ xX])(\]\s+.*)$/; +/** + * Compute the new contents of `requirements.md` with a `status: draft` + * frontmatter override stripped, without touching the disk. Returns + * `null` when there is nothing to clear (no requirements.md, no + * frontmatter, status not draft, or status is one of the user- + * intentional values). The caller stages the returned write through + * `writeFileAndCommit`'s `extraWrites` so the actual fs mutation + * happens atomically alongside the tasks.md tick. + * + * Rationale: `draft` is uniquely the "no work yet" state. A `tick_task` + * is unambiguous evidence that work has started, so a pinned `draft` + * override (typically written by the board's "+" button) is provably + * wrong and would otherwise mask all forward progress. Other override + * values (`planned`, `in-progress`, `in-review`, `done`, `blocked`, + * `cancelled`) may reflect explicit user intent — we leave those alone. + */ +async function plannedDraftOverrideClear( + cwd: string, + specDir: string, +): Promise<{ relPath: string; content: string } | null> { + const reqAbs = safeJoin(specDir, 'requirements.md'); + let raw: string; + try { + raw = await fs.readFile(reqAbs, 'utf8'); + } catch { + return null; + } + const parsed = matter(raw, {}); + const data = { ...(parsed.data ?? {}) } as Record; + if (data.status !== 'draft') return null; + delete data.status; + const newFm = Object.keys(data).length > 0 ? `---\n${yaml.dump(data)}---\n` : ''; + const body = parsed.content.startsWith('\n') ? parsed.content.slice(1) : parsed.content; + const newContent = newFm + body; + const relPath = path.relative(cwd, reqAbs).split(path.sep).join('/'); + return { relPath, content: newContent }; +} + async function tickOrUntick(cwd: string, name: string, n: number, checked: boolean): Promise<{ commit: string }> { const reader = makeDiskFsReader(cwd); const cfg = await loadConfig(reader); @@ -83,13 +121,32 @@ async function tickOrUntick(cwd: string, name: string, n: number, checked: boole } } if (!mutated) throw new Error(`no task at index ${n} in ${name}`); + + // Tick (only) clears a stale `status: draft` override on requirements.md + // so the board doesn't keep the card pinned in the draft column after + // the user has started working. Untick intentionally does NOT clear — + // an untick might be undoing an accidental tick on a draft spec, and + // re-pinning to draft would be the right move there. + const extraWrites: Array<{ relPath: string; content: string }> = []; + if (checked) { + const cleared = await plannedDraftOverrideClear(cwd, specDir); + if (cleared) extraWrites.push(cleared); + } + const op = checked ? 'tick' : 'untick'; + const opts: { + log: { specName: string; action: string }; + extraWrites?: ReadonlyArray<{ relPath: string; content: string }>; + } = { + log: { specName: name, action: `${op}_task(${n})` }, + }; + if (extraWrites.length > 0) opts.extraWrites = extraWrites; return writeFileAndCommit( cwd, tasksRel, lines.join('\n'), `[zg] ${op}: ${name}#${n}`, - { log: { specName: name, action: `${op}_task(${n})` } }, + opts, ); } diff --git a/packages/mcp-server/src/util/write-and-commit.ts b/packages/mcp-server/src/util/write-and-commit.ts index 3766ee3..db9061a 100644 --- a/packages/mcp-server/src/util/write-and-commit.ts +++ b/packages/mcp-server/src/util/write-and-commit.ts @@ -28,6 +28,17 @@ export interface WriteAndCommitOptions { action: string; agentId?: string; }; + /** + * Additional `{ relPath, content }` pairs written and committed + * atomically with the main file. The helper performs the temp+rename + * write BEFORE running conformance, so all files are on disk by the + * time INDEX.md regenerates — keeping the failure mode symmetric with + * the single-file path (a conformance throw leaves a consistent + * working-tree diff rather than a half-applied state). Used by tools + * that need a multi-file atomic edit, e.g. `tick_task` clearing a + * `status: draft` override in requirements.md alongside the tick. + */ + extraWrites?: ReadonlyArray<{ relPath: string; content: string }>; } export async function writeFileAndCommit( @@ -43,6 +54,19 @@ export async function writeFileAndCommit( await fs.writeFile(tmp, content, 'utf8'); await fs.rename(tmp, fileAbs); + // Stage any caller-supplied extra writes the same way (temp+rename) + // BEFORE running conformance, so INDEX.md regenerates against the + // post-write state of every file in this transaction. + if (options?.extraWrites) { + for (const w of options.extraWrites) { + const abs = path.join(cwd, w.relPath); + await fs.mkdir(path.dirname(abs), { recursive: true }); + const t = `${abs}.tmp`; + await fs.writeFile(t, w.content, 'utf8'); + await fs.rename(t, abs); + } + } + // Regen — load config first so we can reuse it for the .log.md path too. const reader = makeDiskFsReader(cwd); const cfg = await loadConfig(reader); @@ -87,6 +111,10 @@ export async function writeFileAndCommit( if (wrote) filesToAdd.push(logRelPath); } + if (options?.extraWrites) { + for (const w of options.extraWrites) filesToAdd.push(w.relPath); + } + await execFileP('git', ['add', ...filesToAdd], { cwd }); await execFileP('git', ['commit', '-m', commitMessage], { cwd }); const { stdout } = await execFileP('git', ['rev-parse', 'HEAD'], { cwd }); diff --git a/packages/mcp-server/tests/tools/write.test.ts b/packages/mcp-server/tests/tools/write.test.ts index bcc8753..30f6ba6 100644 --- a/packages/mcp-server/tests/tools/write.test.ts +++ b/packages/mcp-server/tests/tools/write.test.ts @@ -69,6 +69,78 @@ describe('writeTools', () => { expect(after.startsWith('- [ ] one')).toBe(true); }); + it('tick_task clears a stale `status: draft` override on requirements.md', async () => { + // Simulate the board's "+" button: it pins `status: draft` so the + // new card lands in the draft column. Without auto-clear, ticking + // every task would still show the spec as draft because the + // frontmatter override beats the derived status. + const reqPath = path.join(tmp, 'specs', 'foo', 'requirements.md'); + await fs.writeFile(reqPath, '---\nstatus: draft\ndepends_on: []\n---\n# foo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'pin draft'], { cwd: tmp }); + + await tickTaskTool.handler({ name: 'foo', n: 1 }, { cwd: tmp }); + + const after = await fs.readFile(reqPath, 'utf8'); + expect(after).not.toContain('status:'); + // Other frontmatter fields survive untouched. + expect(after).toContain('depends_on: []'); + expect(after).toContain('# foo'); + }); + + it('tick_task clears a `status: draft`-only frontmatter and drops the fence', async () => { + // When `status: draft` is the only frontmatter key, clearing it + // should produce a file with no frontmatter block at all (the + // dump-empty-then-fence branch in plannedDraftOverrideClear). + const reqPath = path.join(tmp, 'specs', 'foo', 'requirements.md'); + await fs.writeFile(reqPath, '---\nstatus: draft\n---\n# foo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'pin draft alone'], { cwd: tmp }); + + await tickTaskTool.handler({ name: 'foo', n: 1 }, { cwd: tmp }); + + const after = await fs.readFile(reqPath, 'utf8'); + expect(after).not.toContain('---'); + expect(after).not.toContain('status'); + expect(after).toContain('# foo'); + }); + + it('tick_task leaves other status overrides alone', async () => { + // `blocked` is a documented v0.1 frontmatter override. Ticking a + // task on a blocked spec MUST NOT silently un-block it. + const reqPath = path.join(tmp, 'specs', 'foo', 'requirements.md'); + await fs.writeFile(reqPath, '---\nstatus: blocked\nblocked_by: waiting\n---\n# foo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'pin blocked'], { cwd: tmp }); + + await tickTaskTool.handler({ name: 'foo', n: 1 }, { cwd: tmp }); + + const after = await fs.readFile(reqPath, 'utf8'); + expect(after).toContain('status: blocked'); + expect(after).toContain('blocked_by: waiting'); + }); + + it('untick_task does NOT clear a draft override', async () => { + // Untick is symmetrical-undo, not a forward-progress signal — leave + // any pin in place so the user can reset a card back to draft. + const reqPath = path.join(tmp, 'specs', 'foo', 'requirements.md'); + await fs.writeFile(reqPath, '---\nstatus: draft\n---\n# foo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'pin draft'], { cwd: tmp }); + + // First tick clears the override (per previous test); restore it so + // we're testing untick in isolation. + await tickTaskTool.handler({ name: 'foo', n: 1 }, { cwd: tmp }); + await fs.writeFile(reqPath, '---\nstatus: draft\n---\n# foo\n'); + await execFileP('git', ['add', '.'], { cwd: tmp }); + await execFileP('git', ['commit', '-q', '-m', 'restore pin'], { cwd: tmp }); + + await untickTaskTool.handler({ name: 'foo', n: 1 }, { cwd: tmp }); + + const after = await fs.readFile(reqPath, 'utf8'); + expect(after).toContain('status: draft'); + }); + it('writeSpecFileTool rejects relpath with traversal', async () => { await expect(writeSpecFileTool.handler( { name: 'foo', relpath: '../../evil.txt', content: 'pwn' },