Skip to content

feat: generate schemas and well-known instances resolves #70#71

Open
devjow wants to merge 10 commits intoGlobalTypeSystem:mainfrom
devjow:chore/gts-schema-build
Open

feat: generate schemas and well-known instances resolves #70#71
devjow wants to merge 10 commits intoGlobalTypeSystem:mainfrom
devjow:chore/gts-schema-build

Conversation

@devjow
Copy link
Copy Markdown
Contributor

@devjow devjow commented Feb 27, 2026

Summary by CodeRabbit

  • New Features
    • CLI now supports selecting generation mode: schemas, instances, or both.
    • Added declarative macro/attribute flow to declare well-known instances and generate JSON files with robust parsing and string-literal decoding.
  • Bug Fixes / Security
    • Enforced sandboxed output validation to prevent writing outside allowed boundaries.
  • Documentation
    • Expanded macro usage docs and examples (duplicate section noted).
  • Tests
    • Extensive unit and integration tests for parsing, generation, sandboxing, and macro error cases.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 27, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a compile-time proc-macro for well-known instances, CLI support to extract/generate instances from Rust sources, shared file-walking and sandboxing utilities, schema-generator refactor to reuse helpers, and extensive unit/integration/compile-fail tests.

Changes

Cohort / File(s) Summary
CLI & Manifest
gts-cli/Cargo.toml, gts-cli/src/cli.rs, gts-cli/tests/cli_run_tests.rs
Added workspace dependency entry for gts-id; introduced GenerateMode (Schemas, Instances, All) and threaded a mode field into the GenerateFromRust CLI command.
Shared file & sandbox utilities
gts-cli/src/gen_common.rs
New module providing Rust-file walker with glob/exclude support, auto-ignore dirs, gts:ignore directive handling, sandbox root computation, safe canonicalization for non-existent paths, path validation, and unit tests.
Instance generation module
gts-cli/src/gen_instances/...
gts-cli/src/gen_instances/mod.rs, .../attrs.rs, .../parser.rs, .../string_lit.rs, .../writer.rs
New module to locate #[gts_well_known_instance] annotations, parse/validate attrs, decode string literals, validate JSON bodies, compose IDs, check unsupported forms, and emit per-instance files with sandbox validation; includes broad test coverage.
Schema generation refactor
gts-cli/src/gen_schemas.rs
Replaced ad-hoc traversal with walk_rust_files, centralized exclusion/sandbox logic, moved safe canonicalization and sandbox validation before writes, and updated tests.
Crate module surface
gts-cli/src/lib.rs, gts-cli/src/main.rs
Exposed and wired new gen_common and gen_instances modules in the CLI crate and binary.
Proc macro & docs
gts-macros/src/lib.rs, gts-macros/README.md
Added #[gts_well_known_instance] attribute proc-macro that validates attributes and annotated consts at compile time; README expanded with usage docs (duplicate block present).
Macro compile-fail tests
gts-macros/tests/compile_fail/instance_*.rs, *.stderr
Added ~10 compile-fail tests covering missing attrs, wrong item types, tilde rules, bare wildcard, non-const/static misuse, and expected stderr outputs.
Integration & test suite
gts-cli/tests/gen_instances_tests.rs, gts-cli/tests/*
Large integration test suite for instance generation (positive and negative cases) and unit tests for new helpers (string literal decoding, glob matching, sandbox canonicalization, etc.).

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI/Main
    participant Walker as walk_rust_files
    participant Parser as extract_instances_from_source
    participant Attrs as parse_instance_attrs
    participant Sandbox as sandbox_checks
    participant Writer as generate_single_instance

    CLI->>+Walker: traverse Rust sources (excludes, auto-ignore, gts:ignore)
    Walker->>+Parser: provide candidate file content
    Parser->>Parser: strip comments, find annotations, decode literals
    Parser->>+Attrs: parse attributes, validate composed GTS ID
    Attrs-->>-Parser: return ParsedInstance(s)
    Parser-->>-Walker: parsed instances
    CLI->>+Sandbox: compute_sandbox_root(output_override?)
    Sandbox-->>-CLI: sandbox_root
    loop per instance
        CLI->>+Writer: generate_single_instance(inst, output, sandbox_root)
        Writer->>+Sandbox: safe_canonicalize_nonexistent & validate target path
        Sandbox-->>-Writer: validated target path
        Writer->>Writer: inject id, create parents, write file
        Writer-->>-CLI: written file path
    end
    CLI->>CLI: aggregate results and print summary
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • MikeFalcon77
  • Artifizer

Poem

🐇 I nibble annotations in the night,
const crumbs of JSON snug and bright.
I hop through paths and guard the gate,
stitch each id and validate,
then scamper off — those files take flight. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing feature to generate schemas and well-known instances, with a clear reference to the resolved issue #70.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan for PR comments
  • Generate coding plan

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
gts-macros/README.md (1)

459-462: Add language specifier to fenced code block.

The code block showing the instance ID format is missing a language specifier. Consider adding text or leaving it as a simple inline code example.

🔧 Proposed fix
 The full instance ID is `schema_id + instance_segment`, e.g.:
-```
+```text
 gts.x.core.events.topic.v1~x.commerce._.orders.v1.0
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In @gts-macros/README.md around lines 459 - 462, Update the fenced code block
that shows the instance ID format (the line containing
"gts.x.core.events.topic.v1~x.commerce._.orders.v1.0") to include a language
specifier (e.g., change the opening triple backticks to "```text") so the block
is marked as plain text; keep the content unchanged and only modify the fence
marker around the instance ID example.


</details>

</blockquote></details>
<details>
<summary>gts-cli/src/gen_instances/mod.rs (1)</summary><blockquote>

`123-132`: **Consider centralizing output-path composition.**

Line 123 through Line 132 duplicates path-building logic that also exists in `gts-cli/src/gen_instances/writer.rs`. If one side changes, duplicate-path detection can diverge from actual write behavior.

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In `@gts-cli/src/gen_instances/mod.rs` around lines 123 - 132, The path-building
logic around composed, file_rel and raw_path is duplicated between
gen_instances/mod.rs and gen_instances/writer.rs; extract this into a single
helper (e.g., compute_instance_path or build_instance_output_path) that accepts
inst.attrs.schema_id, inst.attrs.instance_segment, inst.attrs.dir_path,
inst.source_file, the optional output parameter, and sandbox_root, returns the
final PathBuf, and replace the local composition in both mod.rs (where
composed/file_rel/raw_path are currently built) and writer.rs to call this
helper so path construction is centralized and consistent.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

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

Inline comments:
In @gts-cli/src/gen_common.rs:

  • Around line 229-255: The function that builds canonical_ancestor (used by
    safe_canonicalize_nonexistent) can return a relative path when the original
    input is relative and no existing ancestor is found; fix by first normalizing
    the input path to an absolute path (e.g., let mut path = if path.is_absolute() {
    path.to_path_buf() } else { std::env::current_dir()?.join(path) }) before
    computing existing_ancestor/suffix_components so that canonical_ancestor is
    always absolute; update references to existing_ancestor, suffix_components and
    the final canonical_ancestor logic to operate on this normalized absolute path.

In @gts-cli/src/gen_instances/attrs.rs:

  • Around line 126-153: The raw-string handling can continue without advancing
    pos when a closing delimiter isn't found, causing an infinite loop; in the
    branch inside the 'raw loop (the code that searches from content_start using
    scan, comparing bytes and counting hashes) ensure that if the closing delimiter
    is not found you advance pos past the scanned region (e.g., set pos = scan or
    pos = len) before the continue so the outer parsing loop progresses; update the
    block that currently does "continue" after the 'raw loop to move pos forward
    when no match is found and then continue, referencing variables pos, scan,
    content_start, hashes, bytes and the 'raw loop.

In @gts-cli/tests/gen_instances_tests.rs:

  • Around line 298-316: The current check
    assert!(!root.join("../../etc").exists()) is brittle because
    root.join("../../etc") may resolve to a real system path (e.g., /etc); instead
    change the assertion to canonicalize both the sandbox root and the attempted
    path (e.g., let attempted = root.join("../../etc")), then if attempted.exists()
    assert that attempted.canonicalize()? starts_with(&root.canonicalize()?) so any
    created directory must be inside the sandbox; use std::fs::canonicalize on root
    and attempted to deterministically verify the attempted path is not created
    outside the sandbox.

Nitpick comments:
In @gts-cli/src/gen_instances/mod.rs:

  • Around line 123-132: The path-building logic around composed, file_rel and
    raw_path is duplicated between gen_instances/mod.rs and gen_instances/writer.rs;
    extract this into a single helper (e.g., compute_instance_path or
    build_instance_output_path) that accepts inst.attrs.schema_id,
    inst.attrs.instance_segment, inst.attrs.dir_path, inst.source_file, the optional
    output parameter, and sandbox_root, returns the final PathBuf, and replace the
    local composition in both mod.rs (where composed/file_rel/raw_path are currently
    built) and writer.rs to call this helper so path construction is centralized and
    consistent.

In @gts-macros/README.md:

  • Around line 459-462: Update the fenced code block that shows the instance ID
    format (the line containing
    "gts.x.core.events.topic.v1~x.commerce._.orders.v1.0") to include a language
    specifier (e.g., change the opening triple backticks to "```text") so the block
    is marked as plain text; keep the content unchanged and only modify the fence
    marker around the instance ID example.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

**Configuration used**: defaults

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between cd8ed5b04a03fb9e761b3659edbe6f0785f2d279 and 4cf619b8809e73b4cf54c62330c07e9496b2ad07.

</details>

<details>
<summary>⛔ Files ignored due to path filters (1)</summary>

* `Cargo.lock` is excluded by `!**/*.lock`

</details>

<details>
<summary>📒 Files selected for processing (31)</summary>

* `gts-cli/Cargo.toml`
* `gts-cli/src/cli.rs`
* `gts-cli/src/gen_common.rs`
* `gts-cli/src/gen_instances/attrs.rs`
* `gts-cli/src/gen_instances/mod.rs`
* `gts-cli/src/gen_instances/parser.rs`
* `gts-cli/src/gen_instances/string_lit.rs`
* `gts-cli/src/gen_instances/writer.rs`
* `gts-cli/src/gen_schemas.rs`
* `gts-cli/src/lib.rs`
* `gts-cli/src/main.rs`
* `gts-cli/tests/cli_run_tests.rs`
* `gts-cli/tests/gen_instances_tests.rs`
* `gts-macros/README.md`
* `gts-macros/src/lib.rs`
* `gts-macros/tests/compile_fail/instance_const_wrong_type.rs`
* `gts-macros/tests/compile_fail/instance_const_wrong_type.stderr`
* `gts-macros/tests/compile_fail/instance_missing_dir_path.rs`
* `gts-macros/tests/compile_fail/instance_missing_dir_path.stderr`
* `gts-macros/tests/compile_fail/instance_missing_instance_segment.rs`
* `gts-macros/tests/compile_fail/instance_missing_instance_segment.stderr`
* `gts-macros/tests/compile_fail/instance_missing_schema_id.rs`
* `gts-macros/tests/compile_fail/instance_missing_schema_id.stderr`
* `gts-macros/tests/compile_fail/instance_on_non_const.rs`
* `gts-macros/tests/compile_fail/instance_on_non_const.stderr`
* `gts-macros/tests/compile_fail/instance_schema_id_no_tilde.rs`
* `gts-macros/tests/compile_fail/instance_schema_id_no_tilde.stderr`
* `gts-macros/tests/compile_fail/instance_segment_bare_wildcard.rs`
* `gts-macros/tests/compile_fail/instance_segment_bare_wildcard.stderr`
* `gts-macros/tests/compile_fail/instance_segment_ends_with_tilde.rs`
* `gts-macros/tests/compile_fail/instance_segment_ends_with_tilde.stderr`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment thread gts-cli/src/gen_common.rs
Comment thread gts-cli/src/gen_instances/attrs.rs
Comment thread gts-cli/tests/gen_instances_tests.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
gts-cli/tests/gen_instances_tests.rs (1)

295-316: ⚠️ Potential issue | 🟡 Minor

Brittle sandbox escape assertion may cause false failures.

At line 315, root.join("../../etc").exists() can resolve to /etc on Unix systems, which typically exists. This makes the test fail for the wrong reason (it's asserting that /etc doesn't exist, not that no escape occurred).

The safe_canonicalize_nonexistent function rejects .. components before any filesystem operations (as seen in the relevant snippet from gen_common.rs), so the directory creation should never be attempted. However, the assertion itself is testing the wrong thing.

🔧 Proposed fix: Assert against a unique escape target
 #[test]
 fn sandbox_escape_rejected() {
     let (_tmp, root) = sandbox();
+    // Use a unique escape target that won't exist on any system
+    let escape_target = format!("gts_escape_test_{}", std::process::id());
     let src = concat!(
         "#[gts_well_known_instance(\n",
-        "    dir_path = \"../../etc\",\n",
+        "    dir_path = \"../__ESCAPE__\",\n",
         "    schema_id = \"gts.x.core.events.topic.v1~\",\n",
         "    instance_segment = \"x.commerce._.orders.v1.0\"\n",
         ")]\n",
         "pub const FOO: &str = \"{\\\"name\\\":\\\"x\\\"}\";\n"
-    );
-    write(&root, "escape.rs", src);
+    )
+    .replace("__ESCAPE__", &escape_target);
+    write(&root, "escape.rs", &src);

     let err = run(root.to_str().unwrap(), Some(root.to_str().unwrap()), &[]).unwrap_err();
     let msg = err.to_string();
     assert!(
         msg.contains("Security error") || msg.contains("sandbox") || msg.contains("'..'"),
         "Got: {msg}"
     );
     // No directory should have been created outside sandbox
-    assert!(!root.join("../../etc").exists());
+    let outside = root.parent().unwrap().join(&escape_target);
+    assert!(
+        !outside.exists(),
+        "Unexpected directory created: {}",
+        outside.display()
+    );
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@gts-cli/tests/gen_instances_tests.rs` around lines 295 - 316, The test
sandbox_escape_rejected currently checks !root.join("../../etc").exists(), which
can be true for the wrong reason on Unix; change the assertion to verify no
creation of a uniquely named escape target so it cannot collide with existing
system paths: in the sandbox_escape_rejected test replace the ../../etc check
with a check for a uniquely named path (e.g.
"../../gts_cli_sandbox_escape_marker" or similar unlikely name) and assert it
does not exist after run; this keeps the intent (no directory created outside
sandbox) and aligns with safe_canonicalize_nonexistent behavior used elsewhere
(see safe_canonicalize_nonexistent).
🧹 Nitpick comments (2)
gts-cli/src/gen_instances/mod.rs (1)

122-138: Path normalization may miss symbolic link edge cases.

The path key is built by collecting components and converting to string (lines 134-138). This doesn't resolve symlinks or perform full canonicalization, which could allow two different-looking paths to resolve to the same file. However, given that generate_single_instance performs sandbox validation with full canonicalization before writing, this is defense-in-depth rather than the primary guard.

💡 Optional: Use canonicalization for stricter duplicate path detection

If stricter duplicate detection is desired, consider using safe_canonicalize_nonexistent here as well. The current approach is acceptable since the writer validates paths before writing.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@gts-cli/src/gen_instances/mod.rs` around lines 122 - 138, The current loop
builds a path key by collecting raw_path components and to_string_lossy
(variables composed, file_rel, raw_path) which doesn't resolve symlinks; replace
or augment that with a canonicalized path using
safe_canonicalize_nonexistent(raw_path) to get a stable key (and fall back to
the existing components->to_string_lossy string if canonicalization fails), so
duplicate detection uses the resolved/canonical form similar to
generate_single_instance's sandbox validation.
gts-cli/src/gen_instances/writer.rs (1)

60-76: Consider handling JSON deserialization errors more explicitly.

Line 62 deserializes inst.json_body directly. If the JSON is malformed or not an object, serde_json::from_str will return an error that propagates with minimal context. The upstream validate_json_body in the parser should catch this, but defensive error wrapping here would improve debuggability.

💡 Optional: Add context to JSON parsing error
     // Build JSON with injected "id" field
     let mut obj: serde_json::Map<String, serde_json::Value> =
-        serde_json::from_str(&inst.json_body)?;
+        serde_json::from_str(&inst.json_body).map_err(|e| {
+            anyhow::anyhow!(
+                "{}:{}: Failed to parse JSON body: {}",
+                inst.source_file,
+                inst.line,
+                e
+            )
+        })?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@gts-cli/src/gen_instances/writer.rs` around lines 60 - 76, The direct call to
serde_json::from_str(&inst.json_body) can yield an opaque error when the JSON is
malformed; wrap or map that error to add context before propagating so failures
indicate which instance and payload failed to parse (e.g., reference
inst.json_body and composed id), by replacing the direct call in writer.rs with
a fallible parse that maps the serde error into a contextual error (using
map_err or the crate's error/context helper) mentioning inst.json_body and
composed, then proceed to insert "id" into the resulting Map and continue
writing to raw_output_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@gts-cli/tests/gen_instances_tests.rs`:
- Around line 295-316: The test sandbox_escape_rejected currently checks
!root.join("../../etc").exists(), which can be true for the wrong reason on
Unix; change the assertion to verify no creation of a uniquely named escape
target so it cannot collide with existing system paths: in the
sandbox_escape_rejected test replace the ../../etc check with a check for a
uniquely named path (e.g. "../../gts_cli_sandbox_escape_marker" or similar
unlikely name) and assert it does not exist after run; this keeps the intent (no
directory created outside sandbox) and aligns with safe_canonicalize_nonexistent
behavior used elsewhere (see safe_canonicalize_nonexistent).

---

Nitpick comments:
In `@gts-cli/src/gen_instances/mod.rs`:
- Around line 122-138: The current loop builds a path key by collecting raw_path
components and to_string_lossy (variables composed, file_rel, raw_path) which
doesn't resolve symlinks; replace or augment that with a canonicalized path
using safe_canonicalize_nonexistent(raw_path) to get a stable key (and fall back
to the existing components->to_string_lossy string if canonicalization fails),
so duplicate detection uses the resolved/canonical form similar to
generate_single_instance's sandbox validation.

In `@gts-cli/src/gen_instances/writer.rs`:
- Around line 60-76: The direct call to serde_json::from_str(&inst.json_body)
can yield an opaque error when the JSON is malformed; wrap or map that error to
add context before propagating so failures indicate which instance and payload
failed to parse (e.g., reference inst.json_body and composed id), by replacing
the direct call in writer.rs with a fallible parse that maps the serde error
into a contextual error (using map_err or the crate's error/context helper)
mentioning inst.json_body and composed, then proceed to insert "id" into the
resulting Map and continue writing to raw_output_path.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf619b and 64ecfd3.

📒 Files selected for processing (3)
  • gts-cli/src/gen_instances/mod.rs
  • gts-cli/src/gen_instances/writer.rs
  • gts-cli/tests/gen_instances_tests.rs

@codecov-commenter
Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
gts-cli/tests/gen_instances_tests.rs (1)

295-314: ⚠️ Potential issue | 🟡 Minor

sandbox_escape_rejected should also assert no out-of-sandbox side effects.

Right now this test only checks the error message. It can still pass if directory creation happens before the error path. Add a deterministic assertion that a uniquely named outside path was not created.

💡 Proposed hardening
 fn sandbox_escape_rejected() {
     let (_tmp, root) = sandbox();
-    let src = concat!(
+    let escape_component = format!(
+        "gts_escape_{}",
+        root.file_name().unwrap().to_string_lossy()
+    );
+    let src = concat!(
         "#[gts_well_known_instance(\n",
-        "    dir_path = \"../../etc\",\n",
+        "    dir_path = \"../__ESCAPE__\",\n",
         "    schema_id = \"gts.x.core.events.topic.v1~\",\n",
         "    instance_segment = \"x.commerce._.orders.v1.0\"\n",
         ")]\n",
         "pub const FOO: &str = \"{\\\"name\\\":\\\"x\\\"}\";\n"
-    );
-    write(&root, "escape.rs", src);
+    )
+    .replace("__ESCAPE__", &escape_component);
+    write(&root, "escape.rs", &src);

     let err = run(root.to_str().unwrap(), Some(root.to_str().unwrap()), &[]).unwrap_err();
     let msg = err.to_string();
     assert!(
         msg.contains("Security error") || msg.contains("sandbox") || msg.contains("'..'"),
         "Got: {msg}"
     );
+    let outside = root.parent().unwrap().join(&escape_component);
+    assert!(!outside.exists(), "Unexpected directory created: {}", outside.display());
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@gts-cli/tests/gen_instances_tests.rs` around lines 295 - 314, The test
sandbox_escape_rejected currently only checks the error message; update the
sandbox_escape_rejected test to also assert there were no out-of-sandbox side
effects by creating a uniquely named path outside the sandbox (e.g. based on
root plus a distinct filename) before invoking write/run and after run returns
Err assert that this outside path does not exist; locate the
sandbox_escape_rejected test and use the existing sandbox(), write(), and run()
helpers to compute the outside path and assert its non-existence after the
error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gts-cli/tests/gen_instances_tests.rs`:
- Around line 561-572: The test fixture in function
zero_hash_raw_string_is_accepted uses invalid Rust raw string syntax for the
ZERO_HASH constant; update the src fixture so the constant contains a valid Rust
string (e.g. use a raw string with a different delimiter like r#"... "# or
escape the inner quotes) so the declaration pub const ZERO_HASH: &str = ...; is
syntactically correct while preserving the intended JSON payload.

---

Duplicate comments:
In `@gts-cli/tests/gen_instances_tests.rs`:
- Around line 295-314: The test sandbox_escape_rejected currently only checks
the error message; update the sandbox_escape_rejected test to also assert there
were no out-of-sandbox side effects by creating a uniquely named path outside
the sandbox (e.g. based on root plus a distinct filename) before invoking
write/run and after run returns Err assert that this outside path does not
exist; locate the sandbox_escape_rejected test and use the existing sandbox(),
write(), and run() helpers to compute the outside path and assert its
non-existence after the error.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64ecfd3 and cb9896a.

📒 Files selected for processing (1)
  • gts-cli/tests/gen_instances_tests.rs

Comment thread gts-cli/tests/gen_instances_tests.rs Outdated
@devjow devjow force-pushed the chore/gts-schema-build branch from cb9896a to 3db66f7 Compare February 28, 2026 13:09
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gts-cli/src/gen_common.rs`:
- Around line 45-55: The glob-to-regex conversion currently only escapes '.' so
other regex metacharacters in pattern (e.g. +()[]?|) are treated as regex;
change the flow in the code that builds regex_pattern to first call
regex::escape(pattern) (use the regex::escape function on the incoming pattern)
and then perform the glob-specific replacements on the escaped string: replace
the escaped "\*\*" token (or "\\*\\*" depending on escape) with a placeholder,
then replace the escaped "\*" with the glob fragment "[^/]*", then restore the
placeholder to ".*"; finally use
Regex::new(&format!("(^|/){regex_pattern}($|/)")) (and keep the existing
fallback to normalized.contains(pattern)) so patterns with literal regex
characters are matched literally.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cb9896a and 3db66f7.

📒 Files selected for processing (2)
  • gts-cli/src/gen_common.rs
  • gts-cli/tests/gen_instances_tests.rs

Comment thread gts-cli/src/gen_common.rs Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
gts-cli/src/gen_common.rs (1)

212-265: Relative path input may cause false sandbox rejections.

The past review concern about normalizing to absolute paths is still present. If path is relative and no existing ancestor is found (e.g., when running from a different working directory), the function returns a relative path. Since sandbox_root is absolute, canonical.starts_with(sandbox_root) will always fail.

In practice this may be mitigated by how callers construct paths, but the function documentation doesn't specify this constraint. Consider defensive normalization to absolute paths at the start.

♻️ Suggested fix
 pub fn safe_canonicalize_nonexistent(path: &Path) -> Result<PathBuf> {
+    // Normalize relative paths to absolute before processing
+    let path = if path.is_absolute() {
+        path.to_path_buf()
+    } else {
+        std::env::current_dir()?.join(path)
+    };
+
     // Eagerly reject any .. component in the raw path before any filesystem ops.
     // This covers cases like /tmp/nonexistent/../escape where .. appears in the middle.
-    for component in path.components() {
+    for component in path.components() {
         if component == std::path::Component::ParentDir {

Run the following script to check if there are callers that could pass relative paths:

#!/bin/bash
# Search for usages of safe_canonicalize_nonexistent to understand caller patterns
rg -n "safe_canonicalize_nonexistent" --type rust -B 3 -A 3
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@gts-cli/src/gen_common.rs` around lines 212 - 265,
safe_canonicalize_nonexistent currently can return a relative path when given a
relative input (no existing ancestor), which breaks callers that expect an
absolute canonical path; fix by normalizing relative inputs to absolute at the
start of the function: if path.is_relative() (or !path.has_root()), resolve it
against std::env::current_dir() (e.g., let path = current_dir.join(path)) before
performing the ..-component check and all subsequent logic in
safe_canonicalize_nonexistent so the returned PathBuf is always absolute and
compatible with sandbox_root comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@gts-cli/src/gen_common.rs`:
- Around line 212-265: safe_canonicalize_nonexistent currently can return a
relative path when given a relative input (no existing ancestor), which breaks
callers that expect an absolute canonical path; fix by normalizing relative
inputs to absolute at the start of the function: if path.is_relative() (or
!path.has_root()), resolve it against std::env::current_dir() (e.g., let path =
current_dir.join(path)) before performing the ..-component check and all
subsequent logic in safe_canonicalize_nonexistent so the returned PathBuf is
always absolute and compatible with sandbox_root comparisons.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3db66f7 and ffbaa1c.

📒 Files selected for processing (3)
  • gts-cli/src/gen_common.rs
  • gts-cli/src/gen_instances/attrs.rs
  • gts-cli/tests/gen_instances_tests.rs

@devjow devjow requested a review from Artifizer February 28, 2026 15:17
Comment thread gts-macros/README.md Outdated
Comment thread gts-macros/README.md Outdated
devjow added 8 commits March 6, 2026 12:07
Signed-off-by: devjow <me@devjow.com>
…nite loop in raw-string blanking, and fix zero-hash raw-string test fixture

Signed-off-by: devjow <me@devjow.com>
Signed-off-by: devjow <me@devjow.com>
Signed-off-by: devjow <me@devjow.com>
…ingle id attribute

Signed-off-by: devjow <me@devjow.com>
…rror context

Signed-off-by: devjow <me@devjow.com>
@devjow devjow force-pushed the chore/gts-schema-build branch from 0d7ca79 to 8276d66 Compare March 9, 2026 16:50
devjow added 2 commits March 10, 2026 12:43
Signed-off-by: devjow <me@devjow.com>
Signed-off-by: devjow <me@devjow.com>
@devjow devjow force-pushed the chore/gts-schema-build branch from 7ef3cfa to 516d58f Compare March 12, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants