From d0eba51328d7bde286b0a9ae0e5b5432f48b1686 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 17:37:44 +0530 Subject: [PATCH 1/6] fix(runtime): stop the verification reminder throwing away a finished answer The reminder guarded only `iterations < budget`, which cannot see a clock. Injected against a wall budget with nothing left, the turn goes round, the deadline check at the top of the next iteration fires, and a turn holding a finished answer exits as WallBudgetExhaustedError with status 2. Both budgets are now read through `canAffordAnotherRound`: stepsRemaining against a floor of three, plus the deadline consulted directly, because the clock-to-steps conversion is not trusted until MIN_RATE_SAMPLES steps have gone into it and a short turn would otherwise slip past. --- packages/tests/runtime/turnSummary.test.ts | 71 +++++++++++++++++++++- runtime/loop.ts | 34 ++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/packages/tests/runtime/turnSummary.test.ts b/packages/tests/runtime/turnSummary.test.ts index dd1bfb1..e647137 100644 --- a/packages/tests/runtime/turnSummary.test.ts +++ b/packages/tests/runtime/turnSummary.test.ts @@ -1,5 +1,11 @@ -import { describe, test, expect, mock } from "bun:test"; +import { describe, test, expect, mock, afterEach } from "bun:test"; import { agentLoop } from "../../../runtime/loop"; +import { + WALL_RESERVE_SEC, + clearDeadline, + remainingMs, + setDeadline, +} from "../../../runtime/deadline"; import { toolEffect } from "../../../runtime/toolEffects"; import { toolRegistry } from "../../../tools"; import { MockTool, MockToolRegistry } from "../shared/mocks"; @@ -29,6 +35,18 @@ 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 +430,55 @@ 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"); + + // Armed once on the real clock to learn where the deadline lands, then + // re-armed on a fake one positioned 1.5s short of it. `setDeadline` + // computes from the process start either way, so both calls place it at + // the same instant and only the clock reading it changes. + const wallSeconds = WALL_RESERVE_SEC + 3600; + setDeadline(wallSeconds); + const deadlineAt = Date.now() + remainingMs()!; + let fakeNow = deadlineAt - 1_500; + setDeadline(wallSeconds, { now: () => fakeNow }); + process.env.WOOPCODE_MAX_WALL_SEC = String(wallSeconds); + + 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. + fakeNow += 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); + }); }); diff --git a/runtime/loop.ts b/runtime/loop.ts index d3a8f5a..efdc5d5 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -188,6 +188,38 @@ 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; + +/** + * 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, + budget: number, + minSteps: number, +): boolean { + if (deadlineReached()) return false; + return state.stepsRemaining(budget) >= minSteps; +} + /** * Raised when the loop runs out of budget, of either kind. * @@ -642,7 +674,7 @@ function finishTurn( if ( state.hasUnverifiedEdits() && state.verificationReminders < MAX_VERIFICATION_REMINDERS && - state.iterations < maxIterations + canAffordAnotherRound(state, maxIterations, VERIFICATION_GATE_MIN_STEPS) ) { state.verificationReminders++; messages.push({ From 7661f12b03eb3b0d805d815f56db8eadc1ccd161 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 17:44:11 +0530 Subject: [PATCH 2/6] feat(runtime): keep the turn's own question in the window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit recentMessages counts user messages, and the loop pushes user messages of its own — the wind-down warning, the verification reminder, a truncated-stream resume. Six of those and the request no longer contains the task being worked on, while the turn carries on working on it. The loop captures the turn-initiating message at entry, as an index because the array only grows, and recentMessages carries it back in when the window has moved past it. A window that still holds it is assembled byte-identically. The replay corpus cannot measure this: its recordings hold one conversation turn each, because injected messages were never written to the event log, so the pin never fires there and the baseline is unchanged. Also gates agentController.test.ts's recentMessages stub on stubActive, like the session stubs beside it. Ungated, its identity window was installed for the whole run — under it these tests would have passed vacuously. --- config/config.ts | 47 +++++- packages/tests/replay/cli.ts | 17 ++- .../tests/runtime/agentController.test.ts | 17 ++- packages/tests/runtime/taskPin.test.ts | 138 ++++++++++++++++++ runtime/loop.ts | 10 +- 5 files changed, 224 insertions(+), 5 deletions(-) create mode 100644 packages/tests/runtime/taskPin.test.ts diff --git a/config/config.ts b/config/config.ts index 68297cb..8b99dce 100644 --- a/config/config.ts +++ b/config/config.ts @@ -491,9 +491,43 @@ 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. + * + * 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. + * + * Only ever prepended when it is genuinely outside the window, 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 +547,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/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/taskPin.test.ts b/packages/tests/runtime/taskPin.test.ts new file mode 100644 index 0000000..0094642 --- /dev/null +++ b/packages/tests/runtime/taskPin.test.ts @@ -0,0 +1,138 @@ +/** + * 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]!); + }); + + 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/runtime/loop.ts b/runtime/loop.ts index efdc5d5..c853cd3 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, @@ -727,6 +727,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 { @@ -787,7 +793,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 From 2aa994547d01caedf5be8294782d3c95c3d9e483 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 17:58:12 +0530 Subject: [PATCH 3/6] feat(runtime): ask an unattended turn to prove the task's requirements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of three failed trials in tb2-post-1.1 ended early, confident and wrong, with ~70% of both budgets unspent. The only completion gate looked for edits that had gone unchecked, and theirs had been checked — overfull-hbox ran its chosen check three times. It verified a property the task never asked about. So a second finish gate: an unattended turn that responds with no tool calls, with ten steps left and no wind-down warning outstanding, is asked once to enumerate every stated requirement and quote the command output proving each. Both gates answer with one message when both apply, because a second injection costs one of the six conversation turns the window keeps. The duplicate threshold is cleared as the gate fires: the check a turn most needs to re-run is usually the one it has already run twice, where the loop would answer that the result is already in a conversation the window dropped. TurnSummary gains requirementReminders and requirementGateActedOn so a run can be read for "asked and ignored" rather than only "asked". Reasoning, rejected alternatives and how this gets judged are in the ADR; CONTEXT.md is new and holds the turn-lifecycle vocabulary. --- CONTEXT.md | 76 ++++++ commands/agent.tsx | 4 + commands/agentController.ts | 17 +- config/types.ts | 11 + ...002-finish-gates-for-an-unattended-turn.md | 117 +++++++++ packages/tests/runtime/eventLog.test.ts | 8 + .../tests/runtime/requirementGate.test.ts | 234 ++++++++++++++++++ packages/tests/runtime/turnSummary.test.ts | 162 +++++++++++- runtime/loop.ts | 140 +++++++++-- runtime/turnState.ts | 60 +++++ 10 files changed, 809 insertions(+), 20 deletions(-) create mode 100644 CONTEXT.md create mode 100644 docs/adr/0002-finish-gates-for-an-unattended-turn.md create mode 100644 packages/tests/runtime/requirementGate.test.ts diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..e466b35 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,76 @@ +# 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. + +### 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..7d85122 100644 --- a/commands/agent.tsx +++ b/commands/agent.tsx @@ -302,6 +302,10 @@ async function runHeadless( }; const controller = new AgentController(provider, apiKey, selectedModel, callbacks, baseUrl); + // Nobody is reading the answer as it streams, so the loop's finish gates + // apply. Set beside the store's flag above, which says the same thing to the + // approval path. + 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/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..3a8c8db --- /dev/null +++ b/docs/adr/0002-finish-gates-for-an-unattended-turn.md @@ -0,0 +1,117 @@ +--- +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. + +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. + +## 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. + +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/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..2255b30 --- /dev/null +++ b/packages/tests/runtime/requirementGate.test.ts @@ -0,0 +1,234 @@ +/** + * 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 { + WALL_RESERVE_SEC, + clearDeadline, + remainingMs, + setDeadline, +} from "../../../runtime/deadline"; +import { createRuntimeTest } from "../shared/testHelpers"; +import type { Message, ProviderClient, StreamEvent, TurnSummary } 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(); +}); + +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; +} + +/** 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. */ +function talkingProvider(replies: string[]): ProviderClient { + let n = 0; + return { + async *stream(): AsyncGenerator { + yield { type: "text", content: replies[Math.min(n, replies.length - 1)]! }; + n++; + yield { type: "done" } as StreamEvent; + }, + } as unknown as ProviderClient; +} + +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: summaryOf(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(finishAt: number): ProviderClient { + let n = 0; + return { + async *stream(): AsyncGenerator { + n++; + if (n <= 3) advance(10_000); + + yield { type: "text", content: `step ${n}` }; + if (n >= finishAt) { + yield { type: "done" } as StreamEvent; + return; + } + // Salvaged and resumed, so the turn continues without a tool. + throw new Error("socket hang up"); + }, + } as unknown as ProviderClient; + } + + let fakeNow = 0; + const advance = (ms: number) => { + fakeNow += ms; + }; + + /** Arms a 70s budget on a clock the test drives. */ + function armClock() { + const wallSeconds = WALL_RESERVE_SEC + 70; + setDeadline(wallSeconds); + const deadlineAt = Date.now() + remainingMs()!; + fakeNow = deadlineAt - 70_000; + setDeadline(wallSeconds, { now: () => fakeNow }); + process.env.WOOPCODE_MAX_WALL_SEC = String(wallSeconds); + } + + test("a turn still under the wind-down warning is not asked", async () => { + armClock(); + + // 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(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 () => { + armClock(); + + // 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(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/turnSummary.test.ts b/packages/tests/runtime/turnSummary.test.ts index e647137..f3feb04 100644 --- a/packages/tests/runtime/turnSummary.test.ts +++ b/packages/tests/runtime/turnSummary.test.ts @@ -15,7 +15,7 @@ import { createTextEvent, createToolCallEvent, } from "../shared/factories"; -import type { TurnSummary } from "../../../config/types"; +import type { Message, TurnSummary } from "../../../config/types"; const mockToolRegistry = new MockToolRegistry(); const getTool = mock((name: string) => mockToolRegistry.get(name)); @@ -482,3 +482,163 @@ describe("agentLoop - asking the turn to verify its edits", () => { 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"; + + test("an unattended turn that edited blindly gets one message, not two", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("edit_file", "Edit applied"); + + let n = 0; + const provider = { + async *stream() { + if (n++ === 0) { + yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); + yield createDoneEvent(); + return; + } + yield createTextEvent("All done."); + yield createDoneEvent(); + }, + } as any; + + await agentLoop(provider, 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); + }); + + test("an attended turn that edited blindly still gets only the verify ask", async () => { + const { callbacks, messages } = createRuntimeTest(); + registerTool("edit_file", "Edit applied"); + + let n = 0; + const provider = { + async *stream() { + if (n++ === 0) { + yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); + yield createDoneEvent(); + return; + } + yield createTextEvent("All done."); + yield createDoneEvent(); + }, + } as any; + + await agentLoop(provider, 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"); + + let n = 0; + const provider = { + async *stream() { + n++; + if (n === 1) { + yield createTextEvent("Looks right to me."); + yield createDoneEvent(); + return; + } + if (n === 2) { + // The gate landed and the model went and checked. + yield createToolCallEvent("run_tests", { command: "bun test" }, "c1"); + yield createDoneEvent(); + return; + } + yield createTextEvent("Verified against the stated requirements."); + yield createDoneEvent(); + }, + } as any; + + 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" }; + let n = 0; + const provider = { + async *stream() { + n++; + // Twice, which exhausts the threshold, then an answer. + if (n <= 2) { + yield createToolCallEvent("run_terminal", check, `c${n}`); + yield createDoneEvent(); + return; + } + if (n === 3) { + yield createTextEvent("No overfull boxes. Done."); + yield createDoneEvent(); + return; + } + // After the gate: the same command again, which without the amnesty is + // skipped as a duplicate and executes nothing. + if (n === 4) { + yield createToolCallEvent("run_terminal", check, "c4"); + yield createDoneEvent(); + return; + } + yield createTextEvent("Re-checked, with output."); + yield createDoneEvent(); + }, + } as any; + + 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); + }); +}); diff --git a/runtime/loop.ts b/runtime/loop.ts index c853cd3..7cc39bb 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -201,6 +201,61 @@ const MAX_VERIFICATION_REMINDERS = 1; */ 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? * @@ -328,6 +383,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; @@ -633,13 +700,28 @@ 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; +} + /** * 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[], @@ -647,6 +729,7 @@ function finishTurn( state: TurnState, assistantText: string, maxIterations: number, + gates: FinishGates, truncated?: Error, ): TurnEnding { messages.push({ role: "assistant", content: assistantText }); @@ -671,23 +754,40 @@ 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 && - canAffordAnotherRound(state, maxIterations, VERIFICATION_GATE_MIN_STEPS) - ) { - 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.", - }); + canAffordAnotherRound(state, maxIterations, VERIFICATION_GATE_MIN_STEPS); + + // 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. + const askForRequirements = + gates.unattended && + gates.useTools && + state.requirementReminders < MAX_REQUIREMENT_REMINDERS && + !state.windDownWarned && + canAffordAnotherRound(state, maxIterations, REQUIREMENT_GATE_MIN_STEPS); + + if (askToVerify || askForRequirements) { + const asks: string[] = []; + + if (askToVerify) { + state.verificationReminders++; + asks.push(VERIFICATION_REMINDER); + } + + if (askForRequirements) { + state.noteRequirementGate(); + asks.push(REQUIREMENT_REMINDER); + } + + messages.push({ role: "user", content: asks.join("\n\n") }); callbacks.onStatus?.( - "⚠️ files changed without a check - asking the agent to verify", + askToVerify + ? "⚠️ files changed without a check - asking the agent to verify" + : "⚠️ finishing early with budget left - asking the agent to check the task's requirements", ); return { kind: "continue" }; } @@ -711,6 +811,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. @@ -849,6 +952,7 @@ export async function agentLoop( state, assistantText, budget, + { unattended, useTools }, 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, From e63ae27eb3339c4e7be91521726a5bd1bd8210ea Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 18:00:13 +0530 Subject: [PATCH 4/6] feat(bench): surface the requirement gate in trial metadata run_end already carried the two fields; this lifts them beside woopcode_unverified_edits so a job can be read without opening a trajectory. Kept as two keys because they answer different questions: how often a trial tried to stop early, and how often being asked sent it back to run something. A gate that fires and is answered in prose changes nothing, and in the score alone that is indistinguishable from a gate that never fired. --- harbor_woopcode/agent.py | 9 +++++++++ harbor_woopcode/test_agent.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) 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) From 6d87b7751cf421f5a221db3c27217b6adb4a2e8a Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 18:20:44 +0530 Subject: [PATCH 5/6] refactor(tests): fold the finish-gate work into the shared fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings from the two-axis pass over this branch. Standards: both new test files hand-rolled the deadline incantation the bun-test skill says to take from packages/tests/shared — so the movable-clock case joins budgetWith there as budgetDrivenBy, summaryOf moves to shared as turnSummaryOf instead of being copied per file, and the four inline stream generators become createStreamingProvider. canAffordAnotherRound's ceiling parameter is named for what it is. Spec: the merged injection reported only the verification gate on the status channel, which is what a headless operator watches and what the event log records — the notice now names every gate that fired. canAffordAnotherRound guards deadlineReached on whether this turn set a wall budget at all, mirroring the loop's own check, so two readers of one piece of module state cannot disagree. The ADR gains the revert-to-red evidence per mechanism, which was run but never written down. --- commands/agent.tsx | 11 +- ...002-finish-gates-for-an-unattended-turn.md | 13 ++ .../tests/runtime/requirementGate.test.ts | 72 +++----- packages/tests/runtime/turnSummary.test.ts | 159 +++++++----------- packages/tests/shared/deadline.ts | 47 +++++- packages/tests/shared/testHelpers.ts | 19 ++- runtime/loop.ts | 44 +++-- 7 files changed, 199 insertions(+), 166 deletions(-) diff --git a/commands/agent.tsx b/commands/agent.tsx index 7d85122..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,9 +307,9 @@ async function runHeadless( }; const controller = new AgentController(provider, apiKey, selectedModel, callbacks, baseUrl); - // Nobody is reading the answer as it streams, so the loop's finish gates - // apply. Set beside the store's flag above, which says the same thing to the - // approval path. + // 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); diff --git a/docs/adr/0002-finish-gates-for-an-unattended-turn.md b/docs/adr/0002-finish-gates-for-an-unattended-turn.md index 3a8c8db..4b19a16 100644 --- a/docs/adr/0002-finish-gates-for-an-unattended-turn.md +++ b/docs/adr/0002-finish-gates-for-an-unattended-turn.md @@ -109,6 +109,19 @@ 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 diff --git a/packages/tests/runtime/requirementGate.test.ts b/packages/tests/runtime/requirementGate.test.ts index 2255b30..e886221 100644 --- a/packages/tests/runtime/requirementGate.test.ts +++ b/packages/tests/runtime/requirementGate.test.ts @@ -15,14 +15,15 @@ */ 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 { - WALL_RESERVE_SEC, - clearDeadline, - remainingMs, - setDeadline, -} from "../../../runtime/deadline"; -import { createRuntimeTest } from "../shared/testHelpers"; -import type { Message, ProviderClient, StreamEvent, TurnSummary } from "../../../config/types"; + 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; @@ -37,14 +38,6 @@ afterEach(() => { clearDeadline(); }); -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; -} - /** The gate's message, identified by its opening rather than by the whole text. */ const requirementAsks = (messages: Message[]) => messages.filter( @@ -52,16 +45,10 @@ const requirementAsks = (messages: Message[]) => ); /** A model that answers in words, calling nothing — the shape the gate exists for. */ -function talkingProvider(replies: string[]): ProviderClient { - let n = 0; - return { - async *stream(): AsyncGenerator { - yield { type: "text", content: replies[Math.min(n, replies.length - 1)]! }; - n++; - yield { type: "done" } as StreamEvent; - }, - } as unknown as ProviderClient; -} +const talkingProvider = (replies: string[]): ProviderClient => + createStreamingProvider( + replies.map((reply) => [createTextEvent(reply), createDoneEvent()]), + ); async function runUnattended( provider: ProviderClient, @@ -78,7 +65,7 @@ async function runUnattended( options.useTools ?? true, { unattended: options.unattended ?? true }, ); - return { text, messages, summary: summaryOf(callbacks) }; + return { text, messages, summary: turnSummaryOf(callbacks) }; } describe("the requirement gate", () => { @@ -172,45 +159,30 @@ describe("the requirement gate against the wind-down warning", () => { * 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(finishAt: number): ProviderClient { + function pacedProvider(clock: DrivenBudget, finishAt: number): ProviderClient { let n = 0; return { async *stream(): AsyncGenerator { n++; - if (n <= 3) advance(10_000); + if (n <= 3) clock.advance(10_000); - yield { type: "text", content: `step ${n}` }; + yield createTextEvent(`step ${n}`); if (n >= finishAt) { - yield { type: "done" } as StreamEvent; + yield createDoneEvent(); return; } // Salvaged and resumed, so the turn continues without a tool. throw new Error("socket hang up"); }, - } as unknown as ProviderClient; - } - - let fakeNow = 0; - const advance = (ms: number) => { - fakeNow += ms; - }; - - /** Arms a 70s budget on a clock the test drives. */ - function armClock() { - const wallSeconds = WALL_RESERVE_SEC + 70; - setDeadline(wallSeconds); - const deadlineAt = Date.now() + remainingMs()!; - fakeNow = deadlineAt - 70_000; - setDeadline(wallSeconds, { now: () => fakeNow }); - process.env.WOOPCODE_MAX_WALL_SEC = String(wallSeconds); + } as ProviderClient; } test("a turn still under the wind-down warning is not asked", async () => { - armClock(); + 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(8)); + const { messages, summary } = await runUnattended(pacedProvider(clock, 8)); expect(summary.iterations).toBe(8); expect(requirementAsks(messages)).toHaveLength(0); @@ -218,12 +190,12 @@ describe("the requirement gate against the wind-down warning", () => { }); test("once the estimate recovers and the warning clears, it is asked", async () => { - armClock(); + 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(10)); + 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. diff --git a/packages/tests/runtime/turnSummary.test.ts b/packages/tests/runtime/turnSummary.test.ts index f3feb04..f6e46f7 100644 --- a/packages/tests/runtime/turnSummary.test.ts +++ b/packages/tests/runtime/turnSummary.test.ts @@ -1,35 +1,39 @@ +/** + * 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 { - WALL_RESERVE_SEC, - clearDeadline, - remainingMs, - setDeadline, -} from "../../../runtime/deadline"; +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 { Message, 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)); @@ -445,16 +449,8 @@ describe("agentLoop - asking the turn to verify its edits", () => { const { callbacks, messages } = createRuntimeTest(); registerTool("edit_file", "Edit applied"); - // Armed once on the real clock to learn where the deadline lands, then - // re-armed on a fake one positioned 1.5s short of it. `setDeadline` - // computes from the process start either way, so both calls place it at - // the same instant and only the clock reading it changes. - const wallSeconds = WALL_RESERVE_SEC + 3600; - setDeadline(wallSeconds); - const deadlineAt = Date.now() + remainingMs()!; - let fakeNow = deadlineAt - 1_500; - setDeadline(wallSeconds, { now: () => fakeNow }); - process.env.WOOPCODE_MAX_WALL_SEC = String(wallSeconds); + // An hour of iterations and a second and a half of clock. + const clock = budgetDrivenBy(3600, 1_500); let n = 0; const provider = { @@ -462,7 +458,7 @@ describe("agentLoop - asking the turn to verify its edits", () => { // 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. - fakeNow += 1_000; + clock.advance(1_000); if (n++ === 0) { yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); yield createDoneEvent(); @@ -491,24 +487,18 @@ describe("both finish gates on one response", () => { 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"); - let n = 0; - const provider = { - async *stream() { - if (n++ === 0) { - yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); - yield createDoneEvent(); - return; - } - yield createTextEvent("All done."); - yield createDoneEvent(); - }, - } as any; - - await agentLoop(provider, messages, "", callbacks, undefined, true, { + await agentLoop(editThenClaim(), messages, "", callbacks, undefined, true, { unattended: true, }); @@ -527,26 +517,24 @@ describe("both finish gates on one response", () => { 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"); - let n = 0; - const provider = { - async *stream() { - if (n++ === 0) { - yield createToolCallEvent("edit_file", { path: "a.ts" }, "c1"); - yield createDoneEvent(); - return; - } - yield createTextEvent("All done."); - yield createDoneEvent(); - }, - } as any; - - await agentLoop(provider, messages, "", callbacks); + await agentLoop(editThenClaim(), messages, "", callbacks); expect(asks(messages, VERIFY_OPENING)).toHaveLength(1); expect(asks(messages, REQUIREMENT_OPENING)).toHaveLength(0); @@ -556,25 +544,12 @@ describe("both finish gates on one response", () => { const { callbacks, messages } = createRuntimeTest(); registerTool("run_tests", "3 pass 0 fail"); - let n = 0; - const provider = { - async *stream() { - n++; - if (n === 1) { - yield createTextEvent("Looks right to me."); - yield createDoneEvent(); - return; - } - if (n === 2) { - // The gate landed and the model went and checked. - yield createToolCallEvent("run_tests", { command: "bun test" }, "c1"); - yield createDoneEvent(); - return; - } - yield createTextEvent("Verified against the stated requirements."); - yield createDoneEvent(); - }, - } as any; + 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, @@ -599,32 +574,16 @@ describe("both finish gates on one response", () => { registerTool("run_terminal", "no overfull boxes found"); const check = { command: "pdflatex doc.tex | grep -i overfull" }; - let n = 0; - const provider = { - async *stream() { - n++; - // Twice, which exhausts the threshold, then an answer. - if (n <= 2) { - yield createToolCallEvent("run_terminal", check, `c${n}`); - yield createDoneEvent(); - return; - } - if (n === 3) { - yield createTextEvent("No overfull boxes. Done."); - yield createDoneEvent(); - return; - } - // After the gate: the same command again, which without the amnesty is - // skipped as a duplicate and executes nothing. - if (n === 4) { - yield createToolCallEvent("run_terminal", check, "c4"); - yield createDoneEvent(); - return; - } - yield createTextEvent("Re-checked, with output."); - yield createDoneEvent(); - }, - } as any; + 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, 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 7cc39bb..9d009ba 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -268,11 +268,18 @@ const REQUIREMENT_REMINDER = */ function canAffordAnotherRound( state: TurnState, - budget: number, + iterationCeiling: number, minSteps: number, + wallBudgeted: boolean, ): boolean { - if (deadlineReached()) return false; - return state.stepsRemaining(budget) >= minSteps; + // 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; } /** @@ -706,6 +713,8 @@ interface FinishGates { 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; } /** @@ -757,7 +766,12 @@ function finishTurn( const askToVerify = state.hasUnverifiedEdits() && state.verificationReminders < MAX_VERIFICATION_REMINDERS && - canAffordAnotherRound(state, maxIterations, VERIFICATION_GATE_MIN_STEPS); + 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 @@ -768,27 +782,35 @@ function finishTurn( gates.useTools && state.requirementReminders < MAX_REQUIREMENT_REMINDERS && !state.windDownWarned && - canAffordAnotherRound(state, maxIterations, REQUIREMENT_GATE_MIN_STEPS); + 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?.( - askToVerify - ? "⚠️ files changed without a check - asking the agent to verify" - : "⚠️ finishing early with budget left - asking the agent to check the task's requirements", - ); + callbacks.onStatus?.(`⚠️ ${reasons.join(", ")} - asking the agent to check its work`); return { kind: "continue" }; } @@ -952,7 +974,7 @@ export async function agentLoop( state, assistantText, budget, - { unattended, useTools }, + { unattended, useTools, wallBudgeted: wallBudget !== null }, truncated, ); From fca8c597777605dd14abb6128fba6185e19f21b2 Mon Sep 17 00:00:00 2001 From: Manas Raghuwanshi Date: Sun, 30 Aug 2026 19:03:35 +0530 Subject: [PATCH 6/6] docs(runtime): state the pin's turn-ceiling exception and the amnesty's bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups. The pin makes MAX_TURNS a bound on the tail rather than on the window: a pinned request carries one conversation turn more than the constant names. That is the only exception in a budget every other context decision treats as absolute, so it is now stated at recentMessages, in the ADR and in CONTEXT.md, and pinned by a test that asserts the arithmetic in both directions. The amnesty is wholesale — every exhausted call may run again, not only the one the gate asks about — and what bounds it is that suppression resumes at once. There was a test for the intended repeat running again and none for the downside; now a third identical call after the gate is asserted to be refused, which fails if the threshold is disabled rather than reset. No change to windDownWarned, which is not latched for the turn: shouldWarnWindDown clears it above twice the threshold and requirementGate.test.ts covers the gate firing after it clears. The gate's comment now says so, since a reader of `!state.windDownWarned` could reasonably assume otherwise. --- CONTEXT.md | 4 +- config/config.ts | 12 ++++- ...002-finish-gates-for-an-unattended-turn.md | 15 ++++++ packages/tests/runtime/taskPin.test.ts | 23 +++++++++ packages/tests/runtime/turnSummary.test.ts | 47 +++++++++++++++++++ runtime/loop.ts | 6 +++ 6 files changed, 104 insertions(+), 3 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index e466b35..ae5e027 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -36,7 +36,9 @@ _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. +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 diff --git a/config/config.ts b/config/config.ts index 8b99dce..e7b3b99 100644 --- a/config/config.ts +++ b/config/config.ts @@ -514,6 +514,13 @@ export function turnInitiatingIndex(messages: Message[]): number | 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 @@ -521,8 +528,9 @@ export function turnInitiatingIndex(messages: Message[]): number | undefined { * prompt, and the gate that asks a model to re-read its task would otherwise be * naming something the model can no longer see. * - * Only ever prepended when it is genuinely outside the window, so a short - * conversation assembles exactly as it did before this existed. + * 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[], diff --git a/docs/adr/0002-finish-gates-for-an-unattended-turn.md b/docs/adr/0002-finish-gates-for-an-unattended-turn.md index 4b19a16..8745ea8 100644 --- a/docs/adr/0002-finish-gates-for-an-unattended-turn.md +++ b/docs/adr/0002-finish-gates-for-an-unattended-turn.md @@ -83,6 +83,13 @@ 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 @@ -95,6 +102,14 @@ 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 diff --git a/packages/tests/runtime/taskPin.test.ts b/packages/tests/runtime/taskPin.test.ts index 0094642..0eb567c 100644 --- a/packages/tests/runtime/taskPin.test.ts +++ b/packages/tests/runtime/taskPin.test.ts @@ -77,6 +77,29 @@ describe("pinning it into the window", () => { 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 diff --git a/packages/tests/runtime/turnSummary.test.ts b/packages/tests/runtime/turnSummary.test.ts index f6e46f7..eff3bd3 100644 --- a/packages/tests/runtime/turnSummary.test.ts +++ b/packages/tests/runtime/turnSummary.test.ts @@ -600,4 +600,51 @@ describe("both finish gates on one response", () => { ), ).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/runtime/loop.ts b/runtime/loop.ts index 9d009ba..2db8b73 100644 --- a/runtime/loop.ts +++ b/runtime/loop.ts @@ -777,6 +777,12 @@ function finishTurn( // 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 &&