Conversation
PR SummaryMedium Risk Overview Introduces a new Updates Docker Compose Reviewed by Cursor Bugbot for commit c985548. 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 incident.io webhook source crate and integrates it into trogon-gateway: new crate, config/validation types, signature verification, Axum POST /incidentio/webhook router, JetStream provisioning and claim-check publishing, Docker compose/env entries, documentation, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/incident.io
participant Handler as Webhook Handler
participant Verifier as Signature Verifier
participant Parser as JSON Parser
participant Validator as EventType Validator
participant Publisher as ClaimCheckPublisher
participant JetStream as NATS JetStream
Client->>Handler: POST /incidentio/webhook (headers + body)
Handler->>Verifier: verify(headers, body, signing_secret, tolerance)
alt signature invalid
Verifier-->>Handler: SignatureError
Handler-->>Client: 401 Unauthorized
else signature valid
Verifier-->>Handler: VerifiedWebhook{id,timestamp}
Handler->>Parser: parse body as JSON
alt invalid JSON
Parser-->>Handler: ParseError
Handler->>Publisher: publish to <prefix>.unroutable (with reject header)
Publisher->>JetStream: publish claim-check
JetStream-->>Publisher: ack / error
Handler-->>Client: 200 OK / 500 on publish error
else valid JSON
Parser-->>Handler: {event_type,...}
Handler->>Validator: validate event_type
alt invalid event_type
Validator-->>Handler: IncidentioEventTypeError
Handler->>Publisher: publish to <prefix>.unroutable (with reject header)
Publisher->>JetStream: publish
JetStream-->>Publisher: ack / error
Handler-->>Client: 200 OK / 500 on publish error
else valid event_type
Validator-->>Handler: IncidentioEventType
Handler->>Publisher: publish original body to <prefix>.<event_type> (with headers)
Publisher->>JetStream: publish claim-check
JetStream-->>Publisher: ack / error
alt publish success
Handler-->>Client: 200 OK
else publish failure
Handler-->>Client: 500 Internal Server Error
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: c985548 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
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 22dbe4c. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-source-incidentio/src/incidentio_event_type.rs (1)
22-23: Consider implementingsource()to preserve error chain.The
IncidentioEventTypeErrorwraps aSubjectTokenViolationbut doesn't expose it viasource(). While not critical since theDisplayimpl provides good messages, implementingsource()would preserve the full error chain for debugging.♻️ Optional: implement source() for error chain
-impl std::error::Error for IncidentioEventTypeError {} +impl std::error::Error for IncidentioEventTypeError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.0) + } +}This requires
SubjectTokenViolationto implementstd::error::Error. Verify that it does before applying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-incidentio/src/incidentio_event_type.rs` around lines 22 - 23, The impl of std::error::Error for IncidentioEventTypeError should implement source() to return the wrapped SubjectTokenViolation so the error chain is preserved; update the impl for IncidentioEventTypeError to add a fn source(&self) -> Option<&(dyn std::error::Error + 'static)> that returns Some(&self.0) (or the appropriate field holding the SubjectTokenViolation), and before applying ensure SubjectTokenViolation itself implements std::error::Error (if it doesn't, either implement Error for it or wrap/convert it to a type that does).rsworkspace/crates/trogon-source-incidentio/src/server.rs (1)
51-71: Consider adding webhook metadata headers to unroutable messages.Unlike routable messages (lines 193-203), unroutable messages don't include
webhook_idorwebhook_timestampheaders. This metadata could be useful for debugging rejected webhooks. However, since unroutable is called beforeverifiedis available in some code paths (invalid JSON), this would require restructuring.This is a minor observability improvement and not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-incidentio/src/server.rs` around lines 51 - 71, publish_unroutable currently sets only NATS_HEADER_REJECT_REASON and omits webhook metadata; update the signature of publish_unroutable (the async fn publish_unroutable<P: JetStreamPublisher, S: ObjectStorePut>) to accept optional webhook metadata parameters (e.g. webhook_id: Option<&str> and webhook_timestamp: Option<&str> or Option<impl Into<String>>), then when building async_nats::HeaderMap insert NATS_HEADER_REJECT_REASON as before and also insert headers for "webhook_id" and "webhook_timestamp" only when their Option is Some; finally update all call sites to pass actual webhook values when available (or None for invalid-JSON/unverified paths) so unroutable messages include metadata when present.
🤖 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-incidentio/src/incidentio_event_type.rs`:
- Around line 22-23: The impl of std::error::Error for IncidentioEventTypeError
should implement source() to return the wrapped SubjectTokenViolation so the
error chain is preserved; update the impl for IncidentioEventTypeError to add a
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> that returns
Some(&self.0) (or the appropriate field holding the SubjectTokenViolation), and
before applying ensure SubjectTokenViolation itself implements std::error::Error
(if it doesn't, either implement Error for it or wrap/convert it to a type that
does).
In `@rsworkspace/crates/trogon-source-incidentio/src/server.rs`:
- Around line 51-71: publish_unroutable currently sets only
NATS_HEADER_REJECT_REASON and omits webhook metadata; update the signature of
publish_unroutable (the async fn publish_unroutable<P: JetStreamPublisher, S:
ObjectStorePut>) to accept optional webhook metadata parameters (e.g.
webhook_id: Option<&str> and webhook_timestamp: Option<&str> or Option<impl
Into<String>>), then when building async_nats::HeaderMap insert
NATS_HEADER_REJECT_REASON as before and also insert headers for "webhook_id" and
"webhook_timestamp" only when their Option is Some; finally update all call
sites to pass actual webhook values when available (or None for
invalid-JSON/unverified paths) so unroutable messages include metadata when
present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63f8a2af-b635-4971-88fb-f1d09d0a4670
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-gateway/README.mdrsworkspace/Cargo.tomlrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/streams.rsrsworkspace/crates/trogon-source-incidentio/Cargo.tomlrsworkspace/crates/trogon-source-incidentio/src/config.rsrsworkspace/crates/trogon-source-incidentio/src/constants.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_event_type.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rsrsworkspace/crates/trogon-source-incidentio/src/lib.rsrsworkspace/crates/trogon-source-incidentio/src/server.rsrsworkspace/crates/trogon-source-incidentio/src/signature.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
186-209: Prefer typed value objects at the Incident.io config boundary.This section introduces raw
String/u64primitives for domain fields, which allows invalid states before resolution. Consider shifting these to domain-specific value objects (or typed wrappers) as early as possible in construction.As per coding guidelines,
Prefer domain-specific value objects over primitives (e.g., AcpPrefix not String). Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable.🤖 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 186 - 209, The IncidentioConfig uses raw primitives (String/u64) for domain fields which permits invalid states; replace those primitives with domain-specific value objects (e.g., SubjectPrefix, StreamName, SigningSecretOption wrapper, StreamMaxAgeSecs, NatsAckTimeoutSecs, TimestampToleranceSecs) that enforce validation at construction and make invalid states unrepresentable. Update the IncidentioConfig definition to use these types for the fields signing_secret, subject_prefix, stream_name, stream_max_age_secs, nats_ack_timeout_secs, and timestamp_tolerance_secs, implement fallible factories (or FromStr/Deserialize) on each value object to validate env input when the config is built, and add conversion/mapper code where the config is consumed so callers receive already-validated domain types. Ensure each type's factory returns a Result or implements TryFrom/FromStr with clear errors and wire those into your config loading path so invalid env values fail fast.
🤖 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`:
- Line 1072: The test data currently inlines literal-looking secrets like
"whsec_dGVzdC1zZWNyZXQ=" causing scanners to flag them; update tests to generate
the valid test secret at runtime via a helper (e.g., add a function
build_test_whsec() and call it from incidentio_toml or the call site) and
replace all direct uses (references: incidentio_toml and write_toml) with calls
that use that helper; apply the same replacement for the other occurrences
mentioned (lines around the other incidentio_toml/write_toml calls) so no
literal-looking secrets remain in the source.
- Around line 718-759: The code is converting typed parse errors into Strings
(e.g., in the NatsToken::new, NonZeroDuration::from_secs and
StreamMaxAge::from_secs branches) which loses error context; define a typed
validation error enum/struct (e.g., ConfigValidationError with variants like
InvalidSigningSecret { source: E }, InvalidSubjectPrefix { source: E },
InvalidStreamName { source: E }, ZeroDuration { field: &'static str } etc.),
replace the errors: Vec<String> with errors: Vec<ConfigValidationError>, and
when a parse fails push the appropriate ConfigValidationError variant carrying
the original source error instead of format!() and return/propagate that typed
error (or collect and return a Result/Err containing the typed errors) from the
resolver.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 186-209: The IncidentioConfig uses raw primitives (String/u64) for
domain fields which permits invalid states; replace those primitives with
domain-specific value objects (e.g., SubjectPrefix, StreamName,
SigningSecretOption wrapper, StreamMaxAgeSecs, NatsAckTimeoutSecs,
TimestampToleranceSecs) that enforce validation at construction and make invalid
states unrepresentable. Update the IncidentioConfig definition to use these
types for the fields signing_secret, subject_prefix, stream_name,
stream_max_age_secs, nats_ack_timeout_secs, and timestamp_tolerance_secs,
implement fallible factories (or FromStr/Deserialize) on each value object to
validate env input when the config is built, and add conversion/mapper code
where the config is consumed so callers receive already-validated domain types.
Ensure each type's factory returns a Result or implements TryFrom/FromStr with
clear errors and wire those into your config loading path so invalid env values
fail fast.
🪄 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: e993f6d2-6b1a-4cdf-92c1-5d5a30b9246a
📒 Files selected for processing (6)
devops/docker/compose/.env.exampledevops/docker/compose/services/trogon-gateway/README.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rsrsworkspace/crates/trogon-source-incidentio/src/server.rsrsworkspace/crates/trogon-source-incidentio/src/signature.rs
✅ Files skipped from review due to trivial changes (3)
- devops/docker/compose/services/trogon-gateway/README.md
- devops/docker/compose/.env.example
- rsworkspace/crates/trogon-source-incidentio/src/signature.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rs
- rsworkspace/crates/trogon-source-incidentio/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-source-incidentio/src/signature.rs (1)
20-21: Preserve missing-header context instead of collapsing toMissingHeaders.At Line 96, missing required headers are collapsed into a single
MissingHeaderserror. Include the header name to improve diagnostics and downstream handling.Proposed fix
pub enum SignatureError { - MissingHeaders, + MissingHeader { name: &'static str }, @@ - Self::MissingHeaders => f.write_str("missing required signature headers"), + Self::MissingHeader { name } => write!(f, "missing required header {name}"), @@ - .ok_or(SignatureError::MissingHeaders)? + .ok_or(SignatureError::MissingHeader { name })?Also applies to: 38-41, 93-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-incidentio/src/signature.rs` around lines 20 - 21, The enum currently collapses missing-required-header errors into the unit variant MissingHeaders; change the MissingHeaders variant to carry the header name (e.g., MissingHeaders(String) or &'static str) so the missing-header context is preserved, then update every place that constructs MissingHeaders (look for code in signature.rs that builds MissingHeaders around header parsing/validation) to pass the header name string, update any pattern matches on MissingHeaders to destructure the contained name, and run/adjust tests and From/Display/impl Error conversions that reference MissingHeaders to include the new payload; leave InvalidHeaderValue as-is.
🤖 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-incidentio/src/signature.rs`:
- Around line 62-66: Replace the primitive String fields in VerifiedWebhook
(webhook_id, webhook_timestamp) with domain value objects (e.g., WebhookId,
WebhookTimestamp) that enforce validation at construction; create small newtypes
or structs with validated constructors (new/from_str/try_from) that return
Result and guarantee only valid instances can be created, update the
VerifiedWebhook definition to use these types, and update all places that
construct or serialize/deserialize VerifiedWebhook (the verification/parsing
code that previously populated webhook_id and webhook_timestamp) to construct
the value objects and propagate errors; apply the same pattern to the other
similar struct(s) referenced in the review (the fields at the other occurrence)
so primitives are replaced by validated domain value objects.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-source-incidentio/src/signature.rs`:
- Around line 20-21: The enum currently collapses missing-required-header errors
into the unit variant MissingHeaders; change the MissingHeaders variant to carry
the header name (e.g., MissingHeaders(String) or &'static str) so the
missing-header context is preserved, then update every place that constructs
MissingHeaders (look for code in signature.rs that builds MissingHeaders around
header parsing/validation) to pass the header name string, update any pattern
matches on MissingHeaders to destructure the contained name, and run/adjust
tests and From/Display/impl Error conversions that reference MissingHeaders to
include the new payload; leave InvalidHeaderValue as-is.
🪄 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: 385c6c98-1069-474a-8d60-24fd7da3ae57
📒 Files selected for processing (3)
rsworkspace/crates/trogon-source-incidentio/src/constants.rsrsworkspace/crates/trogon-source-incidentio/src/server.rsrsworkspace/crates/trogon-source-incidentio/src/signature.rs
✅ Files skipped from review due to trivial changes (2)
- rsworkspace/crates/trogon-source-incidentio/src/constants.rs
- rsworkspace/crates/trogon-source-incidentio/src/server.rs
1ca84fa to
1c8ce42
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-incidentio/src/server.rs`:
- Around line 96-101: The JetStream stream is created with the default
duplicate_window (2m) but signature validation uses timestamp_tolerance (5m), so
set the stream's duplicate_window to match or exceed the configured
timestamp_tolerance to prevent replayed webhooks from bypassing dedupe; update
the async_nats::jetstream::stream::Config passed to js.get_or_create_stream
(alongside name/subjects/max_age) to include duplicate_window =
Duration::from_secs(config.timestamp_tolerance) or otherwise convert
config.timestamp_tolerance into the same Duration type so duplicate_window >=
timestamp_tolerance.
🪄 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: 3639c1c3-2088-4d15-95e2-de80eeba23e4
📒 Files selected for processing (5)
rsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-source-incidentio/src/config.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rsrsworkspace/crates/trogon-source-incidentio/src/server.rsrsworkspace/crates/trogon-source-incidentio/src/signature.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rsworkspace/crates/trogon-source-incidentio/src/config.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/streams.rs (1)
112-112: Consider asserting expected stream identities, not only total count.A count-only check can miss wrong stream selection/order regressions; asserting presence of expected stream names would make this test more resilient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/src/streams.rs` at line 112, The test currently only checks count via assert_eq!(js.created_streams().len(), 7) which misses wrong selections/order; update the assertion to verify the actual stream identities by mapping js.created_streams() to their identifying field (e.g., name or id) and assert that the resulting collection matches the expected list/set of stream names/ids (use assert_eq! for an exact set or assert!(contains) checks for presence), referencing created_streams() and the existing assert_eq! call so you replace the count-only check with a comparison against the expected identities.
🤖 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-gateway/src/streams.rs`:
- Line 112: The test currently only checks count via
assert_eq!(js.created_streams().len(), 7) which misses wrong selections/order;
update the assertion to verify the actual stream identities by mapping
js.created_streams() to their identifying field (e.g., name or id) and assert
that the resulting collection matches the expected list/set of stream names/ids
(use assert_eq! for an exact set or assert!(contains) checks for presence),
referencing created_streams() and the existing assert_eq! call so you replace
the count-only check with a comparison against the expected identities.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df3d894e-f1f6-4376-b090-bd325caae9b7
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
devops/docker/compose/.env.exampledevops/docker/compose/compose.ymldevops/docker/compose/services/trogon-gateway/README.mdrsworkspace/Cargo.tomlrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/streams.rsrsworkspace/crates/trogon-source-incidentio/Cargo.tomlrsworkspace/crates/trogon-source-incidentio/src/config.rsrsworkspace/crates/trogon-source-incidentio/src/constants.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_event_type.rsrsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rsrsworkspace/crates/trogon-source-incidentio/src/lib.rsrsworkspace/crates/trogon-source-incidentio/src/server.rsrsworkspace/crates/trogon-source-incidentio/src/signature.rs
✅ Files skipped from review due to trivial changes (5)
- rsworkspace/crates/trogon-gateway/Cargo.toml
- rsworkspace/Cargo.toml
- devops/docker/compose/.env.example
- rsworkspace/crates/trogon-source-incidentio/src/lib.rs
- rsworkspace/crates/trogon-source-incidentio/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- rsworkspace/crates/trogon-source-incidentio/src/config.rs
- devops/docker/compose/compose.yml
- devops/docker/compose/services/trogon-gateway/README.md
- rsworkspace/crates/trogon-source-incidentio/src/incidentio_signing_secret.rs
- rsworkspace/crates/trogon-gateway/src/config.rs
- rsworkspace/crates/trogon-source-incidentio/src/server.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
735437e to
c985548
Compare

Summary
trogon-source-incidentiocrate for Svix-signed incident.io webhookstrogon-gatewayconfig, routing, stream provisioning, compose env, and gateway docsevent_typerouting, setNats-Msg-Idfrom webhook ids, and publish authenticated malformed payloads toincidentio.unroutableTesting
CARGO_TARGET_DIR=/tmp/munich-target-incidentio mise exec -- cargo test -p trogon-source-incidentio --offlineCARGO_TARGET_DIR=/tmp/munich-target-gateway mise exec -- cargo test -p trogon-gateway --offline