refactor(gateway): extract source plugin trait and resolve_common()#161
Conversation
WalkthroughIntroduces a ChangesSourcePlugin abstraction and gateway integration
Sequence Diagram(s)sequenceDiagram
participant GatewayInit
participant ProvisionDispatch as provision_webhook_sources()
participant Plugin as SourcePlugin
participant JetStream
participant MountDispatch as mount_webhook_sources()
participant Router as axum::Router
GatewayInit->>ProvisionDispatch: (client, config)
loop each plugin in [GitHub...Sentry]
ProvisionDispatch->>Plugin: provision(client, config)
Plugin->>JetStream: provision_integrations(callback, config)
end
GatewayInit->>MountDispatch: (base_router, publisher, config)
loop each plugin in [GitHub...Sentry]
MountDispatch->>Plugin: mount(router, publisher, config)
Plugin->>Router: mount_integrations(mount_fn, config)
Router->>Router: nest("/sources/{id}/{integration}/webhook", ...)
end
MountDispatch-->>GatewayInit: fully_nested_router
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR SummaryLow Risk Overview
Per-source behavior is preserved: Reviewed by Cursor Bugbot for commit f1cb201. Bugbot is set up for automated code reviews on this repo. Configure here. |
enilsen16
force-pushed
the
plugin-system-dev
branch
from
a9ebf3e to
dc07fc2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/source_plugin.rs (1)
435-435: 💤 Low valueRemove unnecessary clone on the final mount_one call.
The last call to
mount_oneclonespublisher, but since this is the final use, you can move it instead. This avoids an unnecessary clone operation.♻️ Proposed fix
- mount_one(&SentryPlugin, router, publisher.clone(), resolved) + mount_one(&SentryPlugin, router, publisher, resolved)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs` at line 435, The final call to mount_one(&SentryPlugin, router, publisher.clone(), resolved) performs an unnecessary clone of publisher; change this call to move the publisher (mount_one(&SentryPlugin, router, publisher, resolved)) since this is the last use—ensure no subsequent code expects publisher afterwards, and if earlier mounts needed clones keep those as-is (only remove .clone() on the final mount_one invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 481-531: The resolve_common function bundles four distinct
conversions (subject_prefix via NatsToken::new, stream_name via NatsToken::new,
nats_ack_timeout via NonZeroDuration::from_secs, and stream_max_age via
StreamMaxAge::from_secs) and should be split into small per-field resolver
functions (e.g., resolve_subject_prefix, resolve_stream_name,
resolve_nats_ack_timeout, resolve_stream_max_age) that each return Result<... ,
ConfigValidationError> or Option<...> and push errors into the shared errors
vec; then have resolve_common (or the callers of ResolvedCommon) compose those
per-field resolvers to construct ResolvedCommon so validation is isolated by
type and easier to test/maintain.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs`:
- Line 435: The final call to mount_one(&SentryPlugin, router,
publisher.clone(), resolved) performs an unnecessary clone of publisher; change
this call to move the publisher (mount_one(&SentryPlugin, router, publisher,
resolved)) since this is the last use—ensure no subsequent code expects
publisher afterwards, and if earlier mounts needed clones keep those as-is (only
remove .clone() on the final mount_one invocation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d13776bb-abb5-4e2f-b89b-a838d2651d70
📒 Files selected for processing (6)
PLUGIN_SYSTEM_PLAN.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/source_plugin.rsrsworkspace/crates/trogon-gateway/src/streams.rs
|
@enilsen16 you have some merge conflicts and unresolved comments 😢 |
enilsen16
force-pushed
the
plugin-system-dev
branch
from
dc07fc2 to
847d1e5
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 847d1e5. Configure here.
enilsen16
force-pushed
the
plugin-system-dev
branch
from
044e396 to
624bdac
Compare
…spatch Eliminates the per-source iteration repeated between `http.rs::mount_sources` and `streams.rs::provision` by introducing a gateway-internal `SourcePlugin` trait. Each webhook source has a unit-struct plugin (GithubPlugin, SlackPlugin, ..., MicrosoftGraphPlugin) that owns both its JetStream provisioning loop and its HTTP route mounting loop, including any per-source edge cases. Two dispatch entry points -- `provision_webhook_sources` and `mount_webhook_sources` -- replace the repeated source-by-source dispatch blocks. Adding a new webhook source now touches one file (source_plugin.rs) instead of two. SlackPlugin's `mount` skips socket-mode-only integrations (no webhook config) since their HTTP route would never receive traffic; the runner is spawned in main.rs. Discord is intentionally excluded from `SourcePlugin` because its primary path is a WebSocket gateway runner, not a webhook receiver -- streams.rs still provisions it inline. All 579 gateway tests pass; full workspace tests pass. Signed-off-by: Erik Nilsen <enilsen16@live.com>
Collapses the per-source iteration that was repeated in every non-Slack
`SourcePlugin` impl into two shared helpers:
- `provision_integrations` — `AsyncFn`-bounded helper iterating
`Vec<SourceIntegration<T>>` and calling each source's `provision` per
integration.
- `mount_integrations` — closure helper iterating the same and nesting
each integration's router at `{path_prefix}/{id}`.
Each non-Slack `provision` impl is now a single call to
`provision_integrations(...)`; each non-Slack `mount` impl is a single
call to `mount_integrations(...)`. The iteration / nesting / logging
pattern lives in one place instead of nine. `SlackPlugin::mount` stays
hand-written because its socket-mode webhook filter is a per-source
behavior that the helper deliberately doesn't generalize.
This addresses the Cursor Bugbot finding on the prior commit ("10
identical copies"). Restores the closure-helper pattern that existed on
main (`mount_webhook_integrations`) and adds the symmetric helper for
provisioning that main was missing.
All 579 gateway tests pass.
Signed-off-by: Erik Nilsen <enilsen16@live.com>
coverage-action with publish: true always tries to push to _xml_coverage_reports, but fork PRs get a read-only GITHUB_TOKEN regardless of the permissions block. Gate publish on push events so fork PRs skip the write step entirely. pull-requests: write is kept for PR annotations. Signed-off-by: Erik Nilsen <enilsen16@live.com>
enilsen16
force-pushed
the
plugin-system-dev
branch
from
624bdac to
9bfac32
Compare
Reverts the conditional publish expression per yordis's review. Signed-off-by: Erik Nilsen <enilsen16@live.com>
Signed-off-by: Erik Nilsen <enilsen16@live.com>
enilsen16
force-pushed
the
plugin-system-dev
branch
from
933a0e9 to
f1cb201
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs`:
- Around line 47-62: The generic bound on provision_integrations is using the
unsupported `AsyncFn(...) -> ...` form; update the function signature so the
closure type F is declared with the stable async bound syntax: change the where
clause to `F: async Fn(&C, &T) -> Result<(), C::Error>` (keeping the other
bounds `C: JetStreamContext`) so the async callable is properly typed when
calling `provision_fn(client, &integration.config).await` inside
provision_integrations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77a1f2cb-e8ed-4165-99e2-65bcc543a02a
📒 Files selected for processing (4)
rsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/source_plugin.rsrsworkspace/crates/trogon-gateway/src/streams.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-gateway/src/streams.rs
- rsworkspace/crates/trogon-gateway/src/main.rs
| async fn provision_integrations<C, T, F>( | ||
| integrations: &[SourceIntegration<T>], | ||
| source: SourceId, | ||
| client: &C, | ||
| provision_fn: F, | ||
| ) -> Result<(), C::Error> | ||
| where | ||
| C: JetStreamContext, | ||
| F: AsyncFn(&C, &T) -> Result<(), C::Error>, | ||
| { | ||
| for integration in integrations { | ||
| provision_fn(client, &integration.config).await?; | ||
| info!(source, integration = integration.id.as_str(), "stream provisioned"); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Rust AsyncFn trait bound syntax stabilized 1.85
💡 Result:
In Rust 1.85.0, the AsyncFn, AsyncFnMut, and AsyncFnOnce traits were stabilized as part of the support for async closures [1][2][3]. However, the specific "async fn" trait bound syntax—such as using where F: async Fn() as a shorthand in trait bounds—was not stabilized in Rust 1.85.0 [4][5]. Instead, these traits are used explicitly in bounds (e.g., where F: AsyncFn<()>) [2][3]. The "async" modifier in trait bounds remains gated behind the async_trait_bounds feature flag [4]. Key details regarding this stabilization include: - Async Closures: Rust 1.85.0 introduced async closures (e.g., async || {}) and the corresponding AsyncFn* trait hierarchy, which allows for futures that can borrow from the closure's environment [6][3]. - Trait Availability: The AsyncFn, AsyncFnMut, and AsyncFnOnce traits were added to the standard library prelude in Rust 1.85.0, making them available for use without explicit imports [1][5]. - Bound Syntax: While the traits themselves are stable, the shorthand async Fn() syntax for bounds was explicitly excluded from the 1.85.0 release to avoid blocking the stabilization of async closures while RFCs regarding the exact semantics of that bound modifier (such as whether it implies Future or IntoFuture) continued to be discussed [4][5][7].
Citations:
- 1: https://github.com/rust-lang/rust/releases/tag/1.85.0
- 2: https://aarambhdevhub.medium.com/rust-1-85-0-a-comprehensive-guide-to-the-latest-rust-release-263245f28d7e
- 3: https://blog.rust-lang.org/2025/02/20/Rust-1.85.0/
- 4: Gate async fn trait bound modifier on
async_trait_boundsrust-lang/rust#132612 - 5: Add
AsyncFn*to the prelude in all editions rust-lang/rust#132611 - 6: https://dev.to/kent-tokyo/rust-in-2025-2026-from-most-loved-language-to-core-infrastructure-4l5k
- 7: Delay stabilizing async closures to consider if they should return
impl IntoFutureinstead ofimpl Futurerust-lang/rust#135664
🌐 Web query:
Rust 1.85 std::ops::AsyncFn trait bound syntax F: AsyncFn(&C, &T) -> ... exact form
💡 Result:
In Rust 1.85, the correct syntax for using the AsyncFn trait as a bound is the async modifier applied to the standard Fn trait syntax, such as async Fn(&C, &T) -> R [1][2]. While the underlying standard library trait is named std::ops::AsyncFn [3][4], the language provides a special "async" bound modifier that allows you to write trait bounds as async Fn(...) [1]. This desugars to the AsyncFn trait hierarchy [1]. The exact form for the bound you requested is: F: async Fn(&C, &T) -> R Key details regarding this syntax: 1. Bound Modifier: The async keyword acts as a bound modifier on the parenthesized Fn trait bound [1]. 2. Desugaring: Writing F: async Fn(...) is the idiomatic way to bound a generic type to the AsyncFn family of traits [1][2]. The compiler handles the mapping to AsyncFn, AsyncFnMut, or AsyncFnOnce based on the usage [5][2]. 3. Why this syntax: This approach resolves limitations in previous versions of Rust where it was difficult to properly express higher-ranked function signatures (HRTBs) that return futures, especially when those futures need to borrow from the closure's captures [5][2]. The trait std::ops::AsyncFn is part of the standard library prelude as of Rust 1.85 [6][7], enabling first-class support for async closures [2].
Citations:
- 1: https://rust-lang.github.io/rfcs/3668-async-closures.html
- 2: https://blog.rust-lang.org/2025/02/20/Rust-1.85.0/
- 3: https://doc.rust-lang.org/1.85.0/std/ops/trait.AsyncFn.html
- 4: https://doc.rust-lang.org/stable/std/ops/trait.AsyncFn.html
- 5: https://dev.to/kent-tokyo/rust-in-2025-2026-from-most-loved-language-to-core-infrastructure-4l5k
- 6: https://github.com/rust-lang/rust/releases/tag/1.85.0
- 7: https://releases.rs/docs/1.85.0/
🌐 Web query:
std::ops::AsyncFn trait definition associated types how to write bounds in trait bound list
💡 Result:
The AsyncFn trait is an async-aware version of the standard Fn trait, defined in std::ops [1][2]. Its definition includes a method async_call, which returns a Generic Associated Type (GAT) named CallRefFuture [1][2]. Definition of AsyncFn: pub trait AsyncFn: AsyncFnMut where Args: Tuple { extern "rust-call" fn async_call(&self, args: Args) -> Self::CallRefFuture<'>; } Regarding Associated Types and Bounds: The associated types of the AsyncFn* traits (such as CallRefFuture, CallMutFuture, and CallOnceFuture) are considered implementation details and are generally not nameable by users in stable Rust [3]. Consequently, you cannot directly reference or place trait bounds on these associated types (e.g., you cannot write something like F: AsyncFn<CallRefFuture: Send>) [3]. Instead, the AsyncFn traits are intended to be used via the async bound modifier, which allows you to express trait bounds on functions and closures that return futures [3]. The syntax async Fn* -> T desugars to an AsyncFn* trait bound [3]. If you need to place specific bounds on the futures returned by an async function—for instance, requiring that the returned future is Send—the recommended approach in Rust is to use Return-Position Impl Trait in Trait (RPITIT) [4][5]. By defining a trait method that returns impl Future + Send + ', you can explicitly enforce such bounds [4][5]. Summary of Key Points: 1. Associated Type Privacy: The associated types of AsyncFn are not part of the stable public interface and cannot be named directly [3]. 2. Trait Bound Syntax: To bound functions that return futures, use the async Fn* syntax (e.g., F: async Fn -> Output) [3]. 3. Constraining Returned Futures: To add bounds (like Send) to the future returned by a trait method, define the method to return impl Future<Output = T> + Send + '_ [4][5].
Citations:
- 1: https://doc.rust-lang.org/std/ops/trait.AsyncFn.html
- 2: https://doc.rust-lang.org/src/core/ops/async_function.rs.html
- 3: https://rust-lang.github.io/rfcs/3668-async-closures.html
- 4: https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits/
- 5: https://blog.rust-lang.org/inside-rust/2022/11/17/async-fn-in-trait-nightly/
Fix AsyncFn bound syntax in provision_integrations
In Rust 1.85, the stable syntax for bounding an async callable is F: async Fn(&C, &T) -> R (it desugars to the std::ops::AsyncFn* traits). The current bound F: AsyncFn(&C, &T) -> Result<(), C::Error> doesn’t use the required async bound modifier and is not the supported stable form—switch it to F: async Fn(&C, &T) -> Result<(), C::Error>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rsworkspace/crates/trogon-gateway/src/source_plugin.rs` around lines 47 - 62,
The generic bound on provision_integrations is using the unsupported
`AsyncFn(...) -> ...` form; update the function signature so the closure type F
is declared with the stable async bound syntax: change the where clause to `F:
async Fn(&C, &T) -> Result<(), C::Error>` (keeping the other bounds `C:
JetStreamContext`) so the async callable is properly typed when calling
`provision_fn(client, &integration.config).await` inside provision_integrations.
|
Re: yordis comment on ci-rust.yml — that change was on a superseded commit (9bfac32). The branch was rebuilt onto current main and ci-rust.yml was dropped from the diff. The current PR contains no changes to that file, so there is nothing to revert. |
|
Re: CodeRabbit comment on The current bound Re: unnecessary |
2 participants
refactor(gateway): introduce
SourcePlugintrait for uniform source dispatchEliminates the per-source iteration repeated between
http.rs::mount_sourcesandstreams.rs::provisionby introducing a gateway-internalSourcePlugintrait. Each webhook source has a unit-struct plugin (GithubPlugin,SlackPlugin, ...,MicrosoftGraphPlugin) that owns both its JetStream provisioning loop and its HTTP route mounting loop, including any per-source edge cases.Two dispatch entry points —
provision_webhook_sourcesandmount_webhook_sources— replace the repeated source-by-source dispatch blocks. Adding a new webhook source now touches one file (source_plugin.rs) instead of two.Notes:
SlackPlugin::mountskips socket-mode-only integrations (no webhook config); the socket-mode runners are spawned inmain.rs.SourcePlugin— its primary path is a WebSocket gateway runner, not a webhook receiver.streams.rsstill provisions it inline.MicrosoftGraphPlugincovers the source added on main since this branch was originally opened.All 579 gateway tests pass; full workspace tests pass.
Note: This branch was rebuilt on current
mainafter main independently absorbed the original PR'sresolve_commonhelper (nowresolve_common_integration_fieldsinconfig.rs) and added theVec<SourceIntegration<_>>model +mount_webhook_integrationshelper. The remaining value over main is the uniform iteration via theSourcePlugintrait, which lets new sources land in one file. Original commit before rebuild: dc07fc2.