diff --git a/docs/custom-tools.md b/docs/custom-tools.md index b5f672a79..067c6620f 100644 --- a/docs/custom-tools.md +++ b/docs/custom-tools.md @@ -122,6 +122,7 @@ From `types.ts` and `loader.ts`: - `logger`: shared file logger - `typebox`: injected `@sinclair/typebox` - `pi`: injected `@oh-my-pi/pi-coding-agent` exports +- `pushPendingAction(action)`: register a preview action for hidden `resolve` tool (`docs/resolve-tool-runtime.md`) Loader starts with a no-op UI context and requires host code to call `setUIContext(...)` when real UI is ready. diff --git a/docs/resolve-tool-runtime.md b/docs/resolve-tool-runtime.md new file mode 100644 index 000000000..6cb497cac --- /dev/null +++ b/docs/resolve-tool-runtime.md @@ -0,0 +1,110 @@ +# Resolve tool runtime internals + +This document explains how preview/apply workflows are modeled in coding-agent and how custom tools can participate via `pushPendingAction`. + +## Scope and key files + +- [`src/tools/resolve.ts`](../packages/coding-agent/src/tools/resolve.ts) +- [`src/tools/pending-action.ts`](../packages/coding-agent/src/tools/pending-action.ts) +- [`src/tools/ast-edit.ts`](../packages/coding-agent/src/tools/ast-edit.ts) +- [`src/extensibility/custom-tools/types.ts`](../packages/coding-agent/src/extensibility/custom-tools/types.ts) +- [`src/extensibility/custom-tools/loader.ts`](../packages/coding-agent/src/extensibility/custom-tools/loader.ts) +- [`src/sdk.ts`](../packages/coding-agent/src/sdk.ts) + +## What `resolve` does + +`resolve` is a hidden tool that finalizes a pending preview action. + +- `action: "apply"` executes the pending action callback and persists changes. +- `action: "discard"` drops the pending action without applying. + +If no pending action exists, `resolve` fails with: + +- `No pending action to resolve. Nothing to apply or discard.` + +## Pending actions are a stack (LIFO) + +Pending actions are stored in `PendingActionStore` as a push/pop stack: + +- `push(action)` adds a new pending action on top. +- `peek()` inspects the current top action. +- `pop()` removes and returns the top action. +- `hasPending` indicates whether the stack is non-empty. + +`resolve` always consumes the **topmost** pending action first (`pop()`), so multiple preview-producing tools resolve in reverse order of registration. + +## Built-in producer example (`ast_edit`) + +`ast_edit` previews structural replacements first. When the preview has replacements and is not applied yet, it pushes a pending action that contains: + +- label (human-readable summary) +- `sourceToolName` (`ast_edit`) +- `apply()` callback that reruns AST edit with `dryRun: false` + +`resolve(action="apply")` later executes this callback. + +## Custom tools: `pushPendingAction` + +Custom tools can register resolve-compatible pending actions through `CustomToolAPI.pushPendingAction(...)`. + +`CustomToolPendingAction`: + +- `label: string` (required) +- `apply(): Promise>` (required) +- `details?: unknown` (optional) +- `sourceToolName?: string` (optional, defaults to `"custom_tool"`) + +### Minimal usage example + +```ts +import type { CustomToolFactory } from "@oh-my-pi/pi-coding-agent"; + +const factory: CustomToolFactory = pi => ({ + name: "batch_rename_preview", + label: "Batch Rename Preview", + description: "Previews renames and defers commit to resolve", + parameters: pi.typebox.Type.Object({ + files: pi.typebox.Type.Array(pi.typebox.Type.String()), + }), + + async execute(_toolCallId, params) { + const previewSummary = `Prepared rename plan for ${params.files.length} files`; + + pi.pushPendingAction({ + label: `Batch rename: ${params.files.length} files`, + sourceToolName: "batch_rename_preview", + apply: async () => { + // apply writes here + return { + content: [{ type: "text", text: "Applied batch rename." }], + }; + }, + }); + + return { + content: [{ type: "text", text: `${previewSummary}. Call resolve to apply or discard.` }], + }; + }, +}); + +export default factory; +``` + +## Runtime availability and failures + +`pushPendingAction` is wired by the custom tool loader using the active session `PendingActionStore`. + +If the runtime has no pending-action store, `pushPendingAction` throws: + +- `Pending action store unavailable for custom tools in this runtime.` + +## Tool-choice behavior + +When `PendingActionStore.hasPending` is true, the agent runtime biases tool choice to `resolve` so pending previews are explicitly finalized before normal tool flow continues. + +## Developer guidance + +- Use pending actions only for destructive or high-impact operations that should support explicit apply/discard. +- Keep `label` concise and specific; it is shown in resolve renderer output. +- Ensure `apply()` is deterministic and idempotent enough for one-shot execution. +- If your tool can stage multiple previews, remember LIFO semantics: latest pushed action resolves first. diff --git a/packages/agent/src/types.ts b/packages/agent/src/types.ts index d7203d9d5..04d5c1dd1 100644 --- a/packages/agent/src/types.ts +++ b/packages/agent/src/types.ts @@ -229,6 +229,8 @@ export interface AgentTool Tool | null | Promise - feature toggles (`find.enabled`, `grep.enabled`, etc.) - recursion guard for `task` (`task.maxRecursionDepth` vs `session.taskDepth`) - submit-result mode (`requireSubmitResultTool`) and `todo_write` suppression -5. Instantiates tools in parallel with `Promise.all`, records slow factory timings when `PI_TIMING=1`. -6. Wraps every tool with `wrapToolWithMetaNotice` before returning. +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). The wrapper step is not cosmetic: it enforces uniform meta-notice behavior and normalized error rendering across all tools. @@ -1131,14 +1131,15 @@ Primary file: `packages/coding-agent/src/tools/index.ts`. - `export const BUILTIN_TOOLS: Record = { ... }` - 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: `submit_result`, `report_finding`, `exit_plan_mode`. + - Existing hidden names: `submit_result`, `report_finding`, `exit_plan_mode`, `resolve`. 5. Wire feature gates in `isToolAllowed(name)` when the tool needs runtime enable/disable behavior. - Existing gates use `session.settings.get(".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()` automatically injects `exit_plan_mode` when `toolNames` are specified. +- `createTools()` always injects `exit_plan_mode` when `toolNames` are specified. +- `resolve` is included only when at least one active tool is marked `deferrable: true` (built-in or extension/custom). - `submit_result` is force-added when `session.requireSubmitResultTool === true`. - Python/Bash availability is mode-driven (`PI_PY`, `python.toolMode`) and can auto-fallback to bash. diff --git a/packages/coding-agent/examples/sdk/README.md b/packages/coding-agent/examples/sdk/README.md index 6a7b75d64..2a443ee78 100644 --- a/packages/coding-agent/examples/sdk/README.md +++ b/packages/coding-agent/examples/sdk/README.md @@ -45,7 +45,9 @@ import { ModelRegistry, SessionManager, BUILTIN_TOOLS, + HIDDEN_TOOLS, createTools, + ResolveTool, } from "@oh-my-pi/pi-coding-agent"; // Auth and models setup @@ -104,6 +106,26 @@ session.subscribe((event) => { await session.prompt("Hello"); ``` +## Resolve preview workflow (AST edit apply/discard) + +`ast_edit` now always returns a preview. To finalize, call hidden `resolve` with a required reason. + +- `action: "apply"` → commit pending preview changes +- `action: "discard"` → drop pending preview changes +- `reason: string` is required for both paths + +`createAgentSession()` / `createTools()` include `resolve` automatically, even when filtering `toolNames`. +If you are composing tools manually, use `HIDDEN_TOOLS.resolve` (or `ResolveTool`) and wire the same `pendingActionStore`. + +```typescript +const tools = await createTools(toolSession, ["ast_edit"]); // resolve is auto-included +const resolveTool = tools.find(t => t.name === "resolve") as ResolveTool; + +await resolveTool.execute("call-1", { + action: "apply", + reason: "Preview matches expected replacements", +}); +``` ## Options | Option | Default | Description | diff --git a/packages/coding-agent/src/extensibility/custom-tools/loader.ts b/packages/coding-agent/src/extensibility/custom-tools/loader.ts index cc943c7d1..283754b32 100644 --- a/packages/coding-agent/src/extensibility/custom-tools/loader.ts +++ b/packages/coding-agent/src/extensibility/custom-tools/loader.ts @@ -14,6 +14,7 @@ import type { ExecOptions } from "../../exec/exec"; import { execCommand } from "../../exec/exec"; import type { HookUIContext } from "../../extensibility/hooks/types"; import { getAllPluginToolPaths } from "../../extensibility/plugins/loader"; +import type { PendingActionStore } from "../../tools/pending-action"; import { createNoOpUIContext, resolvePath } from "../utils"; import type { CustomToolAPI, CustomToolFactory, LoadedCustomTool, ToolLoadError } from "./types"; @@ -84,7 +85,7 @@ export class CustomToolLoader { #sharedApi: CustomToolAPI; #seenNames: Set; - constructor(cwd: string, builtInToolNames: string[]) { + constructor(cwd: string, builtInToolNames: string[], pendingActionStore?: PendingActionStore) { this.#sharedApi = { cwd, exec: (command: string, args: string[], options?: ExecOptions) => @@ -94,6 +95,17 @@ export class CustomToolLoader { logger, typebox, pi: piCodingAgent, + pushPendingAction: action => { + if (!pendingActionStore) { + throw new Error("Pending action store unavailable for custom tools in this runtime."); + } + pendingActionStore.push({ + label: action.label, + sourceToolName: action.sourceToolName ?? "custom_tool", + apply: action.apply, + details: action.details, + }); + }, }; this.#seenNames = new Set(builtInToolNames); } @@ -138,8 +150,13 @@ export class CustomToolLoader { * @param cwd - Current working directory for resolving relative paths * @param builtInToolNames - Names of built-in tools to check for conflicts */ -export async function loadCustomTools(pathsWithSources: ToolPathWithSource[], cwd: string, builtInToolNames: string[]) { - const loader = new CustomToolLoader(cwd, builtInToolNames); +export async function loadCustomTools( + pathsWithSources: ToolPathWithSource[], + cwd: string, + builtInToolNames: string[], + pendingActionStore?: PendingActionStore, +) { + const loader = new CustomToolLoader(cwd, builtInToolNames, pendingActionStore); await loader.load(pathsWithSources); return { tools: loader.tools, @@ -160,7 +177,12 @@ export async function loadCustomTools(pathsWithSources: ToolPathWithSource[], cw * @param cwd - Current working directory * @param builtInToolNames - Names of built-in tools to check for conflicts */ -export async function discoverAndLoadCustomTools(configuredPaths: string[], cwd: string, builtInToolNames: string[]) { +export async function discoverAndLoadCustomTools( + configuredPaths: string[], + cwd: string, + builtInToolNames: string[], + pendingActionStore?: PendingActionStore, +) { const allPathsWithSources: ToolPathWithSource[] = []; const seen = new Set(); @@ -193,5 +215,5 @@ export async function discoverAndLoadCustomTools(configuredPaths: string[], cwd: addPath(resolvePath(configPath, cwd), { provider: "config", providerName: "Config", level: "project" }); } - return loadCustomTools(allPathsWithSources, cwd, builtInToolNames); + return loadCustomTools(allPathsWithSources, cwd, builtInToolNames, pendingActionStore); } diff --git a/packages/coding-agent/src/extensibility/custom-tools/types.ts b/packages/coding-agent/src/extensibility/custom-tools/types.ts index 5be0f3c9a..e06a9e703 100644 --- a/packages/coding-agent/src/extensibility/custom-tools/types.ts +++ b/packages/coding-agent/src/extensibility/custom-tools/types.ts @@ -26,6 +26,18 @@ export type { AgentToolResult, AgentToolUpdateCallback }; // Re-export for backward compatibility export type { ExecOptions, ExecResult } from "../../exec/exec"; +/** Pending action entry consumed by the hidden resolve tool */ +export interface CustomToolPendingAction { + /** Human-readable preview label shown in resolve flow */ + label: string; + /** Apply callback invoked when resolve(action="apply") is called */ + apply(): Promise>; + /** Optional details metadata stored with the pending action */ + details?: unknown; + /** Optional source tool name shown by resolve renderer (defaults to "custom_tool") */ + sourceToolName?: string; +} + /** API passed to custom tool factory (stable across session changes) */ export interface CustomToolAPI { /** Current working directory */ @@ -42,6 +54,8 @@ export interface CustomToolAPI { typebox: typeof import("@sinclair/typebox"); /** Injected pi-coding-agent exports */ pi: typeof import("../.."); + /** Push a preview action that can later be resolved with the hidden resolve tool */ + pushPendingAction(action: CustomToolPendingAction): void; } /** @@ -162,7 +176,8 @@ export interface CustomTool { parameters: TParams; /** If true, tool is excluded unless explicitly listed in --tools or agent's tools field */ hidden?: boolean; - + /** If true, tool may stage deferred changes that require explicit resolve/discard. */ + deferrable?: boolean; /** * Execute the tool. * @param toolCallId - Unique ID for this tool call diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index 5976ca3ce..6431d71b6 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -290,7 +290,8 @@ export interface ToolDefinition tool.deferrable === true); + if (!hasDeferrableTools) { + toolRegistry.delete("resolve"); + } else if (!toolRegistry.has("resolve")) { + const resolveTool = await logger.timeAsync("createTools:resolve:session", HIDDEN_TOOLS.resolve, toolSession); + if (resolveTool) { + toolRegistry.set(resolveTool.name, wrapToolWithMetaNotice(resolveTool) as AgentTool); + } + } + let cursorEventEmitter: ((event: AgentEvent) => void) | undefined; const cursorExecHandlers = new CursorExecHandlers({ cwd, diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index a8d5a5f04..b03e04d3f 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -220,9 +220,6 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P if (requestedTools && !requestedTools.includes("exit_plan_mode")) { requestedTools.push("exit_plan_mode"); } - if (requestedTools && !requestedTools.includes("resolve")) { - requestedTools.push("resolve"); - } const pythonMode = getPythonModeFromEnv() ?? session.settings.get("python.toolMode"); const skipPythonPreflight = session.skipPythonPreflight === true; let pythonAvailable = true; @@ -302,25 +299,32 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P } const filteredRequestedTools = requestedTools?.filter(name => name in allTools && isToolAllowed(name)); - - const entries = + const baseEntries = filteredRequestedTools !== undefined - ? filteredRequestedTools.map(name => [name, allTools[name]] as const) + ? filteredRequestedTools.filter(name => name !== "resolve").map(name => [name, allTools[name]] as const) : [ ...Object.entries(BUILTIN_TOOLS).filter(([name]) => isToolAllowed(name)), ...(includeSubmitResult ? ([["submit_result", HIDDEN_TOOLS.submit_result]] as const) : []), ...([["exit_plan_mode", HIDDEN_TOOLS.exit_plan_mode]] as const), - ...([["resolve", HIDDEN_TOOLS.resolve]] as const), ]; - const results = await Promise.all( - entries.map(async ([name, factory]) => { - if (filteredRequestedTools && !filteredRequestedTools.includes(name)) { - return null; - } + const baseResults = await Promise.all( + baseEntries.map(async ([name, factory]) => { const tool = await logger.timeAsync(`createTools:${name}`, factory, session); return tool ? wrapToolWithMetaNotice(tool) : null; }), ); - return results.filter((r): r is Tool => r !== null); + const tools = baseResults.filter((r): r is Tool => r !== null); + const hasDeferrableTools = tools.some(tool => tool.deferrable === true); + if (!hasDeferrableTools) { + return tools; + } + if (tools.some(tool => tool.name === "resolve")) { + return tools; + } + const resolveTool = await logger.timeAsync("createTools:resolve", HIDDEN_TOOLS.resolve, session); + if (resolveTool) { + tools.push(wrapToolWithMetaNotice(resolveTool)); + } + return tools; } diff --git a/packages/coding-agent/src/tools/pending-action.ts b/packages/coding-agent/src/tools/pending-action.ts index 8b2743507..5a39e5bc5 100644 --- a/packages/coding-agent/src/tools/pending-action.ts +++ b/packages/coding-agent/src/tools/pending-action.ts @@ -8,21 +8,25 @@ export interface PendingAction { } export class PendingActionStore { - #action: PendingAction | null = null; + #actions: PendingAction[] = []; - set(action: PendingAction): void { - this.#action = action; + push(action: PendingAction): void { + this.#actions.push(action); } - get(): PendingAction | null { - return this.#action; + peek(): PendingAction | null { + return this.#actions.at(-1) ?? null; + } + + pop(): PendingAction | null { + return this.#actions.pop() ?? null; } clear(): void { - this.#action = null; + this.#actions = []; } get hasPending(): boolean { - return this.#action !== null; + return this.#actions.length > 0; } } diff --git a/packages/coding-agent/src/tools/resolve.ts b/packages/coding-agent/src/tools/resolve.ts index 0928d69ef..bb4dd64c8 100644 --- a/packages/coding-agent/src/tools/resolve.ts +++ b/packages/coding-agent/src/tools/resolve.ts @@ -58,13 +58,10 @@ export class ResolveTool implements AgentTool resolved" : "proposed -> rejected", + color: args.action === "apply" ? "success" : "warning", + }, meta: reason ? [uiTheme.fg("muted", reason)] : undefined, }, uiTheme, diff --git a/packages/coding-agent/test/tools/ast-edit.test.ts b/packages/coding-agent/test/tools/ast-edit.test.ts index d7f3cdde3..371224a7c 100644 --- a/packages/coding-agent/test/tools/ast-edit.test.ts +++ b/packages/coding-agent/test/tools/ast-edit.test.ts @@ -103,7 +103,7 @@ describe("ast_edit tool schema", () => { expect(previewResult.details).toBeDefined(); expect((previewResult.details as { applied?: boolean }).applied).toBe(false); - const pending = pendingActionStore.get(); + const pending = pendingActionStore.peek(); expect(pending).not.toBeNull(); if (!pending) throw new Error("Expected pending action to be registered"); expect(pending.sourceToolName).toBe("ast_edit"); diff --git a/packages/coding-agent/test/tools/index.test.ts b/packages/coding-agent/test/tools/index.test.ts index cba7f7395..0d8b56519 100644 --- a/packages/coding-agent/test/tools/index.test.ts +++ b/packages/coding-agent/test/tools/index.test.ts @@ -131,6 +131,11 @@ describe("createTools", () => { }); it("HIDDEN_TOOLS contains review tools", () => { - expect(Object.keys(HIDDEN_TOOLS).sort()).toEqual(["exit_plan_mode", "report_finding", "submit_result"]); + expect(Object.keys(HIDDEN_TOOLS).sort()).toEqual([ + "exit_plan_mode", + "report_finding", + "resolve", + "submit_result", + ]); }); }); diff --git a/packages/coding-agent/test/tools/resolve.test.ts b/packages/coding-agent/test/tools/resolve.test.ts index a7caa7b11..92b82fd33 100644 --- a/packages/coding-agent/test/tools/resolve.test.ts +++ b/packages/coding-agent/test/tools/resolve.test.ts @@ -37,7 +37,7 @@ describe("ResolveTool", () => { it("discards pending action and clears store", async () => { const pendingActionStore = new PendingActionStore(); - pendingActionStore.set({ + pendingActionStore.push({ label: "AST Edit: 2 replacements in 1 file", sourceToolName: "ast_edit", apply: async () => ({ content: [{ type: "text", text: "should not run" }] }), @@ -62,7 +62,7 @@ describe("ResolveTool", () => { it("applies pending action and clears store", async () => { const pendingActionStore = new PendingActionStore(); let applied = false; - pendingActionStore.set({ + pendingActionStore.push({ label: "AST Edit: 1 replacement in 1 file", sourceToolName: "ast_edit", apply: async () => { @@ -87,6 +87,41 @@ describe("ResolveTool", () => { label: "AST Edit: 1 replacement in 1 file", }); }); + + it("resolves pending actions in LIFO order", async () => { + const pendingActionStore = new PendingActionStore(); + let firstApplied = false; + let secondApplied = false; + + pendingActionStore.push({ + label: "First action", + sourceToolName: "ast_edit", + apply: async () => { + firstApplied = true; + return { content: [{ type: "text", text: "first" }] }; + }, + }); + pendingActionStore.push({ + label: "Second action", + sourceToolName: "ast_edit", + apply: async () => { + secondApplied = true; + return { content: [{ type: "text", text: "second" }] }; + }, + }); + + const tool = new ResolveTool(createSession(pendingActionStore)); + const firstResult = await tool.execute("call-apply-1", { action: "apply", reason: "apply top" }); + expect(getText(firstResult)).toContain("second"); + expect(firstApplied).toBe(false); + expect(secondApplied).toBe(true); + expect(pendingActionStore.hasPending).toBe(true); + + const secondResult = await tool.execute("call-apply-2", { action: "apply", reason: "apply next" }); + expect(getText(secondResult)).toContain("first"); + expect(firstApplied).toBe(true); + expect(pendingActionStore.hasPending).toBe(false); + }); }); it("renders a highlighted apply summary", async () => { diff --git a/packages/tui/test/loader.test.ts b/packages/tui/test/loader.test.ts index 150eef91e..06b35dcfa 100644 --- a/packages/tui/test/loader.test.ts +++ b/packages/tui/test/loader.test.ts @@ -8,7 +8,13 @@ describe("Loader component", () => { it("clamps rendered lines to terminal width", async () => { const term = new VirtualTerminal(1, 4); const tui = new TUI(term); - const loader = new Loader(tui, text => text, text => text, "Checking", ["⠸"]); + const loader = new Loader( + tui, + text => text, + text => text, + "Checking", + ["⠸"], + ); tui.addChild(loader); tui.start();