Skip to content

aitools: add --scope flag, deprecate --project/--global#5234

Open
jamesbroadhead wants to merge 2 commits into
mainfrom
jb/aitools-interface
Open

aitools: add --scope flag, deprecate --project/--global#5234
jamesbroadhead wants to merge 2 commits into
mainfrom
jb/aitools-interface

Conversation

@jamesbroadhead
Copy link
Copy Markdown
Contributor

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:

  • New --scope=project|global flag on install/update/uninstall/list, with --scope=both accepted by update and list.
  • --project and --global are marked deprecated via cobra's Deprecated property: 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.
  • --scope combined with --project/--global is rejected up front with an actionable error.
  • install's --help now documents the non-interactive --agents auto-detect contract so callers know what gets picked.

Stacked on #4917. Base will rebase to main once 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|both is one flag with valid values, not two flags with order-dependent semantics.

Surface

databricks aitools install   --scope=project|global             (--scope=both rejected)
databricks aitools uninstall --scope=project|global             (--scope=both rejected)
databricks aitools update    --scope=project|global|both
databricks aitools list      --scope=project|global|both        (default: both)

databricks aitools install --project    # warns: use --scope=project
databricks aitools install --global     # warns: use --scope=global

Test plan

  • databricks aitools install --scope=project and --scope=global go to the right destination
  • databricks aitools install --scope=both errors with a clear message
  • databricks aitools install --project still works and prints the deprecation warning to stderr
  • databricks aitools install --scope=global --project errors with the conflict message
  • databricks aitools list --scope=both shows both scopes (equivalent to no flag)
  • databricks aitools install --help no longer shows --project/--global; --scope is documented; --agents auto-detect behavior is described
  • Unit: TestParseScopeFlag (table-driven on the translation), TestInstallScopeFlag, TestListScopeFlag — all green

This pull request was AI-assisted by Isaac.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 11, 2026

Approval status: pending

/cmd/aitools/ - needs approval

10 files changed
Suggested: @simonfaltum
Also eligible: @lennartkats-db, @fjakobs, @Shridhad, @atilafassina, @keugenek, @arsenyinfo, @igrekun, @pkosiec, @MarioCadenas, @pffigueiredo, @ditadi, @calvarjorge, @renaudhartert-db, @hectorcast-db, @parthban-db, @tanmay-db, @Divyansh-db, @tejaskochar-db, @mihaimitrea-db, @chrisst, @rauchy

General files (require maintainer)

Files: NEXT_CHANGELOG.md
Based on git history:

  • @simonfaltum -- recent work in ./

Any maintainer (@andrewnester, @anton-107, @denik, @pietern, @shreyas-goenka, @simonfaltum, @renaudhartert-db) can approve all areas.
See OWNERS for ownership rules.

@jamesbroadhead jamesbroadhead force-pushed the jb/aitools-interface branch from 526cd9c to a7e3d03 Compare May 12, 2026 20:13
@jamesbroadhead jamesbroadhead force-pushed the jb/aitools-interface branch from a7e3d03 to 37cf784 Compare May 13, 2026 14:32
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 17:39 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 17:39 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 17:40 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 17:40 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 18:01 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 18:01 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 18:31 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 18:31 — with GitHub Actions Inactive
Base automatically changed from jbroadhead/aitools-public to main May 18, 2026 19:26
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 19:31 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 19:31 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 20:33 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 18, 2026 20:33 — with GitHub Actions Inactive
@jamesbroadhead jamesbroadhead force-pushed the jb/aitools-interface branch from 8f0319a to 7666312 Compare May 18, 2026 21:43
@jamesbroadhead
Copy link
Copy Markdown
Contributor Author

👋 @simonfaltum — Claude here on James's behalf.

#4917 merged earlier today, so this PR is now stacked on main and no longer needs to wait. I've rebased the branch onto current main and force-pushed; the diff is now the actual scope-flag delta (9 files, +219/-13) without the merge noise from the old base.

Ready for a re-review whenever you've got a moment.

(comment posted by Claude)

jamesbroadhead added a commit that referenced this pull request May 18, 2026
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
Copy link
Copy Markdown
Member

@simonfaltum simonfaltum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a couple of should-fix items and one test gap. The affected package tests pass locally: go test ./cmd/aitools.

Comment thread cmd/aitools/list.go
Comment on lines +34 to 41
// 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
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/aitools/scope.go Outdated
Comment on lines +115 to +121
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)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread cmd/aitools/update.go
return err
}

projectFlag, globalFlag, err := parseScopeFlag(scopeFlag, projectFlag, globalFlag, true)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@jamesbroadhead jamesbroadhead force-pushed the jb/aitools-interface branch from 7666312 to 24d5351 Compare May 19, 2026 08:41
@jamesbroadhead
Copy link
Copy Markdown
Contributor Author

👋 @simonfaltum — Claude here on James's behalf.

All three of your should-fix items are addressed in 24d5351fa:

  1. list.go silent semantics regression — moved the validation into parseScopeFlag itself so combining the two deprecated booleans is always an error, regardless of allowBoth. Restored the original message: cannot use --global and --project together. Updated the existing TestListScopeFlag/legacy_both_flags_shows_both case to expect the error (renamed accordingly).

  2. allowBoth=false error messageparseScopeFlag now branches: with allowBoth=true the message says "must be one of project, global, both"; with allowBoth=false (install, uninstall), the message says "must be one of project, global". Added a table case + an explicit NotContains "both" assertion in TestParseScopeFlag since the substring check on the table would have shared a prefix with the allowBoth variant.

  3. Test gap on update/uninstall — added cmd/aitools/update_test.go (TestUpdateScopeFlag) and cmd/aitools/uninstall_test.go (TestUninstallScopeFlag), each table-driven covering: a success case that exercises the new wiring (--scope=global for both, plus --scope=both for update which falls through to global without state), --scope=all invalid value, the --scope=X --project conflict, and --project --global together. To support those without hitting the network, introduced updateSkillsFn and uninstallSkillsFn package-level test-injection vars mirroring the installSkillsForAgentsFn pattern already in install.go. --scope=project success paths stay covered via TestParseScopeFlag and the existing TestResolveScopeForUpdate/UninstallProjectFlagWithState (those need installed state, which is fiddly to set up in a command-level test).

go test ./cmd/aitools passes locally.

(comment posted by Claude)

jamesbroadhead added a commit that referenced this pull request May 19, 2026
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
Copy link
Copy Markdown
Member

@simonfaltum simonfaltum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/aitools/scope.go
Comment on lines +105 to +112
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread cmd/aitools/scope.go
Comment on lines +86 to +95
// 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
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cmd/aitools/list.go
}
// For list: no flag = show both scopes (empty string).

// list: empty scope = show both. Both flags set is equivalent.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants