singularity/singularity-forge
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(gsd): move state machine guards inside transaction in 5 tool handlers (#2752)

Browse source 
plan-task, plan-slice, plan-milestone, reassess-roadmap, and
replan-slice all ran state machine guards (getSlice, getMilestone,
getTask) outside the transaction() callback, then performed writes
in a separate transaction. This created a TOCTOU race: two agents
could both pass the guard simultaneously and both write successfully.

Fix: move all guard checks into the transaction() callback using
the guardError pattern already used by complete-task, complete-slice,
reopen-task, and reopen-slice. The SQLite write lock now covers both
the guard reads and the subsequent writes atomically.

Closes #2723
This commit is contained in:
mastertyko 2026-03-26 23:39:19 +01:00 • committed by GitHub
parent f8814f5a15
commit 032088e409
5 changed files with 191 additions and 130 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

47
src/resources/extensions/gsd/tools/plan-milestone.ts
View file

@ -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);

43
src/resources/extensions/gsd/tools/plan-slice.ts
View file

@ -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();

36
src/resources/extensions/gsd/tools/plan-task.ts
View file

@ -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();

96
src/resources/extensions/gsd/tools/reassess-roadmap.ts
View file

@ -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<string>();
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<string>();
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);

99
src/resources/extensions/gsd/tools/replan-slice.ts
View file

@ -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<string>();
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<string> = 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<string>();
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);