fix: guard activeMilestone.id access in discuss and headless paths (#2776)

* fix: guard activeMilestone.id access in discuss and headless paths

When upstream state corruption (#2772, #2770) produces an activeMilestone
object with undefined id, the existing `!state.activeMilestone` guard
passes (truthy object), and the undefined id propagates to SQLite where
better-sqlite3 throws "Missing named parameter 'mid'".

Strengthen guards at three call sites to check `!state.activeMilestone?.id`
so corrupted state falls through to the no-milestone recovery path.

Closes #2773

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test: add regression tests for activeMilestone.id guard

Covers the #2773 fix where a malformed activeMilestone object with
id: undefined bypassed the old truthiness check and caused a crash
in discuss and headless paths.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
TÂCHES 2026-03-26 20:05:19 -06:00 committed by GitHub
parent 6af72210d1
commit d80927f50d
3 changed files with 96 additions and 4 deletions

View file

@ -71,7 +71,7 @@ export async function handleQuery(basePath: string): Promise<QueryResult> {
// Derive next dispatch action
let next: QuerySnapshot['next']
if (!state.activeMilestone) {
if (!state.activeMilestone?.id) {
next = {
action: 'stop',
reason: state.phase === 'complete' ? 'All milestones complete.' : state.nextAction,

View file

@ -517,8 +517,9 @@ export async function showDiscuss(
const state = await deriveState(basePath);
// No active milestone — check for pending milestones to discuss instead
if (!state.activeMilestone) {
// No active milestone (or corrupted milestone with undefined id) —
// check for pending milestones to discuss instead
if (!state.activeMilestone?.id) {
const pendingMilestones = state.registry.filter(m => m.status === "pending");
if (pendingMilestones.length === 0) {
ctx.ui.notify("No active milestone. Run /gsd to create one first.", "warning");
@ -1043,7 +1044,7 @@ export async function showSmartEntry(
const state = await deriveState(basePath);
if (!state.activeMilestone) {
if (!state.activeMilestone?.id) {
// Guard: if a discuss session is already in flight, don't re-inject the prompt.
// Both /gsd and /gsd auto reach this branch when no milestone exists yet.
// Without this guard, every subsequent /gsd call overwrites pendingAutoStart

View file

@ -0,0 +1,91 @@
/**
* Regression test for #2773 activeMilestone.id guard
*
* When activeMilestone is a non-null object with `id: undefined` (corrupted
* state), the old `!state.activeMilestone` truthiness check passed through,
* causing a downstream crash when code assumed `.id` was a valid string.
*
* The fix uses optional chaining (`!state.activeMilestone?.id`) so all three
* "no usable milestone" shapes are caught:
* 1. activeMilestone === null
* 2. activeMilestone === undefined
* 3. activeMilestone === { id: undefined, title: "..." }
*/
import { describe, it } from 'node:test'
import assert from 'node:assert/strict'
import type { GSDState, ActiveRef } from '../types.ts'
// ─── Guard Under Test ────────────────────────────────────────────────────────
// Extracted guard logic identical to headless-query.ts (line 74) and
// guided-flow.ts (lines 522, 1047).
function activeMilestoneIsUsable(activeMilestone: ActiveRef | null | undefined): boolean {
return !!activeMilestone?.id
}
// ─── Tests ───────────────────────────────────────────────────────────────────
describe('activeMilestone?.id guard (#2773)', () => {
it('rejects null activeMilestone', () => {
assert.equal(activeMilestoneIsUsable(null), false)
})
it('rejects undefined activeMilestone', () => {
assert.equal(activeMilestoneIsUsable(undefined), false)
})
it('rejects malformed activeMilestone with id: undefined', () => {
// This is the crash case from #2773 — object exists but id is undefined
const malformed = { id: undefined, title: 'Ghost Milestone' } as unknown as ActiveRef
assert.equal(activeMilestoneIsUsable(malformed), false)
})
it('rejects malformed activeMilestone with id: empty string', () => {
const malformed = { id: '', title: 'Empty ID Milestone' } as unknown as ActiveRef
assert.equal(activeMilestoneIsUsable(malformed), false)
})
it('accepts valid activeMilestone with a real id', () => {
const valid: ActiveRef = { id: 'M001', title: 'Real Milestone' }
assert.equal(activeMilestoneIsUsable(valid), true)
})
})
describe('headless-query stop behavior with corrupted milestone', () => {
// Simulates the decision logic from handleQuery (headless-query.ts:74-78)
function deriveNextAction(activeMilestone: ActiveRef | null | undefined, phase: string) {
if (!activeMilestone?.id) {
return {
action: 'stop' as const,
reason: phase === 'complete' ? 'All milestones complete.' : 'No active milestone.',
}
}
return { action: 'dispatch' as const, unitId: activeMilestone.id }
}
it('returns stop when activeMilestone is null', () => {
const result = deriveNextAction(null, 'pre-planning')
assert.equal(result.action, 'stop')
})
it('returns stop when activeMilestone has undefined id', () => {
const corrupted = { id: undefined, title: 'Corrupted' } as unknown as ActiveRef
const result = deriveNextAction(corrupted, 'executing')
assert.equal(result.action, 'stop')
assert.equal(result.reason, 'No active milestone.')
})
it('returns dispatch with valid milestone id', () => {
const valid: ActiveRef = { id: 'M001', title: 'Valid' }
const result = deriveNextAction(valid, 'executing')
assert.equal(result.action, 'dispatch')
})
it('returns correct stop reason when phase is complete', () => {
const result = deriveNextAction(null, 'complete')
assert.equal(result.action, 'stop')
assert.equal(result.reason, 'All milestones complete.')
})
})