Conversation
Implement support for delegating tasks to sub-agents by enhancing the collectTools function to create delegate tools for each sub-agent referenced in an agent's spec. This feature allows parent agents to dispatch work to specialized sub-agents. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
| } | ||
|
|
||
| // Check that the context about the user/channel is provided based on the channel type | ||
| // todo(dex) why does this happen at runtime in the agent controller and not when the contact channel is created? |
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e779a02 in 3 minutes and 43 seconds
More details
- Looked at
1830lines of code in18files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. acp/test/utils/toolcall.go:33
- Draft comment:
Avoid hardcoding the 'acp.humanlayer.dev/toolcallrequest' label value ("test123"). Consider parameterizing it for better test flexibility. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. acp/test/utils/toolcall.go:31
- Draft comment:
Avoid using a hardcoded label value ('test123'); consider parameterizing it or adding a clarifying comment for test flexibility. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. README.md:523
- Draft comment:
Typo: In the 'Adding Tools with MCP' section, please remove the apostrophe in "Agent's aren't that interesting without tools" to read "Agents aren't that interesting without tools." - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. README.md:953
- Draft comment:
Typo: Please capitalize 'next' to 'Next' at the beginning of the sentence "next, we can create a router agent that can delegate to the web search agent." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
While technically correct that sentences should start with capital letters, this is a very minor stylistic issue in documentation. The rules state not to make comments that are obvious or unimportant. Capitalization fixes fall into this category - they don't affect functionality and are trivial to spot.
The comment is technically correct about proper English capitalization rules. The change would make the documentation slightly more professional looking.
While correct, this type of minor stylistic fix is exactly what the rules say to avoid - it's an obvious and unimportant change that doesn't affect functionality or understanding.
Delete this comment. It points out a trivial capitalization issue which falls under the "obvious and unimportant" category that we should not comment on.
5. README.md:1203
- Draft comment:
Typo: In the userMessage for the human-expert-task, there's a duplicate 'the' in "Ask an expert what the the fastest animal on the planet is." Please remove one instance to correct it. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. acp/api/v1alpha1/toolcall_types.go:38
- Draft comment:
The comment on the ToolType field (currently stating 'ToolType identifies the type of the tool (MCP, HumanContact)') is outdated, as it should now include 'DelegateToAgent' to be consistent with the updated file-level comment and constant definitions. Please update this comment to reflect all available tool types. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. acp/internal/controller/toolcall/toolcall_controller.go:1118
- Draft comment:
Typographical error: In the comment above, 'the current namends up like -' should be corrected to something like 'the current name ends up like -' for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%
This comment is pointing out a typographical error in a comment within the code. While it is not directly related to the functionality or logic of the code, correcting typographical errors can improve code readability and maintainability. However, it does not suggest a change in the code's behavior or logic, nor does it ask for tests or confirmation of intent. It is more of an informative comment about a non-functional aspect of the code.
8. acp/internal/controller/toolcall/toolcall_controller_test.go:52
- Draft comment:
Typo noticed in the test description: 'todo wth is an MCPTool...' might be better written as 'todo what is an MCPTool...'. Consider fixing this for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_lKvO7yWjiE6IeOXK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| BeforeEach(func() { | ||
| // Create a test LLM | ||
| llm := &acp.LLM{ |
There was a problem hiding this comment.
What was the reason for not using TestLLM from utils?
There was a problem hiding this comment.
oh i tried to refactor this test to follow the patterns better, looks like it didn't pick up the "use utils" instructions.
anyhow fine to improve this more later
| // Check for LLMRequestError with 4xx status code | ||
| // todo(dex) this .As() casting does not work - this error still retries forever | ||
| // | ||
| // langchain API call failed: API returned unexpected status code: 400: An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. The following tool_call_ids did not have response messages: call_N38DB1obDYZF0yDYxZhK6lTe |
There was a problem hiding this comment.
I ran into this too, we gotta deal with this I think
There was a problem hiding this comment.
yeah added to backlog
| } | ||
|
|
||
| // For delegate tools, extract the agent name from the format "delegate_to_agent__agentName" | ||
| parts := strings.Split(tc.Spec.ToolRef.Name, "__") |
There was a problem hiding this comment.
Not gonna fight hard on it just yet, but it bothers me this is the way we're now doing this across all three types. We should consider just passing down an additional field.
There was a problem hiding this comment.
i mean at a certain point, "function name" is a single parameter we expose to the LLM, so at some point we're gonna have to parse a single name back into a multi-field object.
we could contort the jsonschema we feed to the llm to make it call like "delegate" and inject a parameter "agent_name": ..., I guess
agree this could use a clean up - open to proposals.
…:dexhorthy/smallchain into dexter/eng-1116-delegation-to-sub-agents
Important
Add sub-agent delegation functionality to allow agents to delegate tasks to sub-agents, with validation and testing.
AgentSpecinagent_types.goto includeSubAgentsandDescriptionfields.validateSubAgents()inagent_controller.go.processDelegateToAgent()intoolcall_controller.go.agent_controller_test.go,collect_tools_test.go, andtoolcall_controller_test.go.README.mdto include a section on delegating to a sub-agent.kustomization.yamlto update the controller image tag.This description was created by
for e779a02. It will automatically update as commits are pushed.