diff --git a/src/resources/extensions/gsd/tools/plan-milestone.ts b/src/resources/extensions/gsd/tools/plan-milestone.ts index 95bc2ede8..44749a5ce 100644 --- a/src/resources/extensions/gsd/tools/plan-milestone.ts +++ b/src/resources/extensions/gsd/tools/plan-milestone.ts @@ -189,27 +189,34 @@ export async function handlePlanMilestone( return { error: `validation failed: ${(err as Error).message}` }; } - // ── State machine preconditions ───────────────────────────────────────── - const existingMilestone = getMilestone(params.milestoneId); - if (existingMilestone && (existingMilestone.status === "complete" || existingMilestone.status === "done")) { - return { error: `cannot re-plan milestone ${params.milestoneId}: it is already complete` }; - } - - // Validate depends_on: all dependencies must exist and be complete - if (params.dependsOn && params.dependsOn.length > 0) { - for (const depId of params.dependsOn) { - const dep = getMilestone(depId); - if (!dep) { - return { error: `depends_on references unknown milestone: ${depId}` }; - } - if (dep.status !== "complete" && dep.status !== "done") { - return { error: `depends_on milestone ${depId} is not yet complete (status: ${dep.status})` }; - } - } - } + // ── Guards + DB writes inside a single transaction (prevents TOCTOU) ─── + // Guards must be inside the transaction so the state they check cannot + // change between the read and the write (#2723). + let guardError: string | null = null; try { transaction(() => { + const existingMilestone = getMilestone(params.milestoneId); + if (existingMilestone && (existingMilestone.status === "complete" || existingMilestone.status === "done")) { + guardError = `cannot re-plan milestone ${params.milestoneId}: it is already complete`; + return; + } + + // Validate depends_on: all dependencies must exist and be complete + if (params.dependsOn && params.dependsOn.length > 0) { + for (const depId of params.dependsOn) { + const dep = getMilestone(depId); + if (!dep) { + guardError = `depends_on references unknown milestone: ${depId}`; + return; + } + if (dep.status !== "complete" && dep.status !== "done") { + guardError = `depends_on milestone ${depId} is not yet complete (status: ${dep.status})`; + return; + } + } + } + insertMilestone({ id: params.milestoneId, title: params.title, @@ -254,6 +261,10 @@ export async function handlePlanMilestone( return { error: `db write failed: ${(err as Error).message}` }; } + if (guardError) { + return { error: guardError }; + } + let roadmapPath: string; try { const renderResult = await renderRoadmapFromDb(basePath, params.milestoneId); diff --git a/src/resources/extensions/gsd/tools/plan-slice.ts b/src/resources/extensions/gsd/tools/plan-slice.ts index 8229aee52..bb5ab8f3f 100644 --- a/src/resources/extensions/gsd/tools/plan-slice.ts +++ b/src/resources/extensions/gsd/tools/plan-slice.ts @@ -146,24 +146,33 @@ export async function handlePlanSlice( return { error: `validation failed: ${(err as Error).message}` }; } - const parentMilestone = getMilestone(params.milestoneId); - if (!parentMilestone) { - return { error: `milestone not found: ${params.milestoneId}` }; - } - if (parentMilestone.status === "complete" || parentMilestone.status === "done") { - return { error: `cannot plan slice in a closed milestone: ${params.milestoneId} (status: ${parentMilestone.status})` }; - } - - const parentSlice = getSlice(params.milestoneId, params.sliceId); - if (!parentSlice) { - return { error: `missing parent slice: ${params.milestoneId}/${params.sliceId}` }; - } - if (parentSlice.status === "complete" || parentSlice.status === "done") { - return { error: `cannot re-plan slice ${params.sliceId}: it is already complete — use gsd_slice_reopen first` }; - } + // ── Guards + DB writes inside a single transaction (prevents TOCTOU) ─── + // Guards must be inside the transaction so the state they check cannot + // change between the read and the write (#2723). + let guardError: string | null = null; try { transaction(() => { + const parentMilestone = getMilestone(params.milestoneId); + if (!parentMilestone) { + guardError = `milestone not found: ${params.milestoneId}`; + return; + } + if (parentMilestone.status === "complete" || parentMilestone.status === "done") { + guardError = `cannot plan slice in a closed milestone: ${params.milestoneId} (status: ${parentMilestone.status})`; + return; + } + + const parentSlice = getSlice(params.milestoneId, params.sliceId); + if (!parentSlice) { + guardError = `missing parent slice: ${params.milestoneId}/${params.sliceId}`; + return; + } + if (parentSlice.status === "complete" || parentSlice.status === "done") { + guardError = `cannot re-plan slice ${params.sliceId}: it is already complete — use gsd_slice_reopen first`; + return; + } + upsertSlicePlanning(params.milestoneId, params.sliceId, { goal: params.goal, successCriteria: params.successCriteria, @@ -211,6 +220,10 @@ export async function handlePlanSlice( return { error: `db write failed: ${(err as Error).message}` }; } + if (guardError) { + return { error: guardError }; + } + try { const renderResult = await renderPlanFromDb(basePath, params.milestoneId, params.sliceId); invalidateStateCache(); diff --git a/src/resources/extensions/gsd/tools/plan-task.ts b/src/resources/extensions/gsd/tools/plan-task.ts index c640ee22d..3fb9495ca 100644 --- a/src/resources/extensions/gsd/tools/plan-task.ts +++ b/src/resources/extensions/gsd/tools/plan-task.ts @@ -77,21 +77,29 @@ export async function handlePlanTask( return { error: `validation failed: ${(err as Error).message}` }; } - const parentSlice = getSlice(params.milestoneId, params.sliceId); - if (!parentSlice) { - return { error: `missing parent slice: ${params.milestoneId}/${params.sliceId}` }; - } - if (parentSlice.status === "complete" || parentSlice.status === "done") { - return { error: `cannot plan task in a closed slice: ${params.sliceId} (status: ${parentSlice.status})` }; - } - - const existingTask = getTask(params.milestoneId, params.sliceId, params.taskId); - if (existingTask && (existingTask.status === "complete" || existingTask.status === "done")) { - return { error: `cannot re-plan task ${params.taskId}: it is already complete — use gsd_task_reopen first` }; - } + // ── Guards + DB writes inside a single transaction (prevents TOCTOU) ─── + // Guards must be inside the transaction so the state they check cannot + // change between the read and the write (#2723). + let guardError: string | null = null; try { transaction(() => { + const parentSlice = getSlice(params.milestoneId, params.sliceId); + if (!parentSlice) { + guardError = `missing parent slice: ${params.milestoneId}/${params.sliceId}`; + return; + } + if (parentSlice.status === "complete" || parentSlice.status === "done") { + guardError = `cannot plan task in a closed slice: ${params.sliceId} (status: ${parentSlice.status})`; + return; + } + + const existingTask = getTask(params.milestoneId, params.sliceId, params.taskId); + if (existingTask && (existingTask.status === "complete" || existingTask.status === "done")) { + guardError = `cannot re-plan task ${params.taskId}: it is already complete — use gsd_task_reopen first`; + return; + } + if (!existingTask) { insertTask({ id: params.taskId, @@ -117,6 +125,10 @@ export async function handlePlanTask( return { error: `db write failed: ${(err as Error).message}` }; } + if (guardError) { + return { error: guardError }; + } + try { const renderResult = await renderTaskPlanFromDb(basePath, params.milestoneId, params.sliceId, params.taskId); invalidateStateCache(); diff --git a/src/resources/extensions/gsd/tools/reassess-roadmap.ts b/src/resources/extensions/gsd/tools/reassess-roadmap.ts index db916bea9..3aa832120 100644 --- a/src/resources/extensions/gsd/tools/reassess-roadmap.ts +++ b/src/resources/extensions/gsd/tools/reassess-roadmap.ts @@ -104,47 +104,6 @@ export async function handleReassessRoadmap( return { error: `validation failed: ${(err as Error).message}` }; } - // ── Verify milestone exists and is active ──────────────────────── - const milestone = getMilestone(params.milestoneId); - if (!milestone) { - return { error: `milestone not found: ${params.milestoneId}` }; - } - if (milestone.status === "complete" || milestone.status === "done") { - return { error: `cannot reassess a closed milestone: ${params.milestoneId} (status: ${milestone.status})` }; - } - - // ── Verify completedSliceId is actually complete ────────────────── - const completedSlice = getSlice(params.milestoneId, params.completedSliceId); - if (!completedSlice) { - return { error: `completedSliceId not found: ${params.milestoneId}/${params.completedSliceId}` }; - } - if (completedSlice.status !== "complete" && completedSlice.status !== "done") { - return { error: `completedSliceId ${params.completedSliceId} is not complete (status: ${completedSlice.status}) — reassess can only be called after a slice finishes` }; - } - - // ── Structural enforcement ──────────────────────────────────────── - const existingSlices = getMilestoneSlices(params.milestoneId); - const completedSliceIds = new Set(); - for (const slice of existingSlices) { - if (slice.status === "complete" || slice.status === "done") { - completedSliceIds.add(slice.id); - } - } - - // Reject modifications to completed slices - for (const modifiedSlice of params.sliceChanges.modified) { - if (completedSliceIds.has(modifiedSlice.sliceId)) { - return { error: `cannot modify completed slice ${modifiedSlice.sliceId}` }; - } - } - - // Reject removal of completed slices - for (const removedId of params.sliceChanges.removed) { - if (completedSliceIds.has(removedId)) { - return { error: `cannot remove completed slice ${removedId}` }; - } - } - // ── Compute assessment artifact path ────────────────────────────── // Assessment lives in the completed slice's directory const assessmentRelPath = join( @@ -153,9 +112,58 @@ export async function handleReassessRoadmap( `${params.completedSliceId}-ASSESSMENT.md`, ); - // ── Transaction: DB mutations ───────────────────────────────────── + // ── Guards + DB writes inside a single transaction (prevents TOCTOU) ─── + // Guards must be inside the transaction so the state they check cannot + // change between the read and the write (#2723). + let guardError: string | null = null; + try { transaction(() => { + // Verify milestone exists and is active + const milestone = getMilestone(params.milestoneId); + if (!milestone) { + guardError = `milestone not found: ${params.milestoneId}`; + return; + } + if (milestone.status === "complete" || milestone.status === "done") { + guardError = `cannot reassess a closed milestone: ${params.milestoneId} (status: ${milestone.status})`; + return; + } + + // Verify completedSliceId is actually complete + const completedSlice = getSlice(params.milestoneId, params.completedSliceId); + if (!completedSlice) { + guardError = `completedSliceId not found: ${params.milestoneId}/${params.completedSliceId}`; + return; + } + if (completedSlice.status !== "complete" && completedSlice.status !== "done") { + 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 + const existingSlices = getMilestoneSlices(params.milestoneId); + const completedSliceIds = new Set(); + for (const slice of existingSlices) { + if (slice.status === "complete" || slice.status === "done") { + completedSliceIds.add(slice.id); + } + } + + for (const modifiedSlice of params.sliceChanges.modified) { + if (completedSliceIds.has(modifiedSlice.sliceId)) { + guardError = `cannot modify completed slice ${modifiedSlice.sliceId}`; + return; + } + } + + for (const removedId of params.sliceChanges.removed) { + if (completedSliceIds.has(removedId)) { + guardError = `cannot remove completed slice ${removedId}`; + return; + } + } + // Record assessment insertAssessment({ path: assessmentRelPath, @@ -198,6 +206,10 @@ export async function handleReassessRoadmap( return { error: `db write failed: ${(err as Error).message}` }; } + if (guardError) { + return { error: guardError }; + } + // ── Render artifacts ────────────────────────────────────────────── try { const roadmapResult = await renderRoadmapFromDb(basePath, params.milestoneId); diff --git a/src/resources/extensions/gsd/tools/replan-slice.ts b/src/resources/extensions/gsd/tools/replan-slice.ts index f96474825..c6b060597 100644 --- a/src/resources/extensions/gsd/tools/replan-slice.ts +++ b/src/resources/extensions/gsd/tools/replan-slice.ts @@ -90,52 +90,61 @@ export async function handleReplanSlice( return { error: `validation failed: ${(err as Error).message}` }; } - // ── Verify parent slice exists and is not closed ───────────────── - const parentSlice = getSlice(params.milestoneId, params.sliceId); - if (!parentSlice) { - return { error: `missing parent slice: ${params.milestoneId}/${params.sliceId}` }; - } - if (parentSlice.status === "complete" || parentSlice.status === "done") { - return { error: `cannot replan a closed slice: ${params.sliceId} (status: ${parentSlice.status})` }; - } - - // ── Verify blocker task exists and is complete ──────────────────── - const blockerTask = getTask(params.milestoneId, params.sliceId, params.blockerTaskId); - if (!blockerTask) { - return { error: `blockerTaskId not found: ${params.milestoneId}/${params.sliceId}/${params.blockerTaskId}` }; - } - if (blockerTask.status !== "complete" && blockerTask.status !== "done") { - return { error: `blockerTaskId ${params.blockerTaskId} is not complete (status: ${blockerTask.status}) — the blocker task must be finished before a replan is triggered` }; - } - - // ── Structural enforcement ──────────────────────────────────────── - const existingTasks = getSliceTasks(params.milestoneId, params.sliceId); - const completedTaskIds = new Set(); - for (const task of existingTasks) { - if (task.status === "complete" || task.status === "done") { - completedTaskIds.add(task.id); - } - } - - // Reject updates to completed tasks - for (const updatedTask of params.updatedTasks) { - if (completedTaskIds.has(updatedTask.taskId)) { - return { error: `cannot modify completed task ${updatedTask.taskId}` }; - } - } - - // Reject removal of completed tasks - for (const removedId of params.removedTaskIds) { - if (completedTaskIds.has(removedId)) { - return { error: `cannot remove completed task ${removedId}` }; - } - } - - // ── Transaction: DB mutations ───────────────────────────────────── - const existingTaskIds = new Set(existingTasks.map((t) => t.id)); + // ── Guards + DB writes inside a single transaction (prevents TOCTOU) ─── + // Guards must be inside the transaction so the state they check cannot + // change between the read and the write (#2723). + let guardError: string | null = null; + let existingTaskIds: Set = new Set(); try { transaction(() => { + // Verify parent slice exists and is not closed + const parentSlice = getSlice(params.milestoneId, params.sliceId); + if (!parentSlice) { + guardError = `missing parent slice: ${params.milestoneId}/${params.sliceId}`; + return; + } + if (parentSlice.status === "complete" || parentSlice.status === "done") { + guardError = `cannot replan a closed slice: ${params.sliceId} (status: ${parentSlice.status})`; + return; + } + + // Verify blocker task exists and is complete + const blockerTask = getTask(params.milestoneId, params.sliceId, params.blockerTaskId); + if (!blockerTask) { + guardError = `blockerTaskId not found: ${params.milestoneId}/${params.sliceId}/${params.blockerTaskId}`; + return; + } + if (blockerTask.status !== "complete" && blockerTask.status !== "done") { + guardError = `blockerTaskId ${params.blockerTaskId} is not complete (status: ${blockerTask.status}) — the blocker task must be finished before a replan is triggered`; + return; + } + + // Structural enforcement — reject modifications/removal of completed tasks + const existingTasks = getSliceTasks(params.milestoneId, params.sliceId); + const completedTaskIds = new Set(); + for (const task of existingTasks) { + if (task.status === "complete" || task.status === "done") { + completedTaskIds.add(task.id); + } + } + + for (const updatedTask of params.updatedTasks) { + if (completedTaskIds.has(updatedTask.taskId)) { + guardError = `cannot modify completed task ${updatedTask.taskId}`; + return; + } + } + + for (const removedId of params.removedTaskIds) { + if (completedTaskIds.has(removedId)) { + guardError = `cannot remove completed task ${removedId}`; + return; + } + } + + existingTaskIds = new Set(existingTasks.map((t) => t.id)); + // Record replan history insertReplanHistory({ milestoneId: params.milestoneId, @@ -189,6 +198,10 @@ export async function handleReplanSlice( return { error: `db write failed: ${(err as Error).message}` }; } + if (guardError) { + return { error: guardError }; + } + // ── Render artifacts ────────────────────────────────────────────── try { const renderResult = await renderPlanFromDb(basePath, params.milestoneId, params.sliceId);