fix(mcp): avoid replaying tools after ambiguous call failures - #4124

Description

A remote MCP tool can commit a write and then lose its response, return a gateway error, or report an RPC failure. The agent currently disconnects and calls the tool again after any non-stdio call error. In a local HTTP reproduction, one approved create_order invocation executes the server handler twice and is reported as successful after the second call.

This patch limits that automatic replay to pre-execution rejection signals: ErrNotConnected, the SDK's typed expired-session/HTTP 404 error, and typed authorization/reauthorization failures. The agent's reconnect recovery remains bounded to one retry and does not start after caller cancellation. Other attempted-call failures retain their cause and carry an unknown-outcome error with advice to verify remote state before retrying. The agent renders this as a tool-call failure, rather than a failed connection. OAuth preflight, token refresh and connection-based authorization recovery retain their behavior.

Tests exercise an actual SDK-backed HTTP server, discovery/description, approval, call_mcp_tool, and the model-context renderer. They inject 502, EOF and RPC errors after executing the handler, plus cancellation, expired sessions, ordinary tool errors and successful calls. Separately approved later calls remain possible. The MCP guide explains the verification/retry behavior.

Performance and compatibility

• No dependency, database, configuration or JSON response-schema change. The new error sentinel and retry predicate are internal package contracts, and wrapped error causes remain inspectable.
• Successful calls, tool-level isError results and OAuth's existing one-time resource-401 refresh are preserved. Typed expired-session and authorization recovery remain available.
• Generic transport/RPC failures, including failures of read-only tools, no longer cause an automatic replay: WeKnora cannot infer that an opaque remote operation is safe to repeat. There is no new idempotency guarantee or retry setting.
• No performance benchmark or production task-success improvement is claimed. The observed two-to-one execution difference is limited to the injected failure fixtures.
• MCP session management requires reinitialization after an expired session returns 404. The same specification distinguishes a lost connection from cancellation of an already submitted request.

Type of Change

• 🐛 Bug fix
• ✨ New feature
• 💥 Breaking change
• 📚 Documentation update
• 🎨 Refactor
• ⚡ Performance improvement
• 🧪 Test
• 🔧 Configuration / Build / CI

Related Issue

N/A — reproduced on current main while checking the MCP execution path. This retains the session recovery introduced by #3765. It is independent of #4074 (structured result preservation) and #4095 (protocol negotiation).

Testing

Branch point and reproduction baseline: 097bd475f; tested head: 4f597f14c. Ubuntu/WSL, Go 1.26.0, golangci-lint 2.12.2, CGO and SQLite development headers. Local protocol/approval tests use HTTP fixture servers and a fake approval policy, with no live model, external credentials or production writes. Resource-limited test/build commands use GOMAXPROCS=2, GOFLAGS=-p=1 and strip debug symbols (-ldflags='-w -s'); this does not replace the normal GitHub App workflow.

• Before fix, with only mcp_call_retry_test.go copied into an isolated base worktree: go test -ldflags='-w -s' ./internal/agent/tools -run '^TestMCPCallRetryDoesNotRepeatAmbiguousExecution$' -count=1 — four scenarios fail: three log requests=2, server executions=2, approvals=1, and cancellation lacks the model-visible unknown-outcome explanation.
• go test -ldflags='-w -s' ./internal/mcp ./internal/agent/tools ./internal/agent -count=1 — passed.
• go test -race -ldflags='-w -s' ./internal/mcp ./internal/agent/tools -run 'TestMCPCallRetry|TestIsToolCallRetrySafe|TestCallToolOAuthRefreshFailure|TestOAuthCall' -count=1 — passed.
• Full app go vet (all root packages except /docreader/) — passed.
• Full app go test -ldflags='-w -s' (the same package list) — passed.
• go test -ldflags='-w -s' -tags desktop ./internal/container — passed.
• go build -ldflags='-w -s' ./cmd/server — passed (output written outside the worktree).
• golangci-lint run --concurrency=1 --timeout=10m --new-from-merge-base=097bd475f ./... — passed, 0 issues. It emits an existing malformed nolint warning in the unchanged OpenSearch test file.
• Changed-file gofmt -l and git diff --check origin/main...HEAD — passed.
• License source/package checks, generated model catalog and Git-hook checks — passed.
• node scripts/check-links.mjs and node scripts/check-mermaid.mjs in website-docs/ — passed: 319 links and 60 existing diagrams checked.

Checklist

• git diff --check origin/main...HEAD passes
• Changed source files are formatted
• Targeted tests for the changed packages/components pass
• Diff-scoped lint passes where applicable
• Full-repository checks were run, or any unrelated/environment-dependent failures are documented above
• Self-reviewed the code
• Added/updated tests covering the change
• Updated related documentation
• Breaking changes are clearly called out (the retry behavior change is described above; no API schema change)

Screenshots / Recordings

Not applicable; no UI layout or interaction implementation changes. Tests verify approval counts and the exact result supplied to the model.