coilyco-gaming/sirens-echo

Watch

feat(tools): break a failed tool for the rest of its turn #1007

Merged

coilysiren merged 2 commits from aos/claude/tool-breaker into main

2026-08-19 01:49:23 +00:00

coilyco-ops commented

2026-08-19 01:37:56 +00:00

Member

Closes #943. Implements the breaker that issue specified, and settles the question it left open.

The question #943 could not answer from telemetry

It measured configured: 12, listed: 0, reached: 0, cached: true on 21 of 25 listings and asked whether that is "a cache that stores an empty roster and serves it, or a counter that is not written on the cached path."

Neither. reached counts servers that took a round trip, and it is legitimately zero in two unrelated states: every server warm, and every server backing off. mcp.tools.cached is derived from reached == 0, so it reads true for both. On the warm path Open registers from entry.tools and the model gets the full roster. That reading matches the one the engineer seat posted on #939, and I confirmed it against the code rather than inheriting it.

So the 21 spans are healthy warm turns, not a collapse. The instrument was the defect.

Fixed by emitting mcp.tools.unavailable and mcp.tools.registered beside reached. cached is kept rather than replaced, so existing queries do not silently break. A warm roster now reads unavailable: 0, registered: 86; a dead one reads unavailable: 12, registered: 0. A test builds both states against a real in-process MCP server and asserts they differ.

Still unexplained, and I am not claiming otherwise: rounds 9 and 11 of trace 6d1efdd560dd868db1fdbea1394cd3cd offered 0 tools while the turn continued. Both states above are accounted for and neither is that one, so #943's first finding survives this change with better instruments pointed at it. Whoever picks it up next will be able to read registered directly instead of inferring from cached.

The breaker

Per tool, per turn, on the session, which is already per-turn. Once a tool returns a result with IsError, the next call to that same tool returns the recorded error with a line saying the call was skipped.

Told, never dropped. #943 is explicit and it is the right call: this turn's reply was good, it named the outage and answered the answerable half. It could only do that because it knew. A silently missing tool reads as one that might have worked.
Per tool, so one dead server mutes only itself. Not a round cap, not a roster reduction. #940 records Kai declining both, and neither is needed.
First failure only. A later error would describe a call that never ran.
The replay keeps outcomeOf at failed, so the disclosure footer, the metric, and mcp.tool.outcome all stay consistent with what the model was told.

A transport error needs no breaker: it ends the turn, so it cannot repeat.

Verification

just gate passes: build, policy-check, vet, test, test-skips, pre-commit. go test -race ./... clean, which matters because the breaker adds mutable per-turn state.

Five new tests. Non-vacuity checked by reverting the breaker rather than assumed, and the fixture is built so the proof is unambiguous: the registered tool has a nil session, so a dispatched call panics.

--- FAIL: TestASecondCallToAFailedToolIsNotMade
panic: runtime error: invalid memory address or nil pointer dereference

With the breaker in place it returns cleanly. Surviving the call is the evidence no call was made.

Not done, deliberately

The across-turn half. #943 asks for a server with a standing failure record to be marked unavailable rather than called, and that is what stops Deep rediscovering the playwright outage every turn. I did not build it, because it is a bigger design than it looks: the roster caches entry.tools from a previous successful listing, so a tool stays offered after its session dies, and fixing that properly means deciding when a server is re-probed and how a recovery is noticed. That is not a change I would make 15 hours before a live recording.

The within-turn breaker takes the observed cost from six dead calls to one. The across-turn one takes it from one per turn to zero. The first is most of the win and all of the safety, so I would rather land it and leave the second specified than half-build both.

Doc note, same as #1003 and #1001

docs/sirens-echo-tools.md was at 7993 of its 8000-char cap, so the breaker gets one sentence and the reasoning lives here. I compressed three sentences elsewhere on the page to fit it, without dropping a fact. That is the fourth page in a row I have hit this on; sirens-echo-mcp.md is at exactly 8000. Worth a band decision rather than more shaving.

Closes #943. Implements the breaker that issue specified, and settles the question it left open. ## The question #943 could not answer from telemetry It measured `configured: 12, listed: 0, reached: 0, cached: true` on 21 of 25 listings and asked whether that is "a cache that stores an empty roster and serves it, or a counter that is not written on the cached path." **Neither.** `reached` counts servers that took a round trip, and it is legitimately zero in two unrelated states: every server warm, and every server backing off. `mcp.tools.cached` is derived from `reached == 0`, so it reads `true` for both. On the warm path `Open` registers from `entry.tools` and the model gets the full roster. That reading matches the one the engineer seat posted on #939, and I confirmed it against the code rather than inheriting it. So the 21 spans are **healthy warm turns**, not a collapse. The instrument was the defect. Fixed by emitting `mcp.tools.unavailable` and `mcp.tools.registered` beside `reached`. `cached` is kept rather than replaced, so existing queries do not silently break. A warm roster now reads `unavailable: 0, registered: 86`; a dead one reads `unavailable: 12, registered: 0`. **A test builds both states against a real in-process MCP server** and asserts they differ. **Still unexplained, and I am not claiming otherwise:** rounds 9 and 11 of trace `6d1efdd560dd868db1fdbea1394cd3cd` offered 0 tools while the turn continued. Both states above are accounted for and neither is that one, so #943's first finding survives this change with better instruments pointed at it. Whoever picks it up next will be able to read `registered` directly instead of inferring from `cached`. ## The breaker Per tool, per turn, on the session, which is already per-turn. Once a tool returns a result with `IsError`, the next call to that same tool returns the recorded error with a line saying the call was skipped. * **Told, never dropped.** #943 is explicit and it is the right call: this turn's reply was *good*, it named the outage and answered the answerable half. It could only do that because it knew. A silently missing tool reads as one that might have worked. * **Per tool, so one dead server mutes only itself.** Not a round cap, not a roster reduction. #940 records Kai declining both, and neither is needed. * **First failure only.** A later error would describe a call that never ran. * The replay keeps `outcomeOf` at `failed`, so the disclosure footer, the metric, and `mcp.tool.outcome` all stay consistent with what the model was told. A transport error needs no breaker: it ends the turn, so it cannot repeat. ## Verification `just gate` passes: build, policy-check, vet, test, test-skips, pre-commit. `go test -race ./...` clean, which matters because the breaker adds mutable per-turn state. Five new tests. **Non-vacuity checked by reverting the breaker rather than assumed**, and the fixture is built so the proof is unambiguous: the registered tool has a nil session, so a dispatched call panics. ``` --- FAIL: TestASecondCallToAFailedToolIsNotMade panic: runtime error: invalid memory address or nil pointer dereference ``` With the breaker in place it returns cleanly. Surviving the call *is* the evidence no call was made. ## Not done, deliberately **The across-turn half.** #943 asks for a server with a standing failure record to be marked unavailable rather than called, and that is what stops Deep rediscovering the playwright outage every turn. I did not build it, because it is a bigger design than it looks: the roster caches `entry.tools` from a previous successful listing, so a tool stays *offered* after its session dies, and fixing that properly means deciding when a server is re-probed and how a recovery is noticed. That is not a change I would make 15 hours before a live recording. The within-turn breaker takes the observed cost from six dead calls to one. The across-turn one takes it from one per turn to zero. **The first is most of the win and all of the safety**, so I would rather land it and leave the second specified than half-build both. ## Doc note, same as #1003 and #1001 `docs/sirens-echo-tools.md` was at **7993 of its 8000-char cap**, so the breaker gets one sentence and the reasoning lives here. I compressed three sentences elsewhere on the page to fit it, without dropping a fact. That is the fourth page in a row I have hit this on; `sirens-echo-mcp.md` is at exactly 8000. Worth a band decision rather than more shaving.

coilyco-ops added 1 commit

2026-08-19 01:37:56 +00:00

feat(tools): break a failed tool for the rest of its turn

ci / image-build (pull_request) Successful in 42s

Details

ci / test (pull_request) Failing after 1m0s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

b3e57f542a

playwright__browser_navigate was called six times in one turn, twice per
round from round 7 on, with byte-identical 27,867-byte requests, and
returned the same closed-session error every time. The answer was ready
at 39 seconds and the turn took 164.

A tool that reports its own failure is now recorded on the session, which
is per turn, and the next call to that same tool answers from the record
saying it was not made. The model is told rather than denied, because a
tool that vanishes reads as one that might have worked, and this turn
already showed the model does the right thing when it knows: it named the
outage and answered the half that needed no tool.

Per tool rather than per turn, so a dead server mutes only itself. Not a
round cap and not a roster reduction, both of which Kai declined on #940.

The listing span also carries unavailable and registered. mcp.tools.cached
is true both for a warm roster and for one where every server is backing
off, which is what made 21 of 25 listings read as a cache serving zero
tools. It is neither, and now says which.

Closes #943.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-19 01:37:56 +00:00

coilyco-ops referenced this pull request

2026-08-19 01:38:39 +00:00

The docs band is saturated: four pages at their cap, and no room for a 41st page either #1009

coilyco-ops added 1 commit

2026-08-19 01:45:47 +00:00

fix(tools): key the breaker on the arguments, not just the tool

ci / image-build (pull_request) Successful in 35s

Details

ci / test (pull_request) Successful in 1m6s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

b249cb10cb

Reviewing #940 turned up the cost of the per-tool form: a tool that fails
on bad arguments is correctable, and blocking every later call to it would
refuse the corrected retry. forgejo list_issue rejecting a query is a
different thing from playwright holding a closed session, and only the
second is worth refusing.

The observed defect supports the narrower key. Rounds 9 and 11 of the
trace in #943 were byte-identical 27,867-byte requests, so keying on the
arguments blocks every repeat it recorded while leaving a model that fixes
its own mistake able to run.

This diverges from #943's literal acceptance, which asks that no second
call be made to a tool that already failed. Stated on the issue rather
than quietly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops referenced this pull request

2026-08-19 01:46:04 +00:00

Tool surface drops from 86 tools to 0 mid-turn, and a dead tool is re-called six times in one turn with no breaker #943