fix(chat): defer native-main claim until OpenAI helper planning - #4118
fix(chat): defer native-main claim until OpenAI helper planning#4118luvs01 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChat sidecar authentication now defers stored native-main access until an OpenAI helper has a canonical Direct candidate. Vision and web-search planning apply additional eligibility checks. Tests and localized documentation cover the updated behavior. ChangesCursor sidecar authentication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Cursor
participant ChatBridge
participant SidecarPlanner
participant NativeMain
Cursor->>ChatBridge: Send Chat request
ChatBridge->>SidecarPlanner: Plan vision or web-search helper
SidecarPlanner->>NativeMain: Request stored auth only for qualifying Direct helper
NativeMain-->>SidecarPlanner: Return sidecar credential
SidecarPlanner-->>ChatBridge: Build request-local sidecar headers
ChatBridge-->>Cursor: Continue Cursor response flow
Merge Risk: ⚪ Minimal · up to The deferred sidecar-authentication behavior preserves profile switching for unrelated Cursor requests while retaining helper authentication safeguards and account selection. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (9 skipped: 9 unsupported.)
✨ Finishing Touches🧪 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. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review Please review immutable head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected='f5566a4e16a5a9f13fa5909d122e835343518579'
printf '%s\n' '== checked-out revision =='
git rev-parse HEAD
git status --short
printf '%s\n' '== commit object =='
git show -s --format='%H%n%P%n%s' "$expected"
printf '%s\n' '== PR metadata =='
gh pr view 4118 --repo lidge-jun/opencodex --json number,title,baseRefName,baseRefOid,headRefName,headRefOid,files
printf '%s\n' '== diff stat =='
git diff --stat "${expected}^1" "$expected"
printf '%s\n' '== changed files =='
git diff --name-only "${expected}^1" "$expected"
printf '%s\n' '== source outlines for likely changed implementation and test files =='
git diff --name-only "${expected}^1" "$expected" |
while IFS= read -r f; do
case "$f" in
*.ts|*.tsx|*.js|*.mjs|*.cjs)
echo "-- $f"
ast-grep outline "$f" --items all --view expanded || true
;;
esac
done
printf '%s\n' '== focused symbols and references =='
rg -n -C 4 -i \
'stored.main|storedMain|native.main|nativeMain|claim.*main|main.*claim|Direct.*candidate|Pool|exact.account|exactAccount|runTurn|tool.choice|toolChoice|compaction|vision' \
. -g '!node_modules' -g '!dist' -g '!build' | head -n 1800Length of output: 176188 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='57077ca3260494aa4266b4108a7bd4c8a4dac288'
head='f5566a4e16a5a9f13fa5909d122e835343518579'
printf '%s\n' '== implementation diffs =='
git diff --unified=80 "$base" "$head" -- \
src/server/chat-completions.ts \
src/server/responses/core.ts \
src/vision/plan.ts \
src/web-search/index.ts
printf '%s\n' '== focused test diffs =='
git diff --unified=50 "$base" "$head" -- \
tests/codex-integration/bearer-admission-routed-provider.test.ts \
tests/vision/vision-cache.test.ts \
tests/web-search/web-search.test.ts
printf '%s\n' '== changed-line locations =='
git diff --unified=0 "$base" "$head" -- \
src/server/chat-completions.ts src/server/responses/core.ts src/vision/plan.ts src/web-search/index.ts |
grep -E '^\+\+\+|^@@'
printf '%s\n' '== native-main admission implementation =='
fd -a '^native-main-admission\.ts$' . -x sh -c 'echo "-- $1"; cat -n "$1"' sh {}
printf '%s\n' '== response handler planning area =='
sed -n '3170,3700p' src/server/responses/core.ts | cat -n
sed -n '3700,4350p' src/server/responses/core.ts | cat -n
printf '%s\n' '== chat handler area =='
sed -n '70,330p' src/server/chat-completions.ts | cat -n
printf '%s\n' '== planning modules =='
cat -n src/vision/plan.ts
sed -n '185,340p' src/web-search/index.ts | cat -nLength of output: 204182 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== OpenAI sidecar resolver =='
cat -n src/providers/openai-sidecar.ts | sed -n '1,290p'
printf '%s\n' '== response recursive calls and stored-sidecar permission =='
rg -n -C 10 'handleResponses\(|handleResponsesInner\(|allowStoredOpenAiSidecarAuth|openAiSidecarAuth' \
src/server/responses/core.ts src/server/chat-completions.ts src/server \
-g '*.ts'
printf '%s\n' '== helper plans and execution in the response handler =='
rg -n -C 12 'planVisionSidecar|planWebSearch|runWithWebSearch|openAiSidecar|needsOpenAi' \
src/server/responses/core.ts
printf '%s\n' '== routing candidate construction and account-mode combinations =='
rg -n -C 8 'listOpenAiForwardSidecarCandidates|OpenAiForwardSidecarCandidate|codexAccountMode|OPENAI_CODEX_PROVIDER_ID|LEGACY_OPENAI_MULTI_PROVIDER_ID' \
src/providers src/codex src/server -g '*.ts' | head -n 1400
printf '%s\n' '== focused test names and helper-auth assertions =='
rg -n -C 6 'stored|sidecar|native main|native-main|switch|combo|pool|exact|retry|runTurn|tool.choice|compaction' \
tests/codex-integration/bearer-admission-routed-provider.test.ts \
tests/vision/vision-cache.test.ts tests/web-search/web-search.test.ts | head -n 1800Length of output: 201748 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== candidate construction and resolution =='
sed -n '1,240p' src/providers/openai-sidecar.ts | cat -n
printf '%s\n' '== all stored-sidecar permission references =='
rg -n -C 14 'allowStoredOpenAiSidecarAuth' src -g '*.ts'
printf '%s\n' '== combo dispatcher children =='
sed -n '2590,3170p' src/server/responses/core.ts | cat -n
printf '%s\n' '== helper resolution and execution branch =='
sed -n '4400,4740p' src/server/responses/core.ts | cat -n
printf '%s\n' '== post-planning web-search dispatch references =='
rg -n -C 10 'const webSearchPlan|planWebSearch\(|runWithWebSearch\(' src/server/responses/core.ts
printf '%s\n' '== exact new test region =='
sed -n '430,610p' tests/codex-integration/bearer-admission-routed-provider.test.ts | cat -nLength of output: 84387
I found no blocking issues in the focused review. Key checks completed:
I did not run repository tests because the sandbox does not permit executing repository code. The pending import-connected validation and author CI remain the required execution evidence. ✅ Action performedReview finished.
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Completed the requested source-level authentication-boundary review on this exact head. Stored main is claimed before reading, only via the internal Chat permission and an eligible canonical Direct helper candidate; the candidate selector returns only the canonical OpenAI row. The captured pair remains local to sidecar headers and is not assigned to caller/primary/retry authority. Pool and exact-account branches remain separately resolved. Startup/drain fences still apply.
I traced terminal/routed-vision, runTurn/compaction and tool-choice exclusions into final helper planning. The held Cursor request regression checks the actual active request count and a successful profile switch before the upstream settles, while the helper cases observe which bearer reaches each wire. That is materially stronger than checking only a returned status. No blocking credential-boundary defect found in this patch.
Author CI 34352957960 is successful on this exact head; its report is separate from the disclosed incomplete local run. I did not execute the contributor code locally. This review does not bypass repository required CI or authorize a merge; the current upstream rollup contains the intake checks rather than an executed product suite.
리뷰 · 우선순위 76 / 80이 PR은 #4103이 심은 keyless Cursor Chat의 native-main 조기 claim을 되돌리는 핫픽스다. 지금 로컬 고치는 축은 짧다. Chat( 왜 지금 tip 기준으로 점수가 높은가. #4102/#4103 credential-domain·sidecar 열차의 운영 회귀(프로필 전환 막힘)를 직접 고치고, tip이 최근에 올린 Spark quota
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
f5566a4 to
7cf4b84
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7cf4b84 to
4d988bd
Compare
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. |
3 participants
Summary
A keyless Cursor Chat request could claim native main for its entire upstream turn even when it never needed an OpenAI helper. This unnecessarily blocked a native-profile switch. The regression introduced by #4103 is tracked in #4103 (comment).
Defer Chat's optional stored-main sidecar enrichment until the final helper conditions require a canonical Direct candidate. Vision terminal/routed-backend and search tool-choice/compaction/runTurn exclusions now agree with actual planning. The stored pair stays local to sidecar headers, and the existing startup/switch fences, Pool and exact-account selection, and primary/retry credentials remain intact. Public documentation is synchronized in all eight locales.
Verification
4d988bda127ef7cf70587448b6017476d79b13e7, based ondev 386b6a0d9a8acef818b9c40ebd472e4974750199.34439056670: 26/26 jobs passed, bound to4d988bda127ef7cf70587448b6017476d79b13e7. The checklist CI attestation refers to this completed matrix; local focused results are listed separately.Security review
The stored-token read and native-main admission boundary still require maintainer review and merge approval. Earlier independent and automated source reviews found no blocker; they are not merge approvals.
Checklist
Review readiness checklist
Readiness base check: 7 commits behind current dev; within the repository allowance of ten.