[v0.1/P1] Centralize effective-minor send authorization #50
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
natsukium/felis#50
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
The effective minor is documented as a hard send-side authorization rule, but the implementation enforces it through scattered feature-specific branches.
Current examples include:
check_scope_minorfor narrowed switch scope;daemon_statusrefusal;geometry_refusal,exited_refusal, andsession_limit_refusaldowngrade helpers;ConnectionDriverandFrameWriterdo not know an outgoing message's minimum minor. Adding one new send site without a branch can send an unknown oneof arm or enum value to an older peer and force a connection-fatal decode. #19's protobuf breaking check cannot detect this behavioral error because the schema can remain additively compatible.Required change
Make the minor ledger executable metadata and route outgoing additions through one checked send boundary on both peers. A feature's fallback must be explicit: omit a field, down-map to an older typed value, refuse locally, or do not emit the message.
Acceptance criteria
since_minormetadata next to its protocol definition.Why this is separate
#19 checks schema compatibility against the CI base. This issue checks runtime sender authorization and downgrade behavior, which structural protobuf tooling does not prove.
Triage plan (2026-09-03)
Source-grounded triage against
mainat69076d42, reviewed through seven rounds of an independent reviewer (pisol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision.Claim check
Accurate against HEAD.
docs/reference/ipc.md:1689-1708,docs/explanation/architecture/ipc.md:850-881,docs/reference/spec.md:54(REQ-104),preface.rs:35-39, andminor_defines(preface.rs:264-270) exists as "the send-side gate every minor addition asks".check_scope_minor(crates/felis-client-core/src/connector.rs:138-156, used at:952,:971, and by the bridgecli_bridge.rs:2067-2085); clientdaemon_status(:996-1005); daemongeometry_refusal,session_limit_refusal,exited_refusal(crates/felis-daemon/src/serve.rs:1012-1039, used at:475,:496,:594).minor_definesitself has no non-test caller (grep: onlypreface.rs:584-586).MINOR_ENV_BASE(preface.rs:72) is declared but the client'senv_basesend path does not gate on it (grepMINOR_ENV_BASEfinds only the declaration), which is a live instance of the "one new send site without a branch" hazard; harmless only because an older daemon ignores the unknown field.ConnectionDriverholds no minor (driver.rs:176-184);FrameWriter(framing.rs) holds none; the minor lives onConnection.effective_minor(connector.rs:170) andConnParams.effective_minor(serve.rs:172-178) as a plainu16that each feature reads by hand.docs/reference/ipc.md:1745-1752; proto comments atfelis.proto:209,215,222,523,578,1097,1155,1204). #19'sbuf breaking(buf.yamlWIRE_JSON) cannot see any of this.connector.rs:2077-2164,cli_bridge.rs:2067-2085,serve/tests.rsaround geometry) but no matrix.One thing the issue underestimates: after #30, the ledger has one row (minor 0) and every gate above is deleted. The mechanism will ship with zero live rows, so its tests must run on a synthetic fixture.
Verdict
accept-with-changes. Build the mechanism, but size it to the 2.0 baseline: an executable
since_minorper arm and per closed-enum value, one send gate that reads it, an explicitDowngradehook for the enum-value case, and a test fixture that proves the gate with a synthetic addition, since at 2.0 there is nothing real to gate. Drop the "every ledger row exercised in both directions" matrix from the v0.1.0 scope: there are no rows; keep it as the template the first post-2.0 minor must fill (record that in the explanation as the review-gate step).Approach
Metadata (
felis-protocol, on top of #47'sArmMeta):since_minor: u16per arm (already a column in #47). For closed enums that grow values (AttachFailure,StreamErrorReason,RefusalReason,ResourceKind, ...) addfn since_minor(self) -> u16via the existingdata_free_enum!macro (convert.rs:141-167) extended with an optional@sinceper variant (default 0). Fields that change behavior get no runtime metadata: a field is protobuf-tolerant on receive, so its gate is the sender choosing to omit it; record such fields in the ledger only, with the test in step 5 checking the count. Row-codec versions:felis-grid's writer already carries a version tag (docs/reference/row-codec.md); exposeROW_CODEC_SINCE: &[(u8 /*codec*/, u16 /*minor*/)]infelis-protocol/src/row.rs.felis.proto:since=Nin thearm:comment line (#47) and a// since minor Non enum values; the schema-walking test asserts they match the Rust metadata.Send gate (
felis-transport):FrameWritergainseffective_minor: u16set once after the preface (daemon:serve.rs:302-305afterexchange_bootstrap; client:handshake_overafter:302;from_halvestakes it).send::<M: Directed>/send_correlatedcheckmsg.meta().since_minor <= effective_minorand return a typedTransportError::MinorTooOld { arm, needs, effective }(never panic; the daemon logs and drops the connection, the client surfacesConnectError::MinorTooOld, which already exists atconnector.rs:149). Every write in the workspace already goes throughFrameWriter::send*(grepwrite_frame(callers:connector.rs:315forHello, which is minor 0). The daemon's fan-out path (session_task.rsOutEvent→ writer) goes through the same writer, soGrid/Imagearms are covered too.trait Downgrade { fn for_minor(self, effective: u16) -> Option<Self>; }implemented per closed enum (returnsSome(older value)when a documented down-map exists,Nonewhen the sender must refuse locally). At 2.0 every impl is the identity; the threeserve.rs:1012-1039helpers are the shape the first real impl takes, and are deleted by #30. The writer cannot see enum values inside a body, so the call is explicit at the construction site; the executable ledger makes forgetting it visible: step 5.Tests and CI:
felis-protocoltest parses the ledger table indocs/reference/ipc.md(rows after| Minor |) and asserts: number of rows == maxsince_minorover arms/values/codecs + 1, and every arm/value withsince_minor > 0appears by name in its row. Same pattern asthe_protocol_version_matches_the_schema(preface.rs:758-777). This is the "CI checks prose and metadata cover the same additions" criterion, run bycargo testinpr.yml.#[cfg(test)]family with an arm atsince_minor = 1(or a test-onlyArmMetaoverride) provingFrameWriterrefuses it at effective 0 and passes at 1, in bothSides; aDowngradefixture proving the explicit-refuse and down-map branches. Keep the existing per-feature tests until #30 deletes their subjects.felis daemon statusprintsPROTOCOL_MINOR(CHANGELOG entry for minor 5;skills/felis/SKILL.md:220), the connection carrieseffective_minor; add a doc sentence, no code.Docs:
docs/reference/ipc.md:1738-1752ledger gets a lead-in stating the table is machine-checked and what each row must name (arm/value/codec identifiers as they appear infelis.proto);docs/explanation/architecture/ipc.md:883-899"The ledger is the review gate" gains the executable half and the rejected alternative (gating inConnectionDriver::decodeon the receive side, rejected because the receiver cannot know what the sender should have withheld;buf breaking, rejected as structural only);docs/reference/testing.mdnames the synthetic-fixture test. CHANGELOG: none (no user-visible change at 2.0 beyond error strings).skills/felis: none.Dependencies
After #47 (metadata home) and #30 (clean baseline, so the mechanism is not built around minor 1-5 gates that #30 deletes). Order in #12 (item 10: #50 then #29) and #52 (item 4, before #30) conflict; #12's reading is right for the mechanism (build on 2.0), while the deletion of the old gates is #30's. Recommend: #30 lands first, then #50 adds the mechanism on the 2.0 tree, then #29's golden conversations. #19 is independent.
Risk/effort
M (2 days). Main risk: the enum-value gate is by construction not enforceable in the writer (values are inside the body), so the ledger-vs-metadata test in step 5 is the only thing that catches a forgotten
Downgradecall; make that test's failure message name the value and the call site pattern.Labels
Keep
priority/P1,release/v0.1.0for the gate and metadata (the freeze boundary includes "evolution rules"). If the release date is under pressure, the mixed-minor golden matrix (criterion 4) can move to post-v0.1 without weakening the contract: at 2.0 there is nothing to exercise.Review amendments (round 1)
send*" claim is wrong. Production callers ofFrameWriter::write_frameat HEAD:felis-daemon/src/serve.rs:1421(write_event_frame, the fan-out of pre-encodedOutEventbodies),serve.rs:1899(ConnMsgwrites),felis-client/src/main.rs:1738(the client's outgoing queue writer),felis-client-core/src/connector.rs:316(Hello). Correction: makewrite_framepub(crate)in felis-transport (tests use a#[doc(hidden)]write_frame_unchecked), and route all four through the authorizedsend*boundary. The fan-out path carries only(kind, body), sostreaming::body_formust also return the arm'sArmMeta(or at leastsince_minor) alongside the body; the client queue item carries the same. The workspace compiles only when no production caller bypasses the gate, which is the enforcement.Review amendments (round 2)
AuthorizeForMinor(a recursivefn check(&self, effective: u16) -> Result<(), MinorViolation>) derived by the same schema-walking metadata asArmMeta: it checks the arm'ssince_minor, every present field with asince_minorabove 0 (eitherSome/non-default), every closed-enum value'ssince_minor, and the row-codec version. The ordinary writer API (send,send_unflushed,send_correlated) calls it before encoding; a violation isTransportError::MinorTooOld { path, needs, effective }.Downgradeis no longer optional call-site work: a sender that wants to talk to an older minor either downgrades through the trait or gets the typed error.write_frameispub(crate); the unchecked entry point exists only under acfg(feature = "test-util")(orcfg(test)) gate, never as a#[doc(hidden)]public item. The fan-out path (serve.rs:1421) and the client queue (felis-client/src/main.rs:1738) carry aCheckedFramenewtype that can only be constructed by the checked encoder (private constructor in felis-transport), so pre-encoded bodies are unforgeable.streaming::body_forreturns aCheckedFrame.since_minor = 1field and enum value in the test fixture; sending them at effective minor 0 through every public writer method fails withMinorTooOld; the same fixture at effective minor 1 succeeds; a compile-fail test (trybuild) showswrite_frameis unreachable from a downstream crate.