feat(KB-125): implement RETHINK verdict with git reset and session rewind
- Capture pre-step session checkpoint in task_update when status changes to in-progress - Handle RETHINK verdict with git reset --hard to baseline SHA to revert code changes - Rewind agent conversation to pre-step checkpoint via navigateTree with branchWithSummary fallback - Update system prompt to describe RETHINK as automatic revert + rewind behavior - Replace code review REVISE enforcement tests with comprehensive RETHINK rewind tests
This commit is contained in:
Dustin Byrne
@@ -29,7 +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 { reviewStep } from "./reviewer.js";
|
||||
import { execSync } from "node:child_process";
|
||||
import { findWorktreeUser, aiMergeTask } from "./merger.js";
|
||||
import { WorktreePool } from "./worktree-pool.js";
|
||||
@@ -1014,42 +1014,14 @@ describe("TaskExecutor pause behavior", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── Code review verdict enforcement tests ────────────────────────────
|
||||
const mockedReviewStep = vi.mocked(reviewStep);
|
||||
|
||||
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",
|
||||
describe("RETHINK verdict handling", () => {
|
||||
const makeTask = (id = "KB-040") => ({
|
||||
id,
|
||||
title: "Test",
|
||||
description: "Test",
|
||||
column: "in-progress",
|
||||
column: "in-progress" as const,
|
||||
dependencies: [],
|
||||
steps: [],
|
||||
currentStep: 0,
|
||||
@@ -1058,234 +1030,310 @@ async function captureTools(): Promise<Record<string, (id: string, params: any)
|
||||
updatedAt: new Date().toISOString(),
|
||||
});
|
||||
|
||||
const tools: Record<string, any> = {};
|
||||
for (const t of capturedTools) {
|
||||
tools[t.name] = t.execute;
|
||||
/** Return value for store.updateStep that satisfies the task_update tool. */
|
||||
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 };
|
||||
}
|
||||
return tools;
|
||||
}
|
||||
|
||||
describe("Code review verdict tracking", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockedExistsSync.mockReturnValue(true);
|
||||
});
|
||||
/**
|
||||
* 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) {
|
||||
let capturedTools: any[] = [];
|
||||
const mockSessionManager = {
|
||||
getLeafId: vi.fn().mockReturnValue("leaf-checkpoint-123"),
|
||||
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,
|
||||
};
|
||||
|
||||
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;
|
||||
capturedTools = opts.customTools || [];
|
||||
return { session: mockSession } 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(),
|
||||
});
|
||||
const executor = new TaskExecutor(store, "/tmp/test", options);
|
||||
await executor.execute(makeTask());
|
||||
|
||||
// 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");
|
||||
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("task_update with non-done status is not blocked by REVISE", async () => {
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Fix", summary: "Bad" });
|
||||
it("RETHINK verdict triggers git reset --hard to baseline SHA", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const tools = await captureTools();
|
||||
await tools.review_step("c1", { step: 1, type: "code", step_name: "Step1", baseline: "a" });
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK",
|
||||
review: "Wrong approach, try something else",
|
||||
summary: "Rejected approach",
|
||||
});
|
||||
|
||||
// "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");
|
||||
const { toolMap } = await captureTools(store);
|
||||
const reviewTool = toolMap.get("review_step");
|
||||
|
||||
// First call task_update to set in-progress (captures checkpoint)
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Now call review_step with a baseline
|
||||
const result = await reviewTool.execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123def",
|
||||
});
|
||||
|
||||
// Verify git reset was called
|
||||
expect(mockedExecSync).toHaveBeenCalledWith(
|
||||
"git reset --hard abc123def",
|
||||
expect.objectContaining({ cwd: expect.stringContaining(".worktrees/") }),
|
||||
);
|
||||
});
|
||||
|
||||
it("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: "Fundamentally wrong",
|
||||
summary: "Bad approach",
|
||||
});
|
||||
|
||||
const { toolMap, mockNavigateTree } = await captureTools(store);
|
||||
|
||||
// Capture checkpoint
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Trigger RETHINK
|
||||
await toolMap.get("review_step").execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123",
|
||||
});
|
||||
|
||||
// Verify navigateTree was called with checkpoint and summarize: false
|
||||
expect(mockNavigateTree).toHaveBeenCalledWith("leaf-checkpoint-123", { summarize: false });
|
||||
});
|
||||
|
||||
it("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 again",
|
||||
summary: "Rejected",
|
||||
});
|
||||
|
||||
const { toolMap } = await captureTools(store);
|
||||
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
await toolMap.get("review_step").execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123",
|
||||
});
|
||||
|
||||
// updateStep should be called: once for in-progress, once for pending (reset)
|
||||
expect(store.updateStep).toHaveBeenCalledWith("KB-040", 1, "pending");
|
||||
});
|
||||
|
||||
it("RETHINK re-prompt includes reviewer feedback", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK",
|
||||
review: "Your approach uses polling when it should use events",
|
||||
summary: "Wrong architecture",
|
||||
});
|
||||
|
||||
const { toolMap } = await captureTools(store);
|
||||
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
const result = await toolMap.get("review_step").execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123",
|
||||
});
|
||||
|
||||
const text = result.content[0].text;
|
||||
expect(text).toContain("RETHINK");
|
||||
expect(text).toContain("Your approach uses polling when it should use events");
|
||||
expect(text).toContain("Take a different approach");
|
||||
expect(text).toContain("Do NOT repeat the rejected strategy");
|
||||
});
|
||||
|
||||
it("RETHINK without baseline SHA skips git reset but still rewinds conversation", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK",
|
||||
review: "Wrong approach",
|
||||
summary: "Rejected",
|
||||
});
|
||||
|
||||
const { toolMap, mockNavigateTree } = await captureTools(store);
|
||||
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Call review_step WITHOUT baseline
|
||||
await toolMap.get("review_step").execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
// no baseline
|
||||
});
|
||||
|
||||
// git reset should NOT be called (no baseline)
|
||||
const gitResetCalls = mockedExecSync.mock.calls.filter(
|
||||
(c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard"),
|
||||
);
|
||||
expect(gitResetCalls).toHaveLength(0);
|
||||
|
||||
// But session rewind should still happen
|
||||
expect(mockNavigateTree).toHaveBeenCalledWith("leaf-checkpoint-123", { summarize: false });
|
||||
});
|
||||
|
||||
it("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 approach",
|
||||
summary: "Rejected",
|
||||
});
|
||||
|
||||
const { toolMap, mockNavigateTree } = await captureTools(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: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123",
|
||||
});
|
||||
|
||||
// navigateTree should NOT be called (no checkpoint)
|
||||
expect(mockNavigateTree).not.toHaveBeenCalled();
|
||||
|
||||
// Should still return RETHINK feedback
|
||||
expect(result.content[0].text).toContain("RETHINK");
|
||||
});
|
||||
|
||||
it("pre-step checkpoint is captured when task_update sets status to in-progress", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const { toolMap, mockSessionManager } = await captureTools(store);
|
||||
|
||||
const updateTool = toolMap.get("task_update");
|
||||
await updateTool.execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Verify getLeafId was called
|
||||
expect(mockSessionManager.getLeafId).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("RETHINK falls back to branchWithSummary when navigateTree fails", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK",
|
||||
review: "Wrong approach",
|
||||
summary: "Rejected",
|
||||
});
|
||||
|
||||
// Create tools but make navigateTree throw
|
||||
let capturedTools: any[] = [];
|
||||
const mockSessionManager = {
|
||||
getLeafId: vi.fn().mockReturnValue("leaf-checkpoint-456"),
|
||||
branchWithSummary: vi.fn().mockReturnValue("new-branch-id"),
|
||||
};
|
||||
const mockNavigateTree = vi.fn().mockRejectedValue(new Error("navigateTree not available"));
|
||||
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);
|
||||
|
||||
// Capture checkpoint
|
||||
await toolMap.get("task_update").execute("call-1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Trigger RETHINK
|
||||
await toolMap.get("review_step").execute("call-2", {
|
||||
step: 1,
|
||||
type: "code",
|
||||
step_name: "Test Step",
|
||||
baseline: "abc123",
|
||||
});
|
||||
|
||||
// navigateTree was called but failed → should fall back to branchWithSummary
|
||||
expect(mockNavigateTree).toHaveBeenCalled();
|
||||
expect(mockSessionManager.branchWithSummary).toHaveBeenCalledWith(
|
||||
"leaf-checkpoint-456",
|
||||
expect.stringContaining("RETHINK"),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -6,8 +6,8 @@ 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, type ReviewVerdict } from "./reviewer.js";
|
||||
import type { ToolDefinition } from "@mariozechner/pi-coding-agent";
|
||||
import { reviewStep } 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";
|
||||
import { AgentLogger } from "./agent-logger.js";
|
||||
@@ -104,12 +104,8 @@ model, read-only access) to independently assess your work.
|
||||
|
||||
**Handling verdicts:**
|
||||
- **APPROVE** → proceed to next step
|
||||
- **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
|
||||
- **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.
|
||||
|
||||
## Git discipline
|
||||
- Commit after completing each step (not after every file change)
|
||||
@@ -335,12 +331,16 @@ export class TaskExecutor {
|
||||
const codeReviewVerdicts = new Map<number, ReviewVerdict>();
|
||||
|
||||
let taskDone = false;
|
||||
// Mutable ref — populated after createKbAgent, tools access lazily via closure
|
||||
const sessionRef: { current: AgentSession | null } = { current: null };
|
||||
const stepCheckpoints = new Map<number, string>();
|
||||
|
||||
const customTools = [
|
||||
this.createTaskUpdateTool(task.id, codeReviewVerdicts),
|
||||
this.createTaskUpdateTool(task.id, sessionRef, stepCheckpoints),
|
||||
this.createTaskLogTool(task.id),
|
||||
this.createTaskCreateTool(),
|
||||
this.createTaskDoneTool(task.id, () => { taskDone = true; }),
|
||||
this.createReviewStepTool(task.id, worktreePath, detail.prompt, codeReviewVerdicts),
|
||||
this.createReviewStepTool(task.id, worktreePath, detail.prompt, sessionRef, stepCheckpoints),
|
||||
];
|
||||
|
||||
const agentLogger = new AgentLogger({
|
||||
@@ -366,6 +366,9 @@ export class TaskExecutor {
|
||||
defaultThinkingLevel: settings.defaultThinkingLevel,
|
||||
});
|
||||
|
||||
// Make session available to custom tools (task_update checkpoint capture, review_step rewind)
|
||||
sessionRef.current = session;
|
||||
|
||||
// Register session so the pause listener can terminate it
|
||||
this.activeSessions.set(task.id, session);
|
||||
|
||||
@@ -423,7 +426,8 @@ export class TaskExecutor {
|
||||
|
||||
private createTaskUpdateTool(
|
||||
taskId: string,
|
||||
codeReviewVerdicts: Map<number, ReviewVerdict>,
|
||||
sessionRef: { current: AgentSession | null },
|
||||
stepCheckpoints: Map<number, string>,
|
||||
): ToolDefinition {
|
||||
const store = this.store;
|
||||
return {
|
||||
@@ -437,20 +441,12 @@ 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();
|
||||
if (leafId) {
|
||||
stepCheckpoints.set(step, leafId);
|
||||
}
|
||||
}
|
||||
|
||||
const task = await store.updateStep(taskId, step, status as StepStatus);
|
||||
@@ -542,11 +538,21 @@ export class TaskExecutor {
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Create the review_step tool for the executor agent.
|
||||
*
|
||||
* When the reviewer returns a RETHINK verdict, this tool:
|
||||
* 1. Runs `git reset --hard <baseline>` to revert file changes
|
||||
* 2. Rewinds the conversation to the pre-step checkpoint via `session.navigateTree()`
|
||||
* 3. Resets the step status to "pending"
|
||||
* 4. Returns a re-prompt instructing the agent to take a different approach
|
||||
*/
|
||||
private createReviewStepTool(
|
||||
taskId: string,
|
||||
worktreePath: string,
|
||||
promptContent: string,
|
||||
codeReviewVerdicts: Map<number, ReviewVerdict>,
|
||||
sessionRef: { current: AgentSession | null },
|
||||
stepCheckpoints: Map<number, string>,
|
||||
): ToolDefinition {
|
||||
const store = this.store;
|
||||
const options = this.options;
|
||||
@@ -588,29 +594,57 @@ 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":
|
||||
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}`;
|
||||
case "REVISE": text = `REVISE\n\n${result.review}`; break;
|
||||
case "RETHINK": {
|
||||
// 1. Git reset to baseline
|
||||
if (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 {
|
||||
text = `REVISE\n\n${result.review}`;
|
||||
executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`);
|
||||
}
|
||||
|
||||
// 2. Rewind conversation to pre-step checkpoint
|
||||
const checkpointId = stepCheckpoints.get(step);
|
||||
if (checkpointId && sessionRef.current) {
|
||||
try {
|
||||
await sessionRef.current.navigateTree(checkpointId, { summarize: false });
|
||||
executorLog.log(`${taskId}: RETHINK — session rewound to checkpoint ${checkpointId}`);
|
||||
} catch {
|
||||
// Fallback to branchWithSummary
|
||||
try {
|
||||
sessionRef.current.sessionManager.branchWithSummary(
|
||||
checkpointId,
|
||||
`RETHINK: ${result.summary || "Approach rejected by reviewer"}`,
|
||||
);
|
||||
executorLog.log(`${taskId}: RETHINK — branched from checkpoint ${checkpointId}`);
|
||||
} catch (branchErr: any) {
|
||||
executorLog.error(`${taskId}: RETHINK session rewind failed: ${branchErr.message}`);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`);
|
||||
}
|
||||
|
||||
// 3. 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.`;
|
||||
break;
|
||||
case "RETHINK": text = `RETHINK\n\n${result.review}`; break;
|
||||
}
|
||||
default: text = "UNAVAILABLE — reviewer did not produce a usable verdict.";
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user