fix(review): close interpolation strictness holes; make tool-subagent mirror provider lifecycle
Codex round-1 findings, both confirmed:
- renderPrompt: variable lookup now uses Object.hasOwn (an unregistered
{{constructor}} previously resolved through Object.prototype and spliced
function source into the prompt), and a {{ that opens no complete group
while a }} still follows ({{{model}}}, {{a{b}}) now throws instead of
passing or partially interpolating. A lone {{ with no }} after it stays
verbatim; substituted values are never re-scanned.
- tool-subagent: the apply-time provider lookup assumed a load order the
cordis Loader does not guarantee (siblings start concurrently). The seam
now announces subagent/provider-added/-removed and the tool mirrors the
provider's lifecycle: registers when the provider is (or becomes)
available, unregisters when it goes away, re-derives wording on reload.
No load-order requirement remains.
- loop.spec containment test now proves live continuation: after the
contained render failure, a waterfall listener rescues {{cwd}} and the
same agent completes a real model turn.
RFC/READMEs updated to the shipped contract; cordis catalog regenerated.
This commit is contained in:
@@ -34,7 +34,7 @@ Beside `capabilities` sits one DESCRIPTIVE fact, not validated by the service: `
|
||||
|
||||
`provider.start(request)` returns a `SubagentRun`: a handle with a `result` promise, `cancel()`, `dispose()`, and the optional runtime methods. `result` resolves with a `SubagentResult` (`output`, optional `structured`, `stopReason`) — it does **not** reject on a child-level failure (a model/transport failure resolves with `stopReason: 'error'`), so the consumer maps a non-`completed` reason to an `isError` tool result. The consumer MUST `dispose()` on every path (success, error, abort) to reach child quiescence and avoid leaking an idle child / session.
|
||||
|
||||
The service emits `subagent/start` (payload `SubagentRunInfo`) and `subagent/end` (payload `SubagentRunEndInfo`) around the run — both **observe-only** (plain `emit`s; `subagent/end` fires from a detached `.then` and awaits no listener). `subagent/end` carries `lastAssistantMessage` (a deep clone of the child's final `output`) on the settle path, absent when the run rejected at the infrastructure level. The clone keeps the surface observe-only: the end emit fires from a detached `.then` before the caller's `await run.result` resumes, so a shared reference would let a mutating listener corrupt the caller's result. A `subagent/start` listener can still reach the live child via `ctx.agents.get(info.id)`; a `subagent/end` listener can only observe (the run has settled). Any run-affecting decision (continuation, injection that changes the run) is out of scope for this observe-only surface.
|
||||
The service also announces provider lifecycle: `subagent/provider-added` (the live provider) fires after a registration and `subagent/provider-removed` (the name) after an unregistration, so a consumer deriving state from a named provider (the model-facing tool wording) mirrors registry membership instead of assuming load order — the cordis Loader starts sibling plugins concurrently, so "listed earlier" does not mean "registered earlier". The service emits `subagent/start` (payload `SubagentRunInfo`) and `subagent/end` (payload `SubagentRunEndInfo`) around the run — both **observe-only** (plain `emit`s; `subagent/end` fires from a detached `.then` and awaits no listener). `subagent/end` carries `lastAssistantMessage` (a deep clone of the child's final `output`) on the settle path, absent when the run rejected at the infrastructure level. The clone keeps the surface observe-only: the end emit fires from a detached `.then` before the caller's `await run.result` resumes, so a shared reference would let a mutating listener corrupt the caller's result. A `subagent/start` listener can still reach the live child via `ctx.agents.get(info.id)`; a `subagent/end` listener can only observe (the run has settled). Any run-affecting decision (continuation, injection that changes the run) is out of scope for this observe-only surface.
|
||||
|
||||
## Scope (first cut)
|
||||
|
||||
|
||||
@@ -60,6 +60,25 @@ declare module 'cordis' {
|
||||
}
|
||||
|
||||
interface Events {
|
||||
/**
|
||||
* A provider became resolvable in the {@link SubagentService} registry.
|
||||
* Consumers that derive state from a named provider (e.g. the model-facing
|
||||
* tool wording in `dsh-tool-subagent`) react HERE instead of assuming load
|
||||
* order — the cordis Loader starts sibling plugins concurrently, so
|
||||
* "listed earlier in cordis.yml" does not mean "registered earlier".
|
||||
* @param provider - the provider that just registered, live in the registry.
|
||||
* @mode emit
|
||||
*/
|
||||
'subagent/provider-added'(provider: SubagentProvider): void
|
||||
/**
|
||||
* 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.
|
||||
* @param name - the registry name that no longer resolves.
|
||||
* @mode emit
|
||||
*/
|
||||
'subagent/provider-removed'(name: string): void
|
||||
/**
|
||||
* A subagent run started — emitted after the provider is resolved and its
|
||||
* capabilities validated, as the child run begins. Paired with
|
||||
@@ -130,7 +149,9 @@ export class SubagentService extends Service {
|
||||
/**
|
||||
* Register a provider under its `provider.name`. Throws {@link SubagentError}
|
||||
* (`DUPLICATE_PROVIDER`) if the name is already taken. Effect-scoped: disposed
|
||||
* with the calling fiber (HMR-safe).
|
||||
* with the calling fiber (HMR-safe). Emits `subagent/provider-added` after
|
||||
* the registration and `subagent/provider-removed` on unregistration, so
|
||||
* consumers can mirror provider lifecycle instead of assuming load order.
|
||||
* @param provider - the provider; its `name` is the registry key.
|
||||
* @returns the disposer that unregisters the provider.
|
||||
*/
|
||||
@@ -140,9 +161,14 @@ export class SubagentService extends Service {
|
||||
throw new SubagentError(`a subagent provider named "${provider.name}" is already registered`, 'DUPLICATE_PROVIDER')
|
||||
}
|
||||
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.
|
||||
yield () => {
|
||||
this.providers.delete(provider.name)
|
||||
this.ctx.emit('subagent/provider-removed', provider.name)
|
||||
}
|
||||
this.ctx.emit('subagent/provider-added', provider)
|
||||
}.bind(this), 'subagents.registerProvider()')
|
||||
// ctx.effect's disposer returns Promise<void>; our disposer API is
|
||||
// synchronous fire-and-forget — discard the (always-resolved) promise.
|
||||
|
||||
@@ -45,6 +45,38 @@ function baseRequest(overrides: Partial<SubagentStartRequest> = {}): SubagentSta
|
||||
}
|
||||
|
||||
describe('SubagentService', () => {
|
||||
it('announces provider lifecycle: added on register, removed on dispose', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SubagentService)
|
||||
const added: string[] = []
|
||||
const removed: string[] = []
|
||||
ctx.on('subagent/provider-added', provider => void added.push(provider.name))
|
||||
ctx.on('subagent/provider-removed', name => void removed.push(name))
|
||||
|
||||
const dispose = ctx.subagents.registerProvider(new StubProvider('alpha'))
|
||||
expect(added).toEqual(['alpha'])
|
||||
expect(removed).toEqual([])
|
||||
|
||||
dispose()
|
||||
expect(removed).toEqual(['alpha'])
|
||||
})
|
||||
|
||||
it('rolls back the registration when a provider-added listener throws', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SubagentService)
|
||||
let threw = false
|
||||
const off = ctx.on('subagent/provider-added', () => {
|
||||
if (!threw) { threw = true; throw new Error('boom added listener') }
|
||||
})
|
||||
|
||||
expect(() => ctx.subagents.registerProvider(new StubProvider('alpha'))).toThrow('boom added listener')
|
||||
expect(ctx.subagents.getProvider('alpha')).toBeUndefined() // nothing leaked
|
||||
|
||||
off()
|
||||
ctx.subagents.registerProvider(new StubProvider('alpha'))
|
||||
expect(ctx.subagents.getProvider('alpha')).toBeDefined()
|
||||
})
|
||||
|
||||
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