fix(scope): close final ownership races
Drain idle injection flushes before agent teardown, snapshot approval and subagent provider inputs, and gate subagent lifecycle events on real child readiness. Align the RFCs and generated contracts with the hardened behavior.
This commit is contained in:
@@ -2,7 +2,7 @@
|
||||
|
||||
User-approval seam. Owns the `ctx.approval` service ([`ApprovalService`](src/index.ts)) and the one-shot permission vocabulary the harness shares: `ApprovalRequest` (agent + tool identity + reason + abort signal), the closed `ApprovalOutcome` union (`allowed-once` / `rejected` / `cancelled` / `unavailable`), the `ApprovalRequestId` brand pairing the two log-only audit events (`approval/asked` / `approval/decided`), and the `approval/request` waterfall the answerers listen on. It lives in the UI group because its purpose is human permission, while remaining channel-neutral: it depends only on Cordis and core vocabulary packages, never on a concrete UI.
|
||||
|
||||
The contract in one line: `ctx.approval.request(req)` puts exactly one question — "may this specific action proceed?" — to whatever answerers the deployment composed, and always resolves to an outcome, never rejects: an aborted signal yields `cancelled`, a throwing or missing answerer yields `unavailable`, and `allowed-once` is a grant for the single asked-about action, never a class of future ones. The one precondition: ask from inside an open turn — the audit pair is turn-enclosed by contract (the turn is the durable log's commit/replay boundary; a bare event between turns is crash-tail garbage on reload), so an idle ask throws before appending anything.
|
||||
The contract in one line: `ctx.approval.request(req)` puts exactly one question — "may this specific action proceed?" — to whatever answerers the deployment composed, and always resolves to an outcome, never rejects: an aborted signal yields `cancelled`, a throwing or missing answerer yields `unavailable`, and `allowed-once` is a grant for the single asked-about action, never a class of future ones. Acceptance is synchronous: the service shallow-freezes a detached request record before dispatch, preserving the exact `agent` and `AbortSignal` identities while making later caller mutation unable to redirect scope, payload, cancellation, or either audit event. Session observers run after an event enters the append-only log; if one throws, the service recognizes that the audit is already authoritative, contains the observer failure, and completes the pair. The one precondition: ask from inside an open turn — the audit pair is turn-enclosed by contract (the turn is the durable log's commit/replay boundary; a bare event between turns is crash-tail garbage on reload), so an idle ask throws before appending anything.
|
||||
|
||||
The service is the mechanism, answerers are the policy. Answerers are `approval/request` waterfall listeners occupying a single decision slot: answer for an agent you own by returning an outcome without calling `next()`, or delegate an agent you don't recognize by calling `next()` — the chain's built-in default is `unavailable`, so a deployment with no answerer (headless, CI) fails closed with zero configuration. Dispatch is keyed by `req.agent`: a listener registered through `agent.ctx` receives only that agent's questions, while a plain-context listener receives every agent's. Registration order across sibling plugins is not load-order deterministic; compose one terminal answerer per deployment and use `prepend` listeners only for decide-or-delegate gates.
|
||||
|
||||
|
||||
@@ -63,7 +63,10 @@ declare module 'cordis' {
|
||||
* Scope-filtered dispatch (`@deepseek-ai/dsh-scope`) keys the carrier by `req.agent`: a
|
||||
* listener registered through `agent.ctx` receives only that agent's
|
||||
* questions, while a plain-context listener receives every agent's.
|
||||
* @param req - the pending decision (agent, tool identity, reason, signal).
|
||||
* `req` is the service's shallow-frozen acceptance snapshot: later caller
|
||||
* mutation cannot redirect the question, while the `agent` and `signal`
|
||||
* identity capabilities remain exact.
|
||||
* @param req - the accepted decision (agent, tool identity, reason, signal).
|
||||
* @mode waterfall
|
||||
*/
|
||||
'approval/request'(this: Scoped<ApprovalService>, req: ApprovalRequest, next: () => Promise<ApprovalOutcome>): Promise<ApprovalOutcome>
|
||||
@@ -234,7 +237,9 @@ export function setApprovalPolicy(session: Session, policy: ApprovalPolicy): voi
|
||||
* for an answerer to present it and for the audit events to reconstruct what
|
||||
* was asked — it deliberately does NOT carry tool arguments: a UI answerer
|
||||
* attaches the prompt to the already-streamed tool call via `callId` instead
|
||||
* of re-rendering the call.
|
||||
* of re-rendering the call. `request()` synchronously copies and shallow-freezes
|
||||
* this record before crossing an asynchronous boundary. Scalar fields are
|
||||
* detached; the `agent` and `signal` identity capabilities are preserved.
|
||||
*/
|
||||
export interface ApprovalRequest {
|
||||
/**
|
||||
@@ -364,14 +369,36 @@ export class ApprovalService extends Service {
|
||||
* Within that precondition it always resolves to an outcome, never rejects:
|
||||
* an aborted signal yields `'cancelled'`, a missing or throwing answerer
|
||||
* yields `'unavailable'` (fail closed), and a rogue non-vocabulary return
|
||||
* value is normalized to `'unavailable'`. Appends the
|
||||
* value is normalized to `'unavailable'`. The caller-owned request is
|
||||
* synchronously snapshotted, so later mutation cannot split routing,
|
||||
* dispatch payload, cancellation, or the audit pair across agents/sessions.
|
||||
* Appends the
|
||||
* `approval/asked`/`approval/decided` audit pair (log-only) around the
|
||||
* decision regardless of outcome.
|
||||
* decision regardless of outcome. A synchronous session observer failure
|
||||
* after an audit event entered the append-only log is contained; the event
|
||||
* is already authoritative, so the pair still completes and the request
|
||||
* still resolves.
|
||||
* @param req - the pending decision (agent, tool identity, reason, signal).
|
||||
* @returns the closed outcome; `'allowed-once'` is the only grant.
|
||||
*/
|
||||
async request(req: ApprovalRequest): Promise<ApprovalOutcome> {
|
||||
if (!hasOpenTurn(req.agent.session.events)) {
|
||||
// Accept one immutable request shape before the first async boundary. The
|
||||
// caller retains its record and may mutate it as soon as this async method
|
||||
// returns; identity capabilities stay live, but the record is never reread.
|
||||
const agent = req.agent
|
||||
const toolName = req.toolName
|
||||
const callId = req.callId
|
||||
const reason = req.reason
|
||||
const signal = req.signal
|
||||
const accepted: Readonly<ApprovalRequest> = Object.freeze({
|
||||
agent,
|
||||
toolName,
|
||||
...callId !== undefined ? { callId } : {},
|
||||
...reason !== undefined ? { reason } : {},
|
||||
...signal !== undefined ? { signal } : {},
|
||||
})
|
||||
const session = accepted.agent.session
|
||||
if (!hasOpenTurn(session.events)) {
|
||||
throw new Error(
|
||||
'approval.request() outside an open turn: the approval/asked + approval/decided audit pair '
|
||||
+ 'must be turn-enclosed (a bare event between turns is crash-tail garbage on reload). '
|
||||
@@ -379,17 +406,47 @@ export class ApprovalService extends Service {
|
||||
)
|
||||
}
|
||||
const id = ApprovalRequestId(randomUUID())
|
||||
req.agent.session.append('approval/asked', {
|
||||
id,
|
||||
toolName: req.toolName,
|
||||
...req.callId !== undefined ? { callId: req.callId } : {},
|
||||
...req.reason !== undefined ? { reason: req.reason } : {},
|
||||
this.appendAudit(session, 'approval/asked', id, () => {
|
||||
session.append('approval/asked', {
|
||||
id,
|
||||
toolName: accepted.toolName,
|
||||
...accepted.callId !== undefined ? { callId: accepted.callId } : {},
|
||||
...accepted.reason !== undefined ? { reason: accepted.reason } : {},
|
||||
})
|
||||
})
|
||||
const outcome = await this.decide(accepted)
|
||||
this.appendAudit(session, 'approval/decided', id, () => {
|
||||
session.append('approval/decided', { id, outcome })
|
||||
})
|
||||
const outcome = await this.decide(req)
|
||||
req.agent.session.append('approval/decided', { id, outcome })
|
||||
return outcome
|
||||
}
|
||||
|
||||
/**
|
||||
* Append one audit event while distinguishing a post-append observer throw
|
||||
* from a failure that prevented the event entering the log. `Session.append`
|
||||
* pushes first and then notifies synchronously, so log growth proves the
|
||||
* event is already authoritative; that observer failure is reported and
|
||||
* contained so it cannot reject the approval or suppress its matching event.
|
||||
* @param session - the captured session receiving both audit events.
|
||||
* @param type - the audit event currently being appended.
|
||||
* @param id - the request id, used to identify the contained failure.
|
||||
* @param append - the single concrete `Session.append` call.
|
||||
*/
|
||||
private appendAudit(
|
||||
session: Session,
|
||||
type: 'approval/asked' | 'approval/decided',
|
||||
id: ApprovalRequestId,
|
||||
append: () => void,
|
||||
): void {
|
||||
const length = session.events.length
|
||||
try {
|
||||
append()
|
||||
} catch (error) {
|
||||
if (session.events.length === length) throw error
|
||||
this.ctx.logger.warn(`approval request "${id}": ${type} observer threw after the event was appended`)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The session's effective policy: its own `approval/policy` fold, else the
|
||||
* configured default (the schema already defaulted an omitted policy to
|
||||
@@ -401,8 +458,8 @@ export class ApprovalService extends Service {
|
||||
return effectiveApprovalPolicy(agent.session.events) ?? this.config.policy ?? 'ask'
|
||||
}
|
||||
|
||||
/** Dispatch the waterfall, contained and raced against `req.signal`. */
|
||||
private async decide(req: ApprovalRequest): Promise<ApprovalOutcome> {
|
||||
/** Dispatch the waterfall, contained and raced against the accepted signal. */
|
||||
private async decide(req: Readonly<ApprovalRequest>): Promise<ApprovalOutcome> {
|
||||
if (req.signal?.aborted) return 'cancelled'
|
||||
// The 'never' policy is decided HERE, before any dispatch: a listener
|
||||
// registered with `prepend: true` after this service mounts would sit
|
||||
|
||||
@@ -3,7 +3,7 @@ import { Context } from 'cordis'
|
||||
import type { Agent } from '@deepseek-ai/dsh-agent'
|
||||
import { CallId } from '@deepseek-ai/dsh-llm'
|
||||
import { carrierKeyOf, scopeHost } from '@deepseek-ai/dsh-scope'
|
||||
import { Session, SessionId } from '@deepseek-ai/dsh-session'
|
||||
import SessionStore, { Session, SessionId } from '@deepseek-ai/dsh-session'
|
||||
import type { SessionEvent } from '@deepseek-ai/dsh-session'
|
||||
import SystemPrompt from '@deepseek-ai/dsh-system-prompt'
|
||||
import ApprovalService, { ApprovalOutcome, ApprovalRequest, effectiveApprovalPolicy, setApprovalPolicy } from '@deepseek-ai/dsh-user-approval'
|
||||
@@ -78,6 +78,138 @@ describe('ApprovalService.request', () => {
|
||||
expect(Object.keys(appended[0]?.data ?? {}).sort()).toEqual(['id', 'toolName'])
|
||||
})
|
||||
|
||||
it('snapshots request identity, scope, payload, and audit before deferred dispatch', async () => {
|
||||
const ctx = await mounted()
|
||||
const { agent: acceptedAgent, appended: acceptedAudit } = fakeAgent()
|
||||
const { agent: replacementAgent, appended: replacementAudit } = fakeAgent()
|
||||
const host = await scopeHost(ctx, ['approval'])
|
||||
const acceptedScope = host.mint(acceptedAgent)
|
||||
const replacementScope = host.mint(replacementAgent)
|
||||
const dispatchStarted = Promise.withResolvers<'started'>()
|
||||
const answer = Promise.withResolvers<ApprovalOutcome>()
|
||||
const originalSignal = new AbortController().signal
|
||||
const replacementSignal = new AbortController().signal
|
||||
let heardBy: 'accepted' | 'replacement' | undefined
|
||||
let received: ApprovalRequest | undefined
|
||||
let carrier: unknown
|
||||
acceptedScope.ctx.on('approval/request', function (req) {
|
||||
heardBy = 'accepted'
|
||||
received = req
|
||||
carrier = carrierKeyOf(this)
|
||||
dispatchStarted.resolve('started')
|
||||
return answer.promise
|
||||
})
|
||||
replacementScope.ctx.on('approval/request', function (req) {
|
||||
heardBy = 'replacement'
|
||||
received = req
|
||||
carrier = carrierKeyOf(this)
|
||||
dispatchStarted.resolve('started')
|
||||
return answer.promise
|
||||
})
|
||||
const request = requestOf(acceptedAgent, {
|
||||
toolName: 'original-tool',
|
||||
callId: CallId('original-call'),
|
||||
reason: 'original reason',
|
||||
signal: originalSignal,
|
||||
})
|
||||
|
||||
const pending = ctx.approval.request(request)
|
||||
// request() has returned, but the answerer dispatch is deliberately queued
|
||||
// in a microtask. Mutating the caller-owned record must not redirect it.
|
||||
request.agent = replacementAgent
|
||||
request.toolName = 'mutated-before-dispatch'
|
||||
request.callId = CallId('mutated-call')
|
||||
request.reason = 'mutated reason'
|
||||
request.signal = replacementSignal
|
||||
await dispatchStarted.promise
|
||||
// Mutation while the answer is pending must not redirect the final audit.
|
||||
request.toolName = 'mutated-after-dispatch'
|
||||
request.reason = 'mutated again'
|
||||
answer.resolve('allowed-once')
|
||||
|
||||
await expect(pending).resolves.toBe('allowed-once')
|
||||
expect(heardBy).toBe('accepted')
|
||||
expect(carrier).toBe(acceptedAgent)
|
||||
expect(received).not.toBe(request)
|
||||
expect(Object.isFrozen(received)).toBe(true)
|
||||
expect(received).toMatchObject({
|
||||
agent: acceptedAgent,
|
||||
toolName: 'original-tool',
|
||||
callId: 'original-call',
|
||||
reason: 'original reason',
|
||||
signal: originalSignal,
|
||||
})
|
||||
expect(acceptedAudit).toHaveLength(2)
|
||||
expect(acceptedAudit[0]?.data).toMatchObject({
|
||||
toolName: 'original-tool',
|
||||
callId: 'original-call',
|
||||
reason: 'original reason',
|
||||
})
|
||||
expect(acceptedAudit[1]?.data).toMatchObject({ outcome: 'allowed-once' })
|
||||
expect(acceptedAudit[1]?.data['id']).toBe(acceptedAudit[0]?.data['id'])
|
||||
expect(replacementAudit).toEqual([])
|
||||
await host.dispose()
|
||||
})
|
||||
|
||||
it('contains an approval/asked observer throw after append and still completes the pair', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SessionStore)
|
||||
await ctx.plugin(ApprovalService)
|
||||
const session = ctx.sessions.create(SessionId('asked-observer-throw'))
|
||||
session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } })
|
||||
const agent = { session } as unknown as Agent
|
||||
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {})
|
||||
ctx.on('session/event', (_session, event) => {
|
||||
if (event.type === 'approval/asked') throw new Error('observer failed after asked append')
|
||||
})
|
||||
ctx.on('approval/request', () => Promise.resolve<ApprovalOutcome>('allowed-once'))
|
||||
|
||||
await expect(ctx.approval.request(requestOf(agent))).resolves.toBe('allowed-once')
|
||||
|
||||
const audit = session.events.filter(event => event.type.startsWith('approval/'))
|
||||
const asked = session.events.find((event): event is SessionEvent<'approval/asked'> => event.type === 'approval/asked')
|
||||
const decided = session.events.find((event): event is SessionEvent<'approval/decided'> => event.type === 'approval/decided')
|
||||
expect(audit.map(event => event.type)).toEqual(['approval/asked', 'approval/decided'])
|
||||
expect(decided?.data.id).toBe(asked?.data.id)
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining('approval/asked observer threw'))
|
||||
})
|
||||
|
||||
it('contains an approval/decided observer throw after append and still resolves', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SessionStore)
|
||||
await ctx.plugin(ApprovalService)
|
||||
const session = ctx.sessions.create(SessionId('decided-observer-throw'))
|
||||
session.append('turn/start', { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } })
|
||||
const agent = { session } as unknown as Agent
|
||||
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => {})
|
||||
ctx.on('session/event', (_session, event) => {
|
||||
if (event.type === 'approval/decided') throw new Error('observer failed after decided append')
|
||||
})
|
||||
ctx.on('approval/request', () => Promise.resolve<ApprovalOutcome>('rejected'))
|
||||
|
||||
await expect(ctx.approval.request(requestOf(agent))).resolves.toBe('rejected')
|
||||
|
||||
const audit = session.events.filter(event => event.type.startsWith('approval/'))
|
||||
const asked = session.events.find((event): event is SessionEvent<'approval/asked'> => event.type === 'approval/asked')
|
||||
const decided = session.events.find((event): event is SessionEvent<'approval/decided'> => event.type === 'approval/decided')
|
||||
expect(audit.map(event => event.type)).toEqual(['approval/asked', 'approval/decided'])
|
||||
expect(decided?.data).toMatchObject({ id: asked?.data.id, outcome: 'rejected' })
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining('approval/decided observer threw'))
|
||||
})
|
||||
|
||||
it('does not misclassify a pre-append failure as an observer failure', async () => {
|
||||
const ctx = await mounted()
|
||||
const failure = new Error('append failed before log growth')
|
||||
const agent = {
|
||||
session: {
|
||||
events: [{ type: 'turn/start' }],
|
||||
append: () => { throw failure },
|
||||
},
|
||||
} as unknown as Agent
|
||||
|
||||
await expect(ctx.approval.request(requestOf(agent))).rejects.toBe(failure)
|
||||
})
|
||||
|
||||
it('returns the first answering listener outcome (single decision slot)', async () => {
|
||||
const ctx = await mounted()
|
||||
const { agent } = fakeAgent()
|
||||
|
||||
Reference in New Issue
Block a user