Repository navigation
aitools: do not block plugin installs on the latest-release lookup - #7474
Conversation
37c16a6 to
feb1410
Compare
491114e to
3081662
Compare
✅ Integration test reportCommit: 9138ddc
Top 17 slowest tests (at least 2 minutes):
|
feb1410 to
4eff713
Compare
3081662 to
9075cd6
Compare
4eff713 to
056b3e7
Compare
9075cd6 to
4b0c705
Compare
056b3e7 to
02922d2
Compare
4b0c705 to
521c720
Compare
02922d2 to
d467e3f
Compare
521c720 to
dd8dd43
Compare
d467e3f to
4cc5453
Compare
dd8dd43 to
270ba1d
Compare
4cc5453 to
605b293
Compare
270ba1d to
da3637c
Compare
605b293 to
eb09c76
Compare
da3637c to
f435418
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 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?
| // 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 | ||
| } |
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.
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) |
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.
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)) | |||
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.
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.
| 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") | ||
| } |
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.
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).
eb09c76 to
7123aae
Compare
7d445a5 to
401cca3
Compare
|
Thanks for the re-review.
On the doc-comment nit outside the changed lines, as of 21231db:
|
0d74442 to
40e1da8
Compare
21231db to
f6f7904
Compare
40e1da8 to
c2eaa89
Compare
f6f7904 to
9f26165
Compare
c2eaa89 to
20e3ef4
Compare
9f26165 to
8f2ef4e
Compare
20e3ef4 to
06fd311
Compare
8f2ef4e to
ae31019
Compare
simonfaltum
left a comment
simonfaltum
left a comment
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.
06fd311 to
b2ee61c
Compare
ae31019 to
bb8734f
Compare
b2ee61c to
dab2b7c
Compare
bb8734f to
7a75603
Compare
dab2b7c to
c9feaa5
Compare
7a75603 to
9138ddc
Compare
c9feaa5 to
af8a5dd
Compare
9138ddc to
ae5eede
Compare
af8a5dd to
dfb22a8
Compare
ae5eede to
276f78c
Compare
dfb22a8 to
4075f1c
Compare
276f78c to
e84d3cc
Compare
4075f1c to
0134b4b
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>
e84d3cc to
9bfea48
Compare
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.