fix(gateway): preserve inbound event fidelity for Sentry and Linear sources#124
Merged
fix(gateway): preserve inbound event fidelity for Sentry and Linear sources#124
Conversation
PR SummaryMedium Risk Overview
Docs and local compose env examples are updated for Sentry. Reviewer note: new env vars were introduced—please confirm each exists in every environment: Reviewed by Cursor Bugbot for commit 151b6dd. Bugbot is set up for automated code reviews on this repo. Configure here. |
WalkthroughAdds a new Sentry webhook source: introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant Sentry as "External Sentry"
participant Gateway as "Gateway / Handler"
participant Verifier as "Signature Verifier"
participant Parser as "Payload Parser"
participant JetStream as "NATS / JetStream"
Sentry->>Gateway: POST /sentry/webhook (body + sig header)
Gateway->>Verifier: verify(client_secret, body, sig_header)
alt signature valid
Verifier-->>Gateway: OK
Gateway->>Parser: parse body -> {resource, action, ...}
alt payload valid
Parser-->>Gateway: parsed payload
Gateway->>JetStream: publish(subject = prefix.resource.action, body, headers)
JetStream-->>Gateway: published
Gateway-->>Sentry: 200 OK
else payload invalid
Parser-->>Gateway: error
Gateway-->>Sentry: 400 Bad Request
end
else signature invalid
Verifier-->>Gateway: mismatch/error
Gateway-->>Sentry: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 151b6dd Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![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 (1)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
1293-1300: Round out the Sentry config test matrix with token-validation failures.
resolve_sentrynow validates bothsubject_prefixandstream_name, but the new tests only cover the secret and zero-duration branches. Adding the two invalid-token cases would keep Sentry at parity with the existing source coverage and protect those new validation paths from regressions.Also applies to: 1567-1584, 2128-2135, 2419-2445
🤖 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 1293 - 1300, The test matrix for resolve_sentry is missing cases for token-validation failures of subject_prefix and stream_name; update the tests that use sentry_toml and resolve_sentry to add two new cases that provide invalid tokens for subject_prefix and for stream_name (e.g., malformed or empty values) and assert that resolve_sentry returns the expected validation error for each. Locate the tests that call sentry_toml(...) and resolve_sentry(...) and add assertions for the two failure paths so the suite covers secret, zero-duration, subject_prefix, and stream_name validation.
🤖 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/Cargo.toml`:
- Around line 11-13: The new workspace-level clippy denies for expect_used,
panic, and unwrap_used will break CI because existing codebases contain many
violations; revert these global "deny" rules and instead set them to "warn" or
remove them from the top-level Cargo.toml so clippy doesn't fail immediately, or
apply the denies gradually by adding them only to specific crates that have been
remediated; specifically update the Cargo.toml entries for expect_used, panic,
and unwrap_used (the three keys shown) to a non-failing level or move them to
per-crate Cargo.toml files and stage fixes across trogon-std, trogon-gateway,
trogon-source-linear, etc., before enabling "deny" workspace-wide.
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 1098-1120: In resolve_sentry, don’t treat a missing client_secret
as an implicit disable: if resolve_source_status("sentry", ...) indicates the
source is enabled but section.client_secret is None, push a typed
ConfigValidationError (e.g. a dedicated missing-field variant/struct such as
ConfigValidationError::missing("sentry","client_secret") or a new MissingField
error type) to errors and return None; update or add the error type to be a
proper struct/enum that implements Display and std::error::Error, and keep the
existing handling for invalid secrets (the match on SentryClientSecret::new)
unchanged.
In `@rsworkspace/crates/trogon-source-sentry/src/signature.rs`:
- Around line 33-39: The verify function uses expect() when constructing
HmacSha256 which violates the lint; add a new SignatureError variant (e.g.,
InvalidKey or HmacKeyError wrapping the underlying error type) and change
HmacSha256::new_from_slice(secret.as_bytes()).expect(...) to use
new_from_slice(...).map_err(SignatureError::InvalidKey)? (or wrap the
digest::InvalidLength) so the error is propagated instead of panicking; keep the
rest of verify (mac.update, mac.verify_slice) unchanged and return the mapped
SignatureError on failure.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 1293-1300: The test matrix for resolve_sentry is missing cases for
token-validation failures of subject_prefix and stream_name; update the tests
that use sentry_toml and resolve_sentry to add two new cases that provide
invalid tokens for subject_prefix and for stream_name (e.g., malformed or empty
values) and assert that resolve_sentry returns the expected validation error for
each. Locate the tests that call sentry_toml(...) and resolve_sentry(...) and
add assertions for the two failure paths so the suite covers secret,
zero-duration, subject_prefix, and stream_name validation.
🪄 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: b5cf5b00-b223-48d9-a93b-c43087287647
⛔ 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/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-sentry/Cargo.tomlrsworkspace/crates/trogon-source-sentry/src/config.rsrsworkspace/crates/trogon-source-sentry/src/constants.rsrsworkspace/crates/trogon-source-sentry/src/lib.rsrsworkspace/crates/trogon-source-sentry/src/sentry_client_secret.rsrsworkspace/crates/trogon-source-sentry/src/server.rsrsworkspace/crates/trogon-source-sentry/src/signature.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-gateway/src/config.rs (1)
1190-1208: Consider validating the upper bound before constructing the duration.The current order creates
NonZeroDurationfirst, then checks the max bound. This means a value like2will allocate a duration object only to fail the subsequent check. Reversing the order would be slightly more efficient:♻️ Optional: check bound first
+ if section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS { + errors.push(ConfigValidationError::invalid( + "sentry", + "nats_ack_timeout_secs", + DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS), + )); + return None; + } + let nats_ack_timeout = match NonZeroDuration::from_secs(section.nats_ack_timeout_secs) { Ok(duration) => duration, Err(error) => { errors.push(ConfigValidationError::invalid( "sentry", "nats_ack_timeout_secs", error, )); return None; } }; - if section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS { - errors.push(ConfigValidationError::invalid( - "sentry", - "nats_ack_timeout_secs", - DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS), - )); - return None; - }🤖 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 1190 - 1208, Move the SENTRY_MAX_ACK_TIMEOUT_SECS upper-bound check to run before calling NonZeroDuration::from_secs: first if section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS { errors.push(ConfigValidationError::invalid("sentry", "nats_ack_timeout_secs", DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS))); return None; } and then construct NonZeroDuration::from_secs(section.nats_ack_timeout_secs) handling its Err case as before (pushing ConfigValidationError::invalid with the same "sentry"/"nats_ack_timeout_secs" context), so you avoid allocating a duration for values that will be rejected by the max check.
🤖 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/config.rs`:
- Around line 1190-1208: Move the SENTRY_MAX_ACK_TIMEOUT_SECS upper-bound check
to run before calling NonZeroDuration::from_secs: first if
section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS {
errors.push(ConfigValidationError::invalid("sentry", "nats_ack_timeout_secs",
DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS))); return None; } and then
construct NonZeroDuration::from_secs(section.nats_ack_timeout_secs) handling its
Err case as before (pushing ConfigValidationError::invalid with the same
"sentry"/"nats_ack_timeout_secs" context), so you avoid allocating a duration
for values that will be rejected by the max check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c082ff32-53c2-46f8-9818-5b836238f936
⛔ 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/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-sentry/Cargo.tomlrsworkspace/crates/trogon-source-sentry/src/config.rsrsworkspace/crates/trogon-source-sentry/src/constants.rsrsworkspace/crates/trogon-source-sentry/src/lib.rsrsworkspace/crates/trogon-source-sentry/src/sentry_client_secret.rsrsworkspace/crates/trogon-source-sentry/src/server.rsrsworkspace/crates/trogon-source-sentry/src/signature.rs
✅ Files skipped from review due to trivial changes (10)
- rsworkspace/Cargo.toml
- rsworkspace/crates/trogon-gateway/src/http.rs
- devops/docker/compose/.env.example
- rsworkspace/crates/trogon-gateway/Cargo.toml
- devops/docker/compose/services/trogon-gateway/README.md
- rsworkspace/crates/trogon-source-sentry/src/config.rs
- rsworkspace/crates/trogon-source-sentry/Cargo.toml
- rsworkspace/crates/trogon-source-sentry/src/lib.rs
- rsworkspace/crates/trogon-source-sentry/src/sentry_client_secret.rs
- rsworkspace/crates/trogon-source-sentry/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-gateway/src/streams.rs
- rsworkspace/crates/trogon-source-sentry/src/signature.rs
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 151b6dd. Configure here.
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.
Summary
Testing
mise exec -- cargo fmt --all --manifest-path rsworkspace/Cargo.tomlmise exec -- cargo test -p trogon-source-sentry -p trogon-gateway --manifest-path rsworkspace/Cargo.toml