From 0d23cb75c223ecabff871812971b7839e2f25aa9 Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Wed, 1 Jul 2026 00:23:07 +0900 Subject: [PATCH] fix(session): converge plan mode on ask/resolve across continuation paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan-mode convergence (call `ask` or `resolve`) was enforced on only the non-synthetic `prompt()` return. Every other turn-ending path — `agent.continue()` drains, the IRC idle wake, and advisor steer delivery — settles via `agent_end` and bypassed it, so advisor/IRC/follow-on traffic could keep the agent producing non-converging turns indefinitely (the prompt() recovery wait never returns while continuations run, and agent-core re-drains late steering/asides before agent_end). Suppress non-user producers in plan mode: `#routeAdvice` preserves the advisor card (visible + persisted, no turn) and the idle IRC paths (`deliverIrcMessage`, `#resumeStrandedIrcAsides`) record into context without waking a turn. User steers/follow-ups are untouched and still resume planning. Enforce the decision at the universal `agent_end` terminal settle via a bounded-retry counter: a plan-mode turn that stops without `ask`/`resolve` gets the reminder plus a provider-neutral `required` choice (both tools stay available), up to a fixed cap, then yields to the user. A non-decision tool answer (e.g. `read`) does not reset the cap, so it cannot loop or silently end plan mode un-converged; `ask`/`resolve`, a fresh user prompt, or plan-mode exit reset the counter. Todo-completion reminders are gated off in plan mode so they cannot re-wake a turn the cap intends to yield. Removes the now-redundant post-`prompt()` enforcement to keep a single policy. Op: correct Restores: spec:plan-mode turns must converge to ask/resolve or yield after bounded reminders --- packages/coding-agent/CHANGELOG.md | 4 + .../coding-agent/src/session/agent-session.ts | 124 +++++++-- ...gent-session-plan-mode-convergence.test.ts | 263 ++++++++++++++++++ 3 files changed, 374 insertions(+), 17 deletions(-) create mode 100644 packages/coding-agent/test/agent-session-plan-mode-convergence.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 46831be05..46f14363f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed plan mode hanging without converging on `ask`/`resolve` after advisor cards, idle IRC messages, or follow-on turns. Plan-mode decision enforcement ran on only the non-synthetic `prompt()` return; continuation/wake paths settled via `agent_end` and bypassed it. Advisor cards and idle IRC are now recorded into context without waking an autonomous turn, and the `ask`/`resolve` decision is enforced at the universal `agent_end` terminal settle via a bounded-retry counter (provider-neutral `required`, both tools kept available) that reminds-then-forces a fixed number of times and then yields to the user — never looping, never silently ending plan mode un-converged. ([#3910](https://github.com/can1357/oh-my-pi/issues/3910)) + ## [16.2.8] - 2026-06-30 ### Added diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index cf694a150..e044bb6b4 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -340,6 +340,8 @@ import { classifyUnexpectedStop, isUnexpectedStopCandidate } from "./unexpected- import { YieldQueue } from "./yield-queue"; const SESSION_STOP_CONTINUATION_CAP = 8; +const PLAN_MODE_REMINDER_MAX = 3; +const PLAN_DECISION_TOOLS = new Set(["ask", "resolve"]); /** Abort reason for the Gemini reasoning-header runaway interrupt. Surfaced on the * discarded assistant turn only; never reaches the model. */ @@ -1410,6 +1412,8 @@ export class AgentSession { * instruction") does not drive 1/3 → 2/3 → 3/3 without user input. */ #todoReminderAwaitingProgress = false; + #planModeReminderCount = 0; + #planModeReminderAwaitingProgress = false; #todoPhases: TodoPhase[] = []; #replanTitleRefreshInFlight: Promise | undefined = undefined; #toolChoiceQueue = new ToolChoiceQueue(); @@ -1674,6 +1678,21 @@ export class AgentSession { if (this.#canAutoContinueForFollowUp() && this.agent.hasQueuedMessages()) return; const records = this.#pendingIrcAsides; this.#pendingIrcAsides = []; + if (this.#planModeState?.enabled) { + // Plan mode: fold stranded IRC asides into context without waking an + // autonomous turn. Convergence to ask/resolve stays user-driven. + for (const record of records) { + this.agent.appendMessage(record); + this.sessionManager.appendCustomMessageEntry( + record.customType, + record.content, + record.display, + record.details, + record.attribution ?? "agent", + ); + } + return; + } this.#wakeForIrc(records); } @@ -2313,6 +2332,20 @@ export class AgentSession { return; } this.#recordAdvisorInterruptDelivered(); + if (this.#planModeState?.enabled) { + // Plan mode: record advice visibly in context but never wake an + // autonomous turn — only user-driven turns converge on ask/resolve. + this.#preserveAdvisorCard({ + role: "custom", + customType: "advisor", + content, + display: true, + attribution: "agent", + details, + timestamp: Date.now(), + }); + return; + } void this.sendCustomMessage( { customType: "advisor", content, display: true, attribution: "agent", details }, { deliverAs: "steer", triggerTurn: true }, @@ -3132,6 +3165,11 @@ export class AgentSession { } else { await this.#goalRuntime.onToolCompleted(event.toolName); } + this.#planModeReminderAwaitingProgress = false; + if (this.#isPlanDecisionTool(event.toolName)) { + this.#planModeReminderCount = 0; + this.#planModeReminderAwaitingProgress = false; + } } if (event.type === "tool_execution_end" && event.toolName === "yield" && !event.isError) { this.#lastSuccessfulYieldToolCallId = event.toolCallId; @@ -3549,6 +3587,11 @@ export class AgentSession { await emitAgentEndNotification(); return; } + const planModeContinuationScheduled = await this.#enforcePlanModeDecisionAtSettle(); + if (planModeContinuationScheduled) { + await emitAgentEndNotification(); + return; + } const todoContinuationScheduled = await this.#checkTodoCompletion(); if (todoContinuationScheduled) { await emitAgentEndNotification(); @@ -6300,6 +6343,9 @@ export class AgentSession { if (state?.enabled) { this.#planReferenceSent = false; this.#planReferencePath = state.planFilePath; + } else { + this.#planModeReminderCount = 0; + this.#planModeReminderAwaitingProgress = false; } } @@ -6722,6 +6768,8 @@ export class AgentSession { // Agent-initiated synthetic prompts (auto-continue, plan, reminders) do not. if (options?.userInitiated ?? !options?.synthetic) { this.#advisorAutoResumeSuppressed = false; + this.#planModeReminderCount = 0; + this.#planModeReminderAwaitingProgress = false; } // If streaming, queue via steer() or followUp() based on option @@ -6792,9 +6840,6 @@ export class AgentSession { // (e.g., compaction aborted, validation failed). this.#toolChoiceQueue.removeByLabel("eager-todo"); } - if (!options?.synthetic) { - await this.#enforcePlanModeToolDecision(); - } return true; } @@ -10047,41 +10092,67 @@ export class AgentSession { this.#checkpointState = undefined; this.#pendingRewindReport = undefined; } - async #enforcePlanModeToolDecision(): Promise { + #isPlanDecisionTool(name: string): boolean { + return PLAN_DECISION_TOOLS.has(name); + } + + async #enforcePlanModeDecisionAtSettle(): Promise { if (!this.#planModeState?.enabled) { - return; + return false; } const assistantMessage = this.#findLastAssistantMessage(); if (!assistantMessage) { - return; + return false; } if (assistantMessage.stopReason === "error" || assistantMessage.stopReason === "aborted") { - return; + return false; } - const calledRequiredTool = assistantMessage.content.some( - content => content.type === "toolCall" && (content.name === "ask" || content.name === "resolve"), + const calledDecisionTool = assistantMessage.content.some( + content => content.type === "toolCall" && this.#isPlanDecisionTool(content.name), ); - if (calledRequiredTool) { - return; + if (calledDecisionTool) { + this.#planModeReminderCount = 0; + this.#planModeReminderAwaitingProgress = false; + return false; + } + + const hasToolCall = assistantMessage.content.some(content => content.type === "toolCall"); + if (hasToolCall) { + return false; + } + if (this.#planModeReminderAwaitingProgress) { + return false; + } + if (this.#planModeReminderCount >= PLAN_MODE_REMINDER_MAX) { + logger.debug("Plan mode convergence: reminder cap reached; yielding to user"); + return false; } const hasRequiredTools = this.#toolRegistry.has("ask") && this.#toolRegistry.has("resolve"); if (!hasRequiredTools) { logger.warn("Plan mode enforcement skipped because ask/resolve tools are unavailable", { activeToolNames: this.agent.state.tools.map(tool => tool.name), }); - return; + return false; } + this.#planModeReminderCount++; + this.#planModeReminderAwaitingProgress = true; + this.#toolChoiceQueue.pushOnce("required", { label: "plan-mode-decision" }); const reminder = prompt.render(planModeToolDecisionReminderPrompt, { askToolName: "ask", }); + const reminderMessage: Message = { + role: "developer", + content: [{ type: "text", text: reminder }], + attribution: "agent", + timestamp: Date.now(), + }; - await this.prompt(reminder, { - synthetic: true, - expandPromptTemplates: false, - toolChoice: "required", - }); + this.agent.appendMessage(reminderMessage); + this.sessionManager.appendMessage(reminderMessage); + this.#scheduleAgentContinue({ generation: this.#promptGeneration }); + return true; } /** @@ -10230,6 +10301,13 @@ export class AgentSession { return false; } + // Plan mode owns convergence via #enforcePlanModeDecisionAtSettle (remind → + // cap → yield). Todo reminders must not re-wake a turn the cap intends to + // yield to the user. The label is already consumed above, so no leak. + if (this.#planModeState?.enabled) { + return false; + } + // Suppress within a self-continuation chain: if the agent's last turn was driven by a // prior reminder (and the agent took no tool-level action since), do not re-ping. // The agent has already acknowledged; further escalation just wastes context and @@ -13082,6 +13160,18 @@ export class AgentSession { if (autoReply) void this.#runIrcAutoReply(msg); return "injected"; } + // Plan mode: record into context but do not wake an autonomous turn. + if (this.#planModeState?.enabled) { + this.agent.appendMessage(record); + this.sessionManager.appendCustomMessageEntry( + record.customType, + record.content, + record.display, + record.details, + record.attribution ?? "agent", + ); + return "injected"; + } // Idle: wake a real turn so the recipient responds (shared with the stranded-aside resume). this.#wakeForIrc([record]); return "woken"; diff --git a/packages/coding-agent/test/agent-session-plan-mode-convergence.test.ts b/packages/coding-agent/test/agent-session-plan-mode-convergence.test.ts new file mode 100644 index 000000000..540236e79 --- /dev/null +++ b/packages/coding-agent/test/agent-session-plan-mode-convergence.test.ts @@ -0,0 +1,263 @@ +/** + * Contract: plan mode converges on `ask`/`resolve` regardless of how a turn + * ends, and non-user producers cannot keep it spinning. + * + * T1. An advisor concern in plan mode is recorded as a visible card but never + * wakes an autonomous primary turn. + * T2. An idle IRC message in plan mode is folded into context ("injected"), + * not woken. + * T3. A plan-mode turn that stops without `ask`/`resolve` is reminded at the + * terminal settle, bounded by PLAN_MODE_REMINDER_MAX (then yields to the + * user), and either decision tool resets the counter. + */ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { Agent, type AgentMessage, type AgentTool, type StreamFn } from "@oh-my-pi/pi-agent-core"; +import { createMockModel, type MockModel, type MockResponse } from "@oh-my-pi/pi-ai/providers/mock"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import type { IrcMessage } from "@oh-my-pi/pi-coding-agent/irc/bus"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { Snowflake, TempDir } from "@oh-my-pi/pi-utils"; +import { type } from "arktype"; +import planModeReminderPrompt from "../src/prompts/system/plan-mode-tool-decision-reminder.md" with { type: "text" }; + +/** A stable, literal (non-templated) line of the reminder prompt, so the test + * pins the reminder by its real content rather than a hardcoded copy. */ +function deriveReminderFragment(template: string): string { + const line = template + .split("\n") + .map(l => l.trim()) + .find(l => l.length > 20 && !l.includes("{{")); + if (!line) throw new Error("plan-mode reminder template is missing a stable marker line"); + return line; +} +const REMINDER_FRAGMENT = deriveReminderFragment(planModeReminderPrompt); + +function makeTool(name: string): AgentTool { + return { + name, + label: name, + description: `Fake ${name}`, + parameters: type({}), + async execute() { + return { content: [{ type: "text" as const, text: "ok" }] }; + }, + }; +} + +/** Concatenate the text blocks of a message (string or content-array). */ +function messageText(message: AgentMessage): string { + if (!("content" in message)) return ""; + const content = message.content; + if (typeof content === "string") return content; + if (!Array.isArray(content)) return ""; + return content + .filter(block => block.type === "text") + .map(block => block.text) + .join("\n"); +} + +function countReminders(messages: readonly AgentMessage[]): number { + return messages.filter(m => m.role === "developer" && messageText(m).includes(REMINDER_FRAGMENT)).length; +} + +interface PlanHarness { + session: AgentSession; + mock: MockModel; + advisorMock?: MockModel; +} + +describe("AgentSession plan-mode convergence", () => { + let tempDir: TempDir; + let session: AgentSession | undefined; + const authStorages: AuthStorage[] = []; + + beforeEach(() => { + tempDir = TempDir.createSync("@pi-plan-converge-"); + }); + + afterEach(async () => { + try { + await session?.dispose(); + } finally { + session = undefined; + for (const authStorage of authStorages.splice(0)) authStorage.close(); + // dispose() awaits the agent teardown above; no wall-clock wait needed. + await tempDir?.remove(); + } + }); + + async function createPlanSession( + responses: MockResponse[], + options?: { advisorResponses?: MockResponse[] }, + ): Promise { + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) throw new Error("Expected bundled anthropic model to exist"); + + const askTool = makeTool("ask"); + const resolveTool = makeTool("resolve"); + const readTool = makeTool("read"); + + const mock = createMockModel({ responses }); + const agent = new Agent({ + getApiKey: () => "test-key", + // All three tools active so a scripted ask/resolve/read call (and a + // forced "required" choice) can actually execute (isToolChoiceActive). + initialState: { model, systemPrompt: ["Test"], tools: [askTool, resolveTool, readTool], messages: [] }, + streamFn: mock.stream, + }); + + const authStorage = await AuthStorage.create(tempDir.join(`auth-${Snowflake.next()}.db`)); + authStorages.push(authStorage); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + const modelRegistry = new ModelRegistry(authStorage, tempDir.join(`models-${Snowflake.next()}.yml`)); + + let advisorMock: MockModel | undefined; + let advisorStreamFn: StreamFn | undefined; + if (options?.advisorResponses) { + advisorMock = createMockModel({ responses: options.advisorResponses }); + advisorStreamFn = advisorMock.stream; + } + + const created = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings: Settings.isolated({ + "compaction.enabled": false, + "retry.enabled": false, + }), + modelRegistry, + toolRegistry: new Map([ + ["ask", askTool], + ["resolve", resolveTool], + ["read", readTool], + ]), + builtInToolNames: ["ask", "resolve", "read"], + advisorTools: [], + advisorStreamFn, + }); + created.setPlanModeState({ enabled: true, planFilePath: "local://PLAN.md" }); + session = created; + return { session: created, mock, advisorMock }; + } + + it("T1: an advisor concern does not wake the primary in plan mode", async () => { + const harness = await createPlanSession([], { + advisorResponses: [ + { + content: [ + { type: "toolCall", name: "advise", arguments: { note: "tighten the plan", severity: "concern" } }, + ], + }, + ], + }); + harness.session.settings.setModelRole("advisor", "anthropic/claude-sonnet-4-5"); + expect(harness.session.setAdvisorEnabled(true)).toBe(true); + const advisor = harness.session.getAdvisorAgent(); + if (!advisor) throw new Error("Expected advisor agent to be live"); + + await advisor.prompt("inspect current turn").catch(() => {}); + await harness.session.waitForIdle(); + + const advisorCards = harness.session.agent.state.messages.filter( + m => m.role === "custom" && m.customType === "advisor", + ); + expect(advisorCards.length).toBeGreaterThanOrEqual(1); + // The note was preserved/recorded, not used to wake the primary. + expect(harness.mock.calls.length).toBe(0); + // The advisor model actually ran (the test drove the routeAdvice seam). + expect(harness.advisorMock?.calls.length ?? 0).toBeGreaterThanOrEqual(1); + }); + + it("T2: an idle IRC message does not wake an autonomous turn in plan mode", async () => { + const harness = await createPlanSession([]); + const msg: IrcMessage = { id: "m1", from: "peer", to: "me", body: "ping", ts: Date.now() }; + + const outcome = await harness.session.deliverIrcMessage(msg); + + expect(outcome).toBe("injected"); + const sawIrc = harness.session.agent.state.messages.some( + m => m.role === "custom" && m.customType === "irc:incoming", + ); + expect(sawIrc).toBe(true); + expect(harness.mock.calls.length).toBe(0); + }); + + it("T3a: convergence reminders are bounded by the cap, then yield to the user", async () => { + // Alternating text-stop / read cascade: each text stop with awaiting=false + // escalates a reminder; each read clears awaiting (not the count). The 7th + // text stop lands with awaiting=false AND count===cap → cap guard yields. + const harness = await createPlanSession([ + { content: ["planning A"] }, + { content: [{ type: "toolCall", name: "read", arguments: { path: "a" } }] }, + { content: ["planning B"] }, + { content: [{ type: "toolCall", name: "read", arguments: { path: "b" } }] }, + { content: ["planning C"] }, + { content: [{ type: "toolCall", name: "read", arguments: { path: "c" } }] }, + { content: ["planning D"] }, + ]); + + // Todos are ENABLED (default) with an incomplete item, so a missing plan-mode + // gate in #checkTodoCompletion would inject a todo reminder and schedule an + // 8th continuation past the cap — the mock.calls.length === 7 assertion below + // is the behavioral guard for that bypass. + harness.session.setTodoPhases([{ name: "Plan", tasks: [{ content: "draft the plan", status: "pending" }] }]); + + await harness.session.prompt("make a plan"); + await harness.session.waitForIdle(); + + // Exactly PLAN_MODE_REMINDER_MAX (3) reminders, then a cap-yield (no 4th, + // no further continuation), and plan mode is still on (not silently exited). + expect(countReminders(harness.session.agent.state.messages)).toBe(3); + expect(harness.mock.calls.length).toBe(7); + expect(harness.session.getPlanModeState()?.enabled).toBe(true); + }); + + it("T3b: a resolve call resets the convergence counter", async () => { + const harness = await createPlanSession([ + { content: ["planning A"] }, + { content: [{ type: "toolCall", name: "resolve", arguments: { action: "discard", reason: "test reset" } }] }, + { content: ["planning B"] }, + { content: ["planning C"] }, + ]); + + await harness.session.prompt("make a plan"); + await harness.session.waitForIdle(); + + // reminder #1 (text) → resolve resets → reminder #2 (text) → awaiting latch + // suppresses on the final text stop. Two reminders proves the reset re-armed + // the budget; four model calls proves no runaway. + expect(countReminders(harness.session.agent.state.messages)).toBe(2); + expect(harness.mock.calls.length).toBe(4); + }); + + it("T3c: an ask call resets the convergence counter", async () => { + const harness = await createPlanSession([ + { content: ["planning A"] }, + { + content: [ + { + type: "toolCall", + name: "ask", + arguments: { + questions: [ + { id: "q", question: "which?", options: [{ label: "a" }, { label: "b" }], recommended: 0 }, + ], + }, + }, + ], + }, + { content: ["planning B"] }, + { content: ["planning C"] }, + ]); + + await harness.session.prompt("make a plan"); + await harness.session.waitForIdle(); + + expect(countReminders(harness.session.agent.state.messages)).toBe(2); + expect(harness.mock.calls.length).toBe(4); + }); +});