aitools: categorize install errors - #6482
Conversation
Approval status: pending
|
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
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: 501eaf7
6 interesting tests: 6 flaky
Top 12 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.
4 participants
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