Repository navigation
aitools: print a recovery step for every install failure - #7480
Conversation
891a0e3 to
e15f2f9
Compare
23225b1 to
e0ad83a
Compare
|
Commit: Regression test reportSelected 4 changed tests (limit=20, 0 not selected) 4 tests: regression=4 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (eee3c42) ❌ | latest (v1.26.0) ❌main (eee3c42): latest (v1.26.0): TestAccept/experimental/aitools/skills/install-output-json-agent-error: branch ✅ | main (eee3c42) ❌ | latest (v1.26.0) ❌main (eee3c42): latest (v1.26.0): TestAccept/experimental/aitools/skills/install-output-json-agents-skipped: branch ✅ | main (eee3c42) ❌ | latest (v1.26.0) ❌main (eee3c42): latest (v1.26.0): TestAccept/experimental/aitools/skills/install-output-json-error: branch ✅ | main (eee3c42) ❌ | latest (v1.26.0) ❌main (eee3c42): latest (v1.26.0): |
✅ Integration test reportCommit: c9931c1
Top 6 slowest tests (at least 2 minutes):
|
e15f2f9 to
0fb3e5c
Compare
e0ad83a to
06eddb3
Compare
0fb3e5c to
520f504
Compare
06eddb3 to
0254738
Compare
520f504 to
6709c89
Compare
0254738 to
4c846ec
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 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 + "." |
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.
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: |
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.
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 + "." |
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.
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) |
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.
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 { |
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.
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." |
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>.
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) |
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.
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)) |
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.
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 { |
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.
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") |
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.
There was a problem hiding this comment.
Fixed in 2781109: mirror.example.test (and ghe.example.test in the new release-host test).
e7d2703 to
912057b
Compare
4c846ec to
2781109
Compare
|
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. |
912057b to
5f9958b
Compare
2781109 to
255a5ca
Compare
5f9958b to
6f53d25
Compare
255a5ca to
2db10d1
Compare
6f53d25 to
bced029
Compare
d0d5fc2 to
55882e3
Compare
e745931 to
b501f02
Compare
55882e3 to
942cec5
Compare
b501f02 to
bac174e
Compare
942cec5 to
ddd4225
Compare
bac174e to
8772447
Compare
ddd4225 to
45b26f8
Compare
8772447 to
5e5073d
Compare
45b26f8 to
a3e42ce
Compare
5e5073d to
cd0f02f
Compare
a3e42ce to
cad677d
Compare
cd0f02f to
696613f
Compare
cad677d to
2adef03
Compare
696613f to
2b5410a
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at 2b5410a, on Simon's decision.
Your answer to the design question in the earlier automated reviews is accepted, so it no longer blocks. Part of the last automated review did not complete; this approval rests on the parts that did, plus a check of the changes since (4844908 → 2b5410a), which address its comments.
| if a.HasBinary(ctx) { | ||
| reason = fmt.Sprintf("'%s plugin' is not available (%s)", a.Binary, blocked.Detail) | ||
| } | ||
| return reason, fmt.Sprintf("Install or update the %s CLI so '%s' is on PATH, then re-run %s. Or install skill files instead: %s", a.DisplayName, a.Binary, agentRerun(inv, it), skillsOnlyCommand(inv, it)) |
There was a problem hiding this comment.
[nit] When the binary is on PATH (a.HasBinary(ctx)), the reason says "'claude plugin' is not available", for example for an old CLI without the plugin subcommand. But the step still says "Install or update the Claude Code CLI so 'claude' is on PATH", and the CLI is already on PATH. In that case, tell the user to update the CLI to a version that has plugin.
There was a problem hiding this comment.
Fixed in 8997feb: when the binary is on PATH, the step now says "Update the Claude Code CLI to a version with the 'claude plugin' command, then re-run '...'". The not-on-PATH case keeps its wording. TestPluginFailureTextCLIWithoutPluginCommand now checks the step too.
| } | ||
| return reason, fmt.Sprintf("Install or update the %s CLI so '%s' is on PATH, then re-run %s. Or install skill files instead: %s", a.DisplayName, a.Binary, agentRerun(inv, it), skillsOnlyCommand(inv, it)) | ||
| case blocked.Reason == installer.ReasonShimExitedWithError: | ||
| return fmt.Sprintf("'%s plugin --help' exited with an error", a.Binary), |
There was a problem hiding this comment.
[nice to have] For a shim that exited with an error, the text output prints only this reason and step (install.go:663-664). So it drops the stderr and the exit-code commands that #7476 puts in BlockedError.Detail; only the JSON message keeps them. #7476 says that a re-run can show only the help, so "Run 'codex plugin --help' to see the error" may show nothing new. Keep blocked.Detail in the reason, as the TimedOut and ReasonInstallFailed cases do.
There was a problem hiding this comment.
Fixed in 8997feb: the reason now keeps blocked.Detail, as the TimedOut and ReasonInstallFailed cases do: "'codex plugin --help' printed its plugin help but exited with an error (...): ; to see the error and exit code, run ...". The step no longer repeats "run ' plugin --help'". It now reads "Once it exits with 0, re-run '...'. Or install skill files instead: ...". Test case updated.
| return permissionRecovery(inv, err) | ||
| case protos.AitoolsErrorCategoryFilesystem: | ||
| return filesystemRecovery(inv, err) | ||
| default: |
There was a problem hiding this comment.
[nit] A Ctrl-C during a fetch is classified as COMMAND_INTERRUPTED, and that category has no case here. So it falls through to genericRecovery, which tells the user who cancelled to re-run with --debug and to report a bug. Return no step for COMMAND_INTERRUPTED, and add a test case.
There was a problem hiding this comment.
Fixed in 8997feb: COMMAND_INTERRUPTED now returns no step, so a Ctrl-C gets neither --debug nor a bug report. Added a table case for a fetch canceled mid-request.
2adef03 to
b9488bb
Compare
2b5410a to
c8c2ccf
Compare
b9488bb to
3e7a5e5
Compare
c8c2ccf to
1519a26
Compare
3e7a5e5 to
916c212
Compare
1519a26 to
ec2d35f
Compare
916c212 to
b6d9a96
Compare
2ac7f12 to
8997feb
Compare
b6d9a96 to
838c303
Compare
8997feb to
ef4ea8f
Compare
838c303 to
1eca02f
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>
❌ Integration test reportCommit: 532408b
56 interesting tests: 44 FAIL, 9 RECOVERED, 3 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
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.