diff --git a/DESIGN.md b/DESIGN.md index ca050dc..1759107 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -19,6 +19,10 @@ v2 is a rethink of how coder operates: from an MCP tool that Claude calls, to a --- +## Documentation + +- [Prompt Contracts](docs/prompt-contracts.md) — cross-prompt artifact section registry + ## Components ### 1. Daemon diff --git a/docs/prompt-contracts.md b/docs/prompt-contracts.md new file mode 100644 index 0000000..5cd1e11 --- /dev/null +++ b/docs/prompt-contracts.md @@ -0,0 +1,43 @@ +# Prompt Contracts + +## Why + +Machine prompts produce and consume structured artifacts (PLANREVIEW.md, PLAN.md, ISSUE.md, etc.). When section names are hardcoded in multiple files, they drift — a section added to a producer but missing from the consumer silently breaks downstream logic. Hard parsers like `parsePlanVerdict` also key off specific headings and fail silently when names change. + +## The Registry + +`src/machines/prompt-contracts.js` exports a `CONTRACTS` object keyed by artifact name. Each entry declares: + +- `producedBy` — file path of the producing machine +- `consumers` — file paths of consuming machines +- `format` — `"markdown"` or `"json"` +- `sections` — canonical section/field names +- `sectionDescriptions` — optional per-section prose appended by `renderRequiredSections` +- `findingFields` / `findingFieldExamples` — used by `REVIEW_FINDINGS.md` + +Render helpers convert entries into prompt-ready strings: + +- `renderRequiredSections(key)` — numbered list for heading-level output; appends `sectionDescriptions` after an em-dash when present +- `renderCritiqueSectionList(key)` — comma-joined actionable names (strips parentheticals, excludes Verdict) for inline use +- `renderFindingExample(key)` — bulleted `- **Field**: example` lines built from `findingFields` + `findingFieldExamples` +- `getSections(key)` — raw array for programmatic access + +## Adding a Section to an Existing Artifact + +1. Add the section name to the `sections` array in the corresponding `CONTRACTS` entry. +2. Run `node --test test/prompt-contracts.test.js` to verify the registry is consistent. +3. The producer and consumer prompts will pick up the change automatically via render helpers. + +## Adding a New Artifact + +1. Add a new key to `CONTRACTS` with `producedBy`, `consumers`, `format`, and `sections`. +2. Add corresponding tests in `test/prompt-contracts.test.js`. +3. Update producer/consumer machine files to import from the registry. + +## Test Enforcement + +`test/prompt-contracts.test.js` validates: + +- All producer/consumer files exist on disk +- Synthetic artifacts built from registry headings roundtrip through `parsePlanVerdict` and `parseReviewVerdict` +- Render helpers produce output containing all declared sections diff --git a/src/machines/develop/implementation.machine.js b/src/machines/develop/implementation.machine.js index d726a5f..11fb8bf 100644 --- a/src/machines/develop/implementation.machine.js +++ b/src/machines/develop/implementation.machine.js @@ -7,6 +7,7 @@ import { saveState, } from "../../state/workflow-state.js"; import { defineMachine } from "../_base.js"; +import { renderCritiqueSectionList } from "../prompt-contracts.js"; import { makeClaudeSessionId } from "./_session.js"; import { artifactPaths, @@ -131,18 +132,17 @@ Build upon existing correct work. Do not duplicate or revert it. ## Step 1: Address Critique Update ${paths.plan} to resolve every finding in the critique — -Critical Issues, Over-Engineering Concerns, Data Structure Review, -Concerns, and Questions. For each Question: answer it only when the -repo or ISSUE.md gives you an explicit, verifiable answer. When you -can't: -- **Blocking question** (affects required behavior, acceptance - criteria, or API/data-shape the implementation must match): STOP. - Record the question under an \`## Open Questions (BLOCKING)\` - section in ${paths.plan} and do NOT produce an implementation diff. - Let the workflow surface the blocker. -- **Non-blocking question** (style, naming, minor polish): record - the question and the working assumption under \`## Open Questions\` - in ${paths.plan}, then proceed. Make the assumption obvious so +${renderCritiqueSectionList("PLANREVIEW.md")}. Answer each open +question only when the repo or ISSUE.md gives you an explicit, +verifiable answer. When you can't: +- **Blocking** (affects required behavior, acceptance criteria, or + API/data-shape the implementation must match): STOP. Record the + open item under an \`## Open Questions (BLOCKING)\` section in + ${paths.plan} and do NOT produce an implementation diff. Let the + workflow surface the blocker. +- **Non-blocking** (style, naming, minor polish): record the open + item and the working assumption under \`## Open Questions\` in + ${paths.plan}, then proceed. Make the assumption obvious so reviewers can catch it. If the critique says REJECT, revise significantly before proceeding. diff --git a/src/machines/develop/issue-draft.machine.js b/src/machines/develop/issue-draft.machine.js index 4cd28fe..294a966 100644 --- a/src/machines/develop/issue-draft.machine.js +++ b/src/machines/develop/issue-draft.machine.js @@ -13,6 +13,7 @@ import { ScratchpadPersistence } from "../../state/persistence.js"; import { loadState, saveState } from "../../state/workflow-state.js"; import { buildIssueBranchName } from "../../worktrees.js"; import { defineMachine } from "../_base.js"; +import { renderSectionsWithDescriptions } from "../prompt-contracts.js"; import { artifactPaths, checkArtifactCollisions, @@ -362,9 +363,10 @@ preamble, no commentary). If you wrote it to disk via a tool, also output its contents to stdout. ## Required Sections (in order) -1. **Metadata** — Source, Issue ID, Repo Root (relative), Difficulty (1-5). -2. **Problem** — what's wrong or missing, with specific file/function refs. -3. **Requirements** — behavioral requirements in EARS syntax. Follow +${renderSectionsWithDescriptions("ISSUE.md", { + Metadata: "Source, Issue ID, Repo Root (relative), Difficulty (1-5).", + Problem: "what's wrong or missing, with specific file/function refs.", + Requirements: `behavioral requirements in EARS syntax. Follow these sentence forms, substituting concrete project-specific text for the \`<...>\` placeholders. **Do NOT emit the angle-bracket placeholders or backticks verbatim** — every requirement in the @@ -373,9 +375,9 @@ output its contents to stdout. - Event-driven: \`WHEN , the shall .\` - State-driven: \`WHILE , the shall .\` - Unwanted Behavior: \`IF , THEN the shall .\` - - Optional Feature: \`WHERE , the shall .\` -4. **Changes** — exactly which files change and how. -5. **Testing Strategy** — search the codebase for existing test files and + - Optional Feature: \`WHERE , the shall .\``, + Changes: "exactly which files change and how.", + "Testing Strategy": `search the codebase for existing test files and patterns first, then list: existing tests that cover related behavior (paths + what they test), the repo's test framework and conventions, and concrete new test cases (inputs, outputs, edge cases).${ @@ -385,11 +387,12 @@ output its contents to stdout. implementation agent should write BEFORE coding (test name, assertion, expected failure reason — e.g. missing function, wrong return value).` : ` For this low-complexity issue, test-after is acceptable if a failing-test-first approach isn't practical.` - } -6. **Verification** — a concrete shell command or test that proves the + }`, + Verification: `a concrete shell command or test that proves the fix works (e.g. \`npm test\`, \`node -e "..."\`, \`curl ...\`). - Downstream agents use this to close the feedback loop. -7. **Out of Scope** — what this does NOT include. + Downstream agents use this to close the feedback loop.`, + "Out of Scope": "what this does NOT include.", +})} `; const res = await agent.execute(issuePrompt, { diff --git a/src/machines/develop/plan-review.machine.js b/src/machines/develop/plan-review.machine.js index f1ad1b4..e902368 100644 --- a/src/machines/develop/plan-review.machine.js +++ b/src/machines/develop/plan-review.machine.js @@ -15,6 +15,7 @@ import { } from "../../helpers.js"; import { loadState, saveState } from "../../state/workflow-state.js"; import { defineMachine } from "../_base.js"; +import { renderRequiredSections } from "../prompt-contracts.js"; import { withSessionResume } from "./_session.js"; import { artifactPaths, @@ -88,17 +89,7 @@ function buildCritiqueRetryPrompt(planPath, critiquePath, round) { `You are resuming plan review in a **fresh session**. The prior attempt exited successfully but did not create ${critiquePath} and produced no capturable critique in output.\n\n` + `1. Read the implementation plan at **${planPath}** in full.\n` + `2. Write a critical plan critique as markdown to **${critiquePath}**.${roundNote}\n\n` + - `Required sections (in order):\n` + - `1. Critical Issues (Must Fix)\n` + - `2. Over-Engineering Concerns — flag speculative optimizations (caches,\n` + - ` memoization, fancy algorithms) that lack a measurement, and flag fancy\n` + - ` data structures where plain arrays/objects would work for the expected n\n` + - `3. Data Structure Review — is the plan carrying the right data shapes?\n` + - ` Would different data make the algorithm self-evident? Are core types\n` + - ` reused from elsewhere in the repo?\n` + - `4. Concerns (Should Address)\n` + - `5. Questions (Need Clarification)\n` + - `6. Verdict (REJECT | REVISE | PROCEED WITH CAUTION | APPROVED)\n\n` + + `Required sections (in order):\n${renderRequiredSections("PLANREVIEW.md")}\n\n` + `Constraints:\n` + `- Do not modify tracked files.\n` + `- Keep critique concrete with file-level references when possible.\n` + @@ -253,16 +244,7 @@ export default defineMachine({ const basePromptBody = `Review ${paths.plan} and write a critical plan critique to ${paths.critique}.${roundNote} Required sections (in order): -1. Critical Issues (Must Fix) -2. Over-Engineering Concerns — flag speculative optimizations (caches, - memoization, fancy algorithms) that lack a measurement, and flag fancy - data structures where plain arrays/objects would work for the expected n -3. Data Structure Review — is the plan carrying the right data shapes? - Would different data make the algorithm self-evident? Are core types - reused from elsewhere in the repo? -4. Concerns (Should Address) -5. Questions (Need Clarification) -6. Verdict (REJECT | REVISE | PROCEED WITH CAUTION | APPROVED) +${renderRequiredSections("PLANREVIEW.md")} Constraints: - Do not modify tracked files. diff --git a/src/machines/develop/planning.machine.js b/src/machines/develop/planning.machine.js index 110b286..4242c17 100644 --- a/src/machines/develop/planning.machine.js +++ b/src/machines/develop/planning.machine.js @@ -11,6 +11,7 @@ import { z } from "zod"; import { stripAgentNoise } from "../../helpers.js"; import { loadState, saveState } from "../../state/workflow-state.js"; import { defineMachine } from "../_base.js"; +import { renderSectionsWithDescriptions } from "../prompt-contracts.js"; import { executeWithSessionAuthRetry, makeClaudeSessionId, @@ -232,21 +233,23 @@ code is buggier and n is usually small. ## Phase 3: Write ${paths.plan} Required sections: -1. **Summary** — one paragraph. -2. **Approach** — which approach and why, referencing existing patterns. -3. **Files to Modify** — list each existing file with the specific - changes it needs. -4. **Files to Create** — only when absolutely necessary; prefer - modifying existing files. Justify each new file. -5. **Dependencies** — versions + justification. -6. **Testing Strategy** — reference ISSUE.md's strategy, list existing +${renderSectionsWithDescriptions("PLAN.md", { + Summary: "one paragraph.", + Approach: "which approach and why, referencing existing patterns.", + "Files to Modify": `list each existing file with the specific + changes it needs.`, + "Files to Create": `only when absolutely necessary; prefer + modifying existing files. Justify each new file.`, + Dependencies: "versions + justification.", + "Testing Strategy": `reference ISSUE.md's strategy, list existing related tests, describe new test cases (inputs, outputs, edges), and specify the test command. For difficulty ≥ 3, add a Red/Green TDD subsection listing the failing assertions to write BEFORE code (test name + expected failure reason — e.g. "ReferenceError: parseConfig is not defined"). For difficulty < 3, test-after is - acceptable only when a failing-test-first approach isn't practical. -7. **Out of Scope** — what this change does NOT include. + acceptable only when a failing-test-first approach isn't practical.`, + "Out of Scope": "what this change does NOT include.", +})} ## House Rules - Small scope: 1-3 files, existing utilities over new abstractions, diff --git a/src/machines/develop/quality-review.machine.js b/src/machines/develop/quality-review.machine.js index f1f3760..fd1bed9 100644 --- a/src/machines/develop/quality-review.machine.js +++ b/src/machines/develop/quality-review.machine.js @@ -11,6 +11,7 @@ import { } from "../../helpers.js"; import { loadState, saveState } from "../../state/workflow-state.js"; import { defineMachine } from "../_base.js"; +import { renderReviewFindingsTemplate } from "../prompt-contracts.js"; import { executeWithSessionAuthRetry, makeClaudeSessionId, @@ -147,16 +148,7 @@ Write findings to ${paths.reviewFindings} as markdown. Each finding is a minor), **File**, **Lines**, **Issue**, **Suggestion** — like this: \`\`\`markdown -# Review Findings — Round ${round} - -## Finding 1 -- **Severity**: major -- **File**: src/foo.js -- **Lines**: 42-58 -- **Issue**: -- **Suggestion**: - -## VERDICT: REVISE +${renderReviewFindingsTemplate("REVIEW_FINDINGS.md", round)} \`\`\` End the file with \`## VERDICT: APPROVED\` or \`## VERDICT: REVISE\` as diff --git a/src/machines/prompt-contracts.js b/src/machines/prompt-contracts.js new file mode 100644 index 0000000..a5ac143 --- /dev/null +++ b/src/machines/prompt-contracts.js @@ -0,0 +1,394 @@ +/** + * Cross-prompt artifact contracts — single source of truth for section names + * shared between producer and consumer machine prompts. + */ + +export const CONTRACTS = { + "PLANREVIEW.md": { + producedBy: "src/machines/develop/plan-review.machine.js", + consumers: ["src/machines/develop/implementation.machine.js"], + format: "markdown", + sections: [ + "Critical Issues (Must Fix)", + "Over-Engineering Concerns", + "Data Structure Review", + "Concerns (Should Address)", + "Questions (Need Clarification)", + "Verdict (REJECT | REVISE | PROCEED WITH CAUTION | APPROVED)", + ], + sectionDescriptions: { + "Over-Engineering Concerns": + "flag speculative optimizations (caches, memoization, fancy " + + "algorithms) that lack a measurement, and flag fancy data " + + "structures where plain arrays/objects would work for the " + + "expected n", + "Data Structure Review": + "is the plan carrying the right data shapes? Would different " + + "data make the algorithm self-evident? Are core types reused " + + "from elsewhere in the repo?", + }, + }, + "PLAN.md": { + producedBy: "src/machines/develop/planning.machine.js", + consumers: [ + "src/machines/develop/plan-review.machine.js", + "src/machines/develop/implementation.machine.js", + "src/machines/develop/quality-review.machine.js", + ], + format: "markdown", + sections: [ + "Summary", + "Approach", + "Files to Modify", + "Files to Create", + "Dependencies", + "Testing Strategy", + "Out of Scope", + ], + }, + "REVIEW_FINDINGS.md": { + producedBy: "src/machines/develop/quality-review.machine.js", + consumers: ["src/machines/develop/quality-review.machine.js"], + format: "markdown", + // Unlike the section-list contracts above, REVIEW_FINDINGS.md is a + // repeating-findings doc. Producers render the full template via + // renderReviewFindingsTemplate(), which pulls the heading, finding + // fields, and verdict line from this entry so nothing is hardcoded. + findingHeading: "Finding", + verdictHeading: "VERDICT", + verdictValues: ["APPROVED", "REVISE"], + findingFields: ["Severity", "File", "Lines", "Issue", "Suggestion"], + findingFieldExamples: { + Severity: "major", + File: "src/foo.js", + Lines: "42-58", + Issue: "", + Suggestion: "", + }, + }, + "ISSUE.md": { + producedBy: "src/machines/develop/issue-draft.machine.js", + consumers: [ + "src/machines/develop/planning.machine.js", + "src/machines/develop/implementation.machine.js", + "src/machines/develop/quality-review.machine.js", + ], + format: "markdown", + sections: [ + "Metadata", + "Problem", + "Requirements", + "Changes", + "Testing Strategy", + "Verification", + "Out of Scope", + ], + }, + "research/issue-backlog.json": { + producedBy: "src/machines/research/issue-synthesis.machine.js", + consumers: ["src/machines/research/issue-publish.machine.js"], + format: "json", + sections: ["issues", "assumptions", "open_questions"], + issueFields: [ + "id", + "title", + "objective", + "problem", + "changes", + "verification", + "out_of_scope", + "depends_on", + "priority", + "tags", + "estimated_effort", + "acceptance_criteria", + "testing_strategy", + "research_questions", + "risks", + "notes", + "references", + "validation", + ], + issueFieldExamples: { + id: "IDEA-01", + title: "string", + objective: "string", + problem: "string", + changes: ["string"], + verification: "string", + out_of_scope: ["string"], + depends_on: ["IDEA-00"], + priority: "P0|P1|P2|P3", + tags: ["string"], + estimated_effort: "string", + acceptance_criteria: ["string"], + testing_strategy: { + existing_tests: ["path/to/test — what it covers"], + new_tests: ["description of test to write and expected behavior"], + test_patterns: "brief note on repo's test framework/conventions", + }, + research_questions: ["string"], + risks: ["string"], + notes: "string", + references: [ + { + source: "github|show_hn|docs|other", + title: "string", + url: "string", + why: "string", + }, + ], + validation: { + mode: "bug_repro|poc|analysis", + status: "passed|failed|inconclusive|not_run", + method: "string", + evidence: ["string"], + limitations: ["string"], + }, + }, + }, + "research/spec-architect.json": { + producedBy: "src/machines/research/spec-architect.machine.js", + consumers: ["src/machines/research/spec-render.machine.js"], + format: "json", + modes: { + build: { sections: ["domains", "decisions", "phases", "issueSpecs"] }, + ingest: { sections: ["phases", "issueSpecs"] }, + }, + issueFields: [ + "title", + "objective", + "problem", + "changes", + "acceptance_criteria", + "priority", + "domain", + "depends_on", + "tags", + "estimated_effort", + "testing_strategy", + ], + issueFieldExamples: { + title: "string", + objective: "string", + problem: "string", + changes: ["string"], + acceptance_criteria: ["string"], + priority: "P0|P1|P2|P3", + domain: "string", + depends_on: [], + tags: ["string"], + estimated_effort: "string", + testing_strategy: { + existing_tests: [], + new_tests: ["string"], + test_patterns: "string", + }, + }, + modeExamples: { + build: { + domains: [{ name: "string", description: "string", gaps: ["string"] }], + decisions: [ + { + id: "ADR-NNN", + title: "string", + status: "proposed|accepted|deprecated|superseded", + rationale: "string", + }, + ], + phases: [{ id: "phase-N", title: "string", issueSpecs: [] }], + }, + ingest: { + phases: [ + { + id: "phase-N", + title: "string", + issueSpecs: [{ title: "matching issue title" }], + }, + ], + }, + }, + }, +}; + +/** + * Numbered list of all sections for a contract key. When a section has a + * matching entry in `sectionDescriptions`, the description is appended + * after an em-dash on the same line so the rendered output reads naturally + * inside prompts. + */ +export function renderRequiredSections(key) { + const entry = CONTRACTS[key]; + if (!entry) throw new Error(`Unknown contract: ${key}`); + if (!Array.isArray(entry.sections)) + throw new Error( + `renderRequiredSections(${key}): contract has no flat 'sections' array`, + ); + const descriptions = entry.sectionDescriptions || {}; + return entry.sections + .map((s, i) => { + const desc = descriptions[s]; + return desc ? `${i + 1}. ${s} — ${desc}` : `${i + 1}. ${s}`; + }) + .join("\n"); +} + +/** + * Comma-joined actionable section names (excludes Verdict sections), + * with parenthetical suffixes stripped for inline prompt use. + * Uses " and " before the last item — consumers must address *every* + * critique section, not any one of them. + */ +export function renderCritiqueSectionList(key) { + const entry = CONTRACTS[key]; + if (!entry) throw new Error(`Unknown contract: ${key}`); + if (!Array.isArray(entry.sections)) + throw new Error( + `renderCritiqueSectionList(${key}): contract has no flat 'sections' array`, + ); + const actionable = entry.sections + .filter((s) => !/^Verdict\b/.test(s)) + .map((s) => s.replace(/\s*\(.*\)$/, "")); + if (actionable.length <= 1) return actionable.join(""); + return `${actionable.slice(0, -1).join(", ")}, and ${actionable[actionable.length - 1]}`; +} + +/** Raw sections array for programmatic use. */ +export function getSections(key) { + const entry = CONTRACTS[key]; + if (!entry) throw new Error(`Unknown contract: ${key}`); + if (!Array.isArray(entry.sections)) + throw new Error( + `getSections(${key}): contract has no flat 'sections' array`, + ); + return entry.sections; +} + +/** + * Bulleted `- **Field**: example` lines for REVIEW_FINDINGS.md findings. + * Pulls field names from `findingFields` and example values from + * `findingFieldExamples` so producer and consumer share one source. + */ +export function renderFindingExample(key) { + const entry = CONTRACTS[key]; + if (!entry?.findingFields) + throw new Error(`Contract ${key} has no findingFields`); + const examples = entry.findingFieldExamples || {}; + return entry.findingFields + .map((f) => `- **${f}**: ${examples[f] ?? "..."}`) + .join("\n"); +} + +/** + * Full REVIEW_FINDINGS.md example block — the `# Review Findings` title, + * one `## Finding 1` sample with registry-sourced fields, and the + * `## VERDICT: REVISE` closing line. Single source of truth for the + * producer prompt so the template cannot drift from the registry. + */ +export function renderReviewFindingsTemplate(key, round) { + const entry = CONTRACTS[key]; + if (!entry?.findingHeading || !entry?.verdictHeading) + throw new Error(`Contract ${key} is not a review-findings contract`); + const defaultVerdict = + entry.verdictValues?.find((v) => v !== "APPROVED") ?? "REVISE"; + return [ + `# Review Findings — Round ${round}`, + "", + `## ${entry.findingHeading} 1`, + renderFindingExample(key), + "", + `## ${entry.verdictHeading}: ${defaultVerdict}`, + ].join("\n"); +} + +/** + * Numbered `N. **Section** — description` list where `descriptions` is a + * name-keyed map. Enforces that every section in the contract has a + * description and that no stray keys are passed — swapping, reordering, + * or forgetting a section fails loudly instead of silently misaligning + * the prompt text. + */ +export function renderSectionsWithDescriptions(key, descriptions) { + const entry = CONTRACTS[key]; + if (!entry) throw new Error(`Unknown contract: ${key}`); + if (!descriptions || typeof descriptions !== "object") + throw new Error( + `renderSectionsWithDescriptions(${key}): descriptions required`, + ); + const missing = entry.sections.filter((s) => !(s in descriptions)); + if (missing.length > 0) + throw new Error( + `renderSectionsWithDescriptions(${key}): missing descriptions for ${missing.join(", ")}`, + ); + const extra = Object.keys(descriptions).filter( + (k) => !entry.sections.includes(k), + ); + if (extra.length > 0) + throw new Error( + `renderSectionsWithDescriptions(${key}): unknown sections ${extra.join(", ")}`, + ); + return entry.sections + .map((s, i) => `${i + 1}. **${s}** — ${descriptions[s]}`) + .join("\n"); +} + +/** + * Build an example issue object from a contract's issueFields + + * issueFieldExamples. Throws if any declared issueField is missing an + * explicit example — silent fallback to "string" would hide schema + * drift for array/object-typed fields. + */ +export function buildIssueFieldsExample(key) { + const entry = CONTRACTS[key]; + if (!entry?.issueFields) + throw new Error(`Contract ${key} has no issueFields`); + const examples = entry.issueFieldExamples || {}; + const missing = entry.issueFields.filter((f) => !(f in examples)); + if (missing.length > 0) + throw new Error( + `Contract ${key}: issueFields missing from issueFieldExamples: ${missing.join(", ")}`, + ); + return Object.fromEntries(entry.issueFields.map((f) => [f, examples[f]])); +} + +/** + * Full JSON schema example string for the issue-backlog contract. + * Iterates named sections (not positional indices) — the "issues" slot + * holds an issue object built from `issueFields`, every other section + * is a placeholder string array. If the contract ever renames "issues" + * this helper throws loudly. + */ +export function renderIssueBacklogExample() { + const entry = CONTRACTS["research/issue-backlog.json"]; + if (!entry.sections.includes("issues")) + throw new Error( + "research/issue-backlog.json contract must include an 'issues' section", + ); + const issueExample = buildIssueFieldsExample("research/issue-backlog.json"); + return JSON.stringify( + Object.fromEntries( + entry.sections.map((s) => [ + s, + s === "issues" ? [issueExample] : ["string"], + ]), + ), + null, + 2, + ); +} + +/** Full JSON schema example string for the spec-architect contract (build or ingest mode). */ +export function renderSpecArchitectExample(mode) { + const entry = CONTRACTS["research/spec-architect.json"]; + const modeExample = entry.modeExamples?.[mode]; + if (!modeExample) + throw new Error( + `renderSpecArchitectExample: unknown mode "${mode}"; expected one of ${Object.keys(entry.modeExamples || {}).join(", ")}`, + ); + const issueExample = buildIssueFieldsExample("research/spec-architect.json"); + return JSON.stringify( + { ...modeExample, issueSpecs: [issueExample] }, + null, + 2, + ); +} diff --git a/src/machines/research/issue-synthesis.machine.js b/src/machines/research/issue-synthesis.machine.js index 379165b..681e69e 100644 --- a/src/machines/research/issue-synthesis.machine.js +++ b/src/machines/research/issue-synthesis.machine.js @@ -3,6 +3,7 @@ import path from "node:path"; import { z } from "zod"; import { sanitizeUserData } from "../../helpers.js"; import { checkCancel, defineMachine } from "../_base.js"; +import { renderIssueBacklogExample } from "../prompt-contracts.js"; import { appendScratchpad, ensureArtifactOnDisk, @@ -153,49 +154,7 @@ Rules: write with expected behavior, and the repo's test framework/conventions. Return ONLY valid JSON in this schema: -{ - "issues": [ - { - "id": "IDEA-01", - "title": "string", - "objective": "string", - "problem": "string", - "changes": ["string"], - "verification": "string", - "out_of_scope": ["string"], - "depends_on": ["IDEA-00"], - "priority": "P0|P1|P2|P3", - "tags": ["string"], - "estimated_effort": "string", - "acceptance_criteria": ["string"], - "testing_strategy": { - "existing_tests": ["path/to/test — what it covers"], - "new_tests": ["description of test to write and expected behavior"], - "test_patterns": "brief note on repo's test framework/conventions" - }, - "research_questions": ["string"], - "risks": ["string"], - "notes": "string", - "references": [ - { - "source": "github|show_hn|docs|other", - "title": "string", - "url": "string", - "why": "string" - } - ], - "validation": { - "mode": "bug_repro|poc|analysis", - "status": "passed|failed|inconclusive|not_run", - "method": "string", - "evidence": ["string"], - "limitations": ["string"] - } - } - ], - "assumptions": ["string"], - "open_questions": ["string"] -}`; +${renderIssueBacklogExample()}`; const draftRes = await runStructuredStep({ stepName: `draft_issue_backlog_${String(i).padStart(2, "0")}`, artifactName: `draft-${String(i).padStart(2, "0")}`, diff --git a/src/machines/research/spec-architect.machine.js b/src/machines/research/spec-architect.machine.js index 87a6b82..251e858 100644 --- a/src/machines/research/spec-architect.machine.js +++ b/src/machines/research/spec-architect.machine.js @@ -1,5 +1,6 @@ import { z } from "zod"; import { defineMachine } from "../_base.js"; +import { renderSpecArchitectExample } from "../prompt-contracts.js"; import { appendScratchpad, loadPipeline, @@ -57,30 +58,7 @@ Tasks: 6. Sequence issues within the same domain by populating the "depends_on" field with the titles of prerequisite issues Return ONLY valid JSON: -{ - "domains": [{"name": "string", "description": "string", "gaps": ["string"]}], - "decisions": [{"id": "ADR-NNN", "title": "string", "status": "proposed|accepted|deprecated|superseded", "rationale": "string"}], - "phases": [{"id": "phase-N", "title": "string", "issueSpecs": []}], - "issueSpecs": [ - { - "title": "string", - "objective": "string", - "problem": "string", - "changes": ["string"], - "acceptance_criteria": ["string"], - "priority": "P0|P1|P2|P3", - "domain": "string", - "depends_on": [], - "tags": ["string"], - "estimated_effort": "string", - "testing_strategy": { - "existing_tests": [], - "new_tests": ["string"], - "test_patterns": "string" - } - } - ] -}`; +${renderSpecArchitectExample("build")}`; const res = await runStructuredStep({ stepName: "spec_architect", @@ -146,28 +124,7 @@ ${phaseInstruction}Tasks: 6. Sequence issues within the same domain by populating the "depends_on" field with the titles of prerequisite issues Return ONLY valid JSON: -{ - "phases": [{"id": "phase-N", "title": "string", "issueSpecs": [{"title": "matching issue title"}]}], - "issueSpecs": [ - { - "title": "string", - "objective": "string", - "problem": "string", - "changes": ["string"], - "acceptance_criteria": ["string"], - "priority": "P0|P1|P2|P3", - "domain": "string", - "depends_on": [], - "tags": ["string"], - "estimated_effort": "string", - "testing_strategy": { - "existing_tests": [], - "new_tests": ["string"], - "test_patterns": "string" - } - } - ] -}`; +${renderSpecArchitectExample("ingest")}`; const res = await runStructuredStep({ stepName: "spec_architect", diff --git a/src/systemd-run.js b/src/systemd-run.js index 29bac60..4042e61 100644 --- a/src/systemd-run.js +++ b/src/systemd-run.js @@ -194,7 +194,7 @@ export function runShellSync( const fallbackEnv = { ...(env || process.env) }; delete fallbackEnv.CLAUDECODE; - const res = spawnSync("bash", ["-c", maybeTmpFile(command)], { + const res = spawnSync("bash", ["-lc", maybeTmpFile(command)], { cwd, env: fallbackEnv, encoding: "utf8", @@ -203,7 +203,7 @@ export function runShellSync( }); const io = withSpawnErrorText(res); return { - cmd: ["bash", "-c", command], + cmd: ["bash", "-lc", command], exitCode: safeStatus(res), stdout: io.stdout, stderr: io.stderr, diff --git a/test/prompt-contracts.test.js b/test/prompt-contracts.test.js new file mode 100644 index 0000000..11fafe2 --- /dev/null +++ b/test/prompt-contracts.test.js @@ -0,0 +1,447 @@ +import assert from "node:assert/strict"; +import { + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; +import { parsePlanVerdict } from "../src/machines/develop/plan-review.machine.js"; +import { parseReviewVerdict } from "../src/machines/develop/quality-review.machine.js"; +import { + CONTRACTS, + getSections, + renderCritiqueSectionList, + renderIssueBacklogExample, + renderRequiredSections, + renderReviewFindingsTemplate, + renderSectionsWithDescriptions, + renderSpecArchitectExample, +} from "../src/machines/prompt-contracts.js"; +import { renderIdeaIssueMarkdown } from "../src/machines/research/_shared.js"; + +// Resolve contract file paths (which are repo-relative strings like +// "src/machines/develop/planning.machine.js") against the repo root derived +// from this test file's location, not process.cwd(). This lets the test +// suite be invoked from any working directory without false failures. +const repoRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); +const resolveContractPath = (p) => path.resolve(repoRoot, p); + +// --------------------------------------------------------------------------- +// Shape validation +// --------------------------------------------------------------------------- + +test("every producer and consumer file in CONTRACTS exists", () => { + for (const [key, entry] of Object.entries(CONTRACTS)) { + assert.ok( + existsSync(resolveContractPath(entry.producedBy)), + `producer for ${key} not found: ${entry.producedBy}`, + ); + for (const consumer of entry.consumers) { + assert.ok( + existsSync(resolveContractPath(consumer)), + `consumer of ${key} not found: ${consumer}`, + ); + } + } +}); + +// Dedicated helpers bind 1:1 to a specific contract key. When a +// producer calls one, we don't also need the literal key string to +// appear in the source — the helper itself carries the reference. +const DEDICATED_HELPERS = { + "research/issue-backlog.json": "renderIssueBacklogExample", + "research/spec-architect.json": "renderSpecArchitectExample", +}; + +test("every producer file imports the registry AND references its contract key", () => { + // Two-step check. The import ensures the producer is at least + // plugged into the registry. The literal contract-key (or dedicated + // helper) check makes sure the producer is pulling from the + // ARTIFACT'S OWN entry, not just importing some unrelated helper + // while re-hardcoding its section names elsewhere. + const importRe = /from\s+["']\.\.?\/prompt-contracts(?:\.js)?["']/; + for (const [key, entry] of Object.entries(CONTRACTS)) { + const src = readFileSync(resolveContractPath(entry.producedBy), "utf8"); + assert.ok( + importRe.test(src), + `${entry.producedBy} (producer for ${key}) does not import from prompt-contracts.js`, + ); + const referencesKey = src.includes(`"${key}"`) || src.includes(`'${key}'`); + const dedicatedHelper = DEDICATED_HELPERS[key]; + const referencesDedicated = + dedicatedHelper && src.includes(dedicatedHelper); + assert.ok( + referencesKey || referencesDedicated, + `${entry.producedBy} (producer for ${key}) never passes "${key}" to a registry helper and does not call its dedicated helper`, + ); + } +}); + +// The vast majority of consumer-contract pairs are PASSIVE: the consumer +// reads the artifact file's content as opaque text or destructures JS +// fields from parsed data, and never embeds the contract's section names +// back into a prompt. Passive consumers have no section-name drift risk, +// so they are exempt from the literal-key requirement below. +// +// ACTIVE consumer-contract pairs embed the contract's section names in +// their own prompts. For those, we require the literal contract key (or +// a dedicated helper) to appear in the consumer source — mirroring the +// producer test from round 4 — so drift fails loudly. +const ACTIVE_CONSUMER_PAIRS = [ + // implementation.machine.js instructs the programmer to address every + // PLANREVIEW.md critique section, and pulls that list from the + // registry via renderCritiqueSectionList("PLANREVIEW.md"). + { + consumer: "src/machines/develop/implementation.machine.js", + key: "PLANREVIEW.md", + }, +]; + +test("every active consumer pair references its contract key", () => { + const importRe = /from\s+["']\.\.?\/prompt-contracts(?:\.js)?["']/; + for (const { consumer, key } of ACTIVE_CONSUMER_PAIRS) { + assert.ok( + CONTRACTS[key]?.consumers.includes(consumer), + `${consumer} is not listed as a consumer of ${key} in CONTRACTS`, + ); + const src = readFileSync(resolveContractPath(consumer), "utf8"); + assert.ok( + importRe.test(src), + `${consumer} (active consumer of ${key}) does not import from prompt-contracts.js`, + ); + assert.ok( + src.includes(`"${key}"`) || src.includes(`'${key}'`), + `${consumer} (active consumer of ${key}) never passes "${key}" to a registry helper`, + ); + } +}); + +// --------------------------------------------------------------------------- +// Parser roundtrips +// --------------------------------------------------------------------------- + +test("parsePlanVerdict extracts verdict from synthetic registry artifact", () => { + const sections = CONTRACTS["PLANREVIEW.md"].sections; + const md = sections + .map((s) => { + if (/^Verdict\b/.test(s)) return `## ${s}\nAPPROVED`; + return `## ${s}\nNo issues found.`; + }) + .join("\n\n"); + assert.equal(parsePlanVerdict(md), "APPROVED"); +}); + +test("parseReviewVerdict extracts verdict from synthetic registry artifact", () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), "contract-test-")); + const filePath = path.join(tmp, "REVIEW_FINDINGS.md"); + try { + const fields = CONTRACTS["REVIEW_FINDINGS.md"].findingFields; + const finding = fields.map((f) => `- **${f}**: test`).join("\n"); + const md = `# Review Findings — Round 1\n\n## Finding 1\n${finding}\n\n## VERDICT: APPROVED\n`; + writeFileSync(filePath, md); + const result = parseReviewVerdict(filePath); + assert.equal(result.verdict, "APPROVED"); + // `findings` is the full markdown content — prove the parser preserved + // every declared field heading, not just that the string was truthy. + for (const field of fields) { + assert.ok( + result.findings.includes(`**${field}**`), + `parsed content missing field ${field}`, + ); + } + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +}); + +// --------------------------------------------------------------------------- +// Render helpers +// --------------------------------------------------------------------------- + +test("renderRequiredSections includes all contract headings", () => { + const rendered = renderRequiredSections("PLANREVIEW.md"); + for (const section of CONTRACTS["PLANREVIEW.md"].sections) { + assert.ok( + rendered.includes(section), + `renderRequiredSections missing: ${section}`, + ); + } +}); + +test("renderCritiqueSectionList excludes Verdict and strips parentheticals", () => { + const rendered = renderCritiqueSectionList("PLANREVIEW.md"); + assert.ok(rendered.includes("Critical Issues"), "missing Critical Issues"); + assert.ok( + rendered.includes("Over-Engineering Concerns"), + "missing Over-Engineering Concerns", + ); + assert.ok(!rendered.includes("Verdict"), "Verdict should be excluded"); + assert.ok( + !rendered.includes("(Must Fix)"), + "parentheticals should be stripped", + ); + // Must read as a universal "address every section" list, not alternatives. + assert.ok( + /, and \w/.test(rendered), + "must use ' and ' as the final conjunction, not ' or '", + ); + assert.ok(!rendered.includes(", or "), "must not use ' or ' conjunction"); +}); + +test("getSections returns correct section array for ISSUE.md", () => { + assert.deepEqual(getSections("ISSUE.md"), [ + "Metadata", + "Problem", + "Requirements", + "Changes", + "Testing Strategy", + "Verification", + "Out of Scope", + ]); +}); + +// --------------------------------------------------------------------------- +// JSON schema render helpers +// --------------------------------------------------------------------------- + +test("every issueField has an explicit entry in issueFieldExamples", () => { + // Guards against buildIssueFieldsExample silently falling back to the + // generic "string" placeholder when a new array/object-typed field is + // added without a corresponding example. + for (const key of [ + "research/issue-backlog.json", + "research/spec-architect.json", + ]) { + const entry = CONTRACTS[key]; + for (const field of entry.issueFields) { + assert.ok( + entry.issueFieldExamples && field in entry.issueFieldExamples, + `${key}: issueField "${field}" is missing from issueFieldExamples`, + ); + } + } +}); + +test("renderIssueBacklogExample produces valid JSON with all contract fields", () => { + const json = JSON.parse(renderIssueBacklogExample()); + const entry = CONTRACTS["research/issue-backlog.json"]; + for (const section of entry.sections) { + assert.ok(section in json, `missing top-level section: ${section}`); + } + // Issue objects must live under the literal "issues" slot — not + // whatever section happens to be first in the array. This catches + // silent misplacement when the sections array is reordered. + assert.ok(Array.isArray(json.issues), "'issues' must be an array"); + const issue = json.issues[0]; + assert.equal(typeof issue, "object", "'issues[0]' must be an object"); + for (const field of entry.issueFields) { + assert.ok(field in issue, `missing issue field: ${field}`); + } +}); + +test("renderSpecArchitectExample build mode includes exactly the contract sections", () => { + const json = JSON.parse(renderSpecArchitectExample("build")); + const entry = CONTRACTS["research/spec-architect.json"]; + // Exact key equality — catches stale keys from modeExamples after a + // section is removed from modes.build.sections. + assert.deepEqual( + Object.keys(json).sort(), + [...entry.modes.build.sections].sort(), + ); + const issue = json.issueSpecs[0]; + for (const field of entry.issueFields) { + assert.ok(field in issue, `missing issue field: ${field}`); + } +}); + +test("renderSpecArchitectExample ingest mode includes exactly the contract sections", () => { + const json = JSON.parse(renderSpecArchitectExample("ingest")); + const entry = CONTRACTS["research/spec-architect.json"]; + assert.deepEqual( + Object.keys(json).sort(), + [...entry.modes.ingest.sections].sort(), + ); + const issue = json.issueSpecs[0]; + for (const field of entry.issueFields) { + assert.ok(field in issue, `missing issue field: ${field}`); + } +}); + +test("renderSpecArchitectExample throws on unknown mode", () => { + assert.throws( + () => renderSpecArchitectExample("nope"), + /unknown mode "nope"/, + ); +}); + +// --------------------------------------------------------------------------- +// renderReviewFindingsTemplate +// --------------------------------------------------------------------------- + +test("renderReviewFindingsTemplate produces a parser-compatible block", () => { + const tmp = mkdtempSync(path.join(os.tmpdir(), "contract-test-")); + const filePath = path.join(tmp, "REVIEW_FINDINGS.md"); + try { + const block = renderReviewFindingsTemplate("REVIEW_FINDINGS.md", 2); + // Replace example verdict with APPROVED so parseReviewVerdict agrees. + const md = block.replace(/VERDICT:\s*REVISE/, "VERDICT: APPROVED"); + writeFileSync(filePath, `${md}\n`); + const parsed = parseReviewVerdict(filePath); + assert.equal(parsed.verdict, "APPROVED"); + // Heading/verdict text is sourced from the registry — check it matches. + const entry = CONTRACTS["REVIEW_FINDINGS.md"]; + assert.ok(block.includes(`## ${entry.findingHeading} 1`)); + assert.ok(block.includes(`## ${entry.verdictHeading}:`)); + for (const field of entry.findingFields) { + assert.ok(block.includes(`**${field}**`), `missing field ${field}`); + } + // Prove the parser preserved every field in its output, not just + // that `findings` was truthy. + for (const field of entry.findingFields) { + assert.ok( + parsed.findings.includes(`**${field}**`), + `parsed content missing field ${field}`, + ); + } + } finally { + rmSync(tmp, { recursive: true, force: true }); + } +}); + +// --------------------------------------------------------------------------- +// renderSectionsWithDescriptions — name-keyed, drift-proof +// --------------------------------------------------------------------------- + +test("renderSectionsWithDescriptions numbers every contract section", () => { + const descriptions = { + Summary: "S", + Approach: "A", + "Files to Modify": "FM", + "Files to Create": "FC", + Dependencies: "D", + "Testing Strategy": "T", + "Out of Scope": "O", + }; + const rendered = renderSectionsWithDescriptions("PLAN.md", descriptions); + const sections = getSections("PLAN.md"); + sections.forEach((s, i) => { + assert.ok( + rendered.includes(`${i + 1}. **${s}** — ${descriptions[s]}`), + `missing row for ${s}`, + ); + }); +}); + +test("renderSectionsWithDescriptions throws on missing keys", () => { + assert.throws( + () => + renderSectionsWithDescriptions("PLAN.md", { + Summary: "S", + Approach: "A", + }), + /missing descriptions for/, + ); +}); + +test("renderSectionsWithDescriptions throws on unknown keys", () => { + const entry = CONTRACTS["PLAN.md"]; + const descriptions = Object.fromEntries(entry.sections.map((s) => [s, "x"])); + descriptions.Stowaway = "extra"; + assert.throws( + () => renderSectionsWithDescriptions("PLAN.md", descriptions), + /unknown sections Stowaway/, + ); +}); + +// --------------------------------------------------------------------------- +// Issue-backlog contract ↔ renderIdeaIssueMarkdown alignment (Finding 3) +// --------------------------------------------------------------------------- + +test("renderIdeaIssueMarkdown renders all issue-backlog contract fields", () => { + const entry = CONTRACTS["research/issue-backlog.json"]; + // Build a sample issue with recognizable values for every contract field + const sampleIssue = { + id: "TEST-42", + title: "Contract alignment test issue", + objective: "OBJECTIVE_MARKER_abc", + problem: "PROBLEM_MARKER_xyz", + changes: ["CHANGE_MARKER_one", "CHANGE_MARKER_two"], + verification: "VERIFY_MARKER_cmd", + out_of_scope: ["OUTSCOPE_MARKER"], + depends_on: ["IDEA-99"], + priority: "P1", + tags: ["TAG_MARKER_alpha"], + estimated_effort: "EFFORT_MARKER_2d", + acceptance_criteria: ["AC_MARKER_first"], + testing_strategy: { + existing_tests: ["EXISTTEST_MARKER_path"], + new_tests: ["NEWTEST_MARKER_desc"], + test_patterns: "TESTPAT_MARKER_note", + }, + research_questions: ["RQ_MARKER_question"], + risks: ["RISK_MARKER_item"], + notes: "NOTES_MARKER_text", + references: [ + { + source: "github", + title: "REF_MARKER_title", + url: "https://REF_MARKER_URL", + why: "REF_MARKER_why", + }, + ], + validation: { + mode: "poc", + status: "passed", + method: "VALMETHOD_MARKER", + evidence: ["VALEVIDENCE_MARKER"], + limitations: ["VALLIMIT_MARKER"], + }, + }; + + const rendered = renderIdeaIssueMarkdown({ + issue: sampleIssue, + issueId: sampleIssue.id, + title: sampleIssue.title, + repoPath: ".", + pointers: "test pointers", + scratchpadRelPath: ".coder/scratchpad.md", + }); + + // Verify every contract field's value appears in the rendered output + const markers = { + id: "TEST-42", + title: "Contract alignment test issue", + objective: "OBJECTIVE_MARKER_abc", + problem: "PROBLEM_MARKER_xyz", + changes: "CHANGE_MARKER_one", + verification: "VERIFY_MARKER_cmd", + out_of_scope: "OUTSCOPE_MARKER", + depends_on: "IDEA-99", + priority: "P1", + tags: "TAG_MARKER_alpha", + estimated_effort: "EFFORT_MARKER_2d", + acceptance_criteria: "AC_MARKER_first", + testing_strategy: "EXISTTEST_MARKER_path", + research_questions: "RQ_MARKER_question", + risks: "RISK_MARKER_item", + notes: "NOTES_MARKER_text", + references: "REF_MARKER_title", + validation: "VALMETHOD_MARKER", + }; + + for (const field of entry.issueFields) { + const marker = markers[field]; + assert.ok( + marker && rendered.includes(marker), + `renderIdeaIssueMarkdown does not render contract field "${field}" (expected marker: ${marker})`, + ); + } +}); diff --git a/test/repro-gh-60.test.js b/test/repro-gh-60.test.js index 9cc6b05..635b0a3 100644 --- a/test/repro-gh-60.test.js +++ b/test/repro-gh-60.test.js @@ -36,3 +36,12 @@ test("GH-60: strip CLAUDECODE from runShellSync", async () => { else process.env.CLAUDECODE = orig; } }); + +test("runShellSync fallback uses login shell (bash -lc)", () => { + const res = runShellSync("shopt login_shell", { preferSystemd: false }); + assert.match( + res.stdout, + /login_shell\s+on/, + "fallback path should use bash -lc so user profile PATH is available", + ); +});