fix(openai_web_search): multi query web search - #1255
Merged
Merged
Conversation
Parse and execute the current `action.queries` format while preserving legacy `action.query` compatibility. Reject invalid query arrays without fallback, serialize matching client-visible actions and backend bridge arguments, and add coverage for multi-query dispatch and invalid input. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Count `max_tool_calls` in logical tool calls so a multi-query `web_search_call` costs one unit, and bound the (paid) provider requests with a separate per-continuation query cap spent across the whole batch. A call whose queries are clipped or whose later query fails keeps the results it gathered and reports `incomplete`; only a call that gathered nothing reports the bounded failure notice. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Every dispatch outcome for a pending web_search_call re-ran parse_search_request and re-derived the call's bridge id. Parse the batch once in prepare_calls, merge SearchRequest into PreparedCall, and let that struct own its SearchCallIds. Handling the malformed fork at the top of the dispatch loop reduces append_excess_incomplete and append_tool_limit_exceeded to single append_search_turn calls, so both are inlined. Behavior is unchanged. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Reformat the response search dispatch logic and tighten related documentation comments. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Move the missing-query, tool-limit, and over-budget branches out of the dispatch loop into admit_call so execute_pending_searches stays under the line limit. append_search_turn keeps its argument list behind an expect. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
r-papso
marked this pull request as ready for review
September 21, 2026 13:56
leseb
requested changes
Sep 22, 2026
leseb
left a comment
Collaborator
There was a problem hiding this comment.
Summary
PR #1255 correctly separates logical tool-call and provider-query budgets, but introduces two major correctness/conformance regressions. Three inline comments were posted in Conductor; none were posted to GitHub.
Findings
[MAJOR] Empty queries discards a valid legacy query
- File: mod.rs
- What:
{query: "legacy", queries: []}is treated as malformed instead of falling back toquery. - Why: The current OpenAI schema still permits deprecated
queryand places nominItemsconstraint onqueries. The previous code would execute this query. - Fix: Fall back to
legacy_queryfor an empty array; continue rejecting non-string members.
[MAJOR] Zero-result successes are mistaken for non-execution/failure
- File: mod.rs
- What: Partial state is derived from
results.is_empty(), althoughSearchOutcome::Results([])explicitly represents success. - Why: A successful zero-result query followed by failure becomes
failed/“Web search unavailable.” A cap-clipped call after successful zero-result searches says “Web search not performed.” Both misrepresent provider execution to the model. - Fix: Track successful queries separately from result-row count and produce a truthful partial/no-results message.
[NIT] New bare assertions violate the supplied convention
- File: tests.rs
- What: New
assert!calls lack diagnostic messages, including lines 458, 498, 1739, 1802, and 1907. - Fix: Add assertion messages.
Test-gap inventory
| File | Function/path | Tested | Gap |
|---|---|---|---|
web_search/mod.rs |
Empty queries plus legacy query |
Yes | Test enshrines schema-divergent behavior |
web_search/mod.rs |
Zero-result success followed by failure | No | Status and bridge become misleading |
web_search/mod.rs |
Query-cap clipping after zero-result successes | No | Bridge incorrectly says nothing ran |
web_search/tests.rs |
Multi-query budgets/results | Partial | Missing zero-result partial and exact 64/65 boundaries |
openai_agentic_loop.rs |
Buffered multi-query loop | Yes | No streaming or OpenAI SDK coverage |
When parsed_queries is Some(Array) but empty, fall back to legacy single-query behavior instead of rejecting the call. This ensures consistent handling of missing and empty query arrays. Addresses review feedback on PR praxis-proxy#1255. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Derive a dispatched call's status from the number of queries the provider answered rather than from whether any results were gathered. A query that returns no rows still succeeded, so a call that found nothing before a provider failure is now reported incomplete instead of failed, and one clipped by the query cap reports the clip rather than "not performed". Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Close the remaining test gaps from the PR review. Add one unit test pinning both sides of MAX_WEB_SEARCH_QUERIES_PER_CONTINUATION against an empty-results provider, so a call filling the cap exactly completes with the no-results notice and a call one query past it reports the clipped partial instead of claiming nothing ran. Add one integration test for the streamed multi-query round trip, asserting the fan-out stays a single logical tool item that costs one tool-call unit. Extract spawn_counting_body_mock so a counting mock can serve an empty result set. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
The multi-query tests added `assert!(matches!(action, FilterAction::Continue))` without diagnostics. Give each of the six the message for what its dispatch should do. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
PendingSearchBatch, PreparedCall, SearchCallIds, and the admission helpers are private items whose only consumer is WebSearchFilter, but they sat above it, in the slot CONTRIBUTING.md reserves for public types. Move them below the HttpFilter impl so the file reads constants -> public type and impls -> private types -> private helpers. Pure relocation; no lines changed. Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Contributor
Author
Collaborator
|
@r-papso test failure |
Signed-off-by: Rastislav Papso <rpapso@redhat.com>
Contributor
Author
|
@leseb should be OK now. |
leseb
approved these changes
Sep 23, 2026
leseb
enabled auto-merge
September 23, 2026 11:57
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 23, 2026
leseb
enabled auto-merge
September 24, 2026 07:54
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 24, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Sep 24, 2026
Contributor
Author
|
@leseb merge queue doesn't like me.. Do you know what could be the reason of timing-out? |
Collaborator
i was looking into it too, found a ci flake, will see if that helps |
The merge-queue ruleset requires the ubi-image check, but the FIPS workflow only triggered on push/pull_request/workflow_dispatch. It never ran on the merge_group ref, so the required check never reported and the queue evicted every entry after the 60-minute check-response timeout. Add the merge_group trigger so ubi-image runs on the queue ref and the merge queue can complete. Signed-off-by: Sébastien Han <seb@redhat.com>
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
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.
Summary
OpenAI replaced
action.querywith anaction.queriesarray in theweb_search_calloutput item, letting a model issue multi-keyword retrieval in a single logical tool call. Our budget check tookmin(MAX_WEB_SEARCH_CALLS_PER_CONTINUATION, remaining max_tool_calls), which conflates two things that are no longer equivalent: logical tool calls and (paid) provider requests.This PR parses and executes the new format and splits those two limits, so one call with N queries still costs one tool call.
Changes
action.queries, keeping legacyaction.querycompatibility.max_tool_callscounts logical tool calls (1web_search_call= 1 unit, per the decision in Valid multi-query web-search actions are treated as malformed #1151);MAX_WEB_SEARCH_QUERIES_PER_CONTINUATION(64) separately caps provider requests across the batch. A call whose queries are clipped or whose later query fails keeps the results it gathered and reportsincomplete; only a call that gathered nothing reports the bounded failure notice.prepare_calls/PreparedCall) instead of re-parsing per dispatch branch. No behavior change.Closes #1151
Validation
cargo test -p praxis-ai-apismake lintChecklist
Signed-off-bytrailer.Breaking changes
No breaking change, deprecated
action.queryis still supported.