diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..ae5e027 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,78 @@ +# Woopcode + +A terminal-native coding agent: a streaming agent loop driving a React Ink +interface, and the same loop driving a headless single-prompt path. This file is +the glossary — what the words mean here. Mechanism lives in the code, and the +reasoning behind a hard-to-reverse choice lives in `docs/adr/`. + +## Language + +### A unit of work + +**Turn**: +One stretch of work answering one user message, from the loop being entered to +it returning an answer. A turn survives being asked to continue at the iteration +ceiling; it is still the same turn. +_Avoid_: request, session, conversation + +**Iteration**: +One provider response and the tool calls it carried. The unit both budgets and +every counter are measured in. +_Avoid_: loop, cycle, round + +**Step**: +An iteration, when counted against what remains rather than what has happened. +The wall budget is converted into steps at the rate the turn has been running +at, so one warning can serve both budgets. + +**Turn-initiating message**: +The message that started the turn — the last real conversation turn present when +the loop was entered. In a headless run it is the task statement; in the +interface it is what the user just typed. Pinned into the window for the life of +the turn, because the loop pushes messages of its own that would otherwise crowd +it out. +_Avoid_: the prompt, the first message, the task + +**Window**: +The tail of the transcript actually sent to the provider, counted in +conversation turns rather than messages. Distinct from the transcript, which is +everything the turn has accumulated. Its turn ceiling bounds the tail, not the +whole window: a pinned turn-initiating message rides outside it, so a window can +hold one turn more than the ceiling names, and never two. + +### Ending a turn + +**Finish gate**: +A check that runs when the model responds without calling a tool, deciding +whether the turn may end or must go round once more. There are two, and both may +answer at once, in which case the turn receives a single message. + +**Verification reminder**: +The finish gate that fires on unverified edits. Evidence the loop can see. + +**Requirement gate**: +The finish gate that fires on an unattended turn stopping early with budget in +hand, asking it to prove each stated requirement with command output. Aimed at +what the loop cannot see: work that was checked thoroughly against the wrong +property. + +**Unverified edits**: +A turn that changed the workspace and ran no shell command afterwards. Order +within an iteration is what decides it, so this is counted in tool executions +rather than iterations. + +**Unattended**: +Nobody is reading the answer as it arrives, so a wrong one stands. True of the +headless path, false of the interface — where a person can correct a turn for +the cost of one sentence. +_Avoid_: headless, non-interactive, automated + +**Wind-down warning**: +The message telling the model its turn is nearly over and to start nothing new. +Derived from a rate measured on the turn itself, so it can clear again when the +rate recovers. + +**Budget**: +What bounds a turn. Two of them — iterations and wall-clock seconds — and a turn +stops on whichever binds first. Neither is a quota: the provider enforces that +itself. diff --git a/commands/agent.tsx b/commands/agent.tsx index 1c38739..8915305 100644 --- a/commands/agent.tsx +++ b/commands/agent.tsx @@ -205,6 +205,11 @@ async function runHeadless( const selectedModel = await resolveModel(options.model); store.setSelectedModel(selectedModel); + // Half of "nobody is watching this run". This half is the approval path's, + // read through the store; the other half is `controller.setUnattended` below, + // which is the loop's. They are not merged because `runtime/loop.ts` must not + // import the interface's store — so the one fact is stated twice, on purpose, + // and each site names the other. store.setNonInteractive({ autoApprove }); const log = createEventLog(options.events); @@ -302,6 +307,10 @@ async function runHeadless( }; const controller = new AgentController(provider, apiKey, selectedModel, callbacks, baseUrl); + // The other half of the fact stated at `store.setNonInteractive` above: + // nobody is reading the answer as it streams, so the loop's finish gates + // apply to this run. + controller.setUnattended(true); await controller.initialize(options.session); // On stderr, not stdout: stdout is the agent's answer and a caller pipes it. diff --git a/commands/agentController.ts b/commands/agentController.ts index 66339c4..ac1f393 100644 --- a/commands/agentController.ts +++ b/commands/agentController.ts @@ -103,6 +103,8 @@ export class AgentController { * first edit of the next session. */ private sessionMode: SessionMode = "build"; + /** See setUnattended. False for the TUI, true for `-p`. */ + private unattended = false; /** * The session this turn belongs to. Null until `initialize`, and null for the * lifetime of a run started with persistence off. @@ -176,6 +178,19 @@ export class AgentController { return demo ? new Error(demo) : error; } + /** + * Declares that nobody is watching this session's turns. + * + * Set by the headless path only, beside the store's own non-interactive flag. + * A property rather than an argument to `run`, because it is true of the + * session rather than of one prompt — and deliberately not inferred inside + * the loop from a missing callback, which would extend the behaviour to every + * embedder that happens not to pass one. + */ + setUnattended(unattended: boolean) { + this.unattended = unattended; + } + getSessionMode() { return this.sessionMode; } @@ -291,7 +306,7 @@ export class AgentController { !conversational, // Snapshotted as the turn starts, so a Tab pressed while it runs applies // to the next turn rather than changing the rules underneath this one. - { planMode: this.isPlanMode() }, + { planMode: this.isPlanMode(), unattended: this.unattended }, ); const assistantText = response || this.pendingAssistantText; diff --git a/config/config.ts b/config/config.ts index 68297cb..e7b3b99 100644 --- a/config/config.ts +++ b/config/config.ts @@ -491,9 +491,51 @@ function isConversationTurn(message: Message | undefined): boolean { return message?.role === "user" && !message.images?.length; } +/** + * Where the message that started this turn sits in the transcript. + * + * The last real conversation turn at the moment the loop is entered: in a + * headless run that is the task statement, and in the TUI it is what the user + * just typed. Captured as an index rather than a reference because the array + * only ever grows by pushing, so the index stays true for the whole turn while + * an identity check would rest on nothing written down. + * + * Undefined for a transcript with no conversation turn in it at all, which is + * read as "nothing to pin" rather than defaulting to the first message. + */ +export function turnInitiatingIndex(messages: Message[]): number | undefined { + for (let i = messages.length - 1; i >= 0; i--) { + if (isConversationTurn(messages[i])) return i; + } + return undefined; +} + +/** + * The window sent to the provider: the last `maxTurns` conversation turns, plus + * the message that started the turn wherever that has fallen out of them. + * + * **`maxTurns` is a bound on the tail, not on the window.** A pinned request + * carries `maxTurns + 1` conversation turns, and never more — the one exception + * in a budget that is otherwise a hard boundary, and the only place in this + * file where the number of turns sent exceeds the number asked for. It is worth + * stating because every other context decision treats that ceiling as absolute: + * a reader sizing a prompt from `MAX_TURNS` alone will be one message short. + * + * The pin exists because the loop itself pushes user messages — the wind-down + * warning, the finish gates, a truncated-stream resume — and every one of them + * counts as a turn here. Six of those and the window no longer holds the + * question being answered: a benchmark trial ran 200 iterations off a single + * prompt, and the gate that asks a model to re-read its task would otherwise be + * naming something the model can no longer see. + * + * The extra message is the cheapest in the window — one prompt, no tool results + * — and it is only ever prepended when it is genuinely outside the tail, so a + * short conversation assembles exactly as it did before this existed. + */ export function recentMessages( message: Message[], maxTurns: number, + pinnedIndex?: number, ): Message[] { if (maxTurns <= 0 || message.length === 0) { return []; @@ -513,5 +555,16 @@ export function recentMessages( } } - return message.slice(startIndex); + const window = message.slice(startIndex); + + if ( + pinnedIndex === undefined || + pinnedIndex >= startIndex || + pinnedIndex < 0 || + pinnedIndex >= message.length + ) { + return window; + } + + return [message[pinnedIndex]!, ...window]; } diff --git a/config/types.ts b/config/types.ts index 65d59ea..bb513ca 100644 --- a/config/types.ts +++ b/config/types.ts @@ -123,6 +123,17 @@ export interface TurnSummary { salvagedIterations: number; /** Times the turn was asked to check its own edits before finishing. */ verificationReminders: number; + /** Times the turn was asked to re-check the task's requirements before finishing. */ + requirementReminders: number; + /** + * Whether a tool ran after the requirement gate fired. + * + * Absent when the gate never fired, which is a different thing from firing to + * no effect: the failure this gate is aimed at is a model that answers the + * question in prose, from memory, and runs nothing — and in the score alone + * that is indistinguishable from a run where the gate never mattered. + */ + requirementGateActedOn?: boolean; toolCalls: number; /** * Index of the last tool execution that changed the workspace, counting from diff --git a/docs/adr/0002-finish-gates-for-an-unattended-turn.md b/docs/adr/0002-finish-gates-for-an-unattended-turn.md new file mode 100644 index 0000000..8745ea8 --- /dev/null +++ b/docs/adr/0002-finish-gates-for-an-unattended-turn.md @@ -0,0 +1,145 @@ +--- +title: Finish gates for an unattended turn +type: concept +summary: Why a turn that stops early is asked to prove the task's requirements, why the task statement is pinned into the window, and which alternatives were rejected. +prerequisites: [] +related: + - /docs/adr/0001-wall-clock-budget-for-the-agent-loop +since: 1.1.0 +--- + +# Finish gates for an unattended turn + +Status: accepted + +A turn ends when the model responds without calling a tool. Until now one thing +could overrule that: a turn that had changed files and run nothing to check +them was asked, once, to verify. That gate fires on evidence the loop can see. +The failure it cannot see is a turn that verified something thoroughly, and +verified the wrong thing. + +## The measurement + +Two of the three failed trials in the `jobs/tb2-post-1.1` run ended that way, +early and confident, with most of both budgets unspent. + +| task | stopped at | of ceiling | wall used | what it did | +| --- | --- | --- | --- | --- | +| overfull-hbox | iteration 59 | 200 | 26% | ran its chosen check three times, declared success | +| video-processing | iteration 83 | 200 | 7% | confident, wrong | + +`overfull-hbox` is the instructive one. It ran `pdflatex` and a search for +overfull boxes three times over, so the unverified-edits gate had nothing to +say — the edits *were* checked. The task also constrained which wording was +permitted, and nothing it ran tested that. CLAUDE.md already recorded the +pattern from an earlier run: two trials that reported success with accurate +self-verification still scored zero, because they verified the wrong property. + +These are the cheapest points on the board. The agent had roughly 70% of both +budgets left and chose to stop. + +## What was decided, and the alternatives + +**A second finish gate, asked once, headless only.** When an unattended turn +responds with no tool calls and has budget left to act, the loop injects one +user message: enumerate every requirement the task states, including +constraints on what is *not* allowed, and quote the command output that proves +each one — with recollection explicitly refused as evidence. + +Interactive turns are excluded because a person is reading the answer and can +correct it for the cost of one sentence, and because a conversational turn has +no requirements to enumerate. The loop learns this from an explicit +`unattended` option rather than by inferring it from a missing optional +callback, which would have handed the behaviour to every embedder that happened +not to pass one. + +Rejected: **strengthening the system prompt.** Free, and it is where the +instruction to verify already lives — which is the argument against it. The +model is told to verify today and did, three times, against the wrong property. +A prompt is read once at the start of a turn that goes on to run for hundreds +of iterations; a loop mechanic fires at the moment the mistake is being made. + +Rejected: **firing only on turns that wrote something.** Narrower, but it +misses a task whose deliverable is a written answer, and the observed failure is +independent of whether files changed. + +**The gate needs ten steps, and no wind-down warning outstanding.** Ten is twice +the wind-down threshold. The gate asks for work, and inside that zone the loop +is telling the model the opposite — finish what you started, begin nothing new. +The flag is read as well as the count because the count is derived from a +measured rate that moves: it can recover past the floor while the model is still +under a warning issued earlier. + +**Both gates answer with one message.** They are cheap in round trips and +expensive in window: every message the loop pushes costs one of the six +conversation turns the window keeps, and losing the window is what this gate +exists to correct. + +**The task statement is pinned into the window.** The gate tells the model to go +back to the task statement above, and that has to be true. The window counts +user messages, and the loop pushes user messages of its own — the wind-down +warning, the finish gates, a truncated-stream resume — so six of them and the +question being worked on has left the request. The loop captures the +turn-initiating message when it is entered and `recentMessages` carries it back +in when the window has moved past it. + +This makes the turn ceiling a bound on the *tail* rather than on the window: a +pinned request carries one conversation turn more than `MAX_TURNS` names, and +never two. That is the only exception in a budget every other context decision +treats as absolute, so it is written down here as well as at `recentMessages` — +the extra message is one prompt, carrying no tool results, and it is prepended +only when it has genuinely fallen out of the tail. + +Rejected: **quoting the task into the gate's message instead.** Self-contained +and needs no context change, but a long turn still argues from a question it +cannot see. Rejected: **pinning `messages[0]`.** Wrong for an interactive +session, where the first message is usually a greeting, and wrong under +`--resume`, where it came off disk after trimming. + +**The duplicate threshold is cleared when the gate fires.** The two collide head +on: the check a turn most needs to re-run is usually the one it has already run +twice, where the loop answers that the result is already in the conversation — +pointing at output the window dropped long ago. An amnesty rather than an +exemption, since the gate fires once and only above the step floor. + +The reset is wholesale — every previously exhausted call may run again, not only +the one the gate is asking about — so what bounds it is that it happens once and +that suppression resumes immediately: the threshold counts again from zero, and +a third identical call after the gate is refused exactly as it would have been +before. A narrower amnesty, scoped to calls that classified as verification, +was considered and rejected: `TurnState` does not record a classification per +key, and adding one buys a distinction the step floor already pays for. + +## How it will be judged + +`TurnSummary` gains `requirementReminders` and `requirementGateActedOn`, both +written to `run_end` in the events JSONL. They exist to separate three outcomes +that a score alone collapses into one: the gate never fired, the gate fired and +the model went and ran commands, and the gate fired and the model answered in +prose from memory. The last is this mechanism's likeliest failure, and without +the flag it is invisible. + +`requirementGateActedOn` compares tool executions against a snapshot taken as +the gate fires. That reads as "afterwards" rather than "at some point" because +the count never decreases and the gate fires from a response that called no +tool, so nothing can move it in between. + +Every mechanism here was proved by reverting it and watching its test go red, +because a regression test that has never failed proves nothing. Each revert was +confirmed applied before the suite ran: + +| reverted | what went red | +| --- | --- | +| the clock guard on the verification gate | `WallBudgetExhaustedError` in place of the finished answer | +| the pin | the task absent from every request after the sixth injection | +| the requirement gate's `unattended` condition | seven tests, while the three negative ones stayed green | +| the duplicate amnesty | two executions of the repeated check instead of three | +| the composed status line | the merged notice naming only the verification gate | +| the trial metadata keys | `KeyError` in the harness tests | + +The replay harness cannot speak to any of this. Its recordings hold one +conversation turn each — the loop's injected messages were never written to the +event log — so the pin never fires there and the baseline is unchanged by +construction. What settles it is the benchmark: `overfull-hbox` and +`video-processing` first, then the five-task job to check the three passing +tasks did not regress. diff --git a/harbor_woopcode/agent.py b/harbor_woopcode/agent.py index 9d19d64..9e613ff 100644 --- a/harbor_woopcode/agent.py +++ b/harbor_woopcode/agent.py @@ -894,4 +894,13 @@ def populate_context_post_run(self, context: AgentContext) -> None: # anything afterwards. Absent on a run from a CLI that predates the # summary, which is not the same as False. "woopcode_unverified_edits": summary.get("unverifiedEdits"), + # The requirement gate, in two parts, because they answer different + # questions of a job: how often a trial tried to stop early, and + # how often being asked actually sent it back to run something. + # A gate that fires and is answered in prose changes nothing, and + # in the score alone that is invisible. + "woopcode_requirement_reminders": summary.get("requirementReminders"), + "woopcode_requirement_gate_acted_on": summary.get( + "requirementGateActedOn" + ), } diff --git a/harbor_woopcode/test_agent.py b/harbor_woopcode/test_agent.py index a72c30f..2ee2430 100644 --- a/harbor_woopcode/test_agent.py +++ b/harbor_woopcode/test_agent.py @@ -115,6 +115,8 @@ def make_agent(logs_dir: Path, **kwargs) -> WoopCode: "retries": 1, "salvagedIterations": 0, "verificationReminders": 0, + "requirementReminders": 1, + "requirementGateActedOn": False, "toolCalls": 1, "lastWriteStep": 1, "toolCounts": {"create_file": 1}, @@ -536,6 +538,33 @@ def test_unverified_edits_is_read_from_the_run_summary(tmp_path: Path) -> None: assert context.metadata["woopcode_unverified_edits"] is True +def test_the_requirement_gate_is_read_from_the_run_summary(tmp_path: Path) -> None: + """Fired, and answered in prose. + + The two are separate metadata keys because they answer different questions + of a job: how often a trial tried to stop early, and how often being asked + sent it back to run something. A gate that fires and changes nothing is + invisible in the score. + """ + write_events(tmp_path, RUN_WITH_USAGE) + context = AgentContext() + make_agent(tmp_path).populate_context_post_run(context) + + assert context.metadata is not None + assert context.metadata["woopcode_requirement_reminders"] == 1 + assert context.metadata["woopcode_requirement_gate_acted_on"] is False + + +def test_the_requirement_gate_is_none_when_it_never_fired(tmp_path: Path) -> None: + """Never asked is not the same as asked and ignored.""" + write_events(tmp_path, SIMPLE_RUN) + context = AgentContext() + make_agent(tmp_path).populate_context_post_run(context) + + assert context.metadata is not None + assert context.metadata["woopcode_requirement_gate_acted_on"] is None + + def test_unverified_edits_is_none_without_a_summary(tmp_path: Path) -> None: """An older CLI wrote no summary. Unknown is not the same as False.""" write_events(tmp_path, SIMPLE_RUN) diff --git a/packages/tests/replay/cli.ts b/packages/tests/replay/cli.ts index 436ec1e..555ffc4 100644 --- a/packages/tests/replay/cli.ts +++ b/packages/tests/replay/cli.ts @@ -49,9 +49,24 @@ const baseline = (step: ReplayStep) => */ const COMPACTION_BUDGET = toolHistoryBudget() ?? SUGGESTED_TOOL_HISTORY_BUDGET; +/** + * Where the pin lands in a recording. + * + * Every fixture is one headless trial answering a single prompt, which + * `reconstruct` puts at index 0. The loop captures the *last* conversation turn + * present when it is entered, and at entry there is only this one — so the two + * agree here, and the harness must not use `turnInitiatingIndex` on a + * mid-turn transcript, where the last conversation turn is a reminder the loop + * pushed rather than the task. + */ +const PINNED_INDEX = 0; + const current = (step: ReplayStep) => measureSegments( - compactToolHistory(recentMessages(step.messages, MAX_TURNS), COMPACTION_BUDGET), + compactToolHistory( + recentMessages(step.messages, MAX_TURNS, PINNED_INDEX), + COMPACTION_BUDGET, + ), "x".repeat(step.repoContextChars), ); diff --git a/packages/tests/runtime/agentController.test.ts b/packages/tests/runtime/agentController.test.ts index 4eb3d78..fe158c3 100644 --- a/packages/tests/runtime/agentController.test.ts +++ b/packages/tests/runtime/agentController.test.ts @@ -70,6 +70,15 @@ const buildRepositoryContext = mock(async () => mockRepoContext); // any later test asserting on real persistence is silently testing this instead. const actualConfig = await import("../../../config/config"); +/** + * Captured before registration, for the reason `realSessions` below is. + * + * A namespace object's properties follow the module registry, so reading + * `actualConfig.recentMessages` after the stub is installed hands back the stub + * and the delegation below would call itself until the stack ran out. + */ +const realRecentMessages = actualConfig.recentMessages; + // The execution log is stubbed for the same reason the conversation is: the // controller persists both after every turn, and the spread above kept the real // writer — so these tests were writing the developer's own @@ -79,7 +88,13 @@ let mockExecutionRecords: unknown[] = []; mock.module("../../../config/config", () => ({ ...actualConfig, buildRepositoryContext, - recentMessages: (messages: Message[], maxTurns: number) => messages, + // Gated on `stubActive` like the session stubs, and for the same reason: a + // module mock lasts the whole run, so an ungated identity window here is + // installed while *other* files exercise the real one. It made the task-pin + // tests fail against this stub — and worse, it would have made their positive + // assertions pass vacuously, since an identity window contains everything. + recentMessages: (messages: Message[], maxTurns: number, pinnedIndex?: number) => + stubActive ? messages : realRecentMessages(messages, maxTurns, pinnedIndex), })); // Sessions are stubbed as one in-memory record. `getConversation` and diff --git a/packages/tests/runtime/eventLog.test.ts b/packages/tests/runtime/eventLog.test.ts index ca03b0a..a522a46 100644 --- a/packages/tests/runtime/eventLog.test.ts +++ b/packages/tests/runtime/eventLog.test.ts @@ -170,6 +170,8 @@ describe("event log", () => { retries: 0, salvagedIterations: 0, verificationReminders: 0, + requirementReminders: 1, + requirementGateActedOn: true, toolCalls: 1, lastWriteStep: 1, lastShellStep: undefined, @@ -181,9 +183,15 @@ describe("event log", () => { const [record] = readEvents(path); const summary = record!.summary as Record; expect(summary.unverifiedEdits).toBe(true); + // The pair a benchmark run is read through: whether the gate fired, and + // whether anything ran once it had. + expect(summary.requirementReminders).toBe(1); + expect(summary.requirementGateActedOn).toBe(true); expect(Object.keys(summary).sort()).toEqual([ "iterations", "lastWriteStep", + "requirementGateActedOn", + "requirementReminders", "retries", "salvagedIterations", "toolCalls", diff --git a/packages/tests/runtime/requirementGate.test.ts b/packages/tests/runtime/requirementGate.test.ts new file mode 100644 index 0000000..e886221 --- /dev/null +++ b/packages/tests/runtime/requirementGate.test.ts @@ -0,0 +1,206 @@ +/** + * The second finish gate: an unattended turn that stops early, with budget in + * hand, is asked once to prove it satisfied what was actually asked for. + * + * Two of three failed trials in a benchmark run ended exactly that way — one at + * iteration 59 of 200 with 74% of its wall budget unused, having verified a + * property the task never asked about. Nothing in the loop noticed, because the + * only completion gate looked for edits that had gone unchecked, and these had + * been checked. Three times. + * + * No tool is called in this file. Two other files in this directory mock the + * tool module for the whole run, so the tool-dependent cases — the merge with + * the verification reminder, the duplicate amnesty, and whether the gate was + * acted on — live in `turnSummary.test.ts` beside that mock instead. + */ +import { describe, test, expect, afterEach } from "bun:test"; +import { agentLoop } from "../../../runtime/loop"; +import { clearDeadline } from "../../../runtime/deadline"; +import { budgetDrivenBy, type DrivenBudget } from "../shared/deadline"; +import { createDoneEvent, createTextEvent } from "../shared/factories"; +import { + createRuntimeTest, + createStreamingProvider, + turnSummaryOf, +} from "../shared/testHelpers"; +import type { Message, ProviderClient, StreamEvent } from "../../../config/types"; + +const ORIGINAL_ITERATIONS = process.env.WOOPCODE_MAX_ITERATIONS; +const ORIGINAL_WALL = process.env.WOOPCODE_MAX_WALL_SEC; + +afterEach(() => { + if (ORIGINAL_ITERATIONS === undefined) delete process.env.WOOPCODE_MAX_ITERATIONS; + else process.env.WOOPCODE_MAX_ITERATIONS = ORIGINAL_ITERATIONS; + + if (ORIGINAL_WALL === undefined) delete process.env.WOOPCODE_MAX_WALL_SEC; + else process.env.WOOPCODE_MAX_WALL_SEC = ORIGINAL_WALL; + + clearDeadline(); +}); + +/** The gate's message, identified by its opening rather than by the whole text. */ +const requirementAsks = (messages: Message[]) => + messages.filter( + (m) => m.role === "user" && m.content.includes("go back to the task statement above"), + ); + +/** A model that answers in words, calling nothing — the shape the gate exists for. */ +const talkingProvider = (replies: string[]): ProviderClient => + createStreamingProvider( + replies.map((reply) => [createTextEvent(reply), createDoneEvent()]), + ); + +async function runUnattended( + provider: ProviderClient, + options: { unattended?: boolean; useTools?: boolean } = {}, +) { + const { callbacks, messages } = createRuntimeTest(); + callbacks.onError = () => {}; + const text = await agentLoop( + provider, + messages, + "", + callbacks, + undefined, + options.useTools ?? true, + { unattended: options.unattended ?? true }, + ); + return { text, messages, summary: turnSummaryOf(callbacks) }; +} + +describe("the requirement gate", () => { + test("an unattended turn that stops early is asked once", async () => { + const { text, messages, summary } = await runUnattended( + talkingProvider(["Done — the file builds cleanly.", "Confirmed, nothing left."]), + ); + + expect(requirementAsks(messages)).toHaveLength(1); + expect(summary.requirementReminders).toBe(1); + expect(text).toBe("Confirmed, nothing left."); + // Asked and ignored: the model answered in prose without running anything, + // which is this mechanism's likeliest failure and has to be visible in the + // record rather than inferred from a score. + expect(summary.requirementGateActedOn).toBe(false); + }); + + test("an attended turn is never asked", async () => { + const { messages, summary } = await runUnattended(talkingProvider(["Done."]), { + unattended: false, + }); + + // Somebody is reading the answer and can say what was missed for the cost + // of one sentence, which is cheaper than a round trip. + expect(requirementAsks(messages)).toHaveLength(0); + expect(summary.requirementReminders).toBe(0); + expect(summary.requirementGateActedOn).toBeUndefined(); + }); + + test("a turn with no tools is never asked", async () => { + const { messages, summary } = await runUnattended(talkingProvider(["Hello."]), { + useTools: false, + }); + + // The conversational path is given no tools at all, so an instruction to + // go and run a command is one the turn cannot carry out. + expect(requirementAsks(messages)).toHaveLength(0); + expect(summary.requirementReminders).toBe(0); + }); + + test("it is asked once, never twice", async () => { + const { messages, summary } = await runUnattended( + talkingProvider(["First answer.", "Second answer.", "Third answer."]), + ); + + expect(requirementAsks(messages)).toHaveLength(1); + expect(summary.requirementReminders).toBe(1); + expect(summary.iterations).toBe(2); + }); + + test("a turn without the budget to act on it is not asked", async () => { + // Nine steps: one spent answering, eight left, and the gate needs ten. It + // asks for work — enumerate the requirements, run a command per unproven + // one — so asking without room to do it spends a round trip on nothing. + process.env.WOOPCODE_MAX_ITERATIONS = "9"; + + const { messages, summary } = await runUnattended(talkingProvider(["Done."])); + + expect(requirementAsks(messages)).toHaveLength(0); + expect(summary.requirementReminders).toBe(0); + }); + + test("one step over the floor, it is asked", async () => { + // The boundary from the other side, so the test above is pinned to the + // floor rather than to any budget being small. + process.env.WOOPCODE_MAX_ITERATIONS = "11"; + + const { messages } = await runUnattended(talkingProvider(["Done.", "Confirmed."])); + + expect(requirementAsks(messages)).toHaveLength(1); + }); +}); + +/** + * A turn that was warned it is winding down, and then was not. + * + * The wind-down count is derived from a rate measured on the turn itself, and a + * rate moves: a slow patch early trips the warning, and the estimate recovers + * once ordinary steps land beside it. `shouldWarnWindDown` re-arms only above + * twice the threshold, so between ten and eleven steps the flag is still + * latched while the count reads healthy — and the model is still under + * "finish what you started, begin nothing new" from an earlier request. + * + * The gate must not contradict that, so it reads the flag as well as the count. + */ +describe("the requirement gate against the wind-down warning", () => { + /** + * Three slow steps, then free ones. + * + * Trips the warning at step four (40s left at 10s a step reads as four steps), + * then holds the clock still so the mean falls as the iteration count climbs + * and the estimate climbs back through the latch's re-arm point. + */ + function pacedProvider(clock: DrivenBudget, finishAt: number): ProviderClient { + let n = 0; + return { + async *stream(): AsyncGenerator { + n++; + if (n <= 3) clock.advance(10_000); + + yield createTextEvent(`step ${n}`); + if (n >= finishAt) { + yield createDoneEvent(); + return; + } + // Salvaged and resumed, so the turn continues without a tool. + throw new Error("socket hang up"); + }, + } as ProviderClient; + } + + test("a turn still under the wind-down warning is not asked", async () => { + const clock = budgetDrivenBy(70, 70_000); + + // Finishing at step 8: ten steps' worth of clock left, which clears the + // gate's floor, while the flag set at step four has not yet re-armed. + const { messages, summary } = await runUnattended(pacedProvider(clock, 8)); + + expect(summary.iterations).toBe(8); + expect(requirementAsks(messages)).toHaveLength(0); + expect(summary.requirementReminders).toBe(0); + }); + + test("once the estimate recovers and the warning clears, it is asked", async () => { + const clock = budgetDrivenBy(70, 70_000); + + // The same clock, the same rate, two steps later — by which point the + // estimate has passed the re-arm point and the flag is down. The only + // difference between this and the test above is the latch. + const { messages, summary } = await runUnattended(pacedProvider(clock, 10)); + + // Eleven, not ten: the eleventh iteration is the round trip the gate bought, + // which is the whole point of it and the difference from the test above. + expect(summary.iterations).toBe(11); + expect(requirementAsks(messages)).toHaveLength(1); + expect(summary.requirementReminders).toBe(1); + }); +}); diff --git a/packages/tests/runtime/taskPin.test.ts b/packages/tests/runtime/taskPin.test.ts new file mode 100644 index 0000000..0eb567c --- /dev/null +++ b/packages/tests/runtime/taskPin.test.ts @@ -0,0 +1,161 @@ +/** + * The task statement stays in the window for as long as the turn runs. + * + * `recentMessages` counts user messages, and the loop pushes user messages of + * its own — a wind-down warning, a finish gate, a truncated-stream resume. Six + * of those and the question being answered has left the request, while the + * model is still working on it. A benchmark trial ran 200 iterations off one + * prompt, so this is the ordinary case for a headless run rather than an edge. + * + * No tool is called anywhere in this file: two other files in this directory + * mock the tool module for the whole run, and a file that reaches the registry + * is a file whose result depends on which one ran first. + */ +import { describe, test, expect, afterEach } from "bun:test"; +import { agentLoop } from "../../../runtime/loop"; +import { recentMessages, turnInitiatingIndex } from "../../../config/config"; +import { createRuntimeTest } from "../shared/testHelpers"; +import type { Message, ProviderClient, StreamEvent } from "../../../config/types"; + +const ORIGINAL_ITERATIONS = process.env.WOOPCODE_MAX_ITERATIONS; + +afterEach(() => { + if (ORIGINAL_ITERATIONS === undefined) delete process.env.WOOPCODE_MAX_ITERATIONS; + else process.env.WOOPCODE_MAX_ITERATIONS = ORIGINAL_ITERATIONS; +}); + +const user = (content: string): Message => ({ role: "user", content }); +const assistant = (content: string): Message => ({ role: "assistant", content }); +const attachment = (path: string): Message => ({ + role: "user", + content: "The image requested above:", + images: [{ path, mediaType: "image/png" }], +}); + +describe("finding the message that started the turn", () => { + test("is the last conversation turn present", () => { + const conversation = [user("one"), assistant("a"), user("two")]; + expect(turnInitiatingIndex(conversation)).toBe(2); + }); + + test("an attached image is not a turn", () => { + // The loop follows read_image with a user message carrying the picture. + // Nobody typed it, so pinning it would pin the loop's own plumbing. + const conversation = [user("describe these"), attachment("/a.png")]; + expect(turnInitiatingIndex(conversation)).toBe(0); + }); + + test("a transcript with no conversation turn pins nothing", () => { + expect(turnInitiatingIndex([assistant("unprompted")])).toBeUndefined(); + expect(turnInitiatingIndex([])).toBeUndefined(); + }); +}); + +describe("pinning it into the window", () => { + /** A task, then enough loop-pushed turns to push it out of a window of three. */ + const transcript: Message[] = [ + user("TASK: make the tests pass"), + assistant("working"), + user("Your previous message was cut off before it finished."), + assistant("still working"), + user("Only 5 more steps are available before this turn is stopped."), + assistant("nearly there"), + user("You changed files and have not run anything since."), + assistant("done"), + ]; + + test("the task is carried back in once it falls out", () => { + const windowed = recentMessages(transcript, 3, 0); + + expect(windowed[0]).toBe(transcript[0]!); + expect(windowed.filter((m) => m === transcript[0]!)).toHaveLength(1); + }); + + test("without the pin the same window loses it", () => { + // The defect itself, stated as a test: this is what every request after the + // sixth injected message used to look like. + expect(recentMessages(transcript, 3)).not.toContain(transcript[0]!); + }); + + /** + * The one exception to the turn ceiling, stated as arithmetic. + * + * Everything else that budgets context treats `MAX_TURNS` as a hard boundary, + * so the pin's cost is written down here rather than left to be discovered by + * someone sizing a prompt from the constant alone: a pinned request carries + * one more conversation turn than was asked for, and never two. + */ + test("a pinned window carries exactly one turn more than the ceiling", () => { + const turns = (messages: Message[]) => + messages.filter((m) => m.role === "user" && !m.images?.length).length; + + for (const maxTurns of [1, 2, 3]) { + expect(turns(recentMessages(transcript, maxTurns, 0))).toBe(maxTurns + 1); + } + + // And the ceiling is intact without a pin, which is what makes the line + // above an exception rather than an off-by-one. + for (const maxTurns of [1, 2, 3]) { + expect(turns(recentMessages(transcript, maxTurns))).toBe(maxTurns); + } + }); + + test("a window that already holds the task is untouched", () => { + // Byte-identical to the unpinned assembly, so a short conversation — every + // interactive turn, and the first several steps of a headless one — is + // assembled exactly as it was before the pin existed. + expect(recentMessages(transcript, 6, 0)).toEqual(recentMessages(transcript, 6)); + }); + + test("an out-of-range pin is ignored rather than trusted", () => { + expect(recentMessages(transcript, 3, -1)).toEqual(recentMessages(transcript, 3)); + expect(recentMessages(transcript, 3, 99)).toEqual(recentMessages(transcript, 3)); + }); +}); + +/** + * A provider that never volunteers to stop, and never finishes a response. + * + * Each iteration is salvaged and resumed, which pushes an assistant message and + * a user message — so the transcript gains one conversation turn per step + * without a tool ever running. That is the cheapest way to reproduce a long + * turn's pressure on the window. + */ +function truncatingProvider(seen: Message[][]): ProviderClient { + return { + async *stream(messages: Message[]): AsyncGenerator { + seen.push(messages); + yield { type: "text", content: "still working" }; + throw new Error("socket hang up"); + }, + } as unknown as ProviderClient; +} + +describe("a long turn, end to end", () => { + test("every request still carries the task", async () => { + process.env.WOOPCODE_MAX_ITERATIONS = "9"; + + const { callbacks, messages } = createRuntimeTest(); + callbacks.onError = () => {}; + const task = "Test prompt"; + expect(messages[0]).toEqual(user(task)); + + const seen: Message[][] = []; + try { + await agentLoop(truncatingProvider(seen), messages, "", callbacks); + } catch { + // The iteration ceiling ends it; the transcript is what is under test. + } + + // Nine requests, each one a turn further from the prompt. + expect(seen).toHaveLength(9); + for (const request of seen) { + expect(request.some((m) => m.role === "user" && m.content === task)).toBe(true); + } + + // And the last one would have lost it: eight resumes is more than the six + // turns the window keeps. + const unpinned = recentMessages(messages, 6); + expect(unpinned.some((m) => m.role === "user" && m.content === task)).toBe(false); + }); +}); diff --git a/packages/tests/runtime/turnSummary.test.ts b/packages/tests/runtime/turnSummary.test.ts index dd1bfb1..eff3bd3 100644 --- a/packages/tests/runtime/turnSummary.test.ts +++ b/packages/tests/runtime/turnSummary.test.ts @@ -1,34 +1,56 @@ -import { describe, test, expect, mock } from "bun:test"; +/** + * Everything a turn records about its own work, and the gates that read it: + * effect classification, the summary, the verification reminder, and the two + * finish gates meeting on one response. + * + * Four concerns in one file, deliberately. `mock.module` lasts the whole run and + * the last registration of a module wins for every file, so the tool registry is + * mocked in as few places as possible — splitting these out would mean a second + * file stubbing `../../../tools`, and whichever registered last would hand its + * registry to the other. The cases that need no tool live in + * `requirementGate.test.ts` and `taskPin.test.ts` for the same reason. + */ +import { describe, test, expect, mock, afterEach } from "bun:test"; import { agentLoop } from "../../../runtime/loop"; +import { clearDeadline } from "../../../runtime/deadline"; +import { budgetDrivenBy } from "../shared/deadline"; import { toolEffect } from "../../../runtime/toolEffects"; import { toolRegistry } from "../../../tools"; import { MockTool, MockToolRegistry } from "../shared/mocks"; -import { createRuntimeTest, createStreamingProvider } from "../shared/testHelpers"; +import { + createRuntimeTest, + createStreamingProvider, + turnSummaryOf as summaryOf, +} from "../shared/testHelpers"; import { createDoneEvent, createTextEvent, createToolCallEvent, } from "../shared/factories"; -import type { TurnSummary } from "../../../config/types"; +import type { Message } from "../../../config/types"; const mockToolRegistry = new MockToolRegistry(); const getTool = mock((name: string) => mockToolRegistry.get(name)); const actualTools = await import("../../../tools"); mock.module("../../../tools", () => ({ ...actualTools, getTool })); -function summaryOf(callbacks: { - getCallsByName(name: string): Array<{ args: any[] }>; -}): TurnSummary { - const calls = callbacks.getCallsByName("onTurnSummary"); - expect(calls.length).toBe(1); - return calls[0]!.args[0] as TurnSummary; -} - /** Registers a tool that succeeds, replacing any previous one of that name. */ function registerTool(name: string, output = "ok") { mockToolRegistry.register(new MockTool(name, output)); } +const ORIGINAL_WALL = process.env.WOOPCODE_MAX_WALL_SEC; + +afterEach(() => { + if (ORIGINAL_WALL === undefined) delete process.env.WOOPCODE_MAX_WALL_SEC; + else process.env.WOOPCODE_MAX_WALL_SEC = ORIGINAL_WALL; + + // Module state outlives a test. The clock goes back with the deadline: a fake + // one left installed would freeze elapsed time for every file that runs after + // this one. + clearDeadline(); +}); + describe("tool effect classification", () => { test("classifies every registered tool", () => { const unclassified = toolRegistry @@ -412,4 +434,217 @@ describe("agentLoop - asking the turn to verify its edits", () => { expect(result).toBe("No tests exist for this file."); expect(summaryOf(callbacks).unverifiedEdits).toBe(true); }); + + /** + * The reminder costs a round trip, and a round trip has to be affordable. + * + * Iterations are not the binding budget here — 38 of 40 are left — but the + * request that produced the answer spent the last of the clock. Injecting + * anyway sends the loop round to a deadline check that throws, and a turn + * holding a finished answer exits as `WallBudgetExhaustedError` with status + * 2. The guard this covers reads both budgets; the one it replaced read only + * the iteration count, so the case was reachable on every wall-budgeted run. + */ + test("the reminder is withheld when the clock has nothing left", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("edit_file", "Edit applied"); + + // An hour of iterations and a second and a half of clock. + const clock = budgetDrivenBy(3600, 1_500); + + let n = 0; + const provider = { + async *stream() { + // Each request costs a second of the 1.5 remaining, so the second one + // ends past the deadline — the shape of a turn whose final answer + // arrived on the last of its time. + clock.advance(1_000); + if (n++ === 0) { + yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); + yield createDoneEvent(); + return; + } + yield createTextEvent("Fixed."); + yield createDoneEvent(); + }, + } as any; + + const result = await agentLoop(provider, messages, "", callbacks); + + expect(result).toBe("Fixed."); + expect(summaryOf(callbacks).verificationReminders).toBe(0); + // Still recorded as unverified: the turn is not being told this was fine, + // only that there was no time left to ask about it. + expect(summaryOf(callbacks).unverifiedEdits).toBe(true); + }); +}); + +describe("both finish gates on one response", () => { + /** The two gates' messages, by their openings. */ + const asks = (messages: Message[], opening: string) => + messages.filter((m) => m.role === "user" && m.content.includes(opening)); + + const VERIFY_OPENING = "have not run anything"; + const REQUIREMENT_OPENING = "go back to the task statement above"; + + /** Edit something, then declare victory without running anything. */ + const editThenClaim = () => + createStreamingProvider([ + [createToolCallEvent("edit_file", { path: "a.ts" }, "c1"), createDoneEvent()], + [createTextEvent("All done."), createDoneEvent()], + ]); + + test("an unattended turn that edited blindly gets one message, not two", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("edit_file", "Edit applied"); + + await agentLoop(editThenClaim(), messages, "", callbacks, undefined, true, { + unattended: true, + }); + + // One user message carrying both asks. A second injection would cost + // another of the six turns the window keeps, which is the scarce resource + // on the long turns this gate fires in. + const injected = messages.filter( + (m): m is Extract => + m.role === "user" && + (m.content.includes(VERIFY_OPENING) || m.content.includes(REQUIREMENT_OPENING)), + ); + expect(injected).toHaveLength(1); + expect(injected[0]!.content).toContain(VERIFY_OPENING); + expect(injected[0]!.content).toContain(REQUIREMENT_OPENING); + + const summary = summaryOf(callbacks); + expect(summary.verificationReminders).toBe(1); + expect(summary.requirementReminders).toBe(1); + + // The live channel names both. One message reaches the model, but this is + // what a headless operator watches on stderr and what lands in the event + // log — a turn where both gates fired must not read as one where only the + // verification gate did. + const statuses = callbacks + .getCallsByName("onStatus") + .map((call) => String(call.args[0])); + const notice = statuses.find((status) => status.includes("asking the agent")); + expect(notice).toContain("files changed without a check"); + expect(notice).toContain("finishing early with budget left"); + }); + + test("an attended turn that edited blindly still gets only the verify ask", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("edit_file", "Edit applied"); + + await agentLoop(editThenClaim(), messages, "", callbacks); + + expect(asks(messages, VERIFY_OPENING)).toHaveLength(1); + expect(asks(messages, REQUIREMENT_OPENING)).toHaveLength(0); + }); + + test("a tool run after the gate is recorded as acting on it", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("run_tests", "3 pass 0 fail"); + + const provider = createStreamingProvider([ + [createTextEvent("Looks right to me."), createDoneEvent()], + // The gate landed and the model went and checked. + [createToolCallEvent("run_tests", { command: "bun test" }, "c1"), createDoneEvent()], + [createTextEvent("Verified against the stated requirements."), createDoneEvent()], + ]); + + await agentLoop(provider, messages, "", callbacks, undefined, true, { + unattended: true, + }); + + const summary = summaryOf(callbacks); + expect(summary.requirementReminders).toBe(1); + expect(summary.requirementGateActedOn).toBe(true); + }); + + /** + * The gate demands output the duplicate threshold would refuse. + * + * `overfull-hbox` ran its chosen check three times and still scored zero. Told + * to prove a requirement it cannot prove, the model's next move is very often + * that same command — which `executeToolCall` answers with "the result for + * these exact arguments is already in the conversation", pointing at output + * the window dropped long ago. So the gate clears the ledger as it fires. + */ + test("a repeat of an already-exhausted command runs again after the gate", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("run_terminal", "no overfull boxes found"); + + const check = { command: "pdflatex doc.tex | grep -i overfull" }; + const provider = createStreamingProvider([ + // Twice, which exhausts the threshold, then an answer. + [createToolCallEvent("run_terminal", check, "c1"), createDoneEvent()], + [createToolCallEvent("run_terminal", check, "c2"), createDoneEvent()], + [createTextEvent("No overfull boxes. Done."), createDoneEvent()], + // After the gate: the same command again, which without the amnesty is + // skipped as a duplicate and executes nothing. + [createToolCallEvent("run_terminal", check, "c4"), createDoneEvent()], + [createTextEvent("Re-checked, with output."), createDoneEvent()], + ]); + + await agentLoop(provider, messages, "", callbacks, undefined, true, { + unattended: true, + }); + + const summary = summaryOf(callbacks); + expect(summary.requirementReminders).toBe(1); + // Three executions, not two: the post-gate repeat actually ran. + expect(summary.toolCounts.run_terminal).toBe(3); + expect(summary.requirementGateActedOn).toBe(true); + expect( + messages.some( + (m) => m.role === "tool" && m.content.includes("Skipped duplicate"), + ), + ).toBe(false); + }); + + /** + * The amnesty's cost, bounded. + * + * Clearing the ledger lets *every* previously exhausted call run again, not + * only the one the gate is asking about — so the guard against a turn that + * spends its tail replaying expensive commands is that the reset is one-shot. + * The gate fires once, and the threshold starts counting again from zero the + * moment it does. This is the half the test above does not show. + */ + test("the duplicate threshold applies again immediately after the amnesty", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("run_terminal", "no overfull boxes found"); + + const check = { command: "pdflatex doc.tex | grep -i overfull" }; + const call = (id: string) => [ + createToolCallEvent("run_terminal", check, id), + createDoneEvent(), + ]; + + const provider = createStreamingProvider([ + // Two before the gate, which exhausts the threshold. + call("c1"), + call("c2"), + [createTextEvent("No overfull boxes. Done."), createDoneEvent()], + // Three after it. The amnesty buys the first two; the third is refused + // by the same rule that refused the pre-gate repeat. + call("c3"), + call("c4"), + call("c5"), + [createTextEvent("Re-checked, with output."), createDoneEvent()], + ]); + + await agentLoop(provider, messages, "", callbacks, undefined, true, { + unattended: true, + }); + + // Four executions from six attempts: two before the gate, two after, and + // the sixth skipped. A turn cannot loop on one command any more freely + // after the gate than before it. + expect(summaryOf(callbacks).toolCounts.run_terminal).toBe(4); + expect( + messages.filter( + (m) => m.role === "tool" && m.content.includes("Skipped duplicate"), + ), + ).toHaveLength(1); + }); }); diff --git a/packages/tests/shared/deadline.ts b/packages/tests/shared/deadline.ts index 3964b5e..189edf3 100644 --- a/packages/tests/shared/deadline.ts +++ b/packages/tests/shared/deadline.ts @@ -1,4 +1,4 @@ -import { WALL_RESERVE_SEC, setDeadline } from "../../../runtime/deadline"; +import { WALL_RESERVE_SEC, remainingMs, setDeadline } from "../../../runtime/deadline"; /** * Arms a wall-clock budget with `seconds` genuinely left to spend on tools. @@ -18,3 +18,48 @@ import { WALL_RESERVE_SEC, setDeadline } from "../../../runtime/deadline"; export function budgetWith(seconds: number): void { setDeadline(WALL_RESERVE_SEC + seconds, { now: () => 0, startedAt: 0 }); } + +/** A wall budget whose clock the test moves by hand. */ +export interface DrivenBudget { + /** Spends wall time, as a provider request or a tool would. */ + advance(ms: number): void; + /** What was written to `WOOPCODE_MAX_WALL_SEC`, for a caller that wants to assert on it. */ + wallSeconds: number; +} + +/** + * Arms a wall budget for a turn that will run through `agentLoop`, on a clock + * the test drives, positioned `msRemaining` short of the deadline. + * + * `budgetWith` above cannot serve this: it pins the deadline to `startedAt: 0`, + * and `agentLoop` re-arms from `WOOPCODE_MAX_WALL_SEC` the moment the turn + * starts — computing the instant from the *process* start, which no test can + * read. So the deadline is armed once on the real clock to discover where it + * lands, and the fake is then positioned relative to that. Both calls name the + * same budget, so the loop's re-arm lands on the same instant and only the + * clock reading it changes. + * + * The environment variable is set here because the loop reads it rather than + * taking an argument; restoring it belongs to the caller's `afterEach`, beside + * the `clearDeadline` that puts the real clock back. + */ +export function budgetDrivenBy( + usableSeconds: number, + msRemaining: number, +): DrivenBudget { + const wallSeconds = WALL_RESERVE_SEC + usableSeconds; + + setDeadline(wallSeconds); + const deadlineAt = Date.now() + remainingMs()!; + + let now = deadlineAt - msRemaining; + setDeadline(wallSeconds, { now: () => now }); + process.env.WOOPCODE_MAX_WALL_SEC = String(wallSeconds); + + return { + advance(ms: number) { + now += ms; + }, + wallSeconds, + }; +} diff --git a/packages/tests/shared/testHelpers.ts b/packages/tests/shared/testHelpers.ts index 871a075..456f66b 100644 --- a/packages/tests/shared/testHelpers.ts +++ b/packages/tests/shared/testHelpers.ts @@ -1,4 +1,4 @@ -import type { Message, StreamEvent } from "../../../config/types"; +import type { Message, StreamEvent, TurnSummary } from "../../../config/types"; import { MockProviderClient, MockToolRegistry, CallbackSpy } from "./mocks"; import { createUserMessage } from "./factories"; @@ -37,6 +37,23 @@ export function createStreamingProvider(iterations: StreamEvent[][]): any { return builder.build(); } +/** + * The single turn summary a completed loop reported. + * + * Asserts there was exactly one before returning it: a turn emits its summary + * once however it ended, so two means something ran the loop twice and every + * assertion after this point would be reading the wrong turn. + */ +export function turnSummaryOf(callbacks: { + getCallsByName(name: string): Array<{ args: any[] }>; +}): TurnSummary { + const calls = callbacks.getCallsByName("onTurnSummary"); + if (calls.length !== 1) { + throw new Error(`expected one turn summary, got ${calls.length}`); + } + return calls[0]!.args[0] as TurnSummary; +} + /** * Test fixture creator for agent loop tests */ diff --git a/runtime/loop.ts b/runtime/loop.ts index d3a8f5a..2db8b73 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -11,7 +11,7 @@ import { setDeadline, } from "./deadline"; import { TurnState, normalizeToolKey } from "./turnState"; -import { recentMessages } from "../config/config"; +import { recentMessages, turnInitiatingIndex } from "../config/config"; import { SYSTEM_PROMPT } from "../config/systemPrompt"; import type { AgentCallbacks, @@ -188,6 +188,100 @@ const SAME_TOOL_THRESHOLD = 2; */ const MAX_VERIFICATION_REMINDERS = 1; +/** + * Steps the verification reminder needs before it is worth asking. + * + * Two, really — run the check, report what it printed — plus one for the answer + * the model was about to give. What the floor actually guards against is a + * reminder injected against a clock with nothing left: the turn continues, the + * deadline check at the top of the next iteration fires, and a turn that had a + * finished answer in hand ends as `WallBudgetExhaustedError` and exit status 2 + * instead. The old guard was `iterations < budget`, which cannot see a clock at + * all, so the case was reachable on every wall-budgeted run. + */ +const VERIFICATION_GATE_MIN_STEPS = 3; + +/** Asked once, for the same reason the verification reminder is. */ +const MAX_REQUIREMENT_REMINDERS = 1; + +/** + * Steps the requirement gate needs before it is worth asking. + * + * Twice `REMAINING_ITERATIONS_WARNING`, deliberately. This gate asks the model + * to enumerate a task's requirements and run a command for each one it cannot + * prove, which is work — and at the warning threshold the loop is telling it + * the opposite, to finish what it started and begin nothing new. Two + * instructions in one request, contradicting each other. The distance keeps + * them apart, and `windDownWarned` covers the case the distance cannot: a rate + * that dipped, warned, and recovered leaves the model told to wrap up while the + * count reads healthy again. + */ +const REQUIREMENT_GATE_MIN_STEPS = 10; + +/** + * What the turn is told when it has changed files and checked nothing. + * + * Kept as constants because the two gates can fire on the same response, in + * which case the model gets one message rather than a round trip each — window + * slots are the scarce thing here, since every message the loop pushes counts + * as one of the six turns `recentMessages` keeps. + */ +const VERIFICATION_REMINDER = + "You changed files and have not run anything since. Run the project's " + + "tests, build or type check to confirm the change works, then report the " + + "result. If it genuinely cannot be verified — no test exists, or the " + + "tooling is unavailable — say so plainly and finish. Do not claim it was " + + "verified unless a command actually ran."; + +/** + * What the turn is told when it is about to finish with budget to spare. + * + * Aimed at a specific, observed failure rather than at carelessness in general: + * a trial verified its work three times over and still scored zero, because the + * task constrained *which* wording was allowed and every check it ran tested + * only that something was present. So the message names that class outright — + * constraints on what is not allowed, and on the form of the answer — and + * refuses recollection as evidence, since the turn being interrupted is one + * whose recollection is already wrong. + * + * "Above" is load-bearing and true: the task statement is pinned into the + * window for the life of the turn. + */ +const REQUIREMENT_REMINDER = + "Before finishing: go back to the task statement above and list every " + + "requirement it states, including constraints on what is not allowed or what " + + "form the answer must take. For each one, quote the exact command and output " + + "that proves it holds. Do not answer from memory or from what you believe you " + + "did — if you cannot point at output from a command in this session, run the " + + "command now. If a requirement genuinely cannot be checked by a command, say " + + "which and why, then finish."; + +/** + * Can the turn afford another round trip, and the work it is about to ask for? + * + * `stepsRemaining` answers for both budgets at once — it is the iteration + * ceiling floored by the clock, converted at the rate this turn has been + * running at — and the deadline is consulted directly as well, because that + * conversion is deliberately not trusted until `MIN_RATE_SAMPLES` steps have + * gone into it. Without the direct check, a turn that edited and finished + * within two iterations of a nearly-spent budget would still be sent round. + */ +function canAffordAnotherRound( + state: TurnState, + iterationCeiling: number, + minSteps: number, + wallBudgeted: boolean, +): boolean { + // Guarded on `wallBudgeted` for the same reason the loop's own check is + // (`wallBudget !== null && deadlineReached()`): the deadline is module state, + // so an unbudgeted turn that inherited an armed one would have both gates + // silently withheld while the loop itself ran on, never throwing. The + // `finally` that disarms makes that unreachable today; the guard costs a + // parameter and stops the two readers of one clock disagreeing about it. + if (wallBudgeted && deadlineReached()) return false; + return state.stepsRemaining(iterationCeiling) >= minSteps; +} + /** * Raised when the loop runs out of budget, of either kind. * @@ -296,6 +390,18 @@ export interface AgentLoopOptions { * agent is working therefore takes effect on the next turn. */ planMode?: boolean; + /** + * Nobody is reading the answer as it arrives. + * + * Set by the headless path, which is the one where a wrong answer stands: an + * interactive user reads the claim and says what was missed, and the loop can + * be corrected in the next turn for the cost of one sentence. Named for the + * property the loop cares about rather than for the interface, because + * `loop.ts` deliberately knows nothing about interfaces — and inferring it + * from a missing optional callback would hand the behaviour to every embedder + * that happened not to pass one. + */ + unattended?: boolean; } type ToolCallEvent = Extract; @@ -601,13 +707,30 @@ async function executeToolCall( /** Whether the turn is over, or the loop should ask the model once more. */ type TurnEnding = { kind: "continue" } | { kind: "done"; text: string }; +/** What the turn may be asked before it is allowed to end. */ +interface FinishGates { + /** Nobody is reading the answer; see AgentLoopOptions.unattended. */ + unattended: boolean; + /** The turn has tools at all. A conversational turn is given none. */ + useTools: boolean; + /** This turn set a wall budget, so the deadline is its own to read. */ + wallBudgeted: boolean; +} + /** * Decides what happens when the model responds without calling any tool. * - * Usually that means it is finished. Twice it does not: a stream that died - * mid-sentence has to be resumed, and a turn that changed files without - * checking them is asked once to verify. Both push a user message and go round - * again, which is why this returns an instruction rather than a value. + * Usually that means it is finished. Three times it does not: a stream that + * died mid-sentence has to be resumed, a turn that changed files without + * checking them is asked once to verify, and an unattended turn with budget to + * spare is asked once to prove it satisfied what was actually asked for. Each + * pushes a user message and goes round again, which is why this returns an + * instruction rather than a value. + * + * The two gates are evaluated together and answered with one message, because + * they are cheap in round trips and expensive in window: a second injection + * costs one of the six turns the window keeps, and the reason the second gate + * exists at all is a turn that had lost sight of its own question. */ function finishTurn( messages: Message[], @@ -615,6 +738,7 @@ function finishTurn( state: TurnState, assistantText: string, maxIterations: number, + gates: FinishGates, truncated?: Error, ): TurnEnding { messages.push({ role: "assistant", content: assistantText }); @@ -639,24 +763,60 @@ function finishTurn( // The turn is about to end having changed files with nothing run afterwards // to check them. Ask once, then let it finish either way. - if ( + const askToVerify = state.hasUnverifiedEdits() && state.verificationReminders < MAX_VERIFICATION_REMINDERS && - state.iterations < maxIterations - ) { - state.verificationReminders++; - messages.push({ - role: "user", - content: - "You changed files and have not run anything since. Run the project's " + - "tests, build or type check to confirm the change works, then report the " + - "result. If it genuinely cannot be verified — no test exists, or the " + - "tooling is unavailable — say so plainly and finish. Do not claim it was " + - "verified unless a command actually ran.", - }); - callbacks.onStatus?.( - "⚠️ files changed without a check - asking the agent to verify", + canAffordAnotherRound( + state, + maxIterations, + VERIFICATION_GATE_MIN_STEPS, + gates.wallBudgeted, ); + + // The turn is about to end early, confidently, with most of its budget + // unspent and nobody to catch a wrong answer. `useTools` is required because + // a conversational turn is offered no tools at all, and telling it to go run + // a command would be an instruction it cannot carry out. + // + // `windDownWarned` suppresses the gate while the model is under a warning to + // start nothing new, and that suppression is temporary, not a latch for the + // turn: `shouldWarnWindDown` clears the flag once the step estimate recovers + // past twice the threshold, and the gate can fire on a later response in the + // same turn. Both directions are covered in requirementGate.test.ts. + const askForRequirements = + gates.unattended && + gates.useTools && + state.requirementReminders < MAX_REQUIREMENT_REMINDERS && + !state.windDownWarned && + canAffordAnotherRound( + state, + maxIterations, + REQUIREMENT_GATE_MIN_STEPS, + gates.wallBudgeted, + ); + + if (askToVerify || askForRequirements) { + const asks: string[] = []; + // The status names every gate that fired, not just the first. One message + // goes to the model, but this is the live channel — headless writes it to + // stderr and to the event log — and a run where both fired must not read + // as a run where only the verification gate did. + const reasons: string[] = []; + + if (askToVerify) { + state.verificationReminders++; + asks.push(VERIFICATION_REMINDER); + reasons.push("files changed without a check"); + } + + if (askForRequirements) { + state.noteRequirementGate(); + asks.push(REQUIREMENT_REMINDER); + reasons.push("finishing early with budget left"); + } + + messages.push({ role: "user", content: asks.join("\n\n") }); + callbacks.onStatus?.(`⚠️ ${reasons.join(", ")} - asking the agent to check its work`); return { kind: "continue" }; } @@ -679,6 +839,9 @@ export async function agentLoop( const BUDGET_STEP = maxIterations(); let budget = BUDGET_STEP; const planMode = options.planMode === true; + // Read once per turn, like every other switch here: the finish gates consult + // it at the end of a turn that may have started under a different caller. + const unattended = options.unattended === true; // Withholding the writing tools is the first of plan mode's two gates. The // second is the refusal below, which is what covers a write reaching the disk // through run_terminal — a tool this list has to keep. @@ -695,6 +858,12 @@ export async function agentLoop( const wallBudget = maxWallSeconds(); if (wallBudget !== null) setDeadline(wallBudget); + // Captured before the first request, because afterwards it cannot be + // recovered: every message the loop pushes is a user message too, and from + // the array alone the question being answered is indistinguishable from the + // reminders about answering it. + const taskIndex = turnInitiatingIndex(messages); + const state = new TurnState(); try { @@ -755,7 +924,7 @@ export async function agentLoop( // turned it off. When enabled it applies to the request only, because the // execution log is built from `messages` after the turn and shrinking // what is sent is not the same as forgetting what happened. - const windowed = recentMessages(messages, MAX_TURNS); + const windowed = recentMessages(messages, MAX_TURNS, taskIndex); const sentMessages = historyBudget === null ? windowed @@ -811,6 +980,7 @@ export async function agentLoop( state, assistantText, budget, + { unattended, useTools, wallBudgeted: wallBudget !== null }, truncated, ); diff --git a/runtime/turnState.ts b/runtime/turnState.ts index 1a229f7..4a3bd27 100644 --- a/runtime/turnState.ts +++ b/runtime/turnState.ts @@ -146,6 +146,29 @@ export class TurnState { */ verificationReminders = 0; + /** + * Turns asked to re-check the task's requirements before finishing. + * + * The reminder above fires on evidence the loop can see — files changed, + * nothing run. This one fires on what it cannot: a turn that verified + * something, thoroughly, that the task never asked for. Two of three failed + * trials in a benchmark run ended that way with most of both budgets unspent, + * one of them having run its chosen check three times. + */ + requirementReminders = 0; + + /** + * Tool calls executed when the requirement gate fired, or undefined if it + * never did. + * + * The comparison is sound in one direction only, which is the direction that + * matters: `toolCallsExecuted` never decreases, and the gate fires from + * `finishTurn`, which is reached only on a response that called no tool — so + * nothing can move this count between the snapshot and the injection, and any + * later increase happened after it. + */ + private toolCallsAtRequirementGate: number | undefined; + /** How many times each tool ran, by name. */ readonly toolCounts: Record = {}; @@ -231,6 +254,37 @@ export class TurnState { return elapsed / this.iterations; } + /** + * Records the requirement gate firing, and clears the way for it to be obeyed. + * + * The duplicate threshold is reset because the gate's demand collides with it + * head-on: the turn is being told to produce command output for requirements + * it cannot prove, and the check it most needs to re-run is usually the one it + * has already run twice — where `executeToolCall` answers "the result for + * these exact arguments is already in the conversation", pointing at output + * that six turns of window have long since dropped. An amnesty rather than an + * exemption, because the gate fires once and needs `REQUIREMENT_GATE_MIN_STEPS` + * of budget behind it, so the loop cannot spend a long tail repeating itself. + */ + noteRequirementGate(): void { + this.requirementReminders++; + this.toolCallsAtRequirementGate = this.toolCallsExecuted; + this.executedTools.clear(); + } + + /** + * Did anything actually run after the requirement gate fired? + * + * Undefined when it never fired. Note that a call skipped as a duplicate is + * not counted in `toolCallsExecuted` — after the amnesty above that takes + * three identical attempts, which is a model ignoring the gate rather than + * obeying it. + */ + requirementGateActedOn(): boolean | undefined { + if (this.toolCallsAtRequirementGate === undefined) return undefined; + return this.toolCallsExecuted > this.toolCallsAtRequirementGate; + } + /** * Did this turn change files and then run nothing to check them? * @@ -252,6 +306,12 @@ export class TurnState { retries: this.retries, salvagedIterations: this.salvagedIterations, verificationReminders: this.verificationReminders, + requirementReminders: this.requirementReminders, + // Omitted rather than reported as false when the gate never fired, so + // "asked and ignored" cannot be read off a run as "never asked". + ...(this.requirementGateActedOn() === undefined + ? {} + : { requirementGateActedOn: this.requirementGateActedOn() }), toolCalls: this.toolCallsExecuted, lastWriteStep: this.lastWriteStep, lastShellStep: this.lastShellStep,