feat(trogon-gateway): consolidate sources into unified binary#104
Merged
feat(trogon-gateway): consolidate sources into unified binary#104
Conversation
PR SummaryHigh Risk Overview Updates local dev infrastructure ( Refactors source crates (e.g., GitHub/Discord/GitLab) toward being pure libraries (removes Improves telemetry shutdown semantics by returning/propagating errors from Reviewed by Cursor Bugbot for commit 407878d. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR introduces a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Webhook Client
participant Gateway as Trogon Gateway
participant JetStream as NATS JetStream
participant ObjectStore as Object Store (S3/FilePath)
Client->>Gateway: POST /discord/webhook (payload)
activate Gateway
Gateway->>Gateway: Verify ed25519 signature
Gateway->>Gateway: Parse & validate interaction
alt Payload size <= Threshold
Gateway->>JetStream: publish_event(subject, headers, payload)
JetStream-->>Gateway: ack
else Payload size > Threshold
Gateway->>ObjectStore: put(key, payload_bytes)
ObjectStore-->>Gateway: ok
Gateway->>JetStream: publish_event(subject, headers, claim_check)
JetStream-->>Gateway: ack
end
deactivate Gateway
Gateway-->>Client: 200 OK
Note over JetStream,ObjectStore: Claim Check Pattern:<br/>Large payloads stored in Object Store,<br/>small claim metadata published to JetStream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
rsworkspace/crates/trogon-gateway/src/config.rs (2)
400-406: Uses{e:?}(Debug) instead of{e}(Display) forNatsTokenerrors.Lines 403 and 413 format
NatsTokenvalidation errors using Debug ({e:?}) rather than Display ({e}). If the error type implementsDisplay, prefer it for user-facing messages. If it doesn't, this is a sign the error type needs aDisplayimpl.- errors.push(format!("discord: invalid subject_prefix: {e:?}")); + errors.push(format!("discord: invalid subject_prefix: {e}")); ... - errors.push(format!("discord: invalid stream_name: {e:?}")); + errors.push(format!("discord: invalid stream_name: {e}"));This pattern repeats for slack (464, 474), gitlab (575, 585), and linear (634, 644).
Also applies to: 410-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/config.rs` around lines 400 - 406, The error messages use debug formatting `{e:?}` for NatsToken validation which should use display formatting `{e}` (or ensure the error type implements Display); update the match arms where NatsToken::new(subject_prefix) is handled (and the equivalent blocks for slack, gitlab, and linear validations) to log errors with `{e}` instead of `{e:?}` and push messages into the errors vector using the Display representation, e.g. in the subject_prefix handling, change the error push and return in the Err(e) branch to use `{e}`; repeat the same swap for the corresponding variables and functions handling slack, gitlab, and linear token/field validation so all user-facing messages use Display formatting.
11-17:ConfigError::ValidationusesVec<String>instead of typed errors.Per coding guidelines, errors should be typed. The
Validationvariant collects error messages as strings, discarding the original error types from parsing failures (e.g.,NatsTokenvalidation,parse_intents,parse_public_key).This design enables reporting multiple validation errors at once, which is valuable UX. A typed alternative could use:
Typed validation error approach
#[derive(Debug)] pub enum ValidationError { MissingField { source: &'static str, field: &'static str }, InvalidSubjectPrefix { source: &'static str, value: String }, InvalidStreamName { source: &'static str, value: String }, InvalidDiscordMode { value: String }, InvalidDiscordIntents { source: trogon_source_discord::config::UnknownIntentError }, InvalidDiscordPublicKey { source: String }, // or wrap the actual error type InvalidGitlabWebhookSecret { source: String }, // ... etc } pub enum ConfigError { ReadFile(std::io::Error), Interpolation(interpolation::UndefinedVarError), Yaml(serde_yaml::Error), Validation(Vec<ValidationError>), }This preserves type information while still supporting multiple-error reporting. As per coding guidelines: "Errors must be typed—use structs or enums, never
Stringorformat!()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/config.rs` around lines 11 - 17, The ConfigError::Validation variant currently uses Vec<String>; replace it with a typed Vec<ValidationError> and introduce a new ValidationError enum/structs to capture concrete error kinds (e.g., MissingField { source: &'static str, field: &'static str }, InvalidSubjectPrefix { source: &'static str, value: String }, InvalidDiscordIntents { source: trogon_source_discord::config::UnknownIntentError }, InvalidDiscordPublicKey { source: /* wrap parse_public_key error type */ }, etc.) so callers can aggregate multiple typed validation failures; update the ConfigError definition to pub enum ConfigError { ReadFile(std::io::Error), Interpolation(interpolation::UndefinedVarError), Yaml(serde_yaml::Error), Validation(Vec<ValidationError>), } and adapt creation sites (NatsToken validation, parse_intents, parse_public_key) to emit the corresponding ValidationError variants instead of formatted Strings.rsworkspace/crates/trogon-gateway/src/serve.rs (1)
18-18: Error type usesStringinstead of typed errors.Per coding guidelines, errors should be typed structs/enums rather than
String. TheSourceResultalias and allrun_*functions convert typed errors to strings viaformat!(), discarding the original error's type information.Since these errors are only logged (not propagated to callers), this is low-impact, but it sets a precedent that could spread. Consider introducing a simple enum:
Proposed typed error approach
enum SourceError { Provision(Box<dyn std::error::Error + Send + Sync>), Serve(Box<dyn std::error::Error + Send + Sync>), } impl fmt::Display for SourceError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Provision(e) => write!(f, "provision failed: {e}"), Self::Serve(e) => write!(f, "serve failed: {e}"), } } } type SourceResult = (&'static str, Result<(), SourceError>);This preserves the error chain for debugging while satisfying the guideline. As per coding guidelines: "Errors must be typed—use structs or enums, never
Stringorformat!()."Also applies to: 158-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/serve.rs` at line 18, Replace the current SourceResult alias that uses Result<(), String> with a typed error enum (e.g., SourceError) and change SourceResult to type SourceResult = (&'static str, Result<(), SourceError>); implement Display for SourceError to format messages but preserve the underlying error via variants like Provision(Box<dyn std::error::Error + Send + Sync>) and Serve(Box<dyn std::error::Error + Send + Sync>); then update all run_* functions (e.g., run_provision, run_serve) to return SourceError by boxing their original errors into the appropriate variant instead of calling format!() and String::from, so logs still get readable messages while preserving typed error information.rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs (1)
47-61: Gateway loop handles errors but doesn't distinguish recoverable vs fatal conditions.The loop correctly handles
Message::Closeand stream termination, but onSome(Err(source))it only logs a warning and continues. Depending on the error type (e.g., authentication failure vs transient network hiccup), some errors should trigger a reconnect or an exit.This is acceptable for an initial implementation, but consider adding error classification in a follow-up to avoid infinite warn loops on persistent failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs` around lines 47 - 61, The gateway loop currently treats all errors from poll_next the same; update the Some(Err(source)) arm to classify the error (using the error's type/variants from the websocket/shard poll result) and take appropriate action: for fatal/authentication errors (e.g., token invalid, protocol errors) log details and break/return to trigger a full reconnect or shutdown, for transient errors (e.g., IO/timeouts) perform a retry with backoff and continue, and for unknown cases escalate to warn with context; modify the match arm that handles Some(Err(source)) (referencing poll_fn, shard, and bridge.dispatch) to implement this classification and invoke the reconnect/shutdown path or retry/backoff accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 310-327: The GithubConfig (and similarly DiscordConfig)
initialization currently sets nats: NatsConfig::from_env(env) even though
sources' serve functions get NATS infra separately; remove the unused assignment
to NatsConfig::from_env(env) from the resolve_github (and resolve_discord)
config construction so the nats field is no longer populated there, leaving the
rest of the GithubConfig initialization (webhook_secret, port, subject_prefix,
stream_name, stream_max_age, nats_ack_timeout) intact and ensuring code compiles
without referencing NatsConfig::from_env in those functions.
- Around line 521-537: The TelegramSourceConfig currently uses plain String for
subject_prefix and stream_name; change the struct fields in
trogon-source-telegram::config::TelegramSourceConfig to use NatsToken (the same
domain type used by Slack/GitLab/Linear) and update
TelegramSourceConfig::from_env to parse/validate the env/or_env! results into
NatsToken instances (use the same NatsToken parsing/constructor helper the other
sources use and apply the default strings by converting them into NatsToken
values). Also update the gateway resolver code that builds
Some(trogon_source_telegram::TelegramSourceConfig { ... }) to pass the validated
NatsToken values instead of raw Strings so invalid NATS tokens fail fast during
config construction.
In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs`:
- Around line 33-41: The spawned health-server task currently panics on bind
failure via TcpListener::bind(...).await.expect(...) which silently aborts the
task; change this to propagate or handle the error: remove the expect and
propagate the Err back to the caller by making run(...) return Result (match the
webhook mode's serve signature), have the spawned block return a Result and
bubble that up so callers (update serve.rs to accept && handle the Result from
gateway_runner::run) can react to bind failures; if you prefer a smaller change,
replace expect with an explicit match that logs an error via error!/fatal!
including health_addr and the bind error and then cleanly exits the task instead
of panicking. Ensure you update references to the tokio::spawn block,
TcpListener::bind, health_addr and the run function to reflect the new
Result-based flow.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 400-406: The error messages use debug formatting `{e:?}` for
NatsToken validation which should use display formatting `{e}` (or ensure the
error type implements Display); update the match arms where
NatsToken::new(subject_prefix) is handled (and the equivalent blocks for slack,
gitlab, and linear validations) to log errors with `{e}` instead of `{e:?}` and
push messages into the errors vector using the Display representation, e.g. in
the subject_prefix handling, change the error push and return in the Err(e)
branch to use `{e}`; repeat the same swap for the corresponding variables and
functions handling slack, gitlab, and linear token/field validation so all
user-facing messages use Display formatting.
- Around line 11-17: The ConfigError::Validation variant currently uses
Vec<String>; replace it with a typed Vec<ValidationError> and introduce a new
ValidationError enum/structs to capture concrete error kinds (e.g., MissingField
{ source: &'static str, field: &'static str }, InvalidSubjectPrefix { source:
&'static str, value: String }, InvalidDiscordIntents { source:
trogon_source_discord::config::UnknownIntentError }, InvalidDiscordPublicKey {
source: /* wrap parse_public_key error type */ }, etc.) so callers can aggregate
multiple typed validation failures; update the ConfigError definition to pub
enum ConfigError { ReadFile(std::io::Error),
Interpolation(interpolation::UndefinedVarError), Yaml(serde_yaml::Error),
Validation(Vec<ValidationError>), } and adapt creation sites (NatsToken
validation, parse_intents, parse_public_key) to emit the corresponding
ValidationError variants instead of formatted Strings.
In `@rsworkspace/crates/trogon-gateway/src/serve.rs`:
- Line 18: Replace the current SourceResult alias that uses Result<(), String>
with a typed error enum (e.g., SourceError) and change SourceResult to type
SourceResult = (&'static str, Result<(), SourceError>); implement Display for
SourceError to format messages but preserve the underlying error via variants
like Provision(Box<dyn std::error::Error + Send + Sync>) and Serve(Box<dyn
std::error::Error + Send + Sync>); then update all run_* functions (e.g.,
run_provision, run_serve) to return SourceError by boxing their original errors
into the appropriate variant instead of calling format!() and String::from, so
logs still get readable messages while preserving typed error information.
In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs`:
- Around line 47-61: The gateway loop currently treats all errors from poll_next
the same; update the Some(Err(source)) arm to classify the error (using the
error's type/variants from the websocket/shard poll result) and take appropriate
action: for fatal/authentication errors (e.g., token invalid, protocol errors)
log details and break/return to trigger a full reconnect or shutdown, for
transient errors (e.g., IO/timeouts) perform a retry with backoff and continue,
and for unknown cases escalate to warn with context; modify the match arm that
handles Some(Err(source)) (referencing poll_fn, shard, and bridge.dispatch) to
implement this classification and invoke the reconnect/shutdown path or
retry/backoff accordingly.
🪄 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: bb7dd4f6-5be3-473e-933d-ef018fabd6e0
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
rsworkspace/Cargo.tomlrsworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/cli.rsrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/interpolation.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/serve.rsrsworkspace/crates/trogon-source-discord/Cargo.tomlrsworkspace/crates/trogon-source-discord/src/config.rsrsworkspace/crates/trogon-source-discord/src/gateway_runner.rsrsworkspace/crates/trogon-source-discord/src/lib.rsrsworkspace/crates/trogon-source-discord/src/main.rsrsworkspace/crates/trogon-source-github/Cargo.tomlrsworkspace/crates/trogon-source-github/src/main.rsrsworkspace/crates/trogon-source-gitlab/Cargo.tomlrsworkspace/crates/trogon-source-gitlab/src/main.rsrsworkspace/crates/trogon-source-linear/Cargo.tomlrsworkspace/crates/trogon-source-linear/src/lib.rsrsworkspace/crates/trogon-source-linear/src/main.rsrsworkspace/crates/trogon-source-slack/Cargo.tomlrsworkspace/crates/trogon-source-slack/src/main.rsrsworkspace/crates/trogon-source-telegram/src/main.rs
💤 Files with no reviewable changes (11)
- rsworkspace/crates/trogon-source-github/Cargo.toml
- rsworkspace/crates/trogon-source-linear/Cargo.toml
- rsworkspace/crates/trogon-source-slack/Cargo.toml
- rsworkspace/crates/trogon-source-gitlab/Cargo.toml
- rsworkspace/crates/trogon-source-gitlab/src/main.rs
- rsworkspace/crates/trogon-source-telegram/src/main.rs
- rsworkspace/crates/trogon-source-slack/src/main.rs
- rsworkspace/crates/trogon-source-discord/src/main.rs
- rsworkspace/crates/trogon-source-linear/src/main.rs
- rsworkspace/crates/trogon-source-github/src/main.rs
- rsworkspace/crates/trogon-source-discord/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
278-286:⚠️ Potential issue | 🟠 MajorChange
subject_prefixandstream_namefromStringtoNatsTokento match Discord, Slack, and Linear resolvers and comply with the domain-specific value objects guideline.GithubConfig passes raw strings for
subject_prefixandstream_name, while Discord, Slack, and Linear configs useNatsToken. This inconsistency bypasses validation at construction and violates the coding guideline to "Prefer domain-specific value objects over primitives."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/config.rs` around lines 278 - 286, The GithubConfig construction is passing raw strings for subject_prefix and stream_name; change those fields and their usages to use the NatsToken value object instead of String (update trogon_source_github::GithubConfig definition to use NatsToken for subject_prefix and stream_name), and when building the config in this function convert/validate the strings from section (e.g. with NatsToken::try_from or NatsToken::new) and propagate or handle any validation error; also update any downstream call sites that expect String to accept NatsToken so the domain-level validation is enforced at construction.
🧹 Nitpick comments (1)
devops/docker/compose/compose.yml (1)
75-78: Consider adding a healthcheck for the unified gateway.The previous per-source services had healthchecks. Without one, Docker/orchestrators can't verify the gateway is actually serving traffic (beyond process-running). Each source exposes
/healthon its respective port.Example healthcheck targeting one source port
healthcheck: test: ["CMD", "curl", "-f", "http://localhost:8081/health"] interval: 10s timeout: 5s start_period: 15s retries: 3Alternatively, check multiple ports or add a dedicated gateway-level health endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devops/docker/compose/compose.yml` around lines 75 - 78, Add a Docker healthcheck block for the unified gateway service in compose.yml so orchestrators can verify it is actually serving traffic (not just running); insert a healthcheck key under the gateway service (alongside depends_on and restart) that uses a CMD curl -f against one of the source /health endpoints (e.g., http://localhost:8081/health) and configure interval, timeout, start_period and retries (e.g., 10s, 5s, 15s, 3) or expand to probe multiple ports or a dedicated gateway-level /health as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/serve.rs`:
- Around line 184-198: The gateway path currently calls
trogon_source_discord::gateway_runner::run which returns () so failures are
swallowed; change gateway_runner::run to return Result<(), E> (E: impl
std::error::Error or boxed error), update its internal error returns
accordingly, and then update this match arm to await the Result and propagate
errors (e.g., trogon_source_discord::gateway_runner::run(p, &cfg, bot_token,
intents).await.map_err(|e| format!("discord: {e}"))), matching the webhook arm's
error mapping so gateway failures contribute to the overall failure count.
In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs`:
- Around line 51-65: The loop is awaiting bridge.dispatch(&text).await which can
block the shard poll and cause heartbeat timeouts; make dispatch fire-and-forget
by cloning the GatewayBridge and spawning the dispatch call instead. Derive
Clone for the GatewayBridge struct (add #[derive(Clone)] to GatewayBridge) since
its fields are cloneable, then in the loop clone the bridge (e.g., let bridge =
bridge.clone()) and use tokio::spawn or equivalent to call
bridge_clone.dispatch(text).await so NATS publish (publish_event) happens off
the poll thread; alternatively you can decouple publishing via a bounded channel
if you prefer backpressure control.
---
Duplicate comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 278-286: The GithubConfig construction is passing raw strings for
subject_prefix and stream_name; change those fields and their usages to use the
NatsToken value object instead of String (update
trogon_source_github::GithubConfig definition to use NatsToken for
subject_prefix and stream_name), and when building the config in this function
convert/validate the strings from section (e.g. with NatsToken::try_from or
NatsToken::new) and propagate or handle any validation error; also update any
downstream call sites that expect String to accept NatsToken so the domain-level
validation is enforced at construction.
---
Nitpick comments:
In `@devops/docker/compose/compose.yml`:
- Around line 75-78: Add a Docker healthcheck block for the unified gateway
service in compose.yml so orchestrators can verify it is actually serving
traffic (not just running); insert a healthcheck key under the gateway service
(alongside depends_on and restart) that uses a CMD curl -f against one of the
source /health endpoints (e.g., http://localhost:8081/health) and configure
interval, timeout, start_period and retries (e.g., 10s, 5s, 15s, 3) or expand to
probe multiple ports or a dedicated gateway-level /health as appropriate.
🪄 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: af044062-3d81-47a6-8945-a24836f34f87
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-gateway/Dockerfiledevops/docker/compose/services/trogon-gateway/README.mddevops/docker/compose/services/trogon-source-discord/Dockerfiledevops/docker/compose/services/trogon-source-discord/README.mddevops/docker/compose/services/trogon-source-github/Dockerfiledevops/docker/compose/services/trogon-source-github/README.mddevops/docker/compose/services/trogon-source-gitlab/Dockerfiledevops/docker/compose/services/trogon-source-gitlab/README.mddevops/docker/compose/services/trogon-source-linear/Dockerfiledevops/docker/compose/services/trogon-source-linear/README.mddevops/docker/compose/services/trogon-source-slack/README.mddevops/docker/compose/services/trogon-source-telegram/Dockerfilersworkspace/Cargo.tomlrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/serve.rsrsworkspace/crates/trogon-source-discord/src/gateway_runner.rsrsworkspace/crates/trogon-source-linear/src/lib.rs
💤 Files with no reviewable changes (10)
- devops/docker/compose/services/trogon-source-discord/README.md
- devops/docker/compose/services/trogon-source-linear/Dockerfile
- devops/docker/compose/services/trogon-source-gitlab/README.md
- devops/docker/compose/services/trogon-source-github/README.md
- devops/docker/compose/services/trogon-source-linear/README.md
- devops/docker/compose/services/trogon-source-discord/Dockerfile
- devops/docker/compose/services/trogon-source-github/Dockerfile
- devops/docker/compose/services/trogon-source-telegram/Dockerfile
- devops/docker/compose/services/trogon-source-gitlab/Dockerfile
- devops/docker/compose/services/trogon-source-slack/README.md
✅ Files skipped from review due to trivial changes (3)
- devops/docker/compose/services/trogon-gateway/README.md
- rsworkspace/crates/trogon-gateway/src/main.rs
- rsworkspace/crates/trogon-gateway/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-source-linear/src/lib.rs
- rsworkspace/Cargo.toml
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 407878d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
devops/docker/compose/compose.yml (1)
20-73: Consider adding a healthcheck fortrogon-gateway.The previous per-source services had healthchecks, but
trogon-gatewaydoesn't have one. Since the gateway exposesGET /health(perserve.rslines 69-73), adding a healthcheck would allowngrokto usecondition: service_healthyinstead ofservice_started, ensuring the HTTP server is ready before exposing tunnels.🔧 Proposed healthcheck addition
depends_on: nats: condition: service_healthy restart: unless-stopped + healthcheck: + test: ["CMD", "wget", "-qO-", "http://localhost:8080/health"] + interval: 5s + timeout: 3s + start_period: 5s + retries: 3Then update ngrok's dependency:
depends_on: trogon-gateway: - condition: service_started + condition: service_healthy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devops/docker/compose/compose.yml` around lines 20 - 73, Add a Docker Compose healthcheck for the trogon-gateway service that probes its HTTP health endpoint (GET /health as implemented in serve.rs) so compose can detect when the gateway is ready; implement a healthcheck using a simple HTTP probe (curl/wget) against http://localhost:${TROGON_GATEWAY_PORT:-8080}/health with sensible interval, timeout and retries, reference the trogon-gateway service block and the /health path in serve.rs to locate where to add it, and then change ngrok’s depends_on entry to use condition: service_healthy instead of service_started so ngrok waits for the gateway to report healthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 263-279: The resolve_github function currently forwards raw
subject_prefix and stream_name strings to trogon_source_github::GithubConfig,
bypassing NatsToken validation; update resolve_github to validate both
section.subject_prefix and section.stream_name via NatsToken::new (like
resolve_discord/resolve_slack do), push any validation error messages into the
provided errors Vec (remove the underscore from _errors so it is used), and only
construct/return the GithubConfig with the validated NatsToken values (or their
underlying safe representation) when both tokens are valid, otherwise return
None.
---
Nitpick comments:
In `@devops/docker/compose/compose.yml`:
- Around line 20-73: Add a Docker Compose healthcheck for the trogon-gateway
service that probes its HTTP health endpoint (GET /health as implemented in
serve.rs) so compose can detect when the gateway is ready; implement a
healthcheck using a simple HTTP probe (curl/wget) against
http://localhost:${TROGON_GATEWAY_PORT:-8080}/health with sensible interval,
timeout and retries, reference the trogon-gateway service block and the /health
path in serve.rs to locate where to add it, and then change ngrok’s depends_on
entry to use condition: service_healthy instead of service_started so ngrok
waits for the gateway to report healthy.
🪄 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: aec78af6-7b23-42e6-89c3-2baa2d1c379e
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-gateway/Dockerfiledevops/docker/compose/services/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/serve.rsrsworkspace/crates/trogon-source-discord/src/gateway_runner.rsrsworkspace/crates/trogon-source-linear/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- rsworkspace/crates/trogon-gateway/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- devops/docker/compose/services/trogon-gateway/Dockerfile
- rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs
- devops/docker/compose/.env.example
yordis
force-pushed
the
yordis/unified-gateway
branch
2 times, most recently
from
5d4740c to
adac53a
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rsworkspace/crates/trogon-source-slack/src/config.rs (1)
20-20:⚠️ Potential issue | 🟡 MinorStale documentation:
SLACK_MAX_BODY_SIZEis no longer a config option.Line 20 still documents
SLACK_MAX_BODY_SIZEas an environment variable, but this field was removed fromSlackConfig. The body size limit is now enforced via theHTTP_BODY_SIZE_MAXconstant in the AxumDefaultBodyLimitlayer. Remove this line from the doc comment to avoid confusion.📝 Proposed fix
/// - `SLACK_STREAM_MAX_AGE_SECS`: max age of messages in the JetStream stream in seconds (default: 604800 / 7 days) /// - `SLACK_NATS_ACK_TIMEOUT_SECS`: NATS ack timeout in seconds (default: 10) -/// - `SLACK_MAX_BODY_SIZE`: maximum webhook body size in bytes (default: 1048576 / 1 MB) /// - `SLACK_TIMESTAMP_MAX_DRIFT_SECS`: max allowed clock drift for request timestamps in seconds (default: 300 / 5 min)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-slack/src/config.rs` at line 20, Remove the stale doc line referencing SLACK_MAX_BODY_SIZE from the SlackConfig documentation: locate the doc comment that mentions `SLACK_MAX_BODY_SIZE` near the `SlackConfig` struct and delete that line so docs no longer advertise a removed env var; optionally add a brief note (if helpful) that body size is enforced via `HTTP_BODY_SIZE_MAX` used with Axum's `DefaultBodyLimit`, but do not reintroduce `SLACK_MAX_BODY_SIZE` or related fields.rsworkspace/crates/trogon-source-slack/src/server.rs (1)
76-92:⚠️ Potential issue | 🟠 MajorSurface unroutable publish failures to the HTTP response.
This helper only logs a failed
.unroutablepublish. The callers still return200/400, so a NATS or object-store outage can silently drop the malformed payload you were trying to retain for inspection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-slack/src/server.rs` around lines 76 - 92, publish_unroutable currently swallows publish failures; change it to return a Result<(), Error> (or appropriate anyhow/thiserror type) instead of unit, propagate the error from publisher.publish_event(subject, headers, body, ack_timeout).await (don’t just call outcome.log_on_error) and map the outcome into an Err containing the publish error so callers can act; update the call sites that invoke publish_unroutable in this module to check the Result and translate a failed unroutable publish into an HTTP error response (e.g., 5xx) instead of always returning 200/400 so NATS/object-store outages surface to the client.rsworkspace/crates/trogon-source-github/src/server.rs (1)
193-213:⚠️ Potential issue | 🟠 MajorDon’t dedupe missing deliveries as
"unknown".
Nats-Msg-Idis the dedupe key. Falling back to"unknown"means every request withoutX-GitHub-Deliveryshares the same id and later events can be discarded. Reject missing deliveries or omitNats-Msg-Idwhen the header is absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-github/src/server.rs` around lines 193 - 213, The code currently substitutes a missing HEADER_DELIVERY with "unknown", causing all events without X-GitHub-Delivery to share the same NATS_MESSAGE_ID; change the logic so HEADER_DELIVERY is not defaulted to "unknown": retrieve HEADER_DELIVERY into an Option (e.g., let delivery_opt = headers.get(HEADER_DELIVERY).and_then(|v| v.to_str().ok()).map(|s| s.to_owned())), and then either return an error/HTTP 400 when delivery is None (reject missing deliveries) or, if you prefer to accept requests without a delivery id, avoid calling nats_headers.insert(async_nats::header::NATS_MESSAGE_ID, ...) when delivery_opt is None; update uses of delivery (span.record and NATS_HEADER_DELIVERY) to handle the Option (record only when Some) before calling state.publisher.publish_event so no constant "unknown" dedupe id is published.rsworkspace/crates/trogon-source-linear/src/server.rs (1)
327-347:⚠️ Potential issue | 🟠 MajorDon’t reuse
"unknown"asNats-Msg-Id.
Nats-Msg-Idis the JetStream dedupe key. IfwebhookIdis absent, every payload on this path gets the same id and later events can be dropped as duplicates. Either reject missingwebhookIdas unroutable or omitNats-Msg-Idwhen it’s missing.Possible fix
- let webhook_id = parsed - .get("webhookId") - .and_then(|v| v.as_str()) - .unwrap_or("unknown") - .to_owned(); + let webhook_id = parsed + .get("webhookId") + .and_then(|v| v.as_str()) + .map(str::to_owned); @@ - span.record("webhook_id", &webhook_id); + span.record("webhook_id", webhook_id.as_deref().unwrap_or("missing")); @@ - nats_headers.insert("Nats-Msg-Id", webhook_id.as_str()); + if let Some(ref webhook_id) = webhook_id { + nats_headers.insert(async_nats::header::NATS_MESSAGE_ID, webhook_id.as_str()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-linear/src/server.rs` around lines 327 - 347, The code currently defaults missing parsed.get("webhookId") to "unknown" and always inserts a Nats-Msg-Id header, which can cause JetStream dedupe collisions; change the logic so you do NOT use the "unknown" sentinel—either return an error (e.g. reject the request) or, preferably, keep webhook_id as an Option and only insert the "Nats-Msg-Id" into nats_headers when webhook_id is Some(value); update the parsed.get("webhookId") handling and the nats_headers.insert call that precedes state.publisher.publish_event(subject, nats_headers, body, state.nats_ack_timeout).
♻️ Duplicate comments (1)
rsworkspace/crates/trogon-gateway/src/serve.rs (1)
183-185:⚠️ Potential issue | 🟠 MajorPropagate Discord gateway task failures instead of always returning
Ok(()).This closure marks the task successful no matter how
gateway_runner::runterminates, so non-panic gateway failures won’t incrementfailedor participate in the “all tasks failed” exit path. Ifgateway_runner::runcurrently returns(), it needs to return aResulthere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/serve.rs` around lines 183 - 185, The spawned closure currently always returns ("discord-gateway", Ok(())) so gateway_runner failures are ignored; update the closure passed to join_set.spawn to propagate trogon_source_discord::gateway_runner::run's result instead of forcing Ok(()): have run return a Result if it does not already (or map its unit return into Result by catching errors), then return ("discord-gateway", run_result) from the closure so failures increment the failed count and participate in the "all tasks failed" logic; specifically modify the join_set.spawn async block that calls trogon_source_discord::gateway_runner::run(p, &cfg, &token, intents).await to yield the actual Result rather than Ok(()).
🧹 Nitpick comments (6)
rsworkspace/crates/trogon-std/src/http.rs (1)
9-14: Potential truncation on 32-bit platforms and code simplification.The cast
size.as_u64() as usizewill silently truncate values exceedingusize::MAXon 32-bit targets. While the current use cases (≤25 MiB) fit comfortably, consider documenting this limitation or adding a compile-time assertion.The match can also be simplified:
♻️ Simplified constructor
pub const fn new(size: ByteSize) -> Option<Self> { - match NonZeroUsize::new(size.as_u64() as usize) { - Some(n) => Some(Self(n)), - None => None, - } + // Note: truncates on 32-bit targets for sizes > 4 GiB + if let Some(n) = NonZeroUsize::new(size.as_u64() as usize) { + Some(Self(n)) + } else { + None + } }Note:
Option::mapisn't available inconst fncontexts yet, soif letis the idiomatic alternative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-std/src/http.rs` around lines 9 - 14, The constructor pub const fn new(size: ByteSize) -> Option<Self> risks truncating on 32-bit targets due to size.as_u64() as usize and can be simplified; update the function (new) to first check that size.as_u64() <= usize::MAX (or add a compile-time cfg assertion documenting 32-bit limits) and then convert safely, and replace the match on NonZeroUsize::new(...) with an if let pattern to return Some(Self(n)) or None for brevity and clarity.rsworkspace/crates/trogon-source-github/src/config.rs (1)
21-29: Consider using a typedWebhookSecretvalue object for consistency.The
webhook_secretfield is a plainString, while the GitLab source uses a typedWebhookSecretvalue object (withArc<str>, empty validation, and redactedDebug). For consistency across sources and to benefit from:
- Redacted debug output (prevents accidental secret leakage in logs)
- Cheap cloning via
Arc<str>- Compile-time type distinction
Consider either reusing GitLab's
WebhookSecretvia a shared crate or creating a similar type for GitHub.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-github/src/config.rs` around lines 21 - 29, GithubConfig currently exposes webhook_secret as a plain String; change it to use a typed WebhookSecret value object (matching the GitLab source) to get Arc<str> cloning, redacted Debug and consistent type-safety: replace the field GithubConfig::webhook_secret: String with WebhookSecret, import or move the WebhookSecret type (or create an equivalent with Arc<str>, empty validation and redacted Debug), update any constructors, builders and places that construct or clone GithubConfig to accept/convert into WebhookSecret, and ensure serialization/deserialization and trait imports are adjusted accordingly so existing code compiles with the new type.rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs (1)
1-10: Consider returningResultfor better error propagation.The
runfunction returns(), making it impossible for callers to distinguish between graceful shutdown and failures. The calling code inserve.rsspawns this in aJoinSetand always returnsOk(())regardless of what happened.Returning a
Resultwould enable proper error handling and potential restart logic.♻️ Proposed signature change
#[cfg(not(coverage))] -pub async fn run< +pub async fn run< P: trogon_nats::jetstream::JetStreamPublisher, S: trogon_nats::jetstream::ObjectStorePut, >( publisher: trogon_nats::jetstream::ClaimCheckPublisher<P, S>, config: &crate::config::DiscordConfig, bot_token: &str, intents: twilight_model::gateway::Intents, -) { +) -> Result<(), GatewayRunnerError> {Where
GatewayRunnerErroris a typed error enum per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs` around lines 1 - 10, Change the async function run to return a Result so callers can distinguish shutdown from failures: replace the unit return with Result<(), GatewayRunnerError> (create a GatewayRunnerError enum per project guidelines), update gateway_runner::run to return Err(...) on failures and propagate using ? where appropriate, keep the existing #[cfg(not(coverage))] attribute and generic bounds intact, and update the caller in serve.rs that spawns run in the JoinSet to await and handle the Result (propagate or log/restart as needed) instead of always treating it as Ok(()).rsworkspace/crates/trogon-nats/src/jetstream/mocks.rs (1)
836-852: Consider adding a test forfail_next_put.There's a test for
fail_next_getbut none forfail_next_put. Adding one would improve coverage symmetry.💚 Proposed test for fail_next_put
#[tokio::test] async fn mock_object_store_fail_next_put() { let store = MockObjectStore::new(); store.fail_next_put(); let mut data = std::io::Cursor::new(b"test data".to_vec()); let result = ObjectStorePut::put(&store, "key", &mut data).await; assert!(result.is_err()); let mut data = std::io::Cursor::new(b"test data".to_vec()); let result = ObjectStorePut::put(&store, "key", &mut data).await; assert!(result.is_ok()); assert_eq!(store.stored_objects().len(), 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/jetstream/mocks.rs` around lines 836 - 852, Add a symmetric test for MockObjectStore::fail_next_put similar to mock_object_store_fail_next_get: create MockObjectStore::new(), call store.fail_next_put(), attempt an ObjectStorePut::put(&store, "key", &mut data) and assert it returns Err, then perform a second put with fresh data and assert it returns Ok and that store.stored_objects().len() == 1; reference MockObjectStore, fail_next_put, ObjectStorePut::put and stored_objects() to locate where to add the new async #[tokio::test].rsworkspace/crates/trogon-source-telegram/src/config.rs (1)
22-30: Consider using a wrapper type forwebhook_secretfor consistency.The GitLab source uses
WebhookSecretwrapper type (seersworkspace/crates/trogon-source-gitlab/src/webhook_secret.rs) which guarantees non-empty at construction. Using a plainStringhere is inconsistent and loses that guarantee at the type level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-telegram/src/config.rs` around lines 22 - 30, The TelegramSourceConfig currently uses a raw String for webhook_secret; replace its type with the existing WebhookSecret wrapper used by the GitLab source to preserve the non-empty guarantee. Update the struct field signature (TelegramSourceConfig::webhook_secret) to WebhookSecret, add the appropriate use/import of WebhookSecret from the gitlab wrapper module, and adjust any construction/deserialization sites to call WebhookSecret::new/try_from (or the wrapper's validated constructor) instead of passing a raw String; also update any tests/usage that access webhook_secret as a String to use the wrapper's accessor or .as_str() as needed.rsworkspace/crates/trogon-source-gitlab/src/signature.rs (1)
22-36: Unnecessary SHA-256 hashing for simple token comparison.GitLab's
X-Gitlab-Tokenis a shared secret that should be directly compared. The current implementation hashes both values before comparing, which adds unnecessary computation. A constant-time string comparison would be more efficient and semantically clearer.♻️ Proposed simplification using direct constant-time comparison
pub fn verify(secret: &str, provided_token: &str) -> Result<(), SignatureError> { if provided_token.is_empty() { return Err(SignatureError::Missing); } - let expected = Sha256::digest(secret.as_bytes()); - let provided = Sha256::digest(provided_token.as_bytes()); - - let ok = subtle::ConstantTimeEq::ct_eq(expected.as_slice(), provided.as_slice()).unwrap_u8(); + let secret_bytes = secret.as_bytes(); + let provided_bytes = provided_token.as_bytes(); + + // Length-constant comparison: if lengths differ, compare secret against itself + // to maintain constant time, then return mismatch. + if secret_bytes.len() != provided_bytes.len() { + let _ = subtle::ConstantTimeEq::ct_eq(secret_bytes, secret_bytes); + return Err(SignatureError::Mismatch); + } + + let ok = subtle::ConstantTimeEq::ct_eq(secret_bytes, provided_bytes).unwrap_u8(); if ok == 1 { Ok(()) } else { Err(SignatureError::Mismatch) } }Note: The length check introduces a timing side-channel for length, but since GitLab webhook secrets are typically fixed-length UUIDs, this is acceptable. Alternatively, keep the hash approach if variable-length secret timing leakage is a concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-gitlab/src/signature.rs` around lines 22 - 36, The verify function currently hashes secret and provided_token before comparing; instead remove the SHA-256 digest steps and perform a direct constant-time comparison of the raw byte slices: in verify, after the existing empty-check that returns SignatureError::Missing, compare secret.as_bytes() and provided_token.as_bytes() using subtle::ConstantTimeEq::ct_eq (ensure you treat differing lengths as a non-match and return SignatureError::Mismatch), returning Ok(()) when ct_eq yields equality and Err(SignatureError::Mismatch) otherwise; update only verify in signature.rs and keep existing SignatureError variants (Missing, Mismatch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devops/docker/compose/services/trogon-gateway/README.md`:
- Around line 31-38: The README commands assume you're in the service's compose
directory; update the instructions around copying `.env.example` to `.env` and
running `docker compose up` to first instruct the user to "cd into the directory
containing this README (the trogon-gateway compose directory)" or to run the
commands with repo-relative paths, e.g. mention running the copy and `docker
compose` from that compose directory or prefixing paths with the correct
relative path to `.env.example`; ensure the same clarification is applied to the
later lines referenced (lines 88-89) so both copy and compose invocations target
the correct directory.
In `@rsworkspace/crates/acp-nats-ws/src/main.rs`:
- Around line 118-126: The current code always sleeps for a fixed 5s after
process_connections returns; change it to treat 5s as an upper bound by wrapping
the LocalSet run_until in a timeout so we exit early if the LocalSet becomes
idle. Replace the plain rt.block_on(local.run_until(...)) call with a
tokio::time::timeout(drain, local.run_until(...)) (await the timeout result on
the runtime), keeping the same drain Duration variable, so rt.block_on returns
immediately when local.run_until completes or after the 5s bound if tasks are
still running.
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 224-240: In resolve_nats, treat Option<String> values that are
Some("") as absent and validate for half-configured auth: interpret creds, nkey,
token only if the Option is Some(s) and s.trim().is_empty() is false; for
user/password require both section.user and section.password to be present and
non-empty (trimmed) or else return a validation error instead of silently
falling through; change the function to return a Result<NatsConfig, ConfigError>
(or similar error type used in your config path) and return an explicit error
when a half-configured auth is detected (e.g., user without password or empty
creds/nkey/token), otherwise construct
NatsConfig::new(vec![section.url.clone()], auth) with the validated NatsAuth
variants.
- Around line 269-273: The "gateway" branch currently only checks for Some(_) so
an empty string passes; update the check on section.bot_token inside the
"gateway" match arm to validate that the Option contains a non-empty,
non-whitespace token (e.g., match let Some(bot_token) and verify
!bot_token.trim().is_empty()), and if empty push the same error ("discord:
bot_token is required when mode=gateway") and return None; reference the
section.bot_token and bot_token variables in your change.
- Around line 9-11: Replace the untyped Validation variant on ConfigError
(currently Validation(Vec<String>)) with a concrete typed error (e.g., a new
ValidationError enum or struct) and make Validation hold that type
(ConfigError::Validation(ValidationError)); implement std::fmt::Display and
std::error::Error for ValidationError, preserve any per-field context (field
name, violation kind, and message) as structured fields, and update all places
that currently construct ConfigError::Validation(Vec<String>) to build and pass
ValidationError instances instead of formatted strings so callers and tests can
inspect field-level errors.
In `@rsworkspace/crates/trogon-gateway/src/serve.rs`:
- Line 23: Replace the untyped String-based error with a small typed error enum
(e.g., TaskError) and update SourceResult to use TaskError instead of String;
specifically, define TaskError variants that wrap the original error types (for
example TaskError::HttpServer(impl Error) for the http server case referenced by
format!("http server: {e}")) and change all usages (including the spots around
lines 94-95) to construct and return the appropriate TaskError variant rather
than calling format!(); also update any downstream handling/printing to use the
TaskError (preserving source/error chaining) and adjust function signatures and
Result propagation in the affected functions to accept/return TaskError.
In `@rsworkspace/crates/trogon-nats/src/jetstream/claim_check.rs`:
- Around line 155-160: The stored claim-check payload can be orphaned if
publishing/acknowledgement fails because the gateway provisions the bucket with
Default::default() (no max_age) — fix this by either setting a bucket TTL when
provisioning in trogon-gateway (set max_age on the bucket options where the
claim-check bucket is created in serve.rs) or add delete-on-failure cleanup in
the claim-check publish flow: after successfully calling self.store.put(&key,
...) and if the subsequent publish/ack path returns a failure (e.g.
PublishOutcome::PublishFailed or similar), invoke self.store.delete(&key).
Ensure you handle and log any delete errors and don't mask the original publish
error (use the same error return path as PublishOutcome).
- Around line 68-88: The enum ClaimResolveError currently discards wrapped
causes in its Error impl; update the std::error::Error impl for
ClaimResolveError to expose sources by adding a source(&self) -> Option<&(dyn
std::error::Error + 'static)> implementation that returns None for MissingKey,
Some(e) for StoreFailed(e) and Some(e) for ReadFailed(e). To do this, tighten
the trait bounds on the impl to require E: std::error::Error (in addition to the
existing Display/Debug/Send/Sync/'static) so you can return &E as a dyn Error,
and implement source to match on self and return the appropriate reference to
the stored error for the StoreFailed and ReadFailed variants.
In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs`:
- Around line 36-44: The handler in gateway_runner.rs currently silently breaks
on Some(Ok(Message::Close(_))) and None (stream end) which stops the gateway;
modify the match arms for Message::Close and None to either (a) attempt a
reconnect loop with exponential backoff (retrying the connection logic used
earlier in this function, delaying between attempts and resetting backoff on
success) or (b) return a Result::Err (propagate a specific error) so the caller
in serve.rs can restart the shard; update the Some(Err(source)) arm to also
signal fatal vs transient errors if needed. Reference Message::Close and the
match handling in gateway_runner.rs and ensure the function signature propagates
an error (or implements a reconnect loop) so serve.rs can reliably restart
shards.
- Around line 32-46: The loop blocks on bridge.dispatch(&text).await (which
calls publisher.publish_event) and can delay shard.polling; instead decouple
publishing from the poll loop by queuing events to a background publisher task:
create an async mpsc channel (tokio::sync::mpsc) and in the loop send the
received text into the channel (without .awaiting the network call), then run a
separate task (tokio::spawn) that reads from that channel and calls
GatewayBridge::dispatch or publisher.publish_event; if necessary wrap
GatewayBridge or the publisher in an Arc (or Arc<Mutex>/Arc<RwLock>) so the
background task can own the publisher/bridge and perform network I/O without
blocking the shard loop.
In `@rsworkspace/crates/trogon-source-discord/src/server.rs`:
- Around line 266-282: The code currently assigns "unknown" to interaction_id
and unconditionally inserts it into nats_headers as
async_nats::header::NATS_MESSAGE_ID, causing JetStream dedupe collisions; change
the logic to treat the ID as optional: keep the parsed.get("id") result (e.g.,
interaction_id_opt from parsed.get("id").and_then(|v| v.as_str()).map(|s|
s.to_owned())), record and use a real ID when Some, and only insert
NATS_MESSAGE_ID into nats_headers when that Option is Some; alternatively return
a 400/unroutable response when id is missing—do not insert the literal "unknown"
into NATS_MESSAGE_ID. Ensure references to interaction_id, nats_headers,
NATS_MESSAGE_ID, and state.subject_prefix are updated accordingly.
In `@rsworkspace/crates/trogon-source-telegram/src/config.rs`:
- Around line 20-30: The doc mentions TELEGRAM_MAX_BODY_SIZE but the
TelegramSourceConfig struct lacks a max_body_size field; add a pub
max_body_size: usize (or appropriate type) to TelegramSourceConfig and populate
it in the from_env constructor by reading
env.var("TELEGRAM_MAX_BODY_SIZE").ok().and_then(|v|
v.parse().ok()).unwrap_or(DEFAULT_MAX_BODY_SIZE) (use the existing
DEFAULT_MAX_BODY_SIZE constant), ensuring the field name matches usages
elsewhere and imports/types remain consistent.
---
Outside diff comments:
In `@rsworkspace/crates/trogon-source-github/src/server.rs`:
- Around line 193-213: The code currently substitutes a missing HEADER_DELIVERY
with "unknown", causing all events without X-GitHub-Delivery to share the same
NATS_MESSAGE_ID; change the logic so HEADER_DELIVERY is not defaulted to
"unknown": retrieve HEADER_DELIVERY into an Option (e.g., let delivery_opt =
headers.get(HEADER_DELIVERY).and_then(|v| v.to_str().ok()).map(|s|
s.to_owned())), and then either return an error/HTTP 400 when delivery is None
(reject missing deliveries) or, if you prefer to accept requests without a
delivery id, avoid calling
nats_headers.insert(async_nats::header::NATS_MESSAGE_ID, ...) when delivery_opt
is None; update uses of delivery (span.record and NATS_HEADER_DELIVERY) to
handle the Option (record only when Some) before calling
state.publisher.publish_event so no constant "unknown" dedupe id is published.
In `@rsworkspace/crates/trogon-source-linear/src/server.rs`:
- Around line 327-347: The code currently defaults missing
parsed.get("webhookId") to "unknown" and always inserts a Nats-Msg-Id header,
which can cause JetStream dedupe collisions; change the logic so you do NOT use
the "unknown" sentinel—either return an error (e.g. reject the request) or,
preferably, keep webhook_id as an Option and only insert the "Nats-Msg-Id" into
nats_headers when webhook_id is Some(value); update the parsed.get("webhookId")
handling and the nats_headers.insert call that precedes
state.publisher.publish_event(subject, nats_headers, body,
state.nats_ack_timeout).
In `@rsworkspace/crates/trogon-source-slack/src/config.rs`:
- Line 20: Remove the stale doc line referencing SLACK_MAX_BODY_SIZE from the
SlackConfig documentation: locate the doc comment that mentions
`SLACK_MAX_BODY_SIZE` near the `SlackConfig` struct and delete that line so docs
no longer advertise a removed env var; optionally add a brief note (if helpful)
that body size is enforced via `HTTP_BODY_SIZE_MAX` used with Axum's
`DefaultBodyLimit`, but do not reintroduce `SLACK_MAX_BODY_SIZE` or related
fields.
In `@rsworkspace/crates/trogon-source-slack/src/server.rs`:
- Around line 76-92: publish_unroutable currently swallows publish failures;
change it to return a Result<(), Error> (or appropriate anyhow/thiserror type)
instead of unit, propagate the error from publisher.publish_event(subject,
headers, body, ack_timeout).await (don’t just call outcome.log_on_error) and map
the outcome into an Err containing the publish error so callers can act; update
the call sites that invoke publish_unroutable in this module to check the Result
and translate a failed unroutable publish into an HTTP error response (e.g.,
5xx) instead of always returning 200/400 so NATS/object-store outages surface to
the client.
---
Duplicate comments:
In `@rsworkspace/crates/trogon-gateway/src/serve.rs`:
- Around line 183-185: The spawned closure currently always returns
("discord-gateway", Ok(())) so gateway_runner failures are ignored; update the
closure passed to join_set.spawn to propagate
trogon_source_discord::gateway_runner::run's result instead of forcing Ok(()):
have run return a Result if it does not already (or map its unit return into
Result by catching errors), then return ("discord-gateway", run_result) from the
closure so failures increment the failed count and participate in the "all tasks
failed" logic; specifically modify the join_set.spawn async block that calls
trogon_source_discord::gateway_runner::run(p, &cfg, &token, intents).await to
yield the actual Result rather than Ok(()).
---
Nitpick comments:
In `@rsworkspace/crates/trogon-nats/src/jetstream/mocks.rs`:
- Around line 836-852: Add a symmetric test for MockObjectStore::fail_next_put
similar to mock_object_store_fail_next_get: create MockObjectStore::new(), call
store.fail_next_put(), attempt an ObjectStorePut::put(&store, "key", &mut data)
and assert it returns Err, then perform a second put with fresh data and assert
it returns Ok and that store.stored_objects().len() == 1; reference
MockObjectStore, fail_next_put, ObjectStorePut::put and stored_objects() to
locate where to add the new async #[tokio::test].
In `@rsworkspace/crates/trogon-source-discord/src/gateway_runner.rs`:
- Around line 1-10: Change the async function run to return a Result so callers
can distinguish shutdown from failures: replace the unit return with Result<(),
GatewayRunnerError> (create a GatewayRunnerError enum per project guidelines),
update gateway_runner::run to return Err(...) on failures and propagate using ?
where appropriate, keep the existing #[cfg(not(coverage))] attribute and generic
bounds intact, and update the caller in serve.rs that spawns run in the JoinSet
to await and handle the Result (propagate or log/restart as needed) instead of
always treating it as Ok(()).
In `@rsworkspace/crates/trogon-source-github/src/config.rs`:
- Around line 21-29: GithubConfig currently exposes webhook_secret as a plain
String; change it to use a typed WebhookSecret value object (matching the GitLab
source) to get Arc<str> cloning, redacted Debug and consistent type-safety:
replace the field GithubConfig::webhook_secret: String with WebhookSecret,
import or move the WebhookSecret type (or create an equivalent with Arc<str>,
empty validation and redacted Debug), update any constructors, builders and
places that construct or clone GithubConfig to accept/convert into
WebhookSecret, and ensure serialization/deserialization and trait imports are
adjusted accordingly so existing code compiles with the new type.
In `@rsworkspace/crates/trogon-source-gitlab/src/signature.rs`:
- Around line 22-36: The verify function currently hashes secret and
provided_token before comparing; instead remove the SHA-256 digest steps and
perform a direct constant-time comparison of the raw byte slices: in verify,
after the existing empty-check that returns SignatureError::Missing, compare
secret.as_bytes() and provided_token.as_bytes() using
subtle::ConstantTimeEq::ct_eq (ensure you treat differing lengths as a non-match
and return SignatureError::Mismatch), returning Ok(()) when ct_eq yields
equality and Err(SignatureError::Mismatch) otherwise; update only verify in
signature.rs and keep existing SignatureError variants (Missing, Mismatch).
In `@rsworkspace/crates/trogon-source-telegram/src/config.rs`:
- Around line 22-30: The TelegramSourceConfig currently uses a raw String for
webhook_secret; replace its type with the existing WebhookSecret wrapper used by
the GitLab source to preserve the non-empty guarantee. Update the struct field
signature (TelegramSourceConfig::webhook_secret) to WebhookSecret, add the
appropriate use/import of WebhookSecret from the gitlab wrapper module, and
adjust any construction/deserialization sites to call
WebhookSecret::new/try_from (or the wrapper's validated constructor) instead of
passing a raw String; also update any tests/usage that access webhook_secret as
a String to use the wrapper's accessor or .as_str() as needed.
In `@rsworkspace/crates/trogon-std/src/http.rs`:
- Around line 9-14: The constructor pub const fn new(size: ByteSize) ->
Option<Self> risks truncating on 32-bit targets due to size.as_u64() as usize
and can be simplified; update the function (new) to first check that
size.as_u64() <= usize::MAX (or add a compile-time cfg assertion documenting
32-bit limits) and then convert safely, and replace the match on
NonZeroUsize::new(...) with an if let pattern to return Some(Self(n)) or None
for brevity and clarity.
🪄 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: 0c3cdf5f-7c0e-4cf9-aee8-3406ca913a12
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-gateway/Dockerfiledevops/docker/compose/services/trogon-gateway/README.mddevops/docker/compose/services/trogon-source-github/README.mddevops/docker/compose/services/trogon-source-linear/Dockerfiledevops/docker/compose/services/trogon-source-linear/README.mddevops/docker/compose/services/trogon-source-slack/Dockerfiledevops/docker/compose/services/trogon-source-slack/README.mdrsworkspace/Cargo.tomlrsworkspace/crates/AGENTS.mdrsworkspace/crates/acp-nats-ws/src/main.rsrsworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/cli.rsrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/serve.rsrsworkspace/crates/trogon-nats/Cargo.tomlrsworkspace/crates/trogon-nats/src/jetstream/claim_check.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rsrsworkspace/crates/trogon-nats/src/jetstream/mod.rsrsworkspace/crates/trogon-nats/src/jetstream/object_store.rsrsworkspace/crates/trogon-nats/src/jetstream/publish.rsrsworkspace/crates/trogon-source-discord/Cargo.tomlrsworkspace/crates/trogon-source-discord/src/config.rsrsworkspace/crates/trogon-source-discord/src/constants.rsrsworkspace/crates/trogon-source-discord/src/gateway.rsrsworkspace/crates/trogon-source-discord/src/gateway_runner.rsrsworkspace/crates/trogon-source-discord/src/lib.rsrsworkspace/crates/trogon-source-discord/src/server.rsrsworkspace/crates/trogon-source-discord/src/signature.rsrsworkspace/crates/trogon-source-github/Cargo.tomlrsworkspace/crates/trogon-source-github/src/config.rsrsworkspace/crates/trogon-source-github/src/constants.rsrsworkspace/crates/trogon-source-github/src/main.rsrsworkspace/crates/trogon-source-github/src/server.rsrsworkspace/crates/trogon-source-gitlab/Cargo.tomlrsworkspace/crates/trogon-source-gitlab/src/config.rsrsworkspace/crates/trogon-source-gitlab/src/constants.rsrsworkspace/crates/trogon-source-gitlab/src/lib.rsrsworkspace/crates/trogon-source-gitlab/src/server.rsrsworkspace/crates/trogon-source-gitlab/src/signature.rsrsworkspace/crates/trogon-source-gitlab/src/webhook_secret.rsrsworkspace/crates/trogon-source-linear/Cargo.tomlrsworkspace/crates/trogon-source-linear/src/constants.rsrsworkspace/crates/trogon-source-linear/src/lib.rsrsworkspace/crates/trogon-source-linear/src/main.rsrsworkspace/crates/trogon-source-linear/src/server.rsrsworkspace/crates/trogon-source-slack/Cargo.tomlrsworkspace/crates/trogon-source-slack/src/config.rsrsworkspace/crates/trogon-source-slack/src/constants.rsrsworkspace/crates/trogon-source-slack/src/main.rsrsworkspace/crates/trogon-source-slack/src/server.rsrsworkspace/crates/trogon-source-telegram/Cargo.tomlrsworkspace/crates/trogon-source-telegram/src/config.rsrsworkspace/crates/trogon-source-telegram/src/constants.rsrsworkspace/crates/trogon-source-telegram/src/lib.rsrsworkspace/crates/trogon-source-telegram/src/server.rsrsworkspace/crates/trogon-source-telegram/src/signature.rsrsworkspace/crates/trogon-std/Cargo.tomlrsworkspace/crates/trogon-std/src/http.rsrsworkspace/crates/trogon-std/src/lib.rsrsworkspace/crates/trogon-std/src/time/system.rs
💤 Files with no reviewable changes (10)
- devops/docker/compose/services/trogon-source-linear/README.md
- devops/docker/compose/services/trogon-source-github/README.md
- devops/docker/compose/services/trogon-source-slack/Dockerfile
- rsworkspace/crates/trogon-source-github/Cargo.toml
- devops/docker/compose/services/trogon-source-slack/README.md
- devops/docker/compose/services/trogon-source-linear/Dockerfile
- rsworkspace/crates/trogon-source-slack/Cargo.toml
- rsworkspace/crates/trogon-source-linear/src/main.rs
- rsworkspace/crates/trogon-source-github/src/main.rs
- rsworkspace/crates/trogon-source-slack/src/main.rs
✅ Files skipped from review due to trivial changes (10)
- rsworkspace/crates/trogon-std/Cargo.toml
- rsworkspace/crates/trogon-std/src/time/system.rs
- rsworkspace/crates/trogon-nats/Cargo.toml
- rsworkspace/crates/AGENTS.md
- rsworkspace/crates/trogon-gateway/src/cli.rs
- rsworkspace/crates/trogon-source-telegram/Cargo.toml
- rsworkspace/crates/trogon-source-gitlab/Cargo.toml
- rsworkspace/crates/trogon-source-telegram/src/constants.rs
- rsworkspace/crates/trogon-source-discord/src/constants.rs
- rsworkspace/crates/trogon-source-gitlab/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- devops/docker/compose/services/trogon-gateway/Dockerfile
- rsworkspace/crates/trogon-source-linear/Cargo.toml
- rsworkspace/Cargo.toml
- devops/docker/compose/compose.yml
yordis
force-pushed
the
yordis/unified-gateway
branch
from
2aa8629 to
641bca0
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
4 times, most recently
from
0bf5032 to
009c701
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
from
04cb1e1 to
f2c0815
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
2 times, most recently
from
62efa59 to
0f058ca
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
4 times, most recently
from
3b4f358 to
90807b6
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
2 times, most recently
from
a619a6e to
b3f721c
Compare
yordis
force-pushed
the
yordis/unified-gateway
branch
4 times, most recently
from
d4d0d55 to
8d144c3
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8d144c3. Configure here.
yordis
force-pushed
the
yordis/unified-gateway
branch
7 times, most recently
from
d4c19ed to
15a8114
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/unified-gateway
branch
from
15a8114 to
407878d
Compare
yordis
added
the
rust:coverage-baseline-reset
This was referenced Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.