diff --git a/src/resources/extensions/gsd/gsd-db.ts b/src/resources/extensions/gsd/gsd-db.ts index 18bc760ef..9a42646dd 100644 --- a/src/resources/extensions/gsd/gsd-db.ts +++ b/src/resources/extensions/gsd/gsd-db.ts @@ -1183,13 +1183,25 @@ export function insertSlice(s: { }): void { if (!currentDb) throw new GSDError(GSD_STALE_STATE, "gsd-db: No database open"); currentDb.prepare( - `INSERT OR IGNORE INTO slices ( + `INSERT INTO slices ( milestone_id, id, title, status, risk, depends, demo, created_at, goal, success_criteria, proof_level, integration_closure, observability_impact, sequence ) VALUES ( :milestone_id, :id, :title, :status, :risk, :depends, :demo, :created_at, :goal, :success_criteria, :proof_level, :integration_closure, :observability_impact, :sequence - )`, + ) + ON CONFLICT (milestone_id, id) DO UPDATE SET + title = CASE WHEN :raw_title IS NOT NULL THEN excluded.title ELSE slices.title END, + status = CASE WHEN slices.status IN ('complete', 'done') THEN slices.status ELSE excluded.status END, + risk = CASE WHEN :raw_risk IS NOT NULL THEN excluded.risk ELSE slices.risk END, + depends = excluded.depends, + demo = CASE WHEN :raw_demo IS NOT NULL THEN excluded.demo ELSE slices.demo END, + goal = CASE WHEN :raw_goal IS NOT NULL THEN excluded.goal ELSE slices.goal END, + success_criteria = CASE WHEN :raw_success_criteria IS NOT NULL THEN excluded.success_criteria ELSE slices.success_criteria END, + proof_level = CASE WHEN :raw_proof_level IS NOT NULL THEN excluded.proof_level ELSE slices.proof_level END, + integration_closure = CASE WHEN :raw_integration_closure IS NOT NULL THEN excluded.integration_closure ELSE slices.integration_closure END, + observability_impact = CASE WHEN :raw_observability_impact IS NOT NULL THEN excluded.observability_impact ELSE slices.observability_impact END, + sequence = CASE WHEN :raw_sequence IS NOT NULL THEN excluded.sequence ELSE slices.sequence END`, ).run({ ":milestone_id": s.milestoneId, ":id": s.id, @@ -1205,6 +1217,16 @@ export function insertSlice(s: { ":integration_closure": s.planning?.integrationClosure ?? "", ":observability_impact": s.planning?.observabilityImpact ?? "", ":sequence": s.sequence ?? 0, + // Raw sentinel params: NULL when caller omitted the field, used in ON CONFLICT guards + ":raw_title": s.title ?? null, + ":raw_risk": s.risk ?? null, + ":raw_demo": s.demo ?? null, + ":raw_goal": s.planning?.goal ?? null, + ":raw_success_criteria": s.planning?.successCriteria ?? null, + ":raw_proof_level": s.planning?.proofLevel ?? null, + ":raw_integration_closure": s.planning?.integrationClosure ?? null, + ":raw_observability_impact": s.planning?.observabilityImpact ?? null, + ":raw_sequence": s.sequence ?? null, }); } @@ -1884,20 +1906,32 @@ export function reconcileWorktreeDb( FROM wt.milestones `).run()); - // Merge slices — preserve worktree progress (status, summaries, planning) + // Merge slices — preserve worktree progress but never downgrade completed status (#2558). + // Uses INSERT OR REPLACE with a subquery that picks the best status — if the main DB + // already has a completed slice, keep that status even if the worktree copy is stale. merged.slices = countChanges(adapter.prepare(` INSERT OR REPLACE INTO slices ( milestone_id, id, title, status, risk, depends, demo, created_at, completed_at, full_summary_md, full_uat_md, goal, success_criteria, proof_level, integration_closure, observability_impact, sequence, replan_triggered_at ) - SELECT milestone_id, id, title, status, risk, depends, demo, created_at, completed_at, - full_summary_md, full_uat_md, goal, success_criteria, proof_level, - integration_closure, observability_impact, sequence, replan_triggered_at - FROM wt.slices + SELECT w.milestone_id, w.id, w.title, + CASE + WHEN m.status IN ('complete', 'done') AND w.status NOT IN ('complete', 'done') + THEN m.status ELSE w.status + END, + w.risk, w.depends, w.demo, w.created_at, + CASE + WHEN m.status IN ('complete', 'done') AND w.status NOT IN ('complete', 'done') + THEN m.completed_at ELSE w.completed_at + END, + w.full_summary_md, w.full_uat_md, w.goal, w.success_criteria, w.proof_level, + w.integration_closure, w.observability_impact, w.sequence, w.replan_triggered_at + FROM wt.slices w + LEFT JOIN slices m ON m.milestone_id = w.milestone_id AND m.id = w.id `).run()); - // Merge tasks — preserve execution results, status, summaries + // Merge tasks — preserve execution results, never downgrade completed status (#2558) merged.tasks = countChanges(adapter.prepare(` INSERT OR REPLACE INTO tasks ( milestone_id, slice_id, id, title, status, one_liner, narrative, @@ -1906,12 +1940,23 @@ export function reconcileWorktreeDb( description, estimate, files, verify, inputs, expected_output, observability_impact, full_plan_md, sequence ) - SELECT milestone_id, slice_id, id, title, status, one_liner, narrative, - verification_result, duration, completed_at, blocker_discovered, - deviations, known_issues, key_files, key_decisions, full_summary_md, - description, estimate, files, verify, inputs, expected_output, - observability_impact, full_plan_md, sequence - FROM wt.tasks + SELECT w.milestone_id, w.slice_id, w.id, w.title, + CASE + WHEN m.status IN ('complete', 'done') AND w.status NOT IN ('complete', 'done') + THEN m.status ELSE w.status + END, + w.one_liner, w.narrative, + w.verification_result, w.duration, + CASE + WHEN m.status IN ('complete', 'done') AND w.status NOT IN ('complete', 'done') + THEN m.completed_at ELSE w.completed_at + END, + w.blocker_discovered, + w.deviations, w.known_issues, w.key_files, w.key_decisions, w.full_summary_md, + w.description, w.estimate, w.files, w.verify, w.inputs, w.expected_output, + w.observability_impact, w.full_plan_md, w.sequence + FROM wt.tasks w + LEFT JOIN tasks m ON m.milestone_id = w.milestone_id AND m.slice_id = w.slice_id AND m.id = w.id `).run()); // Merge memories — keep worktree-learned insights diff --git a/src/resources/extensions/gsd/tests/insert-slice-no-wipe.test.ts b/src/resources/extensions/gsd/tests/insert-slice-no-wipe.test.ts new file mode 100644 index 000000000..e70e8e166 --- /dev/null +++ b/src/resources/extensions/gsd/tests/insert-slice-no-wipe.test.ts @@ -0,0 +1,88 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { openDatabase, closeDatabase, insertMilestone, insertSlice, getSlice } from '../gsd-db.ts'; + +test('insertSlice with minimal args does not wipe populated fields', (t) => { + t.after(() => { try { closeDatabase(); } catch { /* noop */ } }); + openDatabase(":memory:"); + + insertMilestone({ id: 'M001', title: 'Milestone', status: 'active' }); + + // First insert: full data + insertSlice({ + id: 'S01', + milestoneId: 'M001', + title: 'Auth flow', + status: 'in-progress', + risk: 'high', + demo: 'Login page renders.', + sequence: 3, + planning: { + goal: 'Secure authentication', + successCriteria: 'All tests pass', + proofLevel: 'integration', + integrationClosure: 'Fully integrated', + observabilityImpact: 'Metrics available', + }, + }); + + const before = getSlice('M001', 'S01'); + assert.ok(before, 'slice should exist after first insert'); + assert.equal(before.title, 'Auth flow'); + assert.equal(before.demo, 'Login page renders.'); + assert.equal(before.risk, 'high'); + + // Second insert: minimal "ensure exists" call (mirrors complete-task.ts usage) + insertSlice({ id: 'S01', milestoneId: 'M001' }); + + const after = getSlice('M001', 'S01'); + assert.ok(after, 'slice should still exist after second insert'); + + // These must NOT be wiped to empty strings + assert.equal(after.title, 'Auth flow', 'title must survive minimal re-insert'); + assert.equal(after.demo, 'Login page renders.', 'demo must survive minimal re-insert'); + assert.equal(after.risk, 'high', 'risk must survive minimal re-insert'); + assert.equal(after.sequence, 3, 'sequence must survive minimal re-insert'); + + // Planning fields must also survive + assert.equal(after.goal, 'Secure authentication', 'goal must survive minimal re-insert'); + assert.equal(after.success_criteria, 'All tests pass', 'success_criteria must survive'); + assert.equal(after.proof_level, 'integration', 'proof_level must survive'); + assert.equal(after.integration_closure, 'Fully integrated', 'integration_closure must survive'); + assert.equal(after.observability_impact, 'Metrics available', 'observability_impact must survive'); +}); + +test('insertSlice ON CONFLICT preserves completed status', (t) => { + t.after(() => { try { closeDatabase(); } catch { /* noop */ } }); + openDatabase(":memory:"); + + insertMilestone({ id: 'M001', title: 'Milestone', status: 'active' }); + + insertSlice({ id: 'S01', milestoneId: 'M001', title: 'Done slice', status: 'complete' }); + + // Re-insert with pending status (default) should NOT overwrite complete + insertSlice({ id: 'S01', milestoneId: 'M001' }); + + const after = getSlice('M001', 'S01'); + assert.ok(after); + assert.equal(after.status, 'complete', 'completed status must not be overwritten'); +}); + +test('insertSlice ON CONFLICT allows explicit updates to non-empty values', (t) => { + t.after(() => { try { closeDatabase(); } catch { /* noop */ } }); + openDatabase(":memory:"); + + insertMilestone({ id: 'M001', title: 'Milestone', status: 'active' }); + + insertSlice({ id: 'S01', milestoneId: 'M001', title: 'Original', demo: 'Old demo', risk: 'low' }); + + // Explicit update with real values should overwrite + insertSlice({ id: 'S01', milestoneId: 'M001', title: 'Updated', demo: 'New demo', risk: 'high' }); + + const after = getSlice('M001', 'S01'); + assert.ok(after); + assert.equal(after.title, 'Updated', 'explicit title update should apply'); + assert.equal(after.demo, 'New demo', 'explicit demo update should apply'); + assert.equal(after.risk, 'high', 'explicit risk update should apply'); +}); diff --git a/src/resources/extensions/gsd/tests/plan-milestone.test.ts b/src/resources/extensions/gsd/tests/plan-milestone.test.ts index 68c78804f..91cee10c6 100644 --- a/src/resources/extensions/gsd/tests/plan-milestone.test.ts +++ b/src/resources/extensions/gsd/tests/plan-milestone.test.ts @@ -4,7 +4,7 @@ import { mkdtempSync, mkdirSync, rmSync, readFileSync, existsSync, writeFileSync import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { openDatabase, closeDatabase, getMilestone, getMilestoneSlices, updateSliceStatus } from '../gsd-db.ts'; +import { openDatabase, closeDatabase, getMilestone, getMilestoneSlices, getSlice, updateSliceStatus, deleteSlice } from '../gsd-db.ts'; import { handlePlanMilestone } from '../tools/plan-milestone.ts'; import { parseRoadmap } from '../parsers-legacy.ts'; @@ -198,33 +198,72 @@ test('handlePlanMilestone reruns idempotently and updates existing planning stat } }); -// Regression: #2960 — plan-milestone must refuse to overwrite completed slices -test('handlePlanMilestone refuses to re-plan a milestone with completed slices (#2960)', async () => { +test('handlePlanMilestone preserves completed slice status on re-plan (#2558)', async () => { const base = makeTmpBase(); const dbPath = join(base, '.gsd', 'gsd.db'); openDatabase(dbPath); try { - // First plan succeeds + // Initial plan — both slices start as "pending" const first = await handlePlanMilestone(validParams(), base); - assert.ok(!('error' in first), `initial plan should succeed: ${'error' in first ? first.error : ''}`); + assert.ok(!('error' in first), `unexpected error: ${'error' in first ? first.error : ''}`); - // Mark S01 as complete - updateSliceStatus('M001', 'S01', 'complete'); + // Mark S01 as complete (simulates work done in a worktree) + updateSliceStatus('M001', 'S01', 'complete', new Date().toISOString()); - // Second plan should fail — S01 is already complete - const second = await handlePlanMilestone({ - ...validParams(), - vision: 'Should not overwrite', - }, base); - assert.ok('error' in second, 'should refuse to re-plan when slices are completed'); - assert.match(second.error, /cannot re-plan/i); - assert.match(second.error, /S01/); + const s01Before = getSlice('M001', 'S01'); + assert.equal(s01Before?.status, 'complete', 'S01 should be complete before re-plan'); - // Verify the completed slice was not overwritten - const slices = getMilestoneSlices('M001'); - const s01 = slices.find(s => s.id === 'S01'); - assert.equal(s01?.status, 'complete', 'S01 should still be complete'); + // Re-plan the same milestone — S01 must stay "complete", S02 stays "pending" + const second = await handlePlanMilestone(validParams(), base); + assert.ok(!('error' in second), `unexpected error: ${'error' in second ? second.error : ''}`); + + const s01After = getSlice('M001', 'S01'); + assert.equal(s01After?.status, 'complete', 'S01 status must be preserved as complete after re-plan'); + + const s02After = getSlice('M001', 'S02'); + assert.equal(s02After?.status, 'pending', 'S02 should remain pending'); + } finally { + cleanup(base); + } +}); + +test('plan-milestone re-plan preserves completed status and updates slice fields (#2558)', async () => { + const base = makeTmpBase(); + const dbPath = join(base, '.gsd', 'gsd.db'); + openDatabase(dbPath); + + try { + // Initial plan — both slices start as "pending" + const first = await handlePlanMilestone(validParams(), base); + assert.ok(!('error' in first), `unexpected error: ${'error' in first ? first.error : ''}`); + + // Mark S01 as complete (simulates work done in worktree, then reconciled) + updateSliceStatus('M001', 'S01', 'complete', new Date().toISOString()); + assert.equal(getSlice('M001', 'S01')?.status, 'complete'); + + // Re-plan with updated title for S01. + // The handler must: + // 1. NOT downgrade S01 from "complete" to "pending" + // 2. Update S01's non-status fields (title, risk, depends, demo) + // 3. Keep S02 as "pending" + const updatedParams = { + ...validParams(), + slices: [ + { ...validParams().slices[0], title: 'Updated S01 title', risk: 'high' }, + validParams().slices[1], + ], + }; + const second = await handlePlanMilestone(updatedParams, base); + assert.ok(!('error' in second), `unexpected error: ${'error' in second ? second.error : ''}`); + + const s01After = getSlice('M001', 'S01'); + assert.equal(s01After?.status, 'complete', 'completed slice status must survive re-plan'); + assert.equal(s01After?.title, 'Updated S01 title', 'title should update on re-plan'); + assert.equal(s01After?.risk, 'high', 'risk should update on re-plan'); + + const s02After = getSlice('M001', 'S02'); + assert.equal(s02After?.status, 'pending', 'pending slice stays pending'); } finally { cleanup(base); } diff --git a/src/resources/extensions/gsd/tools/plan-milestone.ts b/src/resources/extensions/gsd/tools/plan-milestone.ts index 2c24ac009..4b6f0e2d0 100644 --- a/src/resources/extensions/gsd/tools/plan-milestone.ts +++ b/src/resources/extensions/gsd/tools/plan-milestone.ts @@ -5,6 +5,7 @@ import { transaction, getMilestone, getMilestoneSlices, + getSlice, insertMilestone, insertSlice, upsertMilestonePlanning, @@ -191,15 +192,19 @@ export async function handlePlanMilestone( return; } - // Guard: refuse to re-plan a milestone that has completed slices (#2960). - // INSERT OR IGNORE on slices won't overwrite existing rows, but a full - // re-plan after worktree recreation or DB resync can create new slice rows - // that shadow completed work. Block early when any slice is already done. + // Guard: refuse to re-plan a milestone that would drop completed slices (#2960). + // Allow re-planning when all completed slices are still present in the + // incoming plan — their status is preserved below (#2558). Block only when + // the new plan omits a completed slice, which could shadow completed work. const existingSlices = getMilestoneSlices(params.milestoneId); const completedSlices = existingSlices.filter(s => isClosedStatus(s.status)); if (completedSlices.length > 0) { - guardError = `cannot re-plan milestone ${params.milestoneId}: ${completedSlices.length} slice(s) already completed (${completedSlices.map(s => s.id).join(", ")}). Use gsd_reassess_roadmap to modify the roadmap.`; - return; + const incomingSliceIds = new Set(params.slices.map(s => s.sliceId)); + const droppedCompleted = completedSlices.filter(s => !incomingSliceIds.has(s.id)); + if (droppedCompleted.length > 0) { + guardError = `cannot re-plan milestone ${params.milestoneId}: ${droppedCompleted.length} completed slice(s) would be dropped (${droppedCompleted.map(s => s.id).join(", ")}). Use gsd_reassess_roadmap to modify the roadmap.`; + return; + } } // Validate depends_on: all dependencies must exist and be complete @@ -239,11 +244,18 @@ export async function handlePlanMilestone( }, params.title); for (const slice of params.slices) { + // Preserve completed/done status on re-plan (#2558). + // Without this, a re-plan after milestone transition would reset + // already-completed slices back to "pending". + const existing = getSlice(params.milestoneId, slice.sliceId); + const status = existing && (existing.status === "complete" || existing.status === "done") + ? existing.status + : "pending"; insertSlice({ id: slice.sliceId, milestoneId: params.milestoneId, title: slice.title, - status: "pending", + status, risk: slice.risk, depends: slice.depends, demo: slice.demo,