Repository navigation
aitools: report the calling agent's own Databricks tooling in the User-Agent - #7442
Conversation
…r-Agent
When a coding agent runs the CLI, the SDK already puts agent/<name> in the
User-Agent. Add aitools-agent/{plugin|skills|none} next to it: whether that
agent has the Databricks plugin or Databricks skills installed in its own
locations. The existing aitools/<tool>_<version> pairs list the plugins of
every tool on the machine, so they cannot answer this.
- agents.CallingAgent maps useragent.AgentProvider() to a registry agent.
- (*Agent).DatabricksTooling checks the agent's own plugin manifest (Claude
Code; project/local scope records count only inside their project), or
the global CLI install state for agents without a manifest reader, and
databricks* dirs in the agent's global and project skills dirs (looked up
from the working directory up to the repository root).
- The Claude Code manifest reader returns every databricks record from every
marketplace entry; DatabricksPluginVersion reports the highest version
instead of the first entry in random map order.
- Project paths match by lexical containment after resolving symlinks, then
by file identity (case-insensitive filesystems, 8.3 names, bind mounts).
Co-authored-by: Isaac <no-reply@databricks.com>
✅ Integration test reportCommit: 9af9b82
Top 3 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Approving. Nothing blocking; the inline comments are nits and nice-to-haves. Two larger points, neither blocking:
- The install hint (
hint.go,claudeToolingInstalled) and the newDatabricksToolingdefine "Claude has Databricks tooling" differently: the hint counts a plugin in any scope plus the shared~/.databricks/aitools/skillsstore and never looks at.claude/skills; the segment does the opposite. The description already calls this out. Either point the hint at the new check, or leave a cross-reference comment so the split reads as deliberate rather than accidental. - The marketplace version fix changes the existing
aitools/claude-code_<version>value on machines with the plugin from two marketplaces. Reasonable to keep here since the new code needs the full record list, but it does weaken the "revert removes only the segment" claim. A separate PR would give a cleaner revert story if you care about that.
Nit: the PR body uses Summary / Evidence / Merge Danger; the repo template (.github/PULL_REQUEST_TEMPLATE.md, enforced by the pr-checklist skill) is Changes / Why / Tests.
| // databricks plugin install for the agent. Only the global state is read: the | ||
| // CLI installs plugins in project scope only for agents with SupportsProjectScope | ||
| // (see mapAgentScope in cmd/aitools), and those have a manifest reader. | ||
| func pluginRecorded(ctx context.Context, agentName string) bool { |
There was a problem hiding this comment.
[nice to have] This re-implements install-state reading from installer: its own .state.json constant (installer.stateFileName), its own home + CanonicalSkillsDir join (installer.GlobalSkillsDir), and a partial struct instead of installer.LoadState. Harmless today (migrateState only bumps the version), but a future state change would silently miss this reader.
The only production caller is WithCallingAgentToolingInUserAgent in installer, which already has all the pieces. Consider computing the Tooling value there and exposing only a plugin-record check from agents, or moving the state types into a leaf package both can import.
There was a problem hiding this comment.
Done in 9c0e30d. agents no longer reads the state file: DatabricksTooling(ctx, pluginRecorded func() bool) takes the check, and installer.WithCallingAgentToolingInUserAgent implements it with GlobalSkillsDir + LoadState (globalPluginRecorded). The duplicated .state.json constant and partial struct are gone. It is a func rather than a bool so the state is still never read for Claude Code (manifest authoritative) or for agents without a plugin.
| func projectDirs(cwd string) []string { | ||
| cwd = resolvePath(cwd) | ||
| dirs := []string{cwd} | ||
| root, err := folders.FindDirWithLeaf(cwd, ".git") |
There was a problem hiding this comment.
[nice to have] Stopping at the nearest .git undercounts in two cases: a Claude Code project that is not a git repo (only cwd is checked, so <project>/.claude/skills/databricks* is missed when the CLI runs from a subdirectory), and a nested repo or submodule inside the agent's project. The doc says this matches Claude Code's own lookup; a source link would settle that (the comments rule asks for one when depending on an external tool's behaviour). If Claude Code walks further, walking to the filesystem root is bounded and has no false positives since ~/.claude/skills is already the global dir.
There was a problem hiding this comment.
Keeping the stop at the repository root, now with the source link (b0cc730): https://code.claude.com/docs/en/skills says Claude Code "loads project skills from .claude/skills/ in the directory where you start it and in every parent directory up to the repository root", and in a linked worktree "only up to the worktree root". Walking to / would count skills the agent does not load, e.g. a .claude/skills in a parent of the repo. For a nested repo or submodule, the walk stops at the inner repo, which matches the doc's "repository root" as far as it says. The doc does not describe non-git directories, so the check stays conservative there (cwd only).
| // and Windows, so it exercises the same identity check on Linux. Mounting needs | ||
| // a private mount namespace, so the test re-runs itself under `unshare -rm` | ||
| // (an unprivileged user namespace) and skips where those are not allowed. | ||
| func TestPluginInstallAppliesToBindMountedProjectPath(t *testing.T) { |
There was a problem hiding this comment.
[nice to have] In CI this skips on all three Linux jobs ("unprivileged mount namespaces are not available"), and TestPluginInstallAppliesToProjectPathInAnotherCase skips there too (ext4 is case-sensitive). So the positive os.SameFile path is only covered on the macOS and Windows runners, and this file adds 57 lines of unshare / syscall.Mount machinery that never runs in CI. Either drop the Linux test or add a deterministic positive case.
There was a problem hiding this comment.
Dropped plugins_linux_test.go in c28c083. The os.SameFile positive path stays covered by TestPluginInstallAppliesToProjectPathInAnotherCase on the macOS and Windows runners.
| // agent has them. Installs the CLI did not perform and that leave no trace in | ||
| // these locations (e.g. a Codex plugin installed with `codex` directly) read as | ||
| // ToolingNone. Read errors read as "not installed". | ||
| func (a *Agent) DatabricksTooling(ctx context.Context) Tooling { |
There was a problem hiding this comment.
[nice to have] Per-command cost under Claude Code: the plugin manifest is now parsed three times (InstalledTools, here, MaybeHint), and cwd is EvalSymlinks-resolved once in projectDirs and again inside isSameOrUnder for every project/local record. Also, pluginRecorded reads and decodes the state file for agents with Plugin == nil (Cursor, OpenCode, Pi, Gemini, Goose, Kiro, Antigravity), where the answer is false by construction. Resolving cwd once and guarding the state read with a.Plugin != nil would remove most of it.
There was a problem hiding this comment.
Done in 9c0e30d: cwd is resolved once in DatabricksTooling (9af9b82 drops a reliance on os.Getwd returning "" on error); projectDirs and isSameOrUnder take the resolved path and no longer call EvalSymlinks on it per record. pluginRecorded is called only for agents with Plugin != nil and no manifest reader, so Cursor, OpenCode, Pi, Gemini, Goose, Kiro and Antigravity do not read the state file (TestDatabricksToolingSkipsInstallState). I left the repeated manifest parse alone: they are small local reads, and I'd add caching only if it shows up in measurements.
| // a plugin removed with the agent's own CLI leaves a stale CLI state record | ||
| // behind, so the record is not consulted for these agents. A plugin that | ||
| // is installed but disabled in the agent's settings still counts; | ||
| // - for agents without a reader (Codex, Copilot), the plugin recorded for |
There was a problem hiding this comment.
[nice to have] The doc explains why the CLI state is ignored for Claude (stale after claude plugin uninstall), but for Codex and Copilot the state is the only source, so plugin here includes plugins since removed through the agent's own CLI. One sentence on that would tell analysts what plugin means for those agents.
There was a problem hiding this comment.
| // installed in more than one scope has several records. | ||
| type pluginInstall struct { | ||
| Version string `json:"version"` | ||
| // Scope is Claude Code's install scope: "managed", "user", "project" or |
There was a problem hiding this comment.
[nit] Source link for the installed_plugins.json scope / projectPath semantics? The comments rule asks for one when relying on an external tool's format; the SDK link at the top of calling.go is the pattern.
There was a problem hiding this comment.
Linked in b0cc730. Claude Code documents that installed_plugins.json "records each install with its scope, installPath, and version" (https://code.claude.com/docs/en/plugins/loading#check-which-stage-a-plugin-reached), and what each scope covers (https://code.claude.com/docs/en/plugins/install#choose-an-install-scope). I found no public doc for projectPath, so the comment now says so instead of claiming Claude Code requires it.
| return useragent.InContext(ctx, callingAgentToolingUserAgentKey, toolingUserAgentValue(a.DatabricksTooling(ctx))) | ||
| } | ||
|
|
||
| func toolingUserAgentValue(t agents.Tooling) string { |
There was a problem hiding this comment.
[nit] Missing doc comment. This reads naturally as a String() method on agents.Tooling.
There was a problem hiding this comment.
Done in 6825641: agents.Tooling now has a documented String() method, and the free function is gone.
| } | ||
| } | ||
|
|
||
| func TestCallingAgentFromEnv(t *testing.T) { |
There was a problem hiding this comment.
[nit] These four TestCallingAgent* functions differ only in env input and expected result; a table fits the repo's testing rule. Also agents.CallingAgent().Name (and AgentForProvider(...).Name above) panics on nil instead of failing via require.NotNil. And TestAgentForProviderCoversRegistry hardcodes the SDK product names, so an SDK rename keeps it green while CallingAgent returns nil in production; driving it through the agents' env vars (CLAUDECODE, COPILOT_CLI, ...) would exercise the real mapping.
There was a problem hiding this comment.
Done in c28c083: a single table-driven TestCallingAgent sets each agent's SDK env var (CLAUDECODE, COPILOT_CLI, GEMINI_CLI, ...) through testutil.SetAgentEnv, uses require.NotNil before .Name, and checks that every registry agent is reached by some case. An SDK product rename now fails it. TestAgentForProvider and TestAgentForProviderCoversRegistry are folded into it.
| // first only about one time in eight), so a reader that keeps only the | ||
| // first databricks entry fails some of these reads. | ||
| got := map[string]int{} | ||
| for range 200 { |
There was a problem hiding this comment.
[nit] The 200-iteration loop re-reads the manifest from disk to defeat map order probabilistically. Asserting len(installs) == 2 from claudePluginInstalls (this package has access) proves the same property deterministically, with one plain DatabricksPluginVersion assertion on top. Same for the loop in calling_test.go.
There was a problem hiding this comment.
Done in c28c083: TestDatabricksPluginVersionAcrossMarketplaces asserts len(installs) == 2 from claudePluginInstalls, plus one DatabricksPluginVersion == "0.3.0" assertion. The loop in calling_test.go is now a single DatabricksTooling call, and 9af9b82 adds TestDatabricksPluginActiveInChecksEveryRecord, which fails deterministically if only the first record is checked.
| ToolingPlugin | ||
| ) | ||
|
|
||
| // DatabricksTooling reports the Databricks AI tooling installed for this agent |
There was a problem hiding this comment.
[nit] This doc restates each branch of the 20-line body and has to move in lockstep with it. The non-obvious facts (the manifest is authoritative for Claude because claude plugin uninstall leaves a stale CLI record; the shared store cannot attribute skills to an agent) fit in about four lines. Same applies to the SetAgentEnv doc.
There was a problem hiding this comment.
Trimmed in 9c0e30d to the facts the code does not show: read errors count as not installed, why Claude's manifest is authoritative, what plugin means for Codex/Copilot, the shared store not attributing skills, plus a pointer to the install hint's different definition (b0cc730). The test helper doc is shorter too (c28c083).
DatabricksTooling no longer parses the CLI install state itself. It takes pluginRecorded, which installer implements with LoadState and GlobalSkillsDir, so a future state change cannot silently miss this reader. pluginRecorded is called only for agents with a plugin and no manifest reader (Codex, Copilot), so agents without a plugin no longer read the state file. The working directory is resolved once per call instead of again for each project-scope record and in projectDirs. Remove HasDatabricksTooling (no production caller) and the nil guard in databricksPluginActiveIn that its only caller already checks. Trim the DatabricksTooling doc to the facts the code does not show. Co-authored-by: Isaac <no-reply@databricks.com>
Only copilot-cli and gemini-cli differ from their registry names, so AgentForProvider maps those two and looks every other product up with ByName. New registry agents no longer need a second list. "multiple", "" and products without a registry entry still return nil. Replace the free toolingUserAgentValue function with a documented String method on agents.Tooling. Co-authored-by: Isaac <no-reply@databricks.com>
Replace the agenttest package with testutil.SetAgentEnv(t, kv...), which SetAgent now builds on. It keeps TMPDIR, TEMP and TMP, so tests no longer have to call t.TempDir before clearing the environment. Make TestCallingAgent table-driven through the agents' own env vars (CLAUDECODE, COPILOT_CLI, ...), so the test exercises the SDK's real product names, fails via require.NotNil instead of panicking, and checks that every registry agent is reachable. Replace the 200-read map-order loops with a deterministic check that claudePluginInstalls returns the records of both marketplace entries. Delete the Linux bind-mount test: it needs unprivileged mount namespaces, which the CI Linux runners do not allow, so it never ran there. The case-folding test still covers the file-identity path on macOS and Windows. Co-authored-by: Isaac <no-reply@databricks.com>
Link the Claude Code skills doc for the project skills lookup that stops at the repository root (the worktree root in a linked worktree), and the plugin docs for the installed_plugins.json scope and version fields. The projectPath field is not in Claude Code's public docs; say so instead of claiming it is required. Cross-reference the install hint's claudeToolingInstalled and DatabricksTooling, so the different definitions read as deliberate. Co-authored-by: Isaac <no-reply@databricks.com>
Add a TestCallingAgent case for AGENT=copilot, which now maps to the registry agent through ByName, and a deterministic test that databricksPluginActiveIn checks every manifest record, not only the first. Stop relying on os.Getwd returning "" on error, say that Codex and Copilot miss plugins installed with their own CLI, and align a test comment with the projectPath doc. Co-authored-by: Isaac <no-reply@databricks.com>
|
Thanks for the review, @simonfaltum. Pushed five commits on top of fd8c2c5 (no rebase, so the delta is reviewable on its own):
Kept as is, with reasons in the threads: the project-skills walk still stops at the repository root (Claude Code's documented lookup); the hint keeps its own definition for now, since changing it would change hint behaviour; the marketplace fix stays here, and the description now spells out what a revert changes. Deferred: caching the repeated manifest parse, until a measurement shows it matters. The PR description now follows the repo template. |
❌ Integration test reportCommit: c820432
38 interesting tests: 19 FAIL, 19 flaky
Top 50 slowest tests (at least 2 minutes):
|
Changes
When a coding agent runs the CLI, the SDK already puts
agent/<name>in the User-Agent. This PR addsaitools-agent/{plugin|skills|none}next to it: whether that agent has the Databricks plugin or Databricks skills installed in its own locations.AGENT=...value that is not a registry agentaitools-agent/pluginaitools-agent/nonedatabricks aitools installaitools-agent/pluginclaude plugin)aitools-agent/none~/.claude/skills/databricks-appsaitools-agent/skillsrepo/jobsrepo/.claude/skills/databricks-apps,repo/.gitaitools-agent/skillsSupporting change in
libs/aitools/agents/plugins.go: the Claude Code manifest reader now returns every databricks install record (version, scope, project path) from every marketplace entry (databricks@<marketplace>). Before, it kept the first matching entry in Go's random map order.DatabricksPluginVersionreports the highest version across those records, so the existingaitools/claude-code_<version>pair is stable too. Project paths match the working directory by containment after resolving symlinks, then by file identity (os.SameFile), which covers case-insensitive filesystems, Windows 8.3 names and bind mounts.Tests use a new
testutil.SetAgentEnv(t, kv...)(whichSetAgentbuilds on) to set the agents' own env vars.Why
Usage analysis needs to know whether the agent calling the CLI already has the Databricks tooling. The existing
aitools/<tool>_<version>pairs cannot answer that: they list the plugins of every tool on the machine, so a Claude Code plugin also shows up on requests made by Codex.Decisions not visible in the code:
~/.databricks/aitools/skillsstore is not consulted: it holds the skills of every agent the CLI installed into, so it cannot say which agent has them. A Claude Code plugin that is installed but disabled in Claude's settings counts as installed.claude plugin uninstallleaves the CLI's own install record behind. For Codex and Copilot the CLI install state is the only source, sopluginmisses plugins installed with the agent's own CLI and includes plugins later removed with it..claude/skillsdirs; the segment does the opposite. So in one command the hint can stay silent while the segment saysnone(plugin for another project only), or fire while the segment saysskills(project skills only). A comment on each function points at the other. Aligning them would change hint behaviour, so it is left for a separate change.No telemetry event or proto changes. The segment travels in the User-Agent of every request the command makes. Before each command run under a detected registry agent, the CLI reads a few local files (Claude Code's plugin manifest, or the global CLI install state for Codex and Copilot, and the agent's skills dirs). Read errors count as "not installed" and never fail the command.
Reverting this PR removes the segment and also changes the
aitools/claude-code_<version>value back to the first marketplace entry in map order, on machines where the plugin comes from more than one marketplace.Tests
databricks@a0.2.9 anddatabricks@b0.3.0, 200 reads):expected: map[string]int{"0.3.0":200},actual: map[string]int{"0.2.9":174, "0.3.0":26}. After:TestDatabricksPluginVersionAcrossMarketplacesasserts deterministically thatclaudePluginInstallsreturns both records and the version is0.3.0.TestWithCallingAgentToolingInUserAgent(10 cases), e.g.CODEX_CI=1with only a Claude Code plugin givesaitools-agent/none,CLAUDECODE=1with the same manifest givesaitools-agent/plugin, and a corrupt CLI state givesaitools-agent/none.TestCallingAgentdrives the mapping through each agent's SDK env var (CLAUDECODE,COPILOT_CLI, ...) and checks that every registry agent is reachable; a free-textAGENT=copilotmaps to Copilot.TestDatabricksTooling(per-agent locations, project-scope records inside and outside the project, stale CLI state, project skills at the repository root seen from a subdirectory, project skills above the repository root not counted),TestDatabricksToolingSkipsInstallState,TestDatabricksPluginActiveInChecksEveryRecord,TestPluginInstallAppliesTo*.TestPluginInstallAppliesToProjectPathInAnotherCaseruns where the temp filesystem folds case (macOS, Windows) and skips on Linux.go build ./...,./task test-exp-aitools,go testoflibs/aitools,cmd/aitools,libs/telemetry,cmd/root,internal/testutiland the othertestutil.SetAgentusers (libs/agent,cmd/auth),./task lint,./task lint-q,./task fmt-q,./task deadcode,./task checks: pass.auth/credentials,cmd/api,bundle/user_agent,cmd/workspace/apps) pass unchanged: the harness clears the agent env vars, so noagent/oraitools-agent/segment appears.This pull request and its description were written by Isaac.