feat: improve sub-agent orchestration tools - #26673
Conversation
cd8be4f to
0d3ef4b
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 22 findings (1 P2, 12 P3, 1 P4, 5 Nit, 3 Note), REQUEST_CHANGES. Review Finding inventoryFinding InventoryFindings
Contested and acknowledgedCRF-3 (Note, Tool.tsx:530) - frontend timeout detection dead code for new-format wait_agent timeouts
CRF-16 (P2, subagent.go:1450) - interrupting half re-raised R3
Law analysisRound 3: effective +1061 -129 (27 files), 449 production / 612 test. Head 3a5a48f. Verdict: Don't split. Enforcement: Advisory. One reviewable idea (disposable to persistent recoverable subagents); concerns overlap on a single chatd subagent lifecycle surface; high test density; no generated cascade. Round logRound 1Panel. Netero + 16 panel reviewers. 6 P3, 4 Nit, 1 Note posted. 4 dropped. Reviewed against 1ae96fc..0d3ef4b. Round 2Churn guard proceeded: 10 addressed, 1 acknowledged, 0 silent. Netero found no new findings. Panel found 1 new P2. Dropped 2. Reviewed against 1ae96fc..ad94452. Round 3Churn guard classified CRF-16 addressed, but the panel re-raised its interrupting half (5 reviewers: Pariston P2, Meruem/Melody/Chopper/Mafu-san P3). Law: Don't split (advisory). Netero: no new findings. New: 5 P3, 1 P4, 2 Nit, 2 Note. Dropped 4 (CRF-29 wrench icon empirically disproved; CRF-30/31/32 low-value). Reviewed against ad94452..3a5a48f. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-measured change. The diagnosis (tool errors cause orchestrator abandonment, not task descriptions) is backed by a controlled comparison, and the fix is proportional: structured payloads instead of bare errors, better naming to reinforce persistence, fleet recovery via list_agents, and two-sentence system prompt guidance. Test density is 66% (544 test LOC / 280 production LOC). The backward-compat alias system is particularly clean: old history dispatches correctly, results preserve the original name, and the frontend handles both names.
Severity summary: 6 P3, 4 Nit, 1 Note.
"The old pattern's safety argument died when the preconditions changed." (Pariston, noting why the fix is at the response format, not just descriptions)
Generated by Coder Agents.
coderd/x/chatd/subagent.go:1373
P3 [CRF-7] closeSubagent is the sole holdout from the close_agent -> interrupt_agent rename:
interrupt_agent (tool) -> interruptAgentArgs (struct)
-> closeSubagent (method) <-- still "close"
-> InterruptChat (underlying API)
A reader of the interrupt_agent handler at line 864 sees p.closeSubagent(...) and must decide whether "close" and "interrupt" mean different things. One caller, one definition; rename to interruptSubagent.
(Gon P3, Leorio Nit)
🤖
site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx:530
Note [CRF-3] With the backend change, wait_agent on timeout now returns a non-error payload (timed_out: true), so subagentIsError is false and this timeout detection branch never executes for new-format responses. The timed_out field is consumed by the LLM to retry, not by the UI. The old error-format detection still works for pre-existing timeout responses in chat history, so backward compat is maintained. No action needed unless a visual timeout indicator is wanted for new-format responses.
(Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
0d3ef4b to
ad94452
Compare
|
In reply to CRF-3 (Note, No action needed, as the reviewer notes. The old error-format detection branch in Tool.tsx still handles legacy timeout responses in chat history. New-format timeout responses (non-error with timed_out: true) correctly bypass the error branch, and the LLM consumes the timed_out field to retry. No code change made. In reply to CRF-7 (P3, Renamed closeSubagent to interruptSubagent. The method signature now returns (database.Chat, bool, error) where the bool indicates whether an actual interrupt occurred. One definition, one call site in the interrupt_agent handler.
|
|
/coder-agents-review model:gpt-5.5 thinking:high |
There was a problem hiding this comment.
Round 2 addressed the first-pass issues well: the rename is cleaner, list_agents now documents best-effort ordering, raw DB errors are wrapped, and the wall-clock timeout test now uses a mock clock. One new issue remains in the timeout recovery path.
Severity summary: 1 P2.
"The live axis is scheduling between the timeout timer and the worker status update."
🤖 This review was automatically generated with Coder Agents.
c49f4e4 to
083f40d
Compare
|
@codex review |
083f40d to
c8aec22
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 083f40d8a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates chatd sub-agent orchestration semantics and UI rendering so orchestrators stop treating delegated agents as one-shot calls. It adds a new list_agents tool, renames close_agent to interrupt_agent with backward-compatible aliasing, and adjusts wait_agent to return structured, recoverable payloads (including on timeout and error status).
Changes:
- Backend:
wait_agentnow returns non-error JSON on timeout (timed_out: true) and structured JSON on error-status completion; addslist_agents; renamesclose_agenttointerrupt_agentwith tool name alias dispatch. - Frontend: Updates tool rendering, labels, and visibility rules for
interrupt_agent, legacyclose_agent, and newlist_agents, including newListAgentsToolUI. - Prompt/testing: Adds
<subagent-orchestration>guidance and expands Go + frontend tests/stories for the new behaviors.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts | Adds coverage for status mapping behavior around interrupted. |
| site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.ts | Updates lifecycle action handling for interrupt. |
| site/src/pages/AgentsPage/components/ChatElements/tools/toolVisibility.test.ts | Tests visibility rules for legacy close_agent, interrupt_agent, and list_agents. |
| site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx | Updates subagent label verbs for interrupt and list. |
| site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx | Adds an icon case for list_agents (but see review comment). |
| site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx | Adds ListAgentsRenderer, routes list_agents away from SubagentRenderer, and tweaks reporting logic. |
| site/src/pages/AgentsPage/components/ChatElements/tools/Tool.stories.tsx | Updates stories from close_agent to interrupt_agent and adds list_agents stories. |
| site/src/pages/AgentsPage/components/ChatElements/tools/SubagentTool.tsx | Updates verb set to include interrupt and list. |
| site/src/pages/AgentsPage/components/ChatElements/tools/subagentDescriptor.ts | Adds interrupt and list actions and maps legacy tool names. |
| site/src/pages/AgentsPage/components/ChatElements/tools/ListAgentsTool.tsx | New UI component to render list_agents output with links to chats. |
| site/src/pages/AgentsPage/components/ChatConversation/messageParsing.test.ts | Extends parsing tests for interrupt_agent and list_agents descriptor behavior. |
| site/src/pages/AgentsPage/AgentChatPage.stories.tsx | Updates example transcripts to use interrupt_agent. |
| coderd/x/chatd/subagent.go | Implements list_agents, interrupt_agent, wait_agent structured timeout/error payloads, and tool-name aliasing. |
| coderd/x/chatd/subagent_internal_test.go | Adds/updates tests for timeout payload, error-status payload, and list_agents behavior. |
| coderd/x/chatd/subagent_catalog.go | Updates subagent tool guidance text (plan mode tool list, persistence guidance). |
| coderd/x/chatd/recording_internal_test.go | Updates timeout behavior expectation: wait_agent timeout is no longer a tool error. |
| coderd/x/chatd/prompt.go | Adds <subagent-orchestration> system prompt section. |
| coderd/x/chatd/instruction_internal_test.go | Verifies the new orchestration prompt block is present. |
| coderd/x/chatd/generation.go | Plumbs tool-name alias map into tool execution. |
| coderd/x/chatd/chatprompt/chatprompt.go | Updates lifecycle-tool detection to include interrupt_agent and legacy close_agent. |
| coderd/x/chatd/chatloop/chatloop.go | Adds alias-aware dispatch (ToolNameAliases) when executing tools. |
| coderd/x/chatd/chatloop/chatloop_run_internal_test.go | Tests tool alias resolution behavior. |
| coderd/x/chatd/chatd.go | Updates plan-mode allow/deny lists and explore tool gating for new/renamed tools. |
| coderd/x/chatd/chatd_test.go | Updates expected tool lists to include interrupt_agent and list_agents. |
| coderd/x/chatd/chatd_internal_test.go | Updates active tool sets for turns to include interrupt_agent and list_agents. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| children := make([]database.Chat, 0, len(rows)) | ||
| for _, row := range rows { | ||
| children = append(children, row.Chat) | ||
| } | ||
| // Sort most recently active first with a stable id | ||
| // tiebreak. The shared query's own ordering is not | ||
| // altered because it backs the chats sidebar; paginate | ||
| // here instead. | ||
| slices.SortStableFunc(children, func(a, b database.Chat) int { |
There was a problem hiding this comment.
Valid observation, but the in-memory sort is the intended design. The shared GetChildChatsByParentIDs query backs the chats sidebar and its ordering was deliberately not altered. Subagent counts per chat are small in practice. Adding DB-level pagination would require a new query or modifying the shared one, which is out of scope. The handler now sorts rows directly (no intermediary array) and uses min() for clamping, which addresses the code cleanliness concern without changing the data access pattern.
🤖 Posted using
/amend-reviewskill via Coder Agents.
c8aec22 to
ad0a318
Compare
The orchestration tools work, but they tell the orchestrator the wrong story, so it acts on the misframing. wait_agent says "final response" and returns a bare error on timeout, which reads as failure rather than "still working." spawn_agent frames a spawn-wait-done lifecycle, so the orchestrator never learns agents persist and can be reused. close_agent sounds like it destroys when it only interrupts, message_agent hides its queue and interrupt behavior, and there is no way to list spawned agents to recover the fleet after a compaction. This is measured, not hypothesized: in the personal-agent chat snapshots ~23% of wait_agent calls time out and ~10% of delegated agents are never cleanly collected. A retry-disciplined harness orphans ~0% with the same descriptions, so the give-up is decided against the tool response and the missing hygiene guidance, not against the description. The fixes land where the decision is made. wait_agent returns an informational timeout payload and a recoverable-aware error payload instead of a bare error. A new list_agents tool recovers the fleet. close_agent becomes interrupt_agent, with a hidden alias so old histories still dispatch. The descriptions and a <subagent-orchestration> section in the system prompt teach persistence, queuing, and not abandoning a working agent. Implements CODAGT-512.
ad0a318 to
afaa14d
Compare
…only, clean up Round 3 review fixes: - SubagentRenderer checks rec.timed_out from the structured payload, not just error strings, so non-error timeout responses render with the clock icon and timeout verb. - list_agents handler rejects delegated chats via ParentChatID.Valid guard, matching the root-only contract. - list_agents sorts rows directly, no intermediary children array; clamps with min() instead of if-chains. - ListAgentsTool shows 'Listed 0 agents' for completed empty lists, not 'Listing agents'. - Remove dead list_agents case from ToolIcon switch (unreachable via isSubagentToolName early return). - interrupt_agent description says 'transitions to waiting' instead of 'stops and waits', matching the async transition behavior. - Add SubagentWaitTimedOutStructured and ListAgentsEmpty stories. - Add DelegatedChatRejected subtest to TestListAgents.
…with min/max The ListAgentsTool label showed total instead of the page size, so a paginated call with 3 of 15 agents displayed 'Listed 15 agents'. The label now shows 'Listed N of M agents' using agents.length for the page count. The limit clamping replaced three if-blocks with min(max(limit, 1), maxListAgentsLimit) and updated the story assertion to match.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5cbc1cba0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ack, docs ListAgentsTool label always uses 'Listed N of M agents' format, dropping the single-agent special case that hid the total on paginated calls. ListAgentsRenderer falls back to the plain string result when the backend returns a text error, so the UI shows the actual error instead of a generic fallback. Docs updated to replace close_agent with interrupt_agent and add list_agents to the tool tables in index.md and architecture.md.
Docs preview📖 View docs preview for |
|
/coder-agents-review set-spend-limit:200 |
| // ToolNameAliases maps a non-advertised tool name to the canonical | ||
| // tool it dispatches to. Used for backward compatibility when a tool | ||
| // is renamed but old chat histories still reference the old name. | ||
| ToolNameAliases map[string]string |
There was a problem hiding this comment.
Do we need to do this? I guess we do so we don't break existing chats, but I feel like models would discover this change anyways with their new tools?
There was a problem hiding this comment.
Honestly, I did not try. I went the "better safe than sorry approach". I did consider it and I think they should probably discover it after one failed call of missing tool. Or they might give up ¯\_(ツ)_/¯. If you want I'll remove the back-compat. FE will still need it unless we migrate the data though.
There was a problem hiding this comment.
Owner already replied in-thread: the back-compat alias is the "better safe than sorry" approach, and the frontend needs it unless chat history data is migrated.
🤖 Posted using
/amend-reviewskill via Coder Agents.
| "Returns the agent's response and status. A timeout is not "+ | ||
| "a failure: the agent is still running. Call wait_agent again "+ | ||
| "or use list_agents to check its status.", |
There was a problem hiding this comment.
Not a finding. Emoji reaction to the wait_agent description, no actionable content.
🤖 Posted using
/amend-reviewskill via Coder Agents.
There was a problem hiding this comment.
Round 3 is a strong pass on prior feedback. Every R1/R2 finding is addressed, and several were over-delivered: CRF-3 (accepted in R2) was actually fixed so the UI now honors the structured timed_out payload, the docs were updated for the rename and list_agents, list_agents got a root-only guard with a matching test, and the new ListAgentsTool component cleanly mirrors ListTemplatesTool. Determinism and alias wiring verified clean.
The re-review surfaces one genuine regression and a cluster of new issues from this round's refactor and new component. The interrupting half of CRF-16 was never closed: the fix paired error but left interrupting classified as a completion in both the poll loop and the timeout-gap path, and it now sits on this PR's own documented interrupt-then-wait flow (Pariston P2; Meruem, Melody, Chopper, Mafu-san P3). That re-raise is why this is REQUEST_CHANGES.
One item needs a human decision rather than an agent call: the orphan fix is entirely advisory (payload shape, descriptions, system prompt) with no backend reaping of children. The product analysis frames a backend cascade-cancel as a separate safety net. Please either file a tracking issue or explicitly accept the advisory-only scope; it should not be left implicit.
Note: three reviewers flagged list_agents rendering a wrench icon. That is incorrect; isSubagentToolName("list_agents") returns BotIcon via the early return in ToolIcon, so it renders a bot. The only residue is a stray blank line, folded into the nit below. Not raising the icon claim.
Severity summary: 1 P2 (re-raised), 5 P3, 1 P4, 2 Nit, 2 Note.
"I went looking for a fight on the lifecycle surface. The alias dispatch holds. One thread still moves when I pull it." (Hisoka)
coderd/x/chatd/subagent.go:832
P3 [CRF-19] message_agent returns "interrupted": args.Interrupt, echoing the request flag instead of whether an interrupt actually happened. This is the unfixed sibling of CRF-6, which corrected exactly this for interrupt_agent (now derived from interruptSubagent's real transition).
SendMessageBusyBehaviorInterrupt only interrupts an active run. When the target is idle (waiting), nothing is interrupted, yet the response still says interrupted: true. This round also rewrote the description to promise interrupt: true "stop[s] the agent's current work," sharpening the contract the response breaks. The orchestrator is told it halted in-progress work that never existed.
Fix: derive interrupted from the target's pre-send status (active vs idle), mirroring interrupt_agent. (Mafuuu P3, Chopper P3)
🤖
🤖 This review was automatically generated with Coder Agents.
…d prompt, clean up dead list action
checkSubagentCompletion now treats interrupting as not-done so the
agent settles into waiting or error before classification. This fixes
both the poll loop and the timeout-gap path returning stale output for
a transient interrupting status.
checkSubagentCompletion returns the chat on the not-done path instead
of an empty Chat{}, eliminating a second unsynchronized GetChatByID in
the timeout handler.
message_agent derives interrupted from the target's pre-send status
instead of echoing args.Interrupt.
The subagent-orchestration prompt block is extracted to a const and
stripped from delegated child chats that cannot call list_agents or
message_agent.
list_agents is removed from actionByToolName and list from
SubagentAction, eliminating dead verb entries and the
name !== list_agents carve-out in Tool.tsx dispatch.
ListAgentsCompleted story now expands and asserts agent rows.
ListAgentsError story covers the failure path. Timeout-gap completion
test drives the error-in-gap branch.
Recording stop timeout extracted to named constant. Stray blank lines
removed. interrupt_agent description tightened.
|
In reply to CRF-19 (P3,
|
list_agents is no longer in actionByToolName, so getSubagentDescriptor returns null for it. The test now asserts null instead of action: "list".
Removing list_agents from actionByToolName made
isSubagentToolName("list_agents") return false, so the early
return in ToolIcon no longer fires. The tool fell through to
the default case and rendered a WrenchIcon instead of a
BotIcon. Add case "list_agents" back to the switch.
Removing list_agents from actionByToolName broke icon rendering (fell through to WrenchIcon) and classification. list_agents is a subagent tool registered in subagentTools(). Restored list in SubagentAction, list_agents in actionByToolName, verb entries, ToolLabel case, and the name !== list_agents dispatch carve-out. Added a comment in subagentDescriptor.ts explaining that list renders through ListAgentsRenderer, not SubagentRenderer.
Restore the 'Keep in sync with coderd/x/chatd/subagent.go' comment on isSubagentLifecycleToolName; the replacement had removed the maintenance coupling. Add missing interrupted field to interrupt_agent results in AgentChatPage.stories.tsx (2 locations). Remove duplicate ListTemplates stories comment block in Tool.stories.tsx. Add comment in checkSubagentCompletion explaining why interrupting is treated as not-done (transient status that settles to waiting or running). Fix interrupt_agent description: 'may briefly read interrupting before transitioning to waiting, or running if there are queued messages'. Removes vague 'worker finalizes' terminology. Update docs to match: 'transitions to waiting or running if there are queued messages'. Fix ListAgentsTool doc comment to say 'Listed N of M agents' instead of 'Listed N agents'.
Remove 'settles into waiting' and 'the worker finalizes' from interrupt_agent description. The agent can transition to waiting or running depending on queued messages, so the description now says 'may briefly read interrupting before transitioning to waiting, or running if there are queued messages'.
The orchestration tools (
spawn_agent,wait_agent,message_agent,close_agent) work, but they tell the orchestrator the wrong story, so it abandons work that is still running. This is measured: in the personal-agent chat snapshots ~23% ofwait_agentcalls time out and ~10% of delegated agents are never cleanly collected. A retry-disciplined harness orphans ~0% with the same descriptions, which puts the fix at the tool response and the missing guidance, not the description alone.The behavior change: the orchestrator is now taught that sub-agents are persistent, reusable, interruptible workers, not one-shot disposable calls. An agent can be created but not removed;
interrupt_agentstops its current work and leaves it waiting. It can be messaged at any time: a queued message (default) is delivered after current work, an interrupting message stops current work and delivers immediately. Agents linger after completing and can be reused viamessage_agentwhen their existing context is useful for a follow-up task. If the orchestrator loses track of them after a compaction,list_agentsrecovers the fleet.The mechanism:
wait_agentreturns an informational payload on timeout (status,timed_out) and a structured payload on error status (last_error,report) instead of a bare error that reads as failure. On timeout, if the agent completed during the gap between the last poll and the timer, the normal completion payload is returned instead (viacheckSubagentCompletion, avoiding a TOCTOU on stale status).list_agents(new, root-only, paginated, most-recently-active first, archived excluded) lists spawned agents so the fleet survives a compaction.close_agentis renamed tointerrupt_agent(response returnsinterruptedinstead ofterminated); a hiddenToolNameAliasesalias keeps old histories dispatching. Theinterruptedfield istrueonly when an actual interrupt occurred, not when the agent was already idle.message_agentexplains queue-by-default andinterrupt: true; theinterruptflag stops current work but does not jump the message ahead of already-queued messages.spawn_agentsays that agents persist and can be reused, and that spawned agents are the orchestrator's responsibility (don't abandon a working one).list_agentsrecovers the fleet. These are precision overlays; the tool descriptions are load-bearing on their own.list_agentsrenders through a dedicatedListAgentsToolcomponent (likeListTemplatesTool) with an expandable agent list and links to each chat.close_agentmaps to theinterruptaction; no separatecloseaction exists in the frontend.Dogfood validation (6 tests on a dev instance):
falsehandled the failure gracefully.message_agentresumed it successfully. (Triggering a real chat-level error requires a provider failure; unit tests cover this path.)wait_agentwith a 2s timeout returnedtimed_out: trueon a long-running agent. A subsequent wait returned the completed report. No contradictorytimed_outwith terminal status observed.list_agentswithlimit=5returned correcttotal,returned, andhas_moreacross three pages (5/5/4).interrupt_agenton a running agent: returnedinterrupted: true, status: interrupting. The agent transitionedrunningtointerruptingtowaiting.close_agentalias: not advertised in the tool list (by design). The hidden alias dispatches old-history calls tointerrupt_agentat theexecuteSingleToollevel.list_agentsin plan mode: available (backend allowlist includes it; unit tests verify).Implements CODAGT-512.