fix: advertise route-aware LAN Control UI links - #98482
Conversation
05a7260 to
0fd2d66
Compare
|
Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 4:59 AM ET / 08:59 UTC. Summary PR surface: Source +371, Tests +494, Docs 0, Other 0. Total +865 across 35 files. Reproducibility: yes. from source and PR discussion: current main uses first-private-interface LAN display links, while the contributor's live output shows a host where that differs from the OS default-route interface. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the route-aware advertised-link behavior after maintainer acceptance of the narrow gateway-runtime SDK seam, keeping local probe behavior loopback-first. Do we have a high-confidence way to reproduce the issue? Yes from source and PR discussion: current main uses first-private-interface LAN display links, while the contributor's live output shows a host where that differs from the OS default-route interface. Is this the best way to solve the issue? Mostly yes: the route-aware resolver plus display/probe split is the right code shape, but the public SDK export remains a maintainer contract decision before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0ba41effeb93. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +371, Tests +494, Docs 0, Other 0. Total +865 across 35 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Follow-up for the review feedback on this PR. What changed in
Inspectable live proof from current branch head {
"platform": "darwin",
"routeHints": [{ "interfaceName": "en9" }],
"candidates": [
{ "interfaceName": "en0", "address": "192.168.1.139", "order": 0 },
{ "interfaceName": "en9", "address": "192.168.1.234", "order": 1 },
{ "interfaceName": "bridge100", "address": "10.211.55.2", "order": 2 },
{ "interfaceName": "bridge101", "address": "10.37.129.2", "order": 3 }
],
"selectedHost": "192.168.1.234",
"advertisedLinks": {
"httpUrl": "http://192.168.1.234:18789/",
"wsUrl": "ws://192.168.1.234:18789"
},
"localProbeLinks": {
"httpUrl": "http://127.0.0.1:18789/",
"wsUrl": "ws://127.0.0.1:18789"
}
}That shows the prior first-private candidate would have been Live Gateway proof on non-default port Live Security/regression check: Validation run after the feedback fix: The earlier SDK export concern should now be addressed at the owner boundary, and the live proof is pasted here so the behavior evidence is inspectable from PR discussion. @clawsweeper re-review |
|
馃馃Ч I asked ClawSweeper to review this item again. |
* Route LAN pairing URLs by default route * Advertise route-aware LAN Control UI links * Fix route-aware LAN test mocks * Narrow advertised LAN SDK export
* Route LAN pairing URLs by default route * Advertise route-aware LAN Control UI links * Fix route-aware LAN test mocks * Narrow advertised LAN SDK export
What Problem This Solves
Fixes an issue where users pairing a nearby device or copying Control UI links would be shown an unreachable LAN URL when the host has multiple private IPv4 interfaces and the first enumerated interface is not the default route.
This affects QR/setup-code pairing and the Control UI links printed by setup, configure, status, and daemon status surfaces. It is easiest to hit on Windows hosts with Hyper-V, WSL, VPN, or Parallels-style virtual adapters.
Related context: no exact GitHub issue matched the route-aware advertised-host bug. Older closed issues such as #19004, #18259, #11329, and #8823 covered sibling LAN/probe/bind behavior, but not the default-route advertised-host selection fixed here.
Why This Change Was Made
The fix separates remote-device display links from same-host probe links. Advertised LAN URLs now prefer the OS default-route interface, while local readiness probes continue to use loopback for
bind=lanso same-host health checks do not accidentally exercise remote pairing behavior.The default-route resolver is shared by setup-code pairing and Control UI display surfaces, with platform-specific route hints for Windows, macOS, and Linux. If route probing is unavailable, the behavior falls back to the existing interface-order LAN selection.
AI-assisted.
User Impact
Users with route-aware LAN environments should now see QR, setup-code, dashboard, configure, onboard, daemon status, and
status --alllinks that point at the LAN address a nearby device can actually reach.Existing local CLI and health-check behavior remains loopback-first, so the earlier local/remote pairing regressions should not come back.
Evidence
pnpm test src/infra/advertised-lan-host.test.ts src/pairing/setup-code.test.ts src/commands/onboard-helpers.test.ts src/commands/configure.wizard.test.ts src/wizard/setup.finalize.test.ts src/cli/daemon-cli/status.gather.test.ts src/cli/daemon-cli/status.print.test.ts src/commands/status-all/format.test.tsnode scripts/run-vitest.mjs src/infra/advertised-lan-host.test.ts src/pairing/setup-code.test.ts src/commands/onboard-helpers.test.ts src/commands/configure.wizard.test.ts src/wizard/setup.finalize.test.ts src/cli/daemon-cli/status.gather.test.ts src/cli/daemon-cli/status.print.test.ts src/commands/status-all/format.test.ts$autoreviewrun before commit; no accepted/actionable findings remained.bind=lansame-host checks still do not depend on the advertised LAN URL.