fix(mode): address PR #239 review — boundary flush ordering, disposal fence, config validation, TUI plan config
Four ds-review-bot findings: - examples/tui-agent composed dsh-mode without the now-required modes.plan.section, so the TUI leaf failed at Loader startup (the keyless smoke only asserts the banner and missed it); graft the same deployment plan instructions the ACP leaf carries. - The prompt-submit and turn-continuation flushes ran before next(), so a session/set_mode arriving while a downstream async listener (the shipped hooks listeners' shape) awaited applied one request late; both listeners now prepend and flush after next(), matching the request-error wrapper, with a regression test pinning the ordering. - An HMR unload during the exit_plan_mode review let a later approval write into the disposed service and claim an exit whose flush could never land; the execute path now checks the fiber lifetime after the await and fails the call (the mode stays plan; the model re-presents). - resolveConfig accepted empty/untrimmed mode names that list()/ACP then advertised while the package invariant rejected their selection, desynchronizing the picker; names are validated non-empty and trimmed at load, the same shape the invariant enforces.
This commit is contained in:
@@ -142,6 +142,12 @@ export function resolveConfig(config: ModeConfig): ResolvedModes {
|
||||
if (name === DEFAULT_MODE) {
|
||||
throw new Error(`ModeConfig: "${DEFAULT_MODE}" is reserved (the absence of policy) and cannot be defined`)
|
||||
}
|
||||
// The same shape the package invariant enforces on `mode/set`: accepting
|
||||
// an empty or untrimmed KEY here would advertise a name whose selection
|
||||
// the invariant then rejects, desynchronizing the picker forever.
|
||||
if (name.trim() === '' || name.trim() !== name) {
|
||||
throw new Error(`ModeConfig: mode name ${JSON.stringify(name)} must be non-empty and trimmed`)
|
||||
}
|
||||
if (typeof definition.section !== 'string') {
|
||||
throw new Error(`ModeConfig: mode "${name}" needs a string \`section\``)
|
||||
}
|
||||
@@ -231,22 +237,32 @@ export class ModesService extends Service {
|
||||
// flushed mode therefore lands before the prompt that should reflect it.
|
||||
// Contained: policy must never block a prompt or turn; onBoundary can throw
|
||||
// only when session.append rejects during teardown.
|
||||
ctx.on('agent/prompt-submit', (agent, _content, _source, _signal, next) => {
|
||||
try {
|
||||
this.onBoundary(agent)
|
||||
} catch (error) {
|
||||
ctx.logger.warn('dsh-mode: boundary flush failed: %o', error)
|
||||
// Flush AFTER next() on every seam (the request-error wrapper below does
|
||||
// the same): downstream listeners may await, and a `session/set_mode`
|
||||
// arriving during that window must still shape the request this boundary
|
||||
// precedes — a pre-next() flush would apply it one request late.
|
||||
ctx.on('agent/prompt-submit', async (agent, _content, _source, _signal, next) => {
|
||||
const decision = await next()
|
||||
if (!disposed) {
|
||||
try {
|
||||
this.onBoundary(agent)
|
||||
} catch (error) {
|
||||
ctx.logger.warn('dsh-mode: boundary flush failed: %o', error)
|
||||
}
|
||||
}
|
||||
return next()
|
||||
})
|
||||
ctx.on('agent/turn-continuation', (agent, _turn, _decision, _signal, next) => {
|
||||
try {
|
||||
this.onBoundary(agent)
|
||||
} catch (error) {
|
||||
ctx.logger.warn('dsh-mode: boundary flush failed: %o', error)
|
||||
return decision
|
||||
}, { prepend: true })
|
||||
ctx.on('agent/turn-continuation', async (agent, _turn, _decision, _signal, next) => {
|
||||
const decision = await next()
|
||||
if (!disposed) {
|
||||
try {
|
||||
this.onBoundary(agent)
|
||||
} catch (error) {
|
||||
ctx.logger.warn('dsh-mode: boundary flush failed: %o', error)
|
||||
}
|
||||
}
|
||||
return next()
|
||||
})
|
||||
return decision
|
||||
}, { prepend: true })
|
||||
ctx.on('agent/request-error', async (
|
||||
agent,
|
||||
_turn,
|
||||
@@ -337,6 +353,14 @@ export class ModesService extends Service {
|
||||
agent,
|
||||
signal: exec.signal,
|
||||
})
|
||||
// The review may outlive this plugin fiber (HMR unload while the user
|
||||
// decides): the boundary listeners that would flush the switch are
|
||||
// already gone, so a success result here would claim an exit that can
|
||||
// never land. Fail the call instead; a remounted service still holds
|
||||
// plan mode and the model re-presents.
|
||||
if (disposed) {
|
||||
throw new Error('the mode service was reloaded while the plan was under review; present the plan again')
|
||||
}
|
||||
const reviewItems = answer.answers.filter(entry => entry.id === 'plan-review')
|
||||
const item = reviewItems.length === 1 ? reviewItems[0] : undefined
|
||||
if (item?.selected.length !== 1 || item.selected[0] !== APPROVE_LABEL || item.custom !== undefined) {
|
||||
|
||||
@@ -143,6 +143,13 @@ describe('resolveConfig', () => {
|
||||
.toThrow('"default" is reserved')
|
||||
})
|
||||
|
||||
it('rejects an empty or untrimmed mode name loudly (the invariant would reject its selection)', () => {
|
||||
expect(() => resolveConfig({ modes: { ...PLAN_CONFIG.modes, '': { section: 'x' } } }))
|
||||
.toThrow('mode name "" must be non-empty and trimmed')
|
||||
expect(() => resolveConfig({ modes: { ...PLAN_CONFIG.modes, ' review ': { section: 'x' } } }))
|
||||
.toThrow('mode name " review " must be non-empty and trimmed')
|
||||
})
|
||||
|
||||
it('rejects a malformed definition loudly', () => {
|
||||
expect(() => resolveConfig({ modes: { ...PLAN_CONFIG.modes, bad: { section: 5 } as unknown as { section: string } } }))
|
||||
.toThrow('needs a string `section`')
|
||||
@@ -229,6 +236,28 @@ describe('the boundary flush', () => {
|
||||
expect(ctx.modes.get(agent)).toEqual({ current: PLAN_MODE })
|
||||
})
|
||||
|
||||
it('flushes a set() that arrives while a downstream listener is still awaiting (post-next ordering)', async () => {
|
||||
const ctx = await setup()
|
||||
const agent = await agentWithSession(ctx)
|
||||
// A downstream async listener (the shipped hooks listeners' shape): the
|
||||
// selection lands DURING its await — after this boundary began, before it
|
||||
// returns. The prepended flush runs after next(), so the mode/set still
|
||||
// precedes the request this boundary gates.
|
||||
ctx.on('agent/turn-continuation', async (_agent, _turn, decision, _signal, next) => {
|
||||
await new Promise(resolve => setTimeout(resolve, 5))
|
||||
ctx.modes.set(agent, PLAN_MODE)
|
||||
await next()
|
||||
return decision
|
||||
})
|
||||
agent.session.append('step/end', { turn: 1, step: 1 })
|
||||
await agentEvents(ctx, agent).waterfall(
|
||||
'agent/turn-continuation', 1, { action: 'stop' }, new AbortController().signal,
|
||||
() => Promise.resolve({ action: 'stop' }),
|
||||
)
|
||||
expect(foldMode(agent.session.events)).toBe(PLAN_MODE)
|
||||
expect(ctx.modes.get(agent)).toEqual({ current: PLAN_MODE })
|
||||
})
|
||||
|
||||
it('flushes at step/end too (a mid-turn flip lands on the following step)', async () => {
|
||||
const ctx = await setup()
|
||||
const agent = await agentWithSession(ctx)
|
||||
@@ -813,6 +842,30 @@ describe('exit_plan_mode', () => {
|
||||
expect(asked[0]?.signal).toBe(controller.signal)
|
||||
})
|
||||
|
||||
it('fails the call when the plugin is disposed while the review awaits (no phantom exit)', async () => {
|
||||
const ctx = new Context()
|
||||
await ctx.plugin(SystemPrompt)
|
||||
await ctx.plugin(ToolRegistry)
|
||||
const fiber = await ctx.plugin(ModesService, PLAN_CONFIG)
|
||||
await ctx.plugin(UserInteractionService)
|
||||
let answer!: (value: { answers: { id: string; selected: string[] }[] }) => void
|
||||
ctx.userInteraction.registerProvider({
|
||||
ask: () => new Promise((resolve) => { answer = resolve }),
|
||||
})
|
||||
const agent = await agentWithSession(ctx, 'agent-1', { mode: PLAN_MODE })
|
||||
const pending = callExit(ctx, agent)
|
||||
// Let execute reach the review await, then unload the plugin (HMR) and
|
||||
// only afterwards approve. The boundary listeners are gone, so a success
|
||||
// would claim an exit that can never flush — the call must fail instead.
|
||||
await new Promise(resolve => setImmediate(resolve))
|
||||
await fiber.dispose()
|
||||
answer({ answers: [{ id: 'plan-review', selected: ['Approve'] }] })
|
||||
const result = await pending
|
||||
expect(result.isError).toBe(true)
|
||||
expect(result.content).toEqual([{ type: 'text', text: 'Error: the mode service was reloaded while the plan was under review; present the plan again' }])
|
||||
expect(foldMode(agent.session.events)).toBe(PLAN_MODE)
|
||||
})
|
||||
|
||||
it('a throwing provider surfaces as the corrective isError and the mode stays plan', async () => {
|
||||
const { ctx, agent } = await setupWithReview()
|
||||
ctx.userInteraction.registerProvider({ ask: () => { throw new Error('review aborted') } })
|
||||
|
||||
Reference in New Issue
Block a user