From 3633cf90c80ab44e984b76580aa4e889dd93a868 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 6 Jul 2026 00:34:47 +0800 Subject: [PATCH] fix(review): contain provider-removed listener failures; pin the model-via-request path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ds-review-bot round 2, both warnings: - subagent/provider-removed now routes through emitLifecycle (per-listener containment, the subagent/start|end precedent) instead of raw ctx.emit, whose dispatch halts on the first throw: a throwing subscriber can no longer starve a later mirror into keeping a stale tool, nor disrupt the backend fiber's teardown mid-disposer. provider-added deliberately keeps propagation (register-time rollback semantics, like the system-prompt registries); the asymmetry is documented on emitLifecycle, the event JSDoc, and the provider-lifecycle RFC. - The documented model-via-agent/request fallback composes with a {{model}} persona via the ownership rule itself: the plugin supplying the model late states it early on the system-prompt/assemble waterfall. Declined re-ordering render after agent/request — it would break the agent/pre-step contract (compaction must measure the prompt the model sees). New loop test pins the supply path end-to-end; the RFC's {{model}} consequence bullet now covers supply as well as switch. --- docs/cordis-catalog/events.md | 8 +++--- docs/cordis-catalog/services.md | 2 +- docs/event-producer-consumer.md | 6 ++--- ...t-variables-and-tool-guidance-ownership.md | 2 +- ...7-05-subagent-provider-lifecycle-events.md | 3 ++- packages/core/agent-loop/tests/loop.spec.ts | 26 ++++++++++++++++++ packages/subagent/subagent/src/index.ts | 27 +++++++++++++++---- .../subagent/subagent/tests/service.spec.ts | 21 +++++++++++++++ 8 files changed, 80 insertions(+), 15 deletions(-) diff --git a/docs/cordis-catalog/events.md b/docs/cordis-catalog/events.md index 8db44c5f1b..d10a7eeabd 100644 --- a/docs/cordis-catalog/events.md +++ b/docs/cordis-catalog/events.md @@ -243,7 +243,7 @@ A subagent run settled — emitted when SubagentRun.result resolves (any stop re 'subagent/end'(info: SubagentRunEndInfo): void ``` -Source: [`packages/subagent/subagent/src/index.ts:96`](../../packages/subagent/subagent/src/index.ts) +Source: [`packages/subagent/subagent/src/index.ts:98`](../../packages/subagent/subagent/src/index.ts) ### `subagent/provider-added` — emit @@ -257,13 +257,13 @@ Source: [`packages/subagent/subagent/src/index.ts:72`](../../packages/subagent/s ### `subagent/provider-removed` — emit -A provider left the registry (its plugin's fiber was disposed — an unload or an HMR reload). Consumers holding provider-derived state drop it here; a reload re-fires `subagent/provider-added` with the fresh provider. +A provider left the registry (its plugin's fiber was disposed — an unload or an HMR reload). Consumers holding provider-derived state drop it here; a reload re-fires `subagent/provider-added` with the fresh provider. Delivered with per-listener containment: a throwing subscriber is logged, never starves later subscribers, and never disrupts the provider's teardown. ```ts cordis-catalog 'subagent/provider-removed'(name: string): void ``` -Source: [`packages/subagent/subagent/src/index.ts:81`](../../packages/subagent/subagent/src/index.ts) +Source: [`packages/subagent/subagent/src/index.ts:83`](../../packages/subagent/subagent/src/index.ts) ### `subagent/start` — emit @@ -273,7 +273,7 @@ A subagent run started — emitted after the provider is resolved and its capabi 'subagent/start'(info: SubagentRunInfo): void ``` -Source: [`packages/subagent/subagent/src/index.ts:89`](../../packages/subagent/subagent/src/index.ts) +Source: [`packages/subagent/subagent/src/index.ts:91`](../../packages/subagent/subagent/src/index.ts) ## `system-prompt/*` diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index ca2ee64334..c91f1578b2 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -176,7 +176,7 @@ list(): string[] start(name: string, request: SubagentStartRequest): SubagentRun ``` -Source: [`packages/subagent/subagent/src/index.ts:142`](../../packages/subagent/subagent/src/index.ts) +Source: [`packages/subagent/subagent/src/index.ts:144`](../../packages/subagent/subagent/src/index.ts) ## `ctx.systemPrompt` — `SystemPrompt` diff --git a/docs/event-producer-consumer.md b/docs/event-producer-consumer.md index f6cd631296..18c2b9faf0 100644 --- a/docs/event-producer-consumer.md +++ b/docs/event-producer-consumer.md @@ -25,10 +25,10 @@ This matrix shows which packages dispatch each harness-owned event and which pac | `session/created` | `emit` | [`packages/core/session/src/index.ts:36`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`emit`) | [`invariants`](../packages/support/invariants), [`session-persistence`](../packages/session-persistence/session-persistence) | | `session/event` | `emit` | [`packages/core/session/src/index.ts:44`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`emit`) | [`acp`](../packages/ui/acp), [`invariants`](../packages/support/invariants), [`session-persistence`](../packages/session-persistence/session-persistence), [`stdio-agent`](../packages/ui/stdio-agent) | | `session/flush` | `parallel` | [`packages/core/session/src/index.ts:54`](../packages/core/session/src/index.ts) | [`agent-loop`](../packages/core/agent-loop) (`parallel`) | [`session-persistence`](../packages/session-persistence/session-persistence) | -| `subagent/end` | `emit` | [`packages/subagent/subagent/src/index.ts:96`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`events.dispatch`) | [`hooks-claude`](../packages/hooks/hooks-claude) | +| `subagent/end` | `emit` | [`packages/subagent/subagent/src/index.ts:98`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`events.dispatch`) | [`hooks-claude`](../packages/hooks/hooks-claude) | | `subagent/provider-added` | `emit` | [`packages/subagent/subagent/src/index.ts:72`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`emit`) | [`tool-subagent`](../packages/subagent/tool-subagent) | -| `subagent/provider-removed` | `emit` | [`packages/subagent/subagent/src/index.ts:81`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`emit`) | [`tool-subagent`](../packages/subagent/tool-subagent) | -| `subagent/start` | `emit` | [`packages/subagent/subagent/src/index.ts:89`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`events.dispatch`) | [`hooks-claude`](../packages/hooks/hooks-claude) | +| `subagent/provider-removed` | `emit` | [`packages/subagent/subagent/src/index.ts:83`](../packages/subagent/subagent/src/index.ts) | - | [`tool-subagent`](../packages/subagent/tool-subagent) | +| `subagent/start` | `emit` | [`packages/subagent/subagent/src/index.ts:91`](../packages/subagent/subagent/src/index.ts) | [`subagent`](../packages/subagent/subagent) (`events.dispatch`) | [`hooks-claude`](../packages/hooks/hooks-claude) | | `system-prompt/assemble` | `waterfall` | [`packages/core/system-prompt/src/index.ts:38`](../packages/core/system-prompt/src/index.ts) | [`system-prompt`](../packages/core/system-prompt) (`waterfall`) | - | | `system-prompt/change` | `emit` | [`packages/core/system-prompt/src/index.ts:44`](../packages/core/system-prompt/src/index.ts) | [`system-prompt`](../packages/core/system-prompt) (`emit`) | - | | `tools/change` | `emit` | [`packages/core/tools/src/index.ts:87`](../packages/core/tools/src/index.ts) | [`tools`](../packages/core/tools) (`emit`) | - | diff --git a/docs/rfc/implemented/architecture/2026-07-05-prompt-variables-and-tool-guidance-ownership.md b/docs/rfc/implemented/architecture/2026-07-05-prompt-variables-and-tool-guidance-ownership.md index f7ba9b4480..24a525307e 100644 --- a/docs/rfc/implemented/architecture/2026-07-05-prompt-variables-and-tool-guidance-ownership.md +++ b/docs/rfc/implemented/architecture/2026-07-05-prompt-variables-and-tool-guidance-ownership.md @@ -64,7 +64,7 @@ Per-tool semantics and when-to-use live in tool DESCRIPTIONS, which already ship ## Consequences - Every fact in the assembled prompt now has exactly one owner, and the hand-maintained tool prose in leaf YAML is gone: loading or dropping a tool plugin no longer means editing any deployment's persona. -- `{{model}}` reflects `AgentOptions.model` at assembly time. A plugin that switches models in the `agent/request` waterfall makes the prompt's claim stale for that step; such a plugin can rewrite `options.system` in the same waterfall if it cares. Accepted. +- `{{model}}` reflects `AgentOptions.model` at assembly time. A plugin that switches models in the `agent/request` waterfall makes the prompt's claim stale for that step, and one that SUPPLIES the model there (options.model unset — the loop's documented fallback) leaves the variable valueless at render, failing a `{{model}}` persona before the waterfall runs. Both have the same remedy, and it is the ownership rule itself: the plugin that owns the late-bound model fact states it early on the `system-prompt/assemble` waterfall (`assembly.variables['model'] = …`) — one owner, both statements; a loop test pins the supply path end-to-end. Accepted. - While a bound provider is absent (not yet activated, unloaded, mid-HMR-reload), the subagent tool does not exist and a model request in that window simply lacks it. That is the honest state — the alternative was a registered tool whose description or execution could not be trusted. - Strictness means a persona can fail a turn at render (e.g. `{{cwd}}` on a cwd-less session). The failure is contained — the turn ends `error`, the loop survives — and it is an authoring error we WANT loud. - No escape syntax for a literal `{{name}}` in prompt prose yet; add one if a real prompt ever needs it. diff --git a/docs/rfc/implemented/architecture/2026-07-05-subagent-provider-lifecycle-events.md b/docs/rfc/implemented/architecture/2026-07-05-subagent-provider-lifecycle-events.md index 2be670ddcd..46e7ea4e71 100644 --- a/docs/rfc/implemented/architecture/2026-07-05-subagent-provider-lifecycle-events.md +++ b/docs/rfc/implemented/architecture/2026-07-05-subagent-provider-lifecycle-events.md @@ -28,6 +28,7 @@ The events also complete the seam's vocabulary: `ctx.subagents` is a named regis ## Consequences -- Consumers deriving state from a named provider react to `subagent/provider-added`/`-removed` instead of reading the registry at `apply` time; `dsh-tool-subagent` is the reference implementation. The [events catalog](../../../cordis-catalog/events.md) carries the exact signatures, and the [producer/consumer map](../../../event-producer-consumer.md) shows `dsh-subagent` emitting and `dsh-tool-subagent` consuming both events. +- Consumers deriving state from a named provider react to `subagent/provider-added`/`-removed` instead of reading the registry at `apply` time; `dsh-tool-subagent` is the reference implementation. +- **The two emits carry asymmetric failure semantics, deliberately.** `provider-removed` fires inside the registration's disposer and is delivered with PER-LISTENER containment (the service's `emitLifecycle`, not raw `ctx.emit`, which halts dispatch on the first throw): a throwing subscriber is logged, never starves a later mirror into holding a stale tool, and never disrupts the backend fiber's teardown — dispose reaches quiescence. `provider-added` propagates: it fires at registration time, where a throwing listener unwinds the yielded rollback — the same fail-loud register-time semantics as the system-prompt registries. The run-time backstop bounds what a stale mirror could cost anyway: `start()` re-resolves the provider by name per run, so a tool that outlived its provider fails that call cleanly instead of dispatching into a dead backend. The [events catalog](../../../cordis-catalog/events.md) carries the exact signatures, and the [producer/consumer map](../../../event-producer-consumer.md) shows `dsh-subagent` emitting and `dsh-tool-subagent` consuming both events. - **A window where the tool is absent.** Between backend disposal and re-registration (an HMR reload), the model sees no subagent tool. This is the honest state — the alternative is a tool that dispatches into nothing — and the tool registry's `tools/change` emit keeps prompt assembly current. - **Two waiting fibers sharing a `toolName` is an invalid config caught late.** If two loads of `dsh-tool-subagent` name different providers but the same `toolName`, both wait, and whichever provider arrives first registers; the second registration throws only when ITS provider arrives. `TODO(subagent-dup-toolname)` in the plugin records this blast radius; the tool registry's duplicate-name rejection remains the backstop. diff --git a/packages/core/agent-loop/tests/loop.spec.ts b/packages/core/agent-loop/tests/loop.spec.ts index 7de441a928..16a9b5e394 100644 --- a/packages/core/agent-loop/tests/loop.spec.ts +++ b/packages/core/agent-loop/tests/loop.spec.ts @@ -218,6 +218,32 @@ describe('agent loop', () => { expect(turnEnds[1]?.type === 'turn/end' && turnEnds[1].data.reason.kind).toBe('completed') }) + it('supports the model-via-agent/request path with a {{model}} persona: the supplier states it via the assemble waterfall', async () => { + // AgentOptions.model unset: the model arrives in the agent/request + // waterfall (the loop's documented fallback — see runStep's no-model + // error). {{model}} renders BEFORE that waterfall, so the SAME plugin + // states the fact early on system-prompt/assemble — the owner of a + // late-bound fact owns stating it wherever it is claimed. + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(adapter, 'You run on {{model}}.') + ctx.on('system-prompt/assemble', async (assembly, _context, next) => { + assembly.variables['model'] = 'mock' + return next() + }) + ctx.on('agent/request', async (_agent, _turn, _step, options, next) => { + options.model = 'mock' + return next() + }) + const agent = ctx.agentLoop.create(AgentId('a-late-model'), {}) + + send(agent, 'hi') + await waitForIdle(ctx, agent) + + expect(adapter.requests).toHaveLength(1) + expect(adapter.requests[0]!.model).toBe('mock') + expect(adapter.requests[0]!.system).toBe('You are an AI agent powered by the DeepSeek Harness SDK.\n\nYou run on mock.') + }) + it('omits the system field when a system-prompt/assemble veto empties the assembly', async () => { // The documented escape valve: a deployment that must drop the harness // openers short-circuits the assemble waterfall; the request then carries diff --git a/packages/subagent/subagent/src/index.ts b/packages/subagent/subagent/src/index.ts index a9541eabce..91230b7ff4 100644 --- a/packages/subagent/subagent/src/index.ts +++ b/packages/subagent/subagent/src/index.ts @@ -74,7 +74,9 @@ declare module 'cordis' { * A provider left the registry (its plugin's fiber was disposed — an * unload or an HMR reload). Consumers holding provider-derived state drop * it here; a reload re-fires `subagent/provider-added` with the fresh - * provider. + * provider. Delivered with per-listener containment: a throwing + * subscriber is logged, never starves later subscribers, and never + * disrupts the provider's teardown. * @param name - the registry name that no longer resolves. * @mode emit */ @@ -163,10 +165,13 @@ export class SubagentService extends Service { this.providers.set(provider.name, provider) // Yield the rollback BEFORE emitting `subagent/provider-added`: a // throwing added-listener then unregisters the provider (and announces - // the removal) instead of leaking it into the registry. + // the removal) instead of leaking it into the registry. The removal + // announcement itself is contained PER LISTENER ({@link emitLifecycle}): + // it runs inside this disposer, where a propagating subscriber would + // disrupt the backend fiber's teardown and starve later mirrors. yield () => { this.providers.delete(provider.name) - this.ctx.emit('subagent/provider-removed', provider.name) + this.emitLifecycle('subagent/provider-removed', provider.name) } this.ctx.emit('subagent/provider-added', provider) }.bind(this), 'subagents.registerProvider()') @@ -265,10 +270,22 @@ export class SubagentService extends Service { * on the first throw — so this resolves the listener callbacks via * `ctx.events.dispatch` and contains each call, the same guarantee * `BashExecutor.notifyTaskDone` gives its own listener set. + * + * `subagent/provider-removed` routes through here too: it fires inside the + * provider registration's DISPOSER, where a propagating listener would + * disrupt the backend fiber's teardown (dispose must reach quiescence) and a + * starved later listener would leave a mirror consumer (`dsh-tool-subagent`) + * holding a tool for a provider that no longer exists. `subagent/provider-added` + * deliberately does NOT: it fires at registration time, where a throwing + * listener unwinds the yielded rollback — the same fail-loud register-time + * semantics as the system-prompt registries. */ + private emitLifecycle(name: 'subagent/start', info: SubagentRunInfo): void + private emitLifecycle(name: 'subagent/end', info: SubagentRunEndInfo): void + private emitLifecycle(name: 'subagent/provider-removed', info: string): void private emitLifecycle( - name: 'subagent/start' | 'subagent/end', - info: SubagentRunInfo | SubagentRunEndInfo, + name: 'subagent/start' | 'subagent/end' | 'subagent/provider-removed', + info: SubagentRunInfo | SubagentRunEndInfo | string, ): void { for (const callback of this.ctx.events.dispatch('emit', [name, info])) { try { diff --git a/packages/subagent/subagent/tests/service.spec.ts b/packages/subagent/subagent/tests/service.spec.ts index 6b5e737a3d..e49b4c12e0 100644 --- a/packages/subagent/subagent/tests/service.spec.ts +++ b/packages/subagent/subagent/tests/service.spec.ts @@ -77,6 +77,27 @@ describe('SubagentService', () => { expect(ctx.subagents.getProvider('alpha')).toBeDefined() }) + it('contains a throwing provider-removed listener: later mirrors still hear it, teardown completes', async () => { + // provider-removed fires inside the registration's DISPOSER, so a + // propagating listener would disrupt the backend's teardown; and cordis + // emit halts on the first throw, so an uncontained one would starve every + // mirror registered after it (a stale model-facing tool). Both are + // prevented by per-listener containment. + const ctx = new Context() + await ctx.plugin(SubagentService) + const warnings: string[] = [] + ctx.logger.warn = ((message: unknown) => void warnings.push(String(message))) as typeof ctx.logger.warn + ctx.on('subagent/provider-removed', () => { throw new Error('boom removed listener') }) + const heard: string[] = [] + ctx.on('subagent/provider-removed', name => void heard.push(name)) + + const dispose = ctx.subagents.registerProvider(new StubProvider('alpha')) + expect(() => { dispose() }).not.toThrow() + expect(heard).toEqual(['alpha']) // the listener AFTER the thrower still ran + expect(ctx.subagents.getProvider('alpha')).toBeUndefined() // teardown reached quiescence + expect(warnings.some(w => w.includes('boom removed listener'))).toBe(true) + }) + it('registers a provider and starts a run on it by name', async () => { const ctx = new Context() await ctx.plugin(SubagentService)