Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe relay handshake now exchanges protocol version offers and capability data. It admits compatible cross-build peers through validated version ranges while preserving build-hash checks for legacy peers. Handshake logs and mismatch frames include protocol versions. New tests cover protocol validation, handshake round trips, capabilities, hostile claims, credentials, and legacy behavior. The SSH mismatch parser now excludes trailing protocol metadata from daemon version captures and has dedicated refusal parsing tests. Priority: ⬆️ High Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The relay handshake changes preserve legacy build-hash fallback while adding validated protocol-range negotiation and refusal parsing support. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation Issue Full details: Out of Scope Changes checkExplanation The changed files implement generic relay protocol negotiation, capability advertisement, protocol-claim validation, and SSH mismatch parsing. These changes do not implement the partition ownership or card restoration behavior required by directly linked issue
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
There was a problem hiding this comment.
ℹ️ No critical issues — two rough edges worth a look.
Reviewed changes
This PR gives the relay a semantic integer RELAY_PROTOCOL_VERSION offered as a [min, max] range in the handshake, so a relay stranded by a content-hash-only update (build hash differs, wire unchanged) can admit a newer client on the first frame, while keeping the build-hash comparison as the fallback for peers that offer no range.
- Protocol negotiation (
src/shared/relay-protocol-version.ts) —RELAY_PROTOCOL_VERSION/MIN_RELAY_PROTOCOL_VERSION, therelayProtocolOfferAdmits()band check, and the crash-safedescribeRelayProtocolVersion()logger. - Handshake envelope (
src/relay/protocol.ts) — the three existing arms grow optionalprotocolVersion/minProtocolVersion;orca-relay-handshake-okalso carriescapabilities. - Daemon admit/refuse (
src/relay/relay-handshake.ts) — admits a peer when the hash matches or the offered range contains the relay's version; replies and refusals now carry the relay's own range and capabilities. - Mismatch parsing (
src/main/ssh/ssh-relay-handshake-mismatch.ts) — thedaemon=capture excludes a trailing comma so the newdaemonProtocol=/ours=fields don't bleed into the version. - Tests — round-trip, admit/refuse, hostile-claim, and parser coverage of the negotiation and fallback paths.
ℹ️ Nitpicks
relayProtocolOfferAdmitsonly tests whether the relay's top (RELAY_PROTOCOL_VERSION) falls inside the peer's range, never whether the relay's advertised floorMIN_RELAY_PROTOCOL_VERSIONoverlaps it. That's equivalent to a full range-overlap check today (MIN == VERSION == 1, a point range), but the moment the floor diverges from the top, a newer relay would refuse an older negotiating peer whose range still covers versions the relay advertises it can speak. Worth a one-line comment pinning the "relay is always the older side" assumption, or a symmetric check once that stops holding.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
| process.stderr.write(`[relay] Handshake OK from version=${msg.version}\n`) | ||
| sock.write(encodeHandshakeFrame({ type: 'orca-relay-handshake-ok', version: launchVersion })) | ||
| process.stderr.write( | ||
| `[relay] Handshake OK from version=${msg.version} (${msg.version === launchVersion ? 'same build' : `cross-build, protocol ${RELAY_PROTOCOL_VERSION}`})\n` |
There was a problem hiding this comment.
msg.version is still interpolated raw here, and this OK path is newly reachable with a hostile non-string version: before this PR it required msg.version === launchVersion, so it was always a string. A peer sending {"version":{"toString":1},"protocolVersion":1} now passes relayProtocolOfferAdmits, and the ${msg.version} throw escapes the frame-decoder callback and kills the daemon. The residual-risk note covers the mismatch line, but this is a second, newly-reachable instance of the same vector.
Technical details
# msg.version interpolation is still crashable on the daemon side
## Affected sites
- src/relay/relay-handshake.ts:173 — OK path interpolates `${msg.version}`; now reachable with a non-string `version`.
- src/relay/relay-handshake.ts:144 — mismatch path interpolates `${msg.version}` (pre-existing, unauthenticated).
## Required outcome
- Any JSON shape for the peer's `version` must not throw out of the frame-decoder callback. The OK-path instance is new to this PR (previously gated by strict string equality).
## Suggested approach
- Render `msg.version` through a string-only guard (a `describeRelayVersion` mirroring `describeRelayProtocolVersion` but accepting `typeof === 'string'`), on both lines. Severity is low — the OK path sits behind the endpoint-credential check and the mismatch path was already crashable — so this can ride the same follow-up as the flagged pre-existing bug.There was a problem hiding this comment.
ℹ️ No new issues in this commit.
Reviewed changes
Since the prior review, one commit landed (639bf32): the --connect mismatch parser's daemon= capture now stops at the trailing comma, and unit tests cover the parser.
- Parser fix (
src/main/ssh/ssh-relay-handshake-mismatch.ts) —daemon=([^\s;]+)→daemon=([^\s;,]+), so the newdaemonProtocol=/ours=fields no longer bleed a comma into the reported daemon version. - Parser tests (
src/main/ssh/ssh-relay-handshake-mismatch.test.ts) — pins the new line form, the pre-negotiation short form, unparseable input, credential-vs-version skew, and non-refusal exit codes.
ℹ️ Note
The expanded PR description's "Incidental find" section now documents the msg.version interpolation crash and calls it a deliberately deferred, pre-existing vector. That prose describes a single site in the mismatch log line; the OK-path instance at src/relay/relay-handshake.ts:173 (flagged in the prior review's inline comment) is newly reachable via this PR, not pre-existing. Worth reconciling the wording so the deferral note distinguishes the two.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
No production path sets it: the only `new RelayAuthCoordinator` is in desktop-relay-service.ts and passes neither `lingerMs` nor `random`, so every shipped linger is the hardcoded ten-minute default. Labelled so the next person tunes the default, which is the only thing a user can feel. Lifted from a dead-code and unread-field sweep that was stranded on an un-PR'd integration branch (94ba0b9); the rest of that sweep belongs to #19861.
…as trailing fields
…le yet relay-protocol-version.ts documents a negotiation no live path can reach, and reads as the fix for #13852. It is not. The deploy path namespaces the relay directory by the CLIENT'S OWN build hash -- remoteInstallDirName is `relay-<fullVersion>` -- and the daemon socket lives inside it. Every runConnectHandshake caller takes its path from that one deploy result, so a bridge can only ever meet a daemon of its own build, `msg.version === launchVersion` short-circuits first, and relayProtocolOfferAdmits never decides anything on any live path. The stranded incumbent this exists for sits in `relay-<otherVersion>/`, which nothing dials. Keeping it is right -- the mechanism is correct and hostile-input-safe, and it is the part that has to exist first. What was missing is the statement of what else has to land with it, written where someone editing this file will see it. Carried from a dead-code and unread-field sweep stranded on the un-PR'd branch nwparker/adv2-map-reconciled (94ba0b9). The lingerMs half of that sweep is a coordinator concern and went to #20061 instead. No behaviour change.
…le, not two The note the previous commit carried lists two prerequisites. There are three, and the missing one is the one a reader cannot see from this file. validateGrant's refusal is a single `if` with two disjuncts. Deleting the `serverBuildId` clause -- prerequisite (2) -- leaves the other standing: `grant.protocolVersion !== PTY_CONSUMER_SESSION_PROTOCOL_VERSION`, a different constant from this file's RELAY_PROTOCOL_VERSION, compared for exact equality with no range, no floor and no fallback. Two peers that just negotiated a compatible relay protocol are still refused if their PTY-session constants differ by one. And there is no channel to resolve it with. Verified rather than assumed: `capabilities` on orca-relay-handshake-ok is written at exactly one site (relay-handshake.ts) and read by nothing outside tests. It is a published field with no consumer, so (3) is not 'delete another clause' -- it is 'give that field a reader first'. The error text compounds it: the refusal interpolates only the build ids, so a pure protocol-version mismatch reports as 'expected build X, got X' with two identical ids, sending the next debugger at the gate that is not the problem. Anyone implementing N+1 from the two-item list ships a broken negotiation and debugs it at the wrong gate. Recorded here because this file is what they will read first.
d0aca79 to
bedc94c
Compare
|
Closing as obsolete after the merged SSH relay retirement in #27214 and #27215. This PR concerns the SSH relay handshake and protocol upgrade path. #27214 removed the desktop relay connection/fallback path; #27215 deleted the SSH relay server, deployment assets, and relay-only tests. SSH hosts now use the managed Orca server (orcad), and unsupported hosts fail explicitly, so this relay-specific work is no longer planned. This closes the proposed relay implementation; it does not claim that any independent local, WSL, or managed-server behavior was fixed. |

ELI5
Every time you update Orca, the relay that runs on your SSH host gets installed into a new folder whose name is a hash of the app's bytes. The relay listens on a socket inside that folder. So after an update, the new Orca looks in a new folder, finds no socket, and starts a second relay — while the old one keeps running forever, still holding every terminal and every agent you had open. They are not dead. They are unreachable. One field host had 11 folders, 11 sockets, and 19 orphans.
The reason it works this way is that
RELAY_VERSIONis the frozen string'0.1.0'and has never moved, so the content hash was added to tell builds apart. But a hash answers "are these the same bytes?", and the question that actually matters is "can these two talk to each other?" — which is almost always yes, because a rebuild changes bytes without changing the wire.This PR gives the relay a real answer to the second question: an integer
RELAY_PROTOCOL_VERSION = 1, offered as a range in the handshake. A relay now accepts a bridge from a different build as long as that bridge says it can speak a protocol the relay speaks. The byte-hash comparison stays as the fallback for every peer that offers no range.This is the same mechanism the local terminal daemon has always used (
src/main/daemon/daemon-protocol-version.ts), where 35 earlier protocol versions stay attachable across an update.One value doing three jobs
The build hash is a legitimate artifact identity — "are these the same bytes?" — that has been pressed into service as two other identities it was never fit for:
RELAY_PROTOCOL_VERSION— this PRSplitting the three is the fix. Release N introduces the protocol one and leaves the other two exactly as they are.
This is Release N of two
The relay already deployed on a user's host never updates itself. So tolerance has to ship one release before any client can use it.
The update to this release still strands once, unavoidably. The relay that gets orphaned by installing this build is the old one, which does not have this code. Nothing shipped now can reach back and fix that. From N+1 onward, it stops.
What changed
src/shared/relay-protocol-version.ts(new) —RELAY_PROTOCOL_VERSION = 1,MIN_RELAY_PROTOCOL_VERSION = 1, andrelayProtocolOfferAdmits().src/relay/protocol.ts— three existing handshake arms gain optional fields:protocolVersion/minProtocolVersionon all three, andcapabilitiesonorca-relay-handshake-ok.src/relay/relay-handshake.ts— the daemon admits a peer when either the build hash matches (unchanged) or the peer's offered range contains this relay's protocol version (new). Replies now carry the relay's own range and capabilities.Why a range and not a version number
Reviewers reach for a single integer first, and it does not work here. The stranded relay is by construction the older side, and the handshake's first frame is the first thing on the wire — there is no earlier round trip in which a newer client could learn what to downgrade to. So the client states a range and the relay checks whether it falls inside it. That lets an old relay say yes immediately, which is the whole point: the relay that needs to accept is the one that can never be updated.
Deliberately not changed
RelayVersionMismatchError, the PTY grantserverBuildIdcheck, and owner-claim recovery all stay. They are Release N+1's to retire.remote-install-gc.ts), so moving it would break reclamation.Wire compatibility
docs/reference/remote-wire-compatibility.mdRule 1 — new optional fields on frames that already ship.parseHandshakeMessagethrows on an unknowntypeand the throw closes the socket, so a new message type was never an option;endpointCredential?is the existing precedent for growing an arm.What an old relay does with the new optional fields: ignores them. Its
parseHandshakeMessagevalidates onlytypeand returns the object; unknown keys are never read. It then compares build hashes exactly as before and refuses on mismatch, exit 42. Verified on a real host — see scenario A below.What a new relay sends to an old client: an
orca-relay-handshake-okcarrying two extra numbers and acapabilitiesobject. The old bridge parses it, reads onlytypeandversion, and connects normally. Verified on a real host below: exit 0, and its log line simply lacks theprotocol=field it cannot read.No new opcode (Rule 2 does not apply —
MessageTypeis untouched). Nothing the relay already published changed meaning (Rule 3): the fields are purely additive and every reader treats them as optional.SSH execution boundary
docs/reference/ssh-execution-boundary.mdalready documents this bug by name and points at the daemon model as the fix. Nothing here kills anything or infers a verdict: this change only ever admits a connection that would previously have been refused. A stranded relay holding live PTYs stayslive— the direction is leak, never kill.Proof — real hosts, real bundles
Two relay bundles built from two different commits (
origin/mainat 2bf298d, and this branch), installed into version directories named exactly like production, sockets bound inside them. Not simulated.Linux (
openclaw, Ubuntu, node v26):Wire compat, both skew directions, same host:
Windows (
awin, node v24): the handshake suites run there against real Windows named pipes (relayTestSocketPathreturns\\.\pipe\…on win32) — 25/25 pass, including the cross-build acceptance. Windows matters here because its pipe name hashes the version folder and the superseded sweep is a no-op there, so Windows orphans are not even logged today; this change is transport-agnostic and behaves identically.win-lowspecwas unreachable (runtime_timeout) for the whole session, so Windows evidence is fromawinonly.Tests
pnpm tcclean · fullsrc/relaysuite 1711 passed / 4 skipped ·oxlintclean · changed-code quality gate: 0 new findings · Windows 25/25.Every new assertion was mutation-tested against the specific regression it claims to catch — 12 mutations, 12 killed, each by the assertion that names it. Eleven are on the negotiation itself (below); the twelfth is on the stderr parser fix, described above.
-okcapabilitiesfrom-ok-mismatchminto the floor instead ofmaxNote for anyone re-running this matrix. The last row kills the vitest worker, not an assertion — the mutated code crashes the process. A reporter reading only
assertionResultstherefore sees zero failures and scores it a phantom survivor; my own harness did exactly that on the first pass. Read the run's exit code and run-level errors, not just assertion status, or that row will make the whole table look untrustworthy. It is a genuine kill.An earlier draft had a twelfth mutation (an inverted-range guard) that was inert:
min > maxcan never pass the band check anyway. The redundant branch was deleted rather than left standing behind a vacuous test.Incidental find: a peer can crash a relay and take its terminals with it
Not part of the protocol work — found while reviewing it, and worth reading on its own terms.
parseHandshakeMessagereturns whateverJSON.parseproduced.JSON.parse('{"version":{"toString":1}}')yields an object whosetoStringis not callable, so interpolating it throwsTypeError: Cannot convert object to primitive value. That interpolation happens in the mismatch log line, inside theFrameDecoderframe callback — andrelay-frame-decoder.tswraps the dispatch loop intry/finally, with nocatch. The throw escapesdecoder.feed(), escapes the socketdatahandler, and becomes an unhandled exception: the relay daemon dies, and every PTY and agent session it owns dies with it.Malicious, not malformed. No legitimate sender emits a non-string
version, so this is not something a buggy peer trips into — it has to be constructed. And it is reachable before authentication: the version gate isrelay-handshake.ts:140, the endpoint-credential gate is:161, so a peer that never presents a credential reaches the crash. The reachable set is any process that canconnect()to the relay's endpoint — which on an SSH host includes anything running as that user, such as an agent CLI inside one of the relay's own PTYs. A hostile process in one terminal can therefore kill every sibling terminal on the host.This PR routes its own new fields through
describeRelayProtocolVersion()so the protocol work does not widen the vector, and pins that with a test asserting the daemon answers and keeps serving. The pre-existingmsg.versioncase is left alone deliberately — it predates this change, it is a different bug from #13852, and folding an unrelated pre-auth availability fix into a wire-compatibility PR would make both harder to review. It needs its own change and its own tests. Flagging rather than fixing is the deliberate call, not an oversight.A consumer this broke, and the fix
parseHandshakeMismatchStderr(src/main/ssh/ssh-relay-handshake-mismatch.ts) scrapes the bridge's stderr to name both versions inRelayVersionMismatchError. Itsdaemon=([^\s;]+)capture stopped at the;— which worked only because the version used to be the last field on the line. AddingdaemonProtocol=andours=after it made the capture swallow the separating comma, so the reported daemon version became0.1.0+bbb,.Narrowed to
[^\s;,]+, with tests covering both the new form and the pre-negotiation short form (a relay from before this change still emits the old line). Mutating the character class back reddens only the new-form test, not the old-form one.The four hash gates, verified on current main
ssh-relay-versioned-install.tscomputeRemoteRelayDir→relay-${fullVersion}; socket bound inside it atssh-relay-deploy.ts:1697. This is the gate that actually bites. Unchanged here.relay-handshake.ts. Both sides read.versionfrom beside their ownrelay.js, so today it only fires when that file is unreadable. This is the gate this PR relaxes, and only when the peer offers a compatible range.serverBuildId—ssh-pty-consumer-session.ts:56-65. Unchanged.ssh-relay-session.ts:1212-1222. Unchanged.Cross-version install isolation stays test-enforced (
ssh-relay-cross-version-isolation.test.ts); this PR touches no deploy path, so that suite is untouched and still green.Refs #13852. Also refs #19194, which is a field measurement of the same mechanism rather than a separate bug — five relays across two socket hashes on one host, the oldest 16 days.
This does not close #18173 (partition ownership, #12721, covered by #19572) or #18191 (orphan inventory, owned elsewhere). Neither is version stranding.
Review pass (main merged at e74c22a)
mainin to refresh CI after the runner backlog; the new head'sPR Checksrun completed with 20 success / 12 skipped / 0 failure read from job steps (skips are path-gated lanes this diff does not touch;cross-version wire compatibilityis gated onsrc/shared/protocol-version*and the terminal/agent wire paths, not the relay handshake, so it skipping here is expected rather than a miss).describeRelayProtocolVersionguards against forprotocolVersionis fixed forversion— and for every future field — in fix(relay): one malformed pre-auth handshake frame closes its connection, not the daemon #19879, kept separate so this wire change stays reviewable. The two branches merge cleanly and the combined relay suites pass (60/60).msg.version: it sits after the credential check, so a peer that pairs a malformedversionwith a valid protocol offer must first present the credential to reach it. The pre-auth surface narrows slightly rather than widening; fix(relay): one malformed pre-auth handshake frame closes its connection, not the daemon #19879 validates the field at the parser so neither site can interpolate a non-string at all.Release N+1 needs three conditions, not two
Shipping with only the first two produces a negotiation that still refuses — at a later, worse point, with a message that names the wrong thing.
Now recorded in the source as well, in two commits added to this branch:
ab445d71682docs(relay): say plainly that the protocol negotiation is not reachable yet. Carried from a dead-code/unread-field sweep that was stranded on the un-PR'd branchnwparker/adv2-map-reconciled(94ba0b9f052). It puts the reachability statement inrelay-protocol-version.ts, where someone editing this feature will actually see it rather than only in a PR body.d0aca79806adocs(relay): the negotiation needs three conditions, not two. The carried note listed two. Correcting it is the point of the commit — a reader implementing N+1 from the two-item list ships a broken negotiation and then debugs it at the wrong gate.(The
lingerMshalf of that same sweep is a coordinator concern and went to #20061 instead of riding along here.)1. Route the bridge to the incumbent's socket. Today
computeRemoteRelayDirnamespaces the remote install by a content hash of the bundle, so a bridge only ever meets its own build.relayProtocolOfferAdmitsis called on the live accept path, butmsg.version === launchVersionshort-circuits the offer away before it is consulted — the band is reachable code whose input never varies. Until the deploy path can point a bridge at another version's socket, nothing this PR adds is ever exercised.2. Drop the build-id half of the grant refusal.
validateGrantinsrc/main/ssh/ssh-pty-consumer-session.tsrefuses ongrant.serverBuildId !== options.expectedServerBuildId. A cross-build peer admitted at the handshake still dies here, atpty.openClient.3. The same condition independently refuses on a protocol-version mismatch — and there is nothing to negotiate it with. This is the one that gets missed. The refusal is a single
ifwith two disjuncts:Deleting only the
serverBuildIdclause leaves an exact-equality check onPTY_CONSUMER_SESSION_PROTOCOL_VERSION— a different constant from theRELAY_PROTOCOL_VERSIONthis PR introduces, with no range, no floor, and no fallback. Two peers that just negotiated a compatible relay protocol are still refused if their PTY-session protocol constants differ by one.And there is no channel to resolve it with: the
capabilitiesfield this PR adds toorca-relay-handshake-okis written at exactly one site (relay-handshake.ts:180) and read by nothing outside tests. It is a published field with no consumer — so N+1 cannot learn the peer's PTY-session protocol from the handshake without first giving that field a reader.The error message compounds it: it interpolates only the build ids, so a pure protocol-version mismatch reports as
Remote relay session contract mismatch — expected build X, got Xwith two identical ids. Anyone debugging N+1 will chase the wrong gate.Anyone wiring this up with conditions 1 and 2 ships a broken negotiation.
The reversible removal option — prepared, not recommended
If the band is judged not worth carrying until N+1, the revert is prepared and green on
nwparker/j4-remove-unreachable-relay-protocol-band@068852777c1— "revert(relay): remove the unreachable protocol-version negotiation".It is deliberately not opened as a PR, and should not be merged. The tradeoff was weighed and declined:
minProtocolVersionis absent frommainand from every release tag, so no deployed peer reads it, and Rule 3 does not apply). That is what makes it reversible, not what makes it right.validateGrantthrows atpty.openClient, which has none. That comparison is a maintainer call, and the call made was to keep the band.The commit's own message opens "Prepared as the reversible option, not a recommendation to merge." Treat it as an escape hatch with a known cost, not a cleanup.
Rebase record (2026-09-11)
Rebased onto
mainat5fa62feda7c264c6eda99ba4788ffebab9aae26c. No conflicts; all 7 commit(s) replayed as-is.d0aca79806a546a09c627bb7fa19f77ba9175106bedc94c5ecbece4e594a1ff23defd0d4fbeeb193