-
Notifications
You must be signed in to change notification settings - Fork 4
fix: stop runaway submit_verdict loop in leaf turn loop #149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import type { ExtensionAPI, ExtensionFactory } from "@earendil-works/pi-coding-agent"; | ||
| import type { VerdictCapture } from "./submit-verdict-tool.js"; | ||
|
|
||
| export interface TurnCounter { | ||
| turns: number; | ||
| } | ||
|
|
||
| // Envelopes don't always set `maxTurns` (it's optional on LeafEnvelope), but the runaway-loop | ||
| // bug this extension exists to prevent has no other guard — so fall back to a hard cap even when | ||
| // the caller omits one, rather than silently skipping turn-limiting for those leaves. | ||
| const DEFAULT_MAX_TURNS = 40; | ||
|
|
||
| /** | ||
| * Backstop for the leaf agent loop: caps total turns even if a verdict is never submitted, and | ||
| * blocks any further tool calls once a verdict HAS been captured (in case the model tries to call | ||
| * more tools after submit_verdict, e.g. as part of a parallel batch that didn't all set | ||
| * `terminate`). The primary fix lives in submit-verdict-tool.ts, which now sets `terminate: true` | ||
| * on its own tool result — agent-loop.ts stops the turn loop as soon as every tool call in a batch | ||
| * sets that flag (AgentToolResult.terminate). This extension exists because that primary fix only | ||
| * covers the "model calls submit_verdict cleanly" path; without a cap, a model that never calls | ||
| * submit_verdict (or calls it alongside other tools that don't terminate) can still spin forever — | ||
| * one observed leaf called submit_verdict ~1,269 times before an external pod kill stopped it. | ||
| * | ||
| * `tool_call`'s `block` result is the only extension hook that can halt execution here — | ||
| * `tool_result`/`turn_end` results carry no loop-control field (AgentSession.afterToolCall drops | ||
| * any `terminate` an extension tries to set on `tool_result`). Blocking still costs one turn (the | ||
| * model sees a "blocked" tool error and gets re-prompted) rather than stopping instantly, so this | ||
| * is a safety net, not the fast path. `runLeaf` already treats a leaf with no captured verdict as | ||
| * `no_verdict` (a normal, already-handled failure outcome), so hitting the turn cap without a | ||
| * verdict fails cleanly instead of hanging the queue message forever. | ||
| */ | ||
| export function verdictTerminationExtension( | ||
| capture: VerdictCapture, | ||
| opts: { maxTurns?: number } = {}, | ||
| ): ExtensionFactory { | ||
| const counter: TurnCounter = { turns: 0 }; | ||
| // Treat a missing OR non-positive maxTurns as "use the default" — a nullish-coalesce alone would | ||
| // let an explicit 0 (or negative) through, which then fails the `> 0` guard below and silently | ||
| // DISABLES the cap (unbounded), the opposite of what a caller passing 0 would expect. | ||
| const maxTurns = typeof opts.maxTurns === "number" && opts.maxTurns > 0 ? opts.maxTurns : DEFAULT_MAX_TURNS; | ||
| return (pi: ExtensionAPI) => { | ||
| pi.on("turn_start", () => { | ||
| counter.turns += 1; | ||
| }); | ||
| pi.on("tool_call", (event) => { | ||
| if (capture.verdict) { | ||
| return { block: true, reason: "Verdict already submitted for this item — task complete." }; | ||
| } | ||
| if (typeof maxTurns === "number" && maxTurns > 0 && counter.turns > maxTurns) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: asymmetry in the cap. Omitting |
||
| return { | ||
| block: true, | ||
| reason: `Turn limit (${maxTurns}) reached without a submitted verdict — stopping.`, | ||
| }; | ||
| } | ||
| return {}; | ||
| }); | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { describe, it, expect } from "vitest"; | ||
| import { verdictTerminationExtension } from "../src/verdict-termination-extension"; | ||
| import type { VerdictCapture } from "../src/submit-verdict-tool"; | ||
|
|
||
| function fakePi() { | ||
| const handlers: Record<string, Function> = {}; | ||
| const pi = { on: (ev: string, h: Function) => { handlers[ev] = h; } }; | ||
| return { pi: pi as any, handlers }; | ||
| } | ||
|
|
||
| describe("verdictTerminationExtension", () => { | ||
| it("does not block tool calls before a verdict is captured and under the turn cap", () => { | ||
| const capture: VerdictCapture = {}; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture, { maxTurns: 5 })(pi); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| it("blocks tool calls once a verdict has already been captured", () => { | ||
| const capture: VerdictCapture = { verdict: { item_id: "i1", verdict: "CLEAR", reason: "r" } }; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture, { maxTurns: 5 })(pi); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result.block).toBe(true); | ||
| expect(result.reason).toMatch(/verdict already submitted/i); | ||
| }); | ||
|
|
||
| it("blocks tool calls once maxTurns is exceeded without a verdict", () => { | ||
| const capture: VerdictCapture = {}; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture, { maxTurns: 2 })(pi); | ||
| // turn_start fires once per turn; simulate 3 turns (exceeds cap of 2) | ||
| handlers["turn_start"](); | ||
| handlers["turn_start"](); | ||
| handlers["turn_start"](); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result.block).toBe(true); | ||
| expect(result.reason).toMatch(/turn limit/i); | ||
| }); | ||
|
|
||
| it("does not block at exactly the turn cap (only after exceeding it)", () => { | ||
| const capture: VerdictCapture = {}; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture, { maxTurns: 2 })(pi); | ||
| handlers["turn_start"](); | ||
| handlers["turn_start"](); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result).toEqual({}); | ||
| }); | ||
|
|
||
| it("applies a default turn cap when maxTurns is omitted (never leaves the loop unbounded)", () => { | ||
| const capture: VerdictCapture = {}; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture)(pi); | ||
| for (let i = 0; i < 41; i++) handlers["turn_start"](); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result.block).toBe(true); | ||
| expect(result.reason).toMatch(/turn limit/i); | ||
| }); | ||
|
|
||
| it("treats maxTurns: 0 as 'use default' rather than disabling the cap", () => { | ||
| const capture: VerdictCapture = {}; | ||
| const { pi, handlers } = fakePi(); | ||
| verdictTerminationExtension(capture, { maxTurns: 0 })(pi); | ||
| // 0 must NOT mean unbounded — it should fall back to the default cap and still block. | ||
| for (let i = 0; i < 41; i++) handlers["turn_start"](); | ||
| const result = handlers["tool_call"]({}); | ||
| expect(result.block).toBe(true); | ||
| expect(result.reason).toMatch(/turn limit/i); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): the backstop only
blocks tool calls — which (as your docstring notes) re-prompts and costs a turn rather than hard-stopping. So a model that keeps emitting tool calls after a verdict, or pastmaxTurns, still advances turns and only stops when it stops calling tools. The docstring's claim that hitting the cap "fails cleanly instead of hanging" relies on an independent hard iteration ceiling inagent-loop.ts. Worth a one-line test/assert that exceedingmaxTurnsactually ends the leaf (not just blocks the tool), so the backstop's guarantee doesn't silently depend on the model choosing to stop.