test: make the provider-option E2E hermetic

[lidge-jun]

Summary

tests/openai-provider-option-e2e.test.ts was not hermetic. It replaced fetch but left globalThis.WebSocket pointing at the public network, so in an environment where wss://chatgpt.com is unreachable the focused test hangs opening the upstream Responses WebSocket instead of failing. Blocking that socket then exposed a second reach: the test and its migration child ran real Windows icacls and principal subprocesses.

• The global WebSocket is now stubbed for the test and throws on any upstream construction, while the saved native constructor still serves the intentional loopback connection. A regression fails loudly instead of hanging.
• Both the parent test and the migration child fixture take injectable sync/async icacls and principal runners, so the Windows assertions exercise the seams rather than the machine.

No assertion was removed or skipped; this is a hygiene fix, not a scope reduction.

Verification

• Before: a preload probe recorded four real connection attempts to wss://chatgpt.com/backend-api/codex/responses.
• After: bun test --isolate --parallel=1 tests/openai-provider-option-e2e.test.ts with the proxy env stripped — 1 pass, 0 fail, 81 expect() calls in 655 ms.
• The test now asserts the intercepted URL, so the stub is proven to be the thing that answered.
• bun run typecheck — passed.
• Per maintainer instruction for this campaign, the repository-wide suite was not run locally; CI is the full-suite gate.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed. Test-only change with no user-facing behavior.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No production code is touched.

Closes #3299

Summary by CodeRabbit

• Tests
  • Expanded migration coverage for Windows-specific configuration security behavior.
  • Added validation for platform-specific access-control and user-principal handling.
  • Improved end-to-end isolation by controlling external WebSocket connections during tests.
  • Added cleanup checks to ensure temporary test state and system integrations are restored reliably.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-02T19:47:18.796908Z	004634e	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: a13333a4-8367-4b20-b900-de0c1b8b4c43

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf3e91 and 004634e.

📒 Files selected for processing (2)

• tests/fixtures/openai-provider-option-migration-child.ts
• tests/openai-provider-option-e2e.test.ts

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

---

📝 Walkthrough

Walkthrough

The E2E test now blocks public upstream WebSockets, preserves loopback WebSocket coverage, replaces Windows ACL and principal subprocesses with test seams, records seam calls, validates isolation, and restores all modified global and module state.

Changes

Provider-option E2E isolation

Layer / File(s)	Summary
Windows ACL and principal seams tests/fixtures/openai-provider-option-migration-child.ts, tests/openai-provider-option-e2e.test.ts	The fixture registers synchronous and asynchronous ACL and principal runners at lines 93–116. The E2E test installs matching successful doubles at lines 271–291. Both paths track seam calls and include the counts in migration results at lines 193–200 and 30–37.
Upstream WebSocket isolation tests/openai-provider-option-e2e.test.ts	The test saves the native WebSocket constructor and replaces the global constructor with a deny-by-default proxy at lines 135–140 and 179–185. The loopback test uses the saved constructor at lines 402–404.
Assertions and state cleanup tests/fixtures/openai-provider-option-migration-child.ts, tests/openai-provider-option-e2e.test.ts	Migration assertions validate ACL and principal seam activity at lines 592–599 and 618–632. Cleanup flushes hardening, clears runners, resets module state, and restores the global WebSocket at lines 227–233, 303–308, and 657.

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

Merge Risk: ⚪ Minimal · up to 00463

This test-only change prevents external WebSocket and host security-command access without changing production behavior; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: wibias

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	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 1 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title, "test: make the provider-option E2E hermetic," clearly describes the primary change in tests/openai-provider-option-e2e.test.ts and is concise.
Linked Issues check	✅ Passed	The changes satisfy issue #3299. tests/openai-provider-option-e2e.test.ts blocks upstream WebSocket URLs, preserves the native loopback WebSocket path, stubs the synchronous and asynchronous Windows A…
Out of Scope Changes check	✅ Passed	The changes are limited to the provider-option E2E test and its migration child fixture. WebSocket blocking, loopback preservation, Windows ACL/principal seam injection, call tracking, and cleanup dir…

Full details: Linked Issues check

Explanation

The changes satisfy issue #3299. tests/openai-provider-option-e2e.test.ts blocks upstream WebSocket URLs, preserves the native loopback WebSocket path, stubs the synchronous and asynchronous Windows ACL and principal runners, restores global and seam state during cleanup, and retains all assertions without increasing the timeout. tests/fixtures/openai-provider-option-migration-child.ts applies the same seams to the migration child fixture and resets them after execution.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the provider-option E2E test and its migration child fixture. WebSocket blocking, loopback preservation, Windows ACL/principal seam injection, call tracking, and cleanup directly support issue #3299. No unrelated production changes or unrelated test changes are identified.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/260903-hermetic-provider-option-e2e

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[lidge-jun]

리뷰 · 우선순위 63 / 80

이 PR은 tests/openai-provider-option-e2e.test.ts가 공개 네트워크와 진짜 Windows ACL/계정 조회에 기대지 않게 만드는 테스트 위생 수정입니다. 지금 dev(HEAD 4cf3e9187, 직전 머지 #3297 liveness 재시도)에서 이 테스트는 fetch만 갈아끼우고 globalThis.WebSocket은 그대로 둡니다. 그래서 websockets: true인 픽스처가 업스트림으로 wss://chatgpt.com/backend-api/codex/responses를 열려고 하면, 그 주소에 닿지 않는 환경(특히 Windows CI·프록시 없는 호스트)에서는 타임아웃으로 한 번 실패합니다. 업스트림 소켓만 막아 두면 이번엔 설정 하드닝·마이그레이션 경로가 진짜 icacls / Windows principal 서브프로세스를 돌리면서 기존 30초 한도를 또 넘깁니다. 이슈 #3299가 그 두 겹을 그대로 적어 둔 상태입니다. 최근 dev에는 #3257/#3258로 Windows ACL·테스트 시임이 이미 들어가 있고, Responses WebSocket 업스트림(#3216)도 옵션으로 열려 있어서, “공개 WS + 진짜 ACL”이 테스트에 새어 나오는 구멍은 지금 축과 바로 맞닿아 있습니다.

고치는 방법은 두 갈래입니다. (1) 부모 테스트에서 globalThis.WebSocket을 Proxy로 감싸 construct 때마다 URL을 기록하고 바로 던집니다. 의도한 루프백 클라이언트→로컬 서버 연결만 저장해 둔 네이티브 생성자(savedWebSocket)로 엽니다. 프로덕션이 업스트림 WS를 시도하다 막히면 기존처럼 목킹된 HTTP/SSE fetch 경로로 떨어집니다. 마지막에 막힌 URL 집합이 정확히 wss://chatgpt.com/backend-api/codex/responses 하나인지 assert해서, 스텁이 “실제로 그 요청을 받았다”는 증거까지 남깁니다. (2) 부모와 마이그레이션 자식(tests/fixtures/openai-provider-option-migration-child.ts) 둘 다 src/lib/windows-secret-acl.ts / src/lib/windows-user-principal.ts의 기존 테스트 시임(setIcaclsRunnerForTests, setAsyncIcaclsRunnerForTests, setWindowsPrincipalRunnerForTests, setAsyncWindowsPrincipalRunnerForTests)에 성공 픽스처를 꽂고, finally에서 null 복구·resetHardenedStateForTests / resetWindowsPrincipalForTests·자식에서는 flushConfigDirHardeningForTests까지 호출합니다. Windows에서는 receipt·부모 카운터가 0보다 큰지 확인해 “시임이 실제로 불렸다”까지 고정합니다. 프로덕션 코드는 손대지 않았고, expect를 빼거나 skip 하지도 않았습니다. 작성자 검증은 프록시 env를 뺀 focused 스위트 1 pass / 81 expect / 약 655ms, typecheck 통과입니다.

같은 버그(#3299)를 고치는 초안 PR #3300이 이미 열려 있습니다. #3300은 루프백 hostname을 허용하는 class extends NativeWebSocket 방식이고 Draft·풀스위트 미완을 스스로 적어 두었습니다. 이 PR(#3301)은 deny-by-default Proxy + 의도 연결만 네이티브, 시임 호출 수 assert, 자식 hardening flush까지 더 꽉 채운 Ready 쪽입니다. types/config 분할 캠페인과는 무관합니다(테스트만).

라인 176~183 (openai-provider-option-e2e.test.ts) - Proxy의 construct가 globalThis.WebSocket로 열리는 모든 소켓을 막습니다. 루프백도 예외가 없어서, 테스트·프로덕션 코드가 실수로 globalThis.WebSocket으로 로컬 서버에 연결하면 바로 터집니다. 지금은 의도 연결만 savedWebSocket을 쓰도록 고쳤지만, 나중에 같은 파일에 WS를 추가할 때 실수하기 쉬운 계약입니다.

라인 399~401 (openai-provider-option-e2e.test.ts) - 루프백 WS를 savedWebSocket으로 여는 선택은 위 deny-all 계약과 짝입니다. 주석이나 헬퍼로 “루프백은 반드시 saved 생성자”를 남겨 두면 이후 회귀가 덜합니다.

경로 tests/fixtures/openai-provider-option-migration-child.ts - 자식은 별도 프로세스라 부모 시임이 안 먹습니다. 이 PR이 자식에도 시임을 심은 건 맞고, finally의 flush/reset도 #3257/#3258 방향과 맞습니다. 다만 receipt의 aclSeamCalls/principalSeamCalls는 전 플랫폼에서 expect.any(Number)라서, Windows가 아닌 CI에서는 0이어도 통과합니다(의도된 완화로 보임).

PR #3300 대비 - 동일 이슈·동일 두 파일을 고치는 중복입니다. Draft #3300을 살려 풀스위트를 기다릴지, Ready인 이쪽을 남길지 하나만 고르면 됩니다.

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

• #3301(Ready, deny-all Proxy + 시임 카운트)과 #3300(Draft, 루프백 허용 subclass) 중 어느 쪽을 남길지
• CI(특히 Windows 샤드·focused 파일)가 초록이 된 뒤에만 머지할지, focused+typecheck만으로도 충분한지
• deny-all Proxy 계약을 유지할지, #3300처럼 루프백 hostname 허용 subclass로 바꿀지

너의 추천
CI 테스트 샤드가 초록이면 #3301을 머지하고 #3299를 닫으세요. 같은 수정인 Draft #3300은 Landed via #3301(또는 superseded) 코멘트 후 landed-via-maintainer로 닫으세요. types/config 분할과 무관하니 리베이스 캠페인 대상이 아닙니다.

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