natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P0] Reject preface acceptance for an unoffered major #45

New issue
Closed
opened 2026-09-03 16:43:17 +09:00 by natsukium · 1 comment
natsukium commented 2026-09-03 16:43:17 +09:00
Owner
Copy link

Problem

At review snapshot 5077d74b, the client offers exactly ClientPreface::CURRENT, but handshake_over matches DaemonPreface::Accept { minor, .. } and discards the returned major. A peer can therefore accept major 9 after the client offered major 1, and the client proceeds to decode frames with the major-1 schema.

The frozen decoder is correct to interpret the trailing words from the status byte alone. The negotiation layer still has to verify that an Accept selected a major the client offered. Without that check, an implementation error turns schema skew into a protobuf failure after negotiation instead of the required loud pre-frame refusal.

Relevant code and contract:

  • crates/felis-client-core/src/connector.rs::handshake_over
  • crates/felis-protocol/src/preface.rs::DaemonPreface
  • crates/felis-daemon/src/serve.rs::exchange_bootstrap
  • docs/reference/ipc.md "Version preface"
  • REQ-104 / REQ-104a

Required decision

Keep the frozen preface layout, but make acceptance semantically exact: a client that offers one major accepts only that same major. Also make the daemon's selected schema and advertised minor one decision, so a future side-by-side major decoder cannot accidentally pair an older schema with the current major's minor.

Acceptance criteria

  • handshake_over rejects Accept.major != ClientPreface.major before constructing a framed Connection.
  • The error is typed and distinct from a daemon Refuse; human and machine callers can report that the daemon sent an invalid acceptance.
  • Daemon acceptance selects (major schema, daemon minor) together rather than calling an API that accepts an arbitrary major while always advertising the current global minor.
  • Duplex tests cover wrong accepted major, the offered major with older/equal/newer daemon minors, Refuse, and unknown status.
  • The normative preface text states the echo/selection invariant and the corruption behavior.
  • #30 updates the protocol-2.0 vectors and tests only after this invariant lands.

Why this is separate

#19 protects protobuf compatibility in CI, and #30 resets the public schema baseline. Neither validates the preface negotiation result at runtime. #42 covers SSH timeout/spawn policy; this bug exists on every carrier.

## Problem At review snapshot `5077d74b`, the client offers exactly `ClientPreface::CURRENT`, but `handshake_over` matches `DaemonPreface::Accept { minor, .. }` and discards the returned `major`. A peer can therefore accept major 9 after the client offered major 1, and the client proceeds to decode frames with the major-1 schema. The frozen decoder is correct to interpret the trailing words from the status byte alone. The negotiation layer still has to verify that an `Accept` selected a major the client offered. Without that check, an implementation error turns schema skew into a protobuf failure after negotiation instead of the required loud pre-frame refusal. Relevant code and contract: - `crates/felis-client-core/src/connector.rs::handshake_over` - `crates/felis-protocol/src/preface.rs::DaemonPreface` - `crates/felis-daemon/src/serve.rs::exchange_bootstrap` - `docs/reference/ipc.md` "Version preface" - REQ-104 / REQ-104a ## Required decision Keep the frozen preface layout, but make acceptance semantically exact: a client that offers one major accepts only that same major. Also make the daemon's selected schema and advertised minor one decision, so a future side-by-side major decoder cannot accidentally pair an older schema with the current major's minor. ## Acceptance criteria - `handshake_over` rejects `Accept.major != ClientPreface.major` before constructing a framed `Connection`. - The error is typed and distinct from a daemon `Refuse`; human and machine callers can report that the daemon sent an invalid acceptance. - Daemon acceptance selects `(major schema, daemon minor)` together rather than calling an API that accepts an arbitrary major while always advertising the current global minor. - Duplex tests cover wrong accepted major, the offered major with older/equal/newer daemon minors, `Refuse`, and unknown status. - The normative preface text states the echo/selection invariant and the corruption behavior. - #30 updates the protocol-2.0 vectors and tests only after this invariant lands. ## Why this is separate #19 protects protobuf compatibility in CI, and #30 resets the public schema baseline. Neither validates the preface negotiation result at runtime. #42 covers SSH timeout/spawn policy; this bug exists on every carrier.
natsukium commented 2026-09-03 20:35:18 +09:00
Author
Owner
Copy link

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12.

Claim check

Accurate against HEAD (69076d42). Nothing in the three post-snapshot commits touched the preface.

  • The client offers exactly ClientPreface::CURRENT (crates/felis-client-core/src/connector.rs:280) and then matches DaemonPreface::Accept { minor, .. } (connector.rs:282-284), discarding major. Only Refuse (:285-294) and Unknown (:295-301) are errors. A status-0 reply carrying major 9 therefore proceeds to FrameReader::new and the Hello (:304-321) under the major-1 schema.
  • The decoder is deliberately status-only: DaemonPreface::decode (crates/felis-protocol/src/preface.rs:218-239) and the test the_status_word_alone_discriminates_the_reply (preface.rs:539-554) pin that [0,0, 0,9, 0,9] decodes as Accept { major: 9, minor: 9 }. That layer is correct and must not change; the missing check is one layer up, exactly as the issue says.
  • Daemon side: exchange_bootstrap (crates/felis-daemon/src/serve.rs:1718-1753) calls DaemonPreface::accept(major) (:1748), and DaemonPreface::accept (preface.rs:181-186) builds Accept { major: client_major, minor: PROTOCOL_MINOR } for any client_major it is handed. supports_major (:1728) is checked first, so today the pairing is right by construction, but the API accepts an arbitrary major while always advertising the global minor, which is the "one decision" gap the issue names. effective_minor is computed separately at serve.rs:1750.
  • Test coverage today: felis-transport/src/preface.rs:115-134 (accept round trip), serve/tests.rs:401 (daemon refuses an unsupported major), connector.rs:2030-2071 (unknown status), connector.rs:2077 (effective minor). No test sends a status-0 reply with a foreign major to the client.
  • Normative text: docs/reference/ipc.md:186-194 says "the status word alone discriminates the reply" and "accept: agreed major, then the daemon's own minor" but never states that the agreed major must equal the offered one; docs/reference/spec.md:54-55 (REQ-104/104a) likewise. docs/explanation/architecture/ipc.md:485-495 argues for the self-describing reply; it does not distinguish "self-describing decode" from "exact negotiation".

Nothing here was already fixed. The issue's file/function references all resolve.

Verdict

accept (as written). This is a correctness hole in a frozen layer, cheap to close, and it has to be closed before #30 regenerates the preface vectors. No principle question arises: it is IPC hygiene inside a surface principle 1 explicitly reserves as the extension surface.

Approach

Protocol crate (felis-protocol, no tokio):

  1. preface.rs: replace DaemonPreface::accept(client_major) with a selection that is one decision. Add 
    /// The schema this build will speak for a client major, or `None`.
pub const fn select(client: ClientPreface) -> Option<DaemonPreface>  // Accept { major: client.major, minor: PROTOCOL_MINOR } iff supports_major
    and keep supports_major as the predicate select uses (or derive it). A future side-by-side major decoder extends select with a (major, minor_for_that_major) table; the minor is never taken from a global independent of the major. Delete accept or make it pub(crate) for tests only. Keep the doc on Accept (preface.rs:153-159) and add the negotiation invariant to the module doc (:29-33): "an accept names the offered major; a client that offered one major closes on an accept naming another."
  2. Add pub fn confirm_accept(offered: ClientPreface, reply: DaemonPreface) -> Result<u16 /* effective minor */, NegotiationError> next to effective_minor, so the transport-free crate owns the rule and the connector just calls it. NegotiationError { AcceptedUnofferedMajor { offered, accepted }, Refused { min, max }, Unknown { status, words } }.

Client (felis-client-core/src/connector.rs):

  1. handshake_over (:281-302): match Accept { major, minor }; on major != preface::PROTOCOL_MAJOR return a new ConnectError::AcceptedUnofferedMajor { offered: u16, accepted: u16 } (:29-60), is_transient() == false (a re-dial reproduces it). Distinct from MajorMismatch (that is the daemon's honest refusal) and from UnknownPrefaceStatus. Message: "daemon accepted protocol major {accepted} but this client offered {offered}; the daemon is not speaking felis's negotiation, rebuild both halves". Do this before FrameReader::new (:304).
  2. The bridge and CLI surface it through the existing error class (skills/felis/SKILL.md:54 "protocol error / a daemon that does not serve this client's protocol major" already covers a preface refusal; keep exit class 2, extend the wording only if the CLI reference names the individual errors).

Daemon (felis-daemon/src/serve.rs):

  1. exchange_bootstrap (:1726-1752): call preface::select(opened.preface); None → the existing refuse path; Some(reply) → write it and compute the effective minor from reply's minor, not from PROTOCOL_MINOR directly, so the two cannot drift.

Tests:

  • felis-protocol unit: select table (offered major, expected reply); confirm_accept on accept-same-major, accept-other-major, refuse, unknown.
  • felis-transport/src/preface.rs duplex tests: add wrong-accepted-major (daemon writes Accept { major: 9, minor: 5 }, client-side confirm_accept errors before any frame), and the offered major with daemon minor older/equal/newer asserting the effective minor.
  • connector.rs tests (beside :2030-2071): a fake listener writing Accept { major: PROTOCOL_MAJOR + 1, .. } and then a Welcome; assert AcceptedUnofferedMajor, non-transient, and that no Hello was written (read the server side and assert zero bytes).
  • Keep the_status_word_alone_discriminates_the_reply untouched: it pins the decoder, which stays lenient on purpose.

Docs cascade (doc-cascade skill):

  • docs/reference/ipc.md:186-206 "Version preface": one paragraph after the status table: an accept's first word MUST equal the offered major; the client verifies it before reading any frame and closes on mismatch with no frame written; the decoder itself does not compare (unchanged).
  • docs/reference/spec.md:54 REQ-104: append "The client accepts only an accept naming the major it offered; any other accept closes the connection before a frame is written." Consider a REQ-104d rather than widening 104 if the sourcing table prefers one requirement per row.
  • docs/explanation/architecture/ipc.md:485-495 "Handshake bootstrap": add the why: a self-describing reply is a decoding property; negotiation is still exact, and a lenient negotiator would turn schema skew into a protobuf failure after the preface, which is the loud-early failure the frozen layer exists to give.
  • CHANGELOG.md Unreleased / Changed: "Wire: a client closes on a preface accept that names a major it did not offer" (user-affecting only in the sense of a new error string; one line).
  • crates/felis-protocol/proto/felis.proto:24-33 header comment: one clause on the echo invariant.
  • #30 regenerates the golden preface bytes (preface.rs:488-490, :512-514) afterwards; this issue does not touch them.

Dependencies

None to land. Sequencing: land before #30 (#30's "development-major peer is refused at the frozen preface" is the daemon half of this invariant; #45 is the client half, and #30's vectors should be regenerated once). #52's order ("resolve #45 before regenerating final preface vectors") still holds. Independent of #19 and #42.

Risk/effort

S (half a day). Main risk: choosing the error surface so the retry loop treats it as permanent (is_transient) and so the bridge's ErrorKind mapping is not silently "protocol" with a misleading message; test both.

Labels

Keep priority/P0, release/v0.1.0. It is a frozen-layer correctness bug and blocks the final preface vectors.

## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. ## Claim check Accurate against HEAD (69076d42). Nothing in the three post-snapshot commits touched the preface. - The client offers exactly `ClientPreface::CURRENT` (`crates/felis-client-core/src/connector.rs:280`) and then matches `DaemonPreface::Accept { minor, .. }` (`connector.rs:282-284`), discarding `major`. Only `Refuse` (`:285-294`) and `Unknown` (`:295-301`) are errors. A status-0 reply carrying major 9 therefore proceeds to `FrameReader::new` and the `Hello` (`:304-321`) under the major-1 schema. - The decoder is deliberately status-only: `DaemonPreface::decode` (`crates/felis-protocol/src/preface.rs:218-239`) and the test `the_status_word_alone_discriminates_the_reply` (`preface.rs:539-554`) pin that `[0,0, 0,9, 0,9]` decodes as `Accept { major: 9, minor: 9 }`. That layer is correct and must not change; the missing check is one layer up, exactly as the issue says. - Daemon side: `exchange_bootstrap` (`crates/felis-daemon/src/serve.rs:1718-1753`) calls `DaemonPreface::accept(major)` (`:1748`), and `DaemonPreface::accept` (`preface.rs:181-186`) builds `Accept { major: client_major, minor: PROTOCOL_MINOR }` for any `client_major` it is handed. `supports_major` (`:1728`) is checked first, so today the pairing is right by construction, but the API accepts an arbitrary major while always advertising the global minor, which is the "one decision" gap the issue names. `effective_minor` is computed separately at `serve.rs:1750`. - Test coverage today: `felis-transport/src/preface.rs:115-134` (accept round trip), `serve/tests.rs:401` (daemon refuses an unsupported major), `connector.rs:2030-2071` (unknown status), `connector.rs:2077` (effective minor). No test sends a status-0 reply with a foreign major to the client. - Normative text: `docs/reference/ipc.md:186-194` says "the status word alone discriminates the reply" and "accept: agreed major, then the daemon's own minor" but never states that the agreed major must equal the offered one; `docs/reference/spec.md:54-55` (REQ-104/104a) likewise. `docs/explanation/architecture/ipc.md:485-495` argues for the self-describing reply; it does not distinguish "self-describing decode" from "exact negotiation". Nothing here was already fixed. The issue's file/function references all resolve. ## Verdict **accept** (as written). This is a correctness hole in a frozen layer, cheap to close, and it has to be closed before #30 regenerates the preface vectors. No principle question arises: it is IPC hygiene inside a surface principle 1 explicitly reserves as the extension surface. ## Approach Protocol crate (`felis-protocol`, no tokio): 1. `preface.rs`: replace `DaemonPreface::accept(client_major)` with a selection that is one decision. Add ```rust /// The schema this build will speak for a client major, or `None`. pub const fn select(client: ClientPreface) -> Option<DaemonPreface> // Accept { major: client.major, minor: PROTOCOL_MINOR } iff supports_major ``` and keep `supports_major` as the predicate `select` uses (or derive it). A future side-by-side major decoder extends `select` with a `(major, minor_for_that_major)` table; the minor is never taken from a global independent of the major. Delete `accept` or make it `pub(crate)` for tests only. Keep the doc on `Accept` (`preface.rs:153-159`) and add the negotiation invariant to the module doc (`:29-33`): "an accept names the offered major; a client that offered one major closes on an accept naming another." 2. Add `pub fn confirm_accept(offered: ClientPreface, reply: DaemonPreface) -> Result<u16 /* effective minor */, NegotiationError>` next to `effective_minor`, so the transport-free crate owns the rule and the connector just calls it. `NegotiationError { AcceptedUnofferedMajor { offered, accepted }, Refused { min, max }, Unknown { status, words } }`. Client (`felis-client-core/src/connector.rs`): 3. `handshake_over` (`:281-302`): match `Accept { major, minor }`; on `major != preface::PROTOCOL_MAJOR` return a new `ConnectError::AcceptedUnofferedMajor { offered: u16, accepted: u16 }` (`:29-60`), `is_transient() == false` (a re-dial reproduces it). Distinct from `MajorMismatch` (that is the daemon's honest refusal) and from `UnknownPrefaceStatus`. Message: "daemon accepted protocol major {accepted} but this client offered {offered}; the daemon is not speaking felis's negotiation, rebuild both halves". Do this before `FrameReader::new` (`:304`). 4. The bridge and CLI surface it through the existing error class (`skills/felis/SKILL.md:54` "protocol error / a daemon that does not serve this client's protocol major" already covers a preface refusal; keep exit class 2, extend the wording only if the CLI reference names the individual errors). Daemon (`felis-daemon/src/serve.rs`): 5. `exchange_bootstrap` (`:1726-1752`): call `preface::select(opened.preface)`; `None` → the existing refuse path; `Some(reply)` → write it and compute the effective minor from `reply`'s minor, not from `PROTOCOL_MINOR` directly, so the two cannot drift. Tests: - `felis-protocol` unit: `select` table (offered major, expected reply); `confirm_accept` on accept-same-major, accept-other-major, refuse, unknown. - `felis-transport/src/preface.rs` duplex tests: add wrong-accepted-major (daemon writes `Accept { major: 9, minor: 5 }`, client-side `confirm_accept` errors before any frame), and the offered major with daemon minor older/equal/newer asserting the effective minor. - `connector.rs` tests (beside `:2030-2071`): a fake listener writing `Accept { major: PROTOCOL_MAJOR + 1, .. }` and then a `Welcome`; assert `AcceptedUnofferedMajor`, non-transient, and that no `Hello` was written (read the server side and assert zero bytes). - Keep `the_status_word_alone_discriminates_the_reply` untouched: it pins the decoder, which stays lenient on purpose. Docs cascade (doc-cascade skill): - `docs/reference/ipc.md:186-206` "Version preface": one paragraph after the status table: an accept's first word MUST equal the offered major; the client verifies it before reading any frame and closes on mismatch with no frame written; the decoder itself does not compare (unchanged). - `docs/reference/spec.md:54` REQ-104: append "The client accepts only an accept naming the major it offered; any other accept closes the connection before a frame is written." Consider a REQ-104d rather than widening 104 if the sourcing table prefers one requirement per row. - `docs/explanation/architecture/ipc.md:485-495` "Handshake bootstrap": add the why: a self-describing reply is a *decoding* property; negotiation is still exact, and a lenient negotiator would turn schema skew into a protobuf failure after the preface, which is the loud-early failure the frozen layer exists to give. - `CHANGELOG.md` Unreleased / Changed: "Wire: a client closes on a preface accept that names a major it did not offer" (user-affecting only in the sense of a new error string; one line). - `crates/felis-protocol/proto/felis.proto:24-33` header comment: one clause on the echo invariant. - #30 regenerates the golden preface bytes (`preface.rs:488-490`, `:512-514`) afterwards; this issue does not touch them. ## Dependencies None to land. Sequencing: land before #30 (#30's "development-major peer is refused at the frozen preface" is the daemon half of this invariant; #45 is the client half, and #30's vectors should be regenerated once). #52's order ("resolve #45 before regenerating final preface vectors") still holds. Independent of #19 and #42. ## Risk/effort **S** (half a day). Main risk: choosing the error surface so the retry loop treats it as permanent (`is_transient`) and so the bridge's `ErrorKind` mapping is not silently "protocol" with a misleading message; test both. ## Labels Keep `priority/P0`, `release/v0.1.0`. It is a frozen-layer correctness bug and blocks the final preface vectors.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#45
No description provided.