fix(codex): restore caller token state in Windows shims - #3909
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughWindows CMD shims now isolate environment changes. PowerShell shims restore the caller's token state after success or failure. Integration tests cover token states and shells. CLI lifecycle documentation was updated in five languages. ChangesWindows token restoration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to New Windows shims restore the caller token after execution while preserving the service token for Codex during execution. The changed shell and failure behavior is covered, with no current merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant CallerShell
participant WindowsShim
participant ensure
participant Codex
CallerShell->>WindowsShim: Invoke shim with caller token state
WindowsShim->>ensure: Load token and run ensure
WindowsShim->>Codex: Launch Codex with file token
Codex-->>WindowsShim: Return exit code
WindowsShim-->>CallerShell: Restore prior token state and exit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (5 skipped: 5 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. Hygiene✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
리뷰 · 우선순위 64 / 80설명 이 PR은 Windows에서 고치는 방식은 단순하다. 테스트( 라인 / 경로 관찰
문서 / 운영 - 5개 언어 lifecycle note는 누출 복구와 재설치 절차를 같이 설명한다. 보안 수정인데 기존 Windows 설치 사용자는 uninstall/install 전까지 옛 래퍼를 쓰게 되므로, 릴리스 노트·changelog에 한 줄 더 강조할지는 운영 판단이다. PR 상태 - draft, mergeable이나 mergeStateStatus BLOCKED(체크리스트/게이트), 작성자 로컬 Windows 83 pass·8 skip 서술은 설득력 있다. 전체 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
I independently verified that the linked contributor workflow run 34127296217 completed successfully with head_sha 0878786, matching this PR. That is real exact-head CI evidence, distinct from the recorded local changed-test timeout and from upstream-required checks. Against the freshly fetched dev aeefb3a, this head is 23 commits behind; there are no intervening dev changes to src/codex/shim.ts in that comparison. Please refresh the base and re-attest the corrected head using the readiness gate. Keep the documented existing-installation reinstall requirement and the caller-state/exit-code cases intact. I have not executed Windows tests locally or completed final security approval, so this comment is not an approval or authorization to merge. |
0878786 to
06829fa
Compare
Ingwannu
left a comment
There was a problem hiding this comment.
Follow-up review of the rebased 06829fa: the linked full workflow 34139742297 is verified at this exact head, with 26 successful jobs. I inspected the generated CMD/PowerShell changes and the runtime fixtures: outer CMD setlocal preserves the token during nested ensure; PowerShell finally restores prior state on exceptions, and the child exit code is captured before cleanup. Missing/empty/explicit caller state, bypass, nonzero ensure and thrown ensure/Codex cases are covered. I found no source-visible blocker in that scoped change. Existing installed wrappers still need the documented recreation; this patch does not silently rewrite them.
@lidge-jun The contributor completed the requested rebase and exact-head CI. Current dev 60bcb90 is now another 37 commits ahead, but neither src/codex/shim.ts nor its test file changed on dev in that interval. Rather than asking for repeated version-only rebases, the remaining integration step is to validate the exact current-base merge candidate and required upstream checks. I am not treating contributor CI as that integration result or authorizing an immediate merge. No local Windows execution, runtime configuration write, or daemon restart was performed during this review.
|
Delivered independently via #3956 as The carry retains the production patch and strengthens explicit-token-with-ensure and PowerShell failure/presence/exit-code oracles. Current candidate Actual landing tree, scoped diff and fetched dev ancestry were verified under the shared merge lock. The separately landed PID fixture is an explicitly recorded candidate-to-destination difference; whole combined-tree execution is not claimed here. Closing the original as carried, not directly merged. Thank you for the original implementation. |
Summary
A Windows Codex shim can load
OPENCODEX_API_AUTH_TOKENfrom the service token file and leave it in the CMD or PowerShell session that invoked the shim. Later commands in that session then inherit a credential that the caller did not supply.Scope the CMD wrapper with
setlocal, and save/restore the caller's PowerShell token state in an outertry/finally. Codex still receives the token; an explicitly supplied caller value remains unchanged. Preserve the existing CMD batch handoff and the Codex exit code, including whenensurereturns a nonzero status. PowerShell cleanup also runs whenensureor Codex throws.The change applies to newly generated Windows wrappers. Existing healthy Windows wrappers are not automatically rewritten; the English, Japanese, Korean, Russian, and Simplified Chinese lifecycle pages document the required
ocx codex-shim uninstallfollowed byocx codex-shim install. Existing wrapper replacement/rollback ownership rules remain in force.Verification
Head
06829fa566c5e9d688a934fd31d481227450828b, based ondevaeefb3ab5433c69f7621695413ec1f77a05ed54f. Tree:48e666e6f42ca0308d0d4ceffda4512628d5c710.bun test tests/codex-integration/codex-shim.test.ts --timeout 60000: 83 passed, 8 skipped on this head with the project-pinned Bun 1.4.0 on Windows.08787869ede5e5d2fa93a43c34a0ee2839c16e01.Full CI for this head passed all 26 jobs on this exact head. Contributor readiness is based on this full CI plus the local checks described above. At the readiness check, the branch was two commits behind dev (within the repository's ten-commit allowance), with no unresolved review threads. Maintainer approval and upstream-required CI remain separate merge requirements.
Checklist
Review readiness checklist
Summary by CodeRabbit
Bug Fixes
OPENCODEX_API_AUTH_TOKENafter execution.Documentation
ocx codex-shim uninstallfollowed byocx codex-shim installto receive the updated behavior.