aitools: categorize install errors - #6482
Conversation
583618b to
5dbbee3
Compare
1bec626 to
475a007
Compare
Review — multi-reviewer passReviewed the incremental diff (against the stacked parent 🔴 Blocking — per-agent category serializes under the wrong JSON key
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 🟡 Nice to have
⚪ Nits
|
anton-107
left a comment
There was a problem hiding this comment.
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.
5dbbee3 to
0f9f04a
Compare
475a007 to
7c97955
Compare
0f9f04a to
af4bb2a
Compare
7c97955 to
929fe31
Compare
Add JSON output to `aitools install`, driven entirely by flags so the run is fully non-interactive: require --scope and --agents (erroring and naming the missing flags otherwise) so no scope prompt, agent picker, or confirm is shown. executePlan now returns a per-agent outcome (name, delivery, status, message) that the JSON payload lists. A top-level failure with no per-agent entry (e.g. a skills-group install failure) is surfaced in a top-level "error" field; per-agent failures stay in their agent entry and are not duplicated there (executePlan wraps them so the two are distinguishable). Once the JSON result is rendered, silence cobra's text "Error:"/usage output so a failure is not reported twice; the non-zero exit still comes from returning the run error. Share the indented-JSON encoder between install and list as renderJSON. Co-authored-by: Isaac <no-reply@databricks.com>
929fe31 to
4882a9b
Compare
af4bb2a to
047ee54
Compare
The changelog validator now requires each fragment to be a single line starting with a `* ` bullet and ending with a period before the trailing PR link group. Reformat the install --output json fragment accordingly. Co-authored-by: Isaac <no-reply@databricks.com>
4882a9b to
0c9f703
Compare
Integration test reportCommit: ef859bc
Top 6 slowest tests (at least 2 minutes):
|
--output json is meant to emit only the structured JSON document, but the
installer still wrote progress lines ("Using skills version", "Fetching
skills manifest...", "Installed N skills.") to stderr, so a consumer saw
non-JSON interleaved with the result. Mark the context quiet in JSON mode
and route those library messages through cmdio.LogProgress, which respects
it. Text mode is unaffected. Update the acceptance test to use the
non-deprecated `aitools install` and assert only JSON is emitted.
Co-authored-by: Isaac <no-reply@databricks.com>
5a6cd9f to
49636b6
Compare
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>
Cover the --output json path where a failure is reported per agent rather than at the top level: - install-output-json-agent-error: claude-code installs while cursor is skipped with an UNSUPPORTED_SCOPE category in its own agents[] entry, so the array mixes a success with a per-agent error and the top-level error_category stays unset. - install-output-json-agents-skipped: every named agent (cursor, codex) is skipped for scope, giving multiple per-agent categories with no successful install and no top-level error. Both exit non-zero because the agents were named explicitly. Co-authored-by: Isaac <no-reply@databricks.com>
2d006b5 to
c0840bd
Compare
There was a problem hiding this comment.
Thanks — all six threads from my previous pass are properly addressed, and the rename went further than the structs (acceptance golden, the script's explanatory comment, and the changelog fragment are all updated; git grep 'json:"[a-z]*[A-Z]' over cmd/aitools, libs/aitools, libs/telemetry/protos is empty). Both blocking items are resolved. I'm leaving my requested-changes in place for now only pending the installer.go:487 question at the bottom — everything else here is non-blocking.
I re-verified rather than taking the resolutions on trust: gofmt and go vet clean, go test ./cmd/aitools/... ./libs/aitools/... ./libs/telemetry/... green, and go test ./acceptance -run TestAccept/experimental/aitools green, so the goldens genuinely match. Two details I liked: you took the drop the guard fork on the nil check (so agentResultsField and buildInstallOutput now agree), and status == outcomeInstalled is an exact success test rather than an approximation, since outcomeStatus has exactly three values.
Two small leftovers from my point on agentResultsField, inline. Both non-blocking.
One request before merge: could you take another look at Grigory's review comment? Its blocking item was the same camelCase tag, so that's covered, but four of its items got no change and no reply, and one of them looks substantive to me:
libs/aitools/installer/installer.go:487—"skill %q is experimental; use --experimental to install"is still a plainfmt.Errorf, soclassifyInstallErrorlands it inUNCATEGORIZED, while the two sibling failures in the sameisSpecificbranch were converted to*SkillError. That's a classification gap in exactly the signal this PR adds — a real, reachable user error that will show up as uncategorized noise. Worth aSkillErrorreason, or an explicit note that uncategorized is intended for it.cmd/aitools/install.go:65—skipErrorholds aprotos.AitoolsErrorCategory, not an error; mildly confusing sitting next toerrorCategory.- The
UNSUPPORTED_SCOPE-on-exit-0 skip semantics question. Your two new acceptance tests document the explicit-agents (exit 1) case nicely, but the auto-detected-agents case still records an error category on a successful run — fine if intended, just never answered. - The missing wire-format test, which I've also flagged inline.
Happy for all of these to be follow-ups if you'd rather keep this PR tight — I'd just like the installer.go:487 one either fixed or explicitly deferred rather than dropped, and then I'll clear the block.
| Status: string(o.status), | ||
| Message: o.message, | ||
| } | ||
| if o.errorCategory != "" { |
There was a problem hiding this comment.
Non-blocking, follow-up to my earlier note on agentResultsField: the trap moved rather than disappeared. Telemetry now keys success off status == outcomeInstalled, but this side still keys off errorCategory != "", so the two consumers of the same slice disagree again — just in the opposite direction from before. A future successful outcome carrying AitoolsErrorCategoryUnspecified would be correctly dropped from agent_results and still emit "error_category": "TYPE_UNSPECIFIED" here, on an agent whose status is installed.
Separately, the if is redundant as written: string("") is "" and the tag is omitempty, so an unconditional assignment is byte-identical on the wire. Keying on o.status != outcomeInstalled (matching telemetry.go) or just dropping the branch both work; the former keeps the two functions aligned.
| // user-authored text. | ||
| type AitoolsAgentResult struct { | ||
| Agent AitoolsAgentType `json:"agent"` | ||
| ErrorCategory AitoolsErrorCategory `json:"error_category"` |
There was a problem hiding this comment.
Non-blocking: with the tag fixed, nothing guards it. No test marshals AitoolsInstallEvent/AitoolsAgentResult and asserts the key names, so the next hand-edit to these structs can reintroduce exactly the bug we just fixed — and it fails silently, on the ingestion side, where nobody sees it.
libs/telemetry/protos/ssh_tunnel_test.go is the precedent in this package: it marshals the event and compares against a literal JSON string. A ~10-line equivalent covering an event with one agent_results entry would pin both error_category fields. Grigory raised this too.
|
Heads up: Fix is just |
Address review feedback on PR #6482: - Add EXPERIMENTAL_SKILL error category so a specific experimental skill requested without --experimental no longer classifies as UNCATEGORIZED; installer returns a *SkillError with ReasonExperimentalSkill. - Key buildInstallOutput's category emission on status != outcomeInstalled to match agentResultsField, so the JSON and telemetry views agree. - Rename skipError to skipErrorCategory (it holds a category, not an error). - Add a telemetry wire-format test pinning the snake_case error_category keys. - Add an acceptance test for the experimental-skill JSON output. Co-authored-by: Isaac <no-reply@databricks.com>
|
@anton-107 about the "UNSUPPORTED_SCOPE on exit-0 skips" feedback, yes this is intentional. The marker for errors in telemetry is exit code |
anton-107
left a comment
There was a problem hiding this comment.
Everything I was holding the block on is resolved — clearing it. ef859bcb fixes the installer.go:487 classification gap, and it also picked up the four items from Grigory's review that had been left without a change or a reply, plus the gofmt slip.
I re-verified rather than taking the commit on trust: gofmt -l empty, go vet clean, go test ./cmd/aitools/... ./libs/aitools/... ./libs/telemetry/... green, and go test ./acceptance -run TestAccept/experimental/aitools green — so the new golden genuinely matches.
Details I checked on the new commit:
EXPERIMENTAL_SKILL— the message is byte-identical to thefmt.Errorfit replaces (skill "test-exp" is experimental; use --experimental to install), sinceDetailis set andSkillError.Error()takes theDetailbranch.resolveSkillsis now internally consistent: all threeisSpecificfailures are*SkillErrorwith distinct reasons, so the classification gap in exactly the signal this PR adds is closed.- The two views now agree —
buildInstallOutputkeys ono.status != outcomeInstalled, matchingagentResultsField. Behaviour is unchanged today:errorCategoryis assigned at only two sites (install.go:547outcomeFailed,install.go:582outcomeSkipped), and bothoutcomeInstalledsites leave it zero — so the new predicate is exactly equivalent to the olderrorCategory != "", minus the trap. - The
ReasonNoPlugin→"some-future-reason"test swap loses no coverage. The constant is still asserted atplugin_test.go:114, and the classify test now exercises thedefaultbranch honestly instead of documenting an unreachable path —InstallPluginForAgentonly returns it whenagent.Plugin == nil, andplanItemFor'sdefaultarm (the only one that yieldsdeliveryPlugin) requiresa.Plugin != nil. - The wire-format test pins what it needs to —
error_categoryon the event and on theagent_resultsentry, plus theagent_resultskey itself. That's the guard the camelCase bug slipped through. - The new acceptance test covers the path end-to-end, including the empty
agentsarray with the top-level category and exit 1.
No new findings. One thing I looked at and cleared, so it doesn't come back as a follow-up: installer.go:309 ("no agents support project-scoped skills") is a semantically scope-related failure that would classify as UNCATEGORIZED, but it's unreachable from both callers — cmd/aitools/install.go only ever passes agents that planItemFor already routed to deliverySkills (project-incompatible ones become deliverySkip/UNSUPPORTED_SCOPE before the group call), and InstallAllSkills passes InstallOptions{}, whose zero-value scope isn't ScopeProject. So there's nothing to categorize there.
Thanks for taking the UNSUPPORTED_SCOPE-on-exit-0 question head-on too — the exit-code-1 marker is the right answer, and it means the skip rows can't inflate error analytics.
Integration test reportCommit: d472995
1048 interesting tests: 876 MISS, 171 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
## Changes Add `--output json` to `aitools install`. This requires `--scope` and `--agents` to avoid needing to prompt users on stdout ## Why Needed for databricks#6482 ## Tests Added unit tests <!-- If your PR needs to be included in the release notes for next release, add a changelog fragment: create .nextchanges/<section>/<name>.md with a one-line description (e.g. .nextchanges/cli/quickstart.md). See .nextchanges/README.md. --> --------- Co-authored-by: Isaac <no-reply@databricks.com>
## Release v1.16.0 ### CLI * `aitools install` now registers the official Claude marketplace if it is missing before installing the Databricks Claude plugin. ([#6485](#6485)) * `databricks aitools install --output json` now reports an `error_category` for a failed or skipped install (per agent, and at the top level for a failure with no per-agent entry), giving coding agents and CI a stable classification of why an install did not complete. ([#6482](#6482)) * `databricks aitools install` honors `--output json`, emitting a structured `{scope, agents[...]}` document that reports each agent's delivery and install status so coding agents and CI can consume the result without scraping the text output. JSON mode requires `--scope` and `--agents` so the command runs without interactive prompts. ([#6481](#6481)) * `databricks bundle sync` now prints sync progress (`Action: PUT`, `Uploaded ...`) by default, matching `databricks sync`. Previously it was silent unless `--output` was passed. Use `--output json` for machine-readable output. ([#6568](#6568)) * Support major-only DBR runtime versions such as `19.x-scala2.13` in the cluster picker used by `databricks auth login --configure-cluster` and `databricks labs`. ([#6574](#6574)) * Deprecated the `databricks environments setup-local --constraints-only` flag in favour of the orthogonal `--no-dbconnect`; the flag still works as a hidden alias but is hidden from `--help` and prints a one-line deprecation notice, and will be removed in a later release. ([#6470](#6470)) * Add orthogonal `--no-constraints` and `--no-dbconnect` flags to `databricks environments setup-local`: `--no-constraints` skips writing the remote Python-version and dependency pins, and `--no-dbconnect` skips the databricks-connect dependency. ([#6464](#6464)) * `databricks environments setup-local` now reports a distinct `E_PROVISION_CONFLICT` error code in `--output json` when the project's dependencies conflict with the pins written for the target environment, making the requirements unsatisfiable (the same conflict surfaced as a `W_USER_CONSTRAINT_CONFLICT` warning); it is reported after the project files are written, without attempting the doomed provisioning, while other provisioning failures continue to report `E_PROVISION`. ([#6479](#6479)) * `databricks ssh connect` and `ssh setup` now verify the tunnel's SSH host key against the key the workspace published for the connection, recorded in `~/.databricks/ssh-tunnel-known-hosts/<name>` instead of `~/.ssh/known_hosts`. Reconnecting with a name used before no longer fails with `Host key verification failed` when the compute behind that name changed, and no longer needs a manual `ssh-keygen -R`; host blocks written by an earlier `databricks ssh setup` pick this up once you re-run it. ([#6557](#6557)) * Stop `databricks ssh connect --ide` from adding a duplicate entry to the IDE's Remote Explorer on every connect: the remote authority is now the SSH host alias alone, instead of embedding the per-instance remote OS user. ([#6550](#6550)) * Add `--max-clients` and `--server-timeout` flags to `databricks ssh setup`, and `--server-timeout` to `databricks ssh connect`. Both are fixed when the SSH tunnel server job is submitted, so `ssh setup` now serializes them into the generated `ProxyCommand` instead of falling back to the built-in defaults. ([#6547](#6547)) * `ssh connect` sessions no longer end when the tunnel's websocket connection is lost. The CLI reattaches to the running session and replays the bytes that were missed, so the shell and everything running in it stay intact, and a transient failure to open a replacement connection for the periodic auth refresh is retried rather than ending the session. Reattaching requires an SSH server started by a CLI that supports it; against an older server the connection behaves as before. ([#6558](#6558)) ### Bundles * Added PyDABs (Python) support for secrets: `Resources.add_secret` and the `secret_mutator` decorator. ([#6553](#6553)) * Fix job and pipeline environment dependencies with a `*` version wildcard (e.g. `numpy==2.5.*`) being treated as local file paths. ([#6555](#6555)) * Add the `postgres_snapshot_schedules` bundle resource for managing a Lakebase Postgres branch's automatic-snapshot schedule (direct deployment engine only). ([#6449](#6449)) ### Dependency Updates * Bump `github.com/databricks/databricks-sdk-go` from v0.175.0 to v0.177.0. ([#6448](#6448)) * Bump Terraform provider from v1.128.0 to v1.131.0. ([#6544](#6544))
## Changes Add `--output json` to `aitools install`. This requires `--scope` and `--agents` to avoid needing to prompt users on stdout ## Why Needed for #6482 ## Tests Added unit tests <!-- If your PR needs to be included in the release notes for next release, add a changelog fragment: create .nextchanges/<section>/<name>.md with a one-line description (e.g. .nextchanges/cli/quickstart.md). See .nextchanges/README.md. --> --------- Co-authored-by: Isaac <no-reply@databricks.com>
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 <!-- If your PR needs to be included in the release notes for next release, add a changelog fragment: create .nextchanges/<section>/<name>.md with a one-line description (e.g. .nextchanges/cli/quickstart.md). See .nextchanges/README.md. --> --------- Co-authored-by: Isaac <no-reply@databricks.com>
## Release v1.16.0 ### CLI * `aitools install` now registers the official Claude marketplace if it is missing before installing the Databricks Claude plugin. ([#6485](#6485)) * `databricks aitools install --output json` now reports an `error_category` for a failed or skipped install (per agent, and at the top level for a failure with no per-agent entry), giving coding agents and CI a stable classification of why an install did not complete. ([#6482](#6482)) * `databricks aitools install` honors `--output json`, emitting a structured `{scope, agents[...]}` document that reports each agent's delivery and install status so coding agents and CI can consume the result without scraping the text output. JSON mode requires `--scope` and `--agents` so the command runs without interactive prompts. ([#6481](#6481)) * `databricks bundle sync` now prints sync progress (`Action: PUT`, `Uploaded ...`) by default, matching `databricks sync`. Previously it was silent unless `--output` was passed. Use `--output json` for machine-readable output. ([#6568](#6568)) * Support major-only DBR runtime versions such as `19.x-scala2.13` in the cluster picker used by `databricks auth login --configure-cluster` and `databricks labs`. ([#6574](#6574)) * Deprecated the `databricks environments setup-local --constraints-only` flag in favour of the orthogonal `--no-dbconnect`; the flag still works as a hidden alias but is hidden from `--help` and prints a one-line deprecation notice, and will be removed in a later release. ([#6470](#6470)) * Add orthogonal `--no-constraints` and `--no-dbconnect` flags to `databricks environments setup-local`: `--no-constraints` skips writing the remote Python-version and dependency pins, and `--no-dbconnect` skips the databricks-connect dependency. ([#6464](#6464)) * `databricks environments setup-local` now reports a distinct `E_PROVISION_CONFLICT` error code in `--output json` when the project's dependencies conflict with the pins written for the target environment, making the requirements unsatisfiable (the same conflict surfaced as a `W_USER_CONSTRAINT_CONFLICT` warning); it is reported after the project files are written, without attempting the doomed provisioning, while other provisioning failures continue to report `E_PROVISION`. ([#6479](#6479)) * `databricks ssh connect` and `ssh setup` now verify the tunnel's SSH host key against the key the workspace published for the connection, recorded in `~/.databricks/ssh-tunnel-known-hosts/<name>` instead of `~/.ssh/known_hosts`. Reconnecting with a name used before no longer fails with `Host key verification failed` when the compute behind that name changed, and no longer needs a manual `ssh-keygen -R`; host blocks written by an earlier `databricks ssh setup` pick this up once you re-run it. ([#6557](#6557)) * Stop `databricks ssh connect --ide` from adding a duplicate entry to the IDE's Remote Explorer on every connect: the remote authority is now the SSH host alias alone, instead of embedding the per-instance remote OS user. ([#6550](#6550)) * Add `--max-clients` and `--server-timeout` flags to `databricks ssh setup`, and `--server-timeout` to `databricks ssh connect`. Both are fixed when the SSH tunnel server job is submitted, so `ssh setup` now serializes them into the generated `ProxyCommand` instead of falling back to the built-in defaults. ([#6547](#6547)) * `ssh connect` sessions no longer end when the tunnel's websocket connection is lost. The CLI reattaches to the running session and replays the bytes that were missed, so the shell and everything running in it stay intact, and a transient failure to open a replacement connection for the periodic auth refresh is retried rather than ending the session. Reattaching requires an SSH server started by a CLI that supports it; against an older server the connection behaves as before. ([#6558](#6558)) ### Bundles * Added PyDABs (Python) support for secrets: `Resources.add_secret` and the `secret_mutator` decorator. ([#6553](#6553)) * Fix job and pipeline environment dependencies with a `*` version wildcard (e.g. `numpy==2.5.*`) being treated as local file paths. ([#6555](#6555)) * Add the `postgres_snapshot_schedules` bundle resource for managing a Lakebase Postgres branch's automatic-snapshot schedule (direct deployment engine only). ([#6449](#6449)) ### Dependency Updates * Bump `github.com/databricks/databricks-sdk-go` from v0.175.0 to v0.177.0. ([#6448](#6448)) * Bump Terraform provider from v1.128.0 to v1.131.0. ([#6544](#6544))
Stacked on #6481
Changes
Categorize
aitools installerrors, and emit those in telemetry and JSON outputWhy
To better understand why installations failed
Tests
Added unit tests