From ed2fd6e3f37467d472475e75a018831dee00be06 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 14 Apr 2026 10:46:15 +0200 Subject: [PATCH] feat(coding-agent): added edit tool consolidation via vim handlers - Removed the standalone vim tool and normalized built-in/requested tooling to edit. - Updated session and SDK tool activation to dedupe lowercase names and track edit state via the edit key. - Added vim-mode argument detection and delegated edit rendering/execution into Vim handlers under edit. - Updated Vim step handling to auto-reorder numeric-positioned commands, including cc/C/S/s/i/I/A cases. - Renamed prompt/changelog text and test expectations to reflect edit-only tool naming and usage. --- packages/coding-agent/CHANGELOG.md | 3 + packages/coding-agent/src/edit/index.ts | 64 ++++++++++++++----- packages/coding-agent/src/edit/renderer.ts | 39 +++++++++++ .../src/modes/components/tool-execution.ts | 3 +- .../coding-agent/src/prompts/tools/vim.md | 4 +- packages/coding-agent/src/sdk.ts | 32 ++-------- .../coding-agent/src/session/agent-session.ts | 36 ++--------- packages/coding-agent/src/tools/index.ts | 17 ++--- packages/coding-agent/src/tools/renderers.ts | 2 - packages/coding-agent/src/tools/vim.ts | 62 +++++++++--------- packages/coding-agent/src/utils/edit-mode.ts | 35 ---------- .../agent-session-message-pipeline.test.ts | 2 +- .../test/sdk-tool-activation.test.ts | 24 +++---- packages/coding-agent/test/tools.test.ts | 9 +-- .../coding-agent/test/tools/index.test.ts | 12 ++-- packages/coding-agent/test/tools/vim.test.ts | 2 +- scripts/edit_benchmark_common.py | 10 +-- scripts/vim-edit-benchmark.py | 10 +-- 18 files changed, 177 insertions(+), 189 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c7203cf31..879a0542e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,8 +1,10 @@ # Changelog ## [Unreleased] + ### Breaking Changes +- Removed the standalone `vim` tool from built-in tool lists, so vim-style editing is now invoked through `edit` in `vim` mode - Removed the `searchDb` field from session and extension tool contexts, so custom tools and extensions no longer receive a shared native search DB handle from `ToolSession`, `CustomToolContext`, `ExtensionContext`, and `CreateAgentSessionOptions` - Changed the `vim` tool API to require either `open: "path"` or `kbd: [...]` per call and removed direct `line`/`col` cursor parameters from `open`, so callers must position the cursor via key sequences after opening - Changed the `edit` schemas for patch, replace, hashline, and chunk modes from top-level request fields to `edits` array entries, requiring path/mode details on each edit and breaking callers that send legacy top-level `path`, `old_text`, `new_text`, `op`, `move`, or `delete` payloads @@ -54,6 +56,7 @@ ### Fixed +- Fixed vim-mode multi-step line edits by auto-reordering ascending line-positioned commands to descending order before execution - Fixed Vim viewport rendering to display the inline highlighted cursor character and keep long cursor lines centered around the cursor in tool previews - Fixed Vim `:global` command defaults to handle only supported subcommands and report unsupported ones explicitly - Fixed Vim ex execution so parsed `:update`, `:yank`, and `:put` commands now run instead of falling through diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index a1091cd2e..c658be514 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -1,5 +1,6 @@ import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import { prompt } from "@oh-my-pi/pi-utils"; +import type { Static } from "@sinclair/typebox"; import { createLspWritethrough, type FileDiagnosticsResult, @@ -12,7 +13,9 @@ import hashlineDescription from "../prompts/tools/hashline.md" with { type: "tex import patchDescription from "../prompts/tools/patch.md" with { type: "text" }; import replaceDescription from "../prompts/tools/replace.md" with { type: "text" }; import type { ToolSession } from "../tools"; -import { DEFAULT_EDIT_MODE, type EditMode, normalizeEditMode, resolveEditMode } from "../utils/edit-mode"; +import { VimTool, vimSchema } from "../tools/vim"; +import { type EditMode, normalizeEditMode, resolveEditMode } from "../utils/edit-mode"; +import type { VimToolDetails } from "../vim/types"; import { type ChunkParams, type ChunkToolEdit, @@ -60,10 +63,12 @@ type TInput = | typeof replaceEditSchema | typeof patchEditSchema | typeof hashlineEditParamsSchema - | typeof chunkEditParamsSchema; + | typeof chunkEditParamsSchema + | typeof vimSchema; -type EditParams = ReplaceParams | PatchParams | HashlineParams | ChunkParams; -type EditToolMode = Exclude; +type VimParams = Static; +type EditParams = ReplaceParams | PatchParams | HashlineParams | ChunkParams | VimParams; +type EditToolResultDetails = EditToolDetails | VimToolDetails; type EditModeDefinition = { description: (session: ToolSession) => string; @@ -75,11 +80,11 @@ type EditModeDefinition = { params: EditParams, signal: AbortSignal | undefined, batchRequest: LspBatchRequest | undefined, - onUpdate?: (partialResult: AgentToolResult) => void, - ) => Promise>; + onUpdate?: (partialResult: AgentToolResult) => void, + ) => Promise>; }; -function resolveConfiguredEditMode(rawEditMode: string): EditToolMode | undefined { +function resolveConfiguredEditMode(rawEditMode: string): EditMode | undefined { if (!rawEditMode || rawEditMode === "auto") { return undefined; } @@ -88,13 +93,14 @@ function resolveConfiguredEditMode(rawEditMode: string): EditToolMode | undefine if (!editMode) { throw new Error(`Invalid PI_EDIT_VARIANT: ${rawEditMode}`); } - if (editMode === "vim") { - return undefined; - } return editMode; } +function isVimParams(params: EditParams): params is VimParams { + return typeof params === "object" && params !== null && "file" in params && typeof params.file === "string"; +} + function resolveAllowFuzzy(session: ToolSession, rawValue: string): boolean { switch (rawValue) { case "true": @@ -228,7 +234,8 @@ export class EditTool implements AgentTool { readonly #allowFuzzy: boolean; readonly #fuzzyThreshold: number; readonly #writethrough: WritethroughCallback; - readonly #editMode?: EditToolMode; + readonly #editMode?: EditMode; + readonly #vimTool: VimTool; readonly #pendingDeferredFetches = new Map(); constructor(private readonly session: ToolSession) { @@ -242,12 +249,12 @@ export class EditTool implements AgentTool { this.#allowFuzzy = resolveAllowFuzzy(session, editFuzzy); this.#fuzzyThreshold = resolveFuzzyThreshold(session, editFuzzyThreshold); this.#writethrough = createEditWritethrough(session); + this.#vimTool = new VimTool(session); } - get mode(): EditToolMode { + get mode(): EditMode { if (this.#editMode) return this.#editMode; - const mode = resolveEditMode(this.session); - return mode === "vim" ? (DEFAULT_EDIT_MODE as EditToolMode) : mode; + return resolveEditMode(this.session); } get description(): string { @@ -262,9 +269,9 @@ export class EditTool implements AgentTool { _toolCallId: string, params: EditParams, signal?: AbortSignal, - onUpdate?: AgentToolUpdateCallback, + onUpdate?: AgentToolUpdateCallback, context?: AgentToolContext, - ): Promise> { + ): Promise> { const modeDefinition = this.#getModeDefinition(); if (!modeDefinition.validate(params)) { throw new Error(modeDefinition.invalidParamsMessage); @@ -399,6 +406,31 @@ export class EditTool implements AgentTool { return executePerFile(entries, batchRequest, onUpdate); }, }, + vim: { + description: () => this.#vimTool.description, + parameters: vimSchema, + invalidParamsMessage: "Invalid edit parameters for vim mode.", + validate: isVimParams, + execute: async ( + tool: EditTool, + params: EditParams, + signal: AbortSignal | undefined, + _batchRequest: LspBatchRequest | undefined, + onUpdate?: (partialResult: AgentToolResult) => void, + ) => { + const handleUpdate = onUpdate + ? (partialResult: AgentToolResult) => { + onUpdate(partialResult as AgentToolResult); + } + : undefined; + return (await tool.#vimTool.execute( + "edit", + params as VimParams, + signal, + handleUpdate, + )) as AgentToolResult; + }, + }, }[this.mode]; } diff --git a/packages/coding-agent/src/edit/renderer.ts b/packages/coding-agent/src/edit/renderer.ts index c18f2ad36..b2f46f482 100644 --- a/packages/coding-agent/src/edit/renderer.ts +++ b/packages/coding-agent/src/edit/renderer.ts @@ -21,7 +21,9 @@ import { shortenPath, truncateDiffByHunk, } from "../tools/render-utils"; +import { type VimRenderArgs, vimToolRenderer } from "../tools/vim"; import { Hasher, type RenderCache, renderStatusLine, truncateToWidth } from "../tui"; +import type { VimToolDetails } from "../vim/types"; import type { DiffError, DiffResult } from "./diff"; import { type ChunkToolEdit, parseChunkEditPath } from "./modes/chunk"; import type { HashlineToolEdit } from "./modes/hashline"; @@ -108,6 +110,31 @@ interface EditRenderArgs { edits?: Partial[]; } +function isVimRenderArgs(args: EditRenderArgs | VimRenderArgs): args is VimRenderArgs { + return ( + typeof args === "object" && + args !== null && + typeof (args as { file?: unknown }).file === "string" && + !("path" in args) && + !("file_path" in args) && + !("edits" in args) + ); +} + +function isVimToolDetails(details: unknown): details is VimToolDetails { + if (!details || typeof details !== "object" || Array.isArray(details)) { + return false; + } + const cursor = (details as { cursor?: unknown }).cursor; + const viewportLines = (details as { viewportLines?: unknown }).viewportLines; + return ( + typeof (details as { file?: unknown }).file === "string" && + typeof cursor === "object" && + cursor !== null && + Array.isArray(viewportLines) + ); +} + /** Extended context for edit tool rendering */ export interface EditRenderContext { /** Pre-computed diff preview (computed before tool executes) */ @@ -406,6 +433,10 @@ export const editToolRenderer = { mergeCallAndResult: true, renderCall(args: EditRenderArgs, options: RenderResultOptions, uiTheme: Theme): Component { + if (isVimRenderArgs(args)) { + return vimToolRenderer.renderCall(args, options, uiTheme); + } + // Extract path from first edit entry when top-level path is absent (new schema) const firstEdit = Array.isArray(args.edits) && args.edits.length > 0 ? args.edits[0] : undefined; const rawPath = args.file_path || args.path || (firstEdit as any)?.path || ""; @@ -431,6 +462,14 @@ export const editToolRenderer = { uiTheme: Theme, args?: EditRenderArgs, ): Component { + if (isVimToolDetails(result.details)) { + return vimToolRenderer.renderResult( + result as { content: Array<{ type: string; text?: string }>; details?: VimToolDetails; isError?: boolean }, + options, + uiTheme, + ); + } + const perFileResults = result.details?.perFileResults; const totalFiles = Array.isArray(args?.edits) ? countEditFiles(args!.edits as any[]) : 0; if (perFileResults && (perFileResults.length > 1 || totalFiles > 1)) { diff --git a/packages/coding-agent/src/modes/components/tool-execution.ts b/packages/coding-agent/src/modes/components/tool-execution.ts index 9e33dc831..831c86f0b 100644 --- a/packages/coding-agent/src/modes/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/components/tool-execution.ts @@ -316,8 +316,7 @@ export class ToolExecutionComponent extends Container { */ #updateSpinnerAnimation(): void { // Spinner for: task tool with partial result, or edit/write while args streaming - const isStreamingArgs = - !this.#argsComplete && (this.#toolName === "edit" || this.#toolName === "write" || this.#toolName === "vim"); + const isStreamingArgs = !this.#argsComplete && (this.#toolName === "edit" || this.#toolName === "write"); const isBackgroundAsyncTask = this.#toolName === "task" && (this.#result?.details as { async?: { state?: string } } | undefined)?.async?.state === "running"; diff --git a/packages/coding-agent/src/prompts/tools/vim.md b/packages/coding-agent/src/prompts/tools/vim.md index 380190cdc..40edff663 100644 --- a/packages/coding-agent/src/prompts/tools/vim.md +++ b/packages/coding-agent/src/prompts/tools/vim.md @@ -1,4 +1,4 @@ -Stateful Vim editor. Every call requires `file`; the buffer loads automatically on first use. +Vim-style `edit` mode. The tool name stays `edit`; every call requires `file`, and the buffer loads automatically on first use. - `{"file": "path"}` - view file - `{"file": "path", "steps": [{"kbd": ["…"], "insert": "…"}]}` - edit file @@ -83,7 +83,7 @@ Ex commands always start with `:` and end with ``. `3,5d` without `:` is NOT ## Session persistence -The vim buffer persists across tool calls. Cursor position, undo history, and file state are maintained until you close the tool. Auto-save happens once after all steps in a non-paused call complete. +The edit buffer in vim mode persists across tool calls. Cursor position, undo history, and file state are maintained until you close the buffer. Auto-save happens once after all steps in a non-paused call complete. ## Supported diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 284f4a0b3..f62bb9ef0 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -131,12 +131,6 @@ import { ToolContextStore } from "./tools/context"; import { getGeminiImageTools } from "./tools/gemini-image"; import { wrapToolWithMetaNotice } from "./tools/output-meta"; import { queueResolveHandler } from "./tools/resolve"; -import { - filterInactiveEditToolName, - normalizeToolNamesForEditMode, - resolveEditToolName, - resolveInactiveEditToolName, -} from "./utils/edit-mode"; import { EventBus } from "./utils/event-bus"; import { buildNamedToolChoice } from "./utils/tool-choice"; @@ -904,17 +898,15 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} if (model) return formatModelString(model); return undefined; }; - const editModeSession = { - settings, - getActiveModelString, - } as const; const toolSession: ToolSession = { cwd, hasUI: options.hasUI ?? false, enableLsp, get hasEditTool() { - const requestedToolNames = normalizeToolNamesForEditMode(options.toolNames, editModeSession); - return !requestedToolNames || requestedToolNames.includes(resolveEditToolName(editModeSession)); + const requestedToolNames = options.toolNames + ? [...new Set(options.toolNames.map(name => name.toLowerCase()))] + : undefined; + return !requestedToolNames || requestedToolNames.includes("edit"); }, skipPythonPreflight: options.skipPythonPreflight, forcePythonWarmup: options.forcePythonWarmup, @@ -1250,17 +1242,6 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} for (const tool of builtinTools) { toolRegistry.set(tool.name, tool); } - const inactiveEditToolName = resolveInactiveEditToolName(editModeSession); - if (!toolRegistry.has(inactiveEditToolName)) { - const inactiveEditTool = await logger.time( - `createTools:${inactiveEditToolName}:hidden`, - BUILTIN_TOOLS[inactiveEditToolName], - toolSession, - ); - if (inactiveEditTool) { - toolRegistry.set(inactiveEditTool.name, wrapToolWithMetaNotice(inactiveEditTool)); - } - } for (const tool of wrappedExtensionTools) { toolRegistry.set(tool.name, tool); } @@ -1368,9 +1349,10 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} return options.systemPrompt(defaultPrompt); }; - const toolNamesFromRegistry = filterInactiveEditToolName(toolRegistry.keys(), editModeSession); + const toolNamesFromRegistry = Array.from(toolRegistry.keys()); const requestedToolNames = - normalizeToolNamesForEditMode(options.toolNames, editModeSession) ?? toolNamesFromRegistry; + (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"); const mcpDiscoveryEnabled = settings.get("mcp.discoveryMode") ?? false; diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index ff3c20d98..76f527da5 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -138,13 +138,7 @@ import { getLatestTodoPhasesFromEntries, type TodoItem, type TodoPhase } from ". import { ToolError } from "../tools/tool-errors"; import { clampTimeout } from "../tools/tool-timeouts"; import { parseCommandArgs } from "../utils/command-args"; -import { - type EditMode, - filterInactiveEditToolName, - normalizeToolNamesForEditMode, - resolveEditMode, - resolveEditToolName, -} from "../utils/edit-mode"; +import { type EditMode, resolveEditMode } from "../utils/edit-mode"; import { resolveFileDisplayMode } from "../utils/file-display-mode"; import { extractFileMentions, generateFileMentionMessages } from "../utils/file-mentions"; import { buildNamedToolChoice } from "../utils/tool-choice"; @@ -1996,7 +1990,7 @@ export class AgentSession { /** Whether the edit tool is registered in this session. */ get hasEditTool(): boolean { - return this.#toolRegistry.has("edit") || this.#toolRegistry.has("vim"); + return this.#toolRegistry.has("edit"); } /** @@ -2010,7 +2004,7 @@ export class AgentSession { * Get all configured tool names (built-in via --tools or default, plus custom tools). */ getAllToolNames(): string[] { - return filterInactiveEditToolName(this.#toolRegistry.keys(), this.#getEditModeSession()); + return Array.from(this.#toolRegistry.keys()); } #getEditModeSession() { @@ -2025,26 +2019,8 @@ export class AgentSession { } async #syncEditToolModeAfterModelChange(previousEditMode: EditMode): Promise { - const activeToolNames = this.getActiveToolNames(); const currentEditMode = this.#resolveActiveEditMode(); - const hasActiveEditTool = activeToolNames.some(name => name === "edit" || name === "vim"); - if (!hasActiveEditTool) { - if (previousEditMode !== currentEditMode) { - await this.refreshBaseSystemPrompt(); - } - return; - } - - const normalizedToolNames = normalizeToolNamesForEditMode(activeToolNames, this.#getEditModeSession()) ?? []; - const toolNamesChanged = - normalizedToolNames.length !== activeToolNames.length || - normalizedToolNames.some((name, index) => name !== activeToolNames[index]); - if (toolNamesChanged) { - await this.#applyActiveToolsByName(normalizedToolNames); - return; - } - - if (previousEditMode !== currentEditMode) { + if (previousEditMode !== currentEditMode && this.getActiveToolNames().includes("edit")) { await this.refreshBaseSystemPrompt(); } } @@ -2096,7 +2072,7 @@ export class AgentSession { toolNames: string[], options?: { persistMCPSelection?: boolean; previousSelectedMCPToolNames?: string[] }, ): Promise { - toolNames = normalizeToolNamesForEditMode(toolNames, this.#getEditModeSession()) ?? []; + toolNames = [...new Set(toolNames.map(name => name.toLowerCase()))]; const previousSelectedMCPToolNames = options?.previousSelectedMCPToolNames ?? this.getSelectedMCPToolNames(); const tools: AgentTool[] = []; const validToolNames: string[] = []; @@ -2489,7 +2465,7 @@ export class AgentSession { planExists, askToolName: "ask", writeToolName: "write", - editToolName: resolveEditToolName(this.#getEditModeSession()), + editToolName: "edit", exitToolName: "exit_plan_mode", reentry: state.reentry ?? false, iterative: state.workflow === "iterative", diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index f6c9bde89..72a58832d 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -16,7 +16,6 @@ import type { CustomMessage } from "../session/messages"; import type { ToolChoiceQueue } from "../session/tool-choice-queue"; import { TaskTool } from "../task"; import type { AgentOutputManager } from "../task/output-manager"; -import { normalizeToolNamesForEditMode, resolveEditToolName } from "../utils/edit-mode"; import type { EventBus } from "../utils/event-bus"; import { SearchTool } from "../web/search"; import { AskTool } from "./ask"; @@ -56,7 +55,6 @@ import { SearchToolBm25Tool } from "./search-tool-bm25"; import { loadSshTool } from "./ssh"; import { SubmitResultTool } from "./submit-result"; import { type TodoPhase, TodoWriteTool } from "./todo-write"; -import { VimTool } from "./vim"; import { WriteTool } from "./write"; // Exa MCP tools (22 tools) @@ -243,7 +241,6 @@ export const BUILTIN_TOOLS: Record = { todo_write: s => new TodoWriteTool(s), web_search: s => new SearchTool(s), search_tool_bm25: SearchToolBm25Tool.createIf, - vim: s => new VimTool(s), write: s => new WriteTool(s), }; @@ -293,12 +290,8 @@ function getPythonModeFromEnv(): PythonToolMode | null { export async function createTools(session: ToolSession, toolNames?: string[]): Promise { const includeSubmitResult = session.requireSubmitResultTool === true; const enableLsp = session.enableLsp ?? true; - const requestedTools = normalizeToolNamesForEditMode( - toolNames && toolNames.length > 0 ? toolNames : undefined, - session, - ); - const activeEditToolName = resolveEditToolName(session); - const inactiveEditToolName = activeEditToolName === "edit" ? "vim" : "edit"; + const requestedTools = + toolNames && toolNames.length > 0 ? [...new Set(toolNames.map(name => name.toLowerCase()))] : undefined; if (requestedTools && !requestedTools.includes("exit_plan_mode")) { requestedTools.push("exit_plan_mode"); } @@ -384,7 +377,7 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P requestedTools.push("ast_grep"); } if ( - requestedTools.includes(activeEditToolName) && + requestedTools.includes("edit") && !requestedTools.includes("ast_edit") && session.settings.get("astEdit.enabled") ) { @@ -427,9 +420,7 @@ export async function createTools(session: ToolSession, toolNames?: string[]): P filteredRequestedTools !== undefined ? filteredRequestedTools.filter(name => name !== "resolve").map(name => [name, allTools[name]] as const) : [ - ...Object.entries(BUILTIN_TOOLS).filter( - ([name]) => name !== inactiveEditToolName && isToolAllowed(name), - ), + ...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), ]; diff --git a/packages/coding-agent/src/tools/renderers.ts b/packages/coding-agent/src/tools/renderers.ts index a1e72730d..e14e89569 100644 --- a/packages/coding-agent/src/tools/renderers.ts +++ b/packages/coding-agent/src/tools/renderers.ts @@ -27,7 +27,6 @@ import { resolveToolRenderer } from "./resolve"; import { searchToolBm25Renderer } from "./search-tool-bm25"; import { sshToolRenderer } from "./ssh"; import { todoWriteToolRenderer } from "./todo-write"; -import { vimToolRenderer } from "./vim"; import { writeToolRenderer } from "./write"; type ToolRenderer = { @@ -63,7 +62,6 @@ export const toolRenderers: Record = { ssh: sshToolRenderer as ToolRenderer, task: taskToolRenderer as ToolRenderer, todo_write: todoWriteToolRenderer as ToolRenderer, - vim: vimToolRenderer as ToolRenderer, gh_run_watch: ghRunWatchToolRenderer as ToolRenderer, web_search: webSearchToolRenderer as ToolRenderer, write: writeToolRenderer as ToolRenderer, diff --git a/packages/coding-agent/src/tools/vim.ts b/packages/coding-agent/src/tools/vim.ts index 0a8145f5a..6336afd63 100644 --- a/packages/coding-agent/src/tools/vim.ts +++ b/packages/coding-agent/src/tools/vim.ts @@ -1,7 +1,7 @@ import type { AgentTool, AgentToolContext, AgentToolResult, AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { Component } from "@oh-my-pi/pi-tui"; import { extractSegments, sliceWithWidth, Text } from "@oh-my-pi/pi-tui"; -import { isEnoent, prompt, untilAborted } from "@oh-my-pi/pi-utils"; +import { isEnoent, logger, prompt, untilAborted } from "@oh-my-pi/pi-utils"; import { type Static, Type } from "@sinclair/typebox"; import * as Diff from "diff"; import type { RenderResultOptions } from "../extensibility/custom-tools/types"; @@ -304,35 +304,37 @@ interface ExecuteVimStepsOptions { onInsertStep?: () => Promise; } -// Auto-reorder insertion steps to descending line order (bottom-up) when all steps -// are simple `NGo`/`NGO` patterns and appear to reference original line numbers. -// Only reorders when steps are in strictly ascending order (common top-down mistake). -function autoReorderInsertSteps(steps: readonly VimStep[]): VimStep[] { +// Auto-reorder line-positioned steps to descending order (bottom-up) when all steps +// are simple `NG` patterns and appear in ascending order (top-down). Bottom-up +// ordering is safe for any mix of insert/replace commands because edits at higher +// line numbers never shift lower line numbers. +function autoReorderSteps(steps: readonly VimStep[]): VimStep[] { if (steps.length < 2) return [...steps]; - // Check if ALL steps follow the pattern: single kbd entry of `NGo` or `NGO` - const linePattern = /^(\d+)G([oO])$/; + // Match single kbd entry of `G` where cmd enters insert mode + const linePattern = /^(\d+)G(o|O|cc|C|S|s|i|I|a|A)$/; const parsed: Array<{ line: number; step: VimStep }> = []; for (const step of steps) { - if (step.kbd.length !== 1) return [...steps]; // Can't reorder non-simple steps + if (step.kbd.length !== 1) return [...steps]; const match = step.kbd[0]!.match(linePattern); - if (!match) return [...steps]; // Not all steps are NGo/NGO — don't reorder + if (!match) return [...steps]; parsed.push({ line: Number(match[1]), step }); } // Only reorder if steps are in strictly ascending order (top-down, likely a mistake). // If already descending, mixed, or equal, the model likely planned the order deliberately. - let isAscending = true; for (let i = 1; i < parsed.length; i++) { if (parsed[i]!.line <= parsed[i - 1]!.line) { - isAscending = false; - break; + return [...steps]; } } - if (!isAscending) return [...steps]; // Sort by descending line number (bottom-up) parsed.sort((a, b) => b.line - a.line); + logger.debug("vim: auto-reordered steps to bottom-up", { + original: steps.map(s => s.kbd[0]), + reordered: parsed.map(p => p.step.kbd[0]), + }); return parsed.map(p => p.step); } @@ -341,9 +343,9 @@ async function executeVimSteps( steps: readonly VimStep[], options: ExecuteVimStepsOptions = {}, ): Promise { - // Execute steps in the order specified. Models should use bottom-up ordering - // (highest line number first) when inserting at multiple locations. - const orderedSteps = [...steps]; + // Auto-reorder ascending line-positioned steps to descending (bottom-up) + // to prevent line-shift corruption from top-down edits. + const orderedSteps = autoReorderSteps(steps); for (let index = 0; index < orderedSteps.length; index += 1) { if (engine.closed) { break; @@ -411,7 +413,7 @@ async function readTextFile( const bytes = await file.bytes(); for (const byte of bytes) { if (byte === 0) { - throw new ToolError("Vim only supports UTF-8 text files in v1"); + throw new ToolError("Edit tool in vim mode only supports UTF-8 text files in v1"); } } const text = utf8Decoder.decode(bytes); @@ -436,7 +438,7 @@ async function readTextFile( }; } if (error instanceof TypeError) { - throw new ToolError("Vim only supports UTF-8 text files in v1"); + throw new ToolError("Edit tool in vim mode only supports UTF-8 text files in v1"); } throw error; } @@ -445,16 +447,16 @@ async function readTextFile( function normalizeTargetPath(inputPath: string, cwd: string): { absolutePath: string; displayPath: string } { const normalized = normalizePathLikeInput(inputPath); if (INTERNAL_URL_PREFIX.test(normalized)) { - throw new ToolError("Vim only supports regular filesystem paths in v1"); + throw new ToolError("Edit tool in vim mode only supports regular filesystem paths in v1"); } if (isReadableUrlPath(normalized)) { - throw new ToolError("Vim only supports local filesystem paths in v1"); + throw new ToolError("Edit tool in vim mode only supports local filesystem paths in v1"); } if (parseArchivePathCandidates(normalized).some(candidate => candidate.archivePath === normalized)) { - throw new ToolError("Vim does not support archive targets in v1"); + throw new ToolError("Edit tool in vim mode does not support archive targets in v1"); } if (parseSqlitePathCandidates(normalized).some(candidate => candidate.sqlitePath === normalized)) { - throw new ToolError("Vim does not support SQLite targets in v1"); + throw new ToolError("Edit tool in vim mode does not support SQLite targets in v1"); } return { absolutePath: resolveToCwd(normalized, cwd), @@ -485,7 +487,7 @@ export class VimTool implements AgentTool { async #loadBuffer(targetPath: string): Promise { const { absolutePath, displayPath } = normalizeTargetPath(targetPath, this.session.cwd); if (await isSqliteFile(absolutePath)) { - throw new ToolError("Vim does not support SQLite targets in v1"); + throw new ToolError("Edit tool in vim mode does not support SQLite targets in v1"); } const loaded = await readTextFile(absolutePath); return { @@ -604,12 +606,12 @@ export class VimTool implements AgentTool { if (this.session.getPlanModeState?.()?.enabled) { if (steps.some(step => step.insert !== undefined)) { - throw new ToolError("Plan mode: vim is read-only; insert payloads are not allowed."); + throw new ToolError("Plan mode: edit is read-only in vim mode; insert payloads are not allowed."); } const preview = engine.clone({ beforeMutate: async () => { throw new VimInputError( - "Plan mode: vim is read-only; only navigation, search, open, and close are allowed.", + "Plan mode: edit is read-only in vim mode; only navigation, search, open, and close are allowed.", ); }, saveBuffer: async () => { @@ -785,7 +787,7 @@ export function resetVimRendererStateForTest(): void { export const vimToolRenderer = { renderCall(args: VimRenderArgs, options: RenderResultOptions, uiTheme: Theme): Component { if (args.file && (!args.steps || args.steps.length === 0)) { - return renderText(`${uiTheme.bold("Vim")} open ${args.file}`); + return renderText(`${uiTheme.bold("Edit")} open ${args.file}`); } // Build a description of the streaming args for the header @@ -822,7 +824,7 @@ export const vimToolRenderer = { { icon: "pending", spinnerFrame: options.spinnerFrame, - title: "Vim", + title: "Edit", description: argsDescription || details.file + modified, meta: [`${langIcon} ${details.totalLines} lines`, position], }, @@ -850,9 +852,9 @@ export const vimToolRenderer = { // Fallback: no previous viewport available (first vim call) if (argsDescription) { - return renderText(`${uiTheme.bold("Vim")} ${argsDescription}`); + return renderText(`${uiTheme.bold("Edit")} ${argsDescription}`); } - return renderText(`${uiTheme.bold("Vim")}`); + return renderText(`${uiTheme.bold("Edit")}`); }, renderResult( result: { content: Array<{ type: string; text?: string }>; details?: VimToolDetails; isError?: boolean }, @@ -932,7 +934,7 @@ export const vimToolRenderer = { { icon, spinnerFrame: options.spinnerFrame, - title: "Vim", + title: "Edit", description: details.file + modified, badge: modeBadge, meta: [`${langIcon} ${details.totalLines} lines`, position], diff --git a/packages/coding-agent/src/utils/edit-mode.ts b/packages/coding-agent/src/utils/edit-mode.ts index ee2af07b7..6f187ec7e 100644 --- a/packages/coding-agent/src/utils/edit-mode.ts +++ b/packages/coding-agent/src/utils/edit-mode.ts @@ -1,7 +1,6 @@ import { $env, $flag } from "@oh-my-pi/pi-utils"; export type EditMode = "replace" | "patch" | "hashline" | "chunk" | "vim"; -export type EditToolName = "edit" | "vim"; export const DEFAULT_EDIT_MODE: EditMode = "hashline"; @@ -49,37 +48,3 @@ export function resolveEditMode(session: EditModeSessionLike): EditMode { const settingsMode = normalizeEditMode(String(session.settings.get("edit.mode") ?? "")); return settingsMode ?? DEFAULT_EDIT_MODE; } - -export function resolveEditToolName(session: EditModeSessionLike): EditToolName { - return resolveEditMode(session) === "vim" ? "vim" : "edit"; -} - -export function resolveInactiveEditToolName(session: EditModeSessionLike): EditToolName { - return resolveEditToolName(session) === "edit" ? "vim" : "edit"; -} - -export function filterInactiveEditToolName(toolNames: Iterable, session: EditModeSessionLike): string[] { - const inactiveEditToolName = resolveInactiveEditToolName(session); - return Array.from(toolNames).filter(name => name !== inactiveEditToolName); -} - -export function normalizeToolNamesForEditMode( - toolNames: Iterable | undefined, - session: EditModeSessionLike, -): string[] | undefined { - if (!toolNames) return undefined; - - const normalized: string[] = []; - const seen = new Set(); - const activeEditToolName = resolveEditToolName(session); - - for (const rawName of toolNames) { - const lowerName = rawName.toLowerCase(); - const nextName = lowerName === "edit" || lowerName === "vim" ? activeEditToolName : lowerName; - if (seen.has(nextName)) continue; - seen.add(nextName); - normalized.push(nextName); - } - - return normalized; -} diff --git a/packages/coding-agent/test/agent-session-message-pipeline.test.ts b/packages/coding-agent/test/agent-session-message-pipeline.test.ts index 492fd4924..6c1f7c8d0 100644 --- a/packages/coding-agent/test/agent-session-message-pipeline.test.ts +++ b/packages/coding-agent/test/agent-session-message-pipeline.test.ts @@ -120,7 +120,7 @@ describe("AgentSession message pipeline", () => { { type: "toolCall", id: "call_1", - name: "vim", + name: "edit", arguments: {}, partialJson: '{"file":"preview.txt","steps":[{"kbd":["ggdGi"],"insert":"rep', }, diff --git a/packages/coding-agent/test/sdk-tool-activation.test.ts b/packages/coding-agent/test/sdk-tool-activation.test.ts index 4dbb3e3a6..f1b67e302 100644 --- a/packages/coding-agent/test/sdk-tool-activation.test.ts +++ b/packages/coding-agent/test/sdk-tool-activation.test.ts @@ -112,7 +112,7 @@ describe("createAgentSession defaultInactive tool activation", () => { } }); - it("normalizes edit tool activation to vim when vim edit mode is configured", async () => { + it("keeps edit active when vim edit mode is configured", async () => { const tempDir = path.join(os.tmpdir(), `pi-sdk-tool-activation-${Snowflake.next()}`); tempDirs.push(tempDir); fs.mkdirSync(tempDir, { recursive: true }); @@ -135,21 +135,21 @@ describe("createAgentSession defaultInactive tool activation", () => { }); try { - expect(session.getActiveToolNames()).toContain("vim"); - expect(session.getActiveToolNames()).not.toContain("edit"); - expect(session.getAllToolNames()).toContain("vim"); - expect(session.getAllToolNames()).not.toContain("edit"); + expect(session.getActiveToolNames()).toContain("edit"); + expect(session.getActiveToolNames()).not.toContain("vim"); + expect(session.getAllToolNames()).toContain("edit"); + expect(session.getAllToolNames()).not.toContain("vim"); await session.setActiveToolsByName(["read", "edit"]); - expect(session.getActiveToolNames()).toContain("vim"); - expect(session.getActiveToolNames()).not.toContain("edit"); + expect(session.getActiveToolNames()).toContain("edit"); + expect(session.getActiveToolNames()).not.toContain("vim"); } finally { await session.dispose(); } }); - it("swaps the visible edit-capable tool when the active model changes edit modes", async () => { + it("keeps the visible edit tool stable when the active model changes edit modes", async () => { const tempDir = path.join(os.tmpdir(), `pi-sdk-tool-activation-${Snowflake.next()}`); tempDirs.push(tempDir); fs.mkdirSync(tempDir, { recursive: true }); @@ -195,10 +195,10 @@ describe("createAgentSession defaultInactive tool activation", () => { await session.setModel(vimModel); - expect(session.getActiveToolNames()).toContain("vim"); - expect(session.getActiveToolNames()).not.toContain("edit"); - expect(session.getAllToolNames()).toContain("vim"); - expect(session.getAllToolNames()).not.toContain("edit"); + expect(session.getActiveToolNames()).toContain("edit"); + expect(session.getActiveToolNames()).not.toContain("vim"); + expect(session.getAllToolNames()).toContain("edit"); + expect(session.getAllToolNames()).not.toContain("vim"); } finally { await session.dispose(); } diff --git a/packages/coding-agent/test/tools.test.ts b/packages/coding-agent/test/tools.test.ts index e2aec47c9..df716bf1e 100644 --- a/packages/coding-agent/test/tools.test.ts +++ b/packages/coding-agent/test/tools.test.ts @@ -669,12 +669,13 @@ describe("Coding Agent Tools", () => { const result = await editTool.execute("test-call-5", { edits: [{ path: testFile, old_text: "world", new_text: "testing" }], }); + const details = result.details as { diff?: string } | undefined; expect(getTextOutput(result)).toContain("Successfully replaced"); - expect(result.details).toBeDefined(); - expect(result.details!.diff).toBeDefined(); - expect(typeof result.details!.diff).toBe("string"); - expect(result.details!.diff).toContain("testing"); + expect(details).toBeDefined(); + expect(details?.diff).toBeDefined(); + expect(typeof details?.diff).toBe("string"); + expect(details?.diff).toContain("testing"); }); it("should fail if text not found", async () => { diff --git a/packages/coding-agent/test/tools/index.test.ts b/packages/coding-agent/test/tools/index.test.ts index ab5d5babb..34ea76f29 100644 --- a/packages/coding-agent/test/tools/index.test.ts +++ b/packages/coding-agent/test/tools/index.test.ts @@ -70,7 +70,7 @@ describe("createTools", () => { expect(names).not.toContain("vim"); }); - it("exposes vim instead of edit when vim edit mode is active", async () => { + it("keeps edit visible when vim edit mode is active", async () => { const session = createTestSession({ settings: createSettingsWithOverrides({ "edit.mode": "vim", @@ -79,8 +79,8 @@ describe("createTools", () => { const tools = await createTools(session); const names = tools.map(t => t.name); - expect(names).toContain("vim"); - expect(names).not.toContain("edit"); + expect(names).toContain("edit"); + expect(names).not.toContain("vim"); }); it("includes bash and python when python mode is both", async () => { @@ -149,16 +149,16 @@ describe("createTools", () => { expect(names).toEqual(["read", "write", "exit_plan_mode"]); }); - it("maps requested edit to vim when vim edit mode is active", async () => { + it("ignores vim as an unknown requested tool even when vim edit mode is active", async () => { const session = createTestSession({ settings: createSettingsWithOverrides({ "edit.mode": "vim", }), }); - const tools = await createTools(session, ["read", "edit"]); + const tools = await createTools(session, ["read", "vim"]); const names = tools.map(t => t.name); - expect(names).toEqual(["read", "vim", "exit_plan_mode", "ast_edit"]); + expect(names).toEqual(["read", "exit_plan_mode"]); }); it("lowercases requested tool subset", async () => { diff --git a/packages/coding-agent/test/tools/vim.test.ts b/packages/coding-agent/test/tools/vim.test.ts index ada0452f6..e075649a7 100644 --- a/packages/coding-agent/test/tools/vim.test.ts +++ b/packages/coding-agent/test/tools/vim.test.ts @@ -667,7 +667,7 @@ describe("vim renderer", () => { const uiStub = { requestRender() {} } as unknown as TUI; const component = new ToolExecutionComponent( - "vim", + "edit", { file: "preview.txt", steps: [step(["ggdGi"])], diff --git a/scripts/edit_benchmark_common.py b/scripts/edit_benchmark_common.py index 8590b87d7..2b7c37859 100644 --- a/scripts/edit_benchmark_common.py +++ b/scripts/edit_benchmark_common.py @@ -722,7 +722,7 @@ def run_benchmark_for_model( token_input = 0 token_output = 0 turns_used = 0 - edit_vim_tool_calls = 0 + edit_tool_calls = 0 success = False feedback = "" error_msg: str | None = None @@ -747,11 +747,11 @@ def run_benchmark_for_model( def handle_tool_count(event: ToolExecutionStartEvent) -> None: - nonlocal edit_vim_tool_calls, turns_used + nonlocal edit_tool_calls, turns_used if counting_edit_turns: turns_used += 1 - if event.tool_name in {"edit", "vim"}: - edit_vim_tool_calls += 1 + if event.tool_name == "edit": + edit_tool_calls += 1 tool_count_remover = client.on_tool_execution_start(handle_tool_count) @@ -808,7 +808,7 @@ def run_benchmark_for_model( success=success, turns_used=turns_used, prompt_attempts=prompt_attempts, - edit_calls=edit_vim_tool_calls, + edit_calls=edit_tool_calls, token_input=token_input, token_output=token_output, feedback=feedback.strip(), diff --git a/scripts/vim-edit-benchmark.py b/scripts/vim-edit-benchmark.py index 21353b123..84751fef5 100755 --- a/scripts/vim-edit-benchmark.py +++ b/scripts/vim-edit-benchmark.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 """ -Vim edit benchmark: tests the vim tool across models with a simple edit task. +Vim edit-mode benchmark: tests the edit tool in vim mode across models with a simple edit task. """ from __future__ import annotations @@ -8,7 +8,7 @@ from edit_benchmark_common import BenchmarkSpec, EDIT_DIFF, EXPECTED_CONTENT, ru EDIT_PROMPT = f"""\ -Use the `read` tool to inspect `test.rs`, then use the `vim` tool to make `test.rs` exactly match the requested change. +Use the `read` tool to inspect `test.rs`, then use the `edit` tool in vim mode to make `test.rs` exactly match the requested change. Apply this diff: ```diff @@ -20,12 +20,12 @@ Final expected file content: """ VIM_BENCHMARK = BenchmarkSpec( - description="Benchmark vim tool across models with simple edit tasks.", + description="Benchmark edit tool in vim mode across models with simple edit tasks.", workspace_prefix="vim-benchmark", - tools=("vim", "read"), + tools=("edit", "read"), env={"PI_EDIT_VARIANT": "vim", "PI_STRICT_EDIT_MODE": "1"}, initial_prompt=EDIT_PROMPT, - retry_instruction="Please try again using the vim tool.", + retry_instruction="Please try again using the edit tool in vim mode.", )