aitools: add --scope flag, deprecate --project/--global#5234
aitools: add --scope flag, deprecate --project/--global#5234jamesbroadhead wants to merge 2 commits into
Conversation
Approval status: pending
|
526cd9c to
a7e3d03
Compare
a7e3d03 to
37cf784
Compare
8f0319a to
7666312
Compare
|
👋 @simonfaltum — Claude here on James's behalf. #4917 merged earlier today, so this PR is now stacked on Ready for a re-review whenever you've got a moment. (comment posted by Claude) |
Teaches list to render as a structured {release, skills[...], summary{}}
document when --output json is passed. Text rendering is unchanged.
Stacked on jb/aitools-interface (#5234). Original branch was rebased
onto current main + that PR's tip; layout drift from #4917's pre-merge
shape was reconciled (cmd/aitools/* paths, unexported listSkillsFn,
3-value installer.GetSkillsRef signature).
Co-authored-by: Isaac
simonfaltum
left a comment
There was a problem hiding this comment.
I found a couple of should-fix items and one test gap. The affected package tests pass locally: go test ./cmd/aitools.
| // list: empty scope = show both. Both flags set is equivalent. | ||
| var scope string | ||
| if projectFlag { | ||
| switch { | ||
| case projectFlag && !globalFlag: | ||
| scope = installer.ScopeProject | ||
| } else if globalFlag { | ||
| case globalFlag && !projectFlag: | ||
| scope = installer.ScopeGlobal | ||
| } |
There was a problem hiding this comment.
Should-fix: this changes list --project --global from an error on main into a successful "both scopes" invocation. Since --scope=both is the new explicit spelling, I would keep the old validation for the deprecated flags so accidental invalid invocations don't start succeeding silently.
| case scopeBoth: | ||
| if !allowBoth { | ||
| return false, false, errors.New("--scope=both is not supported for this command; use 'project' or 'global'") | ||
| } | ||
| return true, true, nil | ||
| default: | ||
| return false, false, fmt.Errorf("invalid --scope %q: must be one of project, global, both", scopeFlag) |
There was a problem hiding this comment.
Should-fix: for commands that pass allowBoth=false (install and uninstall), an invalid value like --scope=all still reports both as a valid option, but retrying with --scope=both immediately errors. Can this branch on allowBoth and say project or global for those commands?
| return err | ||
| } | ||
|
|
||
| projectFlag, globalFlag, err := parseScopeFlag(scopeFlag, projectFlag, globalFlag, true) |
There was a problem hiding this comment.
Test gap: parseScopeFlag is covered, and install/list have command-level tests, but update/uninstall don't exercise the new Cobra flag registration and RunE wiring. A small table test for --scope=project|global|both, invalid value, and legacy flag conflict would protect these paths.
Adds --scope=project|global|both as the single source of truth for scope selection on install/update/uninstall/list. Keeps --project and --global working via cobra.Deprecated (hidden from --help, emit stderr warning). Mixing --scope with --project/--global is rejected up front. Also documents the --agents auto-detect contract in install --help. Stacked-on-#4917 base rewritten onto current main (16 noisy merge/cherry-pick commits collapsed into this single change; underlying diff unchanged at 9 files / +219 / -13). Co-authored-by: Isaac
- Restore the pre-refactor validation that combining --project and --global on `list` is an error, not a silent equivalent of --scope=both. Moved the check into parseScopeFlag itself so it applies uniformly across commands. - For commands that pass allowBoth=false (install, uninstall), the invalid-value error no longer mentions "both" as a valid option. - Added TestUpdateScopeFlag and TestUninstallScopeFlag covering the new Cobra wiring (--scope=global/both, invalid value, legacy-flag conflict, and both-deprecated-flags-together cases). Introduced updateSkillsFn and uninstallSkillsFn package-level test-injection vars mirroring the existing installSkillsForAgentsFn pattern in install.go. Co-authored-by: Isaac
7666312 to
24d5351
Compare
|
👋 @simonfaltum — Claude here on James's behalf. All three of your should-fix items are addressed in
(comment posted by Claude) |
Teaches list to render as a structured {release, skills[...], summary{}}
document when --output json is passed. Text rendering is unchanged.
Stacked on jb/aitools-interface (#5234). Original branch was rebased
onto current main + that PR's tip; layout drift from #4917's pre-merge
shape was reconciled (cmd/aitools/* paths, unexported listSkillsFn,
3-value installer.GetSkillsRef signature).
Co-authored-by: Isaac
simonfaltum
left a comment
There was a problem hiding this comment.
Fresh re-review after the fixup. The original findings look addressed, but I found one new update regression plus a couple of follow-ups around the deprecated flag UX.
| if scopeFlag == "" { | ||
| // Preserve the pre-refactor behavior: combining the two deprecated flags | ||
| // is always wrong, regardless of allowBoth. Users who want both scopes | ||
| // should use --scope=both (where supported). | ||
| if projectFlag && globalFlag { | ||
| return false, false, errors.New("cannot use --global and --project together") | ||
| } | ||
| return projectFlag, globalFlag, nil |
There was a problem hiding this comment.
Should-fix: this creates a new regression for aitools update --project --global. On main, resolveScopeForUpdate(ctx, true, true, ...) is the supported "update both scopes" path, and the existing resolver tests still cover that behavior. With this shared rejection in front of resolveScopeForUpdate, existing update scripts using the two deprecated flags now fail, even though the PR says the deprecated flags continue to function. Can we preserve legacy both-flag behavior for update until those flags are removed, while still rejecting it for install/list/uninstall?
| // markScopeBoolsDeprecated hides --project and --global from help and emits a | ||
| // stderr warning pointing at --scope when they're used. The booleans are kept | ||
| // so existing scripts and the experimental backward-compat aliases keep | ||
| // working through the next release. | ||
| func markScopeBoolsDeprecated(cmd *cobra.Command) { | ||
| cmd.Flags().Lookup("project").Deprecated = "use --scope=project" | ||
| cmd.Flags().Lookup("project").Hidden = true | ||
| cmd.Flags().Lookup("global").Deprecated = "use --scope=global" | ||
| cmd.Flags().Lookup("global").Hidden = true | ||
| } |
There was a problem hiding this comment.
Since this hides --project and --global, the remaining user-facing guidance should stop recommending those hidden deprecated flags. The auto-detect and not-installed errors below still say things like use --global, --project, or both flags, install --project, install --global, and Run without --project. Those should point users at --scope=project, --scope=global, or --scope=both instead, so the error messages match the new public surface.
| } | ||
| // For list: no flag = show both scopes (empty string). | ||
|
|
||
| // list: empty scope = show both. Both flags set is equivalent. |
There was a problem hiding this comment.
Nit: this comment is stale after the fixup. Both deprecated flags are now rejected by parseScopeFlag; the "both" behavior is only the empty/default scope or explicit --scope=both.
Summary
Split out from #4917. While that PR keeps responsibility for moving the aitools skills-management surface out of
experimental/, this PR makes the user-facing interface changes that should land at the same moment:--scope=project|globalflag oninstall/update/uninstall/list, with--scope=bothaccepted byupdateandlist.--projectand--globalare marked deprecated via cobra'sDeprecatedproperty: hidden from--help, emit a stderr deprecation warning when used, continue to function so existing scripts don't break. They're slated for removal in a later release.--scopecombined with--project/--globalis rejected up front with an actionable error.install's--helpnow documents the non-interactive--agentsauto-detect contract so callers know what gets picked.Stacked on #4917. Base will rebase to
mainonce that lands. Splitting because (a) #4917 is otherwise a pure file move and reviewers asked to keep it that way, and (b) the interface change has its own product question (boolean pair vs. enum) worth landing as a discrete unit.Why land this with the rename
aitools is being declared a stable top-level surface in #4917. This is the cheapest moment to fix the two-boolean shape before external scripts depend on it. An enum is also better for agent-driven invocations than two booleans with implicit precedence:
--scope=project|global|bothis one flag with valid values, not two flags with order-dependent semantics.Surface
Test plan
databricks aitools install --scope=projectand--scope=globalgo to the right destinationdatabricks aitools install --scope=botherrors with a clear messagedatabricks aitools install --projectstill works and prints the deprecation warning to stderrdatabricks aitools install --scope=global --projecterrors with the conflict messagedatabricks aitools list --scope=bothshows both scopes (equivalent to no flag)databricks aitools install --helpno longer shows--project/--global;--scopeis documented;--agentsauto-detect behavior is describedTestParseScopeFlag(table-driven on the translation),TestInstallScopeFlag,TestListScopeFlag— all greenThis pull request was AI-assisted by Isaac.