feat(trogon-nats): add trogon-nats pkg - #4
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Msg as messaging.rs
participant Client as NatsClient (impl)
participant Server as async_nats::Server
participant Trace as OpenTelemetry
rect rgba(200,230,255,0.5)
App->>Msg: publish(request, options)
Msg->>Trace: inject_trace_context(headers)
Msg->>Client: publish_with_headers(subject, headers, payload)
Client->>Server: send publish
Server-->>Client: ack
Client-->>Msg: result
alt flush requested
Msg->>Client: flush()
Client->>Server: flush
Client-->>Msg: flush result
end
Msg-->>App: success / PublishOperationError (after retries)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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
add-nats-crate
branch
5 times, most recently
from
February 18, 2026 17:20
4eb4d1f to
d6b09cc
Compare
yordis
marked this pull request as ready for review
February 18, 2026 17:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
rsworkspace/Cargo.toml (1)
7-7:strip = "symbols"removes all symbol names, making production backtraces address-only.
strip = "symbols"strips the full symbol table (function names, etc.), not just DWARF debug info. Backtraces in panics or crash dumps will show raw addresses with no function names, complicating incident investigation.strip = "debuginfo"achieves the primary goal (smaller binary, no DWARF sections) while preserving the symbol table for readable stack traces.♻️ Proposed change
-strip = "symbols" +strip = "debuginfo"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/Cargo.toml` at line 7, The Cargo.toml currently sets strip = "symbols", which removes the symbol table; change that value to strip = "debuginfo" (or remove the strip key) in the same Cargo.toml so the build strips DWARF debug info but preserves symbol names for readable backtraces; locate the existing strip = "symbols" entry and replace it with strip = "debuginfo" (or delete the line) to apply the fix..github/workflows/ci-rust.yml (3)
41-46:cargo build --release+cargo llvm-covcauses two independent full compilations.
cargo build --releaseuses a different build profile andRUSTFLAGSthancargo llvm-cov(which compiles with LLVM instrumentation in debug mode). The two artifact trees cannot be shared, so the entire workspace is compiled twice on every CI run.If the release-build validation step exists only to confirm the code compiles, consider whether a single
cargo llvm-covrun is sufficient for this CI job, or extract the release build into a separate dedicated job to make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-rust.yml around lines 41 - 46, The CI currently runs two full Rust compilations because "cargo build --release" and "cargo llvm-cov" use different profiles/RUSTFLAGS and cannot share artifacts; remove the redundant "cargo build --release" step or move it to a separate job so the workspace is not compiled twice — either keep only "cargo llvm-cov --all-features --workspace" for compilation+coverage, or create a dedicated release job that runs "cargo build --release" separately (and update job names/steps accordingly) to make intent explicit and avoid duplicate builds.
45-46:cargo llvm-covwithout output flags does not generatelcov.info.The command as written produces a terminal summary only. The
lcov.infoentry in.gitignoreimplies intent to generate a report file locally (--lcov --output-path lcov.info), but there is no corresponding upload step in CI.Consider adding a report-generation and upload step to make coverage tracking actionable:
➕ Example: generate report and upload to Codecov
- - name: Run tests with coverage - run: cargo llvm-cov --all-features --workspace + - name: Run tests with coverage + run: cargo llvm-cov --all-features --workspace --lcov --output-path lcov.info working-directory: rsworkspace + + - name: Upload coverage to Codecov + uses: codecov/codecov-action@v4 + with: + files: rsworkspace/lcov.info + fail_ci_if_error: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-rust.yml around lines 45 - 46, The workflow step named "Run tests with coverage" currently runs cargo llvm-cov --all-features --workspace which only prints a terminal summary and doesn't create lcov.info; modify that step to generate a report (add the --lcov and --output-path lcov.info flags to the cargo llvm-cov invocation) and then add a follow-up CI step to publish the artifact (e.g., upload lcov.info via actions/upload-artifact or send it to Codecov with codecov/codecov-action) so the generated lcov.info is saved and available for coverage tracking.
28-31: Pintaiki-e/install-actionto an exact version for consistency.Every other action in this file is pinned to a full
major.minor.patchtag (e.g.,@v4.3.1,@v3.6.1,@v2.8.2).@v2is a floating major-version reference — the action code itself can drift without notice, unlike the SHA256-verified tool downloads it manages.The action README itself documents
@v2as a valid shorthand when you "do not need to pin the versions," and notes you can omit the minor version when the major version is ≥ 1. For a security-conscious CI pipeline, prefer an exact version to stay consistent with the rest of the file.♻️ Proposed fix
- uses: taiki-e/install-action@v2 + uses: taiki-e/install-action@v2.x.y # replace with the current exact version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-rust.yml around lines 28 - 31, The workflow uses taiki-e/install-action with a floating tag "taiki-e/install-action@v2"; replace that with an exact major.minor.patch tag (pin to a full version like `@v2.x.y`) for the step installing cargo-llvm-cov so the action cannot drift. Update the uses entry referencing taiki-e/install-action@v2 in the Install cargo-llvm-cov step to the exact release used elsewhere in the file (major.minor.patch), and ensure the change is applied consistently with other pinned actions.rsworkspace/crates/trogon-nats/src/auth.rs (1)
82-96: Consider validating against empty auth strings.
env.var()returningOk("")(empty string) forNATS_CREDS,NATS_NKEY, orNATS_TOKENwould produce auth configs likeCredentials(PathBuf::from(""))orToken(""), which would fail cryptically downstream. A quick.filter(|v| !v.is_empty())before accepting each value would prevent silent misconfiguration.Example for one variant
- if let Ok(creds_path) = env.var(ENV_NATS_CREDS) { + if let Ok(creds_path) = env.var(ENV_NATS_CREDS).and_then(|v| if v.is_empty() { Err(std::env::VarError::NotPresent) } else { Ok(v) }) { return NatsAuth::Credentials(PathBuf::from(creds_path)); }Or extract a helper:
fn non_empty_var<E: ReadEnv>(env: &E, key: &str) -> Result<String, ...>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/auth.rs` around lines 82 - 96, The auth_from_env function currently accepts empty strings from env.var and can return invalid NatsAuth variants; change it to reject empty values (e.g., use .filter(|v| !v.is_empty()) or add a helper non_empty_var that calls env.var and returns Err if the string is empty) before constructing NatsAuth::Credentials(PathBuf::from(...)), NatsAuth::NKey(...), and NatsAuth::Token(...); keep the existing user/password behavior but ensure user and password are non-empty if you want to reject empty credentials as well—update auth_from_env to use that helper or inline checks for ENV_NATS_CREDS, ENV_NATS_NKEY, ENV_NATS_USER, ENV_NATS_PASSWORD, and ENV_NATS_TOKEN.rsworkspace/crates/trogon-nats/src/messaging.rs (1)
260-276: Minor:payloadis cloned asVec<u8>on every retry — convert toBytesonce.
payload.clone().into()clones theVec<u8>and converts toByteson each attempt. Converting toBytesonce before the retry loop would make subsequent clones cheap (Arc-based).Proposed optimization
let payload = serde_json::to_vec(request).map_err(NatsError::Serialize)?; + let payload: bytes::Bytes = payload.into(); let headers = headers_with_trace_context(); options .publish_retry_policy .execute( || async { client .publish_with_headers( subject.to_string(), headers.clone(), - payload.clone().into(), + payload.clone(), ) .await .map_err(|e| PublishOperationError(e.to_string())) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/messaging.rs` around lines 260 - 276, The current retry block passes payload.clone().into() into client.publish_with_headers on every retry, causing a full Vec<u8> clone each attempt; convert the payload to a Bytes once before calling options.publish_retry_policy.execute (e.g. create a Bytes value from payload) and then pass that Bytes.clone() (cheap Arc-style clone) into client.publish_with_headers inside the closure used by execute so retries avoid repeated full Vec copies; update references around publish_retry_policy.execute, the closure, and client.publish_with_headers to use the precomputed Bytes value instead of payload.clone().rsworkspace/crates/trogon-nats/src/mocks.rs (1)
36-43:published_messages()discards payloads — consider exposing them.
published_messages()only returns subjects, and thepayloadfield onPublishedMessageis#[allow(dead_code)]. Tests may need to assert on published payloads. Consider either exposing a method that returns(subject, payload)pairs, or removing thepayloadfield if there's no plan to use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/mocks.rs` around lines 36 - 43, published_messages() currently returns only subjects and discards the payload field (which is marked #[allow(dead_code)]) — add an accessor that returns both subject and payload so tests can assert on published payloads: implement a new method (e.g., published_messages_with_payloads or published_messages_full) on the same type that locks self.published, iterates over the Vec<PublishedMessage>, and returns a Vec<(String, Vec<u8>)> (or Vec<PublishedMessage> if PublishedMessage is Clone) by cloning subject and payload; alternatively, if payload is truly unused, remove the payload field from the PublishedMessage struct and delete #[allow(dead_code)] to keep the code clean.rsworkspace/crates/trogon-nats/src/client.rs (1)
6-32: Consider whether the single-trait design aligns with the crate's coding guideline.The crate guideline says "Prefer one trait per operation over a single trait with multiple operations."
NatsClientbundles four operations (subscribe, request, publish, flush). The cohesion is reasonable here since all operations share a connection, and the separate associated error types per method already provide good granularity. Flagging for awareness rather than as a mandatory change.As per coding guidelines:
rsworkspace/crates/**/*.rs: "Prefer one trait per operation over a single trait with multiple operations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/client.rs` around lines 6 - 32, The NatsClient trait groups multiple operations contrary to the crate guideline; split it into one-trait-per-operation: define SubscribeClient (with associated type SubscribeError and method subscribe), RequestClient (RequestError + request_with_headers), PublishClient (PublishError + publish_with_headers) and FlushClient (FlushError + flush), then implement these traits for the existing client type and update any bounds/usages that reference NatsClient to use the specific operation traits; alternatively, if you intentionally keep the single trait, add a brief doc comment on NatsClient explaining the cohesion rationale and reference the guideline to justify the exception.rsworkspace/crates/trogon-nats/src/connect.rs (2)
44-55: Considerdebug!instead ofinfo!for reconnect delay logging.During extended outages,
reconnect_delaywill be called on every reconnect attempt, makinginfo!-level logs noisy. Significant state changes (connected, disconnected) already log atinfo!/warn!inhandle_event. Demoting this todebug!keeps the signal-to-noise ratio high.Suggested change
- info!( + debug!( attempts, delay_secs = delay.as_secs(), "NATS reconnect delay" );(Add
debugto thetracingimport on line 4.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/connect.rs` around lines 44 - 55, The reconnect_delay function currently logs every reconnect attempt at info! which is noisy during outages; change the logging macro from info! to debug! inside reconnect_delay (function name reconnect_delay) and ensure the tracing crate import includes debug by adding debug to the tracing import list so the debug-level macro is available.
78-84: Redundant entry log —#[instrument]already records these fields.The
#[instrument]attribute on line 78 recordsserversandauthas span fields and logs span entry. Theinfo!at lines 80-84 re-emits the same data. Any log within the span already inherits those fields, so the explicit entry log can be removed to reduce noise.Suggested change
pub async fn connect(config: &NatsConfig) -> Result<Client, ConnectError> { - info!( - servers = ?config.servers, - auth = %config.auth.description(), - "Connecting to NATS" - ); - let connect_result = match &config.auth {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/connect.rs` around lines 78 - 84, The #[instrument(name = "nats.connect", skip(config), fields(servers = ?config.servers, auth = %config.auth.description()))] on the connect function already records servers and auth on span entry, so remove the redundant info! call inside pub async fn connect(config: &NatsConfig) -> Result<Client, ConnectError> that re-logs the same fields (the info! block that references servers = ?config.servers and auth = %config.auth.description()) to avoid duplicate logs; if you still want an entry message, replace the info! with a plain message not repeating those span fields.
🤖 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`:
- Line 9: The workspace-wide profile sets panic = "abort" which will cause
panics inside async tasks (e.g., tokio/async-nats usage in trogon-nats) to abort
the process; remove or scope the panic = "abort" setting from the workspace
Cargo.toml and instead apply it only to specific binary crates that require it
(or delete the line entirely) so that default unwinding/catch_unwind behavior
remains for libraries using tokio/async-nats; locate the literal panic = "abort"
entry in the Cargo.toml and either move it into individual package [profile.*]
sections or remove it to preserve panic unwinding semantics.
In `@rsworkspace/crates/trogon-nats/src/connect.rs`:
- Around line 57-68: The Event::SlowConsumer binding in async fn handle_event is
misleadingly named `subject` but actually contains a u64 subscription id; rename
the pattern binding to `sid` in the match arm (Event::SlowConsumer(sid)) and
change the log key from subject to sid so the emitted warn! uses the correct
identifier and type for the subscription ID; update the Event::SlowConsumer
match arm in handle_event accordingly.
In `@rsworkspace/crates/trogon-nats/src/messaging.rs`:
- Around line 58-59: The current tracing::debug! call logs the full response
payload (payload_str) which can leak sensitive data; update the logging in
messaging.rs (the handling around response.payload and payload_str) to avoid raw
payload output — either log only metadata (e.g., payload length via
response.payload.len()), a truncated safe preview (e.g., first N characters of
payload_str with an indicator like "...[truncated]"), or gate full payload
logging behind a feature flag/config toggle so verbose payload output is opt-in;
change the tracing::debug! invocation to use one of these safer alternatives and
ensure payload_str is not logged raw by default.
- Around line 112-146: The exponential backoff calculation can panic when
shifting by >=32 bits; update the loop in the publish retry logic (where
attempts, self.max_retries and self.initial_retry_delay are used) to clamp the
exponent before shifting—compute let exp = (attempts - 1).min(31) (or otherwise
cap to 31) and use that when computing delay instead of (1 << (attempts - 1)),
or replace the shift with a checked/saturating multiply so delay =
self.initial_retry_delay * (1u32 << exp) (or equivalent saturating logic) to
avoid panics/wraps for large max_retries. Ensure the final delay variable used
in tokio::time::sleep is a valid Duration.
---
Nitpick comments:
In @.github/workflows/ci-rust.yml:
- Around line 41-46: The CI currently runs two full Rust compilations because
"cargo build --release" and "cargo llvm-cov" use different profiles/RUSTFLAGS
and cannot share artifacts; remove the redundant "cargo build --release" step or
move it to a separate job so the workspace is not compiled twice — either keep
only "cargo llvm-cov --all-features --workspace" for compilation+coverage, or
create a dedicated release job that runs "cargo build --release" separately (and
update job names/steps accordingly) to make intent explicit and avoid duplicate
builds.
- Around line 45-46: The workflow step named "Run tests with coverage" currently
runs cargo llvm-cov --all-features --workspace which only prints a terminal
summary and doesn't create lcov.info; modify that step to generate a report (add
the --lcov and --output-path lcov.info flags to the cargo llvm-cov invocation)
and then add a follow-up CI step to publish the artifact (e.g., upload lcov.info
via actions/upload-artifact or send it to Codecov with codecov/codecov-action)
so the generated lcov.info is saved and available for coverage tracking.
- Around line 28-31: The workflow uses taiki-e/install-action with a floating
tag "taiki-e/install-action@v2"; replace that with an exact major.minor.patch
tag (pin to a full version like `@v2.x.y`) for the step installing cargo-llvm-cov
so the action cannot drift. Update the uses entry referencing
taiki-e/install-action@v2 in the Install cargo-llvm-cov step to the exact
release used elsewhere in the file (major.minor.patch), and ensure the change is
applied consistently with other pinned actions.
In `@rsworkspace/Cargo.toml`:
- Line 7: The Cargo.toml currently sets strip = "symbols", which removes the
symbol table; change that value to strip = "debuginfo" (or remove the strip key)
in the same Cargo.toml so the build strips DWARF debug info but preserves symbol
names for readable backtraces; locate the existing strip = "symbols" entry and
replace it with strip = "debuginfo" (or delete the line) to apply the fix.
In `@rsworkspace/crates/trogon-nats/src/auth.rs`:
- Around line 82-96: The auth_from_env function currently accepts empty strings
from env.var and can return invalid NatsAuth variants; change it to reject empty
values (e.g., use .filter(|v| !v.is_empty()) or add a helper non_empty_var that
calls env.var and returns Err if the string is empty) before constructing
NatsAuth::Credentials(PathBuf::from(...)), NatsAuth::NKey(...), and
NatsAuth::Token(...); keep the existing user/password behavior but ensure user
and password are non-empty if you want to reject empty credentials as
well—update auth_from_env to use that helper or inline checks for
ENV_NATS_CREDS, ENV_NATS_NKEY, ENV_NATS_USER, ENV_NATS_PASSWORD, and
ENV_NATS_TOKEN.
In `@rsworkspace/crates/trogon-nats/src/client.rs`:
- Around line 6-32: The NatsClient trait groups multiple operations contrary to
the crate guideline; split it into one-trait-per-operation: define
SubscribeClient (with associated type SubscribeError and method subscribe),
RequestClient (RequestError + request_with_headers), PublishClient (PublishError
+ publish_with_headers) and FlushClient (FlushError + flush), then implement
these traits for the existing client type and update any bounds/usages that
reference NatsClient to use the specific operation traits; alternatively, if you
intentionally keep the single trait, add a brief doc comment on NatsClient
explaining the cohesion rationale and reference the guideline to justify the
exception.
In `@rsworkspace/crates/trogon-nats/src/connect.rs`:
- Around line 44-55: The reconnect_delay function currently logs every reconnect
attempt at info! which is noisy during outages; change the logging macro from
info! to debug! inside reconnect_delay (function name reconnect_delay) and
ensure the tracing crate import includes debug by adding debug to the tracing
import list so the debug-level macro is available.
- Around line 78-84: The #[instrument(name = "nats.connect", skip(config),
fields(servers = ?config.servers, auth = %config.auth.description()))] on the
connect function already records servers and auth on span entry, so remove the
redundant info! call inside pub async fn connect(config: &NatsConfig) ->
Result<Client, ConnectError> that re-logs the same fields (the info! block that
references servers = ?config.servers and auth = %config.auth.description()) to
avoid duplicate logs; if you still want an entry message, replace the info! with
a plain message not repeating those span fields.
In `@rsworkspace/crates/trogon-nats/src/messaging.rs`:
- Around line 260-276: The current retry block passes payload.clone().into()
into client.publish_with_headers on every retry, causing a full Vec<u8> clone
each attempt; convert the payload to a Bytes once before calling
options.publish_retry_policy.execute (e.g. create a Bytes value from payload)
and then pass that Bytes.clone() (cheap Arc-style clone) into
client.publish_with_headers inside the closure used by execute so retries avoid
repeated full Vec copies; update references around publish_retry_policy.execute,
the closure, and client.publish_with_headers to use the precomputed Bytes value
instead of payload.clone().
In `@rsworkspace/crates/trogon-nats/src/mocks.rs`:
- Around line 36-43: published_messages() currently returns only subjects and
discards the payload field (which is marked #[allow(dead_code)]) — add an
accessor that returns both subject and payload so tests can assert on published
payloads: implement a new method (e.g., published_messages_with_payloads or
published_messages_full) on the same type that locks self.published, iterates
over the Vec<PublishedMessage>, and returns a Vec<(String, Vec<u8>)> (or
Vec<PublishedMessage> if PublishedMessage is Clone) by cloning subject and
payload; alternatively, if payload is truly unused, remove the payload field
from the PublishedMessage struct and delete #[allow(dead_code)] to keep the code
clean.
yordis
force-pushed
the
add-nats-crate
branch
3 times, most recently
from
February 18, 2026 17:36
0ad2fba to
95c70f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-nats/src/messaging.rs (1)
648-797: FourRetryPolicy::executetests are unnecessarily gated behind#[cfg(feature = "test-support")].
test_retry_policy_execute_success_first_attempt,test_retry_policy_execute_success_after_retries,test_retry_policy_execute_exhausted, andtest_retry_policy_no_retries_fails_immediatelyuse onlyArc<Mutex<i32>>andPublishOperationError— no mock client at all. Gating them behindtest-supportmeans they're silently skipped in a standardcargo testrun, reducing coverage confidence.♻️ Proposed fix
- #[tokio::test] - #[cfg(feature = "test-support")] - async fn test_retry_policy_execute_success_first_attempt() { + #[tokio::test] + async fn test_retry_policy_execute_success_first_attempt() { - #[tokio::test] - #[cfg(feature = "test-support")] - async fn test_retry_policy_execute_success_after_retries() { + #[tokio::test] + async fn test_retry_policy_execute_success_after_retries() { - #[tokio::test] - #[cfg(feature = "test-support")] - async fn test_retry_policy_execute_exhausted() { + #[tokio::test] + async fn test_retry_policy_execute_exhausted() { - #[tokio::test] - #[cfg(feature = "test-support")] - async fn test_retry_policy_no_retries_fails_immediately() { + #[tokio::test] + async fn test_retry_policy_no_retries_fails_immediately() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/messaging.rs` around lines 648 - 797, The four tests (test_retry_policy_execute_success_first_attempt, test_retry_policy_execute_success_after_retries, test_retry_policy_execute_exhausted, test_retry_policy_no_retries_fails_immediately) are unnecessarily gated by #[cfg(feature = "test-support")] and thus skipped in normal runs; remove the #[cfg(feature = "test-support")] attribute from these test functions so they run under normal cargo test, leaving the #[tokio::test] attribute and keeping the existing use of RetryPolicy::standard()/no_retries() and PublishOperationError logic unchanged.
🤖 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-nats/src/client.rs`:
- Around line 6-32: The NatsClient trait bundles four distinct operations
(subscribe, request_with_headers, publish_with_headers, flush) which violates
the "one trait per operation" guideline; split it into four small traits (e.g.,
SubscribeClient with subscribe and associated SubscribeError, RequestClient with
request_with_headers and RequestError, PublishClient with publish_with_headers
and PublishError, FlushClient with flush and FlushError), keep the same method
signatures and bounds (impl Future + Send) and retain required trait bounds
(Send + Sync + Clone + 'static) on implementations as needed, implement these
new traits for the existing concrete client types that currently implement
NatsClient, and update callers/tests to depend on the specific operation
trait(s) they need and adjust mocks to only implement the relevant trait(s)
instead of a monolithic NatsClient.
---
Duplicate comments:
In `@rsworkspace/crates/trogon-nats/src/connect.rs`:
- Line 63: The Event::SlowConsumer pattern currently binds the payload as
"subject" and logs it with warn!(subject, "NATS slow consumer detected"), but
SlowConsumer carries a u64 subscription id (sid) not a subject string; update
the pattern to bind the value as sid (e.g., Event::SlowConsumer(sid)) and change
the warn! call to log the numeric sid (include sid in the message or as a
key/value) so the log reflects the subscription id correctly instead of a
subject name.
In `@rsworkspace/crates/trogon-nats/src/messaging.rs`:
- Around line 58-64: The code unconditionally logs the raw response payload via
payload_str in the tracing::debug! call (and in the error path) which may leak
PII/secrets; change this to avoid logging raw payloads—remove or redact
payload_str from the unconditional tracing::debug! and only log a safe,
non-sensitive summary (e.g., payload length, truncated hash, or an explicit
"[REDACTED]") before calling serde_json::from_slice(&response.payload), and in
the serde_json::from_slice error map (where NatsError::Deserialize is created)
log only the safe summary and the serialization error (e) without including the
full payload; reference payload_str, tracing::debug!, serde_json::from_slice,
response.payload, and NatsError::Deserialize when making the changes.
- Around line 130-132: The backoff calculation uses 1 << (attempts - 1) which
overflows for attempts-1 >= 32; change it to a safe, capped shift or checked
shift to avoid panics/wraps. Replace the raw bit-shift expression with something
like: compute exponent = (attempts.saturating_sub(1)).min(31) (or use
checked_shl on a u64 and fallback), then calculate delay =
self.initial_retry_delay.saturating_mul(1u64.checked_shl(exponent).unwrap_or(u64::MAX))
or otherwise cap to a max delay; update the code around last_error, attempts,
self.max_retries and self.initial_retry_delay to use the safe exponent
calculation so retries >= 33 no longer panic.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-nats/src/messaging.rs`:
- Around line 648-797: The four tests
(test_retry_policy_execute_success_first_attempt,
test_retry_policy_execute_success_after_retries,
test_retry_policy_execute_exhausted,
test_retry_policy_no_retries_fails_immediately) are unnecessarily gated by
#[cfg(feature = "test-support")] and thus skipped in normal runs; remove the
#[cfg(feature = "test-support")] attribute from these test functions so they run
under normal cargo test, leaving the #[tokio::test] attribute and keeping the
existing use of RetryPolicy::standard()/no_retries() and PublishOperationError
logic unchanged.
yordis
force-pushed
the
add-nats-crate
branch
from
February 18, 2026 17:44
95c70f4 to
bd72ad5
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
add-nats-crate
branch
from
February 18, 2026 17:50
bd72ad5 to
70cf205
Compare
This was referenced Feb 24, 2026
This was referenced Mar 6, 2026
This was referenced Mar 14, 2026
This was referenced Mar 25, 2026
yordis
added a commit
that referenced
this pull request
May 26, 2026
yordis
added a commit
that referenced
this pull request
Jun 16, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 16, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
#2 — recovering flag now flips to false only after a successful checkpoint write, not after the first tool turn. With checkpointing_disabled=true the flag stays true throughout so every subsequent tool turn consults the KV result cache, preventing re-execution after a crash in a disabled-checkpoint run. #4 — write_promise_terminal retries with a fresh KV revision on CAS conflict. When checkpointing is disabled the pre-LLM heartbeat advances the revision without updating the local copy, making the stored revision stale. The retry reloads get_promise and re-attempts the write so the promise is durably marked terminal instead of staying Running. #9 — Linear post_comment now embeds a trogon-idempotency-key HTML marker in the comment body and scans existing comments for that marker before posting, mirroring the defence-in-depth approach used for GitHub post_pr_comment. The Idempotency-Key header is still passed as primary protection; the marker scan is a fallback for header-window expiry. Three pipeline e2e tests updated to mock the new get_comments pre-check request. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
…tomation, reload keeps rev Four durability/correctness fixes: #1 — Paginate GitHub PR comment dedup check The dedup fetch used GitHub's default pagination (~30 items). Comments on active PRs frequently exceed this, placing the original comment on page 2+ where the dedup check missed it and posted a duplicate. Now paginates with per_page=100 up to 10 pages (1000 comments), breaking early when the marker is found or the page is the last one. #4 — HTML-safe idempotency marker The marker format `<!-- trogon-idempotency-key: {key} -->` is terminated by the first `--` sequence in the key. If the key contains `--`, the HTML comment closes prematurely and `body.contains(&marker)` always returns false, silently bypassing the dedup check. Added `idempotency_marker()` helper in tools/mod.rs that replaces `--` with `__` before embedding the key. Used by both Linear and GitHub; the substitution is applied identically on write and scan so matching is still exact. #10 — Startup recovery marks disabled automations PermanentFailed A disabled automation left a stale promise cycling on every process restart — the code only checked for deleted automations, not disabled ones. Merged the deleted and disabled cases into a shared `perm_fail_reason` block that marks the promise PermanentFailed and continues, eliminating the cycle. #11 — CAS/timeout reload failure keeps checkpoint revision instead of None When a checkpoint write timed out or CAS-conflicted and the subsequent get_promise reload also failed, `checkpoint` was set to `None` — permanently blocking terminal-status writes (Resolved, PermanentFailed) for the rest of the run, leaving the promise stuck as Running. Now: only a definitive `Ok(Ok(None))` response (promise genuinely gone from KV) sets checkpoint=None; errors and timeouts preserve the current revision so the terminal write can still succeed, and the next checkpoint write will retry with that revision. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
Gap #4: the ext_method reply was a bare value, and ExtResponse(RawValue) accepts any JSON, so a runner Error masqueraded as a successful body — every ext error (compactor down, session not found, ...) showed as "invalid ext response" instead of the real message, in both the CLI and the IDE proxy. Fix the ext reply structurally (not heuristically): the serve loop now wraps the ext response in a discriminated envelope { "result": <body> } on success and { "error": { code, message, .. } } on error. Both clients (trogon-cli ext_method + acp-nats agent/ext_method) parse the envelope and surface the error message. Targeted to ext only — the typed methods already fall back to the Error on a failed typed parse, so they don't mask. Clients also tolerate the old bare format (bare Error / bare ExtResponse) so an un-upgraded runner still works during rollout. Tests: acp-nats-agent 70, acp-nats 739, trogon-cli 220 — all green, incl. a new empirical test asserting a runner error surfaces (not masked). Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 23, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 25, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 25, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 25, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 25, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
…al decision Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 26, 2026
…al decision 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.
Signed-off-by: Yordis Prieto yordis.prieto@gmail.com