feat(KB-124): enforce code review REVISE verdict before step advancement

- Track per-step code review verdicts in executor via codeReviewVerdicts map - Block task_update(status="done") when last code review returned REVISE - Update system prompt with enforcement language distinguishing code vs plan reviews - Add comprehensive tests for verdict tracking, blocking, clearing, and independence across steps
2026-03-27 01:25:06 -04:00
parent 948928d258
commit 488b9260d5
2 changed files with 330 additions and 6 deletions
--- a/packages/engine/src/executor.test.ts
+++ b/packages/engine/src/executor.test.ts
@@ -29,6 +29,7 @@ vi.mock("node:fs", () => ({

 import { TaskExecutor, buildExecutionPrompt } from "./executor.js";
 import { createKbAgent } from "./pi.js";
+import { reviewStep as mockedReviewStepFn } from "./reviewer.js";
 import { execSync } from "node:child_process";
 import { findWorktreeUser, aiMergeTask } from "./merger.js";
 import { WorktreePool } from "./worktree-pool.js";
@@ -1012,3 +1013,279 @@ describe("TaskExecutor pause behavior", () => {
    expect(store.logEntry).not.toHaveBeenCalledWith("KB-001", expect.anything());
  });
 });
+
+// ── Code review verdict enforcement tests ────────────────────────────
+
+const mockedReviewStep = vi.mocked(mockedReviewStepFn);
+
+/**
+ * Helper: executes a task and captures the custom tools passed to createKbAgent.
+ * Returns a map of tool name → tool execute function for direct testing.
+ */
+async function captureTools(): Promise<Record<string, (id: string, params: any) => Promise<any>>> {
+  const store = createMockStore();
+  store.updateStep.mockResolvedValue({
+    steps: [
+      { name: "Preflight", status: "done" },
+      { name: "Implement", status: "in-progress" },
+      { name: "Testing", status: "pending" },
+    ],
+  });
+  mockedExistsSync.mockReturnValue(true);
+
+  let capturedTools: any[] = [];
+  mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
+    capturedTools = opts.customTools || [];
+    return {
+      session: {
+        prompt: vi.fn().mockResolvedValue(undefined),
+        dispose: vi.fn(),
+      },
+    } as any;
+  });
+
+  const executor = new TaskExecutor(store, "/tmp/test");
+  await executor.execute({
+    id: "KB-TEST",
+    title: "Test",
+    description: "Test",
+    column: "in-progress",
+    dependencies: [],
+    steps: [],
+    currentStep: 0,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+  });
+
+  const tools: Record<string, any> = {};
+  for (const t of capturedTools) {
+    tools[t.name] = t.execute;
+  }
+  return tools;
+}
+
+describe("Code review verdict tracking", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockedExistsSync.mockReturnValue(true);
+  });
+
+  it("code review REVISE sets tracking state", async () => {
+    mockedReviewStep.mockResolvedValue({
+      verdict: "REVISE",
+      review: "Fix the bug",
+      summary: "Needs fixes",
+    });
+
+    const tools = await captureTools();
+    const result = await tools.review_step("call1", {
+      step: 1,
+      type: "code",
+      step_name: "Implement",
+      baseline: "abc123",
+    });
+
+    expect(result.content[0].text).toContain("REVISE");
+    expect(result.content[0].text).toContain("cannot be marked done");
+
+    // Now task_update(step=1, status="done") should be blocked
+    const updateResult = await tools.task_update("call2", { step: 1, status: "done" });
+    expect(updateResult.content[0].text).toContain("Cannot mark Step 1 as done");
+    expect(updateResult.content[0].text).toContain("REVISE");
+  });
+
+  it("code review APPROVE clears tracking state", async () => {
+    // First: REVISE
+    mockedReviewStep.mockResolvedValue({
+      verdict: "REVISE",
+      review: "Fix the bug",
+      summary: "Needs fixes",
+    });
+
+    const tools = await captureTools();
+    await tools.review_step("call1", {
+      step: 1,
+      type: "code",
+      step_name: "Implement",
+      baseline: "abc123",
+    });
+
+    // Verify it's blocked
+    const blocked = await tools.task_update("call2", { step: 1, status: "done" });
+    expect(blocked.content[0].text).toContain("Cannot mark Step 1 as done");
+
+    // Now: APPROVE
+    mockedReviewStep.mockResolvedValue({
+      verdict: "APPROVE",
+      review: "Looks good",
+      summary: "All good",
+    });
+
+    await tools.review_step("call3", {
+      step: 1,
+      type: "code",
+      step_name: "Implement",
+      baseline: "def456",
+    });
+
+    // Now task_update should succeed
+    const updateResult = await tools.task_update("call4", { step: 1, status: "done" });
+    expect(updateResult.content[0].text).toContain("→ done");
+  });
+
+  it("plan review REVISE does NOT set tracking state", async () => {
+    mockedReviewStep.mockResolvedValue({
+      verdict: "REVISE",
+      review: "Reconsider approach",
+      summary: "Plan issues",
+    });
+
+    const tools = await captureTools();
+    const result = await tools.review_step("call1", {
+      step: 1,
+      type: "plan",
+      step_name: "Implement",
+    });
+
+    // Plan REVISE should use the non-enforced text format
+    expect(result.content[0].text).toContain("REVISE");
+    expect(result.content[0].text).not.toContain("cannot be marked done");
+
+    // task_update should still work (plan reviews are advisory)
+    const updateResult = await tools.task_update("call2", { step: 1, status: "done" });
+    expect(updateResult.content[0].text).toContain("→ done");
+  });
+});
+
+describe("Code review verdict enforcement - task_update blocking", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockedExistsSync.mockReturnValue(true);
+  });
+
+  it("task_update(status='done') is rejected when last code review was REVISE", async () => {
+    mockedReviewStep.mockResolvedValue({
+      verdict: "REVISE",
+      review: "Fix issues",
+      summary: "Needs work",
+    });
+
+    const tools = await captureTools();
+    await tools.review_step("call1", {
+      step: 1,
+      type: "code",
+      step_name: "Implement",
+      baseline: "abc",
+    });
+
+    const result = await tools.task_update("call2", { step: 1, status: "done" });
+    expect(result.content[0].text).toContain("Cannot mark Step 1 as done");
+    expect(result.content[0].text).toContain("review_step");
+  });
+
+  it("task_update succeeds after a subsequent APPROVE", async () => {
+    const tools = await captureTools();
+
+    // REVISE first
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
+    await tools.review_step("c1", { step: 1, type: "code", step_name: "Impl", baseline: "a" });
+
+    // Then APPROVE
+    mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "OK", summary: "Good" });
+    await tools.review_step("c2", { step: 1, type: "code", step_name: "Impl", baseline: "b" });
+
+    const result = await tools.task_update("c3", { step: 1, status: "done" });
+    expect(result.content[0].text).toContain("→ done");
+  });
+
+  it("task_update succeeds when no code review was requested (review level 0)", async () => {
+    const tools = await captureTools();
+
+    // No review_step calls at all
+    const result = await tools.task_update("c1", { step: 1, status: "done" });
+    expect(result.content[0].text).toContain("→ done");
+  });
+
+  it("plan-only REVISE does NOT block advancement", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Rethink", summary: "Plan issue" });
+
+    const tools = await captureTools();
+    await tools.review_step("c1", { step: 1, type: "plan", step_name: "Impl" });
+
+    const result = await tools.task_update("c2", { step: 1, status: "done" });
+    expect(result.content[0].text).toContain("→ done");
+  });
+
+  it("multiple steps tracked independently (REVISE on step 1 doesn't block step 2)", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
+
+    const tools = await captureTools();
+    await tools.review_step("c1", { step: 1, type: "code", step_name: "Step1", baseline: "a" });
+
+    // Step 1 is blocked
+    const blocked = await tools.task_update("c2", { step: 1, status: "done" });
+    expect(blocked.content[0].text).toContain("Cannot mark Step 1 as done");
+
+    // Step 2 is NOT blocked (no review for step 2)
+    const allowed = await tools.task_update("c3", { step: 2, status: "done" });
+    expect(allowed.content[0].text).toContain("→ done");
+  });
+
+  it("REVISE tool response text includes re-review instructions", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Bug found", summary: "Issues" });
+
+    const tools = await captureTools();
+    const result = await tools.review_step("c1", { step: 1, type: "code", step_name: "Implement", baseline: "abc" });
+
+    expect(result.content[0].text).toContain("cannot be marked done");
+    expect(result.content[0].text).toContain("review_step");
+    expect(result.content[0].text).toContain('type="code"');
+  });
+
+  it("EXECUTOR_SYSTEM_PROMPT contains code review enforcement language", async () => {
+    // Capture the system prompt passed to createKbAgent
+    let capturedSystemPrompt = "";
+    mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
+      capturedSystemPrompt = opts.systemPrompt || "";
+      return {
+        session: {
+          prompt: vi.fn().mockResolvedValue(undefined),
+          dispose: vi.fn(),
+        },
+      } as any;
+    });
+
+    const store = createMockStore();
+    const executor = new TaskExecutor(store, "/tmp/test");
+    await executor.execute({
+      id: "KB-SYS",
+      title: "Test",
+      description: "Test",
+      column: "in-progress",
+      dependencies: [],
+      steps: [],
+      currentStep: 0,
+      log: [],
+      createdAt: new Date().toISOString(),
+      updatedAt: new Date().toISOString(),
+    });
+
+    // Verify enforcement language is present in system prompt
+    expect(capturedSystemPrompt).toContain("enforced");
+    expect(capturedSystemPrompt).toContain("will be rejected until the code review passes");
+    expect(capturedSystemPrompt).toContain("REVISE (plan review)");
+    expect(capturedSystemPrompt).toContain("advisory");
+  });
+
+  it("task_update with non-done status is not blocked by REVISE", async () => {
+    mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
+
+    const tools = await captureTools();
+    await tools.review_step("c1", { step: 1, type: "code", step_name: "Step1", baseline: "a" });
+
+    // "in-progress" should still work even with REVISE
+    const result = await tools.task_update("c2", { step: 1, status: "in-progress" });
+    expect(result.content[0].text).toContain("→ in-progress");
+  });
+});
--- a/packages/engine/src/executor.ts
+++ b/packages/engine/src/executor.ts
@@ -6,7 +6,7 @@ import { findWorktreeUser } from "./merger.js";
 import { generateWorktreeName } from "./worktree-names.js";
 import { Type, type Static } from "@mariozechner/pi-ai";
 import { createKbAgent } from "./pi.js";
-import { reviewStep } from "./reviewer.js";
+import { reviewStep, type ReviewVerdict } from "./reviewer.js";
 import type { ToolDefinition } from "@mariozechner/pi-coding-agent";
 import type { AgentSemaphore } from "./concurrency.js";
 import type { WorktreePool } from "./worktree-pool.js";
@@ -104,7 +104,11 @@ model, read-only access) to independently assess your work.

 **Handling verdicts:**
 - **APPROVE** → proceed to next step
- **REVISE** → read the feedback, fix the issues, commit again, then proceed
+- **REVISE (code review)** → **enforced**. You MUST fix the issues, commit again,
+  and re-run \`review_step(type="code")\` before the step can be marked done.
+  \`task_update(status="done")\` will be rejected until the code review passes.
+- **REVISE (plan review)** → advisory. Incorporate the feedback at your discretion
+  and proceed with implementation. No re-review is required.
 - **RETHINK** → reconsider your approach, adjust plan, then implement

 ## Git discipline
@@ -326,13 +330,17 @@ export class TaskExecutor {
      }

      // Build custom tools for the worker
+      // Track the last code review verdict per step so we can enforce REVISE
+      // (block task_update status="done" until the agent re-reviews and gets APPROVE).
+      const codeReviewVerdicts = new Map<number, ReviewVerdict>();
+
      let taskDone = false;
      const customTools = [
-        this.createTaskUpdateTool(task.id),
+        this.createTaskUpdateTool(task.id, codeReviewVerdicts),
        this.createTaskLogTool(task.id),
        this.createTaskCreateTool(),
        this.createTaskDoneTool(task.id, () => { taskDone = true; }),
-        this.createReviewStepTool(task.id, worktreePath, detail.prompt),
+        this.createReviewStepTool(task.id, worktreePath, detail.prompt, codeReviewVerdicts),
      ];

      const agentLogger = new AgentLogger({
@@ -413,7 +421,10 @@ export class TaskExecutor {

  // ── Custom tools for the worker agent ──────────────────────────────

-  private createTaskUpdateTool(taskId: string): ToolDefinition {
+  private createTaskUpdateTool(
+    taskId: string,
+    codeReviewVerdicts: Map<number, ReviewVerdict>,
+  ): ToolDefinition {
    const store = this.store;
    return {
      name: "task_update",
@@ -425,6 +436,23 @@ export class TaskExecutor {
      parameters: taskUpdateParams,
      execute: async (_id: string, params: Static<typeof taskUpdateParams>) => {
        const { step, status } = params;
+
+        // Enforce code review REVISE: block advancing to "done" when the last
+        // code review for this step returned REVISE. The agent must fix the
+        // issues and call review_step(type="code") again before proceeding.
+        if (status === "done" && codeReviewVerdicts.get(step) === "REVISE") {
+          return {
+            content: [{
+              type: "text" as const,
+              text: `Cannot mark Step ${step} as done — the last code review returned REVISE. ` +
+                `Fix the issues from the code review, commit your changes, and call ` +
+                `review_step(step=${step}, type="code") again. The step can only advance ` +
+                `after the code review passes.`,
+            }],
+            details: {},
+          };
+        }
+
        const task = await store.updateStep(taskId, step, status as StepStatus);
        const stepInfo = task.steps[step];
        const progress = task.steps.filter((s) => s.status === "done").length;
@@ -518,6 +546,7 @@ export class TaskExecutor {
    taskId: string,
    worktreePath: string,
    promptContent: string,
+    codeReviewVerdicts: Map<number, ReviewVerdict>,
  ): ToolDefinition {
    const store = this.store;
    const options = this.options;
@@ -559,10 +588,28 @@ export class TaskExecutor {
          );
          reviewerLog.log(`${taskId}: Step ${step} ${reviewType} → ${result.verdict}`);

+          // Track code review verdicts for enforcement. Plan reviews remain
+          // advisory — only code reviews write to the verdict map.
+          if (reviewType === "code") {
+            if (result.verdict === "REVISE") {
+              codeReviewVerdicts.set(step, "REVISE");
+            } else if (result.verdict === "APPROVE") {
+              codeReviewVerdicts.delete(step);
+            }
+          }
+
          let text: string;
          switch (result.verdict) {
            case "APPROVE": text = "APPROVE"; break;
-            case "REVISE": text = `REVISE\n\n${result.review}`; break;
+            case "REVISE":
+              if (reviewType === "code") {
+                text = `REVISE — this step cannot be marked done until the code review passes.\n\n` +
+                  `Fix the issues below, commit your changes, and call review_step(step=${step}, ` +
+                  `type="code", step_name="${step_name}", baseline="<new SHA>") again.\n\n${result.review}`;
+              } else {
+                text = `REVISE\n\n${result.review}`;
+              }
+              break;
            case "RETHINK": text = `RETHINK\n\n${result.review}`; break;
            default: text = "UNAVAILABLE — reviewer did not produce a usable verdict.";
          }