root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(KB-182): strengthen spec review post-session gate to require explicit APPROVE

Browse Source 
- Change post-session gate from only blocking REVISE to requiring explicit APPROVE verdict
- Block tasks from reaching executor when review_spec was never called (null verdict)
- Block tasks on RETHINK, UNAVAILABLE, and reviewer-error paths
- Add tests for all non-APPROVE verdict paths: null, RETHINK, UNAVAILABLE, reviewer throw
- Update existing tests to invoke review_spec with APPROVE so they pass the stricter gate
This commit is contained in:
Dustin Byrne
2026-03-28 17:41:10 -04:00
parent acb246a659
commit 2854553199
3 changed files with 307 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

335
packages/engine/src/triage.test.ts
View File

@@ -1425,12 +1425,25 @@ describe("TriageProcessor dependency parsing", () => {
`;
await writePromptMd(tmpDir, "KB-001", promptContent);
mockedCreateHaiAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
},
} as any);
mockedReviewStep.mockResolvedValue({
verdict: "APPROVE",
review: "Good",
summary: "Approved",
});
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
if (reviewSpecTool) await reviewSpecTool.execute("call-1", {});
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
@@ -1469,12 +1482,25 @@ describe("TriageProcessor dependency parsing", () => {
`;
await writePromptMd(tmpDir, "KB-001", promptContent);
mockedCreateHaiAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
},
} as any);
mockedReviewStep.mockResolvedValue({
verdict: "APPROVE",
review: "Good",
summary: "Approved",
});
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
if (reviewSpecTool) await reviewSpecTool.execute("call-1", {});
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
@@ -1506,12 +1532,25 @@ describe("TriageProcessor dependency parsing", () => {
`;
await writePromptMd(tmpDir, "KB-001", promptContent);
mockedCreateHaiAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
},
} as any);
mockedReviewStep.mockResolvedValue({
verdict: "APPROVE",
review: "Good",
summary: "Approved",
});
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
if (reviewSpecTool) await reviewSpecTool.execute("call-1", {});
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
@@ -1822,29 +1861,58 @@ describe("TriageProcessor global pause agent kill", () => {
});
describe("TriageProcessor enginePaused soft pause (no agent termination)", () => {
let tmpDir: string;
beforeEach(() => {
vi.clearAllMocks();
tmpDir = mkdtempSync(join(tmpdir(), "kb-triage-epause-"));
});
afterEach(async () => {
await rm(tmpDir, { recursive: true, force: true });
});
async function writePromptMd(rootDir: string, taskId: string, content: string) {
const dir = join(rootDir, ".kb", "tasks", taskId);
await mkdir(dir, { recursive: true });
await writeFile(join(dir, "PROMPT.md"), content);
}
it("does NOT terminate active triage sessions when enginePaused transitions false→true", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
const disposeFn = vi.fn();
mockedCreateHaiAgent.mockImplementation(async () => ({
session: {
prompt: vi.fn().mockImplementation(async () => {
// Trigger engine pause while the session is active
store._trigger("settings:updated", {
settings: { enginePaused: true },
previous: { enginePaused: false },
});
// Session continues normally — no error thrown
}),
dispose: disposeFn,
},
} as any));
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
const triage = new TriageProcessor(store, "/tmp/test");
mockedReviewStep.mockResolvedValue({
verdict: "APPROVE",
review: "Good",
summary: "Approved",
});
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
// Trigger engine pause while the session is active
store._trigger("settings:updated", {
settings: { enginePaused: true },
previous: { enginePaused: false },
});
// Session continues normally — get APPROVE so task proceeds
if (reviewSpecTool) await reviewSpecTool.execute("call-1", {});
}),
dispose: disposeFn,
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask({
id: "KB-001",
@@ -1868,21 +1936,37 @@ describe("TriageProcessor enginePaused soft pause (no agent termination)", () =>
it("does NOT clear specifying status when enginePaused transitions false→true", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
mockedCreateHaiAgent.mockImplementation(async () => ({
session: {
prompt: vi.fn().mockImplementation(async () => {
store._trigger("settings:updated", {
settings: { enginePaused: true },
previous: { enginePaused: false },
});
// Session continues normally
}),
dispose: vi.fn(),
},
} as any));
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
const triage = new TriageProcessor(store, "/tmp/test");
mockedReviewStep.mockResolvedValue({
verdict: "APPROVE",
review: "Good",
summary: "Approved",
});
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
store._trigger("settings:updated", {
settings: { enginePaused: true },
previous: { enginePaused: false },
});
// Session continues normally — get APPROVE so task proceeds
if (reviewSpecTool) await reviewSpecTool.execute("call-1", {});
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask({
id: "KB-001",
@@ -2243,4 +2327,165 @@ describe("TriageProcessor review_spec tool", () => {
expect(reviewResult.content[0].text).toContain("Missing test requirements");
expect(reviewResult.content[0].text).toContain("call review_spec() again");
});
it("post-session gate blocks when agent finishes without ever calling review_spec (verdict null)", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
mockedCreateHaiAgent.mockResolvedValue({
session: {
prompt: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any);
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
// Task should NOT move to todo
expect(store.moveTask).not.toHaveBeenCalled();
// Status should be cleared
expect(store.updateTask).toHaveBeenCalledWith("KB-001", { status: null });
// Should log that review_spec was never called
const logCalls = store.logEntry.mock.calls;
const gateLog = logCalls.find((c: any[]) =>
typeof c[1] === "string" && c[1].includes("review_spec was never called"),
);
expect(gateLog).toBeDefined();
});
it("post-session gate blocks when last verdict is RETHINK", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
// Agent calls review_spec, gets RETHINK, then finishes without APPROVE
if (reviewSpecTool) {
await reviewSpecTool.execute("call-1", {});
}
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
navigateTree: vi.fn().mockResolvedValue(undefined),
},
} as any;
});
mockedReviewStep.mockResolvedValue({
verdict: "RETHINK",
review: "Wrong approach entirely",
summary: "Rejected",
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
// Task should NOT move to todo
expect(store.moveTask).not.toHaveBeenCalled();
// Status should be cleared
expect(store.updateTask).toHaveBeenCalledWith("KB-001", { status: null });
// Should log the RETHINK verdict
const logCalls = store.logEntry.mock.calls;
const gateLog = logCalls.find((c: any[]) =>
typeof c[1] === "string" && c[1].includes("verdict was RETHINK"),
);
expect(gateLog).toBeDefined();
});
it("post-session gate blocks when reviewer throws (verdict stays null)", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
// Agent calls review_spec but reviewer throws
if (reviewSpecTool) {
await reviewSpecTool.execute("call-1", {});
}
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
mockedReviewStep.mockRejectedValue(new Error("Reviewer API unavailable"));
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
// Task should NOT move to todo — verdict stays null because catch block doesn't set it
expect(store.moveTask).not.toHaveBeenCalled();
// Status should be cleared
expect(store.updateTask).toHaveBeenCalledWith("KB-001", { status: null });
// Should log that review_spec was never called (verdict is null)
const logCalls = store.logEntry.mock.calls;
const gateLog = logCalls.find((c: any[]) =>
typeof c[1] === "string" && c[1].includes("review_spec was never called"),
);
expect(gateLog).toBeDefined();
});
it("post-session gate blocks when verdict is UNAVAILABLE", async () => {
const store = createMockStore();
store.parseDependenciesFromPrompt.mockResolvedValue([]);
const promptContent = "# Task: KB-001\n\n**Size:** S\n\n## Steps\n";
await writePromptMd(tmpDir, "KB-001", promptContent);
let reviewSpecTool: any;
mockedCreateHaiAgent.mockImplementation(async (opts: any) => {
reviewSpecTool = opts.customTools?.find((t: any) => t.name === "review_spec");
return {
session: {
prompt: vi.fn().mockImplementation(async () => {
if (reviewSpecTool) {
await reviewSpecTool.execute("call-1", {});
}
}),
dispose: vi.fn(),
sessionManager: { getLeafId: vi.fn().mockReturnValue("leaf-1") },
},
} as any;
});
mockedReviewStep.mockResolvedValue({
verdict: "UNAVAILABLE",
review: "",
summary: "Reviewer error",
});
const triage = new TriageProcessor(store, tmpDir);
await triage.specifyTask(makeTask());
// Task should NOT move to todo
expect(store.moveTask).not.toHaveBeenCalled();
// Status should be cleared
expect(store.updateTask).toHaveBeenCalledWith("KB-001", { status: null });
// Should log the UNAVAILABLE verdict
const logCalls = store.logEntry.mock.calls;
const gateLog = logCalls.find((c: any[]) =>
typeof c[1] === "string" && c[1].includes("verdict was UNAVAILABLE"),
);
expect(gateLog).toBeDefined();
});
});

18
packages/engine/src/triage.ts
View File

@@ -376,7 +376,7 @@ export class TriageProcessor {
* - **APPROVE**: the spec is accepted and the task moves to `todo`
* - **REVISE**: the agent revises the spec and calls `review_spec()` again.
* If the agent finishes without getting APPROVE, the task is NOT moved to
* `todo` — a post-session gate checks the last verdict.
* `todo` — a post-session gate requires an explicit APPROVE verdict.
* - **RETHINK**: the conversation rewinds to a pre-specification checkpoint
* and the agent starts over with a fundamentally different approach.
*/
@@ -475,15 +475,21 @@ export class TriageProcessor {
// Re-raise errors that pi-coding-agent swallowed after exhausting retries.
checkSessionError(session);
// Post-session REVISE gate: if the last review_spec verdict was REVISE
// and the agent finished without getting APPROVE, don't move to todo.
if (specReviewVerdictRef.current === "REVISE") {
// Post-session APPROVE gate: only advance to todo when the spec
// reviewer explicitly approved. Any other verdict (REVISE,
// RETHINK, UNAVAILABLE) or a missing review (null) keeps the task
// in triage so unreviewed / rejected specs never reach execution.
if (specReviewVerdictRef.current !== "APPROVE") {
const verdictDesc =
specReviewVerdictRef.current === null
? "review_spec was never called"
: `verdict was ${specReviewVerdictRef.current}`;
triageLog.log(
`${task.id} spec review ended with REVISE — not moving to todo`,
`${task.id} spec review not approved (${verdictDesc}) — not moving to todo`,
);
await this.store.logEntry(
task.id,
"Spec review ended with REVISE verdict — specification not approved",
`Spec review not approved (${verdictDesc}) — specification not approved`,
);
await this.store.updateTask(task.id, { status: null });
return;