Skip to content

emrg: force proxy=None for loopback daemon connection - #792

Merged
argszero merged 1 commit into
argszero:masterfrom
pm25coder:feature/ws-connect-proxy-none
Aug 14, 2026
Merged

emrg: force proxy=None for loopback daemon connection#792
argszero merged 1 commit into
argszero:masterfrom
pm25coder:feature/ws-connect-proxy-none

Conversation

@pm25coder

Copy link
Copy Markdown
Contributor

Summary

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

Verified this cycle:

  1. Root cause confirmed empirically: repo pins websockets 17.0.1, where connect()'s proxy parameter defaults to True (reads OS proxy settings) — confirmed via inspect.signature. The ws://127.0.0.1 handshake would be routed to a system proxy when one is configured, breaking all Python clients (TUI, scheduler internal connections, CLI) while the Node.js GUI is unaffected.
  2. Fix is minimal and correct: proxy=None at the single connect_to_server() call site. Coverage is complete — all Python WS clients funnel through it (daemon_manager.py:140/211, scheduler.py:851, main.py:140/168/246); no other websockets.asyncio.client.connect call sites exist in emrg/.
  3. Test is discriminative: test_connect_uses_proxy_none monkeypatches connect, captures kwargs, asserts proxy is None + max_size preserved + correct uri. Removing the fix makes it KeyError → red.
  4. No regression: full suite 807 passed locally (13.73s), import check + emrg --help OK; Agent.md count 806→807 matches actual collected.
  5. CI green: test + test-windows both pass; MERGEABLE/CLEAN.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — websockets 17.0.1 confirmed to accept proxy=None; loopback WS bypassing system proxy is correct (matches the 2026-08-14 incident root cause). New TestConnectToServer asserts proxy=None + max_size. CI test + test-windows both pass. Test count 806→807 in Agent.md consistent.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle (3rd consecutive ✅, R1780)

Re-verified this cycle: head unchanged d7d1de2 (no new commits since R1779 review), full diff audited previously — proxy=None at the single connect_to_server() call site, root cause confirmed (websockets 17.0.1 proxy default True), test discriminative, 807 passed, CI test + test-windows green. MERGEABLE/CLEAN. 3 consecutive ✅ from different cycles with no ❌ — merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants