Merge pull request #3670 from Tibsfox/fix/needs-remediation-revalidation

fix(gsd): force re-validation when verdict is needs-remediation

src/resources/extensions/gsd/state.ts

@@ -647,12 +647,15 @@ export async function deriveStateFromDb(basePath: string): Promise<GSDState> {
     const validationFile = resolveMilestoneFile(basePath, activeMilestone.id, "VALIDATION");
     const validationContent = validationFile ? await loadFile(validationFile) : null;
     const validationTerminal = validationContent ? isValidationTerminal(validationContent) : false;
+    const verdict = validationContent ? extractVerdict(validationContent) : undefined;
     const sliceProgress = {
       done: activeMilestoneSlices.length,
       total: activeMilestoneSlices.length,
     };
 
-    if (!validationTerminal) {
+    // Force re-validation when verdict is needs-remediation — remediation slices
+    // may have completed since the stale validation was written (#3596).
+    if (!validationTerminal || verdict === 'needs-remediation') {
       return {
         activeMilestone, activeSlice: null, activeTask: null,
         phase: 'validating-milestone',
@@ -1148,22 +1151,25 @@ export async function _deriveStateImpl(basePath: string): Promise<GSDState> {
       const validationFile = resolveMilestoneFile(basePath, mid, "VALIDATION");
       const validationContent = validationFile ? await cachedLoadFile(validationFile) : null;
       const validationTerminal = validationContent ? isValidationTerminal(validationContent) : false;
+      const verdict = validationContent ? extractVerdict(validationContent) : undefined;
+      // needs-remediation is terminal but requires re-validation (#3596)
+      const needsRevalidation = !validationTerminal || verdict === 'needs-remediation';
 
       if (summaryFile) {
         // Summary exists → milestone is complete regardless of validation state.
         // The summary is the terminal artifact (#864).
         registry.push({ id: mid, title, status: 'complete' });
-      } else if (!validationTerminal && !activeMilestoneFound) {
-        // No summary and no terminal validation → validating-milestone
+      } else if (needsRevalidation && !activeMilestoneFound) {
+        // No summary and needs (re-)validation → validating-milestone
         activeMilestone = { id: mid, title };
         activeRoadmap = roadmap;
         activeMilestoneFound = true;
         registry.push({ id: mid, title, status: 'active' });
-      } else if (!validationTerminal && activeMilestoneFound) {
-        // No summary and no terminal validation, but another milestone is already active
+      } else if (needsRevalidation && activeMilestoneFound) {
+        // Needs (re-)validation, but another milestone is already active
         registry.push({ id: mid, title, status: 'pending' });
       } else if (!activeMilestoneFound) {
-        // Terminal validation but no summary → completing-milestone
+        // Terminal validation (pass/needs-attention) but no summary → completing-milestone
         activeMilestone = { id: mid, title };
         activeRoadmap = roadmap;
         activeMilestoneFound = true;
@@ -1348,12 +1354,15 @@ export async function _deriveStateImpl(basePath: string): Promise<GSDState> {
     const validationFile = resolveMilestoneFile(basePath, activeMilestone.id, "VALIDATION");
     const validationContent = validationFile ? await cachedLoadFile(validationFile) : null;
     const validationTerminal = validationContent ? isValidationTerminal(validationContent) : false;
+    const verdict = validationContent ? extractVerdict(validationContent) : undefined;
     const sliceProgress = {
       done: activeRoadmap.slices.length,
       total: activeRoadmap.slices.length,
     };
 
-    if (!validationTerminal) {
+    // Force re-validation when verdict is needs-remediation — remediation slices
+    // may have completed since the stale validation was written (#3596).
+    if (!validationTerminal || verdict === 'needs-remediation') {
       return {
         activeMilestone,
         activeSlice: null,

src/resources/extensions/gsd/tests/needs-remediation-revalidation.test.ts

@@ -0,0 +1,48 @@
+/**
+ * Regression test for #3670 — needs-remediation verdict forces re-validation
+ *
+ * When validation returns needs-remediation, the state machine must route
+ * back to validating-milestone instead of completing-milestone. Without this,
+ * dispatch blocks completion for needs-remediation while state derives
+ * completing-milestone, creating a permanent deadlock.
+ *
+ * This structural test verifies the verdict === 'needs-remediation' guard
+ * exists at all three derivation paths in state.ts.
+ */
+
+import { describe, test } from 'node:test';
+import assert from 'node:assert/strict';
+import { readFileSync } from 'node:fs';
+import { fileURLToPath } from 'node:url';
+import { dirname, join } from 'node:path';
+
+const __filename = fileURLToPath(import.meta.url);
+const __dirname = dirname(__filename);
+
+const source = readFileSync(join(__dirname, '..', 'state.ts'), 'utf-8');
+
+describe('needs-remediation revalidation guard (#3670)', () => {
+  test('verdict === needs-remediation guard exists in state.ts', () => {
+    const matches = source.match(/verdict\s*===\s*['"]needs-remediation['"]/g);
+    assert.ok(matches, 'verdict === "needs-remediation" check must exist in state.ts');
+    assert.ok(matches.length >= 2,
+      `Expected at least 2 needs-remediation guards (deriveStateFromDb + _deriveStateImpl), found ${matches.length}`);
+  });
+
+  test('needsRevalidation variable is derived from verdict', () => {
+    assert.match(source, /needsRevalidation.*=.*verdict\s*===\s*['"]needs-remediation['"]/,
+      'needsRevalidation should incorporate verdict === "needs-remediation"');
+  });
+
+  test('deriveStateFromDb path uses needs-remediation guard', () => {
+    assert.match(source, /!validationTerminal\s*\|\|\s*verdict\s*===\s*['"]needs-remediation['"]/,
+      'deriveStateFromDb should check !validationTerminal || verdict === "needs-remediation"');
+  });
+
+  test('extractVerdict is called on validation content', () => {
+    const extractCalls = source.match(/extractVerdict\(validationContent\)/g);
+    assert.ok(extractCalls, 'extractVerdict should be called on validation content');
+    assert.ok(extractCalls.length >= 2,
+      `Expected at least 2 extractVerdict calls, found ${extractCalls.length}`);
+  });
+});

src/resources/extensions/gsd/tests/validate-milestone.test.ts

@@ -163,16 +163,15 @@ test("deriveState returns completing-milestone when VALIDATION exists with termi
   }
 });
 
-test("deriveState treats needs-remediation as terminal — does not re-enter validating-milestone (#832)", async () => {
+test("deriveState treats needs-remediation as non-terminal — re-enters validating-milestone (#832)", async () => {
   const base = makeTmpBase();
   try {
     writeRoadmap(base, "M001", ALL_DONE_ROADMAP);
     writeValidation(base, "M001", "---\nverdict: needs-remediation\nremediation_round: 0\n---\n\n# Validation\nNeeds fixes.");
 
     const state = await deriveState(base);
-    // needs-remediation is now terminal — milestone needs a SUMMARY to be fully complete
-    // Without SUMMARY, it enters completing-milestone (not validating-milestone)
-    assert.notEqual(state.phase, "validating-milestone");
+    // needs-remediation routes back to validating-milestone for re-validation
+    assert.equal(state.phase, "validating-milestone");
     assert.equal(state.activeMilestone?.id, "M001");
   } finally {
     cleanup(base);