aitools: add --output json to install - #6481
Merged
Merged
Conversation
rclarey
force-pushed
the
aitools-install-output-json
branch
from
September 2, 2026 11:10
583618b to
5dbbee3
Compare
rugpanov
reviewed
Sep 2, 2026
rugpanov
left a comment
rugpanov
left a comment
Contributor
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.
rclarey
force-pushed
the
aitools-install-output-json
branch
2 times, most recently
from
September 3, 2026 13:46
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>
rclarey
force-pushed
the
aitools-install-output-json
branch
from
September 3, 2026 14:04
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>
Collaborator
Integration test reportCommit: 28dd936
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>
rugpanov
approved these changes
Sep 7, 2026
rugpanov
left a comment
rugpanov
left a comment
Contributor
There was a problem hiding this comment.
Approving. The blocking issue from the earlier pass (a failed --output json run printing a duplicate Error: line — SilenceErrors is a no-op since root prints errors itself) is fixed correctly via root.ErrAlreadyPrinted, and it's now covered end-to-end through root.Execute (TestInstallOutputJSONThroughRoot). --path + --output json is rejected, the changelog fragment and acceptance test are in, and a fresh multi-reviewer round found nothing blocking. installOutputIsJSON keeping its own nil-safe check (rather than root.OutputType) is justified by the detached legacy skills install alias — confirmed.
One non-blocking cleanup you can take or defer: executePlan's quiet bool duplicates the cmdio.WithQuiet/LogProgress mechanism this PR already uses in the installer — converting its progress LogString calls to LogProgress would drop the param and its guards with no behavior change. LGTM either way.
Collaborator
Integration test reportCommit: dc00178
697 interesting tests: 631 MISS, 65 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
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
3 participants
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.
Changes
Add
--output jsontoaitools install. This requires--scopeand--agentsto avoid needing to prompt users on stdoutWhy
Needed for #6482
Tests
Added unit tests