feat: request detailed reviewer guidance from cycle 3
When a run is about to start its 3rd cycle, the host review bundle asks Codex to supply a step-by-step implementation plan via the new decision field implementationGuidance, which is injected into the executor fix prompt (with re-sent plan context) so the executor can converge faster. - Add DETAILED_GUIDANCE_CYCLE_THRESHOLD and bundle signal in reviewWorkflow - Add optional detailedGuidanceRequested/reviewerInstructions to the bundle - Add optional implementationGuidance to HostDecisionV1 (validated, <=20000 chars) - Inject guidance section and re-send plan/criteria on marked fix cycles - Cover with integration, executor, and config validation tests
This commit is contained in:
@@ -97,6 +97,8 @@ Generate a Codex review bundle after executor completion. Checks HEAD stability
|
||||
agent-workflow review --run <run-id>
|
||||
```
|
||||
|
||||
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 <run-id> --input <decision.json|->
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -87,6 +87,13 @@ import type {
|
||||
|
||||
const activeAbortControllers = new Set<AbortController>();
|
||||
|
||||
/**
|
||||
* 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<void> {
|
||||
// 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<void> {
|
||||
: {}),
|
||||
...(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");
|
||||
}
|
||||
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -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))!;
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user