feat(gateway): unblock Twitter/X event ingestion#125
Merged
Conversation
Member
yordis
commented
Apr 16, 2026
yordis
commented
- Unblock Twitter/X activity from entering the same gateway path the rest of our inbound sources already use.
- Keep gateway growth predictable by giving X its own source boundary instead of pushing provider-specific behavior into shared ingress code.
PR SummaryMedium Risk Overview Introduces a new Reviewed by Cursor Bugbot for commit be68155. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR introduces Twitter/X webhook source support to the Trogon system. It creates a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Gateway as Trogon Gateway<br/>/twitter/webhook
participant Sig as Signature Module
participant Parser as JSON Parser
participant JS as JetStream
Client->>Gateway: POST /webhook with body<br/>(x-twitter-webhooks-signature header)
Gateway->>Sig: verify(consumer_secret, body, header)
alt Signature Valid
Sig-->>Gateway: Ok(())
Gateway->>Parser: parse body & classify<br/>event_type, payload_kind
alt Classification Succeeds
Parser-->>Gateway: DottedNatsToken, PayloadKind
Gateway->>JS: publish(subject, body)<br/>with NATS headers
JS-->>Gateway: publish result
alt Publish Success
Gateway-->>Client: 200 OK
else Publish Fails
Gateway-->>Client: 500 Internal Server Error
end
else Classification Fails
Parser-->>Gateway: unroutable payload
Gateway->>JS: publish(unroutable subject)<br/>with reject reason header
JS-->>Gateway: publish result
alt Unroutable Publish Success
Gateway-->>Client: 200 OK
else Unroutable Publish Fails
Gateway-->>Client: 500 Internal Server Error
end
end
else Signature Invalid
Sig-->>Gateway: SignatureError
Gateway-->>Client: 401 Unauthorized
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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rsworkspace/crates/trogon-gateway/README.md (1)
34-55:⚠️ Potential issue | 🟡 MinorGateway README is missing Sentry rows.
The webhook routes table (Lines 34-41) and the source-enablement table (Lines 47-55) document Twitter/X but not Sentry, even though this PR also adds
/sentry/webhookandTROGON_SOURCE_SENTRY_CLIENT_SECRET(seedevops/docker/compose/services/trogon-gateway/README.mdandhttp.rs). Please add Sentry rows for consistency. Same for incident.io/Notion if they were similarly omitted previously.📝 Proposed addition
| Twitter/X | `/twitter/webhook` | | GitLab | `/gitlab/webhook` | | Linear | `/linear/webhook` | +| Sentry | `/sentry/webhook` || Twitter/X | `TROGON_SOURCE_TWITTER_CONSUMER_SECRET` | | GitLab | `TROGON_SOURCE_GITLAB_WEBHOOK_SECRET` | | Linear | `TROGON_SOURCE_LINEAR_WEBHOOK_SECRET` | +| Sentry | `TROGON_SOURCE_SENTRY_CLIENT_SECRET` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-gateway/README.md` around lines 34 - 55, Update the two README tables to include Sentry: add a row in the webhook routes table mapping Sentry to `/sentry/webhook` and add a row in the source enablement table with Sentry and the env var `TROGON_SOURCE_SENTRY_CLIENT_SECRET`; also check and add similar rows for incident.io and Notion if those sources were added elsewhere (refer to the new route `/sentry/webhook` and the env var symbol `TROGON_SOURCE_SENTRY_CLIENT_SECRET` and any incident.io/Notion env var names present in the repo) so the documentation matches the code changes.
🧹 Nitpick comments (6)
rsworkspace/crates/trogon-source-twitter/Cargo.toml (1)
18-18: Consider droppingtokio"full" in normal deps.Sibling source crates (e.g. sentry) don't enable tokio in
[dependencies]at all — the runtime comes from the gateway binary.features = ["full"]pulls in a lot of unused surface (signal, process, fs, net...) into a library crate. If#[tokio::main]or specific runtime primitives aren't actually used in the library code, remove it; otherwise scope it to the minimum required features (e.g.sync,time).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-twitter/Cargo.toml` at line 18, The Cargo.toml currently enables tokio with features = ["full"] which pulls unnecessary runtime components into this library crate; update the dependency for the trogon-source-twitter crate to only request the minimal tokio features actually used (or remove tokio from [dependencies] entirely if the runtime is provided by the gateway binary), e.g., replace features = ["full"] with the specific features such as ["sync","time"] or remove the dependency from Cargo.toml and rely on the application binary to supply the runtime; check usages in this crate for #[tokio::main], tokio::spawn, time or sync primitives to determine the exact features required.rsworkspace/crates/trogon-source-twitter/src/config.rs (1)
26-32: Consider derivingDebugonTwitterConfig(same forSentryConfig).
TwitterConsumerSecret's redactingDebugmeans a derivedDebugonTwitterConfigis safe and useful for diagnostics/tracing of the resolved gateway config. Without it, callers can't{:?}-log the config. Same applies toSentryConfiginrsworkspace/crates/trogon-source-sentry/src/config.rs.Proposed change
+#[derive(Debug)] pub struct TwitterConfig { pub consumer_secret: TwitterConsumerSecret, pub subject_prefix: NatsToken, pub stream_name: NatsToken, pub stream_max_age: StreamMaxAge, pub nats_ack_timeout: NonZeroDuration, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-twitter/src/config.rs` around lines 26 - 32, Add a derived Debug impl for the TwitterConfig struct (and likewise for SentryConfig) so callers can {:?}-log resolved gateway configs; since TwitterConsumerSecret already provides a redacting Debug, simply update the struct definitions (TwitterConfig and SentryConfig) to include #[derive(Debug)] (or add Debug to their existing derive lists) to enable safe diagnostic printing while preserving secret redaction.rsworkspace/crates/trogon-gateway/src/config.rs (2)
1238-1265: Note the asymmetric validation forclient_secret— consider a comment.When
status == "enabled"andclient_secretis missing, a validation error is raised. Whenstatusis unset (default-enabled) andclient_secretis missing, the source is silently disabled. This is tested (sentry_missing_client_secret_returns_none_when_status_unspecifiedvssentry_enabled_without_client_secret_is_invalid) and matches the "opt-in with explicit validation" pattern, but the behavior is subtle. A short comment explaining the intent would help future readers avoid unifying the branches.🤖 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 1238 - 1265, The code intentionally treats a missing client_secret differently depending on explicit status: when explicitly_enabled (computed from section.status) we validate and push ConfigValidationError::missing for "sentry"/"client_secret", but when status is unset we silently disable the source (after resolve_source_status) — add a short clarifying comment near the explicitly_enabled calculation and the client_secret match (referencing explicitly_enabled, resolve_source_status("sentry", ...), section.client_secret and SentryClientSecret::new) that explains this "opt-in with explicit validation" behavior and references the corresponding tests (sentry_missing_client_secret_returns_none_when_status_unspecified vs sentry_enabled_without_client_secret_is_invalid) so future readers do not collapse the branches.
1291-1309: Optional: reorder max-timeout check beforeNonZeroDuration::from_secs.
nats_ack_timeoutis constructed first and then potentially discarded when the max-value check fails. Swapping the order avoids the throwaway binding and yields a cleaner flow:♻️ Proposed reorder
+ 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 1291 - 1309, The current flow constructs nats_ack_timeout with NonZeroDuration::from_secs and then rejects it if section.nats_ack_timeout_secs exceeds SENTRY_MAX_ACK_TIMEOUT_SECS, creating a throwaway binding; instead, first check if section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS and, if so, push ConfigValidationError::invalid (using DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS)) and return None, then call NonZeroDuration::from_secs to create nats_ack_timeout and handle its Err by pushing ConfigValidationError::invalid and returning None.rsworkspace/crates/trogon-source-sentry/src/server.rs (1)
186-199: Minor: log uses the pre-shadowed&strforresource, which is fine — just confirming intent.Line 189 references
resourcewhile it still holds the trimmed&strvalue (from Line 164), and then it's shadowed with theSentryResourceenum on Line 186'smatchbinding. Works correctly, but the shadowing is easy to miss during later edits; consider renaming the enum binding toparsed_resourcefor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-sentry/src/server.rs` around lines 186 - 199, The match currently shadows the original &str `resource` with the parsed SentryResource; rename the enum binding to parsed_resource (e.g., let parsed_resource = match resource.parse::<SentryResource>() { ... }) to avoid subtle shadowing, update subsequent uses that expect the enum to use parsed_resource (and keep the original `resource` string in the warn! call that logs the raw value), and ensure any later code that relied on the previous shadowed name now references parsed_resource or resource appropriately.rsworkspace/crates/trogon-source-twitter/src/server.rs (1)
290-309: Consider: multi-event payloads fall back toaccount_activity, but the message id extraction iterates in declaration order.When a single payload contains multiple known account-activity event types, the subject is
account_activity(correct fallback), butextract_message_idon lines 322–330 usesfind_mapoverACCOUNT_ACTIVITY_EVENT_TYPESand picks the first matching array/object id. That id may not correspond to any specific semantics the consumer expects, and when arrays have >1 items no id is produced (by design on line 326). Whether this matters depends on how downstream dedup is supposed to behave for these rare composite payloads — worth documenting the chosen semantics or deliberately skippingNATS_MESSAGE_IDin the fallback case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-twitter/src/server.rs` around lines 290 - 309, The code falls back to the generic subject "account_activity" when multiple ACCOUNT_ACTIVITY_EVENT_TYPES are present, but extract_message_id currently picks an id by iterating ACCOUNT_ACTIVITY_EVENT_TYPES in declaration order which can be misleading; update the logic in extract_message_id (the function that find_maps over ACCOUNT_ACTIVITY_EVENT_TYPES and reads NATS_MESSAGE_ID) to detect the multi-event fallback (i.e., when the earlier account_activity_types collection has len() > 1 or when event_type == "account_activity") and return None (skip setting NATS_MESSAGE_ID) for that case, or alternatively add explicit documentation and a comment in extract_message_id explaining the chosen semantics if you decide to keep the current behavior. Ensure you reference ACCOUNT_ACTIVITY_EVENT_TYPES and the fallback "account_activity" subject 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 `@rsworkspace/crates/trogon-source-sentry/src/server.rs`:
- Around line 169-176: The Sentry timestamp is read into the variable
`timestamp` via `header_value(&headers, HEADER_TIMESTAMP)` but never validated
for clock skew; add a configurable max-drift check (e.g.,
`timestamp_max_drift_secs` on your existing config) and reject requests whose
parsed `Sentry-Hook-Timestamp` differs from now by more than that threshold. In
`server.rs` where `HEADER_TIMESTAMP`, `header_value`, and `timestamp` are used,
parse the header into an integer/float (handle parse errors by returning
`StatusCode::BAD_REQUEST`), compute the absolute difference against the current
system time, and if it exceeds the configured max drift return
`StatusCode::BAD_REQUEST` (and log a warning) before proceeding to forward
headers (including `HEADER_REQUEST_ID` and NATS publishing).
---
Outside diff comments:
In `@rsworkspace/crates/trogon-gateway/README.md`:
- Around line 34-55: Update the two README tables to include Sentry: add a row
in the webhook routes table mapping Sentry to `/sentry/webhook` and add a row in
the source enablement table with Sentry and the env var
`TROGON_SOURCE_SENTRY_CLIENT_SECRET`; also check and add similar rows for
incident.io and Notion if those sources were added elsewhere (refer to the new
route `/sentry/webhook` and the env var symbol
`TROGON_SOURCE_SENTRY_CLIENT_SECRET` and any incident.io/Notion env var names
present in the repo) so the documentation matches the code changes.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-gateway/src/config.rs`:
- Around line 1238-1265: The code intentionally treats a missing client_secret
differently depending on explicit status: when explicitly_enabled (computed from
section.status) we validate and push ConfigValidationError::missing for
"sentry"/"client_secret", but when status is unset we silently disable the
source (after resolve_source_status) — add a short clarifying comment near the
explicitly_enabled calculation and the client_secret match (referencing
explicitly_enabled, resolve_source_status("sentry", ...), section.client_secret
and SentryClientSecret::new) that explains this "opt-in with explicit
validation" behavior and references the corresponding tests
(sentry_missing_client_secret_returns_none_when_status_unspecified vs
sentry_enabled_without_client_secret_is_invalid) so future readers do not
collapse the branches.
- Around line 1291-1309: The current flow constructs nats_ack_timeout with
NonZeroDuration::from_secs and then rejects it if section.nats_ack_timeout_secs
exceeds SENTRY_MAX_ACK_TIMEOUT_SECS, creating a throwaway binding; instead,
first check if section.nats_ack_timeout_secs > SENTRY_MAX_ACK_TIMEOUT_SECS and,
if so, push ConfigValidationError::invalid (using
DurationTooLong::new(SENTRY_MAX_ACK_TIMEOUT_SECS)) and return None, then call
NonZeroDuration::from_secs to create nats_ack_timeout and handle its Err by
pushing ConfigValidationError::invalid and returning None.
In `@rsworkspace/crates/trogon-source-sentry/src/server.rs`:
- Around line 186-199: The match currently shadows the original &str `resource`
with the parsed SentryResource; rename the enum binding to parsed_resource
(e.g., let parsed_resource = match resource.parse::<SentryResource>() { ... })
to avoid subtle shadowing, update subsequent uses that expect the enum to use
parsed_resource (and keep the original `resource` string in the warn! call that
logs the raw value), and ensure any later code that relied on the previous
shadowed name now references parsed_resource or resource appropriately.
In `@rsworkspace/crates/trogon-source-twitter/Cargo.toml`:
- Line 18: The Cargo.toml currently enables tokio with features = ["full"] which
pulls unnecessary runtime components into this library crate; update the
dependency for the trogon-source-twitter crate to only request the minimal tokio
features actually used (or remove tokio from [dependencies] entirely if the
runtime is provided by the gateway binary), e.g., replace features = ["full"]
with the specific features such as ["sync","time"] or remove the dependency from
Cargo.toml and rely on the application binary to supply the runtime; check
usages in this crate for #[tokio::main], tokio::spawn, time or sync primitives
to determine the exact features required.
In `@rsworkspace/crates/trogon-source-twitter/src/config.rs`:
- Around line 26-32: Add a derived Debug impl for the TwitterConfig struct (and
likewise for SentryConfig) so callers can {:?}-log resolved gateway configs;
since TwitterConsumerSecret already provides a redacting Debug, simply update
the struct definitions (TwitterConfig and SentryConfig) to include
#[derive(Debug)] (or add Debug to their existing derive lists) to enable safe
diagnostic printing while preserving secret redaction.
In `@rsworkspace/crates/trogon-source-twitter/src/server.rs`:
- Around line 290-309: The code falls back to the generic subject
"account_activity" when multiple ACCOUNT_ACTIVITY_EVENT_TYPES are present, but
extract_message_id currently picks an id by iterating
ACCOUNT_ACTIVITY_EVENT_TYPES in declaration order which can be misleading;
update the logic in extract_message_id (the function that find_maps over
ACCOUNT_ACTIVITY_EVENT_TYPES and reads NATS_MESSAGE_ID) to detect the
multi-event fallback (i.e., when the earlier account_activity_types collection
has len() > 1 or when event_type == "account_activity") and return None (skip
setting NATS_MESSAGE_ID) for that case, or alternatively add explicit
documentation and a comment in extract_message_id explaining the chosen
semantics if you decide to keep the current behavior. Ensure you reference
ACCOUNT_ACTIVITY_EVENT_TYPES and the fallback "account_activity" subject when
making the change.
🪄 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: 47cf8c07-5a2a-463a-9cbf-6c84267921aa
⛔ 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/services/trogon-gateway/README.mdrsworkspace/Cargo.tomlrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/README.mdrsworkspace/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.rsrsworkspace/crates/trogon-source-twitter/Cargo.tomlrsworkspace/crates/trogon-source-twitter/src/config.rsrsworkspace/crates/trogon-source-twitter/src/constants.rsrsworkspace/crates/trogon-source-twitter/src/lib.rsrsworkspace/crates/trogon-source-twitter/src/server.rsrsworkspace/crates/trogon-source-twitter/src/signature.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-twitter-webhook
branch
from
622b406 to
057bbc9
Compare
|
Code Coverage SummaryDetailsDiff against mainResults for commit: be68155 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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 add74f7. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rsworkspace/crates/trogon-source-twitter/src/server.rs (2)
269-284: Classification precedence isdata.event_type→matching_rules→ account-activity keys — worth a brief comment.Today Twitter's filtered-stream payloads don't carry
data.event_type, so this ordering is safe, but the precedence is load-bearing and easy to break silently when new payload shapes are added. A one-line comment documenting the intentional order (and the assumption that X Activity and filtered-stream shapes are disjoint ondata.event_type) would reduce the odds of a future regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-twitter/src/server.rs` around lines 269 - 284, Add a one-line comment above the classification block that checks payload.get("data").and_then(... event_type) and the subsequent payload.get("matching_rules") branch to document the intentional precedence: "classification precedence is data.event_type → matching_rules → account-activity keys" and note the assumption that X Activity and filtered-stream shapes are disjoint on data.event_type (so filtered-stream payloads lack data.event_type). Place the comment near the DottedNatsToken::new(...) / PayloadKind::XActivity and the matching_rules check to make the reasoning for choosing DottedNatsToken and PayloadKind explicit for future maintainers.
198-204: Missing-signature-header path is uncovered by tests.
invalid_signature_returns_unauthorizedexercises thesignature::verifyfailure path, but the earlierheaders.get(HEADER_SIGNATURE)absent/non-UTF8 branch has no direct test. A small test postingwebhook_request(body, None)asserting401 UNAUTHORIZEDwould lock in the behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-source-twitter/src/server.rs` around lines 198 - 204, Add a unit/integration test that posts a webhook with no signature header to exercise the branch where headers.get(HEADER_SIGNATURE) returns None: create a test similar to invalid_signature_returns_unauthorized but call webhook_request(body, None) (or otherwise omit the HEADER_SIGNATURE) and assert the handler returns StatusCode::UNAUTHORIZED; place the test near the existing invalid_signature_returns_unauthorized test so it references the same setup and verifies the missing-signature path covered by the signature extraction in server.rs.rsworkspace/crates/trogon-source-twitter/src/signature.rs (1)
37-62: Consider accepting&TwitterConsumerSecretrather than&str.Both
crc_response_tokenandverifyare public and currently re-admit a primitive at the boundary (consumer_secret: &str), which means callers outsideserver.rscan bypass theTwitterConsumerSecretinvariant. Taking&TwitterConsumerSecret(and internally calling.as_str()/.as_bytes()) keeps the domain guarantee intact at the module boundary.As per coding guidelines: "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString). 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-source-twitter/src/signature.rs` around lines 37 - 62, Change the public APIs crc_response_token and verify to accept &TwitterConsumerSecret instead of &str so the consumer-secret invariant is preserved at the module boundary; update the function signatures (crc_response_token(consumer_secret: &TwitterConsumerSecret, ...) and verify(consumer_secret: &TwitterConsumerSecret, ...)), use consumer_secret.as_bytes() / consumer_secret.as_str() where you currently call .as_bytes(), and update all callers/tests to pass &TwitterConsumerSecret (and import the type) so construction always enforces the domain invariant.
🤖 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-twitter/src/server.rs`:
- Around line 300-332: The extracted message id can come from a different
account-activity sub-event than the routed subject; update extract_message_id to
avoid cross-bucket collisions by namespacing the dedup key with the routed event
type (e.g. return format!("{event_type}:{id}")), or else return None for the
generic "account_activity" bucket; concretely, change extract_message_id to
accept the resolved event_type (the same value passed into DottedNatsToken::new
/ the logic around account_activity_types / PayloadKind::AccountActivity) and
when it finds an id from ACCOUNT_ACTIVITY_EVENT_TYPES, prefix it with the routed
event_type (or skip returning any id if event_type == "account_activity"), so
Nats-Msg-Id is either namespaced or omitted when using the generic bucket.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-source-twitter/src/server.rs`:
- Around line 269-284: Add a one-line comment above the classification block
that checks payload.get("data").and_then(... event_type) and the subsequent
payload.get("matching_rules") branch to document the intentional precedence:
"classification precedence is data.event_type → matching_rules →
account-activity keys" and note the assumption that X Activity and
filtered-stream shapes are disjoint on data.event_type (so filtered-stream
payloads lack data.event_type). Place the comment near the
DottedNatsToken::new(...) / PayloadKind::XActivity and the matching_rules check
to make the reasoning for choosing DottedNatsToken and PayloadKind explicit for
future maintainers.
- Around line 198-204: Add a unit/integration test that posts a webhook with no
signature header to exercise the branch where headers.get(HEADER_SIGNATURE)
returns None: create a test similar to invalid_signature_returns_unauthorized
but call webhook_request(body, None) (or otherwise omit the HEADER_SIGNATURE)
and assert the handler returns StatusCode::UNAUTHORIZED; place the test near the
existing invalid_signature_returns_unauthorized test so it references the same
setup and verifies the missing-signature path covered by the signature
extraction in server.rs.
In `@rsworkspace/crates/trogon-source-twitter/src/signature.rs`:
- Around line 37-62: Change the public APIs crc_response_token and verify to
accept &TwitterConsumerSecret instead of &str so the consumer-secret invariant
is preserved at the module boundary; update the function signatures
(crc_response_token(consumer_secret: &TwitterConsumerSecret, ...) and
verify(consumer_secret: &TwitterConsumerSecret, ...)), use
consumer_secret.as_bytes() / consumer_secret.as_str() where you currently call
.as_bytes(), and update all callers/tests to pass &TwitterConsumerSecret (and
import the type) so construction always enforces the domain invariant.
🪄 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: 69808b43-4b84-4860-8bb9-102d477c7ef3
⛔ 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/README.mdrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/http.rsrsworkspace/crates/trogon-gateway/src/streams.rsrsworkspace/crates/trogon-source-twitter/Cargo.tomlrsworkspace/crates/trogon-source-twitter/src/config.rsrsworkspace/crates/trogon-source-twitter/src/constants.rsrsworkspace/crates/trogon-source-twitter/src/lib.rsrsworkspace/crates/trogon-source-twitter/src/server.rsrsworkspace/crates/trogon-source-twitter/src/signature.rs
✅ Files skipped from review due to trivial changes (8)
- rsworkspace/Cargo.toml
- rsworkspace/crates/trogon-gateway/Cargo.toml
- devops/docker/compose/.env.example
- rsworkspace/crates/trogon-gateway/README.md
- rsworkspace/crates/trogon-source-twitter/Cargo.toml
- rsworkspace/crates/trogon-source-twitter/src/lib.rs
- rsworkspace/crates/trogon-source-twitter/src/config.rs
- rsworkspace/crates/trogon-source-twitter/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/trogon-gateway/src/http.rs
- devops/docker/compose/services/trogon-gateway/README.md
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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.