From 8f711c14c63bfbd933df5c7273badbe813e060ed Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:18:30 +0700 Subject: [PATCH 01/28] Name the four assumptions the tool path is built on 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 --- ...2026-08-13-tool-path-determinism-design.md | 229 ++++++++++++++++++ .../agent/harness_hypothesis_probe_test.go | 80 ++++++ 2 files changed, 309 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-13-tool-path-determinism-design.md create mode 100644 internal/agent/harness_hypothesis_probe_test.go diff --git a/docs/superpowers/specs/2026-08-13-tool-path-determinism-design.md b/docs/superpowers/specs/2026-08-13-tool-path-determinism-design.md new file mode 100644 index 0000000..49ebc67 --- /dev/null +++ b/docs/superpowers/specs/2026-08-13-tool-path-determinism-design.md @@ -0,0 +1,229 @@ +# Tool Path Determinism Design + +## Summary + +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 are not 36 unrelated mistakes. They are four assumptions, each +repeated in many places: + +1. **Position stands in for identity.** A tool result is recognised by sitting + next to its call rather than by carrying its id. +2. **A byte stands in for a character.** Text is cut at byte offsets. +3. **A name stands in for a capability.** What a tool may do is decided by how + it is spelled. +4. **A failure is reported as a success.** Parse errors, early stream ends, and + skipped files all produce cheerful empty results. + +This design removes those four assumptions from the paths that carry tool calls +and tool results. It does not attempt all 36 findings; the rest are recorded +here as deferred, with the reason. + +## Goals + +- A tool result reaches the model whenever the tool actually ran. +- Text handed to a provider, stored, or displayed is always valid UTF-8. +- A destructive command is classified by what the tool can do, not its name. +- A failure surfaces as a failure, naming what went wrong. +- Every fix is pinned by a test that fails before it and passes after. + +## Non-goals + +- Fixing all 36 findings in one pass. +- Changing the tool-calling wire protocol or any provider's request shape + beyond the specific defects named below. +- Reworking compaction strategy, context-window sizing, or RAG ranking. +- Any behaviour that cannot be proven by a test in this repository. + +## Evidence + +Every defect below was reproduced by running code, not by reading it. Five were +proven directly in the repository worktree +(`internal/agent/harness_hypothesis_probe_test.go`, currently red on purpose); +the rest were proven in throwaway copies by four parallel reviewers. + +One finding was checked against the live provider the deployment actually uses +(`glm-5.2` through the local OpenAI-compatible proxy) and found **latent**: that +proxy emits `index` on every tool-call frame, sends `[DONE]`, and does not +repeat the function name. The streaming-correlation defects are therefore real +for other providers but are not currently harming this deployment, which is why +they sit at the end of the plan rather than the front. + +## Pattern 1 — Identity comes from `tool_call_id` + +### The defect + +`ensureToolResults` (`internal/agent/agent.go:1199-1246`) accepts a tool message +only when it sits immediately after the assistant turn that called it, and +silently drops any other tool message. The agent loop violates that adjacency +itself: when the repetition guard fires it appends a user-role nudge at +`agent.go:620` *before* `executeTools` appends the results, producing +`[assistant(tool_calls), user(nudge), tool(result)]`. + +Every real result is then discarded and replaced with +`"[no result recorded — the previous run was interrupted before this tool +finished]"` — a statement that is not true. The model concludes its work did not +happen and repeats it, which drives the same guard to `exceeded()` and aborts +the run. + +The guard fires on ordinary work because `repeatKey` +(`internal/agent/harness.go:159-177`) fingerprints `edit_file` and `write_file` +by path alone, discarding the arguments. Three different edits to one file are +therefore "the same call three times" at the default `repeat_limit: 3`. + +### The change + +`ensureToolResults` indexes every tool message in the transcript by +`ToolCallID`, then for each assistant turn emits that turn followed by its +results in call order. Messages of other roles that were interleaved are emitted +after the results they were mixed into, so nothing is dropped and ordering stays +valid for providers. A stub is written only when no result exists for a call id, +and it says that no result was recorded — it does not assert an interruption. + +`repeatKey` loses its `write_file`/`edit_file` special case and fingerprints +every tool the same way, on the normalised arguments. Identical arguments remain +a repeat; different arguments are progress. The `vps_upload` special case goes +for the same reason. + +The nudge is appended after the tool results rather than before, so the guard +cannot produce an invalid transcript even if a future caller reintroduces +adjacency assumptions. + +### Same pattern, separate site + +`follow` (`internal/server/livechat.go:71-75`) clamps its cursor to `lr.base` +only in the outer loop, and drops the lock around `send`. A publisher trimming +the event window while a slow follower is inside a send moves `lr.base` past the +cursor, and `lr.events[i-lr.base]` panics with a negative index, killing the SSE +connection mid-turn. The cursor is re-clamped after every lock re-acquisition. + +## Pattern 2 — Cut on runes, report runes + +### The defect + +Byte slicing on UTF-8 strings appears in at least six places: +`trimForModel` (`agent.go:1169-1179`), `truncate` (`compact.go:291-296`), +`readCapped` (`prompt.go:233-242`), the auto-context assembler +(`ragcontext.go:177-179`), the reranker's candidate body (`rag/rerank.go`), and +`plugin.truncate` (`plugin/plugin.go:342-347`). + +Any non-ASCII content over the limit reaches the model with broken bytes at the +seams, which `json.Marshal` rewrites as U+FFFD. `trimForModel` additionally +reports the count in bytes while calling them characters, which for CJK text is +wrong by a factor of three. + +### The change + +One helper, `textutil.TruncateRunes`, is introduced and used at every site. It +never splits a rune, and it reports the number of runes removed. Head-and-tail +truncation keeps both ends on rune boundaries. The notice text names runes, +because that is what was measured. + +## Pattern 3 — Capability, not spelling + +### The defect + +`dangerIn` (`internal/agent/approval.go:199-207`) returns immediately unless the +tool is named exactly `terminal`. `vps_run` executes arbitrary shell on a remote +host and is never scanned; in the default `approval_mode: auto` a remote root +wipe runs without even a transcript notice. Unparseable arguments also return +`""`, which reads as "safe". + +`untrustedTool` (`agent.go:1141-1147`) has the same shape: a fixed list of four +names decides which output is treated as attacker-controlled. + +### The change + +A tool that carries a shell command declares it: + +```go +// ShellCommander is implemented by tools that execute a shell command, whether +// locally or on a remote host. +type ShellCommander interface { + ShellCommand(args json.RawMessage) (string, bool) +} +``` + +`terminal` and `vps_run` implement it. `dangerIn` takes the resolved tool and +asks it for the command instead of comparing names, so a future tool that runs +commands is covered when it is written rather than when someone remembers to +extend a list. Arguments that fail to parse are treated as requiring approval +rather than as safe. + +Untrusted output moves to the same shape: an `UntrustedOutput() bool` capability +on the tool, with the MCP prefix rule retained for dynamically registered tools. + +The regex table stays, but only as the human-readable *reason*. It is not the +gate; the structural `NeedsApproval` check is. A denylist can never be complete, +and this design does not pretend otherwise. + +One entry in it is nonetheless plainly broken and is corrected. The recursive +delete pattern requires the path to be exactly `/`, `~`, `$HOME`, or `*` +followed by whitespace or end of line, so `rm -rf /home/nvdorman` and +`rm -rf ~/projects` — deleting a home directory and a project tree — are not +matched at all, while `rm -rf /` is. The pattern is corrected to match a +recursive delete of any absolute or home-relative path, which is what the +message it prints already claims to describe. + +## Pattern 4 — A failure is a failure + +Four sites turn a failure into a success: + +**The plugin policy gate.** `call` (`plugin/plugin.go:320-329`) returns before +parsing stdout whenever the process exits non-zero or times out, and `Dispatch` +(`plugin.go:245-250`) then continues as though the plugin had no opinion. For +observational events that is right. For `pre_tool_call`, which is the only +policy gate in the codebase, it means a script that prints `{"deny":true}` and +exits non-zero permits the call. Stdout is parsed regardless of exit status, and +a failure on `pre_tool_call` denies. + +**MCP content.** `Call` (`mcp/client.go:256-272`) understands only `text`, so an +embedded resource loses its text, an unknown type vanishes, and the fallback +`"(no content returned)"` is returned with `IsError` false. Embedded resource +text and blobs are decoded; content the client cannot represent becomes an error +naming the type, distinct from a genuinely empty result. + +**grep.** Files over 8 MiB are skipped silently (`tools/search.go:311-313`), and +when the skipped file held the only match the tool reports `No matches`. Skips +are recorded and surfaced in the warnings the header already carries. + +**Stream framing.** No adapter requires a terminal marker, so a body that ends +early is a complete answer; and `toolCallAccumulator.result` +(`llm/client.go:526-528`) substitutes `"{}"` for arguments that never arrived, +producing a dispatchable call with no parameters. Adapters track whether the +provider's terminal event was seen and return an error otherwise. Missing +arguments are never fabricated: a tool-use block with no argument delta is an +incomplete call and fails the turn into the existing retry path. + +## Testing + +The five probes already in `internal/agent/harness_hypothesis_probe_test.go` +are the acceptance criteria for Patterns 1, 2 and 3; they are red today and must +be green at the end. Each task adds its own regression tests covering the +specific defect it fixes, written before the fix and confirmed failing first. + +Tests use fakes rather than live providers: fake tools for the harness, an +in-process stdio server for MCP, and recorded SSE bodies for stream framing. No +test requires a credential or network access. + +## Deferred + +Recorded so they are not lost, with why they are not in this wave: + +- **Anthropic thinking blocks replayed as text, signature discarded.** Would + break multi-turn tool use on the `anthropic` provider. Could not be verified — + the configured Anthropic and OpenAI keys both return 401, and the deployment's + active path is an OpenAI-compatible proxy. Needs a working credential first. +- **`max_tokens` sent to models that require `max_completion_tokens`.** Same + verification problem. +- **Non-atomic file writes.** A failed write truncates the target; the fix is + write-temp-then-rename. Real, but independent of the four patterns. +- **MCP add/delete from the dashboard orphans child processes** and leaves the + registry stale, because the handler calls `Connect` rather than `Refresh`. +- **Context window is one global number** for every model, and the measured + token count the provider already returns is discarded. +- **Compaction boundaries** (`compact.go`) drop tool results and can delete an + assistant turn outright. +- **MCP server-name sanitisation collides**, so which server answers a tool call + can change between restarts. +- **Six compaction config knobs** are declared, defaulted, and never read. diff --git a/internal/agent/harness_hypothesis_probe_test.go b/internal/agent/harness_hypothesis_probe_test.go new file mode 100644 index 0000000..b1730d7 --- /dev/null +++ b/internal/agent/harness_hypothesis_probe_test.go @@ -0,0 +1,80 @@ +package agent + +import ( + "strings" + "testing" + "unicode/utf8" + + "github.com/enowdev/antares/internal/llm" +) + +// A user message interleaved between an assistant's tool_calls and their +// results is exactly what the repetition guard produces: it appends its nudge +// to history before executeTools appends the results. The real results must +// still reach the model. +func TestProbeInterleavedNudgeKeepsRealToolResults(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleUser, Content: "read the file"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{ + {ID: "c1", Name: "read_file", Arguments: `{"path":"a.go"}`}, + }}, + {Role: llm.RoleUser, Content: "You have called read_file with the same arguments several times."}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "REAL FILE CONTENT"}, + } + + out := ensureToolResults(history) + + var got string + for _, m := range out { + if m.Role == llm.RoleTool && m.ToolCallID == "c1" { + got = m.Content + } + } + if got != "REAL FILE CONTENT" { + t.Fatalf("tool result for c1 = %q, want %q", got, "REAL FILE CONTENT") + } +} + +// Tool output handed to the model must stay valid UTF-8 no matter where the +// truncation boundary lands. +func TestProbeTrimForModelKeepsValidUTF8(t *testing.T) { + content := strings.Repeat("é", 100) // 200 bytes, 2 bytes per rune + got := trimForModel(content, 51) + if !utf8.ValidString(got) { + t.Fatalf("trimForModel produced invalid UTF-8: %q", got) + } +} + +// Running a destructive command on a remote host is no less destructive than +// running it locally. +func TestProbeDangerScanCoversRemoteExecution(t *testing.T) { + why := dangerIn("vps_run", `{"command":"rm -rf / --no-preserve-root"}`) + if why == "" { + t.Fatalf("vps_run with a root wipe was not classified as dangerous") + } +} + +// Deleting a user's entire home directory is destructive even though the path +// does not stop at the first slash. +func TestProbeDangerScanCatchesHomeDirectoryWipe(t *testing.T) { + why := dangerIn("terminal", `{"command":"rm -rf /home/nvdorman"}`) + if why == "" { + t.Fatalf("rm -rf /home/nvdorman was not classified as dangerous") + } +} + +// Editing the same file several times in one turn is ordinary work, not a +// stuck model repeating an identical call. +func TestProbeDistinctEditsToSameFileAreNotRepeats(t *testing.T) { + r := newRepeatTracker(3) + calls := []llm.ToolCall{ + {Name: "edit_file", Arguments: `{"path":"main.go","old":"import a","new":"import b"}`}, + {Name: "edit_file", Arguments: `{"path":"main.go","old":"func x","new":"func y"}`}, + {Name: "edit_file", Arguments: `{"path":"main.go","old":"return 1","new":"return 2"}`}, + } + for i, c := range calls { + if tripped := r.record([]llm.ToolCall{c}); len(tripped) > 0 { + t.Fatalf("edit %d to the same file flagged as a repeat: %v", i+1, tripped) + } + } +} From 3a406d0478424f554ba0e37ff85c74cb867d540d Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:20:08 +0700 Subject: [PATCH 02/28] Plan the tool-path determinism refactor as nine tested tasks Co-authored-by: Cursor --- .../plans/2026-08-13-tool-path-determinism.md | 725 ++++++++++++++++++ 1 file changed, 725 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-13-tool-path-determinism.md diff --git a/docs/superpowers/plans/2026-08-13-tool-path-determinism.md b/docs/superpowers/plans/2026-08-13-tool-path-determinism.md new file mode 100644 index 0000000..9df80e4 --- /dev/null +++ b/docs/superpowers/plans/2026-08-13-tool-path-determinism.md @@ -0,0 +1,725 @@ +# Tool Path Determinism Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Remove the four assumptions the tool path is built on — position as identity, byte as character, name as capability, failure as success — from the code that carries tool calls and tool results. + +**Architecture:** Each task fixes one assumption at one site, guarded by a test written first. No task changes a provider's wire format beyond the specific defect named. Shared behaviour (rune-safe truncation, capability interfaces) is introduced once and adopted at every call site in the same task, so no site is left on the old path. + +**Tech Stack:** Go 1.x, standard library only. Tests use `go test`, fakes, and recorded fixtures — never a live provider or a credential. + +## Global Constraints + +- Design spec: `docs/superpowers/specs/2026-08-13-tool-path-determinism-design.md`. Its wording governs; where this plan and the spec disagree, stop and ask. +- Repository: worktree `/home/nvdorman/antares/.worktrees/harness-review`, branch `refactor/harness-tool-calls`, based on upstream `enowdev/antares` main at `51d860f`. +- `internal/agent/harness_hypothesis_probe_test.go` holds five probes that are red today. They are acceptance criteria, not scratch work. Do not delete or weaken them; Tasks 2, 3, 1 and 5 turn them green. +- Every task writes its failing test first, runs it, confirms it fails for the intended reason, then implements. +- No new third-party dependency. +- No test may require network access, an API key, or a running daemon. +- Run `gofmt -l .` before each commit; it must print nothing. +- Do not fix defects outside your task. The spec's "Deferred" section is out of scope; if you believe a deferred item blocks your task, stop and report rather than widening scope. +- Existing behaviour not named in your task must keep working: run the full package test suite for every package you touch. + +--- + +### Task 1: Rune-safe truncation everywhere + +**Files:** +- Create: `internal/textutil/truncate.go` +- Create: `internal/textutil/truncate_test.go` +- Modify: `internal/agent/agent.go` (`trimForModel`, ~1169-1179) +- Modify: `internal/agent/compact.go` (`truncate`, ~291-296) +- Modify: `internal/agent/prompt.go` (`readCapped`, ~233-242) +- Modify: `internal/agent/ragcontext.go` (~177-179) +- Modify: `internal/rag/rerank.go` (candidate body slice, ~`body[:1200]`) +- Modify: `internal/plugin/plugin.go` (`truncate`, ~342-347) + +**Interfaces:** +- Produces: `textutil.TruncateRunes(s string, limit int) string` and + `textutil.TruncateMiddle(s string, limit int) (out string, removed int)`. + `limit` counts runes. `TruncateMiddle` keeps a head and a tail and returns how + many runes it removed, for callers that print a notice. + +- [ ] **Step 1: Write the failing test** + +```go +package textutil + +import ( + "strings" + "testing" + "unicode/utf8" +) + +func TestTruncateRunesNeverSplitsARune(t *testing.T) { + for _, limit := range []int{1, 7, 33, 99} { + got := TruncateRunes(strings.Repeat("é", 200), limit) + if !utf8.ValidString(got) { + t.Fatalf("limit %d produced invalid UTF-8: %q", limit, got) + } + if n := utf8.RuneCountInString(got); n > limit { + t.Fatalf("limit %d produced %d runes", limit, n) + } + } +} + +func TestTruncateMiddleKeepsBothEndsAndCountsRunes(t *testing.T) { + in := strings.Repeat("あ", 300) + out, removed := TruncateMiddle(in, 51) + if !utf8.ValidString(out) { + t.Fatalf("invalid UTF-8: %q", out) + } + if removed != 300-51 { + t.Fatalf("removed = %d, want %d", removed, 300-51) + } + if !strings.HasPrefix(out, "あ") || !strings.HasSuffix(out, "あ") { + t.Fatalf("head or tail missing: %q", out) + } +} + +func TestTruncateShorterThanLimitIsUnchanged(t *testing.T) { + if got := TruncateRunes("héllo", 50); got != "héllo" { + t.Fatalf("got %q", got) + } + if out, removed := TruncateMiddle("héllo", 50); out != "héllo" || removed != 0 { + t.Fatalf("got %q, %d", out, removed) + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/textutil/ -v` +Expected: build failure — package does not exist. + +- [ ] **Step 3: Implement the helper** + +Count runes, never slice mid-rune. `TruncateMiddle` splits the budget two-thirds head, one-third tail, matching the existing `trimForModel` proportions. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/textutil/ -v` +Expected: PASS. + +- [ ] **Step 5: Adopt it at every call site** + +Replace each byte slice listed under Files. `trimForModel` uses +`TruncateMiddle` and prints the rune count it returns, so the notice reads +`… %d characters truncated …` with a number that is now actually the count of +characters removed rather than bytes. Every other site uses `TruncateRunes`. +`Tools.MaxOutputChars` keeps its name and its default of 60000; it is now +interpreted as runes, which is what the field already claims to be. + +- [ ] **Step 6: Verify the harness probe turns green** + +Run: `go test ./internal/agent/ -run TestProbeTrimForModelKeepsValidUTF8 -v` +Expected: PASS. + +- [ ] **Step 7: Run every touched package** + +Run: `go test ./internal/textutil/ ./internal/agent/ ./internal/rag/ ./internal/plugin/ -count=1` +Expected: all pass except the four probes owned by Tasks 2, 3 and 5, which stay red. + +- [ ] **Step 8: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Cut text on runes, not bytes" +``` + +--- + +### Task 2: Correlate tool results by call id + +**Files:** +- Modify: `internal/agent/agent.go` (`ensureToolResults`, ~1199-1246) +- Test: `internal/agent/tool_results_test.go` (create) + +**Interfaces:** +- Consumes: nothing from earlier tasks. +- Produces: `ensureToolResults([]llm.Message) []llm.Message` keeps its signature. Its contract becomes: every tool message whose `ToolCallID` matches a call in the transcript is emitted directly after that call's assistant turn, in the order the calls were made; messages of other roles keep their relative order but are emitted after any tool results they were interleaved with; a call with no matching result gets a stub. + +- [ ] **Step 1: Write the failing test** + +```go +package agent + +import ( + "strings" + "testing" + + "github.com/enowdev/antares/internal/llm" +) + +func TestEnsureToolResultsMatchesByCallID(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleUser, Content: "go"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{ + {ID: "c1", Name: "read_file"}, + {ID: "c2", Name: "grep"}, + }}, + {Role: llm.RoleUser, Content: "a nudge that slipped in"}, + {Role: llm.RoleTool, ToolCallID: "c2", Name: "grep", Content: "GREP OUT"}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "FILE OUT"}, + } + + out := ensureToolResults(history) + + // The assistant turn is followed immediately by its results, in call order. + var ai int = -1 + for i, m := range out { + if m.Role == llm.RoleAssistant && len(m.ToolCalls) == 2 { + ai = i + } + } + if ai < 0 { + t.Fatal("assistant turn missing") + } + if out[ai+1].ToolCallID != "c1" || out[ai+1].Content != "FILE OUT" { + t.Fatalf("first result = %+v", out[ai+1]) + } + if out[ai+2].ToolCallID != "c2" || out[ai+2].Content != "GREP OUT" { + t.Fatalf("second result = %+v", out[ai+2]) + } + // The interleaved user message survives, after the results. + found := false + for _, m := range out[ai+3:] { + if m.Role == llm.RoleUser && strings.Contains(m.Content, "nudge") { + found = true + } + } + if !found { + t.Fatal("interleaved user message was dropped") + } +} + +func TestEnsureToolResultsStubsOnlyMissingCallsAndTellsTheTruth(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{ + {ID: "c1", Name: "read_file"}, + {ID: "c2", Name: "grep"}, + }}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "FILE OUT"}, + } + + out := ensureToolResults(history) + + var stub string + for _, m := range out { + if m.ToolCallID == "c2" { + stub = m.Content + } + if m.ToolCallID == "c1" && m.Content != "FILE OUT" { + t.Fatalf("real result was replaced: %q", m.Content) + } + } + if stub == "" { + t.Fatal("missing call c2 was not stubbed") + } + if strings.Contains(stub, "interrupted") { + t.Fatalf("stub asserts an interruption that did not happen: %q", stub) + } +} + +func TestEnsureToolResultsDropsResultsWithNoMatchingCall(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleUser, Content: "hi"}, + {Role: llm.RoleTool, ToolCallID: "orphan", Name: "read_file", Content: "x"}, + } + for _, m := range ensureToolResults(history) { + if m.Role == llm.RoleTool { + t.Fatalf("orphan tool result was kept: %+v", m) + } + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/agent/ -run TestEnsureToolResults -v` +Expected: `TestEnsureToolResultsMatchesByCallID` fails — the results are stubbed and the real content is dropped. + +- [ ] **Step 3: Implement** + +Build a map from `ToolCallID` to the tool message, over the whole slice. Walk the transcript once emitting non-tool messages; on an assistant turn with `ToolCalls`, emit the turn, then for each call emit its mapped result or a stub reading `[no result was recorded for this tool call]`. Skip tool messages during the walk — they are placed by the map. A tool message whose id matches no call is dropped, as today. A duplicate id is consumed once. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/agent/ -run TestEnsureToolResults -v` +Expected: PASS. + +- [ ] **Step 5: Verify the harness probe turns green** + +Run: `go test ./internal/agent/ -run TestProbeInterleavedNudgeKeepsRealToolResults -v` +Expected: PASS. + +- [ ] **Step 6: Run the package** + +Run: `go test ./internal/agent/ -count=1` +Expected: pass except the probes owned by Tasks 3 and 5. + +- [ ] **Step 7: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Match a tool result to its call by id" +``` + +--- + +### Task 3: Fingerprint every tool the same way + +**Files:** +- Modify: `internal/agent/harness.go` (`repeatKey`, ~154-177) +- Modify: `internal/agent/agent.go` (repeat-guard block, ~612-630) +- Test: `internal/agent/repeat_guard_test.go` (create) + +**Interfaces:** +- Consumes: `ensureToolResults` from Task 2 (its ordering contract is what makes the nudge move safe). +- Produces: `repeatKey(llm.ToolCall) string` now returns `name + "\x00" + normaliseArgs(args)` for every tool, with no per-tool special case. + +- [ ] **Step 1: Write the failing test** + +```go +package agent + +import ( + "testing" + + "github.com/enowdev/antares/internal/llm" +) + +func TestRepeatKeyDistinguishesDifferentArguments(t *testing.T) { + a := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"a","new":"b"}`} + b := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"c","new":"d"}`} + if repeatKey(a) == repeatKey(b) { + t.Fatal("two different edits to one file share a fingerprint") + } +} + +func TestRepeatKeyStillCatchesAnIdenticalCall(t *testing.T) { + a := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"a","new":"b"}`} + b := llm.ToolCall{Name: "edit_file", Arguments: `{"new":"b","old":"a","path":"m.go"}`} + if repeatKey(a) != repeatKey(b) { + t.Fatal("the same call re-serialised was not recognised as a repeat") + } +} + +func TestRepeatTrackerTripsOnIdenticalCallsOnly(t *testing.T) { + r := newRepeatTracker(3) + same := llm.ToolCall{Name: "grep", Arguments: `{"pattern":"x"}`} + for i := 1; i <= 2; i++ { + if tripped := r.record([]llm.ToolCall{same}); len(tripped) > 0 { + t.Fatalf("tripped early at %d", i) + } + } + if tripped := r.record([]llm.ToolCall{same}); len(tripped) == 0 { + t.Fatal("three identical calls did not trip the guard") + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/agent/ -run TestRepeatKey -v` +Expected: `TestRepeatKeyDistinguishesDifferentArguments` fails — both keys are `edit_file\x00m.go`. + +- [ ] **Step 3: Implement** + +Delete the `write_file`/`edit_file` and `vps_upload` cases from `repeatKey`, leaving the single normalised-arguments return. In `agent.go`, move the `history = append(history, ...)` nudge so it runs after the results loop rather than before `executeTools`; keep the `EventNotice` where it is so the user still sees it immediately. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/agent/ -run 'TestRepeatKey|TestRepeatTracker' -v` +Expected: PASS. + +- [ ] **Step 5: Verify the harness probe turns green** + +Run: `go test ./internal/agent/ -run TestProbeDistinctEditsToSameFileAreNotRepeats -v` +Expected: PASS. + +- [ ] **Step 6: Run the package** + +Run: `go test ./internal/agent/ -count=1` +Expected: pass except the two danger probes owned by Task 5. + +- [ ] **Step 7: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Tell a stuck loop apart from ordinary progress" +``` + +--- + +### Task 4: Keep a slow SSE follower from panicking + +**Files:** +- Modify: `internal/server/livechat.go` (`follow`, ~60-90) +- Test: `internal/server/livechat_follow_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `follow` never indexes below `lr.base`; a follower whose cursor was trimmed past resumes at the oldest retained event. + +- [ ] **Step 1: Write the failing test** + +Drive a `liveRun` from two goroutines: one follower whose send function blocks on a channel until released, one publisher that emits more than `maxLiveEvents` events while the follower is blocked. Release the follower and assert `follow` returns without panicking. + +```go +func TestFollowSurvivesWindowTrimWhileSending(t *testing.T) { + lr := newLiveRun() // use whatever the package's constructor is + release := make(chan struct{}) + done := make(chan any, 1) + + go func() { + defer func() { done <- recover() }() + first := true + _ = lr.follow(context.Background(), 0, func(agent.Event, int) error { + if first { + first = false + <-release // stall inside send, with the lock dropped + } + return nil + }) + done <- nil + }() + + // Let the follower enter its first send, then overrun the window. + time.Sleep(50 * time.Millisecond) + for i := 0; i < maxLiveEvents+200; i++ { + lr.publish(agent.Event{Type: agent.EventToolProgress, Chunk: "x"}) + } + close(release) + lr.finish() + + if r := <-done; r != nil { + t.Fatalf("follow panicked: %v", r) + } +} +``` + +The names above are the real ones: `newLiveRun() *liveRun` (`livechat.go:31`), +`publish` (`:39`), `finish` (`:53`), `follow(ctx, cursor int, send func(agent.Event, int) error) error` (`:65`), +and `maxLiveEvents = 4000` (`:21`). The assertion — no panic — is what matters. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/server/ -run TestFollowSurvivesWindowTrim -v` +Expected: FAIL with `index out of range [-N]`. + +- [ ] **Step 3: Implement** + +Inside the inner loop, after re-acquiring the lock, clamp the cursor: if `i < lr.base`, set `i = lr.base` before computing the slice offset. Do it on every iteration, not only on entry. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/server/ -run TestFollowSurvivesWindowTrim -v -race` +Expected: PASS with no race reported. + +- [ ] **Step 5: Run the package** + +Run: `go test ./internal/server/ -count=1` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Stop a stalled follower from crashing its own stream" +``` + +--- + +### Task 5: Classify danger by capability + +**Files:** +- Modify: `internal/tools/registry.go` (add the interface next to `Approval`, ~97) +- Modify: `internal/tools/shell.go` (`terminalTool`, ~581) +- Modify: `internal/tools/vps.go` (`vpsRunTool`, ~85) +- Modify: `internal/tools/register.go` (add the accessor next to `NeedsApproval`, ~106) +- Modify: `internal/agent/approval.go` (`dangerIn`, `checkApproval`, the `dangerous` table) +- Test: `internal/agent/danger_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: + - `tools.ShellCommander interface { ShellCommand(args json.RawMessage) (string, bool) }` + - `tools.CommandOf(t Tool, args json.RawMessage) (string, bool)` — returns the command when the tool implements the interface. + - `dangerIn(tool tools.Tool, arguments string) string` — signature changes from `(toolName, arguments string)`. Its only caller is `checkApproval`, which already holds the resolved tool. + +- [ ] **Step 1: Write the failing test** + +```go +func TestDangerScanFollowsCapabilityNotName(t *testing.T) { + cases := []struct { + tool string + args string + want bool + }{ + {"terminal", `{"command":"rm -rf /"}`, true}, + {"terminal", `{"command":"rm -rf /home/someone"}`, true}, + {"terminal", `{"command":"rm -rf ~/projects"}`, true}, + {"terminal", `{"command":"ls -la"}`, false}, + {"vps_run", `{"vps":"prod","command":"rm -rf / --no-preserve-root"}`, true}, + {"vps_run", `{"vps":"prod","command":"mkfs.ext4 /dev/sda1"}`, true}, + {"vps_run", `{"vps":"prod","command":"systemctl status nginx"}`, false}, + } + for _, c := range cases { + got := dangerIn(lookupTool(t, c.tool), c.args) != "" + if got != c.want { + t.Errorf("%s %s -> danger=%v, want %v", c.tool, c.args, got, c.want) + } + } +} + +func TestUnparseableArgumentsFailClosed(t *testing.T) { + if dangerIn(lookupTool(t, "terminal"), "not json at all") == "" { + t.Fatal("arguments that cannot be parsed were treated as safe") + } +} +``` + +Define `lookupTool` in the same test file. It resolves from the process registry +so the test proves the real wiring rather than a stub: + +```go +func lookupTool(t *testing.T, name string) tools.Tool { + t.Helper() + tool, ok := tools.Default().Resolve(name) + if !ok { + t.Fatalf("tool %q is not registered", name) + } + return tool +} +``` + +If the registry accessor is spelled differently in `internal/tools/registry.go`, +use that spelling — the requirement is that the tool comes from the real +registry, not a hand-built fake. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/agent/ -run 'TestDangerScan|TestUnparseable' -v` +Expected: compile failure on the new `dangerIn` signature, then failures on the `vps_run` and home-directory rows. + +- [ ] **Step 3: Implement** + +Add `ShellCommander` and `CommandOf`. `terminalTool.ShellCommand` decodes `{"command":...}`; `vpsRunTool.ShellCommand` decodes its own `{"command":...}`. `dangerIn` asks `CommandOf`; a tool that does not implement it is not scanned, and a tool that does but whose arguments fail to decode returns a fixed reason such as `its arguments could not be read, so it could not be checked`. Correct the recursive-delete regex so it matches a delete of any absolute or home-relative path, not only a bare `/`, `~`, `$HOME` or `*`. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/agent/ -run 'TestDangerScan|TestUnparseable' -v` +Expected: PASS. + +- [ ] **Step 5: Move untrusted-output classification to the same shape** + +Add `tools.UntrustedOutputer interface { UntrustedOutput() bool }`, implement it on `web_fetch`, `web_search`, `browser` and `http_request`, and rewrite `untrustedTool` in `agent.go` to consult the resolved tool, keeping the `tools.MCPPrefix` rule for dynamically registered tools. Add a test asserting each of those four is still wrapped and an ordinary tool is not. + +- [ ] **Step 6: Verify both harness probes turn green** + +Run: `go test ./internal/agent/ -run TestProbeDanger -v` +Expected: PASS for both. + +- [ ] **Step 7: Run every touched package** + +Run: `go test ./internal/agent/ ./internal/tools/ -count=1` +Expected: PASS, and every probe in `harness_hypothesis_probe_test.go` is now green. + +- [ ] **Step 8: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Scan for danger by what a tool can do" +``` + +--- + +### Task 6: Make the policy gate fail closed + +**Files:** +- Modify: `internal/plugin/plugin.go` (`call` ~320-329, `Dispatch` ~245-250) +- Test: `internal/plugin/gate_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: for `PreToolCall`, a plugin's stdout is parsed regardless of exit status, and a plugin that cannot be run or times out yields `Deny` with a reason. All other events keep today's fail-open behaviour. + +- [ ] **Step 1: Write the failing test** + +Three cases, each a small shell script written to `t.TempDir()`: one printing `{"deny":true,"reason":"policy"}` then `exit 1`; one sleeping past the timeout; one printing valid JSON and exiting 0. Assert the first two deny and the third is honoured. Add a fourth asserting a failing plugin on `PostToolCall` still does **not** deny. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/plugin/ -run TestGate -v` +Expected: the first two cases fail — the call is permitted. + +- [ ] **Step 3: Implement** + +In `call`, capture stdout and attempt to parse it before returning the process error. In `Dispatch`, when the event is `PreToolCall` and the plugin errored, use the parsed reply if it decoded; otherwise synthesise `Deny` with a reason naming the plugin and the failure. Leave the other events on the existing `continue`. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/plugin/ -run TestGate -v` +Expected: PASS. + +- [ ] **Step 5: Run the package** + +Run: `go test ./internal/plugin/ -count=1` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Let a denying plugin deny even when it exits badly" +``` + +--- + +### Task 7: Represent MCP content honestly + +**Files:** +- Modify: `internal/mcp/client.go` (`content` struct ~34-40, `Call` ~256-272, `ReadResource` ~340-344) +- Test: `internal/mcp/content_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `Call` returns the embedded resource's text; a result carrying only content types the client cannot represent is an error naming the type; an empty `contents` from `ReadResource` is "not found here" rather than a successful empty string. + +- [ ] **Step 1: Write the failing test** + +Table-driven over decoded `tools/call` results, using the package's existing in-process server fixture: + +```go +{name: "embedded resource text", raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a","mimeType":"text/plain","text":"HELLO"}}]}`, wantText: "HELLO", wantErr: false}, +{name: "audio only", raw: `{"content":[{"type":"audio","data":"...","mimeType":"audio/wav"}]}`, wantErr: true}, +{name: "empty content", raw: `{"content":[]}`, wantErr: false, wantText: "(no content returned)"}, +``` + +Assert the audio case's error message names `audio`. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/mcp/ -run TestCallContent -v` +Expected: the resource case yields `[resource: text/plain]` with no text; the audio case succeeds with `(no content returned)`. + +- [ ] **Step 3: Implement** + +Add a nested `Resource` field to the content struct carrying `uri`, `mimeType`, `text` and `blob`. Emit `text` when present; for a blob, emit a line naming the URI and media type. Track whether any content item was of a type the client could not represent, and when nothing renderable was produced, return an error naming those types. Keep `(no content returned)` only for a genuinely empty `content` array. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/mcp/ -run TestCallContent -v` +Expected: PASS. + +- [ ] **Step 5: Run the package** + +Run: `go test ./internal/mcp/ -count=1` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Read what an MCP server actually returned" +``` + +--- + +### Task 8: Never let a skipped file read as no match + +**Files:** +- Modify: `internal/tools/search.go` (`grepTool.Execute`, size gate ~311-313, warning join ~321-322) +- Test: `internal/tools/grep_skip_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: files skipped by the size gate are counted and reported through the existing `warnings` slice. + +- [ ] **Step 1: Write the failing test** + +Create a temp directory holding one 9 MiB file whose first line contains `NEEDLE_TOKEN`, run `grep` for `NEEDLE_TOKEN`, and assert the output mentions the skip. A second case asserts a normal small-file match still reports no warning. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/tools/ -run TestGrepReportsSkippedFiles -v` +Expected: FAIL — output is `No matches for "NEEDLE_TOKEN" under …`. + +- [ ] **Step 3: Implement** + +Count skipped files, and when the count is above zero append a warning naming the count and the limit. Leave the 8 MiB gate itself in place. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/tools/ -run TestGrepReportsSkippedFiles -v` +Expected: PASS. + +- [ ] **Step 5: Run the package** + +Run: `go test ./internal/tools/ -count=1` +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Say when grep did not look at a file" +``` + +--- + +### Task 9: A stream that stopped early is an error + +**Files:** +- Modify: `internal/llm/client.go` (`toolCallAccumulator.result`, ~526-528) +- Modify: `internal/llm/openai.go` (stream loop, terminal detection ~322-389) +- Modify: `internal/llm/anthropic.go` (stream loop, terminal detection ~324-400) +- Test: `internal/llm/stream_framing_test.go` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `Stream` returns a retryable error when the body ends without the provider's terminal signal; a tool call whose arguments never arrived is never emitted with `{}`. + +- [ ] **Step 1: Write the failing test** + +Serve recorded SSE bodies from `httptest.NewServer`: + +```go +// OpenAI: a tool call whose arguments are cut mid-JSON and no [DONE]. +// Expect: error, and llm.Retryable(err) == true. +// Anthropic: content_block_start for tool_use, then the body ends. +// Expect: error; no tool call with Arguments == "{}" is returned. +// Control: a complete stream with [DONE] still succeeds and yields the call. +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/llm/ -run TestStreamFraming -v` +Expected: both truncated cases return `err == nil` with a fabricated call. + +- [ ] **Step 3: Implement** + +Track a `sawTerminal` flag: `[DONE]` or a chunk carrying a `finish_reason` for OpenAI-compatible routes, `message_stop` for Anthropic. When the reader ends without it, return an error that `llm.Retryable` classifies as retryable, so the agent's existing turn-level retry handles it. In `toolCallAccumulator.result`, drop the `"{}"` substitution: a call with a name but no arguments is incomplete and must not be returned as complete. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/llm/ -run TestStreamFraming -v` +Expected: PASS. + +- [ ] **Step 5: Run the package hermetically** + +Run: `env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY -u GEMINI_API_KEY go test ./internal/llm/ -count=1` +Expected: PASS. The package holds opt-in live tests; unset the keys so they stay skipped. + +- [ ] **Step 6: Commit** + +```bash +gofmt -l . && git add -A && git commit -m "Refuse to call a cut-off stream a finished answer" +``` + +--- + +## Final verification + +After Task 9, run the whole repository hermetically: + +```bash +env -u OPENAI_API_KEY -u ANTHROPIC_API_KEY -u GEMINI_API_KEY go test ./... -count=1 +go vet ./... +gofmt -l . +``` + +All five probes in `internal/agent/harness_hypothesis_probe_test.go` must be green, and no package may regress. From 169269dd0b0387d400d3f881c755668f013b083d Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:26:37 +0700 Subject: [PATCH 03/28] Cut text on runes, not bytes 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 --- internal/agent/agent.go | 10 ++-- internal/agent/compact.go | 17 +++++-- internal/agent/prompt.go | 11 ++-- internal/agent/ragcontext.go | 10 ++-- internal/plugin/plugin.go | 6 ++- internal/rag/rerank.go | 6 +-- internal/textutil/truncate.go | 74 +++++++++++++++++++++++++++ internal/textutil/truncate_test.go | 80 ++++++++++++++++++++++++++++++ 8 files changed, 193 insertions(+), 21 deletions(-) create mode 100644 internal/textutil/truncate.go create mode 100644 internal/textutil/truncate_test.go diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 1f43da8..02b6fdf 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -30,6 +30,7 @@ import ( "github.com/enowdev/antares/internal/roles" "github.com/enowdev/antares/internal/skills" "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/tools" ) @@ -1166,16 +1167,17 @@ func namesOf(m map[string]tools.Tool) []string { return out } +// trimForModel caps text at limit characters, keeping both ends and naming at +// the seam how many characters of the middle are missing. func trimForModel(s string, limit int) string { if limit <= 0 { limit = 60000 } - if len(s) <= limit { + head, tail, removed := textutil.TruncateMiddleParts(s, limit) + if removed == 0 { return s } - head := limit * 2 / 3 - tail := limit - head - return s[:head] + fmt.Sprintf("\n\n… %d characters truncated …\n\n", len(s)-limit) + s[len(s)-tail:] + return head + fmt.Sprintf("\n\n… %d characters truncated …\n\n", removed) + tail } func firstNonEmpty(vals ...string) string { diff --git a/internal/agent/compact.go b/internal/agent/compact.go index 95abaaa..7afb46f 100644 --- a/internal/agent/compact.go +++ b/internal/agent/compact.go @@ -6,10 +6,12 @@ import ( "fmt" "log/slog" "strings" + "unicode/utf8" "github.com/enowdev/antares/internal/llm" "github.com/enowdev/antares/internal/providers" "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/textutil" ) // contextWindowFor returns the active model's token budget for the usage event. @@ -279,18 +281,25 @@ func (a *Agent) prunedToolResults(history []llm.Message) []llm.Message { copy(out, history) for i := 0; i < len(out)-protect; i++ { m := out[i] - if m.Role != llm.RoleTool || len(m.Content) <= minChars { + if m.Role != llm.RoleTool { + continue + } + // Budget and notice both count characters, so the message never claims + // to have dropped text it kept. + chars := utf8.RuneCountInString(m.Content) + if chars <= minChars { continue } out[i].Content = truncate(m.Content, minChars/2) + - fmt.Sprintf("\n\n[tool result pruned: %d characters removed to free context]", len(m.Content)-minChars/2) + fmt.Sprintf("\n\n[tool result pruned: %d characters removed to free context]", chars-minChars/2) } return out } func truncate(s string, n int) string { - if len(s) <= n { + out := textutil.TruncateRunes(s, n) + if out == s { return s } - return s[:n] + "…" + return out + "…" } diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 6a78da2..77211f4 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -13,6 +13,7 @@ import ( "github.com/enowdev/antares/internal/engagement" "github.com/enowdev/antares/internal/llm" "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/tools" "github.com/enowdev/antares/internal/version" ) @@ -229,16 +230,18 @@ func projectBlock(projectDir, antaresWorkspace string) string { return b.String() } -// readCapped returns a file's text truncated to max bytes, or "" if unreadable. +// readCapped returns a file's text truncated to max characters, or "" if +// unreadable. func readCapped(path string, max int) string { data, err := os.ReadFile(path) if err != nil { return "" } - if len(data) > max { - return strings.TrimSpace(string(data[:max])) + "\n… (truncated)" + text := string(data) + if capped := textutil.TruncateRunes(text, max); capped != text { + return strings.TrimSpace(capped) + "\n… (truncated)" } - return strings.TrimSpace(string(data)) + return strings.TrimSpace(text) } // shallowTree lists the immediate entries of dir (dirs marked with a trailing diff --git a/internal/agent/ragcontext.go b/internal/agent/ragcontext.go index d371781..d9b2f06 100644 --- a/internal/agent/ragcontext.go +++ b/internal/agent/ragcontext.go @@ -7,10 +7,12 @@ import ( "path/filepath" "strings" "time" + "unicode/utf8" "github.com/enowdev/antares/internal/llm" "github.com/enowdev/antares/internal/rag" "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/tools" ) @@ -174,8 +176,10 @@ func (a *Agent) autoContext(ctx context.Context, req Request, sess *store.Sessio continue } seen[body] = true - if chars+len(body) > maxChars { - body = body[:max(0, maxChars-chars)] + n := utf8.RuneCountInString(body) + if chars+n > maxChars { + n = max(0, maxChars-chars) + body = textutil.TruncateRunes(body, n) } if strings.TrimSpace(body) == "" { continue @@ -186,7 +190,7 @@ func (a *Agent) autoContext(ctx context.Context, req Request, sess *store.Sessio } fmt.Fprintf(&b, "\n[%s] %s\n", src, body) total++ - chars += len(body) + chars += n if chars >= maxChars { break } diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go index 540ef3a..4673c31 100644 --- a/internal/plugin/plugin.go +++ b/internal/plugin/plugin.go @@ -26,6 +26,7 @@ import ( "sync" "time" + "github.com/enowdev/antares/internal/textutil" "gopkg.in/yaml.v3" ) @@ -340,8 +341,9 @@ func call(ctx context.Context, man Manifest, p Payload) (Reply, error) { } func truncate(s string, max int) string { - if len(s) <= max { + out := textutil.TruncateRunes(s, max) + if out == s { return s } - return s[:max] + "…" + return out + "…" } diff --git a/internal/rag/rerank.go b/internal/rag/rerank.go index 4f62fae..0553cf5 100644 --- a/internal/rag/rerank.go +++ b/internal/rag/rerank.go @@ -13,6 +13,7 @@ import ( "github.com/enowdev/antares/internal/config" "github.com/enowdev/antares/internal/llm" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/tools" "github.com/enowdev/antares/internal/version" ) @@ -147,10 +148,7 @@ func (r *llmReranker) rerank(ctx context.Context, query string, in []tools.RAGRe var b strings.Builder fmt.Fprintf(&b, "Query: %s\n\nPassages:\n", query) for i, c := range in { - body := c.Content - if len(body) > 1200 { - body = body[:1200] - } + body := textutil.TruncateRunes(c.Content, 1200) fmt.Fprintf(&b, "[%d] %s\n\n", i, strings.ReplaceAll(body, "\n", " ")) } diff --git a/internal/textutil/truncate.go b/internal/textutil/truncate.go new file mode 100644 index 0000000..041e540 --- /dev/null +++ b/internal/textutil/truncate.go @@ -0,0 +1,74 @@ +// Package textutil cuts text to a budget measured in runes. +// +// A byte budget applied to non-ASCII text is wrong twice over: it keeps far +// less than it promises, and it can end mid-rune, which JSON encoding then +// rewrites as U+FFFD. Every limit here counts runes and every cut lands on a +// rune boundary, so the output is always valid UTF-8. +package textutil + +import "unicode/utf8" + +// TruncateRunes returns the first limit runes of s. A limit of zero or less +// keeps nothing. +func TruncateRunes(s string, limit int) string { + if limit <= 0 { + return "" + } + // No rune is shorter than a byte, so a string within the byte budget is + // already within the rune budget. + if len(s) <= limit { + return s + } + n := 0 + for i := range s { + if n == limit { + return s[:i] + } + n++ + } + return s +} + +// TruncateMiddleParts keeps a head and a tail of s within limit runes, drops +// everything between them, and reports how many runes it dropped. The budget +// goes two thirds to the head and one third to the tail. Callers that mark the +// seam need the two pieces; TruncateMiddle is the same cut already joined. +func TruncateMiddleParts(s string, limit int) (head, tail string, removed int) { + total := utf8.RuneCountInString(s) + if limit <= 0 { + return "", "", total + } + if total <= limit { + return s, "", 0 + } + headRunes := limit * 2 / 3 + return TruncateRunes(s, headRunes), lastRunes(s, limit-headRunes), total - limit +} + +// TruncateMiddle keeps the head and tail of s within limit runes and drops the +// middle, returning the kept text and how many runes it removed so a caller can +// say how much is missing. +func TruncateMiddle(s string, limit int) (out string, removed int) { + head, tail, removed := TruncateMiddleParts(s, limit) + return head + tail, removed +} + +// lastRunes returns the final limit runes of s. +func lastRunes(s string, limit int) string { + if limit <= 0 { + return "" + } + if len(s) <= limit { + return s + } + n := 0 + for i := len(s); i > 0; { + _, size := utf8.DecodeLastRuneInString(s[:i]) + i -= size + n++ + if n == limit { + return s[i:] + } + } + return s +} diff --git a/internal/textutil/truncate_test.go b/internal/textutil/truncate_test.go new file mode 100644 index 0000000..765c236 --- /dev/null +++ b/internal/textutil/truncate_test.go @@ -0,0 +1,80 @@ +package textutil + +import ( + "strings" + "testing" + "unicode/utf8" +) + +func TestTruncateRunesNeverSplitsARune(t *testing.T) { + for _, limit := range []int{1, 7, 33, 99} { + got := TruncateRunes(strings.Repeat("é", 200), limit) + if !utf8.ValidString(got) { + t.Fatalf("limit %d produced invalid UTF-8: %q", limit, got) + } + if n := utf8.RuneCountInString(got); n > limit { + t.Fatalf("limit %d produced %d runes", limit, n) + } + } +} + +func TestTruncateMiddleKeepsBothEndsAndCountsRunes(t *testing.T) { + in := strings.Repeat("あ", 300) + out, removed := TruncateMiddle(in, 51) + if !utf8.ValidString(out) { + t.Fatalf("invalid UTF-8: %q", out) + } + if removed != 300-51 { + t.Fatalf("removed = %d, want %d", removed, 300-51) + } + if !strings.HasPrefix(out, "あ") || !strings.HasSuffix(out, "あ") { + t.Fatalf("head or tail missing: %q", out) + } +} + +func TestTruncateShorterThanLimitIsUnchanged(t *testing.T) { + if got := TruncateRunes("héllo", 50); got != "héllo" { + t.Fatalf("got %q", got) + } + if out, removed := TruncateMiddle("héllo", 50); out != "héllo" || removed != 0 { + t.Fatalf("got %q, %d", out, removed) + } +} + +func TestTruncateRunesCountsRunesNotBytes(t *testing.T) { + if got := TruncateRunes("héllo wörld", 5); got != "héllo" { + t.Fatalf("got %q, want %q", got, "héllo") + } + if got := TruncateRunes("abc", 0); got != "" { + t.Fatalf("limit 0 got %q, want empty", got) + } + if got := TruncateRunes("abc", -3); got != "" { + t.Fatalf("negative limit got %q, want empty", got) + } +} + +func TestTruncateMiddleSpendsTwoThirdsOnTheHead(t *testing.T) { + in := "0123456789" + strings.Repeat("x", 80) + "abcdefghij" + head, tail, removed := TruncateMiddleParts(in, 9) + if head != "012345" { + t.Fatalf("head = %q, want %q", head, "012345") + } + if tail != "hij" { + t.Fatalf("tail = %q, want %q", tail, "hij") + } + if removed != 100-9 { + t.Fatalf("removed = %d, want %d", removed, 100-9) + } + + out, joined := TruncateMiddle(in, 9) + if out != head+tail || joined != removed { + t.Fatalf("TruncateMiddle = %q, %d; want %q, %d", out, joined, head+tail, removed) + } +} + +func TestTruncateMiddleWithoutABudgetKeepsNothing(t *testing.T) { + out, removed := TruncateMiddle("héllo", 0) + if out != "" || removed != 5 { + t.Fatalf("got %q, %d; want %q, %d", out, removed, "", 5) + } +} From 3fe1cb948baefe46f79b06358594c6cb65d4c9b2 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:30:53 +0700 Subject: [PATCH 04/28] Cut tool output on runes too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/tools/browser.go | 9 ++- internal/tools/shell.go | 10 ++- internal/tools/truncation_utf8_test.go | 105 +++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 internal/tools/truncation_utf8_test.go diff --git a/internal/tools/browser.go b/internal/tools/browser.go index c35ea06..5942611 100644 --- a/internal/tools/browser.go +++ b/internal/tools/browser.go @@ -8,9 +8,11 @@ import ( "strings" "sync" "time" + "unicode/utf8" "github.com/enowdev/antares/internal/browser" "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/textutil" ) // browserSessions keeps one browser per conversation. A page that survives @@ -381,11 +383,14 @@ func saveScreenshot(workspace string, png []byte) (string, error) { return path, nil } +// truncateTool caps tool output at max characters and says how long the whole +// text was, so the model knows what it is missing. func truncateTool(s string, max int) string { - if len(s) <= max { + out := textutil.TruncateRunes(s, max) + if out == s { return s } - return s[:max] + fmt.Sprintf("\n\n… truncated, %d characters total", len(s)) + return out + fmt.Sprintf("\n\n… truncated, %d characters total", utf8.RuneCountInString(s)) } // challengeDetectJS reports the kind of bot-challenge on the current page, or "" diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 8df2f54..42353e6 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -17,6 +17,7 @@ import ( "github.com/enowdev/antares/internal/config" "github.com/enowdev/antares/internal/sandbox" + "github.com/enowdev/antares/internal/textutil" ) // ShellManager owns one long-lived shell per session so `cd`, exported @@ -662,16 +663,17 @@ func (terminalTool) Execute(ctx context.Context, in Input) Result { return Result{Content: out, Meta: map[string]any{"exit_code": 0}} } +// trimOutput caps command output at limit characters, keeping both ends and +// naming at the seam how many characters of the middle are missing. func trimOutput(s string, limit int) string { if limit <= 0 { limit = 60000 } - if len(s) <= limit { + head, tail, removed := textutil.TruncateMiddleParts(s, limit) + if removed == 0 { return s } - head := limit * 2 / 3 - tail := limit - head - return s[:head] + fmt.Sprintf("\n\n… %d characters omitted …\n\n", len(s)-limit) + s[len(s)-tail:] + return head + fmt.Sprintf("\n\n… %d characters omitted …\n\n", removed) + tail } func max(a, b int) int { diff --git a/internal/tools/truncation_utf8_test.go b/internal/tools/truncation_utf8_test.go new file mode 100644 index 0000000..0e07cd4 --- /dev/null +++ b/internal/tools/truncation_utf8_test.go @@ -0,0 +1,105 @@ +package tools + +import ( + "context" + "encoding/json" + "runtime" + "strings" + "testing" + "unicode/utf8" + + "github.com/enowdev/antares/internal/config" +) + +// Tool output is handed straight to the provider, so a cut that lands inside a +// rune ships invalid UTF-8 that JSON encoding rewrites as U+FFFD. Terminal and +// browser output is where multi-byte text actually turns up. +func TestTrimOutputKeepsValidUTF8AndCountsRunes(t *testing.T) { + got := trimOutput(strings.Repeat("é", 100), 51) + if !utf8.ValidString(got) { + t.Fatalf("trimOutput produced invalid UTF-8: %q", got) + } + want := strings.Repeat("é", 34) + "\n\n… 49 characters omitted …\n\n" + strings.Repeat("é", 17) + if got != want { + t.Fatalf("trimOutput = %q, want %q", got, want) + } +} + +// The notice must state characters removed, not bytes removed: 100 runes cut to +// 51 loses 49 characters, whatever each one weighs. +func TestTrimOutputReportsCharactersNotBytes(t *testing.T) { + got := trimOutput(strings.Repeat("字", 100), 51) + if !strings.Contains(got, "… 49 characters omitted …") { + t.Fatalf("trimOutput notice does not report 49 characters removed: %q", got) + } +} + +// MaxOutputChars is a character budget, so text inside it survives whole no +// matter how many bytes each character takes. +func TestTrimOutputDefaultBudgetCountsCharacters(t *testing.T) { + in := strings.Repeat("字", 30000) // 90000 bytes, inside the 60000-character default + if got := trimOutput(in, 0); got != in { + t.Fatalf("trimOutput cut a %d-character string under the 60000-character default (%d bytes kept)", + utf8.RuneCountInString(in), len(got)) + } +} + +// truncateTool caps browser page text, diagnostics, image-endpoint errors and +// the schedule listing. Its notice reports a total, so that total must count +// characters too. +func TestTruncateToolKeepsValidUTF8AndCountsRunes(t *testing.T) { + got := truncateTool(strings.Repeat("字", 200), 100) + if !utf8.ValidString(got) { + t.Fatalf("truncateTool produced invalid UTF-8: %q", got) + } + if !strings.HasPrefix(got, strings.Repeat("字", 100)) { + t.Fatalf("truncateTool kept %d characters, want 100: %q", utf8.RuneCountInString(got), got) + } + if !strings.Contains(got, "… truncated, 200 characters total") { + t.Fatalf("truncateTool notice does not report 200 characters total: %q", got) + } +} + +// The hook tools cap their body with truncateTool at Tools.MaxOutputChars +// (hooks.go), defaulting to 60000. The hook path itself shells out to an +// embedded Python program, so the budget is covered here rather than by running +// one: what matters is that the cap holds characters, not bytes. +func TestHookOutputBudgetCountsCharacters(t *testing.T) { + in := strings.Repeat("字", 30000) // 90000 bytes, inside the 60000-character default + if got := truncateTool(in, 60000); got != in { + t.Fatalf("truncateTool cut a %d-character body under a 60000-character budget (%d bytes kept)", + utf8.RuneCountInString(in), len(got)) + } +} + +// End to end through the terminal tool, the call site that actually feeds the +// model: a command whose output exceeds MaxOutputChars must still come back as +// valid UTF-8. +func TestTerminalToolOutputStaysValidUTF8(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX persistent shell protocol does not apply on Windows") + } + + cfg := &config.Config{ + Terminal: config.Terminal{Sandbox: "none"}, + Tools: config.Tools{MaxOutputChars: 51}, + } + m := NewShellManager(cfg.Terminal) + t.Cleanup(m.CloseAll) + + args, _ := json.Marshal(map[string]any{"command": "printf '%s' '" + strings.Repeat("é", 100) + "'"}) + res := (terminalTool{}).Execute(context.Background(), Input{ + Args: args, SessionID: "session-utf8", Workspace: t.TempDir(), + Deps: &Deps{Shell: m, Config: cfg}, Emit: func(Progress) {}, + }) + if res.IsError { + t.Fatalf("terminal tool failed: %s", res.Content) + } + if !utf8.ValidString(res.Content) { + t.Fatalf("terminal output is not valid UTF-8: %q", res.Content) + } + want := strings.Repeat("é", 34) + "\n\n… 49 characters omitted …\n\n" + strings.Repeat("é", 17) + if res.Content != want { + t.Fatalf("terminal output = %q, want %q", res.Content, want) + } +} From c9da0618930680f6843f85d3bbd0deb65f631d28 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:39:31 +0700 Subject: [PATCH 05/28] Pin the two character budgets that were hand-written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/agent/context_budget_test.go | 145 ++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 internal/agent/context_budget_test.go diff --git a/internal/agent/context_budget_test.go b/internal/agent/context_budget_test.go new file mode 100644 index 0000000..f5938e2 --- /dev/null +++ b/internal/agent/context_budget_test.go @@ -0,0 +1,145 @@ +package agent + +import ( + "context" + "fmt" + "strings" + "testing" + "unicode/utf8" + + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/llm" + "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/tools" +) + +// pruneConfig sets the two knobs prunedToolResults reads. ProactivePruneMinChars +// is a character budget, like Tools.MaxOutputChars. +func pruneConfig(minChars int) *config.Config { + cfg := config.Default() + cfg.Compression.ProactivePruneMinChars = minChars + cfg.Compression.ProtectLastN = 4 + return cfg +} + +// historyWithToolResult puts one tool result ahead of a protected tail long +// enough for prunedToolResults to reach it. +func historyWithToolResult(content string) []llm.Message { + h := []llm.Message{{Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: content}} + for i := 0; i < 8; i++ { + h = append(h, llm.Message{Role: llm.RoleUser, Content: "turn"}) + } + return h +} + +// The pruned result and the notice that explains it must agree: the notice +// counts what was actually dropped, and what is kept is still valid UTF-8. +func TestPrunedToolResultsCountsCharactersNotBytes(t *testing.T) { + const minChars = 100 + a := agentWithConfig(pruneConfig(minChars)) + content := strings.Repeat("字", 200) // 600 bytes + + got := a.prunedToolResults(historyWithToolResult(content))[0].Content + + if !utf8.ValidString(got) { + t.Fatalf("pruned tool result is not valid UTF-8: %q", got) + } + kept, notice, found := strings.Cut(got, "\n\n[tool result pruned:") + if !found { + t.Fatalf("pruned tool result carries no notice: %q", got) + } + keptRunes := utf8.RuneCountInString(strings.TrimSuffix(kept, "…")) + if keptRunes != minChars/2 { + t.Fatalf("kept %d characters, want %d: %q", keptRunes, minChars/2, kept) + } + removed := utf8.RuneCountInString(content) - keptRunes + want := fmt.Sprintf(" %d characters removed to free context]", removed) + if notice != want { + t.Fatalf("notice = %q, want %q — the number must be the characters actually removed", notice, want) + } +} + +// A result inside the minimum is left exactly as it is, however many bytes its +// characters happen to weigh. +func TestPrunedToolResultsLeavesResultsInsideTheMinimum(t *testing.T) { + const minChars = 100 + a := agentWithConfig(pruneConfig(minChars)) + content := strings.Repeat("字", minChars) // 300 bytes, exactly the minimum + + if got := a.prunedToolResults(historyWithToolResult(content))[0].Content; got != content { + t.Fatalf("a %d-character result inside the %d-character minimum was pruned to %q", + utf8.RuneCountInString(content), minChars, got) + } +} + +// stubRAG answers with fixed hits per collection, so a test controls exactly +// what autoContext folds into its budget. +type stubRAG struct{ hits map[string][]tools.RAGResult } + +func (stubRAG) Name() string { return "stub" } + +func (s stubRAG) Search(_ context.Context, collection, _ string, _ int) ([]tools.RAGResult, error) { + return s.hits[collection], nil +} + +func (stubRAG) Index(context.Context, string, []tools.RAGDoc) (int, error) { return 0, nil } +func (stubRAG) Collections(context.Context) ([]string, error) { return nil, nil } +func (stubRAG) Delete(context.Context, string) error { return nil } + +// autoContextWith runs autoContext over the given bodies, all returned from the +// default knowledge collection. +func autoContextWith(bodies ...string) string { + cfg := config.Default() + cfg.RAG.AutoContext = true + a := agentWithConfig(cfg) + + hits := make([]tools.RAGResult, 0, len(bodies)) + for _, body := range bodies { + hits = append(hits, tools.RAGResult{Content: body}) + } + a.rag = stubRAG{hits: map[string][]tools.RAGResult{"antares": hits}} + + return a.autoContext(context.Background(), Request{Message: "what did we decide?"}, &store.Session{ID: "s1"}) +} + +// The retrieval budget counts characters, so multi-byte bodies that fit inside +// it arrive whole rather than being cut to a third of their length. +func TestAutoContextBudgetCountsCharactersNotBytes(t *testing.T) { + // 3000 characters in total, inside the 4000-character budget, but 9000 + // bytes — over it three times if the accumulator measures bytes. + bodies := []string{ + strings.Repeat("あ", 1000), + strings.Repeat("い", 1000), + strings.Repeat("う", 1000), + } + block := autoContextWith(bodies...) + + if !utf8.ValidString(block) { + t.Fatalf("auto-context block is not valid UTF-8: %q", block) + } + for i, body := range bodies { + if !strings.Contains(block, body) { + t.Fatalf("body %d (1000 characters, 3000 bytes) did not survive the 4000-character budget", i+1) + } + } +} + +// A body that overruns the budget is cut on a character boundary, and only the +// characters that fit are charged to the budget. +func TestAutoContextTruncatesARetrievedBodyOnACharacterBoundary(t *testing.T) { + block := autoContextWith(strings.Repeat("あ", 3000), strings.Repeat("い", 3000)) + + if !utf8.ValidString(block) { + t.Fatalf("auto-context block is not valid UTF-8: %q", block) + } + first, second := strings.Count(block, "あ"), strings.Count(block, "い") + if first != 3000 { + t.Fatalf("first body contributed %d characters, want 3000", first) + } + if second != 1000 { + t.Fatalf("second body contributed %d characters, want the 1000 left in the budget", second) + } + if first+second != 4000 { + t.Fatalf("retrieved %d characters, want the 4000-character budget", first+second) + } +} From 907dff06757584748545271ebdfa1269b23ad317 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:46:26 +0700 Subject: [PATCH 06/28] Match a tool result to its call by id Co-authored-by: Cursor --- internal/agent/agent.go | 78 +++++++------- internal/agent/toolresults_test.go | 164 ++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 43 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 02b6fdf..6b75133 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -1191,58 +1191,54 @@ func firstNonEmpty(vals ...string) string { // ensureToolResults guarantees the invariant every OpenAI-compatible provider // enforces: an assistant message carrying tool_calls must be immediately -// followed by a tool message for each tool_call_id. A turn interrupted after -// the assistant's tool_calls were persisted but before (all) their results -// were — or a history reshaped by compaction — can otherwise leave a dangling -// tool_call, which the provider rejects with "insufficient tool messages -// following tool_calls message". For any tool_call with no matching result, a -// synthetic stub result is spliced in so the request is always well-formed. -// This is a send-time repair and does not mutate what is persisted. +// followed by one tool message per tool_call_id, and a tool message must answer +// a call. A result is correlated with its call by id, never by position, +// because the two are not always adjacent: the repetition guard appends its +// nudge to history before the results land, and compaction reshapes the tail. +// Each assistant turn is therefore re-joined with its results in call order, +// and whatever was interleaved keeps its relative order but follows them. A +// call with no result anywhere in the transcript — an interrupted run, or a +// history reshaped by compaction — gets a synthetic stub, without which the +// provider rejects the request with "insufficient tool messages following +// tool_calls message"; a result answering no call is dropped for the same +// reason. This is a send-time repair and does not mutate what is persisted. func ensureToolResults(msgs []llm.Message) []llm.Message { + // Results are queued per id in transcript order rather than kept one per + // id: an id is only unique within a turn, and Gemini synthesises + // "call__" when it omits one, so the same id recurs across + // turns. Each call takes the earliest result of its id left unclaimed. + pending := make(map[string][]int) + for i, m := range msgs { + if m.Role == llm.RoleTool { + pending[m.ToolCallID] = append(pending[m.ToolCallID], i) + } + } + out := make([]llm.Message, 0, len(msgs)) - for i := 0; i < len(msgs); i++ { - m := msgs[i] - // A tool message is only valid immediately after an assistant message - // carrying tool_calls; those are consumed in the inner loop below. Any - // tool message that reaches here is an orphan — its assistant tool_calls - // was dropped (e.g. by compaction), and providers reject a tool message - // that does not answer a preceding tool_calls. Drop it. + for _, m := range msgs { + // Tool messages are emitted below, beside the call they answer. One + // reaching here answers no call in this transcript — its assistant + // turn was dropped (e.g. by compaction) — so drop it. if m.Role == llm.RoleTool { continue } out = append(out, m) - if m.Role != llm.RoleAssistant || len(m.ToolCalls) == 0 { + if m.Role != llm.RoleAssistant { continue } - ids := make(map[string]bool, len(m.ToolCalls)) for _, tc := range m.ToolCalls { - ids[tc.ID] = true - } - // Emit the tool results that follow and match one of this turn's call - // ids, recording which ids are covered; unknown or duplicate tool - // results are dropped. - covered := make(map[string]bool, len(m.ToolCalls)) - j := i + 1 - for j < len(msgs) && msgs[j].Role == llm.RoleTool { - t := msgs[j] - if ids[t.ToolCallID] && !covered[t.ToolCallID] { - covered[t.ToolCallID] = true - out = append(out, t) - } - j++ - } - // Stub any call that never produced a matching result. - for _, tc := range m.ToolCalls { - if !covered[tc.ID] { - out = append(out, llm.Message{ - Role: llm.RoleTool, - ToolCallID: tc.ID, - Name: tc.Name, - Content: "[no result recorded — the previous run was interrupted before this tool finished]", - }) + if q := pending[tc.ID]; len(q) > 0 { + pending[tc.ID] = q[1:] + out = append(out, msgs[q[0]]) + continue } + out = append(out, llm.Message{ + Role: llm.RoleTool, + ToolCallID: tc.ID, + Name: tc.Name, + Content: "[no result was recorded for this tool call]", + }) } - i = j - 1 // skip the tool messages we just processed } return out } diff --git a/internal/agent/toolresults_test.go b/internal/agent/toolresults_test.go index 2daabb6..49a845d 100644 --- a/internal/agent/toolresults_test.go +++ b/internal/agent/toolresults_test.go @@ -1,6 +1,8 @@ package agent import ( + "reflect" + "strings" "testing" "github.com/enowdev/antares/internal/llm" @@ -16,11 +18,11 @@ func toolIDs(msgs []llm.Message) []string { return ids } -// countStubs reports how many tool messages carry the interrupted-stub marker. +// countStubs reports how many tool messages carry the missing-result marker. func countStubs(msgs []llm.Message) int { n := 0 for _, m := range msgs { - if m.Role == llm.RoleTool && m.Content == "[no result recorded — the previous run was interrupted before this tool finished]" { + if m.Role == llm.RoleTool && m.Content == "[no result was recorded for this tool call]" { n++ } } @@ -76,3 +78,161 @@ func TestEnsureToolResults_NoToolCalls(t *testing.T) { t.Fatalf("plain conversation altered: %+v", out) } } + +func TestEnsureToolResultsMatchesByCallID(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleUser, Content: "go"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{ + {ID: "c1", Name: "read_file"}, + {ID: "c2", Name: "grep"}, + }}, + {Role: llm.RoleUser, Content: "a nudge that slipped in"}, + {Role: llm.RoleTool, ToolCallID: "c2", Name: "grep", Content: "GREP OUT"}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "FILE OUT"}, + } + + out := ensureToolResults(history) + + // The assistant turn is followed immediately by its results, in call order. + var ai int = -1 + for i, m := range out { + if m.Role == llm.RoleAssistant && len(m.ToolCalls) == 2 { + ai = i + } + } + if ai < 0 { + t.Fatal("assistant turn missing") + } + if out[ai+1].ToolCallID != "c1" || out[ai+1].Content != "FILE OUT" { + t.Fatalf("first result = %+v", out[ai+1]) + } + if out[ai+2].ToolCallID != "c2" || out[ai+2].Content != "GREP OUT" { + t.Fatalf("second result = %+v", out[ai+2]) + } + // The interleaved user message survives, after the results. + found := false + for _, m := range out[ai+3:] { + if m.Role == llm.RoleUser && strings.Contains(m.Content, "nudge") { + found = true + } + } + if !found { + t.Fatal("interleaved user message was dropped") + } +} + +func TestEnsureToolResultsStubsOnlyMissingCallsAndTellsTheTruth(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{ + {ID: "c1", Name: "read_file"}, + {ID: "c2", Name: "grep"}, + }}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "FILE OUT"}, + } + + out := ensureToolResults(history) + + var stub string + for _, m := range out { + if m.ToolCallID == "c2" { + stub = m.Content + } + if m.ToolCallID == "c1" && m.Content != "FILE OUT" { + t.Fatalf("real result was replaced: %q", m.Content) + } + } + if stub == "" { + t.Fatal("missing call c2 was not stubbed") + } + if strings.Contains(stub, "interrupted") { + t.Fatalf("stub asserts an interruption that did not happen: %q", stub) + } +} + +func TestEnsureToolResultsDropsResultsWithNoMatchingCall(t *testing.T) { + history := []llm.Message{ + {Role: llm.RoleUser, Content: "hi"}, + {Role: llm.RoleTool, ToolCallID: "orphan", Name: "read_file", Content: "x"}, + } + for _, m := range ensureToolResults(history) { + if m.Role == llm.RoleTool { + t.Fatalf("orphan tool result was kept: %+v", m) + } + } +} + +// Gemini synthesises a call id from the call's position and name when it omits +// one, so two turns that call the same tool first recur under the same id. +// Each turn must still be answered with its own result. +func TestEnsureToolResults_RepeatedIDAcrossTurnsKeepsBothResults(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "FIRST"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "SECOND"}, + } + out := ensureToolResults(in) + var got []string + for _, m := range out { + if m.Role == llm.RoleTool { + got = append(got, m.Content) + } + } + if len(got) != 2 || got[0] != "FIRST" || got[1] != "SECOND" { + t.Fatalf("tool results = %q, want [FIRST SECOND]", got) + } + if countStubs(out) != 0 { + t.Fatalf("a real result was stubbed: %+v", out) + } +} + +func TestEnsureToolResults_SecondResultForOneCallIsDropped(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "a", Name: "grep"}}}, + {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "first"}, + {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "second"}, + } + out := ensureToolResults(in) + if ids := toolIDs(out); len(ids) != 1 { + t.Fatalf("want one tool message, got %d (%v)", len(ids), ids) + } + for _, m := range out { + if m.Role == llm.RoleTool && m.Content != "first" { + t.Fatalf("later duplicate won: %q", m.Content) + } + } +} + +func TestEnsureToolResults_InterleavedMessagesKeepTheirOrder(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "a", Name: "grep"}}}, + {Role: llm.RoleUser, Content: "first nudge"}, + {Role: llm.RoleUser, Content: "second nudge"}, + {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "ok"}, + } + out := ensureToolResults(in) + var order []string + for _, m := range out { + order = append(order, m.Role+":"+m.Content) + } + want := []string{"assistant:", "tool:ok", "user:first nudge", "user:second nudge"} + if !reflect.DeepEqual(order, want) { + t.Fatalf("order = %q, want %q", order, want) + } +} + +// The repair applies to what is sent, so the history it reads — which is what +// gets persisted — must come back untouched. +func TestEnsureToolResults_DoesNotMutateItsInput(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleUser, Content: "go"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "a", Name: "grep"}, {ID: "b", Name: "read_file"}}}, + {Role: llm.RoleUser, Content: "nudge"}, + {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "ok"}, + } + before := append([]llm.Message(nil), in...) + ensureToolResults(in) + if !reflect.DeepEqual(in, before) { + t.Fatalf("input history was rewritten:\n got %+v\nwant %+v", in, before) + } +} From e715bbdb428eaf4a31568473ae171f23389310ad Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 18:56:16 +0700 Subject: [PATCH 07/28] Bind a tool result inside its own turn before looking further Co-authored-by: Cursor --- internal/agent/agent.go | 78 ++++++++++++++++++++++---- internal/agent/toolresults_test.go | 90 +++++++++++++++++++++++++++--- 2 files changed, 148 insertions(+), 20 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 6b75133..5e723e7 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -1192,8 +1192,9 @@ func firstNonEmpty(vals ...string) string { // ensureToolResults guarantees the invariant every OpenAI-compatible provider // enforces: an assistant message carrying tool_calls must be immediately // followed by one tool message per tool_call_id, and a tool message must answer -// a call. A result is correlated with its call by id, never by position, -// because the two are not always adjacent: the repetition guard appends its +// a call. A result is bound to its call by id, and among results sharing an id +// by which turn they fall inside — never by bare adjacency, because a result +// and its call are not always neighbours: the repetition guard appends its // nudge to history before the results land, and compaction reshapes the tail. // Each assistant turn is therefore re-joined with its results in call order, // and whatever was interleaved keeps its relative order but follows them. A @@ -1203,18 +1204,72 @@ func firstNonEmpty(vals ...string) string { // tool_calls message"; a result answering no call is dropped for the same // reason. This is a send-time repair and does not mutate what is persisted. func ensureToolResults(msgs []llm.Message) []llm.Message { - // Results are queued per id in transcript order rather than kept one per - // id: an id is only unique within a turn, and Gemini synthesises + // A call id is only unique within a turn — Gemini synthesises // "call__" when it omits one, so the same id recurs across - // turns. Each call takes the earliest result of its id left unclaimed. - pending := make(map[string][]int) + // turns — which makes an id alone too weak to bind a result to a call. + // Every result is therefore indexed by id in transcript order, and each + // call takes one in two passes: the whole transcript is bound to the + // results inside each turn's own span before any call is allowed to look + // outside it. Reversing that order lets a turn whose result was compacted + // away reach forward and take the result of a later turn sharing its id, + // leaving the live call with a stub and the model with stale output. + byID := make(map[string][]int) for i, m := range msgs { if m.Role == llm.RoleTool { - pending[m.ToolCallID] = append(pending[m.ToolCallID], i) + byID[m.ToolCallID] = append(byID[m.ToolCallID], i) } } + // A turn's span runs to the next assistant message: anything else between + // a turn and its results — the guard's nudge — was interleaved, but a new + // assistant message means the turn ended. + type turn struct { + at int + end int + bound []int // parallel to the turn's ToolCalls; -1 until a result binds + } + var turns []turn + for i, m := range msgs { + if m.Role != llm.RoleAssistant { + continue + } + if n := len(turns); n > 0 { + turns[n-1].end = i + } + t := turn{at: i, end: len(msgs), bound: make([]int, len(m.ToolCalls))} + for j := range t.bound { + t.bound[j] = -1 + } + turns = append(turns, t) + } + + claimed := make([]bool, len(msgs)) + bind := func(t *turn, inSpan bool) { + for j := range t.bound { + if t.bound[j] >= 0 { + continue + } + for _, ri := range byID[msgs[t.at].ToolCalls[j].ID] { + if claimed[ri] || (inSpan && (ri < t.at || ri >= t.end)) { + continue + } + claimed[ri] = true + t.bound[j] = ri + break + } + } + } + for i := range turns { + bind(&turns[i], true) + } + // Only now may a call reach outside its span, for the result that a later + // assistant message was appended in front of. + for i := range turns { + bind(&turns[i], false) + } + out := make([]llm.Message, 0, len(msgs)) + next := 0 for _, m := range msgs { // Tool messages are emitted below, beside the call they answer. One // reaching here answers no call in this transcript — its assistant @@ -1226,10 +1281,11 @@ func ensureToolResults(msgs []llm.Message) []llm.Message { if m.Role != llm.RoleAssistant { continue } - for _, tc := range m.ToolCalls { - if q := pending[tc.ID]; len(q) > 0 { - pending[tc.ID] = q[1:] - out = append(out, msgs[q[0]]) + t := turns[next] + next++ + for j, tc := range m.ToolCalls { + if t.bound[j] >= 0 { + out = append(out, msgs[t.bound[j]]) continue } out = append(out, llm.Message{ diff --git a/internal/agent/toolresults_test.go b/internal/agent/toolresults_test.go index 49a845d..9490858 100644 --- a/internal/agent/toolresults_test.go +++ b/internal/agent/toolresults_test.go @@ -1,6 +1,7 @@ package agent import ( + "encoding/json" "reflect" "strings" "testing" @@ -18,17 +19,31 @@ func toolIDs(msgs []llm.Message) []string { return ids } +// noResultStub is the content ensureToolResults gives a call the transcript +// holds no result for. Spelled out here rather than shared with the production +// constant so a reworded stub has to be reviewed, not just propagated. +const noResultStub = "[no result was recorded for this tool call]" + // countStubs reports how many tool messages carry the missing-result marker. func countStubs(msgs []llm.Message) int { n := 0 for _, m := range msgs { - if m.Role == llm.RoleTool && m.Content == "[no result was recorded for this tool call]" { + if m.Role == llm.RoleTool && m.Content == noResultStub { n++ } } return n } +// roleAndContent renders the emitted sequence for tests that pin whole shapes. +func roleAndContent(msgs []llm.Message) []string { + out := make([]string, 0, len(msgs)) + for _, m := range msgs { + out = append(out, m.Role+":"+m.Content) + } + return out +} + func TestEnsureToolResults_StubsDanglingCall(t *testing.T) { in := []llm.Message{ {Role: llm.RoleUser, Content: "do it"}, @@ -144,8 +159,11 @@ func TestEnsureToolResultsStubsOnlyMissingCallsAndTellsTheTruth(t *testing.T) { if stub == "" { t.Fatal("missing call c2 was not stubbed") } - if strings.Contains(stub, "interrupted") { - t.Fatalf("stub asserts an interruption that did not happen: %q", stub) + // Pinned exactly: the stub may state that no result was recorded and + // nothing else. Any other wording — an interruption, a failure, a refusal — + // tells the model something the transcript does not support. + if stub != noResultStub { + t.Fatalf("stub = %q, want %q", stub, noResultStub) } } @@ -210,17 +228,56 @@ func TestEnsureToolResults_InterleavedMessagesKeepTheirOrder(t *testing.T) { {Role: llm.RoleUser, Content: "second nudge"}, {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "ok"}, } - out := ensureToolResults(in) - var order []string - for _, m := range out { - order = append(order, m.Role+":"+m.Content) - } + order := roleAndContent(ensureToolResults(in)) want := []string{"assistant:", "tool:ok", "user:first nudge", "user:second nudge"} if !reflect.DeepEqual(order, want) { t.Fatalf("order = %q, want %q", order, want) } } +// Compaction splices the first protectFirstN messages in verbatim +// (compact.go:73,104) and rebalanceToolBoundary only repairs the middle/tail +// boundary, so the head can end on a tool-call turn whose result was +// summarised away. That dangling call must not reach forward and take the +// result belonging to the live call of the same id. +func TestEnsureToolResults_DanglingHeadCallDoesNotStealALaterResult(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleUser, Content: "[Compacted summary of the earlier conversation]"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "FRESH CONTENT FOR THE LATEST CALL"}, + } + got := roleAndContent(ensureToolResults(in)) + want := []string{ + "assistant:", + "tool:" + noResultStub, + "user:[Compacted summary of the earlier conversation]", + "assistant:", + "tool:FRESH CONTENT FOR THE LATEST CALL", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("the live call was not answered with its own result:\n got %q\nwant %q", got, want) + } +} + +// persistContextCompact cuts the persisted history at throughSeq with no +// tool-boundary rebalance (compact.go:148-149) and loadHistory rebuilds the +// tail from every later row (session.go:180-191), so a history can open with a +// tool result whose call is gone. Adopting it would answer a fresh call with +// stale output and nothing would mark it as stale. +func TestEnsureToolResults_OrphanResultIsNotAdoptedByALaterCall(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "STALE ORPHAN FROM AN EARLIER TURN"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "FRESH RESULT FOR THIS CALL"}, + } + got := roleAndContent(ensureToolResults(in)) + want := []string{"assistant:", "tool:FRESH RESULT FOR THIS CALL"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("stale orphan was adopted:\n got %q\nwant %q", got, want) + } +} + // The repair applies to what is sent, so the history it reads — which is what // gets persisted — must come back untouched. func TestEnsureToolResults_DoesNotMutateItsInput(t *testing.T) { @@ -230,8 +287,23 @@ func TestEnsureToolResults_DoesNotMutateItsInput(t *testing.T) { {Role: llm.RoleUser, Content: "nudge"}, {Role: llm.RoleTool, ToolCallID: "a", Name: "grep", Content: "ok"}, } - before := append([]llm.Message(nil), in...) + // A shallow copy would share the ToolCalls backing array with in, so a + // write through msgs[i].ToolCalls[j] would land in both and go unseen. + // A JSON round-trip detaches every field. + raw, err := json.Marshal(in) + if err != nil { + t.Fatalf("snapshot: %v", err) + } + var before []llm.Message + if err := json.Unmarshal(raw, &before); err != nil { + t.Fatalf("snapshot: %v", err) + } + if !reflect.DeepEqual(in, before) { + t.Fatalf("the snapshot itself is lossy, so this test cannot detect a write:\n got %+v\nwant %+v", before, in) + } + ensureToolResults(in) + if !reflect.DeepEqual(in, before) { t.Fatalf("input history was rewritten:\n got %+v\nwant %+v", in, before) } From e5f5aa9398b91c99d898013d4f4d5469fe518572 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 19:15:55 +0700 Subject: [PATCH 08/28] Tell a stuck loop apart from ordinary progress Co-authored-by: Cursor --- internal/agent/agent.go | 19 ++++++++++----- internal/agent/harness.go | 26 ++++----------------- internal/agent/harness_test.go | 32 ------------------------- internal/agent/repeat_guard_test.go | 36 +++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 59 deletions(-) create mode 100644 internal/agent/repeat_guard_test.go diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 5e723e7..901f49e 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -610,6 +610,12 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error continue } + // The nudge is held back rather than appended here: a user message + // between the assistant's tool_calls and their results is not a valid + // transcript, and repairing it later is no substitute for not writing + // it. The notice still fires now, so the user sees the repetition the + // moment it is detected. + var repeatNudge string if stuck := repeats.record(resp.ToolCalls); len(stuck) > 0 { if repeats.exceeded() { _ = emit(Event{Type: EventNotice, Message: "stopped: the same tool call kept repeating"}) @@ -618,12 +624,9 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error break } _ = emit(Event{Type: EventNotice, Message: "repeating " + strings.Join(stuck, ", ")}) - history = append(history, llm.Message{ - Role: llm.RoleUser, - Content: "You have called " + strings.Join(stuck, " and ") + - " with the same arguments several times and it is not getting you anywhere. " + - "Do not call it again. Either try a different approach, or say what is blocking you.", - }) + repeatNudge = "You have called " + strings.Join(stuck, " and ") + + " with the same arguments several times and it is not getting you anywhere. " + + "Do not call it again. Either try a different approach, or say what is blocking you." } results := a.executeTools(runCtx, resp.ToolCalls, byName, req, sess, emit) @@ -645,6 +648,10 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error } } + if repeatNudge != "" { + history = append(history, llm.Message{Role: llm.RoleUser, Content: repeatNudge}) + } + // Notes typed while this run was already going land here, which is the // first point the model can act on them without discarding work. for _, note := range drainSteering(sess.ID) { diff --git a/internal/agent/harness.go b/internal/agent/harness.go index 94b4803..3f82642 100644 --- a/internal/agent/harness.go +++ b/internal/agent/harness.go @@ -151,28 +151,12 @@ func normaliseArgs(raw string) string { return string(b) } -// repeatKey builds the fingerprint for a tool call. For write_file and -// edit_file the key uses only the tool name and the target path — not the -// full arguments — so repeated writes to the same file with different -// content are recognised as the same stuck call. Other tools use the full -// normalised arguments as before. +// repeatKey builds the fingerprint for a tool call. Every tool is fingerprinted +// the same way, on the full normalised arguments, and no tool gets a coarser +// key: three different edits to one file are three pieces of work, not one call +// made three times, and a key that discards the arguments cannot tell the +// difference. func repeatKey(c llm.ToolCall) string { - switch c.Name { - case "write_file", "edit_file": - var args struct { - Path string `json:"path"` - } - if json.Unmarshal([]byte(c.Arguments), &args) == nil && args.Path != "" { - return c.Name + "\x00" + args.Path - } - case "vps_upload": - var args struct { - RemotePath string `json:"remote_path"` - } - if json.Unmarshal([]byte(c.Arguments), &args) == nil && args.RemotePath != "" { - return c.Name + "\x00" + args.RemotePath - } - } return c.Name + "\x00" + normaliseArgs(c.Arguments) } diff --git a/internal/agent/harness_test.go b/internal/agent/harness_test.go index 6d2fc1f..a48ace9 100644 --- a/internal/agent/harness_test.go +++ b/internal/agent/harness_test.go @@ -133,38 +133,6 @@ func TestNormaliseArgsToleratesGarbage(t *testing.T) { } } -func TestRepeatKeyWriteFileSamePathDifferentContent(t *testing.T) { - r := newRepeatTracker(2) - // Same path, different content — the old full-args fingerprint would not - // trip. With the path-aware key, repeated writes to the same file are - // recognised as a stuck loop. - path := `{"path":"config.yaml","content":"v1"}` - r.record([]llm.ToolCall{{Name: "write_file", Arguments: path}}) - path2 := `{"path":"config.yaml","content":"v2"}` - got := r.record([]llm.ToolCall{{Name: "write_file", Arguments: path2}}) - if len(got) != 1 || got[0] != "write_file" { - t.Fatalf("same-path different-content write should trip on 2nd call, got %v", got) - } -} - -func TestRepeatKeyEditFileSamePathDifferentContent(t *testing.T) { - r := newRepeatTracker(2) - r.record([]llm.ToolCall{{Name: "edit_file", Arguments: `{"path":"main.go","old_string":"a","new_string":"b"}`}}) - got := r.record([]llm.ToolCall{{Name: "edit_file", Arguments: `{"path":"main.go","old_string":"c","new_string":"d"}`}}) - if len(got) != 1 || got[0] != "edit_file" { - t.Fatalf("same-path different-content edit should trip on 2nd call, got %v", got) - } -} - -func TestRepeatKeyVpsUploadSameRemotePath(t *testing.T) { - r := newRepeatTracker(2) - r.record([]llm.ToolCall{{Name: "vps_upload", Arguments: `{"remote_path":"/tmp/x","local_path":"/a/b"}`}}) - got := r.record([]llm.ToolCall{{Name: "vps_upload", Arguments: `{"remote_path":"/tmp/x","local_path":"/c/d"}`}}) - if len(got) != 1 || got[0] != "vps_upload" { - t.Fatalf("same-remote-path different-local vps_upload should trip, got %v", got) - } -} - func TestRepeatKeyWriteFileDifferentPathDoesNotTrip(t *testing.T) { r := newRepeatTracker(2) r.record([]llm.ToolCall{{Name: "write_file", Arguments: `{"path":"a.txt","content":"x"}`}}) diff --git a/internal/agent/repeat_guard_test.go b/internal/agent/repeat_guard_test.go new file mode 100644 index 0000000..02e9427 --- /dev/null +++ b/internal/agent/repeat_guard_test.go @@ -0,0 +1,36 @@ +package agent + +import ( + "testing" + + "github.com/enowdev/antares/internal/llm" +) + +func TestRepeatKeyDistinguishesDifferentArguments(t *testing.T) { + a := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"a","new":"b"}`} + b := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"c","new":"d"}`} + if repeatKey(a) == repeatKey(b) { + t.Fatal("two different edits to one file share a fingerprint") + } +} + +func TestRepeatKeyStillCatchesAnIdenticalCall(t *testing.T) { + a := llm.ToolCall{Name: "edit_file", Arguments: `{"path":"m.go","old":"a","new":"b"}`} + b := llm.ToolCall{Name: "edit_file", Arguments: `{"new":"b","old":"a","path":"m.go"}`} + if repeatKey(a) != repeatKey(b) { + t.Fatal("the same call re-serialised was not recognised as a repeat") + } +} + +func TestRepeatTrackerTripsOnIdenticalCallsOnly(t *testing.T) { + r := newRepeatTracker(3) + same := llm.ToolCall{Name: "grep", Arguments: `{"pattern":"x"}`} + for i := 1; i <= 2; i++ { + if tripped := r.record([]llm.ToolCall{same}); len(tripped) > 0 { + t.Fatalf("tripped early at %d", i) + } + } + if tripped := r.record([]llm.ToolCall{same}); len(tripped) == 0 { + t.Fatal("three identical calls did not trip the guard") + } +} From 2e6e1a271338eccfb4fb38b2189a865a56a1abe8 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 19:25:35 +0700 Subject: [PATCH 09/28] Cover every tool name the repeat guard used to special-case 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 --- HANDOFF.md | 29 ++++++++--- internal/agent/agent.go | 38 ++++++++++---- internal/agent/harness_test.go | 9 ---- internal/agent/repeat_guard_test.go | 41 +++++++++++++++ internal/agent/turn_messages_test.go | 77 ++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+), 26 deletions(-) create mode 100644 internal/agent/turn_messages_test.go diff --git a/HANDOFF.md b/HANDOFF.md index 8c2d9fe..7cabaa0 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -108,13 +108,28 @@ below is done and green unless called out under **Next Steps**. 1. **Push the 11 local commits** once the user is ready (they have not asked yet — do NOT push without confirmation). -2. **Agent loop guardrail (explicitly deferred, user picked "frontend first").** - The agent can loop writing the same file with slightly different contents; the - repeat tracker fingerprints name+args so changing content never trips it, and - `grContinue` lets the 60-call hard stop reset up to 9× (~600 calls). Proposed - fix: detect repeated `write_file`/`edit_file` to the SAME path (regardless of - content) as a repeat. This is what produced the giant turn that caused the OOM - in item 4 — the frontend is now hardened, but the loop itself remains. +2. **Agent loop guardrail (still open — the fix once proposed here was tried and + reverted).** The agent can loop writing the same file with slightly different + contents; the repeat tracker fingerprints name+args, so changing the content + never trips it. The fix this note used to recommend — treat repeated + `write_file`/`edit_file` to the SAME path as a repeat regardless of content — + was implemented and then removed again in "Tell a stuck loop apart from + ordinary progress". Keying on the path alone cannot tell three different edits + to one file from one edit made three times, so it fired on ordinary work, which + for a coding agent is most of the work. **Do not reintroduce it.** `repeatKey` + is now uniform over the full normalised arguments for every tool, and + `internal/agent/repeat_guard_test.go` fails if any tool is given a coarser key + again. The loop itself is therefore still unsolved: a model that varies the + content each time is bounded only by the ceilings below, and any replacement + needs a signal other than the call fingerprint. This is what produced the giant + turn that caused the OOM in item 4 — the frontend is now hardened, but the loop + remains. + + The ceilings as they actually stand: the hard stop is 60 tool calls per segment + (`HardStopAfter`), `grContinue` may reset it up to 4× (`maxGuardrailContinues`, + `harness.go:477`) for five segments, and `AbsoluteMaxToolCalls: 200` + (`config/defaults.go:137`) caps the whole run regardless — so 200 calls, not the + ~600 this note previously claimed. 3. **Rotate exposed credentials.** The Z.ai API key and Voyage embed key were visible in `~/.antares/config.yaml` read during earlier sessions. Still outstanding; user's call. diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 901f49e..9139318 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -631,7 +631,6 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error results := a.executeTools(runCtx, resp.ToolCalls, byName, req, sess, emit) for i, r := range results { - history = append(history, r.message) if r.isError && i < len(resp.ToolCalls) { failures = append(failures, toolFailure{ Tool: resp.ToolCalls[i].Name, Args: resp.ToolCalls[i].Arguments, Error: r.message.Content, @@ -648,19 +647,14 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error } } - if repeatNudge != "" { - history = append(history, llm.Message{Role: llm.RoleUser, Content: repeatNudge}) - } - // Notes typed while this run was already going land here, which is the // first point the model can act on them without discarding work. - for _, note := range drainSteering(sess.ID) { + notes := drainSteering(sess.ID) + for _, note := range notes { _ = emit(Event{Type: EventNotice, Message: "steering: " + note}) - history = append(history, llm.Message{ - Role: llm.RoleUser, - Content: "A new instruction arrived while you were working: " + note, - }) } + + history = appendTurnMessages(history, results, repeatNudge, notes) } if turn > maxTurns { @@ -746,6 +740,30 @@ type toolOutcome struct { isError bool } +// appendTurnMessages assembles the tail of one turn: every tool result first, +// then the repetition nudge, then any steering note. The order is the whole +// point. A user message sitting between an assistant's tool_calls and their +// results is not a valid transcript, and ensureToolResults repairing it at send +// time is no reason to write it — the repair is silent, so a nudge that drifts +// back above the results would leave every test green while the transcript we +// build is wrong. Keeping the order in one pure function is what makes it +// assertable without a client, a store or a server. +func appendTurnMessages(history []llm.Message, results []toolOutcome, nudge string, notes []string) []llm.Message { + for _, r := range results { + history = append(history, r.message) + } + if nudge != "" { + history = append(history, llm.Message{Role: llm.RoleUser, Content: nudge}) + } + for _, note := range notes { + history = append(history, llm.Message{ + Role: llm.RoleUser, + Content: "A new instruction arrived while you were working: " + note, + }) + } + return history +} + // executeTools runs the requested calls, in parallel when the config allows. func (a *Agent) executeTools( ctx context.Context, diff --git a/internal/agent/harness_test.go b/internal/agent/harness_test.go index a48ace9..2c2b7f5 100644 --- a/internal/agent/harness_test.go +++ b/internal/agent/harness_test.go @@ -132,12 +132,3 @@ func TestNormaliseArgsToleratesGarbage(t *testing.T) { t.Fatalf("got %q", got) } } - -func TestRepeatKeyWriteFileDifferentPathDoesNotTrip(t *testing.T) { - r := newRepeatTracker(2) - r.record([]llm.ToolCall{{Name: "write_file", Arguments: `{"path":"a.txt","content":"x"}`}}) - got := r.record([]llm.ToolCall{{Name: "write_file", Arguments: `{"path":"b.txt","content":"x"}`}}) - if len(got) != 0 { - t.Fatalf("different paths should not trip: %v", got) - } -} diff --git a/internal/agent/repeat_guard_test.go b/internal/agent/repeat_guard_test.go index 02e9427..710a803 100644 --- a/internal/agent/repeat_guard_test.go +++ b/internal/agent/repeat_guard_test.go @@ -22,6 +22,47 @@ func TestRepeatKeyStillCatchesAnIdenticalCall(t *testing.T) { } } +// The special cases this guard used to carry were dispatched on tool name, so +// every name is its own branch and needs its own assertion. One tool can be +// given back a fingerprint that ignores most of its arguments while every other +// tool stays uniform, and nothing about the remaining tools would notice. These +// are the two names that carried a case; edit_file is here for the third. +func TestRepeatKeyUsesFullArgumentsForEveryTool(t *testing.T) { + for _, tc := range []struct { + tool string + a, b string + what string + }{ + { + tool: "write_file", + a: `{"path":"config.yaml","content":"v1"}`, + b: `{"path":"config.yaml","content":"v2"}`, + what: "two writes of different content to one path", + }, + { + tool: "vps_upload", + a: `{"remote_path":"/tmp/x","local_path":"/a/b"}`, + b: `{"remote_path":"/tmp/x","local_path":"/c/d"}`, + what: "two uploads of different local files to one remote path", + }, + { + tool: "edit_file", + a: `{"path":"main.go","old_string":"a","new_string":"b"}`, + b: `{"path":"main.go","old_string":"c","new_string":"d"}`, + what: "two different edits to one file", + }, + } { + t.Run(tc.tool, func(t *testing.T) { + x := llm.ToolCall{Name: tc.tool, Arguments: tc.a} + y := llm.ToolCall{Name: tc.tool, Arguments: tc.b} + if got := repeatKey(x); got == repeatKey(y) { + t.Fatalf("%s share the fingerprint %q, so %s is keyed on a subset of its arguments", + tc.what, got, tc.tool) + } + }) + } +} + func TestRepeatTrackerTripsOnIdenticalCallsOnly(t *testing.T) { r := newRepeatTracker(3) same := llm.ToolCall{Name: "grep", Arguments: `{"pattern":"x"}`} diff --git a/internal/agent/turn_messages_test.go b/internal/agent/turn_messages_test.go new file mode 100644 index 0000000..ba05b7d --- /dev/null +++ b/internal/agent/turn_messages_test.go @@ -0,0 +1,77 @@ +package agent + +import ( + "reflect" + "testing" + + "github.com/enowdev/antares/internal/llm" +) + +func toolOutcomes(ids ...string) []toolOutcome { + out := make([]toolOutcome, 0, len(ids)) + for _, id := range ids { + out = append(out, toolOutcome{ + message: llm.Message{Role: llm.RoleTool, ToolCallID: id, Name: "read_file", Content: "result " + id}, + }) + } + return out +} + +// The repetition nudge used to be appended before the tools ran, which put a +// user message between the assistant's tool_calls and their results. That is +// the malformation ensureToolResults repairs at send time, and because the +// repair is silent the whole suite stayed green while the transcript being +// built was invalid. This is the assertion that notices. +func TestAppendTurnMessagesKeepsEveryToolResultBeforeTheNudge(t *testing.T) { + const nudge = "You have called read_file with the same arguments several times." + + out := appendTurnMessages( + []llm.Message{{Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "c1"}, {ID: "c2"}}}}, + toolOutcomes("c1", "c2"), + nudge, + []string{"a steering note"}, + ) + + nudgeAt := -1 + for i, m := range out { + if m.Role == llm.RoleUser && m.Content == nudge { + nudgeAt = i + break + } + } + if nudgeAt < 0 { + t.Fatalf("the nudge was never appended: %q", roleAndContent(out)) + } + + for i, m := range out { + if m.Role == llm.RoleTool && i > nudgeAt { + t.Fatalf("tool result %q lands at %d, after the nudge at %d — a user message between an "+ + "assistant's tool_calls and their results is not a valid transcript: %q", + m.ToolCallID, i, nudgeAt, roleAndContent(out)) + } + } +} + +func TestAppendTurnMessagesOrdersResultsThenNudgeThenNotes(t *testing.T) { + out := appendTurnMessages(nil, toolOutcomes("c1", "c2"), "stop repeating", []string{"first note", "second note"}) + + want := []string{ + "tool:result c1", + "tool:result c2", + "user:stop repeating", + "user:A new instruction arrived while you were working: first note", + "user:A new instruction arrived while you were working: second note", + } + if got := roleAndContent(out); !reflect.DeepEqual(got, want) { + t.Fatalf("turn tail assembled out of order:\n got %q\nwant %q", got, want) + } +} + +func TestAppendTurnMessagesOmitsAnAbsentNudge(t *testing.T) { + out := appendTurnMessages(nil, toolOutcomes("c1"), "", nil) + + want := []string{"tool:result c1"} + if got := roleAndContent(out); !reflect.DeepEqual(got, want) { + t.Fatalf("an empty nudge became a message:\n got %q\nwant %q", got, want) + } +} From bf29f637461526f345f4fefedeb352d9313c089b Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 19:36:30 +0700 Subject: [PATCH 10/28] Stop a stalled follower from crashing its own stream Co-authored-by: Cursor --- internal/server/livechat.go | 19 ++++-- internal/server/livechat_follow_test.go | 83 +++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) create mode 100644 internal/server/livechat_follow_test.go diff --git a/internal/server/livechat.go b/internal/server/livechat.go index 7ff6596..b950b14 100644 --- a/internal/server/livechat.go +++ b/internal/server/livechat.go @@ -66,12 +66,19 @@ func (lr *liveRun) follow(ctx context.Context, cursor int, send func(agent.Event i := cursor // absolute event index for { lr.mu.Lock() - // If the cursor points at events already trimmed, fast-forward to the - // oldest retained event rather than reading a negative slice index. - if i < lr.base { - i = lr.base - } - for i-lr.base < len(lr.events) { + for { + // If the cursor points at events already trimmed, fast-forward to the + // oldest retained event rather than reading a negative slice index. + // This has to run on every re-acquisition of the lock, not just on + // entry: the lock is dropped around send below, so a client too slow + // to drain its socket can be overtaken there by a publisher trimming + // the window. + if i < lr.base { + i = lr.base + } + if i-lr.base >= len(lr.events) { + break + } e := lr.events[i-lr.base] i++ // A reconnect may have thousands of token-sized deltas waiting. Collapse diff --git a/internal/server/livechat_follow_test.go b/internal/server/livechat_follow_test.go new file mode 100644 index 0000000..ea4ac5c --- /dev/null +++ b/internal/server/livechat_follow_test.go @@ -0,0 +1,83 @@ +package server + +import ( + "context" + "testing" + "time" + + "github.com/enowdev/antares/internal/agent" +) + +// A client whose socket buffer is full stalls inside send with the run's lock +// dropped, while the turn keeps publishing — a build streaming through terminal +// emits one tool_progress event per chunk, and those are not coalesced — so the +// window trims past the stalled cursor. follow must survive that and pick up at +// the oldest event still retained. +func TestFollowSurvivesWindowTrimWhileSending(t *testing.T) { + lr := newLiveRun() + // Seed one event so the follower has a backlog to send at once: the handshake + // below needs it inside send, not parked waiting for the first publish. + lr.publish(agent.Event{Type: agent.EventToolProgress, Chunk: "seed"}) + + type followResult struct { + panicked any + err error + cursors []int + } + result := make(chan followResult, 1) + entered := make(chan struct{}) + release := make(chan struct{}) + + go func() { + var res followResult + defer func() { + res.panicked = recover() + result <- res + }() + stalled := false + res.err = lr.follow(context.Background(), 0, func(_ agent.Event, cursor int) error { + res.cursors = append(res.cursors, cursor) + if !stalled { + stalled = true + close(entered) + <-release + } + return nil + }) + }() + + // The follower is now inside send with the lock released; overrun the window + // from under it. + <-entered + const overrun = 200 + for i := 0; i < maxLiveEvents+overrun; i++ { + lr.publish(agent.Event{Type: agent.EventToolProgress, Chunk: "x"}) + } + close(release) + lr.finish() + + var res followResult + select { + case res = <-result: + case <-time.After(30 * time.Second): + t.Fatal("follow did not return") + } + if res.panicked != nil { + t.Fatalf("follow panicked: %v", res.panicked) + } + if res.err != nil { + t.Fatalf("follow: %v", res.err) + } + + // One seed plus maxLiveEvents+overrun published, so the oldest retained event + // sits at absolute index overrun+1 and send reports the cursor just past it. + if len(res.cursors) < 2 { + t.Fatalf("follower saw %d events, want the seed plus the retained window", len(res.cursors)) + } + if got, want := res.cursors[1], overrun+2; got != want { + t.Fatalf("resumed at cursor %d, want %d (oldest retained event)", got, want) + } + if got, want := len(res.cursors), 1+maxLiveEvents; got != want { + t.Fatalf("follower saw %d events, want %d", got, want) + } +} From 3fefdf1502429b7598531ce86f497ba0e28b9336 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 19:49:59 +0700 Subject: [PATCH 11/28] Scan for danger by what a tool can do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/agent/agent.go | 13 +++--- internal/agent/approval.go | 42 +++++++++++++---- internal/agent/approval_test.go | 2 +- internal/agent/danger_test.go | 77 +++++++++++++++++++++++++++++++ internal/agent/untrusted_test.go | 11 +++-- internal/tools/browser.go | 4 ++ internal/tools/capability_test.go | 58 +++++++++++++++++++++++ internal/tools/httprequest.go | 4 ++ internal/tools/register.go | 42 +++++++++++++++++ internal/tools/registry.go | 18 ++++++++ internal/tools/shell.go | 7 +++ internal/tools/vps.go | 8 ++++ internal/tools/web.go | 7 +++ 13 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 internal/agent/danger_test.go create mode 100644 internal/tools/capability_test.go diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 9139318..1792281 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -941,7 +941,7 @@ func (a *Agent) executeTools( // What the model sees may be fenced as untrusted; what the UI shows stays // raw. Errors are our own messages, so they are never fenced. modelContent := content - if !res.IsError && a.config().Agent.WrapUntrustedOutput && untrustedTool(call.Name) { + if !res.IsError && a.config().Agent.WrapUntrustedOutput && untrustedTool(tool) { modelContent = wrapUntrusted(call.Name, content) } @@ -1163,13 +1163,14 @@ func (a *Agent) guardrailTripped(toolCalls int, emit Emit) bool { // untrustedTool reports whether a tool returns content fetched from outside — // web pages, HTTP responses, search snippets, or MCP servers — which an attacker -// could have seeded with instructions aimed at the model. -func untrustedTool(name string) bool { - switch name { - case "web_fetch", "web_search", "browser", "http_request": +// could have seeded with instructions aimed at the model. A tool borrowed from +// an MCP server is written outside this codebase and so cannot declare the +// capability in Go; for those the namespace is the declaration. +func untrustedTool(tool tools.Tool) bool { + if tools.ReturnsUntrustedOutput(tool) { return true } - return strings.HasPrefix(name, tools.MCPPrefix) + return strings.HasPrefix(tool.Name(), tools.MCPPrefix) } // wrapUntrusted fences external content so the model reads it as data. The diff --git a/internal/agent/approval.go b/internal/agent/approval.go index 92e3663..2d0e053 100644 --- a/internal/agent/approval.go +++ b/internal/agent/approval.go @@ -84,7 +84,7 @@ func (a *Agent) checkApproval(ctx context.Context, call llm.ToolCall, tool tools mode = "auto" } - danger := dangerIn(call.Name, call.Arguments) + danger := dangerInTool(tool, call.Arguments) switch mode { case "auto": @@ -176,11 +176,18 @@ func approvalMessage(r *ApprovalRequest) string { // dangerous names commands that are worth stopping for even when approval is // otherwise off. Each entry says, in words that finish "…and it", what the // command does — the message is only useful if it explains the risk. +// +// This table explains a call; it does not decide whether one is gated. No list +// of patterns can enumerate what a shell can do, so tools.NeedsApproval stays +// the gate and a command matching nothing here is not thereby safe. var dangerous = []struct { re *regexp.Regexp why string }{ - {regexp.MustCompile(`\brm\s+(-[a-zA-Z]*[rf][a-zA-Z]*\s+)+(/|~|\$HOME|\*)(\s|$)`), "it deletes a whole tree from your home or root"}, + // The target may be any path under root or a home directory, not only a + // bare one: rm -rf /home/someone empties an account as surely as rm -rf / + // empties a machine. + {regexp.MustCompile(`\brm\s+(-[a-zA-Z]*[rf][a-zA-Z]*\s+)+(/|~|\$HOME|\*)\S*`), "it deletes a whole tree from your home or root"}, {regexp.MustCompile(`\bmkfs(\.\w+)?\b`), "it formats a filesystem"}, {regexp.MustCompile(`\bdd\b[^\n]*\bof=/dev/`), "it writes directly to a device"}, {regexp.MustCompile(`>\s*/dev/(sd|nvme|hd)`), "it writes directly to a disk"}, @@ -194,19 +201,34 @@ var dangerous = []struct { {regexp.MustCompile(`\bsudo\b`), "it runs as root"}, } -// dangerIn reports why a call is destructive, or an empty string when it is -// ordinary. Only the terminal is scanned: it is the tool that can do anything. +// unreadableArguments is the reason for a call nobody can decode. A call that +// cannot be read is unknown, and unknown must not pass as ordinary. +const unreadableArguments = "its arguments could not be read, so it could not be checked" + +// dangerIn reports why the named call is destructive, or an empty string when +// it is ordinary. It resolves the name against the process registry; a caller +// that already holds the tool passes it to dangerInTool, so that the object +// scanned is the object about to run. func dangerIn(toolName, arguments string) string { - if toolName != "terminal" { + tool, ok := tools.Default().Get(toolName) + if !ok { return "" } - var args struct { - Command string `json:"command"` - } - if json.Unmarshal([]byte(arguments), &args) != nil { + return dangerInTool(tool, arguments) +} + +// dangerInTool reports why a call is destructive, or an empty string when it +// is ordinary. Every tool that runs shell commands is scanned and no other, +// whatever any of them is called: the danger is in what reaches a shell, not +// in which tool carried it there. +func dangerInTool(tool tools.Tool, arguments string) string { + if !tools.RunsShellCommands(tool) { return "" } - cmd := args.Command + cmd, ok := tools.CommandOf(tool, json.RawMessage(arguments)) + if !ok { + return unreadableArguments + } if strings.TrimSpace(cmd) == "" { return "" } diff --git a/internal/agent/approval_test.go b/internal/agent/approval_test.go index f5a7860..39e4db5 100644 --- a/internal/agent/approval_test.go +++ b/internal/agent/approval_test.go @@ -58,7 +58,7 @@ func TestAutoModeStillNamesDangerousCommands(t *testing.T) { return nil } call := llm.ToolCall{ID: "1", Name: "terminal", Arguments: `{"command":"sudo systemctl restart nginx"}`} - if res := a.checkApproval(context.Background(), call, writingTool{"terminal"}, "s", emit); res != nil { + if res := a.checkApproval(context.Background(), call, lookupTool(t, "terminal"), "s", emit); res != nil { t.Fatalf("auto mode blocked: %s", res.Content) } if len(notices) != 1 || !strings.Contains(notices[0], "root") { diff --git a/internal/agent/danger_test.go b/internal/agent/danger_test.go new file mode 100644 index 0000000..aadee29 --- /dev/null +++ b/internal/agent/danger_test.go @@ -0,0 +1,77 @@ +package agent + +import ( + "context" + "strings" + "testing" + + "github.com/enowdev/antares/internal/llm" + "github.com/enowdev/antares/internal/tools" +) + +// lookupTool resolves a tool from the process registry, so these tests fail if +// the capability is declared on anything other than the object that runs. +func lookupTool(t *testing.T, name string) tools.Tool { + t.Helper() + tool, ok := tools.Default().Get(name) + if !ok { + t.Fatalf("tool %q is not registered", name) + } + return tool +} + +func TestDangerScanFollowsCapabilityNotName(t *testing.T) { + cases := []struct { + tool string + args string + want bool + }{ + {"terminal", `{"command":"rm -rf /"}`, true}, + {"terminal", `{"command":"rm -rf /home/someone"}`, true}, + {"terminal", `{"command":"rm -rf ~/projects"}`, true}, + {"terminal", `{"command":"ls -la"}`, false}, + {"vps_run", `{"vps":"prod","command":"rm -rf / --no-preserve-root"}`, true}, + {"vps_run", `{"vps":"prod","command":"mkfs.ext4 /dev/sda1"}`, true}, + {"vps_run", `{"vps":"prod","command":"systemctl status nginx"}`, false}, + } + for _, c := range cases { + got := dangerInTool(lookupTool(t, c.tool), c.args) != "" + if got != c.want { + t.Errorf("%s %s -> danger=%v, want %v", c.tool, c.args, got, c.want) + } + } +} + +func TestUnparseableArgumentsFailClosed(t *testing.T) { + if dangerInTool(lookupTool(t, "terminal"), "not json at all") == "" { + t.Fatal("arguments that cannot be parsed were treated as safe") + } +} + +// A tool that runs no commands has nothing for the scan to read, however +// alarming its arguments look. +func TestAToolWithoutAShellIsNotScanned(t *testing.T) { + if why := dangerInTool(lookupTool(t, "write_file"), `{"path":"sudo.txt"}`); why != "" { + t.Errorf("write_file was scanned for shell danger: %s", why) + } +} + +// The remote wipe this refactor exists for: auto mode runs it, but it has to +// say so. +func TestAutoModeNamesADangerousRemoteCommand(t *testing.T) { + a := agentWithMode("auto") + var notices []string + emit := func(e Event) error { + if e.Type == EventNotice { + notices = append(notices, e.Message) + } + return nil + } + call := llm.ToolCall{ID: "1", Name: "vps_run", Arguments: `{"vps":"prod","command":"rm -rf / --no-preserve-root"}`} + if res := a.checkApproval(context.Background(), call, lookupTool(t, "vps_run"), "s", emit); res != nil { + t.Fatalf("auto mode blocked: %s", res.Content) + } + if len(notices) != 1 || !strings.Contains(notices[0], "deletes") { + t.Fatalf("expected a notice that the remote command deletes a tree, got %v", notices) + } +} diff --git a/internal/agent/untrusted_test.go b/internal/agent/untrusted_test.go index d34300d..9b2597a 100644 --- a/internal/agent/untrusted_test.go +++ b/internal/agent/untrusted_test.go @@ -5,13 +5,18 @@ import "strings" import "testing" func TestUntrustedToolClassification(t *testing.T) { - for _, name := range []string{"web_fetch", "web_search", "browser", "http_request", "mcp__server__tool"} { - if !untrustedTool(name) { + for _, name := range []string{"web_fetch", "web_search", "browser", "http_request"} { + if !untrustedTool(lookupTool(t, name)) { t.Errorf("%s should be treated as untrusted", name) } } + // A tool borrowed from an MCP server is written outside this codebase, so + // its name is the only declaration available. + if !untrustedTool(writingTool{"mcp__server__tool"}) { + t.Error("a tool from an MCP server should be treated as untrusted") + } for _, name := range []string{"read_file", "terminal", "memory", "skill"} { - if untrustedTool(name) { + if untrustedTool(lookupTool(t, name)) { t.Errorf("%s should not be treated as untrusted", name) } } diff --git a/internal/tools/browser.go b/internal/tools/browser.go index 5942611..d7e2584 100644 --- a/internal/tools/browser.go +++ b/internal/tools/browser.go @@ -135,6 +135,10 @@ func (browserTool) Schema() map[string]any { // a page as the signed-in user. func (browserTool) RequiresApproval() bool { return true } +// UntrustedOutput reports that page text, snapshots, and console output are +// written by the site being driven. +func (browserTool) UntrustedOutput() bool { return true } + func (browserTool) Execute(ctx context.Context, in Input) Result { var args struct { Action string `json:"action"` diff --git a/internal/tools/capability_test.go b/internal/tools/capability_test.go new file mode 100644 index 0000000..c9fcbea --- /dev/null +++ b/internal/tools/capability_test.go @@ -0,0 +1,58 @@ +package tools + +import ( + "encoding/json" + "testing" +) + +func TestCommandOfReadsWhateverToolHoldsAShell(t *testing.T) { + cases := []struct { + tool Tool + args string + want string + }{ + {terminalTool{}, `{"command":"ls -la"}`, "ls -la"}, + {vpsRunTool{}, `{"vps":"prod","command":"systemctl restart nginx"}`, "systemctl restart nginx"}, + // Listing the saved servers takes no command at all. + {vpsRunTool{}, `{"vps":"prod"}`, ""}, + } + for _, c := range cases { + got, ok := CommandOf(c.tool, json.RawMessage(c.args)) + if !ok { + t.Errorf("%s %s: arguments were not readable", c.tool.Name(), c.args) + continue + } + if got != c.want { + t.Errorf("%s %s -> %q, want %q", c.tool.Name(), c.args, got, c.want) + } + } +} + +func TestCommandOfSeparatesNoShellFromUnreadableArguments(t *testing.T) { + if RunsShellCommands(namedTestTool("write_file")) { + t.Error("a tool that runs no commands was reported as holding a shell") + } + if _, ok := CommandOf(namedTestTool("write_file"), json.RawMessage(`{"path":"a"}`)); ok { + t.Error("a tool that runs no commands returned a command") + } + + if !RunsShellCommands(terminalTool{}) { + t.Fatal("the terminal was not reported as holding a shell") + } + if _, ok := CommandOf(terminalTool{}, json.RawMessage("not json at all")); ok { + t.Error("unreadable arguments were reported as read") + } +} + +func TestUntrustedOutputIsDeclaredByTheToolsThatFetch(t *testing.T) { + for _, tool := range []Tool{webFetchTool{}, webSearchTool{}, browserTool{}, httpRequestTool{}} { + if !ReturnsUntrustedOutput(tool) { + t.Errorf("%s does not declare that its output comes from outside", tool.Name()) + } + } + for _, tool := range []Tool{terminalTool{}, readFileTool{}} { + if ReturnsUntrustedOutput(tool) { + t.Errorf("%s declares outside content it does not fetch", tool.Name()) + } + } +} diff --git a/internal/tools/httprequest.go b/internal/tools/httprequest.go index f73d305..73621a8 100644 --- a/internal/tools/httprequest.go +++ b/internal/tools/httprequest.go @@ -84,6 +84,10 @@ func (httpRequestTool) Schema() map[string]any { // state-changing requests (POST/PUT/DELETE) to external services. func (httpRequestTool) RequiresApproval() bool { return true } +// UntrustedOutput reports that a response body is written by the service being +// called. +func (httpRequestTool) UntrustedOutput() bool { return true } + func (httpRequestTool) Execute(ctx context.Context, in Input) Result { var args struct { Method string `json:"method"` diff --git a/internal/tools/register.go b/internal/tools/register.go index 8efd2ba..0b86fef 100644 --- a/internal/tools/register.go +++ b/internal/tools/register.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -110,6 +111,47 @@ func NeedsApproval(t Tool) bool { return false } +// RunsShellCommands reports whether a tool hands commands to a shell. +func RunsShellCommands(t Tool) bool { + _, ok := t.(ShellCommander) + return ok +} + +// CommandOf returns the command a call would run at a shell. It reports false +// both for a tool that runs no commands and for one that does but whose +// arguments could not be read; RunsShellCommands tells those two apart, and a +// caller that inspects commands must not read either as "nothing to run". +func CommandOf(t Tool, args json.RawMessage) (string, bool) { + c, ok := t.(ShellCommander) + if !ok { + return "", false + } + return c.ShellCommand(args) +} + +// ReturnsUntrustedOutput reports whether a tool's result carries content from +// outside this machine. +func ReturnsUntrustedOutput(t Tool) bool { + u, ok := t.(UntrustedOutputer) + return ok && u.UntrustedOutput() +} + +// commandArgument decodes the "command" argument shared by the tools that run +// shell commands. Absent arguments read as no command, matching Input.Bind, so +// only arguments that are present and malformed count as unreadable. +func commandArgument(args json.RawMessage) (string, bool) { + if strings.TrimSpace(string(args)) == "" { + return "", true + } + var decoded struct { + Command string `json:"command"` + } + if json.Unmarshal(args, &decoded) != nil { + return "", false + } + return decoded.Command, true +} + // ---- delegate_task ---------------------------------------------------------- type delegateTool struct{} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index f89627f..1a095bf 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -98,6 +98,24 @@ type Approval interface { RequiresApproval() bool } +// ShellCommander is implemented by a tool that hands a command to a shell, +// whichever machine that shell runs on. Anything inspecting commands has to +// find its subjects by this capability rather than by name: a command sent to +// a remote host destroys as thoroughly as the same command run locally. +type ShellCommander interface { + // ShellCommand returns the command these arguments would run. It reports + // false when the arguments cannot be read, which is unknown rather than + // harmless and must not be treated as "no command". + ShellCommand(args json.RawMessage) (string, bool) +} + +// UntrustedOutputer is implemented by a tool whose result carries content from +// outside this machine — a page, a search snippet, an API response — which +// whoever wrote it may have seeded with instructions aimed at the model. +type UntrustedOutputer interface { + UntrustedOutput() bool +} + // Registry holds the process-wide tool set. type Registry struct { mu sync.RWMutex diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 42353e6..170f06f 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -3,6 +3,7 @@ package tools import ( "bytes" "context" + "encoding/json" "fmt" "io" "log/slog" @@ -594,6 +595,12 @@ func (terminalTool) Schema() map[string]any { }, "command") } +// ShellCommand lets callers read the command this call would run without +// knowing which tool holds the shell. +func (terminalTool) ShellCommand(args json.RawMessage) (string, bool) { + return commandArgument(args) +} + func (terminalTool) Execute(ctx context.Context, in Input) Result { var args struct { Command string `json:"command"` diff --git a/internal/tools/vps.go b/internal/tools/vps.go index 54af544..af3382f 100644 --- a/internal/tools/vps.go +++ b/internal/tools/vps.go @@ -2,6 +2,7 @@ package tools import ( "context" + "encoding/json" "errors" "fmt" "strings" @@ -104,6 +105,13 @@ func (vpsRunTool) Schema() map[string]any { // potentially destructive action — gate it behind approval like the terminal. func (vpsRunTool) RequiresApproval() bool { return true } +// ShellCommand exposes the command bound for the remote host to the same +// inspection the local terminal gets: the host it lands on changes nothing +// about what the command does. +func (vpsRunTool) ShellCommand(args json.RawMessage) (string, bool) { + return commandArgument(args) +} + func (vpsRunTool) Execute(ctx context.Context, in Input) Result { var args struct { VPS string `json:"vps"` diff --git a/internal/tools/web.go b/internal/tools/web.go index e6536b8..12207ff 100644 --- a/internal/tools/web.go +++ b/internal/tools/web.go @@ -33,6 +33,9 @@ func (webFetchTool) Schema() map[string]any { }, "url") } +// UntrustedOutput reports that a fetched page is written by whoever owns it. +func (webFetchTool) UntrustedOutput() bool { return true } + func (webFetchTool) Execute(ctx context.Context, in Input) Result { var args struct { URL string `json:"url"` @@ -154,6 +157,10 @@ func (webSearchTool) Schema() map[string]any { }, "query") } +// UntrustedOutput reports that titles and snippets are written by the pages +// they point at. +func (webSearchTool) UntrustedOutput() bool { return true } + func (webSearchTool) Execute(ctx context.Context, in Input) Result { var args struct { Query string `json:"query"` From 50e1fa4d6ef99ca9bc48c39fefa5083d402af011 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 19:59:49 +0700 Subject: [PATCH 12/28] Read a command the way the shell will read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/agent/approval.go | 22 +++++++++++----- internal/agent/danger_test.go | 44 +++++++++++++++++++++++++++++++ internal/tools/capability_test.go | 23 ++++++++++++++++ internal/tools/register.go | 13 ++++----- 4 files changed, 89 insertions(+), 13 deletions(-) diff --git a/internal/agent/approval.go b/internal/agent/approval.go index 2d0e053..ef031ec 100644 --- a/internal/agent/approval.go +++ b/internal/agent/approval.go @@ -177,17 +177,25 @@ func approvalMessage(r *ApprovalRequest) string { // otherwise off. Each entry says, in words that finish "…and it", what the // command does — the message is only useful if it explains the risk. // -// This table explains a call; it does not decide whether one is gated. No list -// of patterns can enumerate what a shell can do, so tools.NeedsApproval stays -// the gate and a command matching nothing here is not thereby safe. +// A reason from this table does also force a call through approval, but that +// decides nothing on its own: every tool that reaches a shell already requires +// approval, so those calls are gated whether or not the table recognises them. +// What the table decides is what the person is told, which is why an entry that +// describes a command wrongly is worse than no entry at all. No list of +// patterns can enumerate what a shell can do, so matching nothing here never +// amounts to safe. var dangerous = []struct { re *regexp.Regexp why string }{ - // The target may be any path under root or a home directory, not only a - // bare one: rm -rf /home/someone empties an account as surely as rm -rf / - // empties a machine. - {regexp.MustCompile(`\brm\s+(-[a-zA-Z]*[rf][a-zA-Z]*\s+)+(/|~|\$HOME|\*)\S*`), "it deletes a whole tree from your home or root"}, + // A bare target is the whole of something: / is the machine, ~ and $HOME + // the account, * everything in the current directory. + {regexp.MustCompile(`\brm\s+(-[a-zA-Z]*[rf][a-zA-Z]*\s+)+(/|~|\$HOME|\*)(\s|$)`), "it deletes everything at that path"}, + // A recursive delete takes the named directory and everything under it, + // wherever it sits: rm -rf /home/someone empties an account. Only -r earns + // this reason, because rm -f on a path removes the files named and nothing + // more, and describing routine cleanup as a wipe teaches people to skim. + {regexp.MustCompile(`\brm\s+(-[a-zA-Z]*[rR][a-zA-Z]*\s+)+(/|~|\$HOME|\*)\S*`), "it deletes a whole directory tree"}, {regexp.MustCompile(`\bmkfs(\.\w+)?\b`), "it formats a filesystem"}, {regexp.MustCompile(`\bdd\b[^\n]*\bof=/dev/`), "it writes directly to a device"}, {regexp.MustCompile(`>\s*/dev/(sd|nvme|hd)`), "it writes directly to a disk"}, diff --git a/internal/agent/danger_test.go b/internal/agent/danger_test.go index aadee29..7115a54 100644 --- a/internal/agent/danger_test.go +++ b/internal/agent/danger_test.go @@ -56,6 +56,50 @@ func TestAToolWithoutAShellIsNotScanned(t *testing.T) { } } +// Execute reads the arguments with a decoder that stops at the end of the +// first JSON value, so a scan that rejected the trailing byte would answer +// "could not be read" about a root wipe the shell is still about to run. +func TestTrailingDataCannotHideTheCommand(t *testing.T) { + why := dangerInTool(lookupTool(t, "terminal"), `{"command":"rm -rf /"} x`) + if why != "it deletes everything at that path" { + t.Fatalf("reason = %q, want the root wipe named", why) + } +} + +// The reason is the sentence approvalMessage puts in front of whoever is +// deciding, so it has to describe the command in hand. Calling routine cleanup +// a wipe of home or root teaches people to skim the question. +func TestDeleteReasonsDescribeWhatIsDeleted(t *testing.T) { + const ( + everything = "it deletes everything at that path" + wholeTree = "it deletes a whole directory tree" + ) + cases := []struct { + cmd string + want string + }{ + {"rm -rf /", everything}, + {"rm -rf ~", everything}, + {"rm -rf $HOME", everything}, + {"rm -f *", everything}, + {"rm -rf /home/someone", wholeTree}, + {"rm -rf ~/projects", wholeTree}, + {"rm -rf /tmp/scratch-1234", wholeTree}, + {"rm -Rf /var/lib/thing", wholeTree}, + {"rm -f /tmp/build.log", ""}, + {"rm -f ~/.cache/thumb.png", ""}, + {"rm -f *.o", ""}, + {"npm run build && rm -f /tmp/x.log", ""}, + {"git rm -f /tmp/x", ""}, + } + for _, c := range cases { + got := dangerInTool(lookupTool(t, "terminal"), `{"command":`+quote(c.cmd)+`}`) + if got != c.want { + t.Errorf("%s -> %q, want %q", c.cmd, got, c.want) + } + } +} + // The remote wipe this refactor exists for: auto mode runs it, but it has to // say so. func TestAutoModeNamesADangerousRemoteCommand(t *testing.T) { diff --git a/internal/tools/capability_test.go b/internal/tools/capability_test.go index c9fcbea..1226835 100644 --- a/internal/tools/capability_test.go +++ b/internal/tools/capability_test.go @@ -44,6 +44,29 @@ func TestCommandOfSeparatesNoShellFromUnreadableArguments(t *testing.T) { } } +// Whatever inspects a call and whatever runs it are handed the same bytes, so +// they have to read them the same way. Input.Bind stops at the end of the first +// JSON value; a stricter read here would let one trailing byte give the two a +// different view of the same command. +func TestCommandOfReadsArgumentsTheWayExecuteWill(t *testing.T) { + const args = `{"command":"rm -rf /"} x` + + scanned, ok := CommandOf(terminalTool{}, json.RawMessage(args)) + if !ok { + t.Fatal("trailing data made the command unreadable to the scan") + } + var bound struct { + Command string `json:"command"` + } + in := Input{Args: json.RawMessage(args)} + if err := in.Bind(&bound); err != nil { + t.Fatalf("Bind rejected what Execute will be given: %v", err) + } + if scanned != bound.Command { + t.Fatalf("the scan reads %q, Execute will run %q", scanned, bound.Command) + } +} + func TestUntrustedOutputIsDeclaredByTheToolsThatFetch(t *testing.T) { for _, tool := range []Tool{webFetchTool{}, webSearchTool{}, browserTool{}, httpRequestTool{}} { if !ReturnsUntrustedOutput(tool) { diff --git a/internal/tools/register.go b/internal/tools/register.go index 0b86fef..4788a2e 100644 --- a/internal/tools/register.go +++ b/internal/tools/register.go @@ -137,16 +137,17 @@ func ReturnsUntrustedOutput(t Tool) bool { } // commandArgument decodes the "command" argument shared by the tools that run -// shell commands. Absent arguments read as no command, matching Input.Bind, so -// only arguments that are present and malformed count as unreadable. +// shell commands. It reads through Input.Bind rather than alongside it: an +// inspection that parsed these bytes even slightly more strictly than Execute +// does would let a call be dismissed as unreadable while the shell went ahead +// and ran it. Absent arguments read as no command, which is what Bind leaves +// behind, so only arguments that are present and malformed are unreadable. func commandArgument(args json.RawMessage) (string, bool) { - if strings.TrimSpace(string(args)) == "" { - return "", true - } var decoded struct { Command string `json:"command"` } - if json.Unmarshal(args, &decoded) != nil { + in := Input{Args: args} + if in.Bind(&decoded) != nil { return "", false } return decoded.Command, true From d9d5df00ac351cdead7cf2ea2136090dc733aade Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 20:37:21 +0700 Subject: [PATCH 13/28] Let a denying plugin deny even when it exits badly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/plugin/gate_test.go | 242 +++++++++++++++++++++++++++++++++++ internal/plugin/plugin.go | 85 ++++++++---- 2 files changed, 305 insertions(+), 22 deletions(-) create mode 100644 internal/plugin/gate_test.go diff --git a/internal/plugin/gate_test.go b/internal/plugin/gate_test.go new file mode 100644 index 0000000..ebceaa5 --- /dev/null +++ b/internal/plugin/gate_test.go @@ -0,0 +1,242 @@ +package plugin + +import ( + "context" + "runtime" + "strings" + "testing" +) + +// Every plugin here is a POSIX shell script, so there is nothing to run these +// against on Windows. +func skipWithoutShell(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("these plugins are POSIX shell scripts") + } +} + +func TestGateHonoursARefusalThatExitsNonZero(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // Refusing and then exiting non-zero is an ordinary shell idiom. + writePlugin(t, root, "guard", ` +name: guard +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '{\"deny\":true,\"reason\":\"policy\"}'\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("a refusal was thrown away because the plugin exited non-zero: %+v", reply) + } + if reply.Reason != "policy" { + t.Fatalf("reason = %q, want the plugin's own words", reply.Reason) + } +} + +func TestGateHonoursAnEmptyReplyFromAFailingGate(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // "{}" is a plugin saying it has no objection. Falling over after saying + // it is a bug in the plugin, not a change of mind — which is the whole + // difference between answering and never answering. + writePlugin(t, root, "shrugger", ` +name: shrugger +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '{}'\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if reply.Deny { + t.Fatalf("a plugin that answered before it fell over was read as a refusal: %+v", reply) + } +} + +func TestGateDeniesWhenAPluginTimesOut(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // exec so the timeout kills the sleep itself rather than leaving it + // holding the pipe open long after the shell is gone. + writePlugin(t, root, "slow", ` +name: slow +command: ./run.sh +hooks: [pre_tool_call] +timeout_ms: 200 +`, "#!/bin/sh\nexec sleep 5\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("a plugin that never answered was read as permission: %+v", reply) + } + if !strings.Contains(reply.Reason, "slow") { + t.Fatalf("reason = %q, want it to name the plugin that failed", reply.Reason) + } + if !strings.Contains(reply.Reason, "timed out") { + t.Fatalf("reason = %q, want it to say what went wrong", reply.Reason) + } +} + +func TestGateDeniesWhenAPluginCannotBeRun(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // The manifest is well formed, so the plugin loads and is asked; only + // starting it fails. + writePlugin(t, root, "missing", ` +name: missing +command: ./not-here.sh +hooks: [pre_tool_call] +`, "") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("a gate that could not be started was read as permission: %+v", reply) + } + if !strings.Contains(reply.Reason, "missing") { + t.Fatalf("reason = %q, want it to name the plugin that failed", reply.Reason) + } +} + +func TestGateDeniesWhenAPluginAnswersWithGibberish(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // Exits cleanly, but nothing it said can be read as a verdict. + writePlugin(t, root, "babbler", ` +name: babbler +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho 'allow, I guess?'\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("an answer nobody can read was taken for a yes: %+v", reply) + } + if !strings.Contains(reply.Reason, "babbler") { + t.Fatalf("reason = %q, want it to name the plugin that failed", reply.Reason) + } +} + +func TestGateLetsACleanReplyThrough(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "watcher", ` +name: watcher +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '{\"notice\":\"looks fine\"}'\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if reply.Deny { + t.Fatalf("a plugin that answered and succeeded was treated as a refusal: %+v", reply) + } + if reply.Notice != "looks fine" { + t.Fatalf("notice = %q", reply.Notice) + } +} + +func TestGateTreatsAQuietSuccessAsConsent(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // Saying nothing and exiting cleanly is how a plugin declines to have an + // opinion. Only failure closes the gate, not silence. + writePlugin(t, root, "quiet", ` +name: quiet +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if reply.Deny { + t.Fatalf("a plugin with no opinion refused the call: %+v", reply) + } +} + +func TestGateOnlyClosesForPreToolCall(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "crasher", ` +name: crasher +command: ./run.sh +hooks: [post_tool_call] +`, "#!/bin/sh\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + // A result is already in hand; a broken observer has nothing to refuse. + reply := m.Dispatch(context.Background(), Payload{Event: PostToolCall, Tool: "terminal", Result: "done"}) + if reply.Deny { + t.Fatalf("a failing post_tool_call plugin denied something: %+v", reply) + } +} + +func TestGateHonoursAReplyFromAFailingObserver(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // Every event reads stdout, not just the gate: what the plugin said + // stands on its own whatever the exit status was. + writePlugin(t, root, "rewriter", ` +name: rewriter +command: ./run.sh +hooks: [post_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '{\"result\":\"rewritten\"}'\nexit 3\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PostToolCall, Tool: "terminal", Result: "original"}) + if reply.Result != "rewritten" { + t.Fatalf("result = %q, want the reply the plugin printed before it exited badly", reply.Result) + } +} + +func TestGateDoesNotDenyForAPluginThatNeverRan(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + // Declares the gate but cannot be loaded, so it is never invoked. + writePlugin(t, root, "unloadable", "name: unloadable\nhooks: [pre_tool_call]\n", "") + // Loads, but wants a different event. + writePlugin(t, root, "elsewhere", ` +name: elsewhere +command: ./run.sh +hooks: [session_start] +`, "#!/bin/sh\nexit 1\n") + // Loads and wants the gate, but the operator turned it off. + writePlugin(t, root, "switched-off", ` +name: switched-off +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + if !m.SetEnabled("switched-off", false) { + t.Fatal("could not disable a plugin that exists") + } + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if reply.Deny { + t.Fatalf("a plugin that was never invoked still refused the call: %+v", reply) + } +} diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go index 4673c31..11f055a 100644 --- a/internal/plugin/plugin.go +++ b/internal/plugin/plugin.go @@ -232,6 +232,10 @@ func (m *Manager) SetEnabled(name string, enabled bool) bool { // Plugins run in order, each seeing the previous one's changes. A deny from // any of them ends it: a refusal is not something a later plugin should be // able to quietly undo. +// +// A plugin that fails is still heard out — whatever it managed to print stands +// on its own. When it left nothing readable behind, PreToolCall refuses the +// call and every other event carries on without it. func (m *Manager) Dispatch(ctx context.Context, p Payload) Reply { m.mu.RLock() plugins := make([]Manifest, len(m.plugins)) @@ -243,11 +247,23 @@ func (m *Manager) Dispatch(ctx context.Context, p Payload) Reply { if man.Error != "" || !man.Enabled || !wants(man, p.Event) { continue } - reply, err := call(ctx, man, p) + reply, replied, err := call(ctx, man, p) if err != nil { - // A broken plugin must not break the agent. Log it and carry on. slog.Warn("plugin failed", "plugin", man.Name, "event", p.Event, "error", err) - continue + if !replied { + if p.Event == PreToolCall { + // The gate is the one event whose answer is a decision, + // and none of this is an answer. A guard that could not + // be asked has not agreed to anything. + return Reply{ + Deny: true, + Reason: fmt.Sprintf("%s could not answer: %v", man.Name, err), + } + } + // Every other event is watching something already decided, + // so a broken plugin must not break the agent. + continue + } } if reply.Deny { reply.Reason = strings.TrimSpace(reply.Reason) @@ -283,10 +299,12 @@ func wants(man Manifest, e Event) bool { return false } -// call runs one plugin once. -func call(ctx context.Context, man Manifest, p Payload) (Reply, error) { - var reply Reply - +// call runs one plugin once. It reports what the plugin said, whether it said +// anything readable at all, and how it failed if it did — three separate +// facts, because a plugin can print a verdict and still exit badly, and a +// caller that has to decide something needs to tell that apart from a plugin +// that never got a word out. +func call(ctx context.Context, man Manifest, p Payload) (reply Reply, replied bool, err error) { timeout := time.Duration(man.TimeoutMS) * time.Millisecond if timeout <= 0 { timeout = 5 * time.Second @@ -303,7 +321,7 @@ func call(ctx context.Context, man Manifest, p Payload) (Reply, error) { payload, err := json.Marshal(p) if err != nil { - return reply, err + return Reply{}, false, err } cmd := exec.CommandContext(runCtx, command, man.Args...) @@ -318,26 +336,49 @@ func call(ctx context.Context, man Manifest, p Payload) (Reply, error) { cmd.Stdout = &stdout cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { - if runCtx.Err() != nil { - return reply, fmt.Errorf("timed out after %s", timeout) - } - msg := strings.TrimSpace(stderr.String()) - if msg == "" { - msg = err.Error() - } - return reply, errors.New(msg) + runErr := cmd.Run() + + // Read the plugin out before judging how it exited. Printing an answer and + // then exiting non-zero is an ordinary shell accident, and the answer is + // no less considered for it. + reply, replied, parseErr := readReply(stdout.Bytes()) + if runErr != nil { + return reply, replied, runFailure(runCtx, runErr, stderr.String(), timeout) } + return reply, replied, parseErr +} - out := bytes.TrimSpace(stdout.Bytes()) +// failureChars bounds an excerpt of what a plugin printed when quoting it back. +const failureChars = 200 + +// readReply decodes what a plugin printed. Silence is not a failure: it is how +// a plugin says it has no opinion. +func readReply(stdout []byte) (reply Reply, replied bool, err error) { + out := bytes.TrimSpace(stdout) if len(out) == 0 { - // Silence is a valid answer: the plugin observed and had no opinion. - return reply, nil + return Reply{}, false, nil } if err := json.Unmarshal(out, &reply); err != nil { - return reply, fmt.Errorf("returned something that is not JSON: %s", truncate(string(out), 200)) + return Reply{}, false, fmt.Errorf("returned something that is not JSON: %s", + truncate(string(out), failureChars)) + } + return reply, true, nil +} + +// runFailure says why a run failed, preferring the plugin's own words on +// stderr. The text reaches an operator's log and, for a refused tool call, the +// model, so it is kept to a length both can carry. +func runFailure(runCtx context.Context, runErr error, stderr string, timeout time.Duration) error { + switch { + case errors.Is(runCtx.Err(), context.DeadlineExceeded): + return fmt.Errorf("timed out after %s", timeout) + case runCtx.Err() != nil: + return fmt.Errorf("was stopped: %w", runCtx.Err()) + } + if msg := strings.TrimSpace(stderr); msg != "" { + return errors.New(truncate(msg, failureChars)) } - return reply, nil + return runErr } func truncate(s string, max int) string { From 7fb043f593899e7173e3a2a1a54e8d22912c0333 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 20:48:44 +0700 Subject: [PATCH 14/28] Read what an MCP server actually returned Co-authored-by: Cursor --- internal/mcp/client.go | 122 +++++++++++++++++++++----- internal/mcp/client_test.go | 26 ++++++ internal/mcp/content_test.go | 163 +++++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 23 deletions(-) create mode 100644 internal/mcp/content_test.go diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 0cfaba6..bfca057 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -37,6 +37,88 @@ type content struct { // Non-text content is summarised rather than inlined. MimeType string `json:"mimeType,omitempty"` Data string `json:"data,omitempty"` + // Resource holds an embedded resource's own payload. Filesystem, git and + // docs servers answer with these rather than with plain text. + Resource *resourceContents `json:"resource,omitempty"` +} + +// resourceContents is a resource's payload: inline text or base64 bytes. Both +// an embedded resource in a tool result and a resources/read reply use it. +type resourceContents struct { + URI string `json:"uri"` + MimeType string `json:"mimeType"` + Text string `json:"text"` + Blob string `json:"blob"` +} + +// describe names a resource for a summary line. +func (r resourceContents) describe() string { + uri := r.URI + if uri == "" { + uri = "no uri" + } + mime := r.MimeType + if mime == "" { + mime = "unknown type" + } + return fmt.Sprintf("%s (%s)", uri, mime) +} + +// render returns the text this resource contributes, or "" when it carries no +// payload. Bytes are named rather than inlined: the model cannot use base64 and +// it would crowd out the rest of the context. +func (r resourceContents) render() string { + switch { + case r.Text != "": + return r.Text + case r.Blob != "": + return fmt.Sprintf("[resource: %s, %d bytes base64]", r.describe(), len(r.Blob)) + default: + return "" + } +} + +// flattenContent renders a tool result to text and reports the kinds of content +// it could not represent. The two are kept apart because "the server said +// nothing" and "the server said something this client cannot read" call for +// different answers to the model. +func flattenContent(items []content) (string, []string) { + var b strings.Builder + var skipped []string + skip := func(kind string) { + for _, seen := range skipped { + if seen == kind { + return + } + } + skipped = append(skipped, kind) + } + + for _, item := range items { + switch item.Type { + case "text": + b.WriteString(item.Text) + b.WriteString("\n") + case "image": + fmt.Fprintf(&b, "[image: %s, %d bytes base64]\n", item.MimeType, len(item.Data)) + case "resource": + var line string + if item.Resource != nil { + line = item.Resource.render() + } + if line == "" { + skip("resource with no text or blob") + continue + } + b.WriteString(line) + b.WriteString("\n") + case "": + skip("item with no type") + default: + skip(item.Type) + } + } + return strings.TrimSpace(b.String()), skipped } // CallResult is the outcome of calling an MCP tool. @@ -253,20 +335,15 @@ func (c *Client) Call(ctx context.Context, tool string, args map[string]any) (*C return nil, fmt.Errorf("decode tool result: %w", err) } - var b strings.Builder - for _, item := range out.Content { - switch item.Type { - case "text": - b.WriteString(item.Text) - b.WriteString("\n") - case "image": - fmt.Fprintf(&b, "[image: %s, %d bytes base64]\n", item.MimeType, len(item.Data)) - case "resource": - fmt.Fprintf(&b, "[resource: %s]\n", item.MimeType) - } - } - text := strings.TrimSpace(b.String()) + text, skipped := flattenContent(out.Content) if text == "" { + // Nothing renderable came back. If the server did send something, say + // what it was: reporting it as an empty success would tell the model the + // tool ran and had nothing to report, so it proceeds instead of retrying. + if len(skipped) > 0 { + return nil, fmt.Errorf("tool %q returned only content this client cannot represent: %s", + tool, strings.Join(skipped, ", ")) + } text = "(no content returned)" } return &CallResult{Text: text, IsError: out.IsError}, nil @@ -318,23 +395,22 @@ func (c *Client) ReadResource(ctx context.Context, uri string) (string, error) { return "", resp.Error } var out struct { - Contents []struct { - URI string `json:"uri"` - MimeType string `json:"mimeType"` - Text string `json:"text"` - Blob string `json:"blob"` - } `json:"contents"` + Contents []resourceContents `json:"contents"` } if err := json.Unmarshal(resp.Result, &out); err != nil { return "", err } + // No contents at all is this server saying it does not hold the resource. + // Manager.ReadResource asks one server after another, so this has to be an + // error for the search to continue past the first server that lacks it. + if len(out.Contents) == 0 { + return "", fmt.Errorf("server %q has no resource with uri %q", c.name, uri) + } var b strings.Builder for _, part := range out.Contents { - if part.Text != "" { - b.WriteString(part.Text) + if line := part.render(); line != "" { + b.WriteString(line) b.WriteString("\n") - } else if part.Blob != "" { - fmt.Fprintf(&b, "[binary resource: %s, %d bytes base64]\n", part.MimeType, len(part.Blob)) } } text := strings.TrimSpace(b.String()) diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go index b3f9f1f..2eef4b5 100644 --- a/internal/mcp/client_test.go +++ b/internal/mcp/client_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "os/exec" + "strings" "testing" "time" @@ -262,6 +263,13 @@ func TestHelperServer(t *testing.T) { // transport self-closes. continue } + // "raw" replies with the result frame the caller supplied, so a test + // can drive Call with any content shape the protocol allows. + if p.Name == "raw" { + result, _ := p.Arguments["result"].(string) + reply(req.ID, json.RawMessage(result)) + continue + } if p.Name != "echo" { reply(req.ID, map[string]any{ "isError": true, @@ -273,6 +281,24 @@ func TestHelperServer(t *testing.T) { reply(req.ID, map[string]any{ "content": []map[string]any{{"type": "text", "text": "echo: " + text}}, }) + case "resources/read": + var p struct { + URI string `json:"uri"` + } + _ = json.Unmarshal(req.Params, &p) + // A "raw:" uri carries the result frame to answer with, so a test + // can drive ReadResource with any contents shape. + if result, ok := strings.CutPrefix(p.URI, "raw:"); ok { + reply(req.ID, json.RawMessage(result)) + continue + } + out := map[string]any{ + "jsonrpc": "2.0", + "id": req.ID, + "error": map[string]any{"code": -32002, "message": "no such resource " + p.URI}, + } + b, _ := json.Marshal(out) + os.Stdout.Write(append(b, '\n')) } } } diff --git a/internal/mcp/content_test.go b/internal/mcp/content_test.go new file mode 100644 index 0000000..930ed06 --- /dev/null +++ b/internal/mcp/content_test.go @@ -0,0 +1,163 @@ +package mcp + +import ( + "context" + "os" + "strings" + "testing" + "time" +) + +// helperClient connects to the in-process stdio fixture (see TestHelperServer). +func helperClient(t *testing.T) *Client { + t.Helper() + client, err := Connect(context.Background(), "fake", ServerConfig{ + Transport: "stdio", + Command: os.Args[0], + Args: []string{"-test.run=TestHelperServer"}, + Env: map[string]string{"ANTARES_MCP_HELPER": "1"}, + }) + if err != nil { + t.Fatalf("connect: %v", err) + } + t.Cleanup(func() { _ = client.Close() }) + return client +} + +// TestCallContent drives Call over the tool result shapes a server may legally +// return. A result the client cannot represent must say so rather than pass for +// an empty success: the model would otherwise proceed believing the tool ran and +// had nothing to report. +func TestCallContent(t *testing.T) { + client := helperClient(t) + + cases := []struct { + name string + raw string + // wantErr is a substring the error must name; empty means no error. + wantErr string + wantText string + }{ + { + name: "text", + raw: `{"content":[{"type":"text","text":"PLAIN"}]}`, + wantText: "PLAIN", + }, + { + name: "embedded resource text", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a","mimeType":"text/plain","text":"HELLO"}}]}`, + wantText: "HELLO", + }, + { + name: "embedded resource blob", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a.png","mimeType":"image/png","blob":"QUJDRA=="}}]}`, + wantText: "[resource: file:///a.png (image/png), 8 bytes base64]", + }, + { + name: "image", + raw: `{"content":[{"type":"image","mimeType":"image/png","data":"QUJDRA=="}]}`, + wantText: "[image: image/png, 8 bytes base64]", + }, + { + name: "audio only", + raw: `{"content":[{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, + wantErr: "audio", + }, + { + name: "unknown type only", + raw: `{"content":[{"type":"hologram","data":"QUJDRA=="}]}`, + wantErr: "hologram", + }, + { + name: "resource without a payload", + raw: `{"content":[{"type":"resource"}]}`, + wantErr: "resource", + }, + { + name: "text alongside audio", + raw: `{"content":[{"type":"text","text":"HELLO"},{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, + wantText: "HELLO", + }, + { + name: "empty content", + raw: `{"content":[]}`, + wantText: "(no content returned)", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + res, err := client.Call(ctx, "raw", map[string]any{"result": tc.raw}) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("Call returned %+v with no error, want one naming %q", res, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %v, want it to name %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("call: %v", err) + } + if res.Text != tc.wantText { + t.Fatalf("text = %q, want %q", res.Text, tc.wantText) + } + if res.IsError { + t.Fatalf("IsError = true for %q, want false", res.Text) + } + }) + } +} + +// TestCallErrorResultKeepsItsText checks that a server-flagged error still comes +// back as a result rather than a transport error, since its content is the +// explanation the model needs. +func TestCallErrorResultKeepsItsText(t *testing.T) { + client := helperClient(t) + res, err := client.Call(context.Background(), "raw", + map[string]any{"result": `{"isError":true,"content":[{"type":"text","text":"file not found"}]}`}) + if err != nil { + t.Fatalf("call: %v", err) + } + if !res.IsError || res.Text != "file not found" { + t.Fatalf("result = %+v, want the error text carried through", res) + } +} + +// TestReadResourceEmptyContentsIsNotFound pins that a server answering with no +// contents is a miss, not an empty document: Manager.ReadResource asks every +// server in turn and must keep looking rather than accept the first empty reply. +func TestReadResourceEmptyContentsIsNotFound(t *testing.T) { + client := helperClient(t) + + text, err := client.ReadResource(context.Background(), `raw:{"contents":[]}`) + if err == nil { + t.Fatalf("ReadResource returned %q with no error, want a not-found error", text) + } + + text, err = client.ReadResource(context.Background(), + `raw:{"contents":[{"uri":"mem:///a","mimeType":"text/plain","text":"BODY"}]}`) + if err != nil { + t.Fatalf("read: %v", err) + } + if text != "BODY" { + t.Fatalf("text = %q, want %q", text, "BODY") + } +} + +// TestManagerReadResourceRejectsEmptyAnswer is the same rule seen from the +// manager: with one server that has nothing, the read fails instead of handing +// the model a blank document. +func TestManagerReadResourceRejectsEmptyAnswer(t *testing.T) { + manager := NewManager() + manager.Connect(context.Background(), helperConfig("online")) + defer manager.Close() + + text, err := manager.ReadResource(context.Background(), `raw:{"contents":[]}`) + if err == nil { + t.Fatalf("ReadResource returned %q with no error, want the search to fail", text) + } +} From af66520a54dc6fff57f8a397f33ce913488523b7 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 20:59:34 +0700 Subject: [PATCH 15/28] Tell an empty file apart from content it cannot read Co-authored-by: Cursor --- internal/mcp/client.go | 96 +++++++++++++++------ internal/mcp/client_test.go | 36 +++++--- internal/mcp/content_test.go | 160 ++++++++++++++++++++++++++++++++--- 3 files changed, 241 insertions(+), 51 deletions(-) diff --git a/internal/mcp/client.go b/internal/mcp/client.go index bfca057..ecd17ca 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -17,6 +17,7 @@ import ( "sync" "time" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/version" ) @@ -44,52 +45,83 @@ type content struct { // resourceContents is a resource's payload: inline text or base64 bytes. Both // an embedded resource in a tool result and a resources/read reply use it. +// +// Text and Blob are pointers because an empty file is a legal payload. A server +// answering with "text": "" has represented an empty document exactly; only a +// resource carrying neither key is one this client cannot read. type resourceContents struct { - URI string `json:"uri"` - MimeType string `json:"mimeType"` - Text string `json:"text"` - Blob string `json:"blob"` + URI string `json:"uri"` + MimeType string `json:"mimeType"` + Text *string `json:"text"` + Blob *string `json:"blob"` } +// hasPayload reports whether the server sent a payload at all, empty or not. +func (r resourceContents) hasPayload() bool { return r.Text != nil || r.Blob != nil } + // describe names a resource for a summary line. func (r resourceContents) describe() string { uri := r.URI if uri == "" { uri = "no uri" } - mime := r.MimeType - if mime == "" { - mime = "unknown type" - } - return fmt.Sprintf("%s (%s)", uri, mime) + return fmt.Sprintf("%s (%s)", uri, mimeOrUnknown(r.MimeType)) } -// render returns the text this resource contributes, or "" when it carries no -// payload. Bytes are named rather than inlined: the model cannot use base64 and -// it would crowd out the rest of the context. +// render returns the text this resource contributes, which is empty for an +// empty file. Bytes are named rather than inlined: the model cannot use base64 +// and it would crowd out the rest of the context. func (r resourceContents) render() string { switch { - case r.Text != "": - return r.Text - case r.Blob != "": - return fmt.Sprintf("[resource: %s, %d bytes base64]", r.describe(), len(r.Blob)) + case r.Text != nil: + return *r.Text + case r.Blob != nil && *r.Blob != "": + return fmt.Sprintf("[resource: %s, %d bytes base64]", r.describe(), len(*r.Blob)) default: return "" } } +// mimeOrUnknown names a media type for a summary line. Leaving it out would +// read as though the server had stated one. +func mimeOrUnknown(mime string) string { + if mime == "" { + return "unknown type" + } + return mime +} + +// contentKindChars bounds one content type named back in an error message, and +// maxContentKinds bounds how many are named. Both come from the server. +const ( + contentKindChars = 40 + maxContentKinds = 5 +) + // flattenContent renders a tool result to text and reports the kinds of content // it could not represent. The two are kept apart because "the server said // nothing" and "the server said something this client cannot read" call for // different answers to the model. +// +// Text, images and embedded resources are the three kinds this client has +// rendering code for. Everything else is named back to the caller rather than +// dropped. An item of a kind it does understand but that carries nothing — +// an empty file, an empty string — is understood and simply has nothing to +// show, which is not the same as unreadable. func flattenContent(items []content) (string, []string) { var b strings.Builder var skipped []string + unnamed := 0 + seen := map[string]bool{} skip := func(kind string) { - for _, seen := range skipped { - if seen == kind { - return - } + kind = textutil.TruncateRunes(kind, contentKindChars) + if seen[kind] { + return + } + seen[kind] = true + if len(skipped) >= maxContentKinds { + unnamed++ + return } skipped = append(skipped, kind) } @@ -100,24 +132,32 @@ func flattenContent(items []content) (string, []string) { b.WriteString(item.Text) b.WriteString("\n") case "image": - fmt.Fprintf(&b, "[image: %s, %d bytes base64]\n", item.MimeType, len(item.Data)) - case "resource": - var line string - if item.Resource != nil { - line = item.Resource.render() + // No bytes is not an empty image, it is not an image at all: a + // zero-byte summary line would be this client's assertion rather + // than the server's. + if item.Data == "" { + skip("image with no data") + continue } - if line == "" { + fmt.Fprintf(&b, "[image: %s, %d bytes base64]\n", mimeOrUnknown(item.MimeType), len(item.Data)) + case "resource": + if item.Resource == nil || !item.Resource.hasPayload() { skip("resource with no text or blob") continue } - b.WriteString(line) - b.WriteString("\n") + if line := item.Resource.render(); line != "" { + b.WriteString(line) + b.WriteString("\n") + } case "": skip("item with no type") default: skip(item.Type) } } + if unnamed > 0 { + skipped = append(skipped, fmt.Sprintf("and %d more", unnamed)) + } return strings.TrimSpace(b.String()), skipped } diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go index 2eef4b5..362d712 100644 --- a/internal/mcp/client_test.go +++ b/internal/mcp/client_test.go @@ -169,21 +169,26 @@ func TestRefreshReplacesToolsAndReadiness(t *testing.T) { func helperConfig(mode string) *config.Config { return &config.Config{MCP: config.MCP{ Enabled: true, - Servers: map[string]config.MCPServer{ - "fake": { - Transport: "stdio", - Command: os.Args[0], - Args: []string{"-test.run=TestHelperServer"}, - Env: map[string]string{ - "ANTARES_MCP_HELPER": "1", - "ANTARES_MCP_HELPER_MODE": mode, - }, - Enabled: true, - }, - }, + Servers: map[string]config.MCPServer{"fake": helperServer(mode)}, }} } +// helperServer configures one instance of the fixture. mode picks how it +// behaves: "offline" fails tools/list, "no-resources" holds no resources, and +// anything else is a healthy server. +func helperServer(mode string) config.MCPServer { + return config.MCPServer{ + Transport: "stdio", + Command: os.Args[0], + Args: []string{"-test.run=TestHelperServer"}, + Env: map[string]string{ + "ANTARES_MCP_HELPER": "1", + "ANTARES_MCP_HELPER_MODE": mode, + }, + Enabled: true, + } +} + func TestUnknownTransport(t *testing.T) { if _, err := Connect(context.Background(), "x", ServerConfig{Transport: "carrier-pigeon"}); err == nil { t.Fatal("expected an unknown transport to fail") @@ -286,6 +291,13 @@ func TestHelperServer(t *testing.T) { URI string `json:"uri"` } _ = json.Unmarshal(req.Params, &p) + // A server in "no-resources" mode holds nothing, whatever it is + // asked for, so a test can watch a search cross it to reach a + // server that does answer. + if os.Getenv("ANTARES_MCP_HELPER_MODE") == "no-resources" { + reply(req.ID, map[string]any{"contents": []any{}}) + continue + } // A "raw:" uri carries the result frame to answer with, so a test // can drive ReadResource with any contents shape. if result, ok := strings.CutPrefix(p.URI, "raw:"); ok { diff --git a/internal/mcp/content_test.go b/internal/mcp/content_test.go index 930ed06..6fdc9ac 100644 --- a/internal/mcp/content_test.go +++ b/internal/mcp/content_test.go @@ -2,10 +2,13 @@ package mcp import ( "context" + "fmt" "os" "strings" "testing" "time" + + "github.com/enowdev/antares/internal/config" ) // helperClient connects to the in-process stdio fixture (see TestHelperServer). @@ -27,14 +30,15 @@ func helperClient(t *testing.T) *Client { // TestCallContent drives Call over the tool result shapes a server may legally // return. A result the client cannot represent must say so rather than pass for // an empty success: the model would otherwise proceed believing the tool ran and -// had nothing to report. +// had nothing to report. A result it can represent that happens to be empty — +// an empty file is the everyday one — is not that, and must not be an error. func TestCallContent(t *testing.T) { client := helperClient(t) cases := []struct { name string raw string - // wantErr is a substring the error must name; empty means no error. + // wantErr is a substring the error must contain; empty means no error. wantErr string wantText string }{ @@ -48,36 +52,66 @@ func TestCallContent(t *testing.T) { raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a","mimeType":"text/plain","text":"HELLO"}}]}`, wantText: "HELLO", }, + { + name: "embedded resource holding an empty file", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///empty","mimeType":"text/plain","text":""}}]}`, + wantText: "(no content returned)", + }, { name: "embedded resource blob", raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a.png","mimeType":"image/png","blob":"QUJDRA=="}}]}`, wantText: "[resource: file:///a.png (image/png), 8 bytes base64]", }, + { + name: "embedded resource holding an empty binary file", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///empty.bin","mimeType":"application/octet-stream","blob":""}}]}`, + wantText: "(no content returned)", + }, { name: "image", raw: `{"content":[{"type":"image","mimeType":"image/png","data":"QUJDRA=="}]}`, wantText: "[image: image/png, 8 bytes base64]", }, + { + name: "image with no data", + raw: `{"content":[{"type":"image","mimeType":"image/png"}]}`, + wantErr: "cannot represent: image with no data", + }, { name: "audio only", raw: `{"content":[{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, - wantErr: "audio", + wantErr: "cannot represent: audio", }, { name: "unknown type only", raw: `{"content":[{"type":"hologram","data":"QUJDRA=="}]}`, - wantErr: "hologram", + wantErr: "cannot represent: hologram", }, { - name: "resource without a payload", + name: "resource with no payload key", raw: `{"content":[{"type":"resource"}]}`, - wantErr: "resource", + wantErr: "cannot represent: resource with no text or blob", + }, + { + name: "resource with an empty payload object", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a"}}]}`, + wantErr: "cannot represent: resource with no text or blob", + }, + { + name: "a repeated kind is named once", + raw: `{"content":[{"type":"audio","data":"a"},{"type":"hologram"},{"type":"audio","data":"b"}]}`, + wantErr: "cannot represent: audio, hologram", }, { name: "text alongside audio", raw: `{"content":[{"type":"text","text":"HELLO"},{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, wantText: "HELLO", }, + { + name: "text alongside an empty resource", + raw: `{"content":[{"type":"text","text":"HELLO"},{"type":"resource","resource":{"uri":"file:///empty","text":""}}]}`, + wantText: "HELLO", + }, { name: "empty content", raw: `{"content":[]}`, @@ -92,10 +126,10 @@ func TestCallContent(t *testing.T) { res, err := client.Call(ctx, "raw", map[string]any{"result": tc.raw}) if tc.wantErr != "" { if err == nil { - t.Fatalf("Call returned %+v with no error, want one naming %q", res, tc.wantErr) + t.Fatalf("Call returned %+v with no error, want one saying %q", res, tc.wantErr) } if !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("error = %v, want it to name %q", err, tc.wantErr) + t.Fatalf("error = %v, want it to say %q", err, tc.wantErr) } return } @@ -112,6 +146,35 @@ func TestCallContent(t *testing.T) { } } +// TestCallErrorBoundsWhatTheServerNamed keeps a server from writing the error +// message: the kinds it names are the server's own strings, so each is cut to +// contentKindChars and no more than maxContentKinds of them are listed. +func TestCallErrorBoundsWhatTheServerNamed(t *testing.T) { + client := helperClient(t) + + items := []string{fmt.Sprintf(`{"type":%q}`, strings.Repeat("z", 5000))} + for i := 0; i < 8; i++ { + items = append(items, fmt.Sprintf(`{"type":"kind-%d"}`, i)) + } + + _, err := client.Call(context.Background(), "raw", + map[string]any{"result": `{"content":[` + strings.Join(items, ",") + `]}`}) + if err == nil { + t.Fatal("expected an error naming the content this client cannot represent") + } + msg := err.Error() + if strings.Contains(msg, strings.Repeat("z", contentKindChars+1)) { + t.Fatalf("a server-supplied type reached the error untruncated: %s", msg) + } + // Nine distinct kinds arrived and five are named. + if !strings.Contains(msg, "and 4 more") { + t.Fatalf("error = %s, want it to count the kinds it did not name", msg) + } + if len(msg) > 400 { + t.Fatalf("error is %d bytes, want a bounded message: %s", len(msg), msg) + } +} + // TestCallErrorResultKeepsItsText checks that a server-flagged error still comes // back as a result rather than a transport error, since its content is the // explanation the model needs. @@ -148,16 +211,91 @@ func TestReadResourceEmptyContentsIsNotFound(t *testing.T) { } } -// TestManagerReadResourceRejectsEmptyAnswer is the same rule seen from the +// TestReadResourcePayloads is the other half of that rule: a resource that is +// present but empty reads as an empty document, never as a miss and never as an +// error. +func TestReadResourcePayloads(t *testing.T) { + client := helperClient(t) + + cases := []struct { + name string + contents string + wantText string + }{ + { + name: "text", + contents: `{"uri":"mem:///a","mimeType":"text/plain","text":"BODY"}`, + wantText: "BODY", + }, + { + name: "an empty file", + contents: `{"uri":"mem:///a","mimeType":"text/plain","text":""}`, + wantText: "(resource is empty)", + }, + { + name: "blob", + contents: `{"uri":"mem:///a.bin","mimeType":"application/octet-stream","blob":"QUJDRA=="}`, + wantText: "[resource: mem:///a.bin (application/octet-stream), 8 bytes base64]", + }, + { + name: "an empty binary file", + contents: `{"uri":"mem:///a.bin","mimeType":"application/octet-stream","blob":""}`, + wantText: "(resource is empty)", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + text, err := client.ReadResource(context.Background(), `raw:{"contents":[`+tc.contents+`]}`) + if err != nil { + t.Fatalf("read: %v", err) + } + if text != tc.wantText { + t.Fatalf("text = %q, want %q", text, tc.wantText) + } + }) + } +} + +// TestManagerReadResourceRejectsEmptyAnswer is the client rule seen from the // manager: with one server that has nothing, the read fails instead of handing // the model a blank document. func TestManagerReadResourceRejectsEmptyAnswer(t *testing.T) { manager := NewManager() - manager.Connect(context.Background(), helperConfig("online")) + manager.Connect(context.Background(), helperConfig("no-resources")) defer manager.Close() - text, err := manager.ReadResource(context.Background(), `raw:{"contents":[]}`) + text, err := manager.ReadResource(context.Background(), + `raw:{"contents":[{"uri":"mem:///a","text":"BODY"}]}`) if err == nil { t.Fatalf("ReadResource returned %q with no error, want the search to fail", text) } } + +// TestManagerReadResourceKeepsSearchingPastAnEmptyServer is what that rule is +// for. The manager walks its servers in map order, so the answer must not depend +// on which one it happens to ask first. The repetition is what makes the +// regression visible: an implementation that takes the first empty reply as the +// answer passes a single attempt about half the time. +func TestManagerReadResourceKeepsSearchingPastAnEmptyServer(t *testing.T) { + manager := NewManager() + manager.Connect(context.Background(), &config.Config{MCP: config.MCP{ + Enabled: true, + Servers: map[string]config.MCPServer{ + "empty": helperServer("no-resources"), + "full": helperServer("online"), + }, + }}) + defer manager.Close() + + const uri = `raw:{"contents":[{"uri":"mem:///a","mimeType":"text/plain","text":"BODY"}]}` + for i := 0; i < 8; i++ { + text, err := manager.ReadResource(context.Background(), uri) + if err != nil { + t.Fatalf("attempt %d: %v", i, err) + } + if text != "BODY" { + t.Fatalf("attempt %d: text = %q, want %q", i, text, "BODY") + } + } +} From edbe221820199b41789d98041d4ef80dfe8c224e Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 21:20:32 +0700 Subject: [PATCH 16/28] Let a blob be read when the text beside it is empty Co-authored-by: Cursor --- internal/mcp/client.go | 8 ++-- internal/mcp/content_test.go | 72 +++++++++++++++++++++++++----------- 2 files changed, 55 insertions(+), 25 deletions(-) diff --git a/internal/mcp/client.go b/internal/mcp/client.go index ecd17ca..97fdf39 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -69,11 +69,13 @@ func (r resourceContents) describe() string { } // render returns the text this resource contributes, which is empty for an -// empty file. Bytes are named rather than inlined: the model cannot use base64 -// and it would crowd out the rest of the context. +// empty file. Text wins over bytes only when it has something in it: a server +// marshalling both keys sends "text": "" with every binary resource, and an +// empty string must not hide the bytes sent beside it. Bytes are named rather +// than inlined: the model cannot use base64 and it would crowd out the context. func (r resourceContents) render() string { switch { - case r.Text != nil: + case r.Text != nil && *r.Text != "": return *r.Text case r.Blob != nil && *r.Blob != "": return fmt.Sprintf("[resource: %s, %d bytes base64]", r.describe(), len(*r.Blob)) diff --git a/internal/mcp/content_test.go b/internal/mcp/content_test.go index 6fdc9ac..802d519 100644 --- a/internal/mcp/content_test.go +++ b/internal/mcp/content_test.go @@ -27,6 +27,12 @@ func helperClient(t *testing.T) *Client { return client } +// cannotRepresent is the whole error Call returns for a result it could not +// render, so a table row pins the message instead of a fragment of it. +func cannotRepresent(kinds string) string { + return `tool "raw" returned only content this client cannot represent: ` + kinds +} + // TestCallContent drives Call over the tool result shapes a server may legally // return. A result the client cannot represent must say so rather than pass for // an empty success: the model would otherwise proceed believing the tool ran and @@ -38,7 +44,8 @@ func TestCallContent(t *testing.T) { cases := []struct { name string raw string - // wantErr is a substring the error must contain; empty means no error. + // wantErr is the whole error, not a fragment of it: a substring here + // cannot tell a deduplicated list of kinds from a repeated one. wantErr string wantText string }{ @@ -62,6 +69,18 @@ func TestCallContent(t *testing.T) { raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a.png","mimeType":"image/png","blob":"QUJDRA=="}}]}`, wantText: "[resource: file:///a.png (image/png), 8 bytes base64]", }, + { + // A server marshalling a struct with both fields and no omitempty + // sends "text": "" with every binary resource. + name: "blob beside an empty text", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a.png","mimeType":"image/png","text":"","blob":"QUJDRA=="}}]}`, + wantText: "[resource: file:///a.png (image/png), 8 bytes base64]", + }, + { + name: "text beside a blob", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a.png","mimeType":"image/png","text":"HELLO","blob":"QUJDRA=="}}]}`, + wantText: "HELLO", + }, { name: "embedded resource holding an empty binary file", raw: `{"content":[{"type":"resource","resource":{"uri":"file:///empty.bin","mimeType":"application/octet-stream","blob":""}}]}`, @@ -75,32 +94,32 @@ func TestCallContent(t *testing.T) { { name: "image with no data", raw: `{"content":[{"type":"image","mimeType":"image/png"}]}`, - wantErr: "cannot represent: image with no data", + wantErr: cannotRepresent("image with no data"), }, { name: "audio only", raw: `{"content":[{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, - wantErr: "cannot represent: audio", + wantErr: cannotRepresent("audio"), }, { name: "unknown type only", raw: `{"content":[{"type":"hologram","data":"QUJDRA=="}]}`, - wantErr: "cannot represent: hologram", + wantErr: cannotRepresent("hologram"), }, { name: "resource with no payload key", raw: `{"content":[{"type":"resource"}]}`, - wantErr: "cannot represent: resource with no text or blob", + wantErr: cannotRepresent("resource with no text or blob"), }, { name: "resource with an empty payload object", raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a"}}]}`, - wantErr: "cannot represent: resource with no text or blob", + wantErr: cannotRepresent("resource with no text or blob"), }, { name: "a repeated kind is named once", raw: `{"content":[{"type":"audio","data":"a"},{"type":"hologram"},{"type":"audio","data":"b"}]}`, - wantErr: "cannot represent: audio, hologram", + wantErr: cannotRepresent("audio, hologram"), }, { name: "text alongside audio", @@ -126,10 +145,10 @@ func TestCallContent(t *testing.T) { res, err := client.Call(ctx, "raw", map[string]any{"result": tc.raw}) if tc.wantErr != "" { if err == nil { - t.Fatalf("Call returned %+v with no error, want one saying %q", res, tc.wantErr) + t.Fatalf("Call returned %+v with no error, want %q", res, tc.wantErr) } - if !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("error = %v, want it to say %q", err, tc.wantErr) + if err.Error() != tc.wantErr { + t.Fatalf("error = %q, want %q", err, tc.wantErr) } return } @@ -162,16 +181,12 @@ func TestCallErrorBoundsWhatTheServerNamed(t *testing.T) { if err == nil { t.Fatal("expected an error naming the content this client cannot represent") } - msg := err.Error() - if strings.Contains(msg, strings.Repeat("z", contentKindChars+1)) { - t.Fatalf("a server-supplied type reached the error untruncated: %s", msg) - } - // Nine distinct kinds arrived and five are named. - if !strings.Contains(msg, "and 4 more") { - t.Fatalf("error = %s, want it to count the kinds it did not name", msg) - } - if len(msg) > 400 { - t.Fatalf("error is %d bytes, want a bounded message: %s", len(msg), msg) + // Nine distinct kinds arrived: the first is cut to contentKindChars, four + // more are named, and the rest are counted rather than listed. + want := cannotRepresent(strings.Repeat("z", contentKindChars) + + ", kind-0, kind-1, kind-2, kind-3, and 4 more") + if err.Error() != want { + t.Fatalf("error = %q, want %q", err, want) } } @@ -242,6 +257,16 @@ func TestReadResourcePayloads(t *testing.T) { contents: `{"uri":"mem:///a.bin","mimeType":"application/octet-stream","blob":""}`, wantText: "(resource is empty)", }, + { + name: "blob beside an empty text", + contents: `{"uri":"mem:///a.bin","mimeType":"application/octet-stream","text":"","blob":"QUJDRA=="}`, + wantText: "[resource: mem:///a.bin (application/octet-stream), 8 bytes base64]", + }, + { + name: "text beside a blob", + contents: `{"uri":"mem:///a.bin","mimeType":"application/octet-stream","text":"BODY","blob":"QUJDRA=="}`, + wantText: "BODY", + }, } for _, tc := range cases { @@ -276,7 +301,10 @@ func TestManagerReadResourceRejectsEmptyAnswer(t *testing.T) { // for. The manager walks its servers in map order, so the answer must not depend // on which one it happens to ask first. The repetition is what makes the // regression visible: an implementation that takes the first empty reply as the -// answer passes a single attempt about half the time. +// answer only fails an attempt when the empty server is visited first, which +// Go's iteration over a two-key map does about a quarter of the time. Measured +// at eight attempts such an implementation still passed 5 runs in 40; thirty +// puts that under one in a thousand. func TestManagerReadResourceKeepsSearchingPastAnEmptyServer(t *testing.T) { manager := NewManager() manager.Connect(context.Background(), &config.Config{MCP: config.MCP{ @@ -289,7 +317,7 @@ func TestManagerReadResourceKeepsSearchingPastAnEmptyServer(t *testing.T) { defer manager.Close() const uri = `raw:{"contents":[{"uri":"mem:///a","mimeType":"text/plain","text":"BODY"}]}` - for i := 0; i < 8; i++ { + for i := 0; i < 30; i++ { text, err := manager.ReadResource(context.Background(), uri) if err != nil { t.Fatalf("attempt %d: %v", i, err) From ec4b0b6334e08d8c5d8b0432188e4def73c42ed3 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 21:32:32 +0700 Subject: [PATCH 17/28] Say when grep did not look at a file Co-authored-by: Cursor --- internal/tools/grep_skip_test.go | 75 ++++++++++++++++++++++++++++++++ internal/tools/search.go | 16 ++++++- 2 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 internal/tools/grep_skip_test.go diff --git a/internal/tools/grep_skip_test.go b/internal/tools/grep_skip_test.go new file mode 100644 index 0000000..ad3bddf --- /dev/null +++ b/internal/tools/grep_skip_test.go @@ -0,0 +1,75 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// The size gate keeps grep from opening large files at all, so a run that hit +// it cannot tell whether the pattern is there. Reporting only "No matches" +// reads as "not present", and it does so on exactly the large log and data +// files people reach for grep to search. +func TestGrepReportsSkippedFiles(t *testing.T) { + t.Run("a skipped file is reported", func(t *testing.T) { + workspace := t.TempDir() + writeSparseFile(t, filepath.Join(workspace, "huge.log"), "NEEDLE_TOKEN\n", 9*1024*1024) + + result := grepWorkspace(t, workspace, "NEEDLE_TOKEN") + if result.IsError { + t.Fatalf("grep errored: %s", result.Content) + } + for _, want := range []string{"1 file(s)", "8 MB", "not searched"} { + if !strings.Contains(result.Content, want) { + t.Errorf("skip report is missing %q, got: %q", want, result.Content) + } + } + }) + + t.Run("a run that skipped nothing says nothing", func(t *testing.T) { + workspace := t.TempDir() + if err := os.WriteFile(filepath.Join(workspace, "small.log"), []byte("NEEDLE_TOKEN here\n"), 0o644); err != nil { + t.Fatal(err) + } + + result := grepWorkspace(t, workspace, "NEEDLE_TOKEN") + if result.IsError { + t.Fatalf("grep errored: %s", result.Content) + } + if !strings.Contains(result.Content, "NEEDLE_TOKEN here") { + t.Fatalf("small file should have matched, got: %q", result.Content) + } + if strings.Contains(result.Content, "warning") { + t.Fatalf("nothing was skipped, so nothing should be warned about, got: %q", result.Content) + } + }) +} + +func grepWorkspace(t *testing.T, workspace, pattern string) Result { + t.Helper() + args, err := json.Marshal(map[string]any{"pattern": pattern, "path": "."}) + if err != nil { + t.Fatal(err) + } + return (grepTool{}).Execute(context.Background(), Input{Workspace: workspace, Args: args}) +} + +// writeSparseFile writes head and then extends the file to size with a hole, so +// a file that reports megabytes costs the test only the bytes in head. +func writeSparseFile(t *testing.T, path, head string, size int64) { + t.Helper() + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if _, err := f.WriteString(head); err != nil { + t.Fatal(err) + } + if err := f.Truncate(size); err != nil { + t.Fatal(err) + } +} diff --git a/internal/tools/search.go b/internal/tools/search.go index 6c4c082..ecfd079 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -155,6 +155,11 @@ func globToRegexp(pattern string) (*regexp.Regexp, error) { // ---- grep ------------------------------------------------------------------- +// maxGrepFileBytes caps the size of a file grep will open, so a single huge log +// cannot stall a search across a whole tree. A file above the cap is never +// read, which is why the count of them has to reach the caller. +const maxGrepFileBytes = 8 * 1024 * 1024 + type grepTool struct{} func (grepTool) Name() string { return "grep" } @@ -218,6 +223,7 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { b strings.Builder matches int files int + skipped int stopped bool warnings []string ) @@ -308,7 +314,8 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { if includeRe != nil && !includeRe.MatchString(rel) && !includeRe.MatchString(filepath.Base(rel)) { return nil } - if info, err := d.Info(); err == nil && info.Size() > 8*1024*1024 { + if info, err := d.Info(); err == nil && info.Size() > maxGrepFileBytes { + skipped++ return nil } return searchFile(p, rel) @@ -317,6 +324,13 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { _ = searchFile(root, relTo(in.Workspace, root)) } + // Nothing opened these files, so a bare "no matches" would report them as + // match-free. One line for the whole run: a directory of large files would + // otherwise bury the result under a list of paths. + if skipped > 0 { + warnings = append(warnings, fmt.Sprintf("%d file(s) larger than %d MB were not searched, so this result cannot rule out a match in them", skipped, maxGrepFileBytes/(1024*1024))) + } + warn := "" if len(warnings) > 0 { warn = "\nwarning: " + strings.Join(warnings, "\nwarning: ") From 538e898c87f9feda85add0de5205f3f8e3b8536c Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 22:42:39 +0700 Subject: [PATCH 18/28] Refuse to call a cut-off stream a finished answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/llm/anthropic.go | 17 +- internal/llm/client.go | 45 +++- internal/llm/openai.go | 16 +- internal/llm/retry.go | 5 + internal/llm/stream_framing_test.go | 307 ++++++++++++++++++++++++++++ 5 files changed, 381 insertions(+), 9 deletions(-) create mode 100644 internal/llm/stream_framing_test.go diff --git a/internal/llm/anthropic.go b/internal/llm/anthropic.go index f112fde..2bf3476 100644 --- a/internal/llm/anthropic.go +++ b/internal/llm/anthropic.go @@ -289,6 +289,9 @@ func (c *anthropicClient) Stream(ctx context.Context, req Request, emit func(Eve finish string model = req.Model blockKind = map[int]string{} + // A stop_reason in message_delta is not the end of the stream: Anthropic + // still owes a message_stop, and a gateway can drop the body in between. + sawTerminal bool ) err = sseLines(httpResp.Body, func(evName, data string) error { @@ -356,6 +359,10 @@ func (c *anthropicClient) Stream(ctx context.Context, req Request, emit func(Eve call.Name = ev.ContentBlock.Name return emit(Event{Type: EventToolCallStart, Index: ev.Index, ToolCallID: call.ID, ToolName: call.Name}) } + case "content_block_stop": + if blockKind[ev.Index] == "tool_use" { + acc.markComplete(ev.Index) + } case "content_block_delta": switch ev.Delta.Type { case "text_delta": @@ -388,14 +395,22 @@ func (c *anthropicClient) Stream(ctx context.Context, req Request, emit func(Eve u := usage return emit(Event{Type: EventUsage, Usage: &u}) } + case "message_stop": + sawTerminal = true } return nil }) if err != nil { return nil, err } + if !sawTerminal { + return nil, fmt.Errorf("%w: the stream ended without message_stop", ErrStreamTruncated) + } - calls := acc.result() + calls, err := acc.result() + if err != nil { + return nil, err + } for i, call := range calls { if err := emit(Event{Type: EventToolCallEnd, Index: i, ToolCallID: call.ID, ToolName: call.Name, Delta: call.Arguments}); err != nil { return nil, err diff --git a/internal/llm/client.go b/internal/llm/client.go index e83ecc7..e39e029 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -24,6 +24,14 @@ import ( // ErrUnsupported is returned by adapters that lack a capability. var ErrUnsupported = errors.New("operation not supported by this provider") +// ErrStreamTruncated is returned when a response body ended before the provider +// signalled the answer was finished. A gateway that loses its upstream closes +// the body cleanly, so end of body on its own says nothing about whether the +// answer is complete; only the provider's terminal marker does. Retryable +// classifies it as worth another attempt, since a cut turn produced no usable +// answer and replaying the request is safe. +var ErrStreamTruncated = errors.New("the provider's response ended before it was complete") + // Options configures an adapter instance. type Options struct { Kind string @@ -498,12 +506,13 @@ func truncate(s string, n int) string { // toolCallAccumulator reassembles streamed tool-call fragments in order. type toolCallAccumulator struct { - order []int - calls map[int]*ToolCall + order []int + calls map[int]*ToolCall + complete map[int]bool } func newToolCallAccumulator() *toolCallAccumulator { - return &toolCallAccumulator{calls: map[int]*ToolCall{}} + return &toolCallAccumulator{calls: map[int]*ToolCall{}, complete: map[int]bool{}} } func (a *toolCallAccumulator) ensure(idx int) *ToolCall { @@ -516,17 +525,39 @@ func (a *toolCallAccumulator) ensure(idx int) *ToolCall { return c } -func (a *toolCallAccumulator) result() []ToolCall { +// markComplete records that the provider said this call is fully sent, which is +// the only thing that tells a tool taking no arguments apart from a tool whose +// arguments were still on the way when the connection ended. Anthropic closes +// each call with content_block_stop; the OpenAI dialect has no per-call marker, +// so its adapter marks every call at the stream's terminal. +func (a *toolCallAccumulator) markComplete(idx int) { a.complete[idx] = true } + +func (a *toolCallAccumulator) markAllComplete() { + for _, idx := range a.order { + a.complete[idx] = true + } +} + +// result returns the reassembled calls, or an error if any of them is a +// fragment. Filling in absent arguments with "{}" would hand the caller a +// dispatchable call the model never finished asking for — write_file with no +// path — so a call the provider never completed fails the whole stream instead. +func (a *toolCallAccumulator) result() ([]ToolCall, error) { out := make([]ToolCall, 0, len(a.order)) for _, idx := range a.order { c := a.calls[idx] if c.Name == "" && c.Arguments == "" { continue } - if strings.TrimSpace(c.Arguments) == "" { - c.Arguments = "{}" + if !a.complete[idx] { + if c.Name == "" { + return nil, fmt.Errorf("%w: a tool call arrived without its name", ErrStreamTruncated) + } + if strings.TrimSpace(c.Arguments) == "" { + return nil, fmt.Errorf("%w: the tool call %q arrived without its arguments", ErrStreamTruncated, c.Name) + } } out = append(out, *c) } - return out + return out, nil } diff --git a/internal/llm/openai.go b/internal/llm/openai.go index 410e329..b1d4ffd 100644 --- a/internal/llm/openai.go +++ b/internal/llm/openai.go @@ -297,10 +297,14 @@ func (c *openAIClient) Stream(ctx context.Context, req Request, emit func(Event) finish string model = req.Model started = map[int]bool{} + // Either terminal is enough: real OpenAI sends both, and compatible + // servers exist that send only one. Requiring both would reject them. + sawTerminal bool ) err = sseLines(httpResp.Body, func(_, data string) error { if data == "[DONE]" { + sawTerminal = true return io.EOF } var chunk struct { @@ -337,6 +341,7 @@ func (c *openAIClient) Stream(ctx context.Context, req Request, emit func(Event) for _, ch := range chunk.Choices { if ch.FinishReason != "" { finish = ch.FinishReason + sawTerminal = true } if r := firstNonEmpty(ch.Delta.Reasoning, ch.Delta.ReasoningContent); r != "" { reasoning.WriteString(r) @@ -377,8 +382,17 @@ func (c *openAIClient) Stream(ctx context.Context, req Request, emit func(Event) if err != nil { return nil, err } + if !sawTerminal { + return nil, fmt.Errorf("%w: the stream ended without [DONE] or a finish_reason", ErrStreamTruncated) + } + // The dialect has no per-call terminator, so the stream's own terminal is + // what says every call it carried was fully sent. + acc.markAllComplete() - calls := acc.result() + calls, err := acc.result() + if err != nil { + return nil, err + } for i, call := range calls { if err := emit(Event{Type: EventToolCallEnd, Index: i, ToolCallID: call.ID, ToolName: call.Name, Delta: call.Arguments}); err != nil { return nil, err diff --git a/internal/llm/retry.go b/internal/llm/retry.go index e5d628e..0ab0298 100644 --- a/internal/llm/retry.go +++ b/internal/llm/retry.go @@ -149,6 +149,11 @@ func Retryable(err error) bool { if IsRateLimit(err) || IsUnreachable(err) { return true } + // A body that stopped before the provider's terminal marker produced no + // usable answer, so asking again is safe and usually works. + if errors.Is(err, ErrStreamTruncated) { + return true + } var ae *apiError if errors.As(err, &ae) { if ae.Status >= 500 && ae.Status <= 599 { diff --git a/internal/llm/stream_framing_test.go b/internal/llm/stream_framing_test.go new file mode 100644 index 0000000..f73a031 --- /dev/null +++ b/internal/llm/stream_framing_test.go @@ -0,0 +1,307 @@ +package llm + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// The bodies below are SSE recordings. A truncated one ends where a gateway +// that lost its upstream would close the connection: cleanly, mid-answer, with +// no terminal marker. End of body is not end of answer, and an adapter that +// treats the two as the same reports a cut-off turn as a finished one. + +// sseFramingServer serves one recorded body and closes the connection after it, +// which is what the adapter sees when a stream is cut short. +func sseFramingServer(t *testing.T, body string) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv +} + +func openAIFramingClient(t *testing.T, body string) *openAIClient { + srv := sseFramingServer(t, body) + return &openAIClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}, vendor: "compat"} +} + +func anthropicFramingClient(t *testing.T, body string) *anthropicClient { + srv := sseFramingServer(t, body) + return &anthropicClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}} +} + +func TestStreamFramingOpenAICutStreamsAreRetryableErrors(t *testing.T) { + cases := []struct { + name, body string + }{ + { + // The brief's case: arguments cut mid-JSON, no [DONE]. + "arguments cut mid-json", + `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_1","type":"function","function":{"name":"write_file","arguments":""}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"function":{"arguments":"{\"path\": \"/tm"}}]}}]}` + "\n\n", + }, + { + // The name arrived and nothing else did. Fabricating {} here is what + // turns a cut stream into write_file with no path. + "tool name with no arguments at all", + `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"call_1","type":"function","function":{"name":"write_file","arguments":""}}]}}]}` + "\n\n", + }, + { + // Every argument payload that arrived is valid JSON, so nothing + // downstream can tell this turn was cut. Only the missing terminal can. + "cut between parallel tool calls", + `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"a\"}"}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{"tool_calls":[{"index":1,"id":"c2","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"b\"}"}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{"tool_calls":[{"index":2,"id":"c3","type":"function","function":{"name":"read_file","arguments":""}}]}}]}` + "\n\n", + }, + { + "prose cut mid-sentence", + `data: {"choices":[{"delta":{"content":"I will now "}}]}` + "\n\n", + }, + { + "empty body", + "", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + client := openAIFramingClient(t, c.body) + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err == nil { + t.Fatalf("a body that ended without [DONE] or a finish_reason was accepted as complete: %+v", resp) + } + if !Retryable(err) { + t.Fatalf("a cut stream must reach the turn-level retry, got %v", err) + } + assertNoFabricatedArguments(t, resp) + }) + } +} + +// Requiring both [DONE] and a finish_reason would break servers that send only +// one, so either has to be enough. +func TestStreamFramingOpenAIAcceptsEitherTerminalSignal(t *testing.T) { + call := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"a\"}"}}]}}]}` + "\n\n" + cases := []struct { + name, body string + }{ + {"both", call + `data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}` + "\n\n" + "data: [DONE]\n\n"}, + {"finish_reason only", call + `data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}` + "\n\n"}, + {"done only", call + "data: [DONE]\n\n"}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + client := openAIFramingClient(t, c.body) + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a terminated stream must succeed: %v", err) + } + if len(resp.ToolCalls) != 1 { + t.Fatalf("tool calls = %+v, want the one that was sent", resp.ToolCalls) + } + if got, want := resp.ToolCalls[0].Arguments, `{"path":"a"}`; got != want { + t.Fatalf("arguments = %q, want %q", got, want) + } + }) + } +} + +func TestStreamFramingAnthropicCutStreamsAreRetryableErrors(t *testing.T) { + cases := []struct { + name, body string + }{ + { + // The brief's case: the block that names the tool arrives and the + // body ends. No content_block_stop, so the input never finished. + "tool_use block then nothing", + "event: message_start\n" + + `data: {"type":"message_start","message":{"model":"claude","usage":{"input_tokens":9}}}` + "\n\n" + + "event: content_block_start\n" + + `data: {"type":"content_block_start","index":0,"content_block":{"type":"tool_use","id":"toolu_1","name":"write_file","input":{}}}` + "\n\n", + }, + { + "input cut mid-json", + "event: content_block_start\n" + + `data: {"type":"content_block_start","index":0,"content_block":{"type":"tool_use","id":"toolu_1","name":"write_file","input":{}}}` + "\n\n" + + "event: content_block_delta\n" + + `data: {"type":"content_block_delta","index":0,"delta":{"type":"input_json_delta","partial_json":"{\"path\": \"/tm"}}` + "\n\n", + }, + { + // A stop_reason is not a terminal marker: Anthropic still owes a + // message_stop, and a gateway can drop the body in between. + "message_delta without message_stop", + "event: content_block_delta\n" + + `data: {"type":"content_block_delta","index":0,"delta":{"type":"text_delta","text":"hi"}}` + "\n\n" + + "event: message_delta\n" + + `data: {"type":"message_delta","delta":{"stop_reason":"end_turn"},"usage":{"output_tokens":2}}` + "\n\n", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + client := anthropicFramingClient(t, c.body) + resp, err := client.Stream(context.Background(), Request{Model: "claude"}, func(Event) error { return nil }) + if err == nil { + t.Fatalf("a body that ended without message_stop was accepted as complete: %+v", resp) + } + if !Retryable(err) { + t.Fatalf("a cut stream must reach the turn-level retry, got %v", err) + } + assertNoFabricatedArguments(t, resp) + }) + } +} + +func TestStreamFramingAnthropicCompleteStreamYieldsTheCall(t *testing.T) { + body := "event: message_start\n" + + `data: {"type":"message_start","message":{"model":"claude","usage":{"input_tokens":9}}}` + "\n\n" + + "event: content_block_start\n" + + `data: {"type":"content_block_start","index":0,"content_block":{"type":"tool_use","id":"toolu_1","name":"read_file","input":{}}}` + "\n\n" + + "event: content_block_delta\n" + + `data: {"type":"content_block_delta","index":0,"delta":{"type":"input_json_delta","partial_json":"{\"path\":"}}` + "\n\n" + + "event: content_block_delta\n" + + `data: {"type":"content_block_delta","index":0,"delta":{"type":"input_json_delta","partial_json":"\"a\"}"}}` + "\n\n" + + "event: content_block_stop\n" + + `data: {"type":"content_block_stop","index":0}` + "\n\n" + + "event: message_delta\n" + + `data: {"type":"message_delta","delta":{"stop_reason":"tool_use"},"usage":{"output_tokens":5}}` + "\n\n" + + "event: message_stop\n" + + `data: {"type":"message_stop"}` + "\n\n" + + client := anthropicFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "claude"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a terminated stream must succeed: %v", err) + } + if len(resp.ToolCalls) != 1 { + t.Fatalf("tool calls = %+v, want the one that was sent", resp.ToolCalls) + } + if got, want := resp.ToolCalls[0].Arguments, `{"path":"a"}`; got != want { + t.Fatalf("arguments = %q, want %q", got, want) + } +} + +// A tool that takes no parameters does not arrive as {} on the wire. Anthropic +// documents that such a call emits content_block_start and content_block_stop +// with no input_json_delta between them, so the accumulated input is empty; +// OpenAI opens the call with an empty arguments string. Both are complete +// answers, and the marker that says so is the provider's own close of the call +// — which is what tells them apart from a stream cut before the arguments came. +// Rejecting an empty argument payload on its own would break every one of them. +func TestStreamFramingParameterlessToolCallSurvives(t *testing.T) { + anthropicBodies := map[string]string{ + // The documented shape: nothing at all between start and stop. + "no input_json_delta at all": "", + // Some servers open the run with an empty fragment before the real + // ones; for a parameterless call that fragment is all there is. + "empty partial_json": "event: content_block_delta\n" + + `data: {"type":"content_block_delta","index":0,"delta":{"type":"input_json_delta","partial_json":""}}` + "\n\n", + } + + for name, deltas := range anthropicBodies { + t.Run("anthropic/"+name, func(t *testing.T) { + body := "event: content_block_start\n" + + `data: {"type":"content_block_start","index":0,"content_block":{"type":"tool_use","id":"toolu_1","name":"snooze","input":{}}}` + "\n\n" + + deltas + + "event: content_block_stop\n" + + `data: {"type":"content_block_stop","index":0}` + "\n\n" + + "event: message_delta\n" + + `data: {"type":"message_delta","delta":{"stop_reason":"tool_use"},"usage":{"output_tokens":5}}` + "\n\n" + + "event: message_stop\n" + + `data: {"type":"message_stop"}` + "\n\n" + + client := anthropicFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "claude"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a closed tool_use block with no input is a complete call: %v", err) + } + if len(resp.ToolCalls) != 1 || resp.ToolCalls[0].Name != "snooze" { + t.Fatalf("tool calls = %+v, want snooze", resp.ToolCalls) + } + // The adapter reports what arrived and nothing more. Turning an + // absence of arguments into {} belongs to the caller, which can only + // do it safely because the stream reaching here means it was whole. + if got := resp.ToolCalls[0].Arguments; got != "" { + t.Fatalf("arguments = %q, want the empty input the provider actually sent", got) + } + }) + } + + t.Run("openai", func(t *testing.T) { + body := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"snooze","arguments":""}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}` + "\n\n" + + "data: [DONE]\n\n" + + client := openAIFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a terminated stream with an empty arguments string is a complete call: %v", err) + } + if len(resp.ToolCalls) != 1 || resp.ToolCalls[0].Name != "snooze" { + t.Fatalf("tool calls = %+v, want snooze", resp.ToolCalls) + } + if got := resp.ToolCalls[0].Arguments; got != "" { + t.Fatalf("arguments = %q, want the empty arguments the provider actually sent", got) + } + }) +} + +// Classifying the error as retryable is only worth anything if the retry +// machinery acts on it, so drive a real client through a gateway that drops the +// first body and answers the second. +func TestStreamFramingTruncationIsRetriedNotSurfaced(t *testing.T) { + complete := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"a\"}"}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{},"finish_reason":"tool_calls"}]}` + "\n\n" + + "data: [DONE]\n\n" + + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + attempts++ + if attempts == 1 { + return // upstream lost; body closes clean and empty + } + _, _ = w.Write([]byte(complete)) + })) + t.Cleanup(srv.Close) + + client, err := New(Options{ + Kind: "openai-compatible", BaseURL: srv.URL, APIKey: "k", + HTTPClient: srv.Client(), Retries: 1, RetryBaseDelay: time.Millisecond, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a dropped first body should have been retried, not surfaced: %v", err) + } + if attempts != 2 { + t.Fatalf("attempts = %d, want the cut stream to cost one retry", attempts) + } + if len(resp.ToolCalls) != 1 || resp.ToolCalls[0].Arguments != `{"path":"a"}` { + t.Fatalf("tool calls = %+v, want the call the second attempt sent", resp.ToolCalls) + } +} + +// assertNoFabricatedArguments guards the substitution the accumulator used to +// make: arguments that never arrived became {}, which dispatches as a real call. +func assertNoFabricatedArguments(t *testing.T, resp *Response) { + t.Helper() + if resp == nil { + return + } + for _, call := range resp.ToolCalls { + if call.Arguments == "{}" { + t.Fatalf("tool call %q was returned with fabricated {} arguments", call.Name) + } + } +} From 2b7fa3b2491915d5c413e007dbd4addb206c3b08 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 22:55:30 +0700 Subject: [PATCH 19/28] Treat a severed body and a capped answer as the cut streams they are MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/llm/client.go | 13 ++- internal/llm/openai.go | 9 +- internal/llm/stream_framing_test.go | 164 ++++++++++++++++++++++++++-- 3 files changed, 173 insertions(+), 13 deletions(-) diff --git a/internal/llm/client.go b/internal/llm/client.go index e39e029..390e0be 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -489,7 +489,18 @@ func sseLines(r io.Reader, fn func(event, data string) error) error { } } if err := sc.Err(); err != nil { - return err + // A read that dies part-way through is a body that stopped early, + // whatever the socket called it: unexpected EOF, a peer reset, a + // chunked frame that never landed. Left raw it matches nothing + // Retryable looks for and fails the turn outright. Cancellation and + // the idle timeout are the caller's own doing, and a line too long for + // the buffer arrives the same oversized way however often it is asked + // for, so none of those three are truncation and none is worth a retry. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || + errors.Is(err, bufio.ErrTooLong) { + return err + } + return fmt.Errorf("%w: %w", ErrStreamTruncated, err) } if err := flush(); err != nil && !errors.Is(err, io.EOF) { return err diff --git a/internal/llm/openai.go b/internal/llm/openai.go index b1d4ffd..4a9ffc8 100644 --- a/internal/llm/openai.go +++ b/internal/llm/openai.go @@ -386,8 +386,13 @@ func (c *openAIClient) Stream(ctx context.Context, req Request, emit func(Event) return nil, fmt.Errorf("%w: the stream ended without [DONE] or a finish_reason", ErrStreamTruncated) } // The dialect has no per-call terminator, so the stream's own terminal is - // what says every call it carried was fully sent. - acc.markAllComplete() + // what says every call it carried was fully sent. At the token cap it says + // no such thing: the answer can stop between a call's name and its + // arguments and still be framed correctly, so leave those calls unmarked + // and let result() refuse whatever the model did not finish asking for. + if finish != "length" { + acc.markAllComplete() + } calls, err := acc.result() if err != nil { diff --git a/internal/llm/stream_framing_test.go b/internal/llm/stream_framing_test.go index f73a031..23e6d7d 100644 --- a/internal/llm/stream_framing_test.go +++ b/internal/llm/stream_framing_test.go @@ -2,6 +2,8 @@ package llm import ( "context" + "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -79,7 +81,7 @@ func TestStreamFramingOpenAICutStreamsAreRetryableErrors(t *testing.T) { if !Retryable(err) { t.Fatalf("a cut stream must reach the turn-level retry, got %v", err) } - assertNoFabricatedArguments(t, resp) + assertNoPartialAnswer(t, resp) }) } } @@ -154,7 +156,7 @@ func TestStreamFramingAnthropicCutStreamsAreRetryableErrors(t *testing.T) { if !Retryable(err) { t.Fatalf("a cut stream must reach the turn-level retry, got %v", err) } - assertNoFabricatedArguments(t, resp) + assertNoPartialAnswer(t, resp) }) } } @@ -292,16 +294,158 @@ func TestStreamFramingTruncationIsRetriedNotSurfaced(t *testing.T) { } } -// assertNoFabricatedArguments guards the substitution the accumulator used to -// make: arguments that never arrived became {}, which dispatches as a real call. -func assertNoFabricatedArguments(t *testing.T, resp *Response) { +// severingServer drops the socket mid-body instead of ending it. A handler that +// simply returns produces a clean EOF at the client, which is a different +// failure from a reset or a chunked frame that never lands: those surface as a +// read error out of the scanner rather than as an orderly end of body. +func severingServer(t *testing.T, body string) *httptest.Server { t.Helper() - if resp == nil { - return + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + conn, buf, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + defer conn.Close() + fmt.Fprint(buf, "HTTP/1.1 200 OK\r\nContent-Type: text/event-stream\r\nTransfer-Encoding: chunked\r\n\r\n") + fmt.Fprintf(buf, "%x\r\n%s\r\n", len(body), body) + _ = buf.Flush() + // No terminating zero-length chunk: the body is severed, not finished. + })) + t.Cleanup(srv.Close) + return srv +} + +// The physical form of this failure is usually a reset or a half-sent chunk, +// not a tidy end of body. That error comes back from the scanner and used to be +// returned raw, ahead of the terminal check and matching nothing Retryable +// looks for, so the very case the terminal requirement exists to catch became a +// hard turn error instead of a retry. +func TestStreamFramingSeveredConnectionIsRetryable(t *testing.T) { + t.Run("openai", func(t *testing.T) { + frame := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"write_file","arguments":"{\"path\": \"/tm"}}]}}]}` + "\n\n" + srv := severingServer(t, frame) + client := &openAIClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}, vendor: "compat"} + + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + assertSeveredStream(t, resp, err) + }) + + t.Run("anthropic", func(t *testing.T) { + frame := "event: content_block_start\n" + + `data: {"type":"content_block_start","index":0,"content_block":{"type":"tool_use","id":"toolu_1","name":"write_file","input":{}}}` + "\n\n" + srv := severingServer(t, frame) + client := &anthropicClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}} + + resp, err := client.Stream(context.Background(), Request{Model: "claude"}, func(Event) error { return nil }) + assertSeveredStream(t, resp, err) + }) +} + +func assertSeveredStream(t *testing.T, resp *Response, err error) { + t.Helper() + if err == nil { + t.Fatalf("a severed connection was accepted as a complete answer: %+v", resp) + } + if !errors.Is(err, ErrStreamTruncated) { + t.Fatalf("a read that died mid-body is a truncated stream, got %#v", err) + } + if !Retryable(err) { + t.Fatalf("a severed stream must reach the turn-level retry, got %v", err) + } + assertNoPartialAnswer(t, resp) +} + +// The token cap can fall between a tool call's name and its arguments. The +// stream is framed correctly in that case — finish_reason and [DONE] both +// arrive — so framing vouches for nothing and only the finish reason says the +// answer stopped short of what the model meant to say. +func TestStreamFramingTokenCapDoesNotCompleteAToolCall(t *testing.T) { + t.Run("a call whose arguments never came is refused", func(t *testing.T) { + body := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"write_file","arguments":""}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{},"finish_reason":"length"}]}` + "\n\n" + + "data: [DONE]\n\n" + + client := openAIFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err == nil { + t.Fatalf("write_file with no arguments was accepted because the framing was tidy: %+v", resp) + } + if !Retryable(err) { + t.Fatalf("a cap-truncated call must reach the turn-level retry, got %v", err) + } + assertNoPartialAnswer(t, resp) + }) + + // The cap is not by itself a reason to throw the turn away: a call whose + // arguments did arrive is as usable as any other. + t.Run("a call whose arguments did come survives", func(t *testing.T) { + body := `data: {"choices":[{"delta":{"tool_calls":[{"index":0,"id":"c1","type":"function","function":{"name":"read_file","arguments":"{\"path\":\"a\"}"}}]}}]}` + "\n\n" + + `data: {"choices":[{"delta":{},"finish_reason":"length"}]}` + "\n\n" + + "data: [DONE]\n\n" + + client := openAIFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a cap that fell after the arguments still leaves a usable call: %v", err) + } + if len(resp.ToolCalls) != 1 || resp.ToolCalls[0].Arguments != `{"path":"a"}` { + t.Fatalf("tool calls = %+v, want the call the provider finished sending", resp.ToolCalls) + } + }) +} + +// The retry that fires in production is the one the client declines to make. +// Once tokens have reached the caller, replaying would show the answer twice, +// so the client stops and hands the error up for the agent's turn loop, which +// resets the partial reply before asking again. Pin that division of labour: +// the client must not retry, and the error must stay retryable for the layer +// that can. +func TestStreamFramingCutAfterTokensIsHandedToTheCaller(t *testing.T) { + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + attempts++ + _, _ = w.Write([]byte(`data: {"choices":[{"delta":{"content":"I will now "}}]}` + "\n\n")) + })) + t.Cleanup(srv.Close) + + client, err := New(Options{ + Kind: "openai-compatible", BaseURL: srv.URL, APIKey: "k", + HTTPClient: srv.Client(), Retries: 3, RetryBaseDelay: time.Millisecond, + }) + if err != nil { + t.Fatalf("New: %v", err) } - for _, call := range resp.ToolCalls { - if call.Arguments == "{}" { - t.Fatalf("tool call %q was returned with fabricated {} arguments", call.Name) + + var shown []string + _, err = client.Stream(context.Background(), Request{Model: "glm-5.2"}, func(e Event) error { + if e.Type == EventText { + shown = append(shown, e.Delta) } + return nil + }) + if err == nil { + t.Fatal("a body cut after the first token was accepted as a complete answer") + } + if attempts != 1 { + t.Fatalf("attempts = %d, want the client to stop once tokens had been shown", attempts) + } + if len(shown) != 1 { + t.Fatalf("emitted %d text events, want the partial reply shown once and not replayed", len(shown)) + } + if !Retryable(err) { + t.Fatalf("the agent turn loop only retries what Retryable admits, got %v", err) + } +} + +// A cut stream must hand back no answer at all, not an answer plus an error. +// Returning the partial one would leave the fabricated call the accumulator +// used to invent — write_file with no path — within reach of any caller that +// reads the response before the error, which is a thing callers do. +func assertNoPartialAnswer(t *testing.T, resp *Response) { + t.Helper() + if resp != nil { + t.Fatalf("a cut stream returned a usable answer beside its error: %+v", resp) } } From 69f46547843e0228fd98a7cb09282dcb6f207810 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:30:23 +0700 Subject: [PATCH 20/28] Register an MCP call before its request goes out 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 --- internal/mcp/client.go | 40 +++++++---- internal/mcp/transport_test.go | 127 +++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 14 deletions(-) create mode 100644 internal/mcp/transport_test.go diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 97fdf39..dfbed07 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -634,36 +634,48 @@ func (t *stdioTransport) send(ctx context.Context, req rpcRequest) (*rpcResponse t.sendMu.Lock() defer t.sendMu.Unlock() + // Register a per-call response channel keyed by request ID *before* the + // frame goes out. From the second call onward the background reader is + // already running, so a server that answers while the write is still + // returning would have its reply looked up against an ID the map does not + // hold yet, and the reader would discard it as stale; the caller then waits + // out its whole deadline for an answer that already arrived. A registration + // made first is never too late: the reader cannot see a reply to a request + // that has not been written. + ch := make(chan *rpcResponse, 1) + t.pendingMu.Lock() + if t.pending == nil { + t.pending = map[int64]chan *rpcResponse{} + } + t.pending[req.ID] = ch + t.pendingMu.Unlock() + unregister := func() { + t.pendingMu.Lock() + delete(t.pending, req.ID) + t.pendingMu.Unlock() + } + t.mu.Lock() if t.closed { t.mu.Unlock() + unregister() return nil, fmt.Errorf("mcp connection closed") } err := t.writeFrame(req) t.mu.Unlock() if err != nil { + // Nothing will ever answer a frame that did not go out. + unregister() return nil, err } - // Register a per-call response channel keyed by request ID, then start - // (or reuse) the single background reader. On ctx.Done the entry is + // Start (or reuse) the single background reader. On ctx.Done the entry is // removed so any late reply is discarded by ID mismatch — the transport // stays alive for subsequent calls. - ch := make(chan *rpcResponse, 1) - t.pendingMu.Lock() - if t.pending == nil { - t.pending = map[int64]chan *rpcResponse{} - } - t.pending[req.ID] = ch - t.pendingMu.Unlock() t.startReader() // Ensure the entry is cleaned up no matter how we exit. - defer func() { - t.pendingMu.Lock() - delete(t.pending, req.ID) - t.pendingMu.Unlock() - }() + defer unregister() select { case <-ctx.Done(): diff --git a/internal/mcp/transport_test.go b/internal/mcp/transport_test.go new file mode 100644 index 0000000..e0ded2d --- /dev/null +++ b/internal/mcp/transport_test.go @@ -0,0 +1,127 @@ +package mcp + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "errors" + "os" + "testing" + "time" +) + +// replyOnWrite stands in for the child's stdin. Writing a request frame to it +// answers that frame on the transport's stdout straight away and then pauses, +// which is the ordering a fast local server produces: the reply is on the wire +// before the caller that wrote the request has run another statement. +type replyOnWrite struct { + out *os.File + settle time.Duration + err error // returned instead of writing, when set +} + +func (w *replyOnWrite) Write(p []byte) (int, error) { + if w.err != nil { + return 0, w.err + } + var req struct { + ID *int64 `json:"id"` + } + if err := json.Unmarshal(bytes.TrimSpace(p), &req); err == nil && req.ID != nil { + frame, _ := json.Marshal(rpcResponse{ + JSONRPC: "2.0", + ID: req.ID, + Result: json.RawMessage(`{"ok":true}`), + }) + if _, err := w.out.Write(append(frame, '\n')); err != nil { + return 0, err + } + // Give the background reader time to pick the reply up and look its id + // up in the pending map, so the ordering under test is not a race. + time.Sleep(w.settle) + } + return len(p), nil +} + +func (w *replyOnWrite) Close() error { return nil } + +// eagerTransport builds a stdioTransport wired to a server that answers during +// the write. There is no child process, so the test must not close it. +func eagerTransport(t *testing.T) (*stdioTransport, *replyOnWrite) { + t.Helper() + pr, pw, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + t.Cleanup(func() { + _ = pw.Close() + _ = pr.Close() + }) + stdin := &replyOnWrite{out: pw, settle: 50 * time.Millisecond} + return &stdioTransport{ + stdin: stdin, + stdout: bufio.NewReaderSize(pr, 1<<20), + pending: map[int64]chan *rpcResponse{}, + readerDone: make(chan struct{}), + }, stdin +} + +// TestStdioDeliversReplyThatArrivesBeforeRegistration covers a reply that beats +// its own caller back to the pending map. From the second call onward the +// background reader is already running, so a request registered after its frame +// goes out can have its answer looked up against an id the map does not hold +// yet — the answer is dropped and the caller waits out its whole deadline. +func TestStdioDeliversReplyThatArrivesBeforeRegistration(t *testing.T) { + tr, _ := eagerTransport(t) + + for id := int64(1); id <= 4; id++ { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + resp, err := tr.send(ctx, rpcRequest{JSONRPC: "2.0", ID: id, Method: "tools/call"}) + cancel() + if err != nil { + t.Fatalf("call %d was lost: %v", id, err) + } + if resp == nil || resp.ID == nil || *resp.ID != id { + t.Fatalf("call %d answered by %+v, want the reply carrying id %d", id, resp, id) + } + } +} + +// TestStdioFailedWriteLeavesNoPendingEntry pins the other half of registering +// first: a request whose frame never reached the child must not be left in the +// map, or the next reader error fans out to a caller that is no longer there +// and the entry leaks for the life of the transport. +func TestStdioFailedWriteLeavesNoPendingEntry(t *testing.T) { + tr, stdin := eagerTransport(t) + stdin.err = errors.New("broken pipe") + + if _, err := tr.send(context.Background(), rpcRequest{JSONRPC: "2.0", ID: 7, Method: "tools/call"}); err == nil { + t.Fatal("expected the failed write to surface as an error") + } + + tr.pendingMu.Lock() + left := len(tr.pending) + tr.pendingMu.Unlock() + if left != 0 { + t.Fatalf("pending holds %d entries after a failed write, want 0", left) + } +} + +// TestStdioClosedTransportLeavesNoPendingEntry is the same guard for the other +// early return in send. +func TestStdioClosedTransportLeavesNoPendingEntry(t *testing.T) { + tr, _ := eagerTransport(t) + tr.closed = true + + if _, err := tr.send(context.Background(), rpcRequest{JSONRPC: "2.0", ID: 9, Method: "tools/call"}); err == nil { + t.Fatal("expected a closed transport to refuse the call") + } + + tr.pendingMu.Lock() + left := len(tr.pending) + tr.pendingMu.Unlock() + if left != 0 { + t.Fatalf("pending holds %d entries after a refused call, want 0", left) + } +} From c0c2019296a33181e49ece0266cae1f8932b26f5 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:33:34 +0700 Subject: [PATCH 21/28] Hold gemini to a terminal marker like every other adapter 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 --- internal/llm/gemini.go | 18 +++++- internal/llm/stream_framing_test.go | 95 +++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 3 deletions(-) diff --git a/internal/llm/gemini.go b/internal/llm/gemini.go index 14ccb18..83cecd4 100644 --- a/internal/llm/gemini.go +++ b/internal/llm/gemini.go @@ -529,10 +529,13 @@ func (c *geminiClient) Stream(ctx context.Context, req Request, emit func(Event) for _, p := range cand.Content.Parts { switch { case p.FunctionCall != nil: + // Report the payload that arrived and nothing more. A call + // with no args is either a parameterless tool or a stream + // that stopped before the arguments; the finishReason check + // below is what tells those apart, and filling in {} here + // would hand on a dispatchable call the model never + // finished asking for. args := string(p.FunctionCall.Args) - if strings.TrimSpace(args) == "" { - args = "{}" - } id := p.FunctionCall.ID if id == "" { id = fmt.Sprintf("call_%d_%s", callSeq, p.FunctionCall.Name) @@ -587,6 +590,15 @@ func (c *geminiClient) Stream(ctx context.Context, req Request, emit func(Event) if err != nil { return nil, err } + // The candidate's finishReason is Gemini's terminal marker. Parts arrive + // whole rather than as fragments, so nothing inside a chunk can show that + // the answer stopped early — a gateway that loses its upstream closes the + // body cleanly after a perfectly well-formed functionCall. Only the absence + // of a finishReason says the answer was cut, and every caller downstream + // treats what this returns as a stream the adapter has vouched for. + if finish == "" { + return nil, fmt.Errorf("%w: the stream ended without a finishReason", ErrStreamTruncated) + } // If a functionCall arrived without a signature, inject the CLI dummy so // the next turn does not 400. Prefer part-level sig already stored. for i := range calls { diff --git a/internal/llm/stream_framing_test.go b/internal/llm/stream_framing_test.go index 23e6d7d..febc033 100644 --- a/internal/llm/stream_framing_test.go +++ b/internal/llm/stream_framing_test.go @@ -37,6 +37,11 @@ func anthropicFramingClient(t *testing.T, body string) *anthropicClient { return &anthropicClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}} } +func geminiFramingClient(t *testing.T, body string) *geminiClient { + srv := sseFramingServer(t, body) + return &geminiClient{opts: Options{BaseURL: srv.URL, APIKey: "k", HTTPClient: srv.Client()}} +} + func TestStreamFramingOpenAICutStreamsAreRetryableErrors(t *testing.T) { cases := []struct { name, body string @@ -255,6 +260,96 @@ func TestStreamFramingParameterlessToolCallSurvives(t *testing.T) { }) } +// Gemini's terminal is the candidate's finishReason. Its parts arrive whole +// rather than as fragments, so nothing inside a chunk can show that the answer +// stopped early; only the absence of a finishReason can. +func TestStreamFramingGeminiCutStreamsAreRetryableErrors(t *testing.T) { + cases := []struct { + name, body string + }{ + { + // The name arrived and the body ended. Substituting {} here is what + // turns a cut stream into a dispatchable write_file with no path. + "functionCall then nothing", + `data: {"candidates":[{"content":{"parts":[{"functionCall":{"name":"write_file"}}]}}]}` + "\n\n", + }, + { + // Every part that arrived is well-formed, so the call looks whole. + "functionCall with arguments but no finishReason", + `data: {"candidates":[{"content":{"parts":[{"functionCall":{"name":"read_file","args":{"path":"a"}}}]}}]}` + "\n\n", + }, + { + "prose cut mid-sentence", + `data: {"candidates":[{"content":{"parts":[{"text":"I will now "}]}}]}` + "\n\n", + }, + { + "empty body", + "", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + client := geminiFramingClient(t, c.body) + resp, err := client.Stream(context.Background(), Request{Model: "gemini-3.6-flash"}, func(Event) error { return nil }) + if err == nil { + t.Fatalf("a body that ended without a finishReason was accepted as complete: %+v", resp) + } + if !Retryable(err) { + t.Fatalf("a cut stream must reach the turn-level retry, got %v", err) + } + assertNoPartialAnswer(t, resp) + }) + } +} + +// The other direction: a finishReason makes the answer whole, and a call that +// really took no arguments is reported as the empty payload the provider sent +// rather than as a fabricated {}. Filling that in belongs to the caller, which +// can only do it safely because reaching it means the stream was complete. +func TestStreamFramingGeminiCompleteStreamYieldsTheCall(t *testing.T) { + cases := []struct { + name, parts, wantArgs string + }{ + {"arguments sent", `{"functionCall":{"name":"read_file","args":{"path":"a"}}}`, `{"path":"a"}`}, + {"parameterless call", `{"functionCall":{"name":"snooze"}}`, ""}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + body := `data: {"candidates":[{"content":{"parts":[` + c.parts + `]}}]}` + "\n\n" + + `data: {"candidates":[{"content":{"parts":[]},"finishReason":"STOP"}]}` + "\n\n" + + client := geminiFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "gemini-3.6-flash"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a terminated stream must succeed: %v", err) + } + if len(resp.ToolCalls) != 1 { + t.Fatalf("tool calls = %+v, want the one that was sent", resp.ToolCalls) + } + if got := resp.ToolCalls[0].Arguments; got != c.wantArgs { + t.Fatalf("arguments = %q, want %q", got, c.wantArgs) + } + }) + } +} + +// A plain answer with no tool call is held to the same terminal. +func TestStreamFramingGeminiCompleteTextAnswerSurvives(t *testing.T) { + body := `data: {"candidates":[{"content":{"parts":[{"text":"done"}]}}]}` + "\n\n" + + `data: {"candidates":[{"content":{"parts":[]},"finishReason":"STOP"}]}` + "\n\n" + + client := geminiFramingClient(t, body) + resp, err := client.Stream(context.Background(), Request{Model: "gemini-3.6-flash"}, func(Event) error { return nil }) + if err != nil { + t.Fatalf("a terminated stream must succeed: %v", err) + } + if resp.Content != "done" || resp.FinishReason != "stop" { + t.Fatalf("response = %+v, want the finished answer", resp) + } +} + // Classifying the error as retryable is only worth anything if the retry // machinery acts on it, so drive a real client through a gateway that drops the // first body and answers the second. From 9a9755030b3c0259822d5057d6328a65a8d88165 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:38:14 +0700 Subject: [PATCH 22/28] Never answer a call with output produced before it was made MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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__ 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 --- internal/agent/agent.go | 31 +++++++++++----- internal/agent/toolresults_test.go | 59 ++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 1792281..c382b5a 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -616,13 +616,14 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error // it. The notice still fires now, so the user sees the repetition the // moment it is detected. var repeatNudge string - if stuck := repeats.record(resp.ToolCalls); len(stuck) > 0 { - if repeats.exceeded() { - _ = emit(Event{Type: EventNotice, Message: "stopped: the same tool call kept repeating"}) - lastReply = "I was repeating the same step without making progress, so I stopped. " + - "Tell me what to try differently." - break - } + stuck, stop := repeats.check(resp.ToolCalls) + if stop { + _ = emit(Event{Type: EventNotice, Message: "stopped: the same tool call kept repeating"}) + lastReply = "I was repeating the same step without making progress, so I stopped. " + + "Tell me what to try differently." + break + } + if len(stuck) > 0 { _ = emit(Event{Type: EventNotice, Message: "repeating " + strings.Join(stuck, ", ")}) repeatNudge = "You have called " + strings.Join(stuck, " and ") + " with the same arguments several times and it is not getting you anywhere. " + @@ -1276,7 +1277,16 @@ func ensureToolResults(msgs []llm.Message) []llm.Message { continue } for _, ri := range byID[msgs[t.at].ToolCalls[j].ID] { - if claimed[ri] || (inSpan && (ri < t.at || ri >= t.end)) { + // A result that appears before the call was produced before the + // call was made, so it can never be its answer — not in either + // pass. Only the forward bound is relaxed outside the span, for + // the result a later assistant message was appended in front + // of. Letting the second pass reach backwards too is how a + // history that opens with an orphaned tool message — the tail + // persistContextCompact leaves when it cuts at throughSeq — has + // last hour's output handed to a fresh call under a recurring + // id, with nothing marking it stale. + if claimed[ri] || ri < t.at || (inSpan && ri >= t.end) { continue } claimed[ri] = true @@ -1288,8 +1298,9 @@ func ensureToolResults(msgs []llm.Message) []llm.Message { for i := range turns { bind(&turns[i], true) } - // Only now may a call reach outside its span, for the result that a later - // assistant message was appended in front of. + // Only now may a call reach past the end of its span, for the result that a + // later assistant message was appended in front of. It still may not reach + // back before itself. for i := range turns { bind(&turns[i], false) } diff --git a/internal/agent/toolresults_test.go b/internal/agent/toolresults_test.go index 9490858..03cddc1 100644 --- a/internal/agent/toolresults_test.go +++ b/internal/agent/toolresults_test.go @@ -278,6 +278,65 @@ func TestEnsureToolResults_OrphanResultIsNotAdoptedByALaterCall(t *testing.T) { } } +// The same orphan, with nothing live to lose the race to. persistContextCompact +// 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. A call that then +// arrives under a recurring Gemini id must not be answered with it: that is +// last hour's file contents presented as this turn's read, with nothing saying +// so. A stub that admits no result was recorded is the honest answer. +func TestEnsureToolResults_OrphanBeforeACallIsNeverBoundBackwards(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "STALE ORPHAN FROM AN EARLIER TURN"}, + {Role: llm.RoleUser, Content: "[Compacted summary of the earlier conversation]"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + } + got := roleAndContent(ensureToolResults(in)) + want := []string{ + "user:[Compacted summary of the earlier conversation]", + "assistant:", + "tool:" + noResultStub, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("a call was answered with output produced before it was made:\n got %q\nwant %q", got, want) + } +} + +// The other direction, which is what the second pass is for. A result may sit +// past the end of its turn's span — the span closes at the next assistant +// message, and a result can land after one — and reaching forward for it is the +// difference between the model getting its output and getting a stub. Only +// reaching backwards is forbidden. +func TestEnsureToolResults_CallStillReachesPastItsOwnSpan(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "c1", Name: "read_file"}}}, + {Role: llm.RoleAssistant, Content: "thinking out loud"}, + {Role: llm.RoleTool, ToolCallID: "c1", Name: "read_file", Content: "REAL FILE CONTENT"}, + } + got := roleAndContent(ensureToolResults(in)) + want := []string{"assistant:", "tool:REAL FILE CONTENT", "assistant:thinking out loud"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("a real result was dropped and stubbed instead:\n got %q\nwant %q", got, want) + } +} + +// Both rules at once: the live call takes the result that follows it even +// though a later assistant message separates them, and the orphan in front of +// it is dropped rather than adopted. +func TestEnsureToolResults_ForwardReachDoesNotReopenTheBackwardOne(t *testing.T) { + in := []llm.Message{ + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "STALE ORPHAN"}, + {Role: llm.RoleAssistant, ToolCalls: []llm.ToolCall{{ID: "call_0_read_file", Name: "read_file"}}}, + {Role: llm.RoleAssistant, Content: "thinking out loud"}, + {Role: llm.RoleTool, ToolCallID: "call_0_read_file", Name: "read_file", Content: "FRESH RESULT"}, + } + got := roleAndContent(ensureToolResults(in)) + want := []string{"assistant:", "tool:FRESH RESULT", "assistant:thinking out loud"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("binding picked the wrong result:\n got %q\nwant %q", got, want) + } +} + // The repair applies to what is sent, so the history it reads — which is what // gets persisted — must come back untouched. func TestEnsureToolResults_DoesNotMutateItsInput(t *testing.T) { From 8b1e127bf31b095f24a8ae34786369ef938a377d Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:38:14 +0700 Subject: [PATCH 23/28] Ask whether to stop repeating on every turn, not only when nudging 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 --- internal/agent/harness.go | 15 +++++ internal/agent/repeat_abort_test.go | 99 +++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 internal/agent/repeat_abort_test.go diff --git a/internal/agent/harness.go b/internal/agent/harness.go index 3f82642..030f84a 100644 --- a/internal/agent/harness.go +++ b/internal/agent/harness.go @@ -126,6 +126,21 @@ func processObservation(c llm.ToolCall) bool { } } +// check records a batch of calls and reports both of the guard's answers: the +// tool names worth nudging about, and whether one of them has now repeated so +// far past the limit that nudging has demonstrably failed and the run must stop. +// +// Both come from here because they cannot be derived from each other. record +// reports a key once, on the turn its count reaches the limit, and never again +// — so a caller that only asked whether to stop when it had a name to nudge +// could never see that same key go on to reach limit*2. The abort would then +// need a second, different call to trip first, and a model stuck on one call +// would run to the turn ceiling with a single nudge and no stop. +func (r *repeatTracker) check(calls []llm.ToolCall) (stuck []string, stop bool) { + stuck = r.record(calls) + return stuck, r.exceeded() +} + // exceeded reports a call repeated far past the limit, where nudging has // already failed and the run should stop. func (r *repeatTracker) exceeded() bool { diff --git a/internal/agent/repeat_abort_test.go b/internal/agent/repeat_abort_test.go new file mode 100644 index 0000000..6c98ee8 --- /dev/null +++ b/internal/agent/repeat_abort_test.go @@ -0,0 +1,99 @@ +package agent + +import ( + "strconv" + "testing" + + "github.com/enowdev/antares/internal/llm" +) + +// The guard has two jobs: nudge a model that is repeating itself, and stop one +// that keeps repeating after being nudged. record only reports a key on the +// turn its count reaches the limit, so asking about the stop only when there is +// something to nudge about asks exactly once — on the turn the count is limit, +// which is half of what exceeded() needs. The stop could then only ever fire +// when a *second*, different call tripped after a first had already run past +// twice the limit; a model stuck on one call was never stopped at all. +func TestRepeatGuardStopsAModelStuckOnOneCall(t *testing.T) { + r := newRepeatTracker(3) + call := llm.ToolCall{Name: "read_file", Arguments: `{"path":"a.txt"}`} + + nudges, stoppedAt := 0, 0 + for i := 1; i <= 60; i++ { + stuck, stop := r.check([]llm.ToolCall{call}) + if len(stuck) > 0 { + nudges++ + } + if stop { + stoppedAt = i + break + } + } + + if stoppedAt == 0 { + t.Fatalf("60 identical calls produced %d nudge(s) and no stop at all", nudges) + } + if stoppedAt != 6 { + t.Fatalf("stopped after %d identical calls, want twice the limit of 3", stoppedAt) + } + if nudges != 1 { + t.Fatalf("nudged %d times before stopping, want the one nudge at the limit", nudges) + } +} + +// The stop must not front-run the nudge: a model gets told it is repeating +// itself, and gets the turns between the limit and twice the limit to act on +// that, before the run is taken away from it. +func TestRepeatGuardNudgesBeforeItStops(t *testing.T) { + r := newRepeatTracker(3) + call := llm.ToolCall{Name: "grep", Arguments: `{"pattern":"x"}`} + + for i := 1; i <= 5; i++ { + stuck, stop := r.check([]llm.ToolCall{call}) + if stop { + t.Fatalf("stopped after %d calls, before the nudge had a chance to work", i) + } + if i == 3 && len(stuck) != 1 { + t.Fatalf("the call at the limit did not nudge: %v", stuck) + } + if i != 3 && len(stuck) != 0 { + t.Fatalf("call %d nudged again: %v", i, stuck) + } + } + if _, stop := r.check([]llm.ToolCall{call}); !stop { + t.Fatal("six identical calls at a limit of three did not stop the run") + } +} + +// Ordinary work must survive the same number of turns. Distinct arguments are +// distinct calls, so nothing here should ever reach the limit, let alone twice +// it — and the stop is now asked on every turn, which is where a guard keyed +// too coarsely would show up as an abort in the middle of a real task. +func TestRepeatGuardLetsDistinctWorkRun(t *testing.T) { + r := newRepeatTracker(3) + for i := 0; i < 60; i++ { + stuck, stop := r.check([]llm.ToolCall{{ + Name: "edit_file", + Arguments: `{"path":"main.go","old":"line ` + strconv.Itoa(i) + `","new":"x"}`, + }}) + if stop { + t.Fatalf("distinct edits were stopped as a loop after %d calls", i+1) + } + if len(stuck) > 0 { + t.Fatalf("distinct edits were nudged as a repeat after %d calls: %v", i+1, stuck) + } + } +} + +// Polling a managed process is an observation of changing external state, and +// the guard skips it entirely. Now that the stop is evaluated every turn rather +// than only behind a nudge, a long wait must still not end the run. +func TestRepeatGuardDoesNotStopManagedProcessPolling(t *testing.T) { + r := newRepeatTracker(3) + call := llm.ToolCall{Name: "process", Arguments: `{"action":"wait","process_id":"proc_1","timeout":30}`} + for i := 0; i < 60; i++ { + if _, stop := r.check([]llm.ToolCall{call}); stop { + t.Fatalf("waiting on a managed process was stopped as a loop after %d polls", i+1) + } + } +} From c4a398680c6f8c985979399bb116fbaa9511a607 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:41:57 +0700 Subject: [PATCH 24/28] Cut a grep line on a character, not a byte 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 --- internal/tools/grep_truncation_test.go | 62 ++++++++++++++++++++++++++ internal/tools/search.go | 13 +++++- 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 internal/tools/grep_truncation_test.go diff --git a/internal/tools/grep_truncation_test.go b/internal/tools/grep_truncation_test.go new file mode 100644 index 0000000..e9227a2 --- /dev/null +++ b/internal/tools/grep_truncation_test.go @@ -0,0 +1,62 @@ +package tools + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "unicode/utf8" +) + +// grep caps every line it prints, and the cap landed on a byte offset. Nothing +// makes a source line stop at an ASCII boundary — a comment, a string literal, +// a translation table — and grep is in every toolset including minimal, so this +// is the widest path from a repository into a provider request. A cut inside a +// rune ships bytes that are not UTF-8, which JSON encoding rewrites as U+FFFD. +func TestGrepKeepsValidUTF8WhenALineIsCapped(t *testing.T) { + ws := t.TempDir() + // Three bytes per rune, so the three offsets a byte cap can land on are all + // exercised: one lands on a boundary and two land inside a rune. + for i, pad := range []string{"", "x", "xx"} { + line := pad + strings.Repeat("字", 300) + " needle" + if err := os.WriteFile(filepath.Join(ws, "cjk"+string(rune('a'+i))+".go"), []byte(line+"\n"), 0o644); err != nil { + t.Fatal(err) + } + } + + args, _ := json.Marshal(map[string]any{"pattern": "字", "path": "."}) + res := (grepTool{}).Execute(context.Background(), Input{Workspace: ws, Args: args}) + if res.IsError { + t.Fatalf("grep failed: %s", res.Content) + } + if !utf8.ValidString(res.Content) { + t.Fatalf("grep returned invalid UTF-8: %q", res.Content) + } +} + +// The cap is a character budget, so text within it survives whole however many +// bytes each character takes. At 400 bytes a 300-character line lost two thirds +// of itself while the tool reported nothing missing. +func TestGrepDoesNotCutALineInsideTheBudget(t *testing.T) { + line := strings.Repeat("字", 300) // 900 bytes, 300 characters + if got := truncateLine(line); got != line { + t.Fatalf("a %d-character line was cut to %d characters by a 400-character cap", + utf8.RuneCountInString(line), utf8.RuneCountInString(got)) + } +} + +// A line genuinely past the budget is still cut, on a rune boundary, to the +// number of characters the budget names. +func TestGrepCutsAnOverlongLineOnARuneBoundary(t *testing.T) { + got := truncateLine(strings.Repeat("字", 500)) + if !utf8.ValidString(got) { + t.Fatalf("truncateLine produced invalid UTF-8: %q", got) + } + want := strings.Repeat("字", 400) + "…" + if got != want { + t.Fatalf("truncateLine kept %d characters, want 400 and an ellipsis", + utf8.RuneCountInString(strings.TrimSuffix(got, "…"))) + } +} diff --git a/internal/tools/search.go b/internal/tools/search.go index ecfd079..0660271 100644 --- a/internal/tools/search.go +++ b/internal/tools/search.go @@ -11,6 +11,8 @@ import ( "sort" "strings" "unicode/utf8" + + "github.com/enowdev/antares/internal/textutil" ) // ---- glob ------------------------------------------------------------------- @@ -345,9 +347,16 @@ func (grepTool) Execute(ctx context.Context, in Input) Result { return Text(header + "\n" + b.String() + warn) } +// maxGrepLineChars caps one printed line. It is a character budget: a byte +// budget applied to a line of CJK, or to a comment with an accent in it, both +// keeps a third of what it promises and can cut inside a rune, and grep is in +// every toolset including minimal. +const maxGrepLineChars = 400 + func truncateLine(s string) string { - if len(s) <= 400 { + out := textutil.TruncateRunes(s, maxGrepLineChars) + if len(out) == len(s) { return s } - return s[:400] + "…" + return out + "…" } From 7c9749e3c7b91dd85b2ffed28725b1950a5d4800 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:41:57 +0700 Subject: [PATCH 25/28] Cap eight tool outputs on characters and say how many were dropped 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 --- internal/tools/web.go | 14 ++++- internal/tools/web_truncation_test.go | 76 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 internal/tools/web_truncation_test.go diff --git a/internal/tools/web.go b/internal/tools/web.go index 12207ff..5066a37 100644 --- a/internal/tools/web.go +++ b/internal/tools/web.go @@ -10,8 +10,10 @@ import ( "regexp" "strings" "time" + "unicode/utf8" "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/version" ) @@ -135,11 +137,19 @@ var htmlEntities = strings.NewReplacer( func htmlUnescape(s string) string { return htmlEntities.Replace(s) } +// truncateText caps s at n characters and names how many it dropped. Both +// halves used to be counted in bytes: the cut landed inside a rune on any page +// that was not ASCII, and the count it reported as characters was the bytes +// past the offset, which for CJK is three times the number of characters there. +// Eight model-facing outputs share it, so a page, a response body, a snippet +// and a knowledge hit all arrived the same way. func truncateText(s string, n int) string { - if len(s) <= n { + kept := textutil.TruncateRunes(s, n) + if len(kept) == len(s) { return s } - return s[:n] + fmt.Sprintf("\n\n… truncated (%d more characters)", len(s)-n) + removed := utf8.RuneCountInString(s) - utf8.RuneCountInString(kept) + return kept + fmt.Sprintf("\n\n… truncated (%d more characters)", removed) } // ---- web_search ------------------------------------------------------------- diff --git a/internal/tools/web_truncation_test.go b/internal/tools/web_truncation_test.go new file mode 100644 index 0000000..ebed4c6 --- /dev/null +++ b/internal/tools/web_truncation_test.go @@ -0,0 +1,76 @@ +package tools + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "unicode/utf8" + + "github.com/enowdev/antares/internal/config" +) + +// truncateText is the cap behind eight model-facing outputs: web_fetch's page +// text and its error body, web_search snippets, http_request bodies, knowledge +// hits, intercepted request and response bodies, and Ghidra output. It cut at a +// byte offset and then reported the bytes it dropped as characters, so a page +// of CJK came back as broken bytes under a notice off by a factor of three. +func TestTruncateTextKeepsValidUTF8AndCountsCharacters(t *testing.T) { + in := strings.Repeat("あ", 100) // 300 bytes, 100 characters + got := truncateText(in, 40) + + if !utf8.ValidString(got) { + t.Fatalf("truncateText produced invalid UTF-8: %q", got) + } + if !strings.HasPrefix(got, strings.Repeat("あ", 40)) { + t.Fatalf("truncateText kept the wrong text: %q", got) + } + if !strings.Contains(got, "truncated (60 more characters)") { + t.Fatalf("notice does not report the 60 characters dropped: %q", got) + } +} + +// The budget is a character budget, so text inside it is returned whole and +// unannotated. At a byte offset a 100-character page under a 1000-character +// budget was fine, but the same page under a 100-character budget lost 66 +// characters it had room for. +func TestTruncateTextLeavesTextInsideTheBudgetAlone(t *testing.T) { + in := strings.Repeat("あ", 100) // 300 bytes, 100 characters + if got := truncateText(in, 100); got != in { + t.Fatalf("a %d-character string was cut under a 100-character budget, keeping %d characters", + utf8.RuneCountInString(in), utf8.RuneCountInString(got)) + } +} + +// End to end through web_fetch, which is the caller that was measured: a page +// of CJK 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. +func TestWebFetchReturnsValidUTF8WithAnHonestCount(t *testing.T) { + page := strings.Repeat("あ", 100) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + fmt.Fprint(w, page) + })) + t.Cleanup(srv.Close) + + args, _ := json.Marshal(map[string]any{"url": srv.URL, "max_chars": 10}) + res := (webFetchTool{}).Execute(context.Background(), Input{ + Args: args, + Deps: &Deps{Config: &config.Config{}}, + }) + if res.IsError { + t.Fatalf("web_fetch failed: %s", res.Content) + } + if !utf8.ValidString(res.Content) { + t.Fatalf("web_fetch returned invalid UTF-8: %q", res.Content) + } + if !strings.Contains(res.Content, "truncated (90 more characters)") { + t.Fatalf("web_fetch reported the wrong number of characters dropped: %q", res.Content) + } + if !strings.Contains(res.Content, strings.Repeat("あ", 10)) { + t.Fatalf("web_fetch kept the wrong text: %q", res.Content) + } +} From 0c2a08c102718828bc03d732ec02e4a4edcc2a21 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:45:06 +0700 Subject: [PATCH 26/28] Bound Dispatch by the timeout the manifest declares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/plugin/plugin.go | 28 +++++++ internal/plugin/timeout_test.go | 144 ++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 internal/plugin/timeout_test.go diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go index 11f055a..dc4ddd2 100644 --- a/internal/plugin/plugin.go +++ b/internal/plugin/plugin.go @@ -335,6 +335,13 @@ func call(ctx context.Context, man Manifest, p Payload) (reply Reply, replied bo var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr + // Killing the plugin at its deadline does not end the wait. Run also waits + // on the goroutines copying stdout and stderr, and those cannot finish + // while anything still holds the write end of the pipe — a child the plugin + // backgrounded inherits that descriptor and keeps it open long after the + // shell that spawned it is gone. Without this, timeout_ms bounds the plugin + // process and nothing bounds Dispatch, which is the agent's own turn. + cmd.WaitDelay = pipeGrace runErr := cmd.Run() @@ -351,6 +358,17 @@ func call(ctx context.Context, man Manifest, p Payload) (reply Reply, replied bo // failureChars bounds an excerpt of what a plugin printed when quoting it back. const failureChars = 200 +// pipeGrace is how long Wait may go on waiting for the plugin's I/O pipes after +// the plugin itself is finished with — either it exited or its deadline passed +// — before they are closed out from under whatever is still holding them. +// +// It is short because the window only opens once the plugin is done: everything +// it printed was copied out well before, so cutting the wait short takes +// nothing away from what it said. Whatever was captured is still read and +// honoured; only a plugin that printed nothing usable gets a refusal +// synthesised for it. +const pipeGrace = 250 * time.Millisecond + // readReply decodes what a plugin printed. Silence is not a failure: it is how // a plugin says it has no opinion. func readReply(stdout []byte) (reply Reply, replied bool, err error) { @@ -358,6 +376,16 @@ func readReply(stdout []byte) (reply Reply, replied bool, err error) { if len(out) == 0 { return Reply{}, false, nil } + // A reply is an object. Every other JSON shape fails to unmarshal into one + // except null, which unmarshals into the zero Reply and reports no error at + // all — so a plugin printing it looked exactly like one that had answered + // and had no objection, and a gate that printed it and then failed was + // recorded as having permitted the call. A jq pipeline that matches nothing + // prints precisely that. + if out[0] != '{' { + return Reply{}, false, fmt.Errorf("returned JSON that is not an object: %s", + truncate(string(out), failureChars)) + } if err := json.Unmarshal(out, &reply); err != nil { return Reply{}, false, fmt.Errorf("returned something that is not JSON: %s", truncate(string(out), failureChars)) diff --git a/internal/plugin/timeout_test.go b/internal/plugin/timeout_test.go new file mode 100644 index 0000000..feb73b3 --- /dev/null +++ b/internal/plugin/timeout_test.go @@ -0,0 +1,144 @@ +package plugin + +import ( + "context" + "strings" + "testing" + "time" +) + +// timeout_ms is the only bound a manifest can put on a plugin, and it has to +// bound what Dispatch costs the turn. It did not: Run waits on the goroutines +// copying the child's stdout, and those cannot finish while anything still +// holds the write end of the pipe. Backgrounding a child is enough to hold it — +// the child inherits the descriptor and outlives the shell that spawned it — so +// killing the shell at the deadline frees nothing and Dispatch blocks for as +// long as the grandchild lives. +// +// The existing timeout test uses `exec sleep 5`, which replaces the shell and +// so leaves nothing behind to hold the pipe. That is the one shape of slow +// plugin this bug does not affect. +func TestPluginTimeoutBoundsDispatchWhenAChildOutlivesTheShell(t *testing.T) { + skipWithoutShell(t) + for _, tc := range []struct { + name, script string + }{ + // The shell exits at once and cleanly. Nothing times out and nothing + // fails; the grandchild alone holds Dispatch for its whole lifetime. + {"shell exits and leaves the child behind", "#!/bin/sh\ncat > /dev/null\nsleep 3 &\n"}, + // The shell is still running at the deadline and is killed, but both + // its children hold the pipe past it. + {"shell is killed with a child still running", "#!/bin/sh\ncat > /dev/null\nsleep 3 &\nsleep 5\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + writePlugin(t, root, "leaky", ` +name: leaky +command: ./run.sh +hooks: [pre_tool_call] +timeout_ms: 200 +`, tc.script) + + m := NewManager([]string{root}) + _ = m.Load() + + start := time.Now() + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + elapsed := time.Since(start) + + if elapsed > 2*time.Second { + t.Fatalf("Dispatch took %s for a plugin declaring timeout_ms: 200 — a backgrounded child "+ + "holding the stdout pipe open is not bounded by the deadline", elapsed) + } + if !reply.Deny { + t.Fatalf("a plugin that never answered was read as permission: %+v", reply) + } + if !strings.Contains(reply.Reason, "leaky") { + t.Fatalf("reason = %q, want it to name the plugin that failed", reply.Reason) + } + }) + } +} + +// The bound must not turn a captured verdict into a fabricated one. A plugin +// that prints its answer and then leaks a child has answered: the answer was on +// the wire before the deadline and is in hand when the wait is cut short. Only +// a plugin that printed nothing usable gets a synthesised refusal. +func TestPluginTimeoutHonoursAVerdictPrintedBeforeTheDeadline(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "decisive", ` +name: decisive +command: ./run.sh +hooks: [pre_tool_call] +timeout_ms: 500 +`, "#!/bin/sh\ncat > /dev/null\necho '{\"deny\":true,\"reason\":\"policy says no\"}'\nsleep 3 &\nsleep 5\n") + + m := NewManager([]string{root}) + _ = m.Load() + + start := time.Now() + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + elapsed := time.Since(start) + + if elapsed > 2*time.Second { + t.Fatalf("Dispatch took %s for a plugin declaring timeout_ms: 500", elapsed) + } + if !reply.Deny { + t.Fatalf("the refusal the plugin printed was thrown away: %+v", reply) + } + if reply.Reason != "policy says no" { + t.Fatalf("reason = %q, want the plugin's own words rather than a synthesised refusal", reply.Reason) + } +} + +// The same for an observer, where the answer is content rather than a verdict: +// what the plugin printed before the wait was cut short still stands. +func TestPluginTimeoutKeepsWhatAnObserverPrintedBeforeTheDeadline(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "annotator", ` +name: annotator +command: ./run.sh +hooks: [post_tool_call] +timeout_ms: 500 +`, "#!/bin/sh\ncat > /dev/null\necho '{\"result\":\"rewritten\"}'\nsleep 3 &\nsleep 5\n") + + m := NewManager([]string{root}) + _ = m.Load() + + start := time.Now() + reply := m.Dispatch(context.Background(), Payload{Event: PostToolCall, Tool: "terminal", Result: "original"}) + elapsed := time.Since(start) + + if elapsed > 2*time.Second { + t.Fatalf("Dispatch took %s for a plugin declaring timeout_ms: 500", elapsed) + } + if reply.Result != "rewritten" { + t.Fatalf("result = %q, want what the plugin printed before the wait was cut short", reply.Result) + } +} + +// A plugin that finishes well inside its budget must not be made to wait for it. +func TestPluginTimeoutDoesNotDelayAPromptPlugin(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "prompt", ` +name: prompt +command: ./run.sh +hooks: [pre_tool_call] +timeout_ms: 5000 +`, "#!/bin/sh\ncat > /dev/null\necho '{\"notice\":\"fine\"}'\n") + + m := NewManager([]string{root}) + _ = m.Load() + + start := time.Now() + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("a plugin that answered at once took %s", elapsed) + } + if reply.Deny || reply.Notice != "fine" { + t.Fatalf("reply = %+v, want the notice it printed", reply) + } +} From 7fc788affdbe474a89cd31c8397156d1f5da4c11 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:45:06 +0700 Subject: [PATCH 27/28] Require a plugin's reply to be an object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/plugin/reply_shape_test.go | 111 ++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 internal/plugin/reply_shape_test.go diff --git a/internal/plugin/reply_shape_test.go b/internal/plugin/reply_shape_test.go new file mode 100644 index 0000000..67661d0 --- /dev/null +++ b/internal/plugin/reply_shape_test.go @@ -0,0 +1,111 @@ +package plugin + +import ( + "context" + "strings" + "testing" +) + +// A reply is an object. Every other JSON value unmarshals into the zero Reply +// without error, which reads as a plugin that answered and had no objection — +// so a gate that printed one and then failed was recorded as having permitted +// the call. `null` is not a contrived case: it is exactly what a jq pipeline +// prints when nothing matches its filter. +func TestGateDeniesWhenAPluginPrintsANonObject(t *testing.T) { + skipWithoutShell(t) + for _, tc := range []struct { + name, printed string + }{ + {"null", "null"}, + {"a bare string", `"deny"`}, + {"a list", `[{"deny":true}]`}, + {"a number", "0"}, + {"a boolean", "false"}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + // Printing it and then failing is the shape that was measured: the + // failure alone would close the gate, and the unreadable reply is + // what stopped it from doing so. + writePlugin(t, root, "jq-ish", ` +name: jq-ish +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '"+tc.printed+"'\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("a plugin that printed %s and failed was read as permission: %+v", tc.printed, reply) + } + if !strings.Contains(reply.Reason, "jq-ish") { + t.Fatalf("reason = %q, want it to name the plugin that failed", reply.Reason) + } + }) + } +} + +// The same on a clean exit: printing something that is not a reply is not the +// same as declining to answer, which is what saying nothing at all means. +func TestGateDeniesWhenAPluginPrintsNullAndSucceeds(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "nuller", ` +name: nuller +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho null\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if !reply.Deny { + t.Fatalf("a plugin that printed null was read as an answer: %+v", reply) + } +} + +// An object is still an object however empty it is: "{}" is a plugin saying it +// has looked and has no objection, and must stay distinguishable from `null`. +func TestGateStillAcceptsAnEmptyObject(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "shrug", ` +name: shrug +command: ./run.sh +hooks: [pre_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho '{}'\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PreToolCall, Tool: "terminal"}) + if reply.Deny { + t.Fatalf("an empty object was read as no answer at all: %+v", reply) + } +} + +// An observer that prints a non-object has said nothing usable either, but a +// broken observer must not break the agent: only the gate refuses. +func TestObserverPrintingANonObjectIsIgnored(t *testing.T) { + skipWithoutShell(t) + root := t.TempDir() + writePlugin(t, root, "watcher", ` +name: watcher +command: ./run.sh +hooks: [post_tool_call] +`, "#!/bin/sh\ncat > /dev/null\necho null\nexit 1\n") + + m := NewManager([]string{root}) + _ = m.Load() + + reply := m.Dispatch(context.Background(), Payload{Event: PostToolCall, Tool: "terminal", Result: "original"}) + if reply.Deny { + t.Fatalf("a failing observer denied something: %+v", reply) + } + if reply.Result != "" { + t.Fatalf("result = %q, want nothing adopted from an unreadable reply", reply.Result) + } +} From 5df1c111bb85429f73b5a83e0cbf37bcec67d454 Mon Sep 17 00:00:00 2001 From: Jihad Irfansyah Date: Thu, 13 Aug 2026 23:46:56 +0700 Subject: [PATCH 28/28] Pin where the nudge lands, and say what the key test really covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- HANDOFF.md | 7 +- internal/agent/nudge_position_test.go | 165 ++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 internal/agent/nudge_position_test.go diff --git a/HANDOFF.md b/HANDOFF.md index 7cabaa0..fd2ede5 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -118,8 +118,11 @@ below is done and green unless called out under **Next Steps**. to one file from one edit made three times, so it fired on ordinary work, which for a coding agent is most of the work. **Do not reintroduce it.** `repeatKey` is now uniform over the full normalised arguments for every tool, and - `internal/agent/repeat_guard_test.go` fails if any tool is given a coarser key - again. The loop itself is therefore still unsolved: a model that varies the + `internal/agent/repeat_guard_test.go` fails if `write_file`, `edit_file` or + `vps_upload` is given a coarser key again. Those are the three names that ever + carried a special case, and the test names each of them; it asserts nothing + about any other tool, so a coarser key introduced for a different tool would + pass. The loop itself is therefore still unsolved: a model that varies the content each time is bounded only by the ceilings below, and any replacement needs a signal other than the call fingerprint. This is what produced the giant turn that caused the OOM in item 4 — the frontend is now hardened, but the loop diff --git a/internal/agent/nudge_position_test.go b/internal/agent/nudge_position_test.go new file mode 100644 index 0000000..bd18c54 --- /dev/null +++ b/internal/agent/nudge_position_test.go @@ -0,0 +1,165 @@ +package agent + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) + +// The repetition nudge has to reach history after the tool results, not before. +// appendTurnMessages is pinned for that (turn_messages_test.go), but Run is +// free to ignore it: appending the nudge straight to history above executeTools +// rebuilds the invalid transcript and leaves the whole suite green, because +// ensureToolResults silently repairs the shape at send time. That silence is +// why the malformation went unnoticed in the first place. +// +// Driving Run itself would need a fake provider, so these read the call site +// instead. They are narrower than a behavioural test — a nudge written inline +// as a fresh string literal would pass — but they close the specific +// regression: reintroducing the append that used to be there fails here. + +// runBody returns the parsed body of Run along with the file's position table. +func runBody(t *testing.T) (*token.FileSet, *ast.BlockStmt) { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "agent.go", nil, 0) + if err != nil { + t.Fatalf("parse agent.go: %v", err) + } + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if ok && fn.Name.Name == "Run" && fn.Recv != nil && fn.Body != nil { + return fset, fn.Body + } + } + t.Fatal("Run is no longer a method on Agent in agent.go; this guard needs updating") + return nil, nil +} + +// appendsToHistoryBetween reports the positions of every `append(history, …)` +// falling in (from, to). +func appendsToHistoryBetween(body *ast.BlockStmt, from, to token.Pos) []token.Pos { + var found []token.Pos + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + fn, ok := call.Fun.(*ast.Ident) + if !ok || fn.Name != "append" || len(call.Args) == 0 { + return true + } + target, ok := call.Args[0].(*ast.Ident) + if !ok || target.Name != "history" { + return true + } + if call.Pos() > from && call.Pos() < to { + found = append(found, call.Pos()) + } + return true + }) + return found +} + +// callPos returns the position of the first call to the named function, which +// may be a bare name or a selector's method name. +func callPos(body *ast.BlockStmt, name string) token.Pos { + var at token.Pos + ast.Inspect(body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || at.IsValid() { + return true + } + switch fn := call.Fun.(type) { + case *ast.Ident: + if fn.Name == name { + at = call.Pos() + } + case *ast.SelectorExpr: + if fn.Sel.Name == name { + at = call.Pos() + } + } + return true + }) + return at +} + +// Between deciding what the repetition guard has to say and running the tools, +// nothing may go into history at all. That window is where the nudge used to +// land, and any message put there sits between an assistant's tool_calls and +// their results. +func TestRunAppendsNothingToHistoryBetweenTheRepeatCheckAndTheTools(t *testing.T) { + fset, body := runBody(t) + + check := callPos(body, "check") + if !check.IsValid() { + t.Fatal("Run no longer asks the repeat tracker anything; this guard needs updating") + } + tools := callPos(body, "executeTools") + if !tools.IsValid() { + t.Fatal("Run no longer calls executeTools; this guard needs updating") + } + + for _, at := range appendsToHistoryBetween(body, check, tools) { + t.Errorf("%s: a message is appended to history between the repetition check and executeTools, "+ + "which puts it between the assistant's tool_calls and their results", + fset.Position(at)) + } +} + +// And the nudge itself reaches history only by being handed to +// appendTurnMessages, which puts it after the results. +func TestRunPassesTheNudgeOnlyToAppendTurnMessages(t *testing.T) { + fset, body := runBody(t) + + const nudge = "repeatNudge" + allowed := map[token.Pos]bool{} + handedOver := false + + ast.Inspect(body, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.CallExpr: + fn, ok := node.Fun.(*ast.Ident) + if !ok || fn.Name != "appendTurnMessages" { + return true + } + for _, arg := range node.Args { + if id, ok := arg.(*ast.Ident); ok && id.Name == nudge { + allowed[id.Pos()] = true + handedOver = true + } + } + case *ast.AssignStmt: + for _, lhs := range node.Lhs { + if id, ok := lhs.(*ast.Ident); ok && id.Name == nudge { + allowed[id.Pos()] = true + } + } + case *ast.ValueSpec: + for _, id := range node.Names { + if id.Name == nudge { + allowed[id.Pos()] = true + } + } + } + return true + }) + + if !handedOver { + t.Fatalf("Run never hands %s to appendTurnMessages; either the nudge is gone or it now "+ + "reaches history another way, and this guard needs updating", nudge) + } + + ast.Inspect(body, func(n ast.Node) bool { + id, ok := n.(*ast.Ident) + if !ok || id.Name != nudge || allowed[id.Pos()] { + return true + } + t.Errorf("%s: %s is used somewhere other than its assignment and appendTurnMessages, "+ + "which is the only path that puts it after the tool results", + fset.Position(id.Pos()), nudge) + return true + }) +}