From b0bc0b579294c269e9a988a016f8acd54b200521 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 15 Jun 2026 20:56:17 +0800 Subject: [PATCH] feat(agent-loop): turn-enclosure invariant + post-turn error model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every session event now lives inside a turn (between turn/start and its turn/end). The loop records queued user/message events AFTER turn/start; an idle agent.inject() wraps its context/message in a one-shot injection turn. This makes the turn the single durability/replay boundary so a persistence backend can treat anything after the last turn/end as a crash tail without dropping legitimate between-turn context. A failure once the turn is already closed (rejecting session/flush, a throwing agent/turn-end listener) has no in-turn position for a session error event, so it is reported via agent/error + logger only; the turn stays balanced. failTurn appends an error event only while the turn is open. The dsh-invariants plugin enforces turn-enclosure via a default case: every non-boundary event type — including plugin-added merge-extensible keys — must sit inside an open turn or it throws. Documented in ADR 0017 + architecture.md. --- docs/adr/0017-turn-enclosure-invariant.md | 38 ++++++++ docs/adr/README.md | 1 + docs/architecture.md | 14 ++- packages/agent-loop/README.md | 2 +- packages/agent-loop/src/agent.ts | 49 +++++++++- packages/agent-loop/src/loop.ts | 94 ++++++++++++++----- packages/agent-loop/tests/agent.spec.ts | 76 ++++++++++++++- packages/agent-loop/tests/loop.spec.ts | 49 +++++++++- .../agent-loop/tests/review-fixes.spec.ts | 33 +++++++ packages/agent/README.md | 2 +- packages/agent/src/types.ts | 12 ++- packages/invariants/src/index.ts | 25 ++++- packages/invariants/tests/invariants.spec.ts | 40 +++++++- 13 files changed, 386 insertions(+), 49 deletions(-) create mode 100644 docs/adr/0017-turn-enclosure-invariant.md diff --git a/docs/adr/0017-turn-enclosure-invariant.md b/docs/adr/0017-turn-enclosure-invariant.md new file mode 100644 index 0000000000..f4b88420ea --- /dev/null +++ b/docs/adr/0017-turn-enclosure-invariant.md @@ -0,0 +1,38 @@ +# ADR 0017: Every session event is enclosed in a turn + +Status: accepted (2026-06-15) + +## Context + +A durable session-persistence backend (added in a companion change) uses the **turn** as its crash-recovery boundary: `load` returns events only up to the last complete `turn/end`, and the first post-load `append` truncates whatever follows as a never-committed crash tail. This is safe only if nothing *legitimately* durable can sit after the last `turn/end`. + +That assumption did not hold. Two paths recorded events outside any turn: + +1. **Queued user messages.** The loop drained queued messages and appended `user/message` *before* `turn/start` — so a turn's own prompt sat in the gap between the previous `turn/end` and the next `turn/start`. +2. **Idle context injection.** `agent.inject()` appends a `context/message` directly. Its real production caller is `dsh-tool-bash`, which injects a background-task completion notice from `ctx.bash.onTaskDone` — a callback that fires whenever a background bash task finishes, frequently while the agent is **idle** (between turns). + +In case 2, if the injected `context/message` is the last event before a flush/dispose (no later turn appends a `turn/end`), `scanLog` treats it as crash debris and **drops it on resume** — the injected context is durably on disk but silently lost on reload. Case 1 was benign in isolation (a `user/message` is always followed by the turn it triggered) but made the "what may appear outside a turn" rule fuzzy. + +Two ways to fix it: relax the *reader* (let `scanLog` commit events that sit outside an open turn), or constrain the *producer* (make every event turn-enclosed so the reader's simple "last `turn/end`" rule is both correct and complete). We chose the producer-side invariant: a single, checkable rule beats a more permissive boundary scan that has to reason about partial turns *and* loose between-turn events. + +## Decision + +**Every session event lives inside a turn** — between a `turn/start` and its matching `turn/end`. Concretely: + +- The loop appends queued `user/message` events **after** `turn/start` (inside the turn), not before it. `turn/end` is therefore owed the moment those messages are recorded, and the existing finalizer guarantees it. +- An `agent.inject()` made while the agent is **running** appends its `context/message` into the already-open turn (unchanged). +- An `agent.inject()` made while **idle** wraps its `context/message` in a one-shot turn: `turn/start{trigger:{kind:'injection'}}` → `context/message` → `turn/end{completed}`. A new `injection` variant joins the merge-extensible `TurnTriggerMap`. +- The loop derives the next turn number from the log each iteration (`lastTurnNumber(session) + 1`) instead of keeping a private counter, so an idle injection's one-shot turn cannot collide with the next real turn's number. +- The `dsh-invariants` plugin **enforces** the invariant in dev: a `user/message` / `context/message` / `steering/message` appended while no turn is open throws an `InvariantError`. + +The serializability invariant is enforced at the same source boundary (`Session.append` throws on non-JSON-serializable data), so "what may enter the log" is now governed in one place rather than discovered downstream by whichever backend happens to be watching. + +## Consequences + +The turn is now the *single* durability/replay boundary, so a persistence backend's "last `turn/end` = commit point" rule is complete, not merely sufficient: a backend can discard everything after the last `turn/end` with zero risk of losing between-turn context, because there is no between-turn context. `scanLog` stays simple (no partial-turn boundary walk), and an idle background-task notice survives persist + resume. + +Costs: `agent.inject()` while idle now writes three log lines instead of one, and the derived history gains a turn that carries only injected context (no assistant output) — `deriveMessages()` already derives purely by event type, so this renders identically. The `injection` trigger is a new on-disk vocabulary value; like every `SessionEventMap`/`TurnTriggerMap` addition it is part of the frozen format. Event ordering within a turn changed (`turn/start` now precedes `user/message`), which is observable to anything that asserted the old order — the loop's own tests were the only such consumers. + +The rule is intentionally producer-enforced and dev-checked rather than reader-tolerated: a future backend (SQLite/WAL) inherits the same clean boundary for free, and a plugin that records an event outside a turn fails loudly in dev instead of silently losing data on the next reload. + +The invariant also constrains where the loop may record an `error` event. A failure detected while a turn is open is appended INSIDE the turn (before `turn/end`); but a failure that surfaces once the turn is already closed — a rejecting `session/flush` (which runs as the post-`turn/end` durability checkpoint) or a throwing `agent/turn-end` listener (after `closeTurn` already appended `turn/end`) — has no in-turn position left. Appending an `error` there would land it past the last `turn/end`, exactly the crash-tail position a backend discards. So those post-turn failures are reported via the `agent/error` event and the logger only, never as a `SessionEvent`; the turn stays balanced and persistence keeps its buffered events for the next checkpoint. If durable operational diagnostics are ever needed, they belong on a separate telemetry channel, not the replayable session log. diff --git a/docs/adr/README.md b/docs/adr/README.md index 963e8f47bd..43273a32c1 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -27,3 +27,4 @@ Do NOT write an ADR for: a mechanical or local choice (a variable name, a one-fi | [0013](0013-property-based-testing.md) | Property-based testing for protocol-shaped code | accepted | | [0014](0014-doc-sync-enforcement.md) | Doc-sync enforcement (doc code blocks + event taxonomy) | accepted | | [0015](0015-structured-error-taxonomy.md) | Structured error taxonomy (HarnessError base) | accepted | +| [0017](0017-turn-enclosure-invariant.md) | Every session event is enclosed in a turn | accepted | diff --git a/docs/architecture.md b/docs/architecture.md index 6cab7201b5..17408cb764 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -90,7 +90,7 @@ A `Session` is an append-only log of typed `SessionEvent`s — the single source - `tool/result` → user message carrying a `tool-result` block - `context/message`, `steering/message` → user-role messages wrapped in a tagged envelope (``) at their chronological position — the "system-reminder" pattern; models distinguish them from real user prompts by the envelope. **TODO(review)**: the real adapters now exist (the original precondition); the envelope still wants a deliberate review against live model behavior (`TODO(review)` in dsh-session). -Replay/fork = `ctx.sessions.create(id, seedEvents)`. Trace/telemetry = listen to `session/event`. +Replay/fork = `ctx.sessions.create(id, { seed: seedEvents })`. Trace/telemetry = listen to `session/event`. **Durability seam**: `session/event` is a synchronous notification; persistence plugins buffer (write-behind) and drain at the awaited `session/flush` checkpoint the loop fires at every turn end (see `examples/echo-agent/src/session-jsonl.ts` for the pattern). **TODO**: real persistence backends (JSONL per session dir, sqlite) are a future phase. @@ -114,7 +114,7 @@ Tool schemas are deliberately **part of the assembly**: "what the model is told - `send(content)` — queued message; starts a turn when idle, else next turn - `steer(content)` — mid-turn injection, drained **between steps**; behaves like `send` when idle -- `inject(content)` — in-session context (`context/message` event) without triggering a turn; the next request sees it (Claude Code attachment / system-reminder analog) +- `inject(content)` — in-session context (`context/message` event); the next request sees it (Claude Code attachment / system-reminder analog). An inject made while the agent is *running* joins the open turn; an inject while *idle* is wrapped in a one-shot turn (`turn/start{trigger:injection}` → `context/message` → `turn/end`) so every event stays turn-enclosed (see ADR 0017). - `abort(reason)` — aborts the in-flight step via `AbortSignal` - `session`, `status`, `options` @@ -131,7 +131,7 @@ forever: wait for queued messages (idle) emit agent/status(running) TURN (error-contained — a throwing plugin ends the turn, never the loop): - drain queued → session('user/message'…) → 'turn/start' → emit agent/turn-start + drain queued → 'turn/start' → session('user/message'…) → emit agent/turn-start STEP loop: drain steering (late steering from previous step's listeners) session('step/start'); emit agent/step-start @@ -160,7 +160,11 @@ forever: emit agent/status(idle) unless more queued ``` -Error containment: a throwing `agent/turn-continuation` listener or a rejecting `session/flush` ends the **turn** with an `error` event — never the driver loop. An adapter that ends its stream with a `finish {kind:'error'}` or `{kind:'aborted'}` chunk (the in-band error path, for adapters that can't throw mid-stream) is likewise translated into a step error, so the turn ends `error`/`aborted` instead of logging a normal `completed` assistant message. `abort()` is honored mid-stream **and** between tool calls; disposal mid-turn ends the turn with reason `disposed` and emits `agent/status('disposed')`. +Error containment: a throwing `agent/turn-continuation` listener or a broken step ends the **turn** with an `error` event (appended INSIDE the turn, before `turn/end`) — never the driver loop. An adapter that ends its stream with a `finish {kind:'error'}` or `{kind:'aborted'}` chunk (the in-band error path, for adapters that can't throw mid-stream) is likewise translated into a step error, so the turn ends `error`/`aborted` instead of logging a normal `completed` assistant message. `abort()` is honored mid-stream **and** between tool calls; disposal mid-turn ends the turn with reason `disposed` and emits `agent/status('disposed')`. + +A failure that happens once the turn is already closed has no in-turn position for a session `error` event (appending one after `turn/end` would put it past a persistence backend's commit boundary, where it is dropped as a crash tail — ADR 0017). So a rejecting `session/flush` (the post-`turn/end` durability checkpoint) and a throwing `agent/turn-end` listener are reported via `agent/error` + the logger only, NOT as a session event; the turn stays balanced and the backend keeps its buffered events for the next flush. + +**Turn-enclosure invariant**: every session event lives inside a turn (between a `turn/start` and its `turn/end`). The loop appends queued `user/message` events *after* `turn/start`, and an idle `agent.inject()` wraps its `context/message` in a one-shot `injection` turn. This makes the turn the single durability/replay boundary: a persistence backend can treat anything after the last `turn/end` as an interrupted-crash tail without risking the loss of legitimately-recorded between-turn context. The `dsh-invariants` plugin enforces it in dev (a message event outside an open turn throws). See ADR 0017. ### Event taxonomy @@ -221,7 +225,7 @@ Every MVP feature (including the TODO-marked ones), with the mechanism that impl | Memory | section provider + tool | | Scheduled tasks (cron) | plugin registers model-callable scheduling tools; timer fires → `send(…, {source: {kind: 'cron', …}})` when idle / `inject()` notification when busy | | UI (GUI; CLI emits JSONL) | listen `agent/stream-chunk` + `session/event`; input → `send()` | -| Telemetry / replayable trace | `session/event` → JSONL; replay = `sessions.create(id, seed)` | +| Telemetry / replayable trace | `session/event` → JSONL; replay = `sessions.create(id, { seed })` | | DeepSeek V4 (and other) models | `LlmAdapter` subclass via `registerAdapter`. **Implemented twice**: `dsh-llm-deepseek` (hand-rolled) and `dsh-llm-pi-ai` (pi-ai-backed) | | Plugin hot-reload | every registration is a `ctx.effect` → vendored HMR just works | diff --git a/packages/agent-loop/README.md b/packages/agent-loop/README.md index 49176701c5..2a99fe2b6c 100644 --- a/packages/agent-loop/README.md +++ b/packages/agent-loop/README.md @@ -41,7 +41,7 @@ One invocation of `runLoop()` drives one agent for its whole lifetime: forever: wait for queued messages (idle) TURN (error-contained): - drain queued → session('user/message') → 'turn/start' + drain queued → 'turn/start' → session('user/message') STEP loop: drain steering assembly = systemPrompt.assemble() diff --git a/packages/agent-loop/src/agent.ts b/packages/agent-loop/src/agent.ts index bc8c72650f..7705f8c3ed 100644 --- a/packages/agent-loop/src/agent.ts +++ b/packages/agent-loop/src/agent.ts @@ -12,7 +12,7 @@ import type { Agent } from '@deepseek-ai/dsh-agent' import type { ContentBlock, MessageSource } from '@deepseek-ai/dsh-llm' import type { Session } from '@deepseek-ai/dsh-session' import { Inbox } from './inbox.ts' -import { runLoop } from './loop.ts' +import { isTurnOpen, lastTurnNumber, runLoop } from './loop.ts' /** * The concrete {@link Agent} implementation owned by the agent-loop plugin. @@ -73,7 +73,52 @@ export class LoopAgent implements Agent { inject(content: ContentBlock[], options?: SendOptions): void { if (this._status === 'disposed') throw new Error(`agent "${this.id}" is disposed`) - this.session.append('context/message', { content, source: this.resolveSource(options) }) + const source = this.resolveSource(options) + if (isTurnOpen(this.session)) { + // A turn is open in the LOG (decided from the log, not agent status — + // status can be `running` with no turn open): the context/message is + // turn-enclosed by that turn, so append it directly. + this.session.append('context/message', { content, source }) + return + } + // No turn open: wrap the injection in a one-shot turn so every event stays + // turn-enclosed (the durability/replay boundary is the turn). + const turn = lastTurnNumber(this.session) + 1 + // Once turn/start enters the log, a turn/end is OWED no matter what — even + // if a throwing `session/event` listener escapes from the turn/start append + // (Session.append pushes the event BEFORE notifying listeners) or the + // context/message append throws (non-serializable content, throwing + // listener). The finally re-checks the log via isTurnOpen() and closes the + // turn if one was actually opened, so the log never carries a permanently + // open injection turn that would corrupt later turns/replay. (If the + // turn/start append throws BEFORE pushing — non-serializable trigger, which + // can't happen for our fixed trigger — no turn was opened and none is owed.) + let turnRecorded = false + try { + this.session.append('turn/start', { turn, trigger: { kind: 'injection', source } }) + this.session.append('context/message', { content, source }) + } finally { + // A turn was recorded iff turn/start made it into the log. Close it and + // mark it for the durability checkpoint below — which must run even when + // an append's listener threw (the turn is balanced and in memory, so it + // still needs a flush or a crash before the next turn/dispose loses it). + if (isTurnOpen(this.session)) { + this.session.append('turn/end', { turn, reason: { kind: 'completed' } }) + turnRecorded = true + } + // Checkpoint the one-shot turn for durability, exactly as the loop does at + // every turn/end. The loop is NOT running (we are idle), so nothing else + // will flush this turn. Fire-and-forget with error containment: inject() + // is synchronous, and a persistence backend failing must not throw into + // the caller (e.g. a tool-bash task-done callback). Disposal still drains + // independently, so a slow flush is safe. In the finally so it also runs + // when an append's listener threw (the turn is still balanced + durable). + if (turnRecorded) { + void Promise.resolve(this.ctx.parallel('session/flush', this.session)).catch((error: unknown) => { + this.ctx.logger.warn(`agent "${this.id}": flush after idle injection failed: ${String(error)}`) + }) + } + } } abort(reason?: string): void { diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index c556acfc20..47612c3879 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -91,7 +91,7 @@ export interface LoopHandle { * forever: * wait for queued messages (idle) * TURN (error-contained — a throwing plugin ends the turn, never the loop): - * drain queued → session('user/message'…) → 'turn/start' → emit agent/turn-start + * drain queued → 'turn/start' → session('user/message'…) → emit agent/turn-start * STEP loop: * drain steering → session('steering/message') ⟵ catches late steering * session('step/start'); emit agent/step-start ⟵ append before emit (ADR 0003) @@ -118,24 +118,31 @@ export interface LoopHandle { */ export async function runLoop(ctx: Context, agent: LoopAgent, handle: LoopHandle): Promise { const { session } = agent - let turn = lastTurnNumber(session) // seeded/forked sessions continue numbering while (!handle.isDisposed()) { await agent.inbox.waitForQueued(handle.disposed) if (handle.isDisposed()) break handle.setStatus('running') - turn += 1 + // Re-derive the turn number from the log each iteration (do NOT keep a local + // counter): an idle `agent.inject()` can append its own one-shot turn while + // the loop waits above, so the next real turn must continue from whatever + // turn number is actually last in the log — a stale counter would collide. + const turn = lastTurnNumber(session) + 1 try { await runTurn(ctx, agent, handle, turn) } catch (error: unknown) { - // Backstop: a throwing emit listener (turn boundaries) or a broken - // finalizer must not kill the driver. Record what we can and move on. + // Backstop: runTurn rethrows only a PRE-turn throw (the invariant guard + // before turn/start) — no turn/start was appended, so no turn is open and + // none is owed. A session `error` here would land outside any turn (after + // the previous turn/end), where the persistence backend drops it as a + // crash tail (ADR 0017). Report via agent/error + the logger only; the + // driver survives and moves on. + const err = toError(error) + ctx.logger.warn(`agent "${agent.id}": turn ${turn} failed before it started: ${err.message}`) try { - const err = toError(error) - session.append('error', { turn, step: 0, ...errorData(err) }) ctx.emit('agent/error', agent, turn, 0, err) - } catch { /* the error path itself is broken; nothing left to do */ } + } catch { /* contained: a throwing agent/error listener must not kill the driver */ } } // Steering that arrived too late to join this turn (turn-end listeners, @@ -151,17 +158,15 @@ export async function runLoop(ctx: Context, agent: LoopAgent, handle: LoopHandle async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: number): Promise { const { session } = agent - // --- Pre-turn. A throw here (the invariant guard or a user-message append) - // is owed NO turn/end — turn/start has not been appended — so it propagates - // to runLoop's backstop untouched. + // --- Pre-turn. A throw here (the invariant guard) is owed NO turn/end — + // turn/start has not been appended — so it propagates to runLoop's backstop + // untouched. The queued messages are drained here but appended AFTER + // turn/start (below), so every event in the log lives inside a turn. const queued = agent.inbox.drainQueued() const first = queued[0] /* v8 ignore next 3 -- invariant guard: runLoop only calls runTurn when hasQueued */ if (!first) throw new Error('runTurn invariant violated: no queued message at turn start') const trigger: TurnTrigger = { kind: 'message', source: first.source } - for (const message of queued) { - session.append('user/message', { content: message.content, source: message.source }) - } let reason: TurnEndReason = { kind: 'completed' } let step = 0 @@ -186,16 +191,26 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: } } - // Record a step/turn failure exactly once: append the single `error` event, - // set the error reason, and emit agent/error (contained — trap: a throwing - // agent/error listener must not re-escape and strand the turn). Disposal and - // abort set `reason` directly without calling this (no `error` event for - // those — they are not failures). + // Record a step/turn failure exactly once: append the single `error` event + // (only while the turn is still open — see below), set the error reason, and + // emit agent/error (contained — trap: a throwing agent/error listener must not + // re-escape and strand the turn). Disposal and abort set `reason` directly + // without calling this (no `error` event for those — they are not failures). const failTurn = (err: CodedError): void => { if (errorReported) return errorReported = true - session.append('error', { turn, step, ...errorData(err) }) - reason = { kind: 'error', ...errorData(err) } + // Only append the session `error` INSIDE the turn (before turn/end). If the + // turn has already ended — the only way here is a throwing agent/turn-end + // listener after closeTurn(true) already appended turn/end — appending now + // would land the error AFTER the last turn/end, where the persistence + // backend treats it as a crash tail and drops it on resume (ADR 0017). In + // that case report via agent/error + the logger only; the turn is balanced. + if (!turnEnded) { + session.append('error', { turn, step, ...errorData(err) }) + reason = { kind: 'error', ...errorData(err) } + } else { + ctx.logger.warn(`agent "${agent.id}": agent/turn-end listener threw after turn ${turn} closed: ${err.message}`) + } try { ctx.emit('agent/error', agent, turn, step, err) } catch { @@ -221,6 +236,12 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: // matter what throws below; the catch + closeTurn guarantee it. session.append('turn/start', { turn, trigger }) turnStarted = true + // Record the queued user messages INSIDE the turn (after turn/start), so + // every event in the log is turn-enclosed. turn/end is now owed, so a throw + // while appending these is caught below and the turn is still closed. + for (const message of queued) { + session.append('user/message', { content: message.content, source: message.source }) + } ctx.emit('agent/turn-start', agent, turn) while (true) { @@ -324,9 +345,20 @@ async function runTurn(ctx: Context, agent: LoopAgent, handle: LoopHandle, turn: try { await ctx.parallel('session/flush', session) } catch (error: unknown) { + // The turn is already closed (turn/end appended above) and flush must run + // AFTER turn/end to be a checkpoint — so there is no in-turn position left + // for a session `error` event. Appending one here would land it after the + // last turn/end, where the persistence backend treats it as a crash tail + // and drops it on resume (ADR 0017: every event is turn-enclosed). Report + // the failure via agent/error + the logger only; persistence keeps the + // buffered events for the next flush/dispose, so nothing is lost. const err = toError(error) - session.append('error', { turn, step, ...errorData(err) }) - ctx.emit('agent/error', agent, turn, step, err) + ctx.logger.warn(`agent "${agent.id}": session/flush failed at turn ${turn}: ${err.message}`) + try { + ctx.emit('agent/error', agent, turn, step, err) + } catch { + // contained: a throwing agent/error listener must not escape the loop. + } } } @@ -445,7 +477,21 @@ async function runStep( } /** The last turn number in a (possibly seeded) session log, or 0. */ -function lastTurnNumber(session: Session): number { +export function lastTurnNumber(session: Session): number { const lastStart = session.events.findLast(event => event.type === 'turn/start') return lastStart?.data.turn ?? 0 } + +/** + * Whether a turn is currently open in the session log (a `turn/start` with no + * matching later `turn/end`). Decided from the LOG, not agent status: status + * can be `running` while no turn is open (an `agent/status` listener firing + * before `turn/start`, or the post-`turn/end` flush window before status + * returns to idle), so status is not a reliable open-turn signal. Used by + * `inject()` to choose between appending into an open turn vs. wrapping the + * injection in its own one-shot turn (ADR 0017). + */ +export function isTurnOpen(session: Session): boolean { + const last = session.events.findLast(e => e.type === 'turn/start' || e.type === 'turn/end') + return last?.type === 'turn/start' +} diff --git a/packages/agent-loop/tests/agent.spec.ts b/packages/agent-loop/tests/agent.spec.ts index 26933188fc..32b28be74a 100644 --- a/packages/agent-loop/tests/agent.spec.ts +++ b/packages/agent-loop/tests/agent.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { Context } from 'cordis' import { AgentId } from '@deepseek-ai/dsh-agent' import LlmService from '@deepseek-ai/dsh-llm' @@ -82,6 +82,80 @@ describe('LoopAgent', () => { expect(() => { agent.inject([{ type: 'text', text: 'too late' }]) }).toThrow('disposed') }) + it('inject() decides enclosure from the LOG (open turn), not agent status', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + // Simulate an OPEN turn in the log while the agent is idle (status is not a + // reliable open-turn signal). inject must append into that open turn, NOT + // wrap a new one. + agent.session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) + agent.inject([{ type: 'text', text: 'mid' }], { source: { kind: 'plugin', plugin: 'p' } }) + expect(agent.session.events.filter(e => e.type === 'turn/start')).toHaveLength(1) + expect(agent.session.events.at(-1)!.type).toBe('context/message') + + // Close the turn; now inject must wrap its own one-shot injection turn. + agent.session.append('turn/end', { turn: 1, reason: { kind: 'completed' } }) + agent.inject([{ type: 'text', text: 'after' }], { source: { kind: 'plugin', plugin: 'p' } }) + const starts = agent.session.events.filter(e => e.type === 'turn/start') + expect(starts).toHaveLength(2) + const last = starts[1]! + expect(last.type === 'turn/start' && last.data.trigger.kind).toBe('injection') + expect(agent.session.events.at(-1)!.type).toBe('turn/end') // turn-enclosed + }) + + it('idle inject() contains a failing flush (logs, does not throw into the caller)', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + // A persistence-like listener whose flush rejects. + ctx.on('session/flush', () => { throw new Error('disk gone') }) + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + // inject() is synchronous and fires a fire-and-forget flush; a rejecting + // flush must be contained (logged), never thrown into the caller. + expect(() => { agent.inject([{ type: 'text', text: 'notice' }], { source: { kind: 'plugin', plugin: 'p' } }) }).not.toThrow() + await new Promise(r => setTimeout(r, 20)) // let the contained flush settle + expect(warn).toHaveBeenCalledWith(expect.stringContaining('flush after idle injection failed')) + warn.mockRestore() + }) + + it('idle inject() closes its one-shot turn AND still checkpoints even if the append throws', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + let flushes = 0 + ctx.on('session/flush', () => { flushes += 1 }) + + // Non-serializable injected content makes Session.append throw AFTER + // turn/start was recorded. The turn/end must still be appended (finally), + // AND the durability checkpoint must still fire — the balanced turn is in + // memory and a crash before the next turn/dispose would otherwise lose it. + expect(() => { + agent.inject([{ type: 'text', text: 'x', bad: 1n } as never], { source: { kind: 'plugin', plugin: 'p' } }) + }).toThrow(/non-JSON-serializable/) + const types = agent.session.events.map(e => e.type) + expect(types).toEqual(['turn/start', 'turn/end']) // balanced, no open turn + await new Promise(r => setTimeout(r, 10)) // let the fire-and-forget flush run + expect(flushes).toBe(1) // checkpoint fired despite the throw + }) + + it('idle inject() with a non-serializable source opens no turn (nothing to close)', async () => { + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + // A non-serializable source makes the turn/start append throw BEFORE the + // event is pushed (Session.append validates before push), so NO turn opens. + // The finally's isTurnOpen() guard sees no open turn and appends nothing — + // the log stays empty, not left with a dangling turn/start. + expect(() => { + agent.inject([{ type: 'text', text: 'x' }], { source: { kind: 'plugin', plugin: 'p', bad: 1n } as never }) + }).toThrow(/non-JSON-serializable/) + expect(agent.session.events).toHaveLength(0) + }) + it('steer() when idle falls through to send() and starts a turn', async () => { const adapter = new MockAdapter([textResponse('ok')]) const ctx = await harness(adapter) diff --git a/packages/agent-loop/tests/loop.spec.ts b/packages/agent-loop/tests/loop.spec.ts index 1b47fd8706..94a7181771 100644 --- a/packages/agent-loop/tests/loop.spec.ts +++ b/packages/agent-loop/tests/loop.spec.ts @@ -57,9 +57,10 @@ describe('agent loop', () => { expect(order).toEqual(['agent/turn-start', 'agent/step-start', 'agent/step-end', 'agent/turn-end']) const types = agent.session.events.map(e => e.type) - // user message recorded before turn/start, assembled message + usage present - expect(types[0]).toBe('user/message') - expect(types[1]).toBe('turn/start') + // turn/start opens the turn, THEN the queued user message is recorded inside + // it (every event is turn-enclosed), then assembled message + usage. + expect(types[0]).toBe('turn/start') + expect(types[1]).toBe('user/message') expect(types).toContain('assistant/message') expect(types).toContain('usage') expect(types.at(-1)).toBe('turn/end') @@ -199,16 +200,22 @@ describe('agent loop', () => { expect(agent.session.events.some(e => e.type === 'user/message')).toBe(true) }) - it('inject() appends context visible to the next request without starting a turn', async () => { + it('inject() while idle wraps context in a one-shot turn, visible to the next request', async () => { const adapter = new MockAdapter([textResponse('ok')]) const ctx = await harness(adapter) const agent = ctx.agentLoop.create('a1', { model: 'mock' }) agent.inject([{ type: 'text', text: 'file changed: a.ts' }], { source: { kind: 'plugin', plugin: 'watcher' } }) - // no turn started + // The idle inject records a self-contained turn (turn/start → context/message + // → turn/end) so the event stays turn-enclosed, but does NOT run the model. await new Promise(r => setTimeout(r, 20)) expect(agent.status).toBe('idle') expect(adapter.requests).toHaveLength(0) + const injectedTurn = agent.session.events.filter(e => e.type === 'turn/start') + expect(injectedTurn).toHaveLength(1) + const it0 = injectedTurn[0]! + expect(it0.type === 'turn/start' && it0.data.trigger.kind).toBe('injection') + expect(agent.session.events.at(-1)!.type).toBe('turn/end') // turn-enclosed send(agent, 'go') await waitForIdle(ctx, agent) @@ -217,6 +224,38 @@ describe('agent loop', () => { expect(flat).toContain('') }) + it('inject() while running appends into the open turn (no extra synthetic turn)', async () => { + const adapter = new MockAdapter([ + toolCallResponse('c1', 'noticer', {}, 'calling'), + textResponse('done'), + ]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + // A tool that injects mid-execution: at this point the agent is running, so + // inject must append the context/message into the ALREADY-open turn rather + // than wrap it in its own one-shot turn. + ctx.tools.register(defineTool({ + name: 'noticer', + description: 'injects a notice', + parameters: {}, + async execute() { + agent.inject([{ type: 'text', text: 'mid-turn notice' }], { source: { kind: 'plugin', plugin: 'x' } }) + return [{ type: 'text', text: 'ok' }] + }, + })) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + // Exactly ONE turn ran (no synthetic injection turn), and the mid-turn + // context/message sits inside it. + const turnStarts = agent.session.events.filter(e => e.type === 'turn/start') + expect(turnStarts).toHaveLength(1) + const ts0 = turnStarts[0]! + expect(ts0.type === 'turn/start' && ts0.data.trigger.kind).toBe('message') + expect(agent.session.events.some(e => e.type === 'context/message')).toBe(true) + }) + it('agent/turn-continuation can force-continue (/loop pattern) and force-stop', async () => { // force-continue: model never calls tools, but a plugin forces 3 steps const adapter = new MockAdapter([ diff --git a/packages/agent-loop/tests/review-fixes.spec.ts b/packages/agent-loop/tests/review-fixes.spec.ts index 707da04366..f79f1b17e9 100644 --- a/packages/agent-loop/tests/review-fixes.spec.ts +++ b/packages/agent-loop/tests/review-fixes.spec.ts @@ -854,6 +854,39 @@ describe('P1-5: a started turn (and any open step) is always closed on a boundar expect(adapter.requests).toHaveLength(1) }) + it('a throwing turn-end listener on a SUCCESSFUL turn leaves no event after turn/end (loadable log)', async () => { + // Regression: a normal turn completes, closeTurn(true) appends turn/end and + // emits agent/turn-end whose listener throws. The error must NOT be appended + // as a session event after turn/end — that would sit past the commit + // boundary and be dropped as a crash tail on resume (ADR 0017). It is + // surfaced via agent/error instead, and the log's last event is turn/end. + const adapter = new MockAdapter([textResponse('done'), textResponse('next ok')]) + const ctx = await balancedHarness(adapter) + const agent = ctx.agentLoop.create('a-tend', { model: 'mock' }) + + let threw = false + ctx.on('agent/turn-end', () => { if (!threw) { threw = true; throw new Error('boom turn-end') } }) + const errors: Error[] = [] + ctx.on('agent/error', (_a, _t, _s, error) => void errors.push(error)) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + const c = boundaryCounts(agent) + expect(c.turnEnd).toBe(1) + expect(c.errors).toBe(0) // NO session error event (it would be post-turn/end) + expect(agent.session.events.at(-1)?.type).toBe('turn/end') // last event is the boundary + expect(errors.map(e => e.message)).toEqual(['boom turn-end']) // surfaced via agent/error + // The whole log is loadable (nothing dropped): a fresh replay sees the turn. + const replay = new Session(SessionId('replay'), [...agent.session.events]) + expect(replay.deriveMessages().map(m => m.role)).toEqual(['user', 'assistant']) + + // loop survives. + send(agent, 'again') + await waitForIdle(ctx, agent) + expect(boundaryCounts(agent).turnEnd).toBe(2) + }) + it('a step error followed by a throwing turn-end listener logs the error exactly once (no double-report)', async () => { // The step fails (finish-error) → failTurn records ONE error and sets the // error reason. closeTurn(true) then appends turn/end and emits diff --git a/packages/agent/README.md b/packages/agent/README.md index 5f6fb85893..57b1b87fc4 100644 --- a/packages/agent/README.md +++ b/packages/agent/README.md @@ -45,7 +45,7 @@ The handle every plugin programs against: - `agent.send(content, options?)` — queue a message; starts a turn when idle - `agent.steer(content, options?)` — steer a running turn (inject between steps); behaves like `send` when idle -- `agent.inject(content, options?)` — inject in-session context without triggering a turn (context/message event); next request sees it +- `agent.inject(content, options?)` — inject in-session context (context/message event); next request sees it. While running it joins the open turn; while idle it is wrapped in a one-shot `injection` turn so every event stays turn-enclosed (ADR 0017) - `agent.abort(reason?)` — abort the in-flight step - `agent.session`, `agent.status`, `agent.options`, `agent.id` diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index 0441938b6d..d6adc4760e 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -60,9 +60,15 @@ export interface Agent { /** * Inject in-session context (file-change notices, skill content, cron - * notifications, …): appends a `context/message` session event without - * triggering a turn — the next model request sees it at its chronological - * position, rendered as tagged synthetic context rather than a user prompt. + * notifications, …): appends a `context/message` session event the next model + * request sees at its chronological position, rendered as tagged synthetic + * context rather than a user prompt. Does not run the model. + * + * Turn-enclosure (ADR 0017): an inject while a turn is open joins that turn; + * an inject while idle wraps its `context/message` in a one-shot `injection` + * turn (`turn/start` → `context/message` → `turn/end`) and checkpoints it for + * durability, so every event stays inside a turn and a persistence backend + * never loses a between-turn notice. * * TODO(review): exact envelope/rendering rules live in dsh-session and need * review once a real adapter exists. diff --git a/packages/invariants/src/index.ts b/packages/invariants/src/index.ts index fa72788ae4..ebb4decfd8 100644 --- a/packages/invariants/src/index.ts +++ b/packages/invariants/src/index.ts @@ -105,11 +105,10 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { } trace.lastSeq = event.seq - // Intentionally non-exhaustive: only events that carry ordering structure - // are checked; the rest are trace/replay data with no nesting contract. - // SessionEventMap is merge-extensible, so no assertNever — unknown event - // types fall through untouched. - // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check + // Boundary/step-scoped events have explicit cases; every OTHER event type — + // including plugin-added (merge-extensible) SessionEventMap keys — is caught + // by the `default` and must be turn-enclosed (ADR 0017). No assertNever: an + // unknown variant is valid, not a compile error. switch (event.type) { case 'turn/start': { if (trace.openTurn !== null) { @@ -169,6 +168,22 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { } break } + // Turn-enclosure (ADR 0017): EVERY session event not handled by a boundary + // case above must sit inside an open turn. The durable session log uses the + // turn as its commit/replay boundary (the JSONL backend treats anything + // after the last turn/end as a crash tail), so a bare event between turns is + // silently dropped on reload. The loop records queued user messages after + // turn/start, an idle agent.inject() wraps its context/message in a one-shot + // turn, and usage/error are only appended inside an open turn. A `default` + // (not an enumerated list) is deliberate: SessionEventMap is + // merge-extensible, so a PLUGIN-added event type appended while idle must + // also fail here rather than fall through and be dropped on resume. + default: { + if (trace.openTurn === null) { + throw new InvariantError(`${event.type} appended outside any open turn (every event must be turn-enclosed)`) + } + break + } } } diff --git a/packages/invariants/tests/invariants.spec.ts b/packages/invariants/tests/invariants.spec.ts index bbf93f44dc..13f908b451 100644 --- a/packages/invariants/tests/invariants.spec.ts +++ b/packages/invariants/tests/invariants.spec.ts @@ -85,6 +85,38 @@ describe('session-log invariants', () => { .toThrow(/open is turn 1\/step null/) }) + it('rejects a message event appended outside any open turn (turn-enclosure)', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + // No turn open: every message-bearing event must be turn-enclosed (ADR 0017). + expect(() => session.append('user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } })) + .toThrow(/outside any open turn/) + expect(() => session.append('context/message', { content: [{ type: 'text', text: 'ctx' }], source: { kind: 'user' } })) + .toThrow(/outside any open turn/) + }) + + it('rejects usage/error and plugin-added events appended outside any open turn', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + // usage and error are turn-scoped: outside a turn they would land past the + // commit boundary and be dropped on resume (ADR 0017). + expect(() => session.append('usage', { turn: 1, step: 1, usage: { inputTokens: 1, outputTokens: 1 } })) + .toThrow(/outside any open turn/) + expect(() => session.append('error', { turn: 1, step: 1, message: 'boom' })) + .toThrow(/outside any open turn/) + // A PLUGIN-added (merge-extensible) event type is caught by the default too. + expect(() => session.append('compaction/marker' as never, { foo: 'bar' } as never)) + .toThrow(/outside any open turn/) + }) + + it('accepts message events once a turn is open', async () => { + const { ctx } = await setup({ freeze: false }) + const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) + expect(() => session.append('user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } })) + .not.toThrow() + }) + it('rejects a tool/result with no prior tool/call', async () => { const { ctx } = await setup({ freeze: false }) const session = ctx.sessions.create() @@ -211,6 +243,7 @@ describe('dev-freeze', () => { it('freezes appended event data so mutating a logged event throws', async () => { const { ctx } = await setup() // freeze defaults true const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) const event = session.append('user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } }) expect(Object.isFrozen(event)).toBe(true) expect(Object.isFrozen(event.data)).toBe(true) @@ -221,6 +254,7 @@ describe('dev-freeze', () => { it('does not freeze when freeze:false', async () => { const { ctx } = await setup({ freeze: false }) const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) const event = session.append('user/message', { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } }) expect(Object.isFrozen(event)).toBe(false) }) @@ -228,7 +262,8 @@ describe('dev-freeze', () => { it('freezes seeded events on session/created', async () => { const { ctx } = await setup() const seed = [ - { type: 'user/message' as const, seq: 0, time: 0, data: { content: [{ type: 'text' as const, text: 'seeded' }], source: { kind: 'user' as const } } }, + { type: 'turn/start' as const, seq: 0, time: 0, data: { turn: 1, trigger: { kind: 'message' as const, source: { kind: 'user' as const } } } }, + { type: 'user/message' as const, seq: 1, time: 0, data: { content: [{ type: 'text' as const, text: 'seeded' }], source: { kind: 'user' as const } } }, ] const session = ctx.sessions.create(undefined, { seed }) expect(Object.isFrozen(session.events[0])).toBe(true) @@ -237,6 +272,7 @@ describe('dev-freeze', () => { it('freezes mutable descendants of a shallow-frozen event datum', async () => { const { ctx } = await setup() const session = ctx.sessions.create() + session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) // A caller hands in a SHALLOW-frozen block whose nested array is still // mutable. deepFreeze must descend into the already-frozen object and // freeze the descendant, not short-circuit on the frozen container — @@ -257,7 +293,7 @@ describe('dev-freeze', () => { // non-serializable (incl. cyclic) data at the source, so drive the freeze // handler directly via hand-built session/events — exactly the shape the // invariants listener receives. Open a turn first (seq 0) so the cyclic - // user/message (seq 1) satisfies seq-contiguity. + // user/message (seq 1) satisfies the turn-enclosure invariant. ctx.emit('session/event', session, { type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } } as never) const cyclic: Record = { type: 'text', text: 'x' } cyclic['self'] = cyclic