Repository navigation
Conversation
cc3a109 to
3e0bf18
Compare
37c16a6 to
feb1410
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: 06fd311
Top 15 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at feb1410. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR makes the Claude Code marketplace refresh name the plugin's own marketplace, and stops a failed refresh from failing aitools install. The named refresh is a clean fix, and all CI checks pass. Nothing is blocking, but the continue-on-failure path hides a likely stale install: it logs only at debug and records the latest version. The changelog also names the wrong marketplace, and Codex's update step still refreshes every marketplace. Two design questions remain: whether the non-fatal fallback is needed, and why update and install now treat a refresh failure differently.
Questions
aitools installnow goes on whenclaude plugin marketplace update <name>fails, butaitools updatestill stops on the same refresh failure (UpdatePluginForAgentreturns on the first failing step, plugin.go:312-316). Is that difference intended? If it is, could the reason go in a code comment? If not, should update also tryplugin updateafter a failed refresh, the same way install does?- Naming the marketplace in the refresh already stops an unrelated, unreachable marketplace from failing the install. After that change, a refresh fails only when the plugin's own marketplace can't be refreshed, which is when the local copy is most likely out of date. AGENTS.md asks to avoid speculative fallbacks. Is the continue-on-refresh-failure path still needed? If so, would you surface it (warn, and don't record the latest version)? Or would you keep only the named refresh and restore the fatal refresh with rollback?
Comments outside the changed lines
- [nice to have]
libs/aitools/installer/plugin.go:296: When the refresh fails, the install now proceeds from the local marketplace copy, but the record still storesVersion: DisplaySkillsVersion(ref). Ifclaude-plugins-officialis already registered with an old copy and the refresh fails, the install succeeds with an older plugin. The CLI printsdatabricks plugin v<latest>, andaitools update --check(cmd/aitools/update.go:280-285) andaitools listreport "up to date". Before this PR a failed refresh stopped the install, so this drift could not happen. Consider leavingVersionempty whenrefreshErr != nil, so the record shows "version unknown" until the next successful update. - [nit]
libs/aitools/installer/plugin.go:108: The PR addsrecordedMarketplace, butrecordedInstallTargetstill has the samerec.Marketplace→agent.Plugin.Marketplacefallback written inline. Writereturn plugin + "@" + recordedMarketplace(agent, rec)so the refreshed marketplace and the update target cannot drift.
| } | ||
| } | ||
| return PluginRecord{}, &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} | ||
| log.Debugf(ctx, "%s plugin marketplace refresh failed, installing from the local copy: %v", agent.DisplayName, err) |
There was a problem hiding this comment.
[nice to have] When the refresh fails and the install succeeds, the user gets a clean success with no hint that the plugin may be out of date. .agents/rules/style-guide-go.md:237 says to "Warn for things the user should know about and might act on". The line also logs %v, which for a *process.ProcessError is only "<command>: exit status N" (libs/process/background.go:25-27), so even --debug loses the reason. Log at Warn with stderrOf(err) and an actionable hint, e.g. "could not refresh the marketplace, installed from the local copy; run databricks aitools update later: ".
There was a problem hiding this comment.
Done in 02922d2: the refresh failure is now logged with log.Warnf and stderrOf(err), with the hint: "Could not refresh the marketplace, installing the plugin from the local copy; run databricks aitools update later: ".
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` refreshes only the Databricks Claude Code marketplace, and installs from the local copy when the refresh fails, so another unreachable marketplace no longer blocks the install. ([#7473](https://github.com/databricks/cli/pull/7473)) | |||
There was a problem hiding this comment.
[nice to have] The entry says install "refreshes only the Databricks Claude Code marketplace", but the Claude Code registry entry uses claudePlugin(), i.e. Anthropic's shared claude-plugins-official marketplace (libs/aitools/agents/agents.go:163-168, 182). The refresh therefore runs claude plugin marketplace update claude-plugins-official. The tests' claudeAgent() helper uses databricks-agent-skills, which is probably where the wording came from. Reword it to something like "refreshes only the marketplace the Databricks plugin is listed in", and update the PR title the same way.
There was a problem hiding this comment.
Done in 02922d2: the fragment now says install and update "refresh only the marketplace the Databricks plugin is listed in", and mentions the warning and the "version unknown" record. PR title updated to "refresh only the plugin's own marketplace". The commit subject stays as is for now.
| case agents.NameClaudeCode: | ||
| return [][]string{ | ||
| marketplaceUpdateArgs(agent), | ||
| marketplaceUpdateArgs(agent, recordedMarketplace(agent, rec)), |
There was a problem hiding this comment.
[nice to have] The Codex branch a few lines up (line 187) still runs a bare codex plugin marketplace upgrade. codex plugin marketplace upgrade --help says "Omit MARKETPLACE_NAME to upgrade all configured Git marketplaces", so one unreachable third-party marketplace still fails databricks aitools update for Codex, which is the failure this PR fixes for Claude. Pass the name there too: {"plugin", "marketplace", "upgrade", recordedMarketplace(agent, rec)}.
There was a problem hiding this comment.
Done in 02922d2: Codex's update step now runs codex plugin marketplace upgrade <recordedMarketplace(agent, rec)>; TestUpdatePluginForAgentCodexTwoStep asserts codex plugin marketplace upgrade databricks-agent-skills. Changelog and PR body mention it.
| // A refresh failure (e.g. another marketplace the user added is unreachable) | ||
| // used to fail the install although the local marketplace copy had the plugin. |
There was a problem hiding this comment.
[nit] "A refresh failure (e.g. another marketplace the user added is unreachable)" no longer applies: the refresh is now named, so another marketplace cannot make it fail. Describe the case the fallback is actually for, such as the plugin's own marketplace being unreachable.
There was a problem hiding this comment.
Done in 02922d2: the comment now describes the plugin's own marketplace being unreachable while the local copy still lists the plugin.
| } | ||
|
|
||
| func TestInstallPluginRollsBackMarketplaceOnRefreshFailure(t *testing.T) { | ||
| func TestInstallPluginContinuesWhenRefreshFails(t *testing.T) { |
There was a problem hiding this comment.
[nit] TestInstallPluginContinuesWhenRefreshFails covers only a just-added marketplace, where the local copy is fresh by construction. The case the fallback exists for, a marketplace already registered whose refresh fails, is untested, and so is recordedMarketplace's fallback for a record with an empty Marketplace in pluginUpdateSteps. Add a case where plugin marketplace list returns the marketplace and the refresh fails: assert no marketplace remove and InstalledMarketplace == false. Add another where an update with PluginRecord{Marketplace: ""} refreshes the agent's marketplace.
There was a problem hiding this comment.
Done in 02922d2: added TestInstallPluginContinuesWhenRefreshOfRegisteredMarketplaceFails (marketplace list returns the marketplace, refresh fails: install runs, no marketplace remove, InstalledMarketplace == false, empty Version) and TestUpdatePluginForAgentClaudeRecordWithoutMarketplace (PluginRecord{Marketplace: ""} refreshes and updates against the agent's marketplace).
3e0bf18 to
410f869
Compare
feb1410 to
4eff713
Compare
410f869 to
88362b5
Compare
056b3e7 to
02922d2
Compare
anton-107
changed the title
|
Comments outside the changed lines, both done in 02922d2:
|
88362b5 to
679dd3e
Compare
02922d2 to
d467e3f
Compare
679dd3e to
ca64d39
Compare
4cc5453 to
605b293
Compare
|
Answers to the two Questions in the review, with the change in 605b293: 1. Install and update treat a refresh failure differently. That difference is gone. 2. Is the continue-on-refresh-failure path still needed? Yes, I kept it. A stale local copy that still lists the plugin installs a working plugin, so failing the whole run there blocks a usable install over a refresh that can be retried later. The run no longer hides that it fell back: it prints a warning that tells the user to run |
ca64d39 to
9d4b058
Compare
605b293 to
eb09c76
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at eb09c76. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR names the marketplace in the Claude Code refresh and the Codex upgrade, so an unreachable third-party marketplace no longer breaks aitools install or aitools update. A failed refresh now warns, continues from the local copy, and records the version as unknown. Everything from the earlier review is addressed, CI is green, and nothing is blocking. The main remaining point is that the refresh warning fires before the install or update result, so a failed install shows wrong advice and repeats the refresh error. The rest are small nits, plus one question about whether to split the fail-soft change into its own PR.
Questions
- This PR makes two changes: naming the marketplace in the refresh, which fixes the reported failure on its own, and making a failed refresh non-fatal for install and update, with the version-unknown record. AGENTS.md asks to keep each PR focused on one change. You explained earlier why you kept the fallback. Would you move the fallback into its own PR, so the naming fix can land and be reverted on its own? Or do you prefer to keep them together, and if so, why?
Comments outside the changed lines
- [nit]
cmd/aitools/list.go:394: This PR now writesVersion: ""after every failed refresh.agentStatusLabelusesversion == ""to mean "nothing picked yet". Example: Claude Code is installed in both scopes and the project refresh fails, so project has""and global has0.2.7, with release0.2.7. Project is visited first and is stale. Global then overwritesversion, so the label reads "databricks plugin · v0.2.7 · update available": the up-to-date version shows next to "update available", and the unknown scope is hidden. Track the pick with a separate bool instead ofversion == "". This code is outside the diff, but the PR is what makes the case common. - [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.md:48asks agent-written PRs to follow.github/PULL_REQUEST_TEMPLATE.mdexactly:## Changes,## Why,## Tests, in that order, with no other sections. This body uses## Summary,## Evidence, and## Merge Danger. It also opens with a duplicated "Stacked on" line. Rename the sections to the template headings and trim the Evidence block.
| } | ||
| _, err := runAgentCmd(ctx, pluginCmdTimeout, prepend(bin, args)) | ||
| if err != nil { | ||
| log.Warnf(ctx, "Could not refresh the %s marketplace, using its local copy for the %s plugin; run `databricks aitools update` later: %s", marketplace, agent.DisplayName, stderrOf(err)) |
There was a problem hiding this comment.
[nice to have] refreshMarketplace prints "Could not refresh the X marketplace, using its local copy …; run databricks aitools update later" as soon as the refresh fails, before the install or update has run. If that command then fails, the warning is wrong, and the refresh stderr is printed a second time through withRefreshError in the "Skipped …" line. Example: a first aitools install while GitHub is unreachable. marketplace add fails (its error is ignored at line 291), the refresh fails, and the install fails. No record is saved, so aitools update never retries Claude Code: UpdateInstalledPlugins only walks recorded plugins, and migration needs raw skills. The warning still tells the user to run it. Have refreshMarketplace only run the command and return the error, and warn only after InstallPluginForAgent/UpdatePluginForAgent succeeds. The failure path already carries the refresh error in the BlockedError detail.
There was a problem hiding this comment.
Done in 7123aae. refreshMarketplace now only runs the command and returns the error. The warning moved to warnLocalMarketplaceCopy, which InstallPluginForAgent and UpdatePluginForAgent call only after the install or update succeeds. On failure the refresh error appears once, in the BlockedError detail via withRefreshError, and there is no "run databricks aitools update later" advice.
| } | ||
|
|
||
| func TestInstallPluginRollsBackMarketplaceOnRefreshFailure(t *testing.T) { | ||
| func TestInstallPluginContinuesWhenRefreshFails(t *testing.T) { |
There was a problem hiding this comment.
[nit] On a successful install or update after a failed refresh, the warning is the only thing that tells the user they got the local copy. No test checks it, so it could be dropped or demoted to debug without any test failing. The package already captures logs (update_test.go:651-653 puts a slog.TextHandler on a buffer). Assert the message here and in the update equivalent. If the warning moves after the result, also assert that a failed install does not print it.
There was a problem hiding this comment.
Done in 7123aae. A captureWarnings helper puts a slog.TextHandler on a buffer. TestInstallPluginContinuesWhenRefreshFails (already registered case) and TestUpdatePluginForAgentContinuesWhenRefreshFails (Claude Code and Codex) assert the warning and the agent's stderr. TestInstallPluginRollsBackMarketplaceWhenRefreshAndInstallFail and TestUpdatePluginForAgentReportsRefreshWhenUpdateFails assert that a failed install or update does not print it. Demoting the warning to debug makes the two success tests fail.
| } | ||
| rec.Version = version | ||
| if !refreshed { | ||
| rec.Version = "" |
There was a problem hiding this comment.
[nit] Codex has no manifest reader (pluginInstalls is nil), so InstalledTools uses only the recorded version, and recordedPluginVersions skips empty ones (detect.go:58). After codex plugin marketplace upgrade databricks-agent-skills fails and codex plugin add succeeds, the record goes from 0.2.6 to "". Every later CLI call then reports no codex_* pair, although the plugin is installed. Before this PR, the update was skipped and the old version stayed. If that's intended, a note in InstalledTools would help. Otherwise, keep the previous version for telemetry, or emit an explicit unknown token.
There was a problem hiding this comment.
Kept the empty version and documented it in 7123aae: the InstalledTools doc comment now names this case (an agent without a plugin cache reader, such as Codex, whose last install or update ran from a marketplace copy that could not be refreshed). Keeping 0.2.6 would report a version the CLI can't vouch for: codex plugin add ran from the local copy, which may hold a newer or the same version. The empty record is also what makes aitools list and aitools version say "version unknown" for Codex. The next successful update records the version again. An explicit unknown token would change the <agent>_<version> token format, so I left that out of this PR.
| // After a failed refresh we can't tell which version the local copy held, | ||
| // so the record says "version unknown" until the next successful update. | ||
| version := DisplaySkillsVersion(ref) | ||
| if refreshErr != nil { |
There was a problem hiding this comment.
[nit] When installedMarketplace is true, marketplace add has just cloned the marketplace, so the local copy is current. A refresh failure right after that still records Version: "", and aitools update --check and aitools version then show "version unknown" for an up-to-date install (TestInstallPluginContinuesWhenRefreshFails asserts this). Either skip the refresh when we just added the marketplace, or clear the version only when refreshErr != nil && !installedMarketplace.
There was a problem hiding this comment.
Done in 7123aae. When installedMarketplace is true, marketplace add just cloned the marketplace, so a failed refresh right after it neither clears the version nor prints the warning. The version is cleared and the warning printed only when refreshErr != nil && !installedMarketplace. TestInstallPluginContinuesWhenRefreshFails is now a table: "just added" records 0.2.6 with no warning, and "already registered" records an empty version with the warning. A failed install still carries the refresh error in its detail in both cases.
| // failed refresh is not fatal: the local copy of the marketplace usually still | ||
| // lists the plugin, so the caller goes on with it. The local copy may be out of | ||
| // date, so the caller records the plugin version as unknown. | ||
| func refreshMarketplace(ctx context.Context, agent *agents.Agent, bin, marketplace string, args []string) error { |
There was a problem hiding this comment.
[nit] The marketplace parameter is only used in the log line, and the same name is already inside args. Install passes agent.Plugin.Marketplace twice, and update calls recordedMarketplace both in pluginRefreshArgs and at the call site. The argv is also built in two places: marketplaceUpdateArgs for Claude and pluginRefreshArgs for Codex. A single marketplaceRefreshArgs(agent, marketplace) switch, called inside refreshMarketplace(ctx, agent, bin, marketplace), would serve install and update and remove pluginRefreshArgs.
There was a problem hiding this comment.
Partly done in 7123aae. refreshMarketplace(ctx, bin, args) no longer takes agent or marketplace. The marketplace name is used only in the success-path warning, so recordedMarketplace is called once per path. I kept two argv builders: install refreshes only for Claude Code (marketplaceUpdateArgs), while update refreshes for Claude Code and Codex (pluginRefreshArgs). A single switch used by both would add codex plugin marketplace upgrade to aitools install for Codex, which install has never run. That is a behavior change outside this PR.
| assert.NotContains(t, stub.Commands(), "claude plugin marketplace remove databricks-agent-skills") | ||
| } | ||
|
|
||
| func TestInstallPluginContinuesWhenRefreshOfRegisteredMarketplaceFails(t *testing.T) { |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md:51 says "Use table-driven tests for multiple similar cases". This test differs from TestInstallPluginContinuesWhenRefreshFails (line 166) only in the plugin marketplace list stdout and the InstalledMarketplace assertion. TestUpdatePluginForAgentClaudeRecordWithoutMarketplace (line 243) differs from TestUpdatePluginForAgentClaudeOneStep (line 230) only in rec.Marketplace. Fold each pair into a table, as TestUpdatePluginForAgentContinuesWhenRefreshFails already does.
There was a problem hiding this comment.
Done in 7123aae. TestInstallPluginContinuesWhenRefreshFails is a table over "just added" and "already registered" (marketplace list output, InstalledMarketplace, version, warning). TestUpdatePluginForAgentClaudeOneStep is a table over a record with a marketplace and one without, and it asserts the exact command sequence.
eb09c76 to
7123aae
Compare
|
Re-review at Question 1 (split the fail-soft path into its own PR?). I'd keep them together. Naming the marketplace stops an unrelated marketplace from failing the refresh. After that, the refresh fails only when the plugin's own marketplace can't be reached, for example network or GitHub trouble during
PR template. I kept the body's layout, which the PRs in this stack share. The first line |
9d4b058 to
149db0e
Compare
7123aae to
66c0a4f
Compare
149db0e to
3edb030
Compare
66c0a4f to
b469e66
Compare
3edb030 to
239e195
Compare
b469e66 to
7982dd8
Compare
239e195 to
9cf349f
Compare
7982dd8 to
d50659f
Compare
9cf349f to
005fc18
Compare
d50659f to
efb3351
Compare
005fc18 to
f251254
Compare
efb3351 to
0d74442
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 0d74442. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR names the marketplace in the Claude Code refresh and the Codex upgrade, so an unreachable third-party marketplace no longer breaks aitools install or aitools update. If the plugin's own marketplace can't be refreshed, the commands now continue from the local copy, warn, and record the version as unknown. The naming fix is correct: both real CLIs accept a marketplace name. Every earlier thread is addressed, CI is green, and nothing is blocking. One nice-to-have: a Codex-only user whose refresh failed during an update is told again to install Databricks skills by version --check. There are also three small nits, and an open question on whether the fail-soft path should stay bundled with the naming fix.
Questions
- Thanks for explaining why the fail-soft refresh stays in this PR. One thing to confirm before approving: when an already-registered marketplace can't be refreshed,
aitools installandaitools updatenow exit 0 with a warning and record "version unknown", where before they failed with install-failed. Is the warning plus "version unknown" the signal you want in that case? Or would you rather land the marketplace-naming fix first and the fallback as a follow-up, so each can be reverted on its own?
Comments outside the changed lines
- [nit]
cmd/aitools/update.go:307: This PR makes an empty plugin version a common state. For it,printPluginCheckResultsprintsClaude Code databricks plugin version unknown (version unknown), becauseversionToken("")and the status are the same string. For the same record,aitools listsays "update available", but the check output doesn't tell the user that runningaitools updatewould help. Use a status such as "update may be available" for"", so the line readsdatabricks plugin version unknown (update may be available), and cover it in a test.
| // omitted rather than reported without one. That includes an agent without a | ||
| // plugin cache reader (Codex) whose record has no version because its last | ||
| // install or update ran from a marketplace copy that could not be refreshed. |
There was a problem hiding this comment.
[nice to have] Codex has no plugin cache reader, so MaybeVersionCheckTip relies on InstalledTools to know that a Codex plugin is installed (see its doc in libs/aitools/agents/recommend.go). recordedPluginVersions skips empty versions. Take a Codex-only user whose codex plugin marketplace upgrade databricks-agent-skills fails during aitools update while codex plugin add succeeds. The record becomes "", and from then on every databricks version --check prints "Tip: coding agents detected without Databricks skills. Run 'databricks aitools install'", even though the plugin is installed. The codex_* user-agent pair also disappears, which the earlier thread covered. Make the tip check look at whether a plugin record exists rather than whether a version is recorded. For example, pass a function that lists the recorded plugin agents whatever their version, or have InstalledTools emit an unknown version for them. A smaller point: Codex install never runs a refresh (marketplaceUpdateArgs is nil for Codex), so for Codex, "install or update" in this comment can only mean update.
There was a problem hiding this comment.
Done in 40e1da8. New installer.InstalledPluginAgents lists the agents that have the plugin in their own cache or recorded in the CLI install state, whether or not a version is known. databricks version --check now passes it to MaybeVersionCheckTip instead of InstalledTools, so a Codex-only user whose record has no version doesn't get the install tip. InstalledTools and its <agent>_<version> tokens stay the same. recordedPluginVersions now keeps agents whose record has no version, but a scope without a version never replaces one that has a version. Tests: TestInstalledPluginAgents and two new TestInstalledTools cases. You're right about Codex install never refreshing, so the InstalledTools comment now says "update".
| rec.Version = version | ||
| if !refreshed { | ||
| rec.Version = "" | ||
| } |
There was a problem hiding this comment.
[nit] When the marketplace refresh fails, this clears the version even if the record already holds the current release. For example, Claude Code is recorded at 0.2.7, the release is 0.2.7, and aitools update runs while GitHub is flaky. The refresh fails, and claude plugin update succeeds without changing anything. The record then becomes "", so aitools list shows "version unknown · update available" and update --check shows "version unknown" for a plugin that is up to date. Before this PR the record stayed at 0.2.7. The local copy can only have moved forward since the refresh that wrote that record, so keeping it is safe. Clear the version only when !refreshed && rec.Version != version, and add that case to TestUpdateInstalledPluginsRecordsUnknownVersionWhenRefreshFails.
There was a problem hiding this comment.
Done in 40e1da8. UpdateInstalledPlugins clears the version only when the refresh failed and the record doesn't already hold this release (refreshed || rec.Version == version keeps it). TestUpdateInstalledPluginsRecordsUnknownVersionWhenRefreshFails is now a table: 0.2.6 recorded becomes "", and 0.2.7 recorded stays 0.2.7.
| return PluginRecord{}, &BlockedError{Agent: agent.Name, Reason: ReasonInstallFailed, Detail: stderrOf(err)} | ||
| } | ||
| } | ||
| refreshErr := refreshMarketplace(ctx, bin, marketplaceUpdateArgs(agent, agent.Plugin.Marketplace)) |
There was a problem hiding this comment.
[nit] The Merge Danger section says "No extra network waits". Before this PR, a refresh that hit the 60s pluginCmdTimeout ended the run. Now the install (line 301) or the update runs next and can wait up to another 60s. On a machine where GitHub hangs instead of failing fast, aitools install for Claude Code can take about a minute longer than before. Please correct that sentence in the description.
There was a problem hiding this comment.
Corrected in the description. Merge Danger now says that a refresh which hangs until the 60s timeout is followed by the install or update, which can wait up to another 60s. So aitools install or aitools update can take about a minute longer when GitHub hangs instead of failing fast.
0d74442 to
40e1da8
Compare
|
Third review at Question 1. Yes. When an already-registered marketplace can't be refreshed, the warning plus "version unknown" is the signal I want. A stale local copy that still lists the plugin installs a working plugin. The warning names the marketplace, shows the agent's error and says to run
|
3e979d0 to
99db1f7
Compare
40e1da8 to
c2eaa89
Compare
99db1f7 to
07cf845
Compare
c2eaa89 to
20e3ef4
Compare
… install on it
Claude Code's `plugin marketplace update` with no name refreshes every marketplace the user has added, so one unreachable third-party marketplace failed the Databricks plugin install. The refresh now names the marketplace the Databricks plugin is listed in, and a failed refresh no longer stops the install or the update: both go on from the local copy of the marketplace and, once that succeeds, warn and record the plugin version as unknown. Codex's `plugin marketplace upgrade` in `aitools update` names the marketplace too.
```diff
InstallPluginForAgent (libs/aitools/installer/plugin.go)
marketplace add (if absent)
- claude plugin marketplace update
- on error: remove the marketplace we added, return install-failed
+ claude plugin marketplace update <marketplace>
+ on error: keep the error, continue
claude plugin install databricks@<marketplace>
on error: remove the marketplace we added
- return install-failed with the install error
+ return install-failed with the install error
+ + "(refreshing the marketplace also failed: <refresh error>)" if the refresh failed
- record Version = <ref>
+ if the refresh failed and the marketplace was not just added by us:
+ warn that the plugin came from the local copy (with the agent's stderr)
+ record Version = "" (version unknown)
+ else record Version = <ref>
UpdatePluginForAgent (aitools update)
- claude plugin marketplace update
+ claude plugin marketplace update <recorded marketplace, or the agent's current one>
- on error: return install-failed
+ on error: keep the error, continue
claude plugin update ...
- codex plugin marketplace upgrade
+ codex plugin marketplace upgrade <recorded marketplace, or the agent's current one>
+ on error: same as above
codex plugin add ...
on update error: return install-failed
+ + "(refreshing the marketplace also failed: <refresh error>)" if the refresh failed
- record Version = <ref>
+ if the refresh failed: warn as above, and record Version = "" (version unknown)
+ unless the record already holds <ref>
+ else record Version = <ref>
```
A record without a version still counts as an installed plugin for the `databricks version --check` tip (`InstalledPluginAgents`), and `aitools update --check` shows it as "version unknown (update may be available)".
Changelog fragment: `.nextchanges/cli/aitools-marketplace-refresh.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
07cf845 to
d82eb6b
Compare
20e3ef4 to
06fd311
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at 06fd311, 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 (0d74442 → 06fd311) address its comments.
Stacked on #7472.
Stacked on: aitools: raise the plugin probe timeout from 5s to 20s
Summary
Claude Code's
plugin marketplace updatewith no name refreshes every marketplace the user has added, so one unreachable third-party marketplace failed the Databricks plugin install. The refresh now names the marketplace the Databricks plugin is listed in (claude-plugins-officialfor Claude Code), and a failed refresh no longer stops the install or the update: both go on from the local copy of the marketplace and, once that succeeds, warn and record the plugin version as unknown. Codex'splugin marketplace upgradeinaitools updatenames the marketplace too.Changelog fragment:
.nextchanges/cli/aitools-marketplace-refresh.md.Evidence
libs/aitools/installer/plugin_test.go, run at this head withplugin.gotemporarily reverted to the parent's version and then restored:The existing tests now also check that the refresh command names the marketplace (
claude plugin marketplace update databricks-agent-skills,codex plugin marketplace upgrade databricks-agent-skills) and that a failed refresh leaves the recorded version empty unless we just added the marketplace.TestInstallPluginContinuesWhenRefreshFailsis a table: a just-added marketplace (version recorded, no warning) and an already registered one (install goes on, nomarketplace remove,InstalledMarketplace == false, empty version, warning with the agent's stderr).TestUpdatePluginForAgentClaudeOneStepalso covers a record without a marketplace (falls back to the agent's marketplace). The failed install and failed update tests assert that the local-copy warning is not printed. Foraitools update:TestUpdatePluginForAgentContinuesWhenRefreshFails(Claude Code and Codex: the refresh fails, the update still runs),TestUpdatePluginForAgentReportsRefreshWhenUpdateFails(both errors in the detail), andTestUpdateInstalledPluginsRecordsUnknownVersionWhenRefreshFails(the update succeeds, the state records an empty version).TestUpdateInstalledPluginsRecordsUnknownVersionWhenRefreshFailsalso covers a record that already holds the current release (kept).TestInstalledPluginAgentscovers plugin records without a version, andTestUpdateCheckPrintsPluginStatethe three--checkstatuses.TestAgentStatusLabelKeepsUnknownStaleScopecovers theaitools listlabel when one scope has an unknown version and the other is current. At this head:go build ./..., unit tests forlibs/aitools/...andcmd/aitools/...,task test-exp-aitools, thecmd/aitoolsacceptance tests, lint and format on changed files,task checks, andgo vetfor windows and darwin on the aitools packages pass. Not run on Windows or macOS, and not run against a realclaudebinary.Merge Danger
Door: two-way
Behavior change: a failing refresh used to fail
aitools installwith an install-failed error and remove the marketplace we had just added. Now the install goes on using the local copy of the marketplace; if that copy is stale and lacks the plugin, the install fails and the error includes both messages. A refresh failure that is followed by a successful install prints a warning with the agent's stderr and a hint to rundatabricks aitools update, and the plugin is recorded as "version unknown" soaitools update --checkandaitools listdon't report it as up to date. The warning is printed only after the install succeeds, and not when we just added the marketplace (that copy is fresh, so the version is recorded as usual).aitools listnow shows "version unknown · update available" when one scope has an unknown version and another is current, instead of the current scope's version. No exit code changes. A refresh that hangs until the 60s command timeout used to end the run; now the install or update runs next and can wait up to another 60s, so on a machine where GitHub hangs instead of failing fast,aitools installoraitools updatecan take about a minute longer than before.aitools updateused to skip the plugin with a warning when the refresh failed; now, for both Claude Code and Codex, it warns the same way, still runs the plugin update, and records the version as unknown, unless the record already holds the current release.aitools update --checkshows an unknown version as "version unknown (update may be available)", anddatabricks version --checkstill counts a plugin recorded without a version as installed, so it does not suggest installing Databricks tooling. If the update itself fails, the plugin is skipped as before and the error includes both messages. Both commands refresh only the one marketplace now.Blast Radius: small
Only the Claude Code plugin install and update path in
databricks aitools. This relies onclaude plugin marketplace update <name>andcodex plugin marketplace upgrade <name>accepting a marketplace name; that is checked only through the stubbed commands in the tests here.This pull request and its description were written by Isaac.