Repository navigation
Conversation
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 14:33
37c16a6 to
feb1410
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 14:33
491114e to
3081662
Compare
Contributor
|
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): |
Collaborator
✅ Integration test reportCommit: ae31019
Top 13 slowest tests (at least 2 minutes):
|
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 16:52
feb1410 to
4eff713
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 16:53
3081662 to
9075cd6
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 16:54
4eff713 to
056b3e7
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 16:54
9075cd6 to
4b0c705
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 16:57
056b3e7 to
02922d2
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 16:57
4b0c705 to
521c720
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 17:28
02922d2 to
d467e3f
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 17:28
521c720 to
dd8dd43
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 19:28
d467e3f to
4cc5453
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 19:28
dd8dd43 to
270ba1d
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 19:42
4cc5453 to
605b293
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 19:42
270ba1d to
da3637c
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 19:59
605b293 to
eb09c76
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 19:59
da3637c to
f435418
Compare
simonfaltum
reviewed
Nov 11, 2026
simonfaltum
left a comment
simonfaltum
left a comment
Member
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at f435418. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 blocking issue(s), tagged [blocking]; 2 open design questions.
This PR stops databricks aitools install from failing plugin installs when the GitHub lookup of the latest skills release fails, by falling back to the embedded skills version. Unblocking the install is the right goal, and CI is green. The problem is that the fallback version is saved as the plugin's installed version and as the scope's state.Release, so version, list and update --check then show wrong versions. The code and #7069 already handle this case by recording "version unknown", and doing that here, plus a test of the install call site, would resolve the main concerns.
Questions
- This overlaps #7162, which handles the same failed lookup during plugin installs by recording the plugin version as unknown and leaving
state.Releaseunchanged. #7162 also coversaitools list. Should this PR take over that piece (and #7162 drop it), or should the plugin-install change land only through #7162? - When the release lookup fails,
aitools liststill exits with an error (cmd/aitools/list.go:155, covered byacceptance/cmd/aitools/list/latest-lookup-fails), andaitools updateskips plugin updates and raw-skills-to-plugin migrations (cmd/aitools/update.go:91-133). Is install-only the intended scope here, withlistandupdateleft for a follow-up, or should they get the same handling in this PR?
Comment on lines
+109
to
+117
| // GetPluginSkillsRef returns the skills ref to record for a plugin install, | ||
| // falling back to the embedded skills version when the latest release cannot be | ||
| // resolved. The agent's own CLI fetches the plugin from its marketplace, so the | ||
| // ref is bookkeeping only; an unreachable or rate-limited api.github.com must | ||
| // not block the install. | ||
| func GetPluginSkillsRef(ctx context.Context) (string, error) { | ||
| ref, _, err := getSkillsRef(ctx, true) | ||
| return ref, err | ||
| } |
Member
There was a problem hiding this comment.
[blocking] On a failed lookup, this returns the embedded pin (for example v0.2.32) as if it were the plugin's release. InstallPluginForAgent stores it as PluginRecord.Version (plugin.go:309), and RecordPluginInstalls writes it to state.Release (plugin.go:406). But the agent's marketplace installed its own current version, so the ref is not just bookkeeping.
After a rate-limited databricks aitools install --agents claude-code:
install,aitools versionandaitools listshowdatabricks plugin v0.2.32.aitools update --checkreports "update available" for a plugin that is already current.- If the scope also has raw skills at a newer release,
versionandlistshow those skills at v0.2.32 too.
Mixed installs can hit this as well: the skills group and the plugin path each look up the release. If the first lookup succeeds and the second gets a 403, the older pin overwrites the release the skills install just wrote.
The code already records "version unknown" when the plugin version can't be known (plugin.go:307-311, update.go:471-473), and #7069 says plugin operations treat a failed lookup as unknown.
Suggested change:
- In
executePlan, keepGetSkillsRef. On error, log at debug and continue withref = "", so the record says "version unknown". - In
RecordPluginInstalls, setstate.Releaseonly whenref != "".
With that, GetPluginSkillsRef is not needed.
Contributor
Author
There was a problem hiding this comment.
Done in 401cca3, following your suggestion. executePlan keeps GetSkillsRef; on error it logs at debug and continues with ref = "", so the plugin record says "version unknown". RecordPluginInstalls now sets state.Release only when ref != "", so a failed lookup no longer overwrites the release a skills install in the same run just wrote. GetPluginSkillsRef is removed.
| pluginCount := 0 | ||
| if len(pluginItems) > 0 { | ||
| ref, _, err := installer.GetSkillsRef(ctx) | ||
| ref, err := installer.GetPluginSkillsRef(ctx) |
Member
There was a problem hiding this comment.
[nice to have] No test covers this call site or what gets recorded after the fallback. The cmd tests set DATABRICKS_SKILLS_REF, and the new test calls GetPluginSkillsRef directly, so reverting this line to GetSkillsRef would still pass every test. Please add a test that runs the plugin path while the release lookup fails. For example, point DATABRICKS_CLI_GITHUB_API_URL at a stub that returns 403, as acceptance/cmd/aitools/list/latest-lookup-fails does, and stub installPluginForAgentFn and recordPluginInstallsFn. Assert that the command exits 0, the plugin is installed, and check which ref reaches both functions. That would also catch the recording issue raised on GetPluginSkillsRef.
Contributor
Author
There was a problem hiding this comment.
Added TestExecutePlanPluginWithUnresolvedSkillsRelease in 401cca3. It points DATABRICKS_CLI_GITHUB_API_URL at a stub that returns 403 (compat says "latest"), stubs installPluginForAgentFn and recordPluginInstallsFn, and asserts that executePlan succeeds, the plugin outcome is installed, the lookup ran once, and both functions get an empty ref. Reverting the call site to return the lookup error makes it fail with the 403. TestRecordPluginInstallsUnresolvedRefKeepsRelease covers the state.Release side.
| @@ -0,0 +1 @@ | |||
| * `databricks aitools install` keeps installing plugins when the lookup of the latest skills release on GitHub fails or is rate-limited. ([#7474](https://github.com/databricks/cli/pull/7474)) | |||
Member
There was a problem hiding this comment.
[nit] This entry says install "keeps installing plugins" when the lookup fails, but a mixed install can still be blocked before the plugin step. executePlan runs the skills group first and returns if it fails (cmd/aitools/install.go:581). The skills group disables its own fallback for a legacy global install (skills on disk, no state file) and for state whose release isn't a semver tag. Example: legacy global skills, Claude Code (plugin) plus Cursor (skills), and GitHub returning 403. The command fails and the plugin is not installed. Either run the plugin items when the skills group fails (still returning the skills error), or narrow this entry to plugin-only installs.
Contributor
Author
There was a problem hiding this comment.
Narrowed the entry to plugin-only installs in 401cca3, and it now says the plugin version is recorded as unknown. The order of the skills group and the plugin step is unchanged in this PR.
Comment on lines
+1497
to
+1508
| setBuildVersion(t, "1.22.0") | ||
| t.Setenv("DATABRICKS_SKILLS_REF", "") | ||
| t.Setenv("DATABRICKS_FORCE_EMBEDDED_COMPAT", "false") | ||
| t.Setenv("DATABRICKS_CACHE_ENABLED", "true") | ||
| withCachedCompat(t, skillsLatestSentinel) | ||
| lookups := 0 | ||
| orig := latestSkillsReleaseRefFn | ||
| t.Cleanup(func() { latestSkillsReleaseRefFn = orig }) | ||
| latestSkillsReleaseRefFn = func(context.Context) (string, error) { | ||
| lookups++ | ||
| return "", errors.New("HTTP 403 fetching latest skills release") | ||
| } |
Member
There was a problem hiding this comment.
[nit] This test repeats the setup of setupSkillsFallback and TestGetSkillsRefLatestReleaseLookupFails, and it rewrites the stubLatestSkillsReleaseRef stub just to count calls. .agents/rules/testing.md asks for table-driven tests for near-identical cases. Consider a single table over both entry points: the strict lookup expects the error, the plugin path expects its result. stubLatestSkillsReleaseRef could return a call counter. If GetPluginSkillsRef goes away as suggested above, this test can go too.
Contributor
Author
There was a problem hiding this comment.
GetPluginSkillsRef and this test are removed in 401cca3, so the duplicated setup is gone. The new tests are TestExecutePlanPluginWithUnresolvedSkillsRelease (cmd/aitools) and TestRecordPluginInstallsUnresolvedRefKeepsRelease (installer).
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 22:58
eb09c76 to
7123aae
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 22:58
f435418 to
7d445a5
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 23:01
7d445a5 to
401cca3
Compare
Contributor
Author
|
Thanks for the review. On the two questions:
|
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 23:37
7123aae to
66c0a4f
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 23:37
401cca3 to
87a619f
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 23:50
66c0a4f to
b469e66
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 23:50
87a619f to
203cf1c
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 11, 2026 23:58
b469e66 to
7982dd8
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 11, 2026 23:58
203cf1c to
3ab6a9a
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 00:06
7982dd8 to
d50659f
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 00:06
3ab6a9a to
09533d4
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 00:15
d50659f to
efb3351
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 00:16
09533d4 to
460a662
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 00:27
efb3351 to
0d74442
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 00:27
460a662 to
f1bf38a
Compare
simonfaltum
reviewed
Nov 12, 2026
simonfaltum
left a comment
simonfaltum
left a comment
Member
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at f1bf38a. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR stops databricks aitools install from failing plugin installs when the GitHub lookup of the latest skills release fails. The plugin version is recorded as unknown and the scope's existing state.Release is kept. The code is correct: I traced the empty-ref path through the install, version, list and update code and found no defects. Both new tests pass, CI is green, and all earlier review threads have been addressed. One question about timing remains: the current skills pin in cli-compat.json means this path isn't reached today, and list and update would still fail or skip work if "latest" is restored. Three small nits are also open: two stale comments and the PR body format.
Questions
- The shipped
internal/build/cli-compat.json(embedded and remote on main) pins skills to 0.2.32 for CLI >=1.22.0, and compat resolution falls back to that embedded pin. SoGetSkillsRefdoesn't reach the GitHub lookup today, and the new test reaches it only through a cached manifest that says "latest". If "latest" is restored, a 403 would still makeaitools listfail (cmd/aitools/list.go:150) andaitools updateskip plugin updates (cmd/aitools/update.go:91-139), even with this change. Is the intent to land this install-side piece now as groundwork? Or would you rather ship the install, list and update handling together (through #7162 or a combined PR) when "latest" comes back, keeping only thestate.Releaseguard here if a stacked PR depends on it?
Comments outside the changed lines
- [nit]
libs/aitools/installer/plugin.go:394: The doc comment still says "ref is the resolved skills release the install corresponds to", butrefcan now be empty, and an empty ref keeps the existingstate.Release. The inline comment at line 418 explains this, but a caller reading only the doc comment won't know it..agents/rules/comments.mdasks doc comments to state the contract. Consider adding a sentence such as: "An empty ref (release unresolved) keeps the existing state.Release." - [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.mdasks PR bodies to follow.github/PULL_REQUEST_TEMPLATE.mdexactly:## Changes,## Why,## Tests, with no new sections. It also asks for the agent disclosure as a single italicized last line. This body uses## Summary,## Evidenceand## Merge Dangerinstead, and the disclosure line is plain text. Consider moving the content under the template headings (Summary into Changes and Why, Evidence into Tests, Merge Danger folded into Why) and wrapping the last line in_..._.
| ref, _, err := installer.GetSkillsRef(ctx) | ||
| if err != nil { | ||
| return outcomes, err | ||
| log.Debugf(ctx, "Recording plugin version as unknown: %v", err) |
Member
There was a problem hiding this comment.
[nit] After this change, executePlan no longer fails on the plugin ref lookup. The comment above topLevelFailure in buildInstallOutput (line 710) still lists "ref lookup" among the top-level failures, and the skills group's own lookup is already covered by "skills-group install". Readers of that comment will expect a lookup failure in the top-level error/error_category fields, where it no longer appears. Consider dropping "ref lookup" from that list.
Contributor
Author
There was a problem hiding this comment.
Dropped "ref lookup" from that list in 21231db. The comment now names the skills-group install and plugin recording as the top-level failures.
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 09:11
f1bf38a to
21231db
Compare
Contributor
Author
|
Thanks for the re-review.
On the doc-comment nit outside the changed lines, as of 21231db:
|
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 09:19
0d74442 to
40e1da8
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 09:19
21231db to
f6f7904
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 10:01
40e1da8 to
c2eaa89
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 10:01
f6f7904 to
9f26165
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 10:20
c2eaa89 to
20e3ef4
Compare
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 10:20
9f26165 to
8f2ef4e
Compare
anton-107
force-pushed
the
aitools/databricks-marketplace-refresh
branch
from
November 12, 2026 10:24
20e3ef4 to
06fd311
Compare
Plugin installs no longer fail when the lookup of the latest skills release on api.github.com fails (for example HTTP 403 when rate-limited). The agent's own CLI fetches the plugin from its marketplace, so the skills ref only labels the recorded plugin version. On lookup failure the CLI now records the plugin version as unknown and keeps the scope's existing `state.Release`.
```diff
executePlan (cmd/aitools/install.go)
if plugin items
ref = GetSkillsRef(ctx)
- lookup error -> install fails
+ lookup error -> debug log, ref = "" (plugin version unknown)
install plugins
RecordPluginInstalls (libs/aitools/installer/plugin.go)
- state.Release = ref
+ if ref != "": state.Release = ref
```
`DATABRICKS_SKILLS_REF` and a pinned compat version still take precedence. Skills installs are unchanged.
Changelog fragment: `.nextchanges/cli/aitools-plugin-ref-fallback.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
force-pushed
the
aitools/plugin-ref-fallback
branch
from
November 12, 2026 10:24
8f2ef4e to
ae31019
Compare
simonfaltum
approved these changes
Nov 12, 2026
simonfaltum
left a comment
simonfaltum
left a comment
Member
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at ae31019, 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 (f1bf38a → ae31019) address its comments.
This branch has not been deployed
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
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.
Stacked on #7473.
Stacked on: aitools: refresh only the Databricks marketplace, and do not fail the install on it
Summary
Plugin installs no longer fail when the lookup of the latest skills release on api.github.com fails (for example HTTP 403 when rate-limited). The agent's own CLI fetches the plugin from its marketplace, so the skills ref only labels the recorded plugin version. On lookup failure the CLI now records the plugin version as unknown and keeps the scope's existing
state.Release, the same way a failed marketplace refresh already records "version unknown".executePlan (cmd/aitools/install.go) if plugin items ref = GetSkillsRef(ctx) - lookup error -> install fails + lookup error -> debug log, ref = "" (plugin version unknown) install plugins RecordPluginInstalls (libs/aitools/installer/plugin.go) - state.Release = ref + if ref != "": state.Release = refDATABRICKS_SKILLS_REFand a pinned compat version still take precedence. Skills installs are unchanged.This is the same handling #7162 uses for plugin installs (same code lines, comments and test names), so whichever lands first, the other rebases trivially. The cmd test stubs the lookup through
DATABRICKS_CLI_GITHUB_API_URL, which the release lookup uses on this base.Scope: install only.
aitools listandaitools updatestill fail or skip work on a failed lookup; they are left for a follow-up.Changelog fragment:
.nextchanges/cli/aitools-plugin-ref-fallback.md.Evidence
New test
TestExecutePlanPluginWithUnresolvedSkillsRelease(cmd/aitools): compat says "latest",DATABRICKS_CLI_GITHUB_API_URLpoints at a stub that returns 403, andinstallPluginForAgentFn/recordPluginInstallsFnare stubbed to capture the ref. It asserts the command succeeds, the plugin is installed, the lookup ran once, and both functions receive an empty ref.--- PASS: TestExecutePlanPluginWithUnresolvedSkillsReleaseNew test
TestRecordPluginInstallsUnresolvedRefKeepsRelease(libs/aitools/installer): a plugin record written with an empty ref keeps thestate.Releaserecorded by an earlier install.Also passing at this head:
go build ./...,task test-exp-aitools,task lint-q,task fmt-q,task checks, and thecmd/aitoolsacceptance tests.Merge Danger
Door: two-way
A few lines at the plugin call site and in
RecordPluginInstalls. Reverting restores the old behavior.Blast Radius: small
databricks aitools installwith plugin targets exits 0 and shows the plugin version as unknown instead of failing.state.Releaseis not overwritten.This pull request and its description were written by Isaac.