chore(runtime-host): retire legacy surface hello shim - #3322
Conversation
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at a4e747d68. Small, well-shaped change, and the fixture design is the right instinct — pinning a historical Host by revision and keeping its decoder independent of the current implementation is how a compatibility test should be written, and it is why the tolerant-Host case is genuinely proven rather than asserted.
The P1 below is about which half of epoch 27 the new test covers. Epoch 27 has two Host flavours because #3277 removed the surface plumbing without bumping the epoch, and the constant your fixture pins — a6f33c9522ee2d4366046b84e5ed442aa1aaafe2 — is #3277's own merge commit. So the covered case is the tolerant Host, which was never the one at risk; the intolerant pre-#3277 Host is the one the shim's comment was written about, and against it the epoch bump does not produce structured incompatibility. I traced that end to end rather than inferring it: the decoder at a6f33c952^ requires surface, the decode runs before the epoch comparison, and the failure path is a bare transport.abort().
That may still be the right call — if no epoch-27 build predating #3277 was ever shipped or can still be running, the concern is empty and the epoch comment should say so explicitly, since that fact is what licenses the removal. That is a release-history question I cannot answer from the repo, which is why the finding asks you to establish it rather than asserting the change is wrong.
Everything else checks out. The Client's ClientHello no longer carries surface, the current protocol and server have no reference to it (#3277 removed both sides), and the raw-wire test asserts absence on the actual frame rather than on the typed object, which is the assertion that would catch a re-introduction through a spread. Pinning the CLI restart-guidance test to a literal 27 instead of EPOCH - 1 is a real improvement: the old form silently followed every future bump and would have stopped testing the boundary it was named for.
One coordination note in a second thread — the epoch number itself is contested by five open PRs and will likely need renumbering, but that is not a defect in this diff.
Carrying a P1 → COMMENT rather than approval. I expect to approve once the pre-#3277 Host question is settled either way.
AI disclosure: reviewed with Claude Code. I verified the load-bearing claims myself at this head — I diffed decodeClientFrame at a6f33c952^ against main, traced the decode-before-admit ordering and the abort path in host-kernel.ts, confirmed no surviving surface reference in the protocol or server, and re-read every open PR's epoch constant at its current head. The conclusions are mine and rest on those checks.
Generated-by: Codex
Generated-by: Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Codex-assisted review performed under the maintainer-approved review workflow.
| > { | ||
| const helloProtocol = input.helloProtocol ?? input.protocol; | ||
| const hello: LegacySurfaceClientHello = { | ||
| const hello: ClientHello = { |
There was a problem hiding this comment.
[P1] Keep the private surface sentinel for currently released Hosts. The release-history argument here only rules out a strict epoch-27 release, but the latest stable release, v0.1.11 (published August 18, 2026), is epoch 25 and its decodeClientFrame still calls requireSurface(frame.surface). With this hello, a normally upgrading Client reaches that released Host without surface; the Host aborts in #serveConnection before #admitHandshake can compare epochs and return structured incompatibility. The current Client then classifies the connection as handshake_failed, and local election eventually reports host_unresponsive, bypassing the restart/wait guidance intended to protect resident work. This is a supported latest-release upgrade path, not the unshipped epoch-27 gap documented by the new fixture. Please retain surface: 'desktop' until the oldest supported tagged Host has a tolerant decoder (or provide an equivalent pre-decode compatibility path), and pin the regression to v0.1.11 rather than only the unreleased epoch-27 revision.
Summary
Fixes #3297
Verification
AI use
Select exactly one:
Tool(s) and scope: Codex analyzed the compatibility boundary, implemented the shim removal and regression coverage, resolved the mainline epoch conflict, and reviewed the final diff.
Checklist
Does this PR entail a change in behavior?