Skip to content

test(proxy): skip case-distinct bypass cases on Windows - #6134

Merged
lidge-jun merged 1 commit into
devfrom
codex/t4-final-fix
Sep 27, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/t4-final-fix

Conversation

@lidge-jun

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

Copy link
Copy Markdown
Owner

Summary

The final Cross-platform CI run for release train 4 on dev (run 36346985953, head b383def) failed on windows 6/9. Five cases in tests/server/proxy-env-macos.test.ts, added with the macOS proxy: "auto" discovery work, assume NO_PROXY and no_proxy can hold different values. On Windows, environment variable names are case-insensitive, so writing no_proxy overwrites NO_PROXY and the assertions cannot hold.

This change skips those cases on win32, the same way the neighbouring WebSocket bypass case in the file already does. In the parameterized "inherited %s wins" test, only the upper/lower route assertion is guarded. The discovery path under test only runs on darwin, so no production behavior changes.

Verification

  • bun test tests/server/proxy-env-macos.test.ts on macOS: 43 pass, 0 fail.
  • bun run typecheck: passed.
  • Windows coverage comes from the Cross-platform CI run that will be dispatched on dev after this merges. Per-PR CI was intentionally not run for release train 4 because of runner congestion.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (none needed, test-only)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (none, test-only)

Summary by CodeRabbit

  • Tests
    • Adjusted macOS proxy test coverage to account for case-insensitive environment-variable names on Windows.

Windows environment names are case-insensitive, so NO_PROXY and no_proxy are one variable there. Five macOS "auto" proxy cases assert that the two bypass lists differ and failed on windows 6/9 of the final train 4 dev CI run. Skip them on win32 the same way the neighbouring WebSocket bypass case already does; the macOS discovery path itself only runs on darwin.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 27, 2026 20:25
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 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-27T20:27:52.497442Z c130aa0 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.

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

coderabbitai Bot commented Sep 27, 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: 33c16b30-06c3-465a-8dae-2750b5d8095e

📥 Commits

Reviewing files that changed from the base of the PR and between b383def and c130aa0.

📒 Files selected for processing (1)
  • tests/server/proxy-env-macos.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Proxy environment tests that rely on distinct uppercase and lowercase variable names now skip on Windows. The change covers separate bypass lists, an uppercase-only localhost route, and inherited SOCKS/HTTP bypass assertions.

Changes

Proxy environment test compatibility

Layer / File(s) Summary
Gate case-sensitive proxy tests
tests/server/proxy-env-macos.test.ts:27-30, tests/server/proxy-env-macos.test.ts:74, tests/server/proxy-env-macos.test.ts:221-228
Tests that require distinct NO_PROXY and no_proxy values, including inherited SOCKS/HTTP bypass cases, now skip on Windows. Windows treats environment variable names as case-insensitive.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~4 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c130a

The Windows skips are limited to assertions that cannot apply when environment names are case-insensitive; no merge-blocking issue was identified.

Architecture Summary

Architecture risk: 🔵 Low · up to c130a

The change affects 1 system.

Changed systems: tests

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — tests (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in tests/server/proxy-env-macos.test.ts: The test for separate uppercase and lowercase bypass lists now runs only when environment names are case-sensitive; it is skipped on Windows.
  • observed — Modified behavior in tests/server/proxy-env-macos.test.ts: The configured-localhost test that checks an uppercase-only bypass route is now skipped on Windows.
  • observed — Modified behavior in tests/server/proxy-env-macos.test.ts: The inherited-SOCKS bypass assertions now run only on case-sensitive environments, and the mixed inherited SOCKS/HTTP bypass test is skipped on Windows.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: proxy tests skip case-distinct bypass cases on Windows.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
✨ Finishing Touches
📝 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.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer dev integration (MAINTAINERS.md, dev-only, no second approval): release train 4 coordinator fix for the final dev CI run 36346985953 (head b383def), where windows 6/9 failed on five case-distinct NO_PROXY/no_proxy assertions. Exact head c130aa0. Test-only; macOS focused file 43/0 and typecheck pass locally. Windows proof comes from the post-merge Cross-platform CI run on dev.

@lidge-jun
lidge-jun merged commit 870f39e into dev Sep 27, 2026
21 of 34 checks passed
@lidge-jun
lidge-jun deleted the codex/t4-final-fix branch September 27, 2026 20:31
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