fix: ensure the BuiltInTools entry exists before incrementing CallCount #1754
+7
−2
Conversation
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
…nse in stream handler
WalkthroughAdds guard checks in OpenAI response handlers: validates tool lookup before incrementing CallCount in OaiResponsesHandler, and adds a nil check for streamResponse.Response in OaiResponsesStreamHandler on "response.completed" to avoid nil pointer dereference. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OaiResponsesStreamHandler
participant StreamResponse
Client->>OaiResponsesStreamHandler: Stream event "response.completed"
OaiResponsesStreamHandler->>StreamResponse: Access Response?
alt Response is non-nil
OaiResponsesStreamHandler->>StreamResponse: Read Response.Usage
OaiResponsesStreamHandler-->>Client: Continue/finish
else Response is nil
note over OaiResponsesStreamHandler: Skip usage access to avoid nil deref
OaiResponsesStreamHandler-->>Client: Continue/finish
end
sequenceDiagram
participant Request
participant OaiResponsesHandler
participant BuiltInTools
Request->>OaiResponsesHandler: Handle tool usage
OaiResponsesHandler->>BuiltInTools: Lookup tool by type
alt Tool exists
OaiResponsesHandler->>BuiltInTools: Increment CallCount
else Tool missing
note over OaiResponsesHandler: Log error and continue
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/openai/relay_responses.go (1)
101-103: Remaining panic risk: incrementing CallCount without safe lookupThis still dereferences a possibly nil map holder and/or nil entry. Mirror the guarded pattern used above.
- info.ResponsesUsageInfo.BuiltInTools[dto.BuildInToolWebSearchPreview].CallCount++ + if info != nil && info.ResponsesUsageInfo != nil && info.ResponsesUsageInfo.BuiltInTools != nil { + if toolInfo, ok := info.ResponsesUsageInfo.BuiltInTools[dto.BuildInToolWebSearchPreview]; ok && toolInfo != nil { + toolInfo.CallCount++ + } else { + logger.LogError(c, "BuiltInTools not found or nil for web search tool") + } + } else { + logger.LogError(c, "ResponsesUsageInfo or BuiltInTools is nil; skip tool usage aggregation") + }
🧹 Nitpick comments (1)
relay/channel/openai/relay_responses.go (1)
49-57: Extract and use a safe increment helper for BuiltInTools.CallCount
- Replace direct increments in both locations with a centralized helper:
- Lines 49–57 (loop over responsesResponse.Tools)
- Lines 99–104 (switch on streamResponse.Item.Type)
- Add in this package:
func safeIncBuiltInTool(ctx context.Context, info *relaycommon.RelayInfo, key string) { if info == nil || info.ResponsesUsageInfo == nil || info.ResponsesUsageInfo.BuiltInTools == nil { logger.LogError(ctx, "ResponsesUsageInfo or BuiltInTools is nil; skip tool usage aggregation") return } if ti, ok := info.ResponsesUsageInfo.BuiltInTools[key]; ok && ti != nil { ti.CallCount++ return } logger.LogError(ctx, fmt.Sprintf("BuiltInTools not found or nil for tool type: %s", key)) }
- Update both sites to call
safeIncBuiltInTool(c, info, common.Interface2String(tool["type"]))andsafeIncBuiltInTool(c, info, dto.BuildInToolWebSearchPreview).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
relay/channel/openai/relay_responses.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
relay/channel/openai/relay_responses.go (4)
relay/common/relay_info.go (1)
ResponsesUsageInfo(50-52)common/utils.go (1)
Interface2String(118-136)logger/logger.go (1)
LogError(63-65)dto/openai_response.go (1)
Usage(217-230)
🔇 Additional comments (1)
relay/channel/openai/relay_responses.go (1)
80-93: Nice: nil-check prevents deref on response.completedThe added guard on streamResponse.Response and .Usage addresses the reported panic path. LGTM.
Comment on lines
+51
to
+56
| buildToolinfo, ok := info.ResponsesUsageInfo.BuiltInTools[common.Interface2String(tool["type"])] | ||
| if !ok { | ||
| logger.LogError(c, fmt.Sprintf("BuiltInTools not found for tool type: %v", tool["type"])) | ||
| continue | ||
| } | ||
| buildToolinfo.CallCount++ |
There was a problem hiding this comment.
Also guard nil map holder and nil entry before incrementing CallCount
Good start with the map lookup, but two panic paths remain: info.ResponsesUsageInfo (or its BuiltInTools) can be nil, and the map value itself can be a nil *BuildInToolInfo. Add both checks.
- buildToolinfo, ok := info.ResponsesUsageInfo.BuiltInTools[common.Interface2String(tool["type"])]
- if !ok {
- logger.LogError(c, fmt.Sprintf("BuiltInTools not found for tool type: %v", tool["type"]))
- continue
- }
- buildToolinfo.CallCount++
+ // Defensive: holder/map and entry must exist and be non-nil
+ if info == nil || info.ResponsesUsageInfo == nil || info.ResponsesUsageInfo.BuiltInTools == nil {
+ logger.LogError(c, "ResponsesUsageInfo or BuiltInTools is nil; skip tool usage aggregation")
+ continue
+ }
+ toolType := common.Interface2String(tool["type"])
+ buildToolinfo, ok := info.ResponsesUsageInfo.BuiltInTools[toolType]
+ if !ok || buildToolinfo == nil {
+ logger.LogError(c, fmt.Sprintf("BuiltInTools not found or nil for tool type: %v", tool["type"]))
+ continue
+ }
+ buildToolinfo.CallCount++📝 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.
Suggested change
| buildToolinfo, ok := info.ResponsesUsageInfo.BuiltInTools[common.Interface2String(tool["type"])] | |
| if !ok { | |
| logger.LogError(c, fmt.Sprintf("BuiltInTools not found for tool type: %v", tool["type"])) | |
| continue | |
| } | |
| buildToolinfo.CallCount++ | |
| // Defensive: holder/map and entry must exist and be non-nil | |
| if info == nil || info.ResponsesUsageInfo == nil || info.ResponsesUsageInfo.BuiltInTools == nil { | |
| logger.LogError(c, "ResponsesUsageInfo or BuiltInTools is nil; skip tool usage aggregation") | |
| continue | |
| } | |
| toolType := common.Interface2String(tool["type"]) | |
| buildToolinfo, ok := info.ResponsesUsageInfo.BuiltInTools[toolType] | |
| if !ok || buildToolinfo == nil { | |
| logger.LogError(c, fmt.Sprintf("BuiltInTools not found or nil for tool type: %v", tool["type"])) | |
| continue | |
| } | |
| buildToolinfo.CallCount++ |
🤖 Prompt for AI Agents
In relay/channel/openai/relay_responses.go around lines 51-56, the code assumes
info.ResponsesUsageInfo and its BuiltInTools map exist and that the looked-up
value is non-nil before incrementing CallCount; add guards: first check info !=
nil and info.ResponsesUsageInfo != nil and info.ResponsesUsageInfo.BuiltInTools
!= nil and handle the nil map (either initialize the map if mutation is allowed
or log/continue), then perform the map lookup, and if the retrieved
buildToolinfo is nil either allocate a new BuildInToolInfo struct and store it
back into the map before incrementing CallCount or log/continue to avoid a nil
dereference.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix: #1748
在操作前进行检查，解决因为工具值无法转换，或者异常结构导致返回空值，在空值上操作导致程序panic
Summary by CodeRabbit