Skip to content

Remove leftover code in the global chat planner - #663

Draft
hanna-paasivirta wants to merge 3 commits into
mainfrom
cleanup-planner
Draft

Remove leftover code in the global chat planner#663
hanna-paasivirta wants to merge 3 commits into
mainfrom
cleanup-planner

Conversation

@hanna-paasivirta

Copy link
Copy Markdown
Contributor

Short Description

Removes two pieces of leftover code in global_chat/planner.py: an unreachable call_job_code_agent branch, and a duplicate StreamManager that replaced the one the router passes in. This is a clarity change first — neither is causing a real problem today.

Implementation Details

Context, so this can be skimmed: both are leftovers from earlier merges, and neither has a user-visible cost worth worrying about. The stream manager one does change what goes on the wire, but the only symptom anyone could notice is the opening spinner being sent twice, and spinners are transient thinking events that the client replaces with the next status and never persists. So it flashes and is gone. The reason to fix it is that the code says one thing and does another.

1. Unreachable job code branch (pure deletion)

  • _execute_tool has one production caller, and it is fed other_blocks, built two lines above as [b for b in tool_use_blocks if b.name != "call_job_code_agent"].
  • So its call_job_code_agent branch could never run. Same for the matching case in _tool_status_message.
  • Both went dead in 64da909, which moved job code calls to _execute_job_code_tools_parallel but left the old copies behind. ~65 lines.
  • Two tests were reaching the dead branch, so they were asserting on code that could not run in production. They now target _execute_job_code_tools_parallel, with the same assertions. That is a small coverage gain: it covers the single-block path and the no-job_key case, neither of which was tested before.

2. Duplicate stream manager (5-line deletion)

  • The router builds one StreamManager per request and passes it down, so a handed-over request continues on the same stream rather than starting a second message.
  • run() honoured that on line 103, then unconditionally replaced it on line 115 and used the replacement for the rest of the turn.
  • On the wire that meant a second message_start with a different id, content block indices restarting at 0 alongside blocks already sent, the opening spinner sent twice, and the router's manager never receiving message_stop.
  • Both StreamManager constructions arrived together in 7ce7833 "Release/next (Release 3.1.0 #604)". git log -L shows the merge kept both sides, so this is a merge leftover rather than an intentional change.
  • The fix keeps the _send_spinner pair and deletes the direct send_thinking calls. _send_spinner is a one-line wrapper around send_thinking, so that part is behaviour-neutral.

Why the fix is safe

  • No spent manager can reach the planner. Both subagents deliberately skip end_stream on handover, and their other end_stream calls sit on paths that either return normally or raise an ApolloError straight out of route_and_execute.
  • The planner's finally: end_stream() now closes the router's manager, which is correct — the planner never hands over.
  • The two callers that pass no manager still get a fresh one from line 103.
  • Nothing asserted on the discarded manager. Adds a regression test that a manager passed into run() is the one that receives the events and gets ended.

One cosmetic side effect, out of scope

The single message_start for a planner turn now carries the router's model name, because the router builds the shared manager with its own Haiku model. Direct routes to job_chat and workflow_chat already behave this way, and the field is informational. Worth a separate change if anyone wants it consistent.

Not verified: the live /stream endpoint. Unit tests pin the behaviour, but a real SSE check needs a full planner turn against the API.

_execute_tool is only reached via _execute_tool_blocks, which filters
call_job_code_agent out into _execute_job_code_tools_parallel, so its
call_job_code_agent branch and matching _tool_status_message case were
dead. Retarget the two tests that covered them at the parallel path.
run() built a second StreamManager and used it for the rest of the turn,
discarding the shared one the router passes in. That emitted a second
message_start with its own block indices and left the router's manager
unended.
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.

1 participant