Feat/dart native skills install#481
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesSkills installation feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
packages/stac_cli/test/commands/cli_commands_test.dart (1)
76-87: 🏗️ Heavy liftBehavior-critical
AddCommandpaths still need focused tests.Current tests validate metadata only. Please add cases for traversal rejection, invalid catalog entries, and temp-dir cleanup in
finallyso regressions in install safety are caught.🤖 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 `@packages/stac_cli/test/commands/cli_commands_test.dart` around lines 76 - 87, Add focused unit tests in packages/stac_cli/test/commands/cli_commands_test.dart for AddCommand that exercise its runtime behavior (not just metadata): add a test that simulates filesystem traversal attempting to escape the target directory and assert the command rejects it (reference AddCommand's traversal/validation logic), add a test that supplies invalid/malformed catalog entries and asserts the command fails with the expected error handling path (reference the parsing/validation routine used by AddCommand), and add a test that forces an error during install to verify the temporary directory is always deleted in the finally/cleanup block (assert temp-dir does not remain after failure). Ensure each test constructs AddCommand, invokes the same execution method used by the CLI (e.g., run/execute), stubs/mocks IO as needed, and asserts both the failure mode and that cleanup code ran.
🤖 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 `@packages/stac_cli/lib/src/commands/skills/add_command.dart`:
- Around line 111-120: The loop that reads catalog entries uses skill['name']
and skill['path'] without type-checking and then casts to String for
_containsPathTraversal, which can throw on non-string values; update the loop in
add_command.dart to first check that skillName and skillPath are Strings (e.g.,
skill['name'] is String and skill['path'] is String) before calling
_containsPathTraversal or casting, and if either is not a String skip the entry
(log a warning via ConsoleLogger.warning mentioning the malformed entry) so a
single bad catalog item cannot abort the install flow.
- Around line 130-137: Replace the fragile prefix check on canonicalized paths
(sourceCanonical.startsWith(repoRootCanonical)) with a robust containment test
using path.isWithin(repoRootCanonical, sourceCanonical) or equality check to
ensure the source is either equal to or strictly inside the repo root; update
the same logic where target/skill paths are validated. In addition, harden
_copyDirectory to avoid symlink traversal by calling Directory.list(...,
followLinks: false) and explicitly handling Link/File/Directory entities
(resolving or skipping links) so you never recursively follow symlinks that
could escape repo/target boundaries; ensure any copied path is re-canonicalized
and validated with path.isWithin before writing to the destination.
---
Nitpick comments:
In `@packages/stac_cli/test/commands/cli_commands_test.dart`:
- Around line 76-87: Add focused unit tests in
packages/stac_cli/test/commands/cli_commands_test.dart for AddCommand that
exercise its runtime behavior (not just metadata): add a test that simulates
filesystem traversal attempting to escape the target directory and assert the
command rejects it (reference AddCommand's traversal/validation logic), add a
test that supplies invalid/malformed catalog entries and asserts the command
fails with the expected error handling path (reference the parsing/validation
routine used by AddCommand), and add a test that forces an error during install
to verify the temporary directory is always deleted in the finally/cleanup block
(assert temp-dir does not remain after failure). Ensure each test constructs
AddCommand, invokes the same execution method used by the CLI (e.g.,
run/execute), stubs/mocks IO as needed, and asserts both the failure mode and
that cleanup code ran.
🪄 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: c18dbf50-3131-4008-9aa7-9e634c67460b
⛔ Files ignored due to path filters (1)
packages/stac_cli/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
docs/skills.mdxpackages/stac_cli/bin/stac_cli.dartpackages/stac_cli/lib/src/commands/init_command.dartpackages/stac_cli/lib/src/commands/skills/add_command.dartpackages/stac_cli/lib/src/commands/skills_command.dartpackages/stac_cli/pubspec.yamlpackages/stac_cli/test/commands/cli_commands_test.dartpackages/stac_cli/test/utils/file_utils_test.dart
|
Hey @Pratikdate |
- Add SkillsCommand with 'stac skills add' subcommand - AddCommand fetches repo ZIP, parses skills/catalog.json, and copies skill dirs into .agents/skills/ — no Node/npm required - Prompt users in 'stac init' to optionally install agent skills - Register SkillsCommand in the CLI runner - Add archive: ^4.0.9 dependency for ZIP extraction - Update docs/skills.mdx to show Dart-native path first, keeping npx as an alternative - Add tests for SkillsCommand and AddCommand (7 tests pass) Closes StacDev#480
- Move tempDir cleanup into finally block so temp files are always removed even on early return or exception - Validate skillName and skillPath from catalog.json to prevent path-traversal attacks (reject '..', absolute paths, backslashes, and enforce canonical boundary checks) - Accept optional targetDirectory in AddCommand so stac init installs skills into the correct project directory, not Directory.current - Handle non-zero exit from AddCommand in init with a warning instead of silently printing success - Assert non-null and correct type before casting in test to give cleaner failure output
… safety hardening for skills command
Pratikdate
force-pushed
the
feat/dart-native-skills-install
branch
from
35a4a5f to
617e321
Compare
|
Hii @Potatomonsta , |
Potatomonsta
left a comment
Potatomonsta
left a comment
There was a problem hiding this comment.
@Pratikdate
Could you please review my PR comments? It looks like the add command is trying to access skills/catalog.json before the repository extraction finishes. The extraction call appears to be asynchronous and not awaited, which may be causing the failure.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Description
This PR implements a Dart/Flutter-native path for installing Stac skills without requiring
Node,npm, ornpx, addressing issue #479 (or the relevant issue ID). It adds a newstac skills addCLI command and integrates it into thestac initflow.Key Changes
stac skills add):dioandarchive.skills/catalog.jsonfrom the repository root to find registered skills..agents/skillsrelative to the target directory.skillNameandskillPathagainst patterns like.., absolute paths, and backslashes, ensuring all extractions stay within boundaries.stac initnow prompts the user to optionally install Stac agent skills as part of the setup flow.AddCommandattributes.docs/skills.mdxto prioritize the native CLI pathway overnpx.Related Issues
Closes #479 (or insert the correct issue number here)
Type of Change
Summary by CodeRabbit
stac skills addcommand to install Stac AI agent skills into your project.stac skills add, including an alternativenpx-based command.