refactor(protocol): one subscription handshake instead of four (audit theme 1)

[passcod]

Closes cross-cutting theme 1 from the logic bug audit: error responses on bidi streams are discarded by clients. See the pattern analysis.``

The class

Subscription-style requests (/events/subscribe, /logs/stream) answer on the bidi stream before opening the data stream, and answer with an error routinely — server_busy from the stream semaphore, requirements_invalid/not_found from /logs/stream validation, server_busy from a journal-open failure. In every error branch the server opens no uni stream and keeps the connection alive.

Four call sites re-implemented the client half of that handshake, and each got a different part wrong. The bug is not subtle logic — it is per-caller re-implementation of a wire contract.

The change

OiClient::open_subscription (crates/protocol/src/client.rs) is now the only way to drive one:

• reads the response envelope with read_to_end, matching the FIN-is-the-boundary framing i[stream.control] actually specifies — never read_line, which only ever returned because the FIN produced EOF;
• classifies every outcome through the same parse_response as request(): error envelope → ClientError::Api, zero bytes → ClientError::Protocol, unparseable → Protocol;
• reaches accept_uni only after a confirmed OK, under a 10 s timeout, because the server's own open_uni failure path only logs and returns.

open_subscription_raw takes a pre-serialised envelope for the one caller that must preserve an actor built elsewhere (the web gateway relays the browser session's actor, not its own).

Findings closed

Finding	Severity	How
H16 — subscribe_events swallows error responses, then blocks forever on a uni stream that will never arrive	high	envelope classified; accept_uni unreachable without an OK
H17 — /logs/stream hangs forever when the daemon returns an error	high	start_log_stream on the shared helper
ctl events exits 0 when the server rejects the subscription	low	new SessionOutcome::Rejected → exit 1

Transitively, the web event broker's reconnect back-off (event_broker.rs:81-116) stops being dead weight: it is built entirely on subscribe_events returning promptly.

Two behaviour changes worth naming: the web gateway now relays the daemon's own code and message to the browser instead of flattening every failure to daemon_unavailable, so the UI can tell "no such app" from "daemon down"; and ctl events exits non-zero on a rejection, which a script wrapping it will now notice.

Enforcement

• Spec: new i[stream.subscribe] in docs/spec/interface.md, stating the wire contract (an error response terminates the request; clients must classify before waiting, must treat a response-less close as an error, and must not wait indefinitely) rather than the helper's shape.
• Tests: a quinn loopback stub server in the protocol crate covering all four outcomes — error envelope, empty FIN, OK-but-no-uni, OK-plus-uni. TestOi drives dispatch directly and never touches handle_bidi_stream, so it could not reach this handshake. Each assertion is wrapped in a timeout so a regression fails fast instead of hanging CI.
• CI: etc/ci/check-accept-uni.sh fails the build on accept_uni outside an allowlist (the helper, plus the shell paths whose framing genuinely differs), catching the fifth hand-rolled copy before review does.

OiClient::connect grew a private connect_from taking the local bind address, so the stub-server tests run on hosts without IPv6; production still binds the dual-stack wildcard.

Not in scope

/shells/start uses genuinely different framing (newline-delimited JSON on a bidi that stays open, uni stream IDs announced in the handshake, i[stream.shell.framing]) and keeps its own handling. Data-phase defects on the uni stream itself — the web broker's lag/duplication findings in §17 — are separate.

Overlap with other themes

Independent of the other seven, so it sits directly on main. Theme 7's event-broker item is downstream of H16 but touches different files.

---

Generated by Claude Code

[Copilot]

Pull request overview

This PR refactors the client-side handling of subscription-style OI requests (/events/subscribe, /logs/stream) so the handshake is implemented once (in OiClient::open_subscription{,_raw}) and consistently classifies server error envelopes before waiting for the server-initiated uni stream, preventing clients from discarding errors and hanging indefinitely.

Changes:

• Add OiClient::open_subscription / open_subscription_raw with shared parse_response, FIN-bounded response reading, and a bounded wait for the data uni stream.
• Update ctl and web call sites to use the shared helper and surface daemon rejection details appropriately (including ctl events exiting non-zero on rejection).
• Add protocol-level loopback tests for all handshake outcomes and a CI guard script to prevent new hand-rolled accept_uni subscription handshakes.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
etc/ci/check-accept-uni.sh	CI enforcement to prevent new hand-rolled subscription handshakes using accept_uni outside an allowlist.
docs/spec/interface.md	Adds i[stream.subscribe] specifying the subscription handshake contract and required client classification/timeout behaviour.
crates/web/src/wt.rs	Relays daemon ClientError::Api code/message to the browser instead of flattening to daemon_unavailable.
crates/web/src/daemon.rs	Replaces hand-rolled /logs/stream handshake with OiClient::open_subscription_raw to preserve the browser actor.
crates/protocol/src/client.rs	Implements the shared subscription handshake helper, factors response parsing, adds timeout + tests, and adds connect_from for IPv4-bind testability.
crates/ctl/src/subscribe.rs	Switches event subscription to the shared helper and treats daemon rejection as a non-zero exit.
crates/ctl/src/logs.rs	Switches log streaming to the shared helper, ensuring error envelopes don’t lead to a hang.
.github/workflows/rust.yml	Runs the new check-accept-uni.sh in CI to prevent regressions.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-code-quality]

Code Coverage Overview

Languages: TypeScript, Rust

TypeScript / code-coverage/vitest

The overall coverage in commit ab724d7 in the claude/pr-115-theme-... branch remains at 66%, unchanged from commit b383126 in the main branch.

Rust / code-coverage/rust

The overall coverage in commit ab724d7 in the claude/pr-115-theme-... branch remains at 59%, unchanged from commit b383126 in the main branch.

Show a code coverage summary of the most impacted files.

File	main b383126	claude/pr-115-theme-... ab724d7	+/-
crates/core/src/oi/server.rs	60%	60%	0%
crates/protocol/src/client.rs	37%	59%	+22%

---

_{Updated August 02, 2026 03:12 UTC}

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (2)

etc/ci/check-accept-uni.sh:24

• With set -euo pipefail, this will fail the CI job if grep finds no matches (exit code 1), even though “no accept_uni usages” should be a clean pass. It’s better to treat the “no matches” case as an empty hit list so the script stays well-behaved if accept_uni is ever removed/renamed or temporarily absent in a refactor.

pattern='accept_uni'
mapfile -t hits < <(grep -rln --include='*.rs' "$pattern" crates/ | sort)

docs/spec/interface.md:107

• The spec currently uses both “stream boundary is the message boundary” (i[stream.control]) and “streams begin with a newline-terminated JSON object” (i[stream.dispatch]); in this new subscription contract, it would help to explicitly restate that the response envelope is delimited by FIN/stream close (not by a newline) to avoid ambiguity for anyone implementing a client from the spec alone.

> i[stream.subscribe]
> A subscription-style request — one whose success causes the server to open a server-initiated unidirectional stream carrying the subscribed data — is answered on the bidirectional stream exactly as any other control request, before any unidirectional stream is opened.
> A response carrying an error terminates the request: the server opens no unidirectional stream, and the connection remains usable for further requests.
> Clients must therefore read and classify the response envelope before waiting for the unidirectional stream, must treat closure of the bidirectional stream without a response envelope as an error, must surface the error envelope's `code` and `message` to their caller rather than discarding them, and must not wait indefinitely for a unidirectional stream that a server which answered successfully may still fail to open.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

crates/web/src/wt.rs:263

• This file already imports serde_json::json and uses json!(...) in many places. These new arms use the fully-qualified serde_json::json!(...), which is inconsistent and unnecessary.

                            ClientError::Api { code, message } => serde_json::json!({
                                "error": { "code": code, "message": message }
                            }),
                            _ => serde_json::json!({
                                "error": { "code": "daemon_unavailable", "message": e.to_string() }

.github/workflows/rust.yml:41

• This step relies on etc/ci/check-accept-uni.sh being checked in with the executable bit set. Invoking it via bash is more robust (and still respects set -euo pipefail inside the script).

      - name: No hand-rolled subscription handshakes
        run: etc/ci/check-accept-uni.sh

crates/web/src/wt.rs:253

• ClientError::Api represents a daemon-side rejection (e.g. not_found / requirements_invalid), which is an expected outcome and will be surfaced to the browser. Logging it at error! will likely create noisy gateway error logs for normal user mistakes; consider logging API rejections at info!/warn! and reserving error! for transport/protocol failures.

                    Err(e) => {
                        tracing::error!("log stream setup failed: {e}");