From cc6689aca886a4a7134af12b495ea3d02ada68ea Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 3 Apr 2026 14:19:34 -0400 Subject: [PATCH] fix(phase-runner): add research gate to block planning on unresolved open questions (#1618) * chore: add v1.31.0 npm known-issue notice to issue template config Adds a top-priority contact link to the issue template chooser so users are redirected to the Discussions announcement before opening a duplicate issue about v1.31.0 not being on npm. Co-Authored-By: Claude Sonnet 4.6 * fix(phase-runner): add research gate to block planning on unresolved open questions (#1602) Plan-phase could proceed to planning even when RESEARCH.md had unresolved open questions in its ## Open Questions section. This caused agents to plan and execute with fundamental design decisions still undecided. - Add `research-gate.ts` with pure `checkResearchGate()` function that parses RESEARCH.md for unresolved open questions - Integrate gate into PhaseRunner between research (step 2) and plan (step 3) using existing `invokeBlockerCallback` pattern - Add Dimension 11 (Research Resolution) to gsd-plan-checker.md agent - Gate passes when: no Open Questions section, section has (RESOLVED) suffix, all individual questions marked RESOLVED, or section is empty - Gate fires `onBlockerDecision` callback with PhaseStepType.Research and lists the unresolved questions in the error message - Auto-approves (skip) when no callback registered (headless mode) - 18 new tests: 13 unit tests for checkResearchGate, 5 integration tests for PhaseRunner research gate behavior Closes #1602 Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- agents/gsd-plan-checker.md | 39 +++++++ sdk/src/index.ts | 2 + sdk/src/phase-runner.test.ts | 170 +++++++++++++++++++++++++++++- sdk/src/phase-runner.ts | 34 ++++++ sdk/src/research-gate.test.ts | 190 ++++++++++++++++++++++++++++++++++ sdk/src/research-gate.ts | 94 +++++++++++++++++ 6 files changed, 528 insertions(+), 1 deletion(-) create mode 100644 sdk/src/research-gate.test.ts create mode 100644 sdk/src/research-gate.ts diff --git a/agents/gsd-plan-checker.md b/agents/gsd-plan-checker.md index b70c413da..3320acbf4 100644 --- a/agents/gsd-plan-checker.md +++ b/agents/gsd-plan-checker.md @@ -478,6 +478,45 @@ issue: fix_hint: "Add eslint verification step to each task's block" ``` +## Dimension 11: Research Resolution (#1602) + +**Question:** Are all research questions resolved before planning proceeds? + +**Skip if:** No RESEARCH.md exists for this phase. + +**Process:** +1. Read the phase's RESEARCH.md file +2. Search for a `## Open Questions` section +3. If section heading has `(RESOLVED)` suffix → PASS +4. If section exists: check each listed question for inline `RESOLVED` marker +5. FAIL if any question lacks a resolution + +**Red flags:** +- RESEARCH.md has `## Open Questions` section without `(RESOLVED)` suffix +- Individual questions listed without resolution status +- Prose-style open questions that haven't been addressed + +**Example — unresolved questions:** +```yaml +issue: + dimension: research_resolution + severity: blocker + description: "RESEARCH.md has unresolved open questions" + file: "01-RESEARCH.md" + unresolved_questions: + - "Hash prefix — keep or change?" + - "Cache TTL — what duration?" + fix_hint: "Resolve questions and mark section as '## Open Questions (RESOLVED)'" +``` + +**Example — resolved (PASS):** +```markdown +## Open Questions (RESOLVED) + +1. **Hash prefix** — RESOLVED: Use "guest_contract:" +2. **Cache TTL** — RESOLVED: 5 minutes with Redis +``` + diff --git a/sdk/src/index.ts b/sdk/src/index.ts index d43dc5878..55bf2aecd 100644 --- a/sdk/src/index.ts +++ b/sdk/src/index.ts @@ -299,6 +299,8 @@ export type { FileSpec } from './context-engine.js'; export { truncateMarkdown, extractCurrentMilestone, DEFAULT_TRUNCATION_OPTIONS } from './context-truncation.js'; export type { TruncationOptions } from './context-truncation.js'; export { getToolsForPhase, PHASE_AGENT_MAP, PHASE_DEFAULT_TOOLS } from './tool-scoping.js'; +export { checkResearchGate } from './research-gate.js'; +export type { ResearchGateResult } from './research-gate.js'; export { PromptFactory, extractBlock, extractSteps, PHASE_WORKFLOW_MAP } from './phase-prompt.js'; export { GSDLogger } from './logger.js'; export type { LogLevel, LogEntry, GSDLoggerOptions } from './logger.js'; diff --git a/sdk/src/phase-runner.test.ts b/sdk/src/phase-runner.test.ts index a31bd7a92..088f05b46 100644 --- a/sdk/src/phase-runner.test.ts +++ b/sdk/src/phase-runner.test.ts @@ -1,4 +1,7 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtemp, mkdir, writeFile, rm } from 'node:fs/promises'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; import { PhaseRunner, PhaseRunnerError } from './phase-runner.js'; import type { PhaseRunnerDeps, VerificationOutcome } from './phase-runner.js'; import type { @@ -423,6 +426,171 @@ describe('PhaseRunner', () => { }); }); + // ─── Research gate (#1602) ────────────────────────────────────────────── + + describe('research gate (#1602)', () => { + let tempPhaseDir: string; + + beforeEach(async () => { + tempPhaseDir = await mkdtemp(join(tmpdir(), 'gsd-research-gate-')); + }); + + afterEach(async () => { + await rm(tempPhaseDir, { recursive: true, force: true }); + }); + + it('invokes onBlockerDecision when RESEARCH.md has unresolved open questions', async () => { + // Write a RESEARCH.md with unresolved questions + const researchPath = join(tempPhaseDir, '01-RESEARCH.md'); + await writeFile(researchPath, `# Research + +## Key Findings +TypeScript is the right choice. + +## Open Questions + +1. **Hash prefix** — keep or change? +2. **Cache TTL** — what duration? + +## Recommendations +Use TypeScript.`, 'utf-8'); + + const onBlockerDecision = vi.fn().mockResolvedValue('stop'); + const phaseOp = makePhaseOp({ + has_context: true, + has_research: true, + has_plans: true, + plan_count: 1, + phase_dir: tempPhaseDir, + research_path: researchPath, + }); + const deps = makeDeps(); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1', { + callbacks: { onBlockerDecision }, + }); + + expect(onBlockerDecision).toHaveBeenCalled(); + const callArg = onBlockerDecision.mock.calls[0][0]; + expect(callArg.step).toBe(PhaseStepType.Research); + expect(callArg.error).toContain('unresolved open questions'); + expect(callArg.error).toContain('Hash prefix'); + }); + + it('does not block when RESEARCH.md has no open questions', async () => { + const researchPath = join(tempPhaseDir, '01-RESEARCH.md'); + await writeFile(researchPath, `# Research + +## Key Findings +Everything resolved. + +## Recommendations +Use TypeScript.`, 'utf-8'); + + const onBlockerDecision = vi.fn().mockResolvedValue('stop'); + const phaseOp = makePhaseOp({ + has_context: true, + has_research: true, + has_plans: true, + plan_count: 1, + phase_dir: tempPhaseDir, + research_path: researchPath, + }); + const deps = makeDeps(); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + await runner.run('1', { + callbacks: { onBlockerDecision }, + }); + + // Should NOT have been called for research step + const researchCalls = onBlockerDecision.mock.calls.filter( + (c: any[]) => c[0].step === PhaseStepType.Research, + ); + expect(researchCalls).toHaveLength(0); + }); + + it('does not block when all open questions are resolved', async () => { + const researchPath = join(tempPhaseDir, '01-RESEARCH.md'); + await writeFile(researchPath, `# Research + +## Open Questions (RESOLVED) + +1. **Hash prefix** — RESOLVED: Use "guest_contract:"`, 'utf-8'); + + const onBlockerDecision = vi.fn().mockResolvedValue('stop'); + const phaseOp = makePhaseOp({ + has_context: true, + has_research: true, + has_plans: true, + plan_count: 1, + phase_dir: tempPhaseDir, + research_path: researchPath, + }); + const deps = makeDeps(); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + await runner.run('1', { callbacks: { onBlockerDecision } }); + + const researchCalls = onBlockerDecision.mock.calls.filter( + (c: any[]) => c[0].step === PhaseStepType.Research, + ); + expect(researchCalls).toHaveLength(0); + }); + + it('skips research gate when has_research=false', async () => { + const onBlockerDecision = vi.fn().mockResolvedValue('stop'); + const phaseOp = makePhaseOp({ + has_context: true, + has_research: false, + has_plans: true, + plan_count: 1, + }); + const deps = makeDeps(); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + await runner.run('1', { callbacks: { onBlockerDecision } }); + + // Research gate should not fire when there's no research + const researchCalls = onBlockerDecision.mock.calls.filter( + (c: any[]) => c[0].step === PhaseStepType.Research, + ); + expect(researchCalls).toHaveLength(0); + }); + + it('auto-approves (skip) research gate when no callback registered', async () => { + const researchPath = join(tempPhaseDir, '01-RESEARCH.md'); + await writeFile(researchPath, `# Research + +## Open Questions + +1. **Something** — needs decision`, 'utf-8'); + + const phaseOp = makePhaseOp({ + has_context: true, + has_research: true, + has_plans: true, + plan_count: 1, + phase_dir: tempPhaseDir, + research_path: researchPath, + }); + const deps = makeDeps(); + (deps.tools.initPhaseOp as ReturnType).mockResolvedValue(phaseOp); + + const runner = new PhaseRunner(deps); + const result = await runner.run('1'); // No callbacks + + // Should proceed past research gate (auto-skip) + const stepTypes = result.steps.map(s => s.step); + expect(stepTypes).toContain(PhaseStepType.Plan); + }); + }); + // ─── Human gate: reject halts runner ─────────────────────────────────── describe('human gate reject', () => { diff --git a/sdk/src/phase-runner.ts b/sdk/src/phase-runner.ts index c38a9a2d1..4bebfbcf9 100644 --- a/sdk/src/phase-runner.ts +++ b/sdk/src/phase-runner.ts @@ -26,6 +26,9 @@ 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 { readFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { checkResearchGate } from './research-gate.js'; // ─── Error type ────────────────────────────────────────────────────────────── @@ -185,6 +188,21 @@ export class PhaseRunner { } } + // ── Step 2.5: Research gate (#1602) ── + // Check RESEARCH.md for unresolved open questions before planning + if (!halted && phaseOp.has_research) { + const gateResult = await this.checkResearchGate(phaseOp); + if (!gateResult.pass) { + const questionList = gateResult.unresolvedQuestions.join(', '); + const error = `RESEARCH.md has unresolved open questions: ${questionList}`; + this.logger?.warn(error, { phase: phaseNumber }); + const decision = await this.invokeBlockerCallback(callbacks, phaseNumber, PhaseStepType.Research, error); + if (decision === 'stop') { + halted = true; + } + } + } + // ── Step 3: Plan ── if (!halted) { const result = await this.retryOnce('plan', () => this.runStep(PhaseStepType.Plan, phaseNumber, sessionOpts)); @@ -1076,6 +1094,22 @@ export class PhaseRunner { return 'gaps_found'; } + /** + * Check RESEARCH.md for unresolved open questions (#1602). + * Returns the gate result — pass means safe to proceed to planning. + */ + private async checkResearchGate(phaseOp: PhaseOpInfo): Promise<{ pass: boolean; unresolvedQuestions: string[] }> { + try { + const researchPath = phaseOp.research_path || + join(phaseOp.phase_dir, `${phaseOp.padded_phase}-RESEARCH.md`); + const content = await readFile(researchPath, 'utf-8'); + return checkResearchGate(content); + } catch { + // File doesn't exist or can't be read — pass (nothing to gate on) + return { pass: true, unresolvedQuestions: [] }; + } + } + /** * Invoke the onBlockerDecision callback, falling back to auto-approve. */ diff --git a/sdk/src/research-gate.test.ts b/sdk/src/research-gate.test.ts new file mode 100644 index 000000000..13de542a6 --- /dev/null +++ b/sdk/src/research-gate.test.ts @@ -0,0 +1,190 @@ +import { describe, it, expect } from 'vitest'; +import { checkResearchGate } from './research-gate.js'; + +describe('checkResearchGate', () => { + // ── Pass cases ────────────────────────────────────────────────────────── + + it('passes when no Open Questions section exists', () => { + const content = `# Research + +## Key Findings +Everything is clear. + +## Recommendations +Use TypeScript.`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(true); + expect(result.unresolvedQuestions).toEqual([]); + }); + + it('passes when Open Questions section has (RESOLVED) suffix', () => { + const content = `# Research + +## Open Questions (RESOLVED) + +1. **Hash prefix** — RESOLVED: Use "guest_contract:" +2. **Cache TTL** — RESOLVED: 5 minutes`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(true); + expect(result.unresolvedQuestions).toEqual([]); + }); + + it('passes when Open Questions section is empty', () => { + const content = `# Research + +## Open Questions + +## Next Steps +Proceed to planning.`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(true); + expect(result.unresolvedQuestions).toEqual([]); + }); + + it('passes when all individual questions are marked RESOLVED', () => { + const content = `# Research + +## Open Questions + +1. **Hash prefix** — RESOLVED: Use "guest_contract:" +2. **Cache strategy** — RESOLVED: Use Redis with 5min TTL`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(true); + expect(result.unresolvedQuestions).toEqual([]); + }); + + it('passes with empty research content', () => { + const result = checkResearchGate(''); + expect(result.pass).toBe(true); + }); + + // ── Fail cases ────────────────────────────────────────────────────────── + + it('fails when Open Questions section has unresolved numbered items', () => { + const content = `# Research + +## Open Questions + +1. **Hash prefix** — keep or change? +2. **Cache TTL** — what duration? + +## Recommendations +Use TypeScript.`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(2); + expect(result.unresolvedQuestions[0]).toContain('Hash prefix'); + expect(result.unresolvedQuestions[1]).toContain('Cache TTL'); + }); + + it('fails when Open Questions has bullet-point items', () => { + const content = `# Research + +## Open Questions + +- **Auth strategy** — OAuth vs API keys? +- **Database** — Postgres or SQLite?`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(2); + }); + + it('fails with mix of resolved and unresolved questions', () => { + const content = `# Research + +## Open Questions + +1. **Hash prefix** — RESOLVED: Use "guest_contract:" +2. **Cache TTL** — what duration? +3. **Auth flow** — RESOLVED: OAuth2 +4. **Rate limiting** — needs decision`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(2); + expect(result.unresolvedQuestions[0]).toContain('Cache TTL'); + expect(result.unresolvedQuestions[1]).toContain('Rate limiting'); + }); + + it('fails with prose-style open questions (no list formatting)', () => { + const content = `# Research + +## Open Questions + +We still need to determine the hashing strategy and whether +the cache should be shared across instances. + +## Recommendations +Use TypeScript.`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(1); + expect(result.unresolvedQuestions[0]).toContain('unstructured'); + }); + + it('fails when Open Questions is the last section (no next heading)', () => { + const content = `# Research + +## Key Findings +Good stuff. + +## Open Questions + +1. **Deployment strategy** — containers vs serverless?`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(1); + expect(result.unresolvedQuestions[0]).toContain('Deployment strategy'); + }); + + // ── Edge cases ────────────────────────────────────────────────────────── + + it('handles case-insensitive heading match', () => { + const content = `# Research + +## open questions + +1. **Something** — unclear`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + }); + + it('does not match subsection headings (### Open Questions)', () => { + // Only ## level headings should trigger the gate + const content = `# Research + +## Findings + +### Open Questions +These are just notes, not blocking. + +1. **Minor thing** — just a thought`; + + // ### level = subsection under Findings, not the formal gate section + const result = checkResearchGate(content); + expect(result.pass).toBe(true); + }); + + it('handles asterisk-style bullet points', () => { + const content = `# Research + +## Open Questions + +* **Strategy A** — needs evaluation +* **Strategy B** — RESOLVED: go with B`; + + const result = checkResearchGate(content); + expect(result.pass).toBe(false); + expect(result.unresolvedQuestions).toHaveLength(1); + expect(result.unresolvedQuestions[0]).toContain('Strategy A'); + }); +}); diff --git a/sdk/src/research-gate.ts b/sdk/src/research-gate.ts new file mode 100644 index 000000000..1640b18ce --- /dev/null +++ b/sdk/src/research-gate.ts @@ -0,0 +1,94 @@ +/** + * Research gate — validates RESEARCH.md for unresolved open questions + * before allowing plan-phase to proceed (#1602). + * + * Pure functions: no I/O, no side effects. The caller reads the file + * and passes the content string. + */ + +// ─── Types ────────────────────────────────────────────────────────────────── + +export interface ResearchGateResult { + /** Whether research is clear to proceed to planning */ + pass: boolean; + /** Unresolved questions found (empty if pass=true) */ + unresolvedQuestions: string[]; +} + +// ─── Open questions detection ─────────────────────────────────────────────── + +/** + * Check RESEARCH.md content for unresolved open questions. + * + * Rules: + * - If no "## Open Questions" section exists → pass + * - If section header has "(RESOLVED)" suffix → pass + * - If section exists but is empty (only whitespace before next heading) → pass + * - Otherwise → fail with list of unresolved questions + */ +export function checkResearchGate(researchContent: string): ResearchGateResult { + // Find "## Open Questions" section (case-insensitive) + const sectionMatch = researchContent.match( + /^##\s+Open\s+Questions\b([^\n]*)/im, + ); + + if (!sectionMatch) { + return { pass: true, unresolvedQuestions: [] }; + } + + // Check for (RESOLVED) suffix on the heading + const headingSuffix = sectionMatch[1].trim(); + if (/\(resolved\)/i.test(headingSuffix)) { + return { pass: true, unresolvedQuestions: [] }; + } + + // Extract section content until next heading or EOF + const headingIndex = researchContent.indexOf(sectionMatch[0]); + const afterHeading = researchContent.slice(headingIndex + sectionMatch[0].length); + + // Find next heading at same or higher level + const nextHeadingMatch = afterHeading.match(/\n##\s+[^\n]/); + const sectionBody = nextHeadingMatch + ? afterHeading.slice(0, nextHeadingMatch.index) + : afterHeading; + + // Extract question items (numbered list or bullet points) + const unresolvedQuestions: string[] = []; + let totalQuestionLines = 0; + const lines = sectionBody.split('\n'); + + for (const line of lines) { + const trimmed = line.trim(); + // Match: "1. **Question**", "- **Question**", "* **Question**", "1. Question" + const questionMatch = trimmed.match( + /^(?:\d+[.)]\s*|\*\s+|-\s+)\*{0,2}([^*\n]+)\*{0,2}/, + ); + if (questionMatch) { + totalQuestionLines++; + const questionText = questionMatch[1].trim(); + // Skip questions marked as resolved inline (handles — RESOLVED, - RESOLVED, RESOLVED:, etc.) + if (!/\bresolved\b/i.test(trimmed)) { + unresolvedQuestions.push(questionText); + } + } + } + + // Empty section body → pass + if (sectionBody.trim() === '') { + return { pass: true, unresolvedQuestions: [] }; + } + + // All question lines were resolved → pass + if (totalQuestionLines > 0 && unresolvedQuestions.length === 0) { + return { pass: true, unresolvedQuestions: [] }; + } + + // Unresolved questions found → fail + if (unresolvedQuestions.length > 0) { + return { pass: false, unresolvedQuestions }; + } + + // Section has content but no parseable question lines → fail conservatively + // (e.g., prose-style questions without list formatting) + return { pass: false, unresolvedQuestions: ['(unstructured open questions detected — review ## Open Questions section)'] }; +}