Conversation
Carry the bounded #5893 behavior with fail-closed exception translation and inherited proxy precedence. Co-authored-by: codingbo <9621077+codingbooo@users.noreply.github.com>
Cover inherited SOCKS activation skips and the distinct lowercase Bun and uppercase WebSocket bypass decisions, including an explicitly empty uppercase value.
Map valid leading domain globs to Bun label-boundary suffixes and report their apex widening. Drop only exact link-local default ranges with a privacy-safe diagnostic; keep other unrepresentable rules fail-closed.
When macOS auto installs a previously absent proxy, copy configured noProxy entries into an inherited non-empty lowercase no_proxy so Bun and the WebSocket route keep configured destinations direct. Co-authored-by: codingbo <9621077+codingbooo@users.noreply.github.com>
Preserve configured localhost in uppercase NO_PROXY for the exact-host WebSocket route, while leaving it out of Bun lowercase no_proxy so app.localhost remains proxied. Co-authored-by: codingbo <9621077+codingbooo@users.noreply.github.com>
… bypass When macOS auto discovers a proxy, a configured exact localhost bypass cannot be preserved by non-empty inherited lowercase no_proxy. Refuse before writing proxy variables and retain the existing uppercase-only path when lowercase is absent. Co-authored-by: codingbo <9621077+codingbooo@users.noreply.github.com>
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. |
|
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 (16)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesmacOS proxy reader
Proxy environment integration
Regression tests
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant applyProxyEnvWith
participant readMacOSSystemProxy
participant scutil
participant ProcessEnvironment
applyProxyEnvWith->>readMacOSSystemProxy: Read system settings when inherited proxy variables are absent
readMacOSSystemProxy->>scutil: Run --proxy
scutil-->>readMacOSSystemProxy: Return settings
readMacOSSystemProxy-->>applyProxyEnvWith: Return validated proxy result
applyProxyEnvWith->>ProcessEnvironment: Apply eligible proxy and bypass variables
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was identified. Native macOS proxy settings remain untested, so normal platform validation is still advisable. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Opt-in macOS discovery can change outbound routing for the process. The reviewed code limits when discovery runs and rejects settings it cannot safely translate; no exploitable weakness was established. Native macOS behavior remains to be verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (11 skipped: 11 unsupported.)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 64 / 80이 PR은 맥에서
예외 목록은 두 전송 길이 같은 뜻으로 옮길 수 있을 때만 써요. IP 주소와 라인 - 라인 - 메인테이너의 판단이 필요한 지점 예외를 하나라도 안전하게 못 옮기면 자동 찾기 전체를 포기하는 쪽을 유지할지 정하면 돼요. 회사 맥은 링크 로컬 범위를 빼서 맥 기본 목록( 찾기가 거절되거나 프록시가 꺼져 있을 때, 설정 파일의 #5893은 아직 열려 있어요. 이 PR이 그 내용을 현재 너의 추천 바탕은 218행에서는 맥 설정 읽기만 건너뛰게 하세요. 프록시 환경 변수가 이미 있어도, 윈도우와 같이 설정의 이 댓글은 grok-bot이 작성했습니다 |
Summary
proxy: "auto", carried from feat(proxy): macOS system proxy auto-discovery #5893 onto currentdev. Windows discovery already existed; on macOS, "auto" now reads the globalscutil --proxydictionary once at startup (fixed path/usr/sbin/scutil, 2 s timeout, 64 KiB bound) and applies an enabled static HTTP/HTTPS proxy. Closes Feature: macOS system proxy discovery for proxy: "auto" (slice 2 of #1525) #5853 once this is ondev.proxy: "auto", and it never runs whenHTTP(S)_PROXY, the lowercase forms,ALL_PROXY, orall_proxyis inherited. The inherited route keeps both its proxy and its bypass variables. Nothing runs on the request path.*translate directly.*.<domain>becomes.<domain>(Bun matches on label boundaries, sofoo.localgoes direct andxlocaldoes not; the bare apex also goes direct). The macOS default link-local ranges169.254/16,169.254.0.0/16, andfe80::/10are dropped with a one-line notice. Any other CIDR, glob shape, simple-host rule,ExcludeSimpleHostnames, PAC, or WPAD refuses discovery before any environment write, as does an unrepresentable configuredlocalhostalongside an inherited lowercaseno_proxy. The default macOS list (*.local,169.254/16) therefore activates.no_proxybeforeNO_PROXY, whileresolveProxyRoutehonors an explicit uppercaseNO_PROXY. Proxy URLs are logged in redacted form only.structure/config-proxy.mdand the English plus seven translated server-configuration pages describe the rules.Supersedes #5893. No GUI change.
Verification
bun test tests/server/proxy-env.test.ts tests/server/proxy-env-macos.test.ts tests/lab/core-lab-boundary.test.tswith a temporary HOME: 122 pass, 1 Windows-only skip, 0 fail. Coverage includes activation with the default list, refusal cases with a byte-identical 8-variable snapshot, inherited HTTP(S) and SOCKS precedence, split lowercase/uppercase bypass lists with route assertions for both transports, configurednoProxystaying direct on both, thelocalhostrefusal, malformed and disabledscutiloutput, and unchanged Windows behavior.scutilis mocked, so no real macOS Settings session was exercised..localbypassesfoo.localand notxlocal, while*.localand CIDR entries have no effect.bun run typecheck,bun run structure:check,bun run privacy:scan, and the docs build (537 pages) all exit 0.bun run test:changedis not passing evidence. The implementer's run overlapped test runs from other lane worktrees and exited 1 (26,718 pass / 110 fail / 17 errors, mostly timeouts in unrelated suites). I omitted the full local suite for the same resource-contention reason, so exact-head CI is the broad gate.noProxywas missing from lowercaseno_proxyand that barelocalhostcould be widened or dropped. All three were fixed, and the final round returned PASS. One residual predates this change and is outside it: a configuredlocalhostin uppercaseNO_PROXYis exact for the WebSocket matcher but a suffix for Bun.devbehaves the same way on every proxy path.Checklist
Co-authored-by: codingbo 9621077+codingbooo@users.noreply.github.com
Summary by CodeRabbit
proxy: "auto"now discovers static HTTP/HTTPS proxy settings on macOS as well as Windows, unless macOS proxy settings are inherited from the environment.*, IP addresses, and*.<domain>patterns. Unsupported exclusions prevent discovery without changing proxy environment settings.