diff --git a/docs/tools/exit_plan_mode.md b/docs/tools/exit_plan_mode.md deleted file mode 100644 index 769d69106..000000000 --- a/docs/tools/exit_plan_mode.md +++ /dev/null @@ -1,68 +0,0 @@ -# exit_plan_mode - -> Submits the current plan-mode plan for user approval. - -## Source -- Entry: `packages/coding-agent/src/tools/exit-plan-mode.ts` -- Model-facing prompt: `packages/coding-agent/src/prompts/tools/exit-plan-mode.md` -- Key collaborators: - - `packages/coding-agent/src/tools/plan-mode-guard.ts` — resolves canonical plan paths during plan mode - - `packages/coding-agent/src/plan-mode/approved-plan.ts` — renames approved plan artifact after user approval - - `packages/coding-agent/src/modes/interactive-mode.ts` — approval popup, plan preview, mode exit, tool restoration - - `packages/coding-agent/src/plan-mode/state.ts` — plan-mode state shape - -## Inputs - -| Field | Type | Required | Description | -| --- | --- | --- | --- | -| `title` | `string` | Yes | Final plan title. `.md` is optional; the runtime normalizes to `local://.md`. Allowed characters: letters, numbers, `_`, `-`. | - -## Outputs -- Single-shot success result with `content[0].text = "Plan ready for approval."`. -- `details` contains: - - `planFilePath` — current plan artifact path from plan-mode state, typically `local://PLAN.md` - - `planExists` — whether that file existed at call time - - `title` — normalized title without `.md` - - `finalPlanFilePath` — normalized destination, always `local://<title>.md` -- The actual rename and mode transition happen later in the interactive controller after the user chooses an approval action. - -## Flow -1. `execute()` reads `session.getPlanModeState()` and rejects the call unless `state.enabled` is true. -2. `normalizePlanTitle()` trims whitespace, rejects empty values, rejects `/`, `\\`, and `..`, appends `.md` if missing, and enforces `^[A-Za-z0-9_-]+\.md$`. -3. The tool computes `finalPlanFilePath = local://<normalized>.md` and resolves both source and destination through `resolvePlanPath(...)` to validate them against plan-mode path rules. -4. It `stat`s the current plan file path; if the plan artifact does not exist it throws a `ToolError` telling the caller to write the finalized plan first. -5. On success it returns the approval-ready payload; it does not mutate files itself. -6. `packages/coding-agent/src/modes/controllers/event-controller.ts` watches successful `exit_plan_mode` results and forwards `details` to `InteractiveMode.handleExitPlanModeTool(...)`. -7. The interactive controller aborts the agent, renders the current plan, and shows four choices: `Approve and execute`, `Approve and keep context`, `Refine plan`, `Stay in plan mode`. -8. If the user approves, `#approvePlan(...)` renames `local://PLAN.md` to `local://<title>.md`, exits plan mode, restores the previous tool set, optionally clears session context, writes the approved plan into the new local root when context is reset, and injects a synthetic system prompt instructing execution from the finalized artifact. - -## Side Effects -- Filesystem - - Tool itself only `stat`s the current plan file. - - Approval path later renames the plan artifact via `fs.rename(...)` and may rewrite the approved plan into a fresh local root with `Bun.write(...)`. -- Session state - - Requires active plan-mode state. - - Approval flow aborts the current agent loop, exits plan mode, restores previous active tools, clears or preserves context depending on the user choice, and records the approved plan reference path. -- User-visible prompts / interactive UI - - Successful calls trigger a plan preview and an approval/refinement selector in interactive mode. -- Background work / cancellation - - The controller aborts the running agent before showing the popup to prevent repeated `exit_plan_mode` calls. - -## Limits & Caps -- `title` accepts only `[A-Za-z0-9_-]` plus optional `.md` (`packages/coding-agent/src/tools/exit-plan-mode.ts`). -- Destination must be under the `local:` scheme; approval rename rejects non-`local:` source or destination paths (`packages/coding-agent/src/plan-mode/approved-plan.ts`). -- In plan mode, only the plan file may be edited; other writes are blocked by `enforcePlanModeWrite(...)` in `packages/coding-agent/src/tools/plan-mode-guard.ts`. - -## Errors -- Plan mode inactive: throws `ToolError("Plan mode is not active.")`. -- Empty title: throws `ToolError("Title is required and must not be empty.")`. -- Path traversal / separators: throws `ToolError("Title must not contain path separators or '..'.")`. -- Invalid characters: throws `ToolError("Title may only contain letters, numbers, underscores, or hyphens.")`. -- Missing plan artifact: throws `ToolError("Plan file not found at ... Write the finalized plan ... before calling exit_plan_mode.")`. -- Approval-time failures surface in the UI from `InteractiveMode.handleExitPlanModeTool(...)`, including destination already exists and rename failures from `renameApprovedPlanFile(...)`. - -## Notes -- This tool is hidden/internal: it is injected when `plan.enabled` is on and is not part of normal discoverable built-ins (`packages/coding-agent/src/tools/index.ts`, `packages/coding-agent/src/session/agent-session.ts`). -- The tool returning success does not mean plan mode has ended; it only means the request was handed off to the approval UI. -- `resolvePlanPath(...)` special-cases bare filenames matching the plan basename so `PLAN.md` maps back to the canonical session-scoped `local://PLAN.md` artifact. -- `Approve and keep context` skips the full conversation reset; `Approve and execute` clears context, then copies the approved plan into the new session-local artifact root before execution resumes. diff --git a/docs/tools/search_tool_bm25.md b/docs/tools/search_tool_bm25.md index 886f446bb..c456aa041 100644 --- a/docs/tools/search_tool_bm25.md +++ b/docs/tools/search_tool_bm25.md @@ -110,7 +110,7 @@ - Corpus composition is session-dependent and excludes already-active tools: - MCP entries come from `#discoverableMCPTools`, filtered to names not currently active, mapped with `summary = description`. - Built-in entries appear only in `"all"` mode and only for registry tools whose `loadMode === "discoverable"` and are not currently active. - - Hidden/internal built-ins are intentionally excluded from the built-in corpus: `resolve`, `yield`, `exit_plan_mode`, `report_finding`, `report_tool_issue` are called out in the `#collectDiscoverableBuiltinTools()` comment. + - Hidden/internal built-ins are intentionally excluded from the built-in corpus: `resolve`, `yield`, `report_finding`, `report_tool_issue` are called out in the `#collectDiscoverableBuiltinTools()` comment. - `DiscoverableToolSource` includes `"extension"` and `"custom"`, but `AgentSession.getDiscoverableTools()` currently assembles only built-in and MCP sources. - On startup, `packages/coding-agent/src/sdk.ts` hides non-essential discoverable built-ins in `tools.discoveryMode = "all"`; defaults are `read`, `bash`, and `edit` unless `tools.essentialOverride` changes them. - Query tokenization is simple and deterministic: camelCase is split, non-alphanumerics become spaces, tokens are lowercased, and only non-empty alphanumeric tokens survive. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e329b5e80..51ffce1f5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,24 +1,23 @@ # Changelog ## [Unreleased] +### Breaking Changes -### Fixed +- Removed the dedicated `exit_plan_mode` tool and its prompt, requiring plan-mode completion to use the existing `resolve` tool path instead -- Queued `/skill:<name> [args]` invocations now show as compact `Steer: /skill:<name> [args]` / `Follow-up: /skill:<name> [args]` chips in the pending-messages bar and disappear when the agent consumes the queued message (parity with plain-text steer/follow-up). Previously the queued skill was invisible while queued and rendered as a full skill block at consumption with no chip ever appearing. -- Plan-mode "Approve and compact context" no longer surfaces a red "Operation aborted" line on the plan-mode assistant message; the silent transition into compaction now renders cleanly on both live and replay paths. Real user-cancel aborts on unrelated turns and the existing "Compaction cancelled" path are unchanged. -- Auto-recover conflict-resolution `write`/`read` paths that the agent malformed as `<file>:conflict://<N>` (or `<file>:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `<file>:` prefix is stored on `ParsedConflictUri.recoveredPrefix` and, for writes, surfaces as a trailing note in the result text so the agent learns the correct shape. Clean `conflict://…` URIs are unchanged. -- Fixed hashline edit renderer leaving a stray `@` in the displayed file path when the agent emitted a canonical `@@ PATH` header (or any `@`-run longer than one). Titles like `Edit: @ packages/foo.ts` now render as `Edit: packages/foo.ts`, matching the actual parser in `hashline/input.ts` which already strips every leading `@` before resolving the path. Purely cosmetic — the edit itself was always routed to the correct file. ### Added +- Added optional `extra` metadata object to the `resolve` tool so callers can pass context-specific payloads, including plan approval titles - Added `hide: true` frontmatter option for skill `SKILL.md` files. Hidden skills are still loaded and remain reachable via `skill://<name>` URLs and (when enabled) `/skill:<name>` slash commands, but are omitted from the rendered system prompt's `<skills>` listing so the model won't auto-discover them. Use for skills the user opts into explicitly rather than ones the model should pick up from descriptions. - Added middle elision for streaming tool outputs (bash, ssh, python, js eval) and post-execution tool result spill. When `tools.artifactHeadBytes` is set (default 20 KB), large outputs now keep both the first N KB and the last N KB with an inline `[… N lines elided (M KB) …]` marker between them, instead of dropping everything before the trailing tail. Setting `tools.artifactHeadBytes = 0` reverts to the previous tail-only behavior. The full output is still mirrored to the session artifact (`artifact://<id>`) regardless of elision mode. Exposes `truncateMiddle` and `formatMiddleElisionMarker` from `@oh-my-pi/pi-coding-agent/session/streaming-output`, extends `OutputSinkOptions` with `headBytes`, and adds `direction: "middle"` plus `headRange` / `tailRange` / `elidedLines` / `elidedBytes` to `TruncationMeta`. - Added per-line column cap shared across streaming tool outputs (`bash`, `ssh`, `python`, `js eval`) and the `read` tool. Lines wider than `tools.outputMaxColumns` bytes (default **768**) are ellipsis-truncated at write time and remaining bytes up to the next `\n` are dropped — bounded memory even on multi-MB single-line outputs (e.g. `cat /dev/urandom`). The cap lives on `OutputSink` as the new `maxColumns` option, persists state across chunk boundaries so split-mid-line writes still respect the budget, and exposes `columnDroppedBytes` / `columnTruncatedLines` on `OutputSummary`. Middle-elision byte math subtracts column drops so the "elided from middle" count stays honest. `read` reuses the same setting but trims its already-collected lines via `truncateLine`. Skipped when the read selector is `:raw`. The artifact file (`artifact://<id>`) keeps the full uncapped stream. Set `tools.outputMaxColumns = 0` to disable. - Added Bun HTTP/2 fetch opt-in. Dev scripts (`bun run dev`, `bun run stats`) now pass `bun --experimental-http2-fetch` so every `fetch()` advertises `h2` in the TLS ALPN list and falls back to HTTP/1.1 when the server doesn't select it. Multiplexing collapses parallel requests to the same origin onto one TLS connection. For the installed `omp` binary, export `BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT=1` in your shell to enable the same behavior (the flag has to be set before Bun starts; `process.env` from inside JS is too late). Requires Bun **1.3.14**. - - Added per-subagent cost display (`$X.XX` in the task progress tree and the session-observer stats line). Cost is accumulated incrementally from `message_end` events and shown only when non-zero, using the `statusLineCost` theme color. Providers that do not report per-turn cost data (e.g. subscription/OAuth usage) continue to show nothing. ### Changed +- Changed plan-mode completion to use `resolve { action: "apply", reason, extra: { title } }` to request plan approval rather than calling `exit_plan_mode` +- Changed resolve pending-action previews to trim and truncate long `reason` text for cleaner status-line rendering - Raised the image downscaling default JPEG quality from 75 to 80 in `resizeImage` output generation - Changed image resize metadata notes from coordinate-scale hints to a simple `Image resized from <original> to <displayed>` message and hide the note when the resized dimensions are unchanged - Removed `utils/image-convert.ts` and its `convertToPng` helper; callers now inline `new Bun.Image(bytes).png().toBase64()` from [`Bun.Image`](https://bun.com/docs/runtime/image) (Bun 1.3.14+). @@ -28,9 +27,12 @@ - Changed search truncation metadata/renderer output from match/result-based limits to file-based limits (`fileLimitReached`, `perFileLimitReached`) and updated truncation labels accordingly - Lowered `read.defaultLimit` default from `500` to `300` lines, and split the per-range context padding into asymmetric `RANGE_LEADING_CONTEXT_LINES = 1` / `RANGE_TRAILING_CONTEXT_LINES = 3` (was symmetric `RANGE_CONTEXT_LINES = 3`). Replay analysis over post-summarizer sessions (`scripts/session-stats/optimize_read_config.py`) showed that bare-path reads are over-provisioned at the median (file p50 = 220 lines) and that most follow-up reads are disjoint hops rather than adjacent extensions — so a smaller default plus narrower leading context reclaims tokens without measurably changing first-cover rate. Trailing context stays at 3 lines to keep anchor-stale recovery on narrow reads. Explicit `read.defaultLimit` overrides in settings are honoured unchanged. - ### Fixed +- Queued `/skill:<name> [args]` invocations now show as compact `Steer: /skill:<name> [args]` / `Follow-up: /skill:<name> [args]` chips in the pending-messages bar and disappear when the agent consumes the queued message (parity with plain-text steer/follow-up). Previously the queued skill was invisible while queued and rendered as a full skill block at consumption with no chip ever appearing. +- Plan-mode "Approve and compact context" no longer surfaces a red "Operation aborted" line on the plan-mode assistant message; the silent transition into compaction now renders cleanly on both live and replay paths. Real user-cancel aborts on unrelated turns and the existing "Compaction cancelled" path are unchanged. +- Auto-recover conflict-resolution `write`/`read` paths that the agent malformed as `<file>:conflict://<N>` (or `<file>:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `<file>:` prefix is stored on `ParsedConflictUri.recoveredPrefix` and, for writes, surfaces as a trailing note in the result text so the agent learns the correct shape. Clean `conflict://…` URIs are unchanged. +- Fixed hashline edit renderer leaving a stray `@` in the displayed file path when the agent emitted a canonical `@@ PATH` header (or any `@`-run longer than one). Titles like `Edit: @ packages/foo.ts` now render as `Edit: packages/foo.ts`, matching the actual parser in `hashline/input.ts` which already strips every leading `@` before resolving the path. Purely cosmetic — the edit itself was always routed to the correct file. - Fixed model contextWindow and maxTokens defaulting to `UNK_CONTEXT_WINDOW` (222222) / `UNK_MAX_TOKENS` (8888) when cached or freshly-discovered provider models replace bundled models through `ModelRegistry.#mergeResolvedModels`. The merge now preserves the bundled model's values when the replacement only has sentinel fallbacks. - Fixed headless `browser.open` tab startup on slow Chromium target enumeration by making worker-side stealth user-agent target setup selective, bounded, and best-effort for non-active targets. Worker startup errors are now surfaced directly instead of degrading into the generic tab worker initialization timeout. - Fixed token display for sessions and subagents inflating far beyond the context window. `token_total` status-line segment and the subagent overlay token counter now show `input + output + cacheWrite` instead of `input + output + cacheRead + cacheWrite`. With prompt caching, `cacheRead` per turn equals the full cached context — summing it across all turns produces a cumulative total that is N×context_size (e.g. a 5-turn session with a 1 M-token context reported ~5 M tokens). Cache activity is still visible via the dedicated `cache_read`/`cache_write` status-line segments; billing cost is unaffected. diff --git a/packages/coding-agent/DEVELOPMENT.md b/packages/coding-agent/DEVELOPMENT.md index 9530de175..f86a7e7ff 100644 --- a/packages/coding-agent/DEVELOPMENT.md +++ b/packages/coding-agent/DEVELOPMENT.md @@ -389,7 +389,7 @@ A `ToolFactory` is `(session: ToolSession) => Tool | null | Promise<Tool | null> `createTools(session, toolNames?)` is the entry point. It: -1. Normalizes requested tool names (`toolNames`) and injects `exit_plan_mode` while `plan.enabled` is true. +1. Normalizes requested tool names (`toolNames`). 2. Resolves eval backend allowance via `PI_PY` override (`getEvalBackendsFromEnv()`) or `eval.py` / `eval.js` settings. 3. Performs Python kernel preflight when applicable (`checkPythonKernelAvailability`). 4. Computes effective gating (`isToolAllowed`) from settings and runtime state: @@ -397,7 +397,7 @@ A `ToolFactory` is `(session: ToolSession) => Tool | null | Promise<Tool | null> - recursion guard for `task` (`task.maxRecursionDepth` vs `session.taskDepth`) - yield mode (`requireYieldTool`) and `todo_write` suppression 5. Instantiates selected tools in parallel with `Promise.all`, records slow factory timings when `PI_TIMING=1`, and wraps results with `wrapToolWithMetaNotice`. -6. Includes `resolve` only when at least one instantiated tool has `deferrable: true` (deferred preview/apply workflows). +6. Includes `resolve` unconditionally so plan mode and deferred preview/apply workflows always have it available. The wrapper step is not cosmetic: it enforces uniform meta-notice behavior and normalized error rendering across all tools. @@ -1129,15 +1129,14 @@ Primary file: `packages/coding-agent/src/tools/index.ts`. - `export const BUILTIN_TOOLS: Record<string, ToolFactory> = { ... }` - Key is the external tool name (e.g. `"read"`, `"web_search"`). 4. If it should be hidden/system-only, register under `HIDDEN_TOOLS` instead. - - Existing hidden names: `yield`, `report_finding`, `exit_plan_mode`, `resolve`. + - Existing hidden names: `yield`, `report_finding`, `resolve`. 5. Wire feature gates in `isToolAllowed(name)` when the tool needs runtime enable/disable behavior. - Existing gates use `session.settings.get("<tool>.enabled")` and recursion limits for `task`. 6. If the tool should be selectable by type, update `ToolName = keyof typeof BUILTIN_TOOLS` consumers as needed. Notes from current behavior: -- `createTools()` injects `exit_plan_mode` when `toolNames` are specified and `plan.enabled` is true. -- `resolve` is included only when at least one active tool is marked `deferrable: true` (built-in or extension/custom). +- `createTools()` always includes `resolve`. Plan mode uses it (via the agent calling `resolve` with `extra: { title }`) to submit a finalized plan for user approval; preview/apply tools (e.g. `ast_edit`) use it to gate apply/discard. - `yield` is force-added when `session.requireYieldTool === true`. - Eval availability is mode-driven (`PI_PY`, `eval.py`, `eval.js`); eval falls back to JavaScript when Python is unavailable and JavaScript is enabled. The standalone `bash` tool is always available. diff --git a/packages/coding-agent/src/export/html/template.generated.ts b/packages/coding-agent/src/export/html/template.generated.ts index 954a176b0..a0479e12e 100644 --- a/packages/coding-agent/src/export/html/template.generated.ts +++ b/packages/coding-agent/src/export/html/template.generated.ts @@ -1,2 +1,2 @@ // Auto-generated by scripts/generate-template.ts - DO NOT EDIT -export const TEMPLATE = "<!DOCTYPE html>\n<html lang=\"en\">\n<head>\n <meta charset=\"UTF-8\">\n <meta name=\"viewport\" content=\"width=device-width, initial-scale=1.0\">\n <title>Session Export\n \n \n\n\n \n
\n
\n \n
\n
\n
\n
\n
\n
\n \"\"\n
\n
\n\n \n \n \n \n\n\n"; +export const TEMPLATE = "\n\n\n \n \n Session Export\n \n \n\n\n \n
\n
\n \n
\n
\n
\n
\n
\n
\n \"\"\n
\n
\n\n \n \n \n \n\n\n"; diff --git a/packages/coding-agent/src/export/html/template.js b/packages/coding-agent/src/export/html/template.js index cc7e38f07..e12d3be19 100644 --- a/packages/coding-agent/src/export/html/template.js +++ b/packages/coding-agent/src/export/html/template.js @@ -1155,16 +1155,6 @@ return html; } - function renderExitPlanMode(name, args, result, ctx) { - const badges = args.title ? [String(args.title)] : null; - let html = toolHead('exit_plan_mode', '', badges); - if (result) { - const output = ctx.getResultText(); - if (output) html += formatExpandableOutput(output, 8); - } - return html; - } - function renderResolve(name, args, result, ctx) { const action = str(args.action) || '?'; let html = toolHead('resolve', '', [action]); @@ -1562,7 +1552,6 @@ inspect_image: renderInspectImage, generate_image: renderGenerateImage, ask: renderAsk, - exit_plan_mode: renderExitPlanMode, resolve: renderResolve, github: renderGh, render_mermaid: renderMermaid, diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 9fc61168d..ae905d259 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -13,10 +13,11 @@ import { ToolExecutionComponent } from "../../modes/components/tool-execution"; import { TtsrNotificationComponent } from "../../modes/components/ttsr-notification"; import { getSymbolTheme, theme } from "../../modes/theme/theme"; import type { InteractiveModeContext, TodoPhase } from "../../modes/types"; +import type { PlanApprovalDetails } from "../../plan-mode/approved-plan"; import type { AgentSessionEvent } from "../../session/agent-session"; import { calculatePromptTokens } from "../../session/compaction/compaction"; import { isSilentAbort, readPendingDisplayTag } from "../../session/messages"; -import type { ExitPlanModeDetails } from "../../tools"; +import type { ResolveToolDetails } from "../../tools/resolve"; type AgentSessionEventKind = AgentSessionEvent["type"]; @@ -546,10 +547,13 @@ export class EventController { `Todo update failed${textContent ? `: ${textContent}` : ". Progress may be stale until todo_write succeeds."}`, ); } - if (event.toolName === "exit_plan_mode" && !event.isError) { - const details = event.result.details as ExitPlanModeDetails | undefined; - if (details) { - await this.ctx.handleExitPlanModeTool(details); + if (event.toolName === "resolve" && !event.isError) { + const details = event.result.details as ResolveToolDetails | undefined; + if (details?.sourceToolName === "plan_approval" && details.action === "apply") { + const planDetails = details.sourceResultDetails as PlanApprovalDetails | undefined; + if (planDetails) { + await this.ctx.handlePlanApproval(planDetails); + } } } } diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 2839f20a5..7c21f2285 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -39,7 +39,7 @@ import type { CompactOptions } from "../extensibility/extensions/types"; import { BUILTIN_SLASH_COMMANDS, loadSlashCommands } from "../extensibility/slash-commands"; import { resolveLocalUrlToPath } from "../internal-urls"; import { LSP_STARTUP_EVENT_CHANNEL, type LspStartupEvent } from "../lsp/startup-events"; -import { renameApprovedPlanFile } from "../plan-mode/approved-plan"; +import { normalizePlanTitle, type PlanApprovalDetails, renameApprovedPlanFile } from "../plan-mode/approved-plan"; import planModeApprovedPrompt from "../prompts/system/plan-mode-approved.md" with { type: "text" }; import planModeCompactInstructionsPrompt from "../prompts/system/plan-mode-compact-instructions.md" with { type: "text", @@ -50,9 +50,11 @@ import { HistoryStorage } from "../session/history-storage"; import type { SessionContext, SessionManager } from "../session/session-manager"; import { getRecentSessions } from "../session/session-manager"; import { STTController, type SttState } from "../stt"; -import type { ExitPlanModeDetails, LspStartupServerInfo } from "../tools"; +import type { LspStartupServerInfo } from "../tools"; import { normalizeLocalScheme } from "../tools/path-utils"; +import { runResolveInvocation } from "../tools/resolve"; import { formatPhaseDisplayName } from "../tools/todo-write"; +import { ToolError } from "../tools/tool-errors"; import type { EventBus } from "../utils/event-bus"; import { getEditorCommand, openInEditor } from "../utils/external-editor"; import { getSessionAccentAnsi, getSessionAccentHex } from "../utils/session-color"; @@ -930,8 +932,8 @@ export class InteractiveMode implements InteractiveModeContext { const planFilePath = options?.planFilePath ?? (await this.#getPlanFilePath()); const previousTools = this.session.getActiveToolNames(); - const hasExitTool = this.session.getToolByName("exit_plan_mode") !== undefined; - const planTools = hasExitTool ? [...previousTools, "exit_plan_mode"] : previousTools; + const hasResolveTool = this.session.getToolByName("resolve") !== undefined; + const planTools = hasResolveTool ? [...previousTools, "resolve"] : previousTools; const uniquePlanTools = [...new Set(planTools)]; this.#planModePreviousTools = previousTools; @@ -945,6 +947,7 @@ export class InteractiveMode implements InteractiveModeContext { workflow: options?.workflow ?? "parallel", reentry: this.#planModeHasEntered, }); + this.session.setStandingResolveHandler?.(input => this.#runPlanApprovalResolve(input)); if (this.session.isStreaming) { await this.session.sendPlanModeContext({ deliverAs: "steer" }); } @@ -955,6 +958,50 @@ export class InteractiveMode implements InteractiveModeContext { this.showStatus(`Plan mode enabled. Plan file: ${planFilePath}`); } + /** Standing resolve dispatcher registered while plan mode is active. The agent + * submits the finalized plan by calling `resolve { action: "apply", extra: { title } }`; + * this handler validates the plan file exists, normalizes the title, and shapes the + * payload that `event-controller` forwards to `handlePlanApproval`. */ + #runPlanApprovalResolve(input: unknown): Promise<{ + content: Array<{ type: string; text: string }>; + details?: unknown; + }> { + return runResolveInvocation(input as Parameters[0], { + sourceToolName: "plan_approval", + label: "Plan ready for approval", + apply: async (_reason, extra) => { + const state = this.session.getPlanModeState?.(); + if (!state?.enabled) { + throw new ToolError("Plan mode is not active."); + } + const title = extra?.title; + if (typeof title !== "string" || title.trim() === "") { + throw new ToolError( + 'Plan approval requires `extra: { title: "" }`. Provide a title with letters, numbers, underscores, or hyphens only.', + ); + } + const normalized = normalizePlanTitle(title); + const planFilePath = state.planFilePath; + const planContent = await this.#readPlanFile(planFilePath); + if (planContent === null) { + throw new ToolError( + `Plan file not found at ${planFilePath}. Write the finalized plan to ${planFilePath} before requesting approval.`, + ); + } + const details: PlanApprovalDetails = { + planFilePath, + finalPlanFilePath: `local://${normalized.fileName}`, + title: normalized.title, + planExists: true, + }; + return { + content: [{ type: "text" as const, text: "Plan ready for approval." }], + details, + }; + }, + }); + } + async #exitPlanMode(options?: { silent?: boolean; paused?: boolean }): Promise { if (!this.planModeEnabled) { return; @@ -990,6 +1037,7 @@ export class InteractiveMode implements InteractiveModeContext { } } } + this.session.setStandingResolveHandler?.(null); this.session.setPlanModeState(undefined); this.planModeEnabled = false; this.planModePaused = options?.paused ?? false; @@ -1230,16 +1278,16 @@ export class InteractiveMode implements InteractiveModeContext { } } - async handleExitPlanModeTool(details: ExitPlanModeDetails): Promise { + async handlePlanApproval(details: PlanApprovalDetails): Promise { if (!this.planModeEnabled) { this.showWarning("Plan mode is not active."); return; } - // Abort the agent to prevent it from continuing (e.g., calling exit_plan_mode - // again) while the popup is showing. The event listener fires asynchronously - // (agent's #emit is fire-and-forget), so without this the model sees "Plan - // ready for approval." and immediately calls exit_plan_mode in a loop. + // Abort the agent to prevent it from continuing (e.g., re-submitting the + // plan) while the popup is showing. The event listener fires asynchronously + // (agent's #emit is fire-and-forget), so without this the model sees + // "Plan ready for approval." and immediately re-invokes `resolve` in a loop. await this.session.abort(); const planFilePath = details.planFilePath || this.planModePlanFilePath || (await this.#getPlanFilePath()); diff --git a/packages/coding-agent/src/modes/types.ts b/packages/coding-agent/src/modes/types.ts index 30d6eab72..f4e835988 100644 --- a/packages/coding-agent/src/modes/types.ts +++ b/packages/coding-agent/src/modes/types.ts @@ -11,11 +11,12 @@ import type { } from "../extensibility/extensions"; import type { CompactOptions } from "../extensibility/extensions/types"; import type { MCPManager } from "../mcp"; +import type { PlanApprovalDetails } from "../plan-mode/approved-plan"; import type { AgentSession, AgentSessionEvent } from "../session/agent-session"; import type { CompactionOutcome } from "../session/compaction"; import type { HistoryStorage } from "../session/history-storage"; import type { SessionContext, SessionManager } from "../session/session-manager"; -import type { ExitPlanModeDetails, LspStartupServerInfo } from "../tools"; +import type { LspStartupServerInfo } from "../tools"; import type { AssistantMessageComponent } from "./components/assistant-message"; import type { BashExecutionComponent } from "./components/bash-execution"; import type { CustomEditor } from "./components/custom-editor"; @@ -260,7 +261,7 @@ export interface InteractiveModeContext { handleLoopCommand(args?: string): Promise; disableLoopMode(): void; pauseLoop(): void; - handleExitPlanModeTool(details: ExitPlanModeDetails): Promise; + handlePlanApproval(details: PlanApprovalDetails): Promise; // Hook UI methods initHooksAndCustomTools(): Promise; diff --git a/packages/coding-agent/src/plan-mode/approved-plan.ts b/packages/coding-agent/src/plan-mode/approved-plan.ts index 874eee90f..27f1a3a8f 100644 --- a/packages/coding-agent/src/plan-mode/approved-plan.ts +++ b/packages/coding-agent/src/plan-mode/approved-plan.ts @@ -2,6 +2,40 @@ import * as fs from "node:fs/promises"; import { isEnoent } from "@oh-my-pi/pi-utils"; import { resolveLocalUrlToPath } from "../internal-urls"; import { normalizeLocalScheme } from "../tools/path-utils"; +import { ToolError } from "../tools/tool-errors"; + +/** Shape forwarded from the plan-mode resolve handler to InteractiveMode's + * approval popup. Populated by the standing handler that the resolve tool + * dispatches to when the agent submits `resolve { action: "apply" }`. */ +export interface PlanApprovalDetails { + planFilePath: string; + finalPlanFilePath: string; + title: string; + planExists: boolean; +} + +/** Validate the agent-supplied plan title and derive the destination filename. + * Filename uses the title with a `.md` suffix; characters are restricted to + * letters, numbers, underscores, and hyphens so the value is safe to splice + * into a `local://` URL without escaping. */ +export function normalizePlanTitle(title: string): { title: string; fileName: string } { + const trimmed = title.trim(); + if (!trimmed) { + throw new ToolError("Plan title is required and must not be empty."); + } + + if (trimmed.includes("/") || trimmed.includes("\\") || trimmed.includes("..")) { + throw new ToolError("Plan title must not contain path separators or '..'."); + } + + const withExtension = trimmed.toLowerCase().endsWith(".md") ? trimmed : `${trimmed}.md`; + if (!/^[A-Za-z0-9_-]+\.md$/.test(withExtension)) { + throw new ToolError("Plan title may only contain letters, numbers, underscores, or hyphens."); + } + + const normalizedTitle = withExtension.slice(0, -3); + return { title: normalizedTitle, fileName: withExtension }; +} interface RenameApprovedPlanFileOptions { planFilePath: string; @@ -36,7 +70,7 @@ export async function renameApprovedPlanFile(options: RenameApprovedPlanFileOpti const destinationStat = await fs.stat(resolvedDestination); if (destinationStat.isFile()) { throw new Error( - `Plan destination already exists at ${finalPlanFilePath}. Choose a different title and call exit_plan_mode again.`, + `Plan destination already exists at ${finalPlanFilePath}. Choose a different title and submit the plan for approval again.`, ); } throw new Error(`Plan destination exists but is not a file: ${finalPlanFilePath}`); diff --git a/packages/coding-agent/src/prompts/system/plan-mode-active.md b/packages/coding-agent/src/prompts/system/plan-mode-active.md index a1df8666a..8c692604b 100644 --- a/packages/coding-agent/src/prompts/system/plan-mode-active.md +++ b/packages/coding-agent/src/prompts/system/plan-mode-active.md @@ -6,9 +6,9 @@ You NEVER: - Run state-changing commands (git commit, npm install, etc.) - Make any system changes -To implement: call `{{exitToolName}}` → user approves an execution option → full write access is restored. +To implement: call `resolve` with `action: "apply"`, a `reason`, and `extra: { title: "" }` → user approves an execution option → full write access is restored. `` may only contain letters, numbers, underscores, and hyphens; the approved plan is renamed to `local://.md`. -You NEVER ask the user to exit plan mode for you; you MUST call `{{exitToolName}}` yourself. +You NEVER ask the user to exit plan mode for you; you MUST call `resolve` yourself. ## Plan File @@ -39,7 +39,7 @@ You MUST still make the plan file self-contained: include requirements, decision 3. Decide: - **Different task** → Overwrite plan - **Same task, continuing** → Update and clean outdated sections -4. Call `{{exitToolName}}` when complete +4. Call `resolve` with `action: "apply"` and `extra: { title }` when complete {{/if}} @@ -109,8 +109,8 @@ You MUST ask questions throughout. You NEVER make large assumptions about user i Your turn ends ONLY by: 1. Using `{{askToolName}}` to gather information, OR -2. Calling `{{exitToolName}}` when ready — this triggers user approval, then implementation with full tool access +2. Calling `resolve` with `action: "apply"`, `reason`, and `extra: { title: "" }` when ready — this triggers user approval, then implementation with full tool access -You NEVER ask plan approval via text or `{{askToolName}}`; you MUST use `{{exitToolName}}`. +You NEVER ask plan approval via text or `{{askToolName}}`; you MUST use `resolve`. You MUST keep going until complete. diff --git a/packages/coding-agent/src/prompts/system/plan-mode-tool-decision-reminder.md b/packages/coding-agent/src/prompts/system/plan-mode-tool-decision-reminder.md index 79166baef..db300943d 100644 --- a/packages/coding-agent/src/prompts/system/plan-mode-tool-decision-reminder.md +++ b/packages/coding-agent/src/prompts/system/plan-mode-tool-decision-reminder.md @@ -3,7 +3,7 @@ Plan mode turn ended without a required tool call. You MUST choose exactly one next action now: 1. Call `{{askToolName}}` to gather required clarification, OR -2. Call `{{exitToolName}}` to finish planning and request approval +2. Call `resolve` with `action: "apply"`, `reason`, and `extra: { title: "" }` to finish planning and request approval You NEVER output plain text in this turn. diff --git a/packages/coding-agent/src/prompts/tools/exit-plan-mode.md b/packages/coding-agent/src/prompts/tools/exit-plan-mode.md deleted file mode 100644 index a31f267d8..000000000 --- a/packages/coding-agent/src/prompts/tools/exit-plan-mode.md +++ /dev/null @@ -1,6 +0,0 @@ -Submits a finalized implementation plan for user approval. - -Write the plan to `local://PLAN.md` first, then call this with `title` (e.g. `WP_MIGRATION_PLAN`); on approval the file is renamed to `local://.md` and full tool access is restored. -- Use only after planning implementation steps; not for pure research. -- NEVER call before the plan file exists. -- NEVER use `ask` to request plan approval — this tool does that. diff --git a/packages/coding-agent/src/prompts/tools/resolve.md b/packages/coding-agent/src/prompts/tools/resolve.md index 76982f2df..bff34d67c 100644 --- a/packages/coding-agent/src/prompts/tools/resolve.md +++ b/packages/coding-agent/src/prompts/tools/resolve.md @@ -1,8 +1,9 @@ -Resolves a pending preview action by either applying or discarding it. +Resolves a pending action by either applying or discarding it. - `action` is required: - - `"apply"` persists the pending changes. - - `"discard"` rejects the pending changes. -- `reason` is required and must explain why you chose to apply or discard. + - `"apply"` persists / submits the pending action. + - `"discard"` rejects the pending action. +- `reason` is required and must briefly explain why you chose to apply or discard. +- `extra` (optional) is free-form metadata passed to the resolving tool. Schema depends on context: -Only valid when a pending action exists (typically after a preview step). +Valid whenever a pending action exists — either a preview-style staging (e.g. `ast_edit`) or a long-lived approval gate. Call fails with an error when no pending action exists. diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 2411b46a9..04bfc2509 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -1078,6 +1078,8 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} timestamp: Date.now(), }), peekQueueInvoker: () => session.peekQueueInvoker(), + peekStandingResolveHandler: () => session.peekStandingResolveHandler(), + setStandingResolveHandler: handler => session.setStandingResolveHandler(handler), allocateOutputArtifact: async toolType => { try { return await sessionManager.allocateArtifactPath(toolType); @@ -1503,7 +1505,6 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} (options.toolNames ? [...new Set(options.toolNames.map(name => name.toLowerCase()))] : undefined) ?? toolNamesFromRegistry; const normalizedRequested = requestedToolNames.filter(name => toolRegistry.has(name)); - const includeExitPlanMode = requestedToolNames.includes("exit_plan_mode"); // Effective discovery mode: tools.discoveryMode takes precedence; mcp.discoveryMode is back-compat alias. const toolsDiscoveryModeSetting = settings.get("tools.discoveryMode"); const effectiveDiscoveryMode: "off" | "mcp-only" | "all" = @@ -1516,9 +1517,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} const defaultInactiveToolNames = new Set( registeredTools.filter(tool => tool.definition.defaultInactive).map(tool => tool.definition.name), ); - const requestedActiveToolNames = includeExitPlanMode - ? normalizedRequested - : normalizedRequested.filter(name => name !== "exit_plan_mode"); + const requestedActiveToolNames = normalizedRequested; const initialRequestedActiveToolNames = options.toolNames ? requestedActiveToolNames : requestedActiveToolNames.filter(name => !defaultInactiveToolNames.has(name)); diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 009b10cd4..da3b2bad3 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -959,6 +959,19 @@ export class AgentSession { return this.#toolChoiceQueue.peekInFlightInvoker(); } + /** Standing (long-lived) handler the `resolve` tool falls back to when no + * queue invoker is in flight. Used by plan mode so the agent can submit + * approval via `resolve` without forcing the tool choice every turn. */ + #standingResolveHandler: ((input: unknown) => Promise<unknown> | unknown) | undefined; + + peekStandingResolveHandler(): ((input: unknown) => Promise<unknown> | unknown) | undefined { + return this.#standingResolveHandler; + } + + setStandingResolveHandler(handler: ((input: unknown) => Promise<unknown> | unknown) | null): void { + this.#standingResolveHandler = handler ?? undefined; + } + /** Provider-scoped mutable state store for transport/session caches. */ get providerSessionState(): Map<string, ProviderSessionState> { return this.#providerSessionState; @@ -2691,7 +2704,7 @@ export class AgentSession { /** Collect built-in tools the model can discover via search_tool_bm25. Restricted to tool * definitions whose `loadMode === "discoverable"`. This keeps hidden/internal tools - * (resolve, yield, exit_plan_mode, report_finding, report_tool_issue) out of the index + * (resolve, yield, report_finding, report_tool_issue) out of the index * and avoids mislabeling extension/custom default-inactive tools as built-ins. */ #collectDiscoverableBuiltinTools(): DiscoverableTool[] { const activeNames = new Set(this.getActiveToolNames()); @@ -3405,7 +3418,6 @@ export class AgentSession { askToolName: "ask", writeToolName: "write", editToolName: "edit", - exitToolName: "exit_plan_mode", reentry: state.reentry ?? false, iterative: state.workflow === "iterative", }); @@ -5308,14 +5320,14 @@ export class AgentSession { } const calledRequiredTool = assistantMessage.content.some( - content => content.type === "toolCall" && (content.name === "ask" || content.name === "exit_plan_mode"), + content => content.type === "toolCall" && (content.name === "ask" || content.name === "resolve"), ); if (calledRequiredTool) { return; } - const hasRequiredTools = this.#toolRegistry.has("ask") && this.#toolRegistry.has("exit_plan_mode"); + const hasRequiredTools = this.#toolRegistry.has("ask") && this.#toolRegistry.has("resolve"); if (!hasRequiredTools) { - logger.warn("Plan mode enforcement skipped because ask/exit tools are unavailable", { + logger.warn("Plan mode enforcement skipped because ask/resolve tools are unavailable", { activeToolNames: this.agent.state.tools.map(tool => tool.name), }); return; @@ -5323,7 +5335,6 @@ export class AgentSession { const reminder = prompt.render(planModeToolDecisionReminderPrompt, { askToolName: "ask", - exitToolName: "exit_plan_mode", }); await this.prompt(reminder, { diff --git a/packages/coding-agent/src/tools/exit-plan-mode.ts b/packages/coding-agent/src/tools/exit-plan-mode.ts deleted file mode 100644 index b70f44dcb..000000000 --- a/packages/coding-agent/src/tools/exit-plan-mode.ts +++ /dev/null @@ -1,97 +0,0 @@ -import * as fs from "node:fs/promises"; -import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; -import { isEnoent, prompt } from "@oh-my-pi/pi-utils"; -import { type Static, Type } from "@sinclair/typebox"; -import exitPlanModeDescription from "../prompts/tools/exit-plan-mode.md" with { type: "text" }; -import type { ToolSession } from "."; -import { resolvePlanPath } from "./plan-mode-guard"; -import { ToolError } from "./tool-errors"; - -const exitPlanModeSchema = Type.Object({ - title: Type.String({ description: "final plan title", examples: ["WP_MIGRATION_PLAN"] }), -}); - -type ExitPlanModeParams = Static<typeof exitPlanModeSchema>; - -function normalizePlanTitle(title: string): { title: string; fileName: string } { - const trimmed = title.trim(); - if (!trimmed) { - throw new ToolError("Title is required and must not be empty."); - } - - if (trimmed.includes("/") || trimmed.includes("\\") || trimmed.includes("..")) { - throw new ToolError("Title must not contain path separators or '..'."); - } - - const withExtension = trimmed.toLowerCase().endsWith(".md") ? trimmed : `${trimmed}.md`; - if (!/^[A-Za-z0-9_-]+\.md$/.test(withExtension)) { - throw new ToolError("Title may only contain letters, numbers, underscores, or hyphens."); - } - - const normalizedTitle = withExtension.slice(0, -3); - return { title: normalizedTitle, fileName: withExtension }; -} - -export interface ExitPlanModeDetails { - planFilePath: string; - planExists: boolean; - title: string; - finalPlanFilePath: string; -} - -export class ExitPlanModeTool implements AgentTool<typeof exitPlanModeSchema, ExitPlanModeDetails> { - readonly name = "exit_plan_mode"; - readonly label = "ExitPlanMode"; - readonly description: string; - readonly parameters = exitPlanModeSchema; - readonly strict = true; - readonly concurrency = "exclusive"; - readonly intent = (): string => "present plan"; - - constructor(private readonly session: ToolSession) { - this.description = prompt.render(exitPlanModeDescription); - } - - async execute( - _toolCallId: string, - params: ExitPlanModeParams, - _signal?: AbortSignal, - _onUpdate?: AgentToolUpdateCallback<ExitPlanModeDetails>, - _context?: AgentToolContext, - ): Promise<AgentToolResult<ExitPlanModeDetails>> { - const state = this.session.getPlanModeState?.(); - if (!state?.enabled) { - throw new ToolError("Plan mode is not active."); - } - - const normalized = normalizePlanTitle(params.title); - const finalPlanFilePath = `local://${normalized.fileName}`; - const resolvedPlanPath = resolvePlanPath(this.session, state.planFilePath); - resolvePlanPath(this.session, finalPlanFilePath); - let planExists = false; - try { - const stat = await fs.stat(resolvedPlanPath); - planExists = stat.isFile(); - } catch (error) { - if (!isEnoent(error)) { - throw error; - } - } - - if (!planExists) { - throw new ToolError( - `Plan file not found at ${state.planFilePath}. Write the finalized plan to ${state.planFilePath} before calling exit_plan_mode.`, - ); - } - - return { - content: [{ type: "text", text: "Plan ready for approval." }], - details: { - planFilePath: state.planFilePath, - planExists, - title: normalized.title, - finalPlanFilePath, - }, - }; - } -} diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index ac0740f54..bab59d162 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -29,7 +29,6 @@ import { CalculatorTool } from "./calculator"; import { type CheckpointState, CheckpointTool, RewindTool } from "./checkpoint"; import { DebugTool } from "./debug"; import { EvalTool } from "./eval"; -import { ExitPlanModeTool } from "./exit-plan-mode"; import { FindTool } from "./find"; import { GithubTool } from "./gh"; import { HindsightRecallTool } from "./hindsight-recall"; @@ -70,7 +69,6 @@ export * from "./calculator"; export * from "./checkpoint"; export * from "./debug"; export * from "./eval"; -export * from "./exit-plan-mode"; export * from "./find"; export * from "./gh"; export * from "./hindsight-recall"; @@ -220,6 +218,12 @@ export interface ToolSession { steer?(message: { customType: string; content: string; details?: unknown }): void; /** Peek the currently in-flight tool-choice queue directive's invocation handler. Used by the `resolve` tool to dispatch to the pending action. */ peekQueueInvoker?(): ((input: unknown) => Promise<unknown> | unknown) | undefined; + /** Peek the long-lived "standing" resolve handler registered by a mode (e.g. plan mode). + * Consulted by the `resolve` tool as a fallback when no queue invoker is in flight, + * letting modes accept `resolve` invocations without forcing the tool choice every turn. */ + peekStandingResolveHandler?(): ((input: unknown) => Promise<unknown> | unknown) | undefined; + /** Register or clear the standing resolve handler. Passing `null` clears it. */ + setStandingResolveHandler?(handler: ((input: unknown) => Promise<unknown> | unknown) | null): void; /** Get active checkpoint state if any. */ getCheckpointState?: () => CheckpointState | undefined; /** Set or clear active checkpoint state. */ @@ -303,7 +307,6 @@ export const HIDDEN_TOOLS: Record<string, ToolFactory> = { yield: s => new YieldTool(s), report_finding: () => reportFindingTool, report_tool_issue: s => createReportToolIssueTool(s), - exit_plan_mode: s => new ExitPlanModeTool(s), resolve: s => new ResolveTool(s), }; @@ -353,10 +356,6 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P const enableLsp = session.enableLsp ?? true; const requestedTools = toolNames && toolNames.length > 0 ? [...new Set(toolNames.map(name => name.toLowerCase()))] : undefined; - const planEnabled = session.settings.get("plan.enabled"); - if (planEnabled && requestedTools && !requestedTools.includes("exit_plan_mode")) { - requestedTools.push("exit_plan_mode"); - } const backends = resolveEvalBackends(session); const allowPython = backends.python; const allowJs = backends.js; @@ -428,7 +427,6 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P const allTools: Record<string, ToolFactory> = { ...BUILTIN_TOOLS, ...HIDDEN_TOOLS }; const isToolAllowed = (name: string) => { - if (name === "exit_plan_mode") return planEnabled; if (name === "lsp") return enableLsp && session.settings.get("lsp.enabled"); if (name === "bash") return true; if (name === "eval") return allowEval; @@ -478,7 +476,6 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P .filter(([name]) => isToolAllowed(name)) .map(([name, factory]) => [name, factory] as const), ...(includeYield ? ([["yield", HIDDEN_TOOLS.yield]] as const) : []), - ...(planEnabled ? ([["exit_plan_mode", HIDDEN_TOOLS.exit_plan_mode]] as const) : []), ]; const baseResults = await Promise.all( @@ -488,8 +485,7 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P }), ); const tools = baseResults.filter((r): r is Tool => r !== null); - const hasDeferrableTools = tools.some(tool => tool.deferrable === true); - if (hasDeferrableTools && !tools.some(tool => tool.name === "resolve")) { + if (!tools.some(tool => tool.name === "resolve")) { const resolveTool = await logger.time("createTools:resolve", HIDDEN_TOOLS.resolve, session); if (resolveTool) { tools.push(wrapToolWithMetaNotice(resolveTool)); diff --git a/packages/coding-agent/src/tools/resolve.ts b/packages/coding-agent/src/tools/resolve.ts index 5395d3664..d954fda60 100644 --- a/packages/coding-agent/src/tools/resolve.ts +++ b/packages/coding-agent/src/tools/resolve.ts @@ -14,6 +14,12 @@ import { ToolError } from "./tool-errors"; const resolveSchema = Type.Object({ action: Type.Union([Type.Literal("apply"), Type.Literal("discard")]), reason: Type.String({ description: "reason for action", examples: ["approved by user"] }), + extra: Type.Optional( + Type.Record(Type.String(), Type.Unknown(), { + description: + 'Free-form metadata interpreted by the resolving tool (e.g. plan-mode approval requires `{ title: "<PLAN_TITLE>" }`).', + }), + ), }); type ResolveParams = Static<typeof resolveSchema>; @@ -21,17 +27,12 @@ type ResolveParams = Static<typeof resolveSchema>; export interface ResolveToolDetails { action: "apply" | "discard"; reason: string; + extra?: Record<string, unknown>; sourceToolName?: string; label?: string; sourceResultDetails?: unknown; } -function resolveReasonPreview(reason?: string): string | undefined { - const trimmed = reason?.trim(); - if (!trimmed) return undefined; - return truncateToWidth(trimmed, 72, Ellipsis.Omit); -} - /** * Queue a resolve-protocol handler on the tool-choice queue. Forces the next * LLM call to invoke the hidden `resolve` tool, wraps the caller's apply/reject @@ -47,49 +48,25 @@ export function queueResolveHandler( options: { label: string; sourceToolName: string; - apply(reason: string): Promise<AgentToolResult<unknown>>; - reject?(reason: string): Promise<AgentToolResult<unknown> | undefined>; + apply(reason: string, extra?: Record<string, unknown>): Promise<AgentToolResult<unknown>>; + reject?(reason: string, extra?: Record<string, unknown>): Promise<AgentToolResult<unknown> | undefined>; }, ): void { const queue = session.getToolChoiceQueue?.(); const forced = session.buildToolChoice?.("resolve"); if (!queue || !forced || typeof forced === "string") return; - const detailsFor = (params: ResolveParams): ResolveToolDetails => ({ - action: params.action, - reason: params.reason, - sourceToolName: options.sourceToolName, - label: options.label, - }); - queue.pushOnce(forced, { label: `pending-action:${options.sourceToolName}`, now: true, onRejected: () => "requeue", - onInvoked: async (input: unknown) => { - const params = input as ResolveParams; - const withResolveDetails = (result: AgentToolResult<unknown>): AgentToolResult<ResolveToolDetails> => ({ - ...result, - details: { - ...detailsFor(params), - ...(result.details != null ? { sourceResultDetails: result.details } : {}), - }, - }); - if (params.action === "apply") { - const result = await options.apply(params.reason); - return withResolveDetails(result); - } - if (params.action === "discard" && options.reject != null) { - const result = await options.reject(params.reason); - if (result != null) { - return withResolveDetails(result); - } - } - return { - content: [{ type: "text" as const, text: `Discarded: ${options.label}. Reason: ${params.reason}` }], - details: detailsFor(params), - }; - }, + onInvoked: async (input: unknown) => + runResolveInvocation(input as ResolveParams, { + sourceToolName: options.sourceToolName, + label: options.label, + apply: options.apply, + reject: options.reject, + }), }); session.steer?.({ @@ -103,6 +80,57 @@ export function queueResolveHandler( }); } +/** + * Shared invocation runner used by both queued (in-flight) handlers and + * standing handlers (e.g. plan-mode approval). Discriminates on action, + * routes through the caller's apply/reject, and wraps the resulting tool + * payload with `ResolveToolDetails` so the renderer and event-controller + * see a consistent shape. + */ +export async function runResolveInvocation( + params: ResolveParams, + options: { + sourceToolName: string; + label: string; + apply(reason: string, extra?: Record<string, unknown>): Promise<AgentToolResult<unknown>>; + reject?(reason: string, extra?: Record<string, unknown>): Promise<AgentToolResult<unknown> | undefined>; + }, +): Promise<AgentToolResult<ResolveToolDetails>> { + const baseDetails: ResolveToolDetails = { + action: params.action, + reason: params.reason, + sourceToolName: options.sourceToolName, + label: options.label, + ...(params.extra != null ? { extra: params.extra } : {}), + }; + if (params.action === "apply") { + const result = await options.apply(params.reason, params.extra); + return { + ...result, + details: { + ...baseDetails, + ...(result.details != null ? { sourceResultDetails: result.details } : {}), + }, + }; + } + if (params.action === "discard" && options.reject != null) { + const result = await options.reject(params.reason, params.extra); + if (result != null) { + return { + ...result, + details: { + ...baseDetails, + ...(result.details != null ? { sourceResultDetails: result.details } : {}), + }, + }; + } + } + return { + content: [{ type: "text" as const, text: `Discarded: ${options.label}. Reason: ${params.reason}` }], + details: baseDetails, + }; +} + export class ResolveTool implements AgentTool<typeof resolveSchema, ResolveToolDetails> { readonly name = "resolve"; readonly label = "Resolve"; @@ -112,10 +140,9 @@ export class ResolveTool implements AgentTool<typeof resolveSchema, ResolveToolD readonly strict = true; readonly intent = (args: Partial<ResolveParams>) => { if (args.action === "discard") { - return args.reason ? `discarding: ${args.reason}` : "aiscarding changes"; - } else { - return args.reason ? `accepting: ${args.reason}` : "accepting changes"; + return args.reason ? `discarding: ${args.reason}` : "discarding changes"; } + return args.reason ? `accepting: ${args.reason}` : "accepting changes"; }; constructor(private readonly session: ToolSession) { @@ -130,7 +157,7 @@ export class ResolveTool implements AgentTool<typeof resolveSchema, ResolveToolD _context?: AgentToolContext, ): Promise<AgentToolResult<ResolveToolDetails>> { return untilAborted(signal, async () => { - const invoker = this.session.peekQueueInvoker?.(); + const invoker = this.session.peekQueueInvoker?.() ?? this.session.peekStandingResolveHandler?.(); if (!invoker) { throw new ToolError("No pending action to resolve. Nothing to apply or discard."); } @@ -142,7 +169,8 @@ export class ResolveTool implements AgentTool<typeof resolveSchema, ResolveToolD export const resolveToolRenderer = { renderCall(args: ResolveParams, _options: RenderResultOptions, uiTheme: Theme): Component { - const reason = resolveReasonPreview(args.reason); + const reasonTrimmed = args.reason?.trim(); + const reason = reasonTrimmed ? truncateToWidth(reasonTrimmed, 72, Ellipsis.Omit) : undefined; const text = renderStatusLine( { icon: "pending", diff --git a/packages/coding-agent/test/interactive-mode-plan-review.test.ts b/packages/coding-agent/test/interactive-mode-plan-review.test.ts index 6d962af2f..9f13b3035 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -85,7 +85,7 @@ describe("InteractiveMode plan review rendering", () => { mode.planModePlanFilePath = planFilePath; vi.spyOn(mode, "showHookSelector").mockResolvedValue("Stay in plan mode"); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -100,7 +100,7 @@ describe("InteractiveMode plan review rendering", () => { mode.chatContainer.addChild(marker); await Bun.write(resolvedPlanPath, "# Second plan\n\nbeta"); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -129,7 +129,7 @@ describe("InteractiveMode plan review rendering", () => { mode.planModePlanFilePath = planFilePath; const selector = vi.spyOn(mode, "showHookSelector").mockResolvedValue("Stay in plan mode"); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -168,7 +168,7 @@ describe("InteractiveMode plan review rendering", () => { const clear = vi.spyOn(mode, "handleClearCommand").mockResolvedValue(); const prompt = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -197,7 +197,7 @@ describe("InteractiveMode plan review rendering", () => { const clear = vi.spyOn(mode, "handleClearCommand").mockResolvedValue(); const prompt = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -226,7 +226,7 @@ describe("InteractiveMode plan review rendering", () => { const markSentSpy = vi.spyOn(session, "markPlanReferenceSent"); const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -272,7 +272,7 @@ describe("InteractiveMode plan review rendering", () => { const markSentSpy = vi.spyOn(session, "markPlanReferenceSent"); const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -313,7 +313,7 @@ describe("InteractiveMode plan review rendering", () => { const markSentSpy = vi.spyOn(session, "markPlanReferenceSent"); const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -351,7 +351,7 @@ describe("InteractiveMode plan review rendering", () => { return "ok"; }); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -368,14 +368,14 @@ describe("InteractiveMode plan review rendering", () => { // ========================================================================== // Phase 6 — B layer: #approvePlan flag lifecycle via try/finally. // - // Drives `handleExitPlanModeTool` with each CompactionOutcome variant and + // Drives `handlePlanApproval` with each CompactionOutcome variant and // asserts `session.isPlanCompactAbortPending === false` after `#approvePlan` // resolves/rejects. The flag is the only state that can leak into later // unrelated aborts; the `try/finally` in `#approvePlan` is what protects it. // ========================================================================== /** - * Drives `handleExitPlanModeTool` with the "Approve and compact context" + * Drives `handlePlanApproval` with the "Approve and compact context" * picker outcome and the given compaction-outcome mock. Returns the promise * the harness produces so the caller decides between `await` (B1-B3 happy * paths) and `expect(...).rejects` (B4 throw path). Does NOT swallow errors. @@ -402,7 +402,7 @@ describe("InteractiveMode plan review rendering", () => { } vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", @@ -429,8 +429,8 @@ describe("InteractiveMode plan review rendering", () => { }); it("B4: Approve and compact context + handleCompactCommand throws → showError surfaces the failure AND flag cleared by finally before the outer catch", async () => { - // `handleExitPlanModeTool` wraps `#approvePlan` in a try/catch - // (interactive-mode.ts:1287) that consumes the throw and reports via + // `handlePlanApproval` wraps `#approvePlan` in a try/catch + // in `InteractiveMode` that consumes the throw and reports via // `showError`. The contract under test is: // 1. `#approvePlan`'s own `try/finally` clears the flag BEFORE the // throw bubbles up to that outer catch. @@ -456,7 +456,7 @@ describe("InteractiveMode plan review rendering", () => { const markSpy = vi.spyOn(session, "markPlanCompactAbortPending"); vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); - await mode.handleExitPlanModeTool({ + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN", diff --git a/packages/coding-agent/test/tools/exit-plan-mode.test.ts b/packages/coding-agent/test/tools/exit-plan-mode.test.ts deleted file mode 100644 index 28fe15dd7..000000000 --- a/packages/coding-agent/test/tools/exit-plan-mode.test.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; -import * as fs from "node:fs/promises"; -import * as os from "node:os"; -import * as path from "node:path"; -import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; -import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; -import { ExitPlanModeTool } from "@oh-my-pi/pi-coding-agent/tools/exit-plan-mode"; - -describe("ExitPlanModeTool", () => { - let tmpDir: string; - let artifactsDir: string; - - beforeEach(async () => { - tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "exit-plan-mode-")); - artifactsDir = path.join(tmpDir, "artifacts"); - await fs.mkdir(path.join(artifactsDir, "local"), { recursive: true }); - await Bun.write(path.join(artifactsDir, "local", "PLAN.md"), "# Plan\n"); - }); - - afterEach(async () => { - await fs.rm(tmpDir, { recursive: true, force: true }); - }); - - function createSession(overrides: Partial<ToolSession> = {}): ToolSession { - return { - cwd: tmpDir, - hasUI: false, - getSessionFile: () => null, - getSessionSpawns: () => "*", - settings: Settings.isolated(), - getArtifactsDir: () => artifactsDir, - getSessionId: () => "session-a", - getPlanModeState: () => ({ enabled: true, planFilePath: "local://PLAN.md" }), - ...overrides, - }; - } - - it("normalizes title to .md final plan path", async () => { - const tool = new ExitPlanModeTool(createSession()); - const result = await tool.execute("call-1", { title: "WP_MIGRATION_PLAN" }); - - expect(result.details?.planFilePath).toBe("local://PLAN.md"); - expect(result.details?.title).toBe("WP_MIGRATION_PLAN"); - expect(result.details?.finalPlanFilePath).toBe("local://WP_MIGRATION_PLAN.md"); - expect(result.details?.planExists).toBe(true); - }); - - it("accepts explicit .md suffix in title", async () => { - const tool = new ExitPlanModeTool(createSession()); - const result = await tool.execute("call-2", { title: "WP_MIGRATION_PLAN.md" }); - expect(result.details?.title).toBe("WP_MIGRATION_PLAN"); - expect(result.details?.finalPlanFilePath).toBe("local://WP_MIGRATION_PLAN.md"); - }); - - it("fails early when the draft plan file was never written", async () => { - await fs.rm(path.join(artifactsDir, "local", "PLAN.md"), { force: true }); - const tool = new ExitPlanModeTool(createSession()); - - await expect(tool.execute("call-missing", { title: "WP_MIGRATION_PLAN" })).rejects.toThrow( - "Plan file not found at local://PLAN.md. Write the finalized plan to local://PLAN.md before calling exit_plan_mode.", - ); - }); - - it("rejects invalid title characters", async () => { - const tool = new ExitPlanModeTool(createSession()); - await expect(tool.execute("call-3", { title: "../bad" })).rejects.toThrow( - "Title must not contain path separators or '..'.", - ); - await expect(tool.execute("call-4", { title: "bad name" })).rejects.toThrow( - "Title may only contain letters, numbers, underscores, or hyphens.", - ); - }); -}); diff --git a/packages/coding-agent/test/tools/index.test.ts b/packages/coding-agent/test/tools/index.test.ts index ce295ed08..2fb854f4a 100644 --- a/packages/coding-agent/test/tools/index.test.ts +++ b/packages/coding-agent/test/tools/index.test.ts @@ -64,7 +64,7 @@ describe("createTools", () => { expect(names).toContain("task"); expect(names).toContain("todo_write"); expect(names).toContain("web_search"); - expect(names).toContain("exit_plan_mode"); + expect(names).toContain("resolve"); expect(names).not.toContain("fetch"); expect(names).not.toContain("vim"); }); @@ -123,7 +123,7 @@ describe("createTools", () => { const names = tools.map(t => t.name); expect(names).toContain("eval"); - expect(names).toContain("exit_plan_mode"); + expect(names).toContain("resolve"); }); it("excludes lsp tool when session disables LSP", async () => { @@ -131,7 +131,7 @@ describe("createTools", () => { const tools = await createTools(session, ["read", "lsp", "write"]); const names = tools.map(t => t.name); - expect(names).toEqual(["read", "write", "exit_plan_mode"]); + expect(names).toEqual(["read", "write", "resolve"]); }); it("excludes lsp tool when disabled", async () => { @@ -147,7 +147,7 @@ describe("createTools", () => { const tools = await createTools(session, ["read", "write"]); const names = tools.map(t => t.name); - expect(names).toEqual(["read", "write", "exit_plan_mode"]); + expect(names).toEqual(["read", "write", "resolve"]); }); it("ignores vim as an unknown requested tool even when vim edit mode is active", async () => { @@ -159,7 +159,7 @@ describe("createTools", () => { const tools = await createTools(session, ["read", "vim"]); const names = tools.map(t => t.name); - expect(names).toEqual(["read", "exit_plan_mode"]); + expect(names).toEqual(["read", "resolve"]); }); it("lowercases requested tool subset", async () => { @@ -167,7 +167,7 @@ describe("createTools", () => { const tools = await createTools(session, ["Read", "Write"]); const names = tools.map(t => t.name); - expect(names).toEqual(["read", "write", "exit_plan_mode"]); + expect(names).toEqual(["read", "write", "resolve"]); }); it("includes hidden tools when explicitly requested", async () => { @@ -175,7 +175,7 @@ describe("createTools", () => { const tools = await createTools(session, ["report_finding"]); const names = tools.map(t => t.name); - expect(names).toEqual(["report_finding", "exit_plan_mode"]); + expect(names).toEqual(["report_finding", "resolve"]); }); it("includes yield tool when required", async () => { @@ -230,7 +230,7 @@ describe("createTools", () => { expect(names).not.toContain("calc"); }); - it("excludes exit_plan_mode when plan mode is disabled", async () => { + it("always includes resolve regardless of plan-mode setting", async () => { const session = createTestSession({ settings: createSettingsWithOverrides({ "plan.enabled": false, @@ -238,10 +238,11 @@ describe("createTools", () => { }); const defaultTools = await createTools(session); + expect(defaultTools.map(t => t.name)).toContain("resolve"); expect(defaultTools.map(t => t.name)).not.toContain("exit_plan_mode"); - const requestedTools = await createTools(session, ["read", "exit_plan_mode"]); - expect(requestedTools.map(t => t.name)).toEqual(["read"]); + const requestedTools = await createTools(session, ["read"]); + expect(requestedTools.map(t => t.name)).toEqual(["read", "resolve"]); }); it("includes search_tool_bm25 when MCP tool discovery is enabled and executable", async () => { @@ -258,12 +259,6 @@ describe("createTools", () => { }); it("HIDDEN_TOOLS contains review tools", () => { - expect(Object.keys(HIDDEN_TOOLS).sort()).toEqual([ - "exit_plan_mode", - "report_finding", - "report_tool_issue", - "resolve", - "yield", - ]); + expect(Object.keys(HIDDEN_TOOLS).sort()).toEqual(["report_finding", "report_tool_issue", "resolve", "yield"]); }); });