From 10ba6bff89770f1f723cb2dd507831c24e6b1c53 Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 13 Apr 2026 21:14:35 +0200 Subject: [PATCH] fix(coding-agent): fixed vim command parsing and chunk edit fallback behavior - Changed chunk edit normalization to prefer write operations, then replace, insert, then delete, with empty write values now treated as clear-content writes. - Allowed space motions in vim input as ``, handled them as movement and rendered in error output via token display. - Adjusted benchmark retry flow to reset files on each retry, reduced per-run call limits, and lowered the default per-turn timeout. --- packages/coding-agent/src/edit/modes/chunk.ts | 24 +++++++------ .../coding-agent/src/prompts/tools/vim.md | 6 ++-- packages/coding-agent/src/vim/buffer.ts | 3 +- packages/coding-agent/src/vim/engine.ts | 8 +++-- packages/coding-agent/src/vim/parser.ts | 9 +++-- packages/coding-agent/test/tools/vim.test.ts | 23 ++++++++++++ scripts/edit_benchmark_common.py | 36 +++++++++++-------- 7 files changed, 76 insertions(+), 33 deletions(-) diff --git a/packages/coding-agent/src/edit/modes/chunk.ts b/packages/coding-agent/src/edit/modes/chunk.ts index 906e25aa9..c4ba10676 100644 --- a/packages/coding-agent/src/edit/modes/chunk.ts +++ b/packages/coding-agent/src/edit/modes/chunk.ts @@ -560,33 +560,37 @@ function normalizeChunkEditOperations(edits: ChunkToolEdit[]): { const warnings: string[] = []; const operations = edits.map((edit, index): ChunkEditOperation => { const { selector } = parseChunkEditPath(edit.path); - // When multiple ops are present (model confusion), pick the most substantive one. - // insert with real body > replace with real old/new > write string > delete + // When multiple ops are present (model confusion), prefer write (total replacement) as the + // safest default, then replace (surgical), then insert (additive), then delete. const hasInsert = edit.insert != null && typeof edit.insert.body === "string" && edit.insert.body.length > 0; const hasReplace = edit.replace != null && ((typeof edit.replace.old === "string" && edit.replace.old.length > 0) || (typeof edit.replace.new === "string" && edit.replace.new.length > 0)); - const hasWrite = typeof edit.write === "string"; + const hasWrite = typeof edit.write === "string" && edit.write.length > 0; const opCount = [hasInsert, hasReplace, hasWrite].filter(Boolean).length; if (opCount > 1) { - const chosen = hasInsert ? "insert" : hasReplace ? "replace" : "write"; - const present = [hasInsert && "insert", hasReplace && "replace", hasWrite && "write"] + const chosen = hasWrite ? "write" : hasReplace ? "replace" : "insert"; + const present = [hasWrite && "write", hasReplace && "replace", hasInsert && "insert"] .filter(Boolean) .join(", "); warnings.push( `Edit ${index + 1}: multiple operation fields set (${present}). Each edit entry must have exactly ONE of write/replace/insert — not multiple. Used "${chosen}", ignored the rest.`, ); } - if (hasInsert) { - const op = edit.insert!.loc === "prepend" ? "before" : "after"; - return { op, sel: selector, content: edit.insert!.body }; + if (hasWrite) { + return { op: "put", sel: selector, content: edit.write }; + } + if (typeof edit.write === "string") { + // write: "" (empty string) — clear the chunk content + return { op: "put", sel: selector, content: edit.write }; } if (hasReplace) { return { op: "replace", sel: selector, content: edit.replace!.new, find: edit.replace!.old }; } - if (hasWrite) { - return { op: "put", sel: selector, content: edit.write }; + if (hasInsert) { + const op = edit.insert!.loc === "prepend" ? "before" : "after"; + return { op, sel: selector, content: edit.insert!.body }; } // write: null or no op specified → delete return { op: "delete", sel: selector }; diff --git a/packages/coding-agent/src/prompts/tools/vim.md b/packages/coding-agent/src/prompts/tools/vim.md index 621692d64..731c1ea2b 100644 --- a/packages/coding-agent/src/prompts/tools/vim.md +++ b/packages/coding-agent/src/prompts/tools/vim.md @@ -17,6 +17,8 @@ For `insert` to work, the last `kbd` entry must leave INSERT mode active (`o`, ` Each non-final `kbd` entry must end in NORMAL mode (add ``). +Whitespace in `kbd` is literal. Do not use spaces as separators between keys; `ggdGi` is one sequence, not `ggdG i`. + ## Editing patterns **Insert after line N** — use `NGo` (Go to line N, open below). Include full indentation in insert: @@ -34,7 +36,7 @@ Each non-final `kbd` entry must end in NORMAL mode (add ``). {"file": "f.py", "kbd": ["5Gcc"], "insert": " replacement content"} ``` -**Replace entire file** — `ggdGi` = go to top, delete all, enter INSERT: +**Replace entire file** — `ggdGi` = go to top, delete all, enter INSERT. Use that exact sequence when rewriting the whole file: ```json {"file": "f.py", "kbd": ["ggdGi"], "insert": "entire new file content"} ``` @@ -60,7 +62,7 @@ The vim buffer persists across tool calls. Your cursor position, undo history, a ## Supported Keys: `` `` `` `` `` `` `` `` `` -Motions: `h j k l w b e 0 $ gg G { } f t` with counts +Motions: `h j k l w b e 0 $ gg G { } f t` with counts Operators: `d c y p` with motions and text objects (`iw aw i" a" i( a(`) Insert: `i a o O I A cc C s S` — these all enter INSERT mode; do NOT add another `i` after them Visual: `v V` diff --git a/packages/coding-agent/src/vim/buffer.ts b/packages/coding-agent/src/vim/buffer.ts index eb91fa37f..6994d29b0 100644 --- a/packages/coding-agent/src/vim/buffer.ts +++ b/packages/coding-agent/src/vim/buffer.ts @@ -167,7 +167,8 @@ export class VimBuffer { } setText(text: string, trailingNewline = this.trailingNewline): void { - this.lines = splitText(text); + const normalizedText = trailingNewline && text.endsWith("\n") ? text.slice(0, -1) : text; + this.lines = splitText(normalizedText); this.trailingNewline = trailingNewline; this.clampCursor(); } diff --git a/packages/coding-agent/src/vim/engine.ts b/packages/coding-agent/src/vim/engine.ts index f358ff297..71ed5f9b8 100644 --- a/packages/coding-agent/src/vim/engine.ts +++ b/packages/coding-agent/src/vim/engine.ts @@ -789,6 +789,7 @@ export class VimEngine { this.buffer.setCursor({ line: this.buffer.cursor.line - count, col: this.buffer.cursor.col }); return nextIndex + 1; case "l": + case " ": this.buffer.setCursor({ line: this.buffer.cursor.line, col: this.buffer.cursor.col + count }); return nextIndex + 1; case "w": @@ -999,7 +1000,7 @@ export class VimEngine { } else if (zTarget.value === "b") { this.viewportStart = Math.max(1, this.buffer.cursor.line + 1 - 39); } else { - throw new VimError(`Unsupported z command: z${zTarget.value}`, zTarget); + throw new VimError(`Unsupported z command: z${zTarget.display}`, zTarget); } return nextIndex + 2; } @@ -1016,7 +1017,7 @@ export class VimEngine { }); return nextIndex + 1; default: - throw new VimError(`Unsupported command: ${token.value}`, token); + throw new VimError(`Unsupported command: ${token.display}`, token); } } @@ -1265,6 +1266,7 @@ export class VimEngine { linewise: true, }; case "l": + case " ": return { nextIndex: index + 1, target: { line: this.buffer.cursor.line, col: this.buffer.cursor.col + count }, @@ -1403,7 +1405,7 @@ export class VimEngine { linewise: true, }; default: - throw new VimError(`Unsupported motion: ${token.value}`, token); + throw new VimError(`Unsupported motion: ${token.display}`, token); } } diff --git a/packages/coding-agent/src/vim/parser.ts b/packages/coding-agent/src/vim/parser.ts index 2e1af92be..e4559c993 100644 --- a/packages/coding-agent/src/vim/parser.ts +++ b/packages/coding-agent/src/vim/parser.ts @@ -21,7 +21,12 @@ function normalizeSpecialKey(raw: string): string | undefined { } function toDisplayToken(value: string): string { - return value.length === 1 ? value : `<${value}>`; + switch (value) { + case " ": + return ""; + default: + return value.length === 1 ? value : `<${value}>`; + } } export function parseKeySequences(sequences: string[]): VimKeyToken[] { @@ -78,7 +83,7 @@ export function parseKeySequences(sequences: string[]): VimKeyToken[] { if (char !== "<") { tokens.push({ value: char, - display: char, + display: toDisplayToken(char), sequenceIndex, offset, }); diff --git a/packages/coding-agent/test/tools/vim.test.ts b/packages/coding-agent/test/tools/vim.test.ts index b4d00bc10..de741c4e8 100644 --- a/packages/coding-agent/test/tools/vim.test.ts +++ b/packages/coding-agent/test/tools/vim.test.ts @@ -154,6 +154,11 @@ describe("vim engine", () => { expect(engine.buffer.getText()).toBe(""); expect(engine.statusMessage).toBe("Deleted 3 lines"); }); + + it("renders literal spaces visibly in unsupported command errors", async () => { + const engine = createEngine("alpha"); + await expect(engine.executeTokens(parseKeySequences(["z "]), "z ")).rejects.toThrow(/z/); + }); }); describe("vim tool", () => { @@ -234,6 +239,24 @@ describe("vim tool", () => { expect(textResult(replaced)).toContain("+beta"); }); + it("supports full-file rewrites when models emit a space before i", async () => { + const filePath = path.join(tmpDir, "full-rewrite.ts"); + await Bun.write(filePath, "first\nsecond\n"); + const tool = new VimTool(createSession(tmpDir)); + + await tool.execute("open", { file: "full-rewrite.ts" }); + const rewritten = await tool.execute("rewrite", { + file: "full-rewrite.ts", + kbd: ["ggdG i"], + insert: "alpha\nbeta\n", + }); + + const saved = await Bun.file(filePath).text(); + expect(saved).toBe("alpha\nbeta\n"); + expect(textResult(rewritten)).toContain("+alpha"); + expect(rewritten.details?.cursor.line).toBe(2); + }); + it("rejects another kbd entry after entering insert mode", async () => { const filePath = path.join(tmpDir, "ambiguous.ts"); await Bun.write(filePath, "first\n"); diff --git a/scripts/edit_benchmark_common.py b/scripts/edit_benchmark_common.py index 748d26404..6384f975f 100644 --- a/scripts/edit_benchmark_common.py +++ b/scripts/edit_benchmark_common.py @@ -95,8 +95,8 @@ This is a survey. Answer these 6 questions about your experience using the editi 6. Other thoughts: Anything else? """ -DEFAULT_MAX_TURNS = 20 -MAX_TOOL_CALLS_PER_PROMPT = 10 +DEFAULT_MAX_TURNS = 5 +MAX_TOOL_CALLS_PER_PROMPT = 6 _PRINT_LOCK = threading.Lock() @@ -244,7 +244,8 @@ def resolve_omp_bin(raw: str | None) -> str: def build_retry_prompt(spec: BenchmarkSpec, current_content: str) -> str: return ( - "The file doesn't match the expected result yet.\n\n" + "Previous attempt did not produce the expected result. " + "The file has been reset to its original state — start fresh.\n\n" f"Current content:\n```\n{current_content}```\n\n" f"Expected:\n```\n{EXPECTED_CONTENT}```\n\n" f"{spec.retry_instruction}" @@ -358,34 +359,39 @@ def run_benchmark_for_model( client.install_headless_ui() verbose_cleanup = install_verbose_logging(client, model, log_mode, thinking) - per_prompt_tool_calls = 0 def handle_tool_count(event: ToolExecutionStartEvent) -> None: - nonlocal edit_vim_tool_calls, turns_used, per_prompt_tool_calls - per_prompt_tool_calls += 1 + nonlocal edit_vim_tool_calls, turns_used if counting_edit_turns: turns_used += 1 if event.tool_name in {"edit", "vim"}: edit_vim_tool_calls += 1 - if per_prompt_tool_calls >= MAX_TOOL_CALLS_PER_PROMPT: - try: - client.abort() - except Exception: - pass # best-effort interrupt tool_count_remover = client.on_tool_execution_start(handle_tool_count) try: for turn in range(1, max_turns + 1): prompt_attempts = turn - per_prompt_tool_calls = 0 if turn == 1: client.prompt(spec.initial_prompt) else: - client.prompt(build_retry_prompt(spec, test_file.read_text())) + # Reset file to initial state on retry so the model starts fresh + # instead of trying to fix a potentially corrupted file. + test_file.write_text(INITIAL_CONTENT) + client.prompt(build_retry_prompt(spec, INITIAL_CONTENT)) - client.wait_for_idle(timeout=timeout) + try: + client.wait_for_idle(timeout=timeout) + except Exception: + # Prompt timed out or errored — abort the agent then retry. + try: + client.abort() + time.sleep(1) + client.wait_for_idle(timeout=10) + except Exception: + pass + continue current_content = test_file.read_text() if current_content.strip() == EXPECTED_CONTENT.strip(): @@ -495,7 +501,7 @@ def parse_args(description: str) -> argparse.Namespace: help="Executable to launch. Defaults to the repo checkout CLI, then falls back to `omp` on PATH.", ) parser.add_argument( - "--timeout", type=float, default=300.0, help="Per-turn timeout in seconds." + "--timeout", type=float, default=60.0, help="Per-turn timeout in seconds." ) parser.add_argument( "--max-turns",