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

[v0.1/P1] Extend the shared driver through application phases #46

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

The shared driver is described as the single phase/direction/mode/correlation state machine, but its phase model stops after the application handshake.

At review snapshot 5077d74b:

  • ConnectionDriver::handshake_done moves directly to Phase::Steady after Welcome.
  • The same state is used while waiting for attach/create/subscribe, after a session attach, and for an observer stream.
  • Kind-level admission consequently allows frames whose legality depends on those states; wait_for_attach, route_frame, and observer code add manual guards afterward.
  • Client Connection::recv_msg checks a kind and calls codec::decode directly for uncorrelated session replies, bypassing the driver's direction and correlation path.
  • A second Session::Attach after attachment, a pre-attach grid push, and observer/session traffic are therefore not rejected by one reusable state transition table.

This is an API contract problem for future clients as much as an internal cleanup: the docs promise one validator, while an implementer currently has to reconstruct caller-specific sequencing.

Required change

Extend the typed connection state machine through application setup and terminal roles, then make every inbound frame pass through it exactly once. Coordinate the shape with #20's atomic create/attach redesign before freezing protocol 2.0.

Acceptance criteria

  • States distinguish at least handshake, unattached/setup, attached session, and observer operation (or an equivalent typestate/admission-profile model).
  • Transitions are explicit for attach/create, ready/failure, detach/end, and notify subscription; invalid transitions fail before handler side effects.
  • Mode and phase admission are data owned by the driver, not duplicated match guards in daemon/client call sites.
  • Session replies use the same classify/decode/direction path as every other family; direct decode helpers cannot bypass validation.
  • Each inbound frame advances accounting/state at most once, including frames parked while correlated requests complete.
  • Table-driven tests cover every (side, mode, phase, kind/arm) boundary plus malformed transitions.
  • docs/reference/ipc.md states the actual phases and teardown result; docs/explanation/architecture/ipc.md keeps the rationale for a shared sans-I/O driver.
## Problem The shared driver is described as the single phase/direction/mode/correlation state machine, but its phase model stops after the application handshake. At review snapshot `5077d74b`: - `ConnectionDriver::handshake_done` moves directly to `Phase::Steady` after `Welcome`. - The same state is used while waiting for attach/create/subscribe, after a session attach, and for an observer stream. - Kind-level admission consequently allows frames whose legality depends on those states; `wait_for_attach`, `route_frame`, and observer code add manual guards afterward. - Client `Connection::recv_msg` checks a kind and calls `codec::decode` directly for uncorrelated session replies, bypassing the driver's direction and correlation path. - A second `Session::Attach` after attachment, a pre-attach grid push, and observer/session traffic are therefore not rejected by one reusable state transition table. This is an API contract problem for future clients as much as an internal cleanup: the docs promise one validator, while an implementer currently has to reconstruct caller-specific sequencing. ## Required change Extend the typed connection state machine through application setup and terminal roles, then make every inbound frame pass through it exactly once. Coordinate the shape with #20's atomic create/attach redesign before freezing protocol 2.0. ## Acceptance criteria - States distinguish at least handshake, unattached/setup, attached session, and observer operation (or an equivalent typestate/admission-profile model). - Transitions are explicit for attach/create, ready/failure, detach/end, and notify subscription; invalid transitions fail before handler side effects. - Mode and phase admission are data owned by the driver, not duplicated `match` guards in daemon/client call sites. - Session replies use the same classify/decode/direction path as every other family; direct decode helpers cannot bypass validation. - Each inbound frame advances accounting/state at most once, including frames parked while correlated requests complete. - Table-driven tests cover every `(side, mode, phase, kind/arm)` boundary plus malformed transitions. - `docs/reference/ipc.md` states the actual phases and teardown result; `docs/explanation/architecture/ipc.md` keeps the rationale for a shared sans-I/O driver.
natsukium commented 2026-09-03 20:35:19 +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; nothing post-snapshot touched the driver or the routing loops.

  • Phase is Preface | Handshake | Steady (crates/felis-transport/src/driver.rs:43-48); handshake_done moves straight to Steady (:219-222). admit_kind (:567-605) gates on phase only for Preface/Handshake (:574-587); after that the only filters are sole_sender (:615-626) and mode_admits (:631-646).
  • The same Steady state serves the daemon's pre-attach loop wait_for_attach (crates/felis-daemon/src/serve.rs:440-716), the attached pump's route_frame (:1123-1233), and the observer (:642-707). Each re-derives legality by hand: pre-attach refuses a second Hello at :445-451, Session reply arms at :621-623, non-Subscribe Notify arms at :699-705, "any other kind" at :708-713; post-attach route_frame refuses a second Attach/Create at :1211-1216 and unknown kinds at :1228-1231. ops_denied (:787-790) runs both pre-attach (:631) and in the pump (:1329).
  • Client bypass: Connection::recv_msg (crates/felis-client-core/src/connector.rs:1007-1013) checks frame.kind and calls codec::decode directly; it serves request() (:791-795), i.e. attach and spawn_session/create_with (:915-932). Those Session replies never pass ConnectionDriver::decode's direction check (driver.rs:396-403). correlated_request (:797-858) does go through the driver; is_reply_to (:868-898) is a peek and re-classifies, as its comment says. One more direct decode in non-test code: crates/felis-cli/src/cli_bridge.rs:1950-1955 decodes GridMsg after classify without driver.decode (harmless today: Grid is a one-way family checked by sole_sender, but it is the pattern the issue objects to). The GUI (crates/felis-client/src/main.rs:1773-1870), cli_notifications.rs:230, and cli_sessions.rs:1836 are clean.
  • from_halves (connector.rs:756-773) builds a driver already in Steady, so the bridge's attached connections start with no phase history at all.
  • A pre-attach grid push, observer/session cross-traffic, and a second Attach are therefore rejected by call-site match arms, not by a transition table; the docs promise the latter (docs/reference/ipc.md:440-444, docs/explanation/architecture/ipc.md:686-689).
  • Existing driver tests: each_phase_admits_only_its_own_frames (driver/tests.rs:485), a_second_hello_in_steady_state_closes_the_connection (:523), a_mode_refuses_a_surface_it_never_asked_for (:540). Nothing covers post-handshake states.

One nuance the issue underplays: Steady is not one state on the daemon but three call sites with hand-rolled guards, and on the client it is two (request vs correlated_request). The fix is the same either way.

Verdict

accept-with-changes. Do it, but bound the shape: a small explicit phase enum plus a (side, mode, phase, kind) admission table in the driver, and per-arm phase legality carried on the arm metadata that #47 introduces. Do not build a typestate API (the driver is shared behind a Mutex in the GUI, main.rs:1780, and behind &mut in three daemon loops; a typestate would force three drivers, which docs/explanation/architecture/ipc.md:723-727 already rejects). Sequencing changes from the issue text: this lands after #47/#48 (arm metadata) and #20 (the create/attach flow it must model), as #52's order says.

Approach

felis-transport/src/driver.rs:

  1. Phase::{Preface, Handshake, Setup, Attached, Observing}. Setup is post-Welcome, pre-attach/subscribe (the daemon's wait_for_attach loop, the client between handshake_over and attach). Transitions: handshake_done(mode) → Setup; attached() → Attached (daemon: after the subscribe reply at serve.rs:610, before Ready is written; client: on decoding Session::Ready); observing(stream) → Observing (daemon: after Notify::Subscribed at :667-672; client: cli_notifications); detached() → back to Setup only if #20 keeps a re-attach on one connection (today Route::Detach ends the pump; if so, no transition, the connection closes). A Closing phase is unnecessary: teardown is the connection ending.
  2. One admission table replaces mode_admits and the phase match in admit_kind: const fn admits(side, mode, phase, kind) -> bool written as an exhaustive match (each (phase, kind) row states the modes). Rows: Setup admits Conn(lifecycle only), Session(openers), Ops(queries+mutations per mode), Notify(Subscribe, Observer only); Attached admits Conn, Input, Grid, Image, Push, Session(Detach, ConfigureTheme), Ops, Region, Search; Observing admits Conn and Notify items. Arm-level rows ("openers only in Setup", "reply arms only inbound to the client") come from the #47 arm metadata (phase: PhaseSet) and are checked in decode after the direction check (driver.rs:396-403), so wait_for_attach:621-623, :699-705, route_frame:1211-1216 and the unreachable! arms collapse into Delivery::Deliver / typed DriverError::OutOfPhase { arm, phase }.
  3. DriverError gains OutOfPhase; ModeDenied keeps its pre-attach shape (Refused + close) and post-attach shape (typed request error) but the choice is made by the driver from phase, returning Delivery::RefuseRequest { reply } in Attached (mirrors the existing RefuseStream) so ops_denied and refuse_mode become data-driven.
  4. Guarantee "each inbound frame advances state at most once": today correlated_request parks frames and re-feeds them via pending (connector.rs:862-866, :903-908), classifying only when consumed; keep that, and add a debug counter/test that a parked frame is classified exactly once (the driver test can feed the same OwnedFrame twice and expect the second to be a correlation error, which a_second_terminal_closes_the_connection already models).

Daemon (serve.rs): wait_for_attach and route_frame shrink to routing on Delivery::Deliver(msg); remove the hand guards listed above; call driver.attached() / driver.observing() at the two points named. Client: replace recv_msg with classify + driver.decode::<Q::Msg> (mirror correlated_request:848-856); from_halves takes a Phase argument (bridge passes Setup for a fresh connection and Attached only where it re-wraps an already-attached pair). cli_bridge.rs:1950-1955: decode through the driver.

Tests: a table-driven driver test enumerating every (side, mode, phase, kind) cell (10 kinds × 3 modes × 5 phases × 2 sides = 300 rows) generated from the same admits function's inverse expectation list, plus arm-level cases: second Attach in Attached, Grid push in Setup (client side), Session::Ready inbound at the daemon, Notify::Event to an Attached window, Ops::Destroy from a Window in Setup (Refused) vs Attached (typed error). Daemon integration tests already at serve/tests.rs:3259, :3511, :3623 extend naturally.

Docs: docs/reference/ipc.md:440-444 name the five phases and their transitions (a small table), and state that teardown is the connection ending; :1754-1777 "Handshake" gets the Setup name; docs/explanation/architecture/ipc.md:686-703 keep the rationale, add why phases are data not typestate (three hosts, one driver, :723-727); docs/reference/spec.md REQ-114 wording if it enumerates the corruption sources. CHANGELOG: not user-visible unless error strings change; one line under Changed if Refused vs typed-error timing moves. skills/felis: no CLI change.

Dependencies

Decide #47 (arm metadata shape) and #48 (correlation class on that metadata) first; land #20 before this so Setup→Attached models the atomic create+attach rather than today's Create then Attach loop (serve.rs:551-553). #30 after. #52's order (item 2) holds.

Risk/effort

M (2-3 days after #47/#48/#20). Main risk: the GUI keeps the driver behind a mutex and transitions must be applied on that instance by the reader thread, not by the event loop, or a frame classified between Ready and attached() is refused; second risk is the bridge's from_halves callers assuming Steady.

Labels

Keep priority/P1, release/v0.1.0: it is the contract a non-Rust peer implements against, and the freeze boundary (#12) includes "observable session/window/mirror/update behavior".

## 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; nothing post-snapshot touched the driver or the routing loops. - `Phase` is `Preface | Handshake | Steady` (`crates/felis-transport/src/driver.rs:43-48`); `handshake_done` moves straight to `Steady` (`:219-222`). `admit_kind` (`:567-605`) gates on phase only for `Preface`/`Handshake` (`:574-587`); after that the only filters are `sole_sender` (`:615-626`) and `mode_admits` (`:631-646`). - The same `Steady` state serves the daemon's pre-attach loop `wait_for_attach` (`crates/felis-daemon/src/serve.rs:440-716`), the attached pump's `route_frame` (`:1123-1233`), and the observer (`:642-707`). Each re-derives legality by hand: pre-attach refuses a second `Hello` at `:445-451`, `Session` reply arms at `:621-623`, non-Subscribe `Notify` arms at `:699-705`, "any other kind" at `:708-713`; post-attach `route_frame` refuses a second `Attach`/`Create` at `:1211-1216` and unknown kinds at `:1228-1231`. `ops_denied` (`:787-790`) runs both pre-attach (`:631`) and in the pump (`:1329`). - Client bypass: `Connection::recv_msg` (`crates/felis-client-core/src/connector.rs:1007-1013`) checks `frame.kind` and calls `codec::decode` directly; it serves `request()` (`:791-795`), i.e. `attach` and `spawn_session`/`create_with` (`:915-932`). Those `Session` replies never pass `ConnectionDriver::decode`'s direction check (`driver.rs:396-403`). `correlated_request` (`:797-858`) does go through the driver; `is_reply_to` (`:868-898`) is a peek and re-classifies, as its comment says. One more direct decode in non-test code: `crates/felis-cli/src/cli_bridge.rs:1950-1955` decodes `GridMsg` after `classify` without `driver.decode` (harmless today: `Grid` is a one-way family checked by `sole_sender`, but it is the pattern the issue objects to). The GUI (`crates/felis-client/src/main.rs:1773-1870`), `cli_notifications.rs:230`, and `cli_sessions.rs:1836` are clean. - `from_halves` (`connector.rs:756-773`) builds a driver already in `Steady`, so the bridge's attached connections start with no phase history at all. - A pre-attach grid push, observer/session cross-traffic, and a second `Attach` are therefore rejected by call-site `match` arms, not by a transition table; the docs promise the latter (`docs/reference/ipc.md:440-444`, `docs/explanation/architecture/ipc.md:686-689`). - Existing driver tests: `each_phase_admits_only_its_own_frames` (`driver/tests.rs:485`), `a_second_hello_in_steady_state_closes_the_connection` (`:523`), `a_mode_refuses_a_surface_it_never_asked_for` (`:540`). Nothing covers post-handshake states. One nuance the issue underplays: `Steady` is not one state on the daemon but three call sites with hand-rolled guards, and on the client it is two (`request` vs `correlated_request`). The fix is the same either way. ## Verdict **accept-with-changes.** Do it, but bound the shape: a small explicit phase enum plus a `(side, mode, phase, kind)` admission table in the driver, and per-arm phase legality carried on the arm metadata that #47 introduces. Do not build a typestate API (the driver is shared behind a `Mutex` in the GUI, `main.rs:1780`, and behind `&mut` in three daemon loops; a typestate would force three drivers, which `docs/explanation/architecture/ipc.md:723-727` already rejects). Sequencing changes from the issue text: this lands after #47/#48 (arm metadata) and #20 (the create/attach flow it must model), as #52's order says. ## Approach `felis-transport/src/driver.rs`: 1. `Phase::{Preface, Handshake, Setup, Attached, Observing}`. `Setup` is post-`Welcome`, pre-attach/subscribe (the daemon's `wait_for_attach` loop, the client between `handshake_over` and `attach`). Transitions: `handshake_done(mode)` → `Setup`; `attached()` → `Attached` (daemon: after the subscribe reply at `serve.rs:610`, before `Ready` is written; client: on decoding `Session::Ready`); `observing(stream)` → `Observing` (daemon: after `Notify::Subscribed` at `:667-672`; client: `cli_notifications`); `detached()` → back to `Setup` only if #20 keeps a re-attach on one connection (today `Route::Detach` ends the pump; if so, no transition, the connection closes). A `Closing` phase is unnecessary: teardown is the connection ending. 2. One admission table replaces `mode_admits` and the phase `match` in `admit_kind`: `const fn admits(side, mode, phase, kind) -> bool` written as an exhaustive `match` (each `(phase, kind)` row states the modes). Rows: `Setup` admits `Conn`(lifecycle only), `Session`(openers), `Ops`(queries+mutations per mode), `Notify`(`Subscribe`, Observer only); `Attached` admits `Conn`, `Input`, `Grid`, `Image`, `Push`, `Session`(`Detach`, `ConfigureTheme`), `Ops`, `Region`, `Search`; `Observing` admits `Conn` and `Notify` items. Arm-level rows ("openers only in Setup", "reply arms only inbound to the client") come from the #47 arm metadata (`phase: PhaseSet`) and are checked in `decode` after the direction check (`driver.rs:396-403`), so `wait_for_attach:621-623`, `:699-705`, `route_frame:1211-1216` and the `unreachable!` arms collapse into `Delivery::Deliver` / typed `DriverError::OutOfPhase { arm, phase }`. 3. `DriverError` gains `OutOfPhase`; `ModeDenied` keeps its pre-attach shape (`Refused` + close) and post-attach shape (typed request error) but the *choice* is made by the driver from `phase`, returning `Delivery::RefuseRequest { reply }` in `Attached` (mirrors the existing `RefuseStream`) so `ops_denied` and `refuse_mode` become data-driven. 4. Guarantee "each inbound frame advances state at most once": today `correlated_request` parks frames and re-feeds them via `pending` (`connector.rs:862-866`, `:903-908`), classifying only when consumed; keep that, and add a debug counter/test that a parked frame is classified exactly once (the driver test can feed the same `OwnedFrame` twice and expect the second to be a correlation error, which `a_second_terminal_closes_the_connection` already models). Daemon (`serve.rs`): `wait_for_attach` and `route_frame` shrink to routing on `Delivery::Deliver(msg)`; remove the hand guards listed above; call `driver.attached()` / `driver.observing()` at the two points named. Client: replace `recv_msg` with `classify` + `driver.decode::<Q::Msg>` (mirror `correlated_request:848-856`); `from_halves` takes a `Phase` argument (bridge passes `Setup` for a fresh connection and `Attached` only where it re-wraps an already-attached pair). `cli_bridge.rs:1950-1955`: decode through the driver. Tests: a table-driven driver test enumerating every `(side, mode, phase, kind)` cell (10 kinds × 3 modes × 5 phases × 2 sides = 300 rows) generated from the same `admits` function's inverse expectation list, plus arm-level cases: second `Attach` in `Attached`, `Grid` push in `Setup` (client side), `Session::Ready` inbound at the daemon, `Notify::Event` to an `Attached` window, `Ops::Destroy` from a `Window` in `Setup` (Refused) vs `Attached` (typed error). Daemon integration tests already at `serve/tests.rs:3259`, `:3511`, `:3623` extend naturally. Docs: `docs/reference/ipc.md:440-444` name the five phases and their transitions (a small table), and state that teardown is the connection ending; `:1754-1777` "Handshake" gets the `Setup` name; `docs/explanation/architecture/ipc.md:686-703` keep the rationale, add why phases are data not typestate (three hosts, one driver, `:723-727`); `docs/reference/spec.md` REQ-114 wording if it enumerates the corruption sources. CHANGELOG: not user-visible unless error strings change; one line under Changed if `Refused` vs typed-error timing moves. `skills/felis`: no CLI change. ## Dependencies Decide #47 (arm metadata shape) and #48 (correlation class on that metadata) first; land #20 before this so `Setup`→`Attached` models the atomic create+attach rather than today's `Create` then `Attach` loop (`serve.rs:551-553`). #30 after. #52's order (item 2) holds. ## Risk/effort **M** (2-3 days after #47/#48/#20). Main risk: the GUI keeps the driver behind a mutex and transitions must be applied on that instance by the reader thread, not by the event loop, or a frame classified between `Ready` and `attached()` is refused; second risk is the bridge's `from_halves` callers assuming `Steady`. ## Labels Keep `priority/P1`, `release/v0.1.0`: it is the contract a non-Rust peer implements against, and the freeze boundary (#12) includes "observable session/window/mirror/update behavior".
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#46
No description provided.