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

[v0.1/Proto Review] Freeze protobuf and IPC contracts (7 issues) #52

New issue
Closed
opened 2026-09-03 16:44:28 +09:00 by natsukium · 3 comments
natsukium commented 2026-09-03 16:44:28 +09:00
Owner
Copy link

Scope

This is a source-grounded pre-release review at snapshot 5077d74b. Public compatibility may still be broken, so the review focuses on contracts that become disproportionately expensive to repair after the first tag: the frozen preface semantics, protobuf shapes, correlation, connection state, minor evolution, and receiver admission.

The existing release tracker #12 already chose the major architecture: keep the frozen preface bytes, protobuf framing, a separate row codec, typed correlation, effective-minor send authorization, and connection-local corruption handling. This review does not reopen those choices. It checks whether the implementation and schema actually satisfy them.

New findings

Priority Issue Freeze boundary
P0 #45 — reject preface acceptance for an unoffered major Version negotiation must select the offered schema before any frame is decoded.
P0 #49 — bound receiver allocations from protobuf scalar claims A tiny valid protobuf body must not drive unbounded grid/image allocation.
P1 #46 — extend the shared driver through application phases The promised single validator must cover setup, attach, observer, and steady-state roles.
P1 #47 — reconcile frame-family taxonomy with routing Decide whether growth consumes a kind or a oneof arm and make mode/direction rules auditable.
P1 #48 — make correlation an exact per-arm identity Replace the request+stream optional bag with an exclusive, centrally validated identity.
P1 #50 — centralize effective-minor send authorization Turn the prose minor ledger into checked sender metadata and mixed-minor tests.
P1 #51 — make image transfers canonical and stateful Remove duplicate root encodings and enforce Header -> Chunk* -> Complete.

Findings deliberately folded into existing issues

No duplicate issue was opened for work already isolated by the release review:

  • #14: aggregate daemon/handshake admission;
  • #16: outbound frame and operation-specific payload limits;
  • #19: protobuf compatibility checks against the real CI base;
  • #20: correlated spawn and atomic create/attach;
  • #23 and #29: CLI/bridge machine contracts and schemas;
  • #24 and #26: operation identity and daemon-status resource semantics;
  • #30: reset the first public wire to protocol 2.0 after all breaking edits;
  • #44: focused SSH/carrier review.

The API/UI/control-surface pass found no additional non-duplicate freeze blocker beyond those issues. In particular, unpublished Rust visibility is not treated as a public API per #12; CLI/config/bridge changes remain owned by #23, #27–#29, and the SSH review.

Cross-cutting review conclusions

  1. The wire architecture remains sound. The frozen preface, fixed frame header, protobuf family wrappers, and opaque versioned row codec still match the project's cross-language and hot-path requirements.
  2. The weak point is semantic validation, not protobuf syntax. Prost correctly parses the bytes, but several schema-inexpressible invariants are accepted or normalized downstream even though the docs say ingress validates them once.
  3. Protocol 2.0 must be the landing point, not the first step. #30 should run after #45–#51 and the wire edits in #20/#24/#26; otherwise the supposedly clean baseline immediately accumulates another development-only break.
  4. Structural compatibility and runtime evolution need separate gates. #19 proves field/tag compatibility; #50 must prove that neither sender emits additions above the effective minor.

Recommended dependency order

  1. Decide #47 (family/arm rule) and #48 (correlation shape).
  2. Land #20, then #46 so the driver phases model the final create/attach flow.
  3. Land #49 and #51 together so image state validation uses the same receiver limits.
  4. Land #50 and its mixed-minor matrix.
  5. Resolve #45 before regenerating final preface vectors.
  6. Land #30 after every protobuf/preface/row-codec edit above.
  7. Run #19 and the full #12/#31 release gates against that exact revision.

Closure criteria

  • #45
  • #46
  • #47
  • #48
  • #49
  • #50
  • #51
  • #12 lists these P0/P1 blockers and orders #30 after them.
  • Accepted decisions are cascaded through felis.proto, protocol/domain types, reference/explanation twins, tests/golden conversations, CHANGELOG.md, and skills/felis where the public CLI/IPC surface changes.

Sources reviewed

  • crates/felis-protocol/proto/felis.proto
  • crates/felis-protocol/src/{preface,frame,codec,convert,messages}.rs and family modules
  • crates/felis-transport/src/{preface,framing,driver}.rs
  • crates/felis-daemon/src/serve.rs, serve/session_task.rs, serve/streaming.rs, graphics.rs
  • crates/felis-client-core/src/{connector,shadow,image_shadow}.rs
  • docs/reference/{spec,ipc,row-codec,cli,control-surfaces}.md
  • docs/explanation/{design,principles,non-goals,architecture/ipc}.md
  • open and superseded review issues #5, #9, #10, #12–#44
## Scope This is a source-grounded pre-release review at snapshot `5077d74b`. Public compatibility may still be broken, so the review focuses on contracts that become disproportionately expensive to repair after the first tag: the frozen preface semantics, protobuf shapes, correlation, connection state, minor evolution, and receiver admission. The existing release tracker #12 already chose the major architecture: keep the frozen preface bytes, protobuf framing, a separate row codec, typed correlation, effective-minor send authorization, and connection-local corruption handling. This review does not reopen those choices. It checks whether the implementation and schema actually satisfy them. ## New findings | Priority | Issue | Freeze boundary | |---|---|---| | P0 | #45 — reject preface acceptance for an unoffered major | Version negotiation must select the offered schema before any frame is decoded. | | P0 | #49 — bound receiver allocations from protobuf scalar claims | A tiny valid protobuf body must not drive unbounded grid/image allocation. | | P1 | #46 — extend the shared driver through application phases | The promised single validator must cover setup, attach, observer, and steady-state roles. | | P1 | #47 — reconcile frame-family taxonomy with routing | Decide whether growth consumes a kind or a oneof arm and make mode/direction rules auditable. | | P1 | #48 — make correlation an exact per-arm identity | Replace the request+stream optional bag with an exclusive, centrally validated identity. | | P1 | #50 — centralize effective-minor send authorization | Turn the prose minor ledger into checked sender metadata and mixed-minor tests. | | P1 | #51 — make image transfers canonical and stateful | Remove duplicate root encodings and enforce `Header -> Chunk* -> Complete`. | ## Findings deliberately folded into existing issues No duplicate issue was opened for work already isolated by the release review: - #14: aggregate daemon/handshake admission; - #16: outbound frame and operation-specific payload limits; - #19: protobuf compatibility checks against the real CI base; - #20: correlated spawn and atomic create/attach; - #23 and #29: CLI/bridge machine contracts and schemas; - #24 and #26: operation identity and daemon-status resource semantics; - #30: reset the first public wire to protocol 2.0 after all breaking edits; - #44: focused SSH/carrier review. The API/UI/control-surface pass found no additional non-duplicate freeze blocker beyond those issues. In particular, unpublished Rust visibility is not treated as a public API per #12; CLI/config/bridge changes remain owned by #23, #27–#29, and the SSH review. ## Cross-cutting review conclusions 1. **The wire architecture remains sound.** The frozen preface, fixed frame header, protobuf family wrappers, and opaque versioned row codec still match the project's cross-language and hot-path requirements. 2. **The weak point is semantic validation, not protobuf syntax.** Prost correctly parses the bytes, but several schema-inexpressible invariants are accepted or normalized downstream even though the docs say ingress validates them once. 3. **Protocol 2.0 must be the landing point, not the first step.** #30 should run after #45–#51 and the wire edits in #20/#24/#26; otherwise the supposedly clean baseline immediately accumulates another development-only break. 4. **Structural compatibility and runtime evolution need separate gates.** #19 proves field/tag compatibility; #50 must prove that neither sender emits additions above the effective minor. ## Recommended dependency order 1. Decide #47 (family/arm rule) and #48 (correlation shape). 2. Land #20, then #46 so the driver phases model the final create/attach flow. 3. Land #49 and #51 together so image state validation uses the same receiver limits. 4. Land #50 and its mixed-minor matrix. 5. Resolve #45 before regenerating final preface vectors. 6. Land #30 after every protobuf/preface/row-codec edit above. 7. Run #19 and the full #12/#31 release gates against that exact revision. ## Closure criteria - [ ] #45 - [ ] #46 - [ ] #47 - [ ] #48 - [ ] #49 - [ ] #50 - [ ] #51 - [ ] #12 lists these P0/P1 blockers and orders #30 after them. - [ ] Accepted decisions are cascaded through `felis.proto`, protocol/domain types, reference/explanation twins, tests/golden conversations, `CHANGELOG.md`, and `skills/felis` where the public CLI/IPC surface changes. ## Sources reviewed - `crates/felis-protocol/proto/felis.proto` - `crates/felis-protocol/src/{preface,frame,codec,convert,messages}.rs` and family modules - `crates/felis-transport/src/{preface,framing,driver}.rs` - `crates/felis-daemon/src/serve.rs`, `serve/session_task.rs`, `serve/streaming.rs`, `graphics.rs` - `crates/felis-client-core/src/{connector,shadow,image_shadow}.rs` - `docs/reference/{spec,ipc,row-codec,cli,control-surfaces}.md` - `docs/explanation/{design,principles,non-goals,architecture/ipc}.md` - open and superseded review issues #5, #9, #10, #12–#44
natsukium commented 2026-09-05 12:11:19 +09:00
Author
Owner
Copy link

Triage (2026-09-05): the 2026-09-05 proto review adds #139, #144, #146 as P1 children of this tracker (docs-freeze work, no wire change recommended); #147 is closed onto f56f5529. Closure now waits on #50 plus those three.

Triage (2026-09-05): the 2026-09-05 proto review adds #139, #144, #146 as P1 children of this tracker (docs-freeze work, no wire change recommended); #147 is closed onto `f56f5529`. Closure now waits on #50 plus those three.
natsukium commented 2026-09-07 16:09:11 +09:00
Author
Owner
Copy link

Closed (2026-09-07)

Every child of this tracker is closed and both prose closure criteria have evidence on origin/main.

Children

#45: closed by PR #56 (fix(protocol): reject preface acceptance for an unoffered major)
#46: closed by PR #84 (refactor: carry the connection's role in its phase)
#47: closed by PR #72 (refactor(protocol): make the oneof arm the routing unit)
#48: closed by PR #74 (feat(protocol): make correlation an exclusive identity), with PR #77 and PR #79 finishing the repack and the atomic create/attach
#49: closed by PR #70 (feat(protocol): admit wire claims that order a receiver's allocation)
#50: closed by PR #177 (transport: gate every send on the connection's effective minor)
#51: closed by PR #81 (feat(protocol): make image transfers canonical and stateful)
#139: closed by PR #154 (docs: state why the FRLY carrier caps are 4096 / 1 MiB)
#144: closed by PR #165 (protocol: say "no value" by absence in the session roster)
#146: closed by PR #167 (protocol: ask for a create's default grid by absence), on top of PR #165
#147: closed by decision, not by a PR — f56f5529 already implements option A (major stays 1, minor 9, ledger frozen at 0..9), so the 2.0 reset (#30) was dropped rather than landed

Prose criteria

  • "#12 lists these P0/P1 blockers and orders #30 after them." The #12 body carries #45 and #49 under "P0 — release blockers" and #46, #47, #48, #50, #51, #52 under "P1 — freeze before the first public contract"; its recommended order reads "11. #30 after every other wire edit." followed by "12. Close #52, then run #31 after all P0/P1 work is closed." The ordering held: #30 was reached last and then closed as dropped, per the #147 triage recorded above.
  • "Accepted decisions are cascaded through felis.proto, protocol/domain types, reference/explanation twins, tests/golden conversations, CHANGELOG.md, and skills/felis where the public CLI/IPC surface changes."
    • felis.proto and the domain types: every oneof field carries its // arm: declaration (crates/felis-protocol/proto/felis.proto, mirrored into crates/felis-protocol/src/generated/felis/v1/felis.v1.rs:403-412), and crates/felis-protocol/src/messages/arm.rs holds the ArmMeta table it restates. The proto comment on the image family states the state machine directly: "is Header -> Chunk* -> Complete, and no other transfer may open" (felis.proto:1117).
    • Tests: crates/felis-protocol/src/messages.rs:886-889 asserts the two against each other — "felis.proto's {kind}Msg arm declarations disagree with the ArmMeta table" — and the_schema_spells_no_arm_into_the_preface pins the preface half.
    • Reference/explanation twins: docs/reference/spec.md REQ-104 ("the connection then speaks the effective minor, min(client, daemon). The effective minor is a send-side contract") and REQ-113a ("The routing unit is the oneof arm, not the frame kind"), sourced to reference/ipc.md "The arm table" and explanation/architecture/ipc.md "Kind or arm?"; docs/reference/row-codec.md:178 carries the same send-side rule for codec versions.
    • CHANGELOG.md: "the correlation envelope is now an exclusive identity: a oneof naming either a request_id or a" (:495), "the routing unit is the oneof arm, not the frame kind" (:500), "an image transfer is canonical and stateful" (:443), and "a client closes on a preface accept that names a major it did not offer, before any frame is written" (:345).
    • skills/felis: conditional, and none of these seven changed the public CLI or bridge surface — the CLI-side cascade belongs to #23/#29 and to the CLI review tracker.

Follow-ups moved to the post-v0.1 backlog

None. No open issue is split from any child of this tracker.

## Closed (2026-09-07) Every child of this tracker is closed and both prose closure criteria have evidence on `origin/main`. ### Children #45: closed by PR #56 (fix(protocol): reject preface acceptance for an unoffered major) #46: closed by PR #84 (refactor: carry the connection's role in its phase) #47: closed by PR #72 (refactor(protocol): make the oneof arm the routing unit) #48: closed by PR #74 (feat(protocol): make correlation an exclusive identity), with PR #77 and PR #79 finishing the repack and the atomic create/attach #49: closed by PR #70 (feat(protocol): admit wire claims that order a receiver's allocation) #50: closed by PR #177 (transport: gate every send on the connection's effective minor) #51: closed by PR #81 (feat(protocol): make image transfers canonical and stateful) #139: closed by PR #154 (docs: state why the FRLY carrier caps are 4096 / 1 MiB) #144: closed by PR #165 (protocol: say "no value" by absence in the session roster) #146: closed by PR #167 (protocol: ask for a create's default grid by absence), on top of PR #165 #147: closed by decision, not by a PR — `f56f5529` already implements option A (major stays 1, minor 9, ledger frozen at 0..9), so the 2.0 reset (#30) was dropped rather than landed ### Prose criteria - **"#12 lists these P0/P1 blockers and orders #30 after them."** The #12 body carries `#45` and `#49` under "P0 — release blockers" and `#46`, `#47`, `#48`, `#50`, `#51`, `#52` under "P1 — freeze before the first public contract"; its recommended order reads "11. #30 after every other wire edit." followed by "12. Close #52, then run #31 after all P0/P1 work is closed." The ordering held: #30 was reached last and then closed as dropped, per the #147 triage recorded above. - **"Accepted decisions are cascaded through `felis.proto`, protocol/domain types, reference/explanation twins, tests/golden conversations, `CHANGELOG.md`, and `skills/felis` where the public CLI/IPC surface changes."** - `felis.proto` and the domain types: every oneof field carries its `// arm:` declaration (`crates/felis-protocol/proto/felis.proto`, mirrored into `crates/felis-protocol/src/generated/felis/v1/felis.v1.rs:403-412`), and `crates/felis-protocol/src/messages/arm.rs` holds the `ArmMeta` table it restates. The proto comment on the image family states the state machine directly: "is Header -> Chunk* -> Complete, and no other transfer may open" (`felis.proto:1117`). - Tests: `crates/felis-protocol/src/messages.rs:886-889` asserts the two against each other — "felis.proto's {kind}Msg arm declarations disagree with the ArmMeta table" — and `the_schema_spells_no_arm_into_the_preface` pins the preface half. - Reference/explanation twins: `docs/reference/spec.md` REQ-104 ("the connection then speaks the **effective minor**, `min(client, daemon)`. The effective minor is a send-side contract") and REQ-113a ("The routing unit is the oneof **arm**, not the frame kind"), sourced to `reference/ipc.md` "The arm table" and `explanation/architecture/ipc.md` "Kind or arm?"; `docs/reference/row-codec.md:178` carries the same send-side rule for codec versions. - `CHANGELOG.md`: "the correlation envelope is now an exclusive identity: a `oneof` naming either a `request_id` or a" (:495), "the routing unit is the oneof arm, not the frame kind" (:500), "an image transfer is canonical and stateful" (:443), and "a client closes on a preface accept that names a major it did not offer, before any frame is written" (:345). - `skills/felis`: conditional, and none of these seven changed the public CLI or bridge surface — the CLI-side cascade belongs to #23/#29 and to the CLI review tracker. ### Follow-ups moved to the post-v0.1 backlog None. No open issue is split from any child of this tracker.
natsukium commented 2026-09-07 16:16:40 +09:00
Author
Owner
Copy link

All seven closure children (#45–#51) are closed and #12 lists them as P0/P1 blockers with #30 ordered after them, which satisfies the last closure criterion. Deferred residue from #50 lives on as P2 follow-ups (#164, #175, #176) and is not a freeze dependency. Per #12's dependency order, #31 runs next.

All seven closure children (#45–#51) are closed and #12 lists them as P0/P1 blockers with #30 ordered after them, which satisfies the last closure criterion. Deferred residue from #50 lives on as P2 follow-ups (#164, #175, #176) and is not a freeze dependency. Per #12's dependency order, #31 runs next.
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#52
No description provided.