Repository navigation
fix(jev): apply initial effort to native Chat dispatch (carry #6750) - #6790
Conversation
Carry of #6750 onto current dev with review fixes. A validated JEV choice now owns the first native Chat dispatch, including an explicit no-effort choice; provider pins, caps and wire normalization still apply downstream, and later targets do not ask the judge again. Review fixes on top of the contributor's change: - collapse adjacent identical segments in the Combo requested-effort label, so a provider pin that kept the JEV effort no longer logs high->low->low - assert the native lane actually ran in the initial-effort wire matrix Supersedes #6750 Co-authored-by: GeunwooJun <313474999+geunwoojun99@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (18)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe initial JEV effort decision now applies to both Responses and opt-in native Chat Combo children. Native Chat shaping handles explicit null effort and removes conflicting controls. Effort labels, tests, and documentation now reflect these behaviors and subsequent provider adjustments. ChangesInitial JEV effort
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant JEV
participant ComboDispatch
participant NativeChild
participant ResponsesChild
participant ProviderNormalization
JEV->>ComboDispatch: initial effort decision
ComboDispatch->>NativeChild: initial decision for native lane
ComboDispatch->>ResponsesChild: selected effort for Responses lane
NativeChild->>ProviderNormalization: shaped request body
ResponsesChild->>ProviderNormalization: shaped request body
Merge Risk: ⚪ Minimal · up to This change applies the initial effort decision to the first native Chat child, with matching tests and docs. No merge-blocking risk is evident. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2e234c1dbd
ℹ️ 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 (initialDecision.effort === null) delete body.reasoning_effort; | ||
| else body.reasoning_effort = initialDecision.effort; |
There was a problem hiding this comment.
When native Chat is enabled and JEV selects an effort that has a provider wire alias—such as ultra, which must become max, or xhigh with reasoningEffortMap: { xhigh: "high" }—this assignment sends the semantic value directly. normalizePinnedChatEffort invokes mapReasoningEffort only after a pin or cap rewrite, while buildOpenAIChatPassthroughRequest deliberately preserves the remaining raw value, so an unpinned JEV choice can reach the provider unsupported and be rejected. Normalize this policy-produced effort before dispatch while retaining the semantic value for logging; the updated structure docs currently promise that wire normalization runs afterward.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| // Both lanes applied the initial choice before provider pins/caps; retain those transitions. | ||
| restoreOriginalRequestedEffort(childLog, initialJevDecision?.effort); |
There was a problem hiding this comment.
For a native request where JEV explicitly selects null and a downstream provider pin then supplies a different effort, this restoration drops the pin from both request and attempt labels. For example, caller low plus JEV null plus provider pin high sends high, but comboRequestedEffortLabel("low", "high", null) returns only low because a child label without -> is discarded when the forced effort is null. Record the newly introduced pinned base as a transition so the logs describe the actual wire behavior promised by the updated structure contract.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
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. |
1 participant
Summary
Carries #6750 by @geunwoojun99 onto current
dev, with two small review fixes. The contributor's fork PR could not start hosted CI; this branch can.Problem. With
protocols.rollout.nativeChatComboson, a JEV Combo's first native Chat dispatch ignored the judged effort.core-combo.tscalled the native child without the validated decision andcore-combo-native.tsapplied the ordinary Combo effort instead, so the judge's choice (including an explicit "no effort") never reached the native wire.Change (from #6750). The validated initial JEV decision now owns the first native Chat dispatch, including explicit
null. Provider pins, caps and wire normalization still apply downstream; later failover targets rebuild from the original request and do not ask the judge again. Both lanes use non-force initial-null shaping, so a forced Combo default cannot turn a valid no-effort choice into an exception. The guide and the owning structure contracts are updated.Review fixes added here.
comboRequestedEffortLabelcollapses adjacent identical segments. With callerhigh, JEVlowand a provider pinlow, the log used to readhigh->low->low; it now readshigh->low. New unit case intests/server/combo-requested-effort.test.ts; the 17 existing cases are unchanged.protocolTrace.attempts[0].mode === "native"), so a bridge dispatch can no longer satisfy it.Supersedes #6750 (left open for the maintainer to close with credit).
Co-authored-by: GeunwooJun 313474999+geunwoojun99@users.noreply.github.com
Verification
Local runs were kept to the changed behaviour (lane policy for this carry; the full suite is left to hosted CI):
bun test --preload ./tests/preload.ts tests/responses/jev-initial-effort-wire.test.ts tests/responses/chat-native-combo.test.ts tests/server/combo-requested-effort.test.ts tests/server/jev-decision-scope.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts: 102 pass, 0 failbun run typecheck: exit 0bun run structure:check: passedbun scripts/file-size-ratchet.ts: passedgit patch-id --stableof the applied change matches fix(jev): apply initial effort to native Chat dispatch #6750's net diff at a1dd3cd (14482251973e) before the review fixes.devPRs that share files: no new conflicts except fix(jev): apply initial effort to native Chat dispatch #6750 itself and fix(responses): retain child-owned prepaid send reservations #6751 (both add a test-layout entry afterchat-native-combo.test.ts; the fix(responses): retain child-owned prepaid send reservations #6751 carry reconciles it). All clean unions stay within file-size caps.dev(PASS-WITH-FIXES, the two fixes above). A fresh review of this exact head follows.Left to CI: the full cross-platform suite, docs build, and the remaining import-connected tests.
Checklist
Summary by CodeRabbit