aitools: categorize install errors by rclarey · Pull Request #6482 · databricks/cli

rclarey · 2026-09-02T11:06:44Z

Stacked on #6481

Changes

Categorize aitools install errors, and emit those in telemetry and JSON output

Why

To better understand why installations failed

Tests

Added unit tests

github-actions · 2026-09-02T11:07:41Z

Approval status: pending

`/cmd/aitools/` - needs approval

6 files changed
Suggested: @simonfaltum
Also eligible: @lennartkats-db, @parthban-db, @renaudhartert-db, @fjakobs, @Shridhad, @atilafassina, @keugenek, @igrekun, @pkosiec, @MarioCadenas, @pffigueiredo, @ditadi, @calvarjorge, @hectorcast-db, @tanmay-db, @Divyansh-db, @tejaskochar-db, @mihaimitrea-db, @chrisst, @rauchy

`/libs/aitools/` - needs approval

Files: libs/aitools/installer/errors.go, libs/aitools/installer/installer.go
Suggested: @simonfaltum
Also eligible: @lennartkats-db, @parthban-db, @renaudhartert-db, @fjakobs, @Shridhad, @atilafassina, @keugenek, @igrekun, @pkosiec, @MarioCadenas, @pffigueiredo, @ditadi, @calvarjorge, @hectorcast-db, @tanmay-db, @Divyansh-db, @tejaskochar-db, @mihaimitrea-db, @chrisst, @rauchy

`/libs/telemetry/` - needs approval

Files: libs/telemetry/protos/aitools_install.go
Suggested: @simonfaltum
Also eligible: @parthban-db, @renaudhartert-db, @hectorcast-db, @tanmay-db, @Divyansh-db, @tejaskochar-db, @mihaimitrea-db, @chrisst, @rauchy

General files (require maintainer)

5 files changed
Based on git history:

@simonfaltum -- recent work in cmd/aitools/, libs/aitools/installer/

_{Any maintainer (@andrewnester, @anton-107, @denik, @pietern, @shreyas-goenka, @simonfaltum, @renaudhartert-db, @janniklasrose, @lennartkats-db, @rugpanov) can approve all areas.

See OWNERS for ownership rules.}

rugpanov · 2026-09-02T13:50:26Z

Review — multi-reviewer pass

Reviewed the incremental diff (against the stacked parent aitools-install-output-json) with several independent reviewers and verified each finding against the code. Design and correctness are sound overall, and CI (including integration tests) is green. One blocking wire-format bug, plus a few low-risk cleanups.

🔴 Blocking — per-agent category serializes under the wrong JSON key

libs/telemetry/protos/aitools_install.go:53

type AitoolsAgentResult struct {
	Agent         AitoolsAgentType     `json:"agent"`
	ErrorCategory AitoolsErrorCategory `json:"errorCategory"`   // should be "error_category"
}

This is the only camelCase JSON tag in the whole libs/telemetry/protos package (111 other tags are snake_case), and it disagrees with its own sibling field on the parent event (error_category, line 71) and the existing precedents in ssh_tunnel.go / bundle_config_remote_sync.go. The marshaled payload becomes agent_results:[{"agent":"CODEX","errorCategory":...}], so the per-agent category won't map to the lumberjack proto's error_category field and is likely dropped on ingestion — which is exactly the per-agent signal this PR adds. Fix: json:"error_category".

🟡 Nice to have

A sibling specific-skill failure is left uncategorized — libs/aitools/installer/installer.go:487. The "experimental skill; use --experimental" specific-failure still returns a plain fmt.Errorf, so it classifies as UNCATEGORIZED, while the two adjacent failures in the same isSpecific branch (not-found :475, version-incompatible :495) were converted to *SkillError. Consider a category (e.g. EXPERIMENTAL_SKILL) or making it a SkillError for consistency.
Defensive nil check — cmd/aitools/telemetry.go:58. o.agent == nil in agentResultsField guards against a state that can't occur (every outcome is built from a non-nil plan agent). Per the repo convention on unjustified nil checks, consider removing it (and the synthetic nil test case) or adding a comment on why the invariant might break. (It does mirror the pre-existing pattern in agentsField, so it's at least locally consistent.)
No test asserts the telemetry wire format — nothing marshals AitoolsInstallEvent/AitoolsAgentResult to JSON and checks the keys. A serialization assertion would have caught the blocking finding above.
UNSUPPORTED_SCOPE on exit-0 skips — cmd/aitools/install.go:391,400,555. An ordinary scope-incompatible skip (status skipped, exit 0) still lands in agent_results with UNSUPPORTED_SCOPE. Looks intentional per the doc comment, but conflating expected skips with error rows may inflate error analytics — worth confirming the intended semantics.

⚪ Nits

cmd/aitools/install.go:606 — if o.errorCategory != "" { … } is redundant given the omitempty tag, and inconsistent with the unconditional top-level set just below it.
cmd/aitools/install.go:64 — the field skipError holds a category, not an error; mildly misleading next to errorCategory.
libs/aitools/installer/errors.go:25 — SkillError.Error() produces a trailing space when Detail is empty (only reachable from a unit test today).

anton-107

Nice change — the classification logic itself looks right to me. I built the branch and ran go test ./cmd/aitools/... ./libs/aitools/... ./libs/telemetry/..., go vet and gofmt: all clean. I couldn't find a correctness bug; the defer closure capture of outcomes/runErr, the topLevelFailure gate, and the SkillError message reconstruction (byte-identical to the strings it replaces) all check out.

Requesting changes on naming only — two new errorCategory keys are camelCase where the surrounding code is snake_case, and one of them is a public output contract for aitools install --output json that can't be renamed after release. Details inline.

The remaining inline comments are non-blocking robustness/consistency notes; take or leave them as you see fit.

Two things I checked and cleared: both deliverySkip branches in planItemFor genuinely mean unsupported scope (mapAgentScope's only ok == false return is the project-scope case), and ReasonNoPlugin → UNCATEGORIZED is unreachable since plugin-less agents route to deliverySkills.

anton-107 · 2026-09-03T07:36:02Z

+// user-authored text.
+type AitoolsAgentResult struct {
+	Agent         AitoolsAgentType     `json:"agent"`
+	ErrorCategory AitoolsErrorCategory `json:"errorCategory"`


Blocking (naming): this is tagged errorCategory (camelCase) while every other field in libs/telemetry/protos is snake_case — including error_category 18 lines below in this same file, plus compute_type, resource_job_count, aitools_install_event, etc.

These structs are hand-maintained mirrors of the lumberjack proto, so this nested per-agent field is the one whose wire name diverges from the proto field name. protojson does accept lowerCamelCase, so it probably parses today, but that's a silent dependency that breaks under a strict/snake-only decoder.

Suggested change

ErrorCategory AitoolsErrorCategory `json:"errorCategory"`

ErrorCategory AitoolsErrorCategory `json:"error_category"`

anton-107 · 2026-09-03T07:36:02Z

+	// is the local-only message; ErrorCategory is the classification telemetry
+	// also records.
+	Error         string `json:"error,omitempty"`
+	ErrorCategory string `json:"errorCategory,omitempty"`


Blocking (naming): errorCategory here (and on agentResultJSON below) is a public output contract for aitools install --output json, so it can't be renamed after release.

The CLI's JSON output convention is snake_case throughout — cmd/fs/ls.go (is_directory, last_modified), cmd/auth/profiles.go (account_id, auth_type), cmd/bundle/debug/fetch_repository_info.go (worktree_root). The existing keys here (scope, agents, name, delivery, status, message) are all single words, so this PR is the first to set the multi-word precedent.

Suggested change

ErrorCategory string `json:"errorCategory,omitempty"`

ErrorCategory string `json:"error_category,omitempty"`

anton-107 · 2026-09-03T07:36:02Z

+	Name          string `json:"name"`
+	Delivery      string `json:"delivery"`
+	Status        string `json:"status"`
+	ErrorCategory string `json:"errorCategory,omitempty"`


Same rename here, for the per-agent entry.

Suggested change

ErrorCategory string `json:"errorCategory,omitempty"`

ErrorCategory string `json:"error_category,omitempty"`

anton-107 · 2026-09-03T07:36:02Z

+	for _, o := range outcomes {
+		// A successful agent leaves errorCategory at its zero value ("", not the
+		// TYPE_UNSPECIFIED sentinel), so key on emptiness to drop it here.
+		if o.agent == nil || o.errorCategory == "" {


Non-blocking, two separate notes on this line.

1. The o.agent == nil guard disagrees with the other consumer of the same slice. buildInstallOutput (cmd/aitools/install.go:601) dereferences o.agent.Name with no guard. So either a nil agent is reachable — in which case --output json panics on exactly the input the telemetry path tolerates — or it isn't, and the guard plus its test (telemetry_test.go, "A nil agent is skipped defensively") are dead code that CLAUDE.md's "no defensive nil checks for values the caller always provides" rule asks you to drop. Either way the two functions should agree.

2. Keying success off errorCategory == "" is a trap for future code. The comment correctly notes that "" is the Go zero value and not the AitoolsErrorCategoryUnspecified sentinel — but since the field is typed protos.AitoolsErrorCategory, it's natural for someone to later write errorCategory: protos.AitoolsErrorCategoryUnspecified for a successful agent, which would emit a bogus agent_results entry claiming that agent failed with TYPE_UNSPECIFIED. Keying on o.status != outcomeInstalled (or checking both "" and Unspecified) removes the trap.

anton-107 · 2026-09-03T07:36:02Z

+)
+
+func (e *SkillError) Error() string {
+	return fmt.Sprintf("skill %q %s", e.Skill, e.Detail)


Non-blocking: when Detail is empty this renders as skill "databricks" — trailing space, no reason given. Both current construction sites set Detail, but the new test already builds &installer.SkillError{Skill: "databricks", Reason: "some-future-reason"} without one, which is exactly the shape a future reason will take; that message would then leak into --output json's error field and the text Error: line.

Falling back to Reason when Detail == "" (the way BlockedError.Error() already branches on it) keeps it self-describing.

anton-107 · 2026-09-03T07:36:02Z

+	// ErrorCategory is the top-level command outcome: the category of the error
+	// that failed the run, or Unspecified when the command succeeded. It captures
+	// failures that have no per-agent entry (e.g. a skills-group install failure).
+	ErrorCategory AitoolsErrorCategory `json:"error_category,omitempty"`


Non-blocking: omitempty never fires here, since the success value is AitoolsErrorCategoryUnspecified = "TYPE_UNSPECIFIED", a non-empty string — so every successful install ships "error_category":"TYPE_UNSPECIFIED". Harmless on the wire, but the tag implies an omission that can't happen. The precedent for an always-populated category field, SshTunnelEvent.ErrorCategory (libs/telemetry/protos/ssh_tunnel.go:116), deliberately omits omitempty.

eng-dev-ecosystem-bot · 2026-09-04T10:54:40Z

Integration test report

Commit: 49636b6

Run: 33884561228

	Env	💚RECOVERED	🙈SKIP	✅pass	🙈skip	Time
💚	aws linux	1	1	274	1212	4:10
💚	aws windows	1	1	276	1210	3:33
💚	azure linux	1	1	273	1212	3:54
💚	azure windows	1	1	275	1210	3:31
💚	gcp linux	1	1	274	1212	3:50
💚	gcp windows	1	1	276	1210	3:39

	Test Name	aws linux	aws windows	azure linux	azure windows	gcp linux	gcp windows
💚	TestAccept	💚R	💚R	💚R	💚R	💚R	💚R
🙈	TestAccept/ssh/connection	🙈S	🙈S	🙈S	🙈S	🙈S	🙈S

Top 2 slowest tests (at least 2 minutes):

duration	env	testname
2:09	gcp windows	TestAccept
2:08	azure windows	TestAccept

Classify why an `aitools install` run, or one agent within it, failed into a stable AitoolsErrorCategory, so install failures can be aggregated in telemetry without sending any user-authored error text. Introduce SkillError alongside the existing BlockedError, map both to categories via classifyInstallError, and record the per-agent categories on the install event. Surface the per-agent category in the `--output json` result too. The top-level errorCategory (telemetry and JSON) is set only for a failure with no per-agent entry; a per-agent failure keeps its category in its own entry and leaves the top-level category Unspecified, so it is never counted twice. Co-authored-by: Isaac <no-reply@databricks.com>

- Rename the JSON/telemetry field errorCategory -> error_category (snake_case), matching the CLI's --output json convention and the rest of libs/telemetry/protos. Update the acceptance golden accordingly. - Drop omitempty on AitoolsInstallEvent.ErrorCategory: it is always populated (Unspecified on success), so the tag never fired. - agentResultsField: key on status == outcomeInstalled instead of errorCategory == "", and drop the dead o.agent == nil guard (agents always come from the validated registry, matching buildInstallOutput). - SkillError.Error() falls back to Reason when Detail is empty so the message stays self-describing. Co-authored-by: Isaac <no-reply@databricks.com>

Use the non-deprecated `aitools install` command. With progress now silenced in JSON mode (see the --output json branch), stdout carries only the JSON document, so the golden no longer has non-JSON text before it. Co-authored-by: Isaac <no-reply@databricks.com>

rclarey mentioned this pull request Sep 2, 2026

aitools: add --output json to install #6481

Open

rclarey force-pushed the aitools-install-output-json branch from 583618b to 5dbbee3 Compare September 2, 2026 11:10

rclarey force-pushed the aitools-install-error-categories-stacked branch from 1bec626 to 475a007 Compare September 2, 2026 11:10

anton-107 requested changes Sep 3, 2026

View reviewed changes

rclarey force-pushed the aitools-install-output-json branch from 5dbbee3 to 0f9f04a Compare September 3, 2026 12:21

rclarey force-pushed the aitools-install-error-categories-stacked branch from 475a007 to 7c97955 Compare September 3, 2026 12:21

rclarey force-pushed the aitools-install-output-json branch from 0f9f04a to af4bb2a Compare September 3, 2026 13:46

rclarey force-pushed the aitools-install-error-categories-stacked branch 2 times, most recently from 929fe31 to 4882a9b Compare September 3, 2026 14:04

rclarey force-pushed the aitools-install-output-json branch from af4bb2a to 047ee54 Compare September 3, 2026 14:04

rclarey force-pushed the aitools-install-error-categories-stacked branch from 4882a9b to 0c9f703 Compare September 4, 2026 10:27

rclarey and others added 3 commits September 4, 2026 16:32

rclarey force-pushed the aitools-install-error-categories-stacked branch from 5a6cd9f to 49636b6 Compare September 4, 2026 14:34

Navigation Menu

Uh oh!

aitools: categorize install errors - #6482

aitools: categorize install errors#6482
rclarey wants to merge 3 commits into
aitools-install-output-jsonfrom
aitools-install-error-categories-stacked

rclarey commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

rugpanov commented Sep 2, 2026

Uh oh!

anton-107 left a comment

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

anton-107 Sep 3, 2026

Uh oh!

eng-dev-ecosystem-bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	ErrorCategory AitoolsErrorCategory `json:"errorCategory"`
	ErrorCategory AitoolsErrorCategory `json:"error_category"`

	ErrorCategory string `json:"errorCategory,omitempty"`
	ErrorCategory string `json:"error_category,omitempty"`

Uh oh!

Conversation

rclarey commented Sep 2, 2026

Changes

Why

Tests

Uh oh!

github-actions Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Approval status: pending

/cmd/aitools/ - needs approval

/libs/aitools/ - needs approval

/libs/telemetry/ - needs approval

General files (require maintainer)

Uh oh!

rugpanov commented Sep 2, 2026

Review — multi-reviewer pass

🔴 Blocking — per-agent category serializes under the wrong JSON key

🟡 Nice to have

⚪ Nits

Uh oh!

anton-107 left a comment

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

anton-107 Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

eng-dev-ecosystem-bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integration test report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Sep 2, 2026 •

edited

Loading

`/cmd/aitools/` - needs approval

`/libs/aitools/` - needs approval

`/libs/telemetry/` - needs approval

eng-dev-ecosystem-bot commented Sep 4, 2026 •

edited

Loading