From 4a32a7876f9123bfd5c2fe8484370efb4bdfbcd4 Mon Sep 17 00:00:00 2001 From: Marshall Yount <132798+marshally@users.noreply.github.com> Date: Sat, 13 Jun 2026 07:10:42 -0700 Subject: [PATCH] docs: add decision log appendix to ChatService trait spec Co-Authored-By: Claude Sonnet 4.6 --- ...-06-13-chat-service-trait-and-providers.md | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) diff --git a/docs/superpowers/specs/2026-06-13-chat-service-trait-and-providers.md b/docs/superpowers/specs/2026-06-13-chat-service-trait-and-providers.md index ae266fdc8..5ea5225b2 100644 --- a/docs/superpowers/specs/2026-06-13-chat-service-trait-and-providers.md +++ b/docs/superpowers/specs/2026-06-13-chat-service-trait-and-providers.md @@ -594,3 +594,182 @@ Against `docker run --name mattermost-preview -p 8065:8065 mattermost/mattermost - Multi-team routing. One `team` per Fabro server; all channels must be in that team. - Mattermost slash commands or outgoing webhooks as an alternative inbound path. - A fourth chat provider beyond Slack, Mattermost, and Teams. + +--- + +## Appendix: Decision Log + +Decisions are ordered from most architecturally significant (shapes the whole design) to minor +tactical choices. + +--- + +### 1. Introduce a `ChatService` trait rather than sibling concrete services + +**Decision:** Extract a shared `ChatService` trait that Slack, Mattermost, and Teams all +implement, rather than leaving each integration as an independent concrete struct wired +separately into `server.rs`. + +**Why:** The original Mattermost design (superseded by this one) proposed a +`MattermostService` struct sitting beside `SlackService` in `server.rs` — no shared +abstraction. Bryan Helmkamp's review noted this approach does not scale: adding MS Teams would +require a third round of identical server wiring, and provider-specific details would keep +leaking into `server.rs`. A trait enforces the boundary and makes adding future providers +additive-only changes to the server. + +**Alternatives considered:** Keeping sibling concrete services was the original design. It is +simpler in the short term but makes `server.rs` grow with each new provider and creates no +pressure to keep provider code self-contained. + +--- + +### 2. New `fabro-chat` crate for the trait, not `fabro-types` or a server-internal trait + +**Decision:** The `ChatService` trait lives in a new `lib/crates/fabro-chat/` crate. Provider +crates depend on it and implement it. `fabro-server` depends on it and holds +`Vec>`. + +**Why:** This mirrors the existing `fabro-sandbox` / `SandboxProvider` pattern exactly — +the most referenced multi-provider abstraction in the codebase. Three alternatives were +considered: + +- **Trait in `fabro-types`:** `fabro-types` is a pure-types crate with no async dependencies. + Adding async trait methods and `http::HeaderMap` would pull the crate in the wrong direction. +- **Trait in `fabro-server` (server-internal):** The trait cannot be tested outside the server, + and provider crates cannot implement it without depending on `fabro-server`, which creates a + circular dependency (`fabro-server` → `fabro-slack` → `fabro-server`). +- **New `fabro-chat` crate:** Clean layering, no circular dependencies, testable in isolation. + The new crate analogy to `fabro-sandbox` made this feel natural rather than novel. + +--- + +### 3. `ChatEventContext` trait to break the circular dependency between `fabro-chat` and `fabro-server` + +**Decision:** `handle_event` takes `&dyn ChatEventContext` instead of `&AppState`. `AppState` +implements `ChatEventContext` in `fabro-server`. + +**Why:** `handle_event` needs to fetch run projections and resolve environment variables — both +live in `AppState`. Putting `AppState` directly in the trait signature would require +`fabro-chat` to depend on `fabro-server`, which depends on `fabro-chat` — a circular dependency +Rust's crate system forbids. + +`ChatEventContext` is a narrow interface (`get_run_projection`, `resolve_env`) defined in +`fabro-chat`. `AppState` satisfies it in `fabro-server`. Provider crates never see `AppState`. +This is the standard Rust pattern for breaking large dependency cycles via trait objects — the +equivalent of defining a Ruby module in a gem that `ApplicationController` then `include`s. + +--- + +### 4. `handle_webhook` method on the trait rather than `fn router() -> Option` + +**Decision:** Each provider implements `handle_webhook(sub_path, headers, body) -> WebhookOutcome` +on the trait. The server owns a single `POST /api/v1/webhooks/:provider/*rest` route and +dispatches to the right service. Providers do not return Axum `Router` fragments. + +**Why:** Two options were considered: + +- **Option A — `fn router(&self, state: Arc) -> Option`:** Each provider + builds and returns Axum routes. Maximally flexible — providers own their full HTTP surface. + But it requires every provider crate to depend on `axum`, which today is a dependency of + `fabro-server` only. It also couples the trait to `AppState` via the signature. +- **Option B — `handle_webhook` method (chosen):** The server owns all Axum machinery. + Provider crates use `http` crate types (`HeaderMap`, status codes) but never `axum`. The + `AppState` decoupling is preserved. The existing `github_webhook_routes` function in + `server.rs` — a plain function that returns a `Router` and gets `.merge()`d in — showed that + HTTP routing is already treated as a server-level concern in this codebase. + +The `*rest` wildcard sub-path gives each provider a namespaced URL space without requiring +full router ownership. + +--- + +### 5. `Vec>` in `AppState` rather than named optional fields + +**Decision:** `AppState` holds `chat_services: Vec>` rather than +`slack_service: Option>`, `mattermost_service: Option>`, +etc. + +**Why:** Named fields require `server.rs` to be modified for every new provider — exactly the +coupling the trait exists to eliminate. A `Vec` makes adding a fourth provider a zero-change +operation in `server.rs`. Dispatch in the webhook handler and event loop iterates the vec; +`kind()` is the discriminant. This mirrors how `SandboxProviderRegistry` holds +`Vec>`. + +--- + +### 6. MS Teams included as stubs in this design, not deferred + +**Decision:** A `fabro-teams` stub crate is included in this design even though it ships no +real Teams functionality. + +**Why:** The `ChatService` trait needs to be validated against all three event-delivery shapes +before the implementation plan is written. Slack and Mattermost both have an outbound WebSocket +loop; Teams does not — it is a pure-inbound-HTTP provider. Without a Teams stub, the trait +could accidentally encode WebSocket assumptions that would require a breaking trait change when +Teams is eventually implemented. The stub makes those assumptions visible (e.g. that `start()` +must be a valid no-op) before any code is written. + +--- + +### 7. `start()` is part of the `ChatService` contract even though Teams implements it as a no-op + +**Decision:** `start()` is a required method on the trait. Teams' implementation is a +documented no-op that logs one line and returns immediately. + +**Why:** The alternative — making `start()` optional via a default method — would obscure the +contract for providers that do have an inbound connection loop. Every provider must declare how +it handles inbound events; "I have no connection to start" is a valid answer and is expressed +clearly by a no-op implementation rather than by omitting the method. A no-op that logs is +also operationally useful: it confirms in server logs that Teams is active and waiting for +inbound HTTP. + +--- + +### 8. `WebhookOutcome { status: u16, body: Option }` instead of `axum::response::Response` + +**Decision:** `handle_webhook` returns a lightweight `WebhookOutcome` struct defined in +`fabro-chat`, not an Axum `Response`. + +**Why:** Returning `axum::response::Response` from the trait would require every provider +crate to depend on `axum` — the same problem as the router fragment option (Decision 4). A +plain struct with a status code and optional body is sufficient for all three providers' needs +and is convertible to an Axum response in `fabro-server`'s dispatch handler with two lines. + +--- + +### 9. Single `FABRO_MATTERMOST_TOKEN` instead of separate bot and app tokens + +**Decision:** One vault secret covers both REST API calls and WebSocket authentication. + +**Why:** Mattermost has no equivalent of Slack's Socket Mode app token. Slack requires two +tokens because Socket Mode uses a separate app-level token to open the WebSocket, distinct from +the bot token used for API calls. Mattermost's WebSocket authentication uses the same bearer +token as the REST API. Introducing a second Mattermost token would have no purpose and would +add operator confusion. + +--- + +### 10. `on_submit` stored on `MattermostService` after `start()` rather than passed through every call + +**Decision:** `MattermostService` stores `on_submit` in an `Arc>>` field +populated when `start()` is called. `handle_webhook` reads it from the field. + +**Why:** The trait's `handle_webhook` signature does not include `on_submit` — it has no +`on_submit` parameter. Adding one would make the signature inconsistent with Slack (which never +calls `on_submit` from `handle_webhook`) and would expose a callback that most providers do not +need. Storing it after `start()` is the minimal coupling: `handle_webhook` can fire the +callback without the server needing to pass it explicitly on every HTTP request. + +--- + +### 11. Attachment tests use `insta` inline snapshots; webhook HTTP tests use `expect_axum_*` helpers + +**Decision:** Attachment JSON is asserted with `insta::assert_json_snapshot!`. HTTP status +assertions in server tests use `expect_axum_status` / `expect_axum_ok_json` from `fabro-test`. + +**Why:** The existing Fabro testing strategy (`docs/internal/testing-strategy.md`) specifies: +prefer snapshots over ad hoc assertions for structured output; use shared HTTP assertion helpers +rather than raw `assert_eq!(status, ...)`. Attachment objects are structured JSON payloads — +exactly the "good structured snapshot target" the strategy describes. The `expect_axum_*` +helpers include the request shape in failure output, making test failures easier to diagnose. +Following these conventions keeps new tests consistent with the rest of the test suite.