From 37f34af15ff248e7cd2b8d6cab4d88d65c60abce Mon Sep 17 00:00:00 2001 From: imccyu <276526105+imccyu@users.noreply.github.com> Date: Wed, 29 Jul 2026 00:46:56 +0800 Subject: [PATCH] fix(plan): gate the immediate commit on the open-turn fold, not agent status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TUI keyless smoke exposed the wrong idle signal: /plan submitted while the driver is between turns still saw status running (checkpointing), so the selection queued and the immediate copy never appeared. The commit gate is now the log's own open-turn fold — plan/mode commits at once whenever no turn is open, and queues only inside one. Specs drive the distinction through explicit turn/start / turn/end events instead of a fake status; the keyless smoke pins the new idle copy on both /plan and /plan off; regenerated cordis/service catalogs ride along. --- docs/cordis-catalog/services.md | 12 +++-- docs/event-producer-consumer.md | 2 +- .../tui-agent/tests/tui-keyless-smoke.e2e.ts | 10 ++-- .../cordis/tool-cordis/src/api-catalog.ts | 4 +- packages/plan/plan-mode/src/index.ts | 25 +++++++--- .../plan/plan-mode/tests/plan-mode.spec.ts | 46 ++++++++++++++----- 6 files changed, 70 insertions(+), 29 deletions(-) diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index 332aee578b..8ae1218763 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -859,13 +859,19 @@ Source: [`packages/ui/permission/src/index.ts:97`](../../packages/ui/permission/ get(agent: Agent): { active: boolean; pending?: boolean } /** - * Select whether plan mode should be active from the next request boundary. - * Repeated selection of the current or already-pending state is a no-op. + * Select whether plan mode should be active. An idle agent commits the + * change immediately (no boundary would arrive until the next prompt); a + * running agent holds it as pending intent for the next in-turn request + * boundary. Repeated selection of the current or already-pending state is + * a no-op. * * @param agent The agent to switch. * @param active Whether plan mode should be active. + * @returns what happened: `committed` (logged now), `queued` (awaiting the + * next boundary), `cancelled` (an opposite pending selection was cleared; + * the logged state already matches), or `noop` (already in that state). */ -set(agent: Agent, active: boolean): void +set(agent: Agent, active: boolean): 'committed' | 'queued' | 'cancelled' | 'noop' ``` Types: [Agent](../core-data-structures/core.md) diff --git a/docs/event-producer-consumer.md b/docs/event-producer-consumer.md index dabfeb2517..41665e5f0a 100644 --- a/docs/event-producer-consumer.md +++ b/docs/event-producer-consumer.md @@ -33,7 +33,7 @@ This matrix shows which packages dispatch each harness-owned event and which pac | `llm/stream` | `waterfall` | [`packages/llm/llm/src/index.ts:58`](../packages/llm/llm/src/index.ts) | [`llm`](../packages/llm/llm) (`waterfall`) | [`agent-loop`](../packages/core/agent-loop), [`llm`](../packages/llm/llm), [`llm-replay`](../packages/support/llm-replay), [`session-checkpoint-policy`](../packages/session-persistence/session-checkpoint-policy), [`session-title`](../packages/session-title/session-title) | | `session/created` | `emit` | [`packages/core/session/src/index.ts:71`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | `apiproxy`, [`compact`](../packages/compact/compact), [`goal`](../packages/goal/goal), [`hook-protocol`](../packages/hooks/hook-protocol), [`jsonrpc`](../packages/ui/jsonrpc), [`llm-retry`](../packages/llm/llm-retry), [`plan-mode`](../packages/plan/plan-mode), [`session`](../packages/core/session), [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry), [`tools`](../packages/core/tools), [`user-approval`](../packages/ui/user-approval) | | `session/disposed` | `emit` | [`packages/core/session/src/index.ts:81`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`agent-loop`](../packages/core/agent-loop), `apiproxy`, [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry), [`session-title`](../packages/session-title/session-title) | -| `session/event` | `emit` | [`packages/core/session/src/index.ts:93`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`acp`](../packages/acp/acp), `apiproxy`, [`cli-demo`](../packages/examples/cli-demo), [`compact`](../packages/compact/compact), [`compact-basic`](../packages/compact/compact-basic), [`goal`](../packages/goal/goal), [`goal-session`](../packages/goal/goal-session), [`hook-protocol`](../packages/hooks/hook-protocol), [`jsonrpc`](../packages/ui/jsonrpc), [`plan-mode`](../packages/plan/plan-mode), [`session`](../packages/core/session), [`session-persistence`](../packages/session-persistence/session-persistence), [`session-projection`](../packages/session-projection/session-projection), [`session-telemetry`](../packages/telemetry/session-telemetry), [`session-title`](../packages/session-title/session-title), [`token-meter`](../packages/llm/token-meter), [`tools`](../packages/core/tools), [`tui`](../packages/ui/tui), [`user-approval`](../packages/ui/user-approval), [`workspace-context`](../packages/context/workspace-context) | +| `session/event` | `emit` | [`packages/core/session/src/index.ts:93`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`acp`](../packages/acp/acp), `apiproxy`, [`cli-demo`](../packages/examples/cli-demo), [`compact`](../packages/compact/compact), [`compact-basic`](../packages/compact/compact-basic), [`goal`](../packages/goal/goal), [`goal-session`](../packages/goal/goal-session), [`hook-protocol`](../packages/hooks/hook-protocol), [`jsonrpc`](../packages/ui/jsonrpc), [`session`](../packages/core/session), [`session-persistence`](../packages/session-persistence/session-persistence), [`session-projection`](../packages/session-projection/session-projection), [`session-telemetry`](../packages/telemetry/session-telemetry), [`session-title`](../packages/session-title/session-title), [`token-meter`](../packages/llm/token-meter), [`tools`](../packages/core/tools), [`tui`](../packages/ui/tui), [`user-approval`](../packages/ui/user-approval), [`workspace-context`](../packages/context/workspace-context) | | `session/flush` | `parallel` | [`packages/core/session/src/index.ts:103`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry) | | `slash/input-begin-command` | `bail` | [`packages/client/ui-slash/src/types.ts:230`](../packages/client/ui-slash/src/types.ts) | - | `ui-conversation` | | `slash/input-consume-token` | `bail` | [`packages/client/ui-slash/src/types.ts:244`](../packages/client/ui-slash/src/types.ts) | - | `ui-conversation` | diff --git a/examples/tui-agent/tests/tui-keyless-smoke.e2e.ts b/examples/tui-agent/tests/tui-keyless-smoke.e2e.ts index 3908670e27..7ae98167de 100644 --- a/examples/tui-agent/tests/tui-keyless-smoke.e2e.ts +++ b/examples/tui-agent/tests/tui-keyless-smoke.e2e.ts @@ -149,14 +149,14 @@ describe('tui-agent keyless smoke (real Loader tree in a PTY)', () => { actions: [ { waitFor: 'main-session-', send: '/plan' }, { waitFor: '[off|message] — Enter or leave plan mode', send: '\r' }, - { waitFor: 'Entering plan mode (applies from the next step). Use /plan off to leave.', send: '/exit\r' }, + { waitFor: 'Plan mode on. Use /plan off to leave.', send: '/exit\r' }, ], }) expect(output).toContain('DEEPSEEK') expect(output).toContain('HARNESS') expect(output).toContain('main-session-') expect(output).toContain('[off|message] — Enter or leave plan mode') - expect(output).toContain('Entering plan mode (applies from the next step). Use /plan off to leave.') + expect(output).toContain('Plan mode on. Use /plan off to leave.') // Borderless: no box-drawing frame around the banner. expect(output).not.toContain('╭') expect(output).not.toContain('╮') @@ -183,15 +183,15 @@ describe('tui-agent keyless smoke (real Loader tree in a PTY)', () => { // Gating /status on it keeps the assertion race-free; the diagnostics // card is then exercised through the same real Loader/PTY composition. { waitFor: 'scripted session title — DeepSeek Harness', send: '/plan off\r' }, - { waitFor: 'Leaving plan mode (applies from the next step).', send: 'Confirm the scripted run left plan mode.\r' }, + { waitFor: 'Plan mode off.', send: 'Confirm the scripted run left plan mode.\r' }, { waitFor: 'Default mode confirmed.', send: '/status\r' }, { waitFor: 'Session status', send: '/exit\r' }, ], }) expect(output).toContain('I need one decision before I continue.') expect(output).toContain('Reasoning effort: Max.') - expect(output).toContain('Entering plan mode (applies from the next step). Use /plan off to leave.') - expect(output).toContain('Leaving plan mode (applies from the next step).') + expect(output).toContain('Plan mode on. Use /plan off to leave.') + expect(output).toContain('Plan mode off.') expect(output).toContain('Default mode confirmed.') expect(output).toContain(String.raw`\x1b]2;MODEL_CONTROLLED\x07`) expect(output).toContain(String.raw`\x1b[999CMODEL_CURSOR`) diff --git a/packages/cordis/tool-cordis/src/api-catalog.ts b/packages/cordis/tool-cordis/src/api-catalog.ts index 311f7fc592..f4687c8908 100644 --- a/packages/cordis/tool-cordis/src/api-catalog.ts +++ b/packages/cordis/tool-cordis/src/api-catalog.ts @@ -433,8 +433,8 @@ export const SERVICE_API: readonly ServiceApiEntry[] = [ jsDoc: '/**\n * Read the logged plan state and any selected state awaiting a boundary.\n *\n * @param agent The agent to read.\n * @returns Current logged state plus a pending selection, when present.\n */', }, { - signature: 'set(agent: Agent, active: boolean): void', - jsDoc: '/**\n * Select whether plan mode should be active from the next request boundary.\n * Repeated selection of the current or already-pending state is a no-op.\n *\n * @param agent The agent to switch.\n * @param active Whether plan mode should be active.\n */', + signature: 'set(agent: Agent, active: boolean): \'committed\' | \'queued\' | \'cancelled\' | \'noop\'', + jsDoc: '/**\n * Select whether plan mode should be active. An idle agent commits the\n * change immediately (no boundary would arrive until the next prompt); a\n * running agent holds it as pending intent for the next in-turn request\n * boundary. Repeated selection of the current or already-pending state is\n * a no-op.\n *\n * @param agent The agent to switch.\n * @param active Whether plan mode should be active.\n * @returns what happened: `committed` (logged now), `queued` (awaiting the\n * next boundary), `cancelled` (an opposite pending selection was cleared;\n * the logged state already matches), or `noop` (already in that state).\n */', }, ], }, diff --git a/packages/plan/plan-mode/src/index.ts b/packages/plan/plan-mode/src/index.ts index 79c6e30443..dc6b788825 100644 --- a/packages/plan/plan-mode/src/index.ts +++ b/packages/plan/plan-mode/src/index.ts @@ -149,6 +149,16 @@ const planProjectionSchema: ZodType = zod.object({ pending: zod.boolean(), }) +/** Whether the log holds an opened turn without its closing `turn/end`. */ +function hasOpenTurn(events: readonly SessionEvent[]): boolean { + let open = false + for (const event of events) { + if (event.type === 'turn/start') open = true + else if (event.type === 'turn/end') open = false + } + return open +} + /** Plan state at the last logged request header, or `undefined` before the first header. */ function planModeAtLastHeader(events: readonly SessionEvent[]): boolean | undefined { let lastHeader = -1 @@ -364,9 +374,12 @@ export class PlanModeService extends Service { } /** - * Select whether plan mode should be active. An idle agent commits the - * change immediately (no boundary would arrive until the next prompt); a - * running agent holds it as pending intent for the next in-turn request + * Select whether plan mode should be active. Between turns the change + * commits immediately — no request boundary would arrive until the next + * prompt, so a queued intent would hang (the open-turn fold is the idle + * signal: agent status stays `running` through post-turn checkpointing, + * where a boundary equally never comes). During an open turn the + * selection is held as pending intent for the next in-turn request * boundary. Repeated selection of the current or already-pending state is * a no-op. * @@ -381,12 +394,12 @@ export class PlanModeService extends Service { const pending = this.pendingIntents.get(session) const target = pending?.active ?? foldPlanMode(session.events) if (active === target) return 'noop' - if (agent.status === 'running') { + if (hasOpenTurn(session.events)) { this.pendingIntents.set(session, { active, narrate: true }) return foldPlanMode(session.events) === active ? 'cancelled' : 'queued' } - // Idle: commit now. Delete only after append succeeds so a failed durable - // write leaves the selection retryable rather than silently dropped. + // No open turn: commit now. Delete only after append succeeds so a + // failed durable write leaves the selection retryable, not dropped. if (active === foldPlanMode(session.events)) { this.pendingIntents.delete(session) return 'cancelled' diff --git a/packages/plan/plan-mode/tests/plan-mode.spec.ts b/packages/plan/plan-mode/tests/plan-mode.spec.ts index edfd9bd660..72d273d20f 100644 --- a/packages/plan/plan-mode/tests/plan-mode.spec.ts +++ b/packages/plan/plan-mode/tests/plan-mode.spec.ts @@ -23,13 +23,11 @@ const PLAN_CONFIG = { section: TEST_PLAN_SECTION } satisfies PlanModeConfig * and between-step seams used by the loop. */ -async function agentWithSession(ctx: Context, id = 'agent-1', { active, status = 'running' }: { active?: boolean; status?: 'idle' | 'running' } = {}): Promise { +async function agentWithSession(ctx: Context, id = 'agent-1', { active }: { active?: boolean } = {}): Promise { // A live store session when a store is mounted (the command executor logs // lifecycle events through it); bare otherwise (fold/tool-only benches). - // Boundary tests default to a running agent (the mid-turn shape); the - // idle-commit tests pass status: 'idle' explicitly. const session = new Session(SessionId(id)) - const agent = { id: SessionId(id), session, options: {}, status } as unknown as Agent & { session: Session } + const agent = { id: SessionId(id), session, options: {} } as unknown as Agent & { session: Session } let scoped!: Context await ctx.plugin(Object.assign((inner: Context) => { scoped = createScope(inner, agent).ctx }, { inject: ['tools'], @@ -75,6 +73,16 @@ async function boundary(ctx: Context, agent: Agent & { session: Session }, type: await events.serial('agent/step', 1, 2, new AbortController().signal) } +/** Open a turn so a selection queues for the boundary flush (the mid-turn shape). */ +function openTurn(session: Session, turn = 0): void { + session.append('turn/start', { turn, trigger: { kind: 'message', source: { kind: 'user' } } }) +} + +/** Close the open turn (the between-turns shape: selections commit immediately). */ +function closeTurn(session: Session, turn = 0): void { + session.append('turn/end', { turn, reason: { kind: 'completed' } }) +} + /** Append a minimal `request/header` snapshot so the log has a "what the model was told" anchor. */ function header(session: Session): void { session.append('request/header', { header: { config: { provider: 'test', model: 'test-model' } }, reason: 'initial' }) @@ -169,10 +177,11 @@ describe('ctx.planMode: get/set', () => { expect(ctx.planMode.get(agent)).toEqual({ active: true }) }) - it('selects inactive as the plan exit target while running', async () => { + it('selects inactive as the plan exit target during an open turn', async () => { const ctx = await setup() const agent = await agentWithSession(ctx) agent.session.append('plan/mode', { active: true }) + openTurn(agent.session) expect(ctx.planMode.set(agent, false)).toBe('queued') expect(ctx.planMode.get(agent)).toEqual({ active: true, pending: false }) }) @@ -180,6 +189,7 @@ describe('ctx.planMode: get/set', () => { it('drops a no-op set (target equals pending, else the current fold)', async () => { const ctx = await setup() const agent = await agentWithSession(ctx) + openTurn(agent.session) expect(ctx.planMode.set(agent, false)).toBe('noop') expect(ctx.planMode.get(agent)).toEqual({ active: false }) expect(ctx.planMode.set(agent, true)).toBe('queued') @@ -187,9 +197,9 @@ describe('ctx.planMode: get/set', () => { expect(ctx.planMode.get(agent)).toEqual({ active: false, pending: true }) }) - it('an idle selection commits plan/mode immediately (no boundary would come)', async () => { + it('a between-turns selection commits plan/mode immediately (no boundary would come)', async () => { const ctx = await setup() - const agent = await agentWithSession(ctx, 'agent-idle', { status: 'idle' }) + const agent = await agentWithSession(ctx, 'agent-idle') expect(ctx.planMode.set(agent, true)).toBe('committed') expect(foldPlanMode(agent.session.events)).toBe(true) expect(ctx.planMode.get(agent)).toEqual({ active: true }) @@ -201,20 +211,21 @@ describe('ctx.planMode: get/set', () => { expect(agent.session.events.filter(event => event.type === 'plan/mode')).toHaveLength(2) }) - it('an idle reversal of a mid-turn pending intent cancels without logging', async () => { + it('a between-turns reversal of a mid-turn pending intent cancels without logging', async () => { const ctx = await setup() const agent = await agentWithSession(ctx) + openTurn(agent.session) expect(ctx.planMode.set(agent, true)).toBe('queued') - ;(agent as { status: string }).status = 'idle' + closeTurn(agent.session) // Back to the logged state: the pending intent clears, nothing lands. expect(ctx.planMode.set(agent, false)).toBe('cancelled') expect(agent.session.events.some(event => event.type === 'plan/mode')).toBe(false) expect(ctx.planMode.get(agent)).toEqual({ active: false }) }) - it('an idle commit narrates when the last header told the model otherwise', async () => { + it('a between-turns commit narrates when the last header told the model otherwise', async () => { const ctx = await setup() - const agent = await agentWithSession(ctx, 'agent-idle-narrate', { status: 'idle' }) + const agent = await agentWithSession(ctx, 'agent-idle-narrate') header(agent.session) ctx.planMode.set(agent, true) expect(noticeTexts(agent.session)).toEqual(['The user switched this session to plan mode.']) @@ -225,6 +236,7 @@ describe('the boundary flush', () => { it('does not flush at prompt admission — the seam is pre-turn, so the first step boundary lands it', async () => { const ctx = await setup() const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) // Prompt admission runs before any turn opens; a plan/mode appended there // would sit outside the turn. The pending intent survives admission and @@ -243,6 +255,7 @@ describe('the boundary flush', () => { await ctx.plugin(ToolRegistry) const fiber = await ctx.plugin(PlanModeService, PLAN_CONFIG) const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) // A listener captured in the same dispatch snapshot keeps the plan-mode // callback alive across the unload; the resumed wrapper must not append @@ -261,6 +274,7 @@ describe('the boundary flush', () => { await ctx.plugin(ToolRegistry) const fiber = await ctx.plugin(PlanModeService, PLAN_CONFIG) const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) // Serial dispatch captures its listener list up front; prepending after // the plugin puts this listener ahead of the plugin's own prepended one, @@ -284,6 +298,7 @@ describe('the boundary flush', () => { it('nets out a flip sequence that returns to the folded mode (no append, no notice)', async () => { const ctx = await setup() const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) ctx.planMode.set(agent, false) await boundary(ctx, agent, 'turn/start') @@ -338,6 +353,7 @@ describe('the boundary flush', () => { const warn = vi.fn() ctx.logger.warn = warn as never const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) const original = agent.session.append.bind(agent.session) // Only the flush's own plan/mode append fails; the boundary event itself @@ -363,6 +379,7 @@ describe('the boundary flush', () => { const warn = vi.fn() ctx.logger.warn = warn as never const agent = await agentWithSession(ctx) + openTurn(agent.session) ctx.planMode.set(agent, true) const original = agent.session.append.bind(agent.session) agent.session.append = (((type: string, ...rest: unknown[]) => { @@ -537,6 +554,7 @@ describe('/plan', () => { // The `ctx.inject` child mounts asynchronously once `commands` resolves. await new Promise(resolve => setImmediate(resolve)) const plainAgent = await agentWithSession(ctx, 'plain-plan-command') + openTurn(plainAgent.session) const plainSteer = vi.fn() ;(plainAgent as unknown as { steer: typeof plainSteer }).steer = plainSteer expect(ctx.commands.list(plainAgent)).toEqual([ @@ -555,6 +573,7 @@ describe('/plan', () => { expect(plainSteer).not.toHaveBeenCalled() const messageAgent = await agentWithSession(ctx, 'message-plan-command') + openTurn(messageAgent.session) const messageSteer = vi.fn() ;(messageAgent as unknown as { steer: typeof messageSteer }).steer = messageSteer const plan = await ctx.commands.execute(messageAgent, '/plan draft the migration ', signal) @@ -583,6 +602,7 @@ describe('/plan', () => { expect(ctx.planMode.get(inactive)).toEqual({ active: false }) const entering = await agentWithSession(ctx, 'entering-plan-command') + openTurn(entering.session) const enteringSteer = vi.fn() ;(entering as unknown as { steer: typeof enteringSteer }).steer = enteringSteer await ctx.commands.execute(entering, '/plan', signal) @@ -595,6 +615,7 @@ describe('/plan', () => { expect(entering.session.events.some(event => event.type === 'plan/mode')).toBe(false) const active = await agentWithSession(ctx, 'active-plan-command', { active: true }) + openTurn(active.session) const activeSteer = vi.fn() ;(active as unknown as { steer: typeof activeSteer }).steer = activeSteer expect((await ctx.commands.execute(active, '/plan off', signal))?.result) @@ -612,7 +633,7 @@ describe('/plan', () => { await ctx.plugin(CommandService) await new Promise(resolve => setImmediate(resolve)) const signal = new AbortController().signal - const agent = await agentWithSession(ctx, 'idle-plan-command', { status: 'idle' }) + const agent = await agentWithSession(ctx, 'idle-plan-command') expect((await ctx.commands.execute(agent, '/plan', signal))?.result) .toEqual({ kind: 'success', text: 'Plan mode on. Use /plan off to leave.' }) expect(foldPlanMode(agent.session.events)).toBe(true) @@ -956,6 +977,7 @@ describe('HMR disposal', () => { await ctx.plugin(ToolRegistry) const fiber = await ctx.plugin(PlanModeService, PLAN_CONFIG) const agent = await agentWithSession(ctx, 'disposed-recovery') + openTurn(agent.session) ctx.planMode.set(agent, true) expect(ctx.get('planMode')).toBeInstanceOf(PlanModeService) expect(ctx.tools.get(EXIT_PLAN_MODE)).toBeDefined()