fix: preserve completed slice status on plan-milestone re-plan (#3318)
* fix: preserve completed slice status on plan-milestone re-plan (#2558) When plan-milestone re-plans a milestone that has already-completed slices, the handler now checks existing slice status before inserting. Completed slices retain their status instead of being reset to "pending". Three changes: 1. handlePlanMilestone() checks getSlice() before insertSlice() and passes the existing completed/done status instead of hardcoding "pending". 2. insertSlice() changed from INSERT OR IGNORE to INSERT ... ON CONFLICT upsert that updates non-status fields (title, risk, depends, demo, planning metadata) but preserves completed/done status at the DB layer. 3. reconcileWorktreeDb() slice and task merges now use LEFT JOIN to detect existing completed rows in the main DB and never downgrade their status when merging stale worktree data. Closes #2558 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: relax completed-slice guard to allow re-plan when slices are retained The #2960 guard blocked re-planning entirely when any completed slices existed, conflicting with the #2558 preserve-completed-status logic. Now only blocks when the new plan would drop completed slices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(gsd-db): prevent insertSlice ON CONFLICT from wiping populated fields The ON CONFLICT clause unconditionally overwrote all non-status fields with excluded values. Callers like complete-task.ts and complete-slice.ts use insertSlice as an idempotent "ensure row exists" guard with only id and milestoneId, causing defaults (empty strings, 0) to silently destroy populated titles, demos, goals, and planning data. Fix: use raw sentinel bind params (NULL when caller omitted the field) in CASE guards so the ON CONFLICT UPDATE only overwrites fields the caller actually provided. Initial INSERTs still get proper defaults. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: retrigger CI --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: trek-e <trek-e@users.noreply.github.com>
This commit is contained in:
Tom Boucher
GitHub
parent
0908adcb4e
commit
e0a5f6866e
4 changed files with 224 additions and 40 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue