feat(trg): implement Agent Skills CLI with validation#6
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:
WalkthroughIntroduces a new Rust binary crate Changes
Sequence DiagramsequenceDiagram
participant User
participant Main as main.rs
participant CLI as Commands/AI
participant Handler as Skill Handler
participant Parser as agentskills::parser
participant Validator as agentskills::validator
participant Model as agentskills::models
participant FS as FileSystem
User->>Main: Execute CLI args
Main->>CLI: Parse with Clap
Main->>Handler: Dispatch to skill command
alt Validate Command
Handler->>Parser: resolve_skill_path(path)
Parser->>FS: exists(), read_to_string()
FS-->>Parser: skill file content
Handler->>Validator: validate_skill(fs, path)
Validator->>Parser: read_properties(fs, path)
Parser->>Model: Extract SkillProperties from YAML
Model-->>Parser: SkillProperties
Parser-->>Validator: Result with properties & metadata
Validator->>Validator: Run validation checks
Validator-->>Handler: Result<SkillProperties>
Handler-->>Main: exit code (0 or 1)
else ReadProperties Command
Handler->>Parser: resolve_skill_path(path)
Handler->>Parser: read_properties(fs, path)
Parser->>FS: read_to_string()
Parser->>Model: SkillProperties from YAML
Model->>Model: to_json()
Model-->>Handler: JSON string
Handler-->>Main: print + exit(0)
else ToPrompt Command
Handler->>Parser: resolve_skill_path() for each path
Handler->>Parser: read_properties_with_fs()
Handler->>Parser: find_skill_md() for locations
Parser->>FS: exists(), read_to_string()
Handler->>Model: Generate SkillWithLocation list
Handler->>Handler: to_prompt_with_location()
Handler-->>Main: print prompt + exit(0 or 1)
end
Main->>User: Exit with code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.cargo/config.toml:
- Around line 19-20: Remove the invalid Cargo alias named "verify" (the line
starting with verify = ...) because Cargo aliases cannot use shell chaining
(&&); either delete this alias from .cargo/config.toml or replace it with a
comment directing contributors to run the equivalent chained steps via the
project's justfile (or list the individual cargo subcommands as separate aliases
that do not use &&), and ensure any documentation (README or contributing notes)
points to the justfile for the combined verify workflow.
In @.github/workflows/ci.yml:
- Around line 163-167: Update the "Upload coverage to Codecov" step to use the
latest codecov action (change uses from codecov/codecov-action@v3 to `@v5`) and
add the required token input or environment variable so the action can run on
current runners; preserve existing inputs like files and fail_ci_if_error but
include token: ${{ secrets.CODECOV_TOKEN }} (or set CODECOV_TOKEN in env) so the
action authenticates correctly.
- Around line 142-145: Update the GitHub Actions step named "Run security audit"
to use the maintained action identifier and latest version: replace the uses
value "rustsec/audit-check-action@v1" with "rustsec/audit-check@v2.0.0" so the
workflow uses the correct repository and the v2.0.0 release of the action (which
includes the newer cargo-audit binary).
In `@CONTRIBUTING.md`:
- Line 9: Update the project to either enforce the minimum Rust version or
remove it from documentation: add the rust-version = "1.93.0" field to the
workspace Cargo.toml (or to each crate's Cargo.toml) and adjust CI config (the
workflow file that runs tests against stable) to include testing against the
MSRV, OR remove the Rust 1.93.0 mention from CONTRIBUTING.md so it no longer
claims an enforced minimum; ensure you update whichever artifacts reference the
minimum (CONTRIBUTING.md, Cargo.toml rust-version entries, and CI workflow
matrix) so the codebase and docs remain consistent.
In `@crates/agentskills/Cargo.toml`:
- Around line 15-16: Remove the unused dev-dependency entry for assert_cmd from
the agentskills crate manifest: locate the dev-dependencies block in Cargo.toml
(the line containing "assert_cmd = { workspace = true }") and delete that entry
so only tempfile remains; run cargo check/tests locally to verify no references
to assert_cmd exist and commit the updated Cargo.toml.
In `@crates/agentskills/src/validator.rs`:
- Around line 115-127: The directory-name comparison can produce a confusing
message when skill_path.file_name() is None (dir_name == ""), so update the
check around skill_path / dir_name / normalized_dir: detect the None/empty
dir_name case before comparing to normalized and push a clearer error into
errors (e.g., "skill path has no directory name" or "cannot determine directory
name for skill_path") or return early rather than reporting "must match
directory name ''"; reference skill_path, file_name(), dir_name, normalized_dir,
normalized, and the errors vector when making this change.
In `@crates/trg/src/commands/ai/skills/mod.rs`:
- Around line 34-44: The function resolve_skill_path currently returns an empty
PathBuf for inputs like Path::new("SKILL.md") because path.parent() yields
Some("") instead of None; change the parent handling to treat an empty parent as
absent (e.g., parent().filter(|p| !p.as_os_str().is_empty()).map(|p|
p.to_path_buf()).unwrap_or_else(|| PathBuf::from("."))) so bare "SKILL.md"
resolves to "."; update resolve_skill_path accordingly and add the suggested
unit test test_resolve_skill_path_bare_skill_md to assert
resolve_skill_path(Path::new("SKILL.md")) == PathBuf::from(".").
- Around line 46-69: Add a test for the bare-filename edge case by calling
resolve_skill_path(Path::new("SKILL.md")) and asserting it returns
PathBuf::from("."); update resolve_skill_path to handle the case where
path.parent() is None (i.e., the input is just "SKILL.md") by returning
PathBuf::from(".") instead of unwrapping or returning None so the new test
passes; reference the resolve_skill_path function when making the change.
🧹 Nitpick comments (15)
justfile (3)
44-59:verifyduplicates commands already defined as individual recipes.If the individual recipes change (e.g., adding
--all-features),verifywill silently drift. You could chain them as dependencies or call them viajust:♻️ Suggested refactor
# Run verification suite (fmt check + lint + test) verify: #!/bin/bash set -e echo "🔍 Checking formatting..." - cargo fmt --all -- --check + just fmt-check echo "✓ Format check passed" echo "🔍 Running clippy..." - cargo clippy --workspace --all-targets -- -D warnings + just lint echo "✓ Clippy check passed" echo "🔍 Running tests..." - cargo test --workspace + just test echo "✓ Tests passed" echo "🚀 All verification checks passed!"
90-90:infodepends onjqbut doesn't guard for it.
watch-testandbloatcheck for their tools before use.infoshould do the same forjq, or avoid the dependency entirely.♻️ Option A: add a guard
info: + `@command` -v jq >/dev/null 2>&1 || { echo "error: jq is required (https://jqlang.github.io/jq/)"; exit 1; } `@echo` "Project: trg (Rust)"♻️ Option B: drop jq dependency
- `@cargo` metadata --format-version 1 | jq '.workspace_members[]' -r | sed 's/ .*//' | sed 's/^/ - /' + `@cargo` metadata --format-version 1 --no-deps | python3 -c "import sys,json; [print(f' - {m.split()[0]}') for m in json.load(sys.stdin)['workspace_members']]"
97-105: Auto-installing tools without confirmation could surprise contributors.
cargo installon lines 99 and 104 will silently compile and install crates. Consider printing a message or prompting before installing, so a new contributor isn't caught off-guard by a lengthy compile..cargo/config.toml (1)
3-5: Note:-D warningsinrustflagsapplies globally, including to dependencies.Setting
-D warningsin[build] rustflagswill deny warnings in all compiled code, including third-party crates compiled from source. This can cause build failures from warnings in dependencies you don't control. Consider scoping this to your workspace crates only via[target.'cfg(all())']or relying solely on thecargo lintalias and CI for enforcement..github/pull_request_template.md (1)
34-36: Add a language specifier to the fenced code block to satisfy markdownlint (MD040).Proposed fix
-``` +```text <!-- Paste test output here --> ```CONTRIBUTING.md (1)
143-149: Add a language specifier to the fenced code block (MD040).The commit message format template block triggers markdownlint MD040. Adding
textas the language specifier resolves it.Proposed fix
-``` +```text <type>(<scope>): <subject> <body> <footer> ```crates/agentskills/src/models.rs (1)
24-63: Tests cover the key serialization paths well.Consider adding a deserialization round-trip test to verify that JSON with
"allowed-tools"deserializes back correctly into theallowed_toolsfield, since the serde rename applies in both directions..github/workflows/ci.yml (2)
46-62: Consider consolidating repeated cache steps.The same three cache blocks (registry, git index, build target) are duplicated across
clippy,test, andbuildjobs. This is verbose and a maintenance burden. Consider extracting them into a composite action or a reusable workflow step to keep things DRY.Also applies to: 77-93, 111-127
157-158:cargo install cargo-tarpaulinruns uncached on every CI run.This installs tarpaulin from source each time, adding several minutes. Consider caching
~/.cargo/bin/cargo-tarpaulinor using a pre-built binary (e.g., downloading from GitHub releases).crates/agentskills/src/prompt.rs (1)
16-26: Avoid cloning everySkillPropertiesinto_prompt.
to_promptclones eachSkillPropertiesjust to wrap it inSkillWithLocation. Consider havingSkillWithLocationhold a reference instead, or refactoringto_prompt_with_locationto accept an iterator of(&SkillProperties, Option<&str>).crates/agentskills/Cargo.toml (1)
7-7: Remove the unusedclapdependency from the library crate.The
agentskillscrate listsclapas a dependency but does not use it anywhere in its code. Since this is a library crate (not a binary), this dependency should be removed to reduce the crate's dependency footprint and keep it focused on its core functionality.crates/trg/src/commands/ai/skills/to_prompt.rs (2)
42-48: Useinto_iter()to avoid unnecessary cloning.
skill_itemsis not used after this point, sointo_iter()would avoid cloning theSkillPropertiesandOption<String>values.Suggested fix
- let skills_with_loc: Vec<_> = skill_items - .iter() - .map(|(props, location)| agentskills::prompt::SkillWithLocation { - properties: props.clone(), - location: location.clone(), + let skills_with_loc: Vec<_> = skill_items + .into_iter() + .map(|(props, location)| agentskills::prompt::SkillWithLocation { + properties: props, + location, }) .collect();
9-12: Consider requiring at least one path argument to match the pattern of similar commands.
ValidateArgsandReadPropertiesArgsboth use a single requiredPathBuf, whileToPromptArgsacceptsVec<PathBuf>with no constraints. This means callingtrg ai skills to-promptwith zero paths silently prints an empty prompt and exits 0. For consistency and to prevent accidental empty invocations, add#[arg(required = true)].Suggested fix
#[derive(Args)] pub struct ToPromptArgs { - #[arg(help = "Paths to skill directories or SKILL.md files")] + #[arg(required = true, help = "Paths to skill directories or SKILL.md files")] pub paths: Vec<PathBuf>, }crates/agentskills/src/filesystem.rs (1)
5-11: Consider providing default implementations forfile_nameandparent.Both
RealFSandMemFShave identical implementations forfile_nameandparentsince these are pure path operations with no filesystem I/O. Providing defaults on the trait eliminates the duplication and reduces the burden on future implementors.Suggested refactor
pub trait FileSystem { fn read_to_string(&self, path: &Path) -> io::Result<String>; fn write(&self, path: &Path, contents: &str) -> io::Result<()>; fn exists(&self, path: &Path) -> bool; - fn file_name(&self, path: &Path) -> Option<String>; - fn parent(&self, path: &Path) -> Option<PathBuf>; + + fn file_name(&self, path: &Path) -> Option<String> { + path.file_name() + .and_then(|n| n.to_str()) + .map(|s| s.to_string()) + } + + fn parent(&self, path: &Path) -> Option<PathBuf> { + path.parent().map(|p| p.to_path_buf()) + } }Then remove the duplicate
file_nameandparentimplementations from bothRealFSandMemFS.crates/agentskills/src/lib.rs (1)
9-9: Consider whetherMemFSbelongs in the public API.
MemFSis a test double. Exporting it from the library's top-level public API adds to the surface area that must be maintained. You could gate it behind a#[cfg(feature = "test-support")]feature flag or apub mod test_utilsmodule instead, keeping the default public API lean.
yordis
force-pushed
the
validate-skills
branch
from
b9c767f to
266898e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/SECURITY.md:
- Line 34: The statement "All workflows use pinned action versions" is incorrect
because CI uses floating major tags like actions/checkout@v4 and
dtolnay/rust-toolchain@stable; update the wording to accurately describe current
practice (e.g., "workflows use major version tags") or change those workflow
entries to SHA-pinned references (replace actions/checkout@v4 and
dtolnay/rust-toolchain@stable with full commit SHAs) so the doc matches reality.
- Around line 74-76: The SECURITY.md "SARIF Upload" section claims security
findings are uploaded to the GitHub Security tab but the CI workflows lack a
SARIF upload step; either remove or update that section or add the missing
upload step to CI. Fix by updating the "SARIF Upload" section text in
SECURITY.md to accurately reflect current CI behavior, or add a SARIF upload
job/step to your GitHub Actions workflow using the official action
(github/codeql-action/upload-sarif) and ensure it runs after CodeQL analysis and
publishes the generated sarif file. Reference the "SARIF Upload" heading in
SECURITY.md and the action name github/codeql-action/upload-sarif when making
the change.
In `@crates/agentskills/Cargo.toml`:
- Line 7: Remove the unused clap dependency entry from the agentskills library
Cargo.toml by deleting the line "clap = { workspace = true }" in the
crates/agentskills/Cargo.toml file; ensure no code in the agentskills crate
references clap (the trg binary already declares its own clap), then run cargo
check/build to verify the workspace still compiles and update Cargo.lock if
needed.
In `@crates/agentskills/src/parser.rs`:
- Around line 86-89: The parser currently reads allowed-tools via
yaml_value.get("allowed-tools").and_then(|v| v.as_str()).map(|s| s.to_string())
which silently returns None for YAML sequences; add a validator similar to
validate_compatibility() named validate_allowed_tools() and call it where
allowed_tools is parsed (or replace the simple as_str() path) so that if
allowed-tools is not a scalar string you return a clear error (or diagnostic)
indicating the expected space-separated string format; reference the
allowed_tools variable/yaml_value access and implement validate_allowed_tools()
to accept only scalar strings and reject sequences/mappings with a helpful
message.
In `@crates/trg/src/commands/ai/skills/to_prompt.rs`:
- Around line 38-40: The current logic in to_prompt.rs checks if had_error &&
skill_items.is_empty() and only then returns 1, which lets partial failures
(had_error true with non-empty skill_items) exit 0; change the control flow so
that if had_error is true the command returns a non-zero exit code (e.g., return
2) regardless of skill_items contents. Update the exit/return in the function
that owns had_error/skill_items (search for the block containing had_error and
skill_items in to_prompt.rs) to return a distinct non-zero code on any
had_error, or explicitly document intentional partial-success behavior if you
prefer keeping exit 0.
🧹 Nitpick comments (8)
CONTRIBUTING.md (1)
143-190: Add language specifiers to fenced code blocks.Several code blocks (lines 143, 163, 172, 176, 184) lack language identifiers, which triggers markdownlint MD040. Use
textfor commit message examples and directory trees to satisfy the linter and improve rendering.Proposed fix (representative examples)
-``` +```text <type>(<scope>): <subject>-``` +```text feat(agentskills): add field validation for frontmatter-``` +```text rusty-monorepo/ ├── crates/.github/workflows/ci.yml (2)
46-62: ConsiderSwatinem/rust-cacheinstead of manual cache steps.The three separate
actions/cachesteps for registry, git index, and target directory are repeated across clippy, test, and build jobs.Swatinem/rust-cacheis the standard single-step alternative for Rust projects — it handles all cargo cache paths, uses smarter cache keys, and prunes stale artifacts automatically.Example replacement (apply to each job)
- - name: Cache cargo registry - uses: actions/cache@v4 - with: - path: ~/.cargo/registry - key: ${{ runner.os }}-cargo-registry-${{ hashFiles('**/Cargo.lock') }} - - - name: Cache cargo index - uses: actions/cache@v4 - with: - path: ~/.cargo/git - key: ${{ runner.os }}-cargo-git-${{ hashFiles('**/Cargo.lock') }} - - - name: Cache cargo build - uses: actions/cache@v4 - with: - path: target - key: ${{ runner.os }}-cargo-build-target-${{ hashFiles('**/Cargo.lock') }} + - name: Cache Rust dependencies + uses: Swatinem/rust-cache@v2
157-158:cargo install cargo-tarpaulincompiles from source on every run — slow.Without caching
~/.cargo/bin, this step recompiles tarpaulin each CI run (several minutes). Either cache the cargo bin directory, or use a pre-built binary installation approach.Option A: Use cargo-binstall for pre-built binary
- name: Install tarpaulin - run: cargo install cargo-tarpaulin + run: | + curl -L --proto '=https' --tlsv1.2 -sSf https://raw.githubusercontent.com/cargo-bins/cargo-binstall/main/install-from-binstall-release.sh | bash + cargo binstall --no-confirm cargo-tarpaulinOption B: Cache cargo bin
If using
Swatinem/rust-cache(suggested above), installed binaries are cached automatically via~/.cargo/bin.crates/agentskills/src/models.rs (1)
4-16: Consider derivingPartialEqfor easier testing and comparison.The struct derives
DebugandClonebut notPartialEq. Adding it would enable directassert_eq!on struct instances in tests and simplify downstream comparison logic.♻️ Proposed change
-#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct SkillProperties {crates/agentskills/src/prompt.rs (1)
11-14: Consider adding common derives toSkillWithLocation.
SkillWithLocationis a public struct but lacksDebugandClonederives, which are generally expected on public types for ergonomics and debugging.♻️ Proposed change
+#[derive(Debug, Clone)] pub struct SkillWithLocation { pub properties: SkillProperties, pub location: Option<String>, }crates/trg/src/commands/ai/skills/to_prompt.rs (2)
21-35: Redundantfind_skill_mdcall — the file is already located insideread_properties_with_fs.
read_properties_with_fsinternally callsfind_skill_md(viaread_properties_with_metadata) to locate and read the skill file. Callingfind_skill_mdagain on line 23 duplicates that lookup. Consider usingread_properties_with_metadatadirectly (which returns the parsed YAML alongside properties) and deriving the location from the path returned by a singlefind_skill_mdcall before reading.Proposed refactor
for path in &self.paths { let skill_path = resolve_skill_path(path); - match ( - agentskills::read_properties_with_fs(fs, &skill_path), - agentskills::parser::find_skill_md(fs, &skill_path), - ) { - (Ok(props), Ok(location)) => { + let location = agentskills::parser::find_skill_md(fs, &skill_path).ok(); + match agentskills::read_properties_with_fs(fs, &skill_path) { + Ok(props) => { skill_items.push((props, Some(location.to_string_lossy().to_string()))); } - (Ok(props), Err(_)) => { - skill_items.push((props, None)); - } - (Err(e), _) => { + Err(e) => { eprintln!("✗ Failed to read skill from {:?}: {}", path, e); had_error = true; } }This still has two calls, but avoids the scenario where
read_propertiessucceeds yetfind_skill_mdfails (which shouldn't happen sinceread_propertiesusesfind_skill_mdinternally). Ideally, expose a single function fromagentskillsthat returns both the resolved path and the properties.
42-48: Useinto_iter()to avoid unnecessary cloning.
skill_itemsis not used after buildingskills_with_loc, so you can consume it withinto_iter()and avoid cloning bothpropsandlocation.Proposed fix
- let skills_with_loc: Vec<_> = skill_items - .iter() - .map(|(props, location)| agentskills::prompt::SkillWithLocation { - properties: props.clone(), - location: location.clone(), - }) + let skills_with_loc: Vec<_> = skill_items + .into_iter() + .map(|(props, location)| agentskills::prompt::SkillWithLocation { + properties: props, + location, + }) .collect();crates/agentskills/src/parser.rs (1)
91-105: Non-string metadata values are silently dropped.The
filter_mapon lines 96-101 discards any metadata entry where either the key or value isn't aString. This means numeric, boolean, or nested values vanish without warning. Consider logging a warning or converting simple scalars (numbers, booleans) to their string representation to be more forgiving.
yordis
force-pushed
the
validate-skills
branch
from
266898e to
0b15db0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/trg/src/agentskills/errors.rs`:
- Around line 8-9: The YamlError variant in errors.rs currently references
serde_yaml::Error; replace the dependency with the actively maintained fork and
use serde_yaml_ng instead. Update Cargo.toml to remove serde_yaml and add
serde_yaml_ng (matching any required features), then change the error variant to
use serde_yaml_ng::Error (keep the YamlError(#[from] ...) signature) and update
any imports/usages that referenced serde_yaml throughout the crate so they point
to serde_yaml_ng.
In `@crates/trg/src/agentskills/validator.rs`:
- Around line 112-121: The code currently treats
skill_path.file_name().and_then(|n| n.to_str()).unwrap_or("") as dir_name which
yields an empty string for paths like "/" and leads to a confusing error;
instead, update the validation around skill_path, checking whether
skill_path.file_name() and its to_str() exist before computing normalized_dir
and comparing to normalized, and if they don't, push a clear error via
errors.push (e.g., mention the skill_path has no final component or is not a
valid directory) rather than comparing to an empty dir_name; adjust the logic
around the symbols skill_path, dir_name, normalized_dir, and the errors.push
call so normalization and comparison only run when a real dir_name is present
and a distinct, actionable error is produced otherwise.
🧹 Nitpick comments (7)
crates/trg/src/agentskills/prompt.rs (1)
16-26:to_promptclones everySkillPropertiesunnecessarily.Since
to_prompt_with_locationonly reads the properties, you can avoid the clone by restructuring to borrow. This matters if the function is called with large skill lists.♻️ Suggested refactor
pub fn to_prompt(skills: &[SkillProperties]) -> String { to_prompt_with_location( &skills .iter() - .map(|p| SkillWithLocation { - properties: p.clone(), + .map(|p| SkillWithLocation { + properties: p, location: None, }) .collect::<Vec<_>>(), ) }This would require changing
SkillWithLocationto hold a reference:pub struct SkillWithLocation<'a> { pub properties: &'a SkillProperties, pub location: Option<String>, }…and updating
to_prompt_with_locationto accept&[SkillWithLocation<'_>]. If the caller into_prompt.rsalways has owned values, you could alternatively provide both owned and borrowed variants.crates/trg/src/agentskills/filesystem.rs (1)
5-11: Consider providing default implementations forfile_nameandparent.Both
RealFSandMemFSimplementfile_nameandparentidentically — they're pure path operations that don't depend on filesystem state. Making them default methods on the trait eliminates duplication and ensures consistent behavior for any future implementors.♻️ Suggested refactor
pub trait FileSystem { fn read_to_string(&self, path: &Path) -> io::Result<String>; fn write(&self, path: &Path, contents: &str) -> io::Result<()>; fn exists(&self, path: &Path) -> bool; - fn file_name(&self, path: &Path) -> Option<String>; - fn parent(&self, path: &Path) -> Option<PathBuf>; + + fn file_name(&self, path: &Path) -> Option<String> { + path.file_name().and_then(|n| n.to_str()).map(|s| s.to_string()) + } + + fn parent(&self, path: &Path) -> Option<PathBuf> { + path.parent().map(|p| p.to_path_buf()) + } }Then remove the duplicate implementations from both
RealFSandMemFS.crates/trg/src/main.rs (1)
1-2: Broad#[allow(dead_code)]suppresses warnings for the entireagentskillsmodule.This can mask genuinely unused code as the crate evolves. Consider narrowing the suppression to specific items (e.g.,
MemFS,write,promptfunctions) that are intentionally public but unused in the binary, or use#[cfg(test)]visibility where appropriate.crates/trg/src/agentskills/mod.rs (1)
10-11: The_with_fssuffix on these re-exports is arguably redundant.The underlying functions (
parser::read_properties,validator::validate_skill) already accept anfs: &impl FileSystemparameter — the filesystem dependency is visible at the call site. The rename adds a naming indirection without new information. Consider re-exporting under the original names unless there's a naming collision to avoid.crates/trg/src/agentskills/parser.rs (1)
87-101: Metadata silently drops non-string keys and values.The
filter_mapat Line 92 silently discards any metadata entry where either the key or value is not a string. If a user writesmetadata: { version: 2 }(integer value), the entry vanishes without feedback. This mirrors theallowed-toolsconcern — consider emitting a warning or validation error for unexpected types rather than silently ignoring them.crates/trg/src/agentskills/validator.rs (2)
130-147:validate_descriptionchecks emptiness on trimmed value but length on untrimmed value.Line 133 trims before the empty check, but Line 138 counts characters on the original
desc. A string like" a "(with padding) counts the spaces toward the 1024 limit. If this is intentional (matching the upstream Python implementation), it's fine — but it's worth a brief inline comment to clarify the intent, sincevalidate_nameexplicitly trims before all its checks.
62-80: No validation forallowed_toolsorlicensefield values.
validate_skillvalidates name, description, and optionally compatibility, but skips any value-level validation forallowed_toolsandlicense. Combined with the parser silently dropping non-stringallowed-toolsvalues (flagged in parser.rs), malformed inputs go undetected end-to-end. Even a simple length check or type assertion here would close that gap.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/trg/src/agentskills/parser.rs`:
- Around line 87-101: The metadata extraction currently silently drops
non-string key/values in the yaml_value -> metadata mapping (the code building
the metadata HashMap), so update that block to detect non-string entries and
emit a clear warning instead of silently ignoring them: when iterating the
mapping (the same place using filter_map) detect cases where either the key or
value is not a serde_yaml::Value::String and call the project's logger (e.g.,
tracing::warn!) with the offending key/value and the parent context (e.g., the
YAML node or skill id) so users see which entries were skipped; keep collecting
valid String->String pairs into the HashMap as before, and if the spec requires
strict validation instead change the behavior to return a parsing error from the
enclosing parse function rather than warning.
🧹 Nitpick comments (1)
crates/trg/src/agentskills/models.rs (1)
49-77: Test fragility note:HashMapordering inmetadata.This test passes because there's only one entry in
metadata. If future tests add multiple metadata keys, the JSON field order within the"metadata"object will be non-deterministic (HashMap iteration order is not guaranteed in Rust), causing flaky assertions against exact strings.Consider using
serde_json::Valuecomparison for tests with multiple metadata entries if they're added later, or switch toBTreeMapfor deterministic ordering.
yordis
changed the title
yordis
force-pushed
the
validate-skills
branch
from
4fe84cb to
f880534
Compare
Add `trg ai skills` subcommands: validate, read-properties, to-prompt. Uses gray_matter for frontmatter parsing, typed error variants, and a FileSystem trait for testability. Also fixes rustsec/audit-check CI action. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
validate-skills
branch
from
f880534 to
5e395d1
Compare
1 participant
Summary
agentskillslibrary crate for parsing, validating, and generating prompts from Agent Skills (SKILL.md) files, fully compatible with the upstream agentskills.io specification and Python reference implementationtrgCLI binary (trg ai skills {validate, read-properties, to-prompt}) with colocated command handlers and dependency injection viaFileSystemtraitTest plan
agentskills) — identical output for validate, read-properties, and to-prompt commandscargo fmt --check,cargo clippy -D warnings,cargo test --workspaceall pass