fix(review): contain provider-removed listener failures; pin the model-via-request path
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.
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user