feat(web-search): inert backend union + routed web_search field fix (#2188 L6) - #2238
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Limit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThe change adds xAI, Gemini, and Exa web-search backend identifiers, defaults unset selection to OpenAI, adds Exa API key management, and removes unsupported OpenAI-only search fields from routed xAI requests. ChangesWeb-search backend expansion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ManagementAPI
participant Configuration
participant RoutedProvider
Operator->>ManagementAPI: Set webSearchSidecar.backend and exaApiKey
ManagementAPI->>Configuration: Validate and persist settings
ManagementAPI-->>Operator: Response without exaApiKey
Configuration->>RoutedProvider: Build selected backend request
RoutedProvider-->>Configuration: Remove unsupported OpenAI-only fields when required
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![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: a05f23fa93
ℹ️ 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".
리뷰 · 우선순위 64 / 80#2188 L6임. 베이스가 지금
같이 들어 있는 라우티드 L7 플랜
해결방안: L1-L5 먼저 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head a05f23fa93e434c5a4ddee15b8a4bc976ddeb6d4.
The stacked direction is sound, but three blockers remain before this layer can be accepted:
-
stripOpenAiOnlyWebSearchFieldsis called for every non-ChatGPT-forward Responses provider, not specifically xAI. I reproduced this with an official OpenAI API-key provider athttps://api.openai.com/v1: an input tool carryingexternal_web_access: falseandsearch_context_size: "low"reached the final wire body with both fields removed. Gate this rewrite on an xAI-specific provider identity/capability and add abuildRequestregression proving official OpenAI API-key traffic retains both fields. -
The public contract now accepts
xai,gemini, andexa, plus the write-onlyexaApiKey, but the English configuration reference and CLI help still advertise only the old backend pair and do not explain that the new arms are explicit-only/inert. Update the canonical docs/help and keep translated accepted-value lists consistent. -
exaApiKeywas added only toSENSITIVE_KEY_PATTERN. The shared string redactor still leaves all of these unchanged:
{"exaApiKey":"exa-canary-1234567890"}exaApiKey: exa-canary-1234567890exaApiKey=exa-canary-1234567890&model=x
Add the label to the shared colon/query/JSON string-redaction grammar and cover all three canaries in tests/redact.test.ts. The structured redactSecrets test is useful but does not exercise the error/log string path.
The focused exact-head suites pass 55/55, which confirms the current tests do not catch these boundaries. Please keep this PR stacked on codex/sidecar-cli; it should not be merged directly to dev.
Split the opt-in DTO into a write boolean vs read tri-state; record the concrete #2238 (3) and #2242 (5) review blockers in doc 120; recast doc 150 as the final aggregate gate with the full GUI/i18n/docs chain; replace temporal API-key rows with exact wire+tier assertions; state the explicit wp9->wp8->wp11->wp10 execution sequence.
|
All three blockers addressed in 19376f7: (1) the strip now activates only on an explicit provider capability denial — supportsOpenAiWebSearchToolFields: false, declared by the xAI registry entry — so official OpenAI API-key traffic and unclassified gateways retain external_web_access/search_context_size (buildRequest regression covers both directions, observed red before the fix); (2) English configuration reference + CLI help document the widened backend union (openai|anthropic|xai|gemini|exa, explicit-only arms, write-only exaApiKey) with fr/ja/ko/ru/tr/zh-cn/zh-tw kept consistent; (3) exaApiKey added to the shared colon/query/JSON string-redaction grammar with all three canaries in tests/redact.test.ts (JSON canary also observed red pre-fix). Fresh local: 46/46 on the two changed suites, adapter suites 80/0, tsc clean, privacy:scan pass, docs build 393 pages. Ready for re-review. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Approved on exact head 19376f73732578abd9fdc29305c9c91f9bf47698.
The reviewed blockers are resolved. Stripping of external_web_access and search_context_size now requires explicit provider capability denial, so official OpenAI API-key and unclassified Responses routes retain the caller fields while xAI removes the two proven-incompatible fields. The widened backend union and write-only exaApiKey contract are documented across locales and CLI help, and exaApiKey is covered by string redaction as well as structural redaction.
Independent local verification: 67 focused tests passed, bun run typecheck and bun run privacy:scan passed, and the docs site built all 393 pages. Exact-head required CI and React Doctor are green. This remains a stacked PR; merge it only in the intended stack order after its base layer is integrated.
f0c3cb3 to
bc828cc
Compare
The config union admits the three probed backends so settings can carry them, but planWebSearch fails closed for each until its executor layer lands — no plan means the request takes the normal routed path. resolveSidecarBackend stays explicit-only with the unset->openai pin. Both management routes validate the widened union; exaApiKey persists write/clear-only, never echoed, and redact.ts strips it from logged structures. Structure docs updated to the new contract.
The shared validation loop admitted all five ids for the vision override too; the vision resolver reads a wide id as unset and silently activates a backend the operator never chose. Vision keeps its two-member contract, the web-search override type widens honestly, and regressions pin both directions.
Codex attaches its default hosted web_search config to subagent turns; xAI's /v1/responses rejects the whole request on external_web_access and search_context_size (Argument not supported), killing every routed Grok dispatch with tools enabled. The routed sanitize chain now drops exactly those two proven-fatal fields — user_location and filters pass through, and canonical ChatGPT-forward traffic is untouched.
Gate xAI-only Responses web_search field stripping through registry-enriched capability metadata so official OpenAI API-key requests preserve external_web_access and search_context_size. Add buildRequest regressions for both providers.\n\nAdvertise the five-value web-search backend union in agent and Claude CLI usage, document explicit-only xAI/Gemini/Exa behavior and write-only exaApiKey, and synchronize localized accepted-value tables.\n\nExtend shared string-framing redaction for exaApiKey and cover JSON, colon, and query canaries.\n\nEvidence: 80 adapter tests, 42 redaction tests, 37 registry tests, 47 CLI tests; tsc, privacy scan, and 393-page docs build green.
d9a1fbc to
7d015ba
Compare
…er rebase Cross-layer reconciliation: #2209 introduced the (backend, model) pair contract typed as openai|anthropic; #2238 widened the configured union to five backends. The options module now re-exports WebSearchBackendId and passes config into isActive, so inert arms simply never produce rows. tsc clean; 48/0 across the three web-search suites.
…2242) * feat(web-search): xai executor with opt-in x_search (#2188 L7) runXaiWebSearch POSTs the pinned api.x.ai Responses endpoint with the stored Grok OAuth credential, hosted web_search (+ opt-in x_search with doc-validated handle/date limits), and reduces the SSE stream to text + deduped sources from url_citation annotations unioned with web_search_call action.sources. custom_tool_call items and skeleton action are tolerated per live captures. planWebSearch's xai arm goes live fail-closed; the loop dispatches it without touching forward headers; the registry activates the backend on OAuth presence; PUT validates the xSearch block before persisting. * fix(web-search): pin the xai origin exactly, fail the loop arm closed, round-trip xSearch Origin pinning by prefix admitted https://api.x.ai.evil — parse and compare url.origin, falling back to the canonical endpoint (Critical). A missing xaiSidecar on the xai loop arm now yields an error outcome instead of falling through to the forward-header OpenAI executor (High). GET and the PUT response now carry the persisted xSearch block so a dashboard reload cannot lose visible configuration (High). Tests: lookalike-origin bearer containment, xSearch 400/round-trip/clear. * test(web-search): loop-level regression for the xai fail-closed arm * fix(web-search): close xai executor review blockers Attach abort-driven body cancellation immediately after the xAI fetch, cancel oversized SSE readers upstream, and cover transport release. Stage and validate complete web-search management candidates before commit; reject malformed xSearch types without partial mutation or broadened searches. Document the live Grok OAuth/x_search backend across canonical docs, locales, type comments, and structure sources. Evidence: 47 focused tests pass; bun x tsc --noEmit; docs Astro build 393 pages. * fix(web-search): reject unknown xSearch fields * fix(web-search): unify WebSearchBackend with the five-value union after rebase Cross-layer reconciliation: #2209 introduced the (backend, model) pair contract typed as openai|anthropic; #2238 widened the configured union to five backends. The options module now re-exports WebSearchBackendId and passes config into isActive, so inert arms simply never produce rows. tsc clean; 48/0 across the three web-search suites.
Split the opt-in DTO into a write boolean vs read tri-state; record the concrete #2238 (3) and #2242 (5) review blockers in doc 120; recast doc 150 as the final aggregate gate with the full GUI/i18n/docs chain; replace temporal API-key rows with exact wire+tier assertions; state the explicit wp9->wp8->wp11->wp10 execution sequence.
…#2245) * feat(web-search): exa executor and the non-LLM search lane (#2188 L9) runExaWebSearch POSTs api.exa.ai/search with the operator key and maps ranked results to a digest the routed model synthesizes from. The key never rides the SidecarPlan — core.ts reads it from config at unpack time — and the executor scrubs the literal key from every error string (pattern-based redaction cannot know an arbitrary operator key; canary-tested). Plan, loop, and registry arms fail closed without the key. docs-site gains the explicit-only backend table. * fix(web-search): scrub the exa key before truncating error bodies Reviewer blocker (L9 round 2): error(t.slice(0,200)) truncated before the literal-key scrub, so a key straddling the 200-char boundary left an unscrubbable prefix in the returned tool error. Scrub first, then slice. Adds truncation-boundary and fetch-rejection canaries; 9/9 focused tests, tsc and privacy:scan green. * docs(devlog): integration merge-train roadmap 100-150 (chat default, global order, opt-in switch) Amends the 260820 unit with the audited (3-round sol-medium, round-3 PASS) roadmap: 100 chat-default regression as an atomic #2227+tier-policy unit with a 5-row regression matrix and the E2E reasoning-streaming proof; 110 global cross-train merge order and 21-PR triage matrix (#2072 deferred, #2217 RESHAPE); 120 sidecar L1-L9 merge execution with the fresh blocker inventory; 130 atomic xai Responses opt-in switch (single provider id, auth-mode-scoped sections, virtual PATCH field); 140 release prep; 150 blocking lidge final gate. DeepSeek explicitly out of scope per user decision. * docs(devlog): fold C-gate blockers into roadmap 100-150 Split the opt-in DTO into a write boolean vs read tri-state; record the concrete #2238 (3) and #2242 (5) review blockers in doc 120; recast doc 150 as the final aggregate gate with the full GUI/i18n/docs chain; replace temporal API-key rows with exact wire+tier assertions; state the explicit wp9->wp8->wp11->wp10 execution sequence. * docs(devlog): doc 100 API-key opt-in row preserves current tier forwarding C-gate round 2: current dev forwards caller service_tier verbatim on the API-key + explicit openai-responses route (fastPolicyForModel proof). The tier drop is an OAuth-route policy only; the API-key row now states preserve-current semantics, consistent with doc 130. * docs(devlog): wp9 execution record — all six chain blockers resolved and pushed
…only table (#2262) The #2258 series unconditional strip collided with the #2238 capability contract: official OpenAI API-key traffic lost external_web_access (lidge full suite 1/14025 fail). The table row is now capabilityGated, active only when supportsOpenAiWebSearchToolFields === false; defer_loading stays unconditional. 387/0 across six suites.
2 participants
Summary
Layer 6 of the #2188 follow-up chain (parent: #2211). Widens the web-search backend union INERTLY and fixes the routed-Grok dispatch killer found while probing:
OcxWebSearchSidecarConfig.backendunion gains"xai" | "gemini" | "exa"— explicit-only, andplanWebSearchfails closed (no plan → normal routed path) for each until its executor layer lands.resolveSidecarBackendstays pure with the unset→openai pin;shouldResolveOpenAiWebSearchSidecaris false for the new ids.exaApiKeyconfig field: PUT set/clear only, never echoed by GET or the PUT response, andredact.tsstrips it from logged structures (canary-tested).external_web_access/search_context_sizeto its default hosted web_search config; xAI's/v1/responses400s the whole request on either ("Argument not supported"), killing every routed Grok dispatch with tools enabled. The routed sanitize chain now strips exactly those two proven-fatal fields (live probe: each 400s individually;user_location/filtersaccepted and kept). Canonical ChatGPT-forward traffic untouched. Wire-level proof: identical body BEFORE 400 → AFTER 200.Design doc:
devlog/_plan/260820_sidecar_selection_unification/060_layer6_backend_union.md(rev 2) + 003 research.Verification
bun x tsc --noEmitclean; focused suites green (union 12, routed-fields 2, claude-management 28).reasoning+tools 200, both hosted tools in one request 200, handles+dates 200; BEFORE/AFTER fix 400→200.Checklist
Summary by CodeRabbit