feat(web-search): exa executor and the non-LLM search lane (#2188 L9) - #2245
Conversation
|
Warning Review limit reached
Next review available in: 18 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 (13)
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. |
✅ READY
Hygiene✅ 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: 95ccd33270
ℹ️ 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 (!res.ok) { | ||
| const t = await res.text().catch(() => ""); |
There was a problem hiding this comment.
If the client cancellation or sidecar timeout fires after fetch() resolves but before either res.text() or res.json() attaches a reader, Bun can reject the response body's internal read as an unhandled rejection rather than routing it through this try/catch; the existing web-search executors use cancelBodyOnAbort immediately after receiving headers specifically to close this race. Attach that guard before branching on res.ok and detach it after both the error-body and JSON reads so cancellation degrades through the normal sidecar outcome instead of escaping the request path.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| const res = await fetchWithResetRetry( | ||
| () => fetch(EXA_SEARCH_URL, { |
There was a problem hiding this comment.
When Bun reports a stale pooled-connection reset, fetchWithResetRetry invokes this callback with "connection-reset", but the callback ignores that argument and recreates the same ordinary fetch. As documented by applyUpstreamRecoveryInit in src/lib/upstream-retry.ts, Bun may reuse another half-closed pooled socket unless the retry sets keepalive: false (and Connection: close), so an otherwise recoverable Exa search can exhaust all attempts and return a tool error. Accept the recovery argument and apply it to the request init.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes on exact head 95ccd33270b7e720293755acb6ae7fe767e228cf. The Exa lane is a useful addition and the secret handling is directionally strong: the key stays out of the serialized plan, the literal key is scrubbed before truncation, redirects are disabled, missing credentials fail closed, and the focused suite passes 9/9 with typecheck and privacy scan clean.
Two transport blockers remain, and both automated findings are valid against the current code:
- Attach
cancelBodyOnAbort(res.body, linkedSignal.signal)immediately afterfetchWithResetRetryreturns, before eitherres.text()orres.json()starts reading. Detach it infinallyon both the error-body and success-body paths. Otherwise cancellation between header resolution and reader attachment can escape as Bun's unhandled body rejection instead of a normal sidecar error. - Accept the recovery argument supplied by
fetchWithResetRetryand pass the request init throughapplyUpstreamRecoveryInit. The current callback ignores"connection-reset", so retries can reuse the same half-closed pooled socket instead of settingkeepalive: falseandConnection: close.
Please add focused regressions for the abort-before-reader race and the recovery init on the retry attempt. The current exact-head CI also has test 1/4 red and macOS still pending. This stacked layer must remain open until these fixes land, CI is green, and its parent #2243 blockers are resolved.
리뷰 · 우선순위 58 / 80#2188 L9임. 베이스가 키 위생이 본문임.
같이 들어 있는
해결방안: #2243 머지 후 리타깃. 키는 플랜에 넣지 말 것. 스크럽-before-truncate 유지. 머지오더/Responses 옵트인 독스(100-150, 특히 130)는 이 PR에서 빼거나 #2240 쪽으로. xai 가이드에서 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 9982adf0f6c9bda4b5431d94c506674cb355c2e1. The commits since my 95ccd3327 review add only devlog planning files; src/web-search/exa-executor.ts is unchanged, so both transport blockers remain:
- Attach
cancelBodyOnAbortimmediately after headers and cover both success/error reads with afinallydetach. Add the abort-after-headers regression.\n2. Accept the recovery argument fromfetchWithResetRetryand applyapplyUpstreamRecoveryInitto the retry request. Add a regression that proves the connection-reset retry disables pooled reuse.
The owner review also identifies scope blockers in this head:
3. Remove the six devlog/_plan/.../100 through 150 files from this L9 PR. They are merge/release planning plus the separate Grok Responses default dispute, not Exa executor implementation.\n4. Do not make the x_search documentation part of this Exa layer. Keep the Exa and live Gemini documentation needed by this stack, but leave the separate #2190 opt-in contract out until that direction lands independently.
In addition, this layer is based on #2243, which now has unresolved auth-deadline, GUI backend-provenance, and documentation blockers. Keep #2245 stacked and rerun exact-head CI only after the parent and these current-head issues are fixed.
249cc91 to
ef551e8
Compare
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.
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.
…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.
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.
…rding 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.
9982adf to
796f63c
Compare
2 participants
Summary
L9 of the #2188 sidecar chain: a live Exa executor giving the web-search sidecar its first non-LLM lane.
src/web-search/exa-executor.ts: POST to the pinnedhttps://api.exa.ai/searchorigin (manual redirect), maps results to the shared digest/sources shape.redactSecretString.exaApiKeynever rides the serializedSidecarPlan;core.tsreads it from config at unpack time. Plan carries only anexaConfiguredpresence marker.backend: exawithout a key plans to nothing / errors the tool call; no silent LLM fallback, no fetch, no pool recording.Verification
bun test tests/exa-web-search.test.ts- 9/9 (incl. key canaries: upstream echo, truncation-boundary, fetch-rejection).bun run typecheck,bun run privacy:scangreen locally.Checklist
codex/sidecar-gemini-executor), stacked-PR workflow per AGENTS.md