New data-tooluse AI SDK packet and Tool Approvals Implemented#2407
New data-tooluse AI SDK packet and Tool Approvals Implemented#2407
Conversation
…nai backend to support. has status field, and new description field.
WalkthroughAdds structured tool-use support end-to-end: new UIMessageDataToolUse type, tool-use status/approval constants, and ToolUseData on WaveToolCall. ToolUseData is created during OpenAI streaming, carried into UI messages (parts of type "data-tooluse"), and stripped from function-call inputs before downstream calls. Implements a concurrency-safe approval registry (timeouts + keep-alive), RPC/WS handlers and a frontend RPC method, plus an AIToolUse React component with polling. SSEHandlerCh gains AiMsgData for data-* events. Telemetry TEventProps extended, SyncMap gains SetUnless/TestAndSet, and several dev-only AI UI gates removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)pkg/aiusechat/tools_readfile.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (9)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
419-427: Return the slice element’s address, not the range copy.
&toolpoints at the loop variable copy, so any mutation on the returned pointer won’t flow back toopts.Tools/opts.TabTools. Please take the address of the slice element instead (e.g.&opts.Tools[i]) to preserve shared state.- for _, tool := range opts.Tools { - if tool.Name == toolName { - return &tool - } - } + for i := range opts.Tools { + if opts.Tools[i].Name == toolName { + return &opts.Tools[i] + } + } - for _, tool := range opts.TabTools { - if tool.Name == toolName { - return &tool - } - } + for i := range opts.TabTools { + if opts.TabTools[i].Name == toolName { + return &opts.TabTools[i] + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/app/aipanel/aimessage.tsx(3 hunks)frontend/app/aipanel/aitypes.ts(1 hunks)pkg/aiusechat/openai/openai-backend.go(13 hunks)pkg/aiusechat/openai/openai-convertmessage.go(1 hunks)pkg/aiusechat/tools_screenshot.go(1 hunks)pkg/aiusechat/uctypes/usechat-types.go(6 hunks)pkg/aiusechat/usechat.go(3 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(2 hunks)pkg/web/sse/ssehandler.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/aiusechat/usechat.go (4)
pkg/aiusechat/uctypes/usechat-types.go (8)
WaveChatOpts(404-416)UIMessageDataToolUse(127-133)WaveToolCall(149-154)AIMetrics(217-233)AIToolResult(271-276)ToolUseStatusError(123-123)ToolUseStatusCompleted(124-124)WaveStopReason(156-170)pkg/aiusechat/openai/openai-backend.go (1)
UpdateToolUseData(336-365)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)
pkg/aiusechat/openai/openai-backend.go (2)
pkg/aiusechat/uctypes/usechat-types.go (7)
UIMessageDataToolUse(127-133)WaveChatOpts(404-416)AIOptsType(181-192)WaveContinueResponse(173-178)ToolUseStatusPending(122-122)ToolUseStatusError(123-123)WaveToolCall(149-154)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)
pkg/aiusechat/openai/openai-convertmessage.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessagePart(35-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (9)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
72-121: LGTM! Well-structured telemetry additions.The new WaveAI* telemetry fields comprehensively capture tool-use metrics (counts, errors, timing, detail maps) and align with the PR's goal of richer tool-use tracking. Field naming and JSON tagging are consistent with existing patterns.
pkg/aiusechat/usechat.go (4)
203-211: Anthropic implementation gap is acknowledged.The function only implements tool-use data updates for OpenAI and logs a warning for Anthropic. This aligns with the PR description stating the implementation is "not currently provided for the Anthropic integration."
393-393: LGTM! Improved encapsulation.Moving tool definition resolution to
chatOpts.GetToolDefinitionimproves encapsulation and aligns with the new tool-definition flow introduced in this PR.
481-503: LGTM! Telemetry mapping aligns with new fields.The telemetry event construction correctly maps all the new tool-use metrics (ToolUseCount, ToolUseErrorCount, ToolDetail, etc.) to the corresponding WaveAI* fields added in
telemetrydata.go.
213-266: SSEHandlerCh.AiMsgData signature verified: method defined asAiMsgData(dataType string, id string, data interface{}) errorinpkg/web/sse/ssehandler.go; matches usage.frontend/app/aipanel/aitypes.ts (1)
13-19: LGTM! Type definition aligns with backend.The new
toolusetype structure matches the backendUIMessageDataToolUsetype and uses TypeScript string literal unions for the status field, providing good type safety.frontend/app/aipanel/aimessage.tsx (3)
95-115: LGTM! Clear and intuitive tool-use rendering.The rendering logic for
data-tooluseparts is well-structured with:
- Intuitive status icons (✓ completed, ✗ error, • pending) and color coding
- Proper conditional rendering of optional fields (tooldesc, errormessage)
- Consistent styling with the existing message UI
127-133: LGTM! Maintains backward compatibility.The updated
isDisplayPartfunction correctly includesdata-toolusewhile maintaining backward compatibility with the previoustool-state-based parts.
141-147: LGTM! Content detection includes tool-use parts.The updated
hasContentcheck correctly treatsdata-tooluseparts as content, ensuring tool-use messages display appropriately instead of showing "(no text content)".
| // Create tool use data entry | ||
| toolUseData := &uctypes.UIMessageDataToolUse{ | ||
| ToolCallId: st.toolCallID, | ||
| ToolName: st.toolName, | ||
| Status: uctypes.ToolUseStatusPending, | ||
| } | ||
|
|
||
| // Try to get tool definition | ||
| toolDef := state.chatOpts.GetToolDefinition(st.toolName) | ||
| if toolDef == nil { | ||
| toolUseData.Status = uctypes.ToolUseStatusError | ||
| toolUseData.ErrorMessage = "tool not found" | ||
| } else { | ||
| // Try to unmarshal arguments | ||
| var parsedArgs any | ||
| if err := json.Unmarshal([]byte(ev.Arguments), &parsedArgs); err != nil { | ||
| toolUseData.Status = uctypes.ToolUseStatusError | ||
| toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) | ||
| } else { | ||
| // Get tool description if available | ||
| if toolDef.ToolInputDesc != nil { | ||
| toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| state.toolUseData[st.toolCallID] = toolUseData | ||
| log.Printf("AI data-tooluse %s\n", st.toolCallID) | ||
| _ = sse.AiMsgData("data-tooluse", st.toolCallID, *toolUseData) | ||
| } |
There was a problem hiding this comment.
Populate ToolDesc even when ToolInputDesc is absent.
Right now ToolDesc stays empty unless the tool defines ToolInputDesc, so most tools ship a blank description to the UI. That breaks the richer status display this PR introduces. Initialize the description with toolDef.Desc() (using your new helper) and only override it with the input-specific text when parsing succeeds.
- toolUseData := &uctypes.UIMessageDataToolUse{
- ToolCallId: st.toolCallID,
- ToolName: st.toolName,
- Status: uctypes.ToolUseStatusPending,
- }
+ toolUseData := &uctypes.UIMessageDataToolUse{
+ ToolCallId: st.toolCallID,
+ ToolName: st.toolName,
+ Status: uctypes.ToolUseStatusPending,
+ }
@@
- if toolDef == nil {
+ if toolDef == nil {
toolUseData.Status = uctypes.ToolUseStatusError
toolUseData.ErrorMessage = "tool not found"
} else {
- // Try to unmarshal arguments
+ toolUseData.ToolDesc = toolDef.Desc()
+ // Try to unmarshal arguments
var parsedArgs any
if err := json.Unmarshal([]byte(ev.Arguments), &parsedArgs); err != nil {
toolUseData.Status = uctypes.ToolUseStatusError
toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err)
} else {
- // Get tool description if available
+ // Get tool description if available
if toolDef.ToolInputDesc != nil {
- toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs)
+ if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" {
+ toolUseData.ToolDesc = desc
+ }
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create tool use data entry | |
| toolUseData := &uctypes.UIMessageDataToolUse{ | |
| ToolCallId: st.toolCallID, | |
| ToolName: st.toolName, | |
| Status: uctypes.ToolUseStatusPending, | |
| } | |
| // Try to get tool definition | |
| toolDef := state.chatOpts.GetToolDefinition(st.toolName) | |
| if toolDef == nil { | |
| toolUseData.Status = uctypes.ToolUseStatusError | |
| toolUseData.ErrorMessage = "tool not found" | |
| } else { | |
| // Try to unmarshal arguments | |
| var parsedArgs any | |
| if err := json.Unmarshal([]byte(ev.Arguments), &parsedArgs); err != nil { | |
| toolUseData.Status = uctypes.ToolUseStatusError | |
| toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) | |
| } else { | |
| // Get tool description if available | |
| if toolDef.ToolInputDesc != nil { | |
| toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs) | |
| } | |
| } | |
| } | |
| state.toolUseData[st.toolCallID] = toolUseData | |
| log.Printf("AI data-tooluse %s\n", st.toolCallID) | |
| _ = sse.AiMsgData("data-tooluse", st.toolCallID, *toolUseData) | |
| } | |
| // Create tool use data entry | |
| toolUseData := &uctypes.UIMessageDataToolUse{ | |
| ToolCallId: st.toolCallID, | |
| ToolName: st.toolName, | |
| Status: uctypes.ToolUseStatusPending, | |
| } | |
| // Try to get tool definition | |
| toolDef := state.chatOpts.GetToolDefinition(st.toolName) | |
| if toolDef == nil { | |
| toolUseData.Status = uctypes.ToolUseStatusError | |
| toolUseData.ErrorMessage = "tool not found" | |
| } else { | |
| // Populate with default description | |
| toolUseData.ToolDesc = toolDef.Desc() | |
| // Try to unmarshal arguments | |
| var parsedArgs any | |
| if err := json.Unmarshal([]byte(ev.Arguments), &parsedArgs); err != nil { | |
| toolUseData.Status = uctypes.ToolUseStatusError | |
| toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) | |
| } else { | |
| // Get tool description if available | |
| if toolDef.ToolInputDesc != nil { | |
| if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" { | |
| toolUseData.ToolDesc = desc | |
| } | |
| } | |
| } | |
| } | |
| state.toolUseData[st.toolCallID] = toolUseData | |
| log.Printf("AI data-tooluse %s\n", st.toolCallID) | |
| _ = sse.AiMsgData("data-tooluse", st.toolCallID, *toolUseData) |
🤖 Prompt for AI Agents
In pkg/aiusechat/openai/openai-backend.go around lines 812 to 841, ToolDesc is
only set when ToolInputDesc exists and parsing succeeds, leaving most tools with
an empty description; initialize toolUseData.ToolDesc to toolDef.Desc() (your
new helper) right after retrieving toolDef, then if ToolInputDesc is present and
argument unmarshalling succeeds, override ToolDesc with the input-specific
description; ensure this default assignment happens before parsing and error
handling so UI always receives a description.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/aiusechat/tools_readfile.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/aiusechat/tools_readfile.go (1)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(34-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/aiusechat/tools_readfile.go (1)
27-58: Hoist the shared default line-count constant
DefaultLineCountis still defined inside this helper whilereadTextFileCallbackredeclares it andToolInputDeschard-codes100. This keeps the duplication that was called out earlier and risks the two paths diverging. Please promote the constant to package scope and reuse it everywhere (parser, callback, and descriptor).-const StopReasonMaxBytes = "max_bytes" +const ( + StopReasonMaxBytes = "max_bytes" + DefaultLineCount = 100 +)-func parseReadTextFileInput(input any) (*readTextFileParams, error) { - const DefaultLineCount = 100 - +func parseReadTextFileInput(input any) (*readTextFileParams, error) {- count := 100 + count := DefaultLineCount- if origin == "start" && offset == 0 && count == 100 { + if origin == "start" && offset == 0 && count == DefaultLineCount {pkg/aiusechat/openai/openai-backend.go (1)
833-848: Populate ToolDesc even without ToolInputDesc.We still ship blank descriptions for every tool that lacks
ToolInputDesc, so the new tool-use panel shows an empty body. InitializeToolDescfromtoolDef.Desc()before attempting the specialized override, and only replace it when the callback returns a non-empty string.func createToolUseData(toolCallID, toolName string, toolDef *uctypes.ToolDefinition, arguments string) *uctypes.UIMessageDataToolUse { toolUseData := &uctypes.UIMessageDataToolUse{ ToolCallId: toolCallID, ToolName: toolName, Status: uctypes.ToolUseStatusPending, } if toolDef == nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = "tool not found" return toolUseData } + toolUseData.ToolDesc = toolDef.Desc() + var parsedArgs any if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) return toolUseData } if toolDef.ToolInputDesc != nil { - toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs) + if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" { + toolUseData.ToolDesc = desc + } }
🧹 Nitpick comments (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
429-440: Return pointers to the real ToolDefinition entries.Using
for _, tool := range ... { return &tool }hands back a pointer to the loop variable copy; any later mutation (e.g., toggling approval callbacks) won’t touch the struct stored inTools/TabTools. Iterate by index so you return the actual slice element.- for _, tool := range opts.Tools { - if tool.Name == toolName { - return &tool - } - } - for _, tool := range opts.TabTools { - if tool.Name == toolName { - return &tool - } - } + for i := range opts.Tools { + if opts.Tools[i].Name == toolName { + return &opts.Tools[i] + } + } + for i := range opts.TabTools { + if opts.TabTools[i].Name == toolName { + return &opts.TabTools[i] + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/app/aipanel/aitypes.ts(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/aiusechat/openai/openai-backend.go(13 hunks)pkg/aiusechat/toolapproval.go(1 hunks)pkg/aiusechat/tools_readfile.go(2 hunks)pkg/aiusechat/uctypes/usechat-types.go(7 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/aipanel/aitypes.ts
🧰 Additional context used
🧬 Code graph analysis (8)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (2)
CommandWaveAIToolApproveData(731-735)RpcOpts(297-303)
pkg/wshrpc/wshserver/wshserver.go (4)
frontend/app/store/wshclientapi.ts (1)
WaveAIToolApproveCommand(496-498)pkg/wshrpc/wshclient/wshclient.go (1)
WaveAIToolApproveCommand(591-594)pkg/wshrpc/wshrpctypes.go (1)
CommandWaveAIToolApproveData(731-735)pkg/aiusechat/toolapproval.go (1)
UpdateToolApproval(57-83)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
WaveAIToolApproveCommand(496-498)pkg/wshutil/wshrpc.go (1)
WshRpc(46-60)pkg/wshrpc/wshrpctypes.go (2)
CommandWaveAIToolApproveData(731-735)RpcOpts(297-303)
pkg/aiusechat/tools_readfile.go (2)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(34-40)pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalNeedsApproval(129-129)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (1)
CommandWaveAIToolApproveData(731-735)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
WaveAIToolApproveCommand(496-498)pkg/wshrpc/wshclient/wshclient.go (1)
WaveAIToolApproveCommand(591-594)
pkg/aiusechat/toolapproval.go (1)
pkg/aiusechat/uctypes/usechat-types.go (2)
UIMessageDataToolUse(136-143)ApprovalTimeout(132-132)
pkg/aiusechat/openai/openai-backend.go (2)
pkg/aiusechat/uctypes/usechat-types.go (7)
UIMessageDataToolUse(136-143)WaveChatOpts(414-426)AIOptsType(191-202)ToolDefinition(78-90)ToolUseStatusPending(123-123)ToolUseStatusError(124-124)WaveToolCall(159-164)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/aiusechat/openai/openai-backend.go (1)
829-858: Populate ToolDesc even when ToolInputDesc is absent.
ToolDescremains empty unless the tool definesToolInputDesc, leaving most tools without a description in the UI. InitializetoolUseData.ToolDescwith a default from the tool definition (e.g.,toolDef.DescriptionortoolDef.ShortDescription) immediately after retrievingtoolDefon line 836, then optionally override it with the input-specific description fromToolInputDescif parsing succeeds.Apply this diff to set a default description:
if toolDef == nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = "tool not found" return toolUseData } + + // Set default description from tool definition + if toolDef.ShortDescription != "" { + toolUseData.ToolDesc = toolDef.ShortDescription + } else if toolDef.Description != "" { + toolUseData.ToolDesc = toolDef.Description + } var parsedArgs any if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) return toolUseData } if toolDef.ToolInputDesc != nil { - toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs) + if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" { + toolUseData.ToolDesc = desc + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/aipanel/aimessage.tsx(5 hunks)pkg/aiusechat/openai/openai-backend.go(13 hunks)pkg/aiusechat/toolapproval.go(1 hunks)pkg/aiusechat/tools.go(1 hunks)pkg/aiusechat/tools_readfile.go(5 hunks)pkg/aiusechat/uctypes/usechat-types.go(7 hunks)pkg/aiusechat/usechat.go(9 hunks)pkg/util/ds/syncmap.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/aiusechat/tools_readfile.go
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/aiusechat/tools.go (1)
pkg/aiusechat/tools_readfile.go (1)
GetReadTextFileToolDefinition(207-284)
pkg/aiusechat/toolapproval.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalTimeout(132-132)
frontend/app/aipanel/aimessage.tsx (3)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(24-24)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
pkg/aiusechat/openai/openai-backend.go (3)
pkg/aiusechat/uctypes/usechat-types.go (8)
UIMessageDataToolUse(136-143)WaveChatOpts(418-431)AIOptsType(195-206)ApprovalNeedsApproval(129-129)ToolDefinition(78-90)ToolUseStatusPending(123-123)ToolUseStatusError(124-124)WaveToolCall(163-168)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
pkg/aiusechat/usechat.go (7)
pkg/util/ds/syncmap.go (1)
MakeSyncMap(13-18)pkg/aiusechat/uctypes/usechat-types.go (11)
WaveChatOpts(418-431)UIMessageDataToolUse(136-143)WaveToolCall(163-168)AIMetrics(231-247)AIToolResult(285-290)ToolUseStatusError(124-124)ApprovalNeedsApproval(129-129)ApprovalUserDenied(131-131)ApprovalTimeout(132-132)ToolUseStatusCompleted(125-125)WaveStopReason(170-184)pkg/aiusechat/openai/openai-backend.go (1)
UpdateToolUseData(336-365)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)pkg/aiusechat/toolapproval.go (2)
WaitForToolApproval(99-116)RegisterToolApproval(48-58)pkg/aiusechat/openai/openai-convertmessage.go (1)
ConvertToolResultsToOpenAIChatMessage(362-416)
pkg/aiusechat/uctypes/usechat-types.go (1)
pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
🪛 Biome (2.1.2)
frontend/app/aipanel/aimessage.tsx
[error] 92-92: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (13)
pkg/aiusechat/usechat.go (7)
24-24: LGTM!The SyncMap initialization provides thread-safe tracking of active tools and chats, preventing race conditions during concurrent AI operations.
Also applies to: 45-46
207-215: LGTM!The helper correctly updates tool use data for OpenAI. The warning for Anthropic is expected per PR objectives (Anthropic implementation deferred).
217-304: LGTM!The tool call processing implementation is comprehensive:
- Validates ToolUseData presence
- Handles error status correctly
- Implements approval flow with proper status transitions
- Emits SSE updates at each state change
- Updates metrics including error counts
- Logs truncated input for debugging without exposing full data
306-335: LGTM!The orchestration correctly:
- Marks all tool calls as active with deferred cleanup (lines 308-310)
- Processes each tool call sequentially
- Converts results to API-specific message format
- Posts messages to chat store
Note: Conversion errors on lines 321, 330 are logged but not propagated, which is appropriate since tool execution errors are already captured in the result objects.
339-342: LGTM!The concurrency guard correctly prevents multiple simultaneous runs of the same chat using atomic test-and-set semantics, with guaranteed cleanup via defer.
407-407: LGTM!The integration of
processToolCallsand the telemetry updates are correct. All relevant metrics including the newWaveAIToolUseErrorCountare properly captured and sent.Also applies to: 529-547
600-604: LGTM!The
RegisterToolApprovalcallback is correctly wired intoWaveChatOpts, enabling the approval flow for tool usage.pkg/aiusechat/openai/openai-backend.go (6)
42-49: LGTM!The
ToolUseDatafield andClean()method correctly implement the pattern of attaching UI-specific metadata that must be stripped before sending to the external API. The method safely creates a copy without modifying the original.Also applies to: 97-104
326-331: LGTM!The streaming state additions correctly track per-call tool use data and propagate chat options needed for tool definition lookups during streaming.
336-365: LGTM!The
UpdateToolUseDatafunction correctly:
- Validates chat existence
- Searches for the target function call by CallId
- Creates new copies to avoid mutation
- Posts the updated message to the chat store
- Returns appropriate errors when chat or call is not found
423-424: LGTM!The
Clean()call correctly strips UI-onlyToolUseDatabefore sending function calls to the external API.
812-820: LGTM!The tool use data emission flow correctly:
- Retrieves tool definition
- Creates structured tool use data
- Stores it in streaming state for later attachment
- Registers approval request when needed
- Emits to frontend via SSE
883-900: LGTM!The ToolUseData attachment logic correctly:
- Looks up tool use data by CallId from the streaming state
- Attaches it to
WaveToolCallfor the stop reason- Attaches it to
OpenAIFunctionCallInputfor message storage- Logs when data is unexpectedly missing for debugging
This ensures the UI-specific tool status information is preserved throughout the message flow.
Also applies to: 909-920
| const AIToolUse = memo(({ part }: AIToolUseProps) => { | ||
| const toolData = part.data; | ||
| const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null); | ||
|
|
||
| if (!toolData) return null; | ||
|
|
||
| const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; | ||
| const statusColor = | ||
| toolData.status === "completed" | ||
| ? "text-green-400" | ||
| : toolData.status === "error" | ||
| ? "text-red-400" | ||
| : "text-gray-400"; | ||
|
|
||
| const effectiveApproval = userApprovalOverride || toolData.approval; | ||
|
|
||
| useEffect(() => { | ||
| if (effectiveApproval !== "needs-approval") return; | ||
|
|
||
| const interval = setInterval(() => { | ||
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | ||
| toolcallid: toolData.toolcallid, | ||
| keepalive: true, | ||
| }); | ||
| }, 4000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [effectiveApproval, toolData.toolcallid]); | ||
|
|
||
| const handleApprove = () => { | ||
| setUserApprovalOverride("user-approved"); | ||
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | ||
| toolcallid: toolData.toolcallid, | ||
| approval: "user-approved", | ||
| }); | ||
| }; | ||
|
|
||
| const handleDeny = () => { | ||
| setUserApprovalOverride("user-denied"); | ||
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | ||
| toolcallid: toolData.toolcallid, | ||
| approval: "user-denied", | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={cn("flex items-start gap-2 p-2 rounded bg-gray-800 border border-gray-700", statusColor)}> | ||
| <span className="font-bold">{statusIcon}</span> | ||
| <div className="flex-1"> | ||
| <div className="font-semibold">{toolData.toolname}</div> | ||
| {toolData.tooldesc && <div className="text-sm text-gray-400">{toolData.tooldesc}</div>} | ||
| {toolData.errormessage && <div className="text-sm text-red-300 mt-1">{toolData.errormessage}</div>} | ||
| {effectiveApproval === "needs-approval" && ( | ||
| <div className="mt-2 flex gap-2"> | ||
| <button | ||
| onClick={handleApprove} | ||
| className="px-3 py-1 border border-gray-600 text-gray-300 hover:border-gray-500 hover:text-white text-sm rounded cursor-pointer transition-colors" | ||
| > | ||
| Approve | ||
| </button> | ||
| <button | ||
| onClick={handleDeny} | ||
| className="px-3 py-1 border border-gray-600 text-gray-300 hover:border-gray-500 hover:text-white text-sm rounded cursor-pointer transition-colors" | ||
| > | ||
| Deny | ||
| </button> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Fix hook ordering and clear stale approval overrides
useEffect sits behind the if (!toolData) return null guard, so the hook is skipped on the initial render whenever the packet arrives without data (common mid-stream). React then sees a different hook order when toolData becomes available and throws. Lint already flags this. In addition, userApprovalOverride never resets when a new tool call replaces the part or when the backend reports a different approval; the stale override keeps future requests stuck in the old state so the UI can show “denied” for a brand-new tool and stop emitting keep-alives.
Restructure the component so hooks run unconditionally, gate the keep-alive effect internally, and reset the override when the toolcall or backend approval changes:
const AIToolUse = memo(({ part }: AIToolUseProps) => {
- const toolData = part.data;
- const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null);
-
- if (!toolData) return null;
-
- const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
- const statusColor =
- toolData.status === "completed"
- ? "text-green-400"
- : toolData.status === "error"
- ? "text-red-400"
- : "text-gray-400";
-
- const effectiveApproval = userApprovalOverride || toolData.approval;
-
- useEffect(() => {
- if (effectiveApproval !== "needs-approval") return;
-
- const interval = setInterval(() => {
- RpcApi.WaveAIToolApproveCommand(TabRpcClient, {
- toolcallid: toolData.toolcallid,
- keepalive: true,
- });
- }, 4000);
-
- return () => clearInterval(interval);
- }, [effectiveApproval, toolData.toolcallid]);
+ const toolData = part.data;
+ const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null);
+ const toolCallId = toolData?.toolcallid;
+ const effectiveApproval = userApprovalOverride ?? toolData?.approval ?? "";
+
+ useEffect(() => {
+ if (!toolCallId || effectiveApproval !== "needs-approval") {
+ return;
+ }
+ const interval = setInterval(() => {
+ RpcApi.WaveAIToolApproveCommand(TabRpcClient, {
+ toolcallid: toolCallId,
+ keepalive: true,
+ });
+ }, 4000);
+ return () => clearInterval(interval);
+ }, [effectiveApproval, toolCallId]);
+
+ useEffect(() => {
+ setUserApprovalOverride(null);
+ }, [toolCallId, toolData?.approval]);
+
+ if (!toolData) {
+ return null;
+ }
+
+ const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
+ const statusColor =
+ toolData.status === "completed"
+ ? "text-green-400"
+ : toolData.status === "error"
+ ? "text-red-400"
+ : "text-gray-400";With this shape the hook order stays fixed and the UI no longer carries approval decisions across different tool calls.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const AIToolUse = memo(({ part }: AIToolUseProps) => { | |
| const toolData = part.data; | |
| const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null); | |
| if (!toolData) return null; | |
| const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; | |
| const statusColor = | |
| toolData.status === "completed" | |
| ? "text-green-400" | |
| : toolData.status === "error" | |
| ? "text-red-400" | |
| : "text-gray-400"; | |
| const effectiveApproval = userApprovalOverride || toolData.approval; | |
| useEffect(() => { | |
| if (effectiveApproval !== "needs-approval") return; | |
| const interval = setInterval(() => { | |
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | |
| toolcallid: toolData.toolcallid, | |
| keepalive: true, | |
| }); | |
| }, 4000); | |
| return () => clearInterval(interval); | |
| }, [effectiveApproval, toolData.toolcallid]); | |
| const handleApprove = () => { | |
| setUserApprovalOverride("user-approved"); | |
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | |
| toolcallid: toolData.toolcallid, | |
| approval: "user-approved", | |
| }); | |
| }; | |
| const handleDeny = () => { | |
| setUserApprovalOverride("user-denied"); | |
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | |
| toolcallid: toolData.toolcallid, | |
| approval: "user-denied", | |
| }); | |
| }; | |
| return ( | |
| <div className={cn("flex items-start gap-2 p-2 rounded bg-gray-800 border border-gray-700", statusColor)}> | |
| <span className="font-bold">{statusIcon}</span> | |
| <div className="flex-1"> | |
| <div className="font-semibold">{toolData.toolname}</div> | |
| {toolData.tooldesc && <div className="text-sm text-gray-400">{toolData.tooldesc}</div>} | |
| {toolData.errormessage && <div className="text-sm text-red-300 mt-1">{toolData.errormessage}</div>} | |
| {effectiveApproval === "needs-approval" && ( | |
| <div className="mt-2 flex gap-2"> | |
| <button | |
| onClick={handleApprove} | |
| className="px-3 py-1 border border-gray-600 text-gray-300 hover:border-gray-500 hover:text-white text-sm rounded cursor-pointer transition-colors" | |
| > | |
| Approve | |
| </button> | |
| <button | |
| onClick={handleDeny} | |
| className="px-3 py-1 border border-gray-600 text-gray-300 hover:border-gray-500 hover:text-white text-sm rounded cursor-pointer transition-colors" | |
| > | |
| Deny | |
| </button> | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| const AIToolUse = memo(({ part }: AIToolUseProps) => { | |
| const toolData = part.data; | |
| const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null); | |
| const toolCallId = toolData?.toolcallid; | |
| const effectiveApproval = userApprovalOverride ?? toolData?.approval ?? ""; | |
| useEffect(() => { | |
| if (!toolCallId || effectiveApproval !== "needs-approval") { | |
| return; | |
| } | |
| const interval = setInterval(() => { | |
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | |
| toolcallid: toolCallId, | |
| keepalive: true, | |
| }); | |
| }, 4000); | |
| return () => clearInterval(interval); | |
| }, [effectiveApproval, toolCallId]); | |
| useEffect(() => { | |
| setUserApprovalOverride(null); | |
| }, [toolCallId, toolData?.approval]); | |
| if (!toolData) { | |
| return null; | |
| } | |
| const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; | |
| const statusColor = | |
| toolData.status === "completed" | |
| ? "text-green-400" | |
| : toolData.status === "error" | |
| ? "text-red-400" | |
| : "text-gray-400"; | |
| // …rest of component (handlers, JSX) remains unchanged | |
| }); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 92-92: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In frontend/app/aipanel/aimessage.tsx around lines 76-146, the component
conditionally returns before running hooks which breaks hook ordering and the
userApprovalOverride can become stale across tool calls; move all hooks
(useState and useEffect) to run unconditionally (above the early return), change
the keep-alive effect to internally no-op unless toolData exists and
effectiveApproval === "needs-approval", and add a useEffect that resets
userApprovalOverride to null whenever toolData?.toolcallid or toolData?.approval
changes; also update effect dependency arrays to include toolData.toolcallid and
toolData.approval so keep-alives and resets run correctly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/aiusechat/tools_readfile.go (1)
129-135: Add missingminhelper
Definefunc min(a, b int) int { if a < b { return a } return b }in this package or inline the comparison to avoid the undefinedminsymbol.
♻️ Duplicate comments (3)
pkg/aiusechat/tools_readfile.go (2)
28-61: Consolidate defaults and complete validation in parser.Avoid duplicating DefaultLineCount and add missing checks (origin, count>=1, normalize negative offset). Centralize default for consistency.
- func parseReadTextFileInput(input any) (*readTextFileParams, error) { - const DefaultLineCount = 100 + const DefaultLineCount = 100 + + func parseReadTextFileInput(input any) (*readTextFileParams, error) { result := &readTextFileParams{} @@ - if result.Count == nil { - count := DefaultLineCount - result.Count = &count - } - - return result, nil + if result.Count == nil { + count := DefaultLineCount + result.Count = &count + } + if *result.Count < 1 { + return nil, fmt.Errorf("count must be at least 1, got %d", *result.Count) + } + if *result.Origin != "start" && *result.Origin != "end" { + return nil, fmt.Errorf("invalid origin value %q: must be 'start' or 'end'", *result.Origin) + } + if *result.Offset < 0 { + zero := 0 + result.Offset = &zero + } + return result, nilFrom past review comments
249-278: Use DefaultLineCount in ToolInputDesc (avoid hardcoded 100).- count := 100 + count := DefaultLineCount if parsed.Count != nil { count = *parsed.Count }From past review comments
pkg/aiusechat/openai/openai-backend.go (1)
829-858: Initialize ToolDesc with default value before checking ToolInputDesc.Currently,
ToolDescremains empty unlessToolInputDescis defined (lines 849-851). Tools withoutToolInputDescwill ship a blank description to the UI, undermining the richer status display this PR introduces. InitializeToolDescwithtoolDef.Desc()immediately after the nil check (line 836), then override it ifToolInputDescprovides a more specific description.Apply this diff to provide a default description:
func createToolUseData(toolCallID, toolName string, toolDef *uctypes.ToolDefinition, arguments string) *uctypes.UIMessageDataToolUse { toolUseData := &uctypes.UIMessageDataToolUse{ ToolCallId: toolCallID, ToolName: toolName, Status: uctypes.ToolUseStatusPending, } if toolDef == nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = "tool not found" return toolUseData } + toolUseData.ToolDesc = toolDef.Desc() + var parsedArgs any if err := json.Unmarshal([]byte(arguments), &parsedArgs); err != nil { toolUseData.Status = uctypes.ToolUseStatusError toolUseData.ErrorMessage = fmt.Sprintf("failed to parse tool arguments: %v", err) return toolUseData } if toolDef.ToolInputDesc != nil { - toolUseData.ToolDesc = toolDef.ToolInputDesc(parsedArgs) + if desc := toolDef.ToolInputDesc(parsedArgs); desc != "" { + toolUseData.ToolDesc = desc + } } if toolDef.ToolApproval != nil { toolUseData.Approval = toolDef.ToolApproval(parsedArgs) } return toolUseData }
🧹 Nitpick comments (2)
pkg/util/ds/syncmap.go (1)
55-64: Document that testFn executes under the map lock.testFn runs while holding the mutex. If it blocks or re-enters SyncMap, it can deadlock. Add a doc comment advising testFn be fast/non‑blocking and not call back into SyncMap.
pkg/aiusechat/usechat.go (1)
306-316: Minor: avoid defers inside loop for cleanup (optional).Deferring Delete per iteration is fine for small N; alternatively collect IDs and delete in one deferred function to reduce defer overhead.
- for _, toolCall := range stopReason.ToolCalls { - activeToolMap.Set(toolCall.ID, true) - defer activeToolMap.Delete(toolCall.ID) - } + var ids []string + for _, c := range stopReason.ToolCalls { + activeToolMap.Set(c.ID, true) + ids = append(ids, c.ID) + } + defer func() { + for _, id := range ids { + activeToolMap.Delete(id) + } + }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/app/aipanel/aimessage.tsx(5 hunks)frontend/app/workspace/workspace-layout-model.ts(2 hunks)pkg/aiusechat/openai/openai-backend.go(13 hunks)pkg/aiusechat/toolapproval.go(1 hunks)pkg/aiusechat/tools.go(1 hunks)pkg/aiusechat/tools_readfile.go(5 hunks)pkg/aiusechat/uctypes/usechat-types.go(7 hunks)pkg/aiusechat/usechat.go(9 hunks)pkg/util/ds/syncmap.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/aiusechat/usechat.go (7)
pkg/util/ds/syncmap.go (1)
MakeSyncMap(13-18)pkg/aiusechat/uctypes/usechat-types.go (11)
WaveChatOpts(418-431)UIMessageDataToolUse(136-143)WaveToolCall(163-168)AIMetrics(231-247)AIToolResult(285-290)ToolUseStatusError(124-124)ApprovalNeedsApproval(129-129)ApprovalUserDenied(131-131)ApprovalTimeout(132-132)ToolUseStatusCompleted(125-125)WaveStopReason(170-184)pkg/aiusechat/openai/openai-backend.go (1)
UpdateToolUseData(336-365)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)pkg/aiusechat/toolapproval.go (2)
WaitForToolApproval(99-116)RegisterToolApproval(48-58)pkg/aiusechat/openai/openai-convertmessage.go (1)
ConvertToolResultsToOpenAIChatMessage(362-416)
pkg/aiusechat/tools.go (1)
pkg/aiusechat/tools_readfile.go (1)
GetReadTextFileToolDefinition(207-284)
pkg/aiusechat/toolapproval.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalTimeout(132-132)
pkg/aiusechat/tools_readfile.go (2)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(34-40)pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalNeedsApproval(129-129)
pkg/aiusechat/openai/openai-backend.go (3)
pkg/aiusechat/uctypes/usechat-types.go (8)
UIMessageDataToolUse(136-143)WaveChatOpts(418-431)AIOptsType(195-206)ApprovalNeedsApproval(129-129)ToolDefinition(78-90)ToolUseStatusPending(123-123)ToolUseStatusError(124-124)WaveToolCall(163-168)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
pkg/aiusechat/uctypes/usechat-types.go (1)
pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
frontend/app/aipanel/aimessage.tsx (3)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(24-24)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🪛 Biome (2.1.2)
frontend/app/aipanel/aimessage.tsx
[error] 92-92: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
frontend/app/workspace/workspace-layout-model.ts (1)
10-10: LGTM: Clean import removal.The removal of the
isDevimport is correct and aligns with the removal of development-mode gating throughout this file.pkg/aiusechat/tools.go (1)
128-130: Adding read_text_file tool is correct and well‑scoped behind widgetAccess.Tool includes approval gating; pairs well with screenshot tool. LGTM.
pkg/aiusechat/tools_readfile.go (1)
106-124: Path handling and directory guard look good.Home expansion, stat error paths, and directory rejection are correct and user-friendly.
pkg/aiusechat/usechat.go (4)
207-215: OpenAI chat update path is fine; Anthropic warn is acceptable placeholder.Clean separation; logs on failure avoid hard fail. LGTM.
217-304: Tool-use lifecycle handling (error/approval/completion) is sound.
- Validates ToolUseData, logs input safely, updates SSE and chat.
- Approval wait path sets error/status on deny/timeout.
Good structure.
337-343: Good guard against concurrent RunAIChat per chat.Atomic SetUnless prevents overlapping runs; deferred delete ensures cleanup. LGTM.
527-547: Telemetry fields extended appropriately.WaveAI fields cover tool-use counts and details. LGTM.
pkg/aiusechat/toolapproval.go (1)
60-116: LGTM! Deadlock issue from previous review has been resolved.The approval workflow implementation is now correct. The previous deadlock concern has been addressed by deferring cleanup to
WaitForToolApproval(lines 111-113) rather than attempting to unregister withinUpdateToolApprovalwhile holding the per-request lock. The timer callback at line 54 can now safely invokeUpdateToolApprovalwithout blocking.pkg/aiusechat/uctypes/usechat-types.go (4)
89-89: LGTM!The
ToolApprovalcallback enables tools to declare their approval requirements based on input arguments.
102-110: LGTM!The
Desc()helper appropriately prefersShortDescriptionand includes a nil-receiver check.
122-134: LGTM!The status and approval constants provide clear states for the tool-use lifecycle.
136-147: Empty Approval string treated as approved by design. All tools lacking aToolApprovalcallback default to no-approval-required (empty Approval passesIsApproved()); onlyread_text_filerequests explicit approval.pkg/aiusechat/openai/openai-backend.go (2)
97-104: LGTM!The
Clean()method properly strips internalToolUseDatabefore sending to external APIs.
336-365: LGTM!
UpdateToolUseDatacorrectly creates copies of the message and function call structures before updating, preventing unintended mutations of the original chat history.
| const AIToolUse = memo(({ part }: AIToolUseProps) => { | ||
| const toolData = part.data; | ||
| const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null); | ||
|
|
||
| if (!toolData) return null; | ||
|
|
||
| const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; | ||
| const statusColor = | ||
| toolData.status === "completed" | ||
| ? "text-green-400" | ||
| : toolData.status === "error" | ||
| ? "text-red-400" | ||
| : "text-gray-400"; | ||
|
|
||
| const effectiveApproval = userApprovalOverride || toolData.approval; | ||
|
|
||
| useEffect(() => { | ||
| if (effectiveApproval !== "needs-approval") return; | ||
|
|
||
| const interval = setInterval(() => { | ||
| RpcApi.WaveAIToolApproveCommand(TabRpcClient, { | ||
| toolcallid: toolData.toolcallid, | ||
| keepalive: true, | ||
| }); | ||
| }, 4000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [effectiveApproval, toolData.toolcallid]); | ||
|
|
There was a problem hiding this comment.
Fix conditional hook and unsafe dependency (can crash when part.data is null).
useEffect is after an early return and reads toolData.toolcallid unguarded. Call hooks unconditionally and guard nulls.
Apply this diff:
const AIToolUse = memo(({ part }: AIToolUseProps) => {
const toolData = part.data;
const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null);
-
- if (!toolData) return null;
+ const callId = toolData?.toolcallid;
+
+ // Keep-alive while awaiting approval
+ useEffect(() => {
+ if (effectiveApproval !== "needs-approval" || !callId) return;
+ const interval = setInterval(() => {
+ RpcApi.WaveAIToolApproveCommand(TabRpcClient, {
+ toolcallid: callId,
+ keepalive: true,
+ });
+ }, 4000);
+ return () => clearInterval(interval);
+ }, [effectiveApproval, callId]);
- const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
+ if (!toolData) return null;
+ const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•";
const statusColor =
toolData.status === "completed"
? "text-green-400"
: toolData.status === "error"
? "text-red-400"
: "text-gray-400";
const effectiveApproval = userApprovalOverride || toolData.approval;
-
- useEffect(() => {
- if (effectiveApproval !== "needs-approval") return;
-
- const interval = setInterval(() => {
- RpcApi.WaveAIToolApproveCommand(TabRpcClient, {
- toolcallid: toolData.toolcallid,
- keepalive: true,
- });
- }, 4000);
-
- return () => clearInterval(interval);
- }, [effectiveApproval, toolData.toolcallid]);As per static analysis hints
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 92-92: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
frontend/app/aipanel/aimessage.tsx around lines 76 to 104: the component
currently returns early before calling hooks (creating a conditional hook) and
the useEffect reads toolData.toolcallid unguarded which can crash when part.data
is null; fix by declaring hooks (useState, useEffect) unconditionally before any
early returns, compute effectiveApproval using userApprovalOverride ??
toolData?.approval, and in the effect guard for toolData existence (if
(!toolData || effectiveApproval !== "needs-approval") return;), use optional
chaining for the toolcallid when calling the RPC and put toolData?.toolcallid in
the dependency array so the effect won’t access a null property.
| this.aiPanelWrapperRef = null; | ||
| this.inResize = false; | ||
| this.aiPanelVisible = isDev(); | ||
| this.aiPanelVisible = true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the AI panel is now generally available and ready for production.
The AI panel now defaults to visible for all users (previously gated by isDev()). This is a significant UX change that exposes the new tool-use features to production.
Please confirm:
- This change is intentional and aligns with the product roadmap for making AI features generally available.
- All
isDev()checks related to AI panel functionality have been removed consistently across the codebase (frontend components, handlers, etc.). - The AI panel and tool-use features are fully functional and tested in production environments.
- Existing users who may have had implicit "closed" state won't experience issues (the code shows visibility is persisted in tab metadata at lines 76-82, which should handle this).
Run the following script to verify that all isDev() guards related to AI panel functionality have been removed:
🏁 Script executed:
#!/bin/bash
# Description: Search for remaining isDev() checks related to AI panel or WaveAI functionality.
# Search for isDev usage in frontend AI-related files
echo "=== Searching for isDev() in AI-related frontend files ==="
rg -n --type=ts -C3 'isDev\s*\(' frontend/app/aipanel/ frontend/app/workspace/
# Search for isDev usage specifically around aiPanel or waveai keywords
echo -e "\n=== Searching for isDev() near AI panel keywords ==="
rg -nP --type=ts -C5 'isDev\s*\(' | rg -C5 '(?i)(aipanel|waveai|ai.*panel)'
# Search for any commented-out isDev checks that might indicate incomplete removal
echo -e "\n=== Searching for commented isDev() references ==="
rg -n --type=ts '//.*isDev' frontend/app/aipanel/ frontend/app/workspace/Length of output: 2992
Remove leftover isDev() guards on AI features
The following dev‐only conditionals remain and must be removed so AI features are always available:
- frontend/app/workspace/widgets.tsx (Wave Dev Build label at lines ~222–225 & 246–249)
- frontend/app/tab/tabbar.tsx (waveaiButton at lines ~643–648)
Remove these isDev() checks to ensure consistent production visibility.
| func (opts *WaveChatOpts) GetToolDefinition(toolName string) *ToolDefinition { | ||
| for _, tool := range opts.Tools { | ||
| if tool.Name == toolName { | ||
| return &tool | ||
| } | ||
| } | ||
| for _, tool := range opts.TabTools { | ||
| if tool.Name == toolName { | ||
| return &tool | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: Fix pointer-to-loop-variable bug in GetToolDefinition.
Returning &tool at lines 436 and 441 creates a pointer to the loop variable, which is reused across iterations. All returned pointers reference the same memory location (the last iterated tool), causing incorrect tool definitions to be used at runtime.
Apply this diff to fix the bug by indexing into the slice:
func (opts *WaveChatOpts) GetToolDefinition(toolName string) *ToolDefinition {
- for _, tool := range opts.Tools {
- if tool.Name == toolName {
- return &tool
+ for i := range opts.Tools {
+ if opts.Tools[i].Name == toolName {
+ return &opts.Tools[i]
}
}
- for _, tool := range opts.TabTools {
- if tool.Name == toolName {
- return &tool
+ for i := range opts.TabTools {
+ if opts.TabTools[i].Name == toolName {
+ return &opts.TabTools[i]
}
}
return nil
}🤖 Prompt for AI Agents
In pkg/aiusechat/uctypes/usechat-types.go around lines 433 to 445, the function
GetToolDefinition returns &tool which is a pointer to the loop variable (reused
each iteration) and causes all returned pointers to point to the same memory;
change the loops to use index-based access and return pointers to the actual
slice elements (e.g. return &opts.Tools[i] and &opts.TabTools[j]) or otherwise
take the address of the slice element rather than the loop variable so each
returned pointer references the correct ToolDefinition.
sawka
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
pkg/aiusechat/tools_readfile.go (3)
28-61: Add missing validation to complete the function.The previous critical issue with the double pointer (line 37) has been fixed. However, the validation gaps identified in the earlier review remain:
- Missing origin validation (compare to lines 145-147): No check that
originis "start" or "end"- Missing count validation (compare to lines 157-159): No check that
count >= 1- Missing offset normalization (compare to lines 162-164): Negative offsets should be normalized to 0
Without these checks,
parseReadTextFileInputcannot serve as a complete validation function, limiting its reusability and creating inconsistency withreadTextFileCallback.Apply this diff to add the missing validation:
if result.Count == nil { count := DefaultLineCount result.Count = &count } + if *result.Count < 1 { + return nil, fmt.Errorf("count must be at least 1, got %d", *result.Count) + } + + if *result.Origin != "start" && *result.Origin != "end" { + return nil, fmt.Errorf("invalid origin value '%s': must be 'start' or 'end'", *result.Origin) + } + + if *result.Offset < 0 { + offset := 0 + result.Offset = &offset + } return result, nil }
29-29: Extract duplicate constant to module level.
DefaultLineCountis defined in bothparseReadTextFileInput(line 29) andreadTextFileCallback(line 88). This duplication creates a maintenance risk.Apply this diff to define the constant once at module level:
const StopReasonMaxBytes = "max_bytes" +const DefaultLineCount = 100 type readTextFileParams struct {Then remove the local definitions:
func parseReadTextFileInput(input any) (*readTextFileParams, error) { - const DefaultLineCount = 100 - result := &readTextFileParams{}func readTextFileCallback(input any) (any, error) { - const DefaultLineCount = 100 const DefaultMaxBytes = 50 * 1024 const ReadLimit = 1024 * 1024 * 1024Also applies to: 88-88
249-278: Replace hardcoded value with constant.The
ToolInputDescimplementation provides helpful human-readable descriptions. However, line 263 uses the hardcoded value100instead of referencingDefaultLineCount, creating a maintenance issue if the default changes.Apply this diff after extracting
DefaultLineCountto module level:- count := 100 + count := DefaultLineCount if parsed.Count != nil { count = *parsed.Count } - if origin == "start" && offset == 0 { + if origin == "start" && offset == 0 && count == DefaultLineCount { return fmt.Sprintf("reading %q (first %d lines)", parsed.Filename, count) } - if origin == "end" && offset == 0 { + if origin == "end" && offset == 0 && count == DefaultLineCount { return fmt.Sprintf("reading %q (last %d lines)", parsed.Filename, count) }frontend/app/aipanel/aimessage.tsx (1)
76-101: Fix null safety and add state reset for tool call changes.The component has two critical issues:
toolDataproperties are accessed without null checking (lines 80-88, 95). Ifpart.datais null—which past comments indicate can happen mid-stream—the component will crash when accessingtoolData.status,toolData.approval, ortoolData.toolcallid.
userApprovalOverridenever resets when the tool call changes. If the component instance is reused for a different tool call, the stale override persists, potentially showing "denied" for a brand-new tool and stopping keep-alive emissions.Based on past review comments
Apply this diff to add null safety and reset logic:
const AIToolUse = memo(({ part }: AIToolUseProps) => { const toolData = part.data; const [userApprovalOverride, setUserApprovalOverride] = useState<string | null>(null); + const toolCallId = toolData?.toolcallid; + const effectiveApproval = userApprovalOverride ?? toolData?.approval ?? ""; - const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; - const statusColor = - toolData.status === "completed" - ? "text-green-400" - : toolData.status === "error" - ? "text-red-400" - : "text-gray-400"; - - const effectiveApproval = userApprovalOverride || toolData.approval; - useEffect(() => { - if (effectiveApproval !== "needs-approval") return; + if (!toolCallId || effectiveApproval !== "needs-approval") { + return; + } const interval = setInterval(() => { RpcApi.WaveAIToolApproveCommand(TabRpcClient, { - toolcallid: toolData.toolcallid, + toolcallid: toolCallId, keepalive: true, }); }, 4000); return () => clearInterval(interval); - }, [effectiveApproval, toolData.toolcallid]); + }, [effectiveApproval, toolCallId]); + + useEffect(() => { + setUserApprovalOverride(null); + }, [toolCallId, toolData?.approval]); + + if (!toolData) { + return null; + } + + const statusIcon = toolData.status === "completed" ? "✓" : toolData.status === "error" ? "✗" : "•"; + const statusColor = + toolData.status === "completed" + ? "text-green-400" + : toolData.status === "error" + ? "text-red-400" + : "text-gray-400";
🧹 Nitpick comments (1)
frontend/app/aipanel/aimessage.tsx (1)
73-73: Remove unused prop.The
isStreamingprop is declared but never used in the component.Apply this diff to remove the unused prop:
interface AIToolUseProps { part: WaveUIMessagePart & { type: "data-tooluse" }; - isStreaming: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/aipanel/aimessage.tsx(5 hunks)frontend/app/tab/tabbar.tsx(3 hunks)pkg/aiusechat/tools_readfile.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/aipanel/aimessage.tsx (3)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(24-24)frontend/app/store/wshclientapi.ts (1)
RpcApi(537-537)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
pkg/aiusechat/tools_readfile.go (2)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(34-40)pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalNeedsApproval(129-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (6)
frontend/app/tab/tabbar.tsx (2)
9-9: LGTM! Dev-mode gate removed as intended.The removal of
isDevfrom the imports aligns with making the AI button always visible, promoting the AI features from development-only to general availability.
643-652: LGTM! AI button now always visible.The button implementation is clean and correct. The conditional styling based on
aiPanelOpenprovides clear visual feedback, and the click handler properly toggles the AI panel. This change makes AI features generally available to all users.pkg/aiusechat/tools_readfile.go (4)
106-124: LGTM: Path expansion and validation.The home directory expansion via
wavebase.ExpandHomeDiris correctly applied to all file operations. The directory check (lines 116-118) provides helpful guidance to users about using theread_dirtool instead.
198-198: LGTM: File mode in response.Adding the file mode to the response provides useful metadata about the file's permissions.
211-211: LGTM: Description documents approval requirement.The description update correctly informs users that this tool requires approval, aligning with the new
ToolApprovalfunction.
280-282: LGTM: Tool approval implementation.The
ToolApprovalfunction correctly returnsuctypes.ApprovalNeedsApproval, enabling the approval flow for this tool as described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/aiusechat/tools_readfile.go (3)
28-29: Extract duplicate constant to package level.
DefaultLineCountis defined in bothparseReadTextFileInput(line 29) andreadTextFileCallback(line 100), creating a maintenance issue if the default value needs to change.Apply this diff to define the constant once at package level:
package aiusechat import ( "fmt" "io" "os" "strings" "github.com/wavetermdev/waveterm/pkg/aiusechat/uctypes" "github.com/wavetermdev/waveterm/pkg/util/readutil" "github.com/wavetermdev/waveterm/pkg/util/utilfn" "github.com/wavetermdev/waveterm/pkg/wavebase" ) +const DefaultLineCount = 100 const StopReasonMaxBytes = "max_bytes"Then remove the duplicate definitions from both functions:
func parseReadTextFileInput(input any) (*readTextFileParams, error) { - const DefaultLineCount = 100 - result := &readTextFileParams{}func readTextFileCallback(input any) (any, error) { - const DefaultLineCount = 100 const DefaultMaxBytes = 50 * 1024
28-73: Past review comment remains unaddressed:readTextFileCallbackshould use this function.The past review comment suggested refactoring
readTextFileCallbackto useparseReadTextFileInputinternally to eliminate duplication. Currently,readTextFileCallback(lines 99-217) still contains its own validation logic, duplicating the work done here.Consider refactoring
readTextFileCallbackto use this function:func readTextFileCallback(input any) (any, error) { const DefaultMaxBytes = 50 * 1024 const ReadLimit = 1024 * 1024 * 1024 - var params readTextFileParams - if err := utilfn.ReUnmarshal(¶ms, input); err != nil { - return nil, fmt.Errorf("invalid input format: %w", err) + params, err := parseReadTextFileInput(input) + if err != nil { + return nil, err } - if params.Filename == "" { - return nil, fmt.Errorf("missing filename parameter") - } - maxBytes := DefaultMaxBytes if params.MaxBytes != nil { maxBytes = *params.MaxBytes }Then remove the duplicate validation logic from lines 152-176 since
parseReadTextFileInputalready handles it.
261-290: Replace hardcoded values withDefaultLineCountconstant.Lines 275 and 280 use the hardcoded value
100instead of theDefaultLineCountconstant, creating a maintenance issue if the default changes. This was flagged in a previous review comment.Apply this diff to use the constant:
ToolInputDesc: func(input any) string { parsed, err := parseReadTextFileInput(input) if err != nil { return fmt.Sprintf("error parsing input: %v", err) } origin := "start" if parsed.Origin != nil { origin = *parsed.Origin } offset := 0 if parsed.Offset != nil { offset = *parsed.Offset } - count := 100 + count := DefaultLineCount if parsed.Count != nil { count = *parsed.Count } - if origin == "start" && offset == 0 { + if origin == "start" && offset == 0 && count == DefaultLineCount { return fmt.Sprintf("reading %q (first %d lines)", parsed.Filename, count) } if origin == "end" && offset == 0 { return fmt.Sprintf("reading %q (last %d lines)", parsed.Filename, count) } if origin == "end" { return fmt.Sprintf("reading %q (from end: offset %d lines, count %d lines)", parsed.Filename, offset, count) } return fmt.Sprintf("reading %q (from start: offset %d lines, count %d lines)", parsed.Filename, offset, count) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/aiusechat/tools_readfile.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/aiusechat/tools_readfile.go (2)
pkg/util/utilfn/marshal.go (1)
ReUnmarshal(34-40)pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalNeedsApproval(129-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
pkg/aiusechat/tools_readfile.go (2)
118-136: LGTM! Excellent path handling and early directory detection.The path expansion logic and early directory detection improve usability and provide clear error messages guiding users to the appropriate tool.
292-294: LGTM! Approval function correctly implemented.The
ToolApprovalfunction correctly returnsApprovalNeedsApproval, aligning with the PR's objective to add tool approval support.
| if *result.Offset < 0 { | ||
| return nil, fmt.Errorf("offset must be non-negative, got %d", *result.Offset) | ||
| } |
There was a problem hiding this comment.
Inconsistent offset validation: returns error here but normalizes in readTextFileCallback.
parseReadTextFileInput returns an error for negative offsets, but readTextFileCallback (lines 174-176) normalizes negative offsets to 0. This inconsistency can cause confusion.
Since the past review comment suggested that readTextFileCallback should use parseReadTextFileInput internally, decide on one approach:
- Option 1 (Recommended): Normalize negative offsets to 0 in both places
- Option 2: Return an error for negative offsets in both places
If choosing Option 1, apply this diff to align the behavior:
if *result.Offset < 0 {
- return nil, fmt.Errorf("offset must be non-negative, got %d", *result.Offset)
+ offset := 0
+ result.Offset = &offset
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if *result.Offset < 0 { | |
| return nil, fmt.Errorf("offset must be non-negative, got %d", *result.Offset) | |
| } | |
| if *result.Offset < 0 { | |
| offset := 0 | |
| result.Offset = &offset | |
| } |
🤖 Prompt for AI Agents
In pkg/aiusechat/tools_readfile.go around lines 59-61, parseReadTextFileInput
currently returns an error for negative offsets while readTextFileCallback
normalizes negatives to 0, causing inconsistent behavior; change
parseReadTextFileInput to normalize negative offsets to 0 instead of returning
an error by setting result.Offset = pointerTo(0) (or equivalent) and update any
associated validations/messages accordingly so both functions treat negative
offsets the same (normalized to 0).
1 participant
provides richer information for FE to use to display tools. currently just shows pending/error/completed. but we'll use this in the future to do allow/deny flow for tools as well.
lots of backend plumbing to make it work in OpenAI. currently, not implemented for anthropic.