Skip to content

feat(provider): recover a leaked tool call instead of cutting the turn - #1835

Open
Kiraprint wants to merge 5 commits into
Agent-Field:devfrom
Kiraprint:fix/recover-machinery-tool-calls
Open

Kiraprint wants to merge 5 commits into
Agent-Field:devfrom
Kiraprint:fix/recover-machinery-tool-calls

Conversation

@Kiraprint

Copy link
Copy Markdown

What this does

A model served by an endpoint that does not translate its private tool-calling syntax writes the call as visible text — angle brackets, bars, a tool name, a run of JSON. MachineryLeak already convicts that reply, and the harness throws it away: drop the turn, ask once more, reroute the lane, and on a single-model deployment that is the end of the work.

The complaint in #1706 is that this is treated purely as a failure. But the markup is very often a perfectly well-formed tool call the endpoint simply failed to parse. So machineryCut now tries to read the call back out before it cuts.

A reply whose markup holds a call that

  1. names a tool the request actually advertised,
  2. carries arguments that parse as a JSON object, and
  3. is the reply, rather than a quotation inside it,

has that call put on the response and the turn goes on to dispatch it — indistinguishable from a reply that arrived parsed, at every layer below. That is why no lane, retry or accounting is threaded through: the recovery returns nil and the existing epilogue reads an ordinary answered turn. Only a leak that cannot be read that way is still cut, exactly as before.

The refusals are the interesting half

Each is load-bearing, and each is pinned by a test:

  • a name the request did not advertise is prose about a call, not a call. A reply smuggling a second, unoffered tool beside a real one is refused outright — this is the one that keeps recovery from becoming an escalation path.
  • arguments that do not parse are refused rather than guessed at, with one deliberate exception: a named parameter whose value arrives unencoded (ls, 17, false) is kept as text, because that is what the tool is owed.
  • a long tail of prose after the markup is somebody describing tool calls — the one answer a recovery must never destroy.

The measurement that made rule 3 real

An earlier draft of this change assumed the guard around recovery was enough: that a reply merely quoting an invocation is not symbol-dense enough for MachineryLeak to convict. It is not — the markup itself carries the density, and a test caught the assumption.

What separates the two shapes is proportion:

reply prose outside the markup
captured leak 1 0.25
captured leak 2 0.29
captured leak 3 0.41
a reply quoting an invocation 0.57
a longer reply quoting an invocation 0.64

The bound sits between the groups and nearer the leaks, deliberately: a false refusal falls back to the cut, which is exactly what happened before this change, while a false acceptance dispatches a call nobody made. The measured ratios are asserted in TestTheProseCeilingSeparatesCapturedLeaksFromQuotations, so anyone moving the bound has to move the corpus with it.

Testing

testdata/machineryrecover_dsml.json holds six replies this harness actually threw away in production — three leaks, three post-rejection degenerations — rather than hand-written samples. The narration half matters as much as the leak half: both spell bash in symbols, and the difference is proportion, so a corpus of only leaks would not have found the failure mode.

Call ids are derived from name and arguments rather than random, so a repair repeated over an unchanged reply leaves the prompt prefix byte-identical and the cache still hits.

go build ./..., go vet ./internal/provider/, and the full internal/provider suite pass. A mutation check (removing the unadvertised-name refusal) fails exactly the two tests that claim to cover it.

Measured effect

On one deployment against one model, the leaks stopped killing turns within a day. Before, they were the difference between a task completing and the turn ending in "the reply was the model's own internal markup instead of an answer and was cut".

Notes for review

  • The reason codes (ok, no_text, no_tags, fenced, unadvertised, bad_arguments, shape, prose) are part of the contract and are documented as such, so a blind spot stays visible rather than silent.
  • recoverProseCeiling is the one number a future change might want to tune. The evidence for moving it is in the table above.
  • Nothing outside internal/provider changes behaviour: the manual entry for this failure plane is updated to say the call is read back out and dispatched, and to say what will not be run.

Refs #1706

Some endpoints serve a model without translating its private tool-calling
syntax. Asked to use a tool, such a model writes the call as visible text —
angle brackets, bars, a tool name, a run of JSON. MachineryLeak already
convicts that reply and the harness throws it away: drop the turn, ask once
more, reroute the lane, and on a single-model deployment that is the end of
the work, reported to the person as "the model answered in its own internal
markup instead of words".

But the markup is very often a perfectly well-formed tool call that the
endpoint simply failed to parse. So machineryCut now tries to read the call
back out before it cuts. A reply whose markup holds a call that (a) names a
tool the request actually advertised, (b) carries arguments that parse as a
JSON object, and (c) IS the reply rather than a quotation inside it, has that
call put on the response and the turn goes on to dispatch it — indistinguishable
from a reply that arrived parsed, at every layer below, which is why no lane,
retry or accounting is threaded through. Only a leak that cannot be read that
way is still cut, exactly as before.

The refusals are the interesting half, and each is load-bearing:

  - a name the request did not advertise is prose about a call, not a call,
    and a reply smuggling a second, unoffered tool is refused outright;
  - arguments that do not parse as a JSON object are refused rather than
    guessed at — with the one deliberate exception of a named parameter
    whose value arrives unencoded, which is kept as text because that is what
    the tool is owed;
  - a reply with a long tail of prose after its markup is somebody DESCRIBING
    tool calls, which is the one answer a recovery must never destroy.

That last rule needed a measurement rather than a guess. A reply quoting an
invocation in a sentence is dense enough for MachineryLeak to convict — the
markup itself carries the density — so the guard around recovery does not
keep such a reply out; an earlier draft of this change assumed it did, and a
test caught it. What separates the two is proportion: the three captured
leaks in testdata/machineryrecover_dsml.json measure 0.25, 0.29 and 0.41 of
their length as prose outside the markup, and two quoting replies measure 0.57
and 0.64. The bound sits between the groups and nearer the leaks, because a
false refusal falls back to the cut — which is exactly what happened before
this change — while a false acceptance dispatches a call nobody made.

Call ids are derived from the name and arguments rather than random, so a
repair repeated over an unchanged reply leaves the prompt prefix byte-identical
and the cache still hits.

Tested against six replies this harness actually threw away in production
(three leaks, three post-rejection degenerations), not hand-written samples,
and the safety refusals are pinned by name. Measured on one deployment against
one model, the leaks stopped killing turns within a day; the fixture and the
reason codes are the contract for anyone tuning the bound.

Refs Agent-Field#1706
@CLAassistant

CLAassistant commented Oct 14, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member

Hey, thanks for the PR. Could you sign the CLA when you get a chance so we can review it?

Kiraprint pushed a commit to Kiraprint/CodeAF that referenced this pull request Oct 14, 2026
The rationale and the measurements belong in the changelog entry and the
tests, not inline. Also drops a claim that the density floor keeps quoted
calls out of recovery: it does not, which is what recoverProse exists for,
and the comment cited a test renamed to
TestAQuotingReplyIsRefusedEvenThoughTheGuardConvictsIt.
@Kiraprint
Kiraprint force-pushed the fix/recover-machinery-tool-calls branch from deb7533 to 5fbeced Compare October 14, 2026 23:46

@ZeroPoint95 ZeroPoint95 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kiraprint, the production examples make a credible case for investigating malformed tool output. The first question, though, is whether this recovery approach can reliably distinguish execution intent from quoted examples. The current heuristic doesn’t establish that, so I don’t think we should merge this version.

I reproduced five issues:

  1. Quoted examples become executable calls. “Example only; do not run” and inline-backticked examples both return real bash calls. Adjusting the prose threshold won’t establish intent; recovery needs a conservative contract that excludes ambiguous content.
  2. Truncated commands are accepted. Missing parameter and invocation closing tags become complete JSON arguments, bypassing the existing protection against repairing severed commands. Please require complete structure and cover truncation explicitly.
  3. Argument contents and types change. Recovery folds fullwidth characters, removes literal <tool_calls> text from values, and turns an explicitly string-typed “false” into a boolean. Argument contents and explicit string semantics need to survive unchanged.
  4. Recovered markup remains visible during streaming. Clearing final response content doesn’t retract events already emitted to the UI. This needs coverage through the streaming observer path.
  5. Identical invocations receive duplicate call IDs. A session-level probe pairs only one of their two results. IDs need to distinguish separate occurrences, with a regression covering result pairing.
    These were reproduced using fake HTTP/SSE responses, without executing recovered commands or making live model calls. Builds, vet, laws and manual checks passed; the provider allocation-test failure also reproduces on the base.
    Before revising individual parsing cases, I’d like us to agree on what evidence makes a malformed response safe to recover as an executable call. Complete, unambiguous envelopes or a provider-side correction look like safer directions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants