test: fix the Windows-only failures from full-platform dev CI - #5634
Conversation
|
✅ Deterministic PR hygiene checks passed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe direct-local HTTP transport now settles successful responses after socket closure and supports injectable deadlines. The aside sync flow cancels its deadline after completion. Tests also update proxy-environment handling, request-history state, WebSocket settlement, and service fixtures. ChangesLocal exchange deadlines
Proxy environment test
Integration and service test fixtures
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant AsideSync
participant DirectLocalHttpFetch
participant Socket
AsideSync->>DirectLocalHttpFetch: Send request with abort signal
DirectLocalHttpFetch->>Socket: Connect and write request
Socket-->>DirectLocalHttpFetch: Return response bytes and close
DirectLocalHttpFetch-->>AsideSync: Resolve parsed response
AsideSync->>AsideSync: Cancel exchange deadline
Merge Risk: ⚪ Minimal · up to This change makes local sync deadlines cancellable and makes direct local HTTP calls finish only after their socket closes. It also fixes several Windows-specific test fixtures. Timeouts and aborts still end requests promptly. No concrete defect affecting users or production was found, so the change looks ready to merge once Windows CI confirms the updated tests pass. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
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. |
리뷰 · 우선순위 46 / 80이 PR은 테스트 한 곳만 고칩니다. 프로그램 코드는 그대로입니다. Windows는 환경 변수 이름에서 대문자와 소문자를 같은 것으로 봅니다. 이번 수정은 그 겹침을 Windows에서만 찾고, 그때는 다른 검사를 합니다. 남은 값은 HTTP 프록시입니다. 요청을 받은 가짜 함수에는 tests/server/proxy-env.test.ts:333 - tests/server/proxy-env.test.ts:300 - 테스트 이름은 여전히 "*.localhost는 SOCKS에 남는다"입니다. Windows에서 298행과 299행은 SOCKS가 이미 사라진 상태입니다. 메인테이너의 판단이 필요한 지점 Windows의 그 두 줄을, 만들 수 없는 섞임으로 두고 남은 HTTP 프록시로 간다고 적을지. 이름 localhost도 직접 연결로 남게 제품 코드를 고칠지. 너의 추천 테스트가 Windows에서 실제로 남는 값을 따라가는 쪽은 맞습니다. workflow_dispatch 35816970127의 windows 2/9는 다시 실패했습니다. 깨진 테스트는 이 파일이 아니라 이 댓글은 grok-bot이 작성했습니다 |
Summary
Full-platform CI on
devafter the 2026-09-23 landing round failed on Windows. This PR fixes each failure at its cause. None of the fixes adds a skip, raises a timeout, or weakens an assertion.windows 2/9:proxy-env.test.ts. Two table rows setALL_PROXYandall_proxyto different proxies. Windows environment names are case-insensitive, so the second assignment replaced the first and the SOCKS-only expectation could not hold. On win32 the test now asserts the collapsed environment; other platforms keep every assertion.windows 3/9:claude-native-affinity.test.ts. Cleanup failed withEBUSYon two runs in a row. The policy route opens the process-lifetimerouting-history.sqliteindex under the temp home, and the fixture never closed it. Teardown now closes the index and clears its cache before removing the home, and the test asserts that the policy case opens it.windows 9/9:service-wsl-home-ownership.test.ts. The fixture simulates WSL but built its homes with the host'spath.join, so Windows backslashes never matched the POSIX/mnt/c/Usersdiscovery. The simulated homes are now POSIX paths; the state file stays on a real host path.windows 8/9:service-claim.test.ts. The legacy state path comes fromos.homedir(), which readsUSERPROFILEon win32 and pointed at the shared sandbox. On win32 the test pinsUSERPROFILEto its own home and restores it; the original assertion is unchanged.windows 8/9: onebun test --isolatebatch. The batch hung for 480 s on one attempt and crashed Bun 1.4.0 on the next, both right after the Aside sync test started its listener. Two ownership gaps were found and fixed:src/cli/aside-profiles.ts: the Aside sync deadline stayed armed after a successful sync. It is now cleared on every exit.src/server/direct-local-http.ts: the direct local request settled before its socket closed. An absolute timer now bounds both the request and the socket teardown, and the promise settles exactly once, even ifclosenever arrives ordestroy()throws.connectand noclose, a response that never closes, and a throwingdestroy(). A WebSocket test now awaits its clientclose.Verification
--isolatebatch 92 tests across several runs; direct transport 24.bun run typecheckandgit diff --checkpass.lane=allCross-platform CI run dispatched on this head.Checklist
devSummary by CodeRabbit