feat(bridge): opt-in remote daemon attach for cross-machine sessions - #53
feat(bridge): opt-in remote daemon attach for cross-machine sessions#53doug-w wants to merge 1 commit into
Conversation
puritysb
left a comment
There was a problem hiding this comment.
The same-socket design and test coverage look solid, but the documented opt-in invariant is not currently enforced. startSession() always passes hostHint to resolveDaemonTarget(), and that function processes an explicit host before checking opts.remote. As a result, --daemon-host / AGENTDECK_DAEMON_HOST alone enables a remote connection even when --remote-daemon is absent. This conflicts with the issue decision and PR claim that all remote behavior is gated behind the explicit opt-in switch. Please either gate explicit-host resolution on remote === true and add a regression test, or deliberately change the policy/docs/issue contract so --daemon-host itself is the opt-in. Given the security boundary, this should be unambiguous and tested.
Address PR puritysb#53 review: the documented opt-in invariant was correct in behavior but not self-evident or regression-tested. `resolveDaemonTarget` processed an explicit host before the `remote` switch, which read as "host alone silently enables remote." Clarify the contract in code and docs: the two remote paths each require a distinct, user-typed opt-in and neither runs on ambient state — an explicit `--daemon-host` *is itself* the opt-in (a named node, never a silent auto-attach), while mDNS auto-discovery is gated solely behind the `--remote-daemon` switch. With neither and no local daemon, resolution returns null (local-only, unchanged). - Inject findLocal/probeHealth/discover deps into resolveDaemonTarget so the precedence + opt-in gate are unit-testable without the network. - Add `resolveDaemonTarget precedence + opt-in gate` regression tests: local wins and skips remote; explicit host opts in without touching mDNS; unreachable named host returns null (no LAN fallthrough); mDNS runs ONLY when remote===true; no opt-in never leaves the machine. - Document the security-boundary gate in docs/daemon.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FeDdptNxWmPfGK4Vph6VUk
|
Addressed in dbef6b4. Made the opt-in gate explicit, self-documenting, and regression-tested rather than changing runtime behavior — because the shipping docs already treat an explicit host as its own opt-in (README lists The contract is now stated unambiguously — two distinct, user-typed opt-ins, neither firing on ambient state:
If you'd rather go the other way — require |
puritysb
left a comment
There was a problem hiding this comment.
Thanks for clarifying the intended gate and for adding the regression tests. I re-reviewed the full PR at dbef6b4, including the real-socket tests. The same-socket direction remains sound, but there are several merge-blocking issues that are not covered by the current suite:
-
The agreed opt-in contract is still not enforced. In issue #24 I explicitly agreed to keep
--remote-daemonas the opt-in switch and--daemon-hostas the explicit endpoint, and later reiterated the--remote-daemon+ local-default invariant.resolveDaemonTarget()still processeshostHintwhenremote !== true. The standalone--daemon-hostREADME example is new in this PR, so it cannot be used as the pre-existing contract. Please requireremote === truefor both remote paths and document the explicit form as--remote-daemon --daemon-host mainnode.lan; explicit-host failure should still return null without mDNS fallback. -
The documented SSH local-forward topology is not registered as remote.
session_push_registeronly callsregisterRemoteSessionwhen!isLocalConnection(senderIp). Withssh -L 9120:localhost:9120 mainnodeand--daemon-host 127.0.0.1, the main-node daemon sees the push socket as loopback, skips remote registration, and the worker is not in that machine'ssessions.json, so it never becomes listable/focusable. The E2E test uses a rawWsServerharness and does not exercise this daemon-side classification. Please add an integration test that goes through the real registration handler for the SSH-forward/loopback case. -
A stale socket can delete a newer registration. Re-registering the same
sessionIdreplacessender, but every socket installs an unconditional close handler that removes bysessionIdalone and callshandleSenderClosed(sessionId). If the new socket registers before the old one finishes closing, the old close removes/unfocuses the new connection. Removal and focus teardown need sender identity checks, plus an overlapping old/new socket regression test. The same ownership check should be applied tosession_event_up, and down-frames should be ignored by a worker whenmsg.sessionId !== this.sessionId. -
The daemon pairing token is written to debug logs.
DaemonWsClientlogs the complete URL after appending?token=.... Please log only the redacted host/port. -
Dial-back is described as opt-in but is enabled for every session.
startSession()unconditionally creates and registers an extra accepted callback token, including the default local-only path. In addition, the remote registry is removed when the push socket closes, so the advertised no-live-sender fallback is effectively unavailable after disconnect. Please either remove this fallback for now or put it behind an explicit, tested opt-in and make its lifecycle reachable. -
Remote targets are rejected by port number alone.
resolved.port !== core.portdrops a valid daemon on another host whenever it happens to use the same numeric port as the worker session. The self-port guard must also require a loopback/local host.
This does not require implementing the feature in the Swift daemon if the intended main node is the Node CLI daemon. However, the docs currently say only "daemon", and discovery accepts any /health response with mode === 'daemon'; a worker can therefore attach to the App Store Swift daemon, register successfully, and then silently lack the new same-socket control frames. Please scope the docs to the Node CLI daemon and preferably require an advertised /health capability such as sameSocketControl so unsupported daemons are not selected.
Verification on the PR branch: TypeScript --noEmit passed and the full Vitest suite passed (100 files, 1726 tests). GitHub currently has no check runs for this head. The branch also conflicts with current master in bridge/src/daemon-server.ts and bridge/src/ws-server.ts; please rebase carefully because both files have gained recent steering/topology behavior.
|
Moving this PR back to Draft to match its actual state. The same-socket design and two-machine validation remain the right foundation, but the current head is 133 master commits behind, conflicts in
Please rebase on current master, add the requested integration regressions, and rerun the real Windows-main/Mac-worker scenario before marking ready for review again. Issue #24 remains open and assigned to you. |
|
Friendly ping — no new commits on The seven blockers from that review remain open:
If you can rebase onto current master, add the requested integration regressions, and rerun the real Windows-main/Mac-worker scenario, I'll re-review. Otherwise I'll close this PR as stale by 2026-08-01 to keep the PR queue accurate. Issue #24 stays open and assigned to you so the work isn't lost, and you're welcome to resubmit whenever it's ready. |
Address PR puritysb#53 review: the documented opt-in invariant was correct in behavior but not self-evident or regression-tested. `resolveDaemonTarget` processed an explicit host before the `remote` switch, which read as "host alone silently enables remote." Clarify the contract in code and docs: the two remote paths each require a distinct, user-typed opt-in and neither runs on ambient state — an explicit `--daemon-host` *is itself* the opt-in (a named node, never a silent auto-attach), while mDNS auto-discovery is gated solely behind the `--remote-daemon` switch. With neither and no local daemon, resolution returns null (local-only, unchanged). - Inject findLocal/probeHealth/discover deps into resolveDaemonTarget so the precedence + opt-in gate are unit-testable without the network. - Add `resolveDaemonTarget precedence + opt-in gate` regression tests: local wins and skips remote; explicit host opts in without touching mDNS; unreachable named host returns null (no LAN fallthrough); mDNS runs ONLY when remote===true; no opt-in never leaves the machine. - Document the security-boundary gate in docs/daemon.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FeDdptNxWmPfGK4Vph6VUk
dbef6b4 to
552d71e
Compare
|
Thanks for the rebase — the head is on today's master and merges clean now, and I have updated the branch onto the fixed base so the inherited red checks are no longer in the way (master's tip had drifted on two gates; both are resolved and master's CI is green). I re-checked the seven blockers from the 2026-07-18 review against this head. At least two are unchanged, and (3) Sender identity — still unguarded.
The close handler has the mirror-image bug: sender.once('close', () => {
removeRemoteSession(sessionId);
focusRelay.handleSenderClosed(sessionId);
...
});This removes the entry unconditionally. If worker B re-registers the same id on a new socket and worker A's older socket then closes, A's close evicts B's live registration and drops focus for a session that is still attached — a reconnect race, not an attack, so it will show up in ordinary use. This repo already has the rule written down, from a bug with the same shape on the device side (CLAUDE.md — "Pending device work keys on the device, not on its socket … must be keyed by the device and have its transport re-resolved at use time"). The fix is the same shape here: record the owning socket on the entry, and make both the re-register path and the close path no-op unless (4) Token redaction — the token is still logged.
const url = isLoopbackHost(host)
? `ws://127.0.0.1:${port}`
: `ws://${formatHostForUrl(host)}:${port}${token ? `?token=${encodeURIComponent(token)}` : ''}`;
debug(TAG, `Connecting to daemon at ${url}`);The daemon pairing token — the load-bearing one, not the ephemeral callback token — lands verbatim in the session bridge's debug log. Log host, port, and whether a token was present; never the URL. I have not re-audited 1, 2, 5, 6 and 7 in this pass. Rather than have me guess at their state, please walk all seven point by point the way you did on #62 — that write-up made the difference between a reviewable PR and one that stalls. Issue #24 remains open and assigned to you either way. |
|
All seven blockers from the 2026-07-18 review are addressed in 3079e2f, on a branch freshly rebased onto current master (the
One explicit note on the trust boundary, so it's on the record: Accepted side effect worth naming: with Suite: full bridge run green (1491 passed, 1 pre-existing Windows parallel flake in |
|
I integrated the latest The merge conflicted in
Verification:
The seven original code blockers remain resolved and the branch is now integrated with current |
|
Yep, I'm going to do the manual test pass this evening after work and paste into the PR what was done before taking this out of draft. |
dd1d2f6 to
004dbf5
Compare
Two-machine verification — all passed ✅Setup: Windows 11 main node (daemon + Stream Deck,
Covers blockers 1–4 and 7 end-to-end on real hardware; blockers 5 (dial-back removal) and 6 (host-aware self-port guard) are structural and covered by the unit/integration suites ( |
|
Thanks — this is the manual evidence I was waiting for. The T1–T8 matrix is clear, specific, and covers the real Windows-main / macOS-worker path we agreed was load-bearing in #24. I also re-checked the current squashed head This is the same kind of point-by-point, honestly scoped evidence that made #62 reviewable — thank you. I did one final lifecycle pass before clearing 1. A failed re-resolution keeps using the previous remote targetIn AgentDeck/bridge/src/daemon-ws-client.ts Lines 202 to 223 in 004dbf5 After one successful remote attachment, if the daemon disappears or is replaced by an incapable daemon, Please clear/update the cached target on every provider result, including 2. Capability-less daemons behind
|
004dbf5 to
3664433
Compare
|
Rebased onto current master (head |
3664433 to
06efcc6
Compare
|
All three final-round gaps are addressed in 1. Failed re-resolution no longer keeps the previous remote target
Regressions ( 2. Loopback/tunnel daemons now face the same capability gate
Without the switch the local path is byte-for-byte unchanged (plain local attach, no warning). One asymmetry worth naming explicitly: a capable local daemon still wins over an explicit 3. The push-state aggregator cache is sender-owned whenever a remote registration exists
One residual window, considered and left as-is: after the registered socket closes and the registry entry is pruned, a still-open zombie older socket's push finds no registration and takes the unguarded local path (visible up to the cache's 30s TTL in the dual-registration case). Guarding that would require keying ownership beyond the registration lifetime; it matches the letter of "whenever a remote registration exists" and the same paired-tier trust boundary as before. Non-blocking cleanup
Docs updated to match: |
--remote-daemon gates both remote paths (explicit --daemon-host and mDNS discovery); workers register with an explicit remoteAttach flag negotiated against the daemon's /health sameSocketControl capability, so ssh -L loopback topologies work without IP heuristics. Reverse control rides the worker's own push socket (session_focus_down / session_command_down / session_event_up) -- dial-back is removed entirely. Sender-identity guards plus focus migration protect overlapping reconnects; the pairing token is redacted from logs; the self-port guard is host-aware. Verified end-to-end on a Windows main node + macOS worker across local-only, explicit-host, mDNS, and ssh -L topologies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
06efcc6 to
cc7b2fe
Compare
|
(Head note: the remediation described above is now at |
Summary
Opt-in remote daemon attach: let a session bridge (
agentdeck claude/codex/opencode) attach to a daemon on another machine and be driven from that machine's single Stream Deck — for running agents on several boxes (often over SSH) with one deck on a main node. Default behavior is byte-for-byte unchanged (local-only).Closes #24.
Opt-in contract (review blocker 1)
--remote-daemon/AGENTDECK_REMOTE_DAEMON=1is the opt-in switch and gates both remote paths.--daemon-host <host[:port]>(envAGENTDECK_DAEMON_HOST, aliasAGENTDECK_REMOTE_DAEMON_HOST) only names the endpoint and requires the switch — the recommended explicit form is--remote-daemon --daemon-host mainnode.lan. A host hint without the switch is inert and the CLI warns pre-PTY (post-PTY stdout is swallowed); the warning condition shares one derivation helper (deriveRemoteAttachOpts) with the resolver so the two cannot drift. An unreachable or incapable named host returns null with no mDNS fallthrough. With no opt-in, nothing ever leaves the machine.Reverse control — same-socket ONLY
The worker's outbound push socket (
DaemonWsClient→ daemon) is bidirectional. When the daemon focuses a remote session it sends command frames down the socket the worker already opened — the daemon never dials back. A NAT'd / SSH-only worker needs only a single outbound connection and no inbound reachability.session_focus_down/session_unfocus_down(daemon → worker),session_command_down(daemon → worker, wraps aPluginCommand),session_event_up(worker → daemon, relayedBridgeEvent).applyPluginCommandhandler as the local WS server — local and remote control never diverge.WsServerextra-accepted-token surface, and the focus relay's remote dial path are gone. The machine token is the only accepted WS credential again. A remote session whose push socket dropped is unreachable until its worker reconnects.Remote classification (review blocker 2)
session_push_registercarries an explicitremoteAttach: trueflag — sent when the user opted in AND the resolved daemon advertises thesameSocketControlcapability. The daemon trusts the flag over the socket's source IP, so the documentedssh -L 9120:localhost:9120topology (worker arrives on loopback) registers, lists, and focuses correctly. The non-local-IP heuristic remains as back-compat for older workers.Sender-identity guards (review blocker 3)
Registration, state updates,
session_event_upingestion, and close-teardown are keyed to the session's registered sender socket: a stale socket cannot delete or clobber a newer registration. A reconnecting worker's new socket takes over focus atomically (SessionFocusRelay.migrateSender— old sender unfocused best-effort,session_focus_downre-sent down the new socket). Workers ignore down-frames whosesessionIdis not their own.Capability negotiation (review blocker 7)
The Node daemon's
/healthadvertisessameSocketControl: true. The explicit-host path refuses (null + one-shot PTY-visible warning) a daemon lacking it; the mDNS confirm step filters such candidates and propagates the flag on those it keeps. The Swift (macOS App Store) daemon does not implement the push-channel control frames and is therefore never selected as a remote hub; docs are scoped to the Node CLI daemon.Also fixed
isLoopbackHost(host) && port === core.port), so a remote daemon on the same numeric port is no longer rejected — blocker 6.Trust boundary (explicit, per review discussion)
/healthis unauthenticated and serves the pairing token — any LAN peer can self-serve credentials, soremoteAttachis spoofable by a paired-tier client, and takeover-by-re-register is inherently allowed (legitimate reconnects require it). This matches the pre-existing "paired = trusted" model (a paired client can already send commands). The sender-identity guards target stale-close races, not hostile LAN peers.Final-round lifecycle fixes (2026-07-31 review)
The three gaps from the final lifecycle pass are fixed (details in the point-by-point comment):
DaemonWsClientnow replaces its cached target with the provider result on every resolution, null included — a vanished or downgraded daemon stops being dialed until a valid target resolves again. Plus a client-side capability guard under remote intent (defense against resolver drift).--remote-daemon, the local-first branch also requiressameSocketControl— an incapable daemon behindssh -Lnow gets the one-shot refusal (no host hint) or falls through to the explicitly named host (with--daemon-host), never a silent plain registration. A capable local daemon still wins over--daemon-host(pre-existing precedence, unchanged).session_push_stateis accepted only from the registered sender socket for the whole update — shared push-state cache included, so a stale pre-reconnect socket can't poison the deduped local row. Pure-local pushes are untouched.REVERSE_RELAYED_EVENTSwas also removed — both ends of the push socket now import the one canonicalRELAYED_EVENTSset.Tests
Extracted the push-channel handling into
bridge/src/session-push-channel.ts(narrow injectable deps) so the real registration/state/event path is integration-tested: ssh -L loopback registration → listable → focusable; overlapping old/new sockets with focus migration;session_event_upownership rejection; unconditional ack. Plus: gate-flip regressions (host hint without switch never probes or browses), capability refusal + mDNS filter/propagation tests, worker foreign-sessionId guard over real sockets, stale-socket guards in the registry, and the same-socket E2E. The final-round fixes add 9 regressions: stale-target lifecycle over real sockets (null + incapable-via-real-resolver + client guard), loopback capability enforcement (refusal, hostHint fallthrough,ssh -Ltunnel shape,remote: falseuntouched, warn dedup), and the sender-ownership overlap proof (old socket can change neither the remote registry nor the effective push-state cache; stranger rejected; pure-local path preserved). Full suite green apart from known Windows-host-only failures in files this branch doesn't touch.Verification
Validated end-to-end on the real two-machine rig (Windows 11 main node + macOS worker, both on the remediated build): the full T1–T8 matrix in the verification comment — baseline local-only unchanged, inert host hint + pre-PTY warning, explicit remote attach with full deck reverse control, token redaction,
ssh -Lloopback registration, worker-restart focus migration, capability refusal of a pre-remediation daemon, and mDNS auto-discovery. The final-round fixes change no transport behavior (frames, wire format, and registration untouched), so per the reviewer's stated bar they are covered by the focused regressions + CI rather than a rig re-run.🤖 Generated with Claude Code