From 90b911c92775652fff70cc5151a12c53c190836a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 20 Apr 2026 18:26:46 -0400 Subject: [PATCH] docs(plan): clarify unknown-event handling in client extraction Document that unknown Fabro run events already fall back through EventBody::Unknown, and that the refactor only needs to preserve that behavior during the EventEnvelope move. --- ...20-002-refactor-extract-fabro-client-crate-plan.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md b/docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md index 931b9d497..9a431d4f5 100644 --- a/docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md +++ b/docs/plans/2026-04-20-002-refactor-extract-fabro-client-crate-plan.md @@ -57,7 +57,7 @@ These were all reasonable while the client had exactly one caller. They prevent - Writing a second consumer of `fabro-client` (SDK example, IDE integration, etc.) — the crate exists to enable that, not to deliver it. - Changing wire semantics (OpenAPI contract stays identical). - Migrating existing `~/.fabro/auth.json` entries. The canonical key format for HTTP targets remains identical (same `normalized_http_base_url` logic); Unix-socket canonicalization changes from `fs::canonicalize` (symlink-resolving) to lexical-only (`.`/`..` cleanup, no symlink chasing). Existing sessions against socket paths written as absolute non-symlinked paths continue to match. Users with symlinked socket paths may need to re-login — acceptable per `CLAUDE.md` ("We don't care about migration"). -- (Previously noted as out of scope; now in scope per review feedback.) `RunEvent` unknown-variant handling: add a fallback `Unknown` variant so older CLIs against newer servers survive deserialization. Covered under Unit 2. +- `RunEvent` unknown-variant handling. Already present today via `EventBody::Unknown` at `lib/crates/fabro-types/src/run_event/mod.rs:695` (covered by `:974`). Unit 2 verifies the `EventEnvelope` restructure preserves this existing behavior — no new variant, no new code. - Splitting the ~40 client methods into separate `client/runs.rs`, `client/secrets.rs`, etc. modules. A single `client.rs` is acceptable at ~900 lines; cosmetic domain splitting can happen later if the file grows. ## Context & Research @@ -107,7 +107,7 @@ None. Internal refactor; the patterns are all established locally. ``` On refresh failure modes that previously triggered fallback, `OAuthSession` calls `fallback.resolve()`. If `Some(Credential)`, the session rebuilds the client bundle with that credential; if `None`, it surfaces "session expired." The CLI provides a concrete impl in `fabro-cli` that wraps the existing dev-token-loading logic. Named trait (vs bare `Fn` closure) makes the role obvious at the `OAuthSession::builder` call site. -- **`fabro_types::RunEvent` gains unknown-variant tolerance.** Add `#[serde(other)] Unknown` (or `#[serde(untagged)] Unknown(serde_json::Value)` depending on the enum shape) so a newer server's unknown event type doesn't break an older CLI at deserialize. Preserves today's accidental forward-compat that came from storing events as raw JSON. Small addition to the event enum; benefit extends beyond this plan (any event-consuming path becomes version-mismatch-tolerant). +- **`fabro_types::RunEvent` already tolerates unknown event names.** `EventBody::Unknown { name, properties }` at `lib/crates/fabro-types/src/run_event/mod.rs:695` is the existing fallback — a newer server's unknown event type deserializes into this variant without error. Unit 2's responsibility is to *preserve* this behavior across the `EventEnvelope { seq, #[serde(flatten)] event: RunEvent }` restructure, not to add a new variant. - **Phased delivery.** DTO lift must land before the client extraction — otherwise `fabro-client`'s deps can't be correct. We split the work into three phases (DTO lift → `fabro-client` extraction → `fabro-cli` rewire) and land each phase as a coherent chunk. Each phase compiles and tests pass at its boundary. ## Open Questions @@ -117,7 +117,7 @@ None. Internal refactor; the patterns are all established locally. - **Where do `apply_events`/`apply_event` live after `RunProjection` moves?** In `fabro-store` as an extension trait `RunProjectionReducer` implemented on `fabro_types::RunProjection`. Preserves OOP-style method-call syntax at call sites via `use fabro_store::RunProjectionReducer;`. Chosen over free functions because the workspace leans toward methods over free helpers. - **HTTP canonicalization scope in `ServerTarget::from_url`.** Preserve today's full normalization: lowercase scheme + lowercase host + strip default ports + strip trailing `/` + strip `/api/v1` suffix + rebuild authority. Matches today's `canonical_http_target` in `auth_store.rs:350-383` and the existing `https_normalization_collapses_equivalent_urls` test at line 507. Simpler "strip slash + api-v1 only" form was rejected because it would silently invalidate AuthStore entries for users whose URLs differed only in case or default-port. - **OAuthSession refresh fallback mechanism.** Add a `CredentialFallback` trait; `fabro-cli` implements it against its dev-token sources; `OAuthSession` holds `Option>` and calls `fallback.resolve()` on the failure modes that previously fell back to dev-token. Named trait (not bare `Fn`) chosen for role clarity. -- **`RunEvent` unknown-variant forward-compat.** Add an `Unknown` fallback variant in `fabro_types::RunEvent` so the `EventEnvelope` restructure doesn't regress today's accidental pass-through tolerance for unknown event types. Addressed in Unit 2. +- **`RunEvent` unknown-variant forward-compat.** Already handled today via `EventBody::Unknown` at `lib/crates/fabro-types/src/run_event/mod.rs:695` (tested at `:974`). Unit 2 verifies the `EventEnvelope` restructure preserves this existing behavior — no new variant, no new code. - **Does `EventEnvelope` need `EventPayload` to travel with it?** No. OpenAPI already defines the wire shape as `seq + RunEvent flattened`. `EventPayload` is a storage-internal validation helper and stays behind. - **Does `AuthStore` need a trait-based abstraction?** No. Concrete type with a configurable file path is sufficient; we extract a trait the day a second implementation exists. - **Does `ArtifactUpload` live in `fabro-types` or `fabro-client`?** `fabro-types`. Source is `fabro-workflow` (capture) → sink is `fabro-client` (upload). Placing the DTO in `fabro-types` prevents `fabro-workflow` from having to depend on `fabro-client`. @@ -301,7 +301,6 @@ Client::builder() Construction sites (switch to `fabro_types::EventEnvelope { seq, event }`): - Create: `lib/crates/fabro-types/src/event_envelope.rs` - Modify: `lib/crates/fabro-types/src/lib.rs` (module + re-export) -- Modify: `lib/crates/fabro-types/src/run_event/` (add `Unknown` fallback variant — exact form depends on current enum tagging) - Modify: `lib/crates/fabro-store/src/types.rs` (delete old `EventEnvelope`; keep `EventPayload` internal; update tests at lines 127 and 171) - Modify: `lib/crates/fabro-store/src/slate/run_store.rs` (production constructors at lines 194 and 342) - Modify: `lib/crates/fabro-store/src/run_state.rs` (apply_event body at line 79; test fixtures at lines 728, 741, 1086, 1112; test assertions at lines 1133, 1137) @@ -343,7 +342,7 @@ Write-path `EventPayload::new` sites (UNAFFECTED — stay in fabro-store as inte pub event: RunEvent, } ``` -- Add unknown-variant tolerance to `fabro_types::RunEvent` so the read-path typed deserialization doesn't regress today's forward-compat pass-through behavior. Shape depends on `RunEvent`'s current enum tagging — likely a catch-all `Unknown(serde_json::Value)` variant with `#[serde(other)]` or an `#[serde(untagged)]` fallback. Inspect the existing `fabro_types::RunEvent` definition during implementation and pick the minimal form that preserves lossless round-trip of unknown payloads. +- Confirm today's `EventBody::Unknown { name, properties }` fallback (at `lib/crates/fabro-types/src/run_event/mod.rs:695`) continues to round-trip through the new `EventEnvelope { seq, #[serde(flatten)] event: RunEvent }` shape — the `#[serde(flatten)]` interacts cleanly with the existing catch-all. No new variant is added; we are only confirming the existing forward-compat behavior survives. - `fabro-store` keeps `EventPayload` strictly internal — every existing write path continues to call `EventPayload::new(value, run_id)?` for `expected_run_id` validation before persisting (unchanged behavior). No write path bypasses that check. Only the external-facing read-path return type changes. - At the store's read boundary (`slate/run_store.rs:194, 342` — where the old `EventEnvelope { seq, payload }` was constructed by pairing a separately-tracked seq with decoded payload bytes): deserialize the raw bytes as `RunEvent` (the on-disk format is raw RunEvent JSON without `seq`), then wrap in `fabro_types::EventEnvelope { seq, event }`. The write path at `slate/run_store.rs:201-203` continues to persist `serde_json::to_vec(payload)?` of the `EventPayload`; the `seq` comes from a separate counter, same as today. - CLI callers that did `event.payload.as_value()` + `RunEvent::from_ref(...)` → change to `&event.event`. This is a win at every call site (fewer conversions). @@ -357,7 +356,7 @@ Write-path `EventPayload::new` sites (UNAFFECTED — stay in fabro-store as inte - Happy path: `EventEnvelope` round-trips through `serde_json` preserving both `seq` and all `RunEvent` fields, and deserializes the existing wire format identically. - Integration: existing `fabro-store` test `wire_event_envelope_round_trips` still passes after porting (adapt it to construct the new struct shape). - Integration: CLI integration tests that consume events over SSE or HTTP (e.g., `fabro-cli/tests/it/workflow/mod.rs`, `cmd/attach.rs`) continue to pass — proves the wire shape is unchanged. -- Forward-compat: an `EventEnvelope` JSON payload whose `type` (or equivalent discriminator) is unknown to this build of `fabro-types` deserializes into a `RunEvent::Unknown` fallback variant without error, and re-serializes losslessly. +- Forward-compat regression guard: an `EventEnvelope` JSON payload whose event name is unknown to this build of `fabro-types` deserializes into the existing `EventBody::Unknown { name, properties }` without error and re-serializes losslessly. (Covers: the `EventEnvelope` restructure did not break today's fallback at `run_event/mod.rs:695`.) **Verification:** - `cargo build --workspace` succeeds.