fix(chat): close two tool-output-offload coverage holes — LLC dict payload and approval-gate resume (#14284, #14242)

[mrveiss]

Closes #14284, #14242

Thinking Path

Both issues are holes in the #13997 tool-output offload's coverage, found in
review of PR #14237. I read agent_loop/tool_output_spill.py (the adapter)
and chat_workflow/tool_handler.py/manager.py/graph.py (the two producer
seams) before touching anything, to find the actual root cause rather than
patching the reported symptom.

#14284. spill_execution_results only offloads an envelope field when
isinstance(entry.get(k), str) — a type guard, not a name guard. _handle_llc_tool
stored the raw dict dispatch_llc_tool returns under output, so no amount of
growing the key list could ever reach it. Per the issue's preferred fix shape
(option 1), I serialise at the producer instead of teaching the adapter a
second, dict-shaped contract — every other tool handler already writes a str
there.

#14242. Tracing graph.py::execute_tools (the approval-gate resume node)
to see how the offload could be wired in, I found it calls
manager._process_tool_calls directly and builds its own exec_history,
never routing through _process_tool_results/_handle_execution_summary
(where #13997 hooks the spill) — matching the issue exactly. Wiring the
offload call in exposed two more problems in the same lines, both necessary
for the wiring to do anything at all:

1. the spill run was never bound on this path. _bind_spill_run runs inside
   _run_continuation_loop_iteration, and this node never calls that method
   (that's the whole point of GH#11202's plan-only split) — so _spill_run_id()
   would read None and the offload declines to write ("an artifact that
   could never be re-read is pure cost").
2. the "track execution results" check read isinstance(item, dict) on the
   yielded WorkflowMessage itself, which is a dataclass and is never a
   dict — only its already-computed .to_dict() form (msg_dict) is. So the
   check never matched, and exec_history silently stayed whatever
   state["execution_history"] already held on entry. New tool results never
   reached it, offloaded or not — on the approval-gate path, tool output never
   reached the model's next continuation prompt at all, a strictly worse bug
   than "the offload doesn't fire on it."

All three had to be fixed together: (1)+(2) are what make the offload call
meaningful, and nothing here had a test, which is exactly how a dead
isinstance check and an unbound ContextVar both survived.

What Changed

• autobot-backend/chat_workflow/tool_handler.py — _handle_llc_tool now
  JSON-serialises the LLC dispatch result before storing it under output,
  instead of storing the raw dict. (fix(chat): an LLC tool result stores a dict under output, so the offload silently skips it whatever the key list says #14284)
• autobot-backend/chat_workflow/manager.py — updated _as_output_text's
  docstring, which specifically named _handle_llc_tool as the dict-shape
  example this change removes; left the dict-handling branch itself alone
  (still needed for the MCP bridge's result field). (fix(chat): an LLC tool result stores a dict under output, so the offload silently skips it whatever the key list says #14284, doc accuracy)
• autobot-backend/chat_workflow/graph.py — execute_tools:
  • binds the spill run (manager._bind_spill_run(session_id)) before
    dispatch, since this path never calls the method that does it elsewhere;
  • fixed the dead isinstance(item, dict) check so newly dispatched results
    actually reach exec_history (reading the flat execution_results list
    out of the summary event's metadata, the same shape the non-gated path
    reads);
  • routes those results through manager._offload_oversized_output before
    extending exec_history, so oversized output is offloaded on a resume
    exactly as it already is on the non-gated path. (fix(chat): tool-output offload does not fire on the approval-gate resume path #14242)
• Two new test files (chat_workflow/llc_tool_output_offload_test.py,
  chat_workflow/graph_execute_tools_offload_test.py) — see Verification.

No other producer was switched to a dict payload, and the MCP bridge's dict
under result is untouched, so the adapter's existing dict-handling paths
(_as_output_text) still have a live caller.

Verification

Both test files build their fixtures from the real path the issues name,
not a hand-written dict/exec_history:

• llc_tool_output_offload_test.py drives dispatch_llc_tool →
  _handle_llc_tool for real (only the DB session + WorkItemService are
  mocked, the same pattern as llc/tests/test_agent_tools.py), then runs the
  resulting envelope through the real spill_execution_results.
• graph_execute_tools_offload_test.py calls chat_workflow.graph.execute_tools
  directly with a realistic ChatState, faking only manager._process_tool_calls
  — and that fake yields a summary built by the real
  ToolHandlerMixin._build_execution_summary, not a hand-written
  WorkflowMessage.

Static verification only in this environment per project policy (no local
test execution) — CI runs both files. I hand-traced each assertion against
the code paths above rather than executing them; the mutation table below is
therefore a static trace of what each assertion depends on, not an
executed run.

#	Mutation (fix reverted)	Test expected to fail	Why
1	tool_handler.py: store result (dict) instead of json.dumps(result) under output	llc_tool_output_offload_test.py::TestTheEnvelopeStaysAString::test_a_successful_llc_result_stores_a_str_not_a_dict	asserts isinstance(..., str) directly on the reverted dict
2	same mutation	TestAnOversizedLLCResultOffloads::test_the_dict_payload_is_offloaded_with_excerpt_and_anchor	precondition isinstance(original_output, str) fails immediately; even if bypassed, spill_execution_results would skip the dict key and count == 1 would fail
3	graph.py: remove manager._bind_spill_run(session_id)	graph_execute_tools_offload_test.py::TestTheApprovalGateResumeOffloads::test_oversized_output_is_offloaded_with_excerpt_and_anchor	_spill_run_id() reads None, _offload_oversized_output returns results untouched — len(entry["output"]) < len(_BIG) fails
4	graph.py: revert the isinstance(item, dict) check (drop the msg_dict.get("type") fix)	graph_execute_tools_offload_test.py::TestExecutionHistoryActuallyAccumulates::test_a_dispatched_result_reaches_execution_history	out["execution_history"] stays [] (from state), never equals original
5	same mutation	TestTheApprovalGateResumeOffloads::test_oversized_output_is_offloaded_with_excerpt_and_anchor	out["execution_history"] is empty — IndexError on [0]
6	graph.py: drop the await manager._offload_oversized_output(new_results) call (keep the isinstance/bind fixes)	test_oversized_output_is_offloaded_with_excerpt_and_anchor	entry["output"] is the full _BIG string, len(entry["output"]) < len(_BIG) fails, no anchors key

Also covered: _as_output_text/_format_execution_step still render the
(now-string) LLC output correctly; small/under-threshold results pass through
byte-identical on both paths; the flag-off case leaves the approval-gate turn
untouched; the full spilled output is retrievable through its anchor on both
paths.

Model Used

Claude Opus 5 (1M context)

[mrveiss]

Review — APPROVE

I verified the third finding independently, because it is the one that changes the story.

The latent bug is real. In base, graph.py:1105:

msg_dict = item.to_dict() if hasattr(item, "to_dict") else item
...
if isinstance(item, dict) and item.get("type") == "execution_summary":   # line 1122

_process_tool_calls yields WorkflowMessage objects, which have to_dict — so item is never a dict and that branch never fired. exec_history silently stayed whatever state["execution_history"] already held; new tool results never reached it on this path at all, offloaded or not. So #14242 was not only "the offload does not fire here" — the accumulation did not either.

The fix is also correct in the case the old code was reaching for: if item ever were a dict, msg_dict = item, and msg_dict.get("type") still matches. Strictly more correct in both branches, not one narrow guard swapped for another.

Binding the spill run on this path is necessary and easy to miss — _bind_spill_run only runs inside _run_continuation_loop_iteration, which this node deliberately never calls, so without it the offload would decline to write with no error.

For #14284, serialising at the producer is the right end. spill_execution_results type-guards on str before it looks at any key, so the key list was never the lever — which is exactly why the issue title says "whatever the key list says".

Fixing _as_output_text's docstring matters more than it looks: it cited _handle_llc_tool as its dict-shape example, so it documented the bug as intended behaviour.

One caveat on the evidence, stated plainly

The mutation table is a static trace, not executed mutations. Repo policy forbids running the suite locally, so that is the correct ceiling here — but a traced mutation is weaker than an observed one, and the PR body should not be read as if the mutations were run.

The two new tests build fixtures from the real path rather than hand-written dicts, which is the right shape and the reason these holes survived earlier review. CI executing them is what converts the trace into evidence — worth confirming both new files actually run and pass rather than being skipped.

No blocking findings.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!