feat(engine): harden review pipeline with strict scope, build retry, and E2E tests
Improve the plan→review→approve→merge agent pipeline: - Harden verdict extraction with JSON block parsing and anchored regexes - Consolidate legacy/new merger conflict APIs into thin deprecated wrappers - Add configurable strict scope enforcement (strictScopeEnforcement setting) - Add build retry with timeout to merger (buildRetryCount, buildTimeoutMs) - Add handleChangesRequested to PrCommentHandler for review feedback loop - Remove dead code: handleFsChange, processTaskChange, unused imports/fields - Add E2E multi-verdict sequence tests for the full review pipeline - Fix unused parameter warnings across engine and core packages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -3261,6 +3261,237 @@ describe("Plan RETHINK verdict handling", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── E2E review pipeline sequence tests ─────────────────────────────
|
||||
|
||||
describe("E2E review pipeline — multi-verdict sequence", () => {
|
||||
/**
|
||||
* Exercises the full review pipeline within a single task execution:
|
||||
* plan review → APPROVE
|
||||
* code review → REVISE (blocked)
|
||||
* code review → APPROVE (unblocked)
|
||||
* step done → success
|
||||
*
|
||||
* Verifies that verdicts compose correctly across the full lifecycle.
|
||||
*/
|
||||
|
||||
function makeStepResult(stepIndex: number, status: string) {
|
||||
const steps = Array.from({ length: 3 }, (_, i) => ({
|
||||
name: [`Preflight`, `Implement`, `Tests`][i],
|
||||
status: i === stepIndex ? status : i < stepIndex ? "done" : "pending",
|
||||
}));
|
||||
return { steps };
|
||||
}
|
||||
|
||||
async function captureE2ETools(store: any) {
|
||||
let capturedTools: any[] = [];
|
||||
const mockSessionManager = {
|
||||
getLeafId: vi.fn().mockReturnValue("e2e-checkpoint"),
|
||||
branchWithSummary: vi.fn(),
|
||||
};
|
||||
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({
|
||||
id: "FN-E2E",
|
||||
title: "E2E Test",
|
||||
description: "E2E pipeline 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, mockNavigateTree, mockSessionManager };
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockedExistsSync.mockReturnValue(true);
|
||||
});
|
||||
|
||||
it("full sequence: plan APPROVE → code REVISE (blocked) → code APPROVE (unblocked) → done", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const { tools } = await captureE2ETools(store);
|
||||
|
||||
// Step 1: Start the step
|
||||
await tools.task_update("u1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Step 2: Plan review → APPROVE (advisory, no blocking)
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Good plan", summary: "Approved" });
|
||||
const planResult = await tools.review_step("r1", {
|
||||
step: 1, type: "plan", step_name: "Implement",
|
||||
});
|
||||
expect(planResult.content[0].text).toBe("APPROVE");
|
||||
|
||||
// Step 3: Code review → REVISE (should block advancement)
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "REVISE", review: "Missing error handling in fetchUser()", summary: "Needs fixes",
|
||||
});
|
||||
const reviseResult = await tools.review_step("r2", {
|
||||
step: 1, type: "code", step_name: "Implement", baseline: "sha-1",
|
||||
});
|
||||
expect(reviseResult.content[0].text).toContain("cannot be marked done");
|
||||
|
||||
// Step 4: Attempt to mark done — should be blocked
|
||||
const blockedResult = await tools.task_update("u2", { step: 1, status: "done" });
|
||||
expect(blockedResult.content[0].text).toContain("Cannot mark Step 1 as done");
|
||||
|
||||
// Step 5: Fix issues, re-submit code review → APPROVE
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "APPROVE", review: "Error handling added correctly", summary: "All good",
|
||||
});
|
||||
const approveResult = await tools.review_step("r3", {
|
||||
step: 1, type: "code", step_name: "Implement", baseline: "sha-2",
|
||||
});
|
||||
expect(approveResult.content[0].text).toBe("APPROVE");
|
||||
|
||||
// Step 6: Now marking done should succeed
|
||||
const doneResult = await tools.task_update("u3", { step: 1, status: "done" });
|
||||
expect(doneResult.content[0].text).toContain("→ done");
|
||||
});
|
||||
|
||||
it("full sequence: code RETHINK → git reset + session rewind → retry with APPROVE → done", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const { tools, mockNavigateTree } = await captureE2ETools(store);
|
||||
|
||||
// Step 1: Start the step (captures checkpoint)
|
||||
await tools.task_update("u1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Step 2: Code review → RETHINK (rewind everything)
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK", review: "Using polling instead of events is wrong", summary: "Bad approach",
|
||||
});
|
||||
const rethinkResult = await tools.review_step("r1", {
|
||||
step: 1, type: "code", step_name: "Implement", baseline: "sha-bad",
|
||||
});
|
||||
|
||||
// Verify RETHINK outcomes
|
||||
expect(rethinkResult.content[0].text).toContain("RETHINK");
|
||||
expect(rethinkResult.content[0].text).toContain("Do NOT repeat the rejected strategy");
|
||||
expect(mockedExecSync).toHaveBeenCalledWith(
|
||||
"git reset --hard sha-bad",
|
||||
expect.objectContaining({ cwd: expect.any(String) }),
|
||||
);
|
||||
expect(mockNavigateTree).toHaveBeenCalledWith("e2e-checkpoint", { summarize: false });
|
||||
expect(store.updateStep).toHaveBeenCalledWith("FN-E2E", 1, "pending");
|
||||
|
||||
// Step 3: Restart the step (new approach)
|
||||
await tools.task_update("u2", { step: 1, status: "in-progress" });
|
||||
|
||||
// Step 4: Code review → APPROVE on second attempt
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "APPROVE", review: "Event-driven approach is correct", summary: "Approved",
|
||||
});
|
||||
const approveResult = await tools.review_step("r2", {
|
||||
step: 1, type: "code", step_name: "Implement", baseline: "sha-good",
|
||||
});
|
||||
expect(approveResult.content[0].text).toBe("APPROVE");
|
||||
|
||||
// Step 5: Mark done — should succeed (no REVISE blocking)
|
||||
const doneResult = await tools.task_update("u3", { step: 1, status: "done" });
|
||||
expect(doneResult.content[0].text).toContain("→ done");
|
||||
});
|
||||
|
||||
it("multi-step pipeline: step 1 APPROVE, step 2 REVISE, step 1 remains unaffected", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const { tools } = await captureE2ETools(store);
|
||||
|
||||
// Step 1: Complete with APPROVE
|
||||
await tools.task_update("u1", { step: 1, status: "in-progress" });
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "OK", summary: "Good" });
|
||||
await tools.review_step("r1", { step: 1, type: "code", step_name: "Implement", baseline: "sha-1" });
|
||||
const step1Done = await tools.task_update("u2", { step: 1, status: "done" });
|
||||
expect(step1Done.content[0].text).toContain("→ done");
|
||||
|
||||
// Step 2: Gets REVISE
|
||||
await tools.task_update("u3", { step: 2, status: "in-progress" });
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "REVISE", review: "Tests insufficient", summary: "Bad" });
|
||||
await tools.review_step("r2", { step: 2, type: "code", step_name: "Tests", baseline: "sha-2" });
|
||||
|
||||
// Step 2 blocked
|
||||
const step2Blocked = await tools.task_update("u4", { step: 2, status: "done" });
|
||||
expect(step2Blocked.content[0].text).toContain("Cannot mark Step 2 as done");
|
||||
|
||||
// Step 1 remains unaffected — if agent tries to re-update step 1, it still works
|
||||
// (step isolation: REVISE on step 2 does not affect step 1)
|
||||
});
|
||||
|
||||
it("plan RETHINK followed by plan APPROVE allows code phase to proceed", async () => {
|
||||
const store = createMockStore();
|
||||
store.updateStep.mockImplementation(async (_id: string, step: number, status: string) =>
|
||||
makeStepResult(step, status),
|
||||
);
|
||||
|
||||
const { tools, mockNavigateTree } = await captureE2ETools(store);
|
||||
|
||||
// Start step
|
||||
await tools.task_update("u1", { step: 1, status: "in-progress" });
|
||||
|
||||
// Plan review → RETHINK
|
||||
mockedReviewStep.mockResolvedValue({
|
||||
verdict: "RETHINK", review: "Plan ignores edge cases", summary: "Bad plan",
|
||||
});
|
||||
const rethinkResult = await tools.review_step("r1", {
|
||||
step: 1, type: "plan", step_name: "Implement",
|
||||
});
|
||||
expect(rethinkResult.content[0].text).toContain("Your plan was rejected");
|
||||
|
||||
// Verify plan RETHINK does NOT trigger git reset
|
||||
const gitResetCalls = mockedExecSync.mock.calls.filter(
|
||||
(c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard"),
|
||||
);
|
||||
expect(gitResetCalls).toHaveLength(0);
|
||||
|
||||
// Session was rewound
|
||||
expect(mockNavigateTree).toHaveBeenCalled();
|
||||
|
||||
// Restart step with new plan
|
||||
await tools.task_update("u2", { step: 1, status: "in-progress" });
|
||||
|
||||
// Plan review → APPROVE
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Good plan", summary: "Approved" });
|
||||
await tools.review_step("r2", { step: 1, type: "plan", step_name: "Implement" });
|
||||
|
||||
// Code phase: APPROVE directly
|
||||
mockedReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Clean code", summary: "Good" });
|
||||
await tools.review_step("r3", { step: 1, type: "code", step_name: "Implement", baseline: "sha-1" });
|
||||
|
||||
// Mark done — should succeed (plan reviews are advisory, code APPROVE clears the path)
|
||||
const doneResult = await tools.task_update("u3", { step: 1, status: "done" });
|
||||
expect(doneResult.content[0].text).toContain("→ done");
|
||||
});
|
||||
});
|
||||
|
||||
// ── task_add_dep tool tests ──────────────────────────────────────────
|
||||
|
||||
describe("task_add_dep tool", () => {
|
||||
|
||||
Reference in New Issue
Block a user