-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add stateful Jupyter Notebook option and make browser more reliable #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
self.reasoning_item_ids: list[str] = [] | ||
self.current_reasoning_item_id: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update final response builder for browser fallback
The streaming code now normalizes functions.browser.*
recipients via _resolve_browser_recipient
, so events trigger the browser tool even when the model emits reserved names under the functions.
namespace. However, the non‑streaming path that builds the final ResponseObject
still relies on is_not_builtin_tool
and only recognizes recipients starting with "browser."
. If the model emits functions.browser.search
, streaming will invoke the browser, but the final response will contain a function_call
item instead of a web_search_call
, causing stored responses or non‑SSE clients to report the wrong action. Consider reusing _resolve_browser_recipient
(or similar logic) in generate_response
so both streaming and final responses classify browser calls consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex Can you please fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
- Added shared browser recipient resolution constants and helper to unify tool name normalization across response paths. gpt_oss/responses_api/api_server.pyL74-L122
- Updated the final response builder to convert
functions.browser.*
emissions into proper web search calls using the shared resolver before falling back to generic function calls. gpt_oss/responses_api/api_server.pyL181-L274 - Reused the new resolver in the streaming event pipeline so both streaming and stored responses classify browser invocations consistently. gpt_oss/responses_api/api_server.pyL405-L474
Testing
⚠️ Not run (not requested)
This PR tackles two things:
functions.browser.search
as opposed tobrowser.search
as long as there is no conflicting function defined.