[Post-v0.1/P2] Split coordination hotspots after contracts freeze #33

New issue

Open

opened 2026-09-03 16:18:10 +09:00 by natsukium · 1 comment

natsukium commented

2026-09-03 16:18:10 +09:00

Owner

Follow-up from #12 (P2); start only after the public CLI/IPC choices settle.

Why

cli_bridge.rs, cli_sessions.rs, the connector, and client navigation are coordination hotspots. Refactoring them while contracts are still changing would obscure semantic breaks and make review harder.

Scope

Keep one serialized daemon-session owner.
Extract admission, stream registration, rehydration, and navigation behind explicit boundaries.
Preserve crate dependency direction and the headless felis-client-core seam.

Acceptance criteria

Public wire, CLI, bridge, config, and lifecycle behavior is unchanged.
Existing contract and concurrency tests pass without snapshot churn unrelated to structure.
New module boundaries reduce coordination responsibilities rather than merely moving lines.
No runtime or OS dependency enters felis-protocol.

Follow-up from #12 (P2); start only after the public CLI/IPC choices settle. ## Why `cli_bridge.rs`, `cli_sessions.rs`, the connector, and client navigation are coordination hotspots. Refactoring them while contracts are still changing would obscure semantic breaks and make review harder. ## Scope - Keep one serialized daemon-session owner. - Extract admission, stream registration, rehydration, and navigation behind explicit boundaries. - Preserve crate dependency direction and the headless `felis-client-core` seam. ## Acceptance criteria - [ ] Public wire, CLI, bridge, config, and lifecycle behavior is unchanged. - [ ] Existing contract and concurrency tests pass without snapshot churn unrelated to structure. - [ ] New module boundaries reduce coordination responsibilities rather than merely moving lines. - [ ] No runtime or OS dependency enters `felis-protocol`.

natsukium added the

priority/P2

label

2026-09-03 16:18:10 +09:00

natsukium referenced this issue

2026-09-03 16:18:49 +09:00

[v0.1/Tracker] Close release blockers before v0.1.0 #12

natsukium commented

2026-09-03 20:35:17 +09:00

Author

Owner

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 on the diagnosis; imprecise on where the files live.

The hotspots exist and are large. crates/felis-cli/src/cli_sessions.rs is 2927 lines with run/plan dispatch at :488/:547, one cmd_* per verb (cmd_list :924, cmd_info :968, cmd_send :1048, cmd_kill :1431, cmd_tag :1454, cmd_evict :1511, cmd_switch :1590, cmd_retarget :1675, cmd_capture :2066, cmd_search :2265), and the shared frame drivers drain_frames :1782, drain_rehydrate :1907, drain_region_rows :1965, spawn_on :2211. crates/felis-cli/src/cli_bridge.rs is 2187 lines with admission (parse_request :405, Params :460), the session/stream ownership tables (Core :622, SessionSlot :638, StreamSlot :571, ActiveOp :556), stream registration (Registry :1436, Link :1421, StreamHandle :1499), and the pump (pump :1894, dial :1910, drain_rehydrate :1924) in one file. Note both are in felis-cli, not felis-client-core; the issue's phrasing ("the connector") correctly places crates/felis-client-core/src/connector.rs (2337 lines: dial/offer in connect :210, connect_carrier :450, AttachIntent :500, the request methods :903-996, plus DaemonStatus :683). Client navigation is spread across crates/felis-client/src/event_handler.rs (1205), app_methods.rs (1876), switch_intent.rs, and felis-client-core/src/action.rs + keymap/map.rs.
The contract tests the acceptance criteria rely on exist: crates/felis-cli/tests/cli_sessions.rs (2498 lines) and crates/felis-cli/tests/cli_bridge.rs (1048 lines), plus the felis-daemon head_of_line_rehydrate.rs / real_app_harness.rs concurrency harnesses.
The reason to wait is confirmed by the tracker: eight open v0.1.0 P1 issues rewrite exactly these files — #20 (spawn_on, connector create_with/attach), #21 (bridge Out/Core bounds), #22 (connector/dial reconnect), #23 (cli_output/cli_sessions machine contract), #24 (cmd_retarget/run_retarget, connector retarget_window), #46 (the shared driver through drain_frames/pump), #47/#48 (frame taxonomy and correlation, which change what the drivers match on).

Verdict

defer (keep post-v0.1.0, P2). Deferral does not get more expensive because of the freeze — the opposite: #12's "Contract freeze boundary" explicitly does not freeze "unpublished Rust APIs, private module structure, or every crate's current pub visibility", so this refactor is free after the tag by construction, and every P1 above would have to be re-based over it if it landed first.

One scope adjustment: #46 ("extend the shared driver through application phases") is the first slice of this refactor and lands pre-freeze. After #46, re-survey before starting — the driver extraction may have already removed the biggest duplicated block between cli_sessions.rs and cli_bridge.rs (drain_rehydrate exists in both files today, :1907 and :1924).

Approach

Pure-move commits first, behavior-preserving, one hotspot per commit so git diff --color-moved reviews as moves:

felis-cli/src/cli_sessions.rs → cli_sessions/: mod.rs keeps run, plan, the Reporter selection (point_out/stream_out :718-724), and id resolution (resolve_roster_id :96); read.rs (cmd_list, cmd_info, cmd_capture, cmd_search, print_*); mutate.rs (cmd_send + wait_for_command_end, cmd_kill, cmd_evict, cmd_tag, spawn_on/spawn_args_for); attachment.rs (cmd_switch, cmd_retarget, run_retarget/staged_retarget, the switch-denial reporting). The frame drivers move to whatever module #46 produced (or a driver.rs shared with the bridge if #46 did not).
cli_bridge.rs → cli_bridge/: admission.rs (parse_request, Params, check_scope_minor, is_streaming_op — the last one should become a table shared with the CLI classification per #35), envelope.rs (Envelope, Body, error_object, terminal objects), registry.rs (Registry, Link, StreamHandle, pump), core.rs (Core, SessionSlot, StreamSlot, ActiveOp, settle) — the one serialized session owner the issue's scope keeps.
felis-client-core/src/connector.rs: split the dial/offer/handshake (connect, connect_carrier, Offer, Carrier*, RemoteSpawn) from the Connection request methods and DaemonStatus; rehydration already lives in pull.rs/roster.rs/shadow.rs, so the connector should shrink to dial + typed requests.
Client navigation: extract switch/retarget intent handling out of event_handler.rs/app_methods.rs around the existing switch_intent.rs seam; anything headless (intent resolution, denial mapping) goes to felis-client-core, anything winit-facing stays.
Guard rails: felis-protocol/Cargo.toml unchanged (no tokio/OS dep — the acceptance criterion is mechanical: cargo tree -p felis-protocol shows no tokio); dependency direction in CLAUDE.md "Project shape" unchanged.
Docs: none of the user-facing docs change; docs/reference/workspace.md (layout) and docs/explanation/implementation.md if they name the files; skills extend-ipc step 4 names cli_sessions/cli_bridge by file — update paths in the same change (chore(skills):).
Tests: the two contract test files and the daemon harnesses must pass unchanged; just snapshot-* must show zero churn; run just check.

Dependencies

Land after #20, #21, #22, #23, #24, #46 (and the #47/#48 shape decisions they build on) — all v0.1.0 P1. #35's classification table is a good thing to introduce in the bridge/CLI split (one shared table instead of is_streaming_op :387 and the cli.md table diverging). #12's order still holds; this sits after #31.

Risk/effort

M–L (M per hotspot, four hotspots). Main risk: a semantic change hidden inside a large move. Mitigation is procedural — move-only commits, then behavior commits, never mixed — which is precisely why the issue waits for the contracts to stop moving.

Labels

Keep priority/P2, post-v0.1.0. Consider adding a type/refactor label if one exists, and a note that #46 is the first slice.

## 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 on the diagnosis; imprecise on where the files live. - The hotspots exist and are large. `crates/felis-cli/src/cli_sessions.rs` is 2927 lines with `run`/`plan` dispatch at `:488`/`:547`, one `cmd_*` per verb (`cmd_list :924`, `cmd_info :968`, `cmd_send :1048`, `cmd_kill :1431`, `cmd_tag :1454`, `cmd_evict :1511`, `cmd_switch :1590`, `cmd_retarget :1675`, `cmd_capture :2066`, `cmd_search :2265`), and the shared frame drivers `drain_frames :1782`, `drain_rehydrate :1907`, `drain_region_rows :1965`, `spawn_on :2211`. `crates/felis-cli/src/cli_bridge.rs` is 2187 lines with admission (`parse_request :405`, `Params :460`), the session/stream ownership tables (`Core :622`, `SessionSlot :638`, `StreamSlot :571`, `ActiveOp :556`), stream registration (`Registry :1436`, `Link :1421`, `StreamHandle :1499`), and the pump (`pump :1894`, `dial :1910`, `drain_rehydrate :1924`) in one file. Note both are in **felis-cli**, not felis-client-core; the issue's phrasing ("the connector") correctly places `crates/felis-client-core/src/connector.rs` (2337 lines: dial/offer in `connect :210`, `connect_carrier :450`, `AttachIntent :500`, the request methods `:903-996`, plus `DaemonStatus :683`). Client navigation is spread across `crates/felis-client/src/event_handler.rs` (1205), `app_methods.rs` (1876), `switch_intent.rs`, and `felis-client-core/src/action.rs` + `keymap/map.rs`. - The contract tests the acceptance criteria rely on exist: `crates/felis-cli/tests/cli_sessions.rs` (2498 lines) and `crates/felis-cli/tests/cli_bridge.rs` (1048 lines), plus the felis-daemon `head_of_line_rehydrate.rs` / `real_app_harness.rs` concurrency harnesses. - The reason to wait is confirmed by the tracker: eight open v0.1.0 P1 issues rewrite exactly these files — #20 (`spawn_on`, connector `create_with`/`attach`), #21 (bridge `Out`/`Core` bounds), #22 (connector/dial reconnect), #23 (`cli_output`/`cli_sessions` machine contract), #24 (`cmd_retarget`/`run_retarget`, connector `retarget_window`), #46 (the shared driver through `drain_frames`/`pump`), #47/#48 (frame taxonomy and correlation, which change what the drivers match on). ## Verdict **defer** (keep post-v0.1.0, P2). Deferral does not get more expensive because of the freeze — the opposite: #12's "Contract freeze boundary" explicitly does *not* freeze "unpublished Rust APIs, private module structure, or every crate's current `pub` visibility", so this refactor is free after the tag by construction, and every P1 above would have to be re-based over it if it landed first. One scope adjustment: #46 ("extend the shared driver through application phases") is the first slice of this refactor and lands pre-freeze. After #46, re-survey before starting — the driver extraction may have already removed the biggest duplicated block between `cli_sessions.rs` and `cli_bridge.rs` (`drain_rehydrate` exists in both files today, `:1907` and `:1924`). ## Approach Pure-move commits first, behavior-preserving, one hotspot per commit so `git diff --color-moved` reviews as moves: 1. **`felis-cli/src/cli_sessions.rs` → `cli_sessions/`**: `mod.rs` keeps `run`, `plan`, the `Reporter` selection (`point_out`/`stream_out :718-724`), and id resolution (`resolve_roster_id :96`); `read.rs` (`cmd_list`, `cmd_info`, `cmd_capture`, `cmd_search`, `print_*`); `mutate.rs` (`cmd_send` + `wait_for_command_end`, `cmd_kill`, `cmd_evict`, `cmd_tag`, `spawn_on`/`spawn_args_for`); `attachment.rs` (`cmd_switch`, `cmd_retarget`, `run_retarget`/`staged_retarget`, the switch-denial reporting). The frame drivers move to whatever module #46 produced (or a `driver.rs` shared with the bridge if #46 did not). 2. **`cli_bridge.rs` → `cli_bridge/`**: `admission.rs` (`parse_request`, `Params`, `check_scope_minor`, `is_streaming_op` — the last one should become a table shared with the CLI classification per #35), `envelope.rs` (`Envelope`, `Body`, `error_object`, terminal objects), `registry.rs` (`Registry`, `Link`, `StreamHandle`, `pump`), `core.rs` (`Core`, `SessionSlot`, `StreamSlot`, `ActiveOp`, `settle`) — the one serialized session owner the issue's scope keeps. 3. **`felis-client-core/src/connector.rs`**: split the dial/offer/handshake (`connect`, `connect_carrier`, `Offer`, `Carrier*`, `RemoteSpawn`) from the `Connection` request methods and `DaemonStatus`; rehydration already lives in `pull.rs`/`roster.rs`/`shadow.rs`, so the connector should shrink to dial + typed requests. 4. **Client navigation**: extract switch/retarget intent handling out of `event_handler.rs`/`app_methods.rs` around the existing `switch_intent.rs` seam; anything headless (intent resolution, denial mapping) goes to felis-client-core, anything winit-facing stays. 5. Guard rails: `felis-protocol/Cargo.toml` unchanged (no tokio/OS dep — the acceptance criterion is mechanical: `cargo tree -p felis-protocol` shows no tokio); dependency direction in `CLAUDE.md` "Project shape" unchanged. 6. **Docs**: none of the user-facing docs change; `docs/reference/workspace.md` (layout) and `docs/explanation/implementation.md` if they name the files; skills `extend-ipc` step 4 names `cli_sessions`/`cli_bridge` by file — update paths in the same change (`chore(skills):`). 7. **Tests**: the two contract test files and the daemon harnesses must pass unchanged; `just snapshot-*` must show zero churn; run `just check`. ## Dependencies Land after **#20, #21, #22, #23, #24, #46** (and the #47/#48 shape decisions they build on) — all v0.1.0 P1. #35's classification table is a good thing to introduce in the bridge/CLI split (one shared table instead of `is_streaming_op :387` and the `cli.md` table diverging). #12's order still holds; this sits after #31. ## Risk/effort **M–L** (M per hotspot, four hotspots). Main risk: a semantic change hidden inside a large move. Mitigation is procedural — move-only commits, then behavior commits, never mixed — which is precisely why the issue waits for the contracts to stop moving. ## Labels Keep `priority/P2`, post-v0.1.0. Consider adding a `type/refactor` label if one exists, and a note that #46 is the first slice.

natsukium added the

status/planned

label

2026-09-03 20:35:17 +09:00

natsukium added the

status/blocked

label

2026-09-07 16:24:28 +09:00