[v0.1/P1] Make image transfers canonical and stateful #51

Closed
opened 2026-09-03 16:43:17 +09:00 by natsukium · 1 comment
Owner

Problem

The image wire contract has two representations of the root frame and no enforced transfer state machine.

At review snapshot 5077d74b:

  • docs/reference/ipc.md says frame: None is root and Some(index) is an animation frame with index >= 1.
  • ImageMsg documents Some(i) as 0-based, the protobuf conversion accepts Some(0), and ImageShadow applies Some(0) to frames[0]. Root therefore has two valid-looking encodings with different header semantics.
  • A frame header repeats width, height, and format, but the client ignores them for animation frames because frames inherit root metadata.
  • Chunks are documented as reorderable, but the receiver tracks no covered ranges. It silently drops a chunk without a header or past the announced length, and Complete marks the buffer complete even when bytes are missing (zero fill remains).
  • ImageShadow::apply cannot return an error, despite the IPC contract saying malformed sequencing is connection-fatal and local to that connection.

The reliable ordered carrier and the current daemon producer already emit a contiguous Header -> Chunk* -> Complete sequence, so the extra ambiguity buys no current behavior.

Required decision

Redesign the protocol-2.0 image transfer into one canonical target and one enforceable state machine. Prefer shapes that make invalid combinations unrepresentable; at minimum, ingress must reject them before touching the mirror.

Questions to settle explicitly:

  1. Represent root versus animation frame as an explicit target (oneof/domain enum), with animation indices non-zero.
  2. Remove or justify metadata repeated on frame headers.
  3. Remove total_bytes and/or offset if they are derivable from dimensions and ordered delivery; otherwise define their exact consistency rules.
  4. Decide whether transfers may interleave. If not, use one active-transfer state rather than pretending random-access chunks are supported.

Acceptance criteria

  • Root has exactly one wire representation; animation frame 0 is rejected.
  • A frame cannot begin before its root exists, skip arbitrarily far ahead, or redefine inherited metadata.
  • The receiver rejects chunk-before-header, duplicate/overlapping/out-of-range chunks, a second live header for the same target, incomplete Complete, and terminal events for unknown targets.
  • Valid completion proves exactly the expected decoded byte count arrived.
  • ImageShadow::apply (or a validator in front of it) returns a typed error that the connector turns into connection-local teardown; malformed input is not silently normalized.
  • Tests cover root replacement, frame append/edit, zero-byte edge cases if retained, interleaving policy, and every malformed transition above.
  • Proto comments, domain docs, docs/reference/ipc.md, and Kitty graphics docs state the same indexing and sequencing rules.
  • Coordinate allocation limits with #49 and land the wire shape before #30 freezes protocol 2.0.
## Problem The image wire contract has two representations of the root frame and no enforced transfer state machine. At review snapshot `5077d74b`: - `docs/reference/ipc.md` says `frame: None` is root and `Some(index)` is an animation frame with `index >= 1`. - `ImageMsg` documents `Some(i)` as 0-based, the protobuf conversion accepts `Some(0)`, and `ImageShadow` applies `Some(0)` to `frames[0]`. Root therefore has two valid-looking encodings with different header semantics. - A frame header repeats `width`, `height`, and `format`, but the client ignores them for animation frames because frames inherit root metadata. - Chunks are documented as reorderable, but the receiver tracks no covered ranges. It silently drops a chunk without a header or past the announced length, and `Complete` marks the buffer complete even when bytes are missing (zero fill remains). - `ImageShadow::apply` cannot return an error, despite the IPC contract saying malformed sequencing is connection-fatal and local to that connection. The reliable ordered carrier and the current daemon producer already emit a contiguous `Header -> Chunk* -> Complete` sequence, so the extra ambiguity buys no current behavior. ## Required decision Redesign the protocol-2.0 image transfer into one canonical target and one enforceable state machine. Prefer shapes that make invalid combinations unrepresentable; at minimum, ingress must reject them before touching the mirror. Questions to settle explicitly: 1. Represent root versus animation frame as an explicit target (`oneof`/domain enum), with animation indices non-zero. 2. Remove or justify metadata repeated on frame headers. 3. Remove `total_bytes` and/or `offset` if they are derivable from dimensions and ordered delivery; otherwise define their exact consistency rules. 4. Decide whether transfers may interleave. If not, use one active-transfer state rather than pretending random-access chunks are supported. ## Acceptance criteria - Root has exactly one wire representation; animation frame 0 is rejected. - A frame cannot begin before its root exists, skip arbitrarily far ahead, or redefine inherited metadata. - The receiver rejects chunk-before-header, duplicate/overlapping/out-of-range chunks, a second live header for the same target, incomplete `Complete`, and terminal events for unknown targets. - Valid completion proves exactly the expected decoded byte count arrived. - `ImageShadow::apply` (or a validator in front of it) returns a typed error that the connector turns into connection-local teardown; malformed input is not silently normalized. - Tests cover root replacement, frame append/edit, zero-byte edge cases if retained, interleaving policy, and every malformed transition above. - Proto comments, domain docs, `docs/reference/ipc.md`, and Kitty graphics docs state the same indexing and sequencing rules. - Coordinate allocation limits with #49 and land the wire shape before #30 freezes protocol 2.0.
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

Mostly accurate, with one factual correction that changes the redesign.

  • Docs disagree: docs/reference/ipc.md:1389-1391 says frame: Some(index) addresses "animation frame index ≥ 1" and "frame 0 is the root image, which frame: None delivers"; crates/felis-protocol/src/messages/image.rs:67-69 says Some(i) is "the 0-based animation frame i"; felis.proto:993-995 says "0-based". convert/image.rs:126-133 accepts Some(0); ImageShadow::apply_frame_header (crates/felis-client-core/src/image_shadow.rs:219-242) applies Some(0) to frames[0] in place, and apply_header (:299-332) applies None by replacing the whole entry (dimensions, format, frame list reset to one). So root has two encodings with different semantics: confirmed.
  • Correction: the issue says the daemon producer emits nothing that needs the ambiguity. It does. A root-frame edit (a=f r=1, a=c c=1) goes through images.replace_frame(id, idx) with idx == 0 and emits ImageEvent::TransmitFrame { index: 0 } (crates/felis-daemon/src/graphics.rs:1440-1452, :1636-1645), which frame_sync_messages (:186-218) turns into Header { frame: Some(0) }, Chunk { frame: Some(0) }, Complete { frame: Some(0) }. The client test editing_the_displayed_frame_redirties (image_shadow.rs:723-735) pins exactly this. The two encodings therefore carry two behaviors today: None = fresh transmission (replace image, drop animation frames), Some(0) = edit the root's pixels in place (keep frames). "Reject frame 0" as written would break a=f r=1 / a=c c=1; the redesign must keep the distinction under a shape that has one encoding per meaning.
  • Frame headers repeat width/height/format and the client ignores them (apply_frame_header takes only total_bytes): confirmed. Every frame is a full coalesced canvas (docs/reference/protocols/kitty-graphics.md:191-198), so total_bytes == width * height * bpp for root and frames alike (graphics.rs:156, :203 derive it from the stored buffer).
  • Chunks: image.rs:71-72 documents "any order"; apply_chunk (:334-350) and apply_frame_chunk (:244-265) track no coverage and silently drop chunk-before-header, out-of-range, and (for frames) unknown index; apply_complete (:352-362) marks complete with whatever arrived ("A short transmission leaves trailing zeros; marked complete anyway"). apply returns () (:136), while the IPC contract says malformed sequencing is connection-fatal (docs/reference/ipc.md:446-455). Confirmed.
  • The producer is contiguous and sequential (image_sync_messages, graphics.rs:142-181: Header → Chunk* → Complete for the root, then each frame in order), the carrier is ordered, and the client applies in event_handler.rs:563 in arrival order. No interleaving exists.

Verdict

accept-with-changes: the decision is requested. Recommend:

  1. Target as a oneof, with Kitty's 1-based frame numbering. ImageHeader { id, oneof target { ImageNew new = 2; ImageFrame frame = 3; } } where ImageNew { width, height, format } starts a fresh image (replaces any prior entry and its frames) and ImageFrame { number: uint32 } addresses frame number ≥ 1 of an existing image: 1 is the root in place (today's Some(0)), N > 1 edits or appends (append iff N == frames.len() + 1). This matches the reference's own frame model (kitty-graphics.md:193-194: "frames[0] is the root ... Kitty frame number 1") and ShowFrame should switch to the same 1-based number so one numbering appears on the wire (ShowFrame.index is 0-based today, image.rs:136). Frame 0 becomes unrepresentable-as-valid (unset oneof / zero number is malformed), satisfying "animation frame 0 is rejected" without losing the root-edit behavior.
  2. Drop repeated metadata on frames: ImageFrame carries only number; dimensions and format are inherited and cannot be restated.
  3. Drop total_bytes and offset. Expected bytes are width * height * bpp (ImageFormat::bytes_per_pixel, image.rs:19-27) for every target; chunks are ordered and append. The receiver counts received bytes against expected. This also removes the u64 total_bytes → vec![0; n] allocation from a tiny frame that #49 objects to (image_shadow.rs:310-311, :226-227): the allocation is bounded by the shared dimension cap #49 introduces.
  4. No interleaving. One Option<ActiveTransfer { id, target, expected, received }> per connection on the receiver; Chunk { id, bytes } and Complete { id } must name the active transfer's id (keep id as a cross-check; a mismatch is malformed). A second Header while a transfer is live is malformed. Zero-length chunks: allow as a no-op (they cost nothing to accept; the producer never emits one, chunked at graphics.rs:127-134).

Why this rather than keeping optional frame with ≥ 1: the oneof makes "new image" and "frame" different shapes with different required fields, so the invalid combinations (a frame with dimensions, a new image with a number) cannot be written; and it turns the ImageShadow match frame { Some, None } split (:145-161) into a match target with no zero case.

Approach

Schema/domain (felis-protocol): felis.proto:977-1018 (new ImageHeader, ImageChunk { id, bytes }, ImageComplete { id }, ImageShowFrame { id, number }), messages/image.rs:56-146 (ImageMsg::Header { id, target: ImageTarget }, enum ImageTarget { New { width, height, format }, Frame { number: NonZeroU32 } }), convert/image.rs (reject unset target, zero number, UNSPECIFIED format), image.rs doc comments :48-54, :67-85. Round-trip cases at image.rs:185-289 updated; add rejection tests.

Receiver (felis-client-core/src/image_shadow.rs): apply(&mut self, msg) -> Result<(), ImageShadowError> with variants ChunkWithoutHeader, HeaderWhileActive, FrameForUnknownImage, FrameNumberSkipsAhead { number, have }, ChunkOverrun { expected, got }, IncompleteComplete { expected, received }, TerminalForUnknownTarget, DimensionsOverCap (from #49's shared limit). Replace the three apply_* pairs with one ActiveTransfer state; apply_complete asserts received == expected. Placements naming an unknown image stay tolerated (the comment at :131-135 documents why). Callers: crates/felis-client/src/event_handler.rs:563 maps Err to the same close path forward_frame uses for a driver error (main.rs:1784-1786, "daemon frame refused; closing the connection"); the bridge does not mirror images (confirm with grep ImageShadow in felis-cli: none).

Producer (felis-daemon/src/graphics.rs:142-218): emit New for image_sync_messages, Frame { number: idx + 1 } for frame_sync_messages; ShowFrame { number: current + 1 } at :176-179, :292; TransmitFrame's dedup key (:97) already uses index + 1. The producer is already sequential; add a debug assertion in materialize_image_events that no Header is queued while a prior transfer's Complete is pending (cheap, catches a future interleaving producer).

Tests: shadow unit tests for root replacement (New twice), frame append (number == len + 1), frame edit (number ≤ len), root edit (number == 1), skip-ahead (number == len + 2 → error), chunk-before-header, second header while active, overrun, short Complete, terminal for an unknown id, id mismatch mid-transfer, zero-length chunk no-op, and a daemon-side test that a full image_sync_messages for a two-frame image applies cleanly on the shadow (round trip through the codec). serve/streaming.rs:967 and session_task.rs:2352 match on ImageMsg::Complete { .. } and keep compiling.

Docs: docs/reference/ipc.md:1367-1401 (new shapes, ordered chunks, one transfer at a time, the malformed list, expected-byte rule), docs/reference/protocols/kitty-graphics.md:191-200, :253-259 (1-based numbers on the wire, ShowFrame), docs/explanation/protocols/kitty-graphics.md "Frame storage (pre-coalesced)" if it mentions the wire triple, docs/explanation/architecture/ipc.md a short "Image transfers are one at a time" paragraph with the rejected alternatives (offset-addressed chunks: unneeded on an ordered carrier and unverifiable without coverage tracking; 0-based optional frame: two encodings of the root). felis.proto comments. CHANGELOG under the 2.0 wire entry. skills/felis: none (no CLI surface).

Dependencies

#49 first (or together): the expected-byte computation needs the shared dimension/byte caps #49 places below daemon/client policy (felis-protocol), and #49's "frame index growth" item is subsumed by the skip-ahead rule here. Land before #30. Independent of #47/#48 (Image is uncorrelated and one-way). #52's order (item 3) holds.

Risk/effort

M (2 days). Main risk: the 1-based ShowFrame renumbering touches the daemon animation timer and the client's current index (image_shadow.rs:283-297); an off-by-one there shows as the wrong frame on reattach, so add the reattach round-trip test above. Secondary: rehydrate ships every frame of every image through the same sequential state, so a rehydrate that is interrupted by a Delete for the active image must be defined (recommend: Delete of the active transfer's image aborts the transfer cleanly, tested).

Labels

Keep priority/P1, release/v0.1.0.

## 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 Mostly accurate, with one factual correction that changes the redesign. - Docs disagree: `docs/reference/ipc.md:1389-1391` says `frame: Some(index)` addresses "animation frame `index ≥ 1`" and "frame 0 is the root image, which `frame: None` delivers"; `crates/felis-protocol/src/messages/image.rs:67-69` says `Some(i)` is "the 0-based animation frame `i`"; `felis.proto:993-995` says "0-based". `convert/image.rs:126-133` accepts `Some(0)`; `ImageShadow::apply_frame_header` (`crates/felis-client-core/src/image_shadow.rs:219-242`) applies `Some(0)` to `frames[0]` in place, and `apply_header` (`:299-332`) applies `None` by replacing the whole entry (dimensions, format, frame list reset to one). So root has two encodings with different semantics: confirmed. - **Correction:** the issue says the daemon producer emits nothing that needs the ambiguity. It does. A root-frame edit (`a=f r=1`, `a=c c=1`) goes through `images.replace_frame(id, idx)` with `idx == 0` and emits `ImageEvent::TransmitFrame { index: 0 }` (`crates/felis-daemon/src/graphics.rs:1440-1452`, `:1636-1645`), which `frame_sync_messages` (`:186-218`) turns into `Header { frame: Some(0) }`, `Chunk { frame: Some(0) }`, `Complete { frame: Some(0) }`. The client test `editing_the_displayed_frame_redirties` (`image_shadow.rs:723-735`) pins exactly this. The two encodings therefore carry two *behaviors* today: `None` = fresh transmission (replace image, drop animation frames), `Some(0)` = edit the root's pixels in place (keep frames). "Reject frame 0" as written would break `a=f r=1` / `a=c c=1`; the redesign must keep the distinction under a shape that has one encoding per meaning. - Frame headers repeat `width`/`height`/`format` and the client ignores them (`apply_frame_header` takes only `total_bytes`): confirmed. Every frame is a full coalesced canvas (`docs/reference/protocols/kitty-graphics.md:191-198`), so `total_bytes == width * height * bpp` for root and frames alike (`graphics.rs:156`, `:203` derive it from the stored buffer). - Chunks: `image.rs:71-72` documents "any order"; `apply_chunk` (`:334-350`) and `apply_frame_chunk` (`:244-265`) track no coverage and silently drop chunk-before-header, out-of-range, and (for frames) unknown index; `apply_complete` (`:352-362`) marks complete with whatever arrived ("A short transmission leaves trailing zeros; marked complete anyway"). `apply` returns `()` (`:136`), while the IPC contract says malformed sequencing is connection-fatal (`docs/reference/ipc.md:446-455`). Confirmed. - The producer is contiguous and sequential (`image_sync_messages`, `graphics.rs:142-181`: Header → Chunk* → Complete for the root, then each frame in order), the carrier is ordered, and the client applies in `event_handler.rs:563` in arrival order. No interleaving exists. ## Verdict **accept-with-changes**: the decision is requested. Recommend: 1. **Target as a oneof, with Kitty's 1-based frame numbering.** `ImageHeader { id, oneof target { ImageNew new = 2; ImageFrame frame = 3; } }` where `ImageNew { width, height, format }` starts a fresh image (replaces any prior entry and its frames) and `ImageFrame { number: uint32 }` addresses frame `number ≥ 1` of an existing image: `1` is the root in place (today's `Some(0)`), `N > 1` edits or appends (append iff `N == frames.len() + 1`). This matches the reference's own frame model (`kitty-graphics.md:193-194`: "`frames[0]` is the root ... Kitty frame number 1") and `ShowFrame` should switch to the same 1-based `number` so one numbering appears on the wire (`ShowFrame.index` is 0-based today, `image.rs:136`). Frame `0` becomes unrepresentable-as-valid (unset oneof / zero number is malformed), satisfying "animation frame 0 is rejected" without losing the root-edit behavior. 2. **Drop repeated metadata on frames**: `ImageFrame` carries only `number`; dimensions and format are inherited and cannot be restated. 3. **Drop `total_bytes` and `offset`.** Expected bytes are `width * height * bpp` (`ImageFormat::bytes_per_pixel`, `image.rs:19-27`) for every target; chunks are ordered and append. The receiver counts received bytes against expected. This also removes the `u64 total_bytes → vec![0; n]` allocation from a tiny frame that #49 objects to (`image_shadow.rs:310-311`, `:226-227`): the allocation is bounded by the shared dimension cap #49 introduces. 4. **No interleaving.** One `Option<ActiveTransfer { id, target, expected, received }>` per connection on the receiver; `Chunk { id, bytes }` and `Complete { id }` must name the active transfer's id (keep `id` as a cross-check; a mismatch is malformed). A second `Header` while a transfer is live is malformed. Zero-length chunks: allow as a no-op (they cost nothing to accept; the producer never emits one, `chunked` at `graphics.rs:127-134`). Why this rather than keeping `optional frame` with `≥ 1`: the oneof makes "new image" and "frame" different shapes with different required fields, so the invalid combinations (a frame with dimensions, a new image with a number) cannot be written; and it turns the `ImageShadow` `match frame { Some, None }` split (`:145-161`) into a `match target` with no zero case. ## Approach Schema/domain (`felis-protocol`): `felis.proto:977-1018` (new `ImageHeader`, `ImageChunk { id, bytes }`, `ImageComplete { id }`, `ImageShowFrame { id, number }`), `messages/image.rs:56-146` (`ImageMsg::Header { id, target: ImageTarget }`, `enum ImageTarget { New { width, height, format }, Frame { number: NonZeroU32 } }`), `convert/image.rs` (reject unset target, zero number, `UNSPECIFIED` format), `image.rs` doc comments `:48-54`, `:67-85`. Round-trip cases at `image.rs:185-289` updated; add rejection tests. Receiver (`felis-client-core/src/image_shadow.rs`): `apply(&mut self, msg) -> Result<(), ImageShadowError>` with variants `ChunkWithoutHeader`, `HeaderWhileActive`, `FrameForUnknownImage`, `FrameNumberSkipsAhead { number, have }`, `ChunkOverrun { expected, got }`, `IncompleteComplete { expected, received }`, `TerminalForUnknownTarget`, `DimensionsOverCap` (from #49's shared limit). Replace the three `apply_*` pairs with one `ActiveTransfer` state; `apply_complete` asserts `received == expected`. Placements naming an unknown image stay tolerated (the comment at `:131-135` documents why). Callers: `crates/felis-client/src/event_handler.rs:563` maps `Err` to the same close path `forward_frame` uses for a driver error (`main.rs:1784-1786`, "daemon frame refused; closing the connection"); the bridge does not mirror images (confirm with grep `ImageShadow` in `felis-cli`: none). Producer (`felis-daemon/src/graphics.rs:142-218`): emit `New` for `image_sync_messages`, `Frame { number: idx + 1 }` for `frame_sync_messages`; `ShowFrame { number: current + 1 }` at `:176-179`, `:292`; `TransmitFrame`'s dedup key (`:97`) already uses `index + 1`. The producer is already sequential; add a debug assertion in `materialize_image_events` that no `Header` is queued while a prior transfer's `Complete` is pending (cheap, catches a future interleaving producer). Tests: shadow unit tests for root replacement (`New` twice), frame append (`number == len + 1`), frame edit (`number ≤ len`), root edit (`number == 1`), skip-ahead (`number == len + 2` → error), chunk-before-header, second header while active, overrun, short `Complete`, terminal for an unknown id, id mismatch mid-transfer, zero-length chunk no-op, and a daemon-side test that a full `image_sync_messages` for a two-frame image applies cleanly on the shadow (round trip through the codec). `serve/streaming.rs:967` and `session_task.rs:2352` match on `ImageMsg::Complete { .. }` and keep compiling. Docs: `docs/reference/ipc.md:1367-1401` (new shapes, ordered chunks, one transfer at a time, the malformed list, expected-byte rule), `docs/reference/protocols/kitty-graphics.md:191-200`, `:253-259` (1-based numbers on the wire, `ShowFrame`), `docs/explanation/protocols/kitty-graphics.md` "Frame storage (pre-coalesced)" if it mentions the wire triple, `docs/explanation/architecture/ipc.md` a short "Image transfers are one at a time" paragraph with the rejected alternatives (offset-addressed chunks: unneeded on an ordered carrier and unverifiable without coverage tracking; 0-based optional frame: two encodings of the root). `felis.proto` comments. CHANGELOG under the 2.0 wire entry. `skills/felis`: none (no CLI surface). ## Dependencies #49 first (or together): the expected-byte computation needs the shared dimension/byte caps #49 places below daemon/client policy (`felis-protocol`), and #49's "frame index growth" item is subsumed by the skip-ahead rule here. Land before #30. Independent of #47/#48 (`Image` is uncorrelated and one-way). #52's order (item 3) holds. ## Risk/effort **M** (2 days). Main risk: the 1-based `ShowFrame` renumbering touches the daemon animation timer and the client's `current` index (`image_shadow.rs:283-297`); an off-by-one there shows as the wrong frame on reattach, so add the reattach round-trip test above. Secondary: rehydrate ships every frame of every image through the same sequential state, so a rehydrate that is interrupted by a `Delete` for the active image must be defined (recommend: `Delete` of the active transfer's image aborts the transfer cleanly, tested). ## Labels Keep `priority/P1`, `release/v0.1.0`.
Sign in to join this conversation.
No description provided.