fix(agent): offload oversized tool output at the chat seam, where the model actually sees it (#13997)

[mrveiss]

Thinking Path

#13692 built the tool-output spill and wired it into AgentLoop — a class whose own docstring says it is not instantiated anywhere in production. So on the live chat path nothing ever spilled, no run was ever bound, and the read tool #13919 wired into the dispatch seam returned no_run_bound on every call. Three PRs had made the mechanism safe, dispatchable and correct while it remained unreachable.

Owner decision on #13997 was to wire the live seam rather than park or remove it.

Before building, I checked whether chat already solved this — a second mechanism would be a fork, not a fix. It does not:

Site	Cap	Nature
execute_command	none	unbounded — the likeliest window-consumer
web_search	3000 chars	lossy, remainder discarded
subagent result	1000 chars	lossy
subagent completion	200 chars	lossy

So today the chat path either loses the overflow or does not bound it. The spill is non-lossy: the excerpt carries an anchor and the full output stays retrievable.

What Changed

manager.py::_handle_execution_summary — the point where results extend execution_history, which is what the model sees. One call added before that.

The run binding is in _run_llm_iterations, deliberately. _current_task_id is a ContextVar and asyncio copies the context at Task creation, so a bind in a parent or sibling Task is invisible to the dispatch. That coroutine and every tool call it drives share one Task. Binding "earlier in the request" would have reproduced the current always-no_run_bound behaviour with more code — the trap recorded on #13997 before starting.

Scoped to session_id, so an anchor leaked into another conversation cannot resolve there.

spill_execution_results — a shape adapter, not a second mechanism. The agent loop passes {name: result}; the chat seam collects a list of {"tool", "status", "output"} envelopes. Only the payload field is replaced:

• the envelope survives — status decides error handling downstream (tool_handler counts == "success", delegation checks == "error"), and swapping the whole dict for an excerpt would drop it;
• output stays a str — _as_output_text and tool_handler both read it as one;
• it calls spill_if_oversized, so the measured smaller-than-what-it-replaced guard, the artifact cap and the flag all apply unchanged.

Verification

tests/unit/chat_workflow/     412 passed
agent_loop/                   220 passed
flake8 / black --check        exit 0

Eleven new tests, using the real spill module against a temp directory — no mocking of the reader, since a mocked reader is precisely what let the unreachability survive two "deliveries".

Mutation-tested:

=== A: remove the offload call ===
4 failed, 6 passed

=== B: remove the run binding from the turn ===
1 failed, 10 passed

Mutation B initially passed, and that is worth stating: my first ten tests called _bind_spill_run directly, so deleting the bind from _run_llm_iterations changed nothing they observed. The helper was covered and the wiring was not — the same shape of gap that let this feature ship unreachable in the first place. TestTheTurnActuallyBindsTheRun now drives _run_llm_iterations itself and is the test that fails.

Scope

This does not close #13919. Its last box — "the run binding is present at that seam" — is what this PR supplies, so that issue closes on this landing; I am not claiming it in the commit because the two are separate concerns and #13919 has been falsely closed twice already.

AUTOBOT_TOOL_OUTPUT_SPILL stays off. The reduction is now logged per turn, so the chat-path equivalent of #13692's 95.5% figure will come from real traffic rather than a harness against a dormant class.

Model Used

Opus 5

Closes #13997

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review response — all three fixed. The first two were fatal and you were right to call it a fourth instance.

1. The bind was in a wrapper the production path bypasses

Confirmed, and the warning was in the function I edited, six lines from my change:

Issue #11612: Sets the lightweight_mode_used ContextVar here — the single seam both the legacy continuation loop (_run_llm_iterations) and the LangGraph path (graph.py::_run_llm_iteration) call through — instead of in the outer _execute_llm_continuation_loop wrapper, which the graph path bypasses entirely (causing the cost badge to always read False).

So this exact mistake had already been made once, diagnosed, fixed, and documented in place — and I made it again one layer up. The bind now sits beside _current_lightweight_mode in _run_continuation_loop_iteration, with a comment tying the two together so they cannot drift apart: both are ContextVars, both must be set at that seam or neither works.

TestTheTurnActuallyBindsTheRun drove _run_llm_iterations — the bypassed wrapper — so it was green for a feature dead in production. It now drives _run_continuation_loop_iteration. Mutation-verified: putting the bind back in the wrapper fails it.

2. The adapter missed shell output entirely

Confirmed. _create_execution_result emits {command, host, stdout, stderr, return_code, status, approved} — no output, no result, no tool. My key list of ("output", "result") skipped the single case the docstring names as the primary target.

Fixed: stdout and stderr added, and both are offloaded independently since a failing command can put megabytes in each (anchors is now a list). The tool name falls back to command, because otherwise every spilled shell result would bucket under the literal "tool" and collide anchors across unrelated commands in one session — thank you for catching that second-order effect.

The tests now build their fixture with _create_execution_result rather than hand-writing an envelope, with an explicit precondition asserting the real shape has no output/tool key. Mutation-verified: reverting the key list fails 3 tests.

3. Sync write on the event loop

Confirmed, and it is the worse kind of miss — I added spill_execution_results_async in this same PR, wrote the docstring explaining why the chat seam needs it, and then called the sync variant. _handle_execution_summary and _offload_oversized_output are now async and await it.

4. The approval-gate path — stated, not silently narrowed

You are right that graph.py::execute_tools builds its own exec_history and bypasses _handle_execution_summary, so with AUTOBOT_CHAT_APPROVAL_GATE on the offload does not fire on the resume path. Filed as #14239 rather than left implied, since the docstrings would otherwise read as full coverage.

Verification

tests/unit/chat_workflow/ + agent_loop/   635 passed
flake8 / black --check                    exit 0

=== MUT 1: bind back in the bypassed wrapper ===
FAILED ...::TestTheTurnActuallyBindsTheRun::test_the_shared_seam_binds_the_session

=== MUT 2: drop stdout from the key list ===
FAILED ...::test_a_real_shell_result_is_offloaded
FAILED ...::test_a_shell_result_is_anchored_by_its_command_not_a_literal
FAILED ...::test_both_stdout_and_stderr_offload

The pattern

Four times now: hasattr on an unwired method, registry greps for a seam with no callers, "safe to enable" while errors still grew, and now a bind in a bypassed wrapper plus a fixture shape production never emits. Every time the test drove the layer I had just written. The two fixes here that actually bite are the ones where the test had to be pointed somewhere I had not touched — the seam the other path calls, and the factory that builds the real result.

[mrveiss]

Code review

Reviewed from a sweep of green-but-unreviewed PRs, not as the author. Two findings; the wiring
itself checks out.

1. The reduction metric reads a key shell results do not have — so the one measurement this PR
sets out to collect is broken for the case it was built to fix.

before = sum(len(str(r.get("output", ""))) for r in results)
after  = sum(len(str(r.get("output", ""))) for r in rewritten)
logger.info("Offloaded %d oversized tool result(s): %d -> %d chars (#13997)", ...)

_create_execution_result (autobot-backend/chat_workflow/tool_handler.py:935-943) returns:

{"command", "host", "stdout", "stderr", "return_code", "status", "approved"}

No output key. Verified against origin/Dev_new_gui. For shell results — the PR's own "single
likeliest source of a window-consuming result" — both sums are 0, so a successful stdout
spill logs N -> 0 -> 0 chars. The spilled content and anchors are correct; only the metric is
wrong, and no test asserts on the logged values, which is why it passed.

This matters because the PR states the chat-path equivalent of #13692's 95.5% figure "will come
from real traffic" — via this log line. Suggest summing over the same key set the spill itself
uses rather than output alone.

2. A second dispatch path is not covered by "the chat seam".

autobot-backend/chat_workflow/graph.py:1096 — execute_tools, the AUTOBOT_CHAT_APPROVAL_GATE
node (GH#11202) — calls manager._process_tool_calls(...) directly, bypassing
_process_tool_results / _handle_execution_summary where the offload is wired. There is no
tool_output_spill reference anywhere in graph.py. That branch only runs when a tool call
genuinely needs approval, and both flags are off by default, so the blast radius today is zero —
but oversized output on that path is not offloaded even with AUTOBOT_TOOL_OUTPUT_SPILL on.

Verified as correct, not findings: the binding at manager.py:2887 sits at the shared seam
reached by both the LangGraph default path (graph.py:463) and the legacy loop — not in a
wrapper the graph bypasses, which was #13997/#13919's defect. The threshold is the existing
env-backed SPILL_THRESHOLD_CHARS, not a new hardcoded one. No silent-drop path: a failed
artifact write returns the original value unspilled at every level. And the tests use the real
_create_execution_result output rather than a hand-built envelope, so they are fed a shape the
producer actually emits.

[mrveiss]

Re-review: clean, no blockers

Verdict was no blockers, clean to merge, with all three fixes verified on the property rather than the instance — traced end to end and mutation-tested.

The one I most wanted checked came back sound: the run bind is genuinely on the production path. Traced process_message_stream → _process_via_graph → graph.ainvoke → generate_response → _run_llm_iteration → _run_continuation_loop_iteration, with the tool dispatch later in the same await chain inside the same node call. The three other create_task sites nearby are fire-and-forget side effects that never read the ContextVar, so nothing sits between the bind and the spill.

Key list confirmed complete by enumeration rather than assertion: all 38 execution_results.append sites plus _create_execution_result/_record_failed_step — builtins and MCP/delegate write output/result, shell writes stdout/stderr. That is every current producer.

Two residuals

Fixed here, because it was mine and wrong: the reduction log summed only output, so every shell spill reported 0 -> 0. The single measurement this change exists to produce was blind to its most common case. It now sums across the adapter's own key constant rather than a third hand-written copy.

Filed as #14284, not fixed here: _handle_llc_tool stores a dict under output, and the adapter guards on isinstance(..., str) — so an oversized LLC entity dump is skipped silently no matter how many names the key list grows. It fails on type, not name, which is precisely why the last two fixes could not have caught it. Filing rather than widening this PR, since the honest fix is at the producer.

State

Green, re-reviewed clean, ready to merge. When it lands, #13919's last unticked box — the run binding is present at that seam — can finally be ticked with evidence from a real dispatch rather than a grep of the layer that changed.