Remove four wrong assumptions from the tool-call path by nvdorman · Pull Request #30 · enowdev/antares

nvdorman · 2026-08-13T17:14:33Z

An end-to-end review of the harness, the tool implementations, the provider adapters, the MCP bridge, and context assembly found roughly 36 substantiated defects. They were not 36 unrelated mistakes — they were four assumptions, each repeated in many places:

Position stands in for identity. A tool result was recognised by sitting next to its call rather than by carrying its id.
A byte stands in for a character. Text was cut at byte offsets.
A name stands in for a capability. What a tool may do was decided by how it is spelled.
A failure is reported as a success. Parse errors, early stream ends, and skipped files produced cheerful empty results.

Every defect below was reproduced by running code before it was fixed, and each fix is pinned by a test that fails without it.

What this changes

Tool results are matched by tool_call_id. When the repetition guard fired, it appended its nudge to history before the results, and ensureToolResults — which only accepted a tool message sitting immediately after the assistant turn — discarded every real result and replaced it with [no result recorded — the previous run was interrupted], a statement that was not true. The model concluded its work had not happened and repeated it, which drove the same guard to abort the run. The guard fired on ordinary work because repeatKey fingerprinted edit_file/write_file by path alone, so three different edits to one file counted as one call made three times. Both are fixed, and results now bind to their call within its own turn before any wider search.

Text is cut on runes. trimForModel, compact.truncate, readCapped, auto-context, the reranker, plugin.truncate, shell.trimOutput, browser.truncateTool, grep's match lines, and web.truncateText all sliced UTF-8 at byte offsets. Non-ASCII output reached the model corrupted, and one notice reported bytes while calling them characters. A single internal/textutil helper now serves every site.

Danger is classified by capability. dangerIn only scanned a tool named exactly terminal, so vps_run — which executes shell on a remote host — was never checked. Tools that carry a shell command now declare it through an interface, as do tools returning untrusted output. The recursive-delete pattern was also plainly broken: it matched rm -rf / but not rm -rf /home/someone.

A failure is a failure. A pre_tool_call plugin that printed a denial and exited non-zero used to permit the call. MCP content the client could not represent became an empty success, and embedded resource text was dropped entirely. grep skipped files over 8 MiB and then reported "No matches". A stream body that ended early was a finished answer, and missing tool arguments were fabricated as {}.

Found during review, not in the original plan

The MCP stdio transport dropped replies. The request frame was written before the response channel was registered, so a fast reply was looked up against an unregistered id and discarded while the caller waited out its deadline. Three consecutive losses tore the transport down for the rest of the session.
The repetition guard could not stop anything. exceeded() was only evaluated inside the branch that fires at exactly limit, while it needs limit*2. Sixty identical calls produced one nudge and no abort.
A slow SSE follower crashed its own stream with a negative slice index when the event window was trimmed past its cursor.
timeout_ms did not bound plugin dispatch, because the wait blocks on the stdout copy goroutines while an inherited child holds the pipe.

Deliberately not included

Recorded with reasons in the design document: the Anthropic thinking-block replay and max_tokens parameter naming (neither verifiable without working credentials), non-atomic file writes, per-model context windows, compaction boundary defects, and MCP server-name collisions.

Verification

The full suite passes hermetically across 37 packages, go vet is clean, and gofmt reports only the 29 files already unformatted on main. Five acceptance probes in internal/agent/harness_hypothesis_probe_test.go were committed red at the start of the work and are green at the end.

Upstream moved 47 commits during the work; main is merged in here and the merged tree is green. The only conflict was one import line.

Made with Cursor

An end-to-end review found ~36 substantiated defects across the harness, the tool implementations, the provider adapters, the MCP bridge, and context assembly. They reduce to four repeated assumptions: position stands in for identity, a byte stands in for a character, a name stands in for a capability, and a failure is reported as a success. The probe tests are committed red on purpose. They are the acceptance criteria for the first three patterns, and every claim in the spec was reproduced by running code rather than by reading it. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

Every truncation on the path to the model measured its budget in bytes and sliced the string at that offset. On non-ASCII text that is wrong twice: the model receives a third of the text it was promised (a byte cap of 60000 holds 20000 Japanese characters), and the cut can land inside a rune, leaving a partial sequence that JSON encoding rewrites as U+FFFD. trimForModel also reported the bytes it dropped as "characters truncated", so the notice overstated the loss by up to four times. internal/textutil is one helper used everywhere: limits count runes, cuts land on rune boundaries, and the head-and-tail form reports how many runes it removed so the caller's notice states a number it actually measured. It is stdlib only. Tools.MaxOutputChars keeps its name and its 60000 default and now means what it has always claimed: characters. Non-ASCII tool output therefore reaches the model whole where it used to arrive cut short. TestProbeTrimForModelKeepsValidUTF8 turns green. The other four probes belong to later tasks and stay red on purpose. Co-authored-by: Cursor <cursoragent@cursor.com>

internal/tools was left on byte budgets by the previous commit, so Tools.MaxOutputChars meant characters in internal/agent and bytes here. One config field cannot mean two things, and shell and browser output is precisely where multi-byte text turns up: terminal output is handed to the provider, so these sites are inside the pattern even though the plan's file list missed them. trimOutput was a byte-for-byte copy of the old trimForModel, mid-rune slice and all, printing the bytes it dropped as "characters omitted" — 149 where the true figure was 49. truncateTool cut at a byte offset and reported a byte length as "characters total"; it feeds browser page text, diagnostics, image-endpoint errors, the schedule listing, and every hook tool's body. Both now use internal/textutil. No new local helper. The end-to-end test drives the terminal tool through a real shell so the call site is covered, not just the function. Co-authored-by: Cursor <cursoragent@cursor.com>

Every other site in this refactor was a mechanical swap to internal/textutil and is covered by that package's tests. Two were not: the prune guard and notice in prunedToolResults, and the accumulator in autoContext. Both were hand-written arithmetic with nothing holding them in place. prunedToolResults also reinterprets a second config field, Compression.ProactivePruneMinChars, from bytes to characters, matching Tools.MaxOutputChars. That reading is now asserted rather than implied: a result of 100 characters is inside a 100-character minimum however many bytes it weighs, and the "characters removed" notice states what was actually dropped. autoContext is reached through a stub RAGProvider, so the budget is exercised at the real call site: bodies that fit arrive whole, and one that overruns is cut on a character boundary with only the characters that fit charged to the budget. Verified by reverting each site to its byte arithmetic and watching all four go red for the right reasons — a 550-byte count where 150 characters were removed, a result inside the minimum pruned anyway, and a dangling \xe3 in the assembled block — then restoring. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

The deleted special cases were dispatched on tool name, so a test naming one tool cannot reach another's branch: write_file and vps_upload could each be given back a path-only fingerprint with the suite fully green. Assert distinct keys per tool name, and drop the different-paths test that was vacuous under the uniform key and survived the regression anyway. Extract the per-turn history assembly so the nudge's position after the tool results is assertable without a client. ensureToolResults repairs the interleaving silently, so nothing else would have noticed it moving back. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

dangerIn returned before reading anything unless the tool was spelled "terminal", so vps_run — which hands an arbitrary command to a shell on a remote host — was never scanned, and under the default auto mode a remote root wipe ran without so much as a transcript notice. Arguments that failed to parse also came back empty, which reads as safe. A tool that runs commands now says so by implementing ShellCommander, and the scan asks the resolved tool rather than its name: checkApproval hands over the object that is about to run instead of a name to look up again. Arguments that cannot be read come back with a fixed reason instead of silence. The regex table stays what it was, the human-readable reason and never the gate, but its recursive-delete entry required the target to be a bare /, ~, $HOME or *, so rm -rf /home/someone and rm -rf ~/projects fell straight through. It now matches a delete of any path under root or a home directory. untrustedTool had the same shape and moves the same way: web_fetch, web_search, browser and http_request declare UntrustedOutput, while tools borrowed from MCP servers keep the prefix rule, since a tool written outside this codebase cannot implement a Go interface. Co-authored-by: Cursor <cursoragent@cursor.com>

The scan decoded arguments with json.Unmarshal while Execute decodes them with a json.Decoder, and only the second ignores trailing data. Appending one byte to {"command":"rm -rf /"} was therefore enough to have the scan answer "its arguments could not be read" — the generic reason — about a root wipe that would then run, and in prompt mode that generic sentence is what the person approving reads. commandArgument now decodes through Input.Bind itself, so the two cannot drift apart again. Split the delete pattern in two so each reason is true of the command that matched it. A bare /, ~, $HOME or * keeps the old [rf] class and says it deletes everything at that path; a recursive delete of any path says it deletes a whole directory tree. Together they cover strictly more than the single entry did before this branch, while rm -f /tmp/build.log, rm -f *.o and git rm -f /tmp/x stop being announced as wipes of home or root. The table's one job now is to describe a call to a person, and a description people learn to skim is worse than none. Say what the table does to the comment above it: a reason does force a call through approval, and the reason that changes no outcome is that both tools reaching a shell already require approval on their own. Co-authored-by: Cursor <cursoragent@cursor.com>

call returned before it read stdout whenever the process failed, and Dispatch read that as the plugin having no opinion. For an observer that is right. For pre_tool_call, the only policy gate in this codebase, it meant a script that printed {"deny":true,"reason":"policy"} and then exited 1 permitted the call, and a guard that could not be started, timed out, or answered with gibberish permitted it too. The gate failed open on exactly the plugins that were trying hardest to say no. Two separate facts were being conflated: whether the plugin failed, and whether it managed to say anything. call now reports them apart. Stdout is read before the exit status is judged, for every event, so a plugin that printed a verdict and then fell over still has its verdict honoured — that is as true of a post_tool_call rewrite as it is of a refusal. Only pre_tool_call changes what happens when nothing readable came back: it synthesises a Deny naming the plugin and the failure. Every other event is watching something already decided, so it keeps today's log-and-carry-on. A plugin that is not installed, not matched, or disabled was never asked and still denies nothing. Silence from a plugin that exited cleanly is consent, as before, and so is an explicit {} from one that did not. The reason reaches the model as "refused by policy: ...", so the stderr excerpt it can carry is now bounded by the same rune-safe cut the non-JSON message uses, and a run cut short by the caller no longer claims to have timed out. Verified by putting the flag back to a plain "any failure denies" and watching three go red for the right reasons: a refusal whose own words were overwritten, an empty verdict read as refusal, and a rewrite from a failing observer thrown away. Co-authored-by: Cursor <cursoragent@cursor.com>

Co-authored-by: Cursor <cursoragent@cursor.com>

A response body that ends is not a response that finished. A gateway that loses its upstream closes the body cleanly, so the adapters read end-of-body as success and reported a cut turn as a complete one. Worse, the accumulator filled in arguments that never arrived with "{}", turning a half-sent tool call into a dispatchable one: write_file with no path. Require the provider's own terminal marker — [DONE] or a finish_reason for the OpenAI dialect, message_stop for Anthropic — and classify its absence as retryable so the existing turn-level retry replays the request instead of failing the turn. A tool call the provider never closed is no longer padded with "{}"; it fails the stream. Co-authored-by: Cursor <cursoragent@cursor.com>

The terminal-marker check only ran for bodies that ended tidily. A read that died mid-body returned the scanner's own error ahead of that check, and a bare "unexpected EOF" is neither a TransportError nor an apiError and matches nothing transientMessage looks for — so a reset or a half-sent chunked frame, the ordinary physical form of this failure, failed the turn instead of retrying it. Wrap a failed read as ErrStreamTruncated, passing through cancellation, the idle timeout and an over-long line, none of which is a truncation worth asking again for. At the token cap the stream's terminal vouches for nothing: the answer can stop between a tool call's name and its arguments and still be framed correctly, so marking every call complete handed back write_file with no arguments for the agent to fill in with "{}". Leave the calls unmarked when the finish reason is length and let the accumulator refuse what the model never finished asking for. Co-authored-by: Cursor <cursoragent@cursor.com>

The reader goroutine outlives the first call, so a reply that arrives between the write and the registration was looked up against an id the pending map did not hold yet and discarded as stale. The caller then sat out its whole deadline for an answer that had already come back, and three of those in a row closed the transport for the rest of the session. Registering first cannot be too late: nothing can answer a frame that has not been written. A write that fails takes its entry back out. Co-authored-by: Cursor <cursoragent@cursor.com>

Gemini collected finishReason and never required it, so a body that ended after a well-formed functionCall was a finished answer and a sentence cut in half was a complete reply. Its parts arrive whole, so nothing inside a chunk can show the stream stopped early; only the missing finishReason can. The {} substituted for absent arguments was the same fabrication removed from the shared accumulator: it turned a cut stream into a dispatchable write_file with no path. The adapter now reports what arrived, which is what makes the caller's fill safe. Co-authored-by: Cursor <cursoragent@cursor.com>

The fallback pass dropped the whole span bound, not just its forward half, so a tool message sitting in front of a call could be bound to it. That shape is one persistContextCompact produces: it cuts at throughSeq without rebalancing to a tool boundary, and loadHistory rebuilds the tail from every row past it, so a reloaded history can open with a tool message whose assistant turn was summarised away. Under a recurring call_<index>_<name> id the model then got last hour's file contents as this turn's read, unmarked — and with a live result present, the orphan won the race to it. Reaching past the end of a span is still allowed, which is what the pass is for. Reaching back before the call is not. Co-authored-by: Cursor <cursoragent@cursor.com>

record reports a key once, on the turn its count reaches the limit, and never again. Asking exceeded() only inside that branch asked exactly once per key, at half the count it needs, so the abort could only fire when a second distinct call tripped after a first had already run past twice the limit. A model stuck on one call was never stopped: sixty identical calls produced one nudge and no abort, against a spec whose Pattern 1 narrative has the abort ending the loop. Both answers now come from one place, since neither can be derived from the other. Co-authored-by: Cursor <cursoragent@cursor.com>

Every match line was cut at 400 bytes. Nothing makes a source line stop at an ASCII boundary, and grep is in every toolset including minimal, so this is the widest path from a repository into a provider request: two of the three byte offsets of a CJK line produce bytes that are not UTF-8, and a 300-character line lost two thirds of itself under a cap that promised 400. Co-authored-by: Cursor <cursoragent@cursor.com>

truncateText cut at a byte offset and then reported the bytes past that offset as characters. web_fetch on a page of CJK with max_chars 1000 returned 1094 bytes that were not valid UTF-8, under a notice reading "truncated (270 more characters)" for a 100-character string that had lost 90. Web page text and error bodies, search snippets, http_request bodies, knowledge hits, intercepted bodies and Ghidra output all share it. Co-authored-by: Cursor <cursoragent@cursor.com>

Killing the plugin at its deadline did not end the wait: Run also waits on the goroutines copying stdout and stderr, and those cannot finish while anything holds the write end of the pipe. A child the plugin backgrounds inherits that descriptor and outlives the shell, so a manifest declaring timeout_ms: 200 blocked Dispatch — the agent's own turn — for as long as the grandchild lived. The existing test used `exec sleep 5`, the one shape of slow plugin that leaves nothing behind. WaitDelay closes the pipes shortly after the plugin is done with them. What it printed was copied out long before, and is still read and honoured, so cutting the wait short cannot turn a captured verdict into a fabricated refusal. Co-authored-by: Cursor <cursoragent@cursor.com>

null unmarshals into the zero Reply and reports no error, so a plugin printing it was indistinguishable from one that had answered and had no objection — and a gate that printed it and then failed was recorded as having permitted the call, because the deny branch only fires for a plugin that never got a word out. A jq pipeline that matches nothing prints exactly that. An empty object still means no objection. Co-authored-by: Cursor <cursoragent@cursor.com>

appendTurnMessages is pinned, but Run is free to ignore it: appending the nudge straight to history above executeTools rebuilds the transcript ensureToolResults exists to repair, and the repair is silent, so the suite stays green. Driving Run would need a fake provider, so these read the call site — narrower than a behavioural test, and enough to fail on the append that used to be there. The handoff note claimed repeat_guard_test.go catches a coarser key for any tool. It names write_file, edit_file and vps_upload, the three that ever carried a special case, and asserts nothing about the rest. Co-authored-by: Cursor <cursoragent@cursor.com>

Upstream moved 47 commits while this work was in progress. The only overlap that git could not settle was a single import line: upstream's MCP client tests now use slices, and this branch's tests do not. The merged tree passes the whole hermetic suite, go vet, and all five acceptance probes. Co-authored-by: Cursor <cursoragent@cursor.com>

nvdorman and others added 29 commits August 13, 2026 18:18

Plan the tool-path determinism refactor as nine tested tasks

3a406d0

Co-authored-by: Cursor <cursoragent@cursor.com>

Match a tool result to its call by id

907dff0

Co-authored-by: Cursor <cursoragent@cursor.com>

Bind a tool result inside its own turn before looking further

e715bbd

Co-authored-by: Cursor <cursoragent@cursor.com>

Tell a stuck loop apart from ordinary progress

e5f5aa9

Co-authored-by: Cursor <cursoragent@cursor.com>

Stop a stalled follower from crashing its own stream

bf29f63

Co-authored-by: Cursor <cursoragent@cursor.com>

Read what an MCP server actually returned

7fb043f

Co-authored-by: Cursor <cursoragent@cursor.com>

Tell an empty file apart from content it cannot read

af66520

Co-authored-by: Cursor <cursoragent@cursor.com>

Let a blob be read when the text beside it is empty

edbe221

Co-authored-by: Cursor <cursoragent@cursor.com>

Say when grep did not look at a file

ec4b0b6

Co-authored-by: Cursor <cursoragent@cursor.com>

enowdev merged commit 9d40e79 into enowdev:main Aug 13, 2026
2 checks passed

nvdorman mentioned this pull request Aug 14, 2026

Restore verbatim read_file and exact edit_file #32

Open

7 tasks

Navigation Menu

Remove four wrong assumptions from the tool-call path - #30

Remove four wrong assumptions from the tool-call path#30
enowdev merged 29 commits into
enowdev:mainfrom
nvdorman:refactor/harness-tool-calls

nvdorman commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

nvdorman commented Aug 13, 2026

What this changes

Found during review, not in the original plan

Deliberately not included

Verification

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants