feat(KB-126): add RETHINK rewind for plan and code reviews with REVISE enforcement

- Differentiate RETHINK handling between plan reviews (session rewind only) and code reviews (git reset + session rewind) - Update system prompt with detailed REVISE/RETHINK verdict descriptions for plan vs code review types - Enforce code review REVISE verdict by blocking task_update(done) until re-review passes - Capture pre-step session checkpoints in task_update for conversation rewind on RETHINK - Add comprehensive tests for plan RETHINK, code RETHINK, and REVISE enforcement flows
2026-03-27 01:58:46 -04:00
parent c909271a62
commit 039671b5ad
2 changed files with 597 additions and 28 deletions
--- a/packages/engine/src/executor.test.ts
+++ b/packages/engine/src/executor.test.ts
@@ -29,7 +29,7 @@ vi.mock("node:fs", () => ({

 import { TaskExecutor, buildExecutionPrompt } from "./executor.js";
 import { createKbAgent } from "./pi.js";
-import { reviewStep } from "./reviewer.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";
@@ -1014,7 +1014,291 @@ describe("TaskExecutor pause behavior", () => {
  });
 });

-const mockedReviewStep = vi.mocked(reviewStep);
+
+// ── 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(),
+        sessionManager: {
+          getLeafId: vi.fn().mockReturnValue("leaf-id"),
+          branchWithSummary: vi.fn(),
+        },
+        navigateTree: vi.fn().mockResolvedValue({ cancelled: false }),
+      },
+    } 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(),
+          sessionManager: { getLeafId: vi.fn(), branchWithSummary: vi.fn() },
+          navigateTree: 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");
+  });
+});
+
+// ── RETHINK verdict handling tests ───────────────────────────────────

 describe("RETHINK verdict handling", () => {
  const makeTask = (id = "KB-040") => ({
@@ -1043,7 +1327,7 @@ describe("RETHINK verdict handling", () => {
   * Helper: run executor and capture custom tools from createKbAgent mock.
   * Returns the tools map keyed by tool name.
   */
-  async function captureTools(store: any, options?: any) {
+  async function captureRethinkTools(store: any, options?: any) {
    let capturedTools: any[] = [];
    const mockSessionManager = {
      getLeafId: vi.fn().mockReturnValue("leaf-checkpoint-123"),
@@ -1089,7 +1373,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Rejected approach",
    });

-    const { toolMap } = await captureTools(store);
+    const { toolMap } = await captureRethinkTools(store);
    const reviewTool = toolMap.get("review_step");

    // First call task_update to set in-progress (captures checkpoint)
@@ -1123,7 +1407,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Bad approach",
    });

-    const { toolMap, mockNavigateTree } = await captureTools(store);
+    const { toolMap, mockNavigateTree } = await captureRethinkTools(store);

    // Capture checkpoint
    const updateTool = toolMap.get("task_update");
@@ -1153,7 +1437,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Rejected",
    });

-    const { toolMap } = await captureTools(store);
+    const { toolMap } = await captureRethinkTools(store);

    const updateTool = toolMap.get("task_update");
    await updateTool.execute("call-1", { step: 1, status: "in-progress" });
@@ -1181,7 +1465,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Wrong architecture",
    });

-    const { toolMap } = await captureTools(store);
+    const { toolMap } = await captureRethinkTools(store);

    const updateTool = toolMap.get("task_update");
    await updateTool.execute("call-1", { step: 1, status: "in-progress" });
@@ -1212,7 +1496,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Rejected",
    });

-    const { toolMap, mockNavigateTree } = await captureTools(store);
+    const { toolMap, mockNavigateTree } = await captureRethinkTools(store);

    const updateTool = toolMap.get("task_update");
    await updateTool.execute("call-1", { step: 1, status: "in-progress" });
@@ -1247,7 +1531,7 @@ describe("RETHINK verdict handling", () => {
      summary: "Rejected",
    });

-    const { toolMap, mockNavigateTree } = await captureTools(store);
+    const { toolMap, mockNavigateTree } = await captureRethinkTools(store);

    // Do NOT call task_update for step 2, so no checkpoint exists

@@ -1272,7 +1556,7 @@ describe("RETHINK verdict handling", () => {
      makeStepResult(step, status),
    );

-    const { toolMap, mockSessionManager } = await captureTools(store);
+    const { toolMap, mockSessionManager } = await captureRethinkTools(store);

    const updateTool = toolMap.get("task_update");
    await updateTool.execute("call-1", { step: 1, status: "in-progress" });
@@ -1337,3 +1621,238 @@ describe("RETHINK verdict handling", () => {
    );
  });
 });
+
+// ── Plan RETHINK verdict handling tests ──────────────────────────────
+
+describe("Plan RETHINK verdict handling", () => {
+  const makeTask = (id = "KB-050") => ({
+    id,
+    title: "Test",
+    description: "Test",
+    column: "in-progress" as const,
+    dependencies: [],
+    steps: [],
+    currentStep: 0,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+  });
+
+  function makeStepResult(stepIndex: number, status: string) {
+    const steps = Array.from({ length: Math.max(stepIndex + 1, 3) }, (_, i) => ({
+      name: `Step ${i}`,
+      status: i === stepIndex ? status : "pending",
+    }));
+    return { steps };
+  }
+
+  async function capturePlanRethinkTools(store: any) {
+    let capturedTools: any[] = [];
+    const mockSessionManager = {
+      getLeafId: vi.fn().mockReturnValue("plan-checkpoint-789"),
+      branchWithSummary: vi.fn().mockReturnValue("new-branch-id"),
+    };
+    const mockNavigateTree = vi.fn().mockResolvedValue({ cancelled: false });
+    const mockSession = {
+      prompt: vi.fn().mockResolvedValue(undefined),
+      dispose: vi.fn(),
+      sessionManager: mockSessionManager,
+      navigateTree: mockNavigateTree,
+    };
+
+    mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
+      capturedTools = opts.customTools || [];
+      return { session: mockSession } as any;
+    });
+
+    const executor = new TaskExecutor(store, "/tmp/test");
+    await executor.execute(makeTask());
+
+    const toolMap = new Map<string, any>();
+    for (const tool of capturedTools) {
+      toolMap.set(tool.name, tool);
+    }
+    return { toolMap, mockSession, mockSessionManager, mockNavigateTree };
+  }
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockedExistsSync.mockReturnValue(true);
+  });
+
+  it("plan RETHINK verdict rewinds session to pre-step checkpoint", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "Plan is fundamentally flawed",
+      summary: "Bad plan",
+    });
+
+    const { toolMap, mockNavigateTree } = await capturePlanRethinkTools(store);
+
+    // Capture checkpoint by starting step
+    await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
+
+    // Trigger plan RETHINK
+    await toolMap.get("review_step").execute("call-2", {
+      step: 1,
+      type: "plan",
+      step_name: "Test Step",
+    });
+
+    // Session should be rewound to checkpoint
+    expect(mockNavigateTree).toHaveBeenCalledWith("plan-checkpoint-789", { summarize: false });
+  });
+
+  it("plan RETHINK verdict does NOT trigger git reset", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "Wrong plan",
+      summary: "Rejected",
+    });
+
+    const { toolMap } = await capturePlanRethinkTools(store);
+
+    await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
+
+    // Even if baseline is passed, plan RETHINK should NOT git reset
+    await toolMap.get("review_step").execute("call-2", {
+      step: 1,
+      type: "plan",
+      step_name: "Test Step",
+      baseline: "some-sha-that-should-be-ignored",
+    });
+
+    // git reset should NOT be called for plan reviews
+    const gitResetCalls = mockedExecSync.mock.calls.filter(
+      (c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard"),
+    );
+    expect(gitResetCalls).toHaveLength(0);
+  });
+
+  it("plan RETHINK verdict resets step status to pending", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "Try another plan",
+      summary: "Rejected plan",
+    });
+
+    const { toolMap } = await capturePlanRethinkTools(store);
+
+    await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
+
+    await toolMap.get("review_step").execute("call-2", {
+      step: 1,
+      type: "plan",
+      step_name: "Test Step",
+    });
+
+    // updateStep should be called with "pending" to reset the step
+    expect(store.updateStep).toHaveBeenCalledWith("KB-050", 1, "pending");
+  });
+
+  it("plan RETHINK re-prompt includes reviewer feedback and plan-specific language", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "This plan overlooks critical edge cases in error handling",
+      summary: "Insufficient plan",
+    });
+
+    const { toolMap } = await capturePlanRethinkTools(store);
+
+    await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
+
+    const result = await toolMap.get("review_step").execute("call-2", {
+      step: 1,
+      type: "plan",
+      step_name: "Test Step",
+    });
+
+    const text = result.content[0].text;
+    expect(text).toContain("RETHINK");
+    expect(text).toContain("Your plan was rejected");
+    expect(text).toContain("This plan overlooks critical edge cases in error handling");
+    expect(text).toContain("Take a different approach to planning this step");
+    expect(text).toContain("Do NOT repeat the rejected strategy");
+  });
+
+  it("plan RETHINK without session checkpoint falls back gracefully", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "Bad plan",
+      summary: "Rejected",
+    });
+
+    const { toolMap, mockNavigateTree } = await capturePlanRethinkTools(store);
+
+    // Do NOT call task_update for step 2, so no checkpoint exists
+
+    // Call review_step for step 2 — should not crash
+    const result = await toolMap.get("review_step").execute("call-2", {
+      step: 2,
+      type: "plan",
+      step_name: "Test Step",
+    });
+
+    // navigateTree should NOT be called (no checkpoint)
+    expect(mockNavigateTree).not.toHaveBeenCalled();
+
+    // Should still return RETHINK feedback with plan-specific text
+    expect(result.content[0].text).toContain("RETHINK");
+    expect(result.content[0].text).toContain("Your plan was rejected");
+  });
+
+  it("plan RETHINK logs correctly without git reset info", async () => {
+    const store = createMockStore();
+    store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
+      makeStepResult(step, status),
+    );
+
+    mockedReviewStep.mockResolvedValue({
+      verdict: "RETHINK",
+      review: "Wrong plan",
+      summary: "Plan rejected",
+    });
+
+    const { toolMap } = await capturePlanRethinkTools(store);
+
+    await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
+
+    await toolMap.get("review_step").execute("call-2", {
+      step: 1,
+      type: "plan",
+      step_name: "Test Step",
+    });
+
+    // Verify log entry uses plan-specific message (no git reset reference)
+    expect(store.logEntry).toHaveBeenCalledWith(
+      "KB-050",
+      expect.stringContaining("plan rewound"),
+      "Plan rejected",
+    );
+  });
+});
--- 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, AgentSession, SessionManager } from "@mariozechner/pi-coding-agent";
 import type { AgentSemaphore } from "./concurrency.js";
 import type { WorktreePool } from "./worktree-pool.js";
@@ -104,8 +104,13 @@ 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
- **RETHINK** → your code changes have been reverted and conversation rewound. Read the feedback carefully and take a fundamentally different approach. Do NOT repeat the rejected strategy.
+- **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 (code review)** → your code changes have been reverted and conversation rewound. Read the feedback carefully and take a fundamentally different approach. Do NOT repeat the rejected strategy.
+- **RETHINK (plan review)** → conversation rewound to before the step (no git reset since no code was written). Read the feedback and take a fundamentally different approach to planning this step.

 ## Git discipline
 - Commit after completing each step (not after every file change)
@@ -336,11 +341,11 @@ export class TaskExecutor {
      const stepCheckpoints = new Map<number, string>();

      const customTools = [
-        this.createTaskUpdateTool(task.id, sessionRef, stepCheckpoints),
+        this.createTaskUpdateTool(task.id, codeReviewVerdicts, sessionRef, stepCheckpoints),
        this.createTaskLogTool(task.id),
        this.createTaskCreateTool(),
        this.createTaskDoneTool(task.id, () => { taskDone = true; }),
-        this.createReviewStepTool(task.id, worktreePath, detail.prompt, sessionRef, stepCheckpoints),
+        this.createReviewStepTool(task.id, worktreePath, detail.prompt, codeReviewVerdicts, sessionRef, stepCheckpoints),
      ];

      const agentLogger = new AgentLogger({
@@ -426,6 +431,7 @@ export class TaskExecutor {

  private createTaskUpdateTool(
    taskId: string,
+    codeReviewVerdicts: Map<number, ReviewVerdict>,
    sessionRef: { current: AgentSession | null },
    stepCheckpoints: Map<number, string>,
  ): ToolDefinition {
@@ -441,6 +447,22 @@ export class TaskExecutor {
      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: {},
+          };
+        }
+
        // Capture session checkpoint when a step starts, so RETHINK can rewind to it
        if (status === "in-progress" && sessionRef.current) {
          const leafId = sessionRef.current.sessionManager.getLeafId();
@@ -551,6 +573,7 @@ export class TaskExecutor {
    taskId: string,
    worktreePath: string,
    promptContent: string,
+    codeReviewVerdicts: Map<number, ReviewVerdict>,
    sessionRef: { current: AgentSession | null },
    stepCheckpoints: Map<number, string>,
  ): ToolDefinition {
@@ -594,24 +617,43 @@ 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": {
-              // 1. Git reset to baseline
-              if (baseline) {
+              // For code reviews: git reset to baseline to revert file changes
+              // For plan reviews: skip git reset (no code has been written yet)
+              if (reviewType === "code" && baseline) {
                try {
                  execSync(`git reset --hard ${baseline}`, { cwd: worktreePath, stdio: "pipe" });
                  executorLog.log(`${taskId}: RETHINK — git reset --hard ${baseline}`);
                } catch (gitErr: any) {
                  executorLog.error(`${taskId}: RETHINK git reset failed: ${gitErr.message}`);
                }
-              } else {
+              } else if (reviewType === "code") {
                executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`);
              }

-              // 2. Rewind conversation to pre-step checkpoint
+              // Rewind conversation to pre-step checkpoint
              const checkpointId = stepCheckpoints.get(step);
              if (checkpointId && sessionRef.current) {
                try {
@@ -633,16 +675,24 @@ export class TaskExecutor {
                executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`);
              }

-              // 3. Reset step status to pending
+              // Reset step status to pending
              await store.updateStep(taskId, step, "pending");

-              await store.logEntry(
-                taskId,
-                `RETHINK: Step ${step} rewound — git reset to ${baseline || "N/A"}, session checkpoint ${checkpointId || "N/A"}`,
-                result.summary,
-              );
-
-              text = `RETHINK\n\nYour previous approach was rejected. Here is why:\n\n${result.review}\n\nTake a different approach. Do NOT repeat the rejected strategy. Re-read the step requirements and find an alternative solution.`;
+              if (reviewType === "plan") {
+                await store.logEntry(
+                  taskId,
+                  `RETHINK: Step ${step} plan rewound — session checkpoint ${checkpointId || "N/A"}`,
+                  result.summary,
+                );
+                text = `RETHINK\n\nYour plan was rejected. Here is why:\n\n${result.review}\n\nTake a different approach to planning this step. Do NOT repeat the rejected strategy.`;
+              } else {
+                await store.logEntry(
+                  taskId,
+                  `RETHINK: Step ${step} rewound — git reset to ${baseline || "N/A"}, session checkpoint ${checkpointId || "N/A"}`,
+                  result.summary,
+                );
+                text = `RETHINK\n\nYour previous approach was rejected. Here is why:\n\n${result.review}\n\nTake a different approach. Do NOT repeat the rejected strategy. Re-read the step requirements and find an alternative solution.`;
+              }
              break;
            }
            default: text = "UNAVAILABLE — reviewer did not produce a usable verdict.";