Skip to content

feat(transport,client-core): name a wire version skew at handshake instead of timing out - #529

Open
Zerlight wants to merge 6 commits into
ruocheng/code-640from
ruocheng/code-641
Open

feat(transport,client-core): name a wire version skew at handshake instead of timing out#529
Zerlight wants to merge 6 commits into
ruocheng/code-640from
ruocheng/code-641

Conversation

@Zerlight

@Zerlight Zerlight commented Sep 9, 2026

Copy link
Copy Markdown
Member

Summary

Phase 6 of CODE-627 — Conversation turn graph & immutable attachment store. Linear: https://linear.app/arcbox/issue/CODE-641/feattransport-tunnel-minimum-version-advisory-for-below-floor-clients

Stack: #525this PR (ruocheng/code-641, base ruocheng/code-640) ← top of the stack. Merge bottom-up; this PR's diff is only its own commits.

The precondition for the floor-bump release. The client half of the advisory already existed: since wire v64 (2026-07-31) the pong carries version + minCompatible and LinkCodeClient names both skews from it. What never worked was the transport boundary — every server transport (socket.io, ws, tunnel peer) refused a below-floor frame silently, so an old client's ping was never answered and its handshake died in the 5 s timeout ("daemon unavailable or wire protocol mismatch"), and a newer client dropped an older host's pong the same way, which made the existing "update the host" branch unreachable. One rule fixes both directions on every carrier: parseWireMessage accepts a below-floor frame only if its payload validates as ping or pong; everything else below the floor is still refused. The Hub's existing pong path then answers an old client, and the reply travels back on the same connection — so the tunnel needs no wire-version surface of its own and no relay change (the relay forwards WireMessage frames opaquely; that assumption cannot be verified from this repo). The client turns the skew into a typed WireIncompatibleError { remedy: 'update-host' | 'update-app', peerVersion, peerMinCompatible }, the connection controller stops retrying on it (a skew cannot heal by retrying), mobile renders an update-required screen instead of "Host unavailable", and the workbench's connection state names the side that must update instead of asking to start a daemon that is running. The deprecation-window policy is recorded: the floor may move only after the update screen has shipped in at least two mobile store releases.

Commits

  • feat(schema,transport): accept the handshake below the wire floor so a skew is named instead of timing out
  • feat(client-core): type the wire incompatibility and stop retrying it
  • feat(mobile): show an update-required state for a wire version skew
  • docs: record the version-agnostic handshake and the floor-bump deprecation window
  • fix(workbench): name a wire skew in the connection state instead of asking to start the daemon

Verification

Every commit passed pnpm check:ci and pnpm test at its own tip; the tip (1b280635) is at pnpm check:ci 0 errors, pnpm test 3482 passed / 1 skipped. Tests: the schema contract (a below-floor ping/pong is accepted, a bare {kind:'pong'} claim and every other kind below the floor are refused; the new acceptance test fails on the previous parser), a raw socket.io client and a raw ws client stamping v−1 sending ping through a Hub and receiving pong {version, minCompatible}, a below-floor peer ping delivered through the tunnel peer transport, an older host's below-floor pong reaching LinkCodeClient through the real WsTransport as update-host, both remedies from the handshake, and the controller stopping after one attempt on the typed error (and again after a deliberate retry) while still exhausting its budget on an ordinary failure. Adversarial review (isolated read-only worktree): ACCEPT-WITH-FINDINGS, no P1/P2 — it could not make a below-floor peer do anything beyond having its handshake answered; its findings (a wrong docs anchor, the workbench copy, comment and API tidy-ups, the mobile redial flicker, the ws-carrier test gap) are folded into these commits. Observed running: against the development daemon (tsx watch, hot-reloaded), a raw wire-v70 client's ping got pong {version: 82, minCompatible: 76} back within milliseconds; the webview, pointed at a stand-in daemon answering pings with an out-of-range pong, rendered "This LinkCode build is too old to talk to the daemon at … Update the app." and, with the range inverted, "The daemon at … is too old for this LinkCode build. Update the daemon." Not rendered on a device: the mobile update screen against a skewed daemon (the hook logic is unit-covered; the SwiftUI view is a copy branch).

Checklist

  • pnpm check:ci and pnpm test both pass (no Rust changes)
  • I ran the affected surface and observed the change working — the development daemon answering a below-floor ping, and the webview against a skewed stand-in daemon in both directions
  • Wire: unchanged at 82 — no frame shape changed, only the receiver's acceptance of the handshake; floor unchanged at 76; no migration
  • New code and assets are my own work
  • Docs and comments are updated where behavior changed (docs/ARCHITECTURE.md, root AGENTS.md Invariant 1, docs/RELEASE.md "Moving the compatibility floor")

@linear-code

linear-code Bot commented Sep 9, 2026

Copy link
Copy Markdown

CODE-641

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Important

The mechanism is sound end-to-end — but the release gate this PR exists to enable rests on a desktop "lockstep" claim that the daemon-discovery code refutes. Worth fixing now, while the reasoning is fresh, because the next person counts release windows from that line.

Reviewed changes

  • Version-agnostic handshake. parseWireMessage now accepts a below-floor envelope when its payload kind is ping/pong and the payload validates, so a peer under the floor learns the range it must update into instead of dying in the 5 s timeout.
  • Typed incompatibility. New WireIncompatibleError carries remedy: 'update-host' | 'update-app'; ConnectionController bail()s out of the retry loop on that type rather than burning its budget.
  • Three update-required surfaces. Mobile's connection state, plus workbench's shared state for desktop and webview via the new useWorkbenchRuntimeError hook.
  • Recorded floor-bump policy. A two-mobile-release advisory window in docs/RELEASE.md, docs/ARCHITECTURE.md, and root AGENTS.md.

I traced the things most likely to be quietly wrong here and they hold up, so I'll record them so nobody re-does the work:

  • No below-floor frame escapes onto a send path. hub.ts:125-127 consumes the inbound ping and replies with a freshly createWireMessaged pong, returning before inbound.emit. The ws/socket.io servers and the tunnel only inbound.emit the parsed frame, and there is no cross-peer relay in TunnelTransportServer. So the as ValidatedWireMessage cast at the new branch — which does mint an object violating WireMessageSchema's .min(MIN_COMPATIBLE_WIRE_VERSION) — stays inert.
  • The typed error survives every boundary. foxts' bail() wraps in AsyncRetryAbortError and the handler rethrows .cause verbatim, skipping onFailedAttempt; unwrapConnectionError only unwraps ExplicitRetryError. One bundle per app, so instanceof holds across the package edge.
  • No stale-error false positive. startRecovery publishes error: undefined before flipping status to connecting, so workbench computing message unconditionally can't leak update copy into a healthy reconnect.
  • The mobile prop plumbing works despite host-connection.ts not being in the diffHostConnection is an intersection with HostClientState, and host-connection-scope.tsx:40 spreads ...state.

ℹ️ Invariant 1's "every wire change" is left unadjudicated for receiver-only changes

Holding WIRE_PROTOCOL_VERSION at 82 is defensible on the stated reasoning — no frame shape, field, or meaning changed, and no capability gate anywhere interrogates the version to decide whether a handshake will be answered. I checked; there's no probe that silently misbehaves.

The part worth a sentence somewhere is that two builds both stamping 82 now behave observably differently: a pre-PR v82 daemon leaves a below-floor client in the 5 s timeout, a post-PR v82 daemon hands it a named advisory, and nothing on the wire distinguishes them. That's not a correctness bug — the pre-PR behavior is exactly the status quo — but during a mixed-fleet floor bump it's the difference between "this daemon is broken" and "this daemon is old", and an operator can't tell which they have.

Invariant 1 says the stamp moves on every wire change. This PR is the first case where "wire change" plausibly means "receiver acceptance policy" rather than "frame shape", and the answer chosen here becomes precedent by default. Recording the call — either way — in Invariant 1 itself would keep the next person from re-deriving it. This is a judgment call for you, not something I'd assert an answer to.

ℹ️ Nitpicks

  • VERSION_AGNOSTIC_KINDS is a hand-written Set(['ping', 'pong']) rather than being derived from the keep-alive schema. Renaming either kind would silently disable the entire advisory with no type error — and the failure mode is precisely the timeout this PR removes, so it'd read as a regression in the feature rather than a rename fallout. Low likelihood (renaming those kinds is itself a breaking wire change), but the blast radius is the whole feature.
  • docs/ARCHITECTURE.md:317 still reads that an unchecked object cannot reach a send path, while the sibling comment in message.ts gained the below-floor caveat. The claim is still true for the reasons traced above; it's just now true for a narrower reason than the sentence implies.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread docs/RELEASE.md Outdated
Comment thread apps/mobile/src/components/host/host-connection-state.tsx
@Zerlight
Zerlight added this pull request to stack #518 September 9, 2026 16:54
Copilot AI lite review requested due to automatic review settings September 11, 2026 04:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The cross-layer transport, client, desktop, and mobile changes warrant final human review.

Pull request overview

This PR makes wire-version skew explicit during handshake instead of allowing incompatible peers to time out.

Changes:

  • Accepts validated below-floor ping/pong frames across transports.
  • Adds typed incompatibility errors and stops retries.
  • Adds desktop/mobile update guidance, tests, localization, and documentation.
File summaries
File Description
packages/presentation/i18n/src/locales/zh-cn.ts Adds Chinese skew messages.
packages/presentation/i18n/src/locales/en.ts Adds English skew messages.
packages/foundation/transport/tests/integration/ws-server.test.ts Adds WebSocket handshake coverage.
packages/foundation/transport/tests/integration/socket-io.test.ts Adds Socket.IO handshake coverage.
packages/foundation/transport/src/__tests__/tunnel.test.ts Adds tunnel handshake coverage.
packages/foundation/schema/tests/contract/wire/envelope.test.ts Tests handshake compatibility rules.
packages/foundation/schema/src/wire/message.ts Allows validated below-floor handshake frames.
packages/client/workbench/src/runtime/provider.tsx Exposes connection errors.
packages/client/workbench/src/app/connection-state.tsx Displays skew guidance.
packages/client/core/tests/integration/wire-skew.test.ts Adds end-to-end skew coverage.
packages/client/core/src/wire-incompatible-error.ts Adds the typed incompatibility error.
packages/client/core/src/index.ts Exports the new error.
packages/client/core/src/connection-controller.ts Stops retries for wire skew.
packages/client/core/src/client.ts Detects and preserves typed errors.
packages/client/core/src/__tests__/connection.test.ts Tests incompatibility directions.
packages/client/core/src/__tests__/connection-controller.test.ts Tests retry behavior.
docs/RELEASE.md Documents the compatibility-floor policy.
docs/ARCHITECTURE.md Documents version-agnostic handshakes.
apps/mobile/src/runtime/use-host-client.ts Propagates skew state.
apps/mobile/src/components/host/host-connection-state.tsx Renders update-required states.
apps/mobile/src/components/host/host-client-gate.tsx Passes remedy state to the UI.
AGENTS.md Updates the wire-version invariant.
Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings September 11, 2026 04:14
@Zerlight

Copy link
Copy Markdown
Member Author

Review round (2026-09-11). Both pullfrog findings were real and are fixed; the branch is rebased onto the updated ruocheng/code-640.

  • The desktop exemption was wrong. resolveDaemonUrl() prefers the Developer-tab override, then any live runtime.json advertisement, and the supervisor stands down when another install's daemon already serves the machine — so a pre-advisory desktop build does meet a newer daemon. "Lockstep" described the bundle, not the connection. docs/RELEASE.md now carries its own bullet (desktop is not exempt, only unbounded: no store-review count bounds it, so the trigger is a judgement call), and docs/ARCHITECTURE.md names the desktop release alongside the two mobile ones. Root AGENTS.md needed no edit — Invariant 1 already gates on "the clients it will refuse". The webview loses its exemption on the same ground.
  • The mobile update screen contradicted itself. An update-app skew now renders no Retry button, and neither skew renders the technical failure footnote. update-host keeps Retry, matching hurryAlong, which still redials that one. The retry loop the button offered is pinned by this PR's own connection-controller.test.ts.

Left for the owner, flagged rather than invented: what measurably bounds the desktop window (auto-update adoption has no dwell or adoption trigger the way "two store releases" does), and the N=2 mobile count itself, which is still a proposal.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The English and Chinese update messages direct Android users to the App Store instead of providing platform-appropriate guidance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 22/22 changed files
  • Comments generated: 2
  • Review effort level: Lite

error: 'Unable to reach the host at {url}.',
updateAppTitle: 'Update LinkCode',
updateAppBody:
'This version of the app is too old to talk to that host. Update it from the App Store and try again.',
unavailableTitle: '无法连接 host',
error: '无法连接 host({url})。',
updateAppTitle: '请更新 LinkCode',
updateAppBody: '此版本的应用过旧,无法连接到该 host。请从 App Store 更新后重试。',

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ℹ️ No critical issues — minor suggestions inline.

Both findings from the previous review are genuinely fixed, not relocated. One consequence of the mobile fix is worth a look before merge.

Reviewed changes

  • Corrected the desktop exemption in docs/RELEASE.md and docs/ARCHITECTURE.md. The new "Desktop is not exempt, only unbounded" bullet names the actual non-bundled paths — resolveDaemonUrl(), the runtime.json advertisement, the Developer-tab override, the supervisor stand-down — and folds the webview into the same rule. Root AGENTS.md Invariant 1 was correctly left alone: it already defers to docs/RELEASE.md, so the three-site partial-fix trap I warned about is avoided rather than half-hit.
  • Dropped the Retry button on the mobile update-app screen and suppressed the raw failure footnote under either named skew, so the friendly copy isn't shadowed by triage voice.

ℹ️ Nitpicks

  • packages/client/workbench/src/app/connection-state.tsx:46 still renders Retry unconditionally, including on update-app. I scoped this out last round on the grounds that the workbench surfaces don't share mobile's flip-flop tension, and that reasoning still holds — flagging only because the two platforms now diverge visibly on the same state. No change needed if the divergence is intentional.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

{/* An app below the host's floor has nothing to retry: redialing only flashes
"connecting" and lands back here. Updating the host is a real action, so that
skew keeps the button. */}
{wireRemedy === 'update-app' ? null : (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing the button is right for the flip-flop problem, but it leaves update-app with no in-session recovery path at all. hurryAlong already declines on this remedy (use-host-client.ts:57), so with the button gone nothing can re-dial: connectionFor(host) hands back the same wedged controller for the same host.id, and pruneConnections never disposes the selected host. A skew resolved host-side — someone rolls their daemon back to a compatible build — is then invisible until the app is force-quit.

That's mostly fine, because the intended remedy (an App Store update) restarts the process anyway. The mismatch is the copy: updateAppBody still ends "and try again" (en.ts:1434, zh-cn.ts:1389) on a screen that now offers nothing to try.

Technical details

The three mechanisms that together close every escape:

// apps/mobile/src/runtime/use-host-client.ts:55-58
const { status, error } = controller.getSnapshot();
if (status === 'ready') return;
if (error instanceof WireIncompatibleError && error.remedy === 'update-app') return;
controller.retry();
// apps/mobile/src/runtime/host-connection-pool.ts:20-28 — same id, same controller
// apps/mobile/src/runtime/host-connection-pool.ts:31-37 — prune only disposes what's not in `keep`
// apps/mobile/src/components/shell/host-connection-scope.tsx:16-25
const keep = new Set(
  keepHostsConnected ? hosts.map((entry) => entry.id) : host ? [host.id] : [],
);

keep always contains the selected host, so the wedged controller survives every re-render. The only escapes are force-quit, removing and re-adding the host (which mints a new host.id), or toggling keepHostsConnected for some other host.

Two ways out, either is fine:

  1. Drop "and try again" from updateAppBody in both locales — accepts the wedge and makes the copy honest.
  2. Let the foreground trigger redial once on update-app. The flip-flop the guard avoids is a repeated tap; a single re-dial on app foreground costs one "connecting" flash and catches the host-side rollback.

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