feat(chat): delegate ShowResults to fast subagent (macro-1993)#4267
Open
ehayes2000 wants to merge 1 commit into
Open
feat(chat): delegate ShowResults to fast subagent (macro-1993)#4267ehayes2000 wants to merge 1 commit into
ehayes2000 wants to merge 1 commit into
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a "delegated tool" pattern to decouple argument generation from tool invocation. A new 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/cloud-storage/agent/src/agent_loop.rs (2)
305-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror reasoning deltas into
AssistantContextPart::Thinking.Thinking text is streamed to the client but never recorded in
assistant_context, so delegated tools lose part of the intended transcript context.💡 Proposed fix
diff --git a/rust/cloud-storage/agent/src/agent_loop.rs b/rust/cloud-storage/agent/src/agent_loop.rs @@ other => { if !thinking_buf.is_empty() { - yield Ok(StreamPart::Thinking(std::mem::take(&mut thinking_buf))); + let flushed = std::mem::take(&mut thinking_buf); + assistant_context.push(ai_toolset::AssistantContextPart::Thinking( + flushed.clone(), + )); + yield Ok(StreamPart::Thinking(flushed)); } @@ if !thinking_buf.is_empty() { - yield Ok(StreamPart::Thinking(std::mem::take(&mut thinking_buf))); + let flushed = std::mem::take(&mut thinking_buf); + assistant_context.push(ai_toolset::AssistantContextPart::Thinking( + flushed.clone(), + )); + yield Ok(StreamPart::Thinking(flushed)); }Also applies to: 336-338
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/agent/src/agent_loop.rs` around lines 305 - 313, The reasoning deltas are being accumulated in thinking_buf and streamed to the client, but they are not being recorded in assistant_context, causing delegated tools to lose transcript context. In the ReasoningDelta match arm where reasoning is pushed to thinking_buf, also record the same reasoning content into assistant_context to ensure the thinking is preserved in the intended transcript. Apply the same fix at the second location mentioned in the comment range 336-338.
218-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
assistant_contextat the start of eachsend_messageturn.
assistant_contextis session-scoped and reused across calls, so delegated tools can read stale prior-turn output and growing transcripts instead of only the current turn.💡 Proposed fix
diff --git a/rust/cloud-storage/ai_toolset/src/context.rs b/rust/cloud-storage/ai_toolset/src/context.rs @@ impl AssistantContext { + /// Clear all accumulated parts. + pub fn clear(&self) { + if let Ok(mut parts) = self.0.write() { + parts.clear(); + } + } } diff --git a/rust/cloud-storage/agent/src/agent_loop.rs b/rust/cloud-storage/agent/src/agent_loop.rs @@ pub async fn send_message( &mut self, messages: Vec<Message>, ) -> Result<ChatCompletionStream<'_>, AgentError> { + self.assistant_context.clear(); self.history = messages;Also applies to: 241-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/agent/src/agent_loop.rs` around lines 218 - 223, The send_message method reuses assistant_context across calls, causing delegated tools to read stale data from previous turns. Add a reset or clear operation for the assistant_context field at the start of the send_message method (right after the method signature or before assigning self.history) to ensure each turn starts with a fresh context instead of accumulating data from prior calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/app/packages/core/component/AI/component/tool/Delegated.tsx`:
- Around line 32-33: The args function at lines 32-33 retrieves
ctx.response.args without validating its shape, which can cause delegated
renderers to receive invalid props. Add validation to ensure that the args value
is actually an object with the expected structure before returning it from the
args function. This validation should check that args is not null, is of type
object, and contains only the expected properties. The same validation approach
should be applied to the delegated renderer usage mentioned in lines 35-38 to
ensure consistent validation throughout the component.
In `@rust/cloud-storage/ai_tools/src/delegated.rs`:
- Around line 72-82: The DelegatedTool impl block for Deserialize is discarding
the Result from NoArgs::deserialize(deserializer) by using `let _`, which
silently ignores any deserialization errors and allows invalid payloads to
successfully deserialize. Replace the line `let _ =
NoArgs::deserialize(deserializer);` with `NoArgs::deserialize(deserializer)?;`
to properly propagate any deserialization errors back to the caller, ensuring
that only valid object payloads are accepted and malformed tool calls are
rejected.
In `@rust/cloud-storage/ai_toolset/src/context.rs`:
- Around line 26-132: The code additions to the ai_toolset crate (the
AssistantContextPart enum, AssistantContext struct with its implementation
methods, and RequestContext struct with its implementation) violate the repo
policy that prohibits modifications to rust/cloud-storage/ai_toolset. Either
move the AssistantContextPart, AssistantContext, and RequestContext
implementations to a different crate that is permitted to be modified, or obtain
explicit written approval from the appropriate code owners before merging this
change.
---
Outside diff comments:
In `@rust/cloud-storage/agent/src/agent_loop.rs`:
- Around line 305-313: The reasoning deltas are being accumulated in
thinking_buf and streamed to the client, but they are not being recorded in
assistant_context, causing delegated tools to lose transcript context. In the
ReasoningDelta match arm where reasoning is pushed to thinking_buf, also record
the same reasoning content into assistant_context to ensure the thinking is
preserved in the intended transcript. Apply the same fix at the second location
mentioned in the comment range 336-338.
- Around line 218-223: The send_message method reuses assistant_context across
calls, causing delegated tools to read stale data from previous turns. Add a
reset or clear operation for the assistant_context field at the start of the
send_message method (right after the method signature or before assigning
self.history) to ensure each turn starts with a fresh context instead of
accumulating data from prior calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06057d50-eba3-41ff-b676-7f04480c2431
⛔ Files ignored due to path filters (3)
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/tool.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/types.tsis excluded by!**/generated/**
📒 Files selected for processing (13)
js/app/packages/core/component/AI/component/tool/Delegated.tsxjs/app/packages/core/component/AI/component/tool/DisplayResults.tsxrust/cloud-storage/agent/src/agent_loop.rsrust/cloud-storage/agent/src/hook.rsrust/cloud-storage/ai_tools/src/delegated.rsrust/cloud-storage/ai_tools/src/display_results.rsrust/cloud-storage/ai_tools/src/lib.rsrust/cloud-storage/ai_toolset/src/context.rsrust/cloud-storage/ai_toolset/src/lib.rsrust/cloud-storage/chat/src/domain/service/chat.rsrust/cloud-storage/mcp_service/src/tool_service.rsrust/cloud-storage/prompt/src/display_results_delegate.rsrust/cloud-storage/prompt/src/lib.rs
Comment on lines
+32
to
+33
| const args = () => | ||
| (ctx.response as { args?: Record<string, unknown> } | undefined)?.args; |
Contributor
There was a problem hiding this comment.
Validate delegated response.args shape before rendering.
ctx.response is force-cast and trusted as an object payload. If args is malformed (e.g., non-object), delegated renderers can receive invalid props and fail unpredictably.
Suggested fix
- const args = () =>
- (ctx.response as { args?: Record<string, unknown> } | undefined)?.args;
+ const args = () => {
+ const raw = (ctx.response as { args?: unknown } | undefined)?.args;
+ if (raw && typeof raw === 'object' && !Array.isArray(raw)) {
+ return raw as Record<string, unknown>;
+ }
+ return undefined;
+ };Also applies to: 35-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/app/packages/core/component/AI/component/tool/Delegated.tsx` around lines
32 - 33, The args function at lines 32-33 retrieves ctx.response.args without
validating its shape, which can cause delegated renderers to receive invalid
props. Add validation to ensure that the args value is actually an object with
the expected structure before returning it from the args function. This
validation should check that args is not null, is of type object, and contains
only the expected properties. The same validation approach should be applied to
the delegated renderer usage mentioned in lines 35-38 to ensure consistent
validation throughout the component.
Comment on lines
+72
to
+82
| impl<'de, T> Deserialize<'de> for DelegatedTool<T> { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| // Accept (and discard) any/empty object the primary agent emits. | ||
| let _ = NoArgs::deserialize(deserializer); | ||
| Ok(DelegatedTool { | ||
| _phantom: PhantomData, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow deserialization failures in the delegated no-args wrapper.
Line 78 ignores the NoArgs::deserialize result, so invalid payloads (including non-object JSON) can still deserialize successfully. This weakens the “name-only object” contract and lets malformed tool calls through silently.
Suggested fix
impl<'de, T> Deserialize<'de> for DelegatedTool<T> {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
- // Accept (and discard) any/empty object the primary agent emits.
- let _ = NoArgs::deserialize(deserializer);
+ // Accept only an object payload and discard any fields.
+ let _ =
+ std::collections::BTreeMap::<String, serde::de::IgnoredAny>::deserialize(deserializer)?;
Ok(DelegatedTool {
_phantom: PhantomData,
})
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/ai_tools/src/delegated.rs` around lines 72 - 82, The
DelegatedTool impl block for Deserialize is discarding the Result from
NoArgs::deserialize(deserializer) by using `let _`, which silently ignores any
deserialization errors and allows invalid payloads to successfully deserialize.
Replace the line `let _ = NoArgs::deserialize(deserializer);` with
`NoArgs::deserialize(deserializer)?;` to properly propagate any deserialization
errors back to the caller, ensuring that only valid object payloads are accepted
and malformed tool calls are rejected.
Comment on lines
+26
to
+132
| /// A single part of the assistant's response, captured as it streams so that | ||
| /// tools running mid-turn can see what the primary agent has produced so far. | ||
| /// | ||
| /// This is a self-contained, dependency-free mirror of the streamed assistant | ||
| /// content (the `agent` crate maps its richer stream parts onto this). It backs | ||
| /// the "delegated tool" pattern, where a name-only tool call is fulfilled by a | ||
| /// secondary agent that needs the current assistant response as context. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum AssistantContextPart { | ||
| /// Plain text produced by the assistant in this turn. | ||
| Text(String), | ||
| /// Reasoning / thinking text produced by the assistant in this turn. | ||
| Thinking(String), | ||
| /// A tool call the assistant made in this turn, rendered as | ||
| /// `name(json-args)` for context. | ||
| ToolCall { | ||
| /// The tool name. | ||
| name: String, | ||
| /// The JSON arguments the assistant passed. | ||
| args: serde_json::Value, | ||
| }, | ||
| } | ||
|
|
||
| /// Accumulates the assistant message parts of the in-flight response. | ||
| /// | ||
| /// Shared (via [`Arc`]) between the agent stream — which appends parts as they | ||
| /// stream — and the [`RequestContext`] handed to tool calls, so a tool can read | ||
| /// everything the assistant has emitted in the current turn. Cleared at the | ||
| /// start of each new turn by the producer. | ||
| #[derive(Clone, Debug, Default)] | ||
| pub struct AssistantContext(Arc<RwLock<Vec<AssistantContextPart>>>); | ||
|
|
||
| impl AssistantContext { | ||
| /// Create an empty assistant context. | ||
| pub fn new() -> Self { | ||
| Self::default() | ||
| } | ||
|
|
||
| /// Append a part produced by the assistant. | ||
| pub fn push(&self, part: AssistantContextPart) { | ||
| if let Ok(mut parts) = self.0.write() { | ||
| parts.push(part); | ||
| } | ||
| } | ||
|
|
||
| /// Snapshot the parts accumulated so far in the current turn. | ||
| pub fn snapshot(&self) -> Vec<AssistantContextPart> { | ||
| self.0.read().map(|p| p.clone()).unwrap_or_default() | ||
| } | ||
|
|
||
| /// Render the accumulated parts as a plain-text transcript suitable for | ||
| /// feeding to a secondary agent as context. Returns `None` if empty. | ||
| pub fn to_transcript(&self) -> Option<String> { | ||
| let parts = self.snapshot(); | ||
| if parts.is_empty() { | ||
| return None; | ||
| } | ||
| let mut out = String::new(); | ||
| for part in parts { | ||
| match part { | ||
| AssistantContextPart::Text(text) => { | ||
| out.push_str(&text); | ||
| out.push('\n'); | ||
| } | ||
| AssistantContextPart::Thinking(thinking) => { | ||
| out.push_str("[thinking] "); | ||
| out.push_str(&thinking); | ||
| out.push('\n'); | ||
| } | ||
| AssistantContextPart::ToolCall { name, args } => { | ||
| out.push_str("[tool call] "); | ||
| out.push_str(&name); | ||
| out.push('('); | ||
| out.push_str(&args.to_string()); | ||
| out.push_str(")\n"); | ||
| } | ||
| } | ||
| } | ||
| Some(out) | ||
| } | ||
| } | ||
|
|
||
| /// Request context passed into tool calls, containing per-request data like user identity. | ||
| #[derive(Clone, Debug)] | ||
| pub struct RequestContext { | ||
| /// The ID of the user making the request. | ||
| pub user_id: MacroUserIdStr<'static>, | ||
| /// The assistant message parts produced so far in the current response. | ||
| /// | ||
| /// Populated by the agent stream as the assistant generates; read by tools | ||
| /// (e.g. delegated tools) that need the current response as context. Empty | ||
| /// for non-streaming dispatch paths. | ||
| pub assistant_context: AssistantContext, | ||
| } | ||
|
|
||
| impl RequestContext { | ||
| /// Build a request context with an empty assistant context. | ||
| /// | ||
| /// Use this on non-streaming dispatch paths (single tool calls outside the | ||
| /// agent loop) where no in-flight assistant response exists. | ||
| pub fn new(user_id: MacroUserIdStr<'static>) -> Self { | ||
| Self { | ||
| user_id, | ||
| assistant_context: AssistantContext::new(), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Move this implementation out of ai_toolset or get an explicit exception before merge.
This change modifies rust/cloud-storage/ai_toolset, which is explicitly marked as non-modifiable by repo policy.
As per coding guidelines, rust/cloud-storage/ai_toolset/**/*.rs: "You are not allowed to ever change this crate."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/ai_toolset/src/context.rs` around lines 26 - 132, The code
additions to the ai_toolset crate (the AssistantContextPart enum,
AssistantContext struct with its implementation methods, and RequestContext
struct with its implementation) violate the repo policy that prohibits
modifications to rust/cloud-storage/ai_toolset. Either move the
AssistantContextPart, AssistantContext, and RequestContext implementations to a
different crate that is permitted to be modified, or obtain explicit written
approval from the appropriate code owners before merging this change.
Source: Coding guidelines
Introduce a reusable delegated-tool pattern: the primary agent sees a name-only tool, and a fast (Haiku) secondary agent composes the real arguments from the in-flight assistant response, returned as the tool result. Apply it to DisplayResults (Dynamic UI) so the slow, token-heavy view is generated by the fast subagent instead of the primary agent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ehayes2000
force-pushed
the
ehayes2000/macro-1993-featchat-delegate-showresults-to-fast-subagent
branch
from
01b6d2a to
bb0ac58
Compare
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
1 participant
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.
Adds a reusable delegated-tool pattern in which the primary agent calls a name-only tool and a fast secondary agent composes the real arguments from the current assistant response, and converts the DisplayResults (Dynamic UI) tool to use it so the slow, token-heavy view is generated by the fast subagent instead of the primary agent.
Task: macro-1993
🤖 Generated with Claude Code