diff --git a/HANDOFF.md b/HANDOFF.md index 8c2d9fe..fd2ede5 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -108,13 +108,31 @@ 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 `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 + 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/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. 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/agent.go b/internal/agent/agent.go index 1f43da8..c382b5a 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" ) @@ -609,25 +610,28 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error continue } - 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 - } + // 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 + 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, ", ")}) - 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) 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, @@ -646,13 +650,12 @@ func (a *Agent) Run(ctx context.Context, req Request, emit Emit) (*Result, error // 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 { @@ -738,6 +741,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, @@ -915,7 +942,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) } @@ -1137,13 +1164,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 @@ -1166,16 +1194,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 { @@ -1189,58 +1218,120 @@ 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 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 +// 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 { - 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. + // A call id is only unique within a turn — Gemini synthesises + // "call__" when it omits one, so the same id recurs across + // 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 { - continue + byID[m.ToolCallID] = append(byID[m.ToolCallID], i) } - out = append(out, m) - if m.Role != llm.RoleAssistant || len(m.ToolCalls) == 0 { + } + + // 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 } - ids := make(map[string]bool, len(m.ToolCalls)) - for _, tc := range m.ToolCalls { - ids[tc.ID] = true + if n := len(turns); n > 0 { + turns[n-1].end = i } - // 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) + 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] { + // 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 + t.bound[j] = ri + break } - 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]", - }) + } + for i := range turns { + bind(&turns[i], true) + } + // 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) + } + + 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 + // 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 { + continue + } + 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{ + 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/approval.go b/internal/agent/approval.go index 92e3663..ef031ec 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,26 @@ 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. +// +// 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 }{ - {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"}, @@ -194,19 +209,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/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/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) + } +} diff --git a/internal/agent/danger_test.go b/internal/agent/danger_test.go new file mode 100644 index 0000000..7115a54 --- /dev/null +++ b/internal/agent/danger_test.go @@ -0,0 +1,121 @@ +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) + } +} + +// 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) { + 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/harness.go b/internal/agent/harness.go index 94b4803..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 { @@ -151,28 +166,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_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) + } + } +} diff --git a/internal/agent/harness_test.go b/internal/agent/harness_test.go index 6d2fc1f..2c2b7f5 100644 --- a/internal/agent/harness_test.go +++ b/internal/agent/harness_test.go @@ -132,44 +132,3 @@ func TestNormaliseArgsToleratesGarbage(t *testing.T) { t.Fatalf("got %q", got) } } - -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"}`}}) - 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/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 + }) +} diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 6c55c7f..d9bef39 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" ) @@ -232,16 +233,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/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) + } + } +} diff --git a/internal/agent/repeat_guard_test.go b/internal/agent/repeat_guard_test.go new file mode 100644 index 0000000..710a803 --- /dev/null +++ b/internal/agent/repeat_guard_test.go @@ -0,0 +1,77 @@ +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") + } +} + +// 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"}`} + 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") + } +} diff --git a/internal/agent/toolresults_test.go b/internal/agent/toolresults_test.go index 2daabb6..03cddc1 100644 --- a/internal/agent/toolresults_test.go +++ b/internal/agent/toolresults_test.go @@ -1,6 +1,9 @@ package agent import ( + "encoding/json" + "reflect" + "strings" "testing" "github.com/enowdev/antares/internal/llm" @@ -16,17 +19,31 @@ func toolIDs(msgs []llm.Message) []string { return ids } -// countStubs reports how many tool messages carry the interrupted-stub marker. +// 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 recorded — the previous run was interrupted before this tool finished]" { + 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"}, @@ -76,3 +93,277 @@ 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") + } + // 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) + } +} + +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"}, + } + 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 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) { + 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"}, + } + // 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) + } +} 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) + } +} 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/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..390e0be 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 @@ -481,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 @@ -498,12 +517,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 +536,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/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/openai.go b/internal/llm/openai.go index 410e329..4a9ffc8 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,22 @@ 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. 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 := 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..febc033 --- /dev/null +++ b/internal/llm/stream_framing_test.go @@ -0,0 +1,546 @@ +package llm + +import ( + "context" + "errors" + "fmt" + "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 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 + }{ + { + // 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) + } + assertNoPartialAnswer(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) + } + assertNoPartialAnswer(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) + } + }) +} + +// 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. +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) + } +} + +// 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() + 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) + } + + 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) + } +} diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 2c173f8..a15316d 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -20,6 +20,7 @@ import ( "sync" "time" + "github.com/enowdev/antares/internal/textutil" "github.com/enowdev/antares/internal/version" ) @@ -40,6 +41,129 @@ 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. +// +// 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"` +} + +// 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" + } + return fmt.Sprintf("%s (%s)", uri, mimeOrUnknown(r.MimeType)) +} + +// render returns the text this resource contributes, which is empty for an +// 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 && *r.Text != "": + 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) { + kind = textutil.TruncateRunes(kind, contentKindChars) + if seen[kind] { + return + } + seen[kind] = true + if len(skipped) >= maxContentKinds { + unnamed++ + return + } + skipped = append(skipped, kind) + } + + for _, item := range items { + switch item.Type { + case "text": + b.WriteString(item.Text) + b.WriteString("\n") + case "image": + // 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 + } + 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 + } + 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 } // CallResult is the outcome of calling an MCP tool. @@ -257,20 +381,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 @@ -322,23 +441,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()) @@ -630,36 +748,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/client_test.go b/internal/mcp/client_test.go index 4e4239a..da67351 100644 --- a/internal/mcp/client_test.go +++ b/internal/mcp/client_test.go @@ -170,21 +170,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") @@ -391,6 +396,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, @@ -402,6 +414,31 @@ 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 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 { + 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..802d519 --- /dev/null +++ b/internal/mcp/content_test.go @@ -0,0 +1,329 @@ +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). +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 +} + +// 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 +// 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 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 + }{ + { + 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 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]", + }, + { + // 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":""}}]}`, + 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: cannotRepresent("image with no data"), + }, + { + name: "audio only", + raw: `{"content":[{"type":"audio","data":"QUJDRA==","mimeType":"audio/wav"}]}`, + wantErr: cannotRepresent("audio"), + }, + { + name: "unknown type only", + raw: `{"content":[{"type":"hologram","data":"QUJDRA=="}]}`, + wantErr: cannotRepresent("hologram"), + }, + { + name: "resource with no payload key", + raw: `{"content":[{"type":"resource"}]}`, + wantErr: cannotRepresent("resource with no text or blob"), + }, + { + name: "resource with an empty payload object", + raw: `{"content":[{"type":"resource","resource":{"uri":"file:///a"}}]}`, + 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: cannotRepresent("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":[]}`, + 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 %q", res, tc.wantErr) + } + if err.Error() != tc.wantErr { + t.Fatalf("error = %q, want %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) + } + }) + } +} + +// 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") + } + // 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) + } +} + +// 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") + } +} + +// 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)", + }, + { + 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 { + 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("no-resources")) + defer manager.Close() + + 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 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{ + 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 < 30; 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") + } + } +} 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) + } +} 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 540ef3a..dc4ddd2 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" ) @@ -231,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)) @@ -242,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) @@ -282,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 @@ -302,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...) @@ -316,32 +335,84 @@ func call(ctx context.Context, man Manifest, p Payload) (Reply, error) { var stdout, stderr bytes.Buffer 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) + // 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() + + // 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 +} + +// failureChars bounds an excerpt of what a plugin printed when quoting it back. +const failureChars = 200 - out := bytes.TrimSpace(stdout.Bytes()) +// 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) { + 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 + } + // 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, 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 { - if len(s) <= max { + out := textutil.TruncateRunes(s, max) + if out == s { return s } - return s[:max] + "…" + return out + "…" } 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) + } +} 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) + } +} 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/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) + } +} 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) + } +} diff --git a/internal/tools/browser.go b/internal/tools/browser.go index c35ea06..d7e2584 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 @@ -133,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"` @@ -381,11 +387,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/capability_test.go b/internal/tools/capability_test.go new file mode 100644 index 0000000..1226835 --- /dev/null +++ b/internal/tools/capability_test.go @@ -0,0 +1,81 @@ +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") + } +} + +// 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) { + 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/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/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/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..4788a2e 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,48 @@ 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. 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) { + var decoded struct { + Command string `json:"command"` + } + in := Input{Args: args} + if in.Bind(&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/search.go b/internal/tools/search.go index 6c4c082..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 ------------------------------------------------------------------- @@ -155,6 +157,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 +225,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 +316,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 +326,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: ") @@ -331,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 + "…" } diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 8df2f54..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" @@ -17,6 +18,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 @@ -593,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"` @@ -662,16 +670,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) + } +} 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..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" ) @@ -33,6 +35,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"` @@ -132,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 ------------------------------------------------------------- @@ -154,6 +167,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"` 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) + } +}