Repository navigation
Conversation
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 14:34
23225b1 to
e0ad83a
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 11, 2026 14:34
54c4bc5 to
75d4a60
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): 5 more failure details omitted, see the full report in the run. |
Collaborator
✅ Integration test reportCommit: 3289cf7
Top 10 slowest tests (at least 2 minutes):
|
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/nothing-installed-fails
branch
from
November 11, 2026 16:53
75d4a60 to
7d9546e
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/nothing-installed-fails
branch
from
November 11, 2026 16:55
7d9546e to
a2b82bc
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 11, 2026 16:57
0254738 to
4c846ec
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 11, 2026 16:58
a2b82bc to
0758e7e
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 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.
Comment on lines
+35
to
+57
| // 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} | ||
| } |
Member
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.
Contributor
Author
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.
Comment on lines
283
to
286
| // 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) |
Member
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.
Contributor
Author
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") |
Member
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.
Contributor
Author
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)) | |||
Member
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."
Contributor
Author
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.
Contributor
Author
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.
Comment on lines
+713
to
+714
| shape := funnelShape(funnelEvents(ctx)) | ||
| assert.Equal(t, "FINISHED:FAILURE:UNSUPPORTED_SCOPE", shape[len(shape)-1]) |
Member
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"}.
Contributor
Author
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.
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 11, 2026 17:27
0758e7e to
3f574d9
Compare
Contributor
Author
|
Thanks for the review. The points outside the diff are addressed in 3f574d9:
I'll answer Question 1 separately. |
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 11, 2026 19:33
3f574d9 to
46180b9
Compare
Contributor
Author
|
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 |
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
4 times, most recently
from
November 11, 2026 21:04
2db10d1 to
8938e12
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 11, 2026 21:11
46180b9 to
544ae4b
Compare
Contributor
Author
|
Correction after rebasing onto the updated #7480 (now 544ae4b). #7480 now records an empty |
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/nothing-installed-fails
branch
from
November 11, 2026 22:59
1303ceb to
21cc88b
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 00:46
ad1c106 to
4844908
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 01:00
21cc88b to
1c9dce4
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 1c9dce4. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 3 open design questions.
This PR makes databricks aitools install exit 1 when it sets up nothing, and makes update and uninstall exit 1 when every attempt fails. The core change works: I built the head commit, the new exit codes behave as described, and all CI checks pass. Nothing blocks merging, but three gaps remain. A --skills-only install that resolves zero skills still exits 0. uninstall --agents=X exits 0 when X's copy could not be removed. A plain uninstall reports "could not remove any skill" after it removed every agent's symlink. There are also a few nits and three open questions: the telemetry outcome when no agent is detected, how the exit-code change affects scripts and release notes, and whether update/uninstall belong in this PR.
Questions
- A run with no coding agent detected now finishes as FAILURE with CLI_NOT_ON_PATH. An empty --path dump also exits 1 but stays NOTHING_TO_DO, and the NOTHING_TO_DO doc in libs/telemetry/protos/aitools_events.go still lists "no coding agents detected". With --skills-only, detection doesn't look at PATH at all. Should this run keep NOTHING_TO_DO while still exiting 1, like the empty dump? Or should it stay FAILURE, with the proto doc updated and possibly a category of its own?
- Scripts, dev containers or CI images that run
databricks aitools installas a best-effort step on a machine with no detected agent used to get exit 0 and now get exit 1. The deprecatedexperimental aitools skills installalias changes the same way. Should the no-agent-detected case keep exit 0, failing only when agents were found but none was set up? Or should the changelog entry move to .nextchanges/notable-changes so the release notes call out the new exit codes? - The title and commit describe the install change, but the PR also changes the exit codes of
updateanduninstall, addsUninstallResult.SkillsChanged, and fixes the legacy alias's duplicate error output. AGENTS.md asks for one change per PR. Do you want to keep these together because they follow one rule, or split update/uninstall into their own PR so each can be reviewed and reverted on its own? If they stay together, could the title cover all three commands?
Comments outside the changed lines
- [nice to have]
libs/aitools/installer/uninstall.go:163: Without--agents,removeSymlinksFromAgentsremoves each agent's symlink but reports nothing back, so a skill counts as changed only when its canonical dir is removed. Repro at this head: install forclaude-code,cursor(global, so both get symlinks), make the canonical skill dir unremovable, and rununinstall --scope=global. Both symlinks and the state file are removed. The run then printsUninstalled 1 skill.followed byError: could not remove any skill or plugin: ... permission deniedand exits 1. The--agentspath treats a partial removal as a change and exits 0. Suggest havingremoveSymlinksFromAgentsreturn whether it removed anything and counting that inskillsChanged, so both paths follow one rule. - [nit]
cmd/aitools/uninstall.go: The description doesn't match the code in four places. (1) Blast Radius says telemetry categories are unchanged except for the no-agent run. But anuninstall --agents=a,bthat removes one agent's copy while another targeted agent keeps its entry now finishes SUCCESS. It finished FAILURE at the base; see the new funnel row "a targeted agent kept its entry, another's copy removed". (2) It saysfunnelAllAttemptsFailedis gone, but that name exists in neither the base nor the head. (3) It doesn't mention thelegacy_skills.gochange (SilenceErrors/SilenceUsageon the detached install command). (4) The "Before" transcript showsRe-run with --scope=global to set up Cursor., but the base printsTo set up Cursor in the global scope, run: databricks aitools install --scope=global --agents=cursor --skills-only. Suggest updating those lines.
Agreeing with existing threads
- [nice to have] --skills-only install that resolves zero skills still exits 0: #7481 (review)
- [nit] "Uninstalled N skills." printed above the all-failed error: #7481 (comment)
- [nit] The PR description doesn't use the repo template: #7481 (review)
| removed = true | ||
| } | ||
| } | ||
| if removed { |
Member
There was a problem hiding this comment.
[nice to have] With --agents, skillsChanged also counts the canonical-dir removal, so a run where the only targeted agent's entry could not be removed still counts as a change. Repro at this head: run install --skills-only --scope=global --agents=cursor; with one agent, Cursor gets a copy. Make ~/.cursor/skills/<skill> unremovable, then run uninstall --scope=global --agents=cursor. It prints Uninstalled 1 skill. and exits 0, yet Cursor still has the skill and its state entry is gone. At the base this run finished FAILURE; it now finishes SUCCESS, and the new help text says uninstall exits 1 when every removal it attempts fails. Suggest counting a skill as changed in the targeted path only when removeSkillExposuresFromAgents removed a targeted agent's entry, not when only the canonical dir was removed.
Contributor
Author
There was a problem hiding this comment.
Fixed in 3165427. With --agents, removing only the canonical dir no longer counts as a change when a targeted agent's entry could not be removed. In your repro (uninstall --scope=global --agents=cursor with Cursor's entry unremovable), the run now exits 1 with could not remove any skill or plugin: <cause> and FAILURE, as at the base. It no longer prints "Uninstalled 1 skill." TestUninstallDoesNotCountSkillWithFailedTargetedRemoval checks the count and that the line is not printed.
| // Every attempt failed or was skipped: FAILURE, though it exits 0. | ||
| funnel.finishWith(ctx, protos.AitoolsCommandOutcomeFailure, classifyInstallError(firstErr)) | ||
| if check { | ||
| return fmt.Errorf("nothing was checked: %w", firstErr) |
Member
There was a problem hiding this comment.
[nit] update --check now exits 1 when the manifest can't be fetched, but the output and docs don't say so. With an unreachable manifest it prints Warn: Could not fetch manifest: ..., then No changes. (from FormatUpdateResult at line 204), then Error: nothing was checked: .... The help text (line 56) and the changelog fragment only say that update fails when every update fails. Suggest skipping the FormatUpdateResult print when result.CheckErr != nil, and writing "every update or check" in the help text and changelog.
Contributor
Author
There was a problem hiding this comment.
Fixed in 3165427. When the manifest fetch fails, update --check no longer prints "No changes." The help text and the changelog now say update exits 1 when "every update or check it attempts fails".
| } else { | ||
| // E.g. --agents names agents that have none of the skills. | ||
| funnel.finishWith(ctx, protos.AitoolsCommandOutcomeNothingToDo, protos.AitoolsErrorCategoryUnspecified) | ||
| return fmt.Errorf("could not remove any skill or plugin: %w", failures[0]) |
Member
There was a problem hiding this comment.
[nit] Suppose a scope's only entry is a plugin record for an agent this CLI doesn't know, for example after a downgrade. uninstall keeps the record and returns could not remove any skill or plugin: unknown agent "x" in state. update returns nothing was updated: unknown agent "x" in state. Before this PR both exited 0 with a warning. No CLI command clears the record, so automation that runs update fails on every run until the CLI is upgraded. Suggest leaving unknown-agent records out of the "every attempt failed" decision, or adding a recovery step to the error (upgrade the CLI, or remove the record).
Contributor
Author
There was a problem hiding this comment.
Fixed in 3165427. A record for an agent this CLI does not know (installer.ErrUnknownAgent) is no longer counted as a failed attempt. When it is the only failure, update and uninstall exit 0 with the warning and finish FAILURE / UNKNOWN_AGENT, as at the base. Any other failed attempt still makes the run exit 1. Both funnel tests have a row for it.
| return "" | ||
| } | ||
| if errors.Is(err, errNoSkillsWritten) { | ||
| step := "No skill in this release can be installed by this CLI. Re-run " + rerunPhrase(runtime.GOOS, inv, "--debug") + " to see why each skill was skipped." |
Member
There was a problem hiding this comment.
[nit] This passes the literal "--debug", but debugFlag (line 31) exists for this purpose, and the other recovery steps (lines 268, 336, 433) use it. .agents/rules/style-guide-go.md asks to reuse an existing constant rather than add a parallel literal. Suggest rerunPhrase(runtime.GOOS, inv, debugFlag).
Contributor
Author
There was a problem hiding this comment.
Fixed in 3165427: the step now uses debugFlag.
| // opts.Agents set, a skill whose entry could not be removed from one of them | ||
| // is not counted. | ||
| SkillsRemoved int | ||
| SkillsRemoved int |
Member
There was a problem hiding this comment.
[nit] cmd/aitools/uninstall.go now decides from SkillsChanged, so only tests read SkillsRemoved. The loop still computes both counters, and their doc comments are nearly the same, so a reader has to work out which one drives behavior. Suggest dropping SkillsRemoved and its counter, or keeping a single counter with the intended meaning. The two uninstall counting issues above may decide which.
Contributor
Author
There was a problem hiding this comment.
Done in 3165427. SkillsRemoved and its counter are gone, and SkillsChanged is the only counter. It now counts the skills of which the run removed a copy an agent reads: an agent's symlink or entry, or the canonical dir. With --agents, removing only the canonical dir doesn't count when a targeted agent keeps its entry. removeSymlinksFromAgents now reports whether it removed a symlink, so the plain path follows the same rule (see the PR comment).
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 08:19
1c9dce4 to
3165427
Compare
anton-107
changed the title
Contributor
Author
|
Third review at 1c9dce4, addressed in 3165427: Questions
Comments outside the changed lines
Agreeing with existing threads
|
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:23
4844908 to
1f0c9c4
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 08:24
3165427 to
12f8a05
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:36
1f0c9c4 to
a37d96c
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 08:37
12f8a05 to
5c39d6a
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 08:41
a37d96c to
e745931
Compare
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 08:41
5c39d6a to
9da45c5
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/nothing-installed-fails
branch
from
November 12, 2026 08:48
9da45c5 to
0595a19
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/nothing-installed-fails
branch
from
November 12, 2026 08:51
0595a19 to
52e34eb
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/nothing-installed-fails
branch
2 times, most recently
from
November 12, 2026 09:04
181bc21 to
e8d4f91
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/nothing-installed-fails
branch
from
November 12, 2026 09:06
e8d4f91 to
2eca9b0
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/nothing-installed-fails
branch
from
November 12, 2026 09:31
2eca9b0 to
cf6eeb8
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/nothing-installed-fails
branch
from
November 12, 2026 10:05
cf6eeb8 to
3289cf7
Compare
anton-107
force-pushed
the
aitools/install-recovery-steps
branch
from
November 12, 2026 11:02
696613f to
2b5410a
Compare
`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 a plugin was installed, a release with no skill to install only skips the skills agents.
- `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: 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>
anton-107
force-pushed
the
aitools/nothing-installed-fails
branch
from
November 12, 2026 11:33
3289cf7 to
c6b1b5e
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 #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, as it already did for--path. 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." If a plugin was installed in the same run, only the skills agents are skipped.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." no longer counts skills whose removal failed.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..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. 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.