diff --git a/src/resources/extensions/gsd/tests/validate-milestone-write-order.test.ts b/src/resources/extensions/gsd/tests/validate-milestone-write-order.test.ts new file mode 100644 index 000000000..f78879e15 --- /dev/null +++ b/src/resources/extensions/gsd/tests/validate-milestone-write-order.test.ts @@ -0,0 +1,90 @@ +import { describe, it, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdirSync, existsSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { randomUUID } from "node:crypto"; + +import { handleValidateMilestone } from "../tools/validate-milestone.js"; +import { openDatabase, closeDatabase, _getAdapter, insertMilestone } from "../gsd-db.js"; +import { clearPathCache } from "../paths.js"; +import { clearParseCache } from "../files.js"; + +function makeTmpBase(): string { + const base = join(tmpdir(), `gsd-val-handler-${randomUUID()}`); + mkdirSync(join(base, ".gsd", "milestones", "M001"), { recursive: true }); + return base; +} + +const VALID_PARAMS = { + milestoneId: "M001", + verdict: "pass" as const, + remediationRound: 0, + successCriteriaChecklist: "- [x] All pass", + sliceDeliveryAudit: "| S01 | delivered |", + crossSliceIntegration: "No issues", + requirementCoverage: "All covered", + verdictRationale: "Everything checks out", +}; + +describe("handleValidateMilestone write ordering (#2725)", () => { + let base: string; + + afterEach(() => { + clearPathCache(); + clearParseCache(); + try { closeDatabase(); } catch { /* */ } + if (base) { + try { rmSync(base, { recursive: true, force: true }); } catch { /* */ } + } + }); + + it("writes DB row and disk file on success", async () => { + base = makeTmpBase(); + const dbPath = join(base, ".gsd", "gsd.db"); + openDatabase(dbPath); + insertMilestone({ id: "M001" }); + + const result = await handleValidateMilestone(VALID_PARAMS, base); + assert.ok(!("error" in result), `unexpected error: ${"error" in result ? result.error : ""}`); + + // DB row exists + const adapter = _getAdapter()!; + const row = adapter.prepare( + `SELECT status, scope FROM assessments WHERE milestone_id = 'M001' AND scope = 'milestone-validation'`, + ).get() as { status: string; scope: string } | undefined; + assert.ok(row, "assessment row should exist in DB"); + assert.equal(row!.status, "pass"); + + // Disk file exists + const filePath = join(base, ".gsd", "milestones", "M001", "M001-VALIDATION.md"); + assert.ok(existsSync(filePath), "VALIDATION.md should exist on disk"); + }); + + it("rolls back DB row when disk write fails", async () => { + base = makeTmpBase(); + const dbPath = join(base, ".gsd", "gsd.db"); + openDatabase(dbPath); + insertMilestone({ id: "M001" }); + + // Force disk write failure by replacing the milestone directory with a + // regular file. saveFile() will fail because it cannot write inside a + // non-directory. This works cross-platform (chmod is ignored on Windows). + const milestoneDir = join(base, ".gsd", "milestones", "M001"); + rmSync(milestoneDir, { recursive: true, force: true }); + writeFileSync(milestoneDir, "not-a-directory"); + + const result = await handleValidateMilestone(VALID_PARAMS, base); + + // Should return error + assert.ok("error" in result, "should return error when disk write fails"); + assert.ok(result.error.includes("disk render failed")); + + // DB row should have been rolled back (deleted) + const adapter = _getAdapter()!; + const row = adapter.prepare( + `SELECT * FROM assessments WHERE milestone_id = 'M001' AND scope = 'milestone-validation'`, + ).get(); + assert.equal(row, undefined, "assessment row should be deleted after disk-write rollback"); + }); +}); diff --git a/src/resources/extensions/gsd/tools/validate-milestone.ts b/src/resources/extensions/gsd/tools/validate-milestone.ts index 856ced060..d34fd69fe 100644 --- a/src/resources/extensions/gsd/tools/validate-milestone.ts +++ b/src/resources/extensions/gsd/tools/validate-milestone.ts @@ -76,7 +76,7 @@ export async function handleValidateMilestone( return { error: `verdict must be one of: ${VALIDATION_VERDICTS.join(", ")}` }; } - // ── Filesystem render ────────────────────────────────────────────────── + // ── Resolve paths and render markdown ──────────────────────────────── const validationMd = renderValidationMarkdown(params); let validationPath: string; @@ -89,16 +89,11 @@ export async function handleValidateMilestone( validationPath = join(manualDir, `${params.milestoneId}-VALIDATION.md`); } - try { - await saveFile(validationPath, validationMd); - } catch (renderErr) { - process.stderr.write( - `gsd-db: validate_milestone — disk render failed: ${(renderErr as Error).message}\n`, - ); - return { error: `disk render failed: ${(renderErr as Error).message}` }; - } - - // ── DB write — store in assessments table ────────────────────────────── + // ── DB write first — matches complete-task/complete-slice pattern ─── + // Write DB before disk so a crash between the two leaves a recoverable + // state: the DB row exists but the file is missing, which projection + // rendering can regenerate. The inverse (file exists, no DB row) is + // harder to detect and recover from (#2725). const validatedAt = new Date().toISOString(); transaction(() => { @@ -115,6 +110,23 @@ export async function handleValidateMilestone( }); }); + // ── Filesystem render (outside transaction) ──────────────────────────── + // If disk render fails, roll back the DB row so state stays consistent. + try { + await saveFile(validationPath, validationMd); + } catch (renderErr) { + process.stderr.write( + `gsd-db: validate_milestone — disk render failed, rolling back DB row: ${(renderErr as Error).message}\n`, + ); + const rollbackAdapter = _getAdapter(); + if (rollbackAdapter) { + rollbackAdapter.prepare( + `DELETE FROM assessments WHERE milestone_id = :mid AND scope = 'milestone-validation'`, + ).run({ ":mid": params.milestoneId }); + } + return { error: `disk render failed: ${(renderErr as Error).message}` }; + } + invalidateStateCache(); clearPathCache(); clearParseCache();