fix: MCP error propagation, dedupe replay policy, and tool-trace persistence#51
Merged
Merged
Conversation
A failed shell-tool exec previously survived as ordinary "error: ..." text with no isError signal, so it was indistinguishable from success by the time it reached the runner's dedupe map — which then blocked the model's retry. This silently dropped an IP-ban deployment for 6+ hours on 2026-07-06. - shell-mcp: set isError: true on tool execution failure (error: prefix kept for back-compat with clients/prompts that key off it). - mcp client: parse isError; Call returns a typed *ToolError instead of a plain string when the server reports a tool failure. - executor: a ToolError is deterministic (the call delivered; the tool failed) — execMCPWithRetry stops immediately instead of burning retry attempts, and does not enqueue to the outbound DLQ. - Execute already maps any non-nil error to ToolResult.Error, which the runner's existing dedupe-insert guard (result.Error == "") correctly skips — added a runner-level regression test pinning this exact chain end to end (no production change needed at the runner layer; that's plan 04). - docs/GUIDE.md: documents the error contract.
Adds a ToolTrace struct to Turn/RunRecord so .state/runs/*.jsonl records capture name/args/content/error/duration/replayed per tool call, instead of losing that detail once folded into turn text. Gated by a new persist_runs_detail: none|tools config key (default none, byte-identical to legacy output since Turn.ToolCalls stays nil and omitempty drops the field). Args and result content are secret-redacted (new internal/secret/redact.go, reusing the devtools/bus key-based redaction discipline plus a Bearer/Authorization pattern scrub) and capped per field at persist_runs_tool_cap bytes (default 2048). Wired through cmd_run, cmd_serve, and the curing/tannery RunnerDeps path so scheduled tannery/ban-cycle agents get the same detail. Motivated by a 2026-07-06 incident where a silently-dropped tool failure required reverse-engineering four source files to reconstruct what a tool call actually returned.
…an 04)
Depends on plan 03: with error propagation in place, only genuinely
successful calls enter the dedupe map. This addresses what happens on a
legitimate repeat:
- Dedupe now replays the cached ToolResult ("[replay: identical call
completed earlier this run; result repeated below]") instead of asserting
"already completed successfully" with no content — the model works from
real observed data instead of narrating success it never verified. Log
line changed from a WARN "skipping duplicate..." to an INFO "replaying
duplicate tool call result" — it's expected, policy-governed behavior now,
not a warning.
- ToolDefinition.MaxRepeats (skill YAML: max_repeats, minimum -1) controls
how many successful executions of an identical call are permitted before
further identical calls replay: unset/0 = dedupe-on (1 then replay), N =
N executions then replay, -1 = dedupe disabled entirely. Failed calls never
count against the budget, so a failing call's retry always executes.
- Fixes the production case: a zero-arg tool (e.g. deploy-bans) has a
constant dedupe key, so a second, semantically distinct call after
world-state changed mid-run (a fresh ban plan regenerated between two
deploy calls) was silently dropped as a "duplicate". max_repeats gives such
tools headroom; GUIDE.md recommends restructuring (one job per agent) over
raising it where practical.
- max_repeats lives in skill YAML only (not shell-tools.json): dedupe is a
leather-runner concern, not something the MCP server needs to know about.
docs/GUIDE.md documents the policy table and the error-contract cross-link.
…ol-trace Both branches touch the tool-execution block in internal/runner/runner.go (~L530-620). Reconciled by keeping both additions side by side: the dedupe-policy's effectiveMax/MaxRepeats calculation and the tool-trace's replayed/execStart timing setup are independent locals that both feed the same switch statement below them.
…policy, and tool-trace persistence
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three fixes/features from a tannery incident postmortem, where a failed
deploy-bansMCP tool call was silently treated as a success and blocked its own retry, dropping IP ban deployments for 6+ hours undetected.shell-mcpsetsisError: trueon exec failure;internal/mcp.Callreturns a typed*ToolError; the executor stops retrying deterministic tool errors; the runner's dedupe map is never populated by a failed call. Includes a regression test pinning the exact 2026-07-06 failure end-to-end.max_repeats(skill yaml) so zero-arg write tools aren't permanently blocked from re-running within a run when world state changes mid-run..state/runs/*.jsonlgains an opt-intool_callsfield (name, redacted args, capped content, error, replay flag, duration) viapersist_runs_detail: none|tools(defaultnone, byte-identical legacy output).Review fixes
ToolErrorhandling now returns the clean server-provided error text instead of the executor wrapper.isError: trueis preserved even when a non-conforming server returns empty/no text content.internal/secret.warn.max_repeatsmodel docs now describe it as a narrow stopgap for stateful side-effect workflows.Test plan
env GOCACHE=/tmp/leather-go-build-cache go test ./...env GOCACHE=/tmp/leather-go-build-cache make checkisError, and UTF-8-safe trace caps.fix/mcp-error-and-dedupe-and-tool-trace.