Feat/dart native skills install#481
Open
Pratikdate wants to merge 11 commits into
Open
Conversation
- 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
Contributor
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesSkills Installation Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant AddCommand
participant GitHub
participant FileSystem
Client->>AddCommand: execute(repoUrl?)
AddCommand->>GitHub: GET /archive/HEAD.zip
GitHub-->>AddCommand: ZIP archive
AddCommand->>FileSystem: extract ZIP to tempDir
AddCommand->>FileSystem: read skills/catalog.json
AddCommand->>AddCommand: validate entries (containsPathTraversal, canonical checks)
AddCommand->>FileSystem: copy skill dirs -> <target>/.agents/skills/
AddCommand->>FileSystem: remove tempDir
AddCommand-->>Client: exit code (0/1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
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
… safety hardening for skills command
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.
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
New Features
Documentation
Tests