refactor: remove relay_message from Task Agent tool set - #755
Conversation
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by claude-sonnet-4-6 (Anthropic)
Model: claude-sonnet-4-6 | Client: NeoKai | Provider: Anthropic
Recommendation: REQUEST_CHANGES
Summary
The mechanical removal of relay_message is clean and complete — zero references remain in packages/, all test counts updated correctly (7→6), the schema, handler, MCP registration, and imports are fully removed. The diff is appropriately scoped to exactly what Task 3.1 calls for.
However there is one P1 issue and one P2 issue worth addressing.
P1 — System prompt makes a false claim about the current state
File: packages/daemon/src/lib/space/agents/task-agent.ts, line 293
The updated system prompt now reads:
All agents (including the Task Agent) use
send_messagefor peer communication. Channel topology is enforced uniformly — no agent has a special bypass.
This statement is not true at the end of Task 3.1. The Task Agent currently has no peer communication tool — relay_message was removed and send_message is not added until Task 3.3. The MCP server registered for the Task Agent (createTaskAgentMcpServer) does not include send_message, as confirmed by searching task-agent-tools.ts.
During Tasks 3.1→3.3, if a running Task Agent session encounters a workflow that requires inter-agent coordination and tries to call send_message based on this prompt, it will receive a "tool not found" error from the MCP layer. The prompt is written as if Task 3.3 is already complete.
Suggested fix: Qualify the statement to reflect the transitional state, e.g.:
All agents use
send_messagefor peer communication. Channel topology is enforced uniformly — no agent has a special bypass. (Note:send_messagewill be added to the Task Agent tool set in a follow-up task; peer communication is not available in this transitional state.)
Or defer this system prompt line to Task 3.3 where it becomes accurate.
P2 — Pre-existing comment drift made worse by this PR
File: packages/daemon/src/lib/space/tools/task-agent-tool-schemas.ts, lines 2–10
The file-level comment still reads:
Task Agent MCP Tool Schemas — Zod schemas and TypeScript types for the 5 tools available to the Task Agent session.
And the tool list in the comment omits list_group_members entirely. This stale comment predates the PR (it wasn't updated when list_group_members was added in commit df6a83198), but the PR touches this file and removes a tool, making the "5" count even more visibly wrong (it should now say 6, and list all 6 tools).
This isn't a blocker since the TASK_AGENT_TOOL_SCHEMAS aggregate and the test assertion both correctly say 6, but the file-level doc comment is now actively misleading.
P3 — list_group_members response message is ambiguous in context
File: packages/daemon/src/lib/space/tools/task-agent-tools.ts, line 793
The message returned when no channel topology is declared reads:
No channel topology declared — all messaging is unrestricted.
This made sense when the Task Agent had relay_message (unrestricted relay). Post-removal and pre-Task-3.3, this message is misleading to the Task Agent: it has no peer communication tool, so "unrestricted" is vacuously true but unhelpful. Minor wording nit, not a blocker.
Test removal assessment
The test removals are appropriate. All removed tests tested relay_message behavior specifically (schema validation, handler success/error paths, cross-group rejection, self-relay guard, injector failure surfacing). No tests covering unrelated functionality were dropped. The remaining test suite continues to cover list_group_members, group scoping for send_message, and error paths.
The renumbering of describe blocks (8→9→... down to 8→9→...) is cosmetically tidy.
Correctness verdict
Zero relay_message or RelayMessageSchema/RelayMessageInput references remain in packages/. The only references are in docs/plans/ which are historical/planning documents, consistent with the acceptance criteria ("outside historical docs").
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by claude-sonnet-4-6 (Anthropic)
Model: claude-sonnet-4-6 | Client: NeoKai | Provider: Anthropic
Recommendation: APPROVE
This is a follow-up review after the previous feedback was addressed. All three previously flagged issues have been correctly resolved.
Previous Issues — Verification
P1 (resolved): System prompt falsely claimed Task Agent uses send_message
The false claim has been removed. The Channel Topology section in buildTaskAgentSystemPrompt() (task-agent.ts, lines 287–295) now correctly states that channel topology is enforced uniformly and adds an honest forward-looking note: (Note: peer messaging via send_message will be added in a follow-up task.) — accurate and not misleading.
P2 (resolved): Stale "5 tools" comment should say "6 tools"
Both affected files now correctly state "6 tools":
task-agent-tools.tsline 4: "the business logic for the 6 Task Agent tools"task-agent-tool-schemas.tsline 2: "Zod schemas and TypeScript types for the 6 tools"
The file-level doc comment in task-agent.ts (lines 17–22) also now lists exactly 6 tools with no relay_message.
P3 (resolved): Misleading "unrestricted" language in list_group_members response
The word "unrestricted" is completely absent from task-agent-tools.ts. The list_group_members response message at line 791–795 reads cleanly: "Group has N member(s). No channel topology declared. / Channel topology is active and enforced."
The two remaining "unrestricted" occurrences in task-agent-manager.ts (line 1352) and step-agent-tools.ts (line 347) are in comments about request_peer_input routing — a different tool and a different concept — and were correctly left untouched by this PR.
Completeness Check
- All
relay_messagereferences removed frompackages/directory — confirmed zero matches. RelayMessageSchemaandRelayMessageInputtypes removed fromtask-agent-tool-schemas.ts.- Handler and MCP tool registration removed from
task-agent-tools.ts. - Channel-resolver.ts comments updated (relay_message bypass reference gone).
- Test files cleaned:
task-agent-tools.test.ts,task-agent-tool-schemas.test.ts,cross-agent-messaging.test.ts,cross-agent-messaging-integration.test.tsall free of relay_message references. TASK_AGENT_TOOL_SCHEMASaggregate contains exactly 6 schemas and the schema test at line 286 assertstoHaveLength(6).- The remaining
relay_messageoccurrences are in design docs only (docs/plans/), which are historical records and do not need to be updated.
No new issues were introduced. The implementation is clean and the change is consistent with the stated goal.
The relay_message tool is being removed in favor of the unified send_message tool that all agents (Task Agent and step agents) use. Changes: - Remove RelayMessageSchema and RelayMessageInput from task-agent-tool-schemas.ts - Remove relay_message handler and MCP tool registration from task-agent-tools.ts - Update channel-resolver.ts comments to remove relay_message bypass reference - Update task-agent.ts system prompt to remove relay_message references - Remove all relay_message tests from task-agent-tools.test.ts, task-agent-tool-schemas.test.ts, cross-agent-messaging.test.ts, and cross-agent-messaging-integration.test.ts Task Agent now uses send_message for peer communication like all other agents, with channel topology uniformly enforced.
P1: Remove false claim about send_message in Task Agent system prompt. The Task Agent does not yet have send_message (added in Task 3.3). Replaced with a note that peer messaging will be added in a follow-up. P2: Fix stale "5 tools" comment in task-agent-tool-schemas.ts. Now correctly says "6 tools" and includes list_group_members in the list. P3: Remove misleading "unrestricted" language from list_group_members response message. Without relay_message, "unrestricted" is no longer accurate.
0a0844f to
4ae7d13
Compare
Remove 5 relay_message tests (sections 8 and 11) and renumber remaining sections 7/9/10/11/13 to 6/7/8/9/10.
Walks the full auth lifecycle (login, authenticated call, logout, credential removal, stale-token cleanup, failed credential, re-add after removal) on the Codex bridge provider — the regression target — parametrized over OAuth and API-key auth modes. Unlike the sibling per-scenario suite, this composes the stages on one instance to pin the cross-stage state-machine transitions that broke across stale-token and removal/re-add flows.
The relay_message tool is being removed in favor of the unified
send_message tool that all agents (Task Agent and step agents) use.
Changes:
task-agent-tool-schemas.test.ts, cross-agent-messaging.test.ts, and
cross-agent-messaging-integration.test.ts
Task Agent now uses send_message for peer communication like all other
agents, with channel topology uniformly enforced.