From b994f86d4dab42b2082c3716f5893585b814a893 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 14 Sep 2026 14:03:45 -0700 Subject: [PATCH] fix(chat): pin continuation polish leftovers from emit-and-deliver Summary: update the architecture doc to the emit-and-deliver continuation mechanism; collapse idle-turn arming into a single-delivery contract (closure fires and the flag stays false, or no closure and the flag reports the arming) with a guard test; extract the session deliver generation re-check into runGenerationGuardedDeliver with race-pinning tests. Emit shape, guards, and gates unchanged. Verification: bun test src/tui/deliver-agent-message.test.ts src/agent/compaction.test.ts (38 pass); bun run check (7396 pass, 0 fail). --- docs/ARCHITECTURE.md | 2 +- src/agent/compaction.test.ts | 20 ++++++++++ src/agent/compaction.ts | 16 ++++++-- src/tui/deliver-agent-message.test.ts | 53 +++++++++++++++++++++++++++ src/tui/deliver-agent-message.ts | 22 +++++++++++ src/tui/runner/session.ts | 27 ++++++++------ 6 files changed, 125 insertions(+), 15 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a1d007d18..3e4fdcb7e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -177,7 +177,7 @@ When a cycle's input tokens cross a threshold, the director compacts the inferen - **Idle (end-of-turn)** — An interactive turn can end with a reply and then sit idle with no tool batch to intercept; the governor requests a continuation at that pause and compacts when it arrives. An operator message that races the continuation still compacts first, then re-enters inference to answer it. - **Overflow recovery** — A `context_overflow` inference error would otherwise become a terminal error reply; the governor compacts and retries instead, bounded so a history the compactor cannot shrink does not loop forever. Overflow ignores hysteresis for the compact itself. -The compaction control flow is shaped by a reactor invariant: a `compact` action runs in its own cycle (it cannot be paired with `infer`), and **the reactor delivers no event after a compact cycle**. A director that simply emitted `compact` in place of the follow-up `infer` would leave the loop idle forever — the cause of an earlier stall. Instead the governor, after emitting `compact`, self-delivers a content-less inbound message (a host-supplied `requestContinuation` callback). That message adds no turn (`createInboundTurn` returns `null` for empty content) but re-enters the loop, where the director issues the follow-up `infer` against the freshly truncated history. +The compaction control flow is shaped by a reactor invariant: a `compact` action runs in its own cycle (it cannot be paired with `infer`), and **the reactor delivers no event after a compact cycle**. A director that simply emitted `compact` in place of the follow-up `infer` would leave the loop idle forever — the cause of an earlier stall. Instead the governor pairs `compact` with an emit action (`custom.compaction.continue`), and the host answers that emission by delivering a content-less inbound message (`buildCompactionContinuationMessage()`) through the serial operation queue, generation-guarded like every other deliver. That message adds no turn (`createInboundTurn` returns `null` for empty content) but re-enters the loop, where the director issues the follow-up `infer` against the freshly truncated history. Each emission is answered at most once (a replayed duplicate of an already-answered emission is ignored), and an unsolicited continuation with neither resume flag set answers `wait` rather than burning a billable inference. The legacy `requestContinuation` closure survives only on the sub-agent path, which delivers directly with no host emit hop; idle arming keeps the two channels exclusive (closure fires and the arming flag stays false, or no closure and the flag reports the arming) so a caller honoring both cannot double-deliver. Compaction replaces older turns with a structured, workflow-aware summary rather than a stats blob: sections for **What Happened / What We're Doing / Relevant Links / Action Items / Next Steps**, with the active workflow and step woven in so compacting mid-`/build` or mid-`/plan` preserves the contract. The summary is produced by a one-shot model call; on any failure it falls back to a deterministic summary so a compaction cycle never breaks the session. diff --git a/src/agent/compaction.test.ts b/src/agent/compaction.test.ts index a0f8c96f0..44e9a8808 100644 --- a/src/agent/compaction.test.ts +++ b/src/agent/compaction.test.ts @@ -303,6 +303,26 @@ describe("compaction governor", () => { ).toBe(true); }); + test("idle arming with a closure fires once and never returns true", () => { + // Single-delivery contract: a caller that both installs the legacy closure + // and honors the boolean (appending an emit action on true) must still + // deliver exactly once. The closure fires; the return stays false. + let continuations = 0; + const governor = createCompactionGovernor(() => continuations++); + governor.noteInferenceDone(inferenceDone(overThreshold), tenTurns); + + const terminal: ReactorAction[] = [{ type: "reply", content: "done" }]; + expect(governor.noteIdleTurn(inferenceDone(overThreshold), terminal)).toBe( + false, + ); + expect(continuations).toBe(1); + // A repeated idle turn neither refires nor arms. + expect(governor.noteIdleTurn(inferenceDone(overThreshold), terminal)).toBe( + false, + ); + expect(continuations).toBe(1); + }); + // Idle compact with an empty continuation previously left postCompactInfer // unset, so resumeAfterCompact never fired and notePostCompact never ran — // the Ctx meter stayed on pre-compact lastTurnUsage until the next user turn. diff --git a/src/agent/compaction.ts b/src/agent/compaction.ts index e7092550c..9f3fab454 100644 --- a/src/agent/compaction.ts +++ b/src/agent/compaction.ts @@ -227,8 +227,15 @@ export function createCompactionGovernor( // Interactive sessions can end a turn with a reply and then sit idle, so a // pending compaction would wait indefinitely for the next tool batch. When // the turn ends without follow-up work, request a continuation re-entry and - // compact when it (or the operator's next message) arrives. Returns whether - // this call newly armed the idle continuation. + // compact when it (or the operator's next message) arrives. + // + // Single-delivery contract: the closure channel and the boolean return are + // mutually exclusive, matching continuationActions below. When a legacy + // closure is installed (the sub-agent path) it fires here and this returns + // false, so a caller that also honors the return cannot double-deliver. + // When no closure is installed (the chat path) nothing fires and the return + // reports whether this call newly armed the idle continuation — the caller + // must then append compactionContinuationAction to its returned actions. function noteIdleTurn( event: ReactorInboundEvent, actions: ReactorAction[], @@ -242,7 +249,10 @@ export function createCompactionGovernor( !actions.some((a) => a.type === "infer" || a.type === "execute_tools"); if (!terminal) return false; idlePending = true; - requestContinuation?.(); + if (requestContinuation !== undefined) { + requestContinuation(); + return false; + } return true; } diff --git a/src/tui/deliver-agent-message.test.ts b/src/tui/deliver-agent-message.test.ts index aed319015..904182c64 100644 --- a/src/tui/deliver-agent-message.test.ts +++ b/src/tui/deliver-agent-message.test.ts @@ -3,6 +3,7 @@ import { AgentClosedError } from "@intx/agent"; import { deliverAgentMessage, deliveryResultNotice, + runGenerationGuardedDeliver, } from "./deliver-agent-message.js"; describe("deliverAgentMessage", () => { @@ -72,6 +73,58 @@ describe("deliverAgentMessage", () => { }); }); +describe("runGenerationGuardedDeliver", () => { + test("a reload between enqueue and execution drops the deliver without touching the agent", async () => { + // Pins the reload-vs-async-deliver verdict: the serial op queue is FIFO + // with no preemption, so a continuation answer queued ahead of a reload + // still executes — the generation re-check at execution time is what + // keeps the stale answer from reaching the replaced agent. + let generation = 1; + const stillCurrent = () => generation === 1; + // Enqueue captures the closure; the reload lands before it executes. + const queued = () => + runGenerationGuardedDeliver({ + stillCurrent, + onStale: () => ({ + status: "not-delivered", + reason: "superseded", + detail: "session identity changed before delivery", + }), + run: async () => { + delivered += 1; + return { status: "accepted" as const }; + }, + }); + let delivered = 0; + generation = 2; + const stale = await queued(); + expect(stale).toEqual({ + status: "not-delivered", + reason: "superseded", + detail: "session identity changed before delivery", + }); + expect(delivered).toBe(0); + }); + + test("a current deliver runs the real settle exactly once", async () => { + let runs = 0; + const result = await runGenerationGuardedDeliver({ + stillCurrent: () => true, + onStale: () => ({ + status: "not-delivered", + reason: "superseded", + detail: "stale", + }), + run: async () => { + runs += 1; + return { status: "accepted" as const }; + }, + }); + expect(result).toEqual({ status: "accepted" }); + expect(runs).toBe(1); + }); +}); + describe("deliveryResultNotice", () => { test("closed restored and deferred copy is actionable", () => { const closed = { diff --git a/src/tui/deliver-agent-message.ts b/src/tui/deliver-agent-message.ts index 7b2b8545b..0a70b3f07 100644 --- a/src/tui/deliver-agent-message.ts +++ b/src/tui/deliver-agent-message.ts @@ -59,6 +59,28 @@ export async function deliverAgentMessage( } } +/** + * Settles a deliver that was enqueued on the serial operation queue against + * the shoot generation captured at enqueue time. The queue is FIFO with no + * preemption, so a deliver queued ahead of a reload still executes after the + * reload has replaced the agent — the generation must be re-checked when the + * queued closure runs, not just when it enqueues. A stale deliver takes the + * `onStale` path (the caller reports `not-delivered`); a current deliver runs + * the real settle. This is what closes the reload-vs-async-deliver race: a + * reload that lands while a continuation answer is queued wins, and the stale + * answer is dropped instead of reaching the replaced agent. + */ +export async function runGenerationGuardedDeliver(options: { + stillCurrent: () => boolean; + run: () => Promise; + onStale: () => AgentDeliveryResult; +}): Promise { + if (!options.stillCurrent()) { + return options.onStale(); + } + return options.run(); +} + /** Operator-facing copy for a settled delivery that did not accept. */ export function deliveryResultNotice( result: Exclude, diff --git a/src/tui/runner/session.ts b/src/tui/runner/session.ts index 184de85fc..38f004199 100644 --- a/src/tui/runner/session.ts +++ b/src/tui/runner/session.ts @@ -94,6 +94,7 @@ import type { ToolWatchdogConfig } from "../tool-execution-watchdog.js"; import { deliverAgentMessage, deliveryResultNotice, + runGenerationGuardedDeliver, type AgentDeliveryResult, } from "../deliver-agent-message.js"; import { createProviderFailureAttemptTracker } from "../provider/failure-attempt.js"; @@ -539,20 +540,24 @@ export async function assembleTUISession( ): void => { const stillCurrent = deliveryGeneration.capture(); void sessionOps.enqueue(async () => { - if (!stillCurrent()) { - onSettle?.({ + // The shell already popped the queue item and painted it as delivered + // by the time this runs, so a failed rebuild or closed agent must settle + // here — otherwise the message silently never reaches the agent. The + // generation is re-checked at execution time (the queue is FIFO with no + // preemption), so a reload that lands while this deliver is queued wins + // and the stale answer is dropped as superseded. + const result = await runGenerationGuardedDeliver({ + stillCurrent, + onStale: () => ({ status: "not-delivered", reason: "superseded", detail: "session identity changed before delivery", - }); - return; - } - // The shell already popped the queue item and painted it as delivered - // by the time this runs, so a failed rebuild or closed agent must settle - // here — otherwise the message silently never reaches the agent. - const result = await deliverAgentMessage({ - getFatalBuildError: () => state.fatalBuildError, - deliverToLiveAgent, + }), + run: () => + deliverAgentMessage({ + getFatalBuildError: () => state.fatalBuildError, + deliverToLiveAgent, + }), }); if (onSettle !== undefined) { onSettle(result);