Skip to content

test: fix the Windows-only failures from full-platform dev CI - #5634

Merged
lidge-jun merged 5 commits into
devfrom
codex/260923-windows-proxy-case-collapse
Sep 23, 2026
Merged

lidge-jun merged 5 commits into
devfrom
codex/260923-windows-proxy-case-collapse

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

Summary

Full-platform CI on dev after 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 set ALL_PROXY and all_proxy to 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 with EBUSY on two runs in a row. The policy route opens the process-lifetime routing-history.sqlite index 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's path.join, so Windows backslashes never matched the POSIX /mnt/c/Users discovery. 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 from os.homedir(), which reads USERPROFILE on win32 and pointed at the shared sandbox. On win32 the test pins USERPROFILE to its own home and restores it; the original assertion is unchanged.
  • windows 8/9: one bun test --isolate batch. 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 if close never arrives or destroy() throws.
    • Tests: the regressions cover a socket with no connect and no close, a response that never closes, and a throwing destroy(). A WebSocket test now awaits its client close.

Verification

  • On macOS, each fix's focused files pass: proxy-env 54 pass / 1 skip; claude-native-affinity 10; the three WSL and service files 89; service-claim plus two helper users 52; the six-file --isolate batch 92 tests across several runs; direct transport 24.
  • bun run typecheck and git diff --check pass.
  • Every fix was reviewed independently against its Windows failure log. The direct transport change was also checked for security: listener attestation, capability headers, and the 8 MiB response cap are unchanged.
  • Windows proof comes from the lane=all Cross-platform CI run dispatched on this head.

Checklist

  • Targets dev
  • Focused regressions run locally; the Windows lane is dispatched at this head
  • No skips, timeout increases, weakened assertions, or ratchet cap changes

Summary by CodeRabbit

  • Bug Fixes
    • Local requests now handle timeouts and cancellations more reliably, including when a connection does not close as expected.
    • Aside profile sync now clears its exchange deadline after the operation completes or fails.
  • Tests
    • Expanded coverage for local request cancellation and timeout behavior, deadline cleanup, and proxy handling on Windows.
    • Improved test checks for request admission and history tracking.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 23, 2026 04:06
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 23, 2026
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e500c073-9b5c-421a-8aec-cdd26f5dfb3f

📥 Commits

Reviewing files that changed from the base of the PR and between 3d53842 and a660c62.

📒 Files selected for processing (8)
  • src/cli/aside-profiles.ts
  • src/server/direct-local-http.ts
  • tests/claude-integration/claude-native-affinity.test.ts
  • tests/server/data-plane-admission-identity.test.ts
  • tests/server/local-aside-sync-capability.test.ts
  • tests/server/local-management-direct-transport.test.ts
  • tests/service/service-claim.test.ts
  • tests/service/service-wsl-home-ownership.test.ts

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


📝 Walkthrough

Walkthrough

The 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.

Changes

Local exchange deadlines

Layer / File(s) Summary
Schedule and cancel exchange deadlines
src/cli/aside-profiles.ts, tests/server/local-aside-sync-capability.test.ts
The aside sync flow schedules a deadline that aborts its shared signal and cancels the deadline in a finally block. Tests verify successful-call cleanup, triggered-deadline cleanup, and response-body draining.
Settle direct HTTP requests after socket closure
src/server/direct-local-http.ts, tests/server/local-management-direct-transport.test.ts
The transport waits for socket closure before parsing and resolving a response. Deadline errors settle without waiting for close. Tests cover abort, timeout, completed response framing, and socket-destroy failure.

Proxy environment test

Layer / File(s) Summary
Handle collapsed proxy variables
tests/server/proxy-env.test.ts
On Windows, the test checks the collapsed SOCKS and HTTP environment variable case and verifies direct fetch behavior before returning from the test case.

Integration and service test fixtures

Layer / File(s) Summary
Manage integration test history state
tests/claude-integration/claude-native-affinity.test.ts
The test closes the request-history index and clears health-history state during teardown. The policy-hop test clears the cache and checks that the history index file exists.
Wait for WebSocket close in admission test
tests/server/data-plane-admission-identity.test.ts
The test resolves after the socket closes and uses the recorded open and error events to determine the result.
Set service test environment paths
tests/service/service-claim.test.ts, tests/service/service-wsl-home-ownership.test.ts
The claim test sets and restores USERPROFILE on Windows. The WSL ownership test uses fixed Linux-style home paths.

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
Loading

Merge Risk: ⚪ Minimal · up to a660c

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 6 functions across 9 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing Windows-only failures found in full-platform development CI. It accurately covers the cross-file test and lifecycle fixes without unnecessary detai…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

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-23T04:09:09.084312Z 3d53842 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.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 46 / 80

이 PR은 테스트 한 곳만 고칩니다. 프로그램 코드는 그대로입니다.

Windows는 환경 변수 이름에서 대문자와 소문자를 같은 것으로 봅니다. ALL_PROXY와 all_proxy는 칸이 하나입니다. 표의 두 줄은 그 칸에 SOCKS 주소를 넣은 다음 HTTP 주소를 넣습니다. Windows에서는 나중 값이 앞 값을 지워서, HTTP 주소만 남습니다. SOCKS가 없으면 localhost를 직접 연결로 고정하는 코드도 돌지 않습니다. 예전 검사는 proxy: false를 기다리다가 undefined를 받았습니다. 깨진 자리는 전체 CI의 windows 2/9, tests/server/proxy-env.test.ts였습니다.

이번 수정은 그 겹침을 Windows에서만 찾고, 그때는 다른 검사를 합니다. 남은 값은 HTTP 프록시입니다. 요청을 받은 가짜 함수에는 proxy 옵션이 없습니다. localhost와 app.localhost가 그 함수의 답 "direct"를 돌려주면 통과입니다. Mac, Linux, 그리고 이름이 안 겹치는 줄은 예전과 같습니다. base는 dev입니다.

tests/server/proxy-env.test.ts:333 - "direct"는 320행 가짜 함수가 항상 돌려주는 답입니다. Bun이 그 주소를 어디로 보냈는지는 이 검사가 보지 않습니다. 겹친 뒤 NO_PROXY는 127.0.0.1,::1,[::1]입니다. 이름 localhost는 목록에 없습니다. 진짜 fetch라면 localhost와 app.localhost는 남은 http://proxy.invalid:3128로 갑니다. 329행 주석은 Bun이 정한다고 하고, 통과 조건은 직접 간 것처럼 읽힙니다.

tests/server/proxy-env.test.ts:300 - 테스트 이름은 여전히 "*.localhost는 SOCKS에 남는다"입니다. Windows에서 298행과 299행은 SOCKS가 이미 사라진 상태입니다.

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

Windows의 그 두 줄을, 만들 수 없는 섞임으로 두고 남은 HTTP 프록시로 간다고 적을지. 이름 localhost도 직접 연결로 남게 제품 코드를 고칠지.

너의 추천

테스트가 Windows에서 실제로 남는 값을 따라가는 쪽은 맞습니다. proxy: false를 기대한 예전 검사는 그 OS에 없는 상태를 요구했습니다. 333행의 "direct"는 빼는 것이 좋습니다. 겹친 뒤에는 resolveProxyRoute가 localhost와 app.localhost 둘 다 그 HTTP 프록시를 가리키는지만 보면, 검사가 말하는 것과 실제 길이 같습니다. 제품 코드는 그대로 둬도 됩니다. Windows에서는 두 이름이 한 칸이라 SOCKS와 HTTP를 동시에 둘 수 없고, HTTP 프록시가 있을 때 이름 localhost를 NO_PROXY에 넣지 않는 규칙은 이미 있습니다.

workflow_dispatch 35816970127의 windows 2/9는 다시 실패했습니다. 깨진 테스트는 이 파일이 아니라 codex-cli-update-zero-effect의 spawnSync 시간 초과입니다. 이 분기가 Windows에서 통과했는지는 그 로그에 없습니다. Mac의 54 pass도 이 분기를 실행하지 않습니다.

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

@lidge-jun lidge-jun changed the title test(proxy): assert the Windows case-collapsed env for mixed-case ALL_PROXY rows test: fix the Windows-only failures from full-platform dev CI Sep 23, 2026
@lidge-jun
lidge-jun merged commit 5c5b1b3 into dev Sep 23, 2026
57 of 59 checks passed
@lidge-jun
lidge-jun deleted the codex/260923-windows-proxy-case-collapse branch September 23, 2026 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant