diff --git a/src/resources/extensions/sf/markdown-renderer.js b/src/resources/extensions/sf/markdown-renderer.js index 9297d957b..221ba5811 100644 --- a/src/resources/extensions/sf/markdown-renderer.js +++ b/src/resources/extensions/sf/markdown-renderer.js @@ -266,11 +266,12 @@ function renderRoadmapMarkdown(milestone, slices) { lines.push("## Slices"); lines.push(""); for (const slice of slices) { - const done = slice.status === "complete" ? "x" : " "; + const done = isClosedStatus(slice.status) ? "x" : " "; + const skipped = slice.status === "skipped"; const depends = `[${(slice.depends ?? []).join(",")}]`; - lines.push( - `- [${done}] **${slice.id}: ${slice.title}** \`risk:${slice.risk}\` \`depends:${depends}\``, - ); + let line = `- [${done}] **${slice.id}: ${slice.title}** \`risk:${slice.risk}\` \`depends:${depends}\``; + if (skipped) line += " *(skipped)*"; + lines.push(line); lines.push(` > After this: ${slice.demo}`); lines.push(""); } @@ -700,9 +701,10 @@ export async function renderRoadmapFromDb(basePath, milestoneId) { /** * Render roadmap checkbox states from DB. * - * For each slice in the milestone, sets [x] if status === 'complete', - * [ ] otherwise. Handles bidirectional updates (can uncheck previously - * checked slices if DB says pending). + * For each slice in the milestone, sets [x] if isClosedStatus(slice.status) + * (covers "complete", "done", and "skipped"), [ ] otherwise. + * Handles bidirectional updates (can uncheck previously checked slices + * if DB says pending). * * @returns true if the roadmap was written, false on skip/error */ @@ -733,7 +735,7 @@ export async function renderRoadmapCheckboxes(basePath, milestoneId) { // Apply checkbox patches for each slice let updated = content; for (const slice of slices) { - const isDone = slice.status === "complete"; + const isDone = isClosedStatus(slice.status); const sid = slice.id; if (isDone) { // Set [x]: replace "- [ ] **S01:" with "- [x] **S01:" diff --git a/src/resources/extensions/sf/tests/skipped-slice-render.test.mjs b/src/resources/extensions/sf/tests/skipped-slice-render.test.mjs new file mode 100644 index 000000000..d6fd08f91 --- /dev/null +++ b/src/resources/extensions/sf/tests/skipped-slice-render.test.mjs @@ -0,0 +1,175 @@ +/** + * Tests for skipped-slice render behavior. + * + * Purpose: skipped slices must display distinctly from completed slices + * in all roadmap render paths. Covers: + * 1. renderRoadmapMarkdown — inline *(skipped)* annotation for skipped slices + * 2. renderRoadmapCheckboxes — [x] checkbox for skipped (isClosedStatus) + * 3. reassess_roadmap error message — distinguishes skipped from completed + * + * Consumer: human readers of M*-ROADMAP.md and operators reading error output. + */ +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; +import { + closeDatabase, + insertMilestone, + insertSlice, + openDatabase, +} from "../sf-db.js"; +import { renderRoadmapCheckboxes } from "../markdown-renderer.js"; +import { mkdtempSync, rmSync, writeFileSync, mkdirSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { handleReassessRoadmap } from "../tools/reassess-roadmap.js"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const tmpDirs = []; + +function makeProject() { + const dir = mkdtempSync(join(tmpdir(), "sf-skipped-render-")); + tmpDirs.push(dir); + // Create .sf directory and database + mkdirSync(join(dir, ".sf"), { recursive: true }); + openDatabase(join(dir, ".sf", "sf.db")); + return dir; +} + +afterEach(() => { + closeDatabase(); + while (tmpDirs.length > 0) { + rmSync(tmpDirs.pop(), { recursive: true, force: true }); + } +}); + +describe("skipped-slice render", () => { + test("renderRoadmapCheckboxes sets [x] for skipped slices", async () => { + const project = makeProject(); + openDatabase(join(project, ".sf", "sf.db")); + + insertMilestone({ id: "M990", title: "Skipped slice render test", status: "active" }); + insertSlice({ milestoneId: "M990", id: "S01", title: "Done slice", status: "complete", risk: "low", depends: [], demo: "demo" }); + insertSlice({ milestoneId: "M990", id: "S02", title: "Skipped slice", status: "skipped", risk: "low", depends: [], demo: "skipped-demo" }); + insertSlice({ milestoneId: "M990", id: "S03", title: "Pending slice", status: "pending", risk: "low", depends: [], demo: "pending-demo" }); + + // Write a minimal ROADMAP.md with all three unchecked + const roadmapDir = join(project, ".sf", "milestones", "M990"); + mkdirSync(roadmapDir, { recursive: true }); + const roadmapContent = [ + "# M990: Skipped slice render test", + "", + "## Slices", + "", + `- [ ] **S01: Done slice** \`risk:low\` \`depends:[]\``, + `- [ ] **S02: Skipped slice** \`risk:low\` \`depends:[]\``, + `- [ ] **S03: Pending slice** \`risk:low\` \`depends:[]\``, + "", + ].join("\n"); + writeFileSync(join(roadmapDir, "M990-ROADMAP.md"), roadmapContent); + + const result = await renderRoadmapCheckboxes(project, "M990"); + // The function writes directly to disk, so we check the file + // Since we can't get the absPath from renderRoadmapCheckboxes return, + // we use getMilestoneSlices to check that DB state is consistent + // and check the file directly + const roadmapPath = join(roadmapDir, "M990-ROADMAP.md"); + const content = readFileSync(roadmapPath, "utf-8"); + + // S01 and S02 are isClosedStatus → [x]; S03 is pending → [ ] + // S01 (complete) gets [x] + expect(content).toContain("[x] **S01:"); + // S02 (skipped) gets [x] via isClosedStatus + expect(content).toContain("[x] **S02:"); + // S03 (pending) stays [ ] + expect(content).toContain("[ ] **S03:"); + }); + + test("renderRoadmapCheckboxes unchecks slice that was marked complete but is now pending", async () => { + const project = makeProject(); + openDatabase(join(project, ".sf", "sf.db")); + + insertMilestone({ id: "M991", title: "Uncheck test", status: "active" }); + // S01 is now pending (was incorrectly marked done) + insertSlice({ milestoneId: "M991", id: "S01", title: "Was done", status: "pending", risk: "low", depends: [], demo: "demo" }); + + const roadmapDir = join(project, ".sf", "milestones", "M991"); + mkdirSync(roadmapDir, { recursive: true }); + const roadmapContent = [ + "# M991: Uncheck test", + "", + "## Slices", + "", + `- [x] **S01: Was done** \`risk:low\` \`depends:[]\``, + "", + ].join("\n"); + writeFileSync(join(roadmapDir, "M991-ROADMAP.md"), roadmapContent); + + await renderRoadmapCheckboxes(project, "M991"); + const roadmapPath = join(roadmapDir, "M991-ROADMAP.md"); + const content = readFileSync(roadmapPath, "utf-8"); + + expect(content).toContain("[ ] **S01:"); + expect(content).not.toContain("[x] **S01:"); + }); + + test("reassess_roadmap error distinguishes skipped from completed", async () => { + const project = makeProject(); + openDatabase(join(project, ".sf", "sf.db")); + + insertMilestone({ id: "M992", title: "Error message test", status: "active" }); + // S01 was skipped (never executed) + insertSlice({ milestoneId: "M992", id: "S01", title: "Done slice", status: "complete", risk: "low", depends: [], demo: "done", sequence: 1 }); + insertSlice({ milestoneId: "M992", id: "S02", title: "Skipped slice", status: "skipped", risk: "low", depends: [], demo: "skipped", sequence: 2 }); + + // Try to modify the skipped slice — should get "slice S02 is skipped" not "cannot modify completed slice" + const result = await handleReassessRoadmap( + { + milestoneId: "M992", + completedSliceId: "S01", + verdict: "roadmap-confirmed", + assessment: "Test assessment", + sliceChanges: { modified: [{ sliceId: "S02", title: "Skipped", risk: "low", depends: [], demo: "" }], added: [], removed: [] }, + }, + project, + ); + + expect(result.error).toBeDefined(); + expect(result.error).toContain("S02 is skipped"); + expect(result.error).not.toContain("cannot modify completed slice"); + expect(result.error).not.toMatch(/completed slice S02/i); + }); + + test("reassess_roadmap error message uses 'skipped' not 'completed' for skipped slices", async () => { + const project = makeProject(); + openDatabase(join(project, ".sf", "sf.db")); + + insertMilestone({ id: "M993", title: "Error label test", status: "active" }); + insertSlice({ milestoneId: "M993", id: "S01", title: "Done slice", status: "complete", risk: "low", depends: [], demo: "done", sequence: 1 }); + insertSlice({ milestoneId: "M993", id: "S02", title: "Skipped slice", status: "skipped", risk: "low", depends: [], demo: "skipped", sequence: 2 }); + + // Try to modify both completed and skipped in the same call + // The skipped one should have "skipped" in error, not "completed" + const result = await handleReassessRoadmap( + { + milestoneId: "M993", + completedSliceId: "S01", + verdict: "roadmap-confirmed", + assessment: "Test", + sliceChanges: { + modified: [ + { sliceId: "S02", title: "Changed", risk: "low", depends: [], demo: "" }, + ], + added: [], + removed: [], + }, + }, + project, + ); + + // Error should distinguish skipped from completed + expect(result.error).toBeDefined(); + expect(result.error).toContain("skipped"); + expect(result.error).toContain("S02"); + expect(result.error).not.toContain("cannot modify completed slice"); + }); +}); \ No newline at end of file diff --git a/src/resources/extensions/sf/tools/reassess-roadmap.js b/src/resources/extensions/sf/tools/reassess-roadmap.js index 147098041..783f569d7 100644 --- a/src/resources/extensions/sf/tools/reassess-roadmap.js +++ b/src/resources/extensions/sf/tools/reassess-roadmap.js @@ -162,23 +162,34 @@ export async function handleReassessRoadmap(rawParams, basePath) { guardError = `completedSliceId ${params.completedSliceId} is not complete (status: ${completedSlice.status}) — reassess can only be called after a slice finishes`; return; } - // Structural enforcement — reject modifications/removal of completed slices + // Structural enforcement — reject modifications/removal of closed slices. + // Distinguish "completed" (executed successfully) from "skipped" + // so error messages are precise and future callers can distinguish + // the two cases for policy decisions. const existingSlices = getMilestoneSlices(params.milestoneId); - const completedSliceIds = new Set(); + const closedSliceIds = new Set(); for (const slice of existingSlices) { if (isClosedStatus(slice.status)) { - completedSliceIds.add(slice.id); + closedSliceIds.add(slice.id); } } for (const modifiedSlice of params.sliceChanges.modified) { - if (completedSliceIds.has(modifiedSlice.sliceId)) { - guardError = `cannot modify completed slice ${modifiedSlice.sliceId}`; + if (closedSliceIds.has(modifiedSlice.sliceId)) { + const target = existingSlices.find((s) => s.id === modifiedSlice.sliceId); + const statusLabel = target?.status === "skipped" + ? "skipped" + : "completed"; + guardError = `slice ${modifiedSlice.sliceId} is ${statusLabel} — cannot apply metadata modifications`; return; } } for (const removedId of params.sliceChanges.removed) { - if (completedSliceIds.has(removedId)) { - guardError = `cannot remove completed slice ${removedId}`; + if (closedSliceIds.has(removedId)) { + const target = existingSlices.find((s) => s.id === removedId); + const statusLabel = target?.status === "skipped" + ? "skipped" + : "completed"; + guardError = `slice ${removedId} is ${statusLabel} — cannot remove`; return; } }