From c6d2eeea6b1b510ca9a9ec66dd3f1722eae6acd3 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:36:40 +0800 Subject: [PATCH 1/8] refactor(events): remove the agent/steering mirror emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agent/steering was the last transient mirror of a durable session event: drainSteering appended the durable steering/message {turn, content, source} and emitted the identical fact one line later. Zero production listeners existed — every steering consumer (hook bridges, goldens, deriveMessages) reads the durable event — and the one regression test subscriber asserted a fact the log already records. Remove the declaration (dsh-agent types + JSDoc list + README row), the emit in drainSteering (its ctx parameter goes too), and the emit line in the loop-pseudocode blocks (loop.ts module doc, architecture.md); the cordis catalog is regenerated. The regression test now pins source preservation on the durable steering/message event. Live-notification needs keep their surviving homes: agent/queued at enqueue time, session/event at drain time. RFC: docs/rfc/implemented/simplification/2026-07-04-remove-agent-steering-mirror.md (moved from proposed/, amended to shipped reality). The three implemented RFCs that stated the retention — the boundary-mirror removal, the stream-chunk removal, and event-domain-semantics — are amended to point at that RFC as the record of the removal, per implemented/AGENTS.md. The rejected retire-mid-turn-steering RFC keeps its frozen text (it records the declined proposal); the steering capability itself — steer(), the durable event, continuation forcing — is untouched. --- docs/architecture.md | 2 +- docs/cordis-catalog/events-and-services.md | 14 +-------- docs/rfc/README.md | 2 +- .../2026-06-30-event-domain-semantics.md | 4 +-- ...-20-remove-agent-boundary-mirror-events.md | 11 +++---- .../2026-07-02-remove-stream-chunk-mirror.md | 2 +- ...2026-07-04-remove-agent-steering-mirror.md | 30 +++++++++++++++++++ ...2026-07-04-remove-agent-steering-mirror.md | 28 ----------------- packages/core/agent-loop/src/loop.ts | 9 +++--- .../agent-loop/tests/review-fixes.spec.ts | 7 +++-- packages/core/agent/README.md | 3 +- packages/core/agent/src/types.ts | 9 ++---- 12 files changed, 53 insertions(+), 68 deletions(-) create mode 100644 docs/rfc/implemented/simplification/2026-07-04-remove-agent-steering-mirror.md delete mode 100644 docs/rfc/proposed/simplification/2026-07-04-remove-agent-steering-mirror.md diff --git a/docs/architecture.md b/docs/architecture.md index 0266f1a2e9..23e6f53db4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -176,7 +176,7 @@ forever: session('tool/result') append buffered post-execute additionalContext → session('context/message')(s) ⟵ after ALL tool/results (adjacency) - drain steering → session('steering/message'); emit agent/steering + drain steering → session('steering/message') session('step/end') ⟵ durable step boundary (no agent/* mirror) cont = waterfall agent/turn-continuation(default = {action: hadToolCalls||steered ? 'continue' : 'stop'}) → ContinuationDecision diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index ac7ea069b9..d53f52cdbd 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -49,7 +49,7 @@ A step or turn errored. The loop reports a failure here (plus the logger) even w Types: [Agent](../core-data-structures/core.md) -Source: [`packages/core/agent/src/types.ts:353`](../../packages/core/agent/src/types.ts) +Source: [`packages/core/agent/src/types.ts:348`](../../packages/core/agent/src/types.ts) #### `agent/pre-step` — serial @@ -125,18 +125,6 @@ Types: [Agent](../core-data-structures/core.md) Source: [`packages/core/agent/src/types.ts:246`](../../packages/core/agent/src/types.ts) -#### `agent/steering` — emit - -Steering content was injected into a running turn. - -```ts cordis-catalog -'agent/steering'(agent: Agent, turn: number, content: ContentBlock[], source: MessageSource): void -``` - -Types: [Agent](../core-data-structures/core.md) · [ContentBlock](../core-data-structures/core.md) · [MessageSource](../core-data-structures/core.md) - -Source: [`packages/core/agent/src/types.ts:347`](../../packages/core/agent/src/types.ts) - #### `agent/step-result` — waterfall Waterfall: post-process the assembled assistant Message before tool dispatch (validation, content rewriting, …). diff --git a/docs/rfc/README.md b/docs/rfc/README.md index d53684bf6b..64f3aa6551 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -59,7 +59,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Prune dead core-spine surface — `SurfaceManager.invalidate()`, the loop-internal exports, `ToolExecutionResult.callId`](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | | [Prune producer-less vocabulary variants (block cache hints, the `agent` message source, the `continuation` turn trigger)](proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md) | 2026-07-04 | | [Prune write-only fields and a dead routing knob from the fs seam](proposed/simplification/2026-07-04-prune-write-only-fs-surface.md) | 2026-07-04 | -| [Remove the `agent/steering` mirror emit](proposed/simplification/2026-07-04-remove-agent-steering-mirror.md) | 2026-07-04 | | [Share the app bins' boot glue instead of maintaining twin copies](proposed/simplification/2026-07-04-share-app-bin-boot-glue.md) | 2026-07-04 | | [Tighten the hook-protocol contract — dialect, discarded fields, double defaults, and lib-owned `hook/result` semantics](proposed/simplification/2026-07-04-tighten-hook-protocol-contract.md) | 2026-07-04 | | [Trim unreachable ACP bridge surface — the branding knobs and the kind-sniffing fallback](proposed/simplification/2026-07-04-trim-acp-bridge-unreachable-surface.md) | 2026-07-04 | @@ -120,6 +119,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Stop mirroring durable boundaries as agent events](implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | | [Split the filesystem seam — provider text mutations plus the `dsh-fs-policy` plugin](implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) | 2026-06-26 | | [Stop mirroring the token stream as an agent event](implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) | 2026-07-02 | +| [Remove the `agent/steering` mirror emit](implemented/simplification/2026-07-04-remove-agent-steering-mirror.md) | 2026-07-04 | ### Architecture diff --git a/docs/rfc/implemented/architecture/2026-06-30-event-domain-semantics.md b/docs/rfc/implemented/architecture/2026-06-30-event-domain-semantics.md index 8f5be09b2b..9a9db6359f 100644 --- a/docs/rfc/implemented/architecture/2026-06-30-event-domain-semantics.md +++ b/docs/rfc/implemented/architecture/2026-06-30-event-domain-semantics.md @@ -19,7 +19,7 @@ This is the foundational change in a stack that adds a Hooks subsystem; it estab **Three domains, one job each, with a single boundary rule.** - **`session/*` — the durable, replayable FACT log.** Owns `SessionEventMap`; every entry is JSON-only (no live objects). One `session/event` emit per append, plus the `session/flush` parallel durability checkpoint. It is also the live transcript feed: a consumer that wants to render or react to what happened subscribes here, so live rendering and `session/load` replay share one path. -- **`agent/*` — the LIVE runtime surface.** Always carries the live `Agent`. Two shapes: INTERCEPTION waterfalls (`agent/request`, `agent/step-result`, `agent/turn-continuation`) that mutate or veto, and TRANSIENT emits (`agent/status`, `agent/error`, `agent/created`/`agent/disposed`, `agent/queued`, `agent/steering`) that notify with the `Agent` in hand. Turn and step BOUNDARIES are NOT here — they are durable session events read off `session/event`, and so is the token stream (`assistant/chunk`). +- **`agent/*` — the LIVE runtime surface.** Always carries the live `Agent`. Two shapes: INTERCEPTION waterfalls (`agent/request`, `agent/step-result`, `agent/turn-continuation`) that mutate or veto, and TRANSIENT emits (`agent/status`, `agent/error`, `agent/created`/`agent/disposed`, `agent/queued`) that notify with the `Agent` in hand. Turn and step BOUNDARIES are NOT here — they are durable session events read off `session/event`, and so are the token stream (`assistant/chunk`) and mid-turn steering (`steering/message`). - **`tools/*` — the tool registry + execution seam.** **The boundary rule:** a durable, replayable fact is a `SessionEvent`; a live interception or a transient/live-object signal is an `agent`/`tools` Cordis event. A turn or step boundary is a durable fact, so it lives in the session log and is read off the `session/event` feed — it is NOT mirrored as an `agent/*` emit. @@ -31,5 +31,5 @@ This is the foundational change in a stack that adds a Hooks subsystem; it estab - The loop no longer emits any boundary mirror; `closeStep` appends `step/end` only and `closeTurn` appends `turn/end` only. A throwing `step/end`/`turn/end` session-event listener is the surviving boundary-listener failure path (contained inside `closeStep`/`closeTurn` — `Session.append` pushes the event before notifying listeners, so the boundary is durable and the turn closes balanced regardless). - Tests that observed boundaries via the removed emits now observe the durable `turn/start`/`turn/end`/`step/start`/`step/end` session events — the behavior they pin (boundary ordering, step counting) is unchanged; only the feed they read moved to the canonical one. The tests that exercised a *throwing turn-boundary emit listener* were deleted, because that code path no longer exists (there is no emit to throw from). Per [AGENTS.md "tests document behavior, not golden truth"](../../../../AGENTS.md), the behavior and its test moved (or died) together. - The loop marks the step open (`stepOpen = true`) BEFORE appending `step/start`, because `Session.append` pushes the event to the log before notifying `session/event` listeners (validation throws happen earlier, before the push — see [the session append contract](../../../core-data-structures/session.md)). So a throwing `step/start` session-event listener runs with the step already open and the event already in the log: the loop's outer catch then calls `closeStep()`, which appends the balancing `step/end`, and the turn closes balanced with an error (`turn/start → step/start → step/end → turn/end` — verified by the invariants oracle in the regression test). Closing the open step is owed precisely because the marker is set first. -- The full realization of this is [the simplification RFC "Stop mirroring durable boundaries as agent events"](../simplification/2026-06-20-remove-agent-boundary-mirror-events.md): all four boundary mirrors are removed and every consumer reads boundaries off `session/event`. `agent/steering` (a live control signal, not a boundary mirror) is retained; see that RFC's scope section. +- The full realization of this is [the simplification RFC "Stop mirroring durable boundaries as agent events"](../simplification/2026-06-20-remove-agent-boundary-mirror-events.md): all four boundary mirrors are removed and every consumer reads boundaries off `session/event`. `agent/steering` (not a boundary mirror) stayed outside that RFC's scope and was removed by its own follow-up, [Remove the `agent/steering` mirror emit](../simplification/2026-07-04-remove-agent-steering-mirror.md) — it mirrored the durable `steering/message`. - The cordis catalog (`docs/cordis-catalog/events-and-services.md`) is regenerated to drop the mirror events. diff --git a/docs/rfc/implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md b/docs/rfc/implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md index d8ff017d63..f198ff9ec6 100644 --- a/docs/rfc/implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md +++ b/docs/rfc/implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md @@ -5,10 +5,11 @@ Status: implemented (accepted 2026-07-01) + The original proposal bundled `agent/steering` into the removal; keeping it + out kept this RFC's scope to boundaries. Each retained event was later + removed by its own decision — see + [Stop mirroring the token stream as an agent event](2026-07-02-remove-stream-chunk-mirror.md) + and [Remove the `agent/steering` mirror emit](2026-07-04-remove-agent-steering-mirror.md). --> ## Problem @@ -30,7 +31,7 @@ Removed (durable-boundary mirrors — the session log is authoritative for each) RETAINED — NOT durable-boundary mirrors, so out of scope for this decision: -- `agent/steering` — a live control signal, not a boundary. (The original proposal bundled it into the removal; validating against the code, it is not a duplicate of a durable boundary, so removing it here would have been scope creep. Its fate is a separate future decision.) +- `agent/steering` — not a boundary, so out of scope for THIS decision (the original proposal bundled it into the removal; that would have been scope creep here). It mirrors the durable `steering/message` control record rather than a boundary, and was removed by its own follow-up: [Remove the `agent/steering` mirror emit](2026-07-04-remove-agent-steering-mirror.md). - `agent/stream-chunk` — the live token stream. Out of scope for THIS decision (a mirror of the durable `assistant/chunk`, not a boundary), it was removed by its own follow-up: [Stop mirroring the token stream as an agent event](2026-07-02-remove-stream-chunk-mirror.md). - `agent/created`, `agent/disposed`, `agent/status`, `agent/error`, `agent/queued` — lifecycle/control events that are not transcript data. `agent/queued` in particular is an inbox acknowledgement that fires before any durable event exists (cancelled queued work may never enter the log), so it is deliberately live-only. diff --git a/docs/rfc/implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md b/docs/rfc/implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md index b2ed1bc5d4..73c62a6864 100644 --- a/docs/rfc/implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md +++ b/docs/rfc/implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md @@ -33,7 +33,7 @@ Removed: `agent/stream-chunk`. Not touched: - `assistant/chunk` (the durable session event) — the authoritative token stream, kept exactly as-is. This RFC removes the LIVE MIRROR, not the persistence (the persistence-removal proposal was separately rejected — see above). -- `agent/steering` — a live control signal with no durable twin, retained (its fate remains a separate future decision, per the boundary RFC). +- `agent/steering` — not touched by THIS decision (a control signal, not the token stream). Its durable twin is `steering/message`, and the mirror emit was removed by its own follow-up: [Remove the `agent/steering` mirror emit](2026-07-04-remove-agent-steering-mirror.md). - `agent/status`, `agent/error`, `agent/created`/`agent/disposed`, `agent/queued`, `agent/session-start` — lifecycle/control events that are not transcript data and have no durable duplicate. ## What we give up diff --git a/docs/rfc/implemented/simplification/2026-07-04-remove-agent-steering-mirror.md b/docs/rfc/implemented/simplification/2026-07-04-remove-agent-steering-mirror.md new file mode 100644 index 0000000000..a8a5edd3a2 --- /dev/null +++ b/docs/rfc/implemented/simplification/2026-07-04-remove-agent-steering-mirror.md @@ -0,0 +1,30 @@ +# RFC: Remove the `agent/steering` mirror emit + +Status: implemented (accepted 2026-07-04) + +## Problem + +`agent/steering` was the last remaining transient mirror of a durable session event. The loop's steering drain appends the durable `steering/message { turn, content, source }` and, on the very next line, emitted `agent/steering(agent, turn, content, source)` — the identical fact as a fire-and-forget event (`packages/core/agent-loop/src/loop.ts`, `drainSteering`). It had zero production listeners: the only subscriber anywhere was a loop regression test asserting the emit carried `source` — the same fact the durable event already records one line above. + +Both mirror-removal RFCs retained it while explicitly deferring the decision this RFC makes. The [boundary-mirror removal](2026-06-20-remove-agent-boundary-mirror-events.md) kept it as a live control signal rather than a boundary; the [stream-chunk removal](2026-07-02-remove-stream-chunk-mirror.md) retained it on the reading that it had no durable twin. The second rationale did not survive the code: the durable twin is `steering/message`, appended immediately before the emit with the same payload. The mirrored-vs-live-only line the taxonomy actually draws puts it on the mirror side: `agent/queued` is genuinely live-only (it fires at enqueue time, before any durable event exists, and already carries a `steering: boolean` flag — cancelled queued work never enters the log), while `agent/steering` fired at the exact moment its durable twin landed, carrying nothing the log does not. + +Steering carries real production traffic — the hook bridges' turn-continuation decisions inject their reasons through `inbox.steer()`, landing as durable `steering/message` events that the hook-matrix goldens pin — and every one of those consumers observes the durable event. Nothing observed the mirror. + +## Decision + +`agent/steering` is removed from the agent event taxonomy: the declaration in `packages/core/agent/src/types.ts` (and its mention in the live-events JSDoc list there), the emit in `drainSteering` (whose then-unused `ctx` parameter went with it), the row in `packages/core/agent/README.md`, and the emit line in the loop-pseudocode blocks (the `packages/core/agent-loop/src/loop.ts` module doc and [architecture.md](../../../architecture.md)); the cordis catalog is regenerated without it. The one regression test pins source preservation on the durable `steering/message` event — the fact it pins lives on the log. + +Three implemented RFCs stated the retention, and each is amended per [implemented/AGENTS.md](../AGENTS.md) to point here as the record of the removal: the [boundary RFC](2026-06-20-remove-agent-boundary-mirror-events.md)'s retained-list entry, the [stream-chunk RFC](2026-07-02-remove-stream-chunk-mirror.md)'s scope clause, and the [event-domain-semantics RFC](../architecture/2026-06-30-event-domain-semantics.md)'s transient-emit enumeration. + +## Why not keep it? + +"It is a control signal, not a boundary" — but the taxonomy's operative distinction is mirrored-vs-live-only, not control-vs-boundary, and this event mirrored. A consumer that wants enqueue-time notification has `agent/queued` (with its steering flag); a consumer that wants drain-time notification is by definition asking for the moment `steering/message` is appended, which `session/event` delivers with the same payload plus durability. The rejected [retire-mid-turn-steering RFC](../../rejected/simplification/2026-06-20-retire-mid-turn-steering.md) defended the steering *capability* — `steer()`, the durable event, continuation forcing — all of which this removal keeps untouched. + +## Acceptance criteria + +- The `agent/steering` spelling survives only in RFC prose (this RFC, the three amended RFCs above, and the frozen [rejected steering-capability RFC](../../rejected/simplification/2026-06-20-retire-mid-turn-steering.md), whose text records the proposal it declined); the catalog is regenerated and fresh. +- The retargeted test pins source preservation on `steering/message`; the suite is green. + +## Risks + +None known: zero production listeners existed to migrate, and both live-notification needs (enqueue, drain) have surviving homes (`agent/queued`, `session/event`). diff --git a/docs/rfc/proposed/simplification/2026-07-04-remove-agent-steering-mirror.md b/docs/rfc/proposed/simplification/2026-07-04-remove-agent-steering-mirror.md deleted file mode 100644 index 579401cb75..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-remove-agent-steering-mirror.md +++ /dev/null @@ -1,28 +0,0 @@ -# RFC: Remove the `agent/steering` mirror emit - -Status: proposed - -## Problem - -`agent/steering` is the last remaining transient mirror of a durable session event. The loop's steering drain appends the durable `steering/message { turn, content, source }` and, on the very next line, emits `agent/steering(agent, turn, content, source)` — the identical fact as a fire-and-forget event (`packages/core/agent-loop/src/loop.ts`, `drainSteering`). It has zero production listeners: the only subscriber anywhere is a loop regression test asserting the emit carries `source` — the same fact the durable event already records one line above. - -Both mirror-removal RFCs retained it while explicitly deferring the decision this RFC now makes. The [boundary-mirror removal](../../implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) kept it as "a live control signal, not a boundary"; the [stream-chunk removal](../../implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) kept it as "a live control signal with no durable twin, retained (its fate is a separate future decision)". The second rationale does not survive the code: the durable twin is `steering/message`, appended immediately before the emit with the same payload. The mirrored-vs-live-only line the taxonomy actually draws puts it on the mirror side: `agent/queued` is genuinely live-only (it fires at enqueue time, before any durable event exists, and already carries a `steering: boolean` flag — cancelled queued work never enters the log), while `agent/steering` fires at the exact moment its durable twin lands, carrying nothing the log does not. - -Steering carries real production traffic — the hook bridges' turn-continuation decisions inject their reasons through `inbox.steer()`, landing as durable `steering/message` events that the hook-matrix goldens pin — and every one of those consumers observes the durable event. Nothing observes the mirror. - -## Proposal - -Remove the `agent/steering` declaration from `packages/core/agent/src/types.ts` (and its mention in the live-events JSDoc list there), the emit in `drainSteering` (whose `ctx` parameter becomes unused and goes too), the row in `packages/core/agent/README.md`, and the emit line in the loop-pseudocode blocks (`packages/core/agent-loop/src/loop.ts` module doc and [architecture.md](../../../architecture.md)); run `pnpm run gen-cordis-catalog`. Retarget the one regression test at the durable `steering/message` event — the source-preservation fact it pins lives on the log. The implementing PR amends the two retaining RFCs' scope lines per [implemented/AGENTS.md](../../implemented/AGENTS.md): the boundary RFC's retained-list entry and the stream-chunk RFC's "no durable twin" clause. - -## Why not keep it? - -"It is a control signal, not a boundary" — but the taxonomy's operative distinction is mirrored-vs-live-only, not control-vs-boundary, and this event mirrors. A consumer that wants enqueue-time notification has `agent/queued` (with its steering flag); a consumer that wants drain-time notification is by definition asking for the moment `steering/message` is appended, which `session/event` delivers with the same payload plus durability. The rejected [retire-mid-turn-steering RFC](../../rejected/simplification/2026-06-20-retire-mid-turn-steering.md) defended the steering *capability* — `steer()`, the durable event, continuation forcing — all of which this removal keeps untouched. - -## Acceptance criteria - -- No `agent/steering` spelling outside this RFC and the two amended RFCs; the catalog is regenerated and fresh. -- The retargeted test pins source preservation on `steering/message`; the suite is green. - -## Risks - -None known: zero production listeners exist to migrate, and both live-notification needs (enqueue, drain) have surviving homes (`agent/queued`, `session/event`). diff --git a/packages/core/agent-loop/src/loop.ts b/packages/core/agent-loop/src/loop.ts index 56c6cc1863..ef5c50b7ce 100644 --- a/packages/core/agent-loop/src/loop.ts +++ b/packages/core/agent-loop/src/loop.ts @@ -166,7 +166,7 @@ export interface LoopHandle { * → dispatch → tools/post-execute * session('tool/result') * append buffered post-execute additionalContext → session('context/message')(s) - * drain steering → session('steering/message'); emit agent/steering + * drain steering → session('steering/message') * session('step/end') ⟵ durable step boundary (no agent/* mirror) * cont = waterfall agent/turn-continuation ⟵ ContinuationDecision; default * {action: hadToolCalls||steered ? 'continue':'stop'}; a continue.reason is @@ -420,7 +420,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle, // Steering from the previous round's continuation listeners joins before // the request. - drainSteering(ctx, agent, turn) + drainSteering(agent, turn) // The step's AbortController exists BEFORE any async pre-step work so a // dispose() or cancel() — in a synchronous turn-start listener or an @@ -529,7 +529,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle, if (stepReason) reason = stepReason // Steering that arrived during streaming/tool execution. - const steered = drainSteering(ctx, agent, turn) + const steered = drainSteering(agent, turn) if (closeStep()) break @@ -635,11 +635,10 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle, } /** Drain the steering queue into the session. Returns whether any arrived. */ -function drainSteering(ctx: Context, agent: ReactLoopAgent, turn: number): boolean { +function drainSteering(agent: ReactLoopAgent, turn: number): boolean { const messages = agent.inbox.drainSteering() for (const message of messages) { agent.session.append('steering/message', { turn, content: message.content, source: message.source }, { surfaceOp: 'append' }) - ctx.emit('agent/steering', agent, turn, message.content, message.source) } return messages.length > 0 } diff --git a/packages/core/agent-loop/tests/review-fixes.spec.ts b/packages/core/agent-loop/tests/review-fixes.spec.ts index eddc69a2e6..76c377fd61 100644 --- a/packages/core/agent-loop/tests/review-fixes.spec.ts +++ b/packages/core/agent-loop/tests/review-fixes.spec.ts @@ -410,7 +410,7 @@ describe('MEDIUM: misc registry and config fixes', () => { expect(agent.session.deriveMessages().at(-1)?.content).toEqual([{ type: 'text', text: 'routed' }]) }) - it('agent/queued carries the resolved source; agent/steering carries its source', async () => { + it('agent/queued carries the resolved source; steering/message records its source', async () => { const adapter = new MockAdapter([toolCallResponse('c1', 'noop', {}), textResponse('done')]) const ctx = await harness(adapter) const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) @@ -425,15 +425,16 @@ describe('MEDIUM: misc registry and config fixes', () => { })) const queuedSources: { source: MessageSource; steering: boolean }[] = [] - const steeringSources: MessageSource[] = [] ctx.on('agent/queued', (_agent, _content, info) => void queuedSources.push(info)) - ctx.on('agent/steering', (_agent, _turn, _content, source) => void steeringSources.push(source)) send(agent, 'go') // no explicit source → default {kind:'user'} must be visible await waitForIdle(ctx, agent) expect(queuedSources[0]).toEqual({ source: { kind: 'user' }, steering: false }) expect(queuedSources[1]).toEqual({ source: { kind: 'plugin', plugin: 'goal' }, steering: true }) + // The drain appends the durable steering/message with the caller's source + // intact — the log, not a transient emit, is where consumers read it. + const steeringSources = agent.session.events.flatMap(e => e.type === 'steering/message' ? [e.data.source] : []) expect(steeringSources).toEqual([{ kind: 'plugin', plugin: 'goal' }]) }) }) diff --git a/packages/core/agent/README.md b/packages/core/agent/README.md index f4eb61c2bd..a6446bf7d1 100644 --- a/packages/core/agent/README.md +++ b/packages/core/agent/README.md @@ -50,9 +50,8 @@ Turn and step boundaries are NOT mirrored as `agent/*` emits: a consumer that ne Tool interception is the `tools/pre-execute` / `tools/post-execute` pair in [`dsh-tools`](../tools/README.md) (`PreToolDecision` allow/deny/ask, `PostToolDecision` accept/block) — same typed-Decision idiom, owned there because it is the tool registry's seam. -#### Live control notifications (emit) +#### Error notifications (emit) -- `agent/steering` — steering content injected mid-turn - `agent/error` — step/turn error The model's token stream is NOT an `agent/*` event: read it off the durable `session/event` feed as `assistant/chunk` (the same feed persistence and the ACP bridge use). diff --git a/packages/core/agent/src/types.ts b/packages/core/agent/src/types.ts index dd01831130..e869103c7c 100644 --- a/packages/core/agent/src/types.ts +++ b/packages/core/agent/src/types.ts @@ -20,7 +20,7 @@ * `agent/request`/`agent/step-result`/`agent/turn-continuation` waterfalls and * the serial `agent/pre-step`) that mutate/veto, and TRANSIENT emits * (`agent/status`, `agent/error`, `agent/created`/ - * `agent/disposed`, `agent/queued`, `agent/steering`, `agent/session-start`) + * `agent/disposed`, `agent/queued`, `agent/session-start`) * that notify with the `Agent` in hand. Turn/step boundaries are NOT here — * they are durable `session/event` records. Answers "right now, with the agent * object — intercept or observe." @@ -339,12 +339,7 @@ declare module 'cordis' { */ 'agent/turn-continuation'(agent: Agent, turn: number, defaultDecision: ContinuationDecision, next: () => Promise): Promise - // ---- streaming + tool notifications (emit) ---- - /** - * Steering content was injected into a running turn. - * @mode emit - */ - 'agent/steering'(agent: Agent, turn: number, content: ContentBlock[], source: MessageSource): void + // ---- error notifications (emit) ---- /** * A step or turn errored. The loop reports a failure here (plus the logger) * even when the error has no in-turn position for a session `error` event. From de79553d4f841f247ac0abaf3ce3ae87938720e8 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:42:56 +0800 Subject: [PATCH 2/8] refactor(web): drop the unconsumed observation surface WebService exposed an observation surface nothing in production observes: the web/providers-change event (declared, emitted on every provider registration/disposal, rollback-yield ordered before the emit solely so a throwing change listener unwinds the registration) and the aggregated searchStatus()/fetchStatus() query with its WebCapabilityStatus union. dsh-tool-web executes through ctx.web.search()/fetch() and routes on the structured WebError codes selection throws at execution time; tool registration follows product enablement, not provider availability. The only listeners/callers were the web packages' own tests, and the tool-web README / architecture.md prose claiming the tool 'reads only the aggregated searchStatus()/fetchStatus()' had drifted from the call sites. Remove the event declaration, both emits, and the rollback-before-emit machinery (the plain ctx.effect disposer keeps HMR cleanup, matching LlmService.registerAdapter). Remove searchStatus()/fetchStatus(), resolveStatus(), and WebCapabilityStatus; the provider-private status() stays as the execution-time selection input. Delete the listener-throw rollback test, and rewrite every event/status assertion across the web packages' tests onto caller-observable behavior: a successful search()/fetch() or the structured WEB_PROVIDER_* codes. Regenerate the cordis catalog; update the web/tool-web READMEs, the architecture.md web paragraph, core-data-structures/web.md, and the type-equiv manifest; amend the web capability seam RFC's facts to the shipped surface. This follows the llm/adapter-change precedent: a boot-time backend-registry signal and an availability probe distinct from executing both sit on the cut side of its keep/cut criterion. RFC: docs/rfc/implemented/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md --- docs/architecture.md | 2 +- docs/cordis-catalog/events-and-services.md | 26 +--- docs/core-data-structures/web.md | 16 +-- docs/rfc/README.md | 2 +- .../2026-06-24-web-capability-seam.md | 57 ++++---- ...drop-unconsumed-web-observation-surface.md | 6 +- packages/web/tool-web/README.md | 2 +- packages/web/tool-web/tests/tool-web.spec.ts | 7 +- .../web-fetch-local/tests/fetch-local.spec.ts | 9 +- .../tests/deepseek.spec.ts | 13 +- packages/web/web-search-exa/tests/exa.spec.ts | 10 +- .../tests/perplexity.spec.ts | 10 +- packages/web/web/README.md | 23 ++-- packages/web/web/src/index.ts | 100 +++----------- packages/web/web/src/types.ts | 22 +--- packages/web/web/tests/web.spec.ts | 124 ++++++------------ scripts/type-equiv.manifest.json | 3 +- 17 files changed, 146 insertions(+), 286 deletions(-) rename docs/rfc/{proposed => implemented}/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md (79%) diff --git a/docs/architecture.md b/docs/architecture.md index 0266f1a2e9..3db26e4840 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -87,7 +87,7 @@ The LLM seam has the same topology folded differently: `dsh-llm` carries the int The filesystem capability follows the bash topology with a fourth layer, but the policy is contributed through an **event gate**, not a method service: `dsh-fs` owns the abstract `ctx.fs` provider seam (text IO + atomic mutation primitives whose version guard is optional) and the `fs/*` policy event vocabulary, `dsh-fs-local` provides the local backend, `dsh-tool-fs` is the model-facing `read`/`write`/`edit` tools AND the executor (it reads/writes/edits through `ctx.fs` directly, owns read windowing, dispatches the `fs/*` events), and `dsh-fs-policy` is a policy PLUGIN (no service) that decides the `fs/write-intent`/`fs/edit-intent` waterfalls and records on `fs/observed` to add observed-state + read-before-edit + version-guarded write/edit. Because the tool is not method-coupled to the policy, dropping `dsh-fs-policy` gracefully loses the policy and leaves the unconstrained bare provider rather than breaking the tool at a service-injection boundary. The demo agents (`coding-agent`, `acp-agent`) wire the full stack — `dsh-fs-local` + `dsh-fs-policy` + `dsh-tool-fs` — so `read`/`write`/`edit` are the default file surface (bash stays for shell/tests/search); the tools resolve a relative path against the caller's session cwd, matching bash ([the per-session cwd RFC](rfc/implemented/architecture/2026-07-02-fs-per-session-cwd.md)). See [the fs-policy event-gate RFC](rfc/implemented/architecture/2026-06-26-file-context-as-event-gate.md). -The web capability uses the same three-package split but folds two capabilities onto one seam: `dsh-web` owns the abstract `ctx.web` service, which is a provider REGISTRY (`registerSearchProvider`/`registerFetchProvider`, registration-order-independent selection, the `WebError` taxonomy) rather than a single backend. Providers register capabilities, not tools — `dsh-web-search-exa`, `dsh-web-search-perplexity`, `dsh-web-search-deepseek`, and `dsh-web-fetch-local` each register into `ctx.web` the way an `LlmAdapter` registers into `ctx.llm`, so they are namespace plugins (`inject: ['web']`), not key-owning services. `dsh-tool-web` is the single consumer that owns the model-facing `web_search`/`web_fetch` schemas, prompt sections, and presentation; it reads only the aggregated `ctx.web.searchStatus()`/`fetchStatus()` and executes through `ctx.web.search()`/`fetch()`, so provider selection has one owner. Search and fetch are deliberately one seam (one thing to inject and configure, one selection policy, one abort/error vocabulary) despite sharing no request schema — see the [web capability seam RFC](rfc/implemented/architecture/2026-06-24-web-capability-seam.md). +The web capability uses the same three-package split but folds two capabilities onto one seam: `dsh-web` owns the abstract `ctx.web` service, which is a provider REGISTRY (`registerSearchProvider`/`registerFetchProvider`, registration-order-independent selection, the `WebError` taxonomy) rather than a single backend. Providers register capabilities, not tools — `dsh-web-search-exa`, `dsh-web-search-perplexity`, `dsh-web-search-deepseek`, and `dsh-web-fetch-local` each register into `ctx.web` the way an `LlmAdapter` registers into `ctx.llm`, so they are namespace plugins (`inject: ['web']`), not key-owning services. `dsh-tool-web` is the single consumer that owns the model-facing `web_search`/`web_fetch` schemas, prompt sections, and presentation; it executes through `ctx.web.search()`/`fetch()`, which resolve the provider at execution time and throw the structured `WebError` codes the tool surfaces, so provider selection has one owner. Search and fetch are deliberately one seam (one thing to inject and configure, one selection policy, one abort/error vocabulary) despite sharing no request schema — see the [web capability seam RFC](rfc/implemented/architecture/2026-06-24-web-capability-seam.md). > **"Capability" — two unrelated meanings.** (1) The *seam pattern* above ("one plugin provides a capability, another needs it") is realized by plain Cordis **services + `inject`**: a provider registers a service (`ctx.bash`, declared in `interface Context`); a consumer declares `inject: ['bash']` and its fiber stays pending until the service exists, tearing down via HMR if it later vanishes. No extra library is needed. (2) `@cordisjs/plugin-capability` is a different axis entirely — a **permission/capability-security** service (named permissions with inheritance/dependency, tested against a session via `ctx.capability.test`). It is a candidate for the deferred permissions/sandbox work (the `tools/pre-execute` deny/ask gate), NOT a mechanism for swapping implementations. diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index ac7ea069b9..fdaee10bac 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -327,18 +327,6 @@ Types: [ToolExecution](../core-data-structures/tools.md) Source: [`packages/core/tools/src/index.ts:65`](../../packages/core/tools/src/index.ts) -### `web/*` - -#### `web/providers-change` — emit - -Fired after the provider registry changes — a search or fetch provider was registered or disposed. Carries no payload and no capability graph: it means only "the provider registry changed; observers may recompute status from `ctx.web`". `searchStatus()` / `fetchStatus()` stay derived, not stored. - -```ts cordis-catalog -'web/providers-change'(this: WebService): void -``` - -Source: [`packages/web/web/src/index.ts:65`](../../packages/web/web/src/index.ts) - ## Services The `ctx.` services the harness provides. An abstract seam (e.g. `ctx.bash`) is implemented by a separate package; the interface is what consumers code against. @@ -543,25 +531,23 @@ Source: [`packages/core/tools/src/index.ts:265`](../../packages/core/tools/src/i The web access service. Registered as `ctx.web` (one instance per context). -Selection semantics (identical for status and execution, never order- dependent): +Selection semantics (resolved at execution time, never order-dependent): - A configured id that is registered and `status().available` → that provider. -- A configured id not registered → `configured-missing` / `WEB_PROVIDER_CONFIGURED_MISSING`. -- A configured id registered but unavailable → `configured-unavailable` / `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`. +- A configured id not registered → `WEB_PROVIDER_CONFIGURED_MISSING`. +- A configured id registered but unavailable → `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`. - No id configured, exactly one registered usable provider → that provider. -- No id configured, multiple usable providers → `ambiguous` / `WEB_PROVIDER_AMBIGUOUS`. -- No id configured, no usable provider → `none` / `WEB_PROVIDER_UNAVAILABLE`. +- No id configured, multiple usable providers → `WEB_PROVIDER_AMBIGUOUS`. +- No id configured, no usable provider → `WEB_PROVIDER_UNAVAILABLE`. ```ts cordis-catalog registerSearchProvider(provider: WebSearchProvider): () => void registerFetchProvider(provider: WebFetchProvider): () => void -searchStatus(): WebCapabilityStatus -fetchStatus(): WebCapabilityStatus async search(request: WebSearchRequest, exec?: WebExecContext): Promise async fetch(request: WebFetchRequest, exec?: WebExecContext): Promise ``` -Source: [`packages/web/web/src/index.ts:105`](../../packages/web/web/src/index.ts) +Source: [`packages/web/web/src/index.ts:87`](../../packages/web/web/src/index.ts) ## Inherited tier (cordis core + loader/hmr/timer) diff --git a/docs/core-data-structures/web.md b/docs/core-data-structures/web.md index 1adde3bd75..cbce5bd523 100644 --- a/docs/core-data-structures/web.md +++ b/docs/core-data-structures/web.md @@ -73,9 +73,9 @@ type WebFetchBody = | { readonly kind: 'text'; readonly content: string } ``` -## Provider and capability status +## Provider status -A provider's `status()` is a cheap LOCAL check (credential presence, parseable config) and **must not make network calls**. It is an input to selection, not a health system. +A provider's `status()` is a cheap LOCAL check (credential presence, parseable config) and **must not make network calls**. It is an input to execution-time selection, not a health system: `search()`/`fetch()` read it to pick a usable provider, and a selection failure surfaces as the structured `WebError` the caller routes on — which carries the branchable detail (the missing id, the ambiguous candidate set) in its code and message. ```ts type-equiv type WebProviderStatus = @@ -83,15 +83,7 @@ type WebProviderStatus = | { readonly available: false; readonly reason: 'missing-credential' | 'misconfigured' } ``` -The service aggregates provider status into a `WebCapabilityStatus`: whether the capability has a selected usable provider, or the broad category in which selection fails. It carries the winning `providerId` on the available branch but NOT the per-reason payload (the missing id, the ambiguous set) — that branchable detail lives in the thrown `WebError`, the surface callers route on, so the same fact never gets two homes that can disagree. - -```ts type-equiv -type WebCapabilityStatus = - | { readonly available: true; readonly providerId: string } - | { readonly available: false; readonly reason: 'none' | 'configured-missing' | 'configured-unavailable' | 'ambiguous' } -``` - -Selection never depends on registration, config, or HMR order: a capability has an explicit provider id (config `searchProvider`/`fetchProvider`, or the matching env var feeding the same field), or auto-selects when exactly one usable provider is registered; multiple usable providers with no configured id is `ambiguous`, not first-wins. +Selection never depends on registration, config, or HMR order: a capability has an explicit provider id (config `searchProvider`/`fetchProvider`, or the matching env var feeding the same field), or auto-selects when exactly one usable provider is registered; multiple usable providers with no configured id is `WEB_PROVIDER_AMBIGUOUS`, not first-wins. ## Errors @@ -99,4 +91,4 @@ Selection never depends on registration, config, or HMR order: a capability has ## The service -`WebService` (`ctx.web`, defined in [`packages/web/web/src/index.ts`](../../packages/web/web/src/index.ts)) is a provider registry plus a provider-selecting execution surface, close to `LlmService`'s shape: `registerSearchProvider`/`registerFetchProvider` (duplicate ids throw `WEB_DUPLICATE_PROVIDER`, return disposers, emit `web/providers-change`), `searchStatus`/`fetchStatus` (derived, never stored), and `search`/`fetch` (resolve the provider at call time, throw a structured `WebError` when the capability cannot run). Providers issue requests with the platform-native `fetch` (Node 24), mirroring `dsh-llm-deepseek`; the `dsh-web-fetch-local` provider owns safe retrieval (http/https-only, credential rejection, byte/char/timeout/redirect caps, same-origin-only redirects with per-hop re-validation, charset decoding) while `dsh-tool-web` owns presentation (HTML→markdown). SSRF / private-network blocking is deferred (see the RFC) — until it lands, `web_fetch` must not be enabled where it can reach sensitive internal targets. +`WebService` (`ctx.web`, defined in [`packages/web/web/src/index.ts`](../../packages/web/web/src/index.ts)) is a provider registry plus a provider-selecting execution surface, close to `LlmService`'s shape: `registerSearchProvider`/`registerFetchProvider` (duplicate ids throw `WEB_DUPLICATE_PROVIDER`, return disposers) and `search`/`fetch` (resolve the provider at call time, throw a structured `WebError` when the capability cannot run). Providers issue requests with the platform-native `fetch` (Node 24), mirroring `dsh-llm-deepseek`; the `dsh-web-fetch-local` provider owns safe retrieval (http/https-only, credential rejection, byte/char/timeout/redirect caps, same-origin-only redirects with per-hop re-validation, charset decoding) while `dsh-tool-web` owns presentation (HTML→markdown). SSRF / private-network blocking is deferred (see the RFC) — until it lands, `web_fetch` must not be enabled where it can reach sensitive internal targets. diff --git a/docs/rfc/README.md b/docs/rfc/README.md index d53684bf6b..6ea49aaf67 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -54,7 +54,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | | [Drop the `image` content block until a path can honor it](proposed/simplification/2026-07-04-drop-image-content-block.md) | 2026-07-04 | | [Drop `GenerateOptions.prefill` and `ToolSchema.strict` — request knobs with no working end-to-end path](proposed/simplification/2026-07-04-drop-inert-request-knobs.md) | 2026-07-04 | -| [Drop the unconsumed web observation surface — the `providers-change` event and the status methods](proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md) | 2026-07-04 | | [Fold the stdio UI helper into the stdio app](proposed/simplification/2026-07-04-fold-stdio-ui-helper.md) | 2026-07-04 | | [Prune dead core-spine surface — `SurfaceManager.invalidate()`, the loop-internal exports, `ToolExecutionResult.callId`](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | | [Prune producer-less vocabulary variants (block cache hints, the `agent` message source, the `continuation` turn trigger)](proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md) | 2026-07-04 | @@ -120,6 +119,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Stop mirroring durable boundaries as agent events](implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | | [Split the filesystem seam — provider text mutations plus the `dsh-fs-policy` plugin](implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) | 2026-06-26 | | [Stop mirroring the token stream as an agent event](implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) | 2026-07-02 | +| [Drop the unconsumed web observation surface — the `providers-change` event and the status methods](implemented/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md) | 2026-07-04 | ### Architecture diff --git a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md index 55ada9d576..d0ca9c8b9a 100644 --- a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md +++ b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md @@ -22,7 +22,7 @@ Introduce web access as a first-class capability seam following [the capability- Providers do not register tools. Providers register capabilities. `dsh-tool-web` is the only owner of model-facing names, descriptions, prompt guidance, JSON schemas, and presentation. -Search and fetch are separate capabilities and separate model-facing tools, but they are deliberately one seam. `ctx.web` is a single web-access middle layer between provider packages on one side and the tool consumer on the other: one service to inject, one provider-selection policy owner, one abort/error vocabulary, one place a product configures "how this harness reaches the web." The two halves do not share a request schema and have no shared business logic — search normalizes provider-backed discovery into a portable result with optional answer text and citeable sources, while fetch retrieves a concrete public HTTP(S) URL and returns a status code plus bounded decoded content — but they are parallel registries on one capability surface, not two surfaces. The cost is a `WebService` whose registry/status/exec methods come in `Search`/`Fetch` pairs; that parallelism is intentional, not a missed extraction. Splitting into `dsh-search` and `dsh-fetch` is the rejected alternative below. +Search and fetch are separate capabilities and separate model-facing tools, but they are deliberately one seam. `ctx.web` is a single web-access middle layer between provider packages on one side and the tool consumer on the other: one service to inject, one provider-selection policy owner, one abort/error vocabulary, one place a product configures "how this harness reaches the web." The two halves do not share a request schema and have no shared business logic — search normalizes provider-backed discovery into a portable result with optional answer text and citeable sources, while fetch retrieves a concrete public HTTP(S) URL and returns a status code plus bounded decoded content — but they are parallel registries on one capability surface, not two surfaces. The cost is a `WebService` whose registry/exec methods come in `Search`/`Fetch` pairs; that parallelism is intentional, not a missed extraction. Splitting into `dsh-search` and `dsh-fetch` is the rejected alternative below. `dsh-tool-web` should register model-facing web tools when the product has enabled those tools and the `ctx.web` seam is present. Backend availability is an execution-time concern, not a schema-registration concern: @@ -33,7 +33,7 @@ Search and fetch are separate capabilities and separate model-facing tools, but This keeps the model schema stable without making plugin load order, credential state, or HMR timing part of the model-facing contract. If web search is enabled but no usable search provider exists, `web_search` remains visible and execution fails with a structured `WebError` such as `WEB_PROVIDER_UNAVAILABLE` or `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`. If a provider appears after `dsh-tool-web`, the next execution can use it without changing the schema. If a provider disappears mid-call, execution fails with a structured `WebError` instead of silently choosing another provider or falling through to `UNKNOWN_TOOL`. -The first version's provider-change signal is intentionally small. `web/providers-change` has no payload, carries no capability graph, and does not expose provider metadata. It means only "the provider registry changed; observers may recompute status from `ctx.web`." `searchStatus()` and `fetchStatus()` remain derived, not stored, and they are diagnostics plus execution-resolution inputs rather than tool-schema visibility switches. +The seam deliberately exposes no observation surface — no registry-change event and no aggregated capability-status query. Unavailability is a fact a caller observes by executing: `search()`/`fetch()` resolve the provider at call time and throw the structured `WebError` that names what failed. [The observation-surface RFC](../simplification/2026-07-04-drop-unconsumed-web-observation-surface.md) records that judgment: derived-on-call selection and enablement-based registration leave no consumer that needs a change signal or an availability probe distinct from executing and routing the error, and a future provider-status panel reintroduces the smallest signal or query it actually consumes. ## Package topology @@ -52,7 +52,7 @@ The dependency direction mirrors bash and filesystem: implementation ``` -At runtime, provider packages register capabilities with `ctx.web`; `tool-web` reads capability status and registers stable tools with `ctx.tools`: +At runtime, provider packages register capabilities with `ctx.web`; `tool-web` registers stable tools with `ctx.tools` and executes through the seam: ```mermaid flowchart LR @@ -60,12 +60,12 @@ flowchart LR perplexity["@deepseek-ai/dsh-web-search-perplexity"] -->|registerSearchProvider| web deepseek["@deepseek-ai/dsh-web-search-deepseek"] -->|registerSearchProvider| web fetchLocal["@deepseek-ai/dsh-web-fetch-local"] -->|registerFetchProvider| web - toolWeb["@deepseek-ai/dsh-tool-web"] -->|searchStatus/fetchStatus| web + toolWeb["@deepseek-ai/dsh-tool-web"] -->|search/fetch| web toolWeb -->|ctx.tools.register| webSearch["tool: web_search"] toolWeb -->|ctx.tools.register| webFetch["tool: web_fetch"] ``` -`@deepseek-ai/dsh-web` depends only on Cordis and low-level harness support. It declares `ctx.web`, provider interfaces, request/result types, status types, and error codes. It does not import tool, agent, session, LLM, or provider packages. +`@deepseek-ai/dsh-web` depends only on Cordis and low-level harness support. It declares `ctx.web`, provider interfaces, request/result types, the provider status type, and error codes. It does not import tool, agent, session, LLM, or provider packages. Provider packages depend on `@deepseek-ai/dsh-web` and Cordis. They own credentials, endpoint config, provider-specific request mapping, provider-specific response parsing, and provider-specific error translation into `WebError`. They issue network requests with the platform-native `fetch` (Node 24), mirroring `@deepseek-ai/dsh-llm-deepseek`'s adapter, NOT a cordis HTTP-client service (`ctx.http`/`@cordisjs/plugin-http`) — even where a Perplexity provider's request is shaped like an OpenAI-compatible chat completion, that wire shape is a provider-private detail and does not make the provider depend on `ctx.llm`. A provider does NOT own the `ctx.web` key (two search providers cannot both own it): like `dsh-llm-deepseek`, each provider package is a function/namespace plugin (`inject: ['web']`) whose `apply` constructs the backend and calls `ctx.web.registerSearchProvider` / `registerFetchProvider`. `@deepseek-ai/dsh-web` is the `export default` service that owns the key. @@ -92,9 +92,6 @@ interface WebService { registerSearchProvider(provider: WebSearchProvider): () => void registerFetchProvider(provider: WebFetchProvider): () => void - searchStatus(): WebCapabilityStatus - fetchStatus(): WebCapabilityStatus - search(request: WebSearchRequest, exec?: WebExecContext): Promise fetch(request: WebFetchRequest, exec?: WebExecContext): Promise } @@ -106,39 +103,33 @@ interface WebExecContext { `WebExecContext` is execution control, not business input. The first version should carry only `signal` so `tool-web` can propagate turn cancellation, tool timeout, and agent disposal into provider network requests, SSE readers, and expensive decoding. It should not pass `ToolExecution` through the seam, because that would make `dsh-web` depend on `dsh-tools`. -`@deepseek-ai/dsh-web` should also declare a Cordis event named `web/providers-change`. Provider ids are stable strings and unique within their capability kind. Registering a duplicate search provider id or duplicate fetch provider id should fail rather than silently replace the old provider. Provider registration returns a disposer, emits `web/providers-change` after successful registration, and emits it again when the provider is disposed. The registry should follow the existing `ctx.tools.register()` / `ctx.systemPrompt.section()` pattern: wrap the mutation in `ctx.effect()`, install the rollback disposer before emitting `web/providers-change`, and let a throwing registration-time change listener roll back the just-added provider instead of leaking it into the registry. +Provider ids are stable strings and unique within their capability kind. Registering a duplicate search provider id or duplicate fetch provider id should fail rather than silently replace the old provider. Provider registration returns a disposer and follows the existing `ctx.tools.register()` / `ctx.systemPrompt.section()` pattern: wrap the mutation in `ctx.effect()` so the registration is torn down with the contributing fiber. ## Provider status and selection -Provider status and capability selection are separate concepts, but both stay minimal. A provider reports only whether that concrete implementation is usable by cheap local checks such as credential presence or parseable endpoint config. A provider `status()` must not make network calls. The service reports whether the capability has a selected usable provider, or why execution would fail. +Provider status and capability selection are separate concepts, but both stay minimal. A provider reports only whether that concrete implementation is usable by cheap local checks such as credential presence or parseable endpoint config. A provider `status()` must not make network calls. -`LlmService` has no status type at all: availability is expressed as registry membership plus a resolution-time throw. `ctx.web` needs a small status answer because product apps, diagnostics, tests, and execution can report precise provider-selection failures without probing individual providers from the tool layer. Status must be derived from the configured provider id, registered providers, and each provider's cheap local `status()` on each call; it must not be stored as mutable service state. +`LlmService` has no status type at all: availability is expressed as registry membership plus a resolution-time throw. `ctx.web` follows the same discipline. The seam exposes no aggregated capability-status query — `search()` / `fetch()` derive the selection on each call from the configured provider id, the registered providers, and each provider's cheap local `status()`, and a selection failure is the structured `WebError` thrown at execution time, whose code answers "in which broad category does this capability fail" and whose message answers "exactly which provider/ids/reason." A caller that needs to know whether a capability can run executes and routes that error; nothing is stored as mutable service state. -`WebCapabilityStatus` stays intentionally small: `available` plus a `reason` discriminant, and the selected `providerId` on the available branch so diagnostics can report which provider won. It does NOT carry the per-reason payload (the unavailable provider id, the ambiguous candidate set, the underlying provider-unavailable reason). That branchable detail lives in the structured `WebError` thrown at execution time, which is the surface callers route on; duplicating it into the status union would give the same fact two homes that can disagree. `searchStatus()` / `fetchStatus()` answer "can this capability run, and if not, in which broad category does it fail" — enough for startup diagnostics and the execution-resolution decision — and the thrown error answers "exactly which provider/ids/reason." - -`WebProviderStatus` is an input to selection, not a health system. `tool-web` reads only the aggregated `searchStatus()` / `fetchStatus()`, never each provider's `status()` directly, so selection policy has one owner. +`WebProviderStatus` is an input to selection, not a health system. `tool-web` never calls a provider's `status()` directly — its only path into the seam is `search()` / `fetch()` — so selection policy has one owner. ```ts type WebProviderStatus = | { readonly available: true } | { readonly available: false; readonly reason: 'missing-credential' | 'misconfigured' } - -type WebCapabilityStatus = - | { readonly available: true; readonly providerId: string } - | { readonly available: false; readonly reason: 'none' | 'configured-missing' | 'configured-unavailable' | 'ambiguous' } ``` Selection must not depend on registration order. Cordis load order, config ordering, and HMR timing are not product semantics. -| Situation | Status / behavior | +| Situation | Execution behavior | |---|---| -| A configured provider id is registered and `status().available === true` | `available: true` for that provider | -| A configured provider id is not registered | `configured-missing`; execution fails with `WEB_PROVIDER_CONFIGURED_MISSING` | -| A configured provider id is registered but unavailable | `configured-unavailable`; execution fails with `WEB_PROVIDER_CONFIGURED_UNAVAILABLE` | -| No provider id is configured and exactly one provider for that kind is registered and available | `available: true` for that single provider | -| No provider id is configured and no provider for that kind is registered | `none`; execution fails with `WEB_PROVIDER_UNAVAILABLE` | -| No provider id is configured and multiple usable providers for that kind are registered | `ambiguous`; execution fails with `WEB_PROVIDER_AMBIGUOUS` rather than choosing by registration order | -| No provider id is configured and providers exist but none are usable | `none`; execution fails with `WEB_PROVIDER_UNAVAILABLE` | +| A configured provider id is registered and `status().available === true` | runs that provider | +| A configured provider id is not registered | fails with `WEB_PROVIDER_CONFIGURED_MISSING` | +| A configured provider id is registered but unavailable | fails with `WEB_PROVIDER_CONFIGURED_UNAVAILABLE` | +| No provider id is configured and exactly one provider for that kind is registered and available | runs that single provider | +| No provider id is configured and no provider for that kind is registered | fails with `WEB_PROVIDER_UNAVAILABLE` | +| No provider id is configured and multiple usable providers for that kind are registered | fails with `WEB_PROVIDER_AMBIGUOUS` rather than choosing by registration order | +| No provider id is configured and providers exist but none are usable | fails with `WEB_PROVIDER_UNAVAILABLE` | The "single provider auto-selects" rule is for tests, demos, and simple deployments. Product configs should set explicit provider ids: @@ -167,7 +158,7 @@ The "single provider auto-selects" rule is for tests, demos, and simple deployme Operational overrides such as environment variables may exist, but they must feed the same explicit selection path. For example, `DSH_WEB_SEARCH_PROVIDER=perplexity` is equivalent to config `searchProvider: perplexity`; it is not a hidden priority chain inside `dsh-tool-web`. -`ctx.web.search()` and `ctx.web.fetch()` resolve the provider at execution time using the same rules as the status query. If the selected capability is unavailable, they throw `WebError` with a structured code such as `WEB_PROVIDER_UNAVAILABLE`, `WEB_PROVIDER_CONFIGURED_MISSING`, `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`, or `WEB_PROVIDER_AMBIGUOUS`. If no provider is explicitly configured and no usable provider exists, the status and execution error are both the generic `none` / `WEB_PROVIDER_UNAVAILABLE` case; the first version should not add a diagnostic summary of every unavailable provider. +`ctx.web.search()` and `ctx.web.fetch()` resolve the provider at execution time using the selection rules above. If the selected capability is unavailable, they throw `WebError` with a structured code such as `WEB_PROVIDER_UNAVAILABLE`, `WEB_PROVIDER_CONFIGURED_MISSING`, `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`, or `WEB_PROVIDER_AMBIGUOUS`. If no provider is explicitly configured and no usable provider exists, the execution error is the generic `WEB_PROVIDER_UNAVAILABLE` case; the first version should not add a diagnostic summary of every unavailable provider. ## Search request and result schema @@ -269,14 +260,14 @@ SSRF / private-network protection (blocking private, loopback, link-local, multi `dsh-tool-web` owns two `ToolDefinition`s: `web_search` and `web_fetch`. It owns model-facing JSON schemas, snake_case argument names, prompt sections, result rendering to `ContentBlock[]`, `presentCall`, and `presentResult`. -`dsh-tool-web` must not enumerate providers or call provider `status()` directly. Its execution path is `ctx.web.search()` / `ctx.web.fetch()`, and any optional startup diagnostics should read only `ctx.web.searchStatus()` / `ctx.web.fetchStatus()`. That keeps provider selection in one layer; otherwise the tool package could decide one provider is usable while execution resolves a different state. +`dsh-tool-web` must not enumerate providers or call provider `status()` directly. Its only path into the seam is `ctx.web.search()` / `ctx.web.fetch()`. That keeps provider selection in one layer; otherwise the tool package could decide one provider is usable while execution resolves a different state. Tool registration in the first version is a minimal stable sync: 1. On plugin startup, read the `dsh-tool-web` `Config` (`search?: boolean`, `fetch?: boolean`, both default `true`) that enables or disables each web tool. 2. If web search is enabled, register `web_search` (its disposer is fiber-scoped via the effect-based registry). 3. If web fetch is enabled, register `web_fetch` (likewise fiber-scoped). -4. Do not dispose either tool merely because `ctx.web.searchStatus()` or `ctx.web.fetchStatus()` is unavailable. +4. Do not dispose either tool merely because its selected provider is missing, unusable, or ambiguous. 5. Disposing the `tool-web` fiber tears down its registrations automatically. Provider status changes affect execution results and diagnostics, not whether the model-facing schema exists. If a product wants no web tools at all, it disables `dsh-tool-web` or the individual web tool in config; if it wants web tools but the backend is misconfigured, the model sees a structured tool error at execution time. @@ -311,7 +302,7 @@ Tool execution should let these errors flow through `ToolRegistry.execute()`, wh Tests should prove the seam contract without turning this RFC into an implementation checklist. -`dsh-web` tests cover provider registration and disposal, duplicate provider ids, `web/providers-change` emission, rollback when a registration-time `web/providers-change` listener throws, `searchStatus()` and `fetchStatus()` for the selection table above, execution-time provider resolution, `maxResults` truncation of `sources[]` with `truncated` set when a provider over-returns, abort propagation through `WebExecContext.signal`, and structured `WebError` codes. +`dsh-web` tests cover provider registration and disposal (proved through execution behavior — a registered provider serves `search()`/`fetch()`, a disposed one no longer resolves), duplicate provider ids, the selection table above exercised through execution-time provider resolution, `maxResults` truncation of `sources[]` with `truncated` set when a provider over-returns, abort propagation through `WebExecContext.signal`, and structured `WebError` codes. Search provider tests cover request mapping, response parsing into `content` plus `sources[]`, missing credentials, provider errors, timeout/abort, truncation, and a self-skipping with-key smoke test for each real provider. Perplexity fixtures must include URL-only citations so the optional source fields stay honest. @@ -329,7 +320,7 @@ This is new capability work, so no compatibility migration is required while the Land the work in seam order: -1. Add `packages/web/web` with `ctx.web`, provider registration, provider status, capability status, selection, request/result/error types, and contract tests. +1. Add `packages/web/web` with `ctx.web`, provider registration, provider status, selection, request/result/error types, and contract tests. 2. Add `packages/web/web-search-exa` with parser/unit tests and a self-skipping real-provider smoke test. 3. Add `packages/web/web-search-perplexity` with parser/unit tests and a self-skipping real-provider smoke test. 4. Add `packages/web/web-search-deepseek` with parser/unit tests and a self-skipping real-provider smoke test. @@ -370,7 +361,7 @@ Rejected for the seam. `prompt` turns fetch into LLM summarization and couples p **Perplexity citations may be sparse.** A citation may be only a URL. Making `title` and `snippet` optional keeps the seam truthful but means `tool-web` must render useful fallback labels. -**Stable tool registration can defer misconfiguration to execution.** Keeping the tool visible is correct when the product enabled web access, but product apps that expect web search should surface `configured-missing`, `configured-unavailable`, and `ambiguous` loudly during startup diagnostics so users do not discover setup problems only after the model calls the tool. +**Stable tool registration can defer misconfiguration to execution.** Keeping the tool visible is correct when the product enabled web access, but product apps that expect web search should surface the structured `WEB_PROVIDER_CONFIGURED_MISSING` / `WEB_PROVIDER_CONFIGURED_UNAVAILABLE` / `WEB_PROVIDER_AMBIGUOUS` failures loudly so users do not discover setup problems only after the model calls the tool. **Provider state can change after startup.** A tool can be visible in the request assembled at step start and lose its provider before execution. The execution path must resolve again and fail with a structured error. @@ -388,5 +379,5 @@ Rejected for the seam. `prompt` turns fetch into LLM summarization and couples p ## Open questions -- Should product app packages treat `configured-missing`, `configured-unavailable`, and `ambiguous` as fatal startup errors when web is explicitly configured, or should `dsh-web` only report status and let apps decide? +- Should product app packages probe web configuration at startup (treating `WEB_PROVIDER_CONFIGURED_MISSING`, `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`, and `WEB_PROVIDER_AMBIGUOUS` as fatal when web is explicitly configured), or leave misconfiguration to surface at the first execution? - Where should permission policy for public web access live once the deferred permission system lands: a dedicated web permission plugin on `tools/execute`, provider config, or both? diff --git a/docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md b/docs/rfc/implemented/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md similarity index 79% rename from docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md rename to docs/rfc/implemented/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md index e3516d974f..7c5881b106 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md +++ b/docs/rfc/implemented/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md @@ -1,13 +1,13 @@ # RFC: Drop the unconsumed web observation surface — the `providers-change` event and the status methods -Status: proposed +Status: implemented (proposed and accepted 2026-07-04) ## Problem `WebService` exposes an observation surface no production code observes: - **`web/providers-change`** (`packages/web/web/src/index.ts`) is declared and emitted on every provider registration and disposal, and each registration effect's rollback yield is ordered BEFORE the emit solely so a throwing change listener unwinds the registration. No listener exists outside the package's own two unit tests (one of which exists to pin that rollback ordering). -- **`searchStatus()` / `fetchStatus()` and the `WebCapabilityStatus` union** (same package) have zero production callers: `dsh-tool-web` executes directly through `ctx.web.search()`/`fetch()` and surfaces unavailability as the structured `WebError` codes the seam throws at execution time (`packages/web/tool-web/src/search.ts`, `packages/web/tool-web/src/fetch.ts`); the only status callers are the web packages' own tests. The prose in `packages/web/tool-web/README.md` and [architecture.md](../../../architecture.md) still claims the tool "reads only the aggregated `searchStatus()`/`fetchStatus()`" — drift that survives only because nothing checks prose against call sites. +- **`searchStatus()` / `fetchStatus()` and the `WebCapabilityStatus` union** (same package) have zero production callers: `dsh-tool-web` executes directly through `ctx.web.search()`/`fetch()` and surfaces unavailability as the structured `WebError` codes the seam throws at execution time (`packages/web/tool-web/src/search.ts`, `packages/web/tool-web/src/fetch.ts`); the only status callers are the web packages' own tests. The prose in `packages/web/tool-web/README.md` and [architecture.md](../../../architecture.md) claims the tool "reads only the aggregated `searchStatus()`/`fetchStatus()`" — drift that survives only because nothing checks prose against call sites. The seam's own design starves both surfaces of consumers: tool registration follows product ENABLEMENT, not provider availability (`packages/web/tool-web/src/index.ts`), and provider selection resolves at execution time, never cached — so there is no cache to invalidate, no registration set to recompute, and no caller that needs an availability probe distinct from executing and routing the structured error. HMR cleanup is carried by the effect disposers themselves. @@ -15,7 +15,7 @@ This mirrors [drop the unconsumed `llm/adapter-change` event](../../implemented/ ## Proposal -Delete the event declaration, both emits, and the rollback-before-emit ordering (the plain `ctx.effect` disposer keeps HMR cleanup). Delete `searchStatus()`/`fetchStatus()`/`WebCapabilityStatus` — the provider-private `status()` stays, since it feeds execution-time selection. Delete the two event tests and rewrite the status-based test assertions onto the behavior a real caller observes (a successful `search()`/`fetch()`, or the structured `WebError` codes for unavailable/ambiguous/misconfigured provider sets). Run `pnpm run gen-cordis-catalog`; update `packages/web/web/README.md`, `packages/web/tool-web/README.md` (the drifted reads-status sentence), [web.md](../../../core-data-structures/web.md), and the web paragraph in [architecture.md](../../../architecture.md). The implementing PR amends the [web capability seam RFC](../../implemented/architecture/2026-06-24-web-capability-seam.md)'s facts (it specifies the event and the status aggregation) per [implemented/AGENTS.md](../../implemented/AGENTS.md). +Delete the event declaration, both emits, and the rollback-before-emit ordering (the plain `ctx.effect` disposer keeps HMR cleanup). Delete `searchStatus()`/`fetchStatus()`/`WebCapabilityStatus` — the provider-private `status()` stays, since it feeds execution-time selection. Delete the listener-throw rollback test that exists solely for the removed event, and rewrite the emission assertions and every status-based assertion onto the behavior a real caller observes (a successful `search()`/`fetch()`, or the structured `WebError` codes for unavailable/ambiguous/misconfigured provider sets). Run `pnpm run gen-cordis-catalog`; update `packages/web/web/README.md`, `packages/web/tool-web/README.md` (the drifted reads-status sentence), [web.md](../../../core-data-structures/web.md), and the web paragraph in [architecture.md](../../../architecture.md). Amend the [web capability seam RFC](../../implemented/architecture/2026-06-24-web-capability-seam.md)'s facts (it specified the event and the status aggregation) per [implemented/AGENTS.md](../AGENTS.md). ## Why not keep it? diff --git a/packages/web/tool-web/README.md b/packages/web/tool-web/README.md index f57f38d0d5..1e0a2e3336 100644 --- a/packages/web/tool-web/README.md +++ b/packages/web/tool-web/README.md @@ -27,4 +27,4 @@ Each tool is registered independently; a product that wants only one disables th Tool registration follows product **enablement**, not backend availability. A tool stays visible even when its selected provider is missing, misconfigured, ambiguous, or temporarily unavailable; the seam resolves the provider at execution time and execution fails with a structured `WebError` (e.g. `WEB_PROVIDER_UNAVAILABLE`, `WEB_PROVIDER_AMBIGUOUS`), which `ToolRegistry.execute()` turns into an error tool result the model can read and hooks/UI can route on. This keeps the model schema stable without making plugin load order, credential state, or HMR timing part of the model-facing contract. To remove a web tool entirely, disable it here in config. -The tool reads only the aggregated `ctx.web.searchStatus()` / `fetchStatus()` for diagnostics — never each provider's `status()` directly — so provider selection has one owner. +The tool never calls a provider's `status()` and never enumerates providers — its only execution path is `ctx.web.search()` / `ctx.web.fetch()`, and provider unavailability reaches it as the structured `WebError` codes selection throws at execution time. Provider selection stays entirely inside the seam, with one owner. diff --git a/packages/web/tool-web/tests/tool-web.spec.ts b/packages/web/tool-web/tests/tool-web.spec.ts index 7af1ce7c36..7b3b965c46 100644 --- a/packages/web/tool-web/tests/tool-web.spec.ts +++ b/packages/web/tool-web/tests/tool-web.spec.ts @@ -187,9 +187,12 @@ describe('tool-web registration', () => { }) it('registers web_search even when no provider is available (schema follows enablement, not availability)', async () => { - const { fiber, ctx } = await mountTools() + const { fiber, ctx, call } = await mountTools() expect(ctx.tools.schemas().map(s => s.name)).toContain('web_search') - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'none' }) + // No provider is registered: the schema stays visible and execution reports + // the structured unavailability instead. + const out = await call('web_search', { query: 'q' }) + expect(out.error?.code).toBe('WEB_PROVIDER_UNAVAILABLE') await fiber.dispose() }) diff --git a/packages/web/web-fetch-local/tests/fetch-local.spec.ts b/packages/web/web-fetch-local/tests/fetch-local.spec.ts index 27ed991c08..7d4c4e683b 100644 --- a/packages/web/web-fetch-local/tests/fetch-local.spec.ts +++ b/packages/web/web-fetch-local/tests/fetch-local.spec.ts @@ -377,9 +377,11 @@ describe('web-fetch-local plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID }) const fiber = await ctx.plugin(fetchPlugin, {}) - expect(ctx.web.fetchStatus()).toEqual({ available: true, providerId: LOCAL_FETCH_PROVIDER_ID }) + await expect(ctx.web.fetch({ url: `${base}/` })) + .resolves.toMatchObject({ providerId: LOCAL_FETCH_PROVIDER_ID, statusCode: 200 }) await fiber.dispose() - expect(ctx.web.fetchStatus()).toEqual({ available: false, reason: 'configured-missing' }) + await expect(ctx.web.fetch({ url: `${base}/` })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_MISSING' })) }) it('has no default export (namespace plugin export shape)', () => { @@ -418,7 +420,8 @@ describe('web-fetch-local plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { fetchProvider: LOCAL_FETCH_PROVIDER_ID }) const fiber = await ctx.plugin(fetchPlugin, { maxRedirects: 0 }) - expect(ctx.web.fetchStatus()).toEqual({ available: true, providerId: LOCAL_FETCH_PROVIDER_ID }) + await expect(ctx.web.fetch({ url: `${base}/` })) + .resolves.toMatchObject({ providerId: LOCAL_FETCH_PROVIDER_ID, statusCode: 200 }) await fiber.dispose() }) }) diff --git a/packages/web/web-search-deepseek/tests/deepseek.spec.ts b/packages/web/web-search-deepseek/tests/deepseek.spec.ts index ef688b7ad2..0faab1f35a 100644 --- a/packages/web/web-search-deepseek/tests/deepseek.spec.ts +++ b/packages/web/web-search-deepseek/tests/deepseek.spec.ts @@ -264,12 +264,14 @@ describe('DeepSeekSearchProvider error handling', () => { describe('web-search-deepseek plugin registration', () => { it('registers the provider into ctx.web (HMR-safe)', async () => { + vi.stubGlobal('fetch', vi.fn(async () => jsonResponse(searchResponse()))) const ctx = new Context() await ctx.plugin(WebService, { searchProvider: DEEPSEEK_PROVIDER_ID }) const fiber = await ctx.plugin(deepseekPlugin, { apiKey: 'ds-key' }) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: DEEPSEEK_PROVIDER_ID }) + await expect(ctx.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: DEEPSEEK_PROVIDER_ID }) await fiber.dispose() - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-missing' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_MISSING' })) }) it('rejects maxTokens: 0 at plugin construction', async () => { @@ -315,13 +317,14 @@ describe('web-search-deepseek plugin registration', () => { }) it('boots over ctx.web through the unwrapped module without an inject error', async () => { + vi.stubGlobal('fetch', vi.fn(async () => jsonResponse(searchResponse()))) const ctx = new Context() await ctx.plugin(WebService, { searchProvider: DEEPSEEK_PROVIDER_ID }) const loader = Object.create(Loader.prototype) as Loader const unwrapped = loader.unwrapExports(deepseekPlugin) as Parameters[0] // A collapsed export shape (dropped inject) would throw "without inject" here. const fiber = await ctx.plugin(unwrapped, { apiKey: 'ds-key' }) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: DEEPSEEK_PROVIDER_ID }) + await expect(ctx.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: DEEPSEEK_PROVIDER_ID }) await fiber.dispose() }) @@ -334,7 +337,6 @@ describe('web-search-deepseek plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: DEEPSEEK_PROVIDER_ID }) const fiber = await ctx.plugin(deepseekPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: DEEPSEEK_PROVIDER_ID }) await ctx.web.search({ query: 'q' }) const [url, init] = fetchMock.mock.calls[0] as unknown as [string, RequestInit] expect(url).toBe('https://api.deepseek.com/anthropic/v1/messages') @@ -354,7 +356,8 @@ describe('web-search-deepseek plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: DEEPSEEK_PROVIDER_ID }) await ctx.plugin(deepseekPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-unavailable' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_UNAVAILABLE' })) } finally { if (prev !== undefined) process.env.DEEPSEEK_API_KEY = prev } diff --git a/packages/web/web-search-exa/tests/exa.spec.ts b/packages/web/web-search-exa/tests/exa.spec.ts index 9cf31332f5..204b18f702 100644 --- a/packages/web/web-search-exa/tests/exa.spec.ts +++ b/packages/web/web-search-exa/tests/exa.spec.ts @@ -205,12 +205,14 @@ describe('ExaSearchProvider error handling', () => { describe('web-search-exa plugin registration', () => { it('registers the provider into ctx.web (HMR-safe)', async () => { + vi.stubGlobal('fetch', vi.fn(async () => jsonResponse({ results: [] }))) const ctx = new Context() await ctx.plugin(WebService, { searchProvider: EXA_PROVIDER_ID }) const fiber = await ctx.plugin(exaPlugin, { apiKey: 'exa-key' }) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: EXA_PROVIDER_ID }) + await expect(ctx.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: EXA_PROVIDER_ID }) await fiber.dispose() - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-missing' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_MISSING' })) }) it('has no default export (namespace plugin export shape)', () => { @@ -238,7 +240,6 @@ describe('web-search-exa plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: EXA_PROVIDER_ID }) const fiber = await ctx.plugin(exaPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: EXA_PROVIDER_ID }) await ctx.web.search({ query: 'q' }) const [url] = fetchMock.mock.calls[0] as unknown as [string] expect(url).toBe('https://api.exa.ai/search') @@ -256,7 +257,8 @@ describe('web-search-exa plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: EXA_PROVIDER_ID }) await ctx.plugin(exaPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-unavailable' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_UNAVAILABLE' })) } finally { if (prev !== undefined) process.env.EXA_API_KEY = prev } diff --git a/packages/web/web-search-perplexity/tests/perplexity.spec.ts b/packages/web/web-search-perplexity/tests/perplexity.spec.ts index 70a9a4c98b..df8a98f003 100644 --- a/packages/web/web-search-perplexity/tests/perplexity.spec.ts +++ b/packages/web/web-search-perplexity/tests/perplexity.spec.ts @@ -186,12 +186,14 @@ describe('PerplexitySearchProvider error handling', () => { describe('web-search-perplexity plugin registration', () => { it('registers the provider into ctx.web (HMR-safe)', async () => { + vi.stubGlobal('fetch', vi.fn(async () => jsonResponse({ choices: [{ message: { content: 'a' } }], citations: [] }))) const ctx = new Context() await ctx.plugin(WebService, { searchProvider: PERPLEXITY_PROVIDER_ID }) const fiber = await ctx.plugin(perplexityPlugin, { apiKey: 'pplx-key' }) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: PERPLEXITY_PROVIDER_ID }) + await expect(ctx.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: PERPLEXITY_PROVIDER_ID }) await fiber.dispose() - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-missing' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_MISSING' })) }) it('has no default export (namespace plugin export shape)', () => { @@ -219,7 +221,6 @@ describe('web-search-perplexity plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: PERPLEXITY_PROVIDER_ID }) const fiber = await ctx.plugin(perplexityPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: true, providerId: PERPLEXITY_PROVIDER_ID }) await ctx.web.search({ query: 'q' }) const [url, init] = fetchMock.mock.calls[0] as unknown as [string, RequestInit] expect(url).toBe('https://api.perplexity.ai/chat/completions') @@ -238,7 +239,8 @@ describe('web-search-perplexity plugin registration', () => { const ctx = new Context() await ctx.plugin(WebService, { searchProvider: PERPLEXITY_PROVIDER_ID }) await ctx.plugin(perplexityPlugin, {}) - expect(ctx.web.searchStatus()).toEqual({ available: false, reason: 'configured-unavailable' }) + await expect(ctx.web.search({ query: 'q' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_CONFIGURED_UNAVAILABLE' })) } finally { if (prev !== undefined) process.env.PERPLEXITY_API_KEY = prev } diff --git a/packages/web/web/README.md b/packages/web/web/README.md index 9b9e2ca333..fe5f1e650e 100644 --- a/packages/web/web/README.md +++ b/packages/web/web/README.md @@ -18,8 +18,7 @@ Search and fetch share no request schema and no business logic, but they are del | Member | Semantics | |---|---| -| `registerSearchProvider(provider)` / `registerFetchProvider(provider)` | Register a backend. Throws `WebError` `WEB_DUPLICATE_PROVIDER` on a duplicate id within that capability kind. Returns a disposer; emits `web/providers-change` on register and on dispose. Disposed with the calling fiber. | -| `searchStatus()` / `fetchStatus()` | Derived (never stored) `WebCapabilityStatus`: whether the capability has a selected usable provider, or the broad category it fails in. Diagnostics + execution-resolution input. | +| `registerSearchProvider(provider)` / `registerFetchProvider(provider)` | Register a backend. Throws `WebError` `WEB_DUPLICATE_PROVIDER` on a duplicate id within that capability kind. Returns a disposer. Disposed with the calling fiber. | | `search(request, exec?)` | Resolve the search provider and run one search. Enforces `request.maxResults` on the result (truncates `sources[]`, sets `truncated`). Throws `WebError` when the capability cannot run. | | `fetch(request, exec?)` | Resolve the fetch provider and retrieve one URL. A non-2xx response is a result, not a throw. Throws `WebError` for failures to safely retrieve or represent the resource. | @@ -27,18 +26,18 @@ Providers register **capabilities**, not tools. `dsh-tool-web` is the only owner ## Selection -Selection never depends on registration, config, or HMR order. A capability has an explicit provider id (config `searchProvider`/`fetchProvider`, or env `$DSH_WEB_SEARCH_PROVIDER`/`$DSH_WEB_FETCH_PROVIDER` feeding the same fields), or auto-selects when exactly one usable provider is registered: +Selection never depends on registration, config, or HMR order. A capability has an explicit provider id (config `searchProvider`/`fetchProvider`, or env `$DSH_WEB_SEARCH_PROVIDER`/`$DSH_WEB_FETCH_PROVIDER` feeding the same fields), or auto-selects when exactly one usable provider is registered. `search()`/`fetch()` resolve the provider at execution time: -| Situation | `WebCapabilityStatus` | Execution | -|---|---|---| -| configured id registered and `status().available` | `available` for it | runs | -| configured id not registered | `configured-missing` | `WEB_PROVIDER_CONFIGURED_MISSING` | -| configured id registered but unavailable | `configured-unavailable` | `WEB_PROVIDER_CONFIGURED_UNAVAILABLE` | -| no id, exactly one registered usable provider | `available` for it | runs | -| no id, no usable provider | `none` | `WEB_PROVIDER_UNAVAILABLE` | -| no id, multiple usable providers | `ambiguous` | `WEB_PROVIDER_AMBIGUOUS` | +| Situation | Execution | +|---|---| +| configured id registered and `status().available` | runs that provider | +| configured id not registered | `WEB_PROVIDER_CONFIGURED_MISSING` | +| configured id registered but unavailable | `WEB_PROVIDER_CONFIGURED_UNAVAILABLE` | +| no id, exactly one registered usable provider | runs it | +| no id, no usable provider | `WEB_PROVIDER_UNAVAILABLE` | +| no id, multiple usable providers | `WEB_PROVIDER_AMBIGUOUS` | -`WebCapabilityStatus` carries only `available` + a `reason` discriminant (plus the winning `providerId` on the available branch). The branchable per-reason detail lives in the thrown `WebError`, which is the surface callers route on — so the same fact never gets two homes that can disagree. A provider's own `status()` is a cheap local check (credential presence, parseable config) and **must not make network calls**; `dsh-tool-web` reads only the aggregated `searchStatus()`/`fetchStatus()`, never each provider's `status()` directly. +The failure branches throw `WebError`, whose structured code (plus message detail — the missing id, the ambiguous candidate set) is the surface callers route on. A provider's own `status()` is a cheap local check (credential presence, parseable config) that feeds this execution-time selection and **must not make network calls**; `dsh-tool-web` never calls a provider's `status()` — it executes through `ctx.web.search()`/`fetch()` and routes on the thrown codes, so provider selection has one owner. ## Vocabulary diff --git a/packages/web/web/src/index.ts b/packages/web/web/src/index.ts index 50150f6961..73dddf5e6e 100644 --- a/packages/web/web/src/index.ts +++ b/packages/web/web/src/index.ts @@ -3,15 +3,14 @@ * execution surface for two capabilities — search and fetch. Provider packages * register concrete backends with `registerSearchProvider` / * `registerFetchProvider`; the model-facing consumer - * (`@deepseek-ai/dsh-tool-web`) reads capability status and executes through - * `search()` / `fetch()`. + * (`@deepseek-ai/dsh-tool-web`) executes through `search()` / `fetch()` and + * routes on the structured {@link WebError} codes selection throws. * * The registry half stays close to `LlmService`: a `Map` per * capability kind, register methods that return disposers, duplicate ids that * throw, and execution-time resolution that throws when the selected provider is - * absent or unusable. On top of that sits one small selection-status layer so - * diagnostics and execution can explain why a capability can or cannot run, - * independent of registration order. + * absent or unusable — with selection rules that never depend on registration + * order. * * @module @deepseek-ai/dsh-web */ @@ -19,7 +18,6 @@ import { Context, Service } from 'cordis' import z from 'schemastery' import type { - WebCapabilityStatus, WebExecContext, WebFetchProvider, WebFetchRequest, @@ -35,7 +33,6 @@ export { WebError, } from './types.ts' export type { - WebCapabilityStatus, WebExecContext, WebFetchBody, WebFetchProvider, @@ -52,21 +49,9 @@ declare module 'cordis' { interface Context { web: WebService } - - interface Events { - /** - * Fired after the provider registry changes — a search or fetch provider was - * registered or disposed. Carries no payload and no capability graph: it - * means only "the provider registry changed; observers may recompute status - * from `ctx.web`". `searchStatus()` / `fetchStatus()` stay derived, not - * stored. - * @mode emit - */ - 'web/providers-change'(this: WebService): void - } } -/** Selection inputs shared by the status query and execution resolution. */ +/** Selection inputs for execution-time provider resolution. */ interface Selection

{ /** The configured provider id for this capability, if any. */ readonly configuredId?: string @@ -90,17 +75,14 @@ export interface WebServiceConfig { /** * The web access service. Registered as `ctx.web` (one instance per context). * - * Selection semantics (identical for status and execution, never order- - * dependent): + * Selection semantics (resolved at execution time, never order-dependent): * - A configured id that is registered and `status().available` → that provider. - * - A configured id not registered → `configured-missing` / - * `WEB_PROVIDER_CONFIGURED_MISSING`. - * - A configured id registered but unavailable → `configured-unavailable` / + * - A configured id not registered → `WEB_PROVIDER_CONFIGURED_MISSING`. + * - A configured id registered but unavailable → * `WEB_PROVIDER_CONFIGURED_UNAVAILABLE`. * - No id configured, exactly one registered usable provider → that provider. - * - No id configured, multiple usable providers → `ambiguous` / - * `WEB_PROVIDER_AMBIGUOUS`. - * - No id configured, no usable provider → `none` / `WEB_PROVIDER_UNAVAILABLE`. + * - No id configured, multiple usable providers → `WEB_PROVIDER_AMBIGUOUS`. + * - No id configured, no usable provider → `WEB_PROVIDER_UNAVAILABLE`. */ export class WebService extends Service { /** @@ -126,9 +108,8 @@ export class WebService extends Service { /** * Register a search provider. Throws {@link WebError} `WEB_DUPLICATE_PROVIDER` - * if its id is already registered for search. Returns a disposer; emits - * `web/providers-change` after a successful register and again on dispose. - * Disposed with the calling fiber. + * if its id is already registered for search. Returns a disposer; disposed + * with the calling fiber. */ registerSearchProvider(provider: WebSearchProvider): () => void { return this.registerProvider(this.searchProviders, provider) @@ -136,9 +117,8 @@ export class WebService extends Service { /** * Register a fetch provider. Throws {@link WebError} `WEB_DUPLICATE_PROVIDER` - * if its id is already registered for fetch. Returns a disposer; emits - * `web/providers-change` after a successful register and again on dispose. - * Disposed with the calling fiber. + * if its id is already registered for fetch. Returns a disposer; disposed + * with the calling fiber. */ registerFetchProvider(provider: WebFetchProvider): () => void { return this.registerProvider(this.fetchProviders, provider) @@ -148,39 +128,15 @@ export class WebService extends Service { if (store.has(provider.id)) { throw new WebError(`a web provider with id "${provider.id}" is already registered`, 'WEB_DUPLICATE_PROVIDER') } - const dispose = this.ctx.effect(function* (this: WebService) { + const dispose = this.ctx.effect(function* () { store.set(provider.id, provider) - // Yield the rollback BEFORE emitting `web/providers-change`: the generator - // effect collects each yielded disposer before the next step runs, so a - // throwing change listener removes the just-added provider instead of - // leaking it into the registry. - yield () => { - store.delete(provider.id) - this.ctx.emit('web/providers-change') - } - this.ctx.emit('web/providers-change') - }.bind(this), 'web.registerProvider()') + yield () => store.delete(provider.id) + }, 'web.registerProvider()') // ctx.effect's disposer returns Promise; our disposer API is // synchronous fire-and-forget — discard the (always-resolved) promise. return () => void dispose() } - /** Search-capability selection status, derived live (never stored). */ - searchStatus(): WebCapabilityStatus { - return resolveStatus({ - providers: this.searchProviders, - ...this.searchProviderId !== undefined ? { configuredId: this.searchProviderId } : {}, - }) - } - - /** Fetch-capability selection status, derived live (never stored). */ - fetchStatus(): WebCapabilityStatus { - return resolveStatus({ - providers: this.fetchProviders, - ...this.fetchProviderId !== undefined ? { configuredId: this.fetchProviderId } : {}, - }) - } - /** * Run one search through the selected provider. Resolves the provider at call * time with the selection rules above; throws {@link WebError} when the @@ -215,27 +171,7 @@ interface ResolvableProvider { status(): WebProviderStatus } -/** Compute the capability status from configured id + registered providers. */ -function resolveStatus

(selection: Selection

): WebCapabilityStatus { - const { configuredId, providers } = selection - if (configuredId !== undefined) { - const provider = providers.get(configuredId) - if (!provider) return { available: false, reason: 'configured-missing' } - if (!provider.status().available) return { available: false, reason: 'configured-unavailable' } - return { available: true, providerId: configuredId } - } - const usable = [...providers.values()].filter(provider => provider.status().available) - const [single] = usable - if (single === undefined) return { available: false, reason: 'none' } - if (usable.length > 1) return { available: false, reason: 'ambiguous' } - return { available: true, providerId: single.id } -} - -/** - * Resolve the selected provider or throw the matching {@link WebError}. Shares - * the selection rules with {@link resolveStatus} so status and execution can - * never disagree. - */ +/** Resolve the selected provider or throw the matching {@link WebError}. */ function resolveProvider

(selection: Selection

): P { const { configuredId, providers } = selection if (configuredId !== undefined) { diff --git a/packages/web/web/src/types.ts b/packages/web/web/src/types.ts index 6f85787d1b..f4cda691b8 100644 --- a/packages/web/web/src/types.ts +++ b/packages/web/web/src/types.ts @@ -1,8 +1,8 @@ /** * Vocabulary for the web capability seam (`ctx.web`): the search/fetch * request/result shapes providers produce and consumers format, the provider - * and capability status discriminants selection reports, the execution-control - * context, and the typed error taxonomy. + * status discriminant selection reads, the execution-control context, and the + * typed error taxonomy. * * These types are shared by every provider backend * (`@deepseek-ai/dsh-web-search-exa`, `@deepseek-ai/dsh-web-search-perplexity`, @@ -128,25 +128,15 @@ export type WebFetchBody = /** * Whether one concrete provider implementation is usable, by cheap local checks * only (credential presence, parseable endpoint config). A provider `status()` - * must NOT make network calls. It is an input to selection, not a health system. + * must NOT make network calls. It is an input to execution-time selection, not + * a health system: `WebService.search()`/`fetch()` read it to pick a usable + * provider, and selection failure surfaces as the structured {@link WebError} + * codes callers route on. */ export type WebProviderStatus = | { readonly available: true } | { readonly available: false; readonly reason: 'missing-credential' | 'misconfigured' } -/** - * Whether a capability (search or fetch) has a selected usable provider, or the - * broad category in which selection fails. Intentionally small: it carries the - * winning `providerId` on the available branch (so diagnostics can report which - * provider won) but NOT the per-reason payload (the missing id, the ambiguous - * candidate set). That branchable detail lives in the {@link WebError} thrown at - * execution time — the surface callers route on — so the same fact does not get - * two homes that can disagree. - */ -export type WebCapabilityStatus = - | { readonly available: true; readonly providerId: string } - | { readonly available: false; readonly reason: 'none' | 'configured-missing' | 'configured-unavailable' | 'ambiguous' } - /** * A search-capable backend. Registered with `ctx.web.registerSearchProvider`. * `id` is a stable string, unique within the search capability kind. diff --git a/packages/web/web/tests/web.spec.ts b/packages/web/web/tests/web.spec.ts index e97630ebab..8189e342da 100644 --- a/packages/web/web/tests/web.spec.ts +++ b/packages/web/web/tests/web.spec.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it } from 'vitest' import { Context } from 'cordis' import WebService, { WebError, @@ -42,18 +42,14 @@ async function mountWeb(config: ConstructorParameters[1] = {} } describe('WebService registration', () => { - it('registers and disposes a search provider, emitting providers-change each way', async () => { - const { ctx, web } = await mountWeb() - const changed = vi.fn() - ctx.on('web/providers-change', changed) + it('registers a search provider and unregisters it via the returned disposer', async () => { + const { web } = await mountWeb() const dispose = web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - expect(changed).toHaveBeenCalledTimes(1) - expect(web.searchStatus()).toEqual({ available: true, providerId: 'exa' }) + await expect(web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'exa' }) dispose() - expect(changed).toHaveBeenCalledTimes(2) - expect(web.searchStatus()).toEqual({ available: false, reason: 'none' }) + await expect(web.search({ query: 'q' })).rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_UNAVAILABLE' })) }) it('throws WEB_DUPLICATE_PROVIDER on a duplicate search id', async () => { @@ -69,88 +65,14 @@ describe('WebService registration', () => { expect(() => web.registerFetchProvider(makeFetchProvider('shared', available, fetchResult('shared')))).not.toThrow() }) - it('rolls back a registration when a providers-change listener throws', async () => { - const { ctx, web } = await mountWeb() - ctx.on('web/providers-change', () => { throw new Error('listener boom') }) - expect(() => web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa'))))) - .toThrow('listener boom') - // The throwing listener must not leave the provider in the registry. - expect(web.searchStatus()).toEqual({ available: false, reason: 'none' }) - }) - it('disposes provider registrations when the contributing fiber is disposed (HMR safety)', async () => { const { ctx, web } = await mountWeb() const fiber = await ctx.plugin(Object.assign((inner: Context) => { inner.web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) }, { inject: ['web'] })) - expect(web.searchStatus()).toEqual({ available: true, providerId: 'exa' }) + await expect(web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'exa' }) await fiber.dispose() - expect(web.searchStatus()).toEqual({ available: false, reason: 'none' }) - }) -}) - -describe('WebService selection status', () => { - it('reports none when nothing is registered', async () => { - const { web } = await mountWeb() - expect(web.searchStatus()).toEqual({ available: false, reason: 'none' }) - expect(web.fetchStatus()).toEqual({ available: false, reason: 'none' }) - }) - - it('auto-selects the single usable provider when no id is configured', async () => { - const { web } = await mountWeb() - web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - expect(web.searchStatus()).toEqual({ available: true, providerId: 'exa' }) - }) - - it('reports ambiguous when multiple usable providers exist and none is configured', async () => { - const { web } = await mountWeb() - web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) - expect(web.searchStatus()).toEqual({ available: false, reason: 'ambiguous' }) - }) - - it('ignores unusable providers when auto-selecting', async () => { - const { web } = await mountWeb() - web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - web.registerSearchProvider(makeSearchProvider('perplexity', unavailable, () => Promise.resolve(searchResult('perplexity')))) - expect(web.searchStatus()).toEqual({ available: true, providerId: 'exa' }) - }) - - it('reports none when providers exist but none are usable', async () => { - const { web } = await mountWeb() - web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) - expect(web.searchStatus()).toEqual({ available: false, reason: 'none' }) - }) - - it('honors a configured id over a different registered provider', async () => { - const { web } = await mountWeb({ searchProvider: 'perplexity' }) - web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) - expect(web.searchStatus()).toEqual({ available: true, providerId: 'perplexity' }) - }) - - it('reports configured-missing when the configured id is not registered', async () => { - const { web } = await mountWeb({ searchProvider: 'perplexity' }) - web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) - expect(web.searchStatus()).toEqual({ available: false, reason: 'configured-missing' }) - }) - - it('reports configured-unavailable when the configured id is registered but unusable', async () => { - const { web } = await mountWeb({ searchProvider: 'exa' }) - web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) - expect(web.searchStatus()).toEqual({ available: false, reason: 'configured-unavailable' }) - }) - - it('does not let registration order change auto-selection', async () => { - const a = await mountWeb() - a.web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) - a.web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) - expect(a.web.searchStatus()).toEqual({ available: true, providerId: 'perplexity' }) - - const b = await mountWeb() - b.web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) - b.web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) - expect(b.web.searchStatus()).toEqual({ available: true, providerId: 'perplexity' }) + await expect(web.search({ query: 'q' })).rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_UNAVAILABLE' })) }) }) @@ -160,6 +82,12 @@ describe('WebService execution resolution', () => { await expect(web.search({ query: 'q' })).rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_UNAVAILABLE' })) }) + it('throws WEB_PROVIDER_UNAVAILABLE when providers exist but none are usable', async () => { + const { web } = await mountWeb() + web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) + await expect(web.search({ query: 'q' })).rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_UNAVAILABLE' })) + }) + it('throws WEB_PROVIDER_CONFIGURED_MISSING for an unregistered configured id', async () => { const { web } = await mountWeb({ searchProvider: 'perplexity' }) web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) @@ -179,6 +107,32 @@ describe('WebService execution resolution', () => { await expect(web.search({ query: 'q' })).rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_AMBIGUOUS' })) }) + it('runs the configured provider even when another usable provider is registered', async () => { + const { web } = await mountWeb({ searchProvider: 'perplexity' }) + web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) + web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) + await expect(web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'perplexity' }) + }) + + it('ignores unusable providers when auto-selecting', async () => { + const { web } = await mountWeb() + web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve(searchResult('exa')))) + web.registerSearchProvider(makeSearchProvider('perplexity', unavailable, () => Promise.resolve(searchResult('perplexity')))) + await expect(web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'exa' }) + }) + + it('does not let registration order change auto-selection', async () => { + const a = await mountWeb() + a.web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) + a.web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) + await expect(a.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'perplexity' }) + + const b = await mountWeb() + b.web.registerSearchProvider(makeSearchProvider('perplexity', available, () => Promise.resolve(searchResult('perplexity')))) + b.web.registerSearchProvider(makeSearchProvider('exa', unavailable, () => Promise.resolve(searchResult('exa')))) + await expect(b.web.search({ query: 'q' })).resolves.toMatchObject({ providerId: 'perplexity' }) + }) + it('runs the selected provider and returns its result', async () => { const { web } = await mountWeb() web.registerSearchProvider(makeSearchProvider('exa', available, () => Promise.resolve( diff --git a/scripts/type-equiv.manifest.json b/scripts/type-equiv.manifest.json index 0aca732e03..8b13097e04 100644 --- a/scripts/type-equiv.manifest.json +++ b/scripts/type-equiv.manifest.json @@ -76,7 +76,6 @@ { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchRequest", "source": "packages/web/web/src/types.ts" }, { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchResult", "source": "packages/web/web/src/types.ts" }, { "doc": "docs/core-data-structures/web.md", "symbol": "WebFetchBody", "source": "packages/web/web/src/types.ts" }, - { "doc": "docs/core-data-structures/web.md", "symbol": "WebProviderStatus", "source": "packages/web/web/src/types.ts" }, - { "doc": "docs/core-data-structures/web.md", "symbol": "WebCapabilityStatus", "source": "packages/web/web/src/types.ts" } + { "doc": "docs/core-data-structures/web.md", "symbol": "WebProviderStatus", "source": "packages/web/web/src/types.ts" } ] } From 74261b0e21e051d02c9f61c74d8a139dbc4ce8d5 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:56:51 +0800 Subject: [PATCH 3/8] docs(rfc): finish amending the seam RFC's status-layer description The package-topology paragraph of the web capability seam RFC still described ctx.web as carrying 'one small selection-status layer', contradicting the shipped surface the rest of the amended RFC records (no aggregated status query; execution-time WebError explains a selection failure). State the current shape: two capability kinds and a richer selection policy whose failure the thrown error explains. --- .../implemented/architecture/2026-06-24-web-capability-seam.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md index d0ca9c8b9a..6acaab8459 100644 --- a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md +++ b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md @@ -37,7 +37,7 @@ The seam deliberately exposes no observation surface — no registry-change even ## Package topology -The three-package interface/implementation/consumer split follows bash and filesystem, but the *interface* package is closer to the LLM seam. `LlmService` (`packages/llm/llm/src/index.ts`) is a name-keyed provider registry: `registerAdapter(models, adapter)` stores adapters in a `Map`, returns a disposer, throws `DUPLICATE_ADAPTER` on duplicate keys, and throws `NO_ADAPTER` at resolution time. `ctx.web` follows that registry shape, but has two capability kinds and one small selection-status layer so diagnostics and execution can explain why a search or fetch capability can or cannot run. +The three-package interface/implementation/consumer split follows bash and filesystem, but the *interface* package is closer to the LLM seam. `LlmService` (`packages/llm/llm/src/index.ts`) is a name-keyed provider registry: `registerAdapter(models, adapter)` stores adapters in a `Map`, returns a disposer, throws `DUPLICATE_ADAPTER` on duplicate keys, and throws `NO_ADAPTER` at resolution time. `ctx.web` follows that registry shape, but has two capability kinds and a richer selection policy (a configured provider id, or auto-select when exactly one usable provider is registered), so the `WebError` an execution throws can explain why a search or fetch capability cannot run. The dependency direction mirrors bash and filesystem: From 2745879132f682b59e61ff59c02b6f184c5a275f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:21:13 +0800 Subject: [PATCH 4/8] refactor(llm): drop the image content block until a path can honor it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ImageBlock had no production producer and every consumer dropped it: the deepseek serializer skipped it, the pi-ai converter skipped it as unrepresentable, the ACP bridge neither advertises image prompt capability nor forwards image blocks, and compact-basic charged a flat 85-token estimate and rendered an [image] placeholder. A block constructed today would silently vanish from the wire — the vocabulary advertised a capability no path honors, the silent-data-loss shape the defensive patterns warn against. The only constructors were tests pinning the skip/estimate branches. Remove ImageBlock and its ContentBlockMap entry (its cache?: CacheHint field leaves with it; CacheHint itself and the other two cache? fields are out of scope). compact-basic loses its explicit image estimate and placeholder arms (the merge-extensible default arms absorb the case); the deepseek serializer, pi-ai converter, and ACP codec already handled image in their default arms, so only their image-naming comments change. The codec's inbound rejection of ACP-protocol image prompt content stays — that guards wire content a client can send regardless of our vocabulary. Tests that constructed harness image blocks to pin the removed branches are dropped (the 85-token estimate pin) or retargeted onto plugin-added block types / other non-text blocks, which the surviving default arms own. Docs, the type-equiv pastes, and the content-block vocabulary RFC's block list and multimodal-home consequence are updated in the same change; the RFC moves to implemented/ and the index is regenerated. A real multimodal feature reintroduces image via declaration merging together with the adapter mapping, ACP advertisement, and compaction pricing that honor it. --- docs/architecture.md | 2 +- docs/core-data-structures/core.md | 3 +- docs/core-data-structures/llm-streaming.md | 1 - docs/rfc/README.md | 2 +- .../2026-06-11-content-block-vocabulary.md | 4 +- .../2026-07-04-drop-image-content-block.md | 27 ++++++++++++ .../2026-07-04-drop-image-content-block.md | 27 ------------ ...-prune-producerless-vocabulary-variants.md | 2 +- packages/bash/tool-bash/tests/tools.spec.ts | 2 +- packages/compact/compact-basic/README.md | 4 +- packages/compact/compact-basic/src/index.ts | 17 ++------ .../compact-basic/tests/compact-basic.spec.ts | 42 +++++++++---------- packages/llm/llm-deepseek/README.md | 1 - packages/llm/llm-deepseek/src/serialize.ts | 1 - .../llm/llm-deepseek/tests/serialize.spec.ts | 14 +++++-- packages/llm/llm-pi-ai/README.md | 2 +- packages/llm/llm-pi-ai/src/convert.ts | 2 +- packages/llm/llm-pi-ai/tests/convert.spec.ts | 6 +-- packages/llm/llm/README.md | 2 +- packages/llm/llm/src/types.ts | 16 ++++--- packages/llm/llm/tests/assembler.spec.ts | 12 +++--- packages/llm/llm/tests/properties.spec.ts | 2 +- packages/ui/acp/src/codec.ts | 6 +-- packages/ui/acp/tests/codec.spec.ts | 5 ++- packages/ui/acp/tests/stream-update.spec.ts | 2 +- 25 files changed, 97 insertions(+), 107 deletions(-) create mode 100644 docs/rfc/implemented/simplification/2026-07-04-drop-image-content-block.md delete mode 100644 docs/rfc/proposed/simplification/2026-07-04-drop-image-content-block.md diff --git a/docs/architecture.md b/docs/architecture.md index 0266f1a2e9..84c113c13b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -93,7 +93,7 @@ The web capability uses the same three-package split but folds two capabilities ## The vocabulary (dsh-llm) -Messages are arrays of typed **content blocks** (`text`, `reasoning`, `tool-call`, `tool-result`, `image`); the union is derived from the merge-extensible `ContentBlockMap`, so plugins can add block types via declaration merging. The same merge-extensible-map pattern is used for `MessageSource`, `FinishReason`, `TurnTrigger`, and `TurnEndReason` — typed sum types instead of strings. +Messages are arrays of typed **content blocks** (`text`, `reasoning`, `tool-call`, `tool-result`); the union is derived from the merge-extensible `ContentBlockMap`, so plugins can add block types via declaration merging. The core set is limited to blocks every shipping path honors — multimodal content (images, audio, …) has no core block type; a feature that needs one adds it via the map in the same coordinated change that maps it in the adapters, surfaces it in the UI bridges, and prices it in compaction ([the drop-image RFC](rfc/implemented/simplification/2026-07-04-drop-image-content-block.md)). The same merge-extensible-map pattern is used for `MessageSource`, `FinishReason`, `TurnTrigger`, and `TurnEndReason` — typed sum types instead of strings. Streaming is a raw chunk protocol (`block-start`, `text-delta`, `reasoning-delta`, `tool-call-delta`, `block-end`, `usage`, `finish`). `BlockAssembler` is the single shared implementation that assembles chunks into blocks/messages; the loop logs raw chunks (replay fidelity) while feeding the same chunks through an assembler. diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 9a282e305c..e2f3cd35f5 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -87,11 +87,10 @@ interface ContentBlockMap { 'reasoning': ReasoningBlock 'tool-call': ToolCallBlock 'tool-result': ToolResultBlock - 'image': ImageBlock } ``` -The block interfaces (full fields in source): `TextBlock` (`text`), `ReasoningBlock` (thinking, distinct from visible text), `ToolCallBlock` (`id: CallId`, `name`, raw-JSON `arguments`), `ToolResultBlock` (`toolCallId`, nested `content: ContentBlock[]`, `isError?`), `ImageBlock` (`url`, `mimeType?`). `ContentBlock = ContentBlockMap[ContentBlockType]`. +The block interfaces (full fields in source): `TextBlock` (`text`), `ReasoningBlock` (thinking, distinct from visible text), `ToolCallBlock` (`id: CallId`, `name`, raw-JSON `arguments`), `ToolResultBlock` (`toolCallId`, nested `content: ContentBlock[]`, `isError?`). `ContentBlock = ContentBlockMap[ContentBlockType]`. The core set is limited to blocks every shipping path honors — multimodal content (images, audio, …) has no core block type; a feature that needs one adds it via the merge-extensible map together with the adapter/UI/compaction support that honors it. A `Message` is a role plus blocks: diff --git a/docs/core-data-structures/llm-streaming.md b/docs/core-data-structures/llm-streaming.md index 7439eb14a6..997928a3a0 100644 --- a/docs/core-data-structures/llm-streaming.md +++ b/docs/core-data-structures/llm-streaming.md @@ -59,7 +59,6 @@ interface ContentBlockMap { 'reasoning': ReasoningBlock 'tool-call': ToolCallBlock 'tool-result': ToolResultBlock - 'image': ImageBlock } ``` diff --git a/docs/rfc/README.md b/docs/rfc/README.md index d53684bf6b..f0987d2e65 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -52,7 +52,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | Title | First proposed | |---|---| | [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | -| [Drop the `image` content block until a path can honor it](proposed/simplification/2026-07-04-drop-image-content-block.md) | 2026-07-04 | | [Drop `GenerateOptions.prefill` and `ToolSchema.strict` — request knobs with no working end-to-end path](proposed/simplification/2026-07-04-drop-inert-request-knobs.md) | 2026-07-04 | | [Drop the unconsumed web observation surface — the `providers-change` event and the status methods](proposed/simplification/2026-07-04-drop-unconsumed-web-observation-surface.md) | 2026-07-04 | | [Fold the stdio UI helper into the stdio app](proposed/simplification/2026-07-04-fold-stdio-ui-helper.md) | 2026-07-04 | @@ -120,6 +119,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Stop mirroring durable boundaries as agent events](implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | | [Split the filesystem seam — provider text mutations plus the `dsh-fs-policy` plugin](implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) | 2026-06-26 | | [Stop mirroring the token stream as an agent event](implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) | 2026-07-02 | +| [Drop the `image` content block until a path can honor it](implemented/simplification/2026-07-04-drop-image-content-block.md) | 2026-07-04 | ### Architecture diff --git a/docs/rfc/implemented/architecture/2026-06-11-content-block-vocabulary.md b/docs/rfc/implemented/architecture/2026-06-11-content-block-vocabulary.md index 49d55badac..d2d2162588 100644 --- a/docs/rfc/implemented/architecture/2026-06-11-content-block-vocabulary.md +++ b/docs/rfc/implemented/architecture/2026-06-11-content-block-vocabulary.md @@ -10,12 +10,12 @@ The harness needs one internal language for messages that the loop, session log, ## Decision -Own it: messages are arrays of typed content blocks (`text`, `reasoning`, `tool-call`, `tool-result`, `image`), with the union derived from the merge-extensible `ContentBlockMap` so plugins add block types via declaration merging. The same merge-extensible-map pattern types every "stringly" field (`MessageSource`, `FinishReason`, `TurnTrigger`, `TurnEndReason`). Streaming is a raw chunk protocol; `BlockAssembler` is the single shared assembly implementation. Adapters translate to provider wire formats — mapping cost lives in adapters, where it belongs. +Own it: messages are arrays of typed content blocks (`text`, `reasoning`, `tool-call`, `tool-result`), with the union derived from the merge-extensible `ContentBlockMap` so plugins add block types via declaration merging. The same merge-extensible-map pattern types every "stringly" field (`MessageSource`, `FinishReason`, `TurnTrigger`, `TurnEndReason`). Streaming is a raw chunk protocol; `BlockAssembler` is the single shared assembly implementation. Adapters translate to provider wire formats — mapping cost lives in adapters, where it belongs. In-session context injection (`context/message`, `steering/message`) renders as tagged user-role envelopes (the system-reminder pattern) rather than a new role, so adapters carry zero burden. Live-adapter review has since validated the tagged-envelope rendering against current DeepSeek behavior; a future provider-specific mismatch should be handled in that adapter rather than by adding a new role to the canonical content vocabulary. ## Consequences -- Reasoning, prefill, cache hints, and multimodal content all have a home without provider contortions. +- Reasoning, prefill, and cache hints have a home without provider contortions. Multimodal content deliberately has NO core block type: the core set is limited to blocks every shipping path honors, and a multimodal feature adds its block type through the merge-extensible map in the same coordinated change that maps it in the adapters, surfaces it in the UI bridges, and prices it in compaction — see [the drop-image RFC](../simplification/2026-07-04-drop-image-content-block.md). - Every adapter pays a translation cost; the first real adapters have since validated the streaming protocol, and new adapters should continue proving their provider-specific mapping in adapter-local tests. - IDs that cross package boundaries are branded (`CallId`, `SessionId`, `AgentId`) — nominal typing at zero runtime cost. diff --git a/docs/rfc/implemented/simplification/2026-07-04-drop-image-content-block.md b/docs/rfc/implemented/simplification/2026-07-04-drop-image-content-block.md new file mode 100644 index 0000000000..ccbf5d755d --- /dev/null +++ b/docs/rfc/implemented/simplification/2026-07-04-drop-image-content-block.md @@ -0,0 +1,27 @@ +# RFC: Drop the `image` content block until a path can honor it + +Status: implemented (proposed and accepted 2026-07-04) + +## Problem + +`ImageBlock` (`packages/llm/llm/src/types.ts`) had no production producer, and every consumer on every path DROPPED it: the deepseek adapter's serializer skipped image blocks (a documented MVP limitation), the pi-ai converter skipped them as unrepresentable, the ACP codec neither advertises image prompt capability nor forwarded image blocks outbound and REJECTS image prompt content inbound, and the compaction estimator charged a flat token constant and rendered `[image]`. An `ImageBlock` constructed then would silently vanish from the wire — the vocabulary advertised a capability no path honored, which is the silent-data-loss shape AGENTS.md's defensive patterns warn against. The only constructors anywhere were tests pinning the skip/drop/estimate branches. + +## Decision + +Remove `ImageBlock`, its `ContentBlockMap` entry (and its `cache?: CacheHint` field with it), the explicit `image` estimate/placeholder arms in compact-basic, and the image-naming comments in the deepseek serializer's, pi-ai converter's, and ACP codec's default arms — those default arms absorb the case the way they absorb any unknown block type. Updated in the same change: the vocabulary line in [architecture.md](../../../architecture.md), the block list in `packages/llm/llm/README.md`, the deepseek README's image-skip row, the pi-ai README's images-not-representable row, the compact-basic README's image-estimation and `[image]`-placeholder rows, the pastes in [core.md](../../../core-data-structures/core.md) and [llm-streaming.md](../../../core-data-structures/llm-streaming.md), and the [content-block vocabulary RFC](../architecture/2026-06-11-content-block-vocabulary.md)'s block list and multimodal-home consequence per [implemented/AGENTS.md](../AGENTS.md); the tests that constructed image blocks to exercise the removed branches were dropped (the estimate pin) or retargeted onto the merge-extensible default arms (plugin-added block types). The ACP codec's inbound rejection of image PROMPT content is unaffected — that guard is about protocol content a client can send regardless of our vocabulary, and it stays. + +## Why not keep it? + +This was the most contested cut in the batch. Multimodal input (screenshots) is a plausible near-term coding-agent feature, and the [content-block vocabulary RFC](../architecture/2026-06-11-content-block-vocabulary.md) reserved the slot deliberately. Two responses. First, `ContentBlockMap` is merge-extensible by design: a real multimodal feature reintroduces `image` in core in the same coordinated change that maps it in the adapters, advertises and renders it in ACP, and prices it in compaction — the producer and its consumers arrive together, which is how the map is meant to grow. Second, the middle option — keep the type but make adapters throw UNSUPPORTED instead of silently dropping — converts this into exactly the shape the sibling request-knobs proposal (`2026-07-04-drop-inert-request-knobs`) argues against: surface whose only implementation is rejection. Absence (a compile error at the would-be producer) is strictly clearer than either silent loss or universal throw. + +The recorded fallback, had review landed on keeping the slot: keep `ImageBlock` but replace every silent skip with a loud rejection, and document that policy in the vocabulary — the silent drop was the one state with no defender. Review landed on removal; the fallback stands as the documented alternative should the slot ever return ahead of a full feature. + +## Acceptance criteria + +- No `ImageBlock` / harness `type: 'image'` block construction outside this RFC; the codec's inbound ACP-image rejection still passes its tests. +- Adapter/codec/compaction switches handle the case through their unknown-block default arms (pinned by the plugin-added-block tests). +- Doc pastes, the manifest, and the architecture vocabulary list updated; `pnpm run doc-sync` green. + +## Risks + +Re-adding a core vocabulary type later touches several packages at once — but that coordinated change is the shape a real multimodal feature needs anyway (adapter mapping, ACP advertisement, compaction pricing), and none of it existed to preserve. diff --git a/docs/rfc/proposed/simplification/2026-07-04-drop-image-content-block.md b/docs/rfc/proposed/simplification/2026-07-04-drop-image-content-block.md deleted file mode 100644 index e9144cc3aa..0000000000 --- a/docs/rfc/proposed/simplification/2026-07-04-drop-image-content-block.md +++ /dev/null @@ -1,27 +0,0 @@ -# RFC: Drop the `image` content block until a path can honor it - -Status: proposed - -## Problem - -`ImageBlock` (`packages/llm/llm/src/types.ts`) has no production producer, and every consumer on every path DROPS it: the deepseek adapter's serializer skips image blocks (a documented MVP limitation), the pi-ai converter skips them as unrepresentable, the ACP codec neither advertises image prompt capability nor forwards image blocks outbound and REJECTS image prompt content inbound, and the compaction estimator charges a flat token constant and renders `[image]`. An `ImageBlock` constructed today would silently vanish from the wire — the vocabulary advertises a capability no path honors, which is the silent-data-loss shape AGENTS.md's defensive patterns warn against. The only constructors anywhere are tests pinning the skip/drop/estimate branches. - -## Proposal - -Remove `ImageBlock`, its `ContentBlockMap` entry, the explicit `image` estimate/placeholder arms in compact-basic, and the image-naming comments in the deepseek serializer's, pi-ai converter's, and ACP codec's default arms — those default arms already absorb the case the way they absorb any unknown block type. Update the vocabulary line in [architecture.md](../../../architecture.md), the block list in `packages/llm/llm/README.md`, the deepseek README's image-skip row, the pi-ai README's images-not-representable row, the compact-basic README's image-estimation and `[image]`-placeholder rows, the pastes in [core.md](../../../core-data-structures/core.md) and [llm-streaming.md](../../../core-data-structures/llm-streaming.md), and the type-equiv manifest; amend the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md)'s block list and multimodal-home consequence per [implemented/AGENTS.md](../../implemented/AGENTS.md); drop or retarget the tests that construct image blocks to exercise the removed branches. The ACP codec's inbound rejection of image PROMPT content is unaffected — that guard is about protocol content a client can send regardless of our vocabulary, and it stays. - -## Why not keep it? - -This is the most contested cut in the batch. Multimodal input (screenshots) is a plausible near-term coding-agent feature, and the [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-content-block-vocabulary.md) reserved the slot deliberately. Two responses. First, `ContentBlockMap` is merge-extensible by design: a real multimodal feature reintroduces `image` in core in the same coordinated change that maps it in the adapters, advertises and renders it in ACP, and prices it in compaction — the producer and its consumers arrive together, which is how the map is meant to grow. Second, the middle option — keep the type but make adapters throw UNSUPPORTED instead of silently dropping — converts this into exactly the shape the [request-knobs RFC](2026-07-04-drop-inert-request-knobs.md) argues against: surface whose only implementation is rejection. Absence (a compile error at the would-be producer) is strictly clearer than either silent loss or universal throw. - -If review lands on keeping the slot, the fallback this RFC records is: keep `ImageBlock` but replace every silent skip with a loud rejection, and document that policy in the vocabulary — the current silent drop is the one state with no defender. - -## Acceptance criteria - -- No `ImageBlock` / harness `type: 'image'` block construction outside this RFC; the codec's inbound ACP-image rejection still passes its tests. -- Adapter/codec/compaction switches handle the case through their unknown-block default arms (pinned by the existing plugin-added-block tests where present). -- Doc pastes, the manifest, and the architecture vocabulary list updated; `pnpm run doc-sync` green. - -## Risks - -Re-adding a core vocabulary type later touches several packages at once — but that coordinated change is the shape a real multimodal feature needs anyway (adapter mapping, ACP advertisement, compaction pricing), and none of it exists today to preserve. diff --git a/docs/rfc/proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md b/docs/rfc/proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md index d967e1d3b2..38565c9214 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md +++ b/docs/rfc/proposed/simplification/2026-07-04-prune-producerless-vocabulary-variants.md @@ -28,4 +28,4 @@ The [content-block vocabulary RFC](../../implemented/architecture/2026-06-11-con ## Risks -None operational — nothing can construct these values today. The mirror-event removals (recorded in [the boundary-mirror RFC](../../implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) and [the stream-chunk RFC](../../implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md)) touch only transient `agent/*` events, never the durable vocabulary, so there is no collision. Elsewhere in the vocabulary the admission policy already holds: `rejected`, `prompt/blocked`, and `hook/invoked`/`hook/result` each have live producers — this RFC extends the same bar to the three variants that lack one. If the [image-block RFC](2026-07-04-drop-image-content-block.md) ships first, one of the three `cache?` fields leaves with it; the two proposals are independent and compose in either order. +None operational — nothing can construct these values today. The mirror-event removals (recorded in [the boundary-mirror RFC](../../implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) and [the stream-chunk RFC](../../implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md)) touch only transient `agent/*` events, never the durable vocabulary, so there is no collision. Elsewhere in the vocabulary the admission policy already holds: `rejected`, `prompt/blocked`, and `hook/invoked`/`hook/result` each have live producers — this RFC extends the same bar to the three variants that lack one. If the [image-block RFC](../../implemented/simplification/2026-07-04-drop-image-content-block.md) ships first, one of the three `cache?` fields leaves with it; the two proposals are independent and compose in either order. diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index 82b69fb133..6d5f78b8d0 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -824,7 +824,7 @@ describe('tool-owned UI presentation (presentCall / presentResult)', () => { const ctx = await setup() const present = ctx.tools.get('bash')!.presentResult!( { command: 'x', description: 'x' }, - { content: [{ type: 'image', url: 'https://x/y.png' }], isError: false }, + { content: [{ type: 'reasoning', text: 'unexpected' }], isError: false }, ) expect(present).toBeUndefined() }) diff --git a/packages/compact/compact-basic/README.md b/packages/compact/compact-basic/README.md index c195ae42fb..00cf72bd16 100644 --- a/packages/compact/compact-basic/README.md +++ b/packages/compact/compact-basic/README.md @@ -8,10 +8,10 @@ This is the implementation tier of the compaction capability — see the [interf The abstract contract states only WHAT compaction does; this backend owns every HOW decision: -- **Token estimation** — `estimateContentTokens()`: char/4 with per-block structural overhead (`text`/`reasoning` = `ceil(len/4) + 4`, `tool-call` from name + arguments, `tool-result` recursive, `image` = 85, unknown blocks via JSON length). +- **Token estimation** — `estimateContentTokens()`: char/4 with per-block structural overhead (`text`/`reasoning` = `ceil(len/4) + 4`, `tool-call` from name + arguments, `tool-result` recursive, unknown blocks via JSON length). - **Retention policy** — `compactIfNeeded()` walks the surface nodes tail→head summing per-node token estimates, and retains the smallest tail-run of WHOLE units (a closed step, or a single no-step node such as a pre-step `user/message` or inter-step `steering/message`) whose total reaches `retainTokens`; everything older is compacted. Retention is **turn-agnostic** — turn boundaries play no role, so a single runaway turn that alone exceeds the window compacts its OWN early closed steps rather than being retained verbatim (the failure mode that motivated dropping turn-protection: a tool-heavy turn must stay compactable or the harness dies exactly when compaction is needed). The only structural guard is **tool-pairing balance**: the compacted region's edges are balanced cuts on the surface (no unanswered tool-call crosses either edge), so it never splits a step's `assistant/message` tool-calls from their `tool/result`s. When the only compactable content left is an un-splittable open tail step, it declines (returns `null`) and retries once an older step closes. **Single-unit overflow is out of scope, by design**: if one retained unit (a single closed step, or a large pasted `user/message`) ALONE exceeds the budget, compaction cannot help and the call may go out over-budget — bounding an individual unit's size is a separate concern. `compactRegion()` enforces tool-pairing balance strictly, throwing on a boundary that would split a step. `dsh-session` exports `isToolPairingBalanced` for the check. - **Dynamic convergence** — no static summary-length config pretends to bound what the model will write. If framing/estimator/system overhead leaves the compacted surface above threshold, `compactIfNeeded()` re-compacts the head checkpoint up to `compactionRetries` extra times; if it still cannot get below threshold, it throws. A summary whose estimated stored size is not smaller than the shadowed content fails closed before it mutates the surface. -- **Summarization** — `summarize()`: a `GenerateOptions` request assembled via `BlockAssembler` with a fixed system prompt that asks for a structured checkpoint (Primary Request and Intent · Key Technical Concepts · Files and Code · Errors and Fixes · Pending Tasks · Current Work · Next Step · Critical Context), every section mandatory, exact paths/commands/identifiers preserved. The request runs through the `agent/request` waterfall before `ctx.llm.stream()`, so router agents that choose the concrete model there also route compaction summaries. `maxTokens` is the provider-side generation cap; only text blocks from the model's reply are kept before the checkpoint is stored (reasoning is dropped so private chain-of-thought never leaks into the durable summary, and a stray `tool-call` is dropped so the synthesized `user/message` summary cannot land an orphaned call with no matching `tool-result`). The compacted region is flattened to a plain-text transcript first: text and reasoning contribute their text, and every non-text block (image, tool-call, tool-result, plugin-added types) contributes a type-tagged placeholder (`[image]`, `[tool-call: name(args)]`, …) so the summarizer is told what existed rather than silently dropping it. +- **Summarization** — `summarize()`: a `GenerateOptions` request assembled via `BlockAssembler` with a fixed system prompt that asks for a structured checkpoint (Primary Request and Intent · Key Technical Concepts · Files and Code · Errors and Fixes · Pending Tasks · Current Work · Next Step · Critical Context), every section mandatory, exact paths/commands/identifiers preserved. The request runs through the `agent/request` waterfall before `ctx.llm.stream()`, so router agents that choose the concrete model there also route compaction summaries. `maxTokens` is the provider-side generation cap; only text blocks from the model's reply are kept before the checkpoint is stored (reasoning is dropped so private chain-of-thought never leaks into the durable summary, and a stray `tool-call` is dropped so the synthesized `user/message` summary cannot land an orphaned call with no matching `tool-result`). The compacted region is flattened to a plain-text transcript first: text and reasoning contribute their text, and every non-text block (tool-call, tool-result, plugin-added types) contributes a type-tagged placeholder (`[tool-call: name(args)]`, `[tool-result: …]`, …) so the summarizer is told what existed rather than silently dropping it. - **Checkpoint framing** — the raw summary is not landed directly. `compactRegion()` wraps it in a checkpoint preamble (so a resuming model reads it as a checkpoint, not a fresh user request, and builds on the captured context rather than restating it) plus `…` tags. Because region compaction can be invoked manually, a surface may hold several checkpoints, so the framing does not claim everything after it is recent or verbatim. The tags make a prior checkpoint detectable in the transcript on the next compaction cycle: the summarization prompt then instructs the model to merge it in place (preserve still-true facts, drop stale ones) rather than re-summarize it verbatim — a cheap incremental merge that needs no extra log/event machinery. The unframed summary stays on the `compact/summary` provenance event. - **Surface mutation** — `compactRegion()` appends the `compact/start` → `compact/summary` → `compact/end` log records and the single `user/message` replace node carrying the framed summary (see the interface README). - **Auto-compaction** — an `agent/pre-step` listener delegates to `compactIfNeeded()` before every step (not just a turn's first — a tool-heavy turn grows the surface mid-turn, so a runaway turn still compacts, and per-step firing is the only moment to rescue it before overflow). `agent/pre-step` is a serial (awaited, in-order) surface-mutation checkpoint that fires after `turn/start` and BEFORE the step opens (`step/start`) and its request history is derived, so compaction mutates the surface — with its log-only `compact/*` records landing cleanly outside any step — and the loop derives once from the result: no double-derive, and the listener cannot see (or need to rewrite) an already-assembled `messages` array. The listener owns no threshold logic of its own (the single token-pressure check lives in `compactIfNeeded()`); because Cordis `serial` bails early on non-void return values, the listener returns `void` and does not use the dispatcher's bail channel as a veto surface. diff --git a/packages/compact/compact-basic/src/index.ts b/packages/compact/compact-basic/src/index.ts index f53dace461..2986c3d15d 100644 --- a/packages/compact/compact-basic/src/index.ts +++ b/packages/compact/compact-basic/src/index.ts @@ -44,9 +44,6 @@ export { resolveConfig } from './types.ts' /** Per-block structural overhead for JSON framing / type tag. */ const BLOCK_OVERHEAD = 4 -/** Heuristic token count for an image block (~85 tokens for low-res URL). */ -const IMAGE_TOKEN_COST = 85 - /** Role-field framing overhead added per message in {@link BasicCompactService.estimateTokens}. */ const ROLE_OVERHEAD = 4 @@ -230,9 +227,6 @@ export class BasicCompactService extends CompactService { case 'tool-result': tokens += this.estimateContentTokens(block.content) + BLOCK_OVERHEAD break - case 'image': - tokens += IMAGE_TOKEN_COST - break default: // Unknown block types (merge-extensible ContentBlockMap): // estimate conservatively via JSON stringify. @@ -706,10 +700,10 @@ export class BasicCompactService extends CompactService { /** * Render content blocks to a single plain-text string for the summarization * prompt. Text and reasoning contribute their text; every other block type - * contributes a type-tagged placeholder (`[image]`, `[tool-call: name(args)]`, - * …) so the summarizer is told what non-text content existed in the region - * rather than silently losing it. Blocks join with newlines; empty-text - * blocks contribute nothing. + * contributes a type-tagged placeholder (`[tool-call: name(args)]`, + * `[tool-result: …]`, …) so the summarizer is told what non-text content + * existed in the region rather than silently losing it. Blocks join with + * newlines; empty-text blocks contribute nothing. */ private _blocksToText(blocks: readonly ContentBlock[]): string { const parts: string[] = [] @@ -729,9 +723,6 @@ export class BasicCompactService extends CompactService { parts.push(inner ? `[tool-result: ${inner}]` : '[tool-result]') break } - case 'image': - parts.push('[image]') - break // ContentBlockMap is merge-extensible — render an unknown block as a // bare type-tagged placeholder so a plugin-added block type is still // signalled to the summarizer rather than dropped. diff --git a/packages/compact/compact-basic/tests/compact-basic.spec.ts b/packages/compact/compact-basic/tests/compact-basic.spec.ts index 1929e656be..0e73e1cc1a 100644 --- a/packages/compact/compact-basic/tests/compact-basic.spec.ts +++ b/packages/compact/compact-basic/tests/compact-basic.spec.ts @@ -811,11 +811,6 @@ describe('BasicCompactService token estimation (char/4 heuristic)', () => { ])).toBe(10) }) - it('estimates image blocks at fixed 85 tokens', () => { - const svc = new BasicCompactService(new Context(), cfg({ auto: false })) - expect(svc.estimateContentTokens([{ type: 'image', url: 'https://example.com/img.png' }])).toBe(85) - }) - it('returns 0 for empty content blocks', () => { const svc = new BasicCompactService(new Context(), cfg({ auto: false })) expect(svc.estimateContentTokens([])).toBe(0) @@ -1324,7 +1319,7 @@ describe('BasicCompactService edge cases', () => { s.append('assistant/message', { turn: 1, step: 1, content: [ - { type: 'tool-result', toolCallId: CallId('n1'), content: [{ type: 'image', url: 'https://x/n.png' }] }, + { type: 'tool-result', toolCallId: CallId('n1'), content: [{ type: 'chart', data: 'x' } as unknown as ContentBlock] }, { type: 'custom-widget', payload: 'x' } as unknown as ContentBlock, { type: 'tool-call', id: CallId('b1'), name: 'bash', arguments: '{}' }, ], @@ -1343,7 +1338,7 @@ describe('BasicCompactService edge cases', () => { const nodes = s.surface.nodes await compactRegion(svc, s, nodes[0]!.seq, nodes[nodes.length - 1]!.seq, 'm') const { text } = svc.summarizeCalls[0]! - expect(text).toContain('[tool-result: [image]]') // nested tool-result with content + expect(text).toContain('[tool-result: [chart]]') // nested tool-result with content expect(text).toContain('[custom-widget]') // unknown block placeholder expect(text).toContain('Tool result (call b1): [tool-result]') // empty nested → bare placeholder }) @@ -1510,25 +1505,28 @@ describe('BasicCompactService edge cases', () => { it('renders non-text blocks as type-tagged placeholders across all message kinds', async () => { const svc = createTestService() const s = new Session(SessionId('placeholders')) + // A plugin-added block type (merge-extensible ContentBlockMap) — the + // placeholder path must cover every message kind, not just assistant. + const chart = (id: string): ContentBlock => ({ type: 'chart', data: id } as unknown as ContentBlock) s.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } }) s.append('step/start', { turn: 1, step: 1 }) - // user/message with only an image block → '[image]' placeholder. - s.append('user/message', { content: [{ type: 'image', url: 'https://x/y.png' }], source: { kind: 'user' } }, { surfaceOp: 'append' }) - // assistant/message with an image block AND the tool-call its tool/result - // answers (so the surface is tool-pairing balanced) → '[image]' placeholder. + // user/message with only a plugin-added block → '[chart]' placeholder. + s.append('user/message', { content: [chart('y')], source: { kind: 'user' } }, { surfaceOp: 'append' }) + // assistant/message with a plugin-added block AND the tool-call its + // tool/result answers (so the surface is tool-pairing balanced). s.append('assistant/message', { turn: 1, step: 1, content: [ - { type: 'image', url: 'https://x/z.png' }, + chart('z'), { type: 'tool-call', id: CallId('e1'), name: 'bash', arguments: '{}' }, ], }, { surfaceOp: 'append' }) - // tool/result with an image block → '[image]' placeholder. + // tool/result with a plugin-added block → '[chart]' placeholder. s.append('tool/call', { turn: 1, step: 1, callId: CallId('e1'), name: 'bash', arguments: '{}' }) - s.append('tool/result', { turn: 1, step: 1, callId: CallId('e1'), content: [{ type: 'image', url: 'https://x/r.png' }], isError: false }, { surfaceOp: 'append' }) - // context/message and steering/message with image content. - s.append('context/message', { content: [{ type: 'image', url: 'https://x/c.png' }], source: { kind: 'user' } }, { surfaceOp: 'append' }) - s.append('steering/message', { turn: 1, content: [{ type: 'image', url: 'https://x/s.png' }], source: { kind: 'user' } }, { surfaceOp: 'append' }) + s.append('tool/result', { turn: 1, step: 1, callId: CallId('e1'), content: [chart('r')], isError: false }, { surfaceOp: 'append' }) + // context/message and steering/message with plugin-added content. + s.append('context/message', { content: [chart('c')], source: { kind: 'user' } }, { surfaceOp: 'append' }) + s.append('steering/message', { turn: 1, content: [chart('s')], source: { kind: 'user' } }, { surfaceOp: 'append' }) s.append('step/end', { turn: 1, step: 1 }) s.append('turn/end', { turn: 1, reason: { kind: 'completed' } }) s.append('turn/start', { turn: 2, trigger: { kind: 'message', source: { kind: 'user' } } }) @@ -1537,11 +1535,11 @@ describe('BasicCompactService edge cases', () => { await compactRegion(svc, s, nodes[0]!.seq, nodes[nodes.length - 1]!.seq, 'm') const { text } = svc.summarizeCalls[0]! // Every non-text block surfaces as a placeholder rather than being dropped. - expect(text).toContain('User: [image]') - expect(text).toContain('Assistant: [image]') - expect(text).toContain('Tool result (call e1): [image]') - expect(text).toContain('[Context: [image]]') - expect(text).toContain('[Steering: [image]]') + expect(text).toContain('User: [chart]') + expect(text).toContain('Assistant: [chart]') + expect(text).toContain('Tool result (call e1): [chart]') + expect(text).toContain('[Context: [chart]]') + expect(text).toContain('[Steering: [chart]]') }) }) diff --git a/packages/llm/llm-deepseek/README.md b/packages/llm/llm-deepseek/README.md index 76af182580..66d6ae3e9b 100644 --- a/packages/llm/llm-deepseek/README.md +++ b/packages/llm/llm-deepseek/README.md @@ -34,7 +34,6 @@ A second, independent implementation of the same seam exists in `@deepseek-ai/ds ## Limitations (MVP, documented deliberately) - `prefill` throws `LlmError('UNSUPPORTED')` — DeepSeek's chat-prefix completion is a Beta feature on the `/beta` base URL; future work. -- `image` blocks are skipped (no vision support on these models). - `tool_choice` is not mapped (not part of the core vocabulary). ## Errors diff --git a/packages/llm/llm-deepseek/src/serialize.ts b/packages/llm/llm-deepseek/src/serialize.ts index 4e967d6667..c4e9dcb0a0 100644 --- a/packages/llm/llm-deepseek/src/serialize.ts +++ b/packages/llm/llm-deepseek/src/serialize.ts @@ -11,7 +11,6 @@ * rule for thinking mode — required there, ignored elsewhere, so we save * the tokens elsewhere); `tool-call` → `tool_calls[]` * - `tool-result` → its own `{role: 'tool'}` message (text flattened) - * - `image` → skipped (MVP limitation, documented in the README) * * @module dsh-llm-deepseek/serialize */ diff --git a/packages/llm/llm-deepseek/tests/serialize.spec.ts b/packages/llm/llm-deepseek/tests/serialize.spec.ts index 04387d7aa8..51d4788fe4 100644 --- a/packages/llm/llm-deepseek/tests/serialize.spec.ts +++ b/packages/llm/llm-deepseek/tests/serialize.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { CallId, LlmError } from '@deepseek-ai/dsh-llm' -import type { GenerateOptions, Message } from '@deepseek-ai/dsh-llm' +import type { ContentBlock, GenerateOptions, Message } from '@deepseek-ai/dsh-llm' import { serializeMessages, serializeRequest } from '@deepseek-ai/dsh-llm-deepseek' function request(overrides: Partial = {}): GenerateOptions { @@ -110,11 +110,17 @@ describe('serializeMessages', () => { ]) }) - it('skips image blocks (documented MVP limitation)', () => { + it('skips plugin-added block types (merge-extensible ContentBlockMap)', () => { const wire = serializeMessages([ - { role: 'user', content: [{ type: 'image', url: 'data:image/png;base64,x' }, { type: 'text', text: 'see image' }] }, + { + role: 'user', + content: [ + { type: 'chart', data: 'x' } as unknown as ContentBlock, + { type: 'text', text: 'see chart' }, + ], + }, ]) - expect(wire).toEqual([{ role: 'user', content: 'see image' }]) + expect(wire).toEqual([{ role: 'user', content: 'see chart' }]) }) it('emits an empty user message rather than dropping block-less messages', () => { diff --git a/packages/llm/llm-pi-ai/README.md b/packages/llm/llm-pi-ai/README.md index 70f61fdb62..ca34a8f586 100644 --- a/packages/llm/llm-pi-ai/README.md +++ b/packages/llm/llm-pi-ai/README.md @@ -31,7 +31,7 @@ pi-ai declares the openai/anthropic/google/mistral/AWS SDKs as install-time depe ## Limitations -Same MVP contract as llm-deepseek: `prefill` throws `UNSUPPORTED`, images are not representable, `tool_choice` is not mapped. +Same MVP contract as llm-deepseek: `prefill` throws `UNSUPPORTED`, `tool_choice` is not mapped. ## Testing diff --git a/packages/llm/llm-pi-ai/src/convert.ts b/packages/llm/llm-pi-ai/src/convert.ts index 0ddc41386a..6fa96a0597 100644 --- a/packages/llm/llm-pi-ai/src/convert.ts +++ b/packages/llm/llm-pi-ai/src/convert.ts @@ -93,7 +93,7 @@ export function toPiContext(options: GenerateOptions): PiContext { }) break default: - // image / plugin-added block types: not representable here. + // plugin-added block types: not representable here. break } } diff --git a/packages/llm/llm-pi-ai/tests/convert.spec.ts b/packages/llm/llm-pi-ai/tests/convert.spec.ts index be42c9e9b4..078d2a4d3b 100644 --- a/packages/llm/llm-pi-ai/tests/convert.spec.ts +++ b/packages/llm/llm-pi-ai/tests/convert.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { CallId } from '@deepseek-ai/dsh-llm' -import type { StreamChunk } from '@deepseek-ai/dsh-llm' +import type { ContentBlock, StreamChunk } from '@deepseek-ai/dsh-llm' import type { AssistantMessage, AssistantMessageEvent, Usage } from '@earendil-works/pi-ai' import { mapStopReason, mapUsage, toPiContext, toStreamChunks } from '@deepseek-ai/dsh-llm-pi-ai' @@ -171,13 +171,13 @@ describe('toPiContext', () => { expect(context.messages.map(message => message.role)).toEqual(['user', 'user', 'toolResult']) }) - it('skips image and unknown blocks in assistant content', () => { + it('skips plugin-added (unknown) blocks in assistant content', () => { const context = toPiContext({ model: 'm', messages: [{ role: 'assistant', content: [ - { type: 'image', url: 'data:,x' }, + { type: 'chart', data: 'x' } as unknown as ContentBlock, { type: 'text', text: 'visible' }, ], }], diff --git a/packages/llm/llm/README.md b/packages/llm/llm/README.md index 4227f5fdef..49a188e523 100644 --- a/packages/llm/llm/README.md +++ b/packages/llm/llm/README.md @@ -25,7 +25,7 @@ An adapter registry plus a single streaming call surface, interceptable via a wa ### Content-block vocabulary (`types.ts`) -Messages are arrays of typed content blocks: `text`, `reasoning`, `tool-call`, `tool-result`, `image`. The union is derived from the merge-extensible `ContentBlockMap`, so plugins can add block types via declaration merging. +Messages are arrays of typed content blocks: `text`, `reasoning`, `tool-call`, `tool-result`. The union is derived from the merge-extensible `ContentBlockMap`, so plugins can add block types via declaration merging. The core set is limited to blocks every shipping path honors — multimodal content (images, audio, …) has no core block type; a feature that needs one adds it via the map together with the adapter/UI/compaction support that honors it. Streaming is a raw chunk protocol (`block-start`, `text-delta`, `reasoning-delta`, `tool-call-delta`, `block-end`, `usage`, `finish`). `BlockAssembler` is the single shared implementation that assembles chunks into blocks/messages. diff --git a/packages/llm/llm/src/types.ts b/packages/llm/llm/src/types.ts index 7b1b9bdc47..fbfc7eb523 100644 --- a/packages/llm/llm/src/types.ts +++ b/packages/llm/llm/src/types.ts @@ -57,24 +57,22 @@ export interface ToolResultBlock { cache?: CacheHint } -/** An image, by URL or data URL. */ -export interface ImageBlock { - type: 'image' - url: string - mimeType?: string - cache?: CacheHint -} - /** * All known content block shapes, keyed by their `type` tag. * Merge-extensible: plugins add new block types via declaration merging. + * + * The core set is deliberately limited to blocks every shipping path honors. + * Multimodal content (images, audio, …) has no core block type: a feature + * that needs one adds it via declaration merging in the same coordinated + * change that maps it in the adapters, surfaces it in the UI bridges, and + * prices it in compaction — a producer never lands without its consumers + * (see docs/rfc/implemented/simplification/2026-07-04-drop-image-content-block.md). */ export interface ContentBlockMap { 'text': TextBlock 'reasoning': ReasoningBlock 'tool-call': ToolCallBlock 'tool-result': ToolResultBlock - 'image': ImageBlock } export type ContentBlockType = keyof ContentBlockMap diff --git a/packages/llm/llm/tests/assembler.spec.ts b/packages/llm/llm/tests/assembler.spec.ts index e8ad04e3b5..d9a4fe33f3 100644 --- a/packages/llm/llm/tests/assembler.spec.ts +++ b/packages/llm/llm/tests/assembler.spec.ts @@ -63,12 +63,12 @@ describe('BlockAssembler', () => { it('throws from assemble() when a partial has an unhandled blockType', () => { const assembler = new BlockAssembler() - // Directly push a block-end for an image block whose block-start never - // called ensure — but the image block-type flows through normally. - // What we really need is a partial whose blockType is not text/reasoning/tool-call. - // We can achieve this via a block-start for 'image' followed by blocks(). - assembler.push({ type: 'block-start', index: 0, blockType: 'image' } as unknown as StreamChunk) - expect(() => assembler.blocks()).toThrow('cannot assemble incomplete block of type "image"') + // A partial whose blockType is not text/reasoning/tool-call cannot be + // assembled without its block-end. A plugin-added block type (here + // 'video', via the merge-extensible ContentBlockMap) opened by a + // block-start with no closing block-end exercises that throw. + assembler.push({ type: 'block-start', index: 0, blockType: 'video' } as unknown as StreamChunk) + expect(() => assembler.blocks()).toThrow('cannot assemble incomplete block of type "video"') }) it('mustGet throws when an index is missing from the partials map (invariant violation)', () => { diff --git a/packages/llm/llm/tests/properties.spec.ts b/packages/llm/llm/tests/properties.spec.ts index c63d56abbb..3bb2a76c38 100644 --- a/packages/llm/llm/tests/properties.spec.ts +++ b/packages/llm/llm/tests/properties.spec.ts @@ -81,7 +81,7 @@ describe('BlockAssembler properties', () => { fc.assert(fc.property(streamArb, (chunks) => { const blocks = feed(chunks).blocks() for (const block of blocks) { - expect(['text', 'reasoning', 'tool-call', 'tool-result', 'image']).toContain(block.type) + expect(['text', 'reasoning', 'tool-call', 'tool-result']).toContain(block.type) } })) }) diff --git a/packages/ui/acp/src/codec.ts b/packages/ui/acp/src/codec.ts index 3b03a81c83..444e71545c 100644 --- a/packages/ui/acp/src/codec.ts +++ b/packages/ui/acp/src/codec.ts @@ -69,8 +69,8 @@ export function turnEndToStopReason(reason: TurnEndReason): StopReason { * client as message content. Today only `text` maps; `resource_link` is an * ACP prompt-only input rendered into text by {@link acpPromptToText}; * `reasoning` is surfaced via `agent_thought_chunk` - * streaming rather than as a message block, and `tool-call`/`tool-result`/ - * `image` are handled by the tool-call update path or not advertised. + * streaming rather than as a message block, and `tool-call`/`tool-result` + * are handled by the tool-call update path. */ export function harnessBlockToAcpContent(block: ContentBlock): AcpContentBlock | undefined { switch (block.type) { @@ -78,7 +78,7 @@ export function harnessBlockToAcpContent(block: ContentBlock): AcpContentBlock | return { type: 'text', text: block.text } // reasoning → streamed as agent_thought_chunk, not a message block // tool-call / tool-result → the tool_call / tool_call_update path - // image → not advertised + // plugin-added block types → not surfaced default: return undefined } diff --git a/packages/ui/acp/tests/codec.spec.ts b/packages/ui/acp/tests/codec.spec.ts index 859e8d40cd..b4f0c10792 100644 --- a/packages/ui/acp/tests/codec.spec.ts +++ b/packages/ui/acp/tests/codec.spec.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest' +import type { ContentBlock } from '@deepseek-ai/dsh-llm' import type { TurnEndReason } from '@deepseek-ai/dsh-session' import type { ContentBlock as AcpContentBlock } from '@agentclientprotocol/sdk' import { @@ -33,9 +34,9 @@ describe('harnessBlockToAcpContent', () => { expect(harnessBlockToAcpContent({ type: 'text', text: 'hi' })).toEqual({ type: 'text', text: 'hi' }) }) - it('returns undefined for non-text blocks (reasoning/tool/image)', () => { + it('returns undefined for non-text blocks (reasoning / plugin-added)', () => { expect(harnessBlockToAcpContent({ type: 'reasoning', text: 'think' })).toBeUndefined() - expect(harnessBlockToAcpContent({ type: 'image', url: 'https://x/y.png', mimeType: 'image/png' })).toBeUndefined() + expect(harnessBlockToAcpContent({ type: 'chart', data: 'x' } as unknown as ContentBlock)).toBeUndefined() }) }) diff --git a/packages/ui/acp/tests/stream-update.spec.ts b/packages/ui/acp/tests/stream-update.spec.ts index b580a5fc3d..036bb4f7bb 100644 --- a/packages/ui/acp/tests/stream-update.spec.ts +++ b/packages/ui/acp/tests/stream-update.spec.ts @@ -91,7 +91,7 @@ describe('streamSessionEventUpdate', () => { it('drops non-text tool-result content (text-only)', () => { const update = updatesFor(evt('tool/result', { turn: 1, step: 1, callId: CallId('c1'), - content: [{ type: 'image', url: 'https://x/y.png' }], + content: [{ type: 'reasoning', text: 'private' }], isError: false, }))[0] expect((update as { content: unknown[] }).content).toEqual([]) From bee8132a7f4d5f3a98c92314b9f5f65a36984b93 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:37:05 +0800 Subject: [PATCH 5/8] Add the no-hardcoded-tunables convention and its review check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A number or string that two reasonable deployments could want set differently — a timeout, grace period, output cap, result-count limit, model name, base URL — belongs on the plugin's schemastery Config with the shipped value as its default, not in a bare literal or module constant. A DEFAULT_* constant or a test-only injection seam is not configurability: the test is whether a cordis.yml deployment can change the value without a code edit. Protocol/wire constants, semantic constants, values pinned by an external spec, and security invariants stay hardcoded. The convention lands in AGENTS.md § Conventions (the authoritative source the review skill cites); dsh-code-review gains the matching reviewer-only check, since no mechanical gate can detect a hardcoded tunable. --- .agents/skills/dsh-code-review/SKILL.md | 3 ++- AGENTS.md | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 9dc4de5ca6..d23c68ab2b 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -21,7 +21,7 @@ Independent judgment governs *what to look at* and *how to apply a rule to this These define the conventions and gates this repo is checked against, and they are authoritative. Read them at the source so this skill never drifts out of sync — and apply judgment in *interpreting* them for the case at hand, not in deciding whether they apply. -- **[AGENTS.md](../../../AGENTS.md) § Conventions** — effect-based registrations, declaration-merging for events/ctx keys, waterfall `next()` discipline, discriminated-union match-don't-chain, explicit-over-implicit at seams, the empty-`catch` rule, symmetry. +- **[AGENTS.md](../../../AGENTS.md) § Conventions** — effect-based registrations, declaration-merging for events/ctx keys, waterfall `next()` discipline, discriminated-union match-don't-chain, explicit-over-implicit at seams, no hardcoded tunables in plugins, the empty-`catch` rule, symmetry. - **AGENTS.md § Defensive patterns (hard-won)** — each bullet is a bug class that bit us. Reviewing anything touching process lifecycle, async/await, disposal, or adapter error paths? Re-read this first — then look for the *adjacent* mistake it doesn't name. - **AGENTS.md § Type Safety and Documentation** — the doc-sync rule (code change ⇒ update README + JSDoc in the SAME commit) and the no-hard-wrap markdown convention. - **[packages/AGENTS.md](../../../packages/AGENTS.md)** — per-package conventions (file layout, the HMR-safety test requirement). @@ -44,6 +44,7 @@ Where your independent reasoning earns its keep. Start here, then keep going acr - **e2e verifies the world, not the agent's self-report.** For real-API tests, confirm the assertion re-runs the command/checks the file externally — a keyword probe lets a cheating agent pass (see AGENTS.md e2e bullet). For a behavior change to the agent's real flows, a no-key/mock test alone is usually insufficient: a with-key e2e (especially a smoke test that boots the real example and checks the world) is cheap here and catches "green units, broken product" — encourage it rather than treating real-API tests as expensive (see AGENTS.md § Secrets / .env). - **Plugin export shape + real-loader coverage.** A new/changed `cordis.yml`-loaded plugin: is it a function/namespace plugin (`name`/`inject`/`Config`/`apply` named exports) with NO `export default`? A stray default export makes the Loader's `unwrapExports` drop `inject` and the plugin crashes at load with `cannot get property … without inject` — invisible to hand-built `ctx.plugin({...})` tests and to line coverage. Confirm there's a test driving it through the REAL loader path (the no-key subprocess e2e for ACP is the model). And any opportunistic read of a service NOT in `static inject` should use `ctx.get(name)`, not `ctx.` (the property proxy throws through a foreign shadow). See [packages/AGENTS.md](../../../packages/AGENTS.md) and [docs/postmortem/0001](../../../docs/postmortem/0001-acp-default-export-drops-inject.md). - **Seam discipline.** New swappable capability? Check it's split per the capability-seams RFC (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type. +- **Hardcoded tunables that should be plugin config.** A literal timeout, grace period, output/truncation cap, result-count limit, retry count, buffer size, model name, API base URL, user agent, or filesystem path introduced inside a plugin belongs on the plugin's schemastery `Config` with the shipped value as its default (AGENTS.md § Conventions "No hardcoded tunables in plugins"). A named `DEFAULT_*` constant or a test-only injection seam is not configurability — the question to ask is whether a `cordis.yml` deployment can change the value without a code edit. Protocol/wire constants, semantic constants, values pinned by an external spec, and security invariants are exempt; a new `Config` field also needs its README row and range validation. No gate detects a hardcoded tunable — this check is entirely on the reviewer. - **Test quality — sufficiency, not just coverage.** 100% per-file coverage and a green suite are necessary, not sufficient: they prove the lines *ran*, not that the feature *works the way it ships*. Judge whether the tests are sufficient on two axes. (1) **Would they fail if the behavior regressed?** A test that passes but asserts the wrong thing — or restates the implementation instead of the contract (events fired, disposal reached, the world changed) — is worse than none. (2) **Do they exercise the REAL thing, the way it's actually used?** Prefer the genuine collaborator over a fake, drive the change through its real entry path (the cordis Loader, the ACP bridge, a booted subprocess — not a hand-built `ctx.plugin({...})` that bypasses `unwrapExports`), and verify the WORLD (re-read the file/log/registry externally), not the agent's self-report. A test that fakes the inputs just enough to cover every line will agree with whatever the author assumed; the real thing won't. When a test sets up a *clean/happy* path to reach a line, ask whether the line's PURPOSE is exercised — e.g. a durability/teardown path "tested" by a fully-completed turn never proves the mid-flight teardown it exists for; a torn-tail recovery branch covered by a well-formed log never proves recovery. Flag tests that hit the line but not the scenario. See AGENTS.md § Defensive patterns "Line coverage is not behavior coverage" and "Prefer the REAL implementation over a mock/stand-in in tests". - **Snapshot coverage for transcript/UX changes.** If the PR changes the editor-facing transcript or end-to-end agent UX — the ACP bridge's event→update translation, the agent loop's observable output, tool presentation, or anything an editor renders — it must add or update a snapshot scenario (`examples/*/tests/**/*.snapshot.ts`, goldens under `examples/acp-agent/tests/snapshots/`) or note explicitly why none applies (AGENTS.md § Conventions). Review the golden diff itself: a changed `stdout.golden.txt` / `session.golden.txt` is a behavior change in disguise — confirm it's intended, not an accidental regression someone re-recorded away. A pure internal refactor with no observable-output change is exempt, but the PR should say so. See [docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md](../../../docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md). - **Bilingual docs: review translation quality, not just pairing.** If the PR adds or edits a doc pair, read the changed English and Chinese sides and compare the meaning, not only the mechanical diff. Verify terms against [terminology.md](../../../docs/i18n/terminology.md), including first-occurrence annotations and "do not translate as" prohibitions; if a new term has no established precedent, the PR should keep it in English, list it under `待定术语`, and update the terminology table once the rendering is decided. A green `verify-translation-pairing` only proves hashes, switchers, and structure were recorded — it does not prove the translation is faithful, natural, or correctly termed. Treat [translation-rules.md](../../../docs/i18n/translation-rules.md) MUST/MUST NOT violations as blocking. diff --git a/AGENTS.md b/AGENTS.md index ee5eb7d6f0..f9acd0e629 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -261,6 +261,7 @@ Dev/test/demo run **unbuilt** via tsx + the source `paths` map in the root `tsco - **Plugins, not loop changes**: new behavior goes into a plugin on the documented extension seams (see the plugin sanity checklist in docs/architecture.md). Changing `agent-loop` requires updating that doc. - **Capability seams are three packages**: when adding a swappable capability (an execution backend, a provider integration, …), split it into *interface* (abstract service + vocabulary types, e.g. `bash/`), *implementation* (a concrete subclass, e.g. `bash-local/`), and *consumer* (what the model/plugins see, e.g. `tool-bash/`). Implementations and consumers then evolve independently — a sandboxed executor replaces `bash-local` without touching tool schemas. The LLM seam follows the same shape (`llm/` is interface + consumer surface; adapters are implementations). See docs/architecture.md § "Capability seams" for when NOT to split. - **Explicit > implicit at package seams**: interface/vocabulary types spell out every field a consumer must supply — no optional field that the implementation silently fills with a hidden `?? default`. Put defaulting in the owning implementation as an explicit step (a `resolve(request): Spec` method that turns the optional-field request into the required-field spec), not smuggled inside `run()`/`start()`. Example: `dsh-bash` splits `BashExecRequest` (optional `workdir`/`timeoutMs`, model-facing) from `BashExecSpec` (required, what `run`/`start` act on); the tool layer calls `ctx.bash.resolve()` between them. The reader of a `BashExecSpec` never has to wonder where the working directory came from. +- **No hardcoded tunables in plugins — a deployment knob belongs on `Config`**: a number or string that two reasonable deployments could want set differently — a timeout, grace period, output/truncation cap, result-count limit, retry count, buffer size, model name, API base URL, user agent, filesystem path — is plugin configuration, not a bare literal or module constant. Expose it as a field on the plugin's schemastery `Config` with the shipped value as its `.default(…)`, document it in the package README, and validate the range where garbage would misbehave silently (see `assertPositiveFinite` in `dsh-bash-local`/`dsh-web-fetch-local`). A named `DEFAULT_*` constant does not make a value configurable, and neither does a test-only injection seam (`internals`) — the test is whether a `cordis.yml` deployment can change the value without a code edit. The rule is scoped to genuine tunables: protocol/wire constants (format versions, method names, event tags), semantic constants (exit codes, signal names, HTTP statuses), values pinned by an external spec, and security invariants (the credential-scrub env pattern) stay hardcoded — making those configurable invites misconfiguration, not flexibility. When unsure, ask who would ever set it: a deployer tuning the product (config) or only a maintainer changing the design (constant). The exemplar is `dsh-web-fetch-local`, whose every cap is a defaulted `Config` field. - **Opaque cross-boundary ids are branded, never bare `string`**: an identity that crosses a package seam and that a consumer must store-and-return but never parse (a backend-defined version token, a target key, a task/session/call id) is a `Branded` from `@deepseek-ai/dsh-brand` with a same-named cast factory in the owning package — a zero-cost compile-time guard so semantically-distinct strings stop being interchangeable. Not every string needs it: author-readable names (`ToolName`) and closed code unions (`ErrorCode`) don't. See [Branded IDs everywhere they belong](docs/rfc/implemented/architecture/2026-06-20-branded-ids.md). - **An empty `catch` must name what it swallows and why nothing else can hit it**: a bare `catch {}` hides bugs. When you deliberately ignore a throw, the comment must (a) name the single expected failure, (b) say why ignoring it is correct — usually because the useful state was already captured *before* the `try` — and (c) make clear nothing else of consequence can reach the catch (ideally the `try` wraps a single statement). Example: the error-body `response.json()` parse in `dsh-llm-deepseek`'s adapter sets `code` + HTTP `status` from the status line before the `try`, so a malformed provider body can only cost a richer message, never the real error. - **Symmetry is usually more correct**: when two related values play parallel roles (a test fixture and its expected output, a request shape and its response shape, a buggy input and the test that checks the fix), give them parallel form — both named consts, or both inline, not one each way. Asymmetry is a smell that usually points at a missed extraction. From 774d460889b90ace04eabaf3a8fcff633b6e590a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:37:23 +0800 Subject: [PATCH 6/8] Expose audited hardcoded tunables as plugin config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit swept every packages/*/* plugin for the new AGENTS.md convention (no hardcoded tunables in plugins) and exposes each finding as a defaulted, validated Config field. Defaults are the previously hardcoded values throughout, so no deployment or golden changes. - tool-fs (had NO Config): readLimit, readMaxLineLength, readMaxBytes, readStreamMinSize. The caps thread through ReadToolCaps/ReadWindow — read-render already documented that the consumer applies the caps, so they become explicit per-request fields. - tool-web: searchMaxResults (WEB_SEARCH_MAX_RESULTS stays as the schemastery default). Also fixes the stale GREP_LIMIT references in search.ts and the web-capability-seam RFC (no such constant exists). - bash-local: graceMs (SIGTERM->SIGKILL escalation grace). The RunInternals.graceMs test seam is gone: graceMs is now a required SpawnSpec field filled from config, so tests exercise the real config path and the defaults live in exactly one place. - subagent-acp: disposeEofGraceMs / disposeGraceMs. The AcpRunSpec fields become required for the same one-defaulting-layer reason. - session-persistence-sqlite: journalMode ('wal' default; the rollback-journal modes serve filesystems where WAL's shared-memory files do not work, e.g. network mounts). - hooks-claude + hooks-codex: stderrSummaryMaxChars for the persisted hook/result stderr summary. The duplicated summarize() helpers merge into hook-protocol's summarizeStderr(stderr, maxChars), beside the HookResultRecord field it feeds, with the bound parameterized the same way runHook's defaultTimeoutMs already is. - compact-basic: charsPerToken for the token estimator (default 4, the English-text heuristic; CJK-heavy deployments need ~1-2 or compaction fires far too late). Also corrects the BasicCompactService class doc, which claimed defaults the required-field config never had. - fs-local: deletes the dead STREAM_MIN_SIZE constant and the dead FsIoInternals.streamMinSize seam — the read-routing bound lives in the consumer (tool-fs), where it is now config. This is item 1 of the proposed prune-write-only-fs-surface RFC, annotated accordingly. Every new field gets range validation (following the existing assertPositiveFinite pattern), a README row, and tests covering the configured behavior, the schema default, and load-time rejection. --- docs/core-data-structures/web.md | 2 +- .../2026-06-24-web-capability-seam.md | 2 +- .../2026-07-04-prune-write-only-fs-surface.md | 2 +- packages/bash/bash-local/README.md | 3 +- packages/bash/bash-local/src/index.ts | 12 +++- packages/bash/bash-local/src/run.ts | 11 ++- .../bash/bash-local/tests/executor.spec.ts | 27 +++++--- packages/bash/bash-local/tests/run.spec.ts | 3 +- packages/bash/tool-bash/tests/tools.spec.ts | 16 ++--- packages/compact/compact-basic/README.md | 1 + packages/compact/compact-basic/src/index.ts | 31 +++++---- packages/compact/compact-basic/src/types.ts | 31 +++++++-- .../compact-basic/tests/compact-basic.spec.ts | 17 +++++ packages/fs/fs-local/src/fsio.ts | 10 +-- packages/fs/fs-local/src/index.ts | 1 - packages/fs/tool-fs/README.md | 13 +++- packages/fs/tool-fs/package.json | 3 +- packages/fs/tool-fs/src/index.ts | 49 +++++++++++++- packages/fs/tool-fs/src/read-render.ts | 27 ++++---- packages/fs/tool-fs/src/read.ts | 44 ++++++++---- packages/fs/tool-fs/tests/read-render.spec.ts | 24 +++++-- packages/fs/tool-fs/tests/tools.spec.ts | 67 +++++++++++++++++++ packages/fs/tool-fs/tsconfig.json | 1 + packages/hooks/hook-protocol/src/events.ts | 12 ++++ packages/hooks/hook-protocol/src/index.ts | 2 +- .../hooks/hook-protocol/tests/events.spec.ts | 19 +++++- packages/hooks/hooks-claude/README.md | 1 + packages/hooks/hooks-claude/src/index.ts | 14 ++-- .../hooks/hooks-claude/tests/coverage.spec.ts | 17 ++++- packages/hooks/hooks-codex/README.md | 1 + packages/hooks/hooks-codex/src/index.ts | 13 ++-- .../hooks/hooks-codex/tests/coverage.spec.ts | 17 ++++- .../session-persistence-sqlite/README.md | 3 +- .../session-persistence-sqlite/src/index.ts | 21 ++++-- .../session-persistence-sqlite/src/schema.ts | 21 ++++-- .../tests/sqlite.spec.ts | 50 ++++++++++---- packages/subagent/subagent-acp/README.md | 2 + packages/subagent/subagent-acp/src/index.ts | 32 ++++++++- packages/subagent/subagent-acp/src/run.ts | 37 +++++----- .../subagent-acp/tests/subagent-acp.spec.ts | 18 ++++- packages/web/tool-web/README.md | 1 + packages/web/tool-web/src/index.ts | 22 +++++- packages/web/tool-web/src/search.ts | 14 ++-- packages/web/tool-web/tests/tool-web.spec.ts | 46 +++++++++++++ pnpm-lock.yaml | 3 + 45 files changed, 592 insertions(+), 171 deletions(-) diff --git a/docs/core-data-structures/web.md b/docs/core-data-structures/web.md index 1adde3bd75..42797a8d99 100644 --- a/docs/core-data-structures/web.md +++ b/docs/core-data-structures/web.md @@ -10,7 +10,7 @@ Search and fetch share no request schema and no business logic, but they are del ## Search request and result -The model-facing tool argument is just a `query`; `maxResults` is a consumer-owned bound (`dsh-tool-web`'s `WEB_SEARCH_MAX_RESULTS`, default `8`) passed through the seam and enforced on the way back — if a provider over-returns, the seam truncates `sources[]` and sets `truncated`. +The model-facing tool argument is just a `query`; `maxResults` is a consumer-owned bound (`dsh-tool-web`'s `searchMaxResults` config, default `8`) passed through the seam and enforced on the way back — if a provider over-returns, the seam truncates `sources[]` and sets `truncated`. ```ts type-equiv interface WebSearchRequest { diff --git a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md index 55ada9d576..833cbeebb5 100644 --- a/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md +++ b/docs/rfc/implemented/architecture/2026-06-24-web-capability-seam.md @@ -175,7 +175,7 @@ The first `web_search` model-facing tool should be small. The only model-facing - `query`: required string. -`max_results` is NOT exposed to the model in the first version. It is a `dsh-tool-web`-layer decision: the tool sets the result bound — a default of `8` (aligning with OpenCode's Exa default), as an exported constant mirroring `dsh-tool-fs`'s `READ_LIMIT` / `GREP_LIMIT` — and passes it to the seam as `maxResults` on the `WebSearchRequest`. Keeping it off the model schema means the model just asks a question and the product controls how much context comes back; the field can be promoted to a model-facing argument later without breaking the seam. +`max_results` is NOT exposed to the model in the first version. It is a `dsh-tool-web`-layer decision: the tool sets the result bound — the `searchMaxResults` plugin config, default `8` (aligning with OpenCode's Exa default), mirroring `dsh-tool-fs`'s `readLimit` — and passes it to the seam as `maxResults` on the `WebSearchRequest`. Keeping it off the model schema means the model just asks a question and the product controls how much context comes back; the field can be promoted to a model-facing argument later without breaking the seam. `maxResults` flows tool → seam → provider, and the bound is enforced on the way back: diff --git a/docs/rfc/proposed/simplification/2026-07-04-prune-write-only-fs-surface.md b/docs/rfc/proposed/simplification/2026-07-04-prune-write-only-fs-surface.md index 0a4bc14d89..604c1774d5 100644 --- a/docs/rfc/proposed/simplification/2026-07-04-prune-write-only-fs-surface.md +++ b/docs/rfc/proposed/simplification/2026-07-04-prune-write-only-fs-surface.md @@ -6,7 +6,7 @@ Status: proposed The [fs seam split](../../implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) moved read routing and policy out of the backend into `dsh-tool-fs` and `dsh-fs-policy`. Four pieces of surface kept the pre-split shape — populated on every call, read by nobody: -1. **`STREAM_MIN_SIZE` + `FsIoInternals.streamMinSize` in `dsh-fs-local`** (`packages/fs/fs-local/src/fsio.ts`, re-exported from `packages/fs/fs-local/src/index.ts`): zero readers anywhere, including fs-local's own source and tests. The backend has no read routing — `readWholeText`/`streamWholeText` are separate primitives the caller chooses between — and the real routing constant lives in the consumer (`packages/fs/tool-fs/src/read.ts`, compared against `info.size`). Two mirrors of the 10 MiB fact; the backend's is dead, and the knob's JSDoc claims a "read routing" override that does not exist. +1. **`STREAM_MIN_SIZE` + `FsIoInternals.streamMinSize` in `dsh-fs-local`** — *already removed by the no-hardcoded-tunables audit (the routing bound became `dsh-tool-fs`'s `readStreamMinSize` config); listed here for the record of the full prune, no work remains.* Originally (`packages/fs/fs-local/src/fsio.ts`, re-exported from `packages/fs/fs-local/src/index.ts`): zero readers anywhere, including fs-local's own source and tests. The backend has no read routing — `readWholeText`/`streamWholeText` are separate primitives the caller chooses between — and the real routing constant lives in the consumer (`packages/fs/tool-fs/src/read.ts`, compared against `info.size`). Two mirrors of the 10 MiB fact; the backend's is dead, and the knob's JSDoc claims a "read routing" override that does not exist. 2. **`FsTarget.inputPath`** (`packages/fs/fs/src/types.ts`): every backend and every test fake must fabricate a "diagnostics only" value with zero production readers — the policy plugin and every error message use `targetKey`/`displayPath`. The `listDir` producer exposes the semantic wobble: directory children get the bare entry name, which was nobody's "input". 3. **`FsEditOutcome.replacements` + `.replaceAll`** (`packages/fs/fs/src/types.ts`): `replacements` has zero production readers (the single-match policy itself stays — it is enforced by the `FS_AMBIGUOUS_EDIT`/`FS_EDIT_NOT_FOUND` throws inside the backend, whose error message keeps the internal count); `replaceAll` is read only by `formatEditOutput` in `packages/fs/tool-fs/src/edit.ts` — as an echo of the `replace_all` argument the tool already holds. Shrunk, `FsEditOutcome` becomes `{ version, before, after }`, parallel to `FsWriteOutcome`'s genuinely backend-discovered fields. 4. **`FileReadOutcome.limit` + `.version`** (`packages/fs/tool-fs/src/read-render.ts`): populated by the read tool, but `formatReadOutput` renders `offset`/`lines`/`totalLines`/`truncatedByBytes` only, and the `fs/observed` emit uses `info.version` directly rather than the outcome copy. diff --git a/packages/bash/bash-local/README.md b/packages/bash/bash-local/README.md index 109debc24c..c13a3ab923 100644 --- a/packages/bash/bash-local/README.md +++ b/packages/bash/bash-local/README.md @@ -12,6 +12,7 @@ Local-subprocess implementation of the `@deepseek-ai/dsh-bash` executor seam: `L timeoutMs: 120000 # default foreground timeout maxTimeoutMs: 600000 # cap for per-call overrides maxOutputBytes: 64000 # per-stream in-memory cap; overflow spills to disk + graceMs: 3000 # SIGTERM→SIGKILL escalation grace on kills ``` ## Behavior (and where it came from) @@ -19,7 +20,7 @@ Local-subprocess implementation of the `@deepseek-ai/dsh-bash` executor seam: `L Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; the notable choices: - **Spawn per call, no shell state** — every call is a fresh non-login `bash -c` (deterministic; no rc files). All four surveyed tools spawn per call. `XXX(stateful-shell)` in `src/run.ts` records the two proven stateful designs (Claude Code's cwd-only persistence; Codex's PTY exec sessions) for when real workflows demand them. -- **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after a 3s grace (OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools. +- **Process-group kills with escalation** — children are spawned `detached` (own process group); kills send SIGTERM to the group, then SIGKILL after the `graceMs` grace (default 3s — OpenCode's escalation; pipelines and subshells die with the parent). ESRCH is tolerated; daemons that re-parent away from the group can still survive — same caveat as the surveyed tools. - **Tail-keep truncation + spill files** — output beyond `maxOutputBytes` keeps the in-memory TAIL (errors/results cluster at the end — pi/OpenCode rationale) while the FULL stream is appended to a temp file whose path is reported when available. If the final spill close reports a delayed writeback failure, the executor still returns the tail but withholds the path rather than advertising a possibly incomplete file. - **Model-friendly env + credential scrub** — `process.env` minus credential-shaped vars (`*KEY*`/`*SECRET*`/`*TOKEN*`), then `NO_COLOR=1 TERM=dumb PAGER=cat GIT_PAGER=cat` (Codex's hardcoded set) so pagers and ANSI color don't garble results. This scrub is the security control that keeps the harness's *ambient* credentials out of a spawned command. A spec's `env` is merged LAST (after the scrub), so a caller's explicit entry — a value it already holds — wins even on a credential-shaped name. The spec's `stdin`, when supplied, is written to the child and closed; with none supplied, fd 0 is `/dev/null` — the exact pre-seam default, so a command that probes stdin's file type is unaffected. Both `env`/`stdin` are set by in-process plugins (the hooks bridges); the model-facing tool doesn't expose them. See [the bash-stdin-env RFC](../../../docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md). - **Background tasks** — `start()` returns immediately, no timeout applies (Claude Code detaches timeouts when backgrounding), `readOutput()` is incremental with whole-stream byte offsets, and disposal kills everything. The spec's opaque `owner` token is stored on the tracked task and returned by `ownerOf(id)` — the executor never interprets it (the consumer's access policy does), and because it lives with the task here it survives a `tool-bash` HMR reload. diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index af4c47e71e..6df7b3da7b 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -17,7 +17,7 @@ import { Context } from 'cordis' import z from 'schemastery' import { BashExecutor, BashTaskId } from '@deepseek-ai/dsh-bash' import type { BashExecRequest, BashExecSpec, BashRunResult, BashTask, BashTaskRead, OwnerToken } from '@deepseek-ai/dsh-bash' -import { runBash } from './run.ts' +import { DEFAULT_GRACE_MS, runBash } from './run.ts' import type { RunInternals, RunningBash } from './run.ts' export { DEFAULT_GRACE_MS, ENV_OVERRIDES, killGroup, OutputCollector, runBash } from './run.ts' @@ -33,6 +33,8 @@ export interface Config { maxTimeoutMs?: number /** Per-stream in-memory output cap; overflow spills to a temp file. */ maxOutputBytes?: number + /** Grace period between the SIGTERM and the SIGKILL escalation on a kill. */ + graceMs?: number } /** The shape after schemastery applied the defaults (cwd has none). */ @@ -57,7 +59,7 @@ interface TrackedTask extends BashTask { * Local-subprocess bash executor. Defaults follow the agent-tool survey * consensus: 120s default / 600s max timeout (Claude Code, OpenCode), 64KB * in-memory output with full-stream spill files (pi, OpenCode), - * process-group SIGTERM→SIGKILL kills (OpenCode). + * process-group SIGTERM→SIGKILL kills with a 3s grace (OpenCode). */ export class LocalBashExecutor extends BashExecutor { static Config: z = z.object({ @@ -65,11 +67,12 @@ export class LocalBashExecutor extends BashExecutor { timeoutMs: z.number().default(120_000), maxTimeoutMs: z.number().default(600_000), maxOutputBytes: z.number().default(64_000), + graceMs: z.number().default(DEFAULT_GRACE_MS), }) private tasks = new Map() private nextTaskId = 1 - /** Test seam: timer/spill knobs forwarded to runBash. */ + /** Test seam: spill knobs forwarded to runBash. */ internals: RunInternals = {} /** Validated config (schemastery applied the defaults before construction). */ @@ -83,6 +86,7 @@ export class LocalBashExecutor extends BashExecutor { assertPositiveFinite('timeoutMs', this.config.timeoutMs) assertPositiveFinite('maxTimeoutMs', this.config.maxTimeoutMs) assertPositiveFinite('maxOutputBytes', this.config.maxOutputBytes) + assertPositiveFinite('graceMs', this.config.graceMs) ctx.effect(() => async () => { // Kill every live process group and WAIT for the processes to close so // nothing outlives the fiber (HMR safety) — a TERM-trapping child is @@ -132,6 +136,7 @@ export class LocalBashExecutor extends BashExecutor { cwd: spec.workdir, timeoutMs: spec.timeoutMs, maxOutputBytes: this.config.maxOutputBytes, + graceMs: this.config.graceMs, signal: spec.signal, stdin: spec.stdin, env: spec.env, @@ -150,6 +155,7 @@ export class LocalBashExecutor extends BashExecutor { cwd: spec.workdir, timeoutMs: 0, maxOutputBytes: this.config.maxOutputBytes, + graceMs: this.config.graceMs, signal: spec.signal, stdin: spec.stdin, env: spec.env, diff --git a/packages/bash/bash-local/src/run.ts b/packages/bash/bash-local/src/run.ts index 023ea0e3d1..489d380787 100644 --- a/packages/bash/bash-local/src/run.ts +++ b/packages/bash/bash-local/src/run.ts @@ -73,6 +73,8 @@ export interface SpawnSpec { timeoutMs: number /** Per-stream in-memory cap; overflow spills to disk (tail kept in memory). */ maxOutputBytes: number + /** Grace period between the SIGTERM and the SIGKILL escalation on a kill. */ + graceMs: number /** Abort signal — kills the process group when fired. */ signal?: AbortSignal | undefined /** @@ -100,15 +102,13 @@ export interface SpawnOutcome { stderr: CollectedOutput } -/** Injectable knobs so tests can exercise escalation/spill without long waits. */ +/** Injectable knobs so tests can exercise spill behavior without the OS tmpdir. */ export interface RunInternals { - /** Grace period between SIGTERM and SIGKILL on the process group. */ - graceMs?: number /** Directory for spill files (defaults to the OS temp dir). */ spillDir?: string } -/** Default SIGTERM→SIGKILL grace period (matches OpenCode's 3s). */ +/** Default SIGTERM→SIGKILL grace period (the `graceMs` config; matches OpenCode's 3s). */ export const DEFAULT_GRACE_MS = 3_000 let spillCounter = 0 @@ -292,7 +292,6 @@ export interface RunningBash { * no inherited shell state); revisit when real workflows demand it. */ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningBash { - const graceMs = internals.graceMs ?? DEFAULT_GRACE_MS const spillDir = internals.spillDir ?? privateSpillDir() if (spec.signal?.aborted) { @@ -331,7 +330,7 @@ export function runBash(spec: SpawnSpec, internals: RunInternals = {}): RunningB const kill = (): void => { if (graceTimer !== undefined) return // escalation already in flight killGroup(pid, 'SIGTERM') - graceTimer = setTimeout(() => { killGroup(pid, 'SIGKILL') }, graceMs) + graceTimer = setTimeout(() => { killGroup(pid, 'SIGKILL') }, spec.graceMs) } if (spec.timeoutMs > 0) { diff --git a/packages/bash/bash-local/tests/executor.spec.ts b/packages/bash/bash-local/tests/executor.spec.ts index cf6d1c267e..ce89b2a0ae 100644 --- a/packages/bash/bash-local/tests/executor.spec.ts +++ b/packages/bash/bash-local/tests/executor.spec.ts @@ -11,9 +11,10 @@ const spillDir = mkdtempSync(join(tmpdir(), 'dsh-bash-exec-spec-')) async function setup(config: ConstructorParameters[1] = {}) { const ctx = new Context() - await ctx.plugin(LocalBashExecutor, config) + // A short kill grace via the REAL config path, so escalation tests stay fast. + await ctx.plugin(LocalBashExecutor, { graceMs: 200, ...config }) const bash = ctx.bash as LocalBashExecutor - bash.internals = { spillDir, graceMs: 200 } + bash.internals = { spillDir } return { ctx, bash } } @@ -80,12 +81,22 @@ describe('LocalBashExecutor.run', () => { await expect(setup({ timeoutMs: Number.NaN })).rejects.toThrow(/timeoutMs/) await expect(setup({ maxTimeoutMs: 0 })).rejects.toThrow(/maxTimeoutMs/) await expect(setup({ maxOutputBytes: -1 })).rejects.toThrow(/maxOutputBytes/) + await expect(setup({ graceMs: 0 })).rejects.toThrow(/graceMs/) const { bash } = await setup() expect(() => bash.resolve({ command: 'true', timeoutMs: Number.NaN })).toThrow(/request\.timeoutMs/) expect(() => bash.resolve({ command: 'true', timeoutMs: -1 })).toThrow(/request\.timeoutMs/) }) + it('kill escalation uses the configured graceMs (a TERM-trapping task dies by SIGKILL)', async () => { + const { bash } = await setup() // setup pins graceMs: 200 via config + const task = bash.start(bash.resolve({ command: 'trap \'\' TERM; sleep 60' })) + await new Promise(resolve => setTimeout(resolve, 100)) + bash.kill(task.id) + await task.done + expect(task.signal).toBe('SIGKILL') + }) + it('per-call timeout takes precedence under the cap and kills on expiry', async () => { const { bash } = await setup({ timeoutMs: 60_000 }) const result = await bash.run(bash.resolve({ command: 'sleep 60', timeoutMs: 100 })) @@ -271,9 +282,9 @@ describe('LocalBashExecutor background tasks', () => { it('disposing with already-finished tasks only kills the running ones', async () => { const ctx = new Context() - const fiber = await ctx.plugin(LocalBashExecutor, {}) + const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 }) const bash = ctx.bash as LocalBashExecutor - bash.internals = { spillDir, graceMs: 200 } + bash.internals = { spillDir } const finished = bash.start(bash.resolve({ command: 'true' })) await finished.done @@ -288,9 +299,9 @@ describe('LocalBashExecutor background tasks', () => { it('disposing the executor fiber kills running tasks (no orphans)', async () => { const ctx = new Context() - const fiber = await ctx.plugin(LocalBashExecutor, {}) + const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 }) const bash = ctx.bash as LocalBashExecutor - bash.internals = { spillDir, graceMs: 200 } + bash.internals = { spillDir } const listener = vi.fn() bash.onTaskDone(listener) @@ -337,9 +348,9 @@ describe('review fixes: lifecycle hardening', () => { it('dispose AWAITS a TERM-trapping process (SIGKILL escalation included)', async () => { const ctx = new Context() - const fiber = await ctx.plugin(LocalBashExecutor, {}) + const fiber = await ctx.plugin(LocalBashExecutor, { graceMs: 200 }) const bash = ctx.bash as LocalBashExecutor - bash.internals = { spillDir, graceMs: 200 } + bash.internals = { spillDir } const task = bash.start(bash.resolve({ command: 'trap \'\' TERM; sleep 60' })) await new Promise(resolve => setTimeout(resolve, 100)) diff --git a/packages/bash/bash-local/tests/run.spec.ts b/packages/bash/bash-local/tests/run.spec.ts index 3a1ff7c2c5..d2888e2fee 100644 --- a/packages/bash/bash-local/tests/run.spec.ts +++ b/packages/bash/bash-local/tests/run.spec.ts @@ -28,6 +28,7 @@ function spec(command: string, overrides: Partial[0]> cwd: process.cwd(), timeoutMs: 0, maxOutputBytes: 64_000, + graceMs: 3_000, ...overrides, } } @@ -106,7 +107,7 @@ describe('runBash', () => { }) it('escalates to SIGKILL when SIGTERM is trapped', async () => { - const running = runBash(spec('trap \'\' TERM; echo ready; sleep 60'), { graceMs: 200 }) + const running = runBash(spec('trap \'\' TERM; echo ready; sleep 60', { graceMs: 200 })) await waitForStdout(running, 'ready\n') running.kill() const result = await running.done diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index 82b69fb133..a01bdc0c86 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -21,8 +21,8 @@ async function setup() { await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry) await ctx.plugin(AgentRegistry) - await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) - ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } + await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000, graceMs: 200 }) + ;(ctx.bash as LocalBashExecutor).internals = { spillDir } await ctx.plugin(ToolBash) return ctx } @@ -184,8 +184,8 @@ describe('bash tool', () => { const ctx = new Context() await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry) - await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 }) - ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } + await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100, graceMs: 200 }) + ;(ctx.bash as LocalBashExecutor).internals = { spillDir } await ctx.plugin(ToolBash) const result = await call(ctx, 'bash', { command: 'for i in $(seq 1 100); do printf "line-%04d\\n" $i; done', description: 'test command' }) expect(text(result)).toContain('[output truncated; full output: ') @@ -316,8 +316,8 @@ describe('background tools', () => { const ctx = new Context() await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry) - await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 }) - ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } + await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100, graceMs: 200 }) + ;(ctx.bash as LocalBashExecutor).internals = { spillDir } await ctx.plugin(ToolBash) const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true }) @@ -568,8 +568,8 @@ describe('background task ownership (cross-session isolation)', () => { const ctx = new Context() await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry) - await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) - ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } + await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000, graceMs: 200 }) + ;(ctx.bash as LocalBashExecutor).internals = { spillDir } const fiber = await ctx.plugin(ToolBash) const a = fakeAgent('sess-a') diff --git a/packages/compact/compact-basic/README.md b/packages/compact/compact-basic/README.md index c195ae42fb..cbd1146634 100644 --- a/packages/compact/compact-basic/README.md +++ b/packages/compact/compact-basic/README.md @@ -32,6 +32,7 @@ Every knob is **required** except `auto` — there is no concrete data yet to ju | `maxTokens` | yes | Provider generation cap for the summarization call; may include reasoning tokens. | | `compactionRetries` | yes | Extra compaction attempts after the first if the compacted surface remains over threshold. | | `auto` | no (default `true`) | Register the `agent/pre-step` auto-compaction listener. Set `false` for manual-only. | +| `charsPerToken` | no (default `4`) | Token-estimator text density (estimated tokens = chars / `charsPerToken`; may be fractional). The default suits English text; CJK-heavy deployments should set ~1-2 or the estimate undershoots several-fold and compaction fires too late. | ## Usage diff --git a/packages/compact/compact-basic/src/index.ts b/packages/compact/compact-basic/src/index.ts index f53dace461..26c593a47f 100644 --- a/packages/compact/compact-basic/src/index.ts +++ b/packages/compact/compact-basic/src/index.ts @@ -148,9 +148,11 @@ function finishError(finish: FinishReason): Error | undefined { } /** - * Basic, dependency-light compaction backend. Defaults target a 128K context - * window, compacting at 80% utilization and retaining ~20K tokens of recent - * context. + * Basic, dependency-light compaction backend: estimates the surface's token + * footprint, summarizes the stale prefix through the model, and shadows it + * behind a durable checkpoint. Every threshold/budget knob is required config + * ({@link BasicCompactConfig}); the estimator's text density is the + * `charsPerToken` knob. */ export class BasicCompactService extends CompactService { static inject = ['llm'] @@ -207,24 +209,27 @@ export class BasicCompactService extends CompactService { // ---- Token estimation (overridable hooks) ---- - // TODO: char/4 is a coarse heuristic. Replace with an exact count — a real - // tokenizer, or the provider's post-response `usage` (input tokens) fed back - // as a correction — so threshold decisions match the model's actual budget. + // TODO: chars/charsPerToken is a coarse heuristic. Replace with an exact + // count — a real tokenizer, or the provider's post-response `usage` (input + // tokens) fed back as a correction — so threshold decisions match the + // model's actual budget. /** - * Estimate the token count of content blocks — char/4 with per-block - * overhead. Override in a subclass to plug in a real tokenizer. + * Estimate the token count of content blocks — chars divided by the + * `charsPerToken` config, with per-block overhead. Override in a subclass to + * plug in a real tokenizer. */ estimateContentTokens(blocks: readonly ContentBlock[]): number { + const { charsPerToken } = this.config let tokens = 0 for (const block of blocks) { switch (block.type) { case 'text': case 'reasoning': - tokens += Math.ceil(block.text.length / 4) + BLOCK_OVERHEAD + tokens += Math.ceil(block.text.length / charsPerToken) + BLOCK_OVERHEAD break case 'tool-call': - tokens += Math.ceil(block.name.length / 4) - + Math.ceil(block.arguments.length / 4) + tokens += Math.ceil(block.name.length / charsPerToken) + + Math.ceil(block.arguments.length / charsPerToken) + BLOCK_OVERHEAD break case 'tool-result': @@ -236,7 +241,7 @@ export class BasicCompactService extends CompactService { default: // Unknown block types (merge-extensible ContentBlockMap): // estimate conservatively via JSON stringify. - tokens += BLOCK_OVERHEAD + Math.ceil(JSON.stringify(block).length / 4) + tokens += BLOCK_OVERHEAD + Math.ceil(JSON.stringify(block).length / charsPerToken) } } return tokens @@ -266,7 +271,7 @@ export class BasicCompactService extends CompactService { total += this.estimateContentTokens(msg.content) total += ROLE_OVERHEAD } - if (systemPrompt) total += Math.ceil(systemPrompt.length / 4) + if (systemPrompt) total += Math.ceil(systemPrompt.length / this.config.charsPerToken) return total } diff --git a/packages/compact/compact-basic/src/types.ts b/packages/compact/compact-basic/src/types.ts index 98195d8883..2f10084ac3 100644 --- a/packages/compact/compact-basic/src/types.ts +++ b/packages/compact/compact-basic/src/types.ts @@ -10,10 +10,12 @@ */ /** - * Backend configuration. Every knob is REQUIRED except `auto`: there is no - * concrete data yet to justify default thresholds/budgets, so a consumer must - * state each value explicitly rather than inherit a guessed default. `auto` - * alone defaults to `true` (auto-compaction is the intended posture). + * Backend configuration. Every knob is REQUIRED except `auto` and + * `charsPerToken`: there is no concrete data yet to justify default + * thresholds/budgets, so a consumer must state each value explicitly rather + * than inherit a guessed default. `auto` alone defaults to `true` + * (auto-compaction is the intended posture), and `charsPerToken` defaults to + * the English-text heuristic its estimator was calibrated on. */ export interface BasicCompactConfig { /** Context window size in tokens. */ @@ -30,13 +32,21 @@ export interface BasicCompactConfig { compactionRetries: number /** Enable automatic compaction on the `agent/pre-step` seam (default true). */ auto?: boolean + /** + * Text density for the token estimator: estimated tokens = chars / + * `charsPerToken`. Defaults to 4 (typical English text). A CJK-heavy + * deployment should set ~1-2 — CJK runs at roughly 1-2 chars per token, so + * the default UNDERestimates several-fold and compaction fires far too late. + * May be fractional. + */ + charsPerToken?: number } -/** Resolved config with `auto` defaulted. */ +/** Resolved config with `auto` and `charsPerToken` defaulted. */ export type ResolvedConfig = Required /** - * Default `auto` when unset and reject nonsensical numeric knobs. + * Default `auto`/`charsPerToken` when unset and reject nonsensical numeric knobs. * * Convergence is not a static config invariant: provider generation caps can be * spent on hidden or surfaced reasoning tokens, and the model may emit a summary @@ -46,13 +56,14 @@ export type ResolvedConfig = Required * throwing if the surface still exceeds the threshold. */ export function resolveConfig(config: BasicCompactConfig): ResolvedConfig { - const resolved: ResolvedConfig = { auto: true, ...config } + const resolved: ResolvedConfig = { auto: true, charsPerToken: 4, ...config } assertPositiveInteger('contextWindow', resolved.contextWindow) assertRatio('thresholdRatio', resolved.thresholdRatio) assertNonNegativeInteger('retainTokens', resolved.retainTokens) assertPositiveInteger('maxTokens', resolved.maxTokens) assertNonNegativeInteger('compactionRetries', resolved.compactionRetries) + assertPositiveFinite('charsPerToken', resolved.charsPerToken) if (typeof resolved.summarizationModel !== 'string') { throw new Error('BasicCompactConfig: summarizationModel must be a string.') } @@ -74,6 +85,12 @@ function assertNonNegativeInteger(name: string, value: number): void { } } +function assertPositiveFinite(name: string, value: number): void { + if (typeof value !== 'number' || !Number.isFinite(value) || value <= 0) { + throw new Error(`BasicCompactConfig: ${name} (${value}) must be a positive finite number.`) + } +} + function assertRatio(name: string, value: number): void { if (typeof value !== 'number' || !Number.isFinite(value) || value <= 0 || value > 1) { throw new Error(`BasicCompactConfig: ${name} (${value}) must be a number in (0, 1].`) diff --git a/packages/compact/compact-basic/tests/compact-basic.spec.ts b/packages/compact/compact-basic/tests/compact-basic.spec.ts index 1929e656be..7d5d67b6db 100644 --- a/packages/compact/compact-basic/tests/compact-basic.spec.ts +++ b/packages/compact/compact-basic/tests/compact-basic.spec.ts @@ -820,6 +820,19 @@ describe('BasicCompactService token estimation (char/4 heuristic)', () => { const svc = new BasicCompactService(new Context(), cfg({ auto: false })) expect(svc.estimateContentTokens([])).toBe(0) }) + + it('honors a configured charsPerToken (fractional densities included)', () => { + // 'this is a somewhat longer text block' = 36 chars. + const blocks: ContentBlock[] = [{ type: 'text', text: 'this is a somewhat longer text block' }] + // charsPerToken 2: ceil(36/2)+4 = 22 — a CJK-density config doubles the estimate. + const dense = new BasicCompactService(new Context(), cfg({ auto: false, charsPerToken: 2 })) + expect(dense.estimateContentTokens(blocks)).toBe(22) + // Fractional density is legal: ceil(36/1.5)+4 = 28. + const fractional = new BasicCompactService(new Context(), cfg({ auto: false, charsPerToken: 1.5 })) + expect(fractional.estimateContentTokens(blocks)).toBe(28) + // The system-prompt term scales with the same knob: 36-char prompt at density 2 → ceil(36/2) = 18. + expect(dense.estimateTokens([], 'this is a somewhat longer text block')).toBe(18) + }) }) describe('BasicCompactService HMR safety', () => { @@ -862,6 +875,10 @@ describe('BasicCompactService config validation', () => { )).toThrow(/summarizationModel must be a string/) expect(() => new BasicCompactService(new Context(), cfg({ auto: 'no' } as unknown as Partial))) .toThrow(/auto must be a boolean/) + expect(() => new BasicCompactService(new Context(), cfg({ auto: false, charsPerToken: 0 }))) + .toThrow(/charsPerToken .* positive finite number/) + expect(() => new BasicCompactService(new Context(), cfg({ auto: false, charsPerToken: Number.NaN }))) + .toThrow(/charsPerToken .* positive finite number/) }) it('accepts a large retain budget because convergence is enforced dynamically', () => { diff --git a/packages/fs/fs-local/src/fsio.ts b/packages/fs/fs-local/src/fsio.ts index 980cb4764d..64754b2dd4 100644 --- a/packages/fs/fs-local/src/fsio.ts +++ b/packages/fs/fs-local/src/fsio.ts @@ -26,9 +26,6 @@ import { basename, dirname, join, resolve } from 'node:path' import { TextDecoder } from 'node:util' import { FsError, FsTargetKey, FsVersion } from '@deepseek-ai/dsh-fs' -/** Files at or above this size stream their text; smaller files read whole. */ -export const STREAM_MIN_SIZE = 10 * 1024 * 1024 - const BINARY_SAMPLE_BYTES = 8192 function isENOENT(error: unknown): boolean { @@ -85,13 +82,10 @@ function versionOf(info: Stats): FsVersion { } /** - * Test seam: lets specs force the streaming read path (via a small - * `streamMinSize`) and pin the temp-file name (to prove exclusive-open - * behavior) without a 10 MB fixture or a name race. + * Test seam: lets specs pin the temp-file name (to prove exclusive-open + * behavior) without a name race. */ export interface FsIoInternals { - /** Override {@link STREAM_MIN_SIZE} for read routing. */ - streamMinSize?: number /** Override the generated private staging-dir name (relative to the target dir). */ tempDirName?: (writePath: string) => string /** Override the generated temp-file name (relative to the private staging dir). */ diff --git a/packages/fs/fs-local/src/index.ts b/packages/fs/fs-local/src/index.ts index af74847ed1..abd8d047e6 100644 --- a/packages/fs/fs-local/src/index.ts +++ b/packages/fs/fs-local/src/index.ts @@ -41,7 +41,6 @@ import { import type { FsIoInternals } from './fsio.ts' export { - STREAM_MIN_SIZE, applyLiteralEdit, listDirectory, probe, diff --git a/packages/fs/tool-fs/README.md b/packages/fs/tool-fs/README.md index dacef45590..fedd1ff7a2 100644 --- a/packages/fs/tool-fs/README.md +++ b/packages/fs/tool-fs/README.md @@ -11,11 +11,22 @@ await ctx.plugin(ToolFs) // this package — re `@deepseek-ai/dsh-fs-policy` is **optional**: omit it and the tools run against the bare provider (unconditional write/overwrite/edit, no observed-state). A deployment that loads these tools is expected to also load it, so the behavior is read-before-write/edit. +## Config + +All keys are optional; the defaults are the shipped read caps. + +| Key | Default | Meaning | +|---|---|---| +| `readLimit` | `2000` | Default and maximum lines returned by one `read` call (the tool schema advertises it as the `limit` default). | +| `readMaxLineLength` | `2000` | Characters kept per line before truncation (the suffix names the cap). | +| `readMaxBytes` | `51200` | Byte cap on one `read` call's selected lines; overflow ends the window with a "capped" footer. | +| `readStreamMinSize` | `10485760` | Files at or above this size (or with unknown size) stream instead of loading whole into memory. | + ## Tools (schemas per [the filesystem tool schemas RFC](../../../docs/rfc/implemented/feature/2026-06-17-filesystem-tool-schemas.md)) | Tool | Arguments | Behavior | |---|---|---| -| `read` | `file_path`, `offset?`, `limit?` | Line-numbered UTF-8 content with a pagination footer. `offset` is 1-based; `limit` defaults to and caps at 2000 lines. | +| `read` | `file_path`, `offset?`, `limit?` | Line-numbered UTF-8 content with a pagination footer. `offset` is 1-based; `limit` defaults to and caps at the configured `readLimit` (2000). | | `write` | `file_path`, `content` | Create or fully replace a file. With the policy plugin: overwriting an existing file requires a prior `read` at the unchanged version; creating a new file does not. Without it: unconditional. | | `edit` | `file_path`, non-empty `old_string`, `new_string`, `replace_all?` | Literal replacement; unique match required unless `replace_all` is true. With the policy plugin: requires a prior `read` (any window) and the file unchanged since. Without it: unconditional. | diff --git a/packages/fs/tool-fs/package.json b/packages/fs/tool-fs/package.json index a7f71ce6fa..7e7b78aa38 100644 --- a/packages/fs/tool-fs/package.json +++ b/packages/fs/tool-fs/package.json @@ -22,7 +22,8 @@ ], "license": "BSD-3-Clause", "dependencies": { - "diff": "^9.0.0" + "diff": "^9.0.0", + "schemastery": "^3.18.0" }, "peerDependencies": { "@deepseek-ai/dsh-fs": "^0.0.1", diff --git a/packages/fs/tool-fs/src/index.ts b/packages/fs/tool-fs/src/index.ts index 0aaa0c1a7a..5cc87ba597 100644 --- a/packages/fs/tool-fs/src/index.ts +++ b/packages/fs/tool-fs/src/index.ts @@ -23,11 +23,14 @@ */ import type { Context } from 'cordis' -import { applyReadTool } from './read.ts' +import z from 'schemastery' +import { applyReadTool, READ_LIMIT, STREAM_MIN_SIZE } from './read.ts' import { applyWriteTool } from './write.ts' import { applyEditTool } from './edit.ts' +import { READ_MAX_BYTES, READ_MAX_LINE_LENGTH } from './read-render.ts' export { READ_LIMIT, STREAM_MIN_SIZE, applyReadTool, parseReadArgs } from './read.ts' +export type { ReadToolCaps } from './read.ts' export { applyWriteTool, formatWriteOutput, parseWriteArgs } from './write.ts' export { applyEditTool, formatEditOutput, parseEditArgs } from './edit.ts' export { READ_MAX_BYTES, READ_MAX_LINE_LENGTH, buildWindow, formatReadOutput } from './read-render.ts' @@ -41,9 +44,49 @@ export const name = 'tool-fs' /** Services required by the filesystem tool suite. */ export const inject = ['tools', 'fs', 'systemPrompt'] +/** Plugin config (all optional — `Config` supplies the defaults). */ +export interface Config { + /** Default and maximum number of lines returned by one `read` call. */ + readLimit?: number + /** Maximum characters returned for a single line before truncation. */ + readMaxLineLength?: number + /** Maximum bytes returned for the selected lines of one `read` call. */ + readMaxBytes?: number + /** Files at or above this size stream instead of loading whole into memory. */ + readStreamMinSize?: number +} + +export const Config: z = z.object({ + readLimit: z.number().default(READ_LIMIT), + readMaxLineLength: z.number().default(READ_MAX_LINE_LENGTH), + readMaxBytes: z.number().default(READ_MAX_BYTES), + readStreamMinSize: z.number().default(STREAM_MIN_SIZE), +}) + +/** The shape after schemastery applied the defaults. */ +type ResolvedConfig = Required + +/** A read cap must be a positive finite number to bound output and memory. */ +function assertPositiveFinite(name: string, value: number): void { + if (!Number.isFinite(value) || value <= 0) { + throw new Error(`tool-fs: ${name} must be a positive finite number`) + } +} + /** Register the full `read`/`write`/`edit` filesystem tool suite. */ -export function apply(ctx: Context): void { - applyReadTool(ctx) +export function apply(ctx: Context, config: Config): void { + // schemastery (Config) has already filled every defaulted field. + const resolved = config as ResolvedConfig + assertPositiveFinite('readLimit', resolved.readLimit) + assertPositiveFinite('readMaxLineLength', resolved.readMaxLineLength) + assertPositiveFinite('readMaxBytes', resolved.readMaxBytes) + assertPositiveFinite('readStreamMinSize', resolved.readStreamMinSize) + applyReadTool(ctx, { + limit: resolved.readLimit, + maxLineLength: resolved.readMaxLineLength, + maxBytes: resolved.readMaxBytes, + streamMinSize: resolved.readStreamMinSize, + }) applyWriteTool(ctx) applyEditTool(ctx) } diff --git a/packages/fs/tool-fs/src/read-render.ts b/packages/fs/tool-fs/src/read-render.ts index 97a1384792..ba0f01f214 100644 --- a/packages/fs/tool-fs/src/read-render.ts +++ b/packages/fs/tool-fs/src/read-render.ts @@ -19,21 +19,22 @@ import { FsError } from '@deepseek-ai/dsh-fs' import type { FsVersion } from '@deepseek-ai/dsh-fs' -/** Maximum characters returned for a single line. */ +/** Default maximum characters returned for a single line (the `readMaxLineLength` config). */ export const READ_MAX_LINE_LENGTH = 2000 -/** Maximum bytes returned for selected file lines. */ +/** Default maximum bytes returned for selected file lines (the `readMaxBytes` config). */ export const READ_MAX_BYTES = 50 * 1024 -const READ_MAX_LINE_SUFFIX = `... (line truncated to ${READ_MAX_LINE_LENGTH} chars)` -const LINE_BUFFER_CAP = READ_MAX_LINE_LENGTH + 1 - /** Resolved read window. The consumer applies its defaults/caps before calling. */ export interface ReadWindow { /** 1-based first line to return. */ offset: number /** Maximum number of lines to return. */ limit: number + /** Maximum characters returned for a single line; overflow is truncated with a suffix. */ + maxLineLength: number + /** Maximum bytes of selected output; overflow stops the scan and marks `truncatedByBytes`. */ + maxBytes: number } /** One line returned from a text file. */ @@ -82,8 +83,8 @@ function newAccumulator(): WindowAccumulator { return { lines: [], totalLines: 0, outputBytes: 0, truncatedByBytes: false, done: false } } -function truncateLine(line: string): string { - return line.length > READ_MAX_LINE_LENGTH ? `${line.substring(0, READ_MAX_LINE_LENGTH)}${READ_MAX_LINE_SUFFIX}` : line +function truncateLine(line: string, maxLineLength: number): string { + return line.length > maxLineLength ? `${line.substring(0, maxLineLength)}... (line truncated to ${maxLineLength} chars)` : line } function lineByteSize(line: string, currentLineCount: number): number { @@ -94,9 +95,9 @@ function consumeLine(acc: WindowAccumulator, rawLine: string, request: ReadWindo acc.totalLines += 1 if (acc.totalLines < request.offset || acc.lines.length >= request.limit) return - const text = truncateLine(rawLine) + const text = truncateLine(rawLine, request.maxLineLength) const bytes = lineByteSize(text, acc.lines.length) - if (acc.outputBytes + bytes > READ_MAX_BYTES) { + if (acc.outputBytes + bytes > request.maxBytes) { acc.truncatedByBytes = true acc.done = true return @@ -121,7 +122,7 @@ function finish(acc: WindowAccumulator, request: ReadWindow, displayPath: string * Accepts an `AsyncIterable` (a chunked `streamText`) or an * `Iterable` (a whole-file `readText` wrapped as `[text]`), so one code * path serves both. Scans for newlines with a capped line buffer (a newline-free - * giant line is truncated, never buffered past {@link READ_MAX_LINE_LENGTH}), + * giant line is truncated, never buffered past `request.maxLineLength`), * enforces the byte cap, and throws `FS_NOT_FOUND` for an offset past EOF. */ export async function buildWindow( @@ -130,12 +131,14 @@ export async function buildWindow( displayPath: string, ): Promise { const acc = newAccumulator() + // One char past the truncation point is enough to prove a line overflows. + const lineBufferCap = request.maxLineLength + 1 let lineBuffer = '' function appendToLineBuffer(segment: string): void { - if (lineBuffer.length >= LINE_BUFFER_CAP) return + if (lineBuffer.length >= lineBufferCap) return lineBuffer += segment - if (lineBuffer.length > LINE_BUFFER_CAP) lineBuffer = lineBuffer.slice(0, LINE_BUFFER_CAP) + if (lineBuffer.length > lineBufferCap) lineBuffer = lineBuffer.slice(0, lineBufferCap) } function flushLine(): void { diff --git a/packages/fs/tool-fs/src/read.ts b/packages/fs/tool-fs/src/read.ts index c984b53c9f..68f21231b2 100644 --- a/packages/fs/tool-fs/src/read.ts +++ b/packages/fs/tool-fs/src/read.ts @@ -23,12 +23,27 @@ import { buildWindow, formatReadOutput } from './read-render.ts' import type { FileReadOutcome } from './read-render.ts' import { sessionCwd } from './session-cwd.ts' -/** Default and maximum number of lines returned by one `read` call. */ +/** Default and maximum number of lines returned by one `read` call (the `readLimit` config). */ export const READ_LIMIT = 2000 -/** Files at or above this size stream; smaller files read whole into memory. */ +/** + * Default streaming threshold (the `readStreamMinSize` config): files at or + * above this size stream; smaller files read whole into memory. + */ export const STREAM_MIN_SIZE = 10 * 1024 * 1024 +/** Resolved read-tool caps — plugin config after defaulting (see `Config` in index.ts). */ +export interface ReadToolCaps { + /** Default and maximum number of lines returned by one call. */ + limit: number + /** Maximum characters returned for a single line. */ + maxLineLength: number + /** Maximum bytes returned for selected file lines. */ + maxBytes: number + /** Files at or above this size stream; smaller files read whole into memory. */ + streamMinSize: number +} + /** Validated `read` arguments after defaulting. */ interface ReadInput { filePath: string @@ -43,17 +58,17 @@ function parsePositiveInteger(value: number, name: string): number { return value } -/** Validate value constraints the schema DSL can't express. */ -export function parseReadArgs(args: { file_path: string; offset?: number; limit?: number }): ReadInput { +/** Validate value constraints the schema DSL can't express. `maxLimit` is the deployment's line cap. */ +export function parseReadArgs(args: { file_path: string; offset?: number; limit?: number }, maxLimit: number): ReadInput { if (args.file_path.trim().length === 0) throw new Error('file_path must be a non-empty string') const offset = args.offset === undefined ? 1 : parsePositiveInteger(args.offset, 'offset') - const limit = args.limit === undefined ? READ_LIMIT : parsePositiveInteger(args.limit, 'limit') - if (limit > READ_LIMIT) throw new Error(`limit must be less than or equal to ${READ_LIMIT}`) + const limit = args.limit === undefined ? maxLimit : parsePositiveInteger(args.limit, 'limit') + if (limit > maxLimit) throw new Error(`limit must be less than or equal to ${maxLimit}`) return { filePath: args.file_path, offset, limit } } /** Register the `read` tool and its system-prompt guidance. */ -export function applyReadTool(ctx: Context): void { +export function applyReadTool(ctx: Context, caps: ReadToolCaps): void { ctx.systemPrompt.section({ name: 'tool:read', order: 100, @@ -66,10 +81,10 @@ export function applyReadTool(ctx: Context): void { parameters: { file_path: { type: 'string', required: true, description: 'Path to read, resolved by the filesystem backend.' }, offset: { type: 'number', description: '1-based first line to return. Defaults to 1.' }, - limit: { type: 'number', description: `Maximum number of lines to return. Defaults to ${READ_LIMIT}.` }, + limit: { type: 'number', description: `Maximum number of lines to return. Defaults to ${caps.limit}.` }, }, async execute(args, exec): Promise { - const input = parseReadArgs(args) + const input = parseReadArgs(args, caps.limit) const cwd = sessionCwd(exec) const target = await ctx.fs.resolve(input.filePath, cwd !== undefined ? { cwd } : undefined) @@ -83,10 +98,14 @@ export function applyReadTool(ctx: Context): void { // Stream when the file is large OR size is unknown, so a size-less backend // never buffers an arbitrarily large file. - const chunks = info.size === undefined || info.size >= STREAM_MIN_SIZE + const chunks = info.size === undefined || info.size >= caps.streamMinSize ? await ctx.fs.streamText(target, exec.signal) : [await ctx.fs.readText(target, exec.signal)] - const window = await buildWindow(chunks, { offset: input.offset, limit: input.limit }, target.displayPath) + const window = await buildWindow( + chunks, + { offset: input.offset, limit: input.limit, maxLineLength: caps.maxLineLength, maxBytes: caps.maxBytes }, + target.displayPath, + ) const outcome: FileReadOutcome = { offset: input.offset, @@ -106,7 +125,8 @@ export function applyReadTool(ctx: Context): void { // appended (`Read foo.txt (5 - 8)`), `read` kind (icon), and a follow-along // location whose line is the read's offset (defaulting to 1). The window is // derived from the RAW args (offset/limit as the model passed them), NOT the - // tool's defaulted 1/READ_LIMIT, so an unbounded read shows a bare title. + // tool's defaulted 1/configured limit, so an unbounded read shows a bare + // title (and the presenter stays a pure function of args, config-free). presentCall(args): GenericCallView { const { offset, limit } = args const window = limit !== undefined && limit > 0 diff --git a/packages/fs/tool-fs/tests/read-render.spec.ts b/packages/fs/tool-fs/tests/read-render.spec.ts index b596a47465..c23ad79170 100644 --- a/packages/fs/tool-fs/tests/read-render.spec.ts +++ b/packages/fs/tool-fs/tests/read-render.spec.ts @@ -6,10 +6,11 @@ */ import { describe, expect, it } from 'vitest' -import { buildWindow, READ_MAX_LINE_LENGTH } from '@deepseek-ai/dsh-tool-fs' +import { buildWindow, READ_MAX_BYTES, READ_MAX_LINE_LENGTH } from '@deepseek-ai/dsh-tool-fs' import type { ReadWindow } from '@deepseek-ai/dsh-tool-fs' -const READ_ALL: ReadWindow = { offset: 1, limit: 2000 } +const DEFAULT_CAPS = { maxLineLength: READ_MAX_LINE_LENGTH, maxBytes: READ_MAX_BYTES } +const READ_ALL: ReadWindow = { offset: 1, limit: 2000, ...DEFAULT_CAPS } /** Yield `text` as one chunk (whole-file read shape). */ async function* whole(text: string): AsyncIterable { @@ -34,7 +35,7 @@ describe('buildWindow', () => { }) it('applies offset/limit', async () => { - const result = await buildWindow(whole('one\ntwo\nthree\nfour'), { offset: 2, limit: 2 }, 'f') + const result = await buildWindow(whole('one\ntwo\nthree\nfour'), { offset: 2, limit: 2, ...DEFAULT_CAPS }, 'f') expect(result.lines.map(l => l.number)).toEqual([2, 3]) expect(result.totalLines).toBe(4) }) @@ -62,7 +63,7 @@ describe('buildWindow', () => { }) it('rejects an offset past EOF', async () => { - await expect(buildWindow(whole('one\ntwo'), { offset: 9, limit: 1 }, 'f')).rejects.toMatchObject({ code: 'FS_NOT_FOUND' }) + await expect(buildWindow(whole('one\ntwo'), { offset: 9, limit: 1, ...DEFAULT_CAPS }, 'f')).rejects.toMatchObject({ code: 'FS_NOT_FOUND' }) }) it('flushes a final line with no trailing newline', async () => { @@ -76,9 +77,22 @@ describe('buildWindow', () => { expect(result.totalLines).toBe(2) }) + describe('caps are per-request (the plugin config reaches the window)', () => { + it('truncates lines at a custom maxLineLength and names it in the suffix', async () => { + const result = await buildWindow(whole('abcdefghij'), { offset: 1, limit: 10, maxLineLength: 5, maxBytes: READ_MAX_BYTES }, 'f') + expect(result.lines[0]?.text).toBe('abcde... (line truncated to 5 chars)') + }) + + it('caps output at a custom maxBytes', async () => { + const result = await buildWindow(whole('aaaa\nbbbb\ncccc'), { offset: 1, limit: 10, maxLineLength: 2000, maxBytes: 9 }, 'f') + expect(result.lines.map(l => l.text)).toEqual(['aaaa', 'bbbb']) + expect(result.truncatedByBytes).toBe(true) + }) + }) + describe('chunked input (streamed read shape)', () => { it('windows identically when text arrives in small chunks', async () => { - const result = await buildWindow(chunked('one\ntwo\nthree', 2), { offset: 2, limit: 1 }, 'f') + const result = await buildWindow(chunked('one\ntwo\nthree', 2), { offset: 2, limit: 1, ...DEFAULT_CAPS }, 'f') expect(result.lines).toEqual([{ number: 2, text: 'two' }]) expect(result.totalLines).toBe(3) }) diff --git a/packages/fs/tool-fs/tests/tools.spec.ts b/packages/fs/tool-fs/tests/tools.spec.ts index 6272ac5c9d..638ce2112b 100644 --- a/packages/fs/tool-fs/tests/tools.spec.ts +++ b/packages/fs/tool-fs/tests/tools.spec.ts @@ -495,3 +495,70 @@ describe('result-time contextual diff (meta + presentResult)', () => { expect(view).toEqual({ card: 'diff', title: 'Write a.txt', diffs: [{ path: 'a.txt', oldText: null, newText: 'y' }] }) }) }) + +describe('read caps are plugin config', () => { + async function setupWith(config: ToolFs.Config) { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(FakeFs) + await ctx.plugin(FsPolicy) + await ctx.plugin(ToolFs, config) + return { ctx, fs: ctx.fs as FakeFs } + } + + it('a configured readLimit is both the default and the cap, and the schema names it', async () => { + const { ctx, fs } = await setupWith({ readLimit: 2 }) + fs.files.set('key:a.txt', 'one\ntwo\nthree\nfour') + const result = await call(ctx, 'read', { file_path: 'a.txt' }) + expect(text(result)).toContain('(Showing lines 1-2 of 4. Use offset=3 to continue.)') + const overCap = await call(ctx, 'read', { file_path: 'a.txt', limit: 3 }) + expect(overCap.isError).toBe(true) + expect(text(overCap)).toContain('less than or equal to 2') + const readSchema = ctx.tools.schemas().find(s => s.name === 'read') + expect(JSON.stringify(readSchema)).toContain('Defaults to 2.') + }) + + it('a configured readMaxLineLength truncates lines at the configured length', async () => { + const { ctx, fs } = await setupWith({ readMaxLineLength: 4 }) + fs.files.set('key:a.txt', 'abcdefgh') + const result = await call(ctx, 'read', { file_path: 'a.txt' }) + expect(text(result)).toContain('1: abcd... (line truncated to 4 chars)') + }) + + it('a configured readMaxBytes caps the window at the configured bytes', async () => { + const { ctx, fs } = await setupWith({ readMaxBytes: 9 }) + fs.files.set('key:a.txt', 'aaaa\nbbbb\ncccc') + const result = await call(ctx, 'read', { file_path: 'a.txt' }) + expect(text(result)).toContain('Output capped.') + expect(text(result)).not.toContain('cccc') + }) + + it('a configured readStreamMinSize routes smaller files to the streaming path', async () => { + const { ctx, fs } = await setupWith({ readStreamMinSize: 5 }) + fs.files.set('key:a.txt', 'alpha\nbeta') + const readSpy = vi.spyOn(fs, 'readText') + const streamSpy = vi.spyOn(fs, 'streamText') + const result = await call(ctx, 'read', { file_path: 'a.txt' }) + expect(result.isError).toBe(false) + expect(streamSpy).toHaveBeenCalled() + expect(readSpy).not.toHaveBeenCalled() + }) + + it.each([ + ['readLimit', { readLimit: 0 }], + ['readMaxLineLength', { readMaxLineLength: -1 }], + ['readMaxBytes', { readMaxBytes: Number.NaN }], + ['readStreamMinSize', { readStreamMinSize: 0 }], + ] as const)('rejects a non-positive %s at load', async (name, config) => { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(FakeFs) + await expect(ctx.plugin(ToolFs, config)).rejects.toThrow(new RegExp(`tool-fs: ${name} must be a positive finite number`)) + }) + + it('has no default export (namespace plugin export shape)', () => { + expect('default' in ToolFs).toBe(false) + }) +}) diff --git a/packages/fs/tool-fs/tsconfig.json b/packages/fs/tool-fs/tsconfig.json index 6af16400c0..f0133b1d2b 100644 --- a/packages/fs/tool-fs/tsconfig.json +++ b/packages/fs/tool-fs/tsconfig.json @@ -8,6 +8,7 @@ "references": [ { "path": "../../../vendor/cosmokit" }, { "path": "../../../vendor/cordis" }, + { "path": "../../../vendor/schemastery" }, { "path": "../../llm/llm" }, { "path": "../../core/tools" }, { "path": "../../core/system-prompt" }, diff --git a/packages/hooks/hook-protocol/src/events.ts b/packages/hooks/hook-protocol/src/events.ts index 0db75995c9..d7529b0f45 100644 --- a/packages/hooks/hook-protocol/src/events.ts +++ b/packages/hooks/hook-protocol/src/events.ts @@ -58,6 +58,18 @@ export function appendHookInvoked(session: Session, invocation: HookInvocation): }) } +/** + * Truncate a hook's stderr for {@link HookResultRecord.stderrSummary}: trimmed, + * `undefined` when empty, cut at `maxChars` with an ellipsis when over. The + * bound is a parameter — like `runHook`'s `defaultTimeoutMs`, each bridge owns + * the config default and passes it in. + */ +export function summarizeStderr(stderr: string, maxChars: number): string | undefined { + const t = stderr.trim() + if (t.length === 0) return undefined + return t.length > maxChars ? t.slice(0, maxChars) + '…' : t +} + /** Append a `hook/result` outcome event to `session` (pairs with a prior `hook/invoked`). */ export function appendHookResult(session: Session, record: HookResultRecord): void { session.append('hook/result', { diff --git a/packages/hooks/hook-protocol/src/index.ts b/packages/hooks/hook-protocol/src/index.ts index 686a1480ac..a99fd11b6f 100644 --- a/packages/hooks/hook-protocol/src/index.ts +++ b/packages/hooks/hook-protocol/src/index.ts @@ -34,5 +34,5 @@ export { runHook } from './runner.ts' export type { RunHookOptions, RunHookResult } from './runner.ts' export { mergeHookOutputs } from './merge.ts' export type { MergedDecision, MergedHookOutcome } from './merge.ts' -export { appendHookInvoked, appendHookResult } from './events.ts' +export { appendHookInvoked, appendHookResult, summarizeStderr } from './events.ts' export type { HookInvocation, HookResultRecord } from './events.ts' diff --git a/packages/hooks/hook-protocol/tests/events.spec.ts b/packages/hooks/hook-protocol/tests/events.spec.ts index f63ae2a9cb..9f705916b9 100644 --- a/packages/hooks/hook-protocol/tests/events.spec.ts +++ b/packages/hooks/hook-protocol/tests/events.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import { Session, SessionId } from '@deepseek-ai/dsh-session' -import { appendHookInvoked, appendHookResult } from '@deepseek-ai/dsh-hook-protocol' +import { appendHookInvoked, appendHookResult, summarizeStderr } from '@deepseek-ai/dsh-hook-protocol' describe('hook/* session events', () => { it('appendHookInvoked records a log-only hook/invoked (with matcher when present)', () => { @@ -59,3 +59,20 @@ describe('hook/* session events', () => { expect(result?.type === 'hook/result' && result.data.handlerId).toBe('pair-1') }) }) + +describe('summarizeStderr', () => { + it('returns undefined for empty/whitespace stderr', () => { + expect(summarizeStderr('', 500)).toBeUndefined() + expect(summarizeStderr(' \n\t ', 500)).toBeUndefined() + }) + + it('passes through a summary at or under the cap, trimmed', () => { + expect(summarizeStderr(' blocked: bad tool ', 500)).toBe('blocked: bad tool') + expect(summarizeStderr('abc', 3)).toBe('abc') + }) + + it('truncates past the cap with an ellipsis', () => { + expect(summarizeStderr('abcdef', 4)).toBe('abcd…') + expect(summarizeStderr('x'.repeat(600), 500)).toBe('x'.repeat(500) + '…') + }) +}) diff --git a/packages/hooks/hooks-claude/README.md b/packages/hooks/hooks-claude/README.md index ce1c6b090a..984a86ba18 100644 --- a/packages/hooks/hooks-claude/README.md +++ b/packages/hooks/hooks-claude/README.md @@ -13,6 +13,7 @@ const config: Config = { pluginRoot: '/path/to/plugin', // optional: replaces ${CLAUDE_PLUGIN_ROOT} in command strings projectDir: '/path/to/project', // optional: replaces ${CLAUDE_PROJECT_DIR} AND sets the hook env var; defaults to the session cwd when omitted defaultTimeoutMs: 600_000, // optional: per-hook timeout when a hook sets none (CC default) + stderrSummaryMaxChars: 500, // optional: char cap on the hook/result event's persisted stderr summary } ``` diff --git a/packages/hooks/hooks-claude/src/index.ts b/packages/hooks/hooks-claude/src/index.ts index 6151a11c44..263b201791 100644 --- a/packages/hooks/hooks-claude/src/index.ts +++ b/packages/hooks/hooks-claude/src/index.ts @@ -34,6 +34,7 @@ import { matchesMatcher, mergeHookOutputs, runHook, + summarizeStderr, type HookOutput, type MatcherGroup, type MergedHookOutcome, @@ -73,6 +74,8 @@ export interface Config { projectDir?: string /** Default per-hook timeout in ms when a hook sets none (CC default: 600000). */ defaultTimeoutMs?: number + /** Character cap for the `hook/result` event's persisted stderr summary. */ + stderrSummaryMaxChars?: number } export const Config: z = z.object({ @@ -80,6 +83,7 @@ export const Config: z = z.object({ pluginRoot: z.string(), projectDir: z.string(), defaultTimeoutMs: z.number().default(600_000), + stderrSummaryMaxChars: z.number().default(500), }) /** A stable per-handler id so an invoked/result pair correlates in the log. */ @@ -91,13 +95,6 @@ function nextHandlerId(point: string): string { /** The `{kind:'plugin'}` source stamped on every context this bridge injects. */ const PLUGIN_SOURCE: MessageSource = { kind: 'plugin', plugin: 'hooks-claude' } -/** Truncate a stderr blob for the `hook/result` summary field. */ -function summarize(stderr: string): string | undefined { - const t = stderr.trim() - if (t.length === 0) return undefined - return t.length > 500 ? t.slice(0, 500) + '…' : t -} - export function apply(ctx: Context, config: Config): void { // --- Parse the config ONCE at load. A read/parse failure is contained: the // bridge logs and registers nothing rather than crashing boot (a typo'd path @@ -119,6 +116,7 @@ export function apply(ctx: Context, config: Config): void { } const defaultTimeoutMs = config.defaultTimeoutMs ?? 600_000 + const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 /** * Run every command hook configured for `point` whose matcher selects @@ -182,7 +180,7 @@ export function apply(ctx: Context, config: Config): void { ctx.logger.warn(`hooks-claude: ${point} hook emitted a systemMessage, which is not yet surfaced (ignored)`) } if (session && opts.turn !== undefined) { - const stderrSummary = summarize(output.stderr) + const stderrSummary = summarizeStderr(output.stderr, stderrSummaryMaxChars) appendHookResult(session, { turn: opts.turn, point, handlerId, decision: output.decision ?? (output.continue === false ? 'stop' : 'pass'), diff --git a/packages/hooks/hooks-claude/tests/coverage.spec.ts b/packages/hooks/hooks-claude/tests/coverage.spec.ts index 63e2f611ce..f5091ce7a5 100644 --- a/packages/hooks/hooks-claude/tests/coverage.spec.ts +++ b/packages/hooks/hooks-claude/tests/coverage.spec.ts @@ -27,7 +27,7 @@ function hooks(d: string, h: unknown): string { writeFileSync(join(d, 'hooks.json'), JSON.stringify({ hooks: h })); return join(d, 'hooks.json') } -type HarnessOpts = { pluginRoot?: string; projectDir?: string } +type HarnessOpts = { pluginRoot?: string; projectDir?: string; stderrSummaryMaxChars?: number } async function harness(configPath: string, adapter: MockAdapter, opts: HarnessOpts = {}): Promise { const ctx = new Context() await ctx.plugin(LlmService) @@ -139,6 +139,21 @@ describe('hooks-claude coverage — empty/no-op outcomes and no-agent paths', () await waitForIdle(ctx, agent) const res = events(agent).find(e => e.type === 'hook/result') expect(res?.type === 'hook/result' && res.data.stderrSummary?.endsWith('…')).toBe(true) + expect(res?.type === 'hook/result' && res.data.stderrSummary?.length).toBe(501) // default 500-char cap + ellipsis + }) + + it('the stderr summary cap is plugin config (stderrSummaryMaxChars)', async () => { + const d = dir() + const s = sh(d, 'long.sh', '#!/usr/bin/env bash\nprintf "x%.0s" {1..600} >&2\nexit 2\n') + const path = hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: s }] }] }) + const adapter = new MockAdapter([toolCallResponse('c1', 'echo', {}), textResponse('done')]) + const ctx = await harness(path, adapter, { stderrSummaryMaxChars: 40 }) + ctx.tools.register(defineTool({ name: 'echo', description: 'e', parameters: {}, async execute() { return [{ type: 'text', text: 'ok' }] } })) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]) + await waitForIdle(ctx, agent) + const res = events(agent).find(e => e.type === 'hook/result') + expect(res?.type === 'hook/result' && res.data.stderrSummary).toBe('x'.repeat(40) + '…') }) }) diff --git a/packages/hooks/hooks-codex/README.md b/packages/hooks/hooks-codex/README.md index 72be33f57f..68de9a49ae 100644 --- a/packages/hooks/hooks-codex/README.md +++ b/packages/hooks/hooks-codex/README.md @@ -20,6 +20,7 @@ const config: Config = { configPath: '/path/to/.codex/hooks.json', // required model: 'deepseek-v4', // optional: stamped on every payload (Codex includes `model`) defaultTimeoutMs: 600_000, // optional: per-hook timeout when a hook sets none + stderrSummaryMaxChars: 500, // optional: char cap on the hook/result event's persisted stderr summary } ``` diff --git a/packages/hooks/hooks-codex/src/index.ts b/packages/hooks/hooks-codex/src/index.ts index a704a6af24..4f7d6c9ed9 100644 --- a/packages/hooks/hooks-codex/src/index.ts +++ b/packages/hooks/hooks-codex/src/index.ts @@ -27,6 +27,7 @@ import { matchesMatcher, mergeHookOutputs, runHook, + summarizeStderr, type HookOutput, type MatcherGroup, type MergedHookOutcome, @@ -49,12 +50,15 @@ export interface Config { model?: string /** Default per-hook timeout in ms when a hook sets none (Codex default: 600000). */ defaultTimeoutMs?: number + /** Character cap for the `hook/result` event's persisted stderr summary. */ + stderrSummaryMaxChars?: number } export const Config: z = z.object({ configPath: z.string().required(), model: z.string().default(''), defaultTimeoutMs: z.number().default(600_000), + stderrSummaryMaxChars: z.number().default(500), }) let handlerCounter = 0 @@ -64,12 +68,6 @@ function nextHandlerId(point: string): string { const PLUGIN_SOURCE: MessageSource = { kind: 'plugin', plugin: 'hooks-codex' } -function summarize(stderr: string): string | undefined { - const t = stderr.trim() - if (t.length === 0) return undefined - return t.length > 500 ? t.slice(0, 500) + '…' : t -} - export function apply(ctx: Context, config: Config): void { let parsed: CodexHookConfig = {} try { @@ -85,6 +83,7 @@ export function apply(ctx: Context, config: Config): void { } const defaultTimeoutMs = config.defaultTimeoutMs ?? 600_000 + const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 const model = config.model ?? '' async function runPoint( @@ -140,7 +139,7 @@ export function apply(ctx: Context, config: Config): void { ctx.logger.warn(`hooks-codex: ${point} hook emitted a systemMessage, which is not yet surfaced (ignored)`) } if (session && opts.turn !== undefined) { - const stderrSummary = summarize(output.stderr) + const stderrSummary = summarizeStderr(output.stderr, stderrSummaryMaxChars) appendHookResult(session, { turn: opts.turn, point, handlerId, decision: output.decision ?? (output.continue === false ? 'stop' : 'pass'), diff --git a/packages/hooks/hooks-codex/tests/coverage.spec.ts b/packages/hooks/hooks-codex/tests/coverage.spec.ts index 87032c98ce..861a2691e1 100644 --- a/packages/hooks/hooks-codex/tests/coverage.spec.ts +++ b/packages/hooks/hooks-codex/tests/coverage.spec.ts @@ -23,12 +23,12 @@ function hooks(d: string, h: unknown): string { writeFileSync(join(d, 'hooks.json'), JSON.stringify({ hooks: h })); return join(d, 'hooks.json') } -async function harness(configPath: string, adapter: MockAdapter): Promise { +async function harness(configPath: string, adapter: MockAdapter, opts: { stderrSummaryMaxChars?: number } = {}): Promise { const ctx = new Context() await ctx.plugin(LlmService); await ctx.plugin(SessionStore); await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry); await ctx.plugin(AgentRegistry); await ctx.plugin(AgentLoop, { agents: [] }) await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) - await ctx.plugin(HooksCodex, { configPath, model: 'm' }) + await ctx.plugin(HooksCodex, { configPath, model: 'm', ...opts }) ctx.llm.registerAdapter(['mock'], adapter) return ctx } @@ -204,6 +204,19 @@ describe('hooks-codex coverage — decision mapping paths', () => { agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) const res = events(agent).find(e => e.type === 'hook/result') expect(res?.type === 'hook/result' && res.data.stderrSummary?.endsWith('…')).toBe(true) + expect(res?.type === 'hook/result' && res.data.stderrSummary?.length).toBe(501) // default 500-char cap + ellipsis + }) + + it('the stderr summary cap is plugin config (stderrSummaryMaxChars)', async () => { + const d = dir() + hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: sh(d, 'l.sh', '#!/usr/bin/env bash\nprintf "x%.0s" {1..600} >&2\nexit 2\n') }] }] }) + const adapter = new MockAdapter([toolCallResponse('c1', 'Bash', { command: 'x' }), textResponse('done')]) + const ctx = await harness(join(d, 'hooks.json'), adapter, { stderrSummaryMaxChars: 40 }) + ctx.tools.register(defineTool({ name: 'Bash', description: 'b', parameters: { command: { type: 'string' } }, async execute() { return [{ type: 'text', text: 'ok' }] } })) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + const res = events(agent).find(e => e.type === 'hook/result') + expect(res?.type === 'hook/result' && res.data.stderrSummary).toBe('x'.repeat(40) + '…') }) it('warns on a skipped async hook and a direct apply() defaults the timeout', async () => { diff --git a/packages/session-persistence/session-persistence-sqlite/README.md b/packages/session-persistence/session-persistence-sqlite/README.md index 064cc11fce..d7095633c9 100644 --- a/packages/session-persistence/session-persistence-sqlite/README.md +++ b/packages/session-persistence/session-persistence-sqlite/README.md @@ -8,7 +8,7 @@ A SQLite durable session-persistence backend — a second `SessionPersistence` i Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data, source_event_seqs, surface_op)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). The two `TEXT` columns `source_event_seqs` and `surface_op` are nullable; they store the event's optional surface-metadata fields (see [session surface](../../../docs/rfc/implemented/architecture/2026-06-18-session-surface.md)). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`list` reports exactly the sessions that have a row), so no separate column is needed. -The repo targets Node ≥ 24 (the root `engines` field), which includes the stable `node:sqlite` module. The database opens with `foreign_keys = ON` (so `ON DELETE CASCADE` drops a session's events with its row) and `journal_mode = WAL`. The table-layout version is stored in `PRAGMA user_version` and checked on open: a fresh database is stamped with the current `SCHEMA_VERSION`; a database written by any other, incompatible build (a non-current `user_version`, older or newer) is rejected rather than opened against an unknown layout — there is no migration (unreleased software). +The repo targets Node ≥ 24 (the root `engines` field), which includes the stable `node:sqlite` module. The database opens with `foreign_keys = ON` (so `ON DELETE CASCADE` drops a session's events with its row) and the configured `journal_mode` (default `wal`; pick a rollback-journal mode like `delete` on filesystems where WAL's shared-memory files do not work, e.g. network mounts). The table-layout version is stored in `PRAGMA user_version` and checked on open: a fresh database is stamped with the current `SCHEMA_VERSION`; a database written by any other, incompatible build (a non-current `user_version`, older or newer) is rejected rather than opened against an unknown layout — there is no migration (unreleased software). ## Contract semantics over rows @@ -21,6 +21,7 @@ The repo targets Node ≥ 24 (the root `engines` field), which includes the stab ```ts interface Config { path: string // SQLite database file path, or ':memory:' for an in-process DB + journalMode?: 'wal' | 'delete' | 'truncate' | 'persist' // journal_mode pragma; default 'wal' } ``` diff --git a/packages/session-persistence/session-persistence-sqlite/src/index.ts b/packages/session-persistence/session-persistence-sqlite/src/index.ts index 412c3b58fc..30387b4837 100644 --- a/packages/session-persistence/session-persistence-sqlite/src/index.ts +++ b/packages/session-persistence/session-persistence-sqlite/src/index.ts @@ -28,7 +28,7 @@ import { } from '@deepseek-ai/dsh-session-persistence' import type { Session, SessionEvent, SurfaceEventType, SessionId, SessionHeader } from '@deepseek-ai/dsh-session' import { - openDatabase, rowToMeta, scanRows, type EventRow, type SessionRow, + type JournalMode, openDatabase, rowToMeta, scanRows, type EventRow, type SessionRow, } from './schema.ts' export { SCHEMA_VERSION } from './schema.ts' @@ -54,6 +54,13 @@ export interface Config { * dirs) on construction. */ path: string + /** + * SQLite `journal_mode` pragma. `wal` (the default) is the recorded + * durability model; pick a rollback-journal mode (`delete`/`truncate`/ + * `persist`) on filesystems where WAL's shared-memory files do not work + * (network mounts). See {@link JournalMode}. + */ + journalMode?: JournalMode } /** @@ -66,6 +73,7 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers static Config: z = z.object({ path: z.string().required(), + journalMode: z.union(['wal', 'delete', 'truncate', 'persist'] as const).default('wal'), }) /** @@ -83,18 +91,19 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers super(ctx) // Open the database asynchronously (the parent directory may need creating); // every hook awaits `ready` first. Opening synchronously would force a sync - // mkdir and block plugin apply. - this.ready = this.openDb(config.path) + // mkdir and block plugin apply. schemastery (static Config) has already + // filled `journalMode`; the cast records that runtime fact. + this.ready = this.openDb(config.path, (config as Required).journalMode) this.coordinator = new PersistenceCoordinator(this.ctx, this) } - private async openDb(path: string): Promise { + private async openDb(path: string, journalMode: JournalMode): Promise { if (path !== ':memory:') { const abs = resolve(path) await mkdir(dirname(abs), { recursive: true, mode: 0o700 }) - this.db = openDatabase(abs) + this.db = openDatabase(abs, journalMode) } else { - this.db = openDatabase(path) + this.db = openDatabase(path, journalMode) } } diff --git a/packages/session-persistence/session-persistence-sqlite/src/schema.ts b/packages/session-persistence/session-persistence-sqlite/src/schema.ts index d8db0e087b..2ed6f5853c 100644 --- a/packages/session-persistence/session-persistence-sqlite/src/schema.ts +++ b/packages/session-persistence/session-persistence-sqlite/src/schema.ts @@ -45,10 +45,21 @@ export interface EventRow { surface_op: string | null } +/** + * Journal modes the backend will run under. `wal` is the default and the + * durability model the persistence ADR records; the rollback-journal modes + * (`delete`/`truncate`/`persist`) exist for filesystems where WAL's + * shared-memory files do not work (network mounts). `memory`/`off` are + * excluded: dropping journal durability silently contradicts what this + * backend promises. + */ +export type JournalMode = 'wal' | 'delete' | 'truncate' | 'persist' + /** * Open the database at `path` and apply the schema + pragmas. `foreign_keys` - * makes `ON DELETE CASCADE` drop a session's events with its row; `journal_mode - * = WAL` matches the durability model the ADR records (the row shape maps 1:1 + * makes `ON DELETE CASCADE` drop a session's events with its row; the + * `journal_mode` pragma is set from the plugin's `journalMode` config (`wal` + * default — the durability model the ADR records; the row shape maps 1:1 * onto `SessionEvent`; opencode runs this exact shape on SQLite/WAL). * * The table-layout version is persisted in SQLite's `PRAGMA user_version` and @@ -66,10 +77,12 @@ export interface EventRow { * makes the version check reject both sibling v3 databases instead of opening * one against columns it does not have. */ -export function openDatabase(path: string): DatabaseSync { +export function openDatabase(path: string, journalMode: JournalMode): DatabaseSync { const db = new DatabaseSync(path) db.exec('PRAGMA foreign_keys = ON') - db.exec('PRAGMA journal_mode = WAL') + // journalMode is a closed in-code union (validated by the plugin Config), not + // user-controlled SQL — safe to interpolate (PRAGMA takes no bound params). + db.exec(`PRAGMA journal_mode = ${journalMode.toUpperCase()}`) // `PRAGMA user_version` always returns exactly one row { user_version }. const { user_version: onDisk } = db.prepare('PRAGMA user_version').get() as { user_version: number } if (onDisk !== 0 && onDisk !== SCHEMA_VERSION) { diff --git a/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts b/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts index 7138718aa5..a6eda72685 100644 --- a/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts +++ b/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it } from 'vitest' import { Context } from 'cordis' +import { existsSync } from 'node:fs' import { mkdtemp, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -53,7 +54,7 @@ runCoordinatorContract('sqlite', async (): Promise => { // A row past the committed region whose `data` does not parse: scanRows // bounds the preserved prefix at it and returns its seq as tornFrom, which // the backend surfaces to the coordinator as the tornMarker to delete from. - const db = openDatabase(path) + const db = openDatabase(path, 'wal') const next = (db.prepare('SELECT COALESCE(MAX(seq), -1) + 1 AS n FROM events WHERE session_id = ?') .get(id) as { n: number }).n db.prepare('INSERT INTO events (session_id, seq, type, time, data) VALUES (?, ?, ?, ?, ?)') @@ -192,7 +193,7 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { await b1.ctx.sessionPersistence.append(m.id, oneTurnLog()) // seqs 0..5 await b1.dispose() // Hand-write an interrupted turn (turn/start seq 6, no turn/end). - const db = openDatabase(path) + const db = openDatabase(path, 'wal') db.prepare('INSERT INTO events (session_id, seq, type, time, data) VALUES (?, 6, ?, 7, ?)') .run(m.id, 'turn/start', JSON.stringify({ turn: 2, trigger: { kind: 'message', source: { kind: 'user' } } })) db.close() @@ -204,7 +205,7 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { expect(loaded.events.at(-1)!.type).toBe('turn/end') // load() is mutating: the synthetic turn/end MUST be on disk so the stored log // is balanced and the cursor is truthful (contract: load closes, not defers). - const probe = openDatabase(path) + const probe = openDatabase(path, 'wal') const stored = probe.prepare('SELECT seq, type FROM events WHERE session_id = ? ORDER BY seq').all(m.id) as { seq: number; type: string }[] probe.close() expect(stored.map(r => r.seq)).toEqual([0, 1, 2, 3, 4, 5, 6, 7]) @@ -237,21 +238,21 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { it('rejects opening a database whose schema version is not the current build (newer OR older)', async () => { const path = await freshDbPath() - openDatabase(path).close() // stamp user_version = SCHEMA_VERSION + openDatabase(path, 'wal').close() // stamp user_version = SCHEMA_VERSION // Bump user_version past what this build supports. - const dbNewer = openDatabase(path) + const dbNewer = openDatabase(path, 'wal') dbNewer.exec(`PRAGMA user_version = ${SCHEMA_VERSION + 1}`) dbNewer.close() - expect(() => openDatabase(path)).toThrow(/incompatible with this build/) + expect(() => openDatabase(path, 'wal')).toThrow(/incompatible with this build/) // A stale OLDER version (e.g. a pre-summary-drop v1 DB) is also rejected — // we do not migrate (unreleased software, no backward-compat). const olderPath = await freshDbPath() - openDatabase(olderPath).close() - const dbOlder = openDatabase(olderPath) + openDatabase(olderPath, 'wal').close() + const dbOlder = openDatabase(olderPath, 'wal') dbOlder.exec('PRAGMA user_version = 1') dbOlder.close() - expect(() => openDatabase(olderPath)).toThrow(/incompatible with this build/) + expect(() => openDatabase(olderPath, 'wal')).toThrow(/incompatible with this build/) }) it('rejects a sibling v3 database (the merge-collided version) rather than opening it against missing columns', async () => { @@ -261,11 +262,11 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { // of this build's columns, so it MUST be rejected, not opened. Stamp a v3 // database and confirm the version check refuses it. const path = await freshDbPath() - openDatabase(path).close() // creates + stamps user_version = SCHEMA_VERSION (4) - const db = openDatabase(path) + openDatabase(path, 'wal').close() // creates + stamps user_version = SCHEMA_VERSION (4) + const db = openDatabase(path, 'wal') db.exec('PRAGMA user_version = 3') db.close() - expect(() => openDatabase(path)).toThrow(/schema version 3, incompatible with this build/) + expect(() => openDatabase(path, 'wal')).toThrow(/schema version 3, incompatible with this build/) }) it('a corrupt-JSON row in the uncommitted tail is discarded on load, not unloadable', async () => { @@ -281,7 +282,7 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { // unloadable; a torn tail must be discarded. scanRows finds the last // turn/end on the seq+type columns (never parsing tail `data`), so the // unparsable row after it bounds the preserved prefix and is deleted by load. - const db = openDatabase(path) + const db = openDatabase(path, 'wal') db.prepare('INSERT INTO events (session_id, seq, type, time, data) VALUES (?, 6, ?, 7, ?)') .run(m.id, 'turn/start', '{not valid json') db.close() @@ -370,6 +371,29 @@ describe('SessionPersistenceSqlite: edge cases', () => { await b2.dispose() }) + it('journalMode config reaches the database (default wal, rollback modes selectable)', async () => { + // :memory: databases always report journal_mode=memory, so probe file DBs. + const walPath = await freshDbPath() + const bWal = await backend(walPath) + await bWal.ctx.sessionPersistence.create(meta('jm-wal')) + expect((openDatabase(walPath, 'wal').prepare('PRAGMA journal_mode').get() as { journal_mode: string }).journal_mode).toBe('wal') + await bWal.dispose() + + const deletePath = await freshDbPath() + const ctx = new Context() + await ctx.plugin(SessionStore) + const fiber = await ctx.plugin(SessionPersistenceSqlite, { path: deletePath, journalMode: 'delete' }) + await ctx.sessionPersistence.create(meta('jm-delete')) + // Probe through a second connection: journal_mode=delete is a per-database + // property only insofar as no WAL files exist — assert the world, not the + // backend's self-report (no -wal sidecar after writes in delete mode). + const db = openDatabase(deletePath, 'delete') + expect((db.prepare('PRAGMA journal_mode').get() as { journal_mode: string }).journal_mode).toBe('delete') + db.close() + expect(existsSync(`${deletePath}-wal`)).toBe(false) + await fiber.dispose() + }) + it('HMR: a DIFFERENT session colliding with a materialized on-disk id is rejected', async () => { const path = await freshDbPath() // Instance 1 materializes a session and disposes. diff --git a/packages/subagent/subagent-acp/README.md b/packages/subagent/subagent-acp/README.md index 03990a7369..81bf886067 100644 --- a/packages/subagent/subagent-acp/README.md +++ b/packages/subagent/subagent-acp/README.md @@ -25,6 +25,8 @@ Unlike the in-process backends, the child does NOT share this cordis context — | `cwd` | string | parent cwd | Working directory for the child process and its ACP session. | | `permission` | `'allow' \| 'reject'` | `reject` | How to auto-answer the child's `session/request_permission` prompts. `reject` declines every prompt (answer `cancelled`); `allow` approves via the first allow-shaped option. The first cut surfaces no prompt to a human. | | `env` | Record | `{}` | Extra env vars for the child (e.g. its own `DEEPSEEK_API_KEY`). Forwarded on top of a credential-scrubbed copy of the parent env, so an explicit key reaches the child while ambient secrets do not leak implicitly. | +| `disposeEofGraceMs` | number | `6000` | Dispose ladder tier 1: how long the child gets to quiesce on its own after stdin EOF (flush persistence, tear down its nested subprocesses) before SIGTERM. | +| `disposeGraceMs` | number | `3000` | Dispose ladder tier 2: grace between SIGTERM and the SIGKILL escalation. | ```yaml - id: subagent-acp diff --git a/packages/subagent/subagent-acp/src/index.ts b/packages/subagent/subagent-acp/src/index.ts index 037d32889e..cd425e763c 100644 --- a/packages/subagent/subagent-acp/src/index.ts +++ b/packages/subagent/subagent-acp/src/index.ts @@ -21,7 +21,7 @@ import type { Context } from 'cordis' import z from 'schemastery' import type { SubagentCapabilities, SubagentProvider, SubagentStartRequest } from '@deepseek-ai/dsh-subagent' -import { type AcpRunSpec, type PermissionPolicy, startAcpRun } from './run.ts' +import { type AcpRunSpec, DEFAULT_DISPOSE_EOF_GRACE_MS, DEFAULT_DISPOSE_GRACE_MS, type PermissionPolicy, startAcpRun } from './run.ts' export const name = 'subagent-acp' export const inject = ['subagents'] @@ -52,6 +52,14 @@ export interface Config { * ambient secrets do not leak implicitly. */ env: Record + /** + * Grace period (ms) for the child's EOF-driven quiesce on dispose — its + * window to flush persistence and tear down its own nested subprocesses + * before the parent escalates to a signal. + */ + disposeEofGraceMs?: number + /** Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation on dispose. */ + disposeGraceMs?: number } export const Config: z = z.object({ @@ -61,8 +69,20 @@ export const Config: z = z.object({ cwd: z.string(), permission: z.union(['allow', 'reject'] as const).default('reject'), env: z.dict(z.string()).default({}), + disposeEofGraceMs: z.number().default(DEFAULT_DISPOSE_EOF_GRACE_MS), + disposeGraceMs: z.number().default(DEFAULT_DISPOSE_GRACE_MS), }) +/** A dispose grace must be a positive finite number (it bounds the teardown wait). */ +function assertPositiveFinite(name: string, value: number): void { + if (!Number.isFinite(value) || value <= 0) { + throw new Error(`subagent-acp: ${name} must be a positive finite number`) + } +} + +/** The shape after schemastery applied the defaults (cwd has none). */ +type ResolvedConfig = Required> & Pick + /** * The ACP provider. Advertises NO start-time capabilities: an out-of-process * child cannot honor `outputSchema`/`maxDepth`/`toolFilter` (the service rejects @@ -71,7 +91,7 @@ export const Config: z = z.object({ class AcpProvider implements SubagentProvider { readonly capabilities: SubagentCapabilities = { outputSchema: false, depthLimit: false, toolFilter: false } - constructor(readonly name: string, private readonly ctx: Context, private readonly config: Config) {} + constructor(readonly name: string, private readonly ctx: Context, private readonly config: ResolvedConfig) {} start(request: SubagentStartRequest) { const spec: AcpRunSpec = { @@ -80,6 +100,8 @@ class AcpProvider implements SubagentProvider { cwd: this.config.cwd ?? process.cwd(), permission: this.config.permission, env: this.config.env, + disposeEofGraceMs: this.config.disposeEofGraceMs, + disposeGraceMs: this.config.disposeGraceMs, onError: (error, stopReason) => { // The seam forbids `result` rejecting, so a child-level failure is // flattened to a stop reason — preserve it here rather than losing it. @@ -91,5 +113,9 @@ class AcpProvider implements SubagentProvider { } export function apply(ctx: Context, config: Config): void { - ctx.subagents.registerProvider(new AcpProvider(config.providerName, ctx, config)) + // schemastery (Config) has already filled every defaulted field. + const resolved = config as ResolvedConfig + assertPositiveFinite('disposeEofGraceMs', resolved.disposeEofGraceMs) + assertPositiveFinite('disposeGraceMs', resolved.disposeGraceMs) + ctx.subagents.registerProvider(new AcpProvider(resolved.providerName, ctx, resolved)) } diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index 06f7a9ece8..74291b7e65 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -73,16 +73,16 @@ export interface AcpRunSpec { /** * Grace period (ms) for the child's EOF-driven quiesce in * {@link SubagentRun.dispose} — the window to flush persistence and tear down - * its OWN nested subprocesses before the parent escalates to a signal. Defaults - * to {@link DEFAULT_DISPOSE_EOF_GRACE_MS}; a test injects a small value. + * its OWN nested subprocesses before the parent escalates to a signal. The + * plugin fills this from its `disposeEofGraceMs` config. */ - disposeEofGraceMs?: number + disposeEofGraceMs: number /** * Grace period (ms) between `SIGTERM` and the `SIGKILL` escalation in - * {@link SubagentRun.dispose}. Defaults to {@link DEFAULT_DISPOSE_GRACE_MS}; - * a test injects a small value to exercise the escalation without a long wait. + * {@link SubagentRun.dispose}. The plugin fills this from its + * `disposeGraceMs` config. */ - disposeGraceMs?: number + disposeGraceMs: number /** * Sink for a child-level failure that the run flattened into a stop reason * (the seam contract forbids `result` rejecting). The driver calls this with @@ -94,19 +94,20 @@ export interface AcpRunSpec { } /** - * Default grace for the child's EOF-driven quiesce on dispose — the window for it - * to flush persistence and tear down its OWN nested subprocesses (which may run - * their own `SIGTERM`→`SIGKILL` escalation) before the parent escalates to a - * signal. Deliberately LARGER than {@link DEFAULT_DISPOSE_GRACE_MS}: a cooperative - * child whose teardown is itself waiting on a signal-trapping grandchild (e.g. a - * bash subprocess in its own ~3s SIGTERM→SIGKILL grace) plus a final flush needs - * MORE than a single signal-grace of headroom, or the parent's SIGTERM cuts it off - * exactly as it reaches its own SIGKILL+flush. The child is an arbitrary ACP agent, - * so this is a standalone generous default, NOT derived from any child's internals. + * Default grace for the child's EOF-driven quiesce on dispose (the + * `disposeEofGraceMs` config) — the window for it to flush persistence and tear + * down its OWN nested subprocesses (which may run their own `SIGTERM`→`SIGKILL` + * escalation) before the parent escalates to a signal. Deliberately LARGER than + * {@link DEFAULT_DISPOSE_GRACE_MS}: a cooperative child whose teardown is itself + * waiting on a signal-trapping grandchild (e.g. a bash subprocess in its own ~3s + * SIGTERM→SIGKILL grace) plus a final flush needs MORE than a single + * signal-grace of headroom, or the parent's SIGTERM cuts it off exactly as it + * reaches its own SIGKILL+flush. The child is an arbitrary ACP agent, so this is + * a standalone generous default, NOT derived from any child's internals. */ export const DEFAULT_DISPOSE_EOF_GRACE_MS = 6_000 -/** Default grace between SIGTERM and SIGKILL on dispose (mirrors the bash executor). */ +/** Default grace between SIGTERM and SIGKILL on dispose (the `disposeGraceMs` config; mirrors the bash executor). */ export const DEFAULT_DISPOSE_GRACE_MS = 3_000 /** @@ -372,8 +373,8 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su // Reach quiescence, not merely request it (dispose must AWAIT the child // actually stopping). If the child is already gone, nothing to do. if (child.exitCode !== null || child.signalCode !== null) return - const eofGraceMs = spec.disposeEofGraceMs ?? DEFAULT_DISPOSE_EOF_GRACE_MS - const graceMs = spec.disposeGraceMs ?? DEFAULT_DISPOSE_GRACE_MS + const eofGraceMs = spec.disposeEofGraceMs + const graceMs = spec.disposeGraceMs // 1. Graceful: end the ACP request stream (stdin EOF) and let the child // quiesce ON ITS OWN. Our acp-agent has NO SIGTERM handler in a normal // session — it tears down via the server bridge's connection-close path diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index 9819320ec5..e1e510d19d 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -8,7 +8,7 @@ import { fileURLToPath } from 'node:url' import SubagentService from '@deepseek-ai/dsh-subagent' import type { Agent } from '@deepseek-ai/dsh-agent' import * as acp from '../src/index.ts' -import { acpStopReason, acpContentText, buildChildEnv, SENSITIVE_ENV_PATTERN, startAcpRun, toAcpPrompt, type AcpRunSpec } from '../src/run.ts' +import { acpStopReason, acpContentText, buildChildEnv, DEFAULT_DISPOSE_EOF_GRACE_MS, DEFAULT_DISPOSE_GRACE_MS, SENSITIVE_ENV_PATTERN, startAcpRun, toAcpPrompt, type AcpRunSpec } from '../src/run.ts' /** * Keyless integration tests for the ACP subagent backend. Each spawns a REAL @@ -171,7 +171,7 @@ describe('dsh-subagent-acp', () => { const run = startAcpRun( { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent, signal: controller.signal }, // `touch ` — runs only if the process is actually spawned. - { command: 'touch', args: [sentinel], cwd: tmp, permission: 'reject', env: {} }, + { command: 'touch', args: [sentinel], cwd: tmp, permission: 'reject', env: {}, disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS, disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS }, ) const result = await run.result expect(result.stopReason).toBe('aborted') @@ -390,7 +390,7 @@ describe('dsh-subagent-acp', () => { // absent-sink branch). const run = startAcpRun( { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, - { command: '/nonexistent/acp-agent-binary', args: [], cwd: process.cwd(), permission: 'reject', env: {} }, + { command: '/nonexistent/acp-agent-binary', args: [], cwd: process.cwd(), permission: 'reject', env: {}, disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS, disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS }, ) const result = await run.result // The seam contract: a child-level failure resolves error, never rejects. @@ -398,6 +398,16 @@ describe('dsh-subagent-acp', () => { await run.dispose() }) + it('rejects a non-positive dispose grace at load', async () => { + for (const bad of [{ disposeEofGraceMs: 0 }, { disposeGraceMs: -1 }, { disposeEofGraceMs: Number.NaN }]) { + const ctx = new Context() + await ctx.plugin(SubagentService) + await expect(ctx.plugin(acp, { providerName: 'acp', command: 'true', args: [], permission: 'reject', env: {}, ...bad })) + .rejects.toThrow(/subagent-acp: dispose(?:Eof)?GraceMs must be a positive finite number/) + await ctx.fiber.dispose() + } + }) + it('resolves error via the provider (real load path) when the command does not exist', async () => { const ctx = new Context() await ctx.plugin(SubagentService) @@ -428,6 +438,8 @@ describe('dsh-subagent-acp', () => { cwd: process.cwd(), permission: 'reject', env: {}, + disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS, + disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS, onError: (error, stopReason) => { errors.push({ message: error.message, stopReason }) }, }, ) diff --git a/packages/web/tool-web/README.md b/packages/web/tool-web/README.md index f57f38d0d5..e789720cbb 100644 --- a/packages/web/tool-web/README.md +++ b/packages/web/tool-web/README.md @@ -17,6 +17,7 @@ Each tool is registered independently; a product that wants only one disables th |---|---|---| | `search` | `true` | Register `web_search`. | | `fetch` | `true` | Register `web_fetch`. | +| `searchMaxResults` | `8` | Upper bound on sources returned by one `web_search` call (the seam truncates a longer provider list and flags it). | ```yaml - id: tool-web diff --git a/packages/web/tool-web/src/index.ts b/packages/web/tool-web/src/index.ts index df8029466a..c0191c023b 100644 --- a/packages/web/tool-web/src/index.ts +++ b/packages/web/tool-web/src/index.ts @@ -20,7 +20,7 @@ import type { Context } from 'cordis' import z from 'schemastery' import type {} from '@deepseek-ai/dsh-web' -import { applyWebSearchTool } from './search.ts' +import { applyWebSearchTool, WEB_SEARCH_MAX_RESULTS } from './search.ts' import { applyWebFetchTool } from './fetch.ts' export { WEB_SEARCH_MAX_RESULTS, applyWebSearchTool, formatSearchOutput, parseSearchArgs, presentSearchCall } from './search.ts' @@ -38,13 +38,26 @@ export interface Config { search?: boolean /** Register `web_fetch`. Defaults to true. */ fetch?: boolean + /** Upper bound on sources returned by one `web_search` call. */ + searchMaxResults?: number } export const Config: z = z.object({ search: z.boolean().default(true), fetch: z.boolean().default(true), + searchMaxResults: z.number().default(WEB_SEARCH_MAX_RESULTS), }) +/** The shape after schemastery applies its defaults to every field. */ +type ResolvedConfig = Required + +/** The result cap must be a positive integer (it bounds a provider's source list). */ +function assertPositiveInteger(name: string, value: number): void { + if (!Number.isInteger(value) || value < 1) { + throw new Error(`tool-web: ${name} must be a positive integer`) + } +} + /** * Register the enabled web tools. `search`/`fetch` default to true; a product * that wants only one disables the other in config. The tools' disposers are @@ -52,6 +65,9 @@ export const Config: z = z.object({ * teardown is needed. */ export function apply(ctx: Context, config: Config): void { - if (config.search !== false) applyWebSearchTool(ctx) - if (config.fetch !== false) applyWebFetchTool(ctx) + // schemastery (Config) has already filled every defaulted field. + const resolved = config as ResolvedConfig + assertPositiveInteger('searchMaxResults', resolved.searchMaxResults) + if (resolved.search) applyWebSearchTool(ctx, resolved.searchMaxResults) + if (resolved.fetch) applyWebFetchTool(ctx) } diff --git a/packages/web/tool-web/src/search.ts b/packages/web/tool-web/src/search.ts index 6394d3f7e0..28e4e9a2e9 100644 --- a/packages/web/tool-web/src/search.ts +++ b/packages/web/tool-web/src/search.ts @@ -13,10 +13,10 @@ import type { WebSearchResult } from '@deepseek-ai/dsh-web' import type {} from '@deepseek-ai/dsh-system-prompt' /** - * Default upper bound on returned sources. Owned by the consumer (not the - * provider or model), mirroring `dsh-tool-fs`'s `READ_LIMIT`/`GREP_LIMIT`. The - * model just asks a question; the product controls how much context returns. - * The default `8` aligns with OpenCode's Exa default. + * Default upper bound on returned sources (the `searchMaxResults` config). + * Owned by the consumer (not the provider or model), mirroring `dsh-tool-fs`'s + * `READ_LIMIT`. The model just asks a question; the product controls how much + * context returns. The default `8` aligns with OpenCode's Exa default. */ export const WEB_SEARCH_MAX_RESULTS = 8 @@ -67,8 +67,8 @@ export function presentSearchCall(args: { query: string }): GenericCallView { return { card: 'generic', title: args.query, kind: 'search', rawInput: args.query } } -/** Register the `web_search` tool and its system-prompt guidance. */ -export function applyWebSearchTool(ctx: Context): void { +/** Register the `web_search` tool and its system-prompt guidance. `maxResults` is the deployment's source cap. */ +export function applyWebSearchTool(ctx: Context, maxResults: number): void { ctx.systemPrompt.section({ name: 'tool:web_search', order: 110, @@ -84,7 +84,7 @@ export function applyWebSearchTool(ctx: Context): void { async execute(args, exec): Promise { const input = parseSearchArgs(args) const result = await ctx.web.search( - { query: input.query, maxResults: WEB_SEARCH_MAX_RESULTS }, + { query: input.query, maxResults }, exec.signal ? { signal: exec.signal } : undefined, ) return [{ type: 'text', text: formatSearchOutput(result) }] diff --git a/packages/web/tool-web/tests/tool-web.spec.ts b/packages/web/tool-web/tests/tool-web.spec.ts index 7af1ce7c36..c253bebfef 100644 --- a/packages/web/tool-web/tests/tool-web.spec.ts +++ b/packages/web/tool-web/tests/tool-web.spec.ts @@ -15,6 +15,7 @@ import { presentFetchCall, renderBody, htmlToMarkdown, + WEB_SEARCH_MAX_RESULTS, } from '@deepseek-ai/dsh-tool-web' const available: WebProviderStatus = { available: true } @@ -279,3 +280,48 @@ describe('tool-web execution through the real registry', () => { await fiber.dispose() }) }) + +describe('searchMaxResults is plugin config', () => { + it('forwards the default cap to the seam when unconfigured', async () => { + const seen: { maxResults?: number | undefined } = {} + const provider: WebSearchProvider = { + id: 'stub-search', + status: () => available, + search: (request) => { seen.maxResults = request.maxResults; return Promise.resolve({ providerId: 'stub-search', query: 'q', sources: [], truncated: false }) }, + } + const { fiber, call } = await mountTools({ webConfig: { searchProvider: 'stub-search' }, search: provider }) + await call('web_search', { query: 'q' }) + expect(seen.maxResults).toBe(WEB_SEARCH_MAX_RESULTS) + await fiber.dispose() + }) + + it('forwards a configured cap to the seam, which enforces it', async () => { + const sources = Array.from({ length: 5 }, (_, i) => ({ url: `https://s${i}.test` })) + const provider: WebSearchProvider = { + id: 'stub-search', + status: () => available, + search: request => Promise.resolve({ providerId: 'stub-search', query: request.query, sources, truncated: false }), + } + const { fiber, call } = await mountTools({ config: { searchMaxResults: 2 }, webConfig: { searchProvider: 'stub-search' }, search: provider }) + const out = await call('web_search', { query: 'q' }) + expect(out.isError).toBe(false) + const body = out.content.map(b => b.text).join('') + expect(body).toContain('https://s1.test') + expect(body).not.toContain('https://s2.test') + expect(body).toContain('Showing the first 2 sources.') + await fiber.dispose() + }) + + it.each([ + ['zero', 0], + ['negative', -3], + ['fractional', 1.5], + ])('rejects a %s searchMaxResults at load', async (_label, value) => { + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(WebService, {}) + await expect(ctx.plugin(ToolWeb, { searchMaxResults: value })) + .rejects.toThrow(/tool-web: searchMaxResults must be a positive integer/) + }) +}) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8045aa9cc3..a58d9cb72d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -323,6 +323,9 @@ importers: diff: specifier: ^9.0.0 version: 9.0.0 + schemastery: + specifier: ^3.18.0 + version: 3.18.0 devDependencies: '@deepseek-ai/dsh-agent': specifier: workspace:^ From 48d25cdd44abe6cfea67b1bd2584049d7c84860a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 18:06:35 +0800 Subject: [PATCH 7/8] Fix review findings: validate the hooks cap, integer read caps, doc drift, config plumb-through test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Codex review pass on the draft caught four real gaps and two solid suggestions; all addressed except one pushed back on the merits: - hooks-claude/hooks-codex: stderrSummaryMaxChars was the one new knob with NO range validation — a negative/NaN cap would silently misbehave inside slice(). Both bridges now assert a positive integer at the TOP of apply() (before the config-file parse's early return, so a bad value fails the load loudly), with rejection tests. - tool-fs: the read caps count lines/chars/bytes, so positive-FINITE was too loose (a fractional readLimit would flow into windowing arithmetic and the schema description). All four now require a positive integer, matching tool-web's cap. - Doc drift the gates cannot catch: tool-web's README tools table still named WEB_SEARCH_MAX_RESULTS as the mechanism; compact-basic's README/module doc and the compaction-capability-seam RFC still described estimation as fixed char/4 rather than the charsPerToken default. - subagent-acp: the dispose graces were tested only at the startAcpRun level, so a regression that stopped threading plugin config into AcpRunSpec would have survived. A provider-path test now drives the trap-escalation scenario through ctx.subagents.start with small config graces and bounds dispose at 4s. Pushed back on: converting compact-basic's charsPerToken to a schemastery field. The package's whole config is deliberately hand-rolled (resolveConfig, every threshold REQUIRED with no default — a documented design posture); one schemastery field beside it would be incoherent. The knob is cordis.yml-reachable, defaulted, and validated, which is what the convention requires; migrating the package to schemastery wholesale is pre-existing config-surface hygiene out of this change's scope. --- .../2026-06-18-compaction-capability-seam.md | 2 +- packages/compact/compact-basic/README.md | 4 +-- packages/compact/compact-basic/src/index.ts | 3 +- packages/fs/tool-fs/src/index.ts | 16 +++++----- packages/fs/tool-fs/tests/tools.spec.ts | 5 +-- packages/hooks/hooks-claude/src/index.ts | 12 ++++++- .../hooks/hooks-claude/tests/coverage.spec.ts | 10 ++++++ packages/hooks/hooks-codex/src/index.ts | 12 ++++++- .../hooks/hooks-codex/tests/coverage.spec.ts | 10 ++++++ .../subagent-acp/tests/subagent-acp.spec.ts | 31 +++++++++++++++++++ packages/web/tool-web/README.md | 2 +- 11 files changed, 90 insertions(+), 17 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md b/docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md index 9e08df2fbd..f09dab5cd1 100644 --- a/docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md +++ b/docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md @@ -17,7 +17,7 @@ Two forces shape the design. First, compaction is **swappable**: token counting Per the [capability-seams RFC](../../implemented/architecture/2026-06-13-capability-seams.md), compaction ships as separate packages so the contract, the algorithm, and (later) the consumer surface evolve independently: 1. **Interface** — `@deepseek-ai/dsh-compact`: an abstract `CompactService` owning the `ctx.compact` key, the `CompactionResult` vocabulary, and the `compact/*` session events. It declares `compactIfNeeded()` and `compactRegion()` as **abstract** — the contract states *what* compaction does, not *how*. -2. **Implementation** — `@deepseek-ai/dsh-compact-basic`: a concrete `BasicCompactService` that owns the entire algorithm — token estimation (char/4 + per-block overhead), the tail→head retention walk, summarization via `ctx.llm.stream()`, the surface replacement, the lock, and the `agent/pre-step` auto-compaction listener. A tokenizer-based or template-based backend is a sibling package (or a subclass overriding the two protected estimation/summarization hooks). +2. **Implementation** — `@deepseek-ai/dsh-compact-basic`: a concrete `BasicCompactService` that owns the entire algorithm — token estimation (chars per token — the `charsPerToken` config, default 4 — + per-block overhead), the tail→head retention walk, summarization via `ctx.llm.stream()`, the surface replacement, the lock, and the `agent/pre-step` auto-compaction listener. A tokenizer-based or template-based backend is a sibling package (or a subclass overriding the two protected estimation/summarization hooks). 3. **Consumer** — deferred. A `/compact` tool and slash command will `inject: ['compact']` and call the contract; they are intentionally out of scope here so the seam settles first. ### The contract depends on `dsh-session` and `dsh-llm` — a deliberate deviation diff --git a/packages/compact/compact-basic/README.md b/packages/compact/compact-basic/README.md index cbd1146634..ba976d2833 100644 --- a/packages/compact/compact-basic/README.md +++ b/packages/compact/compact-basic/README.md @@ -1,6 +1,6 @@ # @deepseek-ai/dsh-compact-basic -The **basic compaction backend**: a `BasicCompactService` implementing the `@deepseek-ai/dsh-compact` seam with a char/4 token heuristic, token-budget retention, and summarization routed through the agent request pipeline. +The **basic compaction backend**: a `BasicCompactService` implementing the `@deepseek-ai/dsh-compact` seam with a chars-per-token heuristic (the `charsPerToken` config, default 4), token-budget retention, and summarization routed through the agent request pipeline. This is the implementation tier of the compaction capability — see the [interface package](../compact/README.md) for the seam and the [capability-seam RFC](../../../docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md) for the design. @@ -8,7 +8,7 @@ This is the implementation tier of the compaction capability — see the [interf The abstract contract states only WHAT compaction does; this backend owns every HOW decision: -- **Token estimation** — `estimateContentTokens()`: char/4 with per-block structural overhead (`text`/`reasoning` = `ceil(len/4) + 4`, `tool-call` from name + arguments, `tool-result` recursive, `image` = 85, unknown blocks via JSON length). +- **Token estimation** — `estimateContentTokens()`: chars divided by the `charsPerToken` config (default 4) with per-block structural overhead (`text`/`reasoning` = `ceil(len/charsPerToken) + 4`, `tool-call` from name + arguments, `tool-result` recursive, `image` = 85, unknown blocks via JSON length). - **Retention policy** — `compactIfNeeded()` walks the surface nodes tail→head summing per-node token estimates, and retains the smallest tail-run of WHOLE units (a closed step, or a single no-step node such as a pre-step `user/message` or inter-step `steering/message`) whose total reaches `retainTokens`; everything older is compacted. Retention is **turn-agnostic** — turn boundaries play no role, so a single runaway turn that alone exceeds the window compacts its OWN early closed steps rather than being retained verbatim (the failure mode that motivated dropping turn-protection: a tool-heavy turn must stay compactable or the harness dies exactly when compaction is needed). The only structural guard is **tool-pairing balance**: the compacted region's edges are balanced cuts on the surface (no unanswered tool-call crosses either edge), so it never splits a step's `assistant/message` tool-calls from their `tool/result`s. When the only compactable content left is an un-splittable open tail step, it declines (returns `null`) and retries once an older step closes. **Single-unit overflow is out of scope, by design**: if one retained unit (a single closed step, or a large pasted `user/message`) ALONE exceeds the budget, compaction cannot help and the call may go out over-budget — bounding an individual unit's size is a separate concern. `compactRegion()` enforces tool-pairing balance strictly, throwing on a boundary that would split a step. `dsh-session` exports `isToolPairingBalanced` for the check. - **Dynamic convergence** — no static summary-length config pretends to bound what the model will write. If framing/estimator/system overhead leaves the compacted surface above threshold, `compactIfNeeded()` re-compacts the head checkpoint up to `compactionRetries` extra times; if it still cannot get below threshold, it throws. A summary whose estimated stored size is not smaller than the shadowed content fails closed before it mutates the surface. - **Summarization** — `summarize()`: a `GenerateOptions` request assembled via `BlockAssembler` with a fixed system prompt that asks for a structured checkpoint (Primary Request and Intent · Key Technical Concepts · Files and Code · Errors and Fixes · Pending Tasks · Current Work · Next Step · Critical Context), every section mandatory, exact paths/commands/identifiers preserved. The request runs through the `agent/request` waterfall before `ctx.llm.stream()`, so router agents that choose the concrete model there also route compaction summaries. `maxTokens` is the provider-side generation cap; only text blocks from the model's reply are kept before the checkpoint is stored (reasoning is dropped so private chain-of-thought never leaks into the durable summary, and a stray `tool-call` is dropped so the synthesized `user/message` summary cannot land an orphaned call with no matching `tool-result`). The compacted region is flattened to a plain-text transcript first: text and reasoning contribute their text, and every non-text block (image, tool-call, tool-result, plugin-added types) contributes a type-tagged placeholder (`[image]`, `[tool-call: name(args)]`, …) so the summarizer is told what existed rather than silently dropping it. diff --git a/packages/compact/compact-basic/src/index.ts b/packages/compact/compact-basic/src/index.ts index 26c593a47f..a1a83cd407 100644 --- a/packages/compact/compact-basic/src/index.ts +++ b/packages/compact/compact-basic/src/index.ts @@ -2,7 +2,8 @@ * `BasicCompactService`: the first implementation of the * `@deepseek-ai/dsh-compact` seam. It owns the entire compaction strategy: * - * - **Token estimation** — char/4 heuristic with per-block structural overhead. + * - **Token estimation** — chars/`charsPerToken` heuristic (config, default 4) + * with per-block structural overhead. * - **Retention policy** — walk surface nodes tail→head, keep recent nodes up * to a token budget, compact everything older. The cutoff is snapped forward * to the next balanced tool-pairing boundary so a compacted region never diff --git a/packages/fs/tool-fs/src/index.ts b/packages/fs/tool-fs/src/index.ts index 5cc87ba597..f5d0d9ef91 100644 --- a/packages/fs/tool-fs/src/index.ts +++ b/packages/fs/tool-fs/src/index.ts @@ -66,10 +66,10 @@ export const Config: z = z.object({ /** The shape after schemastery applied the defaults. */ type ResolvedConfig = Required -/** A read cap must be a positive finite number to bound output and memory. */ -function assertPositiveFinite(name: string, value: number): void { - if (!Number.isFinite(value) || value <= 0) { - throw new Error(`tool-fs: ${name} must be a positive finite number`) +/** Every read cap counts lines/chars/bytes — a positive integer, or windowing arithmetic misbehaves silently. */ +function assertPositiveInteger(name: string, value: number): void { + if (!Number.isInteger(value) || value < 1) { + throw new Error(`tool-fs: ${name} must be a positive integer`) } } @@ -77,10 +77,10 @@ function assertPositiveFinite(name: string, value: number): void { export function apply(ctx: Context, config: Config): void { // schemastery (Config) has already filled every defaulted field. const resolved = config as ResolvedConfig - assertPositiveFinite('readLimit', resolved.readLimit) - assertPositiveFinite('readMaxLineLength', resolved.readMaxLineLength) - assertPositiveFinite('readMaxBytes', resolved.readMaxBytes) - assertPositiveFinite('readStreamMinSize', resolved.readStreamMinSize) + assertPositiveInteger('readLimit', resolved.readLimit) + assertPositiveInteger('readMaxLineLength', resolved.readMaxLineLength) + assertPositiveInteger('readMaxBytes', resolved.readMaxBytes) + assertPositiveInteger('readStreamMinSize', resolved.readStreamMinSize) applyReadTool(ctx, { limit: resolved.readLimit, maxLineLength: resolved.readMaxLineLength, diff --git a/packages/fs/tool-fs/tests/tools.spec.ts b/packages/fs/tool-fs/tests/tools.spec.ts index 638ce2112b..efad0a86f7 100644 --- a/packages/fs/tool-fs/tests/tools.spec.ts +++ b/packages/fs/tool-fs/tests/tools.spec.ts @@ -547,15 +547,16 @@ describe('read caps are plugin config', () => { it.each([ ['readLimit', { readLimit: 0 }], + ['readLimit', { readLimit: 2.5 }], ['readMaxLineLength', { readMaxLineLength: -1 }], ['readMaxBytes', { readMaxBytes: Number.NaN }], ['readStreamMinSize', { readStreamMinSize: 0 }], - ] as const)('rejects a non-positive %s at load', async (name, config) => { + ] as const)('rejects a non-positive or fractional %s at load', async (name, config) => { const ctx = new Context() await ctx.plugin(SystemPrompt) await ctx.plugin(ToolRegistry) await ctx.plugin(FakeFs) - await expect(ctx.plugin(ToolFs, config)).rejects.toThrow(new RegExp(`tool-fs: ${name} must be a positive finite number`)) + await expect(ctx.plugin(ToolFs, config)).rejects.toThrow(new RegExp(`tool-fs: ${name} must be a positive integer`)) }) it('has no default export (namespace plugin export shape)', () => { diff --git a/packages/hooks/hooks-claude/src/index.ts b/packages/hooks/hooks-claude/src/index.ts index 263b201791..06a405bf09 100644 --- a/packages/hooks/hooks-claude/src/index.ts +++ b/packages/hooks/hooks-claude/src/index.ts @@ -95,7 +95,18 @@ function nextHandlerId(point: string): string { /** The `{kind:'plugin'}` source stamped on every context this bridge injects. */ const PLUGIN_SOURCE: MessageSource = { kind: 'plugin', plugin: 'hooks-claude' } +/** The summary cap bounds a persisted event field — a positive integer or the slice misbehaves silently. */ +function assertPositiveInteger(name: string, value: number): void { + if (!Number.isInteger(value) || value < 1) { + throw new Error(`hooks-claude: ${name} must be a positive integer`) + } +} + export function apply(ctx: Context, config: Config): void { + // Validate the cap BEFORE the config-file parse: a bad value must fail the + // load loudly, not be skipped by the parse-failure early return. + const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 + assertPositiveInteger('stderrSummaryMaxChars', stderrSummaryMaxChars) // --- Parse the config ONCE at load. A read/parse failure is contained: the // bridge logs and registers nothing rather than crashing boot (a typo'd path // must not take the agent down). --- @@ -116,7 +127,6 @@ export function apply(ctx: Context, config: Config): void { } const defaultTimeoutMs = config.defaultTimeoutMs ?? 600_000 - const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 /** * Run every command hook configured for `point` whose matcher selects diff --git a/packages/hooks/hooks-claude/tests/coverage.spec.ts b/packages/hooks/hooks-claude/tests/coverage.spec.ts index f5091ce7a5..45ca8f113b 100644 --- a/packages/hooks/hooks-claude/tests/coverage.spec.ts +++ b/packages/hooks/hooks-claude/tests/coverage.spec.ts @@ -142,6 +142,16 @@ describe('hooks-claude coverage — empty/no-op outcomes and no-agent paths', () expect(res?.type === 'hook/result' && res.data.stderrSummary?.length).toBe(501) // default 500-char cap + ellipsis }) + it('rejects a non-positive or fractional stderrSummaryMaxChars at load', async () => { + const d = dir() + const path = hooks(d, {}) + for (const bad of [0, -5, 1.5, Number.NaN]) { + const adapter = new MockAdapter([]) + await expect(harness(path, adapter, { stderrSummaryMaxChars: bad })) + .rejects.toThrow(/hooks-claude: stderrSummaryMaxChars must be a positive integer/) + } + }) + it('the stderr summary cap is plugin config (stderrSummaryMaxChars)', async () => { const d = dir() const s = sh(d, 'long.sh', '#!/usr/bin/env bash\nprintf "x%.0s" {1..600} >&2\nexit 2\n') diff --git a/packages/hooks/hooks-codex/src/index.ts b/packages/hooks/hooks-codex/src/index.ts index 4f7d6c9ed9..accc36714a 100644 --- a/packages/hooks/hooks-codex/src/index.ts +++ b/packages/hooks/hooks-codex/src/index.ts @@ -68,7 +68,18 @@ function nextHandlerId(point: string): string { const PLUGIN_SOURCE: MessageSource = { kind: 'plugin', plugin: 'hooks-codex' } +/** The summary cap bounds a persisted event field — a positive integer or the slice misbehaves silently. */ +function assertPositiveInteger(name: string, value: number): void { + if (!Number.isInteger(value) || value < 1) { + throw new Error(`hooks-codex: ${name} must be a positive integer`) + } +} + export function apply(ctx: Context, config: Config): void { + // Validate the cap BEFORE the config-file parse: a bad value must fail the + // load loudly, not be skipped by the parse-failure early return. + const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 + assertPositiveInteger('stderrSummaryMaxChars', stderrSummaryMaxChars) let parsed: CodexHookConfig = {} try { const raw: unknown = JSON.parse(readFileSync(config.configPath, 'utf8')) @@ -83,7 +94,6 @@ export function apply(ctx: Context, config: Config): void { } const defaultTimeoutMs = config.defaultTimeoutMs ?? 600_000 - const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500 const model = config.model ?? '' async function runPoint( diff --git a/packages/hooks/hooks-codex/tests/coverage.spec.ts b/packages/hooks/hooks-codex/tests/coverage.spec.ts index 861a2691e1..040425cbbe 100644 --- a/packages/hooks/hooks-codex/tests/coverage.spec.ts +++ b/packages/hooks/hooks-codex/tests/coverage.spec.ts @@ -207,6 +207,16 @@ describe('hooks-codex coverage — decision mapping paths', () => { expect(res?.type === 'hook/result' && res.data.stderrSummary?.length).toBe(501) // default 500-char cap + ellipsis }) + it('rejects a non-positive or fractional stderrSummaryMaxChars at load', async () => { + const d = dir() + hooks(d, {}) + for (const bad of [0, -5, 1.5, Number.NaN]) { + const adapter = new MockAdapter([]) + await expect(harness(join(d, 'hooks.json'), adapter, { stderrSummaryMaxChars: bad })) + .rejects.toThrow(/hooks-codex: stderrSummaryMaxChars must be a positive integer/) + } + }) + it('the stderr summary cap is plugin config (stderrSummaryMaxChars)', async () => { const d = dir() hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: sh(d, 'l.sh', '#!/usr/bin/env bash\nprintf "x%.0s" {1..600} >&2\nexit 2\n') }] }] }) diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index e1e510d19d..3eb12fac38 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -398,6 +398,37 @@ describe('dsh-subagent-acp', () => { await run.dispose() }) + it('plugin-config dispose graces reach the run (SIGKILL escalation through the provider)', async () => { + // Same trap scenario as the direct startAcpRun escalation test, but the + // graces arrive via the PLUGIN CONFIG through the registered provider — so a + // regression that stops threading config into AcpRunSpec (falling back to + // the 6s/3s defaults) blows past the 4000ms bound and fails loud. + const tmp = mkdtempSync(join(tmpdir(), 'acp-cfg-trap-')) + const ready = join(tmp, 'trap-armed') + try { + const ctx = new Context() + await ctx.plugin(SubagentService) + await ctx.plugin(acp, { + providerName: 'acp', + command: process.execPath, + args: ['--import', tsxLoader, mockServer], + permission: 'reject', + env: { MOCK_TRAP_SIGTERM: '1', MOCK_TEXT: 'x', MOCK_READY_FILE: ready, TSX_TSCONFIG_PATH: repoTsconfig }, + disposeEofGraceMs: 150, + disposeGraceMs: 150, + }) + const run = ctx.subagents.start('acp', { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }) + await waitForFile(ready) + await expect(Promise.race([ + run.dispose(), + new Promise((_r, reject) => { setTimeout(() => { reject(new Error('dispose did not return — config graces not threaded to the run')) }, 4000) }), + ])).resolves.toBeUndefined() + await ctx.fiber.dispose() + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + it('rejects a non-positive dispose grace at load', async () => { for (const bad of [{ disposeEofGraceMs: 0 }, { disposeGraceMs: -1 }, { disposeEofGraceMs: Number.NaN }]) { const ctx = new Context() diff --git a/packages/web/tool-web/README.md b/packages/web/tool-web/README.md index e789720cbb..f0bd66cdb9 100644 --- a/packages/web/tool-web/README.md +++ b/packages/web/tool-web/README.md @@ -8,7 +8,7 @@ Each tool is registered independently; a product that wants only one disables th | Tool | Args | Behavior | |---|---|---| -| `web_search` | `query` (string) | Discovery. Returns an optional answer plus source URLs. `max_results` is **not** model-facing — the tool sets the bound (`WEB_SEARCH_MAX_RESULTS = 8`) and passes it to the seam. | +| `web_search` | `query` (string) | Discovery. Returns an optional answer plus source URLs. `max_results` is **not** model-facing — the tool sets the bound (the `searchMaxResults` config, default 8) and passes it to the seam. | | `web_fetch` | `url` (string), `timeout_ms` (number, optional) | Retrieves a specific URL. HTML bodies are rendered to markdown-ish text; text bodies pass through. A non-2xx status is reported, not an error. | ## Config From cc15ef54ef9d1d88f3c3986c5d45390900fde1be Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 18:15:54 +0800 Subject: [PATCH 8/8] Sync remaining chars-per-token prose in overview READMEs The convergence pass found four summary-level sites still describing the estimator as fixed char/4: packages/README.md (twice), the compact group and interface READMEs, and compact-basic's package.json description. All now say chars-per-token with the charsPerToken default, matching the authoritative package README/module doc/RFC. --- packages/README.md | 4 ++-- packages/compact/README.md | 2 +- packages/compact/compact-basic/package.json | 2 +- packages/compact/compact/README.md | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/README.md b/packages/README.md index 2233123772..aec3a0a244 100644 --- a/packages/README.md +++ b/packages/README.md @@ -34,7 +34,7 @@ dsh-session ← dsh-llm, dsh-brand dsh-system-prompt ← dsh-llm dsh-agent ← dsh-llm, dsh-session, dsh-brand dsh-compact ← dsh-session, dsh-llm (abstract compaction seam; tool deferred) -dsh-compact-basic ← dsh-compact, dsh-session, dsh-llm, dsh-agent (char/4 + token-budget retention backend) +dsh-compact-basic ← dsh-compact, dsh-session, dsh-llm, dsh-agent (chars-per-token + token-budget retention backend) dsh-tools ← dsh-llm, dsh-system-prompt, dsh-agent dsh-bash-local ← dsh-bash (BashExecutor impl) dsh-tool-bash ← dsh-bash, dsh-tools (bash tool schemas) @@ -89,7 +89,7 @@ The rule: **extension** plugins depend on interfaces, never on the concrete loop | `fs-policy/` | `fs` | Policy gate plugin: observed-state + read-before-edit + version-guarded write/edit via the `fs/*` event gate | (no service — `fs/*` listeners) | | `tool-fs/` | `fs` | Model-facing `read`/`write`/`edit` tools + executor (reads via `ctx.fs`, owns read windowing, dispatches `fs/*`) | (registers on `ctx.tools`) | | `compact/` | `compact` | Abstract compaction seam + `compact/*` events + `CompactionResult` | `ctx.compact` | -| `compact-basic/` | `compact` | A backend: char/4 estimation + token-budget retention + `llm.stream()` summarization | (registers `ctx.compact`) | +| `compact-basic/` | `compact` | A backend: chars-per-token estimation (`charsPerToken`, default 4) + token-budget retention + `llm.stream()` summarization | (registers `ctx.compact`) | | `web/` | `web` | Abstract web seam (search/fetch provider registries + selection + vocabulary + `WebError`) | `ctx.web` | | `web-search-exa/` | `web` | Exa-backed `WebSearchProvider` | (registers on `ctx.web`) | | `web-search-perplexity/` | `web` | Perplexity-backed `WebSearchProvider` | (registers on `ctx.web`) | diff --git a/packages/compact/README.md b/packages/compact/README.md index 10eaf1617a..08c3ddd707 100644 --- a/packages/compact/README.md +++ b/packages/compact/README.md @@ -5,7 +5,7 @@ A three-package capability seam (see [capability seams](../../docs/rfc/implement | Package | Role | ctx key | |---|---|---| | `compact/` | Abstract compaction seam (interface + `compact/*` events + `CompactionResult`) | `ctx.compact` | -| `compact-basic/` | A backend: char/4 estimation + token-budget retention + `llm.stream()` summarization | (registers `ctx.compact`) | +| `compact-basic/` | A backend: chars-per-token estimation (`charsPerToken`, default 4) + token-budget retention + `llm.stream()` summarization | (registers `ctx.compact`) | | `tool-compact/` (deferred) | Model-facing `/compact` tool over `ctx.compact` | (registers on `ctx.tools`) | The interface lives at `compact/compact/`, the backend at `compact/compact-basic/`. Unlike the bash seam, it depends on `dsh-session` and `dsh-llm` — its verbs are defined over a `Session` and its output is the `ContentBlock` vocabulary, so the contract cannot be expressed without naming them. That deviation from the "interface depends only on cordis" guidance is intentional and recorded in the [compaction capability-seam RFC](../../docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md). A tokenizer- or template-based backend would replace `compact-basic` without touching the interface or the tool. diff --git a/packages/compact/compact-basic/package.json b/packages/compact/compact-basic/package.json index c019796e0d..e57e28a8c9 100644 --- a/packages/compact/compact-basic/package.json +++ b/packages/compact/compact-basic/package.json @@ -1,6 +1,6 @@ { "name": "@deepseek-ai/dsh-compact-basic", - "description": "Basic compaction backend (char/4 token estimation + token-budget retention + llm.generate() summarization) for the DeepSeek Harness", + "description": "Basic compaction backend (chars-per-token estimation + token-budget retention + llm.generate() summarization) for the DeepSeek Harness", "version": "0.0.1", "private": true, "type": "module", diff --git a/packages/compact/compact/README.md b/packages/compact/compact/README.md index b6f3cc0920..424ee12bcf 100644 --- a/packages/compact/compact/README.md +++ b/packages/compact/compact/README.md @@ -7,7 +7,7 @@ This package is the interface tier of the compaction capability, split so each c | Package | Role | |---|---| | `@deepseek-ai/dsh-compact` (this) | the interface: abstract service + `compact/*` events + `CompactionResult` | -| `@deepseek-ai/dsh-compact-basic` (deferred) | a backend: char/4 estimation + token-budget retention + `llm.stream()` summarization | +| `@deepseek-ai/dsh-compact-basic` (deferred) | a backend: chars-per-token estimation (`charsPerToken`, default 4) + token-budget retention + `llm.stream()` summarization | | `@deepseek-ai/dsh-tool-compact` (deferred) | the model-facing `/compact` tool over `ctx.compact` | Unlike the bash seam, this interface depends on `@deepseek-ai/dsh-session` and `@deepseek-ai/dsh-llm` — the contract's verbs are defined over a `Session` and its output is the `ContentBlock` vocabulary, so they cannot be expressed without naming those packages. That deviation from the "interface depends only on cordis" guidance is intentional and recorded in the [compaction capability-seam RFC](../../../docs/rfc/implemented/feature/2026-06-18-compaction-capability-seam.md).