From 25d97638783512a42b266dc829ea629bc7c65259 Mon Sep 17 00:00:00 2001 From: Lex Christopherson Date: Sat, 25 Apr 2026 13:55:08 -0600 Subject: [PATCH] fix(sdk): fix executor plan loading, plan ID derivation, and verification outcome parsing Bug 1: phasePlanIndex derived empty planId for bare PLAN.md files. Fixed to use 'PLAN' as the ID, with matching SUMMARY.md detection. Bug 2: executeSinglePlan passed null to buildPrompt instead of the actual parsed plan. The executor needs the plan content (tasks, objectives) to know what to build. Now loads and parses the plan file before building the prompt. Bug 3: parseVerificationOutcome checked session exit code, not what the verifier wrote. A session that runs without errors but writes status: gaps_found to VERIFICATION.md was treated as 'passed'. Now queries check.verification-status to read the actual VERIFICATION.md frontmatter status field. Co-Authored-By: Claude Opus 4.6 (1M context) --- sdk/src/phase-runner.test.ts | 17 ++++++++++- sdk/src/phase-runner.ts | 57 +++++++++++++++++++++++++++++------- sdk/src/query/phase.ts | 8 +++-- 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/sdk/src/phase-runner.test.ts b/sdk/src/phase-runner.test.ts index 088f05b46..dab899055 100644 --- a/sdk/src/phase-runner.test.ts +++ b/sdk/src/phase-runner.test.ts @@ -26,6 +26,18 @@ vi.mock('./session-runner.js', () => ({ runPlanSession: vi.fn(), })); +// Mock plan-parser to avoid real file I/O in executeSinglePlan +vi.mock('./plan-parser.js', () => ({ + parsePlanFile: vi.fn().mockResolvedValue({ + frontmatter: { phase: '01-auth', plan: '01', type: 'execute', wave: 1, depends_on: [], files_modified: [], autonomous: true, requirements: [], must_haves: { truths: [], artifacts: [], key_links: [] } }, + objective: 'Test plan objective', + execution_context: [], + context_refs: [], + tasks: [{ name: 'Test task', type: 'auto', files: [], read_first: [], action: 'do the thing', verify: 'check it', done: 'done', acceptance_criteria: [] }], + raw: '', + }), +})); + import { runPhaseStepSession } from './session-runner.js'; const mockRunPhaseStepSession = vi.mocked(runPhaseStepSession); @@ -129,7 +141,10 @@ function makeDeps(overrides: Partial = {}): PhaseRunnerDeps { initPhaseOp: vi.fn().mockResolvedValue(makePhaseOp()), phaseComplete: vi.fn().mockResolvedValue(undefined), phasePlanIndex: vi.fn().mockResolvedValue(makePlanIndex(1)), - exec: vi.fn(), + exec: vi.fn().mockImplementation((cmd: string) => { + if (cmd === 'check.verification-status') return Promise.resolve({ status: 'pass' }); + return Promise.resolve(undefined); + }), stateLoad: vi.fn(), roadmapAnalyze: vi.fn(), commit: vi.fn(), diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index 4bebfbcf9..8e3539842 100644 --- a/sdk/src/phase-runner.ts +++ b/sdk/src/phase-runner.ts @@ -26,6 +26,7 @@ import type { PromptFactory } from './phase-prompt.js'; import type { ContextEngine } from './context-engine.js'; import type { GSDLogger } from './logger.js'; import { runPhaseStepSession, runPlanSession } from './session-runner.js'; +import { parsePlanFile } from './plan-parser.js'; import { readFile } from 'node:fs/promises'; import { join } from 'node:path'; import { checkResearchGate } from './research-gate.js'; @@ -745,6 +746,8 @@ export class PhaseRunner { /** * Execute a single plan by ID within the execute step. + * Loads the plan file, parses it, and passes the parsed plan to the prompt + * builder so the executor gets the full plan content (tasks, objectives, etc.). */ private async executeSinglePlan( phaseNumber: string, @@ -752,9 +755,17 @@ export class PhaseRunner { sessionOpts: SessionOptions, ): Promise { try { + // Resolve the plan file path from phase directory + planId + const phaseOp = await this.tools.initPhaseOp(phaseNumber); + const planFilename = planId === 'PLAN' ? 'PLAN.md' : `${planId}-PLAN.md`; + const planPath = join(this.projectDir, phaseOp.phase_dir, planFilename); + + // Parse the plan file so the executor prompt includes the actual tasks + const parsedPlan = await parsePlanFile(planPath); + const phaseType = PhaseType.Execute; const contextFiles = await this.contextEngine.resolveContextFiles(phaseType); - const prompt = await this.promptFactory.buildPrompt(phaseType, null, contextFiles); + const prompt = await this.promptFactory.buildPrompt(phaseType, parsedPlan, contextFiles); return await runPhaseStepSession( prompt, @@ -849,8 +860,8 @@ export class PhaseRunner { }; } - // Parse verification outcome from session result - outcome = this.parseVerificationOutcome(lastResult); + // Parse verification outcome from VERIFICATION.md (not just session exit code) + outcome = await this.parseVerificationOutcome(lastResult, phaseNumber); if (outcome === 'passed') { break; @@ -1084,14 +1095,40 @@ export class PhaseRunner { } /** - * Parse the verification outcome from a PlanResult. - * In a real implementation, this would parse the session output for - * structured verification signals. For now, map from success/error. + * Parse the verification outcome by checking VERIFICATION.md on disk. + * The verify session may succeed (no runtime errors) while writing + * status: gaps_found to VERIFICATION.md — we need to check the file, + * not just the session exit code. + * + * Falls back to session result if VERIFICATION.md can't be parsed. */ - private parseVerificationOutcome(result: PlanResult): VerificationOutcome { - if (result.success) return 'passed'; - if (result.error?.subtype === 'human_review_needed') return 'human_needed'; - return 'gaps_found'; + private async parseVerificationOutcome(result: PlanResult, phaseNumber: string): Promise { + // If the session itself crashed, that's a clear failure + if (!result.success) { + if (result.error?.subtype === 'human_review_needed') return 'human_needed'; + return 'gaps_found'; + } + + // Session succeeded — check what the verifier actually wrote to VERIFICATION.md + try { + const verStatus = await this.tools.exec('check.verification-status', [phaseNumber]); + const data = typeof verStatus === 'string' ? JSON.parse(verStatus) : verStatus; + const status = (data?.status ?? '').toLowerCase(); + + if (status === 'pass' || status === 'passed') return 'passed'; + if (status === 'fail' || status === 'gaps_found') return 'gaps_found'; + if (status === 'missing') { + // VERIFICATION.md doesn't exist yet — treat session success as passed + return 'passed'; + } + // Unknown status — log and treat as gaps_found to be safe + this.logger?.warn(`Unknown verification status '${status}' for phase ${phaseNumber}, treating as gaps_found`); + return 'gaps_found'; + } catch (err) { + // Can't parse VERIFICATION.md — fall back to session result + this.logger?.warn(`Could not check verification status for phase ${phaseNumber}: ${err instanceof Error ? err.message : String(err)}`); + return 'passed'; + } } /** diff --git a/sdk/src/query/phase.ts b/sdk/src/query/phase.ts index dc2ef7bae..5974e194d 100644 --- a/sdk/src/query/phase.ts +++ b/sdk/src/query/phase.ts @@ -263,9 +263,9 @@ export const phasePlanIndex: QueryHandler = async (args, projectDir, workstream) const planFiles = phaseFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').sort(); const summaryFiles = phaseFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); - // Build set of plan IDs with summaries + // Build set of plan IDs with summaries — match the planId derivation logic const completedPlanIds = new Set( - summaryFiles.map(s => s.replace('-SUMMARY.md', '').replace('SUMMARY.md', '')) + summaryFiles.map(s => s === 'SUMMARY.md' ? 'PLAN' : s.replace('-SUMMARY.md', '')) ); const plans: Array> = []; @@ -274,7 +274,9 @@ export const phasePlanIndex: QueryHandler = async (args, projectDir, workstream) let hasCheckpoints = false; for (const planFile of planFiles) { - const planId = planFile.replace('-PLAN.md', '').replace('PLAN.md', ''); + // For named plans (01-01-PLAN.md): strip suffix to get '01-01' + // For bare PLAN.md: use the filename itself as the ID + const planId = planFile === 'PLAN.md' ? 'PLAN' : planFile.replace('-PLAN.md', ''); const planPath = join(phaseDir, planFile); const content = await readFile(planPath, 'utf-8'); const fm = extractFrontmatter(content);