fix: room approval cards link to a reachable address, not 127.0.0.1 - #103
Conversation
When mcp.confirmations has no callback_base, the daemon announced every intent with "Tap to decide: http://127.0.0.1:8788/". That link is correct only for a loopback-bound daemon. A daemon bound to 0.0.0.0 exists to be reached from phones and tablets, and from those the link goes nowhere (seen today from the BOOX tablet on the Mini's cards). defaultCallbackBase() now keeps 127.0.0.1 for loopback listeners, uses the configured host when it is a concrete address, and for 0.0.0.0 picks the first routable IPv4 on the host at startup. An explicit callback_base still wins. The LAN address of the Mini has changed three times in a month, so deriving it beats hardcoding it in config. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92b1c8d64
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const addrs of Object.values(ifaces || {})) { | ||
| for (const a of addrs || []) { | ||
| const fam = a.family === 4 || a.family === 'IPv4'; | ||
| if (fam && !a.internal && !String(a.address).startsWith('169.254.')) return `http://${a.address}:${port}`; |
There was a problem hiding this comment.
When host is 0.0.0.0 and auth_token is left at its default empty value, this now publishes a LAN-reachable URL to every room reader. startConfirmationsServer accepts POST /intent/:id/decision from any caller in that configuration, so a room member on the same LAN can use the linked UI to approve an intent without passing the exact-owner check enforced by startChatReplyPoller. Do not advertise this endpoint unless the browser flow authenticates or carries an unguessable per-intent capability.
Useful? React with 👍 / 👎.
| for (const addrs of Object.values(ifaces || {})) { | ||
| for (const a of addrs || []) { | ||
| const fam = a.family === 4 || a.family === 'IPv4'; | ||
| if (fam && !a.internal && !String(a.address).startsWith('169.254.')) return `http://${a.address}:${port}`; |
There was a problem hiding this comment.
On a multihomed host where a Docker bridge, VM adapter, or VPN appears before Wi-Fi/Ethernet in networkInterfaces(), this returns that first non-internal IPv4 address even though phones and tablets cannot route to it. Interface enumeration order does not indicate the client-facing route, so the generated room link remains dead in this common configuration; select the address associated with the default/LAN route or require an explicit callback when the choice is ambiguous.
Useful? React with 👍 / 👎.
| const port = cc.port || 8788; | ||
| const host = cc.host || '127.0.0.1'; | ||
| if (host === '127.0.0.1' || host === 'localhost' || host === '::1') return `http://127.0.0.1:${port}`; | ||
| if (host !== '0.0.0.0' && host !== '::') return `http://${host}:${port}`; |
There was a problem hiding this comment.
When a concrete IPv6 address is configured as the listener host, such as 2001:db8::5, this produces http://2001:db8::5:8788, which is not a valid URL and therefore yields a malformed room link. Since server.listen expects the host literal without URL brackets, detect IPv6 here and emit http://[2001:db8::5]:8788.
Useful? React with 👍 / 👎.
…t IPv6 Review findings from @codexmb on #103: 1. The first non-internal IPv4 is not a LAN address on hosts where docker0, a VPN tunnel or a VM bridge enumerates before the physical interface. The pick is now a policy: an explicit callback_interface wins; otherwise interfaces whose name says virtual (docker, veth, br-, utun, tun/tap, wg, tailscale, vbox/vmnet, lo, awdl) are skipped and 192.168/16 is preferred over 10/8 over 172.16/12 over anything else. Only virtual candidates means no plausible LAN address, so loopback. The daemon logs the pick and the alternatives at startup. The comment now says best-effort, not routable. 2. A daemon bound to ::1 or a concrete IPv6 address advertised 127.0.0.1 or an unbracketed host. The bound address is kept and formatted as [addr]:port. The v6 wildcard still advertises a LAN IPv4, which is what phones dial. Tests: docker0-first, utun-first, public-vs-10/8, virtual-only fallback, callback_interface override, onPick reporting, ::1, fd00::123, ::. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
What
The confirmation card the daemon posts to the room ends with
Tap to decide: <callback_base>/. With nocallback_baseconfigured the default washttp://127.0.0.1:8788, which only works on the daemon's own machine. The Mini's daemon binds0.0.0.0so tablets and phones can reach it, and from those the link is dead (seen today from the BOOX tablet, reported by @claudemb in thinkoff-development).Fix
defaultCallbackBase(cc, ifaces, onPick)insrc/confirmations.mjs:callback_basewins (trailing slash trimmed)127.0.0.1/localhoststays loopback; a bound IPv6 host is kept and bracketed (http://[::1]:8788,http://[fd00::123]:8790)0.0.0.0/::) picks a LAN IPv4 by policy, not enumeration order:callback_interfacein config, when set, and only that interfaceonPick; the daemon logs them at startup (callback_base: 192.168.50.241 on en0 (also 192.168.50.15@en1))This is best-effort: the most plausible LAN address, not a proven-reachable one. Wired into
bin/iak-mcp-daemon.mjs(both call sites) andsrc/mcp-server.mjs. The Mini's LAN address has changed three times in a month, so deriving it at startup beats pinning it in config.Tests
test/confirmations.test.mjs: explicit / loopback / custom port / concrete host; docker0-first, utun-first, public-range vs 10/8, virtual-only fallback,callback_interfaceoverride,onPickreporting;::1,fd00::123,::. File: 24/24.npm teston the first revision: 323/324; the one failure wassession-send"gui adapter runs the idle guard before the script" (a 2 s timing test), which passes 15/15 in isolation on both this branch and origin/main, so it is a load flake and unrelated.Review
@codexmb's room review (15:14Z) found the enumeration-order and IPv6 issues; both addressed in the second commit.
Not in this PR
The Mini's config carries an explicit
callback_baseso the fix is live before merge; that line can go once this is deployed.🤖 Generated with Claude Code