refactor: unify token types under NatsToken in trogon-nats#44
Conversation
PR SummaryMedium Risk Overview Introduces Written by Cursor Bugbot for commit ceef35e. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughCentralized NATS subject-token validation into a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rsworkspace/crates/acp-nats/src/lib.rs (1)
24-24:⚠️ Potential issue | 🟡 MinorMissing
SessionIdErrorexport violates coding guidelines.The
acp_prefixmodule correctly exports bothAcpPrefixandAcpPrefixError, butsession_idonly exportsAcpSessionId. As per coding guidelines: "Export value objects inlib.rsaspub use {module}::{Type, TypeError}".Proposed fix
-pub use session_id::AcpSessionId; +pub use session_id::{AcpSessionId, SessionIdError};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/lib.rs` at line 24, The module export is missing the error type for session_id; update lib.rs to re-export the paired error type along with AcpSessionId by adding a pub use for the session_id error (e.g., add "pub use session_id::SessionIdError;") so both AcpSessionId and its corresponding SessionIdError are exported from the crate.
🧹 Nitpick comments (1)
rsworkspace/crates/trogon-nats/src/subject_token_violation.rs (1)
2-2: Consider addingEqderive for consistency.The enum has
PartialEqbut notEq. Since all variants are trivially equal to themselves (no floating-point or NaN-like semantics),Eqshould be derivable and would enable use in hash sets or as map keys.Proposed fix
-#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq, Eq)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/subject_token_violation.rs` at line 2, The derive on the enum in subject_token_violation.rs currently uses #[derive(Debug, Clone, PartialEq)]—add Eq to the derive list so it becomes #[derive(Debug, Clone, PartialEq, Eq)] to enable full equality semantics; update the derive attribute adjacent to the enum declaration (the same #[derive(...)] attribute in this file) to include Eq.
🤖 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/nats_token.rs`:
- Around line 73-86: The REQUIRE_ASCII branch currently rejects '.'
unconditionally; update the loop in nats_token.rs that checks P::REQUIRE_ASCII
so it consults P::ALLOW_DOTS (i.e., only reject '.' when P::ALLOW_DOTS is
false), while still rejecting '*' , '>' and whitespace and returning the same
SubjectTokenViolation variants; when P::ALLOW_DOTS is true, allow ASCII dots but
add the same consecutive/boundary dot validation used elsewhere (the dot
adjacency/boundary checks present around lines ~95-97) to ensure
leading/trailing or consecutive dots are handled and return the appropriate
SubjectTokenViolation when violated.
---
Outside diff comments:
In `@rsworkspace/crates/acp-nats/src/lib.rs`:
- Line 24: The module export is missing the error type for session_id; update
lib.rs to re-export the paired error type along with AcpSessionId by adding a
pub use for the session_id error (e.g., add "pub use
session_id::SessionIdError;") so both AcpSessionId and its corresponding
SessionIdError are exported from the crate.
---
Nitpick comments:
In `@rsworkspace/crates/trogon-nats/src/subject_token_violation.rs`:
- Line 2: The derive on the enum in subject_token_violation.rs currently uses
#[derive(Debug, Clone, PartialEq)]—add Eq to the derive list so it becomes
#[derive(Debug, Clone, PartialEq, Eq)] to enable full equality semantics; update
the derive attribute adjacent to the enum declaration (the same #[derive(...)]
attribute in this file) to include Eq.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fe3aae2-c9cc-412c-8e13-15fdb1ba2bac
📒 Files selected for processing (11)
rsworkspace/crates/acp-nats/src/acp_prefix.rsrsworkspace/crates/acp-nats/src/ext_method_name.rsrsworkspace/crates/acp-nats/src/lib.rsrsworkspace/crates/acp-nats/src/nats/mod.rsrsworkspace/crates/acp-nats/src/nats_token_policies.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/acp-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/lib.rsrsworkspace/crates/trogon-nats/src/nats_token.rsrsworkspace/crates/trogon-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/token.rs
💤 Files with no reviewable changes (1)
- rsworkspace/crates/acp-nats/src/nats/mod.rs
tsitsiridakisiosif
force-pushed
the
refactor-single-token-and-multi-token-under-nats-token
branch
from
3616890 to
7011e46
Compare
|
The direction is right — deduplicating the validation and moving Requested changesReplace the generic
This removes:
Both structs share the existing private |
tsitsiridakisiosif
force-pushed
the
refactor-single-token-and-multi-token-under-nats-token
branch
from
7011e46 to
d25f9b0
Compare
tsitsiridakisiosif
force-pushed
the
refactor-single-token-and-multi-token-under-nats-token
branch
from
d25f9b0 to
b952bac
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rsworkspace/crates/acp-nats/src/acp_prefix.rs (1)
38-40: UseAsRef<str>inAcpPrefix::newto avoid unnecessary allocation.
impl Into<String>forces allocation for borrowed inputs;impl AsRef<str>aligns withDottedNatsToken::new()which already expectsAsRef<str>, eliminating a redundant conversion layer.♻️ Proposed refactor
- pub fn new(s: impl Into<String>) -> Result<Self, AcpPrefixError> { - let s = s.into(); - DottedNatsToken::new(s).map(Self).map_err(AcpPrefixError) + pub fn new(s: impl AsRef<str>) -> Result<Self, AcpPrefixError> { + DottedNatsToken::new(s.as_ref()).map(Self).map_err(AcpPrefixError) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/acp_prefix.rs` around lines 38 - 40, Change AcpPrefix::new to accept impl AsRef<str> instead of impl Into<String> to avoid allocating for borrowed inputs; call DottedNatsToken::new(s.as_ref()) and propagate errors as before (map(Self).map_err(AcpPrefixError)). Update the parameter name if needed and remove the s.into() allocation so the function forwards the borrowed &str directly to DottedNatsToken::new while preserving error mapping to AcpPrefixError.
🤖 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-nats/src/acp_prefix.rs`:
- Around line 38-40: Change AcpPrefix::new to accept impl AsRef<str> instead of
impl Into<String> to avoid allocating for borrowed inputs; call
DottedNatsToken::new(s.as_ref()) and propagate errors as before
(map(Self).map_err(AcpPrefixError)). Update the parameter name if needed and
remove the s.into() allocation so the function forwards the borrowed &str
directly to DottedNatsToken::new while preserving error mapping to
AcpPrefixError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59990479-0d34-4602-982b-c2926fd5bb8e
📒 Files selected for processing (10)
rsworkspace/crates/acp-nats/src/acp_prefix.rsrsworkspace/crates/acp-nats/src/constants.rsrsworkspace/crates/acp-nats/src/ext_method_name.rsrsworkspace/crates/acp-nats/src/nats/mod.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/acp-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/lib.rsrsworkspace/crates/trogon-nats/src/nats_token.rsrsworkspace/crates/trogon-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/token.rs
💤 Files with no reviewable changes (2)
- rsworkspace/crates/acp-nats/src/nats/mod.rs
- rsworkspace/crates/acp-nats/src/constants.rs
✅ Files skipped from review due to trivial changes (2)
- rsworkspace/crates/trogon-nats/src/subject_token_violation.rs
- rsworkspace/crates/trogon-nats/src/token.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rsworkspace/crates/acp-nats/src/subject_token_violation.rs
- rsworkspace/crates/trogon-nats/src/lib.rs
- rsworkspace/crates/acp-nats/src/session_id.rs
- rsworkspace/crates/acp-nats/src/ext_method_name.rs
Signed-off-by: itsitsiridakis <iosif.tsitsiridakis@fanatics.live>
Signed-off-by: itsitsiridakis <iosif.tsitsiridakis@fanatics.live>
yordis
force-pushed
the
refactor-single-token-and-multi-token-under-nats-token
branch
from
b952bac to
243ba85
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rsworkspace/crates/trogon-nats/src/nats_token.rs (3)
207-214: Add one multibyte boundary test for the 128-byte rule.The current dotted-token length coverage only uses ASCII input, so it does not actually lock in the advertised byte-based limit. A small
é.repeat(64) /é.repeat(65) case would catch an accidental switch fromlen()tochars().count()later.Suggested test
#[test] fn dotted_accepts_non_ascii() { assert!(DottedNatsToken::new("préfixe").is_ok()); } + + #[test] + fn dotted_length_is_measured_in_bytes() { + let max = "é".repeat(64); // 128 bytes + let too_long = "é".repeat(65); // 130 bytes + + assert!(DottedNatsToken::new(&max).is_ok()); + assert_eq!( + DottedNatsToken::new(&too_long), + Err(SubjectTokenViolation::TooLong(130)) + ); + }Also applies to: 241-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/nats_token.rs` around lines 207 - 214, Add a test that verifies the 128-byte limit using multibyte characters so the implementation is measured in bytes not chars: create a new test (or extend the existing dotted_too_long and the similar test around lines 241-244) that uses a multibyte character like "é" repeated 64 (which is 128 bytes) and 65 (130 bytes) and assert DottedNatsToken::new(&multibyte64) is Ok and DottedNatsToken::new(&multibyte65) returns Err(SubjectTokenViolation::TooLong(...)); reference DottedNatsToken::new and SubjectTokenViolation::TooLong to locate where to add the checks.
33-40: Still duplicates shared validation.
DottedNatsToken::newalready gets the*/>/ whitespace rule fromrsworkspace/crates/trogon-nats/src/token.rs, butNatsToken::newre-implements that branch inline. That reintroduces drift between the two constructors. I’d pull the shared wildcard/whitespace predicate intotoken.rsand keep only the ASCII / dot-specific checks here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/nats_token.rs` around lines 33 - 40, Extract the shared wildcard/whitespace predicate used by DottedNatsToken::new into token.rs (e.g., a small function like is_wildcard_or_whitespace(ch: char) -> bool or is_invalid_wildcard_or_whitespace) and have both DottedNatsToken::new and NatsToken::new call that predicate instead of duplicating the logic; then simplify NatsToken::new to only perform the ASCII/dot checks and the MAX_LENGTH check (keeping SubjectTokenViolation::TooLong and InvalidCharacter for '.'/non-ascii) while delegating the '*'/'>'/whitespace checks to the new function in token.rs so the validation logic stays centralized and consistent.
73-79: MoveDottedNatsTokeninto its own file.This file now contains two public value objects. The repository convention expects each value object to live in a file named after the type, so
DottedNatsTokenshould live indotted_nats_token.rsand be re-exported from the crate root.As per coding guidelines
"Every value object lives in its own file named after the type (e.g., acp_prefix.rs, ext_method_name.rs, session_id.rs), never inlined into config, aggregate, or service files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/nats_token.rs` around lines 73 - 79, Move the public value object DottedNatsToken out of its current file into its own module file named dotted_nats_token.rs: copy the pub struct DottedNatsToken(Arc<str>) plus its derives and any impls, ensure it imports Arc and other dependencies, and keep visibility as pub; then add a module declaration and a pub re-export from the crate root (e.g., add pub mod dotted_nats_token; and pub use dotted_nats_token::DottedNatsToken) so external code imports remain unchanged; update any local uses/imports in the original file to reference the re-export or the new module as needed and run tests to confirm compile.
🤖 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/trogon-nats/src/nats_token.rs`:
- Around line 207-214: Add a test that verifies the 128-byte limit using
multibyte characters so the implementation is measured in bytes not chars:
create a new test (or extend the existing dotted_too_long and the similar test
around lines 241-244) that uses a multibyte character like "é" repeated 64
(which is 128 bytes) and 65 (130 bytes) and assert
DottedNatsToken::new(&multibyte64) is Ok and DottedNatsToken::new(&multibyte65)
returns Err(SubjectTokenViolation::TooLong(...)); reference DottedNatsToken::new
and SubjectTokenViolation::TooLong to locate where to add the checks.
- Around line 33-40: Extract the shared wildcard/whitespace predicate used by
DottedNatsToken::new into token.rs (e.g., a small function like
is_wildcard_or_whitespace(ch: char) -> bool or
is_invalid_wildcard_or_whitespace) and have both DottedNatsToken::new and
NatsToken::new call that predicate instead of duplicating the logic; then
simplify NatsToken::new to only perform the ASCII/dot checks and the MAX_LENGTH
check (keeping SubjectTokenViolation::TooLong and InvalidCharacter for
'.'/non-ascii) while delegating the '*'/'>'/whitespace checks to the new
function in token.rs so the validation logic stays centralized and consistent.
- Around line 73-79: Move the public value object DottedNatsToken out of its
current file into its own module file named dotted_nats_token.rs: copy the pub
struct DottedNatsToken(Arc<str>) plus its derives and any impls, ensure it
imports Arc and other dependencies, and keep visibility as pub; then add a
module declaration and a pub re-export from the crate root (e.g., add pub mod
dotted_nats_token; and pub use dotted_nats_token::DottedNatsToken) so external
code imports remain unchanged; update any local uses/imports in the original
file to reference the re-export or the new module as needed and run tests to
confirm compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 261cf29e-a463-4769-b4ab-f923319f33f8
📒 Files selected for processing (10)
rsworkspace/crates/acp-nats/src/acp_prefix.rsrsworkspace/crates/acp-nats/src/constants.rsrsworkspace/crates/acp-nats/src/ext_method_name.rsrsworkspace/crates/acp-nats/src/nats/mod.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/acp-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/lib.rsrsworkspace/crates/trogon-nats/src/nats_token.rsrsworkspace/crates/trogon-nats/src/subject_token_violation.rsrsworkspace/crates/trogon-nats/src/token.rs
💤 Files with no reviewable changes (2)
- rsworkspace/crates/acp-nats/src/constants.rs
- rsworkspace/crates/acp-nats/src/nats/mod.rs
✅ Files skipped from review due to trivial changes (3)
- rsworkspace/crates/trogon-nats/src/subject_token_violation.rs
- rsworkspace/crates/trogon-nats/src/token.rs
- rsworkspace/crates/acp-nats/src/acp_prefix.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rsworkspace/crates/acp-nats/src/subject_token_violation.rs
- rsworkspace/crates/trogon-nats/src/lib.rs
- rsworkspace/crates/acp-nats/src/ext_method_name.rs
…o constants Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/trogon-nats/src/nats_token.rs (2)
205-213: Consider adding a test for multi-byte UTF-8 length behavior.The
dotted_too_longtest uses ASCII strings where chars == bytes. SinceDottedNatsTokenaccepts UTF-8 and enforces a byte-length limit, consider adding a test that verifies behavior with multi-byte characters (e.g., a string with 100 characters but 150 bytes should be rejected).Example test
#[test] fn dotted_too_long_multibyte() { // 'é' is 2 bytes in UTF-8; 65 of them = 130 bytes > 128 let multibyte = "é".repeat(65); assert!(DottedNatsToken::new(&multibyte).is_err()); // 64 'é' = 128 bytes, should be ok assert!(DottedNatsToken::new("é".repeat(64)).is_ok()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/nats_token.rs` around lines 205 - 213, Add a unit test that verifies byte-length enforcement with multi-byte UTF-8 characters: call DottedNatsToken::new with a string of multi-byte chars (e.g., "é".repeat(65)) that exceeds the 128-byte limit and assert it errors, and also assert that a string at the byte limit (e.g., "é".repeat(64)) is accepted; place this alongside the existing dotted_too_long test so DottedNatsToken::new and the byte-length behavior (SubjectTokenViolation::TooLong) are covered for UTF-8 multibyte inputs.
32-36: Inconsistent unit (chars vs bytes) forTooLongerror across types.
NatsToken::new(line 36) reportsTooLong(char_count)counting characters, whileDottedNatsToken::new(line 94) reportsTooLong(s.len())counting bytes. Although the doc comments reflect this difference ("max 128 chars" vs "max 128 bytes"), the sharedSubjectTokenViolation::TooLong(usize)variant provides no indication of the unit.For pure ASCII in
NatsToken, chars == bytes so it's effectively consistent. However, this dual interpretation ofMAX_NATS_TOKEN_LENGTHcould confuse callers or lead to inconsistent error messages.Consider documenting in
SubjectTokenViolationthatTooLongrepresents "the exceeded measurement" or—if stricter consistency is desired—using byte length for both types.Also applies to: 93-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/trogon-nats/src/nats_token.rs` around lines 32 - 36, SubjectTokenViolation::TooLong currently carries a bare usize but is populated by NatsToken::new using char count and by DottedNatsToken::new using byte length, which is ambiguous; either (A) make the semantics explicit by updating SubjectTokenViolation (e.g., document TooLong as "exceeded measurement" and add enum docs/comments stating whether the usize is bytes or chars) and update NatsToken::new and DottedNatsToken::new errors to clearly state the unit in their error construction/logging, or (B) standardize to a single unit (prefer bytes) by changing NatsToken::new to measure s.len() (or change DottedNatsToken to use s.chars().count()) and update MAX_NATS_TOKEN_LENGTH docstring accordingly; locate and modify the enum SubjectTokenViolation::TooLong, the constructors NatsToken::new and DottedNatsToken::new, and the constant MAX_NATS_TOKEN_LENGTH to keep units consistent.
🤖 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/trogon-nats/src/nats_token.rs`:
- Around line 205-213: Add a unit test that verifies byte-length enforcement
with multi-byte UTF-8 characters: call DottedNatsToken::new with a string of
multi-byte chars (e.g., "é".repeat(65)) that exceeds the 128-byte limit and
assert it errors, and also assert that a string at the byte limit (e.g.,
"é".repeat(64)) is accepted; place this alongside the existing dotted_too_long
test so DottedNatsToken::new and the byte-length behavior
(SubjectTokenViolation::TooLong) are covered for UTF-8 multibyte inputs.
- Around line 32-36: SubjectTokenViolation::TooLong currently carries a bare
usize but is populated by NatsToken::new using char count and by
DottedNatsToken::new using byte length, which is ambiguous; either (A) make the
semantics explicit by updating SubjectTokenViolation (e.g., document TooLong as
"exceeded measurement" and add enum docs/comments stating whether the usize is
bytes or chars) and update NatsToken::new and DottedNatsToken::new errors to
clearly state the unit in their error construction/logging, or (B) standardize
to a single unit (prefer bytes) by changing NatsToken::new to measure s.len()
(or change DottedNatsToken to use s.chars().count()) and update
MAX_NATS_TOKEN_LENGTH docstring accordingly; locate and modify the enum
SubjectTokenViolation::TooLong, the constructors NatsToken::new and
DottedNatsToken::new, and the constant MAX_NATS_TOKEN_LENGTH to keep units
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 363dd4b3-903d-4fa5-a5d6-595b342dca54
📒 Files selected for processing (6)
rsworkspace/crates/acp-nats/src/acp_prefix.rsrsworkspace/crates/acp-nats/src/ext_method_name.rsrsworkspace/crates/acp-nats/src/lib.rsrsworkspace/crates/acp-nats/src/session_id.rsrsworkspace/crates/trogon-nats/src/constants.rsrsworkspace/crates/trogon-nats/src/nats_token.rs
💤 Files with no reviewable changes (1)
- rsworkspace/crates/acp-nats/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
- rsworkspace/crates/trogon-nats/src/constants.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rsworkspace/crates/acp-nats/src/ext_method_name.rs
- rsworkspace/crates/acp-nats/src/session_id.rs
2 participants
No description provided.