Repository navigation
Conversation
a5568f8 to
2b4b8c6
Compare
2e276b6 to
848f252
Compare
|
Commit: Regression test reportSelected 12 changed tests (limit=20, 0 not selected) 12 tests: regression=8 unreleased=0 coverage=4 failing=0
TestAccept/cmd/aitools/install-host-metadata: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/apps-init-tip: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/hint: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/update-uninstall: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/workspace-unreachable: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/version: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): |
✅ Integration test reportCommit: 66114a4
Top 10 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 848f252. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR adds a line pointing to databricks aitools install to the text output of databricks version --check on every outcome except development builds. JSON output and plain version are unchanged. The template renders correctly in every case, the tests and changelog fragment look right, and CI is green. Two things are open: whether this hint should be gated like the other aitools install suggestions (it also shows to users who already installed and to users with no coding agent), and moving it from stdout to stderr like the other hints. There is also a small note on the PR description format.
Questions
- Should this line be shown unconditionally? The other places that suggest
databricks aitools installall check first:agents.MaybeHint(libs/aitools/agents/hint.go),RecommendSkillsInstall(libs/aitools/agents/recommend.go) andaitools version. They stay quiet when the plugin or skills are already installed, or when no coding agent is present, and the other version notices skip Databricks Runtime. With this change,version --checkalso tells users who already ranaitools installto set it up, and it shows on DBR. Users with no coding agent are sent to a command that replies 'No supported coding agents found on PATH'. Claude Code sessions without tooling get this line plus the separate MaybeHint line on stderr. Options: (a) keep it unconditional and explain in the description why it differs from the other hints, or (b) route it through thelibs/aitools/agentshint helpers so it shows only when an agent is detected and no Databricks plugin or skills are present.
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary,## Evidenceand## Merge Danger..agents/skills/pr-checklist/SKILL.md(PR description section) asks for the.github/PULL_REQUEST_TEMPLATE.mdheadings (## Changes,## Why,## Tests) in that order, with no invented sections. As a result, the description has no## Whyexplaining the reason for adding this hint. Could you move the content under the template headings (Summary → Changes, Evidence → Tests) and add a short Why?
| {{green "You're on the latest version."}} | ||
| {{- end}} | ||
| {{- if not .DevelopmentBuild}} | ||
| Set up Databricks AI tools for your coding agents: databricks aitools install |
There was a problem hiding this comment.
[nice to have] The hint is part of updateCheckTemplate, so cmdio.RenderWithTemplate writes it to stdout together with the version status. Every other aitools install hint (agents.MaybeHint, PrintSkillsTip) goes to stderr through cmdio.LogString. As a result, a script or agent reading the text result, for example databricks version --check | tail -n1 or a check that the last line is "You're on the latest version.", now gets the aitools line instead, and redirecting stderr can't remove it. Suggest dropping the line from the template and printing it after the render with cmdio.LogString(ctx, ...) (still skipped for development builds), so stdout keeps only the update-check result.
There was a problem hiding this comment.
Done in 90c8726: the line is no longer part of updateCheckTemplate. renderUpdateCheck renders the result and then prints the line with cmdio.LogString (stderr), only in text mode and not for development builds, so stdout keeps only the update-check result. TestRenderUpdateCheckAIToolsHint covers the stdout/stderr split, the dev-build skip and --output json.
848f252 to
90c8726
Compare
2b4b8c6 to
bbc2b80
Compare
90c8726 to
125c8ca
Compare
bbc2b80 to
6cd41f6
Compare
0148734 to
7d1dfc8
Compare
|
Re Question 1: went with (b) in 7d1dfc8. |
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 7d1dfc8. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 blocking issue(s), tagged [blocking]; 1 open design question.
This PR prints a databricks aitools install suggestion on stderr after databricks version --check when a coding agent's config dir is found without Databricks tooling. Stdout and JSON output are unchanged, and the tests pass. The check misses plugin installs for Codex CLI and GitHub Copilot, which are recorded only in the CLI's install state, so those users keep seeing the line after they install. That needs fixing before merge. Claude Code sessions also get this line in addition to the existing root hint. There is also an open question about whether this hint should go through the shared hint helpers.
Questions
- This adds a second install-hint path in
cmd/versionalongsideagents.MaybeHintandPrintSkillsTip. UnlikeMaybeHint, it shows to anyone who has a coding agent's config directory, including people at a terminal. It shows on everyversion --checkrun, with no session throttle, CI suppression or opt-out, and it uses new wording, althoughhint.gonotes thathintTextwas tuned for agents. Is that difference intended? Options: (a) keep it incmd/versionand explain in the description why its audience and frequency differ; (b) move it intolibs/aitools/agentsas a helper next toPrintSkillsTip, sharing wording, throttle and dedupe withMaybeHint; (c) drop it and rely onMaybeHint, extending that to other agents if the coverage matters.
Agreeing with existing threads
- [nit] PR description does not use the repo template: #7471 (review)
| if dbr.HasDetection(ctx) && dbr.RunsOnRuntime(ctx) { | ||
| return false | ||
| } | ||
| return !agents.HasDatabricksToolingInstalled(ctx) |
There was a problem hiding this comment.
[blocking] HasDatabricksToolingInstalled only recognizes a plugin through an agent's own manifest reader, and only Claude Code has one. For Codex CLI and GitHub Copilot, databricks aitools install installs the plugin by default, records it only in ~/.databricks/aitools/skills/.state.json and writes no skill dirs. So after a successful install, every version --check still tells the user to run databricks aitools install. I reproduced this with a release build: with ~/.codex and a recorded Codex plugin, databricks aitools version lists Plugin (Codex CLI, global, user scope): v0.2.6 and version --check still prints the line. Could you also count recorded plugin installs? For example, return !agents.HasDatabricksToolingInstalled(ctx) && len(installer.InstalledTools(ctx)) == 0, since InstalledTools already merges the manifests with the recorded state. Please add a test case for a recorded Codex plugin.
There was a problem hiding this comment.
Fixed in b53927b. The tip now also stays quiet when installer.InstalledTools reports a databricks plugin, so plugin installs recorded only in the CLI install state (Codex CLI, GitHub Copilot) count. The check lives in agents.MaybeVersionCheckTip, which takes installer.InstalledTools as a parameter because libs/aitools/installer imports libs/aitools/agents. Tests: TestRenderUpdateCheckAIToolsTip/codex_plugin_recorded_in_the_install_state (cmd/version) writes ~/.codex plus a recorded Codex plugin in .state.json and expects no tip. TestMaybeVersionCheckTip/plugin_recorded_in_install_state covers the same gate in the agents package.
| if output == flags.OutputText && !result.DevelopmentBuild && showAIToolsHint(ctx) { | ||
| cmdio.LogString(ctx, aitoolsHint) |
There was a problem hiding this comment.
[nice to have] In a Claude Code session without the Databricks tooling, agents.MaybeHint (root pre-run, cmd/root/root.go:95) already prints its install recommendation for version. version --check now adds a second, differently worded line for the same command. MaybeHint shows at most once per hour per session, but this line repeats on every check in the session. Agents are sent to this command by UpdateCLIAdvice (libs/genie/agentstream/renderer.go:33). I reproduced both lines with a release build and CLAUDECODE=1. Could you skip this line when MaybeHint covers the caller (Claude Code as the agent provider), for example with a small exported helper in libs/aitools/agents, and add a test case for it?
There was a problem hiding this comment.
Done in b53927b. agents.MaybeVersionCheckTip returns early when Claude Code is the agent provider, because agents.MaybeHint covers that caller in the root pre-run hook with its per-session throttle. A command now prints at most one install hint. Test: TestMaybeVersionCheckTip/claude_code_caller_is_left_to_MaybeHint.
| if dbr.HasDetection(ctx) && dbr.RunsOnRuntime(ctx) { | ||
| return false | ||
| } | ||
| return !agents.HasDatabricksToolingInstalled(ctx) |
There was a problem hiding this comment.
[nice to have] HasDatabricksSkillsInstalled only looks in $HOME/.databricks/aitools/skills and the legacy dir. After databricks aitools install --scope project for a skills-only agent (Kiro, Goose, Pi, Gemini), running version --check in that project still says to install. I reproduced this with ~/.kiro and project-scoped databricks-apps skills under .databricks/aitools/skills/ and .kiro/skills/. Consider also returning false when agents.DetectProjectInstalled(cwd) finds skills in the current project.
There was a problem hiding this comment.
Done in b53927b. The tip also stays quiet when agents.DetectProjectInstalled(cwd) finds Databricks skills in the current project. Test: TestMaybeVersionCheckTip/project_skills sets up ~/.kiro and .kiro/skills/databricks-apps in the working directory and expects no tip. Project-scoped plugin records are already covered by installer.InstalledTools, which reads both the global and the project install state.
| home := t.TempDir() | ||
| t.Setenv("HOME", home) | ||
| t.Setenv("USERPROFILE", home) | ||
| for _, name := range []string{"CLAUDE_CONFIG_DIR", "PI_CODING_AGENT_DIR", "GEMINI_CLI_HOME", "APPDATA", "XDG_CONFIG_HOME", "GOOSE_PATH_ROOT"} { |
There was a problem hiding this comment.
[nit] setupAgentHome hard-codes the config-dir override env vars, duplicating clearAgentDirOverrides in libs/aitools/installer/installer_test.go. If an agent adds a new override, this list goes stale and the "no agent detected" case can start reading the host's real agent dir. testutil.SetAgentEnv(t, "HOME", home, "USERPROFILE", home) (internal/testutil/agent.go, already used in cmd/aitools/funnel_test.go) clears the whole environment and avoids that.
There was a problem hiding this comment.
Done in b53927b. setupAgentHome is gone. Both tests use testutil.SetAgentEnv(t, "HOME", home, "USERPROFILE", home) and t.Chdir(t.TempDir()), so neither the host's agent dirs nor the repo's project dirs leak in.
7d1dfc8 to
b53927b
Compare
|
Re Question 1 (second review): went with (b) in b53927b. The line moved into |
6cd41f6 to
8643a8e
Compare
b53927b to
14f43cd
Compare
8643a8e to
4653c00
Compare
14f43cd to
8e754f4
Compare
4653c00 to
cf15bb3
Compare
8e754f4 to
14cb376
Compare
cf15bb3 to
9dac556
Compare
14cb376 to
d03528e
Compare
9dac556 to
e5b0edd
Compare
d03528e to
0aed2cb
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 0aed2cb. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR prints the apps init skills tip on stderr after text-mode databricks version --check when a coding agent is detected without Databricks tooling. It skips Claude Code callers, Databricks Runtime, development builds and JSON output, and stdout is unchanged. Every point from the earlier review rounds is fixed and CI is green. The remaining findings are non-blocking: the tip still appears in a project subdirectory or when skills live only in an agent's own skills dir, the dev-build skip leaves it without acceptance coverage, the doc comment is long, and the description doesn't use the template. One question is left: should apps init share the new suppression checks in this PR or in a follow-up?
Questions
MaybeVersionCheckTipnow also counts plugins recorded in the CLI install state and project-scoped skills. Butapps initprints the same tip text and still decides withHasDatabricksToolingInstalledalone (cmd/apps/init.go:1916). So a Codex CLI or GitHub Copilot user who installed the plugin throughdatabricks aitools installgets no tip fromversion --checkbut still sees it fromapps init. Is that split intended? Options: (a) keep this PR scoped toversion --checkand alignapps initin a follow-up; (b) move these checks into the shared predicate here, for example with apluginRecorded-style parameter like the one(*Agent).DatabricksToolingtakes. Option (b) makes both commands agree and drops the[]stringcallback.
Agreeing with existing threads
- [nit] PR description doesn't use the repo template: #7471 (review)
| if HasDatabricksToolingInstalled(ctx) || len(installedPlugins(ctx)) > 0 { | ||
| return | ||
| } | ||
| if cwd, err := os.Getwd(); err == nil && len(DetectProjectInstalled(cwd)) > 0 { |
There was a problem hiding this comment.
[nice to have] The new checks look only at the exact working directory (DetectProjectInstalled(cwd)) and, for global skills, only at the shared ~/.databricks/aitools/skills store. So the tip can claim skills are missing when the agent already loads them. Example: with ~/.kiro and a project install at the repo root (<repo>/.kiro/skills/databricks-apps, as aitools install --scope project writes), databricks version --check stays quiet in <repo> but prints the tip from <repo>/src. The same happens when the skills exist only in an agent's own skills dir, for example ~/.cursor/skills/databricks-apps written by another installer. I reproduced both with a release-versioned build of this commit. (*Agent).DatabricksTooling in libs/aitools/agents/calling.go already handles both cases: it checks a.SkillsDir(ctx) and walks projectDirs(cwd) up to the repository root. Could the project check walk projectDirs(resolvePath(cwd)) and also check each detected agent's own SkillsDir, or reuse DatabricksTooling for the detected agents? A test case run from a project subdirectory would cover it.
There was a problem hiding this comment.
Done in 9dd6d21. After the existing checks, MaybeVersionCheckTip now asks each registry agent's DatabricksTooling. That covers the agent's own skills dir (a.SkillsDir) and walks projectDirs(resolvePath(cwd)) up to the repository root. Plugins recorded in the install state are already handled by installedPlugins before that loop, so the loop passes a pluginRecorded that returns false. The old cwd-only DetectProjectInstalled(cwd) check is gone, since the walk covers it. New cases in TestMaybeVersionCheckTip: project skills at the repository root run from <repo>/src (with a .git at the root), and skills only in ~/.cursor/skills/databricks-apps. All three skills cases fail if the loop is removed.
| if err != nil { | ||
| return err | ||
| } | ||
| if output == flags.OutputText && !result.DevelopmentBuild { |
There was a problem hiding this comment.
[nice to have] The tip is skipped for development builds, and the comment doesn't say why. Acceptance binaries are always development builds, so this condition is why no acceptance test can show the new stderr line. .agents/rules/testing.md asks for acceptance tests first for user-visible cmd/... output. The unit tests call renderUpdateCheck directly, so they don't cover the RunE wiring (root.OutputType(cmd)) or how the tip interacts with the root pre-run MaybeHint line. Acceptance runs already set HOME to an empty temp dir (acceptance/internal/prepare_server.go), so dropping !result.DevelopmentBuild wouldn't change the existing acceptance/cmd/version output. Suggest dropping it (the tip is about agent tooling, not the release check). Then add cases to acceptance/cmd/version using sethome with a committed home/.codex (tip on stderr) and CLAUDECODE=1 (only the root hint). If dev builds should stay quiet, a one-line comment saying why would be enough.
There was a problem hiding this comment.
Dropped the skip in 9dd6d21. It didn't stand for anything about agent tooling, so development builds now get the tip as well. New cases in acceptance/cmd/version: sethome ./codex-home with a committed .codex shows the tip on stderr after the dev-build line; --output json in the same home prints no tip; sethome ./claude-home with CLAUDECODE=1 prints only the root MaybeHint line. The existing cases are unchanged, because the default acceptance HOME has no agent dirs. TestRenderUpdateCheckAIToolsTip now expects the tip for a development build too.
| // MaybeVersionCheckTip prints skillsTipText after `databricks version --check` | ||
| // when a coding agent is detected without Databricks tooling. It prints | ||
| // nothing: | ||
| // - when Claude Code drives the CLI: MaybeHint covers that caller in the | ||
| // root pre-run hook, with its per-session throttle, so a command prints at | ||
| // most one install hint; | ||
| // - on Databricks Runtime, like the other version notices; | ||
| // - when no agent is detected, a detected agent has the databricks plugin in | ||
| // its own manifest, or Databricks skills are installed globally | ||
| // (HasDatabricksToolingInstalled); | ||
| // - when installedPlugins reports a databricks plugin recorded in the CLI | ||
| // install state (Codex CLI and GitHub Copilot record plugin installs only | ||
| // there). Callers pass installer.InstalledTools, which this package cannot | ||
| // import; | ||
| // - when Databricks skills are installed in the current project. | ||
| // | ||
| // It logs no telemetry: `databricks version` never resolves auth, so the event | ||
| // would not be uploaded (see logHintShown). |
There was a problem hiding this comment.
[nit] This 18-line doc comment lists every early return, which .agents/rules/comments.md asks to avoid ("A doc comment gives the caller the contract and any surprising behavior, not a line-by-line re-spec"), and the list will drift as the gates change. Suggest keeping only what the code can't say: Claude Code is skipped because MaybeHint covers it, installedPlugins is a parameter because this package can't import installer, and no telemetry is logged because version never resolves auth.
There was a problem hiding this comment.
Shortened in 9dd6d21. The doc comment now covers only the three points you listed: Claude Code is skipped because MaybeHint covers it, installedPlugins is a parameter because this package can't import the installer, and no telemetry is logged because version never resolves auth. The checks themselves are in the code and the test table.
0aed2cb to
9dd6d21
Compare
|
Re Question 1 (third review): (a). This PR stays scoped to |
e5b0edd to
e1c95af
Compare
9dd6d21 to
7e2cf0e
Compare
e1c95af to
1a67570
Compare
7e2cf0e to
2920789
Compare
1a67570 to
f379423
Compare
`databricks version --check` (text output) is followed by the `apps init` skills tip on stderr when a coding agent is detected without Databricks tooling. Stdout keeps only the update-check result: ```diff Databricks CLI v0.245.0 You're on the latest version. +Tip: coding agents detected without Databricks skills. Run 'databricks aitools install' to install them. ``` The tip comes from `agents.MaybeVersionCheckTip`. It is not printed when an agent has the databricks plugin (in its own manifest or recorded in the CLI install state) or Databricks skills (in the shared store, in the agent's own skills dir, or in the project dirs up to the repository root), on Databricks Runtime, or when Claude Code drives the CLI (`agents.MaybeHint` already covers that caller). Changelog fragment: `.nextchanges/cli/version-check-aitools-hint.md`. Co-authored-by: Isaac <no-reply@databricks.com>
2920789 to
66114a4
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at 66114a4, on Simon's decision.
The design questions from the earlier automated reviews no longer block this PR: your answers are accepted, and any still-open ones are non-blocking. The changes since the last automated review (0aed2cb → 66114a4) address its comments.
Stacked on #7470.
Stacked on: aitools: add funnel telemetry to update, list, version and uninstall
This change is independent of the telemetry PRs below it in the stack; it touches
cmd/versionand adds one helper tolibs/aitools/agents/recommend.go.Summary
databricks version --check(text output) is followed on stderr by theapps initskills tip when a coding agent is detected without Databricks tooling. Stdout keeps only the update-check result:Databricks CLI v0.245.0 You're on the latest version. +Tip: coding agents detected without Databricks skills. Run 'databricks aitools install' to install them.The same tip follows "A new version is available ...", the download link, "Could not reach GitHub ..." and the development-build line. It comes from
agents.MaybeVersionCheckTip(next toPrintSkillsTip) and is not printed:installer.InstalledTools, which covers Codex CLI and GitHub Copilot), or Databricks skills in the shared store, in the agent's own skills dir, or in the project dirs up to the repository root ((*Agent).DatabricksTooling);agents.MaybeHintalready covers that caller with its per-session throttle, so a command prints at most one install hint;--output json.It logs no hint telemetry, because
databricks versionnever resolves auth and the event would not be uploaded. Changelog fragment:.nextchanges/cli/version-check-aitools-hint.md.Evidence
TestMaybeVersionCheckTipinlibs/aitools/agents/recommend_test.gocovers the shown case and each hidden case: no agent, plugin in the agent manifest, plugin recorded in the install state, global skills, project skills, project skills at the repository root when run from a subdirectory, skills only in the agent's own skills dir, Databricks Runtime, Claude Code caller.TestRenderUpdateCheckAIToolsTipincmd/version/version_test.gochecks that the tip goes to stderr (development builds included), that a Codex plugin recorded in.state.jsonsuppresses it end to end, and that--output jsonprints none.acceptance/cmd/versionruns the real command: a committedcodex-home/.codexshows the tip on stderr,--output jsondoes not, andCLAUDECODE=1prints only the rootMaybeHintline.TestUpdateCheckTemplatechecks that stdout is unchanged for all update-check outcomes.MaybeVersionCheckTipdisabled in turn, tests kept):go test ./libs/aitools/agents ./cmd/version→--- FAILfor the matching cases (recorded plugin, project and agent skills, Claude Code caller, Databricks Runtime)ok github.com/databricks/cli/libs/aitools/agents,ok github.com/databricks/cli/cmd/version,go test ./acceptance -run TestAccept/cmd/version→okMerge Danger
Door: two-way
Adds one stderr line after the text-mode update check; reverting removes it.
Blast Radius: small
version --checkis unchanged; the extra line is on stderr.--output jsonis unchanged.databricks versionwithout--checkis unchanged.This pull request and its description were written by Isaac.