feat(stac_cli): add unit testing infrastructure and core command tests#472
Open
Pratikdate wants to merge 12 commits into
Open
feat(stac_cli): add unit testing infrastructure and core command tests#472Pratikdate wants to merge 12 commits into
Pratikdate wants to merge 12 commits into
Conversation
|
|
Contributor
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds two test suites to the STAC CLI package: one verifying FileUtils directory access and file I/O operations, and another verifying that Build, Init, and Deploy commands are properly registered in the CLI runner. ChangesTest Infrastructure for FileUtils and CLI Commands
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![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: 1
🧹 Nitpick comments (1)
packages/stac_cli/test/utils/file_utils_test.dart (1)
10-18: ⚡ Quick winConsider validating directory paths beyond non-empty checks.
The current tests only verify that
homeDirectoryandconfigDirectoryreturn non-empty strings. They don't validate that these are actual valid directory paths or that they exist. While acceptable for initial test infrastructure, consider adding path validity checks in future iterations.📁 Example enhancement to validate paths
test('homeDirectory returns a non-empty string on this OS', () { final home = FileUtils.homeDirectory; expect(home, isNotEmpty); + expect(Directory(home).existsSync(), isTrue, + reason: 'homeDirectory should point to an existing directory'); }); test('configDirectory path is generated', () { final config = FileUtils.configDirectory; expect(config, isNotEmpty); + expect(path.isAbsolute(config), isTrue, + reason: 'configDirectory should be an absolute path'); });🤖 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/utils/file_utils_test.dart` around lines 10 - 18, The tests only assert non-empty strings; update the tests for FileUtils.homeDirectory and FileUtils.configDirectory to also validate that the returned paths point to existing directories by constructing a Directory from each path and asserting Directory(path).exists() (and optionally Directory(path).stat() indicates a directory) so the tests ensure the paths are valid and are directories rather than merely non-empty strings.
🤖 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/test/commands/cli_commands_test.dart`:
- Around line 13-26: The tests set environment variables in the setUp using
configureEnvironment but never clear them, so add a tearDown block that resets
or clears those keys after each test; implement tearDown(() {
configureEnvironment({'STAC_BASE_API_URL': null, 'STAC_GOOGLE_CLIENT_ID': null,
'STAC_FIREBASE_API_KEY': null}); }) or call your existing environment-reset
helper to remove those variables so configureEnvironment and setUp no longer
leak state between tests.
---
Nitpick comments:
In `@packages/stac_cli/test/utils/file_utils_test.dart`:
- Around line 10-18: The tests only assert non-empty strings; update the tests
for FileUtils.homeDirectory and FileUtils.configDirectory to also validate that
the returned paths point to existing directories by constructing a Directory
from each path and asserting Directory(path).exists() (and optionally
Directory(path).stat() indicates a directory) so the tests ensure the paths are
valid and are directories rather than merely non-empty strings.
🪄 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: 4105edd3-5e6e-4cc4-b1e5-44517536bfa9
📒 Files selected for processing (2)
packages/stac_cli/test/commands/cli_commands_test.dartpackages/stac_cli/test/utils/file_utils_test.dart
Member
|
Hey @Pratikdate!! Can you please add all the proper test cases for the cli? |
Author
|
Yes , I do update |
- 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/init_command.dart`:
- Around line 93-96: The skills install block currently calls
AddCommand().execute() which runs against Directory.current instead of the
intended targetDir and ignores non-zero exit codes; change the call to execute
the AddCommand within the targetDir (either by passing targetDir into AddCommand
or setting the working directory before calling AddCommand().execute()) and
capture its return/exit code, logging an error via ConsoleLogger.error and
aborting or returning non-zero when the installer fails so success messages
printed later are conditional on a successful install. Ensure you update
references to shouldInstallSkills / AddCommand().execute() to use the
targetDir-aware invocation and explicitly check the result to handle failures.
In `@packages/stac_cli/lib/src/commands/skills/add_command.dart`:
- Around line 56-89: The tempDir created with
Directory.systemTemp.createTemp('stac_skills_') is only cleaned up on the
success path; wrap the download/extract/validation logic in a try/finally
(declare tempDir before the try) and in the finally always call await
tempDir.delete(recursive: true) to remove the temporary files even on early
returns or exceptions; update the function containing this logic (e.g., the add
command function that uses tempDir, zipFile, extractDir and catalogFile) so all
exit paths go through the finally block and keep existing return codes and error
logging behavior.
- Around line 101-116: The code uses untrusted catalog values (skillName,
skillPath) to build sourceSkillDir and targetSkillDir and perform filesystem
ops; validate and canonicalize both values before any filesystem access: reject
empty, absolute paths, or values containing path traversal or path separators
for skillName (enforce a single safe token), normalize skillPath and compute its
absolute/resolved path and ensure it is a descendant of repoRoot (use resolved
canonical paths) before reading/copying, and ensure targetSkillDir resolved path
is inside targetAgentsDir before deleting or writing; if checks fail, log a
warning and skip the entry. Use the existing variables skillName, skillPath,
sourceSkillDir, targetSkillDir, repoRoot, and targetAgentsDir when adding these
validations.
In `@packages/stac_cli/test/commands/cli_commands_test.dart`:
- Around line 64-65: The test performs a direct cast with "as SkillsCommand"
which can throw before assertions run; change it to first assert that
runner.commands['skills'] is not null and is a SkillsCommand (e.g.
expect(runner.commands['skills'], isNotNull) and
expect(runner.commands['skills'], isA<SkillsCommand>())), then safely cast into
a local variable (e.g. final skillsCommand = runner.commands['skills'] as
SkillsCommand) and finally assert that skillsCommand.subcommands['add']
isNotNull; reference the runner.commands map, the SkillsCommand type, and the
subcommands['add'] lookup when making these changes.
🪄 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: 58b6229c-c406-4453-9120-352eca8263d2
⛔ Files ignored due to path filters (1)
packages/stac_cli/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
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.dart
✅ Files skipped from review due to trivial changes (1)
- docs/skills.mdx
Comment on lines
+93
to
+96
| if (shouldInstallSkills) { | ||
| ConsoleLogger.info('Installing skills...'); | ||
| await AddCommand().execute(); | ||
| } |
Contributor
There was a problem hiding this comment.
Install location and failure handling are disconnected from init --directory.
At Line 95, AddCommand().execute() runs against Directory.current, not targetDir, so skills can be installed in a different project than the one being initialized. Also, non-zero exit from execute() is ignored, yet success is still printed later.
Pass targetDir into the skill installer (or run with that working dir) and handle non-zero exit codes explicitly.
🤖 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/lib/src/commands/init_command.dart` around lines 93 - 96,
The skills install block currently calls AddCommand().execute() which runs
against Directory.current instead of the intended targetDir and ignores non-zero
exit codes; change the call to execute the AddCommand within the targetDir
(either by passing targetDir into AddCommand or setting the working directory
before calling AddCommand().execute()) and capture its return/exit code, logging
an error via ConsoleLogger.error and aborting or returning non-zero when the
installer fails so success messages printed later are conditional on a
successful install. Ensure you update references to shouldInstallSkills /
AddCommand().execute() to use the targetDir-aware invocation and explicitly
check the result to handle failures.
Comment on lines
+101
to
+116
| for (final skill in catalog) { | ||
| final skillName = skill['name']; | ||
| final skillPath = skill['path']; | ||
|
|
||
| if (skillName == null || skillPath == null) continue; | ||
|
|
||
| final sourceSkillDir = Directory(path.join(repoRoot.path, skillPath)); | ||
| if (!await sourceSkillDir.exists()) { | ||
| ConsoleLogger.warning('Skill directory $skillPath not found, skipping.'); | ||
| continue; | ||
| } | ||
|
|
||
| final targetSkillDir = Directory(path.join(targetAgentsDir.path, skillName)); | ||
| if (await targetSkillDir.exists()) { | ||
| await targetSkillDir.delete(recursive: true); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate and constrain skillName/skillPath before filesystem operations.
At Line 107 and Line 113-116, untrusted values from catalog.json are used to build source/target paths and to recursively delete directories. A crafted name or path (for example with .. or absolute paths) can escape the repo/.agents/skills boundaries and overwrite/delete arbitrary local directories.
Use canonical paths and enforce boundary checks before read/copy/delete.
Suggested fix
+ String _canonical(String p) => Directory(p).resolveSymbolicLinksSync();
+
for (final skill in catalog) {
- final skillName = skill['name'];
- final skillPath = skill['path'];
+ final skillName = skill['name'] as String?;
+ final skillPath = skill['path'] as String?;
if (skillName == null || skillPath == null) continue;
+ if (path.isAbsolute(skillName) || path.isAbsolute(skillPath)) {
+ ConsoleLogger.warning('Invalid absolute path in catalog, skipping.');
+ continue;
+ }
final sourceSkillDir = Directory(path.join(repoRoot.path, skillPath));
+ final canonicalSource = _canonical(sourceSkillDir.path);
+ final canonicalRepoRoot = _canonical(repoRoot.path);
+ if (!path.isWithin(canonicalRepoRoot, canonicalSource)) {
+ ConsoleLogger.warning('Skill path escapes repository root, skipping.');
+ continue;
+ }
if (!await sourceSkillDir.exists()) {
ConsoleLogger.warning('Skill directory $skillPath not found, skipping.');
continue;
}
final targetSkillDir = Directory(path.join(targetAgentsDir.path, skillName));
+ final canonicalTargetBase = _canonical(targetAgentsDir.path);
+ final canonicalTarget = _canonical(targetSkillDir.path);
+ if (!path.isWithin(canonicalTargetBase, canonicalTarget)) {
+ ConsoleLogger.warning('Skill name escapes target directory, skipping.');
+ continue;
+ }
if (await targetSkillDir.exists()) {
await targetSkillDir.delete(recursive: true);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (final skill in catalog) { | |
| final skillName = skill['name']; | |
| final skillPath = skill['path']; | |
| if (skillName == null || skillPath == null) continue; | |
| final sourceSkillDir = Directory(path.join(repoRoot.path, skillPath)); | |
| if (!await sourceSkillDir.exists()) { | |
| ConsoleLogger.warning('Skill directory $skillPath not found, skipping.'); | |
| continue; | |
| } | |
| final targetSkillDir = Directory(path.join(targetAgentsDir.path, skillName)); | |
| if (await targetSkillDir.exists()) { | |
| await targetSkillDir.delete(recursive: true); | |
| } | |
| String _canonical(String p) => Directory(p).resolveSymbolicLinksSync(); | |
| for (final skill in catalog) { | |
| final skillName = skill['name'] as String?; | |
| final skillPath = skill['path'] as String?; | |
| if (skillName == null || skillPath == null) continue; | |
| if (path.isAbsolute(skillName) || path.isAbsolute(skillPath)) { | |
| ConsoleLogger.warning('Invalid absolute path in catalog, skipping.'); | |
| continue; | |
| } | |
| final sourceSkillDir = Directory(path.join(repoRoot.path, skillPath)); | |
| final canonicalSource = _canonical(sourceSkillDir.path); | |
| final canonicalRepoRoot = _canonical(repoRoot.path); | |
| if (!path.isWithin(canonicalRepoRoot, canonicalSource)) { | |
| ConsoleLogger.warning('Skill path escapes repository root, skipping.'); | |
| continue; | |
| } | |
| if (!await sourceSkillDir.exists()) { | |
| ConsoleLogger.warning('Skill directory $skillPath not found, skipping.'); | |
| continue; | |
| } | |
| final targetSkillDir = Directory(path.join(targetAgentsDir.path, skillName)); | |
| final canonicalTargetBase = _canonical(targetAgentsDir.path); | |
| final canonicalTarget = _canonical(targetSkillDir.path); | |
| if (!path.isWithin(canonicalTargetBase, canonicalTarget)) { | |
| ConsoleLogger.warning('Skill name escapes target directory, skipping.'); | |
| continue; | |
| } | |
| if (await targetSkillDir.exists()) { | |
| await targetSkillDir.delete(recursive: true); | |
| } |
🤖 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/lib/src/commands/skills/add_command.dart` around lines 101
- 116, The code uses untrusted catalog values (skillName, skillPath) to build
sourceSkillDir and targetSkillDir and perform filesystem ops; validate and
canonicalize both values before any filesystem access: reject empty, absolute
paths, or values containing path traversal or path separators for skillName
(enforce a single safe token), normalize skillPath and compute its
absolute/resolved path and ensure it is a descendant of repoRoot (use resolved
canonical paths) before reading/copying, and ensure targetSkillDir resolved path
is inside targetAgentsDir before deleting or writing; if checks fail, log a
warning and skip the entry. Use the existing variables skillName, skillPath,
sourceSkillDir, targetSkillDir, repoRoot, and targetAgentsDir when adding these
validations.
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
3 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 adds a comprehensive unit testing layer to the
stac_clipackage and integrates it into the monorepo's CI pipeline. Key changes include:test/directory withinpackages/stac_clito manage command and utility tests.STAC_BASE_API_URLandSTAC_GOOGLE_CLIENT_ID) during tests to prevent initialization crashes without real API keys.BuildCommand,InitCommand, andDeployCommandto verify registration, name, and descriptions.FileUtilsto validate file system operations (create, read, delete) with automated sandbox cleanup.testcommand to the rootmelos.yaml..github/workflows/all_plugins.yamlto include a global test step for all packages.Related Issues
Closes # (Insert issue number if applicable)
Type of Change
Summary by CodeRabbit