Repository navigation
aitools: install skill files when a detected agent's CLI cannot run - #7482
Conversation
54c4bc5 to
75d4a60
Compare
5afe64b to
d5fb33f
Compare
|
Commit: Regression test reportSelected 1 changed tests (limit=20, 0 not selected) 1 tests: regression=1 unreleased=0 coverage=0 failing=0
TestAccept/experimental/aitools/skills/install-non-interactive: branch ✅ | main (eebf099) ❌ | latest (v1.26.0) ❌main (eebf099): latest (v1.26.0): |
✅ Integration test reportCommit: 4e71af1
Top 17 slowest tests (at least 2 minutes):
|
75d4a60 to
7d9546e
Compare
d5fb33f to
6dfcf53
Compare
7d9546e to
a2b82bc
Compare
6dfcf53 to
0de8029
Compare
a2b82bc to
0758e7e
Compare
0de8029 to
3e2f9ef
Compare
0758e7e to
3f574d9
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 3e2f9ef. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 blocking issue(s), tagged [blocking]; 3 open design questions.
This PR makes databricks aitools install install skill files for a detected agent whose CLI cannot run, keep an existing plugin instead of adding duplicate skills, and drop stale plugin records after a fallback. The approach is reasonable, and CI passes. Two things block merging. First, the branch is built on an older revision of #7481, so it conflicts with its base and the kept-plugin logic has to be redone on top of anyInstalled/errNoAgentSetUp. Second, fallback agents are installed in a separate skills batch, so they keep stale skill copies after an upgrade. The other comments cover edge cases (kept-path cleanup, unreadable manifest, update warnings) and doc, test and description tidy-ups; three questions on the telemetry and fallback design are open.
Questions
- When a detected agent keeps its plugin and another agent's install fails, the run exits 0 but the funnel records FAILURE. An installed agent plus the same failure records SUCCESS. The current #7481 removed the exit-0-but-FAILURE branch, and per-agent results for kept or fallback agents carry the same CLI_NOT_ON_PATH entry as a real failure. Should kept be treated like installed (SUCCESS, or NOTHING_TO_DO when nothing else happened), or is the difference intended? If it is intended, could the per-agent telemetry distinguish 'kept' and 'fell back to skills' from 'failed'?
- The fallback is decided at run time inside executePlan. That is why the picker label, plan summary and outcomes disagree, and why fallback agents need a second skills install. Would deciding it at plan time in planItemFor work instead, for non-explicit plugin agents whose CLI is missing (StateInstalledCLIMissing)? That puts them in the single skills batch and keeps the summary accurate; the run-time path would only be needed when the binary is present but 'plugin --help' fails. Or is there a reason to keep it at run time?
- A global-scope fallback ignores a project or local Claude plugin that applies to the current directory, so that project ends up with both the plugin and global raw skills. Is accepting duplicates in that project the intended trade-off? The alternatives are skipping the fallback, or printing a warning when the cwd has a project-scope plugin.
Comments outside the changed lines
- [nit]
cmd/aitools/install.go:673: IfrecordPluginInstallsFnfails, executePlan returns before the fallback block. The fallback agents get nothing, even though "Installing skill files instead" was already printed for them. Run the fallback before the early return, or fold it into a single skills batch. - [nit]
.github/PULL_REQUEST_TEMPLATE.md: The repo's pr-checklist asks for the template sections## Changes,## Whyand## Tests, with no invented sections, and an agent disclosure as a single italic line. The description uses## Summary,## Evidenceand## Merge Danger. Its claim "No extra network calls beyond the existing skills manifest fetch" is also inaccurate: the fallback fetches the manifest a second time. Please restructure the description and correct that claim.
| case runErr == nil: | ||
| if category, ok := nothingInstalledCategory(outcomes); ok { | ||
| funnel.finishWith(ctx, protos.AitoolsCommandOutcomeFailure, category) | ||
| } else if onlyKeptPlugins(outcomes) { |
There was a problem hiding this comment.
[blocking] This branch sits on an older revision of #7481 (0758e7e). #7481 has since been force-pushed to 3f574d9, the base. GitHub reports this PR as not mergeable, and even a rebase of just this PR's commit conflicts in install.go, telemetry.go and funnel_test.go. The current #7481 removed nothingInstalledCategory, newNothingInstalledError and nothingInstalledError, and runErr == nil now always records SUCCESS. The kept-plugin logic here is written against those removed helpers. If the conflicts are resolved by taking the base's anyInstalled (which only checks outcomeInstalled), a kept-only run returns errNoAgentSetUp and exits 1. If the base's case runErr == nil is taken, the NOTHING_TO_DO outcome is lost. Please rebase onto the current aitools/nothing-installed-fails, count outcomeKept in anyInstalled, and redo the kept-plugin funnel handling on top of the base's runErr == nil branch.
There was a problem hiding this comment.
Rebased onto the current aitools/nothing-installed-fails (46180b9) in 7137e09. anyInstalled now counts outcomeKept, so a run whose only agent kept its plugin exits 0. The kept-plugin funnel handling sits in the base's runErr == nil branch through keptPluginsFinish: NOTHING_TO_DO when the run only kept plugins, FAILURE with the failed agent's category when it only kept plugins next to a failed install, SUCCESS when anything was installed. These outcomes are the same as in the previous revision.
| if !quiet { | ||
| installer.PrintInstallingFor(ctx, fallbackAgents) | ||
| } | ||
| if err := installSkillsForAgentsFn(ctx, src, fallbackAgents, opts); err != nil { |
There was a problem hiding this comment.
[blocking] Fallback agents are installed by a second installSkillsForAgentsFn call. At global scope a one-agent batch copies files instead of symlinking (useSymlinks = len(detectedAgents) > 1), so Cursor and Claude Code each get their own copy. On a later install with a newer skill version, the Cursor batch updates the canonical files and sets state.Skills to the new version. The fallback batch then sees a matching version and an existing Claude copy (allAgentsHaveSkill only checks that the copy exists), so it skips Claude. The command succeeds while Claude keeps the old files, on every upgrade, because the skills batch always runs first. Two batches also resolve the ref and fetch the manifest twice and print the install banner twice, as the new acceptance output shows, which contradicts the description's "no extra network calls". Please install the fallback agents in the same InstallSkillsForAgents call as the other skills agents: run the plugin loop first, or decide the fallback at plan time.
There was a problem hiding this comment.
Fixed in 7137e09. The fallback is decided in planItemFor, so these agents are deliverySkills items in the run's single InstallSkillsForAgents call: symlinks when there are several agents, one ref lookup, one manifest fetch, one banner (see the acceptance output: "Installing Databricks skills for Claude Code, Cursor..."). The second batch is gone.
| cmdio.LogString(ctx, fmt.Sprintf("Installing databricks plugin for %s...", it.agent.DisplayName)) | ||
| } | ||
| rec, err := installPluginForAgentFn(ctx, it.agent, it.scope, ref) | ||
| if err != nil && fallsBackToSkills(it, err) && pluginAlreadyInstalledFn(ctx, it.agent, opts.Scope) { |
There was a problem hiding this comment.
[nice to have] The kept branch exists to avoid duplicate skills, but it never calls cleanupLegacyFn, so raw skills written by an earlier fallback stay next to the plugin. Scenario: run 1 has no claude on PATH, falls back, and drops the plugin record. The user then installs the plugin through Claude directly. Run 2, again without claude on PATH, keeps the plugin and leaves ~/.claude/skills alone, so Claude lists every skill twice. With no record, aitools update keeps refreshing the raw files. Please call cleanupLegacyFn(ctx, it.agent, opts.Scope) in the kept branch too, as the plugin-install path does.
There was a problem hiding this comment.
Done in 7137e09: the kept path calls cleanupLegacyFn(ctx, it.agent, opts.Scope) and logs a failure at debug level, as the plugin path does. The acceptance test now runs your scenario: the first run falls back, then a Claude manifest with the plugin appears, and the second run keeps the plugin and removes Claude Code's skill files.
| project = cwd | ||
| } | ||
| } | ||
| return a.DatabricksPluginActiveIn(ctx, project) |
There was a problem hiding this comment.
[nice to have] claudePluginInstalls returns (nil, false) on any read or parse error, and DatabricksPluginActiveIn discards the ok. So pluginAlreadyInstalled returns false, the run falls back, and ForgetPluginInstalls removes a valid claude-code plugin record. If installed_plugins.json is mid-write or its schema changes, every non-interactive install without claude on PATH writes raw skills next to the installed plugin and drops the record. Please tell "no manifest file" apart from "manifest present but unreadable". In the unreadable case, fall back to the CLI state record, or keep the plugin.
There was a problem hiding this comment.
Done in 7137e09. The manifest reader now returns an error when installed_plugins.json exists but cannot be read or parsed (a missing file is still "not installed"), and DatabricksPluginActiveIn returns it. pluginAlreadyInstalled then uses the CLI's state record for the scope, so an unreadable manifest no longer drops a valid record. DatabricksTooling keeps treating a read error as not installed.
| for _, a := range fallbackAgents { | ||
| outcomes = append(outcomes, agentOutcome{agent: a, delivery: deliverySkills, status: outcomeInstalled}) | ||
| } | ||
| // These agents have no plugin (pluginAlreadyInstalled said so), so a |
There was a problem hiding this comment.
[nice to have] A fallback agent ends up with raw skills and no plugin record, which makes it a plugin-migration candidate in update (pluginMigrationCandidatesForScope). Every aitools update then calls the plugin install, fails, and prints Skipped Claude Code: claude-code: cli-not-on-path: … (the raw BlockedError). Before this PR only --skills-only users reached that state; now desktop and IDE users reach it by default. Consider skipping the migration attempt when the CLI is not on PATH, or printing the same friendly reason and recovery that install prints.
There was a problem hiding this comment.
Done in 7137e09: pluginMigrationCandidatesForScope skips an agent whose CLI is not on PATH, matching install, so aitools update keeps refreshing its raw skills without a failing plugin install on every run. Covered by TestPluginMigrationCandidatesNeedTheCLI.
| return false | ||
| } | ||
| blocked, ok := errors.AsType[*installer.BlockedError](err) | ||
| return ok && blocked.Reason == installer.ReasonCLINotOnPath && !blocked.TimedOut |
There was a problem hiding this comment.
[nit] probePluginCLI returns ReasonCLINotOnPath both when the binary is missing and when it is on PATH but plugin --help fails, for example an old CLI or a probe false negative. fallsBackToSkills doesn't tell these apart, so an on-PATH CLI that is too old now gets raw skills instead of the "update your CLI" failure. The doc comment and PR description both say "not on PATH". Either check !it.agent.HasBinary(ctx) or update the comment and description.
There was a problem hiding this comment.
Fixed in 7137e09: the fallback now checks !a.HasBinary(ctx) when the run is planned, so only a missing binary falls back. A CLI on PATH whose plugin --help fails (too old, or a probe false negative) gets the "Install or update the ... CLI" failure, as before this PR. The description says so.
|
|
||
| // A kept plugin does not hide another detected agent's failed install: the | ||
| // funnel reports the failure with that agent's category. | ||
| func TestInstallFunnelKeptPluginWithAFailedInstallIsFailure(t *testing.T) { |
There was a problem hiding this comment.
[nit] TestInstallFunnelKeptPluginIsNothingToDo and TestInstallFunnelKeptPluginWithAFailedInstallIsFailure repeat the setup of TestInstallFunnelDetectedPluginFailureIsNotSuccess. They differ only in the PATH binaries, the installer stub and the expected funnel shape. The same goes for TestExecutePlanFallsBackToSkillsWhenCLIIsMissing and TestExecutePlanKeepsAnInstalledPluginInsteadOfAddingSkills in install_test.go. .agents/rules/testing.md asks for table-driven tests for similar cases.
There was a problem hiding this comment.
Done in 7137e09: TestInstallFunnelDetectedPluginAgents is one table (the only install failed, skill files instead, plugin kept, plugin kept next to a failed install), and TestExecutePlanWhenTheCLIIsMissing covers the fallback and kept cases in one table.
| }) | ||
| continue | ||
| } | ||
| if err != nil && fallsBackToSkills(it, err) { |
There was a problem hiding this comment.
[nit] err != nil && fallsBackToSkills(it, err) is checked at line 609 and again here, followed by a third if err != nil branch for the same decision. One switch, or a single if with an inner already-installed / not-installed branch, would read more clearly (style-guide-go prefers switch for one decision).
There was a problem hiding this comment.
No longer applies in 7137e09: with the decision in planItemFor, the plugin loop in executePlan is back to its single if err != nil branch, and planItemFor makes the decision in one switch (scope unsupported, CLI missing, plugin).
| // from the state file for the given CLI scope, for agents that no longer have | ||
| // the plugin. Agents without a record, or a scope without state, are left as | ||
| // they are. | ||
| func ForgetPluginInstalls(ctx context.Context, cliScope string, agentNames []string) error { |
There was a problem hiding this comment.
[nit] ForgetPluginInstalls has no test in plugin_test.go. It is only exercised indirectly from cmd/aitools, so its no-state and no-record paths are untested. fallsBackToSkills also has no direct test: nothing checks that a TimedOut CLI_NOT_ON_PATH error or an explicit agent does not fall back. The kept notice (pluginKeptText) has no acceptance coverage either. Please add a small table test for fallsBackToSkills in recovery_test.go and a ForgetPluginInstalls test in plugin_test.go.
There was a problem hiding this comment.
Added in 7137e09: TestForgetPluginInstalls in plugin_test.go (no state, an agent without a record, a recorded agent; it also checks PluginRecorded per scope). fallsBackToSkills is gone; the plan decision that replaces it is covered by the table test TestPlanItemForAgentWithoutCLI (CLI on PATH, CLI missing, missing with the plugin, named via --agents, scope unsupported). A timeout can no longer lead to a fallback, because planning does not run the CLI. The kept notice is now in the acceptance output.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` installs skill files for a detected coding agent whose CLI cannot run (for example a Claude Code desktop or IDE install), and says how to switch to the plugin later. ([#7482](https://github.com/databricks/cli/pull/7482)) | |||
There was a problem hiding this comment.
[nit] The pr-checklist says a fragment that leads with the command should use the present tense with "now". Suggest: "databricks aitools install now installs skill files for a detected coding agent whose CLI cannot run …".
There was a problem hiding this comment.
Done in 7137e09: "databricks aitools install now installs skill files for a detected coding agent whose CLI is not on PATH (for example a Claude Code desktop or IDE install), unless it already has the plugin, and says how to switch to the plugin later." The PR link is kept.
3f574d9 to
46180b9
Compare
3e2f9ef to
7137e09
Compare
|
Addressed in 7137e09:
|
7137e09 to
4fd042b
Compare
|
On Question 3 (global-scope fallback next to a project or local Claude plugin), in 4fd042b: the fallback still runs, because the global skill files serve every other project and the run cannot tell which projects the user works in. When the current directory has a project or local databricks plugin for that agent, install now also warns: "Claude Code has the databricks plugin for this project, so this project will see both the plugin and the global skill files." The check reads Claude Code's own manifest for the current directory; an unreadable manifest prints no warning. Covered in |
|
On Question 1, in 4fd042b: a kept plugin now counts like an installed one. A run that sets up an agent by keeping its plugin finishes with The per-agent results keep the |
46180b9 to
544ae4b
Compare
4fd042b to
5261133
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 5261133. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR makes databricks aitools install give skill files to a detected agent whose CLI isn't on PATH, and an agent that already has the plugin keeps it. It also drops stale plugin records and stops update from retrying the plugin migration for such agents. Every point from the earlier review round is addressed at this head, and no CI check has failed. Two non-blocking issues remain. Plugin agents that aren't installed at all now appear in the picker as "skills · CLI not found" and get skill files if picked. And after a run that keeps a plugin, the next aitools update writes the raw skills back next to it. There are a few nits, plus one question about picker preselection.
Questions
- In the interactive picker, a detected agent whose CLI isn't on PATH (for example Claude Code desktop) is labeled "skills · CLI not found" but stays unticked, because
IsPreselected(libs/aitools/agents/detect.go:82) only pre-checks agents that can complete an install automatically. A non-interactive run of the same command now installs skill files for that agent. Is the difference intended? Or should these agents be preselected now that they get skill files?
Agreeing with existing threads
- [nit] Telemetry per-agent results report a kept plugin as a CLI_NOT_ON_PATH failure: #7482 (comment)
| item.reason = reason | ||
| item.skipErrorCategory = protos.AitoolsErrorCategoryUnsupportedScope | ||
| } else { | ||
| case !explicit && !a.HasBinary(ctx): |
There was a problem hiding this comment.
[nice to have] The new !explicit && !a.HasBinary(ctx) branch doesn't check that the agent is detected. The interactive picker calls planItemFor for every agent in agents.Registry. So on a machine without Codex or Copilot (no config dir, no CLI), those rows now print "skills · CLI not found" where they used to print "not found". If the user picks one, skill files are written for an agent that isn't installed. That creates its config dir, so every later non-interactive run treats the agent as detected and installs for it. Before this change, that pick failed with recovery steps. Please limit the fallback to detected agents, e.g. case !explicit && !a.HasBinary(ctx) && a.Detected(ctx):, and add a not-detected case to TestPlanItemForAgentWithoutCLI.
There was a problem hiding this comment.
Fixed in 09d55e8: the branch is now case !explicit && !a.HasBinary(ctx) && a.Detected(ctx):, so an agent with neither its config dir nor its CLI is labeled "not found" in the picker again and, if picked, gets the plugin attempt and its recovery steps as before. Added a "not installed" case (Copilot) to TestPlanItemForAgentWithoutCLI.
| } | ||
| // Raw skills an earlier run gave this agent would show every skill | ||
| // twice next to the plugin, so remove them as a plugin install does. | ||
| if err := cleanupLegacyFn(ctx, it.agent, opts.Scope); err != nil { |
There was a problem hiding this comment.
[nice to have] The kept path removes the agent's raw skills but writes no plugin record to the CLI state. aitools update writes raw skills to every agent from DetectInstalled that excludePluginAgents doesn't filter, and that filter only checks state.Plugins. So the next update that changes a skill (a new release, or --force) writes the raw skills back into ~/.claude/skills next to the plugin. That brings back the duplicate listing the kept path exists to prevent. Example: Claude Code desktop plus Cursor; the user installs the plugin in Claude, runs aitools install (kept, raw skills removed), then aitools update. Please record the kept plugin in the state, or have update skip agents whose own manifest shows the plugin, and extend the acceptance sequence through an update.
There was a problem hiding this comment.
Fixed in 09d55e8 on the update side: excludePluginAgents now also drops an agent whose own manifest shows the plugin for the scope, next to the existing check of the CLI's records. I did not record the kept plugin in the state: a record would make update and uninstall run the agent's CLI for it, which is the CLI that is missing. The acceptance sequence now runs aitools update --force after the kept install and checks that Claude Code still has no skill files (it fails without the fix). Unit test: TestExcludePluginAgentsReadsTheAgentsManifest.
| project = cwd | ||
| } | ||
| } | ||
| active, err := a.DatabricksPluginActiveIn(ctx, project) |
There was a problem hiding this comment.
[nit] DatabricksPluginActiveIn counts a plugin that is disabled in Claude's settings. In the kept path, cleanupLegacyFn then removes the raw skills from an earlier fallback. A user whose plugin is installed but disabled ends up with no active databricks skills, and the run reports success. Consider removing the raw skills only when the plugin is enabled, or mention in the kept notice that the plugin is disabled.
There was a problem hiding this comment.
Leaving this one as is. The CLI has no reader for Claude Code's enabled and disabled plugins: that lives in Claude's layered settings files (user, project and local), and reading them reliably is a change of its own. A disabled plugin is also a choice the user made in Claude Code, and the kept notice already says the plugin is there and how to refresh it. Running databricks aitools install --skills-only gives the skill files back if someone wants them while the plugin stays disabled.
There was a problem hiding this comment.
Correction to my earlier reply: --skills-only is not a lasting workaround for a disabled plugin. The next default install keeps the plugin and removes those files, and update now does the same. A disabled plugin stays a known limitation of the kept path, because the CLI does not read Claude Code's enabled and disabled settings.
| # Claude Code records its plugins in installed_plugins.json. The run keeps the | ||
| # plugin and removes the skill files the previous run gave Claude Code. | ||
| "$mkdir_bin" -p "${USERPROFILE:-$HOME}/.claude/plugins" | ||
| printf '%s\n' '{"plugins":{"databricks@claude-plugins-official":[{"scope":"user","version":"0.2.9"}]}}' > "${USERPROFILE:-$HOME}/.claude/plugins/installed_plugins.json" |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md says to commit static test inputs instead of creating them in script at test time. The installed_plugins.json written here with printf is a fixed literal. Please check it in next to the test and cp it into ~/.claude/plugins/.
There was a problem hiding this comment.
Done in 09d55e8: the manifest is checked in as installed_plugins.json next to the test, and the script copies it into ~/.claude/plugins/.
| // HasPluginManifest reports whether the CLI can read the agent's own plugin | ||
| // manifest (Claude Code). For such agents the manifest is authoritative: a | ||
| // plugin removed with the agent's own CLI leaves the CLI's state record behind. | ||
| func (a *Agent) HasPluginManifest() bool { |
There was a problem hiding this comment.
[nit] HasPluginManifest() returns true for Claude Code even when installed_plugins.json doesn't exist. It reports that the CLI has a manifest reader for the agent. The comment in pluginAlreadyInstalled ("When the agent has a readable plugin manifest") reads the same way. Consider renaming it to HasPluginManifestReader (or similar) so callers don't take it as a file-existence check.
There was a problem hiding this comment.
Renamed to HasPluginManifestReader in 09d55e8, with a doc comment saying it reports a reader, not that the file exists. The comment in pluginAlreadyInstalled now says "When the CLI can read the agent's own plugin manifest", and the scope rules moved into a small manifestPluginActive helper that update uses too.
| // cliMissingReason is why an agent whose CLI is not on PATH does not get the | ||
| // plugin. | ||
| func cliMissingReason(it agentPlanItem) string { | ||
| return fmt.Sprintf("'%s' is not on PATH", it.agent.Binary) |
There was a problem hiding this comment.
[nit] cliMissingReason builds '%s' is not on PATH, and pluginFailureText (recovery.go:395) builds the same string separately. Both strings appear in the acceptance output (the fallback notice and the named-agent failure). If one changes, they drift apart. Have pluginFailureText call cliMissingReason for the binary-missing case.
There was a problem hiding this comment.
Done in 09d55e8: pluginFailureText uses cliMissingReason(it) for the binary-missing case, so the fallback notice and the named-agent failure share one string.
5261133 to
09d55e8
Compare
52e34eb to
181bc21
Compare
a5688b2 to
1c5b9d5
Compare
181bc21 to
e8d4f91
Compare
1c5b9d5 to
357f7ff
Compare
e8d4f91 to
2eca9b0
Compare
357f7ff to
122bf1b
Compare
2eca9b0 to
cf6eeb8
Compare
122bf1b to
461f2f4
Compare
cf6eeb8 to
3289cf7
Compare
461f2f4 to
eae0662
Compare
3289cf7 to
c6b1b5e
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
cmd/aitools/install.go:320: [nice to have] After a picker selection, rerun.agents names the selected agents, but rerun.skillsOnly stays false. If the skill download fails for a fallback agent, the recovery step suggests databricks aitools install --agents=claude-code ... without --skills-only. A named agent never falls back ("--agents never changes delivery", line 583), so that re-run tries the plugin and fails on the missing CLI. Add --skills-only to the re-run when the failed agents are fallback agents, or name them in a separate skills-only command.
| // This runs before the skills batch: a newer release there replaces the | ||
| // recorded checksums, and older copies would then look user-modified | ||
| // and stay. | ||
| if err := cleanupLegacyFn(ctx, it.agent, opts.Scope); err != nil { |
There was a problem hiding this comment.
[nit] Codex and Copilot have no manifest reader, so pluginAlreadyInstalled uses only installer.PluginRecorded for them (line 616). That record stays after codex plugin uninstall. If the CLI is then also off PATH, the agent is kept, and this call removes its raw skills. The agent then has neither the plugin nor skills, but the output says that it already has the plugin. Skip the cleanup when the record is the only evidence, or add a comment that accepts this trade-off.
There was a problem hiding this comment.
Fixed in 4e71af1. The kept path now removes raw skills only when the agent's own manifest shows the plugin (manifestPluginActive). For Codex and Copilot the CLI's record is the only evidence, and it outlives a removal with the agent's own CLI, so their raw skills stay. The comment there says so. Covered by the new "plugin kept on the CLI's record alone" case in TestExecutePlanWhenTheCLIIsMissing.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` now installs skill files for a detected coding agent whose CLI is not on PATH (for example a Claude Code desktop or IDE install), unless it already has the plugin, says how to switch to the plugin later, and warns when the current project also has the plugin. ([#7482](https://github.com/databricks/cli/pull/7482)) | |||
There was a problem hiding this comment.
[nit] This fragment names only aitools install, but aitools update changes too. It no longer tries the plugin migration for an agent without a CLI. It also skips raw skills for an agent whose manifest shows the plugin, and it removes the raw skills that such an agent still has. Add aitools update to the fragment.
There was a problem hiding this comment.
Done in 4e71af1. The fragment now also says that databricks aitools update no longer tries to install the plugin for such an agent, and no longer writes skill files next to a plugin that such an agent already has. The PR link is kept.
| cmdio.LogString(ctx, cmdio.Yellow(ctx, projectPluginWarning(it))) | ||
| } | ||
| } | ||
| outcomes = append(outcomes, cliMissingOutcome(it, outcomeFailed)) |
There was a problem hiding this comment.
[Question] An agent that falls back to skill files gets two outcomes: this plugin outcome with status failed and CLI_NOT_ON_PATH, and a skills outcome with status installed. Is that intended? The answer matters because a coding agent that reads --output json can see failed for claude-code and tell the user that the install failed, although the run set it up.
There was a problem hiding this comment.
Yes, this is intended, and the --output json case cannot happen. JSON output requires --scope and --agents, and an agent named via --agents never falls back (planItemFor checks !explicit). So the plugin outcome with failed / CLI_NOT_ON_PATH next to the skills outcome with installed only shows up in a run without --agents.
There, the text output prints the fallback notice, and the run counts as a success. The plugin outcome keeps the reason the plugin was not installed in the per-agent telemetry results, and the skills outcome adds the SKILLS_INSTALLED step. Telling a fallback apart from a failed install in the per-agent results can follow in a separate PR.
| // plugin would show every skill twice, so remove them as | ||
| // install does when it keeps the plugin. | ||
| if !check { | ||
| for _, a := range manifestPlugins { |
There was a problem hiding this comment.
[nit] This removes the raw skills of every agent whose manifest shows the plugin. But install --skills-only writes raw skills for such an agent (planItemFor, line 560). So a user who runs install --agents=claude-code --skills-only next to a marketplace plugin loses those skills at the next update, and only a debug log says so. Limit this cleanup to agents whose CLI is missing (the case that the kept path creates), or print a line when it removes files.
There was a problem hiding this comment.
Fixed in 4e71af1. excludePluginAgents now uses the agent's own manifest only when its CLI is not on PATH, which is the case the kept path creates. With the CLI on PATH, raw skills next to the plugin are the user's choice (install --skills-only). Update refreshes them as before and removes nothing. Covered in TestExcludePluginAgentsReadsTheAgentsManifest.
85990ef to
c59988f
Compare
eae0662 to
4e71af1
Compare
|
On the review note about |
c59988f to
965ae5a
Compare
4e71af1 to
4baa441
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 4baa441. Each finding was verified against the code; simonfaltum may follow up.
No blocking issues found, approving. 6 non-blocking comment(s) below.
This PR makes databricks aitools install give skill files to a detected agent whose CLI is not on PATH, or keep the plugin it already has, and changes aitools update to match. The earlier update --scope=project failure is fixed, every later review note has a fix or an answer, and CI passes. Two non-blocking issues remain. After a picker selection, the recovery step for a release with no installable skill still names a fallback agent without --skills-only. And aitools update with no install state now exits 0 silently when the only detected agents are CLI-less agents that have the plugin. The rest are nits: a missing duplicate warning at project scope, the help text, and two style-guide points.
Accepted answers
- Fallback for Codex CLI and GitHub Copilot, whose plugin list the CLI cannot read: #7482 (comment)
- A disabled Claude plugin counts as kept: #7482 (comment)
- Raw skills removed next to a manifest plugin, including --skills-only ones: #7482 (comment)
- Picker does not pre-tick a CLI-missing agent: #7482 (comment)
- Kept and fallback agents report CLI_NOT_ON_PATH per-agent results: #7482 (comment)
- Unreadable Claude manifest handling: #7482 (comment)
| if errors.Is(skillsErr, installer.ErrNoSkillsResolved) && (pluginCount > 0 || len(keptItems) > 0) { | ||
| recovery := installRecovery(ctx, rerun, skillsErr) |
There was a problem hiding this comment.
[nice to have] After a picker selection, rerun.agents names every picked agent. So the step built here (a release with no skill this CLI can install, next to an installed or kept plugin) says to re-run with --agents=claude-code,... and no --skills-only. In that re-run, Claude Code is a named agent, and a named agent never falls back. It tries the plugin and fails with "'claude' is not on PATH", and it still gets no skill files, even with --experimental added. Example: in the picker, tick Claude Code ("skills · CLI not found") and Codex (CLI on PATH), with a release that has only experimental skills. Codex's plugin installs, and Claude Code's recovery is databricks aitools install --scope=global --agents=claude-code,codex --debug. Build this step from the invocation skillsBatchError uses (the skills agents plus --skills-only) when the batch holds a fallback agent, e.g. through a shared helper. The errors.Join return after a failed recordPluginInstallsFn (line 901) has the same gap.
| if len(manifestPlugins) > 0 && !slices.ContainsFunc(fileAgents, func(a *agents.Agent) bool { | ||
| return scope != installer.ScopeProject || a.SupportsProjectScope | ||
| }) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
[nice to have] This skip also runs when the scope has no install state. With no state anywhere, resolveScopeForUpdate sends the run to the global scope, so UpdateSkills can report "no skills or plugins installed" or the legacy-install guidance. The comment above this block says the reconcile keeps running for that reason. Now the loop skips the reconcile when every detected agent's own manifest shows the plugin and its CLI is missing. Example: a Claude Code desktop user installed the plugin from Claude's marketplace and never ran databricks aitools install. databricks aitools update then prints nothing and exits 0 with outcome SUCCESS. Before, it exited 1 with NOTHING_TO_DO and said to run install. With a legacy install (skills on disk, no state file), the legacy guidance is lost too. Only skip when state != nil, and add a no-state case to the tests.
| case scope == installer.ScopeGlobal: | ||
| // The global skill files still serve every other project. | ||
| item.projectPlugin = projectPluginActive(ctx, a) |
There was a problem hiding this comment.
[nit] Suppose Claude Code has a user-scope or local databricks plugin and claude is not on PATH. Then install --scope=project installs the project's skill files, as intended for teammates. This user now loads both the plugin and those skill files, so every skill shows twice, and nothing says so. The global-scope case prints projectPluginWarning for the same situation. Consider setting projectPlugin for the project scope too: a plugin active in cwd that isn't the project's own. Give it a message that names the personal plugin.
| (` + strings.Join(agents.SkillsOnlyNames(), ", ") + `) get raw skill files, and so does a | ||
| detected plugin agent whose CLI is not on PATH (for example a desktop or IDE | ||
| install), unless it already has the plugin. For Claude Code that is read from | ||
| its own plugin list; for Codex and GitHub Copilot only a plugin installed by | ||
| 'databricks aitools' is known, so one installed another way shows every skill | ||
| twice next to the skill files. |
There was a problem hiding this comment.
[nit] The help says a detected plugin agent whose CLI is not on PATH gets raw skill files. It doesn't say this only applies when the agent isn't named with --agents. Someone who reads it and runs databricks aitools install --agents=claude-code on a desktop-only install gets "'claude' is not on PATH" instead of skill files. Add "unless it is named with --agents", or point to --skills-only for that case.
| claude := agents.ByName(agents.NameClaudeCode) | ||
| cursor := agents.ByName(agents.NameCursor) | ||
| all := []*agents.Agent{claude, cursor} | ||
| writeManifest := func(scope, projectPath string) { |
There was a problem hiding this comment.
[nit] writeManifest is an inline closure that captures t and home just to name a repeated step. .agents/rules/style-guide-go.md asks for a named function instead. The same "create .claude/plugins and write installed_plugins.json" code is repeated several times in install_test.go and in this file. Extract one named helper, e.g. writeClaudeManifest(t, home, scope, projectPath string), and use it everywhere.
| return false, err | ||
| } | ||
| for _, install := range installs { | ||
| if install.Scope == "project" && install.appliesTo(cwd) { |
There was a problem hiding this comment.
[nit] "project" is now used here and in appliesTo's switch (line 44). .agents/rules/style-guide-go.md asks for a package-level constant once a value is used in more than one place. Add constants for Claude's scope names ("project", "local") and use them in both places.
e487a01 to
d71defa
Compare
4baa441 to
bf467ae
Compare
d71defa to
fd3519d
Compare
A detected coding agent whose CLI is not on PATH (for example a Claude Code desktop or IDE install: `~/.claude` exists, `claude` does not) used to be skipped, so the run left it with nothing. It now gets raw skill files, unless it already has the plugin. The plan decides this, so the picker label and what the run does agree, and the agent joins the one skills batch of the run: ```diff planItemFor (plugin agent, scope supported) - plugin + if CLI not on PATH, the agent is detected (config dir), and not named via --agents + if the agent already has the plugin for this scope + keep it (plugin delivery, nothing installed; outcome "kept") + else + skill files instead (skills delivery) + else + plugin ``` - "Already has the plugin": Claude Code's own `installed_plugins.json` decides (a `claude plugin uninstall` leaves the CLI's record behind); when that file exists but cannot be read, the CLI's install state for the scope decides, as it does for other agents. A global install counts only a machine-wide plugin. A project-scope install counts only a project-scope plugin of this project: its skill files are shared through the repository, while a user or local plugin is personal. For Codex and GitHub Copilot only a plugin installed by `databricks aitools` is known, so one installed another way shows every skill twice next to the skill files. - A kept plugin: install prints how to refresh it and, before the skills batch, removes raw skills an earlier run gave the agent, as a plugin install does, when the agent's own manifest shows the plugin (the CLI's record alone outlives a removal with the agent's own CLI). The picker labels it "plugin installed · CLI not found". - Skill files instead: install prints how to switch to the plugin, and after the skills install drops a stale plugin record from the CLI's install state (failure to do so is a warning). When the skills install fails after a picker selection, the recovery step re-runs the skills agents with `--skills-only`, since a named agent does not fall back. - A global-scope fallback in a directory where the agent has a project or local databricks plugin still runs, and warns that this project will see both the plugin and the global skill files. - A kept plugin counts like an installed one: the run succeeds, with telemetry outcome `NOTHING_TO_DO` when it did nothing else. - Agents named via `--agents`, and agents whose CLI is on PATH but whose `plugin` command fails, keep failing with the exact commands to run. - `aitools update` no longer tries to migrate an agent whose CLI is not on PATH from raw skills to the plugin, and no longer writes raw skills to an agent whose CLI is not on PATH and whose own manifest shows the plugin for the scope (a kept plugin has no CLI record): it removes them instead, and skips the reconcile when no agent is left that can take file skills in the scope. - New: `installer.PluginRecorded` (also used for the User-Agent tooling), `installer.ForgetPluginInstalls`, `agents.Agent.HasPluginManifestReader`, and an exported `agents.Agent.DatabricksPluginActiveIn` that reports an unreadable manifest as an error. - New acceptance test `acceptance/experimental/aitools/skills/install-non-interactive`. - Changelog fragment: `.nextchanges/cli/aitools-install-skills-fallback.md`. Co-authored-by: Isaac <no-reply@databricks.com>
❌ Integration test reportCommit: 8a17f53
67 interesting tests: 55 FAIL, 8 RECOVERED, 4 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
Stacked on #7481.
Changes
databricks aitools installnow gives raw skill files to a detected coding agent whose CLI is not on PATH (for example a Claude Code desktop or IDE install:~/.claudeexists,claudedoes not), unless the agent already has the plugin. Before, the run printed "Skipped Claude Code" and left the agent with nothing.The plan decides this, so the picker label and what the run does agree, and the agent joins the run's one skills batch (one ref lookup, one manifest fetch):
installed_plugins.jsondecides, becauseclaude plugin uninstallleaves the CLI's record behind. When that file exists but cannot be read, the CLI's install state for the scope decides, as it does for the other agents. A global install counts only a machine-wide plugin. A project-scope install counts only a project-scope plugin of this project: its skill files are shared through the repository, while a user or local plugin is personal.databricks aitoolsis known. If one of them got the plugin another way (for example through its app or IDE) and its CLI is not on PATH, the fallback adds skill files next to the plugin and every skill shows twice.install --helpsays so too.--skills-only, since a named agent does not fall back.SUCCESS, orNOTHING_TO_DOwhen it did nothing else.--agents, and agents whose CLI is on PATH but whoseplugincommand fails, still fail with the exact commands to run.aitools update: no longer tries, and fails, to migrate an agent whose CLI is not on PATH from raw skills to the plugin; it keeps refreshing its raw skills. It also no longer writes raw skills to an agent whose CLI is not on PATH and whose own manifest shows the plugin for the scope, since install keeps such a plugin without a CLI record: it removes them instead, and skips the reconcile when no agent is left that can take file skills in the scope.installer.PluginRecorded(now also used for the User-Agent tooling),installer.ForgetPluginInstalls,agents.Agent.HasPluginManifestReader, and an exportedagents.Agent.DatabricksPluginActiveIn, which returns an error for an unreadable manifest.Why
Desktop and IDE installs of Claude Code have the config dir but no
claudeCLI, so a plaindatabricks aitools installset nothing up for them. Skill files are what such an agent can use today, and a later run with the CLI on PATH switches it to the plugin. An agent that already has the plugin keeps it, because skill files next to the plugin would list every skill twice.Tests
acceptance/experimental/aitools/skills/install-non-interactive: the detected-agent fallback in one skills batch, a kept plugin that removes the earlier skill files and anaitools update --forcethat does not write them back, the named-agent failure and its--skills-onlyrecovery, a project-scope fallback that a later user-scope plugin does not stopaitools update --scope=projectfrom refreshing, and a project-scope run that sets up nothing exiting 1.executePlanfor both cases, the funnel outcomes, the scope rules and unreadable manifest inpluginAlreadyInstalled, the project-plugin warning,ForgetPluginInstalls/PluginRecorded, the manifest reader, the project-scope plugin rule (DatabricksProjectPluginIn,excludePluginAgents), the update migration skip, and a project-scope update whose only project-capable agent has the plugin../task test-exp-aitools,./task lint-q,./task checkspass locally on Linux. Not run on Windows or macOS.Changelog fragment:
.nextchanges/cli/aitools-install-skills-fallback.md.This pull request and its description were written by Isaac.