Repository navigation
aitools: fail install, update and uninstall when nothing was done - #7481
Conversation
23225b1 to
e0ad83a
Compare
54c4bc5 to
75d4a60
Compare
|
Commit: Regression test reportSelected 1 changed tests (limit=20, 0 not selected) 1 tests: regression=1 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (532408b) ❌ | latest (v1.26.0) ❌main (532408b): latest (v1.26.0): |
✅ Integration test reportCommit: fd3519d
Top 6 slowest tests (at least 2 minutes):
|
e0ad83a to
06eddb3
Compare
75d4a60 to
7d9546e
Compare
06eddb3 to
0254738
Compare
7d9546e to
a2b82bc
Compare
0254738 to
4c846ec
Compare
a2b82bc to
0758e7e
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 0758e7e. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR makes databricks aitools install exit 1 when every targeted agent is skipped or fails, so a caller can't mistake a no-op for success. The exit-code change works, and tests and CI are green so far. The supporting code needs cleanup: the per-agent cause wrapper is never read in production, the old nothingInstalledCategory branch is now dead, a few comments and the changelog line overstate what changed, and the explicit --agents unit test no longer catches a regression. Several other "nothing set up" paths still exit 0, so please say whether that split is intended.
Questions
- After this change, exit code 1 applies only when agents were targeted non-interactively and none installed. Several other runs that set nothing up still exit 0: no agents detected (install.go:235-238); an interactive run where every detected agent is skipped for the scope (the same
--skills-only --scope=projectinput exits 0 in a terminal and 1 without one); a--pathdump that writes no skills; andupdate/uninstallwhen every attempt fails. Is that split intended? If so, could the changelog and help text say which cases fail? If not, should those paths follow the same rule?
Comments outside the changed lines
- [nice to have]
cmd/aitools/install.go:281:executePlannow returns nil only when at least one outcome isoutcomeInstalled, and the plan is never empty here: "no agents detected" returns at line 238, andresolveAgentNamesrejects an empty list. SonothingInstalledCategory(outcomes)incase runErr == nilalways returns false. The branch,nothingInstalledCategory(telemetry.go:464-479) andTestNothingInstalledCategoryare now dead. The helper's doc still describes the exit-0 warning this PR removes, and the newslices.ContainsFunccheck at line 675 repeats its loop. Suggest deleting the branch and the helper, or replacing them with oneanyInstalled(outcomes)helper used by the check at line 675. - [nit]
cmd/aitools/install.go:537: TheagentErrorsdoc says it "wraps the failures of explicitly named agents", and the comment at lines 283-284 says "Named agents failed or were skipped".agentErrorsnow also wraps the nothing-installed error for auto-detected agents, so both are incomplete. A reader could taketopLevelFailure(err) == nilto mean--agentswas passed and add handling, such as recovery text naming--agents, that misfires for detected-agent runs. Suggest updating both comments to cover the nothing-installed case. - [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary/## Evidence/## Merge Danger..agents/skills/pr-checklist/SKILL.mdasks for the.github/PULL_REQUEST_TEMPLATE.mdsections (## Changes,## Why,## Tests), in order, with no invented sections, and a concise summary. The Evidence block also cites commit 3c7a591cd rather than the current head. Suggest reshaping the description into the template sections.
| // nothingInstalledError is errNothingInstalled carrying the per-agent causes, | ||
| // so classifyInstallError reports the first agent's category instead of | ||
| // UNCATEGORIZED. Its message stays errNothingInstalled's. | ||
| type nothingInstalledError struct { | ||
| causes []error | ||
| } | ||
|
|
||
| func (e *nothingInstalledError) Error() string { return errNothingInstalled.Error() } | ||
| func (e *nothingInstalledError) Unwrap() []error { | ||
| return append([]error{errNothingInstalled}, e.causes...) | ||
| } | ||
|
|
||
| // newNothingInstalledError collects the failed and skipped outcomes as causes, | ||
| // tagged with the category each one already carries. | ||
| func newNothingInstalledError(outcomes []agentOutcome) error { | ||
| var causes []error | ||
| for _, o := range outcomes { | ||
| if o.status != outcomeInstalled { | ||
| causes = append(causes, withCategory(errors.New(o.message), o.errorCategory)) | ||
| } | ||
| } | ||
| return ¬hingInstalledError{causes: causes} | ||
| } |
There was a problem hiding this comment.
[nice to have] nothingInstalledError and its per-agent causes are never read outside TestNothingInstalledCarriesThePerAgentCategory. The error is always wrapped in agentErrors, so topLevelFailure returns nil for it in logInstallEvent, buildInstallOutput and the RunE switch. The FINISHED category then comes from firstFailureCategory(outcomes) (install.go:285), and the deferred funnel.finish does nothing after that. Changing install.go:676 to &agentErrors{err: errNothingInstalled} keeps every cmd/aitools test green and leaves the install-failure acceptance output unchanged.
Three places point readers at the wrong mechanism: this comment, the one at funnel_test.go:389-390, and the PR description. The cause list also has its own first-category rule, which can drift from firstFailureCategory: a cause with an empty category wins here but is skipped there. Suggest returning the bare sentinel and deleting the type, newNothingInstalledError, and its test.
There was a problem hiding this comment.
Done in 3f574d9. nothingInstalledError, newNothingInstalledError and TestNothingInstalledCarriesThePerAgentCategory are gone. executePlan now returns &agentErrors{err: errNoAgentSetUp}, the bare sentinel. The FINISHED category comes from firstFailureCategory(outcomes), so there is only one first-category rule. I fixed the funnel_test.go comment and the PR description to match.
| // Explicit (--agents) blocked install is an error. | ||
| planExplicit := buildPlan([]*agents.Agent{claude}, installer.ScopeGlobal, false, true) | ||
| _, err = executePlan(ctx, nil, planExplicit, installer.InstallOptions{Scope: installer.ScopeGlobal}, false) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
[nice to have] planExplicit holds only the failing Claude Code item, so the new nothing-installed check returns an error even if the it.explicit handling is removed. I deleted the explicitErrs append in the plugin loop of executePlan and the whole cmd/aitools suite still passed. At base this assertion would have caught that, and the other explicit tests (for example TestInstallOutputJSONReportsErrorCategories) also use a single failing agent.
Suggest planning Claude Code and Codex as explicit (Codex succeeding) and asserting require.Error(t, err) plus assert.NotErrorIs(t, err, errNothingInstalled). Then a named agent's failure still has to be an error when another agent installs.
There was a problem hiding this comment.
Done in 3f574d9. planExplicit now names Claude Code (fails) and Codex (installs) as explicit, and the test asserts require.Error plus assert.NotErrorIs(t, err, errNoAgentSetUp). I checked that it now catches the regression: with the explicitErrs append in the plugin loop removed, TestExecutePlanSkipBlockedPluginExit0 fails.
| // errNothingInstalled is returned when every agent in the plan was skipped or | ||
| // failed. Each agent's reason and recovery step is printed above it, so the | ||
| // message only has to make the exit code non-zero. | ||
| var errNothingInstalled = errors.New("nothing was installed; follow the steps above for each agent") |
There was a problem hiding this comment.
[nit] Two pairs of names in this package mean opposite outcomes:
errNothingInstalled/nothingInstalledError: this install set up nothing → FAILURE, exit 1.installer.ErrNothingInstalled/nothingInstalledInScopeError(scope.go:321-326):commandFunnel.finishmaps these to NOTHING_TO_DO (telemetry.go:347).
An errors.Is against the wrong one silently flips the funnel outcome. The new error also lives in recovery.go, away from agentErrors and executePlan, though it has nothing to do with recovery steps. Suggest a distinct name such as errNoAgentSetUp, defined next to agentErrors in install.go.
There was a problem hiding this comment.
Done in 3f574d9. I renamed it to errNoAgentSetUp and moved it into install.go, next to agentErrors. The nothingInstalledError type is deleted (see the other thread), so the only names left with "nothing installed" in them are the NOTHING_TO_DO ones.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` now fails with exit code 1 when no coding agent was set up. ([#7481](https://github.com/databricks/cli/pull/7481)) | |||
There was a problem hiding this comment.
[nit] The entry says "fails with exit code 1 when no coding agent was set up", but two such runs still exit 0:
- A run that detects no agents prints "No supported coding agents found on PATH." and exits 0 (install.go:235-238).
- So does an interactive run where every detected agent is skipped for the scope.
Unless those paths change too, suggest narrowing the wording, e.g. "…now fails with exit code 1 when every detected coding agent is skipped or fails to install."
There was a problem hiding this comment.
Narrowed in 3f574d9: "databricks aitools install now fails with exit code 1 when it runs without prompting and every targeted coding agent is skipped or fails to install." This leaves out the no-agents-detected run and the interactive run, which still exit 0. The PR link at the end is kept.
There was a problem hiding this comment.
Update in 46180b9: the no-agent-detected run, the empty-picker run and the empty --path dump now also exit 1 (see the Question 1 reply). So the entry now lists every case: "databricks aitools install now fails with exit code 1 when it sets up nothing: no coding agent is detected, every agent is skipped or fails to install, or a --path dump writes no skill. databricks aitools update and databricks aitools uninstall now fail with exit code 1 when every update or removal they attempt fails." The PR link at the end is kept.
| shape := funnelShape(funnelEvents(ctx)) | ||
| assert.Equal(t, "FINISHED:FAILURE:UNSUPPORTED_SCOPE", shape[len(shape)-1]) |
There was a problem hiding this comment.
[nit] shape[len(shape)-1] panics with "index out of range [-1]" if no funnel events are logged. Checking only the last event also misses a missing or extra step, such as a spurious SKILLS_INSTALLED. Suggest asserting the full shape like the sibling TestInstallFunnelDetectedPluginFailureIsNotSuccess: []string{"STARTED", "FLAGS_VALIDATED", "SCOPE_RESOLVED", "AGENTS_SELECTED", "PLAN_CONFIRMED", "FINISHED:FAILURE:UNSUPPORTED_SCOPE"}.
There was a problem hiding this comment.
Done in 3f574d9. The test now asserts the full shape, STARTED, FLAGS_VALIDATED, SCOPE_RESOLVED, AGENTS_SELECTED, PLAN_CONFIRMED, FINISHED:FAILURE:UNSUPPORTED_SCOPE, the same way as TestInstallFunnelDetectedPluginFailureIsNotSuccess.
0758e7e to
3f574d9
Compare
|
Thanks for the review. The points outside the diff are addressed in 3f574d9:
I'll answer Question 1 separately. |
3f574d9 to
46180b9
Compare
|
On Question 1 (which runs that set nothing up should fail): no, the split was not intended. In 46180b9 every run that sets nothing up follows the same rule and exits 1:
The changelog fragment, the help text of all three commands (a short "exits with code 1 when ..." line), the PR description and the |
2db10d1 to
8938e12
Compare
46180b9 to
544ae4b
Compare
|
Correction after rebasing onto the updated #7480 (now 544ae4b). #7480 now records an empty |
0595a19 to
52e34eb
Compare
bac174e to
8772447
Compare
181bc21 to
e8d4f91
Compare
8772447 to
5e5073d
Compare
e8d4f91 to
2eca9b0
Compare
5e5073d to
cd0f02f
Compare
2eca9b0 to
cf6eeb8
Compare
cd0f02f to
696613f
Compare
cf6eeb8 to
3289cf7
Compare
696613f to
2b5410a
Compare
3289cf7 to
c6b1b5e
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
cmd/aitools/install.go:409: [nit] This case cannot run in production. agentChoices offers every agent in agents.Registry that has a delivery in the scope, detected or not, and five agents support project scope. So choices is never empty. If it did run, printNoAgentsMessage would say "No supported coding agents found on PATH" for agents that were found but cannot be used in this scope. Remove this case, or print a step about the scope.
| if check { | ||
| return fmt.Errorf("nothing was checked: %w", firstErr) | ||
| } | ||
| return fmt.Errorf("nothing was updated: %w", firstErr) |
There was a problem hiding this comment.
[nice to have] update now exits 1 when every attempt fails, also for a failure that no retry can fix. Example: the Codex plugin is recorded, and the user later removed the codex CLI. Each later databricks aitools update (from cron or an agent session) exits 1 with "nothing was updated". Before this PR, it exited 0 with a warning. uninstall cannot clear the record either: UninstallSkillsWithResult keeps a record when UninstallPluginForAgent fails with cli-not-on-path. So uninstall also exits 1 on every run (uninstall.go:117), and no step tells the user what to do. Count a missing CLI like an unknown agent (not an attempt), or let uninstall drop the record when the CLI is gone and print that step.
There was a problem hiding this comment.
This is intended, so I kept the rule. If the Codex plugin is recorded and the codex CLI is gone, update can't update it. Exiting 1 tells a cron job or an agent session that the install is stale; before this PR it looked like a successful update. The error carries the cause (nothing was updated: codex: cli-not-on-path), and the per-agent warning above it names the agent. A record for an agent this CLI doesn't know at all is not counted, because no command can act on it. A missing CLI can be fixed by reinstalling the CLI or by uninstalling once it's back, so it still counts as a failed attempt.
Letting uninstall drop the record when the CLI is gone, and printing that step, would be a good follow-up. It changes the installer's uninstall behavior, which is outside this PR.
There was a problem hiding this comment.
Thanks. I agree with the rule: a stale plugin record must not look like a successful update. I checked the code at 965ae5a. The error names the agent and the reason (codex: cli-not-on-path), and a record for an unknown agent does not count as an attempt.
Please open a follow-up issue for uninstall when the agent CLI is missing, and link it here. Then a user can clear the stale record without the agent CLI. This does not block this PR.
There was a problem hiding this comment.
Thanks. Noted as a follow-up for after this stack lands: let uninstall drop a plugin record when the agent CLI is missing, and print that step, so a stale record can be cleared without the CLI.
| @@ -286,8 +290,10 @@ Supported agents: ` + strings.Join(agents.SupportedNames(), ", "), | |||
| } | |||
| if len(targetAgents) == 0 { | |||
| printNoAgentsMessage(ctx, rerun) | |||
There was a problem hiding this comment.
[nit] With --skills-only, selectAgents detects agents by their config dir, not by PATH. So when no agent is detected in that mode, this message says "No supported coding agents found on PATH", which is not the reason. Use words that match the skills-only detection.
There was a problem hiding this comment.
Fixed in 65864aa. With --skills-only, the message now reads "No supported coding agent config directories found." Without it, the message still says "No supported coding agents found on PATH." TestNoAgentsMessageMatchesTheDetection covers both.
There was a problem hiding this comment.
Verified at 965ae5a. With --skills-only, the message is now "No supported coding agent config directories found." Without the flag, the PATH message stays. TestNoAgentsMessageMatchesTheDetection passes. Thanks.
| names := make([]string, len(skillsAgents)) | ||
| for i, a := range skillsAgents { | ||
| names[i] = a.DisplayName | ||
| outcomes = append(outcomes, agentOutcome{agent: a, delivery: deliverySkills, status: outcomeSkipped, message: skillsErr.Error(), recovery: recovery}) |
There was a problem hiding this comment.
[nit] These skipped outcomes have no errorCategory, so agentResultsField sends "error_category": "" for each of them. An empty string is not an AitoolsErrorCategory value, and every other skipped outcome sets one. Give these outcomes a category, and set the same category where classifyInstallError meets ErrNoSkillsResolved, so the JSON and telemetry agree.
There was a problem hiding this comment.
Fixed in 65864aa. These skipped outcomes now get classifyInstallError(installer.ErrNoSkillsResolved), the category the run's own error gets when no plugin was installed. The JSON output and telemetry now agree, and no agent result has an empty error_category. TestExecutePlanNoSkillsToInstall asserts it.
There was a problem hiding this comment.
Verified at 965ae5a. The skipped skills outcomes now get classifyInstallError(installer.ErrNoSkillsResolved). That is the same category as the top-level JSON error_category for this error, so the JSON and telemetry agree. No entry has an empty category. TestExecutePlanNoSkillsToInstall asserts it. Thanks.
| return err | ||
| } | ||
| if len(targetSkills) == 0 { | ||
| return ErrNoSkillsResolved |
There was a problem hiding this comment.
[nit] apps init also calls this function, through InstallAllSkills and RecommendSkillsInstall. When every skill is filtered out, apps init now logs "Skills installation failed: no skills to install" after the user said yes. Before this PR, it did nothing. Handle ErrNoSkillsResolved in RecommendSkillsInstall, or name this change in Blast Radius.
There was a problem hiding this comment.
Fixed in 65864aa. InstallAllSkills, the path apps init uses through RecommendSkillsInstall, now handles ErrNoSkillsResolved. It prints "No skill in this release can be installed by this CLI." and returns nil, so apps init no longer logs a skills-install failure for it. The aitools install exit code is unchanged.
There was a problem hiding this comment.
Verified at 965ae5a. For ErrNoSkillsResolved, InstallAllSkills now prints "No skill in this release can be installed by this CLI." and returns nil. So apps init does not log a skills-install failure. The apps-init-tip acceptance test passes. Thanks.
2b5410a to
c8c2ccf
Compare
c6b1b5e to
a07cded
Compare
a07cded to
65864aa
Compare
c8c2ccf to
1519a26
Compare
65864aa to
be7c38c
Compare
1519a26 to
ec2d35f
Compare
be7c38c to
b4a2ce8
Compare
|
On the review body nit about install.go:409, the empty-picker case: agreed, it is unreachable today, since The inline points are answered in their threads; the fixes are in 65864aa. |
ec2d35f to
2ac7f12
Compare
b4a2ce8 to
85990ef
Compare
2ac7f12 to
8997feb
Compare
85990ef to
c59988f
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Follow-up review at 965ae5a.
I checked the fixes for my comments against the code. All three fixes are correct. The rebase onto the new #7480 changes no logic in this PR. go test passes for ./cmd/aitools/..., ./libs/aitools/..., ./libs/telemetry/... and ./cmd/apps/.... The aitools acceptance tests pass, and all CI checks are green.
The empty-picker nit at cmd/aitools/install.go:409: I agree. The guard comes from a lower PR in the stack, so a follow-up is correct.
For the update exit code, I accept the rule. Please link a follow-up issue for uninstall when the agent CLI is missing.
LGTM.
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 965ae5a. Each finding was verified against the code; simonfaltum may follow up.
No blocking issues found, approving. 13 non-blocking comment(s) below.
This PR makes aitools install exit 1 when it sets up nothing, and makes update and uninstall exit 1 when every attempt they make fails. The earlier review points are all fixed or answered. At 965ae5a the unit and acceptance tests, lint and CI pass, and the exit-code decisions match the description. One gap is worth fixing. When no skill in the release resolves and untracked skills are on disk, a plugin can be installed without being recorded while the run reports success. The rest are nits: unknown-agent records and joined errors pick the reported cause or category, the changelog is broader than the help text, and some counts, docs and tests need small fixes. Nothing here blocks the merge.
Accepted answers
- Telemetry outcome when no agent is detected: #7481 (comment)
- Exit-code change for best-effort scripts: #7481 (comment)
- Split update/uninstall into their own PR?: #7481 (comment)
- Use the repo PR template: #7481 (comment)
- update/uninstall exit 1 when the agent CLI is missing: #7481 (comment)
- Category for skipped no-skill outcomes: #7481 (comment)
| if skillsExplicit { | ||
| explicitErrs = append(explicitErrs, &displayError{msg: strings.Join(names, ", ") + ": " + skillsErr.Error(), err: skillsErr}) | ||
| } | ||
| skillsErr = nil |
There was a problem hiding this comment.
[nice to have] Setup: global scope has raw skills on disk without install state (a pre-state install), a detected plan mixes a plugin agent and a skills agent, and no skill in the release can be installed. InstallSkillsForAgents returns ErrNoSkillsResolved without writing state, so the check at line 704 (skillsErr != nil && HasUntrackedSkills) skips recording the plugin that was just installed. This line then clears skillsErr, and the run exits 0. Later update and uninstall fail with the legacy-install error and cannot manage the plugin, and re-running install repeats the same result, since the "next install whose skills step succeeds" can't happen for this release. At the base, the zero-skill install saved state and the plugin was recorded. Suggest clearing skillsErr only when the plugin records were saved, and otherwise keeping the failure, or printing that the plugin was not recorded.
There was a problem hiding this comment.
Fixed in 0e1a08c. When the plugin was installed but not recorded (skills without install state are on disk), executePlan now keeps ErrNoSkillsResolved instead of clearing it. The run exits 1 rather than reporting success for a plugin that update and uninstall can't manage. The skills agents are only turned into skipped outcomes when no plugin installed, or when the installed plugins were recorded.
| } | ||
| for _, pu := range pluginUpdates { | ||
| if pu.Err != nil { | ||
| attemptFailed = attemptFailed || !errors.Is(pu.Err, installer.ErrUnknownAgent) |
There was a problem hiding this comment.
[nit] The PR says a plugin record for an unknown agent is not an attempt, but update still lets one decide the outcome in two places. (1) firstErr keeps the first plugin error, and updates come back sorted by name. With an unknown aider record and a codex record whose CLI is missing, the run exits 1 with nothing was updated: unknown agent "aider" in state and records UNKNOWN_AGENT, hiding Codex's CLI_NOT_ON_PATH. uninstall takes the first counted failure (attempted[0]). (2) At line 139, a failed ref lookup sets attemptFailed for any plugin record. So a state holding only an unknown-agent record exits 1 when the release lookup fails (e.g. rate-limited), and exits 0 when it succeeds. Suggest keeping the first error that is not ErrUnknownAgent for the message and category, and counting only known-agent records at line 139.
There was a problem hiding this comment.
Fixed in 0e1a08c, both cases. Update now keeps firstAttemptErr, the first error that is not ErrUnknownAgent, for the message and category. With aider (unknown) and codex (CLI missing), the run reports Codex's CLI_NOT_ON_PATH; there is a new funnel test row for it. A failed ref lookup now counts as an attempt only when the state records a plugin for a known agent (hasKnownPluginRecord, with a unit test).
|
|
||
| // The skills error has no per-agent entry, so it stays the top-level error; | ||
| // explicit per-agent failures are already in their outcomes. | ||
| if skillsErr != nil { |
There was a problem hiding this comment.
[nit] When every plugin install fails (pluginCount == 0) and the skills agents get ErrNoSkillsResolved, executePlan returns the bare skills error here. funnel.finish then maps it to NOTHING_TO_DO, although a plugin install was attempted and failed. Example: Claude Code (plugin) and Cursor (skills) are detected, claude plugin install fails, and every skill in the release is experimental. The run exits 1, but FINISHED says there was nothing to do instead of FAILURE with PLUGIN_INSTALL_FAILED. Suggest turning ErrNoSkillsResolved into skipped outcomes for the skills agents whenever the plan had plugin agents (len(pluginItems) > 0), and letting the existing explicitErrs / !anyInstalled checks decide. Keep NOTHING_TO_DO for a --skills-only run with nothing else attempted.
There was a problem hiding this comment.
Fixed in 0e1a08c. The skills agents now become skipped outcomes whenever the plan has plugin agents, so explicitErrs and !anyInstalled decide the run. In your example the run finishes FAILURE with PLUGIN_INSTALL_FAILED via errNoAgentSetUp. A --skills-only run with nothing else attempted keeps NOTHING_TO_DO. Covered by TestExecutePlanNoSkillsToInstallWithPluginFailures.
| case err == nil: | ||
| f.finishWith(ctx, protos.AitoolsCommandOutcomeSuccess, protos.AitoolsErrorCategoryUnspecified) | ||
| case errors.Is(err, installer.ErrNothingInstalled): | ||
| case errors.Is(err, installer.ErrNothingInstalled), errors.Is(err, installer.ErrNoSkillsResolved): |
There was a problem hiding this comment.
[nit] When a plugin installs, no skill resolves, and recordPluginInstallsFn fails (e.g. EPERM on the state file), executePlan returns errors.Join(skillsErr, err) (install.go:719). This case matches the sentinel inside the joined error and records NOTHING_TO_DO with no category. installRecovery also matches it first and prints "No skill in this release can be installed... add --experimental" instead of the permission step, while the JSON error_category says PERMISSION_DENIED. Suggest returning the record error without joining ErrNoSkillsResolved once a plugin was installed, or matching the sentinel only when it is the sole error.
There was a problem hiding this comment.
Fixed in 0e1a08c. When the skills step returned ErrNoSkillsResolved and saving the plugin records fails, executePlan now returns only the record error, so the category and recovery step are the permission ones. The same test covers it.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install`, `update` and `uninstall` now exit with code 1 when they do nothing. `install` fails when it sets up nothing: no coding agent is detected, every agent is skipped or fails to install, or no skill in the release can be installed (also with `--path`). `update` fails when every update or check it attempts fails, and `uninstall` when it removes no skill or plugin. Scripts that ran these commands as a best-effort step and relied on exit code 0 should handle the new exit code. ([#7481](https://github.com/databricks/cli/pull/7481)) | |||
There was a problem hiding this comment.
[nit] The fragment says the three commands "exit with code 1 when they do nothing", and that uninstall fails "when it removes no skill or plugin". uninstall --agents=cursor when Cursor has none of the skills removes nothing and exits 0 (NOTHING_TO_DO). A state holding only an unknown-agent plugin record also exits 0 for both commands. The help text this PR adds is accurate ("every removal it attempts fails"). Since scripts will read this notable change, suggest matching the help text: update fails when every update or check it attempts fails, and uninstall when every removal it attempts fails.
There was a problem hiding this comment.
Reworded in 0e1a08c to match the help text: "update fails when every update or check it attempts fails, and uninstall when every removal it attempts fails." The lead sentence now says they exit 1 "when they do nothing they were asked to". The PR link is kept.
| return nil | ||
| // --experimental): there was nothing to write. The run still | ||
| // fails, so a caller does not take the empty directory for skills. | ||
| return withRecovery(ctx, installInvocation{path: pathFlag, skills: skillsFlag, experimental: includeExperimental}, installer.ErrNoSkillsResolved) |
There was a problem hiding this comment.
[nit] The description says a release with no installable skill now fails a --skills-only install "as it already did for --path". At the base (bd05057), an empty --path dump recorded NOTHING_TO_DO and returned nil, so it exited 0. This line is what makes it exit 1. The changelog fragment is right ("also with --path"). Suggest naming the --path exit-code change in the summary and in Merge Danger.
There was a problem hiding this comment.
Fixed in the PR description: the summary and Merge Danger now say that an empty --path dump used to exit 0 and now exits 1.
| // NOTHING_TO_DO when update or uninstall found nothing installed or no skill in | ||
| // the release can be installed (the command still exits non-zero), CANCELLED | ||
| // when the user aborted a prompt or picked no agents, FAILURE otherwise. No-op once the run has finished, so it is safe to |
There was a problem hiding this comment.
[nit] Line 73 is 123 characters because the comment was not rewrapped after the edit. The sentence also says NOTHING_TO_DO is recorded "when update or uninstall found nothing installed or no skill in the release can be installed", but the no-skill case comes from install. Suggest rewrapping and saying it covers update/uninstall with nothing installed, and an install where no skill in the release can be installed.
There was a problem hiding this comment.
Fixed in 0e1a08c: the comment is rewrapped and now attributes the no-skill case to install.
| // act on it. | ||
| failures := slices.Concat(result.PluginFailures, result.SkillFailures) | ||
| if result.PluginsRemoved == 0 && result.SkillsRemoved == 0 { | ||
| attempted := slices.DeleteFunc(slices.Clone(failures), func(err error) bool { return errors.Is(err, installer.ErrUnknownAgent) }) |
There was a problem hiding this comment.
[nit] slices.DeleteFunc(slices.Clone(failures), ...) copies and filters the whole slice, and then only attempted[0] and len(attempted) are read. Suggest i := slices.IndexFunc(failures, func(err error) bool { return !errors.Is(err, installer.ErrUnknownAgent) }) and using failures[i] when i >= 0. The same helper could also fix the update.go unknown-agent message.
There was a problem hiding this comment.
Done in 0e1a08c: uninstall uses slices.IndexFunc, and failures[i] when i >= 0. Update gets the same result by tracking firstAttemptErr as it goes (see the update.go thread).
| // An optional setup step (apps init) has nothing to install; that is no | ||
| // failure for it. | ||
| if errors.Is(err, ErrNoSkillsResolved) { | ||
| cmdio.LogString(ctx, "No skill in this release can be installed by this CLI.") |
There was a problem hiding this comment.
[nit] "No skill in this release can be installed by this CLI." is written out here and again in cmd/aitools/recovery.go:193. .agents/rules/style-guide-go.md asks for a named constant when a string is used in more than one place, so a wording change can't update one copy and miss the other. Suggest an exported constant next to ErrNoSkillsResolved, used by both.
There was a problem hiding this comment.
Done in 0e1a08c: installer.NoSkillsResolvedMessage sits next to ErrNoSkillsResolved, and both InstallAllSkills and the recovery step use it.
| installCmd.SetContext(cmd.Context()) | ||
| // Root prints the returned error; the detached command must not print | ||
| // it, or its usage, a second time. | ||
| installCmd.SilenceErrors = true |
There was a problem hiding this comment.
[nit] Two new behaviors have no test. (1) The SilenceErrors/SilenceUsage lines here make the deprecated alias print its error once and no usage block; nothing in legacy_skills_test.go asserts that. (2) The ErrNoSkillsResolved branch in InstallAllSkills (libs/aitools/installer/installer.go:585) makes apps init treat no skill to install as nothing to do. Removing either change keeps the suite green. Suggest one test that runs the alias with no agent detected and asserts a single Error: line, and one that calls InstallAllSkills against an all-experimental manifest and asserts nil.
There was a problem hiding this comment.
Added both in 0e1a08c. TestLegacySkillsInstallPrintsNoErrorOrUsageItself runs the alias with no agent detected and asserts no Error: or Usage: output from the detached command; it fails without the Silence* lines. TestInstallAllSkillsWithNoSkillToInstall runs InstallAllSkills against an all-experimental manifest and asserts nil plus the message.
`databricks aitools install` used to exit 0 when it set up nothing, so an agent calling it believed the install worked. `update` and `uninstall` did the same when every attempt failed. All three now exit with code 1 in that case.
```diff
install
- no agent detected -> message, exit 0
+ no agent detected -> message, errNoAgentSetUp (NOTHING_TO_DO)
executePlan
if an agent named via --agents failed
return its error
+ if no agent was installed
+ return errNoAgentSetUp # "nothing was installed; follow the steps above"
- no skill in the release to install -> "Installed 0 skills.", exit 0
+ no skill in the release to install -> installer.ErrNoSkillsResolved + step (NOTHING_TO_DO)
update / uninstall
- every attempt failed -> warnings, exit 0
+ every attempt failed -> "nothing was updated|checked: <cause>" / "could not remove any skill or plugin: <cause>"
```
- The FINISHED event keeps the categories it recorded before: the first failed or skipped agent's, or the first cause's for update and uninstall. A run with no agent detected, and one with no skill to install (also with `--path`), finish NOTHING_TO_DO while still exiting 1. When the plan has plugin agents, a release with no skill to install skips the skills agents and the plugin outcomes decide the run. An empty `--path` dump, which exited 0 before, now fails without printing "Wrote 0 skills". The optional skills step of `apps init` treats no skill to install as nothing to do, not a failure.
- `nothingInstalledCategory` is gone: the returned error now carries the outcome. `anyInstalled` replaces it.
- A plugin record for an agent this CLI does not know is not counted as a failed attempt, and does not pick the error or category: no command can act on it, so update and uninstall keep exiting 0 for it.
- Uninstall decides from `UninstallResult.SkillsChanged`, which replaces `SkillsRemoved`: the skills of which the run removed a copy an agent reads (a symlink, an entry, or the canonical dir). With `--agents`, removing only the canonical dir does not count when a targeted agent keeps its entry. "Uninstalled N skills." no longer counts skills whose removal failed.
- `update --check` no longer prints "No changes." when the manifest could not be fetched.
- The legacy `skills install` alias silences cobra's own error and usage output, so the new error prints once.
- The help text of install, update and uninstall says when they exit 1.
- Changelog fragment: `.nextchanges/notable-changes/aitools-install-nothing-installed-fails.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
❌ Integration test reportCommit: eebf099
59 interesting tests: 47 FAIL, 6 KNOWN, 6 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
Stacked on #7480.
Stacked on: aitools: print a recovery step for every install failure
Summary
databricks aitools installused to exit 0 when it set up nothing, so an agent calling it believed the install worked.updateanduninstalldid the same when every attempt failed. All three now exit with code 1 in that case. They stay in one PR because they follow one rule: a run that did nothing fails.errNoAgentSetUpfollows the reasons and recovery steps printed above it, so it adds no step of its own. InexecutePlanit is wrapped inagentErrors, so the JSON output and the install event don't report it as a top-level failure.--experimental) or needs a newer CLI now fails a--skills-onlyinstall and a--pathdump. An empty--pathdump used to exit 0; it now exits 1 without printing "Wrote 0 skills". Nothing is written, and the step reads: "No skill in this release can be installed by this CLI. Re-run '... --debug' to see why each skill was skipped. To include experimental skills, add --experimental." When the plan also has plugin agents, the skills agents are skipped with the same error category as the failed run, and the plugin outcomes decide the run. If an installed plugin could not be recorded, the run still fails. The optional skills step ofapps inittreats this as nothing to do and doesn't log a failure.UninstallResult.SkillsChanged, which replacesSkillsRemoved. It counts the skills of which the run removed a copy an agent reads: an agent's symlink or entry, or the canonical dir. Both paths follow this rule. With--agents, removing only the canonical dir doesn't count when a targeted agent keeps its entry. "Uninstalled N skills." now counts only skills of which a copy was removed or whose state record was dropped without a failure.update --checkexits 1 when it could not fetch the manifest and nothing else ran, and no longer prints "No changes." in that case.experimental aitools skills installalias silences cobra's own error and usage output, so its new error prints once.--skills-only, the no-agent message says no agent config directory was found instead of blaming PATH, since that mode detects agents by config dir..nextchanges/notable-changes/aitools-install-nothing-installed-fails.md, so the release notes call out the new exit codes.Evidence
cmd/aitools/telemetry/install-failurehas a new case: only~/.cursorexists and--scope=projectis set, so nothing can be installed. The script unsets the host's agent config overrides, so agents on the machine running the test are not detected. Runs that end before the plan (no agent detected, no skill to install) upload telemetry depending on timing, so the funnel unit tests cover them. Unit tests also cover a zero-skills install with and without a plugin, update and uninstall where every attempt fails, unknown-agent records, and the uninstall symlink and--agentscounting.Merge Danger
Door: two-way
This is a behavior change. Scripts, dev containers or CI images that ran
aitools installas a best-effort step and got exit 0 when nothing was set up now get exit 1. That covers no agent detected, every agent skipped or failed, and a release with no skill to install, including an empty--pathdump. The deprecatedexperimental aitools skills installalias changes the same way.aitools updateandaitools uninstallnow exit 1 when every update, check or removal they attempt fails. A run that sets up, updates or removes at least one thing still exits 0, with warnings for the rest. Reverting the change restores the old exit codes.Blast Radius: small
Only the exit codes of
aitools install,updateanduninstallchange, plus the error text printed for these runs. Telemetry changes in a few places:--skills-onlyinstall with no skill to install used to finish SUCCESS; it now finishes NOTHING_TO_DO.uninstall --agents=a,bthat removes one agent's copy while another targeted agent keeps its entry used to finish FAILURE; it now finishes SUCCESS.uninstallthat removes the agents' symlinks but not the canonical dir used to finish FAILURE; it now finishes SUCCESS.uninstall --agents=xwhere x's entry can't be removed but the canonical dir could be is FAILURE, as at the base.There are no new network calls. Tested on Linux only; not run on Windows or macOS.
This pull request and its description were written by Isaac.