Repository navigation
Conversation
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 14:34
891a0e3 to
e15f2f9
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 14:34
23225b1 to
e0ad83a
Compare
Contributor
|
Commit: Regression test reportSelected 15 changed tests (limit=20, 0 not selected) 15 tests: regression=11 unreleased=0 coverage=4 failing=0
TestAccept/cmd/aitools/install-host-metadata: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/apps-init-tip: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/hint: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/update-uninstall: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/workspace-unreachable: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): 4 more failure details omitted, see the full report in the run. |
Collaborator
✅ Integration test reportCommit: 696613f
Top 9 slowest tests (at least 2 minutes):
|
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 16:53
e15f2f9 to
0fb3e5c
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 16:53
e0ad83a to
06eddb3
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 16:55
0fb3e5c to
520f504
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 16:55
06eddb3 to
0254738
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 16:57
520f504 to
6709c89
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 16:57
0254738 to
4c846ec
Compare
simonfaltum
reviewed
Nov 11, 2026
simonfaltum
left a comment
simonfaltum
left a comment
Member
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 4c846ec. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR adds a recovery step to every databricks aitools install failure, in the error text and in a new recovery field of --output json, and the re-run commands repeat the run's flags with shell-aware quoting. The approach helps users, the tests pass, and CI is green. I found nothing blocking, but several steps are wrong or misleading for specific inputs. Examples: guard errors that already say what to do get a contradictory "report a bug" line, a dropped download is reported as "returned HTTP 200", the multi-agent --agents value is not quoted for PowerShell, and the skip, prompt, and fallback steps can widen the run or drop --experimental. There are also two design questions: whether recovery text should come from its own error switch, and whether the re-run flags should travel through the context.
Questions
- The recovery text comes from a second switch over the installer error types (
installRecovery/fetchRecovery), which mirrorsclassifyInstallError/fetchErrorCategoryin a different order. The two can already disagree: a rename that fails with EACCES is PERMISSION_DENIED in telemetry but gets the generic "report a bug" step. Do you want to keep a separate switch, key the recovery text off the categoryclassifyInstallErrorreturns, or write the advice into the errors where they are created, as the experimental-skill and legacy-guard errors already do? - The re-run flags travel through
context.WithValue, andctxis reassigned three times inRunE, although every reader (withRecovery,buildInstallOutput,pluginFailureTextviaexecutePlan) is reached fromRunE. Is the context value deliberate? PassinginstallInvocationas a parameter would make the dependency visible, and a missed assignment could not silently fall back to the bare command.
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.mdasks PR bodies to follow.github/PULL_REQUEST_TEMPLATE.md(## Changes,## Why,## Tests, in that order), and recent merged PRs do. This body uses## Summary,## Evidence, and## Merge Danger. Moving the content under the template headings keeps it consistent with the rest of the repo.
| if perr, ok := errors.AsType[*fs.PathError](err); ok && errors.Is(err, fs.ErrPermission) { | ||
| return fmt.Sprintf("Check that you own %s and can write to it (an earlier run with sudo can leave it owned by root), then re-run %s.", perr.Path, rerunCommand(ctx)) | ||
| } | ||
| return "Re-run " + rerunCommand(ctx, "--debug") + " to see details. If it keeps failing, report it at " + cliIssuesURL + "." |
Member
There was a problem hiding this comment.
[nice to have] Untyped installer errors that already include their own fix fall through to the catch-all, which tells the user to re-run the same command with --debug and report a bug. Example: with legacy skills on disk and no state file, databricks aitools install --skills-only --agents=cursor --skills=databricks-sql fails with "legacy install detected ...; run 'databricks aitools install' (without a skill name) first". It then adds "Re-run 'databricks aitools install --scope=global --agents=cursor --skills-only --skills=databricks-sql --debug' to see details. If it keeps failing, report it at https://github.com/databricks/cli/issues". That re-run fails the same way every time. The downgrade refusal ("set DATABRICKS_SKILLS_REF=...") gets the same line. Give these errors a type, or tag them where they are created, and return "" for them in installRecovery, as the experimental-skill case already does.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: the downgrade refusal and the legacy-install guard now return a new installer.RefusedError (message unchanged, still UNCATEGORIZED in telemetry), and installRecovery returns "" for it, so neither gets the --debug / report line.
| return host + " returned HTTP " + strconv.Itoa(ferr.StatusCode) + ", a server-side error. Wait a few minutes and re-run " + rerun + "." | ||
| case ferr.StatusCode == http.StatusForbidden || ferr.StatusCode == http.StatusProxyAuthRequired: | ||
| return host + " refused the download (HTTP " + strconv.Itoa(ferr.StatusCode) + "). A proxy or access policy may block it; ask your network admin to allow " + host + ", then re-run " + rerun + "." | ||
| case ferr.StatusCode != 0: |
Member
There was a problem hiding this comment.
[nice to have] FetchManifest and fetchSkillFile keep StatusCode: 200 when io.ReadAll fails, so a connection that drops mid-body lands in the StatusCode != 0 branch. The user sees "raw.githubusercontent.com returned HTTP 200. Re-run 'databricks aitools install --debug' to see the URL, and report it at https://github.com/databricks/cli/issues" for what is a network failure. Send statuses below 400 to the no-response step (default), or require StatusCode >= 400 in this branch. Add a table case with StatusCode: 200 and a read error.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: downloadRecovery sends any status below 400 to the no-response step ("Could not download Databricks skills from . Check that this machine can reach it ..."). Added the StatusCode: 200 + read-error table case.
|
|
||
| // skipRecovery returns the recovery step for an agent the plan skipped. | ||
| func skipRecovery(it agentPlanItem) string { | ||
| return "Re-run with --scope=global to set up " + it.agent.DisplayName + "." |
Member
There was a problem hiding this comment.
[nice to have] "Re-run with --scope=global to set up Cursor." asks the user to edit their own command, which can widen the run or fail. With --scope=project --agents=claude-code,cursor, Claude Code is installed in the project and Cursor is skipped; re-running with --scope=global also installs Claude Code's plugin at user scope. With the deprecated --project, adding --scope=global fails with "cannot use --scope with --project or --global". Print a complete command for just this agent instead: installCommand with the scope set to global, plus --skills-only when the run used it.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: the skip step is now a complete command for just that agent in the global scope, built from the run's flags, for example To set up Cursor in the global scope, run: databricks aitools install --scope=global --agents=cursor --skills-only. It always spells --scope, so a run that used the deprecated --project gets a command that works too.
| args = append(args, "--scope="+inv.scope) | ||
| } | ||
| if inv.agents != "" { | ||
| args = append(args, "--agents="+inv.agents) |
Member
There was a problem hiding this comment.
[nice to have] --agents is appended without quoting, but WindowsArg in this PR treats , as special, because PowerShell reads it as the array operator. On Windows, after a picker selection of two agents, the step prints databricks aitools install --scope=global --agents=claude-code,cursor --skills-only --skills="a,b": --skills is quoted and --agents is not. Pasted into PowerShell, the agents arrive as two arguments (--agents=claude-code and cursor), and cobra.NoArgs rejects the command. Pass the joined list through quoteArg(goos, ...) like --skills, and add a two-agent Windows case to TestRenderInstallCommandQuotesUserInput.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: --agents now goes through quoteArg(goos, ...) like --skills. Added a two-agent Windows case (--agents="claude-code,cursor" --skills-only --skills="a,b"), plus the POSIX one.
| // installCommand is the install command for one agent in the plan's scope. The | ||
| // scope is always spelled out: without it a non-interactive run defaults to | ||
| // global, which is not what a project-scoped run asked for. | ||
| func installCommand(it agentPlanItem, extra ...string) string { |
Member
There was a problem hiding this comment.
[nice to have] installCommand is a second builder for databricks aitools install lines, and it drops flags the run used. After databricks aitools install --agents=claude-code --experimental hits a plugin failure, the fallback prints databricks aitools install --agents=claude-code --scope=global --skills-only, which installs without the experimental skills. The two builders also put the flags in different orders (--agents ... --scope here, --scope ... --agents in renderInstallCommand). Build the per-agent commands from rerunInvocation(ctx) with agents set to this agent, through renderInstallCommand, so there is one builder.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: there is one builder now. A per-agent command is renderInstallCommand of the run's invocation with agents set to that agent, so --experimental, --skills and --output json carry over, and the flag order is the same everywhere (--scope ... --agents ...). Test: TestPluginFailureTextKeepsTheRunsFlags.
| // Without --skills the command holds no user-typed value, so it is always | ||
| // printed as a command. | ||
| inv.skills = "" | ||
| return step + rerunPhrase(runtime.GOOS, inv) + " to reinstall all skills and rebuild the state file. An install of named skills needs the state file." |
Member
There was a problem hiding this comment.
[nit] The state file also holds plugin records: RecordPluginInstalls writes to the same .state.json. After moving it aside and re-running a --skills-only command for Cursor, only Cursor's skill records come back. A Claude Code plugin recorded in the old file is no longer tracked, and aitools uninstall leaves it installed. "rebuild the state file" overstates this. Say that other agents set up in this scope need their install re-run too, for example databricks aitools install --scope=<scope>.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: "rebuild the state file" is gone. When the re-run is limited to some agents or to --skills-only, the step adds "The state file also tracked the other agents set up in this scope; re-run their install too, for example 'databricks aitools install --scope=global'".
| if e := topLevelFailure(runErr); e != nil { | ||
| out.Error = e.Error() | ||
| out.ErrorCategory = string(classifyInstallError(e)) | ||
| out.Recovery = installRecovery(ctx, e) |
Member
There was a problem hiding this comment.
[nit] In --output json mode, the top-level and per-agent recovery commands omit --output json. An agent that runs the suggested command gets text output instead of the JSON it parses. Record JSON mode in installInvocation and append --output json when rendering.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: installInvocation records JSON mode, and every rendered command, top level and per agent, appends --output json (see the updated install-output-json-* goldens).
| return skillRecovery(ctx, skerr) | ||
| } | ||
| if perr, ok := errors.AsType[*fs.PathError](err); ok && errors.Is(err, fs.ErrPermission) { | ||
| return fmt.Sprintf("Check that you own %s and can write to it (an earlier run with sudo can leave it owned by root), then re-run %s.", perr.Path, rerunCommand(ctx)) |
Member
There was a problem hiding this comment.
[nit] .agents/rules/style-guide-go.md asks for file paths in output to use forward slashes (filepath.ToSlash), so output is the same on every OS. The ownership step (line 185, perr.Path) and the state-file step (line 196, serr.Path) print the OS path. Wrap both in filepath.ToSlash.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: both paths go through filepath.ToSlash. The tests build their paths with filepath.FromSlash, so they check this on Windows too.
| if err == nil { | ||
| return nil | ||
| } | ||
| if _, ok := errors.AsType[*recoveryError](err); ok { |
Member
There was a problem hiding this comment.
[nit] AGENTS.md asks not to add checks for cases the callers rule out. Three checks here never fire. withRecovery checks whether the error already has a step, but none of its four call sites in install.go passes a wrapped error. installRecovery checks err == nil, which both callers already rule out. ManualPluginInstallCommands returns nil for agents without a plugin or binary (plugin.go:471), which cannot happen on the ReasonInstallFailed path; if it ever did, the step would print "Run '' to see the full error". Remove them, or add a comment saying why they are needed.
Contributor
Author
There was a problem hiding this comment.
Partly fixed in 2781109. Removed the "already has a step" check in withRecovery and the err == nil check in installRecovery. withRecovery keeps its nil check, because RunE also calls withRecovery(ctx, rerun, runErr) on success, when runErr is nil; the doc comment says so now. There is no check around ManualPluginInstallCommands to remove: InstallPluginForAgent returns early for an agent without a plugin, and a ReasonInstallFailed error only comes after the agent's CLI ran, so the list is never empty on that path.
| } | ||
|
|
||
| func TestFetchRecoveryNamesOverriddenHost(t *testing.T) { | ||
| t.Setenv("DATABRICKS_SKILLS_BASE_URL", "https://mirror.example.com/skills") |
Member
There was a problem hiding this comment.
[nit] AGENTS.md asks for fixture hosts under an RFC 2606 §2 TLD, and the repo convention is .test. mirror.example.com resolves. The URL is only parsed here, but mirror.example.test follows the rule at no cost.
Contributor
Author
There was a problem hiding this comment.
Fixed in 2781109: mirror.example.test (and ghe.example.test in the new release-host test).
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
2 times, most recently
from
November 11, 2026 19:42
e7d2703 to
912057b
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 19:51
4c846ec to
2781109
Compare
Contributor
Author
|
Thanks for the review. Every inline comment is addressed in 2781109 (replies in each thread). On the two questions and the note outside the diff: Question 1. I keyed the recovery text off the category. Question 2. It wasn't deliberate, and it's a parameter now. RunE builds one PR template. I kept the current layout. This PR is part of a stack whose descriptions all use the same Summary / Evidence / Merge Danger sections, and keeping them alike makes the stack easier to read side by side. The content maps onto the template: Summary is Changes and Why, Evidence is Tests. |
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 20:03
912057b to
5f9958b
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 20:03
2781109 to
255a5ca
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 20:07
5f9958b to
6f53d25
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 20:07
255a5ca to
2db10d1
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 21:04
6f53d25 to
bced029
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 22:27
3d99ac0 to
3cf1d79
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 22:32
e3135af to
5d98a15
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 22:36
3cf1d79 to
3d59637
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 22:39
5d98a15 to
12aee78
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 22:39
3d59637 to
7e52920
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 11, 2026 22:59
12aee78 to
08b09de
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 22:59
7e52920 to
ad1c106
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 00:40
08b09de to
4f38e11
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 00:46
ad1c106 to
4844908
Compare
simonfaltum
reviewed
Nov 12, 2026
simonfaltum
left a comment
simonfaltum
left a comment
Member
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 4844908. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: part of the automated review did not complete; 1 open design question.
This PR gives every failed databricks aitools install a next step in the text output and in a new recovery field of --output json. The re-run commands repeat the run's flags and are quoted for the user's shell. Every thread from the earlier rounds is addressed at this head, CI is green on Linux, macOS and Windows, and I found nothing blocking; only a small doc-comment nit remains. One question about the PR's scope is open: whether the top-level recovery steps and the JSON field should land separately from the per-agent steps.
Questions
- You explained earlier why the steps rebuild the full command. A related question is scope. This PR adds per-agent steps for skipped and failed agents, where users had no next step before. It also adds a step for every top-level failure, the
shellquote.WindowsArghelper, and a newrecoveryfield in--output json, which agent callers may start to depend on. Would you land the per-agent steps first and move the top-level steps and the JSON field to a follow-up, or keep them together? If they stay together, could the description say whetherrecoveryis meant to be a stable field?
| // PowerShell pass to the program as one argument, without expanding anything | ||
| // in it. It returns the argument unquoted unless it contains whitespace (any | ||
| // Unicode space: PowerShell splits arguments at a no-break space too), a | ||
| // cmd.exe command separator, or a |
Member
There was a problem hiding this comment.
[nit] The WindowsArg doc comment breaks early after "a cmd.exe command separator, or a" on line 46, so line 47 reads like a new sentence fragment. Reflow lines 45-47 to the same width as the rest of the comment.
Contributor
Author
There was a problem hiding this comment.
Fixed in 1f0c9c4: reflowed the whole WindowsArg paragraph to the comment's usual width; no wording changes.
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:23
4844908 to
1f0c9c4
Compare
Contributor
Author
|
Thanks. The doc-comment nit is fixed in 1f0c9c4. Question 1. I'd keep them together. The per-agent steps and the top-level steps share the same pieces: the run's invocation, the command builder and On stability: I added a sentence to the description. |
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 08:36
4f38e11 to
d0d5fc2
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
2 times, most recently
from
November 12, 2026 08:41
a37d96c to
e745931
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 08:48
d0d5fc2 to
55882e3
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:48
e745931 to
b501f02
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 08:51
55882e3 to
942cec5
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:51
b501f02 to
bac174e
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 09:00
942cec5 to
ddd4225
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 09:00
bac174e to
8772447
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 09:06
ddd4225 to
45b26f8
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 09:06
8772447 to
5e5073d
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 09:30
45b26f8 to
a3e42ce
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 09:31
5e5073d to
cd0f02f
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 10:01
a3e42ce to
cad677d
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 10:02
cd0f02f to
696613f
Compare
anton-107
force-pushed
the
aitools/skills-failure-keeps-plugins
branch
from
November 12, 2026 11:02
cad677d to
2adef03
Compare
A failed `databricks aitools install` now ends with a step the user (or an agent calling the CLI) can take, in the text output and in a new `recovery` field of `--output json` (top level and per agent).
```text
install RunE
rerun := the run's flags (scope, agents, skills, --path, --skills-only, --experimental, --output json)
... install ...
on error: withRecovery(ctx, rerun, err) # cmd/aitools/recovery.go
per-agent errors -> no extra step (each agent printed its own)
empty picker selection -> name --scope and --agents to skip prompts
switch classifyInstallError(err) # the telemetry category
USER_ABORTED, PROMPT_FAILED -> name --scope and --agents to skip prompts
NETWORK, TIMEOUT (fetch with no response), RELEASE_LOOKUP_FAILED, RATE_LIMITED,
MANIFEST_NOT_FOUND, MANIFEST_INVALID, SKILLS_FETCH_FAILED -> per host / HTTP status / missing release
STATE_INVALID -> move the state file aside, re-run (global scope: without --skills)
SKILL_NOT_FOUND, VERSION_INCOMPATIBLE -> 'databricks aitools list', or upgrade the CLI
EXPERIMENTAL_SKILL -> no extra step (the message names --experimental)
PERMISSION_DENIED -> check ownership (e.g. an earlier sudo run)
FILESYSTEM -> check the path is writable and has free space
otherwise -> RefusedError (downgrade, legacy guard): no extra step;
else re-run with --debug, report at github.com/databricks/cli/issues
per-agent plugin failure / skip -> exact agent CLI commands
(installer.ManualPluginInstallCommands) or the run's
command narrowed to that agent
```
- The recovery step is chosen from the category `classifyInstallError` returns, so the advice and the telemetry category cannot disagree.
- Every re-run command, including the per-agent ones, repeats this run's choices (scope, `--skills-only`, `--skills`, `--experimental`, `--output json`), so a copied command never widens to every agent or the global scope. Agents chosen by flag or in the picker are named; auto-detected agents are not (the retry detects them again). A skipped agent gets the command for just that agent in the global scope.
- `--path`, `--agents` and `--skills` values are quoted for the user's shell: POSIX quoting, or the new `shellquote.WindowsArg` for cmd.exe/PowerShell. If no quoting keeps a value literal (`$`, `` ` ``, `%`, `!`, `"`, control characters on Windows; control characters on POSIX), the step says "your original install command" instead of printing a command that could expand or run something. On Windows, `--path` is printed with forward slashes, so it pastes the same into cmd.exe, PowerShell and Git Bash. A command that contains a single quote is delimited with backticks instead of single quotes.
- The installer's downgrade refusal and legacy-install guard return the new `installer.RefusedError`; their messages already name the fix, so no generic step is added when the refusal is the failure. The legacy-install message now names `--scope=global --skills-only`, the install that rebuilds skills state now that plugins are the default.
- The step appended to an error drops its final period (error strings must not end with punctuation). The error still unwraps to the original, so the telemetry category does not change.
- "No supported coding agents found" now also says how to name an agent that was not detected.
- Changelog fragment: `.nextchanges/cli/aitools-install-recovery.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 11:02
696613f to
2b5410a
Compare
This branch has not been deployed
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
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.
Stacked on #7479.
Stacked on: aitools: keep installing plugins when the skills install fails
Summary
A failed
databricks aitools installnow ends with a step the user (or an agent calling the CLI) can take, in the text output and in a newrecoveryfield of--output json(top level and per agent).The
recoveryfield is a human-readable next-step string, not a stable machine contract: its wording can change, so callers should show it or pass it on, not parse it.classifyInstallErrorreturns, so the advice and the telemetry category cannot disagree.--skills-only,--skills,--experimental,--output json), so a copied command never widens to every agent or the global scope. Agents chosen by flag or in the picker are named; auto-detected agents are not (the retry detects them again). A skipped agent gets the command for just that agent in the global scope.--path,--agentsand--skillsvalues are quoted for the user's shell: POSIX quoting, or the newshellquote.WindowsArgfor cmd.exe/PowerShell. If no quoting keeps a value literal ($,`,%,!,", control characters on Windows; control characters on POSIX), the step says "your original install command" instead of printing a command that could expand or run something. On Windows,--pathis printed with forward slashes, so it pastes the same into cmd.exe, PowerShell and Git Bash. A command that contains a single quote is delimited with backticks instead of single quotes.installer.RefusedError; their messages already name the fix, so no generic step is added when the refusal is the failure. The legacy-install message now names--scope=global --skills-only, the install that rebuilds skills state now that plugins are the default..nextchanges/cli/aitools-install-recovery.md.Evidence
--pathbranch'sreturn withRecovery(...)temporarily changed back toreturn err(not committed):go test ./cmd/aitools/... ./libs/aitools/... ./libs/shellquote/...and./task test-exp-aitools(with thecmd/aitoolsandexperimental/aitoolsacceptance tests) pass on Linux. The acceptance goldens show the new lines, for example:Merge Danger
Door: two-way
Output only: no new network calls, waits, or state writes, and exit codes are unchanged.
Blast Radius: small
--output jsongains an optionalrecoveryfield, which is additive and meant for display, not parsing.shellquote.WindowsArg) is unit-tested withgoospassed explicitly, but these tests have not run on Windows or macOS.GOOS=windows/darwingo vetpasses.This pull request and its description were written by Isaac.