natsukium/felis
Archived
Watch
1
Code Issues 23 Pull requests Projects Releases 6 Packages Wiki Activity Actions

protocol, cli: pre-release review of frozen surfaces — six cheap-before-freeze gaps #347

Closed
opened 2026-09-22 19:18:55 +09:00 by natsukium · 3 comments
natsukium commented 2026-09-22 19:18:55 +09:00
Owner
Copy link

Context

A pre-release review of the hard-to-change surfaces (protobuf schema, IPC versioning, CLI contract) found the design sound; the findings below are all "free to decide now, costs a major after the freeze". Nothing blocks the release.

Findings

1. The negotiated effective minor is unobservable anywhere

ConnWelcome echoes nothing, and OpsStatusReply deliberately carries no version (the handshake is the one authority, #343). But the effective minor — min(client, daemon) — is computed on both ends and published nowhere: not in Welcome, not in felis daemon status (which prints the daemon's own minor, not any connection's), not in any machine output.

Consequences after the minor ledger starts growing:

  • A user cannot see what minor a live connection is actually running, so "my old CLI can't use arm X" is debuggable only through the send gate's refusal string.
  • Post-release observation of minor adoption (bug triage, usage questions) has no surface.

Proposal: add one informational field to ConnWelcome (e.g. effective_minor, or echo client_minor). It is display/diagnostic only; the preface stays the sole version authority — same reasoning #343 used to remove the duplicated copy from OpsStatusReply, applied to an observation point rather than a second copy. Adding it at minor 0 is wire-free.

2. Pin the _UNSPECIFIED mode → Refused { UnknownMode } promise with a test

The contract says a ConnHello naming a mode the daemon does not define is answered Refused { REFUSAL_REASON_UNKNOWN_MODE } and closed — that is the documented remedy for a newer client against an older daemon. But ConnectionMode is a proto enum: a future mode value decodes as _UNSPECIFIED on an older decoder, and whether the daemon then takes the typed-refusal path or the corruption (connection-kill) path is currently unpinned. The implementation is probably right; the promise is not.

The path the first post-release mode addition lands on must not be undefined at freeze time. Add a test asserting: a ConnHello with mode CONNECTION_MODE_UNSPECIFIED produces exactly ConnRefused { reason: UNKNOWN_MODE }, never a decode-fatal teardown. This is a test-gap, not a code change, unless the test finds the decode refuses _UNSPECIFIED before the handler can refuse it.

3. Restate the ConnEnd.count item/trailer criterion at its definition

count is defined as "the results that preceded it, never the acks, markers, and family trailers riding the same stream", and two precedents exist (RegionRowsDone = trailer, out; NotifySubscribed = ack, out). The criterion ("would a family-agnostic terminal carry this?") lives only in the RegionRows discussion. Restate it in one sentence at the ConnEnd bullet so whoever writes the next minor-ledger row does not have to re-derive it. Doc-only; run the doc-cascade sweep.

4. Promise that error.kind tokens never narrow an existing token's meaning

The CLI v:1 epoch freezes the kind vocabulary's spellings and the kind→exit-code mapping, and the schema keeps error.kind open with x-known-values. What is not stated: a new token must not narrow (respell the semantics of) an existing one, the way felis-json v1 states its additive-only rule. One line in cli.md "Machine output" (frozen-for-v:1 paragraph) closes it: new tokens land within the epoch, existing ones are never respelled, remapped, or narrowed.

5. Define sessions send --timeout 0

--timeout <secs> requires --wait, and an unbounded wait is the bare --wait spelling — a sound design. But the meaning of --timeout 0 (immediate timeout? ignored? usage error?) is not pinned anywhere the contract freezes. After the release it cannot be redefined. Check the bridge/schema bound (minimum), pick a meaning, and state it in cli.md's send bullet plus the JSON schema annotation.

6. Reference-side reason for the relay block's version 0 = malformed rule

ipc.md states a payload format version of 0 closes the connection while unknown nonzero versions take the fallback — correct, and deliberately asymmetric. The why (the version word is how a pre-release daemon without the word is told apart from one with it) sits in the explanation doc only. One sentence in the reference section (why this one value is malformed, unlike every other unknown version) so a reference-only reader implementing another relay does not have to find the explanation. Doc-only.

Non-findings (checked, sound)

Field-numbering and reclaim policy, the BREAKING.md base-ack mechanism, arm-as-routing-unit and the descriptor-carried arm table, the field-100 correlation slot, u128 id encoding, zigzag signed ints, the row codec's opaque-bytes/version-byte split, the three-layer versioning (wire minor / CLI v:1 / felis-json v1), the limit tiering table, and the notify_id/notification_id spelling split.

## Context A pre-release review of the hard-to-change surfaces (protobuf schema, IPC versioning, CLI contract) found the design sound; the findings below are all "free to decide now, costs a major after the freeze". Nothing blocks the release. ## Findings ### 1. The negotiated effective minor is unobservable anywhere `ConnWelcome` echoes nothing, and `OpsStatusReply` deliberately carries no version (the handshake is the one authority, #343). But the *effective* minor — `min(client, daemon)` — is computed on both ends and published nowhere: not in `Welcome`, not in `felis daemon status` (which prints the daemon's own minor, not any connection's), not in any machine output. Consequences after the minor ledger starts growing: - A user cannot see what minor a live connection is actually running, so "my old CLI can't use arm X" is debuggable only through the send gate's refusal string. - Post-release observation of minor adoption (bug triage, usage questions) has no surface. Proposal: add one informational field to `ConnWelcome` (e.g. `effective_minor`, or echo `client_minor`). It is display/diagnostic only; the preface stays the sole version authority — same reasoning #343 used to remove the duplicated copy from `OpsStatusReply`, applied to an *observation point* rather than a second copy. Adding it at minor 0 is wire-free. ### 2. Pin the `_UNSPECIFIED` mode → `Refused { UnknownMode }` promise with a test The contract says a `ConnHello` naming a mode the daemon does not define is answered `Refused { REFUSAL_REASON_UNKNOWN_MODE }` and closed — that is the documented remedy for a newer client against an older daemon. But `ConnectionMode` is a proto enum: a future mode value decodes as `_UNSPECIFIED` on an older decoder, and whether the daemon then takes the typed-refusal path or the corruption (connection-kill) path is currently unpinned. The implementation is probably right; the promise is not. The path the first post-release mode addition lands on must not be undefined at freeze time. Add a test asserting: a `ConnHello` with mode `CONNECTION_MODE_UNSPECIFIED` produces exactly `ConnRefused { reason: UNKNOWN_MODE }`, never a decode-fatal teardown. This is a test-gap, not a code change, unless the test finds the decode refuses `_UNSPECIFIED` before the handler can refuse it. ### 3. Restate the `ConnEnd.count` item/trailer criterion at its definition `count` is defined as "the results that preceded it, never the acks, markers, and family trailers riding the same stream", and two precedents exist (`RegionRowsDone` = trailer, out; `NotifySubscribed` = ack, out). The criterion ("would a family-agnostic terminal carry this?") lives only in the `RegionRows` discussion. Restate it in one sentence at the `ConnEnd` bullet so whoever writes the next minor-ledger row does not have to re-derive it. Doc-only; run the doc-cascade sweep. ### 4. Promise that `error.kind` tokens never narrow an existing token's meaning The CLI `v:1` epoch freezes the kind vocabulary's spellings and the kind→exit-code mapping, and the schema keeps `error.kind` open with `x-known-values`. What is not stated: a new token must not *narrow* (respell the semantics of) an existing one, the way `felis-json` v1 states its additive-only rule. One line in cli.md "Machine output" (frozen-for-`v:1` paragraph) closes it: new tokens land within the epoch, existing ones are never respelled, remapped, or narrowed. ### 5. Define `sessions send --timeout 0` `--timeout <secs>` requires `--wait`, and an unbounded wait is the bare `--wait` spelling — a sound design. But the meaning of `--timeout 0` (immediate timeout? ignored? usage error?) is not pinned anywhere the contract freezes. After the release it cannot be redefined. Check the bridge/schema bound (`minimum`), pick a meaning, and state it in cli.md's `send` bullet plus the JSON schema annotation. ### 6. Reference-side reason for the relay block's `version 0 = malformed` rule ipc.md states a payload format version of `0` closes the connection while unknown *nonzero* versions take the fallback — correct, and deliberately asymmetric. The why (the version word is how a pre-release daemon without the word is told apart from one with it) sits in the explanation doc only. One sentence in the reference section (why this one value is malformed, unlike every other unknown version) so a reference-only reader implementing another relay does not have to find the explanation. Doc-only. ## Non-findings (checked, sound) Field-numbering and reclaim policy, the `BREAKING.md` base-ack mechanism, arm-as-routing-unit and the descriptor-carried arm table, the field-100 correlation slot, `u128` id encoding, zigzag signed ints, the row codec's opaque-bytes/version-byte split, the three-layer versioning (wire minor / CLI `v:1` / `felis-json` v1), the limit tiering table, and the `notify_id`/`notification_id` spelling split.
natsukium commented 2026-09-22 20:16:40 +09:00
Author
Owner
Copy link

Two more cheap-before-freeze gaps from the same review pass, both about the schema's claim to be the single cross-language authority. Neither blocks the release.

A. Field-level minor metadata covers only six messages

crates/felis-protocol/src/minor.rs declares field requirements in hand-written tables (CONN_FIELDS, SESSION_FIELDS, OPS_FIELDS, PUSH_FIELDS, SESSION_INFO_FIELDS, SPAWN_ARGS_FIELDS), and gated_by_arm! covers Input / Image / Notify / Region / Search on the assumption that "no post-baseline field or enum value rides inside one". The ledger-coverage test builds its named set from those tables plus arms, closed-enum values, and row-codec versions.

Consequence: a field added after the freeze to SearchQuery, ImageNew, RegionRequest, NotifyEvent, InputResize, and so on has no place to declare its minor. requires() does not rise, and the_ledger_covers_every_executable_addition still passes. Protobuf's unknown-field tolerance then makes an older peer ignore the field silently, so the request loses the semantics it asked for instead of being refused by the send gate — the failure the gate exists to prevent for the six gated messages.

Proposal: give fields a declared minor in the schema itself (a (felis.v1.since) field option, or extend the ArmRouting pattern), and have the ledger test enumerate every field from the descriptor, requiring a minor and a ledger row for each post-baseline one. The GatedField tables then become derived rather than hand-maintained.

B. Bitmap layouts are not in the schema

felis.proto says bitflags carry "raw bits; see messages.rs" for InputMouseEvent.mods (line 990), InputKeyEvent.mods (line 1010), and the type note at lines 129-130. The layouts live only in crates/felis-protocol/src/messages/input.rs: InputMods is the xterm-mouse SHIFT=1 / ALT=2 / CTRL=4, and KeyMods is CTRL=1 / SHIFT=2 / ALT=4 / SUPER=8, which is neither the xterm mouse order nor the Kitty keyboard order (shift=1 / alt=2 / ctrl=4 / super=8). A non-Rust peer reading only the descriptor cannot encode or decode these, and docs/reference/ipc.md "Application messages" promises it needs no other manifest. GridClipboardSet.selection states its bits inline ("c=1, p=2"), so the pattern exists.

Proposal: carry the bit tables as a field option read from the descriptor (the (felis.v1.arm) shape), or at minimum make the proto comments normative and name the standard each set follows. GridKittyKbdFlags.flags deserves the same treatment.

C. Small shape inconsistencies (low priority)

  • Row indices have three widths: SearchMatch.line_index / ColSpan.line_index are sint64, RegionRow.row is sint32, and RowEntry.row is uint32.
  • ResolvedId.ambiguous is a bare uint32 arm while OpsInfoReply.ambiguous is an OpsInfoAmbiguous { matches } message; SpawnRefused and SessionAttachFailed place detail differently.
  • OSC 1 sets only the daemon-side icon name (felis-grid/src/sink.rs), which no GridMsg arm carries, so the client never applies it. The support matrix's check mark may overstate what the sequence does.
Two more cheap-before-freeze gaps from the same review pass, both about the schema's claim to be the single cross-language authority. Neither blocks the release. ### A. Field-level minor metadata covers only six messages `crates/felis-protocol/src/minor.rs` declares field requirements in hand-written tables (`CONN_FIELDS`, `SESSION_FIELDS`, `OPS_FIELDS`, `PUSH_FIELDS`, `SESSION_INFO_FIELDS`, `SPAWN_ARGS_FIELDS`), and `gated_by_arm!` covers `Input` / `Image` / `Notify` / `Region` / `Search` on the assumption that "no post-baseline field or enum value rides inside one". The ledger-coverage test builds its `named` set from those tables plus arms, closed-enum values, and row-codec versions. Consequence: a field added after the freeze to `SearchQuery`, `ImageNew`, `RegionRequest`, `NotifyEvent`, `InputResize`, and so on has no place to declare its minor. `requires()` does not rise, and `the_ledger_covers_every_executable_addition` still passes. Protobuf's unknown-field tolerance then makes an older peer ignore the field silently, so the request loses the semantics it asked for instead of being refused by the send gate — the failure the gate exists to prevent for the six gated messages. Proposal: give fields a declared minor in the schema itself (a `(felis.v1.since)` field option, or extend the `ArmRouting` pattern), and have the ledger test enumerate every field from the descriptor, requiring a minor and a ledger row for each post-baseline one. The `GatedField` tables then become derived rather than hand-maintained. ### B. Bitmap layouts are not in the schema `felis.proto` says bitflags carry "raw bits; see messages.rs" for `InputMouseEvent.mods` (line 990), `InputKeyEvent.mods` (line 1010), and the type note at lines 129-130. The layouts live only in `crates/felis-protocol/src/messages/input.rs`: `InputMods` is the xterm-mouse SHIFT=1 / ALT=2 / CTRL=4, and `KeyMods` is CTRL=1 / SHIFT=2 / ALT=4 / SUPER=8, which is neither the xterm mouse order nor the Kitty keyboard order (shift=1 / alt=2 / ctrl=4 / super=8). A non-Rust peer reading only the descriptor cannot encode or decode these, and `docs/reference/ipc.md` "Application messages" promises it needs no other manifest. `GridClipboardSet.selection` states its bits inline ("c=1, p=2"), so the pattern exists. Proposal: carry the bit tables as a field option read from the descriptor (the `(felis.v1.arm)` shape), or at minimum make the proto comments normative and name the standard each set follows. `GridKittyKbdFlags.flags` deserves the same treatment. ### C. Small shape inconsistencies (low priority) - Row indices have three widths: `SearchMatch.line_index` / `ColSpan.line_index` are `sint64`, `RegionRow.row` is `sint32`, and `RowEntry.row` is `uint32`. - `ResolvedId.ambiguous` is a bare `uint32` arm while `OpsInfoReply.ambiguous` is an `OpsInfoAmbiguous { matches }` message; `SpawnRefused` and `SessionAttachFailed` place `detail` differently. - OSC 1 sets only the daemon-side icon name (`felis-grid/src/sink.rs`), which no `GridMsg` arm carries, so the client never applies it. The support matrix's check mark may overstate what the sequence does.
natsukium commented 2026-09-22 20:18:49 +09:00
Author
Owner
Copy link

Correction to section B above, after checking the reference more carefully:

  • The KeyMods bit layout is in the reference prose (docs/reference/ipc.md line 1187: ctrl 1, shift 2, alt 4, super 8), matching messages.rs. So "the layouts live only in messages.rs" was wrong for KeyMods.
  • The mouse InputMods layout is documented nowhere: the Mouse(MouseEvent) bullet names the mods field but never its bits, and the Key bullet only says it is "wider than the InputMods the mouse arm carries". docs/reference/workspace.md line 129 claims "the keyboard and capability bit tables stay in [ipc.md]", which is not true for the mouse triple.
  • So the real gap is narrower than stated: the descriptor carries no bit table (which is the cross-language concern), and the one bit table the reference does not carry is the mouse mods. I am adding the mouse triple to the Mouse bullet in a doc PR; the descriptor question stands as proposed.
Correction to section B above, after checking the reference more carefully: - The `KeyMods` bit layout *is* in the reference prose (`docs/reference/ipc.md` line 1187: ctrl `1`, shift `2`, alt `4`, super `8`), matching `messages.rs`. So "the layouts live only in messages.rs" was wrong for `KeyMods`. - The mouse `InputMods` layout is documented nowhere: the `Mouse(MouseEvent)` bullet names the `mods` field but never its bits, and the `Key` bullet only says it is "wider than the `InputMods` the mouse arm carries". `docs/reference/workspace.md` line 129 claims "the keyboard and capability bit tables stay in [ipc.md]", which is not true for the mouse triple. - So the real gap is narrower than stated: the descriptor carries no bit table (which is the cross-language concern), and the one bit table the reference does not carry is the mouse `mods`. I am adding the mouse triple to the `Mouse` bullet in a doc PR; the descriptor question stands as proposed.
natsukium commented 2026-09-22 20:38:08 +09:00
Author
Owner
Copy link

Triage (2026-09-22)

Scope narrowed to what is cheaper before the freeze than after it: items 2 (_UNSPECIFIED mode → Refused { UnknownMode } test), 3, 4, 6 (one reference sentence each), and 5 (sessions send --timeout 0 defined in cli.md and the bridge schema). One PR.

Item 1 (effective_minor on Welcome) and the follow-up findings A and B (field-level since metadata, bit tables in the descriptor) are additive minors and moved to #351. Finding C's OSC 1 support-matrix note is a doc-accuracy defect of the same kind as #349 and is folded in there; the remaining shape notes are recorded on #351.

## Triage (2026-09-22) Scope narrowed to what is cheaper before the freeze than after it: items 2 (`_UNSPECIFIED` mode → `Refused { UnknownMode }` test), 3, 4, 6 (one reference sentence each), and 5 (`sessions send --timeout 0` defined in cli.md and the bridge schema). One PR. Item 1 (`effective_minor` on `Welcome`) and the follow-up findings A and B (field-level `since` metadata, bit tables in the descriptor) are additive minors and moved to #351. Finding C's OSC 1 support-matrix note is a doc-accuracy defect of the same kind as #349 and is folded in there; the remaining shape notes are recorded on #351.
Commenting is not possible because the repository is archived.
No Branch/Tag specified
Branches Tags
main
v0.1.0-rc.9
v0.1.0-rc.8
v0.1.0-rc.7
v0.1.0-rc.6
v0.1.0-rc.5
v0.1.0-rc.4
v0.1.0-rc.3
v0.1.0-rc.2
v0.1.0-rc.1
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
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#347
No description provided.