refactor(trogon-nats): require connection_timeout in connect() by yordis · Pull Request #9 · TrogonStack/trogonai

yordis · 2026-02-24T19:30:40Z

Summary

Remove the connect(config) convenience wrapper that hid a hardcoded 10s CONNECTION_TIMEOUT constant
Callers now always pass connection_timeout explicitly to connect(), making the timeout decision visible at the call site
Update doc example in lib.rs to reflect the new signature

Note

The acp-nats crate (on the add-acp branch) already uses connect_with_timeout — it will need a trivial rename to connect when rebased on top of this.

Test plan

cargo check -p trogon-nats passes
cargo check -p trogon-nats --tests passes
CI green

cursor · 2026-02-24T19:30:47Z

PR Summary

Medium Risk
API-breaking signature change that will require updating all call sites; runtime behavior may change if callers choose different timeouts, but the connection logic itself is otherwise unchanged.

Overview
Updates trogon-nats to require callers to pass a connection_timeout: Duration into connect(), removing the previously hardcoded 10s timeout and threading the value through apply_reconnect_options for all auth modes.

Also updates tracing instrumentation to record timeout_secs and adjusts docs/tests by removing the constant-based timeout test and updating the crate example to the new connect(&config, Duration::from_secs(...)) signature.

^{Written by Cursor Bugbot for commit 12a5ba1. This will update automatically on new commits. Configure here.}

coderabbitai · 2026-02-24T19:30:58Z

Warning

Rate limit exceeded

@yordis has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 2 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between c736226 and 12a5ba1.

📒 Files selected for processing (2)

rsworkspace/crates/trogon-nats/src/connect.rs
rsworkspace/crates/trogon-nats/src/lib.rs

Walkthrough

The connect() function and its internal apply_reconnect_options() helper now accept an explicit connection_timeout: Duration parameter instead of using a hardcoded timeout constant. All call sites and documentation were updated to pass this timeout value.

Changes

Cohort / File(s)	Summary
NATS Connection Configuration `rsworkspace/crates/trogon-nats/src/connect.rs`	Updated `connect()` and `apply_reconnect_options()` function signatures to accept explicit `connection_timeout: Duration` parameter; removed hardcoded CONNECTION_TIMEOUT constant; updated all internal match arms to pass timeout to `apply_reconnect_options()`; added timeout_secs to instrumentation metadata.
Module Exports & Documentation `rsworkspace/crates/trogon-nats/src/lib.rs`	Updated documentation example to import Duration and pass 10-second timeout to `connect()`; reordered public re-export declaration from `{ConnectError, connect}` to `{connect, ConnectError}`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 No hidden timeouts in our burrow's code,
Duration explicit on every road,
Each connect call now knows its fate,
Crystal clear—no more to wait! ⏳✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: requiring an explicit connection_timeout parameter in the connect() function instead of using a hardcoded constant.
Description check	✅ Passed	The description clearly explains the refactoring purpose, the API change, and its rationale, directly relating to the changeset modifications in connect.rs and lib.rs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch refactor/nats-connect-require-timeout

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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 (2)

rsworkspace/crates/trogon-nats/src/connect.rs (1)

88-123: ⚠️ Potential issue | 🟡 Minor

CI failing — run cargo fmt before merging.

The pipeline reports rustfmt violations at lines 90 and 112. Run cargo fmt -p trogon-nats to fix the chain/block formatting in the Credentials and UserPassword arms.

🤖 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 88 - 123, The
formatting of the match arms for NatsAuth::Credentials and
NatsAuth::UserPassword breaks rustfmt expectations (chain/block alignment around
ConnectOptions::with_credentials_file and the apply_reconnect_options(...)
.connect(...) chains); run cargo fmt -p trogon-nats to reformat, or adjust the
chained calls in the Credentials arm
(ConnectOptions::with_credentials_file(...).await match block) and the
UserPassword arm
(apply_reconnect_options(ConnectOptions::with_user_and_password(...),
connection_timeout).connect(...).await) so the chains and block indentation
conform to rustfmt style (ensure method chains are broken/indented consistently
and await is aligned).

rsworkspace/crates/trogon-nats/src/lib.rs (1)

47-55: ⚠️ Potential issue | 🟡 Minor

CI failing — run cargo fmt before merging.

The pipeline reports a rustfmt violation at line 47 covering the pub use block. Run cargo fmt -p trogon-nats locally to resolve.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rsworkspace/crates/trogon-nats/src/lib.rs` around lines 47 - 55, CI failed
due to rustfmt formatting in the pub use block; run rustfmt (cargo fmt -p
trogon-nats) and reformat the exports so the `pub use` statements (e.g.,
async_nats::subject::ToSubject, auth::{NatsAuth, NatsConfig},
client::{FlushClient, PublishClient, RequestClient, SubscribeClient},
connect::{connect, ConnectError}, messaging::{FlushPolicy, NatsError,
PublishOperationError, PublishOptions, PublishOptionsBuilder, RetryPolicy,
headers_with_trace_context, inject_trace_context, publish, request,
request_with_timeout}) are properly wrapped/indented per rustfmt rules, then
commit the formatted lib.rs.

🤖 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/connect.rs`:
- Line 77: The telemetry is truncating sub-second durations because
connection_timeout.as_secs() drops fractions; update the instrument attribute on
nats.connect to record a precise timeout: either convert to milliseconds and
rename the field to timeout_ms using connection_timeout.as_millis() (prefer
Display %) or use connection_timeout.as_secs_f64() and keep timeout_secs (use %
for Display); change the fields(...) entry accordingly and replace the current
fields(servers = ?config.servers, auth = %config.auth.description(),
timeout_secs = ?connection_timeout.as_secs()) with the chosen precise value and
Display formatting.

---

Outside diff comments:
In `@rsworkspace/crates/trogon-nats/src/connect.rs`:
- Around line 88-123: The formatting of the match arms for NatsAuth::Credentials
and NatsAuth::UserPassword breaks rustfmt expectations (chain/block alignment
around ConnectOptions::with_credentials_file and the
apply_reconnect_options(...) .connect(...) chains); run cargo fmt -p trogon-nats
to reformat, or adjust the chained calls in the Credentials arm
(ConnectOptions::with_credentials_file(...).await match block) and the
UserPassword arm
(apply_reconnect_options(ConnectOptions::with_user_and_password(...),
connection_timeout).connect(...).await) so the chains and block indentation
conform to rustfmt style (ensure method chains are broken/indented consistently
and await is aligned).

In `@rsworkspace/crates/trogon-nats/src/lib.rs`:
- Around line 47-55: CI failed due to rustfmt formatting in the pub use block;
run rustfmt (cargo fmt -p trogon-nats) and reformat the exports so the `pub use`
statements (e.g., async_nats::subject::ToSubject, auth::{NatsAuth, NatsConfig},
client::{FlushClient, PublishClient, RequestClient, SubscribeClient},
connect::{connect, ConnectError}, messaging::{FlushPolicy, NatsError,
PublishOperationError, PublishOptions, PublishOptionsBuilder, RetryPolicy,
headers_with_trace_context, inject_trace_context, publish, request,
request_with_timeout}) are properly wrapped/indented per rustfmt rules, then
commit the formatted lib.rs.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54c0d00 and c736226.

📒 Files selected for processing (2)

rsworkspace/crates/trogon-nats/src/connect.rs
rsworkspace/crates/trogon-nats/src/lib.rs

Remove the convenience wrapper that hid a hardcoded 10s timeout behind CONNECTION_TIMEOUT. Callers now explicitly provide the timeout, making the configuration decision visible at the call site (main.rs) rather than buried in library internals. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

cursor · 2026-02-24T19:48:45Z

 /// Connect to NATS with automatic reconnection and event handling.
-#[instrument(name = "nats.connect", skip(config), fields(servers = ?config.servers, auth = %config.auth.description()))]
-pub async fn connect(config: &NatsConfig) -> Result<Client, ConnectError> {
+#[instrument(name = "nats.connect", skip(config), fields(servers = ?config.servers, auth = %config.auth.description(), timeout_secs = ?connection_timeout.as_secs()))]


Redundant logging of connection_timeout parameter

Low Severity

The connection_timeout parameter is logged twice in the tracing span: once automatically by the instrument macro (since it's not in skip()) and once explicitly as timeout_secs in the fields(). This creates redundant telemetry data where the same timeout value appears in two formats.

#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.

…ar dedup check Three durability fixes: #1 — Re-enable checkpointing each turn `checkpointing_disabled` was sticky: once set true it never cleared, so a run that hit the size limit on turn N skipped all subsequent checkpoints even if later turns produced smaller payloads. Moved the declaration inside the `tool_use` match arm so it resets to `false` on every iteration. The size-guard re-disables it for the current turn only if trimming still fails. #5 — Reload + retry on Resolved write failure The `end_turn` Resolved write previously just logged a warning on CAS conflict or timeout, leaving the promise `Running`. Added a reload-and-retry path: on any write failure, `get_promise` is called to fetch the current revision; if the promise is still not Resolved, a second `update_promise` is attempted with the fresh revision. Mirrors the existing retry pattern in the checkpoint write path. #9 — Retry Linear `get_comments` pre-check once before graceful degradation A single transient network error in the idempotency pre-check was enough to bypass duplicate detection and post a duplicate comment. Now retries `get_comments` once; only proceeds without the check if both attempts fail.

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>