Add smart scope detection for aitools update and uninstall#4923
Open
simonfaltum wants to merge 7 commits intomainfrom
Open
Add smart scope detection for aitools update and uninstall#4923simonfaltum wants to merge 7 commits intomainfrom
simonfaltum wants to merge 7 commits intomainfrom
Conversation
update and uninstall previously defaulted to global scope when no flag was passed, causing confusing "no skills installed" errors for users with project-only installations. Now both commands auto-detect which scopes have installations and behave accordingly: - Single scope installed: auto-select it - Both scopes + interactive: prompt the user (update offers 3 options including "both", uninstall offers 2) - Both scopes + non-interactive: error with flag guidance - Explicit flag pointing to empty scope: helpful error with CWD guidance update also supports --global --project together to update both scopes in one invocation. Co-authored-by: Isaac
Co-authored-by: Isaac
Address code review findings for the aitools scope detection feature: - Both flags (--project --global) on update now check installed scopes instead of blindly returning both, matching the design truth table - No-flags default case falls through to global scope so the installer layer can detect legacy installs and provide migration guidance - Expanded project-scope error with expected path and CWD guidance - Added cross-scope hints (e.g. "Global skills are installed" when user passes --project but only global exists) - Removed stale "(default)" from --global flag help text - Handle os.Getwd error properly in scope error paths Co-authored-by: Isaac
Global scope with explicit --global flag now always passes through to the installer layer, preserving legacy install detection and migration guidance. Only project scope uses withExplicitScopeCheck (where CWD guidance is useful). crossScopeHint now takes a verb parameter so uninstall hints say "uninstall" instead of "update". Co-authored-by: Isaac
Co-authored-by: Isaac
Move directory resolution to the Cobra RunE boundary in update.go and uninstall.go. Internal functions (detectInstalledScopes, crossScopeHint, scopeNotInstalledError, withExplicitScopeCheck, resolveScopeForUpdate, resolveScopeForUninstall) now take plain string/bool parameters instead of reaching through context, env vars, or os.Getwd(). Tests no longer manipulate global state (t.Setenv, t.Chdir). They pass directory paths directly. detectInstalledScopes, withExplicitScopeCheck, and crossScopeHint tests are now table-driven. Co-authored-by: Isaac
Fix scopeNotInstalledError building a doubled path by using projectDir directly (it already points to the skills directory). Fix resolveScopeForUpdate with both flags to only include scopes that are actually installed, falling through to global only when neither is installed (for legacy detection). Co-authored-by: Isaac
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: @databricks/eng-apps-devex, @lennartkats-db Suggestions based on git history. See OWNERS for ownership rules. |
simonfaltum
requested review from
MarioCadenas
and removed request for
a team and
lennartkats-db
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
1 participant
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.
Why
When running
aitools updateoruninstallwithout--project/--global, the CLI defaults to global scope regardless of what's actually installed. If a user only has project-scoped skills,updatesays "no skills installed" instead of detecting and updating the project install. There's no detection of what's present on disk.Changes
Before:
updateanduninstallalways defaulted to global scope when no flag was passed.--project --globaltogether was an error.Now: both commands auto-detect which scopes have installations and behave accordingly:
--project --globalon update: updates both installed scopes (ignores uninstalled ones)--project --globalon uninstall: always errors (safety, prevents accidental full removal)The implementation adds
detectInstalledScopes,resolveScopeForUpdate, andresolveScopeForUninstalltoscope.go. The existingresolveScope/resolveScopeWithPromptused byinstallare untouched. Legacy installs (skills on disk without.state.json) still get the installer layer's migration guidance.Test plan
scope_test.gocovering all branches of scope detection and resolutiongo test ./experimental/aitools/cmd/... -count=1passesmake checkspasses