From 973846cdc655e5d9f8809b50269d49394ab1bcc9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 21 Mar 2026 10:54:03 -0400 Subject: [PATCH] fix: reset completion state when post_unit_hooks retry_on signal is consumed (#1746) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit consumeRetryTrigger() cleared the in-memory retry flag but did not undo the doctor's [x] checkbox, delete SUMMARY.md, remove from completedUnits, or delete the retry artifact. On the next loop iteration, deriveState() saw the task as done and advanced past it — silently losing the retry. When consumeRetryTrigger() returns a trigger, the code now: 1. Unchecks [x] → [ ] for the task in PLAN.md 2. Deletes SUMMARY.md for the task 3. Removes the unit from s.completedUnits and flushes to completed-units.json 4. Deletes the retry_on artifact (e.g. NEEDS-REWORK.md) 5. Invalidates caches so deriveState reads fresh disk state Also extends the retry trigger type to include retryArtifact so the consumer knows which artifact to clean up. Fixes #1714 Co-authored-by: Claude Opus 4.6 (1M context) --- .../extensions/gsd/auto-post-unit.ts | 55 ++- .../extensions/gsd/post-unit-hooks.ts | 6 +- .../gsd/tests/retry-state-reset.test.ts | 333 ++++++++++++++++++ 3 files changed, 390 insertions(+), 4 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/retry-state-reset.test.ts diff --git a/src/resources/extensions/gsd/auto-post-unit.ts b/src/resources/extensions/gsd/auto-post-unit.ts index ce6492100..2834aa22b 100644 --- a/src/resources/extensions/gsd/auto-post-unit.ts +++ b/src/resources/extensions/gsd/auto-post-unit.ts @@ -19,6 +19,9 @@ import { resolveSliceFile, resolveTaskFile, resolveMilestoneFile, + resolveTasksDir, + buildTaskFileName, + gsdRoot, } from "./paths.js"; import { invalidateAllCaches } from "./cache.js"; import { closeoutUnit, type CloseoutOptions } from "./auto-unit-closeout.js"; @@ -40,6 +43,7 @@ import { isRetryPending, consumeRetryTrigger, persistHookState, + resolveHookArtifactPath, } from "./post-unit-hooks.js"; import { hasPendingCaptures, loadPendingCaptures } from "./captures.js"; import { debugLog } from "./debug-logger.js"; @@ -50,7 +54,10 @@ import { unitVerb, hideFooter, } from "./auto-dashboard.js"; +import { existsSync, unlinkSync } from "node:fs"; import { join } from "node:path"; +import { uncheckTaskInPlan } from "./undo.js"; +import { atomicWriteSync } from "./atomic-write.js"; /** Throttle STATE.md rebuilds — at most once per 30 seconds */ const STATE_REBUILD_MIN_INTERVAL_MS = 30_000; @@ -403,9 +410,55 @@ export async function postUnitPostVerification(pctx: PostUnitContext): Promise<" const trigger = consumeRetryTrigger(); if (trigger) { ctx.ui.notify( - `Hook requested retry of ${trigger.unitType} ${trigger.unitId}.`, + `Hook requested retry of ${trigger.unitType} ${trigger.unitId} — resetting task state.`, "info", ); + + // ── State reset: undo the completion so deriveState re-derives the unit ── + try { + const parts = trigger.unitId.split("/"); + const [mid, sid, tid] = parts; + + // 1. Uncheck [x] → [ ] in PLAN.md + if (mid && sid && tid) { + uncheckTaskInPlan(s.basePath, mid, sid, tid); + } + + // 2. Delete SUMMARY.md for the task + if (mid && sid && tid) { + const tasksDir = resolveTasksDir(s.basePath, mid, sid); + if (tasksDir) { + const summaryFile = join(tasksDir, buildTaskFileName(tid, "SUMMARY")); + if (existsSync(summaryFile)) { + unlinkSync(summaryFile); + } + } + } + + // 3. Remove from s.completedUnits and flush to completed-units.json + s.completedUnits = s.completedUnits.filter( + u => !(u.type === trigger.unitType && u.id === trigger.unitId), + ); + try { + const completedKeysPath = join(gsdRoot(s.basePath), "completed-units.json"); + const keys = s.completedUnits.map(u => `${u.type}/${u.id}`); + atomicWriteSync(completedKeysPath, JSON.stringify(keys, null, 2)); + } catch { /* non-fatal: disk flush failure */ } + + // 4. Delete the retry_on artifact (e.g. NEEDS-REWORK.md) + if (trigger.retryArtifact) { + const retryArtifactPath = resolveHookArtifactPath(s.basePath, trigger.unitId, trigger.retryArtifact); + if (existsSync(retryArtifactPath)) { + unlinkSync(retryArtifactPath); + } + } + + // 5. Invalidate caches so deriveState reads fresh disk state + invalidateAllCaches(); + } catch (e) { + debugLog("postUnitPostVerification", { phase: "retry-state-reset", error: String(e) }); + } + // Fall through to normal dispatch — deriveState will re-derive the unit } } diff --git a/src/resources/extensions/gsd/post-unit-hooks.ts b/src/resources/extensions/gsd/post-unit-hooks.ts index c4e598980..95c978749 100644 --- a/src/resources/extensions/gsd/post-unit-hooks.ts +++ b/src/resources/extensions/gsd/post-unit-hooks.ts @@ -34,7 +34,7 @@ const cycleCounts = new Map(); let retryPending = false; /** Stores the trigger unit info for pending retries so caller knows what to re-run. */ -let retryTrigger: { unitType: string; unitId: string } | null = null; +let retryTrigger: { unitType: string; unitId: string; retryArtifact: string } | null = null; // ─── Public API ──────────────────────────────────────────────────────────── @@ -99,7 +99,7 @@ export function isRetryPending(): boolean { * Returns the trigger unit info for a pending retry, or null. * Clears the retry state after reading. */ -export function consumeRetryTrigger(): { unitType: string; unitId: string } | null { +export function consumeRetryTrigger(): { unitType: string; unitId: string; retryArtifact: string } | null { if (!retryPending || !retryTrigger) return null; const trigger = { ...retryTrigger }; retryPending = false; @@ -191,7 +191,7 @@ function handleHookCompletion(basePath: string): HookDispatchResult | null { activeHook = null; hookQueue = []; retryPending = true; - retryTrigger = { unitType: hook.triggerUnitType, unitId: hook.triggerUnitId }; + retryTrigger = { unitType: hook.triggerUnitType, unitId: hook.triggerUnitId, retryArtifact: config.retry_on }; return null; } // Max cycles reached — fall through to normal completion diff --git a/src/resources/extensions/gsd/tests/retry-state-reset.test.ts b/src/resources/extensions/gsd/tests/retry-state-reset.test.ts new file mode 100644 index 000000000..86cc9239f --- /dev/null +++ b/src/resources/extensions/gsd/tests/retry-state-reset.test.ts @@ -0,0 +1,333 @@ +// GSD Extension — Regression tests for #1714: retry_on signal state reset +// +// Verifies that when a post_unit_hook writes a retry_on artifact, the +// consuming code properly resets all completion state so deriveState +// re-derives the task on the next loop iteration. + +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, existsSync, readFileSync, unlinkSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { createTestContext } from "./test-helpers.ts"; +import { + resetHookState, + consumeRetryTrigger, + isRetryPending, + resolveHookArtifactPath, +} from "../post-unit-hooks.ts"; +import { uncheckTaskInPlan } from "../undo.ts"; + +const { assertEq, assertTrue, report } = createTestContext(); + +// ─── Fixture Helpers ─────────────────────────────────────────────────────── + +function createRetryFixture(): { base: string; cleanup: () => void } { + const base = mkdtempSync(join(tmpdir(), "gsd-retry-reset-")); + + // Create the .gsd structure for M001/S01/T01 + // Plan/Summary resolution uses .gsd/milestones/M001/slices/S01/... + const milestonesTasksDir = join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks"); + mkdirSync(milestonesTasksDir, { recursive: true }); + + // Hook artifact resolution uses .gsd/M001/slices/S01/tasks/... + const hookTasksDir = join(base, ".gsd", "M001", "slices", "S01", "tasks"); + mkdirSync(hookTasksDir, { recursive: true }); + + // Write a PLAN.md with T01 checked [x] (as doctor would do) + const planFile = join(base, ".gsd", "milestones", "M001", "slices", "S01", "S01-PLAN.md"); + writeFileSync(planFile, [ + "# S01: Test Slice", + "", + "**Goal:** regression test.", + "", + "## Tasks", + "", + "- [x] **T01: Implement feature** `est:30m`", + "- [ ] **T02: Write tests** `est:15m`", + ].join("\n"), "utf-8"); + + // Write a SUMMARY.md for T01 (in milestones path where resolveTasksDir looks) + const summaryFile = join(milestonesTasksDir, "T01-SUMMARY.md"); + writeFileSync(summaryFile, "---\ntitle: T01 Summary\n---\nDone.", "utf-8"); + + // Write completed-units.json with T01 + writeFileSync( + join(base, ".gsd", "completed-units.json"), + JSON.stringify(["execute-task/M001/S01/T01"]), + "utf-8", + ); + + // Write the retry_on artifact in the hook artifact path + const retryArtifact = join(hookTasksDir, "T01-NEEDS-REWORK.md"); + writeFileSync(retryArtifact, "Rework needed: test coverage insufficient.", "utf-8"); + + return { + base, + cleanup: () => rmSync(base, { recursive: true, force: true }), + }; +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: consumeRetryTrigger returns retryArtifact field +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== consumeRetryTrigger: returns null when no retry pending ==="); + +{ + resetHookState(); + const trigger = consumeRetryTrigger(); + assertEq(trigger, null, "returns null when no retry pending"); +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: uncheckTaskInPlan reverses doctor's [x] mark +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Retry reset step 1: uncheck [x] → [ ] in PLAN.md ==="); + +{ + const { base, cleanup } = createRetryFixture(); + try { + const planFile = join(base, ".gsd", "milestones", "M001", "slices", "S01", "S01-PLAN.md"); + + // Precondition: T01 is checked + const before = readFileSync(planFile, "utf-8"); + assertTrue(before.includes("- [x] **T01:"), "precondition: T01 is checked [x]"); + + // Step 1: Uncheck T01 + const result = uncheckTaskInPlan(base, "M001", "S01", "T01"); + assertTrue(result, "uncheckTaskInPlan returns true"); + + // Verify T01 is now unchecked + const after = readFileSync(planFile, "utf-8"); + assertTrue(after.includes("- [ ] **T01:"), "T01 is now unchecked [ ]"); + assertTrue(!after.includes("- [x] **T01:"), "T01 no longer has [x]"); + + // T02 is unaffected + assertTrue(after.includes("- [ ] **T02:"), "T02 remains unchanged"); + } finally { + cleanup(); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: Delete SUMMARY.md for the task +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Retry reset step 2: delete SUMMARY.md ==="); + +{ + const { base, cleanup } = createRetryFixture(); + try { + const summaryFile = join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks", "T01-SUMMARY.md"); + + // Precondition: SUMMARY exists + assertTrue(existsSync(summaryFile), "precondition: SUMMARY.md exists"); + + // Step 2: Delete SUMMARY.md + unlinkSync(summaryFile); + assertTrue(!existsSync(summaryFile), "SUMMARY.md deleted"); + } finally { + cleanup(); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: Remove from completedUnits array and flush +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Retry reset step 3: remove from completedUnits ==="); + +{ + const { base, cleanup } = createRetryFixture(); + try { + // Simulate the completedUnits array (as AutoSession would have it) + const completedUnits = [ + { type: "execute-task", id: "M001/S01/T01", startedAt: 1000, finishedAt: 2000 }, + { type: "execute-task", id: "M001/S01/T02", startedAt: 3000, finishedAt: 4000 }, + ]; + + // Step 3: Filter out the retried unit + const filtered = completedUnits.filter( + u => !(u.type === "execute-task" && u.id === "M001/S01/T01"), + ); + + assertEq(filtered.length, 1, "one unit removed from completedUnits"); + assertEq(filtered[0].id, "M001/S01/T02", "T02 still in completedUnits"); + + // Flush to completed-units.json + const completedKeysPath = join(base, ".gsd", "completed-units.json"); + const keys = filtered.map(u => `${u.type}/${u.id}`); + writeFileSync(completedKeysPath, JSON.stringify(keys, null, 2), "utf-8"); + + const onDisk = JSON.parse(readFileSync(completedKeysPath, "utf-8")); + assertEq(onDisk.length, 1, "completed-units.json has one entry"); + assertEq(onDisk[0], "execute-task/M001/S01/T02", "only T02 remains in completed-units.json"); + } finally { + cleanup(); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: Delete the retry_on artifact +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Retry reset step 4: delete retry_on artifact ==="); + +{ + const { base, cleanup } = createRetryFixture(); + try { + const retryArtifactPath = resolveHookArtifactPath(base, "M001/S01/T01", "NEEDS-REWORK.md"); + + // Precondition: artifact exists + assertTrue(existsSync(retryArtifactPath), "precondition: retry artifact exists"); + + // Step 4: Delete retry artifact + unlinkSync(retryArtifactPath); + assertTrue(!existsSync(retryArtifactPath), "retry artifact deleted"); + } finally { + cleanup(); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: Full retry reset sequence (all steps together) +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Full retry reset: all steps combined ==="); + +{ + const { base, cleanup } = createRetryFixture(); + try { + const trigger = { + unitType: "execute-task", + unitId: "M001/S01/T01", + retryArtifact: "NEEDS-REWORK.md", + }; + + const parts = trigger.unitId.split("/"); + const [mid, sid, tid] = parts; + + // Simulate completedUnits + let completedUnits = [ + { type: "execute-task", id: "M001/S01/T01", startedAt: 1000, finishedAt: 2000 }, + ]; + + // ── Execute the full reset sequence (mirrors auto-post-unit.ts logic) ── + + // Step 1: Uncheck in PLAN + if (mid && sid && tid) { + uncheckTaskInPlan(base, mid, sid, tid); + } + + // Step 2: Delete SUMMARY (in milestones path) + const tasksDir = join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks"); + const summaryFile = join(tasksDir, `${tid}-SUMMARY.md`); + if (existsSync(summaryFile)) { + unlinkSync(summaryFile); + } + + // Step 3: Remove from completedUnits + flush + completedUnits = completedUnits.filter( + u => !(u.type === trigger.unitType && u.id === trigger.unitId), + ); + const completedKeysPath = join(base, ".gsd", "completed-units.json"); + writeFileSync(completedKeysPath, JSON.stringify( + completedUnits.map(u => `${u.type}/${u.id}`), + null, 2, + ), "utf-8"); + + // Step 4: Delete retry artifact + const retryArtifactPath = resolveHookArtifactPath(base, trigger.unitId, trigger.retryArtifact); + if (existsSync(retryArtifactPath)) { + unlinkSync(retryArtifactPath); + } + + // ── Verify all state is reset ── + + // PLAN.md: T01 unchecked + const planFile = join(base, ".gsd", "milestones", "M001", "slices", "S01", "S01-PLAN.md"); + const planContent = readFileSync(planFile, "utf-8"); + assertTrue(planContent.includes("- [ ] **T01:"), "after reset: T01 unchecked in PLAN"); + assertTrue(!planContent.includes("- [x] **T01:"), "after reset: T01 not checked in PLAN"); + + // SUMMARY.md: deleted + assertTrue(!existsSync(summaryFile), "after reset: SUMMARY.md deleted"); + + // completed-units.json: empty + const onDisk = JSON.parse(readFileSync(completedKeysPath, "utf-8")); + assertEq(onDisk.length, 0, "after reset: completed-units.json is empty"); + + // Retry artifact: deleted + assertTrue(!existsSync(retryArtifactPath), "after reset: retry artifact deleted"); + } finally { + cleanup(); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: Reset is idempotent — no crash when artifacts are already missing +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== Retry reset: idempotent when artifacts already missing ==="); + +{ + const base = mkdtempSync(join(tmpdir(), "gsd-retry-idempotent-")); + try { + // Create minimal structure — NO summary, NO retry artifact, NO plan + mkdirSync(join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks"), { recursive: true }); + writeFileSync( + join(base, ".gsd", "completed-units.json"), + JSON.stringify([]), + "utf-8", + ); + + const trigger = { + unitType: "execute-task", + unitId: "M001/S01/T01", + retryArtifact: "NEEDS-REWORK.md", + }; + + // These should not throw even with missing files + const parts = trigger.unitId.split("/"); + const [mid, sid, tid] = parts; + + // Uncheck — returns false because no PLAN file + const uncheckResult = uncheckTaskInPlan(base, mid, sid, tid); + assertTrue(!uncheckResult, "uncheck returns false when no PLAN exists"); + + // Summary does not exist — no crash + const summaryFile = join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks", `${tid}-SUMMARY.md`); + assertTrue(!existsSync(summaryFile), "no summary to delete — safe"); + + // Retry artifact does not exist — no crash + const retryPath = resolveHookArtifactPath(base, trigger.unitId, trigger.retryArtifact); + assertTrue(!existsSync(retryPath), "no retry artifact to delete — safe"); + + // completed-units.json filter on empty array — safe + const completedUnits: Array<{ type: string; id: string }> = []; + const filtered = completedUnits.filter( + u => !(u.type === trigger.unitType && u.id === trigger.unitId), + ); + assertEq(filtered.length, 0, "filter on empty array is safe"); + } finally { + rmSync(base, { recursive: true, force: true }); + } +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Test: resolveHookArtifactPath produces correct path for retry artifacts +// ═══════════════════════════════════════════════════════════════════════════ + +console.log("\n=== resolveHookArtifactPath: correct path for retry artifacts ==="); + +{ + const base = "/project"; + const path = resolveHookArtifactPath(base, "M001/S01/T01", "NEEDS-REWORK.md"); + assertEq( + path, + join(base, ".gsd", "M001", "slices", "S01", "tasks", "T01-NEEDS-REWORK.md"), + "retry artifact path resolves to task directory with task prefix", + ); +} + +report();