From fb2ef25250199bb22a6a45c08929c5eee7edb1e1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 30 Mar 2026 16:32:58 -0400 Subject: [PATCH] fix: coerce non-numeric strings in DB columns during manifest serialization (#2962) (#3229) SQLite can store string placeholders like "-", "N/A", or "" in INTEGER columns after schema migrations or manual inserts. snapshotState() was passing these through as-is via type assertions, producing JSON that fails to parse on round-trip. Add toNumeric() helper and apply it to all numeric columns (exit_code, duration_ms, sequence, seq). Co-authored-by: Claude Opus 4.6 --- .../gsd/tests/workflow-manifest.test.ts | 92 +++++++++++++++++++ .../extensions/gsd/workflow-manifest.ts | 27 +++++- 2 files changed, 114 insertions(+), 5 deletions(-) diff --git a/src/resources/extensions/gsd/tests/workflow-manifest.test.ts b/src/resources/extensions/gsd/tests/workflow-manifest.test.ts index fa0618cbb..5e4591f9d 100644 --- a/src/resources/extensions/gsd/tests/workflow-manifest.test.ts +++ b/src/resources/extensions/gsd/tests/workflow-manifest.test.ts @@ -12,6 +12,7 @@ import { insertMilestone, insertSlice, insertTask, + _getAdapter, } from '../gsd-db.ts'; import { writeManifest, @@ -165,6 +166,97 @@ test('workflow-manifest: bootstrapFromManifest restores DB from manifest (round- } }); +// ─── snapshotState: numeric column coercion (#2962) ───────────────────── + +test('workflow-manifest: snapshotState coerces string placeholders in numeric columns to null (#2962)', () => { + const base = tempDir(); + openDatabase(tempDbPath(base)); + try { + // Set up prerequisite rows + insertMilestone({ id: 'M001' }); + insertSlice({ id: 'S01', milestoneId: 'M001' }); + insertTask({ id: 'T01', sliceId: 'S01', milestoneId: 'M001', title: 'Task', status: 'complete' }); + + // Insert verification_evidence with string placeholders in numeric columns + // This simulates what happens after schema migrations or manual inserts + const db = _getAdapter()!; + db.prepare( + `INSERT INTO verification_evidence (task_id, slice_id, milestone_id, command, exit_code, verdict, duration_ms, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, + ).run('T01', 'S01', 'M001', 'npm test', '-', 'pass', '-', new Date().toISOString()); + + // snapshotState should coerce "-" to null for numeric columns + const snap = snapshotState(); + const ev = snap.verification_evidence[0]; + assert.strictEqual(ev.exit_code, null, 'exit_code "-" should be coerced to null'); + assert.strictEqual(ev.duration_ms, null, 'duration_ms "-" should be coerced to null'); + + // Round-trip through JSON should not throw + const json = JSON.stringify(snap, null, 2); + const reparsed = JSON.parse(json); + assert.strictEqual(reparsed.verification_evidence[0].exit_code, null); + assert.strictEqual(reparsed.verification_evidence[0].duration_ms, null); + } finally { + closeDatabase(); + cleanupDir(base); + } +}); + +test('workflow-manifest: snapshotState coerces empty string and N/A in numeric columns (#2962)', () => { + const base = tempDir(); + openDatabase(tempDbPath(base)); + try { + insertMilestone({ id: 'M001' }); + insertSlice({ id: 'S01', milestoneId: 'M001' }); + insertTask({ id: 'T01', sliceId: 'S01', milestoneId: 'M001', title: 'Task', status: 'complete' }); + + const db = _getAdapter()!; + db.prepare( + `INSERT INTO verification_evidence (task_id, slice_id, milestone_id, command, exit_code, verdict, duration_ms, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, + ).run('T01', 'S01', 'M001', 'npm test', 'N/A', 'pass', '', new Date().toISOString()); + + const snap = snapshotState(); + const ev = snap.verification_evidence[0]; + assert.strictEqual(ev.exit_code, null, 'exit_code "N/A" should be coerced to null'); + assert.strictEqual(ev.duration_ms, null, 'duration_ms "" should be coerced to null'); + } finally { + closeDatabase(); + cleanupDir(base); + } +}); + +test('workflow-manifest: snapshotState coerces string placeholders in sequence columns (#2962)', () => { + const base = tempDir(); + openDatabase(tempDbPath(base)); + try { + insertMilestone({ id: 'M001' }); + + // Insert a slice with a string sequence via raw SQL + const db = _getAdapter()!; + db.prepare( + `INSERT INTO slices (milestone_id, id, title, status, risk, depends, demo, created_at, sequence) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ).run('M001', 'S01', 'Test Slice', 'planned', 'low', '[]', '', new Date().toISOString(), '-'); + + db.prepare( + `INSERT INTO tasks (milestone_id, slice_id, id, title, status, sequence) + VALUES (?, ?, ?, ?, ?, ?)`, + ).run('M001', 'S01', 'T01', 'Test Task', 'planned', 'N/A'); + + const snap = snapshotState(); + assert.strictEqual(snap.slices[0].sequence, 0, 'slice sequence "-" should be coerced to 0'); + assert.strictEqual(snap.tasks[0].sequence, 0, 'task sequence "N/A" should be coerced to 0'); + + // JSON round-trip must not throw + const json = JSON.stringify(snap, null, 2); + assert.doesNotThrow(() => JSON.parse(json)); + } finally { + closeDatabase(); + cleanupDir(base); + } +}); + // ─── readManifest: version check ───────────────────────────────────────── test('workflow-manifest: readManifest throws on unsupported version', () => { diff --git a/src/resources/extensions/gsd/workflow-manifest.ts b/src/resources/extensions/gsd/workflow-manifest.ts index d88dda8e9..3d6af0327 100644 --- a/src/resources/extensions/gsd/workflow-manifest.ts +++ b/src/resources/extensions/gsd/workflow-manifest.ts @@ -42,6 +42,23 @@ function requireDb() { return db; } +/** + * Coerce a raw DB value to a number, returning `fallback` for + * null/undefined/non-numeric strings (e.g. "-", "N/A", ""). + * SQLite can store TEXT in INTEGER columns after migrations or manual inserts. + */ +export function toNumeric(value: unknown, fallback: number | null = null): number | null { + if (value === null || value === undefined) return fallback; + if (typeof value === "number") return Number.isFinite(value) ? value : fallback; + if (typeof value === "string") { + const trimmed = value.trim(); + if (trimmed === "" || trimmed === "-" || trimmed === "N/A") return fallback; + const n = Number(trimmed); + return Number.isFinite(n) ? n : fallback; + } + return fallback; +} + // ─── snapshotState ─────────────────────────────────────────────────────── /** @@ -99,7 +116,7 @@ export function snapshotState(): StateManifest { proof_level: (r["proof_level"] as string) ?? "", integration_closure: (r["integration_closure"] as string) ?? "", observability_impact: (r["observability_impact"] as string) ?? "", - sequence: (r["sequence"] as number) ?? 0, + sequence: toNumeric(r["sequence"], 0) as number, replan_triggered_at: (r["replan_triggered_at"] as string) ?? null, })); @@ -129,12 +146,12 @@ export function snapshotState(): StateManifest { expected_output: JSON.parse((r["expected_output"] as string) || "[]"), observability_impact: (r["observability_impact"] as string) ?? "", full_plan_md: (r["full_plan_md"] as string) ?? "", - sequence: (r["sequence"] as number) ?? 0, + sequence: toNumeric(r["sequence"], 0) as number, })); const rawDecisions = db.prepare("SELECT * FROM decisions ORDER BY seq").all() as Record[]; const decisions: Decision[] = rawDecisions.map((r) => ({ - seq: r["seq"] as number, + seq: toNumeric(r["seq"], 0) as number, id: r["id"] as string, when_context: (r["when_context"] as string) ?? "", scope: (r["scope"] as string) ?? "", @@ -153,9 +170,9 @@ export function snapshotState(): StateManifest { slice_id: r["slice_id"] as string, milestone_id: r["milestone_id"] as string, command: r["command"] as string, - exit_code: (r["exit_code"] as number) ?? null, + exit_code: toNumeric(r["exit_code"]), verdict: (r["verdict"] as string) ?? "", - duration_ms: (r["duration_ms"] as number) ?? null, + duration_ms: toNumeric(r["duration_ms"]), created_at: r["created_at"] as string, }));