Skip to content

feat(tool): add Brave Search as a selectable web_search provider - #321

Closed
LeaderOnePro wants to merge 5 commits into
vllm-project:mainfrom
LeaderOnePro:feat/brave-web-search
Closed

LeaderOnePro wants to merge 5 commits into
vllm-project:mainfrom
LeaderOnePro:feat/brave-web-search

Conversation

@LeaderOnePro

@LeaderOnePro LeaderOnePro commented Sep 17, 2026

Copy link
Copy Markdown

Summary

Closes #294 (Phase 2 of #291).

Add Brave Search as a selectable web_search provider alongside You.com, selected via
AGENTIC_WEB_SEARCH_PROVIDER (env) > [web_search].provider (config file) > default "you".

Provider behavior (crates/agentic-server-core/src/tool/web_search/brave.rs):

  • GET /res/v1/web/search with X-Subscription-Token auth and result_filter=web,news,
    normalized into the existing You.com-shaped model-facing wire (no change to the output
    schema: Brave results serialize through the same struct as You.com results).
  • count clamped to the free-tier 20-results-per-section cap (with a debug log); freshness
    mapped to Brave's format.
  • No server-side domain filtering: include_domains/exclude_domains are post-filtered
    client-side with label-boundary matching, IDNA normalization, trailing-dot handling, and
    fail-closed behavior on unparseable URLs.
  • At most one request in flight: providers declare a ceiling via WebSearchProvider::max_concurrent_requests,
    and cap_provider_concurrency folds it into the gateway-wide limit (hard 1 for Brave —
    the ~1 QPS free-tier limit is service-imposed, not an operator knob).
  • 429 responses surface Retry-After verbatim (the header may also be an HTTP-date, so no
    unit is appended); error bodies are read through the existing size-limited reader.

Configuration plumbing:

  • api_key_env now falls back to the selected provider's default variable (YOU_API_KEY /
    BRAVE_API_KEY) when unset, and the generated config file no longer hardcodes
    YOU_API_KEY — switching providers never requires hand-editing the config file.
  • AGENTIC_WEB_SEARCH_PROVIDER is parsed through the same serde wire names the config file
    accepts (WebSearchProviderKind::parse_name, strum-backed VARIANTS), trimmed and
    case-insensitive, with the variant list in the error derived rather than hand-maintained.
  • You.com path is unchanged when no provider is configured (base URL override stays
    YOU_API_BASE_URL; Brave uses AGENTIC_WEB_SEARCH_BASE_URL).

Docs: Brave configuration section in docs/deploying/kubernetes.md; README documents the
api_key_env fallback and the cross-provider include/exclude semantics difference
(You.com rejects the combination, Brave applies the blocklist on top of the allowlist).

Dependency note: strum is added solely for VariantNames/EnumIter on the
2-variant WebSearchProviderKind, keeping the config error message's variant list in
lockstep with what serde actually accepts (enforced by a round-trip test). A hand-written
VARIANTS array would avoid the dependency but reintroduces the drift this exists to
prevent; the lockfile adds exactly two crates, strum and strum_macrosheck, syn,
quote, and proc-macro2 are already in the tree via other derives.

Deviations from the issue

  • Provider name in metadata[]: the issue's Q5 default decision was to surface the
    provider name in metadata. This PR defers that — WebSearchProviderMetadata.provider
    remains wire-invisible so the model-facing payload schema is unchanged (no fields added
    or removed); surfacing it is an additive wire change that recorded replays and strict
    clients would still need to absorb, so it is better landed deliberately as its own change.
  • AGENTIC_WEB_SEARCH_MAX_CONCURRENT_QUERIES: not implemented. Brave's ~1 QPS guidance
    is a service-side limit, and a tunable knob would only invite 429s. The provider-ceiling
    plumbing (max_concurrent_requests + cap_provider_concurrency) is in place if a future
    provider has a genuinely configurable budget.

Test Plan

  • uv tool run --from pre-commit pre-commit run --all-files — all hooks pass.
  • cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings — clean.
  • cargo test --workspace — 64 suites, 0 failures (including 697 tests in
    agentic-server-core lib and the 30-test web_search_tool_test integration suite).
  • scripts/check_rust_file_sizes.py — all 134 production files within the 500-line policy.
  • New coverage for this PR: Axum mock integration tests for 200 (web+news), empty results,
    401, and 429 + Retry-After propagation; DomainFilter label-boundary/IDNA/fail-closed
    cases; an end-to-end concurrency test asserting (via in-flight atomic counters on the mock
    server) that a gateway limit of 5 still serializes Brave to exactly 1 concurrent and
    exactly 3 total requests; parse_name trimmed/case-insensitive and round-trip tests;
    api-key-env fallback tests. All provider interactions run against local Axum mocks — no
    replay cassettes or live-provider calls are involved.
  • Review fixes applied on this branch (api-key fallback bug, verbatim Retry-After,
    unreachable branch removal, serde-backed provider parsing, docs) were verified by multiple
    independent review passes, including a full re-review of the fix commit.

Signed-off-by: LeaderOnePro LeaderOnePro@outlook.com

Add the second web_search provider (Brave) behind the typed provider
contract established in vllm-project#291 Phase 1, and wire provider selection through
the configuration layer.

- config: add `Brave` to `WebSearchProviderKind` (key env `BRAVE_API_KEY`,
  display "Brave"); `WebSearchProviderConfig` now carries the selected
  `kind` alongside the resolved credential and base URL.
- tool/web_search/brave.rs: `BraveSearchProvider` shapes requests against
  `GET /res/v1/web/search` (`X-Subscription-Token`, `result_filter=web,news`)
  and normalizes the response onto the provider-neutral response. Brave has
  no server-side domain filtering, so `include_domains`/`exclude_domains` are
  post-filtered client-side via `DomainFilter`; `count` is clamped to its 20
  cap; the free-tier ~1 QPS rate limit is respected through a provider
  concurrency ceiling of 1. A blank base URL falls back to the Brave default
  endpoint, and 429s surface the upstream `Retry-After` hint without retry.
- tool/web_search/args.rs: add `DomainFilter` (label-boundary host suffix
  match, fail-closed for unparseable URLs) and the `ProviderConcurrency`
  ceiling helper; wire `DomainFilter` now that a provider needs it.
- tool/executors.rs: build the `web_search` handler from the resolved
  provider kind.
- server: `config_file` gains a `[web_search].provider` key and `main`
  resolves it with precedence `AGENTIC_WEB_SEARCH_PROVIDER` > file >
  "you"; non-You providers read `AGENTIC_WEB_SEARCH_BASE_URL`.

You.com behavior is unchanged when no provider is selected.

Closes vllm-project#294 (Phase 2 of vllm-project#291)

Signed-off-by: LeaderOnePro <LeaderOnePro@outlook.com>
- Fall back to the provider's default API key env (e.g. BRAVE_API_KEY) when
  the config file omits api_key_env and stop hardcoding YOU_API_KEY in the
  generated config, so switching providers no longer requires hand-editing
- Parse AGENTIC_WEB_SEARCH_PROVIDER through the same serde wire names the
  config file accepts (strum-backed VARIANTS), with trimmed, case-insensitive
  input, and fix the malformed "expected one of" error quoting
- Pass Retry-After through verbatim, drop the unreachable base_url branch and
  the misleading BRAVE_API_BASE_URL constant, and log count clamping
- Document Brave configuration in the Kubernetes guide and the api_key_env
  fallback plus cross-provider include/exclude semantics in the README
- Add a direct Brave concurrency-ceiling assertion, parse_name tests, and
  mock server lifecycle hygiene

Signed-off-by: LeaderOnePro <LeaderOnePro@outlook.com>
…ip choice

- Assert the concurrency mock received exactly three requests, closing the
  false-negative gap where serialized metadata could pass without every
  query reaching the backend, and hold the mock task in an RAII guard so a
  panic between spawn and assertions cannot leak it
- Document that parse_name deliberately round-trips through serde instead
  of strum's EnumString: FromStr would derive a parallel name set kept in
  sync only by tests, whereas routing through serde makes file and
  environment parsing consistent by construction
- Record the strum dependency's intent in the workspace manifest

Signed-off-by: LeaderOnePro <LeaderOnePro@outlook.com>
@franciscojavierarceo

Copy link
Copy Markdown
Collaborator

Freshness::Range is accepted and then dropped when building the Brave request, and safesearch is never forwarded. a date-limited search therefore becomes unrestricted, while safesearch: "strict" falls back to Brave’s default behavior. we should preserve both supported options and assert the actual HTTP query parameters in the tests.

this also overlaps with #324, which handles those options and the provider metadata/concurrency settings requested in #294. i’m preparing that implementation for merge; we should consolidate any remaining useful coverage there rather than land two competing provider implementations.

@LeaderOnePro

Copy link
Copy Markdown
Author

Thanks for the review! safesearch was an oversight on my part. On Freshness::Range: dropping it was deliberate — Brave's short codes (pd/pw/pm/py) have no date-range equivalent, so the filter is omitted with a debug log — but I agree the observable result is the same, and #324 handles ranges properly.

#324 is the more complete implementation, so I'm closing this in its favor. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Add Brave Search as a selectable web_search provider

2 participants