Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ 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: 031af07204
ℹ️ 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".
| // a candidate that can never go native costs no copy at all. | ||
| const nativeBody = (target: ComboTarget, route: RouteResult | undefined): Rec | undefined => { | ||
| if (!route || route.provider.adapter !== "openai-chat") return undefined; | ||
| const body = envelope.freshBody(); |
There was a problem hiding this comment.
When an eligible Chat combo body is roughly 11–16 MiB, this freshBody() is charged and then discarded after eligibility checking; nativeChild() calls nativeBody() again, and runNativeChatAttempt additionally retains the serialized request. Those three request-sized charges exceed the 32 MiB translator budget, so an otherwise admissible native request returns translation_buffer_limit as a 502 before any upstream send. Cache and reuse the candidate copy for dispatch, or perform eligibility against a non-copying read-only view.
Useful? React with 👍 / 👎.
| if (status < 400) { | ||
| callbacks.onTerminal("completed"); | ||
| callbacks.onResponseComplete(plan.route.modelId); |
There was a problem hiding this comment.
When a successful non-streaming Chat JSON response reports a resolved model different from the configured target, such as a versioned model behind an alias, the response returned to the client contains that upstream model but this callback records plan.route.modelId. Consequently rememberComboForLane stores the wrong response model, and a subsequent request on the same session lane that echoes the model it actually received cannot be recalled to the combo. Pass the completion's reported model through the native completion callback, falling back to the route model only when it is absent.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 63 / 80채팅으로 들어온 콤보 요청에서, 원래 채팅 규격으로 바로 보내도 되는 후보만 그 길로 보낸다. 스위치는 켜져 있을 때의 흐름은 이렇다. 채팅 입구가 원본 요청 봉투를 콤보 루프에 넘긴다. 루프는 후보마다 경로를 다시 정한다. 어댑터가 openai-chat이고 네이티브 규칙을 통과하면, 원본 몸통을 복사해서 그 후보만 네이티브 채팅으로 보낸다. 나머지는 기존 다리를 탄다. 다음 후보로 넘어가기, 보낼 수 있는 횟수, 로그 한 줄은 콤보가 그대로 맡는다.
스트림으로 나간 네이티브 후보는 본문을 미리 엿보지 않아서, 응답 상태 200을 먼저 확정한다. 글자가 하나도 나가기 전에 업스트림이 스트림 안에서 실패해도 다음 후보로 넘어가지 않는다. 글자가 나간 뒤의 실패는 다시 보내지 않는다. 이 한계는 devlog 040에 적혀 있다. src/server/responses/core-combo-native.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact head 031af072041ca2db63668dfcf59ba98833f3a20e. Blocking budget bug: judge() calls nativeBody(), which allocates and charges envelope.freshBody() only to decide eligibility, then nativeChild() calls it again for the actual send. ProtocolEnvelope.freshBody() charges the full JSON size to request_copies on every call, so each native candidate is charged twice; a large request that fits one required copy can incorrectly fail 413 during candidate selection, and an all-skipped reject request can exhaust budget before returning its intended 400. Judge eligibility from the retained source or cached feature facts without allocating a send copy, then call freshBody() exactly once for the candidate actually dispatched. Add a near-budget regression that would fail on a second copy. The focused native-combo and plan suites pass 18/18 under CPUQuota=200%, MemoryMax=4G, MemorySwapMax=0, TasksMax=128, but exact-head parity still fails because /api/protocols, /api/protocols/plan, and /api/protocols/settings lack documented CLI coverage (98 pass, 1 fail in the combined focused run). #5814 remains a lower-stack blocker as well.
A combo child must spend the combo's per-target budget instead of opening a second spend tracker, record its own first output and hold the parent's turn lease. All three are optional; without them the attempt behaves exactly as before.
A combo candidate skipped as unrepresentable leaves no attempt behind, so the skip has to be recorded on the request itself or the trace cannot say why it was passed over.
With a Chat protocol source on the options, a candidate whose settled route passes the native Chat rule runs runNativeChatAttempt on the attempt the combo opened, with the target budget and a fresh source body; under reject an unrepresentable one is never picked.
With the switch on, a combo route carries its source envelope and a native dispatcher into the Responses combo loop, and a Chat-wire answer passes through without the Responses-to-Chat conversion. Switch off leaves the request byte-for-byte unchanged.
Loopback upstreams show the native candidate receives the caller's own body (n and logprobs intact), a failed one hops within the shared budget, output is never re-sent, the switch-off path stays bridged, and reject skips or refuses without a send.
Records how a combo child is settled, judged and sent natively, how its sends and final row are owned, and why a marked child skips the stream preflight.
Policy routes never reach the combo loop, effort-row combos keep the bridge, and a streamed native child commits without the zero-output preflight.
…ends them With nativeChatCombos on the combo loop judges each candidate as its concrete route, so the plan snapshot does too; otherwise preview and observed trace would disagree.
The protocol lanes were created between the send-scope comment and the scope it describes; move them above so each comment sits on the statement it explains.
c4a3159 to
b783a2e
Compare
031af07 to
6dc5f8c
Compare
devin-ai-integration
Bot
added
the
priority: P3
|
Maintainer triage: Criteria (P3): Low: new provider/client integration, large or experimental feature (>2000 LOC or >50 files), RFC/roadmap, or long-stale branch. Related / overlapping PRs:
|
2 participants
Summary
PF-07 of the protocol-first-class unit (
devlog/_plan/260924_protocol_first_class/020_engine_and_codecs.md#pf-07-native-chat-candidates-in-combos). Stacked on #5814 (PF-09).Behind
protocols.rollout.nativeChatCombos(default off): in a Chat Completions combo, each candidate that is native-Chat-eligible is sent on the native Chat lane from its ownfreshBody()copy of the source envelope, instead of through the internal Responses body. Other candidates keep the bridge. Failover, send budget, affinity and logging stay owned by the combo loop.src/server/responses/core-combo-native.ts: settles the target route the way the Chat ingress does (key model scope, Chat static policy, wire override, OpenCode Go transport), checksisNativeChatRouteEligible, and runsrunNativeChatAttempton the attempt the combo opened, with the combo's send budget, abort signal and turn lease. Returns a Chat-wire response marked withmarkClientWire.core-combo.tsdispatches eligible children there; a marked child skipspreflightComboStreamResponse; non-OK native responses go throughconsumeComboFailureunchanged.sendBudget.used, the first settling the hop reservation; transient/429 loops are capped by the remaining base sends.reasoning_effortso a native child keeps forced/default effort. Combos reached through an effort row stay on the bridge.unrepresentable: "reject"a candidate whose path cannot carry a requested feature is skipped before any send withfeature-unrepresentableon the entry trace; if all are skipped the combo returns the ingress refusal.n > 1is never emulated.plan-snapshot.tspreviews combo candidates the same way when the switch is on.Recorded as not migrated (devlog 040): policy routes (the router resolves a policy to one candidate, so it never reaches the combo loop); combos with the switch off are not judged per candidate under
reject; a streamed native child whose upstream fails in-band before any output does not hop (the bridge child would).Verification
bun x tsc --noEmit: exit 0 on this head.bun run structure:check,bun run privacy:scan: passed.Tests (
tests/responses/chat-native-combo.test.ts, 7 cases with fake upstreams; a planner case inprotocol-plan-snapshot.test.ts) were written and registered but run in the full local suite below.Full local run on the stack head (feat(protocols): protocol paths as a first-class concern — PF-01..PF-12 #5820, which contains this change):
bun run test— the only failures are Lab CL-03/CL-07/CL-08/SEC-02 andrelease helpertimeouts, which fail identically on a checkout without this stack (local environment), plus service/toggle cases that pass when run alone;cd gui && bun test --isolate tests— 2398 pass, 0 fail.CI on this head: all required checks pass.
Checklist