Conversation
PR SummaryMedium Risk Overview Updates Extends local Docker Compose to run JetStream-enabled NATS, build/run Written by Cursor Bugbot for commit 5c33189. This will update automatically on new commits. 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 Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub
participant S as trogon-sink-github<br/>HTTP Server
participant JS as NATS<br/>JetStream
participant Sub as Downstream<br/>Subscriber
GH->>S: POST /webhook (headers, body)
S->>S: Verify x-hub-signature-256 (HMAC-SHA256)
alt invalid signature
S-->>GH: 401 Unauthorized
else valid signature
S->>JS: Publish to subject {prefix}.{event} with headers & payload
JS->>JS: Accept/store message and ACK
alt ACK success
S-->>GH: 200 OK
JS->>Sub: Deliver message
else ACK timeout/failure
S-->>GH: 500 Internal Server Error
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 |
yordis
force-pushed
the
github
branch
6 times, most recently
from
b1c9669 to
2155563
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: 1
🧹 Nitpick comments (4)
rsworkspace/crates/trogon-github/Dockerfile (1)
9-9: Use--lockedfor reproducible image builds.Line 9 should enforce
Cargo.lockusage to prevent drift during container builds.Proposed change
-RUN cargo build --release -p trogon-github +RUN cargo build --locked --release -p trogon-github🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-github/Dockerfile` at line 9, The Dockerfile's build step uses "RUN cargo build --release -p trogon-github" without locking dependencies, which can cause non-reproducible builds; update that RUN invocation to include the --locked flag (i.e., run cargo build --release --locked -p trogon-github) so Cargo uses Cargo.lock during image builds and prevents dependency drift.rsworkspace/crates/trogon-github/tests/webhook_e2e.rs (2)
43-47: Consider using port 0 for dynamic allocation in test helpers.The static
PORT_COUNTERstarting at 28000 could theoretically collide with other services. While unlikely in CI, using port 0 withTcpListener::bind("127.0.0.1:0")and retrieving the assigned port would be more robust. However, since the server binds internally andwait_for_portpolls, this approach works and is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-github/tests/webhook_e2e.rs` around lines 43 - 47, Replace the manual static port counter (PORT_COUNTER and next_port) with dynamic allocation via the OS: create a TcpListener bound to "127.0.0.1:0" to obtain the assigned port and use that port for starting the test server (or return it from the helper that previously called next_port); remove or stop using PORT_COUNTER and next_port and update any callers (and wait_for_port usage) to accept the listener-assigned port so tests no longer risk port collisions.
954-985: Consider adding#[cfg(unix)]to signal tests for consistency with workspace patterns.The
kill -TERMandkill -INTcommands at lines 963-966 and 1008-1011 are Unix-specific. While no Windows CI is currently configured in the repository, the codebase uses#[cfg(unix)]guards elsewhere (e.g.,rsworkspace/crates/acp-telemetry/src/signal.rs). Adding similar guards here improves consistency and prevents accidental test failures if platform support changes in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-github/tests/webhook_e2e.rs` around lines 954 - 985, The test that sends Unix signals uses std::process::Command::new("kill") and is Unix-specific; add #[cfg(unix)] above the async test functions that invoke kill (e.g., server_exits_cleanly_on_sigterm and the sibling test that sends SIGINT) so they only run on Unix platforms, leaving the tokio::test attribute and the body (pid = child.id(), Command::new("kill") calls, and child.wait() logic) unchanged.rsworkspace/crates/trogon-github/src/config.rs (1)
22-31: Consider domain-specific value objects forsubject_prefixandstream_name.Per coding guidelines, domain-specific value objects are preferred over primitives. NATS subjects and JetStream stream names have validity constraints (e.g., no spaces, specific character sets). Wrapping these in validated types like
SubjectPrefixandStreamNamewould catch invalid configurations at startup rather than at runtime when NATS rejects them.This can be deferred since NATS provides runtime validation, but it would improve the fail-fast behavior. As per coding guidelines: "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-github/src/config.rs` around lines 22 - 31, The fields subject_prefix and stream_name on GithubConfig are raw Strings but should be domain-specific validated types to fail fast; introduce value objects (e.g., SubjectPrefix and StreamName) with constructors/TryFrom/FromStr and serde deserialization that enforce NATS/JetStream name rules (no spaces, allowed chars, length limits), replace GithubConfig::subject_prefix: String and ::stream_name: String with these new types, update any code that constructs or reads GithubConfig (parsing from env/files) to validate at startup and convert strings into SubjectPrefix/StreamName, and adjust usages of those fields to call the new types' accessors or AsRef<str> so NATS clients get already-validated values.
🤖 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-github/src/main.rs`:
- Around line 18-31: After constructing the config with
GithubConfig::from_env(&SystemEnv), add a startup guard that ensures the GitHub
webhook secret is present and non-empty (check the config field that holds the
secret, e.g., config.github_webhook_secret or by reading
std::env::var("GITHUB_WEBHOOK_SECRET") if the config does not expose it); if the
secret is missing/empty, log an error (using error! with context) and return
ExitCode::FAILURE immediately before calling serve(config, nats). This ensures
the process fails fast rather than starting with webhook auth unenforced.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-github/Dockerfile`:
- Line 9: The Dockerfile's build step uses "RUN cargo build --release -p
trogon-github" without locking dependencies, which can cause non-reproducible
builds; update that RUN invocation to include the --locked flag (i.e., run cargo
build --release --locked -p trogon-github) so Cargo uses Cargo.lock during image
builds and prevents dependency drift.
In `@rsworkspace/crates/trogon-github/src/config.rs`:
- Around line 22-31: The fields subject_prefix and stream_name on GithubConfig
are raw Strings but should be domain-specific validated types to fail fast;
introduce value objects (e.g., SubjectPrefix and StreamName) with
constructors/TryFrom/FromStr and serde deserialization that enforce
NATS/JetStream name rules (no spaces, allowed chars, length limits), replace
GithubConfig::subject_prefix: String and ::stream_name: String with these new
types, update any code that constructs or reads GithubConfig (parsing from
env/files) to validate at startup and convert strings into
SubjectPrefix/StreamName, and adjust usages of those fields to call the new
types' accessors or AsRef<str> so NATS clients get already-validated values.
In `@rsworkspace/crates/trogon-github/tests/webhook_e2e.rs`:
- Around line 43-47: Replace the manual static port counter (PORT_COUNTER and
next_port) with dynamic allocation via the OS: create a TcpListener bound to
"127.0.0.1:0" to obtain the assigned port and use that port for starting the
test server (or return it from the helper that previously called next_port);
remove or stop using PORT_COUNTER and next_port and update any callers (and
wait_for_port usage) to accept the listener-assigned port so tests no longer
risk port collisions.
- Around line 954-985: The test that sends Unix signals uses
std::process::Command::new("kill") and is Unix-specific; add #[cfg(unix)] above
the async test functions that invoke kill (e.g., server_exits_cleanly_on_sigterm
and the sibling test that sends SIGINT) so they only run on Unix platforms,
leaving the tokio::test attribute and the body (pid = child.id(),
Command::new("kill") calls, and child.wait() logic) 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: 378d9a8e-ccde-4c92-92d2-6556170c5f53
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
rsworkspace/.dockerignorersworkspace/crates/trogon-github/Cargo.tomlrsworkspace/crates/trogon-github/Dockerfilersworkspace/crates/trogon-github/docker-compose.ymlrsworkspace/crates/trogon-github/src/config.rsrsworkspace/crates/trogon-github/src/constants.rsrsworkspace/crates/trogon-github/src/lib.rsrsworkspace/crates/trogon-github/src/main.rsrsworkspace/crates/trogon-github/src/server.rsrsworkspace/crates/trogon-github/src/signature.rsrsworkspace/crates/trogon-github/tests/webhook_e2e.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rs
yordis
force-pushed
the
github
branch
2 times, most recently
from
fe5c27f to
2422a39
Compare
yordis
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yordis
force-pushed
the
github
branch
2 times, most recently
from
a012ba4 to
490aa4e
Compare
yordis
changed the title
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 5c33189 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rsworkspace/crates/trogon-sink-github/src/config.rs (1)
37-41: Silent fallback on parse errors may hide configuration mistakes.When
GITHUB_WEBHOOK_PORTcontains an invalid value like"abc", the code silently falls back to the default without logging. This could make deployment debugging harder.Consider logging a warning when a configured value fails to parse:
💡 Optional: Add warning for invalid config values
+use tracing::warn; + impl GithubConfig { pub fn from_env<E: ReadEnv>(env: &E) -> Self { Self { webhook_secret: env.var("GITHUB_WEBHOOK_SECRET").ok(), - port: env - .var("GITHUB_WEBHOOK_PORT") - .ok() - .and_then(|p| p.parse().ok()) - .unwrap_or(DEFAULT_PORT), + port: env + .var("GITHUB_WEBHOOK_PORT") + .ok() + .and_then(|p| p.parse().ok().or_else(|| { + warn!("Invalid GITHUB_WEBHOOK_PORT, using default {DEFAULT_PORT}"); + None + })) + .unwrap_or(DEFAULT_PORT),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-sink-github/src/config.rs` around lines 37 - 41, The GITHUB_WEBHOOK_PORT parsing currently swallows parse errors and silently uses DEFAULT_PORT; change the logic where port is set (the block calling env.var("GITHUB_WEBHOOK_PORT").ok().and_then(|p| p.parse().ok()).unwrap_or(DEFAULT_PORT)) to explicitly handle the parse result: if env.var returns a value attempt p.parse::<u16>() and on Err call log::warn! (including the raw value and the parse error) before falling back to DEFAULT_PORT, and keep using DEFAULT_PORT when the env var is absent; reference the port assignment and DEFAULT_PORT in config.rs to locate where to add the match/if-let and logging.rsworkspace/crates/acp-telemetry/src/service_name.rs (1)
31-41: Consider adding test coverage for the newTrogonSinkGithubvariant.The existing tests verify
AcpNatsStdioandAcpNatsWsbut don't cover the newly addedTrogonSinkGithubvariant:💡 Suggested test additions
#[test] fn as_str_returns_expected_values() { assert_eq!(ServiceName::AcpNatsStdio.as_str(), "acp-nats-stdio"); assert_eq!(ServiceName::AcpNatsWs.as_str(), "acp-nats-ws"); + assert_eq!(ServiceName::TrogonSinkGithub.as_str(), "trogon-sink-github"); } #[test] fn display_delegates_to_as_str() { assert_eq!(format!("{}", ServiceName::AcpNatsStdio), "acp-nats-stdio"); assert_eq!(format!("{}", ServiceName::AcpNatsWs), "acp-nats-ws"); + assert_eq!(format!("{}", ServiceName::TrogonSinkGithub), "trogon-sink-github"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-telemetry/src/service_name.rs` around lines 31 - 41, Add assertions covering the new ServiceName::TrogonSinkGithub variant in the existing tests: update as_str_returns_expected_values to assert ServiceName::TrogonSinkGithub.as_str() == "trogon-sink-github" and update display_delegates_to_as_str to assert format!("{}", ServiceName::TrogonSinkGithub) == "trogon-sink-github"; this ensures both as_str() and Display behavior are tested for the new enum variant.devops/docker/compose/compose.yml (1)
5-5: Consider pinning the NATS image version for reproducibility.Using
nats:latestcan lead to unexpected behavior when the upstream image is updated. Pin to a specific version for reproducible builds.💡 Suggested fix
- image: nats:latest + image: nats:2.10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@devops/docker/compose/compose.yml` at line 5, Replace the floating image reference "image: nats:latest" with a pinned value to ensure reproducible builds; update the compose service entry that currently uses image: nats:latest to either a specific tagged release (e.g., nats:<stable-version>) or an immutable digest (nats@sha256:...), and commit that change so the docker-compose YAML always pulls a known, tested NATS image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@devops/docker/compose/compose.yml`:
- Line 5: Replace the floating image reference "image: nats:latest" with a
pinned value to ensure reproducible builds; update the compose service entry
that currently uses image: nats:latest to either a specific tagged release
(e.g., nats:<stable-version>) or an immutable digest (nats@sha256:...), and
commit that change so the docker-compose YAML always pulls a known, tested NATS
image.
In `@rsworkspace/crates/acp-telemetry/src/service_name.rs`:
- Around line 31-41: Add assertions covering the new
ServiceName::TrogonSinkGithub variant in the existing tests: update
as_str_returns_expected_values to assert ServiceName::TrogonSinkGithub.as_str()
== "trogon-sink-github" and update display_delegates_to_as_str to assert
format!("{}", ServiceName::TrogonSinkGithub) == "trogon-sink-github"; this
ensures both as_str() and Display behavior are tested for the new enum variant.
In `@rsworkspace/crates/trogon-sink-github/src/config.rs`:
- Around line 37-41: The GITHUB_WEBHOOK_PORT parsing currently swallows parse
errors and silently uses DEFAULT_PORT; change the logic where port is set (the
block calling env.var("GITHUB_WEBHOOK_PORT").ok().and_then(|p|
p.parse().ok()).unwrap_or(DEFAULT_PORT)) to explicitly handle the parse result:
if env.var returns a value attempt p.parse::<u16>() and on Err call log::warn!
(including the raw value and the parse error) before falling back to
DEFAULT_PORT, and keep using DEFAULT_PORT when the env var is absent; reference
the port assignment and DEFAULT_PORT in config.rs to locate where to add the
match/if-let and logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8edc023f-8ba8-4703-b10b-a75f32607278
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
devops/docker/compose/compose.ymlrsworkspace/.dockerignorersworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rsrsworkspace/crates/trogon-sink-github/Cargo.tomlrsworkspace/crates/trogon-sink-github/Dockerfilersworkspace/crates/trogon-sink-github/src/config.rsrsworkspace/crates/trogon-sink-github/src/constants.rsrsworkspace/crates/trogon-sink-github/src/lib.rsrsworkspace/crates/trogon-sink-github/src/main.rsrsworkspace/crates/trogon-sink-github/src/server.rsrsworkspace/crates/trogon-sink-github/src/signature.rs
✅ Files skipped from review due to trivial changes (4)
- rsworkspace/.dockerignore
- rsworkspace/crates/trogon-sink-github/Dockerfile
- rsworkspace/crates/trogon-sink-github/Cargo.toml
- rsworkspace/crates/trogon-sink-github/src/constants.rs
yordis
force-pushed
the
github
branch
5 times, most recently
from
849989f to
483ff31
Compare
yordis
force-pushed
the
github
branch
2 times, most recently
from
ff8bf0f to
6786547
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-sink-github/src/signature.rs (1)
30-36: Prefer typed webhook inputs over raw primitives inverify()API.Line 30 currently accepts raw
&str/&[u8]; consider value objects (e.g.,GithubWebhookSecret,GithubSignatureHeader) so invalid header forms are unrepresentable before verification.As per coding guidelines, “Prefer domain-specific value objects over primitives … 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-sink-github/src/signature.rs` around lines 30 - 36, The verify() API currently accepts raw primitives (secret: &str, body: &[u8], signature_header: &str); refactor to use domain value objects so invalid forms are unrepresentable: introduce types like GithubWebhookSecret (wrapping the secret bytes/string) and GithubSignatureHeader (constructed via a fallible try_from/parse that enforces "sha256=" prefix and validates/decodes hex, returning SignatureError variants such as MissingPrefix/InvalidHex), then change pub fn verify to accept GithubWebhookSecret and GithubSignatureHeader (and body: &[u8] can remain or be wrapped if desired) and move the prefix-strip/hex-decode logic out of verify into GithubSignatureHeader::try_from/parse so verify only compares signatures using the already-validated values.rsworkspace/crates/trogon-sink-github/src/config.rs (1)
22-31: Consider domain-specific value objects for configuration fields.The struct uses primitive types (
String,u16) for fields likesubject_prefixandstream_name. Per coding guidelines, domain-specific value objects are preferred over primitives, where each type's factory guarantees correctness at construction. For example, aSubjectPrefixorStreamNametype could validate format constraints (e.g., no invalid NATS subject characters).This is a low-priority suggestion that can be deferred. As per coding guidelines: "Prefer domain-specific value objects over primitives (e.g.,
AcpPrefixnotString), with each type's factory guaranteeing correctness at construction."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-sink-github/src/config.rs` around lines 22 - 31, Replace primitive config fields with domain-specific value objects: introduce SubjectPrefix and StreamName types with constructors/factories that validate allowed characters/format (e.g., NATS subject rules) and convert/parse from String; then change GithubConfig's subject_prefix: String and stream_name: String to subject_prefix: SubjectPrefix and stream_name: StreamName, and update any code that constructs GithubConfig to use the new factories (validate at creation time so rest of code can rely on correct values).
🤖 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/compose.yml`:
- Around line 17-22: The healthcheck uses wget which is not present in the
nats:latest (Alpine) image so it will always fail; update the healthcheck block
(the healthcheck: test: entry) to use a shell-based check that works in Alpine,
e.g. replace the exec form with a shell form that uses /bin/sh -c and either a
TCP probe (/dev/tcp/localhost/8222) or curl if you add it to the image;
specifically modify the healthcheck test in the healthcheck section so it runs
something like /bin/sh -c 'echo > /dev/tcp/localhost/8222 >/dev/null 2>&1 ||
exit 1' or use /bin/sh -c 'curl -sSf http://localhost:8222/healthz || exit 1' to
ensure the container can perform the check without wget.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-sink-github/src/config.rs`:
- Around line 22-31: Replace primitive config fields with domain-specific value
objects: introduce SubjectPrefix and StreamName types with
constructors/factories that validate allowed characters/format (e.g., NATS
subject rules) and convert/parse from String; then change GithubConfig's
subject_prefix: String and stream_name: String to subject_prefix: SubjectPrefix
and stream_name: StreamName, and update any code that constructs GithubConfig to
use the new factories (validate at creation time so rest of code can rely on
correct values).
In `@rsworkspace/crates/trogon-sink-github/src/signature.rs`:
- Around line 30-36: The verify() API currently accepts raw primitives (secret:
&str, body: &[u8], signature_header: &str); refactor to use domain value objects
so invalid forms are unrepresentable: introduce types like GithubWebhookSecret
(wrapping the secret bytes/string) and GithubSignatureHeader (constructed via a
fallible try_from/parse that enforces "sha256=" prefix and validates/decodes
hex, returning SignatureError variants such as MissingPrefix/InvalidHex), then
change pub fn verify to accept GithubWebhookSecret and GithubSignatureHeader
(and body: &[u8] can remain or be wrapped if desired) and move the
prefix-strip/hex-decode logic out of verify into
GithubSignatureHeader::try_from/parse so verify only compares signatures using
the already-validated values.
🪄 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: 8d3a8f17-c4db-40d7-8a01-068eff98990c
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
devops/docker/compose/compose.ymlrsworkspace/.dockerignorersworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rsrsworkspace/crates/trogon-sink-github/Cargo.tomlrsworkspace/crates/trogon-sink-github/Dockerfilersworkspace/crates/trogon-sink-github/src/config.rsrsworkspace/crates/trogon-sink-github/src/constants.rsrsworkspace/crates/trogon-sink-github/src/lib.rsrsworkspace/crates/trogon-sink-github/src/main.rsrsworkspace/crates/trogon-sink-github/src/server.rsrsworkspace/crates/trogon-sink-github/src/signature.rs
✅ Files skipped from review due to trivial changes (4)
- rsworkspace/crates/acp-telemetry/src/service_name.rs
- rsworkspace/.dockerignore
- rsworkspace/crates/trogon-sink-github/src/lib.rs
- rsworkspace/crates/trogon-sink-github/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- rsworkspace/crates/trogon-nats/src/jetstream/mocks.rs
- rsworkspace/crates/trogon-sink-github/src/constants.rs
- rsworkspace/crates/trogon-sink-github/src/server.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/acp-telemetry/src/service_name.rs (1)
27-41: Add test coverage for the newTrogonSinkGithubvariant.The existing tests cover
AcpNatsStdioandAcpNatsWsbut not the newly addedTrogonSinkGithubvariant.Proposed fix to add test coverage
#[test] fn as_str_returns_expected_values() { assert_eq!(ServiceName::AcpNatsStdio.as_str(), "acp-nats-stdio"); assert_eq!(ServiceName::AcpNatsWs.as_str(), "acp-nats-ws"); + assert_eq!(ServiceName::TrogonSinkGithub.as_str(), "trogon-sink-github"); } #[test] fn display_delegates_to_as_str() { assert_eq!(format!("{}", ServiceName::AcpNatsStdio), "acp-nats-stdio"); assert_eq!(format!("{}", ServiceName::AcpNatsWs), "acp-nats-ws"); + assert_eq!(format!("{}", ServiceName::TrogonSinkGithub), "trogon-sink-github"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-telemetry/src/service_name.rs` around lines 27 - 41, Tests for ServiceName currently assert only AcpNatsStdio and AcpNatsWs; add assertions for the new ServiceName::TrogonSinkGithub variant in both tests: update as_str_returns_expected_values to assert ServiceName::TrogonSinkGithub.as_str() returns the expected string (e.g., "trogon-sink-github") and update display_delegates_to_as_str to assert format!("{}", ServiceName::TrogonSinkGithub) equals the same expected string, ensuring you reference the ServiceName::TrogonSinkGithub symbol and the as_str()/Display behavior.rsworkspace/crates/trogon-sink-github/src/server.rs (1)
142-160: Minor: JetStream context created twice inserve().Line 144 creates a
NatsJetStreamClientfor provisioning, and thenrouter()(line 149) creates another one internally (line 126). Consider passing the provisioned client torouter()to avoid recreating it.Proposed refactor
#[cfg(not(coverage))] -pub fn router(config: &GithubConfig, nats: async_nats::Client) -> Router { - let js = NatsJetStreamClient::new(async_nats::jetstream::new(nats)); +pub fn router(config: &GithubConfig, js: NatsJetStreamClient) -> Router { let state = AppState { js, ... }; ... } #[cfg(not(coverage))] pub async fn serve(config: GithubConfig, nats: async_nats::Client) -> Result<(), ServeError> { let js = NatsJetStreamClient::new(async_nats::jetstream::new(nats.clone())); provision(&js, &config) .await .map_err(ServeError::Provision)?; - let app = router(&config, nats); + let app = router(&config, js); ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-sink-github/src/server.rs` around lines 142 - 160, The serve() function currently constructs a NatsJetStreamClient for provisioning and then router() recreates another JetStream context; modify serve() to pass the already-created NatsJetStreamClient into router() instead of letting router() create its own. Update router()'s signature (and any call-sites) to accept the provisioned NatsJetStreamClient (or a reference) alongside GithubConfig and async_nats::Client, remove the internal NatsJetStreamClient::new(...) call inside router(), and use the passed client for any JetStream operations and for calling provision() you already invoked in serve().
🤖 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/acp-telemetry/src/service_name.rs`:
- Around line 27-41: Tests for ServiceName currently assert only AcpNatsStdio
and AcpNatsWs; add assertions for the new ServiceName::TrogonSinkGithub variant
in both tests: update as_str_returns_expected_values to assert
ServiceName::TrogonSinkGithub.as_str() returns the expected string (e.g.,
"trogon-sink-github") and update display_delegates_to_as_str to assert
format!("{}", ServiceName::TrogonSinkGithub) equals the same expected string,
ensuring you reference the ServiceName::TrogonSinkGithub symbol and the
as_str()/Display behavior.
In `@rsworkspace/crates/trogon-sink-github/src/server.rs`:
- Around line 142-160: The serve() function currently constructs a
NatsJetStreamClient for provisioning and then router() recreates another
JetStream context; modify serve() to pass the already-created
NatsJetStreamClient into router() instead of letting router() create its own.
Update router()'s signature (and any call-sites) to accept the provisioned
NatsJetStreamClient (or a reference) alongside GithubConfig and
async_nats::Client, remove the internal NatsJetStreamClient::new(...) call
inside router(), and use the passed client for any JetStream operations and for
calling provision() you already invoked in serve().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d4dab46-5b1c-46fa-bb38-265311a17b53
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
devops/docker/compose/compose.ymlrsworkspace/.dockerignorersworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rsrsworkspace/crates/trogon-sink-github/Cargo.tomlrsworkspace/crates/trogon-sink-github/Dockerfilersworkspace/crates/trogon-sink-github/src/config.rsrsworkspace/crates/trogon-sink-github/src/constants.rsrsworkspace/crates/trogon-sink-github/src/lib.rsrsworkspace/crates/trogon-sink-github/src/main.rsrsworkspace/crates/trogon-sink-github/src/server.rsrsworkspace/crates/trogon-sink-github/src/signature.rs
✅ Files skipped from review due to trivial changes (3)
- rsworkspace/.dockerignore
- rsworkspace/crates/trogon-sink-github/Cargo.toml
- rsworkspace/crates/trogon-sink-github/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rsworkspace/crates/trogon-nats/src/jetstream/mocks.rs
- rsworkspace/crates/trogon-sink-github/src/signature.rs
- rsworkspace/crates/trogon-sink-github/src/lib.rs
yordis
force-pushed
the
github
branch
3 times, most recently
from
29da556 to
aabb40b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-sink-github/src/config.rs (1)
40-67: Consider logging warnings when invalid environment variable values fall back to defaults.Invalid numeric values for
GITHUB_WEBHOOK_PORT,GITHUB_STREAM_MAX_AGE_SECS,GITHUB_NATS_ACK_TIMEOUT_SECS, andGITHUB_MAX_BODY_SIZEsilently fall back to defaults. This can make misconfiguration hard to diagnose in production.Example: Add tracing warnings on parse failures
port: env .var("GITHUB_WEBHOOK_PORT") .ok() .and_then(|p| p.parse().map_err(|e| { tracing::warn!(value = %p, "Invalid GITHUB_WEBHOOK_PORT, using default"); e }).ok()) .unwrap_or(DEFAULT_PORT),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-sink-github/src/config.rs` around lines 40 - 67, The code silently falls back to defaults when parsing env vars for port, stream_max_age, nats_ack_timeout, and max_body_size; update the parsing chains for the fields named port, stream_max_age, nats_ack_timeout, and max_body_size in config.rs to log a warning (e.g., tracing::warn!) when env::var exists but parse() fails before falling back to the DEFAULT_* values, capturing the raw value and context in the log message so misconfigurations are visible in production.
🤖 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-sink-github/src/server.rs`:
- Around line 260-271: The test fixture in test_config() assigns a String to
GithubConfig.webhook_secret which is declared as Option<String>; update
test_config() to supply an Option (e.g., wrap the value with
Some(TEST_SECRET.to_string()) or use None as appropriate) and ensure any related
AppState changes that expect an Option<String> for webhook_secret are applied
consistently (search for test_config, GithubConfig, and AppState to locate
usages to update).
---
Nitpick comments:
In `@rsworkspace/crates/trogon-sink-github/src/config.rs`:
- Around line 40-67: The code silently falls back to defaults when parsing env
vars for port, stream_max_age, nats_ack_timeout, and max_body_size; update the
parsing chains for the fields named port, stream_max_age, nats_ack_timeout, and
max_body_size in config.rs to log a warning (e.g., tracing::warn!) when env::var
exists but parse() fails before falling back to the DEFAULT_* values, capturing
the raw value and context in the log message so misconfigurations are visible in
production.
🪄 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: e1af64d5-5b36-4f0b-bd4d-fb45e6d48336
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
devops/docker/compose/compose.ymlrsworkspace/.dockerignorersworkspace/crates/acp-telemetry/src/service_name.rsrsworkspace/crates/trogon-nats/src/jetstream/mocks.rsrsworkspace/crates/trogon-sink-github/Cargo.tomlrsworkspace/crates/trogon-sink-github/Dockerfilersworkspace/crates/trogon-sink-github/src/config.rsrsworkspace/crates/trogon-sink-github/src/constants.rsrsworkspace/crates/trogon-sink-github/src/lib.rsrsworkspace/crates/trogon-sink-github/src/main.rsrsworkspace/crates/trogon-sink-github/src/server.rsrsworkspace/crates/trogon-sink-github/src/signature.rs
✅ Files skipped from review due to trivial changes (6)
- rsworkspace/.dockerignore
- rsworkspace/crates/trogon-sink-github/Dockerfile
- rsworkspace/crates/trogon-sink-github/Cargo.toml
- rsworkspace/crates/trogon-sink-github/src/signature.rs
- rsworkspace/crates/trogon-sink-github/src/lib.rs
- rsworkspace/crates/trogon-sink-github/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/acp-telemetry/src/service_name.rs
- rsworkspace/crates/trogon-sink-github/src/main.rs
yordis
force-pushed
the
github
branch
2 times, most recently
from
8a6b4fa to
a8ee6e5
Compare
yordis
force-pushed
the
github
branch
4 times, most recently
from
f4ac558 to
277626c
Compare
…ATS JetStream Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
| condition: service_healthy | ||
| restart: unless-stopped | ||
| healthcheck: | ||
| test: ["CMD", "curl", "-sf", "http://localhost:${GITHUB_WEBHOOK_PORT:-8080}/health"] |
There was a problem hiding this comment.
Healthcheck uses curl which is not installed in image
High Severity
The trogon-sink-github healthcheck uses curl, but the Dockerfile's runtime stage (debian:bookworm-slim) only installs ca-certificates — curl is not included in slim images. The healthcheck will always fail, so the container never reaches "healthy" status. Since the smee service depends on condition: service_healthy for trogon-sink-github, the entire dev-profile workflow described in the docs is broken. The same issue was recognized and fixed for the NATS container (switched to nats:alpine for wget), but wasn't addressed here.
Additional Locations (1)
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
trogon-sink-githubcrate: an axum webhook receiver that validates GitHub signatures, maps events to NATS subjects (github.<event>), and publishes to JetStream with deduplication viaNats-Msg-Idheader (X-GitHub-DeliveryUUID)routerandserveare generic overJetStreamPublisher/JetStreamContexttraits — concreteNatsJetStreamClientwired only inmain, enabling full test coverage without#[cfg(not(coverage))]gates on library codeJetStreamPublisher::publish_with_headersreturn type (PublishFuture<'a, T>) to satisfy axum'sHandlerSend bounds with generic handlersSTOPSIGNAL SIGTERM, and health checkdevprofile) with how-to guide for local GitHub App webhook developmentbytesize::ByteSizeformax_body_sizeconfig instead of rawusizeasync_nats::header::NATS_MESSAGE_IDinstead of custom constantSignatureErrorproperly implementsError::source()forInvalidHexvariant