fix(test): reserve the picker port pair through listener startup

[codingbooo]

Closes #6014

What

tests/claude-integration/claude-desktop-picker-routes.test.ts discovered a consecutive CONNECT/picker port pair by binding probe servers and closing them before startPicker() created the real runtime listeners. Those values were observations, not reservations, so any other listener — including the fixture's own ephemeral TLS listener bound to port 0 — could claim either port in the gap. Exact-head CI of #6006 hit:

startPicker logged listen EADDRINUSE: address already in use 127.0.0.1:42429

The runtime correctly cleared the picker/controller on that failure, so the management route returned 503 instead of the test's intended 409 policy assertion. The test was flaky, not the product.

Fix

The fixture no longer probes and releases. It injects a CONNECT-listener factory that binds the real picker listener on port 0, so the OS assigns the pair atomically and ownership is held continuously through listener startup. Production binding behaviour is unchanged.

Regression

The intended callerAddedTrust refusal path (the 409 policy plus trust cleanup) is now exercised without depending on a released consecutive port pair. The 503 assertion was not retried or papered over.

Verification

• bun test tests/claude-integration/claude-desktop-picker-routes.test.ts — 9 passing
• same command repeated 10 consecutive times — 9 passing every run, 0 failures
• bun test tests/server/claude-intercept-integration.test.ts tests/claude-integration/claude-picker-runtime.test.ts — 20 passing
• bun run typecheck, bun run structure:check, bun run privacy:scan, git diff --check — clean

Summary by CodeRabbit

• Tests
  • Expanded Claude Desktop integration coverage for proxy routing with dynamically assigned ports, and for trust-refusal behavior that removes trust without probing the port.
• Documentation
  • Clarified how Claude Desktop proxy tests handle dynamically assigned ports and how the optional proxy-binding override works. Default proxy binding behavior remains unchanged.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• Required local validation passed; commands, results, and any full-suite exception are documented.

• I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

[coderabbitai]

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: 16e9c365-30eb-4302-818a-a48fa4c67ca1

📥 Commits

Reviewing files that changed from the base of the PR and between d8b85ad and 3f178a2.

📒 Files selected for processing (4)

• src/claude/intercept/runtime.ts
• structure/clients/claude-desktop.md
• structure/runtime.md
• tests/claude-integration/claude-desktop-picker-routes.test.ts

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

---

📝 Walkthrough

Walkthrough

The intercept runtime now accepts an optional CONNECT proxy binder for both proxies. The Claude Desktop picker route test uses real proxies bound on port 0 and checks the actual picker port in the applied profile URL.

Changes

Picker proxy binding

Layer / File(s)	Summary
Inject proxy binding and verify picker port src/claude/intercept/runtime.ts, tests/claude-integration/claude-desktop-picker-routes.test.ts, structure/clients/claude-desktop.md, structure/runtime.md	The runtime uses the injected binder when provided and otherwise uses the production binder. The integration test replaces free-port-pair probing with real proxies bound on port 0, checks the requested ports and actual picker port, and uses that port in the applied profile URL assertion. The documentation describes the optional test factory and the picker route test.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 3f178

Production binding remains the default, while the integration fixture uses actual bound proxy ports. No actionable merge-blocking risk is established by the reviewed changes.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3f178

The server continues to use its existing loopback proxy binding, while the test uses the same binder to hold real ephemeral listeners. No new production attack path was identified, but use of the exported override by consumers outside this repository is not covered.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — No additional production network reachability is evident: the production caller supplies no override, and its selected binder retains loopback binding. An independently written embedding caller is outside the inspected scope.

Trust Boundaries and Controls

• observed — The test override forwards the original proxy options to the production binder, retaining its binding and CONNECT-policy implementation rather than installing a separate proxy.

Resilience and Maintainability Implications

• observed — Published active state is cleared only when it still belongs to the stopping handle; that handle closes its captured picker, proxies and listener.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2…	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 clearly summarizes the main change: the picker test now reserves the port pair through listener startup by using an injected CONNECT-listener factory.
Linked Issues check	✅ Passed	Issue #6014 requires continuous port ownership, unchanged production binding, and coverage for the caller-added-trust refusal path. In src/claude/intercept/runtime.ts, `StartClaudeInterceptOptions.s…
Out of Scope Changes check	✅ Passed	The changes stay within Issue #6014. The runtime change adds only the test factory seam. The picker-route test changes port setup and adds assertions for real bound ports and the refusal path. The doc…

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[lidge-jun]

리뷰 · 우선순위 32 / 80

이 PR은 테스트가 포트를 빌렸다가 바로 반납해서 가끔 실패하던 것을 고칩니다.

Claude Desktop 피커 테스트는 빈 포트 두 개를 찾아 열었다가, 진짜 서버를 켜기 전에 닫았습니다. 닫힌 번호는 아무도 잡고 있지 않습니다. 그 사이에 테스트 안의 TLS 수신이나 다른 프로그램이 같은 번호를 가져갈 수 있습니다. 피커 수신이 EADDRINUSE로 실패하면 런타임은 피커를 없앱니다. 관리 화면은 거절(409) 대신 서버 없음(503)을 돌려줍니다. 이 실패에서 그 동작은 맞습니다. 흔들린 쪽은 테스트입니다. #6006 CI 로그가 그 경우입니다.

이제는 번호를 빌렸다가 놓지 않습니다. startClaudeIntercept에 테스트용 serverDeps.startConnectProxy가 생겼습니다. 이 테스트만 그 함수를 바꿉니다. 바뀐 함수는 런타임이 고른 번호를 적어 두고, 실제 소켓은 포트 0으로 엽니다. 운영체제가 빈 포트를 정하고, 소켓은 테스트가 끝날 때까지 유지됩니다. 제품을 보통대로 켜면 계산된 포트에 묶입니다. 거절(409)과, 방금 넣은 신뢰를 다시 빼는 검사는 그대로입니다. 베이스는 dev입니다.

같은 이슈 #6014를 닫는 PR #6015도 dev에 열려 있습니다. 프로브를 없애고 포트 0에 묶는 점도 같습니다.

라인 - src/claude/intercept/runtime.ts 119행 serverDeps. 같은 옵션의 테스트 칸은 createPicker, pickerSecurity처럼 이름만 있습니다. #6015는 그 옆에 startProxy를 둡니다. 이 PR은 함수 하나를 serverDeps 안에 넣습니다. 두 PR을 같이 머지하면 이 자리가 충돌합니다.

라인 - tests/claude-integration/claude-desktop-picker-routes.test.ts 79행. 포트 세 개가 서로 다른지만 봅니다. 리스너 포트가 비어 있어도, 프록시 두 개만 다르면 이 검사는 통과합니다. #6015는 각 값이 1 이상의 정수인지도 봅니다.

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

#6015와 #6024 중 하나만 남겨야 합니다.

테스트 칸을 serverDeps로 묶을지, #6015처럼 startProxy 하나로 둘지 정해야 합니다.

너의 추천

#6024를 남기고 #6015를 닫으세요. 설명 위치는 이쪽이 맞습니다. 피커 절과 structure/runtime.md에 적혀 있습니다. #6015의 문단은 User-Agent 설명 중간에 있습니다.

119행의 칸 이름은 startProxy로 바꾸세요. 79행에서는 세 포트가 모두 1 이상의 정수인지도 보세요. 10200과 10201을 요청하는지 보는 검사는 유지하세요. 공개 포트 10100에 100을 더한 기본 동작입니다. 베이스는 dev로 두세요.

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

[github-actions]

✅ Deterministic PR hygiene checks passed.

[Ingwannu]

Closing as a duplicate of the earlier #6015 for #6014. Both fixes replace the probe-and-release fixture with real port-0 listeners that remain bound through teardown, while #6015 also asserts all assigned ports are positive/distinct and already has successful exact-head executable CI. #6024 currently has action_required checks with no jobs. The PR-body wording that the OS assigns the pair atomically is also imprecise: these are two sequential live binds, although the ownership fix itself is sound. Keep #6014 open until #6015 lands.

[github-actions]

⏳ DRAFT

• review readiness checklist open (0/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

• ⬜ Required local validation passed; commands, results, and any full-suite exception are documented.
• ⬜ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
• ⬜ I resolved all correct Codex and CodeRabbit findings.
• ⬜ My PR is ready for review.

0/4 boxes ticked.

Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

Hygiene

✅ Deterministic PR hygiene checks passed.