fix(commands): harden registry and UI ordering

This commit is contained in:
Tianyi Cui
2026-07-20 11:24:02 +08:00
parent 38abe7cac6
commit 2dd3b1dfba
67 changed files with 284 additions and 101 deletions

View File

@@ -41,9 +41,9 @@ One id-keyed record map plus exact agent-object checks route every event, prompt
## Human commands
After `session/new` and `session/load`, the bridge emits ACP's full `available_commands_update` snapshot for that exact agent. A global or scoped registry change refreshes every live session from its independently resolved view, so clients replace rather than merge cached catalogs. Names omit the slash; descriptions and optional unstructured-input hints map directly to ACP `AvailableCommand`.
After `session/new` and `session/load`, the bridge emits ACP's full `available_commands_update` snapshot for that exact agent. A new session's server-generated id is introduced by the RPC response before its snapshot enters the connection write queue. A global or scoped registry change refreshes every live session from its independently resolved view, so clients replace rather than merge cached catalogs. Names omit the slash; descriptions and optional unstructured-input hints map directly to ACP `AvailableCommand`.
ACP v1 permits a command prompt to carry additional content blocks. The bridge applies its ordinary lossless flattening for supported `text` and `resource_link` blocks, then dispatches when the result begins with `/`. Known commands execute without a model request. Unknown or malformed slash input returns a direct error instead of falling back to the model. Expected handler errors, thrown failures, and successful text stream as UI-only `agent_message_chunk` output and end the request; cancellation returns `cancelled`. See the [command Agent Note](../../../.agents/notes/implemented/feature/2026-07-19-plugin-command-registration.md) and the [ACP v1 slash-command contract](https://agentclientprotocol.com/protocol/v1/slash-commands).
ACP v1 permits a command prompt to carry additional content blocks. The bridge applies its ordinary lossless flattening for supported `text` and `resource_link` blocks, then dispatches when the result begins with `/`. Known commands execute without a model request. Unknown or malformed slash input returns a direct error instead of falling back to the model; prefix whitespace when literal slash-leading text must reach the model. Expected handler errors, thrown failures, and successful text stream as UI-only `agent_message_chunk` output and end the request; cancellation returns `cancelled`. See the [command Agent Note](../../../.agents/notes/implemented/feature/2026-07-19-plugin-command-registration.md) and the [ACP v1 slash-command contract](https://agentclientprotocol.com/protocol/v1/slash-commands).
## Session config options

View File

@@ -17,6 +17,7 @@ import {
PROTOCOL_VERSION,
RequestError,
type Agent as AcpAgent,
type AnyMessage,
type AuthenticateRequest,
type AvailableCommand,
type CancelNotification,
@@ -94,6 +95,34 @@ function renderThrown(value: unknown): string {
}
}
/** Return a server-created session id carried by an outbound success response. */
function responseSessionId(message: AnyMessage): SessionId | undefined {
if (!('result' in message) || typeof message.result !== 'object' || message.result === null
|| !('sessionId' in message.result) || typeof message.result.sessionId !== 'string') {
return undefined
}
return SessionId(message.result.sessionId)
}
/** Observe messages only after the wrapped ACP transport has written them. */
function observeOutbound(stream: Stream, onWritten: (message: AnyMessage) => void): Stream {
const writer = stream.writable.getWriter()
return {
readable: stream.readable,
writable: new WritableStream<AnyMessage>({
async write(message) {
await writer.write(message)
onWritten(message)
},
/* v8 ignore start -- the ACP SDK never closes or aborts its outbound stream;
preserve the wrapped Stream contract for other consumers nonetheless */
close: () => writer.close(),
abort: (reason: unknown) => writer.abort(reason),
/* v8 ignore stop */
}),
}
}
/** Preserve failed-turn detail; plain handler errors become a generic wire internal error. */
function internalError(detail: string): RequestError {
return RequestError.internalError(undefined, detail)
@@ -393,6 +422,9 @@ export function apply(ctx: Context, config: AcpConfig): void {
const sessions = new Map<SessionId, SessionRecord>()
// Reserve an id before resume so pipelined load/new requests cannot duplicate it.
const loadingIds = new Set<SessionId>()
// A new-session response introduces its server-generated id to the client;
// keep its initial command snapshot pending until that response is written.
const pendingCommandSnapshots = new Map<SessionId, SessionRecord>()
// Async creation checks this after awaits to avoid publishing after teardown.
let closed = false
// Each new or loaded session snapshots the latest connection capability.
@@ -499,10 +531,23 @@ export function apply(ctx: Context, config: AcpConfig): void {
})
}
/** Enqueue a new session's first command snapshot behind its written RPC response. */
const announceInitialCommands = (message: AnyMessage): void => {
const sessionId = responseSessionId(message)
if (sessionId === undefined) return
const rec = pendingCommandSnapshots.get(sessionId)
if (rec === undefined) return
pendingCommandSnapshots.delete(sessionId)
notifyCommands(rec)
}
// Registration and HMR removal can affect global or one scoped view; refresh
// every bridge-owned session and let the registry resolve each exact agent.
// every announced bridge-owned session and let the registry resolve each
// exact agent. A pending new-session snapshot will read the latest registry.
ctx.on('commands/change', () => {
for (const rec of sessions.values()) notifyCommands(rec)
for (const rec of sessions.values()) {
if (!pendingCommandSnapshots.has(rec.agent.session.id)) notifyCommands(rec)
}
})
/** Settle the in-flight prompt with a stop reason, exactly once (no-op if none pending). */
@@ -711,7 +756,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
await handle.dispose()
throw internalError('connection closed during session/new')
}
sessions.set(sessionId, {
const record: SessionRecord = {
agent: handle.agent,
dispose: () => handle.dispose(),
presenter: makePresenter(handle.agent),
@@ -720,8 +765,9 @@ export function apply(ctx: Context, config: AcpConfig): void {
inflight: undefined,
commandAbort: undefined,
pendingSwitches: {},
})
notifyCommands(requireSession(sessionId))
}
sessions.set(sessionId, record)
pendingCommandSnapshots.set(sessionId, record)
const configOptions = configOptionsFor(handle.agent, directory)
return { sessionId, ...configOptions.length > 0 ? { configOptions } : {} }
},
@@ -998,7 +1044,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
Writable.toWeb(process.stdout) as WritableStream<Uint8Array>,
Readable.toWeb(process.stdin) as ReadableStream<Uint8Array>,
)
conn = new AgentSideConnection(makeAgent, stream)
conn = new AgentSideConnection(makeAgent, observeOutbound(stream, announceInitialCommands))
/**
* Tear ALL live sessions down to quiescence (docs/defensive-patterns.md "dispose must reach
@@ -1036,6 +1082,7 @@ export function apply(ctx: Context, config: AcpConfig): void {
// installed yet) must observe this after its await and refuse to install a
// post-teardown record. Set even when there are no live sessions.
closed = true
pendingCommandSnapshots.clear()
const recs = [...sessions.values()]
sessions.clear()
if (recs.length === 0) return Promise.resolve()

View File

@@ -41,13 +41,15 @@ describe('ACP plugin commands', () => {
await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
expect(commandUpdates(harness, sessionId).at(-1)?.update).toEqual({
sessionUpdate: 'available_commands_update',
availableCommands: [{
name: 'inspect',
description: 'Inspect the session',
input: { hint: '<target>' },
}],
await vi.waitFor(() => {
expect(commandUpdates(harness!, sessionId).at(-1)?.update).toEqual({
sessionUpdate: 'available_commands_update',
availableCommands: [{
name: 'inspect',
description: 'Inspect the session',
input: { hint: '<target>' },
}],
})
})
const dispose = harness.ctx.commands.register({
@@ -88,6 +90,23 @@ describe('ACP plugin commands', () => {
})
})
it('coalesces registry changes before a new session command snapshot is announced', async () => {
harness = await makeBridgeHarness({ storageDir })
await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
harness.ctx.commands.register({
name: 'raced', description: 'Registered after the response', handler: () => ({ kind: 'success' }),
})
await vi.waitFor(() => {
expect(commandUpdates(harness!, sessionId)).toHaveLength(1)
expect(commandUpdates(harness!, sessionId)[0]?.update).toMatchObject({
availableCommands: [{ name: 'raced' }],
})
})
})
it('executes a known single-text command directly and never sends it to the model', async () => {
harness = await makeBridgeHarness({ storageDir })
const seen = vi.fn(() => ({ kind: 'success' as const, text: 'DIRECT RESULT' }))

View File

@@ -1,4 +1,4 @@
import { afterEach, beforeEach, describe, expect, it } from 'vitest'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { mkdtemp, rm } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
@@ -24,6 +24,9 @@ describe('acp bridge — demux & config edges', () => {
harness = await makeBridgeHarness({ storageDir, script: [textResponse('foreign')] })
await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
await vi.waitFor(() => {
expect(harness!.updates.some(update => update.sessionUpdate === 'available_commands_update')).toBe(true)
})
const before = harness.updates.length
const { agent: foreign } = await harness.ctx.agents.create({ sessionId: SessionId('foreign-session'), agentOptions: { provider: 'mock', model: 'mock' } })

View File

@@ -4,7 +4,7 @@ Plugin-owned human-command registry shared by the TUI and ACP adapters. The [plu
## Service contract
`ctx.commands.register(definition)` registers one lowercase command name, description, optional ACP-compatible unstructured-input hint, optional surface list, and abortable handler. A plain-context registration is global. A command-producing plugin mounted beneath `agent.ctx` declares its own `commands` injection and creates an exact agent-scoped definition; it shadows a global definition with the same name. This child-injection shape preserves the agent scope without making the core agent loop depend on a UI service. Duplicate names within one layer fail during registration. Every disposer is the exact Cordis effect disposer, and registration or removal emits `commands/change` so live adapters can refresh discovery.
`ctx.commands.register(definition)` registers one lowercase command name, description, optional ACP-compatible unstructured-input hint, optional surface list, and abortable handler. A plain-context registration is global. A command-producing plugin mounted beneath `agent.ctx` declares its own `commands` injection and creates an exact agent-scoped definition; it shadows a global definition with the same name. This child-injection shape preserves the agent scope without making the core agent loop depend on a UI service. Duplicate names within one layer fail during registration. Every disposer is the exact Cordis effect disposer, and registration or removal notifies every `commands/change` observer so live adapters can refresh discovery; observer failures are logged and cannot veto the registry mutation or starve later observers.
`list(agent, surface)` returns immutable, name-sorted descriptors after scoped shadowing and surface filtering. `find(agent, surface, name)` returns the corresponding definition. `execute(agent, surface, line, signal)` uses `parseCommand()` and runs only a known command, returning `undefined` for invalid syntax, unknown names, or commands hidden from that surface.

View File

@@ -88,6 +88,7 @@ declare module 'cordis' {
/**
* A command was registered or unregistered. This is an unfiltered registry
* notification because a global or scoped change may affect any UI view.
* Observer failures are contained and cannot veto the registry mutation.
* @mode emit
*/
'commands/change'(): void
@@ -115,6 +116,15 @@ function abortError(signal: AbortSignal): Error {
return new Error(typeof signal.reason === 'string' ? signal.reason : 'command aborted')
}
/** Render arbitrary thrown values without trusting their string coercion. */
function renderThrown(value: unknown): string {
try {
return String(value)
} catch {
return '<unrenderable thrown value>'
}
}
/** Stop awaiting an uncooperative handler once its owning UI request aborts. */
function withAbort<T>(promise: Promise<T>, signal: AbortSignal): Promise<T> {
if (signal.aborted) return Promise.reject(abortError(signal))
@@ -133,7 +143,7 @@ function withAbort<T>(promise: Promise<T>, signal: AbortSignal): Promise<T> {
signal.removeEventListener('abort', onAbort)
reject(error instanceof Error
? error
: new Error('command handler rejected with a non-Error value'))
: new Error(`command handler rejected with a non-Error value: ${renderThrown(error)}`, { cause: error }))
},
)
})
@@ -144,17 +154,26 @@ function normalizeDefinition(definition: CommandDefinition): RegisteredCommand {
if (!COMMAND_NAME.test(definition.name)) {
throw new TypeError(`command name "${definition.name}" must match ${String(COMMAND_NAME)}`)
}
if (typeof definition.description !== 'string') {
throw new TypeError(`command "${definition.name}" description must be a string`)
}
if (definition.description.trim().length === 0) {
throw new TypeError(`command "${definition.name}" description must not be empty`)
}
if (typeof definition.handler !== 'function') {
throw new TypeError(`command "${definition.name}" handler must be a function`)
}
const input = definition.input === undefined
? undefined
: Object.freeze({ hint: definition.input.hint })
if (input !== undefined && input.hint.trim().length === 0) {
throw new TypeError(`command "${definition.name}" input hint must not be empty`)
const rawInput: unknown = definition.input
let input: CommandInputDescriptor | undefined
if (rawInput !== undefined) {
if (typeof rawInput !== 'object' || rawInput === null || !('hint' in rawInput)
|| typeof rawInput.hint !== 'string') {
throw new TypeError(`command "${definition.name}" input hint must be a string`)
}
if (rawInput.hint.trim().length === 0) {
throw new TypeError(`command "${definition.name}" input hint must not be empty`)
}
input = Object.freeze({ hint: rawInput.hint })
}
const surfaces = [...(definition.surfaces ?? DEFAULT_SURFACES)]
if (surfaces.length === 0) {
@@ -240,9 +259,9 @@ export class CommandService extends Service {
yield () => {
layer.delete(registered.definition.name)
if (scope !== undefined && layer.size === 0) this.scoped.delete(scope)
this.ctx.emit('commands/change')
this.notifyChange()
}
this.ctx.emit('commands/change')
this.notifyChange()
}.bind(this), 'commands.register()')
// eslint-disable-next-line @typescript-eslint/no-misused-promises -- exact synchronous disposer preserves composite teardown order
return dispose
@@ -314,6 +333,23 @@ export class CommandService extends Service {
}
return layer
}
/** Notify every registry observer without making UI refresh load-bearing. */
private notifyChange(): void {
// Cordis emit uses Array.map: one synchronous throw starves later listeners,
// and returned promises are discarded. Registry notifications are
// non-vetoing, so contain each callback independently.
for (const callback of this.ctx.events.dispatch('emit', ['commands/change'])) {
try {
const returned: unknown = callback()
void Promise.resolve(returned).catch((error: unknown) => {
this.ctx.logger.warn(`commands/change listener rejected: ${renderThrown(error)}`)
})
} catch (error: unknown) {
this.ctx.logger.warn(`commands/change listener threw: ${renderThrown(error)}`)
}
}
}
}
export default CommandService

View File

@@ -107,7 +107,7 @@ describe('CommandService', () => {
expect(() => scope.ctx.commands.register(command('same'))).toThrow(/already registered in this scope/)
})
it('emits on registration and disposal and rolls back when notification fails', async () => {
it('notifies on registration and disposal while containing broken observers', async () => {
const ctx = await mount()
const changed = vi.fn()
ctx.on('commands/change', changed)
@@ -116,11 +116,39 @@ describe('CommandService', () => {
dispose()
expect(changed).toHaveBeenCalledTimes(2)
const explode = ctx.on('commands/change', () => { throw new Error('observer failed') })
expect(() => ctx.commands.register(command('rollback'))).toThrow('observer failed')
explode()
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
ctx.on('commands/change', () => { throw new Error('observer threw') })
// eslint-disable-next-line @typescript-eslint/no-misused-promises -- exercises rejected-listener containment
ctx.on('commands/change', () => Promise.reject(new Error('observer rejected')))
const afterFailures = vi.fn()
ctx.on('commands/change', afterFailures)
const removeContained = ctx.commands.register(command('contained'))
const { agent } = await mintAgentScope(ctx, 'a')
expect(ctx.commands.find(agent, 'tui', 'rollback')).toBeUndefined()
expect(ctx.commands.find(agent, 'tui', 'contained')).toBeDefined()
expect(afterFailures).toHaveBeenCalledTimes(1)
await vi.waitFor(() => {
expect(warn).toHaveBeenCalledWith('commands/change listener threw: Error: observer threw')
expect(warn).toHaveBeenCalledWith('commands/change listener rejected: Error: observer rejected')
})
removeContained()
expect(ctx.commands.find(agent, 'tui', 'contained')).toBeUndefined()
expect(afterFailures).toHaveBeenCalledTimes(2)
})
it('rejects non-string descriptions and input hints with boundary diagnostics', async () => {
const ctx = await mount()
expect(() => ctx.commands.register({
...command('description-type'),
description: undefined,
} as unknown as CommandDefinition)).toThrow('command "description-type" description must be a string')
expect(() => ctx.commands.register({
...command('hint-type'),
input: { hint: 42 },
} as unknown as CommandDefinition)).toThrow('command "hint-type" input hint must be a string')
expect(() => ctx.commands.register({
...command('input-type'),
input: null,
} as unknown as CommandDefinition)).toThrow('command "input-type" input hint must be a string')
})
it('passes exact invocation context and detaches valid handler results', async () => {
@@ -187,7 +215,20 @@ describe('CommandService', () => {
handler: () => Promise.reject('not an Error'),
})
await expect(ctx.commands.execute(agent, 'tui', '/reject-value', new AbortController().signal))
.rejects.toThrow('command handler rejected with a non-Error value')
.rejects.toThrow('command handler rejected with a non-Error value: not an Error')
const hostile = { toString(): string { throw new Error('cannot render') } }
ctx.commands.register({
name: 'reject-hostile',
description: 'Reject an unrenderable value',
// eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- exercise hostile plugin normalization
handler: () => Promise.reject(hostile),
})
await expect(ctx.commands.execute(agent, 'tui', '/reject-hostile', new AbortController().signal))
.rejects.toMatchObject({
message: 'command handler rejected with a non-Error value: <unrenderable thrown value>',
cause: hostile,
})
})
it('observes an abort triggered synchronously inside the handler', async () => {

View File

@@ -1229,6 +1229,7 @@ export function createTuiChat(
commandControllers.add(controller)
void ctx.commands.execute(agent, 'tui', text, controller.signal).then(
(result) => {
if (disposed) return
if (result === undefined) {
appendNotice(`Unknown command: ${text}`, 'warning')
} else if (result.text !== undefined && result.text !== '') {
@@ -1342,7 +1343,12 @@ export function createTuiChat(
} catch (error: unknown) {
disposed = true
detachListeners()
void commandFiber.dispose()
void commandFiber.dispose().catch(
/* v8 ignore next 2 -- command registration cleanup is non-throwing; this guards a future disposer regression */
(cleanupError: unknown) => {
ctx.logger.warn(`ui-tui: command cleanup after startup failure failed: ${renderThrown(cleanupError)}`)
},
)
clearStatus()
disposeUserInteraction()
ui.stop()

View File

@@ -510,6 +510,35 @@ describe('pi-tui chat lifecycle and transcript', () => {
await result.ctx.fiber.dispose()
})
it('suppresses a successful plugin result that settles as TUI disposal starts', async () => {
const result = await setup()
let started!: () => void
const ready = new Promise<void>((resolve) => { started = resolve })
let resolveCommand!: (result: { kind: 'success'; text: string }) => void
result.ctx.commands.register({
name: 'late-success',
description: 'Resolve while the TUI closes',
surfaces: ['tui'],
handler: () => new Promise((resolve) => {
resolveCommand = resolve
started()
}),
})
result.terminal.send('/late-success')
result.terminal.send('\r')
await ready
resolveCommand({ kind: 'success', text: 'must not render after disposal' })
// Let the command boundary accept the result before disposal, but leave the
// TUI continuation queued so the success-side disposal guard owns the race.
await Promise.resolve()
await result.controller.dispose()
await tick()
expect(result.terminal.output).not.toContain('must not render after disposal')
await result.ctx.fiber.dispose()
})
it('cancels before /exit while running and handles agent errors/disposal', async () => {
const result = await setup({ status: 'running' })
result.terminal.send('/exit')