Remove leftover code in the global chat planner - #663
Draft
hanna-paasivirta wants to merge 3 commits into
Draft
Conversation
_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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short Description
Removes two pieces of leftover code in
global_chat/planner.py: an unreachablecall_job_code_agentbranch, and a duplicateStreamManagerthat 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_toolhas one production caller, and it is fedother_blocks, built two lines above as[b for b in tool_use_blocks if b.name != "call_job_code_agent"].call_job_code_agentbranch could never run. Same for the matching case in_tool_status_message._execute_job_code_tools_parallelbut left the old copies behind. ~65 lines._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_keycase, neither of which was tested before.2. Duplicate stream manager (5-line deletion)
StreamManagerper 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.message_startwith a different id, content block indices restarting at 0 alongside blocks already sent, the opening spinner sent twice, and the router's manager never receivingmessage_stop.StreamManagerconstructions arrived together in 7ce7833 "Release/next (Release 3.1.0 #604)".git log -Lshows the merge kept both sides, so this is a merge leftover rather than an intentional change._send_spinnerpair and deletes the directsend_thinkingcalls._send_spinneris a one-line wrapper aroundsend_thinking, so that part is behaviour-neutral.Why the fix is safe
end_streamon handover, and their otherend_streamcalls sit on paths that either return normally or raise anApolloErrorstraight out ofroute_and_execute.finally: end_stream()now closes the router's manager, which is correct — the planner never hands over.run()is the one that receives the events and gets ended.One cosmetic side effect, out of scope
The single
message_startfor a planner turn now carries the router's model name, because the router builds the shared manager with its own Haiku model. Direct routes tojob_chatandworkflow_chatalready behave this way, and the field is informational. Worth a separate change if anyone wants it consistent.Not verified: the live
/streamendpoint. Unit tests pin the behaviour, but a real SSE check needs a full planner turn against the API.