refactor(gateway): extract source plugin trait and common config validation

Eliminates ~400 lines of duplication across http.rs, streams.rs, and
the 10 resolve_* functions in config.rs by introducing:

- A gateway-internal SourcePlugin trait with enum dispatch (unit
  structs + monomorphised entry points, zero vtables) replacing
  the 9-arm if-let chains in http.rs and streams.rs.

- A shared resolve_common() helper in config.rs extracting the
  four repeated validation blocks (subject_prefix, stream_name,
  nats_ack_timeout, stream_max_age) that appeared in every
  per-source resolver.

- Nine thin wrapper impls (GithubPlugin, SlackPlugin, etc.) that
  delegate to the existing source crate routers and provision
  fns. No source crate code or dependency changes needed.

Discord is intentionally excluded from SourcePlugin: its
webhook-less primary path is handled by the gateway runner
spawned in main.rs, which stays untouched.

All 606+ workspace tests pass unchanged.

Author: enilsen16

PLUGIN_SYSTEM_PLAN.md

@@ -0,0 +1,276 @@
+# Source-Plugin Refactor Plan
+
+**Worktree:** `/Users/enilsen/Projects/TrogonStack/trogonai-plugin-system-dev`
+**Branch:** `plugin-system-dev` (currently equal to `main`)
+**Archive of prior attempt:** branch `plugin-system-wip-archive` (broken, do not merge)
+**Authored:** 2026-05-14
+
+---
+
+## 1. Why this plan replaces the previous one
+
+The previous `PLUGIN_SYSTEM_PLAN.md` (now on `plugin-system-wip-archive`) over-fits the problem. It frames the work as building a "plugin marketplace" — discovery, dynamic `.so/.dll/.dylib` loading, permission models, sandboxing, signed plugins, hot reload, health-monitoring background tasks, plus a `ConfigSchema` DSL with `ConfigField`/`ValidationRule` types. None of that is needed for 10 first-party source connectors that ship in the same binary. The implementation attempt produced ~2500 lines of new infrastructure to eliminate ~500 lines of real duplication, and the trait/registry/caller signatures don't even line up across layers (concrete `NatsJetStreamClient` vs `Arc<dyn JetStreamPublisher>` vs `impl JetStreamPublisher`). The archive branch contains two non-matching plugin impls per source and runs the legacy `nest("/github", ...)` chain in parallel with the registry — a guaranteed runtime `.nest()` panic.
+
+The original plan also references `crates/trogon-std/src/source_plugin.rs` as if it existed. It doesn't.
+
+This plan starts from the **real, measured** duplication and proposes the minimum abstraction that removes it.
+
+---
+
+## 2. The actual problem (measured)
+
+Three files on `main`:
+
+| File | Total lines | Duplicated dispatch / boilerplate |
+|---|---|---|
+| `crates/trogon-gateway/src/http.rs` | 145 | ~36 lines (9× 4-line `if let Some(ref cfg)` blocks calling `nest()`). Discord skipped (uses a gateway runner, not a webhook). |
+| `crates/trogon-gateway/src/streams.rs` | 165 | ~40 lines (10× 4-line `if let Some(ref cfg)` blocks calling `provision()`). |
+| `crates/trogon-gateway/src/config.rs` | 2677 | ~340 lines across 10 `resolve_X` functions. Each is ~60-70 lines; about 34 lines per function are pure copy-paste (subject_prefix / stream_name / nats_ack_timeout_secs / stream_max_age_secs validation). The rest is per-source secret types and source-specific fields (e.g. Discord `gateway_intents`, Linear `timestamp_tolerance_secs`, Sentry's 1s ack-timeout cap). |
+
+Total real duplication: ~400 lines. Not a "marketplace" problem — a trait-and-helper problem.
+
+Discord is a partial exception: it has both a webhook path (`http.rs`/`streams.rs`) **and** a long-running gateway runner spawned in `main.rs`. The runner is a separate concern and should not contort the trait.
+
+---
+
+## 3. Goals & explicit non-goals
+
+### Goals
+1. Replace the 10-arm `if let Some(ref cfg)` chains in `http.rs` and `streams.rs` with one iteration over a registry.
+2. Eliminate the ~340 lines of common-field validation duplication in `config.rs`.
+3. Source crates own their own typed config and webhook router (no behavioral change for any source).
+4. Adding an 11th source = implementing one trait + one line in the registry, not editing three gateway files.
+5. The diff compiles, `cargo test --workspace` passes, no behavioral regression.
+
+### Non-goals (explicit — these are what blew up the previous plan)
+- ❌ Dynamic plugin loading (`.so`/`.dll`/`.dylib` / `libloading`). Static linking is fine.
+- ❌ Plugin discovery / `plugin.toml` manifests / filesystem scanning.
+- ❌ Permission model, sandboxing, signature verification.
+- ❌ Hot reload, plugin lifecycle states (`Starting`/`Stopped`/`Failed`), health-monitoring background task.
+- ❌ `ConfigSchema` DSL (`ConfigField`, `ConfigFieldType`, `ValidationRule`). The existing typed config structs already do this in Rust's type system.
+- ❌ Separate `WebhookHandler` trait split from `SourcePlugin`. One trait. Routes are part of what a webhook source does.
+- ❌ `catch_unwind` "failure isolation". Panics in first-party source crates are bugs to fix, not isolate; and `catch_unwind` around an `async` block only catches panics during future *construction*, not execution — the reasoning is wrong anyway.
+- ❌ Any new workspace crate (e.g. `trogon-plugin`, `trogon-source-api`). The trait is small enough to live in the gateway crate at `crates/trogon-gateway/src/source_plugin.rs`. A separate crate also creates a circular dependency since the trait takes `&ResolvedConfig`, which is defined in the gateway.
+- ❌ Replacing the typed `ResolvedConfig { github: Option<...>, slack: Option<...>, ... }` with a `HashMap<String, ResolvedSourceConfig> + serde_json::Value` bag. The typed-per-source shape is correct; we only need a *registry* over it.
+
+---
+
+## 4. Proposed design
+
+### 4.1 Where things live
+
+```
+crates/trogon-gateway/src/
+├── source_plugin.rs       (NEW, ~120 lines: trait, enum dispatch, wrapper impls)
+├── config.rs              (MODIFIED: extract resolve_common(), keep per-source resolve_X)
+├── http.rs                (MODIFIED: iterate over enum variants instead of 9 if-let arms)
+├── streams.rs             (MODIFIED: iterate over enum variants instead of 10 if-let arms)
+└── main.rs                (no source-crate registration needed — enum lives in source_plugin.rs)
+```
+
+**No new workspace crates.** The trait is gateway-internal. Source crates stay untouched — their `Cargo.toml` files and `lib.rs` files do not change. The gateway's `source_plugin.rs` holds one wrapper struct per source (e.g., `struct GithubPlugin;`) that delegates to the source crate's existing `router()` and `provision()` functions.
+
+Why no new crate: putting the trait in a `trogon-source-api` crate (as one earlier draft suggested) creates a circular-dependency problem — the trait methods need to take `&crate::config::ResolvedConfig`, which lives in `trogon-gateway`. The trait would have to either duplicate the config type or take generic associated types, both worse than keeping it inside the gateway.
+
+### 4.2 The trait + enum dispatch (recommended approach)
+
+Use **enum dispatch**, not `Box<dyn SourcePlugin>`. The trait stays generic over the publisher/context types so the existing `MockJetStreamPublisher` / `MockJetStreamContext` mocks keep working unchanged. The enum collapses the runtime choice without needing `dyn`-safety.
+
+```rust
+// crates/trogon-gateway/src/source_plugin.rs
+
+use axum::Router;
+use trogon_nats::jetstream::{
+    ClaimCheckPublisher, JetStreamContext, JetStreamPublisher, ObjectStorePut,
+};
+
+use crate::config::ResolvedConfig;
+
+/// Stable identifier for a source. Matches the TOML key and the URL prefix.
+pub type SourceId = &'static str;
+
+/// One source connector (one webhook receiver). Discord's *gateway runner*
+/// is NOT a SourcePlugin — see §6.
+pub trait SourcePlugin: Send + Sync {
+    fn id(&self) -> SourceId;
+    fn is_enabled(&self, resolved: &ResolvedConfig) -> bool;
+
+    async fn provision<C: JetStreamContext>(
+        &self,
+        js: &C,
+        resolved: &ResolvedConfig,
+    ) -> Result<(), C::Error>;
+
+    fn routes<P, S>(
+        &self,
+        publisher: ClaimCheckPublisher<P, S>,
+        resolved: &ResolvedConfig,
+    ) -> Router
+    where
+        P: JetStreamPublisher,
+        S: ObjectStorePut;
+}
+
+// Wrappers. Each is a unit struct that delegates to the source crate's
+// existing pub fns. No changes to the source crates themselves.
+pub struct GithubPlugin;
+pub struct SlackPlugin;
+pub struct TelegramPlugin;
+pub struct TwitterPlugin;
+pub struct GitlabPlugin;
+pub struct IncidentioPlugin;
+pub struct LinearPlugin;
+pub struct NotionPlugin;
+pub struct SentryPlugin;
+// (Discord is intentionally absent: its webhook-less primary path is the
+// gateway runner in main.rs. See §6.)
+
+impl SourcePlugin for GithubPlugin {
+    fn id(&self) -> SourceId { "github" }
+    fn is_enabled(&self, r: &ResolvedConfig) -> bool { r.github.is_some() }
+    async fn provision<C: JetStreamContext>(
+        &self, js: &C, r: &ResolvedConfig,
+    ) -> Result<(), C::Error> {
+        if let Some(cfg) = &r.github { trogon_source_github::provision(js, cfg).await } else { Ok(()) }
+    }
+    fn routes<P, S>(&self, publisher: ClaimCheckPublisher<P, S>, r: &ResolvedConfig) -> Router
+    where P: JetStreamPublisher, S: ObjectStorePut,
+    {
+        r.github.as_ref()
+            .map(|cfg| trogon_source_github::router(publisher, cfg))
+            .unwrap_or_else(Router::new)
+    }
+}
+// ...similar for the other 8.
+
+/// Iterate the registry. Generic-method dispatch resolves at compile time
+/// per concrete (P, S, C) — no vtables.
+pub fn for_each_plugin<F: FnMut(&dyn ErasedIter)>(_f: F) {
+    // see below — actually iterate concretely
+}
+```
+
+**Iterating without `dyn`:** since the methods are generic, you can't store the plugins in a `Vec<Box<dyn SourcePlugin>>`. Two viable patterns — pick one:
+
+- **Pattern A (tuple + macro): preferred.** A small `for_each_source!` macro that expands to a sequence of statements over `(GithubPlugin, SlackPlugin, …)`. Each call site (provision loop, mount loop) writes the macro invocation. Adding a source = adding one entry to the macro list. ~15 lines for the macro. **This is the recommended pattern.**
+- **Pattern B (explicit fns):** `pub fn provision_all<C>(js: &C, r: &ResolvedConfig) -> ...` and `pub fn mount_all<P, S>(router: Router, publisher: ..., r: &ResolvedConfig) -> Router` each containing the 9 calls in sequence (no `if let` — `is_enabled` gates inside each wrapper). Slightly more boilerplate, slightly clearer in stack traces.
+
+Either way, `http.rs` shrinks from 9 if-let arms to one call (`source_plugin::mount_all(router, publisher, &config)`); `streams.rs` similarly shrinks to one call.
+
+**Why not `dyn`:** It would require concrete-typed methods (e.g. `&NatsJetStreamClient`, `ClaimCheckPublisher<Arc<dyn JetStreamPublisher>, Arc<dyn ObjectStorePut>>`), which forces blanket impls of the publisher/context traits for `Arc<dyn T>` that this codebase doesn't currently have. Adding those blanket impls is doable but unrelated work; enum/macro dispatch sidesteps the question for now. If the project ever wants runtime-loaded plugins, revisit then.
+
+### 4.3 Common-field validation extraction
+
+Add to `config.rs`, around line ~470 (above the per-source resolve fns):
+
+```rust
+struct ResolvedCommon {
+    subject_prefix: NatsToken,
+    stream_name: NatsToken,
+    stream_max_age: StreamMaxAge,
+    nats_ack_timeout: NonZeroDuration,
+}
+
+fn resolve_common(
+    source: &'static str,
+    subject_prefix: String,
+    stream_name: String,
+    nats_ack_timeout_secs: u64,
+    stream_max_age_secs: u64,
+    errors: &mut Vec<ConfigValidationError>,
+) -> Option<ResolvedCommon> {
+    // Move the 4 repeated match blocks here. Return None on any failure
+    // (errors pushed). Returns Some only when all four validate.
+}
+```
+
+Each `resolve_X` shrinks from ~65 lines to ~20-25 lines (status check + source-specific secret/field + `let common = resolve_common(...)?;` + final struct).
+
+Net deletion in `config.rs`: ~250 lines. (Less than 340 because each call site keeps a few lines, and we add ~30 lines of helper.)
+
+### 4.4 Source crates stay untouched
+
+All `impl SourcePlugin` blocks live in `gateway/src/source_plugin.rs` and delegate to the existing `pub fn router(...)` and `pub fn provision(...)` of each source crate. No source crate's `Cargo.toml` or `lib.rs` needs to change — this is purely a gateway-internal refactor. If `gateway/src/source_plugin.rs` grows past ~300 LoC, revisit and consider moving impls into source crates as a follow-up; behavior would be identical.
+
+---
+
+## 5. Phases
+
+Each phase is a small, mergeable PR.
+
+### Phase 1 — Trait + GitHub-only spike (1 day)
+1. In `gateway/src/source_plugin.rs` (new file): define the `SourcePlugin` trait (§4.2), the `GithubPlugin` wrapper, and the dispatch entry points (`provision_all` / `mount_all` containing just one call each for now).
+2. In `gateway/src/main.rs`: no change yet (the dispatch fns are called from `http.rs` / `streams.rs`).
+3. In `http.rs`: **replace** the `if let Some(ref cfg) = config.github { app = app.nest(...) }` arm with a call to `source_plugin::mount_all(app, publisher, &config)` (which internally only mounts GitHub for now). Other 8 arms untouched.
+4. In `streams.rs`: **replace** the `if let Some(ref cfg) = config.github { ... }` arm with `source_plugin::provision_all(client, config).await?;` (which internally only provisions GitHub). Other 9 arms untouched.
+5. **Do not leave both paths active for GitHub.** This is the trap the archive fell into.
+6. **Verify:** `cargo build --workspace --all-targets` clean, `cargo test --workspace` green. The existing `mount_sources_with_all_sources_builds_router` and `provision_all_sources_creates_all_streams` tests must still pass byte-identical results.
+7. **PR boundary:** open and merge this PR before starting Phase 2. The diff is small (~200 LoC), reviewable, zero behavioral change. Phase 2 must rebase on the merged Phase 1 — do **not** stack all four phases on one branch.
+
+### Phase 2 — Migrate the remaining 8 (~1 hour each, ship separately or batched per ~3)
+Discord has **no** webhook route on `main` (verified: `http.rs` has 9 if-let arms, Discord is not among them — only the gateway runner spawn in `main.rs`). Do not add Discord to `SourcePlugin`.
+
+Order (least → most exotic):
+- slack, gitlab, telegram, twitter (pure webhook + signing secret pattern, identical to GitHub)
+- linear (adds `timestamp_tolerance_secs`)
+- notion (`verification_token` not a secret)
+- sentry (1s ack-timeout cap — preserve the cap logic in `resolve_sentry`)
+- incidentio (`signing_secret` + `timestamp_tolerance_secs`)
+
+For each: add the wrapper to `gateway/src/source_plugin.rs`, add to `build_registry()`, delete the corresponding if-let arm from `http.rs`/`streams.rs`.
+
+**Verify after each:** existing tests still pass. Don't batch — migrate, test, commit, repeat. The gateway's `mount_sources_with_all_sources_builds_router` test is the canary.
+
+### Phase 3 — Extract `resolve_common()` (half day)
+Refactor `config.rs` per §4.3. Pure code motion; no behavior change. All existing `config.rs` tests must still pass byte-for-byte.
+
+### Phase 4 — Cleanup (half day)
+- Delete `config.rs` value-object newtypes that have become unused (GitHub gating its own newtype in `trogon-source-github` is the existing pattern — verify none of the moved ones are needed elsewhere).
+- Update any docs in `crates/trogon-gateway/AGENTS.md` referencing the dispatch chain.
+- Final `cargo clippy --workspace -- -D warnings` (workspace already sets `all = "deny"`).
+
+---
+
+## 6. Discord's gateway runner — leave alone
+
+`main.rs` spawns `trogon_source_discord::gateway_runner::run(...)` as a long-running task. **Do not** try to model this through `SourcePlugin`. It's a different lifecycle (long-running, no HTTP route, no JetStream stream provisioning at the same shape). Keep the explicit spawn in `main.rs`. Mention this in a one-line comment so the next reader knows it's intentional.
+
+If a future need arises to abstract multi-runtime sources (e.g., Discord-style WebSocket connectors), add a separate trait (`LongRunningSource` or similar) at that point. Not now.
+
+---
+
+## 7. Definition of done
+
+1. `cargo build --workspace --all-targets` clean on `plugin-system-dev`.
+2. `cargo test --workspace` passes (same test count or higher than `main`).
+3. `cargo clippy --workspace --all-targets -- -D warnings` clean.
+4. `http.rs` and `streams.rs` each contain at most one explicit reference to a specific source crate (the discord gateway-runner spawn lives in `main.rs`, not these files).
+5. Adding a hypothetical 11th source = ~50 lines: a `router()` and `provision()` in the source crate (existing pattern), a wrapper in `gateway/src/source_plugin.rs`, one line in `build_registry()`, and a `resolve_<x>` in `config.rs` that calls `resolve_common()`.
+6. `config.rs` is shorter than on `main` (target: -200 LoC or better).
+7. **No new top-level crates.**
+8. No new dependencies added to any `Cargo.toml`.
+9. PRs ship in the order in §5. Each is independently revertable.
+
+---
+
+## 8. For the implementing model — don't do these things
+
+These are the specific failure modes from the prior attempt; flagging them so they're not repeated:
+
+1. **Don't put both the new path and the legacy path side-by-side** ("Use plugin registry to mount routes" *then* keep the `if let Some(ref cfg) = config.github { app = app.nest("/github", ...) }` block "for sources not yet migrated"). That double-mounts at runtime → `nest()` panics on the second call. Either fully migrate a source or leave it alone for the PR; never both.
+
+2. **Don't define the trait with concrete types in one place and `dyn`-erased generics elsewhere.** Pick one and stick to it. See §4.2.
+
+3. **Don't write two separate `impl SourcePlugin` for one source** (the archive has `trogon-source-github/src/plugin.rs` AND `plugin_adapter.rs`, neither of which matches the trait).
+
+4. **Don't add `catch_unwind` around `async` blocks** and call it "failure isolation". It doesn't catch panics inside the future. Don't add panic isolation at all — see non-goals.
+
+5. **Don't replace the typed `ResolvedConfig { github: Option<...>, ... }` with a `HashMap<String, ResolvedSourceConfig>` of `serde_json::Value` bags.** The typed shape is a feature, not duplication.
+
+6. **Don't introduce a `WebhookHandler` trait separate from `SourcePlugin`.** One trait. If you can't fit routes into the trait without coupling to Axum, just couple to Axum — the gateway uses Axum already; this trait is gateway-internal.
+
+7. **Don't create a `ConfigSchema` / `ConfigField` / `ValidationRule` runtime DSL.** Rust's type system is the schema.
+
+8. **Don't add any new workspace crate.** The trait lives in `crates/trogon-gateway/src/source_plugin.rs`. A separate crate for the trait creates a circular dependency with `ResolvedConfig`.
+
+9. **Reset to `main` before starting.** The archive on `plugin-system-wip-archive` is for reference only — do not cherry-pick from it. The trait shapes there are wrong.