diff --git a/src/resources/extensions/sf/state.js b/src/resources/extensions/sf/state.js index 343bb62e5..55d933bac 100644 --- a/src/resources/extensions/sf/state.js +++ b/src/resources/extensions/sf/state.js @@ -182,6 +182,25 @@ function buildDbRecoveryRequiredState() { progress: { milestones: { done: 0, total: 0 } }, }; } + +/** + * Load SUMMARY content for a milestone and return it only if it is readable + * AND classifies as terminal (success/unknown, not failure). + * + * Purpose: centralise the fail-closed SUMMARY check so the pattern + * "only treat a SUMMARY as terminal if we can read it AND it passes the + * classifier" cannot drift between the five call sites in state derivation. + * Returning null on either an unreadable file or a failure summary ensures + * that a corrupt or failure SUMMARY never silently marks a milestone complete. + * + * Consumer: getActiveMilestoneId() and _deriveStateImpl() Phase 1 + Phase 2. + */ +async function loadTerminalSummary(summaryFile, loadFn) { + if (!summaryFile) return null; + const sc = await loadFn(summaryFile); + if (sc == null || !isTerminalMilestoneSummaryContent(sc)) return null; + return sc; +} /** * Invalidate the deriveState() cache. Call this whenever planning files on disk * may have changed (unit completion, merges, file writes). @@ -237,22 +256,14 @@ export async function getActiveMilestoneId(basePath) { const content = roadmapFile ? await loadFile(roadmapFile) : null; if (!content) { const summaryFile = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (summaryFile) { - // Fail-closed: only skip if SUMMARY is readable AND terminal (not a failure report). - const sc = await loadFile(summaryFile); - if (sc != null && isTerminalMilestoneSummaryContent(sc)) continue; - } + if (await loadTerminalSummary(summaryFile, loadFile)) continue; if (isGhostMilestone(basePath, mid)) continue; return mid; } const roadmap = parseRoadmap(content); if (!isMilestoneComplete(roadmap)) { const summaryFile = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (summaryFile) { - // Fail-closed: only treat as complete if readable AND terminal. - const sc = await loadFile(summaryFile); - if (sc != null && isTerminalMilestoneSummaryContent(sc)) continue; - } + if (await loadTerminalSummary(summaryFile, loadFile)) continue; return mid; } } @@ -1228,12 +1239,8 @@ export async function _deriveStateImpl(basePath) { const rc = rf ? await cachedLoadFile(rf) : null; if (!rc) { const sf = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (sf) { - const sc = await cachedLoadFile(sf); - // Fail-closed: only count as complete if SUMMARY is readable AND terminal. - if (sc && isTerminalMilestoneSummaryContent(sc)) - completeMilestoneIds.add(mid); - } + if (await loadTerminalSummary(sf, cachedLoadFile)) + completeMilestoneIds.add(mid); continue; } const rmap = parseRoadmap(rc); @@ -1242,21 +1249,13 @@ export async function _deriveStateImpl(basePath) { // Summary is the terminal artifact — if it exists and is terminal, the milestone is // complete even when roadmap checkboxes weren't ticked (#864). const sf = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (sf) { - const sc = await cachedLoadFile(sf); - // Fail-closed: only count as complete if SUMMARY is readable AND terminal. - if (sc && isTerminalMilestoneSummaryContent(sc)) - completeMilestoneIds.add(mid); - } + if (await loadTerminalSummary(sf, cachedLoadFile)) + completeMilestoneIds.add(mid); continue; } const sf = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (sf) { - const sc = await cachedLoadFile(sf); - // Fail-closed: only count as complete if SUMMARY is readable AND terminal. - if (sc && isTerminalMilestoneSummaryContent(sc)) - completeMilestoneIds.add(mid); - } + if (await loadTerminalSummary(sf, cachedLoadFile)) + completeMilestoneIds.add(mid); } // Phase 2: Build registry using cached roadmaps (no re-parsing or re-reading) const registry = []; @@ -1276,20 +1275,14 @@ export async function _deriveStateImpl(basePath) { if (!roadmap) { // No roadmap — check if a terminal summary exists (completed milestone without roadmap) const summaryFile = resolveMilestoneFile(basePath, mid, "SUMMARY"); - if (summaryFile) { - const summaryContent = await cachedLoadFile(summaryFile); - // Fail-closed: only mark complete if SUMMARY is readable AND terminal. - if ( - summaryContent && - isTerminalMilestoneSummaryContent(summaryContent) - ) { - const summaryTitle = parseSummary(summaryContent).title || mid; - registry.push({ id: mid, title: summaryTitle, status: "complete" }); - completeMilestoneIds.add(mid); - continue; - } - // Failure summary or unreadable — milestone is not yet done; fall through + const sc = await loadTerminalSummary(summaryFile, cachedLoadFile); + if (sc) { + const summaryTitle = parseSummary(sc).title || mid; + registry.push({ id: mid, title: summaryTitle, status: "complete" }); + completeMilestoneIds.add(mid); + continue; } + // Failure summary or unreadable — milestone is not yet done; fall through // Ghost milestone (only META.json, no CONTEXT/ROADMAP/SUMMARY) — skip entirely if (isGhostMilestone(basePath, mid)) continue; // No roadmap and no summary — treat as incomplete/active @@ -1348,19 +1341,12 @@ export async function _deriveStateImpl(basePath) { // needs-remediation is terminal but requires re-validation (#3596) const needsRevalidation = !validationTerminal || verdict === "needs-remediation"; - if (summaryFile) { - const summaryContent = await cachedLoadFile(summaryFile); - // Fail-closed: only mark complete if SUMMARY is readable AND terminal. - if ( - summaryContent && - isTerminalMilestoneSummaryContent(summaryContent) - ) { - // Terminal summary → milestone is complete. The summary is the terminal artifact (#864). - registry.push({ id: mid, title, status: "complete" }); - continue; - } - // Failure summary or unreadable — fall through to re-validation / active logic below + if (await loadTerminalSummary(summaryFile, cachedLoadFile)) { + // Terminal summary → milestone is complete. The summary is the terminal artifact (#864). + registry.push({ id: mid, title, status: "complete" }); + continue; } + // Failure summary or unreadable — fall through to re-validation / active logic below if (needsRevalidation && !activeMilestoneFound) { // No terminal summary and needs (re-)validation → validating-milestone activeMilestone = { id: mid, title }; @@ -1383,15 +1369,7 @@ export async function _deriveStateImpl(basePath) { // Roadmap slices not all checked — but if a terminal summary exists, the // milestone is still complete. The summary is the terminal artifact (#864). const summaryFile = resolveMilestoneFile(basePath, mid, "SUMMARY"); - const summaryContent = summaryFile - ? await cachedLoadFile(summaryFile) - : null; - // Fail-closed: only mark complete if SUMMARY is readable AND terminal. - if ( - summaryFile && - summaryContent && - isTerminalMilestoneSummaryContent(summaryContent) - ) { + if (await loadTerminalSummary(summaryFile, cachedLoadFile)) { registry.push({ id: mid, title, status: "complete" }); } else if (!activeMilestoneFound) { // Check milestone-level dependencies before promoting to active.