From c62ab2d53e01d2aa2801a040da86955309b8dadc Mon Sep 17 00:00:00 2001 From: can1357 Date: Mon, 13 Apr 2026 12:27:10 +0200 Subject: [PATCH] chore(benchmarks-misc-fixes): cleaned benchmark task/pid validation - Added `vim` as an edit variant in benchmark CLI/config and rating script coverage. - Expanded benchmark execution so `vim` is treated as a mutation tool for retries, stats, and edit intent checks. - Adjusted `TaskTool` output schema precedence so explicit params override agent frontmatter. - Fixed `TaskTool` success counting by excluding aborted tasks from success totals. - Improved validation guidance in `SubmitResultTool`/`TodoWriteTool` for clearer recovery when payloads are missing or invalid. - Added background command PID regression coverage in `executeBash` to confirm a real, terminateable PID is returned. --- packages/coding-agent/src/task/index.ts | 6 +- .../coding-agent/src/tools/submit-result.ts | 7 +- packages/coding-agent/src/tools/todo-write.ts | 10 +- .../coding-agent/test/bash-executor.test.ts | 19 ++++ .../typescript-edit-benchmark/src/index.ts | 15 ++- .../typescript-edit-benchmark/src/runner.ts | 100 +++++++++++------- scripts/rate-edit-tool.py | 63 +++++------ 7 files changed, 132 insertions(+), 88 deletions(-) diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index 05824f301..1e5dd5ee9 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -530,8 +530,8 @@ export class TaskTool implements AgentTool { }); const thinkingLevelOverride = effectiveAgent.thinkingLevel; - // Output schema priority: agent frontmatter > params > inherited from parent session - const effectiveOutputSchema = effectiveAgent.output ?? outputSchema ?? this.session.outputSchema; + // Output schema priority: caller params > agent frontmatter > inherited from parent session + const effectiveOutputSchema = outputSchema ?? effectiveAgent.output ?? this.session.outputSchema; // Handle empty or missing tasks if (!params.tasks || params.tasks.length === 0) { @@ -1116,8 +1116,8 @@ export class TaskTool implements AgentTool { } // Build final output - match plugin format - const successCount = results.filter(r => r.exitCode === 0 && !r.error).length; const cancelledCount = results.filter(r => r.aborted).length; + const successCount = results.filter(r => r.exitCode === 0 && !r.error && !r.aborted).length; const totalDuration = Date.now() - startTime; const summaries = results.map(r => { diff --git a/packages/coding-agent/src/tools/submit-result.ts b/packages/coding-agent/src/tools/submit-result.ts index 4dac49761..43b4fc6f9 100644 --- a/packages/coding-agent/src/tools/submit-result.ts +++ b/packages/coding-agent/src/tools/submit-result.ts @@ -57,7 +57,8 @@ export class SubmitResultTool implements AgentTool readonly label = "Submit Result"; readonly description = "Finish the task with structured JSON output. Call exactly once at the end of the task.\n\n" + - "If you cannot complete the task, call with an error message payload."; + "Pass `result: { data: }` for success, or `result: { error: \"message\" }` for failure.\n" + + "The `data`/`error` wrapper is required — do not put your output directly in `result`."; readonly parameters: TSchema; strict = true; lenientArgValidation = true; @@ -171,7 +172,9 @@ export class SubmitResultTool implements AgentTool throw new Error("result cannot contain both data and error"); } if (errorMessage === undefined && data === undefined) { - throw new Error("result must contain either data or error"); + throw new Error( + "result must contain either `data` or `error`. Use `{result: {data: }}` for success or `{result: {error: \"message\"}}` for failure.", + ); } const status = errorMessage !== undefined ? "aborted" : "success"; diff --git a/packages/coding-agent/src/tools/todo-write.ts b/packages/coding-agent/src/tools/todo-write.ts index e57bdd8e5..aeb997312 100644 --- a/packages/coding-agent/src/tools/todo-write.ts +++ b/packages/coding-agent/src/tools/todo-write.ts @@ -251,7 +251,9 @@ function applyOps(file: TodoFile, ops: TodoWriteParams["ops"]): { file: TodoFile case "update": { const task = findTask(file.phases, op.id); if (!task) { - errors.push(`Task "${op.id}" not found`); + const totalTasks = file.phases.reduce((sum, p) => sum + p.tasks.length, 0); + const hint = totalTasks === 0 ? " (todo list is empty — was it replaced or not yet created?)" : ""; + errors.push(`Task "${op.id}" not found${hint}`); break; } if (op.status !== undefined) task.status = op.status; @@ -271,7 +273,11 @@ function applyOps(file: TodoFile, ops: TodoWriteParams["ops"]): { file: TodoFile break; } } - if (!removed) errors.push(`Task "${op.id}" not found`); + if (!removed) { + const totalTasks = file.phases.reduce((sum, p) => sum + p.tasks.length, 0); + const hint = totalTasks === 0 ? " (todo list is empty)" : ""; + errors.push(`Task "${op.id}" not found${hint}`); + } break; } } diff --git a/packages/coding-agent/test/bash-executor.test.ts b/packages/coding-agent/test/bash-executor.test.ts index e39403018..ccf18d8b2 100644 --- a/packages/coding-agent/test/bash-executor.test.ts +++ b/packages/coding-agent/test/bash-executor.test.ts @@ -100,6 +100,25 @@ describe("executeBash", () => { expect(Date.now() - start).toBeLessThan(3000); }); + it("returns a real PID for background external commands", async () => { + if (process.platform === "win32") { + return; + } + + const result = await executeBash("sleep 10 & echo $!", { + cwd: tempDir, + timeout: 5000, + }); + const pid = Number.parseInt(result.output.trim(), 10); + expect(Number.isInteger(pid)).toBe(true); + expect(pid).toBeGreaterThan(0); + expect(() => process.kill(pid, 0)).not.toThrow(); + process.kill(pid, "SIGTERM"); + await Bun.sleep(100); + expect(() => process.kill(pid, 0)).toThrow(); + }); + + it("times out commands", async () => { if (process.platform === "win32") { return; diff --git a/packages/typescript-edit-benchmark/src/index.ts b/packages/typescript-edit-benchmark/src/index.ts index 1829aaf82..4c5e8919c 100644 --- a/packages/typescript-edit-benchmark/src/index.ts +++ b/packages/typescript-edit-benchmark/src/index.ts @@ -74,7 +74,7 @@ Options: --tasks Comma-separated task IDs to run (default: all) --max-tasks Max tasks to sample (default: 80, 0 = all) --fixtures Fixtures directory or .tar.gz archive (default: built-in) - --edit-variant Edit variant: replace, patch, hashline, chunk, auto (default: auto) + --edit-variant Edit variant: replace, patch, hashline, chunk, vim, auto (default: auto) --edit-fuzzy Fuzzy matching: true, false, auto (default: auto) --edit-fuzzy-threshold Fuzzy threshold 0-1 or auto (default: auto) --auto-format Auto-format output files after verify (debug only) @@ -307,9 +307,16 @@ async function main(): Promise { tasksToRun = Array.from({ length: maxTasks }, (_, i) => sorted[Math.floor(i * step)]!); } - const editVariant = values["edit-variant"] as "replace" | "patch" | "hashline" | "chunk" | "auto" | undefined; - if (editVariant && !["replace", "patch", "hashline", "chunk", "auto"].includes(editVariant)) { - console.error(`Invalid edit-variant: ${editVariant}. Must be replace, patch, hashline, chunk, or auto.`); + const editVariant = values["edit-variant"] as + | "replace" + | "patch" + | "hashline" + | "chunk" + | "vim" + | "auto" + | undefined; + if (editVariant && !["replace", "patch", "hashline", "chunk", "vim", "auto"].includes(editVariant)) { + console.error(`Invalid edit-variant: ${editVariant}. Must be replace, patch, hashline, chunk, vim, or auto.`); process.exit(1); } diff --git a/packages/typescript-edit-benchmark/src/runner.ts b/packages/typescript-edit-benchmark/src/runner.ts index 9301bb422..7571184b1 100644 --- a/packages/typescript-edit-benchmark/src/runner.ts +++ b/packages/typescript-edit-benchmark/src/runner.ts @@ -70,7 +70,7 @@ export interface BenchmarkConfig { requireReadToolCall?: boolean; noEditRequired?: boolean; autoFormat?: boolean; - editVariant?: "replace" | "patch" | "hashline" | "chunk" | "auto"; + editVariant?: "replace" | "patch" | "hashline" | "chunk" | "vim" | "auto"; editFuzzy?: boolean | "auto"; editFuzzyThreshold?: number | "auto"; guided?: boolean; @@ -191,6 +191,17 @@ const HASHLINE_SUBTYPES = ["set", "set_range", "insert"] as const; const CHUNK_OP_SUBTYPES = ["append", "prepend", "replace", "delete"] as const; +const BENCHMARK_TOOL_NAMES = ["read", "edit", "vim", "write"] as const; +const EDIT_TOOL_NAMES = ["edit", "vim"] as const; + +function isEditTool(toolName: unknown): toolName is (typeof EDIT_TOOL_NAMES)[number] { + return toolName === "edit" || toolName === "vim"; +} + +function isMutationTool(toolName: unknown): boolean { + return isEditTool(toolName) || toolName === "write"; +} + function countChunkEditSubtypes(args: unknown): Record { const counts: Record = Object.fromEntries(CHUNK_OP_SUBTYPES.map(k => [k, 0])); if (!args || typeof args !== "object") return counts; @@ -603,7 +614,7 @@ async function buildGuidedContext( function buildInstructions(config: BenchmarkConfig): string { return config.noEditRequired ? "Read the relevant files first, then apply the fix." - : "Read the relevant files first, then use the edit tool to apply the fix."; + : "Read the relevant files first, then use the edit or vim tool to apply the fix."; } type BenchmarkPromptDelivery = { @@ -703,7 +714,7 @@ function buildBenchmarkRpcArgs(config: BenchmarkConfig, multiFile: boolean, prov "--append-system-prompt", buildBenchmarkSystemPrompt({ multiFile, config }), "--tools", - "read,edit,write", + BENCHMARK_TOOL_NAMES.join(","), "--no-skills", "--no-title", "--no-rules", @@ -923,7 +934,7 @@ async function runSingleTask( cwd, model: config.model, appendSystemPrompt: buildBenchmarkSystemPrompt({ multiFile: false, config }), - tools: ["read", "edit", "write"], + tools: [...BENCHMARK_TOOL_NAMES], editVariant: config.editVariant, editFuzzy: config.editFuzzy, editFuzzyThreshold: config.editFuzzyThreshold, @@ -1018,9 +1029,7 @@ async function runSingleTask( const providerFailure = detectProviderFailure(events); const hasMutationToolCall = events.some( event => - event.type === "tool_execution_start" && - ((event as { toolName?: unknown }).toolName === "edit" || - (event as { toolName?: unknown }).toolName === "write"), + event.type === "tool_execution_start" && isMutationTool((event as { toolName?: unknown }).toolName), ); if (providerFailure && !hasMutationToolCall) { await logEvent({ @@ -1066,11 +1075,14 @@ async function runSingleTask( if (event.type === "tool_execution_start") { const e = event as { toolName?: string; toolCallId?: string; args?: unknown }; const toolName = e.toolName; - if (toolName === "read") toolStats.read++; - else if (toolName === "edit") { + if (toolName === "read") { + toolStats.read++; + } else if (isEditTool(toolName)) { toolStats.edit++; if (e.toolCallId) pendingEdits.set(e.toolCallId, e.args); - } else if (toolName === "write") toolStats.write++; + } else if (toolName === "write") { + toolStats.write++; + } // Count input chars from args if (e.args) { @@ -1078,7 +1090,7 @@ async function runSingleTask( } } else if (event.type === "tool_execution_end") { const e = event as { toolName?: string; toolCallId?: string; isError?: boolean; result?: unknown }; - if (e.toolName === "edit" && e.toolCallId && pendingEdits.has(e.toolCallId)) { + if (isEditTool(e.toolName) && e.toolCallId && pendingEdits.has(e.toolCallId)) { const args = pendingEdits.get(e.toolCallId) ?? null; pendingEdits.delete(e.toolCallId); if (config.editVariant === "hashline" && args) { @@ -1104,13 +1116,15 @@ async function runSingleTask( editFailures.push({ toolCallId: e.toolCallId, args, error }); } else { toolStats.editSuccesses++; - const warningMessages = extractHashlineWarnings(e.result); - if (warningMessages.length > 0) { - editWarnings.push(...warningMessages); - toolStats.editWarnings += warningMessages.length; - if (hasHashlineAutocorrectWarning(warningMessages)) { - editAutocorrectCount++; - toolStats.editAutocorrects++; + if (e.toolName === "edit") { + const warningMessages = extractHashlineWarnings(e.result); + if (warningMessages.length > 0) { + editWarnings.push(...warningMessages); + toolStats.editWarnings += warningMessages.length; + if (hasHashlineAutocorrectWarning(warningMessages)) { + editAutocorrectCount++; + toolStats.editAutocorrects++; + } } } } @@ -1118,12 +1132,13 @@ async function runSingleTask( } } + // Retry if the model didn't attempt any edit/write (read-only or no tool calls) const madeEditAttempt = toolStats.edit > 0 || toolStats.write > 0; if (!madeEditAttempt && zeroToolRetries < noOpRetryLimit) { zeroToolRetries++; await logEvent({ type: "zero_tool_retry", attempt: attempt + 1, retryNumber: zeroToolRetries }); - retryContext = `Previous attempt read files but made no edit — you must use the edit tool to apply the fix. Retry ${zeroToolRetries}/${noOpRetryLimit}.`; + retryContext = `Previous attempt read files but made no edit attempt — you must use the edit or vim tool to apply the fix. Retry ${zeroToolRetries}/${noOpRetryLimit}.`; attempt--; // Don't consume a regular attempt slot continue; } @@ -1345,9 +1360,7 @@ async function _runRpcBenchmarkRun( const providerFailure = detectProviderFailure(events); const hasMutationToolCall = events.some( event => - event.type === "tool_execution_start" && - ((event as { toolName?: unknown }).toolName === "edit" || - (event as { toolName?: unknown }).toolName === "write"), + event.type === "tool_execution_start" && isMutationTool((event as { toolName?: unknown }).toolName), ); if (providerFailure && !hasMutationToolCall) { await logEvent({ @@ -1392,18 +1405,21 @@ async function _runRpcBenchmarkRun( if (event.type === "tool_execution_start") { const e = event as { toolName?: string; toolCallId?: string; args?: unknown }; const toolName = e.toolName; - if (toolName === "read") toolStats.read++; - else if (toolName === "edit") { + if (toolName === "read") { + toolStats.read++; + } else if (isEditTool(toolName)) { toolStats.edit++; if (e.toolCallId) pendingEdits.set(e.toolCallId, e.args); - } else if (toolName === "write") toolStats.write++; - + } else if (toolName === "write") { + toolStats.write++; + } + if (e.args) { toolStats.totalInputChars += JSON.stringify(e.args).length; } } else if (event.type === "tool_execution_end") { const e = event as { toolName?: string; toolCallId?: string; isError?: boolean; result?: unknown }; - if (e.toolName === "edit" && e.toolCallId && pendingEdits.has(e.toolCallId)) { + if (isEditTool(e.toolName) && e.toolCallId && pendingEdits.has(e.toolCallId)) { const args = pendingEdits.get(e.toolCallId) ?? null; pendingEdits.delete(e.toolCallId); if (config.editVariant === "hashline" && args) { @@ -1429,38 +1445,40 @@ async function _runRpcBenchmarkRun( editFailures.push({ toolCallId: e.toolCallId, args, error: toolError }); } else { toolStats.editSuccesses++; - const warningMessages = extractHashlineWarnings(e.result); - if (warningMessages.length > 0) { - editWarnings.push(...warningMessages); - toolStats.editWarnings += warningMessages.length; - if (hasHashlineAutocorrectWarning(warningMessages)) { - editAutocorrectCount++; - toolStats.editAutocorrects++; + if (e.toolName === "edit") { + const warningMessages = extractHashlineWarnings(e.result); + if (warningMessages.length > 0) { + editWarnings.push(...warningMessages); + toolStats.editWarnings += warningMessages.length; + if (hasHashlineAutocorrectWarning(warningMessages)) { + editAutocorrectCount++; + toolStats.editAutocorrects++; + } } } } } } } - + // Retry if the model didn't attempt any edit/write (read-only or no tool calls) const madeEditAttempt = toolStats.edit > 0 || toolStats.write > 0; if (!madeEditAttempt && zeroToolRetries < noOpRetryLimit) { zeroToolRetries++; await logEvent({ type: "zero_tool_retry", attempt: attempt + 1, retryNumber: zeroToolRetries }); - retryContext = `Previous attempt read files but made no edit — you must use the edit tool to apply the fix. Retry ${zeroToolRetries}/${noOpRetryLimit}.`; + retryContext = `Previous attempt read files but made no edit attempt — you must use the edit or vim tool to apply the fix. Retry ${zeroToolRetries}/${noOpRetryLimit}.`; attempt--; // Don't consume a regular attempt slot continue; } - + patchApplied = toolStats.edit > 0; - + const filesToVerify = task.files.length > 0 ? task.files : undefined; const verification = await verifyExpectedFileSubset(expectedDir, cwd, filesToVerify); if (config.autoFormat) { await formatDirectory(cwd); } - + verificationPassed = verification.success; indentScore = verification.indentScore; formattedEquivalent = verification.formattedEquivalent; @@ -1470,11 +1488,11 @@ async function _runRpcBenchmarkRun( if (!verification.success && verification.error) { error = verification.error; } - + if (verification.success) { break; } - + const mutationIntentSuffix = mutationIntentValidation ? `\n\nMutation intent: ${mutationIntentValidation.matched ? "matched" : "not matched"} (${mutationIntentValidation.reason})` : ""; diff --git a/scripts/rate-edit-tool.py b/scripts/rate-edit-tool.py index c52da6428..98f80c3ef 100755 --- a/scripts/rate-edit-tool.py +++ b/scripts/rate-edit-tool.py @@ -5,6 +5,7 @@ import argparse import asyncio import json import os +import random import shutil import sys import tempfile @@ -52,7 +53,6 @@ MODELS = [ "openrouter/anthropic/claude-haiku-4.5", "openrouter/anthropic/claude-sonnet-4.6", "openrouter/google/gemini-3-flash-preview", - "openrouter/deepseek/deepseek-v3.2", "openrouter/z-ai/glm-5-turbo", "openrouter/minimax/minimax-m2.7", ] @@ -61,40 +61,28 @@ ORACLE_MODEL = "openrouter/anthropic/claude-opus-4.6" PROMPT = textwrap.dedent( """\ - You are evaluating the current code-reading and code-editing tools on the files in this directory. + You are evaluating the code-reading and code-editing tools on files in this directory. - Use `main.ts`, `main.rs`, `main.py`, and `main.md` as the test surfaces. - - Treat `main.py` and `main.md` as explicit edge-case fixtures: - - `main.py` stresses indentation-sensitive editing, decorators, docstrings, and no-brace block structure. - - `main.md` stresses prose-oriented routing, headings, task lists, tables, fenced code blocks, and non-AST text editing. + {FIXTURE_SURFACE} Work in this order: - 1. Map the real surface area. - - For each tool, identify the operations, selectors, addressing modes, and result shapes that actually work now. - - Compare how behavior differs across AST-heavy files (`main.ts`, `main.rs`), indentation-sensitive code (`main.py`), and prose/text (`main.md`). + 1. Map the surface area. Identify operations, selectors, and addressing modes that actually work. Note differences across file types. - 2. Exercise the supported paths. - - For reading: cover whole-file, structural chunks, nested members, line ranges, and raw source if available. - - For editing: cover replacing existing code, inserting into containers, inserting before/after anchors, deleting code, and the smallest addressable edits you can reach. - - On `main.md`, explicitly check whether prose-routed files can still be edited reliably and how addressing differs from code files. + 2. Exercise the supported paths. Read: whole-file, structural chunks, nested members, line ranges, raw source. Edit/vim: replace, insert into containers, insert before/after, delete. On `main.md`, check how addressing differs from code files. - 3. Push into awkward cases. - - Check first/last-child edits, container-relative vs file-relative behavior, indentation and delimiter preservation, and attached nodes such as doc comments, decorators, attributes, impl members, enum variants, markdown lists, tables, and fenced code blocks. - - If something fails, note whether the error message was clear and whether it told you how to recover. + 3. Push into awkward cases. Test first/last-child edits, indentation preservation, decorators, docstrings, enum variants, markdown tables, and fenced code blocks. Note whether error messages were clear and actionable. - 4. Verify the files after meaningful edits. - - Re-read the files in full after each meaningful edit round and confirm the tool did not make unintended changes. + 4. Verify after edits. Re-read each file after meaningful edits and confirm no unintended changes. - When finished, report concrete findings: - - what felt awkward or required workarounds + Report concrete findings: + - what required workarounds - what was impossible - which errors were clear vs unclear - what was ambiguous or under-documented - - what changes would make the tools more trustworthy and easier to use + - suggested improvements - Be specific about observable behavior. Generic success summaries are not useful. + Be specific. Generic success summaries are not useful. """ ).strip() @@ -157,8 +145,8 @@ ORACLE_REVIEW_PROMPT = textwrap.dedent( ).strip() TODOS = [ - "Map the current read and edit tool surface area on main.ts, main.rs, main.py, and main.md.", - "Exercise supported read and edit paths with concrete before/after verification across code and prose fixtures.", + "Map the current read, edit, and vim surface area on main.ts, main.rs, main.py, and main.md.", + "Exercise supported read, edit, and vim paths with concrete before/after verification across code and prose fixtures.", "Probe awkward selector, indentation, and boundary cases including decorators, docstrings, tables, and fenced blocks.", "Summarize what was awkward, impossible, ambiguous, or under-documented with concrete examples.", ] @@ -640,6 +628,13 @@ FIXTURES: tuple[tuple[str, str], ...] = ( ("markdown", "main.md"), ) +FIXTURE_DESCRIPTIONS: dict[str, str] = { + "typescript": "TypeScript/AST", + "rust": "Rust/AST", + "python": "indentation-sensitive", + "markdown": "prose/non-AST", +} + WORKSPACE_FILES = { "main.ts": TS_FIXTURE, "main.rs": RUST_FIXTURE, @@ -649,14 +644,9 @@ WORKSPACE_FILES = { def build_fixture_prompt(fixture_language: str, fixture_file: str) -> str: - return textwrap.dedent( - f"""\ - {PROMPT} - - This run is constrained to `{fixture_file}` (`{fixture_language}`). - Keep all operations and edits scoped to this fixture unless absolutely required to inspect shared context. - """ - ).strip() + description = FIXTURE_DESCRIPTIONS.get(fixture_language, fixture_language) + surface = f"Test surface: `{fixture_file}` ({description})." + return PROMPT.format(FIXTURE_SURFACE=surface) + "\n\nKeep all operations and edits scoped to this fixture." @dataclass @@ -707,7 +697,7 @@ class ModelProgress: todo_items: dict[str, tuple[str, str]] = field(default_factory=dict) -TOOL_WHITELIST = ("read", "edit", "todo_write", "report_tool_issue") +TOOL_WHITELIST = ("read", "edit", "vim", "todo_write", "report_tool_issue") MODEL_LABEL_WIDTH = 30 STATUS_WIDTH = 7 TOKENS_WIDTH = 9 @@ -1614,10 +1604,11 @@ async def run_all(args: argparse.Namespace) -> int: workspace_root.mkdir(parents=True, exist_ok=True) selected_models = args.models or MODELS + model_fixtures = {model: random.sample(list(FIXTURES), min(2, len(FIXTURES))) for model in selected_models} run_specs = [ (f"{model}|{fixture_language}", f"{shorten_model_name(model)}:{fixture_language}") for model in selected_models - for fixture_language, _ in FIXTURES + for fixture_language, _ in model_fixtures[model] ] printer = ProgressPrinter(run_specs) printer.configure(fixtures_dir=fixtures_dir, results_dir=results_dir) @@ -1636,7 +1627,7 @@ async def run_all(args: argparse.Namespace) -> int: openrouter_key=openrouter_key, ) for model in selected_models - for fixture_language, fixture_file in FIXTURES + for fixture_language, fixture_file in model_fixtures[model] ] results = await asyncio.gather(*tasks)