Consolidate GTS ID logic into gts-id crate and add fallible type-safe constructors#101
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe ChangesGTS ID Crate Extraction and System Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant gts_macros as gts-macros (proc-macro)
participant GtsId
participant parse_gts_id
participant GtsIdSegment
rect rgba(70, 130, 180, 0.5)
note over Client,GtsIdSegment: Compile-time validation (gts_instance! / struct_to_gts_schema)
Client->>gts_macros: #[gts_instance!] or #[struct_to_gts_schema]
gts_macros->>GtsId: try_new(id_literal)
GtsId->>parse_gts_id: parse_gts_id(id, allow_wildcards=false)
parse_gts_id->>GtsIdSegment: parse(num, segment, false)
GtsIdSegment-->>parse_gts_id: Ok(GtsIdSegment) or Err(segment msg)
parse_gts_id-->>GtsId: Ok(Vec~GtsIdSegment~) or GtsIdError{segment: Some(...)}
GtsId-->>gts_macros: Ok(GtsId) or GtsIdError
gts_macros-->>Client: compile error with segment `#N` @ offset M
end
rect rgba(60, 179, 113, 0.5)
note over Client,GtsIdSegment: Runtime validation (serde deserialization)
Client->>GtsInstanceId: deserialize(str)
GtsInstanceId->>GtsId: try_new(str)
GtsId-->>GtsInstanceId: Result~GtsId, GtsIdError~
GtsInstanceId->>GtsInstanceId: is_type() == false check
GtsInstanceId-->>Client: Ok(GtsInstanceId) or serde error
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gts/src/gts.rs (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
serdeinput through the typed constructors.Line 89 and Line 293 now accept any string and wrap it directly, so invalid IDs or type/instance mismatches can be deserialized into
GtsInstanceId/GtsTypeIdwithout error. That breaks the new type-safe-constructor invariant right at the external input boundary.Proposed fix
impl<'de> serde::Deserialize<'de> for GtsInstanceId { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { let s = String::deserialize(deserializer)?; - Ok(GtsInstanceId(GtsEntityId::new(&s))) + GtsInstanceId::try_new(&s).map_err(serde::de::Error::custom) } } @@ impl<'de> serde::Deserialize<'de> for GtsTypeId { fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> where D: serde::Deserializer<'de>, { let s = String::deserialize(deserializer)?; - Ok(GtsTypeId(GtsEntityId::new(&s))) + GtsTypeId::try_new(&s).map_err(serde::de::Error::custom) } }Also applies to: 287-294
🤖 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 `@gts/src/gts.rs` around lines 83 - 90, The Deserialize impls currently accept any string and wrap it directly (in impl serde::Deserialize for GtsInstanceId and the analogous impl for GtsTypeId), bypassing the type-safe constructors; change each impl to parse/validate the deserialized string via the public typed constructor/validator (e.g., call GtsInstanceId::try_from / GtsInstanceId::from_str / GtsInstanceId::parse or the crate's equivalent validated constructor) and map any construction error into a serde error with serde::de::Error::custom so invalid IDs fail deserialization rather than being wrapped unvalidated. Ensure you apply the same pattern to the GtsTypeId deserializer as well.gts/src/entities.rs (1)
263-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the parent chain without adding a second
~separator.Each
GtsIdSegment.segmentalready carries its trailing~for type segments, sojoin("~")produces malformed parents once the chain is deeper than two segments. A schema likegts.a.v1~b.v1~c.v1~will derivegts.a.v1~~b.v1~, which breakstype_id-based parent resolution.Suggested fix
- let parent_segments: Vec<&str> = gts_id - .segments() - .iter() - .take(gts_id.segments().len() - 1) - .map(|seg| seg.segment.as_str()) - .collect(); - if !parent_segments.is_empty() { - // Join segments - they already have ~ at the end if they're types - // The full chain format is: gts.seg1~seg2~seg3~ - // For parent, we want: gts.seg1~ (if only one parent segment) - // or gts.seg1~seg2~ (if multiple parent segments) - let parent_id = format!("gts.{}", parent_segments.join("~")); + let parent_segments: Vec<&str> = gts_id + .segments() + .iter() + .take(gts_id.segments().len() - 1) + .map(|seg| seg.segment.as_str()) + .collect(); + if !parent_segments.is_empty() { + let parent_id = format!("gts.{}", parent_segments.concat()); // Ensure it ends with ~ (parent is always a schema) let parent_id = if parent_id.ends_with('~') { parent_id } else { format!("{parent_id}~")🤖 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 `@gts/src/entities.rs` around lines 263 - 285, The parent chain construction is adding extra "~" because each GtsIdSegment.segment already includes its trailing "~"; in the block with self.gts_id (and variables parent_segments and parent_id) stop using join("~") and instead concatenate the segment strings directly (e.g., join("") or fold) to avoid inserting another "~" between segments, then prefix with "gts." and ensure a single trailing '~' (check parent_id.ends_with('~') and append one only if missing).
🧹 Nitpick comments (1)
gts/src/ops.rs (1)
569-585: ⚡ Quick winAdd regression coverage for the new pattern parsing branches.
This path now depends on
GtsWildcard::newcontinuing to accept zero-*concrete patterns, and on the newcandidate.contains('*')branch behaving as intended. Please add one exact-pattern test and one wildcard-candidate test so future parser changes do not silently altermatch_id_patternsemantics.🤖 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 `@gts/src/ops.rs` around lines 569 - 585, Add two unit tests for match_id_pattern: one that passes a concrete pattern and concrete candidate (no '*' in either) to verify GtsWildcard::new accepts zero-'*' patterns and match_id_pattern returns a match for exact ids, and one that passes a wildcard candidate (candidate contains '*') with a wildcard pattern to verify the candidate.contains('*') branch uses GtsWildcard::new for the candidate and matching behaves as expected; locate match_id_pattern and exercise it directly in tests (e.g., test_exact_pattern_matches and test_wildcard_candidate_matches) asserting the returned GtsIdMatchResult indicates success and the expected matched segments.
🤖 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 `@gts-id/src/gts_id.rs`:
- Around line 99-106: is_valid currently checks GTS_PREFIX on the raw input so
strings with leading/trailing whitespace (which GtsID::new trims) are rejected;
fix is_valid by applying the same trimming as new() (trim the input string
first, e.g., to a trimmed &str) and then perform the prefix check and call
Self::new on the trimmed value so both methods accept the same inputs (refer to
is_valid, Self::new, and GTS_PREFIX).
In `@gts-id/src/gts_wildcard.rs`:
- Around line 126-178: The overlaps() and is_subset_of() helpers currently use
prefix_str and raw string prefix checks which miss segment-level rules (e.g.
absent ver_minor matches any minor); change both to parse the patterns into
segments using the same parser used by matches_segments() and perform
segment-wise comparisons: for overlaps(), determine whether there exists a
concrete ID that would satisfy both segment constraints (treat absent ver_minor
as a wildcard that matches any numeric minor), and for is_subset_of(), ensure
every segment in self is at least as specific as other (i.e. any absent/
wildcard in other accepts the concrete values in self, and a present concrete
minor in self must match other or be accepted by other’s wildcard/absent minor).
Replace uses of prefix_str in overlaps/is_subset_of with this segment-aware
logic and reuse/mirror matches_segments() semantics to keep behavior consistent.
- Around line 47-123: The function matches_segments currently returns true for
exact patterns even when the candidate is longer; change the final unconditional
"true" to require equal segment counts when no wildcard was hit by replacing the
final return with "pattern_segs.len() == candidate.len()". Keep existing early
return behavior inside the p_seg.is_wildcard branch (which should still accept
any remaining segments), and ensure this check is applied in matches_segments
(use the pattern_segs and candidate variables to locate the code).
In `@gts/src/store_test.rs`:
- Around line 2210-2214: The mock reader's read_by_id currently matches only on
e.g. gts_id but GtsStore::populate_from_reader() keys by effective_id(), so
update the read_by_id implementation to compare the provided entity_id against
the entity's effective_id() (call the GtsEntity method effective_id()) instead
of gts_id; keep the rest of the iterator chain and cloning behavior the same so
tests resolve entities addressable only via their effective ID.
---
Outside diff comments:
In `@gts/src/entities.rs`:
- Around line 263-285: The parent chain construction is adding extra "~" because
each GtsIdSegment.segment already includes its trailing "~"; in the block with
self.gts_id (and variables parent_segments and parent_id) stop using join("~")
and instead concatenate the segment strings directly (e.g., join("") or fold) to
avoid inserting another "~" between segments, then prefix with "gts." and ensure
a single trailing '~' (check parent_id.ends_with('~') and append one only if
missing).
In `@gts/src/gts.rs`:
- Around line 83-90: The Deserialize impls currently accept any string and wrap
it directly (in impl serde::Deserialize for GtsInstanceId and the analogous impl
for GtsTypeId), bypassing the type-safe constructors; change each impl to
parse/validate the deserialized string via the public typed
constructor/validator (e.g., call GtsInstanceId::try_from /
GtsInstanceId::from_str / GtsInstanceId::parse or the crate's equivalent
validated constructor) and map any construction error into a serde error with
serde::de::Error::custom so invalid IDs fail deserialization rather than being
wrapped unvalidated. Ensure you apply the same pattern to the GtsTypeId
deserializer as well.
---
Nitpick comments:
In `@gts/src/ops.rs`:
- Around line 569-585: Add two unit tests for match_id_pattern: one that passes
a concrete pattern and concrete candidate (no '*' in either) to verify
GtsWildcard::new accepts zero-'*' patterns and match_id_pattern returns a match
for exact ids, and one that passes a wildcard candidate (candidate contains '*')
with a wildcard pattern to verify the candidate.contains('*') branch uses
GtsWildcard::new for the candidate and matching behaves as expected; locate
match_id_pattern and exercise it directly in tests (e.g.,
test_exact_pattern_matches and test_wildcard_candidate_matches) asserting the
returned GtsIdMatchResult indicates success and the expected matched segments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 169b359d-ec92-4065-b242-93753a39c308
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
gts-id/Cargo.tomlgts-id/src/error.rsgts-id/src/gts_id.rsgts-id/src/gts_id_segment.rsgts-id/src/gts_wildcard.rsgts-id/src/lib.rsgts-id/src/parse.rsgts-macros/src/instance.rsgts-macros/src/lib.rsgts-macros/tests/compile_fail/instance_id_invalid_format.rsgts-macros/tests/compile_fail/instance_id_invalid_format.stderrgts-macros/tests/compile_fail/instance_id_wildcard_in_instance.rsgts-macros/tests/compile_fail/instance_id_wildcard_in_instance.stderrgts-macros/tests/compile_fail/instance_raw_no_tilde.stderrgts-macros/tests/compile_fail/invalid_gts_id_missing_prefix.stderrgts-macros/tests/compile_fail/invalid_gts_id_too_many_tokens.stderrgts-macros/tests/compile_fail/version_missing_both.stderrgts-macros/tests/compile_fail/version_missing_in_schema.stderrgts-macros/tests/integration_tests.rsgts-validator/src/validator.rsgts/Cargo.tomlgts/src/entities.rsgts/src/gts.rsgts/src/lib.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/store.rsgts/src/store_test.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
57-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the architecture section for the extracted
gts-idcrate.This section still says the workspace is just
gtsplusgts-cli, and it still describesgts.rsas the home of ID parsing and wildcard matching. After this extraction, that crate boundary description is stale for readers following the new ID API.🤖 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 `@README.md` around lines 57 - 78, Update the architecture section to reflect the extracted gts-id crate: replace references that say ID parsing/wildcard matching live in gts.rs or the gts crate with a note that those responsibilities are now provided by the new gts-id crate (mention gts-id crate name explicitly), move "GTS ID parsing, validation, wildcard matching" from the gts (Library Crate) entry to a new bullet for gts-id, and adjust the workspace list to include three crates: gts, gts-id, and gts-cli so readers can find the new ID API.gts-id/src/parse.rs (1)
125-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow terminal
*~wildcard segments.Line 132 only accepts wildcard IDs ending in
.*or~*, so patterns likegts.*~orgts.x.core.events.event.v1.*~are rejected beforeGtsIdSegment::parsecan setis_type = true. That conflicts with the segment parser and withgts-id/src/gts_id_wildcard.rs, which already usesis_type()to distinguish wildcard type matching from instance matching. Widen the suffix gate to accept terminal*~and add a regression test for it.Proposed fix
- if raw.contains('*') && !raw.ends_with(".*") && !raw.ends_with("~*") { + if raw.contains('*') + && !raw.ends_with(".*") + && !raw.ends_with("~*") + && !raw.ends_with("*~") + { return Err(GtsIdError::new( id, "The wildcard '*' token is allowed only at the end of the pattern", )); }🤖 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 `@gts-id/src/parse.rs` around lines 125 - 138, The current gate in parse.rs (in the allow_wildcards block checking raw.ends_with(".*") || raw.ends_with("~*")) wrongly rejects terminal "*~" suffixes; update the condition to also accept raw.ends_with("*~") so terminal "*~" wildcards like "gts.*~" or "gts.x.core.events.event.v1.*~" pass through to GtsIdSegment::parse (which sets is_type()) and remain consistent with gts-id/src/gts_id_wildcard.rs and its is_type() behavior; after changing the suffix check, add a regression test that parses a wildcard ID ending in "*~" and asserts it is accepted and treated as a type wildcard.gts/src/entities.rs (1)
268-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the parent
type_idwithout injecting extra~separators.Line 279 joins
GtsIdSegment::raw()with"~", butraw()already carries the trailing~for type segments. That makes 3+ segment schema chains derive invalid parents likegts.a~~b~, so nested schema lookup/cast logic ends up keyed by the wrongtype_id.Suggested fix
let parent_segments: Vec<&str> = gts_id .segments() .iter() .take(gts_id.segments().len() - 1) .map(gts_id::GtsIdSegment::raw) .collect(); if !parent_segments.is_empty() { - let parent_id = format!("gts.{}", parent_segments.join("~")); - // Ensure it ends with ~ (parent is always a schema) - let parent_id = if parent_id.ends_with('~') { - parent_id - } else { - format!("{parent_id}~") - }; + let parent_id = format!("gts.{}", parent_segments.join("")); // Use parent from chain as type_id when current value isn't // already a GTS Type Identifier (e.g. $schema held a JSON // Schema dialect URL, or no $schema was present).Cross-file evidence:
gts-id/src/gts_id.rsreconstructs type IDs by concatenatingGtsIdSegment::raw()values withjoin("").🤖 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 `@gts/src/entities.rs` around lines 268 - 285, The parent type_id is being built by joining GtsIdSegment::raw() with "~", but raw() already includes trailing '~', producing duplicate separators (e.g., "gts.a~~b~"); update the construction in the block using parent_segments/GtsIdSegment::raw() so you concatenate the raw pieces without inserting additional "~" (e.g., join with "" or strip the extra "~" before joining), then ensure the final parent_id string still ends with a single '~' (keep the existing ends_with check), so parent_id is always "gts.<segments>~" with no doubled tildes.
♻️ Duplicate comments (1)
gts/src/store_test.rs (1)
2210-2215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPast review concern remains unaddressed: mock should match on
effective_id().The refactor updated the accessor syntax to
gts_id::GtsId::id, but the underlying issue flagged in the previous review persists:MockGtsReader::read_by_idstill matches one.gts_idinstead ofe.effective_id(), causing the mock to diverge from howGtsStore::populate_from_reader()keys entities.🔧 Suggested fix
fn read_by_id(&self, entity_id: &str) -> Option<GtsEntity> { self.entities .iter() - .find(|e| e.gts_id.as_ref().map(gts_id::GtsId::id) == Some(entity_id)) + .find(|e| e.effective_id().as_deref() == Some(entity_id)) .cloned() }🤖 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 `@gts/src/store_test.rs` around lines 2210 - 2215, The mock's read_by_id implementation is matching on the raw gts_id field instead of using the entity's effective_id(), so update MockGtsReader::read_by_id to compare the passed entity_id against e.effective_id() (mirroring GtsStore::populate_from_reader) and return the cloned GtsEntity when they match; locate the read_by_id function in the test and replace the current e.gts_id-based comparison with a call to the GtsEntity::effective_id accessor (or equivalent) so the mock behavior matches production keying.
🤖 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 `@gts-id/src/gts_id_segment.rs`:
- Around line 44-52: The public enum GtsIdSegment exposes UuidTail(String)
allowing callers to forge invalid UUID tails; make the inner enum private
(rename or move the current GtsIdSegment to a private enum like
GtsIdSegmentInner) and provide a public opaque wrapper type (e.g., pub struct
GtsIdSegment) with controlled constructors such as
GtsIdSegment::from_uuid_tail(uuid::Uuid) and parsing constructors that validate
and only produce a UuidTail when the string is a valid UUID; make all enum
variants and their fields private, update the accessor uuid() to rely on the
validated internal representation and return a guaranteed Some(uuid::Uuid) (or
Option only when appropriate), and adjust parsing/creation call sites to use the
new public constructors and accessors so downstream crates cannot construct or
relabel variants directly.
In `@gts/src/ops.rs`:
- Around line 45-50: The serialization currently always sets
GtsIdSegmentInfo.ver_major to Some(seg.ver_major()), which turns the wildcard
sentinel 0 into an explicit 0; change the assignment in ops.rs so ver_major is
None when seg.ver_major() == 0 and Some(value) otherwise (i.e., preserve the "no
major version" sentinel), leaving ver_minor and other fields unchanged; locate
the code around the construction of GtsIdSegmentInfo (using symbols seg and
GtsIdSegmentInfo.ver_major) and replace the unconditional Some(...) with a
conditional mapping that returns None for 0.
In `@gts/src/store.rs`:
- Around line 604-606: The current check only calls GtsId::is_valid on the
stripped ref_uri which allows non-schema targets like instance IDs; change the
validation to reject non-schema IDs by either attempting to construct/parse a
schema/type ID (using the same constructor used by GtsRetriever::new) or at
minimum require the stripped gts_id to be a type/schema ID (e.g., ensure it ends
with the type marker `~`) before accepting the $ref; update the branch that
inspects ref_uri (and the GTS_URI_PREFIX check) to perform that stronger
validation so only schema/type targets accepted, matching entity.is_schema
registration in GtsRetriever::new.
In `@README.md`:
- Around line 567-570: The README examples use the removed field
GtsId::gts_id_segments and the old wildcard check GtsIdWildcard::matches(&id);
update those snippets to call the current public API instead: drop any direct
access to gts_id_segments and use GtsId's public accessor(s) or methods for
inspecting segments, and replace uses of GtsIdWildcard::matches(&id) with the
documented GtsId::wildcard_match call; apply the same fixes to both affected
examples (the snippet around the former gts_id_segments usage and the one at the
later location noted in the comment).
---
Outside diff comments:
In `@gts-id/src/parse.rs`:
- Around line 125-138: The current gate in parse.rs (in the allow_wildcards
block checking raw.ends_with(".*") || raw.ends_with("~*")) wrongly rejects
terminal "*~" suffixes; update the condition to also accept raw.ends_with("*~")
so terminal "*~" wildcards like "gts.*~" or "gts.x.core.events.event.v1.*~" pass
through to GtsIdSegment::parse (which sets is_type()) and remain consistent with
gts-id/src/gts_id_wildcard.rs and its is_type() behavior; after changing the
suffix check, add a regression test that parses a wildcard ID ending in "*~" and
asserts it is accepted and treated as a type wildcard.
In `@gts/src/entities.rs`:
- Around line 268-285: The parent type_id is being built by joining
GtsIdSegment::raw() with "~", but raw() already includes trailing '~', producing
duplicate separators (e.g., "gts.a~~b~"); update the construction in the block
using parent_segments/GtsIdSegment::raw() so you concatenate the raw pieces
without inserting additional "~" (e.g., join with "" or strip the extra "~"
before joining), then ensure the final parent_id string still ends with a single
'~' (keep the existing ends_with check), so parent_id is always
"gts.<segments>~" with no doubled tildes.
In `@README.md`:
- Around line 57-78: Update the architecture section to reflect the extracted
gts-id crate: replace references that say ID parsing/wildcard matching live in
gts.rs or the gts crate with a note that those responsibilities are now provided
by the new gts-id crate (mention gts-id crate name explicitly), move "GTS ID
parsing, validation, wildcard matching" from the gts (Library Crate) entry to a
new bullet for gts-id, and adjust the workspace list to include three crates:
gts, gts-id, and gts-cli so readers can find the new ID API.
---
Duplicate comments:
In `@gts/src/store_test.rs`:
- Around line 2210-2215: The mock's read_by_id implementation is matching on the
raw gts_id field instead of using the entity's effective_id(), so update
MockGtsReader::read_by_id to compare the passed entity_id against
e.effective_id() (mirroring GtsStore::populate_from_reader) and return the
cloned GtsEntity when they match; locate the read_by_id function in the test and
replace the current e.gts_id-based comparison with a call to the
GtsEntity::effective_id accessor (or equivalent) so the mock behavior matches
production keying.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae460a03-4a8f-4c75-9a21-7d23a43bfca4
📒 Files selected for processing (23)
README.mdgts-id/src/error.rsgts-id/src/gts_id.rsgts-id/src/gts_id_segment.rsgts-id/src/gts_id_wildcard.rsgts-id/src/lib.rsgts-id/src/parse.rsgts-macros/src/instance.rsgts-macros/src/lib.rsgts-macros/tests/compile_fail/instance_id_invalid_format.rsgts-macros/tests/compile_fail/instance_id_wildcard_in_instance.rsgts-macros/tests/integration_tests.rsgts-validator/src/format/markdown.rsgts-validator/src/normalize.rsgts-validator/src/validator.rsgts/src/entities.rsgts/src/gts.rsgts/src/lib.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/store.rsgts/src/store_test.rsgts/src/x_gts_ref.rs
✅ Files skipped from review due to trivial changes (5)
- gts-validator/src/format/markdown.rs
- gts-macros/tests/compile_fail/instance_id_invalid_format.rs
- gts-validator/src/normalize.rs
- gts/src/x_gts_ref.rs
- gts-macros/tests/compile_fail/instance_id_wildcard_in_instance.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- gts-macros/src/instance.rs
- gts/src/schema_cast.rs
- gts-validator/src/validator.rs
- gts-id/src/error.rs
- gts/src/gts.rs
- gts-id/src/gts_id.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gts-id/src/gts_id_segment.rs`:
- Around line 127-130: The is_wildcard method currently moves the
GtsIdSegmentKind out of self by using matches!(self.0, ...); change the pattern
to borrow the enum instead (match on &self.0) so you don't move the contained
String—update the matches! invocation in pub fn is_wildcard(&self) -> bool to
use &self.0 and keep the same pattern GtsIdSegmentKind::Wildcard(_) to avoid
ownership 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97c54a1e-9d45-4bfd-b1c3-96dcdda7aa05
📒 Files selected for processing (6)
README.mdgts-id/src/gts_id.rsgts-id/src/gts_id_segment.rsgts/src/ops.rsgts/src/store.rsgts/src/store_test.rs
💤 Files with no reviewable changes (1)
- gts-id/src/gts_id.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- gts/src/ops.rs
aviator5
force-pushed
the
gts-id-extraction
branch
from
aa1c5d5 to
eee7b52
Compare
Artifizer
reviewed
Jun 12, 2026
| { | ||
| let s = String::deserialize(deserializer)?; | ||
| Ok(GtsInstanceId(GtsEntityId(s))) | ||
| Ok(GtsInstanceId(GtsEntityId::new(&s))) |
Contributor
There was a problem hiding this comment.
Serde deserialization still bypasses the new validated constructors. This accepts invalid IDs and also allows a type ID to deserialize as GtsInstanceId even though GtsInstanceId::try_new rejects it. Could we route this through the validated constructor instead, e.g. GtsInstanceId::try_new(&s).map_err(serde::de::Error::custom)? Same issue applies to GtsTypeId below, which should use GtsTypeId::try_new(&s).
Contributor
|
One remaining issue I noticed in |
Artifizer
reviewed
Jun 12, 2026
| // Delegate all parsing to the shared parser (single source of truth). | ||
| // `allow_wildcards = false`: a concrete identifier never contains '*'. | ||
| let segments = parse_gts_id(raw, false)?; | ||
|
|
Contributor
There was a problem hiding this comment.
This changes GtsId::new/GtsId::is_valid behavior compared with the current gts::GtsID: the old constructor delegated to validate_gts_id(raw, true), so strings like gts.x.core.* were considered valid by GtsID::is_valid, while this now rejects wildcards. That tightening may be the right model for a concrete GtsId, but it affects every migrated is_valid call site (for example ref discovery / pointer-resolved values that may contain wildcard refs). Could we call this out in the PR description and add a regression test for the intended behavior?
Contributor
Author
There was a problem hiding this comment.
Now there are two structs in gts-id crate:
GtsId- validated GTS identifier.GtsIdPattern- GTS identifier match pattern (exact or trailing-*wildcard).
Both structs have try_new() constructors and is_valid() helper-methods that could be used for different cases. So, for example, for ref discovery, you need to use GtsIdPattern, for validating concrete id in the macros - GtsId, etc.
I've prepared gts-id/README.md where shortly described all available functionality of the gts-id crate.
Artifizer
reviewed
Jun 12, 2026
| "The wildcard '*' token is allowed only once", | ||
| )); | ||
| } | ||
| if raw.contains('*') && !raw.ends_with(".*") && !raw.ends_with("~*") { |
Contributor
There was a problem hiding this comment.
There is still an inconsistency around *~: the pattern-level rule only accepts wildcard patterns ending in .* or ~*, so gts.a.b.*~ is rejected here, but GtsIdSegment::parse can parse *~ as a wildcard with is_type = true, and matches_segments has an is_type check in its wildcard arm. Since the PR is centralizing parsing semantics, could we either accept *~ at the pattern level or remove the unreachable wildcard-is_type handling from the segment parser/matcher?
Contributor
|
Versioning/rebase note: this branch is behind |
aviator5
force-pushed
the
gts-id-extraction
branch
3 times, most recently
from
53de854 to
6f2829f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@gts-id/src/gts_id.rs`:
- Around line 373-377: The test cases in test_gts_id_invalid_characters function
are incomplete GTS IDs that fail on segment count validation before testing the
invalid character handling. Modify each test input to be a complete,
structurally valid GTS ID with the proper number of segments, but keep only the
target token malformed with invalid characters. This ensures the assertions
actually exercise the invalid-character validation logic rather than passing on
a "too few segments" error. Apply the same fix pattern to all similar
malformed-token test cases in the test suite that currently use incomplete ID
inputs.
In `@gts-macros/tests/inheritance_tests.rs`:
- Around line 841-843: The test in inheritance_tests.rs uses weak assertions
with contains() and ends_with() to validate the generated instance ID, which
could pass even with unexpected intermediate text. Since the generated ID is
deterministic, replace the two separate assert! calls (the one checking for
contains("gts.x.core.events.topic.v1~") and the one checking for
ends_with(segment)) with a single assert_eq! that validates the exact expected
ID value constructed from the known prefix and the segment variable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 772062a6-8b03-4e8c-b121-cceb301029b2
📒 Files selected for processing (17)
README.mdgts-id/src/gts_id.rsgts-id/src/gts_id_segment.rsgts-id/src/gts_id_wildcard.rsgts-id/src/parse.rsgts-macros/tests/inheritance_tests.rsgts-macros/tests/integration_tests.rsgts-macros/tests/value_dispatch_tests.rsgts-validator/src/format/markdown.rsgts-validator/src/normalize.rsgts-validator/src/validator.rsgts/src/entities.rsgts/src/gts.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/store.rsgts/src/store_test.rs
✅ Files skipped from review due to trivial changes (4)
- gts-validator/src/format/markdown.rs
- gts-macros/tests/value_dispatch_tests.rs
- README.md
- gts-validator/src/normalize.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- gts/src/schema_cast.rs
- gts-validator/src/validator.rs
- gts/src/store_test.rs
- gts-id/src/parse.rs
- gts-macros/tests/integration_tests.rs
- gts/src/ops.rs
- gts/src/gts.rs
- gts/src/entities.rs
- gts/src/store.rs
- gts-id/src/gts_id_segment.rs
aviator5
force-pushed
the
gts-id-extraction
branch
from
6f2829f to
cf2afe8
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gts-id/src/gts_id.rs (1)
373-414:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse complete IDs in the malformed-token tests.
These test inputs have fewer than the required six parts (e.g.,
gts.x.test!.v1~has only four: prefix, vendor, malformed-package, version), so they fail on segment-count validation before the parser can exercise the targeted invalid-character or invalid-start rules.Constructing each test case with the full
gts.vendor.package.namespace.type.versionstructure—inserting the malformed token into one of the five required fields—will ensure the assertions actually validate the intended character/start rules.🤖 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 `@gts-id/src/gts_id.rs` around lines 373 - 414, The test cases in test_gts_id_invalid_characters, test_gts_id_uppercase_rejected, test_gts_id_hyphen_rejected, test_gts_id_digit_start_segment, and test_gts_id_segment_start_underscore are using incomplete GtsId structures with fewer than six required parts. These incomplete IDs fail segment-count validation before the parser can exercise the targeted character or start-position validation rules. Update each test case to use the complete six-part structure gts.vendor.package.namespace.type.version by inserting the malformed token into the appropriate segment being tested while providing valid values for all other required segments. For example, in test_gts_id_invalid_characters, expand inputs like gts.x.test!.v1~ to gts.x.test!.namespace.type.v1~ to ensure the invalid-character validation actually executes.
🤖 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.
Duplicate comments:
In `@gts-id/src/gts_id.rs`:
- Around line 373-414: The test cases in test_gts_id_invalid_characters,
test_gts_id_uppercase_rejected, test_gts_id_hyphen_rejected,
test_gts_id_digit_start_segment, and test_gts_id_segment_start_underscore are
using incomplete GtsId structures with fewer than six required parts. These
incomplete IDs fail segment-count validation before the parser can exercise the
targeted character or start-position validation rules. Update each test case to
use the complete six-part structure gts.vendor.package.namespace.type.version by
inserting the malformed token into the appropriate segment being tested while
providing valid values for all other required segments. For example, in
test_gts_id_invalid_characters, expand inputs like gts.x.test!.v1~ to
gts.x.test!.namespace.type.v1~ to ensure the invalid-character validation
actually executes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dc1c32d-e404-452f-8e76-45456b3065c2
📒 Files selected for processing (23)
README.mdgts-id/src/gts_id.rsgts-id/src/gts_id_pattern.rsgts-id/src/gts_id_segment.rsgts-id/src/lib.rsgts-id/src/parse.rsgts-macros/src/instance.rsgts-macros/src/lib.rsgts-macros/tests/compile_fail/instance_id_invalid_format.rsgts-macros/tests/compile_fail/instance_id_wildcard_in_instance.rsgts-macros/tests/inheritance_tests.rsgts-macros/tests/integration_tests.rsgts-macros/tests/value_dispatch_tests.rsgts-validator/src/format/markdown.rsgts-validator/src/normalize.rsgts-validator/src/validator.rsgts/src/entities.rsgts/src/gts.rsgts/src/lib.rsgts/src/ops.rsgts/src/schema_cast.rsgts/src/store.rsgts/src/store_test.rs
✅ Files skipped from review due to trivial changes (6)
- gts-macros/tests/compile_fail/instance_id_invalid_format.rs
- gts-validator/src/format/markdown.rs
- gts-macros/tests/compile_fail/instance_id_wildcard_in_instance.rs
- gts-validator/src/normalize.rs
- gts-macros/tests/value_dispatch_tests.rs
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- gts/src/schema_cast.rs
- gts/src/store_test.rs
- gts-macros/tests/inheritance_tests.rs
- gts-id/src/gts_id_segment.rs
- gts-macros/tests/integration_tests.rs
- gts/src/store.rs
- gts/src/entities.rs
- gts-id/src/parse.rs
aviator5
force-pushed
the
gts-id-extraction
branch
6 times, most recently
from
2c79348 to
2c0694d
Compare
Split the monolithic gts-id crate into focused modules (parse, gts_id, gts_id_segment, gts_wildcard, error). GtsID / GtsWildcard fields are now private behind accessors (id(), segments(), into_segments()), and the error type is the GtsIdError struct. Move deterministic UUID v5 derivation into gts-id as a GtsID::to_uuid inherent method, gated behind the crate's optional `uuid` feature (off by default). The gts crate enables the feature; gts-macros depends on gts-id without it, so uuid stays out of the proc-macro build graph and out of gts-id's default public API. This replaces the former GtsIdUuidExt extension trait that previously lived in the gts crate. Adapt the dependent crates to the tightened API: field access -> accessor methods, gts_id_segments -> segments(), GtsError -> GtsIdError. gts.rs is slimmed to re-export the id primitives from gts-id and keep the schema-aware GtsTypeId / GtsInstanceId wrappers. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
Add validating constructors that enforce the type/instance discrimination
via the type system instead of downstream `ends_with('~')` checks:
- `GtsTypeId::try_new` accepts only trailing-`~` type ids.
- `GtsInstanceId::try_new` accepts only chained, non-`~` instance ids.
Both parse and structurally validate via `GtsID::new`, then classify by
kind, returning `GtsIdError` on a malformed string or a kind mismatch.
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- GtsIdSegment is now an enum (Concrete / UuidTail variants) instead of a plain struct - Rename GtsID → GtsId, GtsWildcard → GtsIdPattern, GtsSegmentError → GtsIdSegmentError - Rename new() → try_new() to follow Rust convention for fallible constructors - Privatise segment fields; expose them via accessor methods - Add gts-id/README.md with full API usage examples Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
…tern - Implements From<&GtsId> and From<GtsId> for GtsIdPattern, reusing validated segments without re-parsing - Adds ergonomic GtsId::to_pattern() borrowing form as an alias for From<&GtsId> - Adds tests covering roundtrip fidelity, chained instance ids, derived-type coverage (spec §3.6), and From ref/owned agreement Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
- Accept the glued `v*` version wildcard (spec §10 rule 4): a `v*` token at the version position parses as a wildcard meaning "any version". Simplify the structural wildcard gate to require `*` as the terminal character, which admits `v*` for free while still rejecting `*~`, mid-chain wildcards, and partial-token wildcards like `msg*`. - Reimplement `GtsIdPattern::covers` over segments instead of string prefixes so minor-version flexibility is honoured: `…v1~*` now covers `…v1.0~*` and `…v1~` covers `…v1.0~` (the string-prefix test missed these). Drop the now-unused `prefix_str` helper. - Correct stale docs: a concrete pattern is not exact-match — it covers derived chains per spec §3.6 implicit derived-type coverage. - Add unit tests for v* parsing/matching/rejections and minor-version covers. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-id-extraction
branch
from
7f73558 to
cc0fb6c
Compare
Contributor
Author
yes, rebased and bumped in the last commit |
Contributor
Author
good catch, fixed |
…tests - Add public getter methods to GtsIdSegmentParts for all fields (vendor, package, namespace, type_name, ver_major, ver_minor, is_type, raw) - Add comprehensive tests for segment parts accessors covering all field combinations - Improve UUID tail and pattern matching test cases with explicit assertions Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-id-extraction
branch
from
1613359 to
e403309
Compare
This was referenced Jun 18, 2026
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
3 participants
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.
Centralizes all GTS ID-related logic into the gts-id crate. Previously, identifier handling was scattered between gts-id (parsing, validation) and gts (type/instance discrimination, UUID derivation, schema-aware wrappers). Now:
Key Changes:
Benefits
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements