Skip to content

fix(web-search): combine replay-cache isolation with deadline-safe quota evidence - #5480

Closed
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:fix/web-search-combined
Closed

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:fix/web-search-combined

Conversation

@luvs01

@luvs01 luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Web-search replay results could be reused outside the request context that produced them. Scope the replay cache to the conversation, caller, selected model/provider, and serving credentials, and restore replay data using the current serving identity.

When an upstream 429 supplies a Retry-After longer than the remaining sidecar deadline, preserve the original quota evidence instead of dropping it. This keeps downstream quota and failover decisions tied to the upstream response.

The changes cover the replay cache, request preparation and transport, and web-search executor. Regression coverage includes request-context isolation, credential changes, and Retry-After values that exceed the sidecar deadline. The inline-document fixture explicitly records its developer-message destination to match the current role-folding contract.

Verification

Previously recorded implementation validation:

  • bun test tests/web-search/web-search-bridge-replay.test.ts tests/web-search/web-search-sidecar-429.test.ts tests/server/server-key-failover-e2e.test.ts tests/responses/chat-inline-document-bytes.test.ts - 65 passed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Improved web-search replay isolation across conversations, callers, providers, models, adapters, and credentials.
    • Replay restoration now uses the current serving credentials and identity.
    • Preserved upstream 429 responses when retry delays exceed the remaining request deadline.
    • Improved handling of retry cleanup, deadlines, and caller cancellation.
  • Documentation

    • Clarified web-search replay isolation and deadline-based retry behavior.

…ota evidence

Combines two fork PRs on the web-search path: isolate the replay cache by request context (#563) and preserve quota evidence when Retry-After would outlive the sidecar deadline (#568), rebased onto current dev. The #568 test's unrecorded-destination case is adapted to the current foldDeveloperRoleToSystem fixture semantics.

bun test: web-search-bridge-replay + web-search-sidecar-429 + server-key-failover-e2e + chat-inline-document-bytes: 65 pass
@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_coauthor_credit — This pull request says it reimplements, supersedes, carries, or rebases another author's pull request, but no Co-authored-by trailer names that author. Prose in a commit body is not read by anything; the trailer is what GitHub counts. Add it to the description or a commit, or obtain attribution-approved. Paths: #568.

@github-actions github-actions Bot added the bug Something isn't working label Sep 21, 2026
@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 377c03b5-8c01-4575-908c-7cd0a22fdc27

📥 Commits

Reviewing files that changed from the base of the PR and between 498498d and 7ffc8b8.

📒 Files selected for processing (1)
  • src/server/responses/request-prepare.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The pull request scopes web-search replay restoration to caller and serving identities, binds restoration after route refresh, and bounds OpenAI sidecar 429 retries by the remaining deadline. Tests and documentation cover the updated behavior.

Changes

Web-search replay scoping

Layer / File(s) Summary
Replay scope contract
src/types/request.ts, src/responses/bridge-search-replay-cache.ts, src/server/responses/request-prepare.ts
OcxReasoningReplayScopeRef records the client principal. bridgeSearchReplayScope builds a fail-closed key from caller, conversation, and serving identity fields.
Route-bound restoration
src/server/responses/request-transport.ts, src/adapters/openai-responses/passthrough.ts, src/server/responses/passthrough-delivery.ts, structure/providers-and-adapters.md
Replay restoration uses the refreshed route scope instead of the provider base URL. Documentation records the expanded miss conditions.
Replay boundary validation
tests/web-search/web-search-bridge-replay.test.ts, tests/server/server-key-failover-e2e.test.ts
Tests cover mismatched principals, conversations, providers, adapters, models, destinations, credentials, incomplete scopes, and dispatch-time credential changes.

OpenAI sidecar retry handling

Layer / File(s) Summary
Deadline-bounded 429 retry
src/web-search/executor.ts, tests/web-search/web-search-sidecar-429.test.ts, structure/providers-and-adapters.md, structure/runtime.md
429 backoff now fits both the retry ceiling and remaining sidecar deadline. Cleanup deadline expiry preserves the observed 429 and records it as the outcome. Tests cover insufficient backoff time and blocked body cleanup.

Chat document role coverage

Layer / File(s) Summary
Developer role assertions
tests/responses/chat-inline-document-bytes.test.ts
The test verifies that the document remains a file part while the role stays developer or folds to system according to configuration.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ResponsesRequest
  participant RouteSelection
  participant BridgeReplayCache
  participant Upstream
  Client->>ResponsesRequest: submit responses request
  ResponsesRequest->>RouteSelection: refresh adapter and credential
  RouteSelection-->>ResponsesRequest: refreshed serving identity
  ResponsesRequest->>BridgeReplayCache: restore using bound replay scope
  BridgeReplayCache-->>ResponsesRequest: matching replay or unchanged search cell
  ResponsesRequest->>Upstream: send rebuilt request
Loading

Possibly related PRs

  • lidge-jun/opencodex#2264: Defines the serving-identity lifecycle consumed by this pull request before replay restoration.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 11 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies both primary changes: replay-cache isolation and deadline-safe handling of web-search quota evidence. It is concise, specific, and uses the conventional fix scope format.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 21:01
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 61 / 80

웹 검색 길에서 서로 겹치던 고침 두 개를 한 PR로 모았습니다. 첫째, 브리지가 대신 돌려 준 검색 결과를 다음 턴에 다시 끼워 넣을 때, 예전에는 공급자 주소(baseUrl)만 같으면 같은 칸으로 봤습니다. 이제는 누가 보냈는지, 어느 대화인지, 어느 공급자·어댑터·모델·목적지·키인지까지 맞아야만 꺼내 옵니다. 칸이 비어 있으면 꺼내 주지 않습니다. 키를 도중에 바꾼 뒤에도, 새로 고른 키로 다시 묶은 다음에 본문을 다시 만들어, 옛 키의 결과가 새 키 쪽으로 새지 않게 했습니다. 둘째, 사이드카가 HTTP 429를 받았을 때 Retry-After가 남은 시간보다 길면 다시 보내지 않고, 그 429를 그대로 남깁니다. 그래서 풀이 “할당량이 막혔다”는 증거를 타임아웃으로 바꿔 버리지 않습니다. 테스트와 structure 문서도 같이 손봤습니다. base는 dev이고 tip과 맞춰 있습니다. types.ts·config.ts 분할이나 프리뷰 배포 이야기는 없습니다.

라인 - 본문의 #563 / #568 — 이 저장소에서는 #568이 예전에 머지된 ocx opencode PR(작성자 Wibias)을 가리킵니다. #563은 여기 없습니다. 실제 원본은 포크 luvs01/opencodex의 같은 번호 PR들입니다. 그래서 hygiene의 missing_coauthor_credit이 Wibias 쪽으로 잘못 걸린 상태로 보입니다. 링크를 포크 URL로 고치거나, 정말 다른 사람 글을 가져온 경우에만 Co-authored-by / attribution-approved를 쓰면 됩니다.

라인 - src/server/responses/request-prepare.ts · clientPrincipalIdcontextPrincipalIdOf(admission)에 없고 loopback이면 무조건 "loopback"으로 넣습니다. 같은 파일 근처의 resolveContextPrincipal은 루프백에서도 API 키를 내면 키마다 다른 주체를 줍니다. 지금 방식은 로컬에서 키를 다르게 낸 호출자끼리 검색 재생 칸을 같이 씁니다. 셀 id를 알면(대화 기록에 있음) 격리 주장이 한 칸 약해집니다.

라인 - 상태 — draft이고 intake: hygiene-blocked입니다. hygiene·enforce-target이 빨간 상태라 Ready로 올리기 전 게이트를 먼저 통과해야 합니다.

메인테이너의 판단이 필요한 지점

루프백에서 검색 재생 주체를 전부 "loopback"으로 묶을지, 히스토리 소유와 같이 resolveContextPrincipal로 키별 주체를 쓸지 정해야 합니다. 로컬 단일 사용자면 지금이 단순하고, 한 프로세스에 여러 키가 붙는 배치·공유 프록시면 키별 주체가 맞습니다. 포크 원본이 본인 Codex 작업이면 공동 저자 줄은 필요 없고, 본문 링크만 고치면 됩니다.

너의 추천

방향(재생 칸을 대화·서빙 신원으로 좁히기, 키 교체 후 다시 묶기, 데드라인 밖 429 유지)은 맞습니다. 머지 전에 본문 이슈 번호를 포크 URL로 고쳐 hygiene을 풀고, 루프백 주체 규칙을 한 줄로 문서/코드에 맞추세요. 그다음 Ready로 올리면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 21, 2026
@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Review feedback applied on 7ffc8b858f:nn- Body: #563/#568 references now point at the fork URLs, and the stale upstream-blocked note is removed.n- Loopback principal: clientPrincipalIdnow resolves throughresolveContextPrincipal(same rule as context-history ownership), so loopback callers that present distinct opencodex API keys get per-key replay cells; keyless loopback still shares theloopback bucket.nnTests: web-search-bridge-replay10 pass,web-search-sidecar-429` 5 pass on the pushed head.

@luvs01
luvs01 marked this pull request as ready for review September 22, 2026 00:14
@github-actions
github-actions Bot marked this pull request as draft September 22, 2026 00:14
@github-actions
github-actions Bot marked this pull request as ready for review September 22, 2026 00:16
@luvs01

luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated into #5562 as a single related-function aggregate.

Source head: 7ffc8b858f4021ffe04f205c6bf24768bd6c1c75. Replacement head: 6b122cd2f024fb56f25667dad235575b5c50f001.

Both source contributions are preserved by exact stable patch IDs and original author/date metadata. Final-tree review retains replay isolation by principal, thread and serving identity, credential rebinding before restoration, and original sidecar 429 evidence. The combined regression also checks that repaired search output cannot replay under another principal, conversation or credential. The final unchanged eight-file test scope passed 199/199 tests with 976 assertions after fixing fixture-owned cancellation and spend-lease cleanup; type, structure, privacy and ratchet checks passed. Updated-head hosted CI and security review remain tracked on the draft replacement.

Closing this duplicate standalone review entry as part of the requested consolidation after verifying coverage. This is not a merge or release claim; remaining integration checks and reviews are tracked on the replacement. Original branches are retained.

@lidge-jun

Copy link
Copy Markdown
Owner

Thanks @luvs01. The replay-isolation and deadline-safe 429 work from this PR landed on dev in #5575 (b7351ddef3), with your authorship and a co-author trailer.

Two parts changed on the way in. A caller with no opencodex API key now gets no replay scope instead of the shared loopback principal, because that bucket let separate keyless callers read each other's retained results. The sidecar's reset and 429 recovery now share one three-send budget. The inline-document fixture edit was left as dev has it, since it is unrelated to this change.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants