Consolidate GTS ID logic into gts-id crate and add fallible type-safe constructors#101
Open
aviator5 wants to merge 4 commits into
Open
Consolidate GTS ID logic into gts-id crate and add fallible type-safe constructors#101aviator5 wants to merge 4 commits into
aviator5 wants to merge 4 commits into
Conversation
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>
📝 WalkthroughWalkthroughThis PR restructures GTS identifier parsing from scattered implementations into a cohesive ChangesGTS ID Crate Extraction and System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
- Restructure GtsIdSegment from flat struct with bool flags to sum type:
- Concrete(GtsIdSegmentParts): type/version segments
- Wildcard(GtsIdSegmentParts): wildcard pattern segments
- UuidTail(String): trailing anonymous-instance UUIDs
- Eliminates illegal state representation (UUID with vendor, etc.)
- Moves segment parsing to GtsIdSegment::parse constructor
- Privatize all GtsIdSegment fields; expose via accessor methods:
- vendor(), package(), namespace(), type_name(), ver_major(), ver_minor()
- is_type(), is_wildcard(), raw(), uuid_tail()
- Optional #[cfg(uuid)] uuid() for parsed UUID from UuidTail
- Unify public type naming under GtsId* prefix for consistency:
- GtsID → GtsId (yadda yadda, IDEs struggle with this)
- GtsWildcard → GtsIdWildcard
- SegmentParts → GtsIdSegmentParts (new, public payload struct)
- GtsSegmentError → GtsIdSegmentError
- Rename module: gts_wildcard.rs → gts_id_wildcard.rs
- Remove dead fields num/offset from segments (position tracking now
lives exclusively in GtsIdError.segment for error reporting)
- Update all consumers to use accessors:
- gts/{entities,ops,store}.rs
- gts-macros tests
- gts-validator
- All tests updated for new API
Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
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
- is_valid: drop redundant prefix check so it trims consistently with new() - GtsIdSegment: make it an opaque wrapper over a private enum so the UUID-tail invariant can't be forged by downstream crates - ops: serialize a wildcard's unspecified major as null while keeping a concrete v0 as 0 (is_wildcard discriminates the sentinel) - store: require gts:// $ref targets to be type ids via GtsTypeId::try_new (only schema docs are registered, so instance refs must fail fast) - store_test: match the mock reader on effective_id() to mirror GtsStore::populate_from_reader - README: fix stale segments()/wildcard_match() direct-API examples Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
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
Comment on lines
+127
to
+130
| /// `true` when this is a wildcard segment. | ||
| #[must_use] | ||
| pub fn is_wildcard(&self) -> bool { | ||
| matches!(self.0, GtsIdSegmentKind::Wildcard(_)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant portions around is_wildcard and the enum definition
FILE="gts-id/src/gts_id_segment.rs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "== is_wildcard context =="
nl -ba "$FILE" | sed -n '90,160p'
echo
echo "== GtsIdSegmentKind context =="
# Print where GtsIdSegmentKind is defined (and surrounding lines)
# If not found, show closest enum/struct related area.
if rg -n "GtsIdSegmentKind" "$FILE" >/dev/null; then
rg -n "enum GtsIdSegmentKind|GtsIdSegmentKind" "$FILE" | head -n 20
# show the first enum definition area if present
LINENUM=$(rg -n "enum GtsIdSegmentKind" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${LINENUM:-}" ]; then
start=$((LINENUM-20))
end=$((LINENUM+80))
nl -ba "$FILE" | sed -n "${start},${end}p"
fi
else
echo "No occurrences of GtsIdSegmentKind found in $FILE" >&2
fiRepository: GlobalTypeSystem/gts-rust
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="gts-id/src/gts_id_segment.rs"
[ -f "$FILE" ] || { echo "Missing file: $FILE" >&2; exit 1; }
echo "== is_wildcard matches =="
rg -n "fn is_wildcard" "$FILE" || true
echo
echo "== GtsIdSegmentKind matches (first few) =="
rg -n "GtsIdSegmentKind" "$FILE" | head -n 50 || true
echo
echo "== Print around is_wildcard (±40 lines) =="
LINE=$(rg -n "fn is_wildcard" "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-40))
END=$((LINE+60))
awk -v s="$START" -v e="$END" 'NR>=s && NR<=e {printf "%5d:%s\n", NR, $0}' "$FILE"
echo
echo "== Print around enum GtsIdSegmentKind (±80 lines) =="
ENUM_LINE=$(rg -n "enum GtsIdSegmentKind" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${ENUM_LINE:-}" ]; then
START=$((ENUM_LINE-40))
END=$((ENUM_LINE+120))
awk -v s="$START" -v e="$END" 'NR>=s && NR<=e {printf "%5d:%s\n", NR, $0}' "$FILE"
else
echo "No `enum GtsIdSegmentKind` found in $FILE" >&2
fiRepository: GlobalTypeSystem/gts-rust
Length of output: 12199
Borrow the enum in is_wildcard()
In gts-id/src/gts_id_segment.rs line 130, matches!(self.0, ...) moves GtsIdSegmentKind out of &self (the enum contains String, so it’s not Copy). Match on &self.0 instead.
💡 Minimal fix
pub fn is_wildcard(&self) -> bool {
- matches!(self.0, GtsIdSegmentKind::Wildcard(_))
+ matches!(&self.0, GtsIdSegmentKind::Wildcard(_))
}🤖 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_segment.rs` around lines 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.
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
2 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
New Features
Improvements
Documentation