diff --git a/DESIGN.md b/DESIGN.md index ca050dc..1d354ae 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -211,6 +211,8 @@ coder run develop --repo ./api --input "$(coder artifacts get ) Internally, the artifact JSON is validated against the downstream workflow's input schema before dispatch. Validation failure is a hard error with a clear message. +See also: [Cross-Prompt Contract Patterns](docs/prompt-contracts.md) for the section heading specifications used to validate artifact outputs. + --- ### 6. Agent Skill diff --git a/docs/prompt-contracts.md b/docs/prompt-contracts.md new file mode 100644 index 0000000..d16b4c0 --- /dev/null +++ b/docs/prompt-contracts.md @@ -0,0 +1,77 @@ +# Cross-Prompt Contract Registry + +`src/machines/prompt-contracts.js` is the single source of truth for artifact +section headings, producer/consumer file paths, and JSON payload field schemas +shared across develop and research machine prompts. + +## Why + +Prompt builders in different machines independently define the same section +headings. When a heading is renamed in the producer, consumers silently +reference a stale name, causing the agent to skip sections. The registry +eliminates this by centralizing section names with render helpers that surface +mismatches at test time. + +## Access Patterns + +Three tiers serve different prompt-assembly needs: + +| Pattern | When to use | Example | +|---------|-------------|---------| +| `renderRequiredSections(key)` | Plain numbered list, no inline prose | `plan-review.machine.js` | +| `renderCritiqueSectionList(key, names)` | Prose references; **throws** if name not found | `implementation.machine.js` | +| `CONTRACTS[key].sections` iteration | Interleaved instructional prose between items | `planning.machine.js`, `issue-draft.machine.js` | + +### `renderRequiredSections(contractKey)` + +Returns a numbered list string: + +``` +1. Critical Issues (Must Fix) +2. Over-Engineering Concerns +... +``` + +### `renderCritiqueSectionList(contractKey, sectionNames)` + +Returns a comma-joined string of the requested names. Throws an `Error` +(matching `/not found/`) if any name is absent from the registry. + +### Direct `CONTRACTS[key].sections` iteration + +For prompts that need to insert conditional or multi-line prose after specific +sections (e.g., EARS syntax after "Requirements", Red/Green TDD after "Testing +Strategy"), iterate the sections array and use **name-based checks**: + +```js +CONTRACTS["ISSUE.md"].sections.map((s, i) => { + let line = `${i + 1}. **${s.name}**: ${s.description}`; + if (s.name === "Requirements") { + line += ":\n - Ubiquitous: ..."; + } + return line; +}).join("\n") +``` + +## Adding a Section + +1. Add the `{ name, description }` entry to the appropriate `CONTRACTS` key in + `src/machines/prompt-contracts.js`. +2. Update the producer machine to emit the new section heading. +3. Update consumer machines that reference sections (the throw-on-miss helper + catches stale references at test time). +4. Run `node ./scripts/run-tests.mjs test/prompt-contracts.test.js` to verify. + +## Adding an Artifact + +1. Add a new key to `CONTRACTS` with `description`, `producedBy`, `consumers`, + and either `sections` (markdown) or `fields` (JSON). +2. Update the test expectations in `test/prompt-contracts.test.js` (the + `expectedKeys` array). +3. Run the full suite: `npm test`. + +## JSON Payload Contracts + +Research machines use `fields` entries (e.g., `{ issues: "array" }`) with +`requirePayloadFields()` from `research/_shared.js`. Changing a field name in +the registry automatically updates the validation call site. diff --git a/src/helpers.js b/src/helpers.js index 4ff7214..18e9480 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -3,6 +3,7 @@ import { createHash } from "node:crypto"; import { existsSync, readFileSync, writeFileSync } from "node:fs"; import path from "node:path"; import { jsonrepair } from "jsonrepair"; +import { CONTRACTS } from "./machines/prompt-contracts.js"; import { DEFAULT_PASS_ENV } from "./pass-env.js"; import { runPpcommitBranch, runPpcommitNative } from "./ppcommit.js"; import { runShellSync } from "./systemd-run.js"; @@ -679,14 +680,28 @@ export function runPlanreview(repoDir, planPath, critiquePath) { } /** - * Run plan review using gemini CLI directly (with native search grounding). - * This alternative to the planreview tool better leverages Gemini's - * ability to search for and verify external API documentation. + * Build the Gemini plan-review prompt from CONTRACTS["PLANREVIEW.md"] sections. + * Exported for testing — ensures Gemini review headings stay in sync with the registry. */ -export function runPlanreviewWithGemini(repoDir, planPath, critiquePath) { - const planContent = readFileSync(planPath, "utf8"); +export function buildGeminiReviewPrompt(planContent) { + const critiqueSections = CONTRACTS["PLANREVIEW.md"].sections; + const sectionMarkdown = critiqueSections + .map((s) => { + if (s.name.startsWith("Verdict")) { + return `### ${s.name} +IMPORTANT: Your verdict line MUST be exactly one of these four options, with no other words: +- REJECT +- REVISE +- PROCEED WITH CAUTION +- APPROVED + +Do NOT paraphrase (e.g. do not write "Needs Revision" or "Needs Rework"). Write the exact keyword.`; + } + return `### ${s.name}\n${s.description}.`; + }) + .join("\n\n"); - const reviewPrompt = `You are a rigorous, experienced senior principal engineer reviewing a technical plan. + return `You are a rigorous, experienced senior principal engineer reviewing a technical plan. ## CRITICAL: Verify External Dependencies @@ -740,29 +755,20 @@ ${planContent} After verifying external APIs via search, provide your critique: -### Critical Issues (Must Fix) -Issues that would cause the plan to fail or violate constraints. - -### Over-Engineering Concerns -Unnecessary complexity, abstractions, or scope creep. - -### Concerns (Should Address) -Problems that should be addressed but won't cause immediate failure. - -### Questions (Need Clarification) -Ambiguities or assumptions that need to be verified. - -### Verdict -IMPORTANT: Your verdict line MUST be exactly one of these four options, with no other words: -- REJECT -- REVISE -- PROCEED WITH CAUTION -- APPROVED - -Do NOT paraphrase (e.g. do not write "Needs Revision" or "Needs Rework"). Write the exact keyword. +${sectionMarkdown} Be specific. Reference what you found in your searches about the external APIs. Reference specific sections in the plan when identifying over-engineering.`; +} + +/** + * Run plan review using gemini CLI directly (with native search grounding). + * This alternative to the planreview tool better leverages Gemini's + * ability to search for and verify external API documentation. + */ +export function runPlanreviewWithGemini(repoDir, planPath, critiquePath) { + const planContent = readFileSync(planPath, "utf8"); + const reviewPrompt = buildGeminiReviewPrompt(planContent); // Use gemini CLI with yolo mode and text output const cmd = heredocPipe(reviewPrompt, "gemini --yolo -o text"); diff --git a/src/machines/develop/implementation.machine.js b/src/machines/develop/implementation.machine.js index c4aaf29..4fddc1c 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, @@ -130,7 +131,7 @@ Build upon existing correct work. Do not duplicate or revert it. const implPrompt = `${recoveryContext}Read ${paths.plan} and ${paths.critique}. ## Step 1: Address Critique -Update ${paths.plan} to address any Critical Issues or Over-Engineering Concerns from the critique. +Update ${paths.plan} to address any ${renderCritiqueSectionList("PLANREVIEW.md", ["Critical Issues (Must Fix)", "Over-Engineering Concerns"])} from the critique. If critique says REJECT, revise the plan significantly before proceeding. ${ useRedGreen diff --git a/src/machines/develop/issue-draft.machine.js b/src/machines/develop/issue-draft.machine.js index 8d3f7b9..e5a9ab7 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 { CONTRACTS } from "../prompt-contracts.js"; import { artifactPaths, checkArtifactCollisions, @@ -359,16 +360,18 @@ Output ONLY markdown suitable for writing directly to ISSUE.md. If you wrote ISSUE.md to disk via a tool, also output its full contents to stdout. ## Required Sections (in order) -1. **Metadata**: Source, Issue ID, Repo Root (relative path), Difficulty (1-5) -2. **Problem**: What's wrong or missing — reference specific files/functions -3. **Requirements**: Behavioral requirements using EARS Syntax Patterns: +${CONTRACTS["ISSUE.md"].sections + .map((s, i) => { + let line = `${i + 1}. **${s.name}**: ${s.description}`; + if (s.name === "Requirements") { + line += `: - Ubiquitous: The shall . - 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 need to change and how -5. **Testing Strategy**: Search the codebase for existing test files/patterns, then specify: + - Optional Feature: WHERE , the shall .`; + } else if (s.name === "Testing Strategy") { + line += `. Search the codebase for existing test files/patterns, then specify: - **Existing tests**: Which test files cover related behavior (paths + what they test) - **Test patterns**: The repo's test framework, conventions, assertion style - **New test cases**: Concrete test cases to write — inputs, expected outputs, edge cases @@ -379,9 +382,13 @@ If you wrote ISSUE.md to disk via a tool, also output its full contents to stdou - Test name, assertion, and expected failure reason (missing function, wrong return value, etc.) - These form the RED phase — they must fail for the right reasons before implementation begins` : `- For this low-complexity issue, a lightweight test-after approach is acceptable if a failing-test-first approach isn't practical` - } -6. **Verification**: A concrete shell command or test to prove the fix works (e.g. \`npm test\`, \`node -e "..."\`, \`curl ...\`). This is critical — downstream agents use this to close the feedback loop. -7. **Out of Scope**: What this does NOT include + }`; + } else if (s.name === "Verification") { + line += ` (e.g. \`npm test\`, \`node -e "..."\`, \`curl ...\`). This is critical — downstream agents use this to close the feedback loop.`; + } + return line; + }) + .join("\n")} `; 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 f22dbc3..b8581c5 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, @@ -78,7 +79,7 @@ function tryWriteCritiqueFromStdout(reviewRes, critiquePath) { * Fresh-session retry: same task spec as the primary review prompt so the agent * reads PLAN.md and applies round/constraints — not a generic template critique. */ -function buildCritiqueRetryPrompt(planPath, critiquePath, round) { +export function buildCritiqueRetryPrompt(planPath, critiquePath, round) { const roundNote = round > 0 ? `\n\nNote: This is revision round ${round + 1}. The prior plan was rejected. Focus on whether the issues from the prior critique have been addressed.` @@ -89,11 +90,8 @@ function buildCritiqueRetryPrompt(planPath, critiquePath, round) { `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\n` + - `3. Concerns (Should Address)\n` + - `4. Questions (Need Clarification)\n` + - `5. Verdict (REJECT | REVISE | PROCEED WITH CAUTION | APPROVED)\n\n` + + renderRequiredSections("PLANREVIEW.md") + + `\n\n` + `Constraints:\n` + `- Do not modify tracked files.\n` + `- Keep critique concrete with file-level references when possible.\n` + @@ -248,11 +246,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 -3. Concerns (Should Address) -4. Questions (Need Clarification) -5. 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 5e21eb2..6650b52 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 { CONTRACTS } from "../prompt-contracts.js"; import { executeWithSessionAuthRetry, makeClaudeSessionId, @@ -229,21 +230,22 @@ Select the simplest approach that solves the problem. ## Phase 3: Write Plan to ${paths.plan} Structure: -1. **Summary**: One paragraph describing what will change -2. **Approach**: Which approach and why (reference existing patterns) -3. **Files to Modify**: List each file with specific changes -4. **Files to Create**: Only if absolutely necessary (prefer modifying existing files) -5. **Dependencies**: Any new dependencies with version and justification -6. **Testing Strategy**: - - Reference the testing strategy from ISSUE.md if present +${CONTRACTS["PLAN.md"].sections + .map((s, i) => { + let line = `${i + 1}. **${s.name}**: ${s.description}`; + if (s.name === "Testing Strategy") { + line += `:\n - Reference the testing strategy from ISSUE.md if present - List existing test files that validate related behavior - Describe specific test cases to write (inputs, expected outputs, edge cases) - Specify the test command to run - **Red/Green TDD** (when ISSUE.md difficulty >= 3 or change is non-trivial): - RED phase: List the exact test files to create/modify and the failing assertions to write BEFORE implementation. Each test should target one requirement from ISSUE.md. Describe the expected failure (e.g. "ReferenceError: parseConfig is not defined", "Expected 3 but got undefined"). - GREEN phase: Which files to implement and in what order to make tests pass incrementally. - - If ISSUE.md difficulty < 3 and the change is straightforward, note that test-after is acceptable. -7. **Out of Scope**: Explicitly list what this change does NOT include + - If ISSUE.md difficulty < 3 and the change is straightforward, note that test-after is acceptable.`; + } + return line; + }) + .join("\n")} ## Complexity Budget - Prefer modifying 1-3 files over touching many files diff --git a/src/machines/develop/quality-review.machine.js b/src/machines/develop/quality-review.machine.js index 9f1a79a..b004314 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 { CONTRACTS } from "../prompt-contracts.js"; import { executeWithSessionAuthRetry, makeClaudeSessionId, @@ -44,20 +45,17 @@ export function parseReviewVerdict(filePath) { } export function buildSpecDeltaPrompt(issuePath, planPath) { + const deltaSections = CONTRACTS.SPEC_DELTA.sections + .map((s) => `### ${s.name}\n(${s.description})`) + .join("\n\n"); + return `Compare ${issuePath} (original requirements) with ${planPath} (technical plan). Write a concise "Spec Delta Summary" with these sections: ## Spec Delta Summary -### Additions -(New technical constraints or approaches introduced in the plan) - -### Refinements -(Changes in approach from the original issue) - -### Omissions -(Requirements deferred or removed in the plan) +${deltaSections} Keep each section to 2-5 bullets max. Output only the summary, no other text.`; } diff --git a/src/machines/prompt-contracts.js b/src/machines/prompt-contracts.js new file mode 100644 index 0000000..4062833 --- /dev/null +++ b/src/machines/prompt-contracts.js @@ -0,0 +1,274 @@ +/** + * Cross-prompt contract registry. + * + * Single source of truth for artifact section headings, producer/consumer + * file paths, and JSON payload field schemas used across develop and research + * machine prompts. + */ + +export const CONTRACTS = { + "PLANREVIEW.md": { + description: "Plan review critique with verdict", + producedBy: "src/machines/develop/plan-review.machine.js", + consumers: [ + { + file: "src/helpers.js", + usage: "Gemini plan-review prompt sections", + }, + { + file: "src/machines/develop/implementation.machine.js", + usage: "Step 1 critique sections", + }, + { + file: "src/machines/develop/quality-review.machine.js", + usage: "spec delta + review", + }, + ], + sections: [ + { + name: "Critical Issues (Must Fix)", + description: "Blocking problems that must be resolved", + }, + { + name: "Over-Engineering Concerns", + description: "Unnecessary complexity or abstraction", + }, + { + name: "Concerns (Should Address)", + description: "Non-blocking issues worth addressing", + }, + { + name: "Questions (Need Clarification)", + description: "Ambiguities requiring answers", + }, + { + name: "Verdict (REJECT | REVISE | PROCEED WITH CAUTION | APPROVED)", + description: "Final review verdict", + }, + ], + }, + + "PLAN.md": { + description: "Structured implementation plan", + producedBy: "src/machines/develop/planning.machine.js", + consumers: [ + { + file: "src/machines/develop/plan-review.machine.js", + usage: "review input", + }, + { + file: "src/machines/develop/implementation.machine.js", + usage: "implementation guide", + }, + { + file: "src/machines/develop/quality-review.machine.js", + usage: "spec delta + review", + }, + ], + sections: [ + { + name: "Summary", + description: "One paragraph describing what will change", + }, + { name: "Approach", description: "Which approach and why" }, + { + name: "Files to Modify", + description: "List each file with specific changes", + }, + { + name: "Files to Create", + description: "New files only if absolutely necessary", + }, + { + name: "Dependencies", + description: "New dependencies with version and justification", + }, + { + name: "Testing Strategy", + description: "Test cases, existing tests, and test commands", + }, + { + name: "Out of Scope", + description: "What this change does NOT include", + }, + ], + }, + + "ISSUE.md": { + description: "Structured issue specification", + producedBy: "src/machines/develop/issue-draft.machine.js", + consumers: [ + { + file: "src/machines/develop/planning.machine.js", + usage: "planning input", + }, + { + file: "src/machines/develop/implementation.machine.js", + usage: "implementation scope", + }, + { + file: "src/machines/develop/quality-review.machine.js", + usage: "review scope", + }, + ], + sections: [ + { + name: "Metadata", + description: "Source, Issue ID, Repo Root, Difficulty", + }, + { name: "Problem", description: "What is wrong or missing" }, + { + name: "Requirements", + description: "Behavioral requirements using EARS syntax", + }, + { name: "Changes", description: "Which files need to change and how" }, + { + name: "Testing Strategy", + description: "Existing tests, patterns, and new test cases", + }, + { + name: "Verification", + description: "Concrete command to prove the fix works", + }, + { name: "Out of Scope", description: "What this does NOT include" }, + ], + }, + + "REVIEW_FINDINGS.md": { + description: "Quality review findings with verdict", + producedBy: "src/machines/develop/quality-review.machine.js", + consumers: [ + { + file: "src/machines/develop/quality-review.machine.js", + usage: "iterative review loop", + }, + ], + sections: [ + { + name: "Finding N", + description: + "Individual finding with severity, file, lines, issue, suggestion", + }, + { name: "VERDICT", description: "APPROVED or REVISE" }, + ], + verdictPrefix: "## VERDICT: ", + verdictValues: ["APPROVED", "REVISE"], + }, + + SPEC_DELTA: { + description: "Spec delta summary comparing issue vs plan", + producedBy: "src/machines/develop/quality-review.machine.js", + consumers: [ + { + file: "src/machines/develop/quality-review.machine.js", + usage: "plan adherence review", + }, + ], + sections: [ + { + name: "Additions", + description: + "New technical constraints or approaches introduced in the plan", + }, + { + name: "Refinements", + description: "Changes in approach from the original issue", + }, + { + name: "Omissions", + description: "Requirements deferred or removed in the plan", + }, + ], + }, + + "synthesis-draft": { + description: "Research issue backlog draft payload", + producedBy: "src/machines/research/issue-synthesis.machine.js", + consumers: [ + { + file: "src/machines/research/issue-publish.machine.js", + usage: "issue publishing", + }, + ], + fields: { issues: "array" }, + }, + + "synthesis-review": { + description: "Research issue backlog critique payload", + producedBy: "src/machines/research/issue-synthesis.machine.js", + consumers: [ + { + file: "src/machines/research/issue-synthesis.machine.js", + usage: "next iteration feedback", + }, + ], + fields: { must_fix: "array", should_fix: "array" }, + }, + + "spec-architect-build": { + description: "Spec architect build-mode payload", + producedBy: "src/machines/research/spec-architect.machine.js", + consumers: [ + { + file: "src/machines/research/spec-architect.machine.js", + usage: "build result", + }, + ], + fields: { + domains: "array", + decisions: "array", + phases: "array", + issueSpecs: "array", + }, + }, + + "spec-architect-ingest": { + description: "Spec architect ingest-mode payload", + producedBy: "src/machines/research/spec-architect.machine.js", + consumers: [ + { + file: "src/machines/research/spec-architect.machine.js", + usage: "ingest result", + }, + ], + fields: { phases: "array", issueSpecs: "array" }, + }, +}; + +/** + * Render a numbered list of section headings for the given contract key. + * @param {string} contractKey + * @returns {string} + */ +export function renderRequiredSections(contractKey) { + const entry = CONTRACTS[contractKey]; + if (!entry?.sections) { + throw new Error(`renderRequiredSections: no sections for "${contractKey}"`); + } + return entry.sections.map((s, i) => `${i + 1}. ${s.name}`).join("\n"); +} + +/** + * Return a comma-joined list of section names for embedding in prose. + * Throws if any requested name is not in the contract. + * @param {string} contractKey + * @param {string[]} sectionNames + * @returns {string} + */ +export function renderCritiqueSectionList(contractKey, sectionNames) { + const entry = CONTRACTS[contractKey]; + if (!entry?.sections) { + throw new Error( + `renderCritiqueSectionList: no sections for "${contractKey}"`, + ); + } + const known = new Set(entry.sections.map((s) => s.name)); + for (const name of sectionNames) { + if (!known.has(name)) { + throw new Error( + `renderCritiqueSectionList: section "${name}" not found in "${contractKey}"`, + ); + } + } + return sectionNames.join(", "); +} diff --git a/src/machines/research/issue-synthesis.machine.js b/src/machines/research/issue-synthesis.machine.js index b5bdd05..9ec680a 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 { CONTRACTS } from "../prompt-contracts.js"; import { appendScratchpad, ensureArtifactOnDisk, @@ -201,7 +202,7 @@ Return ONLY valid JSON in this schema: saveSessionState(runDir, sessionState); const draftPayload = requirePayloadFields( draftRes.payload, - { issues: "array" }, + CONTRACTS["synthesis-draft"].fields, `draft_issue_backlog_${i}`, ); if (draftPayload.issues.length === 0) { @@ -266,7 +267,7 @@ Return ONLY valid JSON in this schema: saveSessionState(runDir, sessionState); finalReview = requirePayloadFields( reviewRes.payload, - { must_fix: "array", should_fix: "array" }, + CONTRACTS["synthesis-review"].fields, `review_issue_backlog_${i}`, ); diff --git a/src/machines/research/spec-architect.machine.js b/src/machines/research/spec-architect.machine.js index b5e3171..6a15897 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 { CONTRACTS } from "../prompt-contracts.js"; import { appendScratchpad, loadPipeline, @@ -87,12 +88,7 @@ Return ONLY valid JSON: requirePayloadFields( res.payload, - { - domains: "array", - decisions: "array", - phases: "array", - issueSpecs: "array", - }, + CONTRACTS["spec-architect-build"].fields, "spec_architect (build)", ); const { domains, decisions, phases, issueSpecs } = res.payload; @@ -173,7 +169,7 @@ Return ONLY valid JSON: requirePayloadFields( res.payload, - { phases: "array", issueSpecs: "array" }, + CONTRACTS["spec-architect-ingest"].fields, "spec_architect (ingest)", ); const { phases, issueSpecs } = res.payload; 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/src/test-runner.js b/src/test-runner.js index 767a475..49f7091 100644 --- a/src/test-runner.js +++ b/src/test-runner.js @@ -34,11 +34,12 @@ export function detectTestCommand(repoDir) { return null; } -export function runTestCommand(repoDir, argv) { - const res = spawnSync(argv[0], argv.slice(1), { +export function runTestCommand(repoDir, argv, spawnFn = spawnSync) { + const res = spawnFn(argv[0], argv.slice(1), { cwd: repoDir, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"], + env: process.env, }); const stderr = res.stderr || ""; return { diff --git a/test/prompt-contracts.test.js b/test/prompt-contracts.test.js new file mode 100644 index 0000000..c98d6e6 --- /dev/null +++ b/test/prompt-contracts.test.js @@ -0,0 +1,200 @@ +import assert from "node:assert/strict"; +import { existsSync, mkdtempSync, readFileSync, 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 { buildGeminiReviewPrompt } from "../src/helpers.js"; +import { + buildCritiqueRetryPrompt, + parsePlanVerdict, +} from "../src/machines/develop/plan-review.machine.js"; +import { + buildSpecDeltaPrompt, + parseReviewVerdict, +} from "../src/machines/develop/quality-review.machine.js"; +import { + CONTRACTS, + renderCritiqueSectionList, + renderRequiredSections, +} from "../src/machines/prompt-contracts.js"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(__dirname, ".."); + +test("CONTRACTS object has expected artifact keys", () => { + const expectedKeys = [ + "PLANREVIEW.md", + "PLAN.md", + "ISSUE.md", + "REVIEW_FINDINGS.md", + "SPEC_DELTA", + "synthesis-draft", + "synthesis-review", + "spec-architect-build", + "spec-architect-ingest", + ]; + for (const key of expectedKeys) { + assert.ok(CONTRACTS[key], `missing CONTRACTS entry: ${key}`); + } +}); + +test("registry producer/consumer paths point to existing files", () => { + for (const [key, entry] of Object.entries(CONTRACTS)) { + assert.ok( + existsSync(path.resolve(repoRoot, entry.producedBy)), + `producer file missing for ${key}: ${entry.producedBy}`, + ); + for (const consumer of entry.consumers) { + assert.ok( + existsSync(path.resolve(repoRoot, consumer.file)), + `consumer file missing for ${key}: ${consumer.file}`, + ); + } + } +}); + +test("renderRequiredSections outputs all section headings for PLANREVIEW.md", () => { + const rendered = renderRequiredSections("PLANREVIEW.md"); + assert.match(rendered, /Critical Issues/); + assert.match(rendered, /Over-Engineering Concerns/); + assert.match(rendered, /Concerns/); + assert.match(rendered, /Questions/); + assert.match(rendered, /Verdict/); +}); + +test("renderRequiredSections outputs numbered list for PLAN.md", () => { + const rendered = renderRequiredSections("PLAN.md"); + assert.match(rendered, /1\..*Summary/); + assert.match(rendered, /7\..*Out of Scope/); + // Verify all 7 sections present + const lines = rendered.trim().split("\n").filter(Boolean); + assert.equal(lines.length, 7); +}); + +test("renderCritiqueSectionList throws on unknown section name", () => { + assert.throws( + () => renderCritiqueSectionList("PLANREVIEW.md", ["Nonexistent Section"]), + /not found/, + ); +}); + +test("renderCritiqueSectionList returns joined names for valid sections", () => { + const result = renderCritiqueSectionList("PLANREVIEW.md", [ + "Critical Issues (Must Fix)", + "Over-Engineering Concerns", + ]); + assert.ok(result.includes("Critical Issues (Must Fix)")); + assert.ok(result.includes("Over-Engineering Concerns")); +}); + +test("parsePlanVerdict roundtrips on synthetic PLANREVIEW.md", () => { + const sections = CONTRACTS["PLANREVIEW.md"].sections; + const parts = []; + for (const s of sections) { + if (s.name.startsWith("Verdict")) { + parts.push(`## ${s.name}\nAPPROVED`); + } else { + parts.push(`## ${s.name}\n- No issues found.`); + } + } + const syntheticMd = parts.join("\n\n"); + assert.strictEqual(parsePlanVerdict(syntheticMd), "APPROVED"); +}); + +test("parseReviewVerdict roundtrips on synthetic REVIEW_FINDINGS.md using verdictPrefix/verdictValues", () => { + const entry = CONTRACTS["REVIEW_FINDINGS.md"]; + const syntheticContent = + "# Review Findings — Round 1\n\n" + + "## Finding 1\n- **Severity**: minor\n- **File**: foo.js\n- **Issue**: Nit\n- **Suggestion**: Fix it\n\n" + + entry.verdictPrefix + + entry.verdictValues[0]; + + const tmp = mkdtempSync(path.join(os.tmpdir(), "review-verdict-")); + const filePath = path.join(tmp, "REVIEW_FINDINGS.md"); + writeFileSync(filePath, syntheticContent); + + const result = parseReviewVerdict(filePath); + assert.strictEqual(result.verdict, "APPROVED"); +}); + +test("markdown entries have sections array, json entries have fields object", () => { + for (const [key, entry] of Object.entries(CONTRACTS)) { + if (entry.sections) { + assert.ok(Array.isArray(entry.sections), `${key}.sections is not array`); + for (const s of entry.sections) { + assert.ok(s.name, `${key} section missing name`); + assert.ok(s.description, `${key} section missing description`); + } + } + if (entry.fields) { + assert.equal(typeof entry.fields, "object", `${key}.fields not object`); + } + } +}); + +// --------------------------------------------------------------------------- +// Call-site integration tests: verify real prompt builders include all +// CONTRACTS section headings so registry changes flow end-to-end. +// --------------------------------------------------------------------------- + +test("buildGeminiReviewPrompt includes all PLANREVIEW.md section headings from CONTRACTS", () => { + const prompt = buildGeminiReviewPrompt("# Fake Plan\nSome content."); + for (const s of CONTRACTS["PLANREVIEW.md"].sections) { + assert.ok( + prompt.includes(s.name), + `Gemini review prompt missing CONTRACTS section: "${s.name}"`, + ); + } +}); + +test("buildCritiqueRetryPrompt includes all PLANREVIEW.md section headings from CONTRACTS", () => { + const prompt = buildCritiqueRetryPrompt( + "/tmp/PLAN.md", + "/tmp/PLANREVIEW.md", + 0, + ); + for (const s of CONTRACTS["PLANREVIEW.md"].sections) { + assert.ok( + prompt.includes(s.name), + `Critique retry prompt missing CONTRACTS section: "${s.name}"`, + ); + } +}); + +test("buildSpecDeltaPrompt includes all SPEC_DELTA section headings from CONTRACTS", () => { + const prompt = buildSpecDeltaPrompt("/tmp/ISSUE.md", "/tmp/PLAN.md"); + for (const s of CONTRACTS.SPEC_DELTA.sections) { + assert.ok( + prompt.includes(s.name), + `Spec delta prompt missing CONTRACTS section: "${s.name}"`, + ); + } +}); + +test("planning.machine.js and issue-draft.machine.js import and use CONTRACTS", () => { + // Structural check: these machines build prompts inline in execute() using + // CONTRACTS directly. Verify the source imports CONTRACTS and references + // the expected contract key so hardcoded-section regressions are caught. + const checks = [ + { + file: "src/machines/develop/planning.machine.js", + key: 'CONTRACTS["PLAN.md"]', + }, + { + file: "src/machines/develop/issue-draft.machine.js", + key: 'CONTRACTS["ISSUE.md"]', + }, + ]; + for (const { file, key } of checks) { + const src = readFileSync(path.resolve(repoRoot, file), "utf8"); + assert.ok( + src.includes('from "../prompt-contracts.js"'), + `${file} does not import from prompt-contracts.js`, + ); + assert.ok( + src.includes(key), + `${file} does not reference ${key} — sections may be hardcoded`, + ); + } +}); 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", + ); +}); diff --git a/test/test-runner.test.js b/test/test-runner.test.js index 626520c..2f86a3c 100644 --- a/test/test-runner.test.js +++ b/test/test-runner.test.js @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; import os from "node:os"; import path from "node:path"; +import process from "node:process"; import test from "node:test"; import { loadTestConfig, @@ -37,6 +38,20 @@ test("runTestCommand preserves specific exit codes", () => { assert.equal(res.exitCode, 42); }); +test("runTestCommand explicitly passes process.env to spawn options", () => { + let capturedOptions; + const mockSpawn = (_cmd, _args, opts) => { + capturedOptions = opts; + return { status: 0, stdout: "", stderr: "" }; + }; + runTestCommand(os.tmpdir(), ["node", "--version"], mockSpawn); + assert.ok( + "env" in capturedOptions, + "env should be explicitly set in options", + ); + assert.strictEqual(capturedOptions.env, process.env); +}); + test("loadTestConfig: testConfigPath to unified coder.json accepts test.command object", () => { const dir = mkdtempSync(path.join(os.tmpdir(), "coder-loadtest-")); writeFileSync(