Conversation
PR SummaryMedium Risk Overview Wires the source into local Docker Compose (new service, env vars, optional ngrok tunnel), extends shared workspace deps with Reviewed by Cursor Bugbot for commit fa6ca2f. 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:
WalkthroughAdds a new trogon-source-discord service: Docker Compose entries and Dockerfile, a README, and a new Rust crate implementing Discord Gateway and Webhook ingestion with configuration, constants, signature verification, gateway bridge, webhook server, JetStream provisioning/publishing, a binary entrypoint, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant DiscordGateway as Discord Gateway
participant Shard as Twilight Shard
participant Bridge as GatewayBridge
participant JetStream as JetStream (NATS)
DiscordGateway->>Shard: Emit Gateway Event
Shard->>Bridge: next_event() -> Event
Bridge->>Bridge: map event -> name, serialize JSON
Bridge->>JetStream: publish("discord.<event>", payload, headers, ack_timeout)
JetStream-->>Bridge: Ack / Error
sequenceDiagram
participant HTTP as Discord (Webhook)
participant Axum as Axum Server
participant Sig as Signature Verifier
participant JetStream as JetStream (NATS)
HTTP->>Axum: POST /webhook (x-signature-ed25519, x-signature-timestamp, body)
Axum->>Sig: verify(public_key, timestamp, body, signature)
alt Signature Invalid
Sig-->>Axum: SignatureError
Axum-->>HTTP: 401 Unauthorized
else Signature Valid
Axum->>Axum: parse JSON -> type_id
alt type_id == 1 (PING)
Axum-->>HTTP: 200 {"type":1}
else Known Interaction
Axum->>JetStream: publish("discord.<type>", body, headers)
JetStream-->>Axum: Ack / Error
alt Publish Success
Axum-->>HTTP: 200 {"type":5 or 8}
else Publish Failure
Axum-->>HTTP: 500 Internal Server Error
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: fa6ca2f Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
yordis/trogon-source-discord
branch
3 times, most recently
from
b1d9f4b to
e495480
Compare
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
e495480 to
9a23880
Compare
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
9a23880 to
1c138b8
Compare
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
1c138b8 to
7ac50b0
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: 3
🧹 Nitpick comments (9)
rsworkspace/crates/trogon-source-discord/src/gateway.rs (5)
130-140: Redundant wildcard arm after explicitPingmatch.The
_ => Nonearm (Line 137) is unreachable afterInteraction::Ping(_) => None(Line 136) unless Serenity adds new variants in the future. If the intent is forward compatibility, consider adding a comment; otherwise, the wildcard can be removed.♻️ Option 1: Add comment for clarity
Interaction::Modal(i) => i.guild_id.map(|g| g.get()), Interaction::Ping(_) => None, - _ => None, + _ => None, // Forward compatibility for future interaction types🤖 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.rs` around lines 130 - 140, In interaction_create, the match includes an explicit Interaction::Ping(_) => None followed by an unreachable `_ => None` arm; remove the redundant wildcard arm (or if you intentionally want forward-compatibility, replace it with a short comment noting future Serenity variants) so the match only covers the concrete Interaction::Command/Component/Autocomplete/Modal arms plus the Ping arm; update the match in the interaction_create method accordingly to eliminate the unreachable `_` pattern.
46-77: Consider simplifying the outcome handling.The
is_ok()check followed bylog_on_errorworks but is slightly redundant sincelog_on_erroralready handles thePublishedcase as a no-op. You could simplify this.♻️ Suggested simplification
let outcome = publish_event( self.js.as_ref(), subject, headers, Bytes::from(json), self.nats_ack_timeout, ) .await; - if outcome.is_ok() { - debug!(event = event_name, "published gateway event to NATS"); - } else { - outcome.log_on_error(event_name); + outcome.log_on_error(event_name); + if outcome.is_ok() { + debug!(event = event_name, "published gateway event to NATS"); }🤖 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.rs` around lines 46 - 77, In publish (async fn publish) simplify the outcome handling by removing the explicit if outcome.is_ok() { debug!(...) } else { outcome.log_on_error(event_name); } branch and instead call outcome.log_on_error(event_name) unconditionally after awaiting publish_event; this relies on publish_event's return type and its log_on_error method to be a no-op on Published and to log errors otherwise, preserving behaviour while removing redundancy.
378-380:thread_member_updatealso publishes withguild_id: None.Similar to
thread_delete, theThreadMemberstruct might contain guild context. This could impact downstream consumers that filter by guild.🤖 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.rs` around lines 378 - 380, The handler thread_member_update currently calls publish with guild_id hardcoded to None; change it to forward the guild context from the ThreadMember by passing thread_member.guild_id (or the equivalent field on ThreadMember) into publish so downstream consumers can filter by guild; keep the None fallback if the ThreadMember.guild_id is None. Ensure the change is made in async fn thread_member_update and references the ThreadMember argument.
152-163: Missing guild_id inreaction_remove_allevent.The
reaction_remove_allhandler publishes withguild_id: None, but this event can occur in guild channels. The Serenityreaction_remove_allcallback doesn't provideguild_iddirectly, so this is a limitation of the Serenity API rather than a bug in this code. Consider documenting this limitation or potentially looking up the channel to retrieve the guild_id if needed downstream.🤖 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.rs` around lines 152 - 163, The reaction_remove_all handler currently omits guild_id in the published payload; update the ReactionRemoveAll payload to include an Option<u64> guild_id, attempt to resolve the channel's guild_id via the Context (e.g., look up channel with ctx.cache or ctx.http to get Channel.guild_id), set guild_id to Some(gid.get()) when present or None otherwise, and pass the new payload to self.publish in async fn reaction_remove_all so downstream consumers receive guild context when available; if lookup fails leave guild_id None and consider adding a brief comment documenting the Serenity limitation.
369-371: Extractguild_idfromPartialGuildChannelinthread_deletefor consistency with other event handlers.The
PartialGuildChannelstruct contains aguild_idfield. Currently passingNoneprevents downstream filtering by guild. Extract it using the same pattern asthread_create,thread_update, and other handlers:let guild_id = thread.guild_id.get(); self.publish("thread_delete", Some(guild_id), &thread).await;🤖 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.rs` around lines 369 - 371, The thread_delete handler currently calls self.publish with None for guild id; change it to extract the guild id from the PartialGuildChannel and pass it to publish like the other handlers. In the async fn thread_delete(&self, _ctx: Context, thread: PartialGuildChannel, _full_thread_data: Option<GuildChannel>) use thread.guild_id.get() to produce a guild_id and call self.publish("thread_delete", Some(guild_id), &thread).await so downstream filtering works consistently with thread_create/thread_update.rsworkspace/crates/trogon-source-discord/src/server.rs (2)
122-126:router()panics ifpublic_keyisNone.The
expect()will panic at runtime ifpublic_keyis not set. The caller (main.rsLine 67-68) guards this withif config.public_key.is_some(), but this is a runtime invariant that could be violated if refactored. Consider acceptingVerifyingKeydirectly instead of&DiscordConfig.♻️ Alternative: Accept VerifyingKey directly
-pub fn router<P: JetStreamPublisher>(js: P, config: &DiscordConfig) -> Router { - let public_key = config - .public_key - .expect("router requires DISCORD_PUBLIC_KEY to be set"); +pub fn router<P: JetStreamPublisher>( + js: P, + public_key: VerifyingKey, + subject_prefix: String, + nats_ack_timeout: Duration, + max_body_size: ByteSize, +) -> Router { let state = AppState { js, public_key, - subject_prefix: config.subject_prefix.clone(), - nats_ack_timeout: config.nats_ack_timeout, + subject_prefix, + nats_ack_timeout, }; // ... - .layer(RequestBodyLimitLayer::new( - config.max_body_size.as_u64() as usize, - )) + .layer(RequestBodyLimitLayer::new(max_body_size.as_u64() as usize))This makes the requirement explicit 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-discord/src/server.rs` around lines 122 - 126, The router function currently calls expect on config.public_key which can panic; change the router signature from fn router<P: JetStreamPublisher>(js: P, config: &DiscordConfig) -> Router to accept the parsed VerifyingKey (or whatever concrete type is used for public keys) directly — e.g. fn router<P: JetStreamPublisher>(js: P, public_key: VerifyingKey) -> Router — then update uses inside router (AppState construction and any references to config.public_key) to use that public_key parameter and remove the expect() call so the requirement is enforced at the type level (adjust callers of router to pass the validated VerifyingKey).
58-66: INFO-level logging on every successful publish may be noisy.Logging at INFO level for every published interaction could generate significant log volume in high-traffic environments. Consider DEBUG level for routine success.
♻️ Consider using DEBUG level for success
fn outcome_to_status<E: fmt::Display>(outcome: PublishOutcome<E>) -> StatusCode { if outcome.is_ok() { - info!("Published Discord interaction to NATS"); + tracing::debug!("Published Discord interaction to NATS"); StatusCode::OK } else { outcome.log_on_error("discord"); StatusCode::INTERNAL_SERVER_ERROR } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/server.rs` around lines 58 - 66, The function outcome_to_status currently logs every successful publish at INFO level (info!("Published Discord interaction to NATS")), which is noisy; change the success log to DEBUG level by replacing that info! call with debug! while keeping the same message, so PublishOutcome successes are logged at debug level; leave the error path using outcome.log_on_error("discord") and the StatusCode returns unchanged.rsworkspace/crates/trogon-source-discord/src/config.rs (1)
94-104: Default intents include privileged intents requiring Discord approval.
GUILD_MEMBERSandMESSAGE_CONTENTare privileged intents that require explicit enablement in the Discord Developer Portal. Consider documenting this requirement or logging a warning when privileged intents are used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/config.rs` around lines 94 - 104, The default_intents() function currently includes privileged GatewayIntents::GUILD_MEMBERS and GatewayIntents::MESSAGE_CONTENT; update the code to not enable these privileged intents by default and instead require explicit opt-in or surface a warning: remove GUILD_MEMBERS and MESSAGE_CONTENT from default_intents(), add documentation comment in config.rs explaining that those two intents are privileged and must be enabled in the Discord Developer Portal, and, where configuration is parsed (the code that calls default_intents() or builds GatewayIntents), log or return a clear warning if the user explicitly requests GUILD_MEMBERS or MESSAGE_CONTENT so the operator is informed about the approval requirement.rsworkspace/crates/trogon-source-discord/src/main.rs (1)
111-111:unreachable!()depends on config validation invariant.This relies on
DiscordConfig::from_envpanicking if neither token nor key is provided. While currently valid, if the config validation changes or is bypassed, this would cause an unclear panic. Consider a more descriptive error message.♻️ Consider a more descriptive message
- (None, None) => unreachable!("config validation ensures at least one is set"), + (None, None) => { + error!("Neither gateway nor interactions configured - this should have been caught by config validation"); + return Err("No input paths configured".into()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/main.rs` at line 111, The match arm using unreachable!("config validation ensures at least one is set") is fragile; replace it with a clear, descriptive failure that includes context (e.g., panic! or return Err) so the error is informative if validation is bypassed — update the (None, None) arm to panic!("Neither Discord token nor key were provided: DiscordConfig::from_env must supply at least one") or, preferably, return a Result::Err with that message from main so callers get a descriptive error instead of an ambiguous unreachable! panic; reference the match arm and DiscordConfig::from_env when making the change.
🤖 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-source-discord/README.md`:
- Around line 79-90: The environment variables table in README.md is missing
DISCORD_STREAM_MAX_AGE_SECS which is documented in .env.example; add a table row
for `DISCORD_STREAM_MAX_AGE_SECS` (required: no, Default: `604800`, Description:
max age in seconds for JetStream stream messages or similar) so the README
matches the .env.example and code expectations.
In `@rsworkspace/crates/trogon-source-discord/src/main.rs`:
- Around line 86-112: The current match over (gateway_handle,
interactions_handle) only logs Ok(Err(e)) from JoinHandle results and ignores
Err(join_error) which means the task panicked; update all places that consume
the join handle (the tokio::select! arms and the single-await branches for gw
and http) to also match and log Err(join_error) (and include the join_error
details in the log) in addition to the existing Ok(Err(e)) handling so panics
are not silently ignored — specifically adjust the logic around gateway_handle /
interactions_handle (variables gw and http) and the tokio::select! arms to
handle both Ok(Err(e)) and Err(join_error) cases with appropriate error!
logging.
In `@rsworkspace/crates/trogon-source-discord/src/server.rs`:
- Around line 249-253: The code currently defaults interaction_id to the literal
"unknown" when parsed.get("id") is missing, which can cause NATS deduplication
collisions; update the handling in the function/block that assigns
interaction_id (the parsed.get("id").and_then(|v| v.as_str()).unwrap_or(...)
code) to either (a) reject the request early with an error/response when the id
field is missing, or (b) generate a unique fallback ID (e.g., a UUID/v4 or a
monotonic counter) and use that as the NATS message ID instead of the fixed
"unknown", ensuring the rest of the flow that publishes to NATS uses the new
variable.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-source-discord/src/config.rs`:
- Around line 94-104: The default_intents() function currently includes
privileged GatewayIntents::GUILD_MEMBERS and GatewayIntents::MESSAGE_CONTENT;
update the code to not enable these privileged intents by default and instead
require explicit opt-in or surface a warning: remove GUILD_MEMBERS and
MESSAGE_CONTENT from default_intents(), add documentation comment in config.rs
explaining that those two intents are privileged and must be enabled in the
Discord Developer Portal, and, where configuration is parsed (the code that
calls default_intents() or builds GatewayIntents), log or return a clear warning
if the user explicitly requests GUILD_MEMBERS or MESSAGE_CONTENT so the operator
is informed about the approval requirement.
In `@rsworkspace/crates/trogon-source-discord/src/gateway.rs`:
- Around line 130-140: In interaction_create, the match includes an explicit
Interaction::Ping(_) => None followed by an unreachable `_ => None` arm; remove
the redundant wildcard arm (or if you intentionally want forward-compatibility,
replace it with a short comment noting future Serenity variants) so the match
only covers the concrete Interaction::Command/Component/Autocomplete/Modal arms
plus the Ping arm; update the match in the interaction_create method accordingly
to eliminate the unreachable `_` pattern.
- Around line 46-77: In publish (async fn publish) simplify the outcome handling
by removing the explicit if outcome.is_ok() { debug!(...) } else {
outcome.log_on_error(event_name); } branch and instead call
outcome.log_on_error(event_name) unconditionally after awaiting publish_event;
this relies on publish_event's return type and its log_on_error method to be a
no-op on Published and to log errors otherwise, preserving behaviour while
removing redundancy.
- Around line 378-380: The handler thread_member_update currently calls publish
with guild_id hardcoded to None; change it to forward the guild context from the
ThreadMember by passing thread_member.guild_id (or the equivalent field on
ThreadMember) into publish so downstream consumers can filter by guild; keep the
None fallback if the ThreadMember.guild_id is None. Ensure the change is made in
async fn thread_member_update and references the ThreadMember argument.
- Around line 152-163: The reaction_remove_all handler currently omits guild_id
in the published payload; update the ReactionRemoveAll payload to include an
Option<u64> guild_id, attempt to resolve the channel's guild_id via the Context
(e.g., look up channel with ctx.cache or ctx.http to get Channel.guild_id), set
guild_id to Some(gid.get()) when present or None otherwise, and pass the new
payload to self.publish in async fn reaction_remove_all so downstream consumers
receive guild context when available; if lookup fails leave guild_id None and
consider adding a brief comment documenting the Serenity limitation.
- Around line 369-371: The thread_delete handler currently calls self.publish
with None for guild id; change it to extract the guild id from the
PartialGuildChannel and pass it to publish like the other handlers. In the async
fn thread_delete(&self, _ctx: Context, thread: PartialGuildChannel,
_full_thread_data: Option<GuildChannel>) use thread.guild_id.get() to produce a
guild_id and call self.publish("thread_delete", Some(guild_id), &thread).await
so downstream filtering works consistently with thread_create/thread_update.
In `@rsworkspace/crates/trogon-source-discord/src/main.rs`:
- Line 111: The match arm using unreachable!("config validation ensures at least
one is set") is fragile; replace it with a clear, descriptive failure that
includes context (e.g., panic! or return Err) so the error is informative if
validation is bypassed — update the (None, None) arm to panic!("Neither Discord
token nor key were provided: DiscordConfig::from_env must supply at least one")
or, preferably, return a Result::Err with that message from main so callers get
a descriptive error instead of an ambiguous unreachable! panic; reference the
match arm and DiscordConfig::from_env when making the change.
In `@rsworkspace/crates/trogon-source-discord/src/server.rs`:
- Around line 122-126: The router function currently calls expect on
config.public_key which can panic; change the router signature from fn router<P:
JetStreamPublisher>(js: P, config: &DiscordConfig) -> Router to accept the
parsed VerifyingKey (or whatever concrete type is used for public keys) directly
— e.g. fn router<P: JetStreamPublisher>(js: P, public_key: VerifyingKey) ->
Router — then update uses inside router (AppState construction and any
references to config.public_key) to use that public_key parameter and remove the
expect() call so the requirement is enforced at the type level (adjust callers
of router to pass the validated VerifyingKey).
- Around line 58-66: The function outcome_to_status currently logs every
successful publish at INFO level (info!("Published Discord interaction to
NATS")), which is noisy; change the success log to DEBUG level by replacing that
info! call with debug! while keeping the same message, so PublishOutcome
successes are logged at debug level; leave the error path using
outcome.log_on_error("discord") and the StatusCode returns unchanged.
🪄 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: 292b0834-438a-40a3-a94b-b22a3c098490
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-source-discord/Dockerfiledevops/docker/compose/services/trogon-source-discord/README.mdrsworkspace/crates/acp-telemetry/src/service_name.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/lib.rsrsworkspace/crates/trogon-source-discord/src/main.rsrsworkspace/crates/trogon-source-discord/src/server.rsrsworkspace/crates/trogon-source-discord/src/signature.rs
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
7ac50b0 to
50a4735
Compare
yordis
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-source-discord/src/config.rs`:
- Around line 25-34: DiscordConfig currently accepts subject_prefix and
stream_name as raw Strings (fields subject_prefix and stream_name), allowing
blank/invalid NATS identifiers; replace or guard these by introducing/using
domain value objects (e.g., NatsSubjectPrefix, StreamName) or perform
trimming+validation in the config factory/constructor (reject empty after trim
and validate allowed NATS identifier characters/length) so invalid instances are
unrepresentable; update all call sites that construct DiscordConfig to use the
validated factories and apply the same change/validation to the other config
struct fields referenced around lines 81-86.
- Around line 50-55: Change the default_intents() implementation so it returns
only non-privileged intents (use GatewayIntents::non_privileged()) instead of
including GUILD_MEMBERS and MESSAGE_CONTENT; keep parse_gateway_intents(&s) and
the DISCORD_GATEWAY_INTENTS env var behavior the same but document in comments
that privileged intents must be explicitly opted into via
DISCORD_GATEWAY_INTENTS (e.g. "non_privileged,guild_members,message_content") so
bots without portal-enabled privileged intents won’t be disconnected on startup.
- Around line 15-23: The current SourceMode enum (variants Gateway and Webhook)
forces mutually exclusive ingestion paths; change it to a configuration struct
that allows both paths to be enabled simultaneously by replacing pub enum
SourceMode { Gateway { bot_token: String, intents: GatewayIntents }, Webhook {
public_key: VerifyingKey }, } with a struct like SourceConfig { gateway:
Option<GatewayConfig>, webhook: Option<WebhookConfig> } (define GatewayConfig
and WebhookConfig with the existing fields), update serde (if present) and all
usages of SourceMode (constructors, match arms, functions expecting SourceMode)
to handle Option<GatewayConfig> and Option<WebhookConfig>, and ensure code that
previously assumed exclusivity (e.g., connection setup/validation logic) now
permits both to coexist and prefers the interactions receiver when an
interactions endpoint/public_key is configured while still running the gateway
bridge when gateway config is present.
In `@rsworkspace/crates/trogon-source-discord/src/gateway.rs`:
- Around line 170-180: Update the wrapper payloads to preserve and forward
optional context instead of dropping it: in guild_member_removal add a
member_data_if_available: Option<Member> field to the MemberRemove payload and
pass the incoming _member_data_if_available through to publish; in
guild_role_delete include the role (or optional role data param) on the payload
instead of discarding _role; in integration_delete include the optional
application_id on the payload and forward the incoming _application_id; in
thread_delete add fields for full_thread_data: Option<Channel> (or the thread
type used) and guild_id: Option<u64>, extract guild_id from the thread parameter
(e.g. thread.guild_id() or thread.guild_id) when present and forward the
full_thread_data instead of hard-coding None; update the corresponding publish
calls (event names: "guild_member_remove", "guild_role_delete",
"integration_delete", "thread_delete") to send these enriched payloads. Ensure
the new payload structs are serde::Serialize and use .get() on IDs only when
present.
- Around line 46-76: The gateway publish path currently awaits publish_event
(which waits for JetStream ACK) inside async fn publish, blocking callbacks;
change it to enqueue the prepared message into a bounded mpsc channel and return
immediately. Create a background publisher task (spawned when the gateway/Js
client is created) that reads from the channel and calls
publish_event(self.js.as_ref(), subject, headers, Bytes::from(json),
self.nats_ack_timeout). Use a bounded tokio::sync::mpsc channel to provide
backpressure, use try_send (or try_send + a drop/log strategy) in publish to
avoid blocking the callback, and have the background task log
outcome.log_on_error(event_name) / debug on success. Ensure unique symbols:
modify async fn publish to build subject/header/json then send to the new
channel instead of awaiting publish_event; add a publisher loop that uses
publish_event and handles errors.
🪄 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: f594c8de-77d7-4c41-80a1-3c8d5e2c322a
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-source-discord/Dockerfiledevops/docker/compose/services/trogon-source-discord/README.mdrsworkspace/crates/acp-telemetry/src/service_name.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/lib.rsrsworkspace/crates/trogon-source-discord/src/main.rsrsworkspace/crates/trogon-source-discord/src/server.rsrsworkspace/crates/trogon-source-discord/src/signature.rs
✅ Files skipped from review due to trivial changes (6)
- devops/docker/compose/services/trogon-source-discord/README.md
- rsworkspace/crates/trogon-source-discord/Cargo.toml
- rsworkspace/crates/trogon-source-discord/src/lib.rs
- devops/docker/compose/services/trogon-source-discord/Dockerfile
- rsworkspace/crates/trogon-source-discord/src/constants.rs
- rsworkspace/crates/trogon-source-discord/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rsworkspace/crates/acp-telemetry/src/service_name.rs
- devops/docker/compose/.env.example
- devops/docker/compose/compose.yml
- rsworkspace/crates/trogon-source-discord/src/main.rs
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
50a4735 to
af3ba04
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-source-discord/src/config.rs (1)
81-86: Inconsistent empty-string handling for optional fields.
bot_tokenandpublic_key_hexcorrectly filter empty strings (lines 47, 63), butsubject_prefixandstream_namedo not. If a user explicitly setsDISCORD_SUBJECT_PREFIX="", the empty string is accepted rather than falling back to the default, which could cause cryptic NATS errors at runtime.🔧 Proposed fix to filter empty strings consistently
subject_prefix: env .var("DISCORD_SUBJECT_PREFIX") + .ok() + .filter(|s| !s.is_empty()) - .unwrap_or_else(|_| DEFAULT_SUBJECT_PREFIX.to_string()), + .unwrap_or_else(|| DEFAULT_SUBJECT_PREFIX.to_string()), stream_name: env .var("DISCORD_STREAM_NAME") + .ok() + .filter(|s| !s.is_empty()) - .unwrap_or_else(|_| DEFAULT_STREAM_NAME.to_string()), + .unwrap_or_else(|| DEFAULT_STREAM_NAME.to_string()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/config.rs` around lines 81 - 86, subject_prefix and stream_name accept explicit empty strings from the environment; update their parsing to mirror bot_token/public_key_hex by filtering out empty values and falling back to DEFAULT_SUBJECT_PREFIX/DEFAULT_STREAM_NAME. Locate the code that sets subject_prefix and stream_name in config.rs and replace the direct unwrap_or_else(...) with a pattern that reads the env var, maps/ok() it, filters out empty strings (e.g., .ok().filter(|s| !s.is_empty())), and then unwraps to the default constants DEFAULT_SUBJECT_PREFIX and DEFAULT_STREAM_NAME so empty env values behave like missing ones.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rsworkspace/crates/trogon-source-discord/src/config.rs`:
- Around line 81-86: subject_prefix and stream_name accept explicit empty
strings from the environment; update their parsing to mirror
bot_token/public_key_hex by filtering out empty values and falling back to
DEFAULT_SUBJECT_PREFIX/DEFAULT_STREAM_NAME. Locate the code that sets
subject_prefix and stream_name in config.rs and replace the direct
unwrap_or_else(...) with a pattern that reads the env var, maps/ok() it, filters
out empty strings (e.g., .ok().filter(|s| !s.is_empty())), and then unwraps to
the default constants DEFAULT_SUBJECT_PREFIX and DEFAULT_STREAM_NAME so empty
env values behave like missing ones.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53e376c9-79c4-4dba-aac2-9226cbbabdf7
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-source-discord/Dockerfiledevops/docker/compose/services/trogon-source-discord/README.mdrsworkspace/crates/acp-telemetry/src/service_name.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/lib.rsrsworkspace/crates/trogon-source-discord/src/main.rsrsworkspace/crates/trogon-source-discord/src/server.rsrsworkspace/crates/trogon-source-discord/src/signature.rs
✅ Files skipped from review due to trivial changes (7)
- devops/docker/compose/.env.example
- devops/docker/compose/services/trogon-source-discord/Dockerfile
- rsworkspace/crates/trogon-source-discord/Cargo.toml
- devops/docker/compose/services/trogon-source-discord/README.md
- rsworkspace/crates/trogon-source-discord/src/signature.rs
- rsworkspace/crates/trogon-source-discord/src/main.rs
- rsworkspace/crates/trogon-source-discord/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rsworkspace/crates/acp-telemetry/src/service_name.rs
- rsworkspace/crates/trogon-source-discord/src/lib.rs
- devops/docker/compose/compose.yml
- rsworkspace/crates/trogon-source-discord/src/server.rs
- rsworkspace/crates/trogon-source-discord/src/gateway.rs
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
af3ba04 to
2e2e20b
Compare
yordis
force-pushed
the
yordis/trogon-source-discord
branch
4 times, most recently
from
2bb5fab to
0668af3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rsworkspace/crates/trogon-source-discord/src/gateway.rs (1)
54-55:⚠️ Potential issue | 🟠 MajorGateway intake is still blocked on JetStream ACK round-trips.
Line 54 and Line 77 await publishing/ACK on the hot event path; combined with
bridge.dispatch(&event).awaitin the shard loop, broker slowness can stall gateway event processing up tonats_ack_timeoutper event.#!/bin/bash set -euo pipefail gw="$(fd -p 'gateway.rs' | rg 'trogon-source-discord/src/gateway.rs$')" mainf="$(fd -p 'main.rs' | rg 'trogon-source-discord/src/main.rs$')" pubf="$(fd -p 'publish.rs' | rg 'trogon-nats/src/jetstream/publish.rs$')" echo "=== gateway dispatch/publish await chain ===" rg -n -C2 'dispatch\(|publish_bytes\(|publish_event\(' "$gw" echo echo "=== gateway event loop call site ===" rg -n -C2 'next_event|dispatch\(&event\)\.await' "$mainf" echo echo "=== publish_event ack timeout behavior ===" rg -n -C3 'publish_with_headers|timeout\(ack_timeout' "$pubf"Also applies to: 77-84
🤖 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.rs` around lines 54 - 55, The gateway is blocked because the shard loop awaits publish_bytes (and similarly the other publish path), which ties broker ACK latency into bridge.dispatch(&event).await; change the hot-path publish to fire-and-forget: spawn a detached task (e.g., tokio::spawn) that calls publish_bytes asynchronously and logs any error instead of awaiting the ACK inline; ensure you clone/move required values (name, guild_id, dedup_id, json or a prepared Bytes) or the client handle (e.g., Arc/clone of the publisher) so the spawned task does not borrow the shard loop or self, and leave bridge.dispatch(&event).await unchanged so gateway intake is no longer blocked by JetStream ACKs.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-source-discord/src/signature.rs (1)
7-11: Consider renamingInvalidSignatureHextoInvalidHexfor semantic accuracy.This variant is reused in
parse_public_key()(line 60) for public key hex decoding errors, but its name and display message ("invalid hex in signature") are signature-specific. This could confuse callers who see a signature-related error when parsing a public key.♻️ Suggested refactor
pub enum SignatureError { InvalidPublicKey, - InvalidSignatureHex(hex::FromHexError), + InvalidHex(hex::FromHexError), InvalidSignatureLength, Mismatch, }And update the Display impl accordingly:
- SignatureError::InvalidSignatureHex(_) => f.write_str("invalid hex in signature"), + SignatureError::InvalidHex(_) => f.write_str("invalid hex encoding"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-discord/src/signature.rs` around lines 7 - 11, The enum variant name InvalidSignatureHex in SignatureError is misleading because it’s also used for public-key hex decoding in parse_public_key(); rename the variant to InvalidHex (and update any pattern matches) and update the Display implementation to use a generic message (e.g., "invalid hex") rather than "invalid hex in signature" so both signature and public-key hex decode errors are accurately represented; ensure you update all references to SignatureError::InvalidSignatureHex (including in parse_public_key and any tests) to SignatureError::InvalidHex.
🤖 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-source-discord/src/gateway.rs`:
- Around line 301-304: The current deduplication uses dedup_key2(event_name,
guild_id, user.id) for lifecycle events (Event::MemberAdd, Event::MemberRemove,
Event::BanAdd, Event::BanRemove) which can erroneously drop legitimate repeat
events; change the logic in the match/handler that generates the NATS-Msg-Id so
these lifecycle events do not call dedup_key2 and instead return no dedup key
(or a truly unique key if you can derive one per event) for
MemberAdd/MemberRemove/BanAdd/BanRemove; update the branch handling these
variants in the function that builds the dedup key (where dedup_key2 is
currently invoked) to skip deduplication for those event variants.
---
Duplicate comments:
In `@rsworkspace/crates/trogon-source-discord/src/gateway.rs`:
- Around line 54-55: The gateway is blocked because the shard loop awaits
publish_bytes (and similarly the other publish path), which ties broker ACK
latency into bridge.dispatch(&event).await; change the hot-path publish to
fire-and-forget: spawn a detached task (e.g., tokio::spawn) that calls
publish_bytes asynchronously and logs any error instead of awaiting the ACK
inline; ensure you clone/move required values (name, guild_id, dedup_id, json or
a prepared Bytes) or the client handle (e.g., Arc/clone of the publisher) so the
spawned task does not borrow the shard loop or self, and leave
bridge.dispatch(&event).await unchanged so gateway intake is no longer blocked
by JetStream ACKs.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-source-discord/src/signature.rs`:
- Around line 7-11: The enum variant name InvalidSignatureHex in SignatureError
is misleading because it’s also used for public-key hex decoding in
parse_public_key(); rename the variant to InvalidHex (and update any pattern
matches) and update the Display implementation to use a generic message (e.g.,
"invalid hex") rather than "invalid hex in signature" so both signature and
public-key hex decode errors are accurately represented; ensure you update all
references to SignatureError::InvalidSignatureHex (including in parse_public_key
and any tests) to SignatureError::InvalidHex.
🪄 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: a46c8889-3725-401f-bca8-43a6570d60b3
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-source-discord/Dockerfiledevops/docker/compose/services/trogon-source-discord/README.mdrsworkspace/Cargo.tomlrsworkspace/crates/acp-telemetry/src/service_name.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/lib.rsrsworkspace/crates/trogon-source-discord/src/main.rsrsworkspace/crates/trogon-source-discord/src/server.rsrsworkspace/crates/trogon-source-discord/src/signature.rs
✅ Files skipped from review due to trivial changes (9)
- rsworkspace/Cargo.toml
- devops/docker/compose/.env.example
- devops/docker/compose/services/trogon-source-discord/Dockerfile
- rsworkspace/crates/trogon-source-discord/Cargo.toml
- rsworkspace/crates/trogon-source-discord/src/lib.rs
- devops/docker/compose/services/trogon-source-discord/README.md
- rsworkspace/crates/trogon-source-discord/src/constants.rs
- rsworkspace/crates/trogon-source-discord/src/server.rs
- rsworkspace/crates/trogon-source-discord/src/config.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rsworkspace/crates/acp-telemetry/src/service_name.rs
- devops/docker/compose/compose.yml
- rsworkspace/crates/trogon-source-discord/src/main.rs
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
0dfcf98 to
ea3af55
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rsworkspace/crates/trogon-source-discord/src/gateway.rs (1)
50-51:⚠️ Potential issue | 🟠 MajorDon’t wait for JetStream ACKs on the gateway hot path.
dispatch()blocks untilpublish_event()finishes its ACK round-trip (or timeout). Since this bridge forwards every gateway event, one slow NATS ACK will backpressure the entire Discord feed. Hand the payload off to a bounded channel and let a background publisher task own the JetStream publish/ACK path instead.Also applies to: 73-80
🤖 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.rs` around lines 50 - 51, The gateway currently awaits JetStream ACKs on the hot path (calls to self.publish_bytes(...) from dispatch()), which blocks dispatch() and causes backpressure; instead create a bounded mpsc channel and a background publisher task (e.g., spawn a publisher owning the JetStream client used by publish_bytes/publish_event) and change dispatch() to enqueue the Bytes payload + metadata (name, guild_id, dedup_id) into that channel without awaiting ACKs; apply the same change to the other hot-path call sites (the similar calls around lines 73-80) so those places also send to the channel, and ensure the background task performs the actual publish_bytes/publish_event calls and handles ACKs/retries/timeouts there.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-source-discord/src/gateway.rs (1)
468-543: Please add a regression test for the member/ban dedup carve-out.The new
extract_dedup_id()branch is the subtle part of this change, but the suite still only proves generic msg-id present/absent behavior. A targeteddispatch()case forMemberAdd/MemberRemove/BanAdd/BanRemovewould lock in the false-collision fix.🤖 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.rs` around lines 468 - 543, Add a regression test that exercises the new extract_dedup_id() branch by calling bridge_with_mock() and b.dispatch() with Event::MemberAdd, Event::MemberRemove, Event::BanAdd, and Event::BanRemove and asserting that their published NATS message IDs (headers.get(async_nats::header::NATS_MESSAGE_ID)) are either omitted or unique per the dedup carve-out; locate this behavior around the dispatch() function and the extract_dedup_id() logic and add assertions similar to the existing publish/dispatch tests to ensure no false collisions occur for member/ban events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@rsworkspace/crates/trogon-source-discord/src/gateway.rs`:
- Around line 50-51: The gateway currently awaits JetStream ACKs on the hot path
(calls to self.publish_bytes(...) from dispatch()), which blocks dispatch() and
causes backpressure; instead create a bounded mpsc channel and a background
publisher task (e.g., spawn a publisher owning the JetStream client used by
publish_bytes/publish_event) and change dispatch() to enqueue the Bytes payload
+ metadata (name, guild_id, dedup_id) into that channel without awaiting ACKs;
apply the same change to the other hot-path call sites (the similar calls around
lines 73-80) so those places also send to the channel, and ensure the background
task performs the actual publish_bytes/publish_event calls and handles
ACKs/retries/timeouts there.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-source-discord/src/gateway.rs`:
- Around line 468-543: Add a regression test that exercises the new
extract_dedup_id() branch by calling bridge_with_mock() and b.dispatch() with
Event::MemberAdd, Event::MemberRemove, Event::BanAdd, and Event::BanRemove and
asserting that their published NATS message IDs
(headers.get(async_nats::header::NATS_MESSAGE_ID)) are either omitted or unique
per the dedup carve-out; locate this behavior around the dispatch() function and
the extract_dedup_id() logic and add assertions similar to the existing
publish/dispatch tests to ensure no false collisions occur for member/ban
events.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b034b71-92a7-4782-9e51-0723aec3f48d
📒 Files selected for processing (2)
rsworkspace/crates/trogon-source-discord/src/config.rsrsworkspace/crates/trogon-source-discord/src/gateway.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rsworkspace/crates/trogon-source-discord/src/config.rs
yordis
force-pushed
the
yordis/trogon-source-discord
branch
2 times, most recently
from
bfec406 to
6c42d35
Compare
yordis
force-pushed
the
yordis/trogon-source-discord
branch
7 times, most recently
from
8511f18 to
26f462b
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 26f462b. Configure here.
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
26f462b to
91c4b29
Compare
…t-reply Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/trogon-source-discord
branch
from
91c4b29 to
fa6ca2f
Compare
yordis
added
the
rust:coverage-baseline-reset
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.
trogon-source-*pattern as a dumb inbound pipe into NATS JetStreamDISCORD_MODE:gateway(WebSocket, all events) orwebhook(HTTP Interactions Endpoint, interactions only)