Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (8.10%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
- Coverage 59.06% 52.86% -6.21%
==========================================
Files 140 143 +3
Lines 9656 10923 +1267
==========================================
+ Hits 5703 5774 +71
- Misses 3462 4640 +1178
- Partials 491 509 +18
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a --scope concept to CLI operations (install/remove/list/get) intended to target different configuration layers (user/project/local), and updates platform interfaces/tests/docs accordingly.
Changes:
- Introduces
cli.ScopeplusDetermineScope()and wires--scopeinto several commands. - Updates
cli.Platforminterface signatures to acceptscopeand propagates call-site changes across commands and tests. - Updates generated reference docs to include the new
--scopeflag on affected commands.
Reviewed changes
Copilot reviewed 54 out of 55 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/git/git.go | Adds IsRepo helper for git work-tree detection. |
| internal/cli/platform.go | Introduces Scope, updates Platform interface to include scope, adds DetermineScope/prompting. |
| internal/cli/platform_test.go | Updates adapter tests for new InstallCommand(..., scope) signature. |
| go.mod | Promotes go-fuzzyfinder to a direct dependency. |
| go.sum | Updates dependency checksums (incl. go-cmp). |
| cmd/aix/commands/status.go | Uses ScopeDefault for status collection list calls. |
| cmd/aix/commands/status_test.go | Updates mock platform methods for new scoped signatures. |
| cmd/aix/commands/skill/show.go | Passes parsed scope to GetSkill. |
| cmd/aix/commands/skill/edit.go | Passes parsed scope to GetSkill lookup. |
| cmd/aix/commands/skill/list.go | Adds scope propagation to ListSkills output paths. |
| cmd/aix/commands/skill/install.go | Adds --scope flag and uses DetermineScope() for install flows. |
| cmd/aix/commands/skill/install_test.go | Updates install helper calls to include scope. |
| cmd/aix/commands/skill/remove.go | Adds --scope flag and uses DetermineScope() for uninstall. |
| cmd/aix/commands/skill/remove_test.go | Updates mock platform methods for new scoped signatures. |
| cmd/aix/commands/mcp/show.go | Passes parsed scope to GetMCP. |
| cmd/aix/commands/mcp/list.go | Adds scope propagation to ListMCP output paths. |
| cmd/aix/commands/mcp/list_test.go | Updates list tests for scoped APIs and masking assertions. |
| cmd/aix/commands/mcp/install.go | Adds --scope flag and uses DetermineScope() for install flows. |
| cmd/aix/commands/mcp/install_test.go | Updates install helper calls to include scope. |
| cmd/aix/commands/mcp/add.go | Adds --scope flag and passes scope to AddMCP. |
| cmd/aix/commands/mcp/remove.go | Adds --scope flag and uses DetermineScope() for removal flows. |
| cmd/aix/commands/mcp/remove_test.go | Updates mock platform methods for new scoped signatures. |
| cmd/aix/commands/mcp/enable.go | Uses ScopeDefault when checking MCP existence. |
| cmd/aix/commands/mcp/enable_test.go | Updates mock platform methods for new scoped signatures. |
| cmd/aix/commands/mcp/mock_platform_test.go | Updates shared MCP mock platform for new scoped signatures. |
| cmd/aix/commands/command/show.go | Passes parsed scope to GetCommand. |
| cmd/aix/commands/command/list.go | Adds scope propagation to ListCommands output paths. |
| cmd/aix/commands/command/install.go | Adds --scope flag and uses DetermineScope() for install flows. |
| cmd/aix/commands/command/install_test.go | Updates tests for new scoped APIs and adds directory/path helper tests. |
| cmd/aix/commands/command/edit.go | Adds --scope flag and passes scope into edit lookup. |
| cmd/aix/commands/command/edit_test.go | Updates edit tests to include scope param. |
| cmd/aix/commands/command/remove.go | Adds --scope flag and uses DetermineScope() for uninstall. |
| cmd/aix/commands/command/mock_platform_test.go | Updates command mock platform for new scoped signatures. |
| cmd/aix/commands/agent/show.go | Passes parsed scope to GetAgent. |
| cmd/aix/commands/agent/show_test.go | Updates agent tests for scoped GetAgent. |
| cmd/aix/commands/agent/list.go | Adds scope propagation to ListAgents output paths. |
| cmd/aix/commands/agent/list_test.go | Updates list tests for scoped APIs. |
| cmd/aix/commands/agent/install.go | Adds --scope flag and uses DetermineScope() for install flows. |
| cmd/aix/commands/agent/remove.go | Adds --scope flag and uses DetermineScope() for uninstall. |
| cmd/aix/commands/agent/remove_test.go | Updates agent remove tests for scoped APIs. |
| cmd/aix/commands/agent/edit.go | Passes parsed scope to GetAgent lookup. |
| cmd/aix/commands/agent/mock_platform_test.go | Updates agent mock platform for new scoped signatures. |
| cmd/aix/commands/flags/flags.go | Adds global scopeFlag plumbing and AddScopeFlag. |
| aix-docs/content/docs/reference/aix_skill_remove.md | Documents new --scope flag for skill remove. |
| aix-docs/content/docs/reference/aix_skill_install.md | Documents new --scope flag and --all-from-repo example. |
| aix-docs/content/docs/reference/aix_mcp_remove.md | Documents new --scope flag for mcp remove. |
| aix-docs/content/docs/reference/aix_mcp_install.md | Documents new --scope flag and --all-from-repo example. |
| aix-docs/content/docs/reference/aix_mcp_add.md | Documents new --scope flag for mcp add. |
| aix-docs/content/docs/reference/aix_command_remove.md | Documents new --scope flag for command remove. |
| aix-docs/content/docs/reference/aix_command_list.md | Documents new --scope flag for command list. |
| aix-docs/content/docs/reference/aix_command_install.md | Documents new --scope flag and --all-from-repo example. |
| aix-docs/content/docs/reference/aix_command_edit.md | Documents new --scope flag for command edit. |
| aix-docs/content/docs/reference/aix_agent_remove.md | Documents new --scope flag for agent remove. |
| aix-docs/content/docs/reference/aix_agent_list.md | Documents new --scope flag for agent list. |
| aix-docs/content/docs/reference/aix_agent_install.md | Documents new --scope flag and --all-from-repo example. |
| if errors.Is(err, errors.New("no command file found")) { | ||
| t.Error("directory logic failed to find command.md") |
There was a problem hiding this comment.
This assertion will never work as written: errors.Is(err, errors.New("no command file found")) compares against a newly created error value, so it won’t match. Prefer checking strings.Contains(err.Error(), "no command file found") or introduce/compare against a shared sentinel error for the "no command file found" case.
| func TestInstallFromLocal_InvalidCommand(t *testing.T) { | ||
| // Create a temp directory with invalid command file (missing name) | ||
| func Test_installFromLocal_InvalidCommand(t *testing.T) { | ||
| t.Skip("Skipping due to parser panic on invalid content") |
There was a problem hiding this comment.
This test is currently skipped due to a parser panic. Skipping leaves the invalid-command behavior unverified and can hide regressions. Prefer fixing the underlying parser to return an error (not panic) for invalid content, then re-enable the test.
cmd/aix/commands/mcp/list_test.go
Outdated
| "API_KEY": "sk-secret-key-value", | ||
| }, |
There was a problem hiding this comment.
Avoid using realistic secret-like values (e.g., sk-...) in test fixtures. This can trigger secret scanners and create unnecessary operational noise. Prefer obviously fake placeholders (e.g., dummy-api-key / test-key-1234) that still exercise masking behavior.
| if errors.Is(err, errors.New("no command file found")) { | ||
| t.Error("directory logic failed to find .md file") |
There was a problem hiding this comment.
Same issue here: errors.Is(err, errors.New("no command file found")) will never match because it compares against a fresh error instance. Use substring matching or a shared sentinel error value instead.
| func (a *claudeAdapter) InstallSkill(skill any, scope Scope) error { | ||
| s, ok := skill.(*claude.Skill) | ||
| if !ok { | ||
| return errors.Newf("expected *claude.Skill, got %T", skill) | ||
| } | ||
| return errors.Wrap(a.p.InstallSkill(s), "installing skill to Claude") | ||
| } |
There was a problem hiding this comment.
The new scope parameter is ignored: this method always uses the adapter’s preconstructed ClaudePlatform (defaulting to user scope) and does not map scope to Claude/OpenCode/Gemini path scoping. As a result, --scope/DetermineScope won’t actually change where skills are installed. Consider either (1) constructing/caching per-scope platform instances (e.g., via claude.NewClaudePlatform(claude.WithScope(...), claude.WithProjectRoot(...))) and dispatching based on scope, or (2) moving scope handling into the underlying platform APIs and passing it through, but avoid accepting a scope in the CLI interface if it has no effect.
| if requested != "" { | ||
| return ParseScope(requested), nil | ||
| } |
There was a problem hiding this comment.
When the user explicitly provides --scope, invalid values are silently treated as ScopeDefault via ParseScope. This is risky because a typo could cause edits to land in the default/merged layer instead of the intended one. Suggest validating the input: if requested != "" and ParseScope(requested) == ScopeDefault, return an error listing the supported values.
| // IsRepo returns true if the given path is within a git repository. | ||
| func IsRepo(path string) bool { | ||
| // We use git command directly to avoid duplicating logic. | ||
| // This helper is used by DetermineScope. | ||
| cmd := exec.Command("git", "-C", path, "rev-parse", "--is-inside-work-tree") | ||
| return cmd.Run() == nil | ||
| } |
There was a problem hiding this comment.
IsRepo duplicates the new internal/git.IsRepo helper (same implementation) and pulls os/exec into this package solely for repo detection. To avoid divergence and keep git-related logic in one place, prefer calling git.IsRepo here and remove one of the duplicate helpers.
| func IsRepo(path string) bool { | ||
| cmd := exec.Command("git", "-C", path, "rev-parse", "--is-inside-work-tree") | ||
| return cmd.Run() == nil | ||
| } |
There was a problem hiding this comment.
This adds git.IsRepo, but it appears unused while an identical cli.IsRepo helper was also introduced. Either switch callers to use this helper (to centralize repo detection) or remove it to avoid duplicated logic drifting over time.
Introduce the Scope enum (User, Project, Local, Managed) and update the Platform interface to accept a Scope parameter for all mutation operations (Install, Uninstall, Add, Remove). All platform adapters and command callers have been updated to satisfy the new signature, defaulting to ScopeUser for now. Mocks in tests have also been updated to ensure compilation and test parity.
Add --scope flag to all resource commands (install, remove, list, show, edit). Implement DetermineScope logic to resolve scope from flag, interactive prompt, or context (Git). Update all command implementations to respect the resolved scope. Refactor tests to match new command signatures and fix pre-existing test fragilities.
Adds a helper function to check if a file is ignored by git using 'git check-ignore'.
Adds stretchr/testify for improved testing assertions.
- Implements nested configuration for Claude MCP local scope. - Updates Platform interface with IsLocalConfigIgnored. - Adds collision detection for Claude agents, skills, and commands. - Refactors mocks to use mockery generation. - Updates tests to reflect interface changes and new features. - Fixes wrapcheck lint errors. - Updates reference documentation.
thoreinstein
force-pushed
the
epic/aix-dpl-claude-scopes
branch
from
cdfb771 to
02bc241
Compare
1 participant
Description
Fixes # (issue)
Type of change
How Has This Been Tested?
go test ./...)Checklist:
go test ./...)golangci-lint run) and fixed any issuesFurther comments