aitools: add --output json to install - #6481
Conversation
Approval status: pending
|
583618b to
5dbbee3
Compare
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Automated multi-reviewer pass (Isaac, Codex, and Claude reviewers, cross-checked against the code). The feature itself is sound and mirrors aitools list --output json. Inline comments below; a few findings that don't anchor to a changed line:
[nice to have] --path + --output json silently ignores JSON (cmd/aitools/install.go:124) — the --path block returns before the jsonMode check (:138) and render (:197), so install --path DIR --output json does a text dump with exit 0 and no JSON. --skills-only+--path is already rejected at :105; per CLAUDE.md's "reject incompatible inputs early" rule, either reject this combination or honor JSON in the dump path.
[nice to have] Coverage / convention gaps — (1) no .nextchanges/cli/ changelog fragment for this user-visible output mode (the analogous aitools list --output json, #5233, shipped one; not CI-blocking but expected by .agents/rules/changelog.md); (2) only unit tests were added — .agents/rules/testing.md prefers acceptance coverage for user-visible output, and the suite already exists at acceptance/experimental/aitools/skills/install/. An acceptance case would also have caught the stderr finding below, since it runs the real binary through root.Execute.
[nit] Help text (cmd/aitools/install.go:83) — the Long help / "Escape hatches" list doesn't mention --output json or its --scope+--agents requirement, so users hit the error only at runtime.
| // also print runErr as a text "Error:" line. Silence cobra's | ||
| // error/usage output; the non-zero exit still comes from returning | ||
| // runErr. | ||
| cmd.SilenceErrors = true |
There was a problem hiding this comment.
[blocking] SilenceErrors doesn't suppress the Error: line — the JSON contract is broken in production.
This repo's root prints errors itself: cmd/root/root.go:166 does fmt.Fprintf(cmd.ErrOrStderr(), "Error: %s\n", …), gated only on errors.Is(err, ErrAlreadyPrinted) (root already sets SilenceErrors: true at root.go:40, so cobra never prints on its own). So setting SilenceErrors here is a no-op, and a failed install … --output json writes the JSON to stdout and Error: … to stderr.
TestInstallOutputJSON passes only because it calls the leaf cmd.Execute(), which bypasses root.Execute.
Established fix — see cmd/environments/output.go:36-38: after the successful render, return root.ErrAlreadyPrinted on failure (non-zero exit, no text line) instead of relying on SilenceErrors. Requires importing github.com/databricks/cli/cmd/root. Please also add a test that goes through root.Execute so the stderr assertion is meaningful.
| return cmd | ||
| } | ||
|
|
||
| func installOutputIsJSON(cmd *cobra.Command) bool { |
There was a problem hiding this comment.
[nice to have] This duplicates root.OutputType(cmd), which the sibling list.go:141 already uses. The f == nil fallback is dead in production — --output is always a persistent root flag — and is the kind of speculative/defensive branch CLAUDE.md asks us to drop. Prefer root.OutputType(cmd) == flags.OutputJSON for consistency with list.
| if jsonMode { | ||
| if jerr := renderJSON(cmd.OutOrStdout(), buildInstallOutput(opts.Scope, outcomes, runErr)); jerr != nil { | ||
| // Rendering failed, so the JSON the caller parses is broken. | ||
| // Report the render error (which also drives the deferred |
There was a problem hiding this comment.
[nit] "which also drives the deferred telemetry" is inaccurate — the deferred logInstallEvent fires regardless of the returned error, so returning runErr here doesn't drive telemetry. Worth trimming the parenthetical.
| // --- Test helpers --- | ||
|
|
||
| // newTestInstallCmd builds the install command with the persistent --output flag | ||
| // the root command supplies in production, so root.OutputType does not panic when |
There was a problem hiding this comment.
[nit] "so root.OutputType does not panic" — install uses installOutputIsJSON, not root.OutputType; the persistent flag is actually needed so cobra can parse --output on a detached command. (Moot if the command switches to root.OutputType.)
0f9f04a to
af4bb2a
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>
af4bb2a to
047ee54
Compare
2 participants
Changes
Add
--output jsontoaitools install. This requires--scopeand--agentsto avoid needing to prompt users on stdoutWhy
Needed for #6482
Tests
Added unit tests