diff --git a/README.md b/README.md index 6d7d9b7..6518442 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,8 @@ Generate a Codex review bundle after executor completion. Checks HEAD stability agent-workflow review --run ``` +Once the run is at or past the guidance threshold — the executor cycle about to start is `>= 3` (i.e. reviewing cycle 2, so the next fix cycle would be the third round) — the bundle sets `detailedGuidanceRequested: true` and includes a `reviewerInstructions` string. This asks Codex, if it decides `fix`, to return a concrete step-by-step implementation plan so the executor can converge quickly. + ### `agent-workflow decide` Submit a Codex decision (fix, accept, or needs_human). A fix decision resumes the same executor session. @@ -105,6 +107,8 @@ Submit a Codex decision (fix, accept, or needs_human). A fix decision resumes th agent-workflow decide --run --input ``` +A `fix` decision may include an optional `implementationGuidance` string (up to 20000 characters). When present, it is injected verbatim into the executor's fix prompt under a `## Detailed Implementation Guidance` section, and the frozen plan's `planMarkdown` and acceptance criteria are re-sent alongside it so the executor has full context. This is the channel Codex uses to answer the bundle's `detailedGuidanceRequested` signal, and it flows through the normal fix, `retry-execute`, and `extend` paths. + ### `agent-workflow retry-review` Retry review after the executor has removed approved out-of-scope artifacts. diff --git a/src/workflow-config.test.ts b/src/workflow-config.test.ts new file mode 100644 index 0000000..72dd54d --- /dev/null +++ b/src/workflow-config.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect } from "vitest"; +import { validateHostDecision, WorkflowConfigError } from "./workflow-config.js"; +import type { HostDecisionV1 } from "./workflow-types.js"; + +function baseFixDecision(): HostDecisionV1 { + return { + version: "1", + outcome: "fix", + findingDecisions: [ + { findingId: "codex-1", disposition: "accept", summary: "fix the bug" }, + ], + decidedAt: new Date().toISOString(), + }; +} + +describe("validateHostDecision implementationGuidance", () => { + it("accepts a decision with a valid string implementationGuidance", () => { + const decision = { ...baseFixDecision(), implementationGuidance: "1. Edit index.ts\n2. Run tests" }; + expect(() => validateHostDecision(decision)).not.toThrow(); + }); + + it("accepts a decision that omits implementationGuidance (backward compatible)", () => { + expect(() => validateHostDecision(baseFixDecision())).not.toThrow(); + }); + + it("rejects a non-string implementationGuidance", () => { + const decision = { ...baseFixDecision(), implementationGuidance: 42 }; + expect(() => validateHostDecision(decision)).toThrow(WorkflowConfigError); + expect(() => validateHostDecision(decision)).toThrow(/implementationGuidance must be a string/); + }); + + it("rejects an over-length implementationGuidance", () => { + const decision = { ...baseFixDecision(), implementationGuidance: "x".repeat(20001) }; + expect(() => validateHostDecision(decision)).toThrow(/at most 20000 characters/); + }); + + it("accepts implementationGuidance exactly at the 20000-character bound", () => { + const decision = { ...baseFixDecision(), implementationGuidance: "x".repeat(20000) }; + expect(() => validateHostDecision(decision)).not.toThrow(); + }); +}); diff --git a/src/workflow-config.ts b/src/workflow-config.ts index 31509ad..7017fad 100644 --- a/src/workflow-config.ts +++ b/src/workflow-config.ts @@ -297,4 +297,12 @@ export function validateHostDecision(raw: unknown): asserts raw is import("./wor if (typeof obj.decidedAt !== "string") { throw new WorkflowConfigError("Decision decidedAt must be a string"); } + if (obj.implementationGuidance !== undefined) { + if (typeof obj.implementationGuidance !== "string") { + throw new WorkflowConfigError("Decision implementationGuidance must be a string"); + } + if (obj.implementationGuidance.length > 20000) { + throw new WorkflowConfigError("Decision implementationGuidance must be at most 20000 characters"); + } + } } diff --git a/src/workflow-engine.ts b/src/workflow-engine.ts index c959855..edb7670 100644 --- a/src/workflow-engine.ts +++ b/src/workflow-engine.ts @@ -87,6 +87,13 @@ import type { const activeAbortControllers = new Set(); +/** + * When the executor cycle that is *about to start* reaches this value, the host + * review bundle asks Codex for a more detailed implementation plan so the + * executor can converge faster. Compared against (cycleIndex + 1) in reviewWorkflow. + */ +export const DETAILED_GUIDANCE_CYCLE_THRESHOLD = 3; + process.on("SIGTERM", () => { for (const ac of activeAbortControllers) { ac.abort(); @@ -851,6 +858,8 @@ export async function reviewWorkflow(opts: ReviewWorkflowOpts): Promise { // 4. Persist all evidence needed by the Codex host. The CLI does not invoke // another reviewer: Codex owns review and final acceptance. const hostReviewFile = cycleHostReviewFile(dir, cycleIndex); + const nextExecutorCycle = cycleIndex + 1; + const detailedGuidanceRequested = nextExecutorCycle >= DETAILED_GUIDANCE_CYCLE_THRESHOLD; const bundle: HostReviewBundleV1 = { version: "1", runId: state.runId, @@ -867,6 +876,17 @@ export async function reviewWorkflow(opts: ReviewWorkflowOpts): Promise { : {}), ...(cycleRecord.executorReport ? { executorReport: cycleRecord.executorReport } : {}), verificationCommands: state.plan.verificationCommands, + ...(detailedGuidanceRequested + ? { + detailedGuidanceRequested: true, + reviewerInstructions: + `This run is at or past cycle ${nextExecutorCycle} of ${state.maxCycles}. ` + + 'If you return outcome "fix", populate the decision\'s implementationGuidance ' + + "field with a concrete, step-by-step plan (files to touch, functions/edits, and " + + "the order to apply them) so the executor can finish quickly. Keep it within the " + + "frozen scope.", + } + : {}), }; fs.writeFileSync(hostReviewFile, JSON.stringify(bundle, null, 2), "utf8"); cycleRecord.hostReviewFile = path.relative(dir, hostReviewFile); @@ -1333,6 +1353,16 @@ function buildAcceptedFindingsText( if (d.rationale) lines.push(`- Rationale: ${d.rationale}`); } + if (decision.implementationGuidance?.trim()) { + lines.push( + "", + "## Detailed Implementation Guidance", + "The reviewer provided a detailed plan. Follow it to complete the fixes quickly:", + "", + decision.implementationGuidance.trim(), + ); + } + return lines.join("\n"); } diff --git a/src/workflow-executor.test.ts b/src/workflow-executor.test.ts index ebbb895..770ceb7 100644 --- a/src/workflow-executor.test.ts +++ b/src/workflow-executor.test.ts @@ -789,6 +789,66 @@ describe("Safety constraints in executor prompts", () => { } }); + it("fix prompt injects detailed guidance and re-sends plan context when the marker is present", async () => { + const fakeBin = writeFakeCli(tmpDir, {}); + const sessionFile = path.join(tmpDir, "session.jsonl"); + fs.writeFileSync(sessionFile, "{}"); + + const adapter = createExecutorAdapter("reasonix"); + await adapter.resume({ + repoRoot: tmpDir, + plan: makeValidPlan(), + acceptedFindings: [ + "# Codex Review Findings", + "", + "### Finding codex-1", + "- Issue: index.ts is wrong", + "", + "## Detailed Implementation Guidance", + "The reviewer provided a detailed plan. Follow it to complete the fixes quickly:", + "", + "Edit index.ts line 1 to set x to 3.", + ].join("\n"), + handle: { kind: "reasonix", sessionFilePath: sessionFile }, + runDir: tmpDir, + cycleIndex: 3, + config: { binary: fakeBin }, + }); + + const content = fs.readFileSync(path.join(tmpDir, "cycle-3-prompt.txt"), "utf8"); + expect(content).toContain("# Fix Request: Test plan"); + expect(content).toContain("## Detailed Implementation Guidance"); + expect(content).toContain("Edit index.ts line 1 to set x to 3."); + // Plan context (planMarkdown + acceptance criteria) is re-sent alongside the guidance. + expect(content).toContain("## Plan"); + expect(content).toContain("Do something."); + expect(content).toContain("## Acceptance Criteria"); + expect(content).toContain("Tests pass"); + }); + + it("plain fix prompt (no guidance marker) stays lean and omits re-sent plan context", async () => { + const fakeBin = writeFakeCli(tmpDir, {}); + const sessionFile = path.join(tmpDir, "session.jsonl"); + fs.writeFileSync(sessionFile, "{}"); + + const adapter = createExecutorAdapter("reasonix"); + await adapter.resume({ + repoRoot: tmpDir, + plan: makeValidPlan(), + acceptedFindings: "# Codex Review Findings\n\n### Finding codex-1\n- Issue: plain finding", + handle: { kind: "reasonix", sessionFilePath: sessionFile }, + runDir: tmpDir, + cycleIndex: 2, + config: { binary: fakeBin }, + }); + + const content = fs.readFileSync(path.join(tmpDir, "cycle-2-prompt.txt"), "utf8"); + expect(content).toContain("# Fix Request: Test plan"); + expect(content).not.toContain("## Detailed Implementation Guidance"); + expect(content).not.toContain("## Plan"); + expect(content).not.toContain("## Acceptance Criteria"); + }); + it("scope remediation prompt authorizes only listed cleanup paths", async () => { const fakeBin = writeFakeCli(tmpDir, {}); const sessionFile = path.join(tmpDir, "session.jsonl"); diff --git a/src/workflow-executor.ts b/src/workflow-executor.ts index 11e34b5..b927a67 100644 --- a/src/workflow-executor.ts +++ b/src/workflow-executor.ts @@ -314,8 +314,15 @@ ${plan.verificationCommands.map((cmd) => ` ${cmd.join(" ")}`).join("\n")} ${getSafetyConstraints(opts.vcsKind)}`; } - return `# Fix Request: ${plan.title} + const hasDetailedGuidance = acceptedFindings.includes("## Detailed Implementation Guidance"); + const planContext = hasDetailedGuidance + ? `\n## Plan\n\n${plan.planMarkdown}\n\n## Acceptance Criteria\n\n${plan.acceptanceCriteria + .map((c, i) => ` ${i + 1}. ${c}`) + .join("\n")}\n` + : ""; + return `# Fix Request: ${plan.title} +${planContext} The code review found the following issues that must be fixed: ${acceptedFindings} diff --git a/src/workflow-integration.test.ts b/src/workflow-integration.test.ts index 28e1649..c075c81 100644 --- a/src/workflow-integration.test.ts +++ b/src/workflow-integration.test.ts @@ -3,7 +3,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { execFileSync } from "node:child_process"; -import { startWorkflow, reviewWorkflow, decideWorkflow, retryExecuteWorkflow } from "./workflow-engine.js"; +import { startWorkflow, reviewWorkflow, decideWorkflow, retryExecuteWorkflow, DETAILED_GUIDANCE_CYCLE_THRESHOLD } from "./workflow-engine.js"; import { readState, runDir } from "./workflow-state.js"; import type { HostDecisionV1 } from "./workflow-types.js"; @@ -208,6 +208,13 @@ fi expect(reviewBundle.repoRoot).toBe(repoRoot); expect(reviewBundle.diffFile).toBe(path.join(runDir(repoRoot, runId), "cycle-2.diff")); expect(reviewBundle.executorLogFile).toBe(path.join(runDir(repoRoot, runId), "cycle-2-exec-retry-1.log")); + // Reviewing cycle 2 means the executor cycle about to start is 3, which hits + // DETAILED_GUIDANCE_CYCLE_THRESHOLD: the bundle must ask Codex for a detailed plan. + expect(reviewBundle.cycleIndex + 1).toBeGreaterThanOrEqual(DETAILED_GUIDANCE_CYCLE_THRESHOLD); + expect(reviewBundle.detailedGuidanceRequested).toBe(true); + expect(typeof reviewBundle.reviewerInstructions).toBe("string"); + expect(reviewBundle.reviewerInstructions.length).toBeGreaterThan(0); + expect(reviewBundle.reviewerInstructions).toContain("implementationGuidance"); expect(state4.cycles[1].executorAttemptLogs).toEqual([ "cycle-2-exec.log", "cycle-2-exec-retry-1.log", @@ -226,6 +233,7 @@ fi .rejects.toThrow(/require a non-empty summary/); expect(readState(runDir(repoRoot, runId))?.status).toBe("awaiting_host"); + const guidanceMarkerText = "Replace `const x = 2;` in index.ts with `const x = 3;`"; const fixDecision: HostDecisionV1 = { version: "1", outcome: "fix", @@ -235,6 +243,7 @@ fi summary: "index.ts sets x to 2 instead of the required value 3", path: "index.ts", }], + implementationGuidance: guidanceMarkerText, decidedAt: new Date().toISOString() }; const fixDecisionPath = path.join(process.env.HOME!, "decision-fix.json"); @@ -245,6 +254,17 @@ fi expect(state5.status).toBe("awaiting_review"); expect(state5.currentCycle).toBe(3); + // The detailed implementation guidance must reach the executor's cycle-3 fix prompt, + // along with the re-sent Plan and Acceptance Criteria for full context. + const cycle3Prompt = fs.readFileSync( + path.join(runDir(repoRoot, runId), "cycle-3-prompt.txt"), + "utf8", + ); + expect(cycle3Prompt).toContain("## Detailed Implementation Guidance"); + expect(cycle3Prompt).toContain(guidanceMarkerText); + expect(cycle3Prompt).toContain("## Plan"); + expect(cycle3Prompt).toContain("Add an index.ts file."); + // --- STEP 6: PREPARE CODEX RE-REVIEW (Cycle 3) --- await reviewWorkflow({ runId, cwd: tmpDir }); let state6 = readState(runDir(repoRoot, runId))!; diff --git a/src/workflow-types.ts b/src/workflow-types.ts index 8fd4e9b..c59dc8c 100644 --- a/src/workflow-types.ts +++ b/src/workflow-types.ts @@ -274,6 +274,13 @@ export interface HostDecisionV1 { followUps?: string[]; /** Codex-supplied verification evidence (command outputs, test results). */ verificationEvidence?: string[]; + /** + * Optional detailed, step-by-step implementation plan the host provides so the + * executor can finish quickly. When present, it is injected verbatim into the + * executor fix prompt. Typically supplied when the bundle set + * detailedGuidanceRequested (the run is approaching/at the guidance threshold). + */ + implementationGuidance?: string; /** ISO timestamp when Codex submitted this decision. */ decidedAt: string; } @@ -293,6 +300,17 @@ export interface HostReviewBundleV1 { executorLogFile?: string; executorReport?: string; verificationCommands: string[][]; + /** + * True when the run is at or past the guidance threshold (the executor cycle + * about to start >= DETAILED_GUIDANCE_CYCLE_THRESHOLD). Signals Codex to return + * a more detailed implementation plan if it decides "fix". + */ + detailedGuidanceRequested?: boolean; + /** + * Human-meaningful instructions telling Codex what extra detail to supply. + * Only set when detailedGuidanceRequested is true. + */ + reviewerInstructions?: string; } // ---------------------------------------------------------------------------