[WRONG BRANCH] fix(responses): preserve forced hosted web_search choice - #255
[WRONG BRANCH] fix(responses): preserve forced hosted web_search choice#255luvs01 wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe response parser now maps ChangesHosted web-search routing
Estimated code review effort: 1 (Trivial) | ~5 minutes Mergeability Score: ⚪ Minimal · up to This localized parser change preserves forced hosted web-search behavior without introducing a supported merge-blocking correctness, security, or availability risk. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
![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
🤖 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 `@tests/responses-parser.test.ts`:
- Around line 164-176: Add a focused test alongside the existing forced hosted
web_search test in the responses parser suite, using tool_choice type
"web_search_preview" and asserting parsed.options.toolChoice equals { name:
"web_search" }. Keep the same request setup and preserve the existing web_search
coverage.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: d8bc499e-8d5d-4dab-a66d-1d6d033f1207
📒 Files selected for processing (2)
src/responses/parser.tstests/responses-parser.test.ts
| test("maps a forced hosted web_search choice to the synthetic routed tool", () => { | ||
| const parsed = parseRequest({ | ||
| model: "umans/umans-kimi-k2.7", | ||
| input: "search", | ||
| tools: [ | ||
| { type: "web_search" }, | ||
| { type: "function", name: "run_shell", parameters: { type: "object" } }, | ||
| ], | ||
| tool_choice: { type: "web_search" }, | ||
| }); | ||
|
|
||
| expect(parsed.options.toolChoice).toEqual({ name: "web_search" }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add coverage for web_search_preview.
The parser now handles both web_search and web_search_preview, but this test covers only web_search. Add a second case that expects { name: "web_search" } for tool_choice: { type: "web_search_preview" }.
As per path instructions, behavior changes in src/ should have focused regression coverage in tests/.
🤖 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 `@tests/responses-parser.test.ts` around lines 164 - 176, Add a focused test
alongside the existing forced hosted web_search test in the responses parser
suite, using tool_choice type "web_search_preview" and asserting
parsed.options.toolChoice equals { name: "web_search" }. Keep the same request
setup and preserve the existing web_search coverage.
Source: Path instructions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9840c5c2
ℹ️ 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (t === "web_search" || t === "web_search_preview") { | ||
| return { name: WEB_SEARCH_TOOL_NAME }; |
There was a problem hiding this comment.
For a routed Responses request whose tool declaration and choice both use the supported web_search_preview type, this new branch sets toolChoice to { name: "web_search" }, but extractHostedWebSearch() recognizes only type === "web_search". Consequently _webSearch remains unset, planWebSearch() returns no plan, the synthetic tool is never injected, and adapter filtering removes every unrelated declared tool, so the model produces an ordinary answer without performing the forced search. Normalize or extract web_search_preview as a hosted search too, and add a routed-planning regression case for this branch.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
1 participant
Motivation
{ type: "web_search" }shape that the internal parser mapped to"auto", which widened the advertised tool set and allowed routed models to access unrelated client/local tools.web_searchdeclarations to the internal synthetic routed web-search tool so sidecar injection does not leak other declared tools.Description
mapToolChoiceinsrc/responses/parser.tsto maptype: "web_search"andtype: "web_search_preview"to the synthetic routed tool viaWEB_SEARCH_TOOL_NAMEinstead of falling through to"auto".tests/responses-parser.test.tsthat asserts a forced hostedweb_searchtool_choiceis parsed to the synthetic routedweb_searchtool even when other client tools are declared.Testing
bun test tests/responses-parser.test.ts, and the updatedResponses parsertests passed.bun run typecheck, which completed successfully.bun run privacy:scan, which completed successfully.Codex Task
Summary by CodeRabbit