chore(rsworkspace): derive error implementations - #208
Merged
Conversation
yordis
commented
Jun 8, 2026
yordis
commented
Member
- Reduce the maintenance burden of hand-rolled error plumbing across Rust crates.
- Keep error reporting consistent as the workspace grows.
PR SummaryLow Risk Overview Library errors drop hand-written Binaries and top-level flows use Tests and docs are updated for new error shapes and Reviewed by Cursor Bugbot for commit 39b4631. Bugbot is set up for automated code reviews on this repo. Configure here. |
yordis
force-pushed
the
yordis/kathmandu-v1
branch
from
June 8, 2026 18:18
85925c5 to
a60b5c5
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/kathmandu-v1
branch
from
June 8, 2026 18:20
a60b5c5 to
276ef57
Compare
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 39b4631 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Replace ad-hoc String formatting in connection and telemetry shutdown paths with structured error types so sources chain correctly in logs and display output. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
|
Warning Review limit reached
More reviews will be available in 37 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughWorkspace dependencies add ChangesWorkspace-wide error handling refactor
Sequence Diagram(s)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 25dd712. Configure here.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added
the
rust:coverage-baseline-reset
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-nats/src/messaging.rs (1)
317-319: ⚖️ Poor tradeoffConsider preserving typed error context in
PublishOperationError.
PublishOperationError(pub String)discards the original typed error. The usages at lines 287 and 306 call.to_string()onasync_nats::PublishErrorandasync_nats::FlushError, losing the error chain. Consider wrapping a typed error or usingBox<dyn std::error::Error + Send + Sync>to preserve context.This is outside the immediate scope of this derive-migration PR but would align better with the coding guideline: "Never discard error context by converting a typed error into a string."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rsworkspace/crates/trogon-nats/src/messaging.rs` around lines 317 - 319, PublishOperationError currently stores a plain String which drops typed error context; change its inner type to a boxed error (e.g., PublishOperationError(pub Box<dyn std::error::Error + Send + Sync>)) and update the error construction sites (where async_nats::PublishError and async_nats::FlushError are converted via .to_string() around the PublishOperationError instantiation) to wrap the original errors with Box::new(err) instead of calling to_string(); ensure the error derives/impls (thiserror::Error/Display) delegate to the boxed source so the error chain is preserved and consider adding From impls or constructors for ergonomic conversion from async_nats errors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/acp-nats/src/jetstream/provision.rs`:
- Around line 6-8: ProvisionError currently stores a flattened String; change it
to a typed error that preserves the underlying JetStream SDK error and stream
id—replace the tuple-struct ProvisionError(pub String) with a struct or enum
variant like ProvisionError { stream: String, #[source] source: Box<dyn
std::error::Error + Send + Sync> } (or the concrete SDK error type if available)
and update the code that constructs it (the site using format! on the source) to
supply the stream and the original error in the source field instead of
formatting to a String so source() chaining is preserved.
In `@rsworkspace/crates/trogon-gateway/src/main.rs`:
- Line 47: The current alias SourceResult = (&'static str, Result<(), String>)
and usages that call map_err(|e| e.to_string()) or format!(...) discard error
types and their source chains; change SourceResult to hold a preserved error
type (e.g., Result<(), Box<dyn std::error::Error + Send + Sync>> or a concrete
typed error enum) and update all places that convert errors to String to instead
wrap or propagate the original error (replace map_err(|e| e.to_string()) with
map_err(|e| Box::new(e)) or return the typed error variant), and update any code
handling SourceResult to account for the boxed/typed error so root-cause context
is preserved (apply this to the three occurrences you noted plus any other
map_err/format! sites).
---
Nitpick comments:
In `@rsworkspace/crates/trogon-nats/src/messaging.rs`:
- Around line 317-319: PublishOperationError currently stores a plain String
which drops typed error context; change its inner type to a boxed error (e.g.,
PublishOperationError(pub Box<dyn std::error::Error + Send + Sync>)) and update
the error construction sites (where async_nats::PublishError and
async_nats::FlushError are converted via .to_string() around the
PublishOperationError instantiation) to wrap the original errors with
Box::new(err) instead of calling to_string(); ensure the error derives/impls
(thiserror::Error/Display) delegate to the boxed source so the error chain is
preserved and consider adding From impls or constructors for ergonomic
conversion from async_nats errors.
🪄 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: e433a74f-3f30-4fc2-a62c-a494236baba0
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (113)
rsworkspace/Cargo.tomlrsworkspace/crates/acp-nats-agent/Cargo.tomlrsworkspace/crates/acp-nats-agent/src/connection.rsrsworkspace/crates/acp-nats-server/Cargo.tomlrsworkspace/crates/acp-nats-server/src/acp_connection_id.rsrsworkspace/crates/acp-nats-server/src/config.rsrsworkspace/crates/acp-nats-server/src/connection.rsrsworkspace/crates/acp-nats-server/src/main.rsrsworkspace/crates/acp-nats-server/src/transport.rsrsworkspace/crates/acp-nats-stdio/Cargo.tomlrsworkspace/crates/acp-nats-stdio/src/main.rsrsworkspace/crates/acp-nats/Cargo.tomlrsworkspace/crates/acp-nats/src/acp_prefix.rsrsworkspace/crates/acp-nats/src/client/ext.rsrsworkspace/crates/acp-nats/src/client/fs_read_text_file.rsrsworkspace/crates/acp-nats/src/client/fs_write_text_file.rsrsworkspace/crates/acp-nats/src/client/request_permission.rsrsworkspace/crates/acp-nats/src/client/terminal_create.rsrsworkspace/crates/acp-nats/src/client/terminal_kill.rsrsworkspace/crates/acp-nats/src/client/terminal_release.rsrsworkspace/crates/acp-nats/src/client/terminal_wait_for_exit.rsrsworkspace/crates/acp-nats/src/ext_method_name.rsrsworkspace/crates/acp-nats/src/jetstream/provision.rsrsworkspace/crates/acp-nats/src/pending_prompt_waiters.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/mcp-nats-server/Cargo.tomlrsworkspace/crates/mcp-nats-server/src/allowed_host.rsrsworkspace/crates/mcp-nats-server/src/config.rsrsworkspace/crates/mcp-nats-server/src/main.rsrsworkspace/crates/mcp-nats-stdio/Cargo.tomlrsworkspace/crates/mcp-nats-stdio/src/config.rsrsworkspace/crates/mcp-nats-stdio/src/main.rsrsworkspace/crates/mcp-nats/Cargo.tomlrsworkspace/crates/mcp-nats/src/mcp_peer_id.rsrsworkspace/crates/mcp-nats/src/mcp_prefix.rsrsworkspace/crates/mcp-nats/src/transport.rsrsworkspace/crates/trogon-decider-nats/Cargo.tomlrsworkspace/crates/trogon-decider-nats/src/snapshot_store.rsrsworkspace/crates/trogon-decider-nats/src/store.rsrsworkspace/crates/trogon-decider-nats/src/stream_store.rsrsworkspace/crates/trogon-decider-runtime/Cargo.tomlrsworkspace/crates/trogon-decider-runtime/src/execution.rsrsworkspace/crates/trogon-decider-runtime/src/headers/from_entries_error.rsrsworkspace/crates/trogon-decider-runtime/src/headers/header_name.rsrsworkspace/crates/trogon-decider-runtime/src/headers/header_value.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/codec/encoded_snapshot.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/codec/snapshot_decode_error.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/codec/snapshot_encode_error.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/codec/snapshot_envelope_decode_error.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/codec/snapshot_envelope_encode_error.rsrsworkspace/crates/trogon-decider-runtime/src/snapshot/snapshot_type.rsrsworkspace/crates/trogon-decider-runtime/src/stream/read_stream.rsrsworkspace/crates/trogon-decider-runtime/src/stream/stream_position.rsrsworkspace/crates/trogon-decider/Cargo.tomlrsworkspace/crates/trogon-decider/src/decision.rsrsworkspace/crates/trogon-decider/src/lib.rsrsworkspace/crates/trogon-decider/src/testing.rsrsworkspace/crates/trogon-decider/tests/ui/common.rsrsworkspace/crates/trogon-decider/tests/ui/pass/double_given.rsrsworkspace/crates/trogon-gateway/Cargo.tomlrsworkspace/crates/trogon-gateway/src/config.rsrsworkspace/crates/trogon-gateway/src/main.rsrsworkspace/crates/trogon-gateway/src/source/discord/config.rsrsworkspace/crates/trogon-gateway/src/source/github/signature.rsrsworkspace/crates/trogon-gateway/src/source/gitlab/gitlab_signing_token.rsrsworkspace/crates/trogon-gateway/src/source/incidentio/incidentio_event_type.rsrsworkspace/crates/trogon-gateway/src/source/incidentio/incidentio_signing_secret.rsrsworkspace/crates/trogon-gateway/src/source/notion/notion_event_type.rsrsworkspace/crates/trogon-gateway/src/source/notion/server.rsrsworkspace/crates/trogon-gateway/src/source/notion/signature.rsrsworkspace/crates/trogon-gateway/src/source/notion/verification_token.rsrsworkspace/crates/trogon-gateway/src/source/sentry/server.rsrsworkspace/crates/trogon-gateway/src/source/sentry/signature.rsrsworkspace/crates/trogon-gateway/src/source/slack/config.rsrsworkspace/crates/trogon-gateway/src/source/slack/signature.rsrsworkspace/crates/trogon-gateway/src/source/slack/socket_mode.rsrsworkspace/crates/trogon-gateway/src/source/standard_webhooks.rsrsworkspace/crates/trogon-gateway/src/source/telegram/config.rsrsworkspace/crates/trogon-gateway/src/source/telegram/registration.rsrsworkspace/crates/trogon-gateway/src/source/telegram/signature.rsrsworkspace/crates/trogon-gateway/src/source/twitter/signature.rsrsworkspace/crates/trogon-gateway/src/source_integration_id.rsrsworkspace/crates/trogon-gateway/src/source_status.rsrsworkspace/crates/trogon-nats/Cargo.tomlrsworkspace/crates/trogon-nats/src/connect.rsrsworkspace/crates/trogon-nats/src/jetstream/object_store.rsrsworkspace/crates/trogon-nats/src/lease/lease_config_error.rsrsworkspace/crates/trogon-nats/src/lease/mod.rsrsworkspace/crates/trogon-nats/src/lease/renew_interval.rsrsworkspace/crates/trogon-nats/src/lease/ttl.rsrsworkspace/crates/trogon-nats/src/messaging.rsrsworkspace/crates/trogon-nats/src/mocks.rsrsworkspace/crates/trogon-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-scheduler/Cargo.tomlrsworkspace/crates/trogon-scheduler/src/commands/create_schedule.rsrsworkspace/crates/trogon-scheduler/src/commands/domain/message.rsrsworkspace/crates/trogon-scheduler/src/commands/domain/schedule.rsrsworkspace/crates/trogon-scheduler/src/commands/domain/schedule_id.rsrsworkspace/crates/trogon-scheduler/src/commands/pause_schedule.rsrsworkspace/crates/trogon-scheduler/src/commands/remove_schedule.rsrsworkspace/crates/trogon-scheduler/src/commands/resume_schedule.rsrsworkspace/crates/trogon-scheduler/src/commands/state.rsrsworkspace/crates/trogon-std/Cargo.tomlrsworkspace/crates/trogon-std/src/duration.rsrsworkspace/crates/trogon-std/src/secret_string.rsrsworkspace/crates/trogon-telemetry/Cargo.tomlrsworkspace/crates/trogon-telemetry/src/lib.rsrsworkspace/crates/trogon-telemetry/src/log.rsrsworkspace/crates/trogon-telemetry/src/metric.rsrsworkspace/crates/trogon-telemetry/src/trace.rsrsworkspace/crates/trogonai-proto/Cargo.tomlrsworkspace/crates/trogonai-proto/src/convert.rsrsworkspace/crates/trogonai-proto/src/scheduler/schedules/codec.rs
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
mariorha
added a commit
that referenced
this pull request
Jun 9, 2026
Brings in #207 (ADR taxonomy docs), #208 (derive error implementations via thiserror), #210 (testcontainers ADR). Conflict resolutions (converge to main's thiserror direction, keep platform functionality): - connect.rs: thiserror derive + keep AuthorizationViolation variant and platform's richer connect logic; MAX_RECONNECT_DELAY from crate::constants - lease_config_error.rs: thiserror derive, keep platform tests - verification_token.rs: thiserror derive; keep #[allow(dead_code)] on latest (lib/bin split makes it dead in the lib crate) - provision.rs: typed ProvisionError { source: Box<dyn Error> }, keep platform's create-and-update behavior Collateral fixes: - trogon-wasm-runtime: anyhow pin =1.0.98 -> workspace (=1.0.102) - acp-nats config.rs tests: AcpPrefixError is now an enum, match the variant Verified: cargo build --workspace, all workspace tests compile, affected crate tests pass, cargo clippy --workspace clean.
jramirezhdez02
added a commit
that referenced
this pull request
Jun 19, 2026
PR #208's derive refactor re-added #[cfg(not(coverage))] to the ProvisionObjectStoreError enum, but NatsObjectStore::provision (only coverage(off), so still compiled under coverage) returns it — so the coverage build failed with E0425/E0433 'cannot find type'. Type defs aren't instrumented, so the enum needs no cfg gate; remove it, restoring the state established in d6fdd10. Fixes the 'Run tests with coverage' step.
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
Brings in #207 (ADR taxonomy docs), #208 (derive error implementations via thiserror), #210 (testcontainers ADR). Conflict resolutions (converge to main's thiserror direction, keep platform functionality): - connect.rs: thiserror derive + keep AuthorizationViolation variant and platform's richer connect logic; MAX_RECONNECT_DELAY from crate::constants - lease_config_error.rs: thiserror derive, keep platform tests - verification_token.rs: thiserror derive; keep #[allow(dead_code)] on latest (lib/bin split makes it dead in the lib crate) - provision.rs: typed ProvisionError { source: Box<dyn Error> }, keep platform's create-and-update behavior Collateral fixes: - trogon-wasm-runtime: anyhow pin =1.0.98 -> workspace (=1.0.102) - acp-nats config.rs tests: AcpPrefixError is now an enum, match the variant Verified: cargo build --workspace, all workspace tests compile, affected crate tests pass, cargo clippy --workspace clean. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
added a commit
that referenced
this pull request
Jun 22, 2026
PR #208's derive refactor re-added #[cfg(not(coverage))] to the ProvisionObjectStoreError enum, but NatsObjectStore::provision (only coverage(off), so still compiled under coverage) returns it — so the coverage build failed with E0425/E0433 'cannot find type'. Type defs aren't instrumented, so the enum needs no cfg gate; remove it, restoring the state established in d6fdd10. Fixes the 'Run tests with coverage' step. 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.