Repository navigation
Conversation
5637f13 to
21d6b41
Compare
a5568f8 to
2b4b8c6
Compare
|
Commit: Regression test reportSelected 11 changed tests (limit=20, 0 not selected) 11 tests: regression=7 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): |
✅ Integration test reportCommit: f379423
Top 13 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 2b4b8c6. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR extends the aitools_command_event funnel from install to update, list, version and uninstall. It also adds installer results so that runs where every attempt only produced a warning are reported as FAILURE, and nothing-installed runs as NOTHING_TO_DO. The wiring is correct, error text and exit codes are unchanged, and CI is green; I found nothing blocking. Two questions are open: whether these four mostly local commands should pay the background auth and upload cost, and whether the installer refactor belongs in this PR. Separately, a few edge cases report the wrong outcome or category (untagged prompt errors, update --check without a manifest, unknown-agent records), and there are some test and changelog nits.
Questions
version,list,updateanduninstallnow resolve workspace auth in the background on every run, only so their telemetry can upload.versionanduninstallmade no network calls before, andlistis whatcmd/quickstart/quickstart-agent.mdtells agents to run. The cost is at least 3 s per run while the workspace is unreachable (up to about 6 s when it is slow), and these commands can now hit the abandoned OAuth refresh described intelemetry.go:85-90. Is the data from these four funnels worth that? Options: keep it as is; instrument onlyupdateanduninstalland leave the purely localversionandlistalone; or keep the funnel oninstallonly.- This PR combines the funnel wiring with installer API changes:
UninstallSkillsWithResult/UninstallResult, theremoveAllFnseam,removeSkillExposuresFromAgentsreturning results, sorted removal on full uninstall, andPluginUpdate.Errwith failed plugins returned last. They exist to classify outcomes, but they change the installer API and behavior on their own. Should they stay here, or move into a separate refactor PR that this one builds on?
Comments outside the changed lines
- [nit]
libs/aitools/installer/uninstall.go:109: A plugin record for an agent this CLI does not know is skipped with a warning but never added topluginFailures.UpdateInstalledPluginsdoes the same (update.go:455-458). Suppose the state holds only such a record, for example one written by a newer CLI.uninstallandupdatethen act on nothing, record no failure, and finishSUCCESS. Record these skips: append an error topluginFailureshere, and return aPluginUpdatewithErrset inUpdateInstalledPlugins.
| return nil, errors.New("skills are installed in both global and project scopes; use --scope=global, --scope=project, or --scope=both to specify which to update") | ||
| return nil, withCategory(errors.New("skills are installed in both global and project scopes; use --scope=global, --scope=project, or --scope=both to specify which to update"), protos.AitoolsErrorCategoryInvalidFlags) | ||
| } | ||
| scopes, err := promptUpdateScopeSelection(ctx) |
There was a problem hiding this comment.
[nice to have] update and uninstall now log the funnel, but their prompt errors are returned without promptError. That covers the scope pickers here and at line 260, and the confirm prompt (cmdio.AskYesOrNo via confirmUninstall, uninstall.go:123). If huh cannot open the terminal it returns an *fs.PathError, so the run finishes FAILURE:FILESYSTEM; that is the mislabel promptError exists to prevent. Ctrl-D at Proceed? [y/N] returns io.EOF and finishes FAILURE:UNCATEGORIZED. install reports both as PROMPT_FAILED. Wrap these three returns with promptError(err). A Ctrl-C abort keeps its CANCELLED outcome.
There was a problem hiding this comment.
Wrapped all three with promptError: the update and uninstall scope pickers (inside defaultPromptUpdateScopeSelection / defaultPromptUninstallScopeSelection, around the huh call only, as install's prompts now do, so a home-dir or cwd error is not tagged PROMPT_FAILED) and cmdio.AskYesOrNo in confirmUninstall (not the whole function, so a state-load error keeps its own category). Ctrl-C still finishes CANCELLED. TestUninstallConfirmPromptFailure covers EOF at the confirm: FAILURE:PROMPT_FAILED. bbc2b80
| if err != nil { | ||
| return err | ||
| } | ||
| updatedAnything = true |
There was a problem hiding this comment.
[nice to have] updatedAnything is set whenever updateSkillsFn returns nil. In --check mode, though, installer.UpdateSkills turns a manifest fetch failure into a warning and returns an empty result (libs/aitools/installer/update.go:105-107). Take aitools update --check with file skills installed, where the release ref resolves but the manifest fetch fails (for example, a pinned ref whose manifest host is unreachable). It finishes SUCCESS although nothing was checked, while the plugin path in the same situation already reports FAILURE. Carry the swallowed error back (for example, a field on UpdateResult) and treat it like firstPluginErr, keeping exit code 0.
There was a problem hiding this comment.
Added UpdateResult.CheckErr, set when a --check run only warns that the manifest could not be fetched. update treats it like a failed plugin (first error kept, exit code still 0), so a run where it is the only attempt finishes FAILURE with its category. The plugin check printed in the same run is work done too, so a --check run that checked recorded plugins still finishes SUCCESS. Two new cases in TestUpdateFunnelWhenPluginsFail cover both. bbc2b80
| var failures []error | ||
| removed, failures = removeSkillExposuresFromAgents(ctx, name, canonicalDir, scope, cwd, state, opts.Agents) | ||
| skillFailures = append(skillFailures, failures...) | ||
| removeCanonical = !hasRemainingSkillExposure(ctx, name, canonicalDir, scope, cwd, opts.Agents) |
There was a problem hiding this comment.
[nit] With --agents, hasRemainingSkillExposure skips every targeted agent, including one whose entry removal just failed. Say only Cursor has skill X and removing ~/.cursor/skills/X fails (for example, EACCES on a copied dir). The canonical dir is still deleted, removed becomes true, SkillsRemoved is 1, and the funnel reports SUCCESS while Cursor keeps an untracked copy. The deletion predates this PR, but the new counter should not report it as success. Either don't count the skill when a targeted entry failed, or keep the canonical dir when a targeted removal failed.
There was a problem hiding this comment.
A skill whose entry could not be removed from one of the targeted agents no longer counts in SkillsRemoved, so your Cursor example finishes FAILURE with the removal error's category. I left the canonical-dir deletion as is: it predates this PR, and changing what uninstall deletes is a separate fix. New test TestUninstallDoesNotCountSkillWithFailedTargetedRemoval (fails without the change). bbc2b80
There was a problem hiding this comment.
Still left as is in 8643a8e: deleting the canonical dir after a failed targeted removal predates this PR, and changing what uninstall --agents deletes is a behavior fix of its own, separate from the funnel. This PR only makes sure that case no longer counts as removed (it finishes FAILURE with the removal error's category).
| } | ||
| // A plugin or skill that could not be removed is only a warning. | ||
| failures := slices.Concat(result.PluginFailures, result.SkillFailures) | ||
| if len(failures) > 0 && result.PluginsRemoved == 0 && result.SkillsRemoved == 0 { |
There was a problem hiding this comment.
[nit] Consider aitools uninstall --agents cursor when only Claude Code has the skills. SkillsRemoved and PluginsRemoved are both 0 and there are no failures, so the run finishes SUCCESS; TestUninstallCountsRemovedSkills covers this state. NOTHING_TO_DO ("nothing installed to update or uninstall") seems to fit. If SUCCESS is intended, add a short comment saying so. Otherwise finish NOTHING_TO_DO when both counts are 0 and there are no failures.
There was a problem hiding this comment.
An uninstall that removed nothing and recorded no failure now finishes NOTHING_TO_DO (new funnelNothingToDo; exit code unchanged). Added a --agents=cursor case for it to TestUninstallFunnelWhenRemovalsFail. bbc2b80
| // FINISHED (outcome from the returned error). | ||
| func withCommandFunnel(command protos.AitoolsCommand, runE func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) error { | ||
| return func(cmd *cobra.Command, args []string) (err error) { | ||
| f := startFunnel(cmd.Context(), command, "") |
There was a problem hiding this comment.
[nit] withCommandFunnel never calls setScope or setAgents. So update, uninstall and list events carry neither field, even for uninstall --scope=global --agents=cursor; the install-success golden has no scope key for uninstall --scope=global. Install's events carry both, so these funnels cannot be split the same way. Add a context helper like funnelCancelled (for example, funnelSetScope(ctx, scope)) and call it once update and uninstall resolve a single scope.
There was a problem hiding this comment.
Added funnelSetScope and funnelSetAgents. uninstall sets the scope once resolved and the agents from --agents; update sets the scope when it resolved to a single one (the enum has no "both", so --scope=both reports none). The install-success golden now shows "scope":"GLOBAL" on the update and uninstall FINISHED events, and the funnel tests assert scope and agents. bbc2b80
| trace print_telemetry | ||
|
|
||
| title "list\n" | ||
| trace $CLI aitools list --scope=global > /dev/null |
There was a problem hiding this comment.
[nit] aitools list writes its table through cmdio.LogString to stderr, so this redirect discards nothing; the table is still in output.txt. Drop the redirect, or send the output to LOG.list as .agents/rules/testing.md asks for noisy output.
There was a problem hiding this comment.
Dropped the redirect. I then removed the list and version runs from this golden: they no longer wait for auth at exit (see the reply on Question 1), so whether their events upload against the mock server depends on timing. Their funnels are covered by unit tests. bbc2b80
|
|
||
| # The golden pins the error text and exit code, which stay as before. | ||
| title "update and uninstall with nothing installed: FINISHED with NOTHING_TO_DO, no error category\n" | ||
| errcode trace $CLI aitools update --scope=global |
There was a problem hiding this comment.
[nit] The update and uninstall runs on lines 27 and 29 must fail. .agents/rules/testing.md says: "Assert an expected failure with musterr, not … a bare errcode." The golden would catch an unexpected success today, so this is about following the rule. Use musterr trace … and regenerate the golden. musterr does not print Exit code: 1, so if pinning the exact exit code matters, say so in a comment and keep errcode.
There was a problem hiding this comment.
Switched both to musterr trace. The exit code is still pinned through exit_code in the uploaded events. bbc2b80
| func TestUninstallCountsRemovedSkills(t *testing.T) { | ||
| tmp := setupTestHome(t) | ||
| // Agent dirs derive from the temp HOME, not from the host's overrides. | ||
| for _, v := range []string{"CLAUDE_CONFIG_DIR", "PI_CODING_AGENT_DIR", "GEMINI_CLI_HOME", "GOOSE_PATH_ROOT", "APPDATA", "XDG_CONFIG_HOME"} { |
There was a problem hiding this comment.
[nit] The six-variable list (CLAUDE_CONFIG_DIR, PI_CODING_AGENT_DIR, …) is repeated at line 477 and nearly matches isolateAgents in cmd/aitools/funnel_test.go. When an agent adds a config-dir override, every copy needs updating, and a missed one makes the test read the host's agent dirs. Move the list into one helper next to setupTestHome; .agents/rules/testing.md says to extract values that must change together.
There was a problem hiding this comment.
Moved the list into clearAgentDirOverrides next to setupTestHome in installer_test.go; both tests use it. isolateAgents in cmd/aitools is in another package and also pins the calling agent, so it keeps its own list. bbc2b80
| require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".claude"), 0o755)) | ||
| require.NoError(t, os.MkdirAll(filepath.Join(tmp, ".cursor"), 0o755)) | ||
|
|
||
| install := func(a string) { |
There was a problem hiding this comment.
[nit] install := func(a string) here and saveCopilotPlugin := func(t *testing.T) at plugin_test.go:470 are inline closures that only name a repeated step. .agents/rules/style-guide-go.md: "Do not declare inline function closures just to name a repeated operation. Extract it as a regular named function instead." Make them named helpers that take t/ctx explicitly.
There was a problem hiding this comment.
Both are named helpers now: installSQLSkillForAgent(t, ctx, name) in uninstall_test.go and saveCopilotPlugin(t) in plugin_test.go. bbc2b80
| @@ -0,0 +1 @@ | |||
| * `databricks aitools` commands now send usage telemetry on how each run went (started, steps reached, outcome, error category), and the CLI records when it recommends installing Databricks AI tools. To upload it, `aitools update`, `list`, `version` and `uninstall` now contact the configured workspace like `aitools install` does: auth resolves in the background, and at the end of a run the command waits for it (at most 3 seconds) and then for the upload (`DATABRICKS_CLI_TELEMETRY_TIMEOUT`, default 3 seconds), so a short run can take up to about 6 seconds longer on a slow workspace and lasts at least 3 seconds while connections to the workspace are refused or time out. Set `DATABRICKS_CLI_DISABLE_TELEMETRY=1` to skip both. ([#7467](https://github.com/databricks/cli/pull/7467), [#7469](https://github.com/databricks/cli/pull/7469), [#7470](https://github.com/databricks/cli/pull/7470)) | |||
There was a problem hiding this comment.
[nit] The entry is about 900 characters, roughly twice the next-longest fragment, and explains internals: background auth resolution and two separate timeouts. The pr-checklist asks for the user-facing effect, not the implementation. Something shorter would do, for example: "databricks aitools commands now send usage telemetry; update, list, version and uninstall contact the configured workspace for it and can take up to about 6 seconds longer when it is slow or unreachable. Set DATABRICKS_CLI_DISABLE_TELEMETRY=1 to opt out." Keep the PR links.
There was a problem hiding this comment.
Shortened to the user-facing effect (about 370 characters plus the three PR links). It also states the no-wait change from Question 1: install, update and uninstall can take up to about 6 seconds longer; list, version and runs that stop early never wait. bbc2b80
8fcfd3e to
d5ef5c6
Compare
2b4b8c6 to
bbc2b80
Compare
|
Thanks for the review. Replies to the parts outside the inline threads, all in bbc2b80: Unknown-agent records (outside the diff): Question 1: All four funnels stay, but the wait now depends on how far a run gets. Question 2: I kept the installer changes in this PR. They exist only to classify these funnel outcomes and have no other caller, so a separate PR would add one more to an already long stack without making either easier to review. |
d5ef5c6 to
3b60b60
Compare
bbc2b80 to
6cd41f6
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 6cd41f6. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR adds the aitools_command_event funnel and best-effort background auth to aitools update, list, version and uninstall. It also adds installer results so that runs where every attempt only warned count as FAILURE, and runs with nothing installed count as NOTHING_TO_DO. The wiring is correct, error text and exit codes are unchanged, every fix from the previous round is in this head, and CI is green; nothing is blocking. The main open point: list and version can still wait up to 3 s for the upload once auth resolves from cached credentials, which the changelog says never happens. There are also two design questions (whether to instrument list/version, and where the installer API changes should live) and a few nits.
Questions
listandversionnow start a background auth resolution on every run and upload their events only if it finishes before the command does. The acceptance script notes that this depends on timing. How often do you expect these two commands' events to arrive in practice, and is that enough to keep instrumenting them? Options: keep them as they are; keep them with a shorter upload budget for runs that do not wait; or instrument onlyinstall,updateanduninstall.- On keeping the installer changes in this PR: one smaller variant would change
UninstallSkillsOpts's return type in place instead of replacing it withUninstallSkillsWithResult, which avoids touching about 20 test call sites. Would you take that variant, or keep the current rename? Separately, would you move the API changes into a small refactor PR under this one?
Comments outside the changed lines
- [nice to have]
cmd/aitools/telemetry.go:125:listandversionskip only the wait for auth. Once the background resolution has finished,applyputs the config on the context, and root runs the upload with the default 3 s timeout. With a PAT or a valid cached OAuth token and host metadata in the 1 h disk cache, the resolution needs no network, so it is almost always done by the timelistfinishes its manifest fetch (about 0.8 s). On a slow or blackholed workspace (for example a private-link host with the VPN down),aitools listthen exits up to 3 s later; before this PR it made no workspace request at all. That contradicts the changelog ("list,versionand runs that stop early never wait for it") and the Merge Danger note that their events are dropped on a slow or unreachable workspace. Give runs that never calledwaitForTelemetryAutha short upload budget (hintOnlyUploadTimeout= 1 s inlibs/telemetry/logger.gois the precedent), or say in the changelog and Merge Danger that these commands can still take up to 3 s longer.
Agreeing with existing threads
- [nit] Targeted uninstall still deletes the shared skill dir after a failed entry removal: #7470 (comment)
| // From here the run removes files and runs agent CLIs, so it waits for | ||
| // the telemetry auth; the flag errors and the declined confirm above | ||
| // do not. | ||
| waitForTelemetryAuth(ctx) |
There was a problem hiding this comment.
[nit] With nothing installed, aitools uninstall (and update --scope=global) reach waitForTelemetryAuth before the installer returns ErrNothingInstalled, so on a slow or unreachable workspace they take up to 3 s to print "no skills or plugins installed". --scope=project gets the same NOTHING_TO_DO outcome from withExplicitScopeCheck before the wait and exits at once. For uninstall, confirmUninstall/detectInstalledScopes already know the scope has no state. Skip the wait when the scope has no state, or say in the description and README that this early exit is meant to wait. Trade-off: those events would then upload only if auth had already resolved, and the workspace-unreachable golden would need a seeded state.
There was a problem hiding this comment.
update and uninstall now call waitForTelemetryAuth only once the scope they act on has state (update checks each scope's LoadState in its loop, and uninstall loads the state once, and confirmUninstall now takes that state instead of loading it again). With nothing installed (or only a legacy install) they exit at once and upload only if auth has already resolved. I seeded state in the workspace-unreachable golden by running install first, so update and uninstall still wait there. The nothing-installed section of install-success now depends on timing, so I removed it; TestNothingInstalledFinishesNothingToDo pins those events and errors. New cases in TestInstrumentedCommandsDoNotWaitForTelemetryAuth fail if the run always waits. 8643a8e
| # aitools list uploads its telemetry events at exit. | ||
| [[Server]] | ||
| Pattern = "POST /telemetry-ext" | ||
| Response.Body = '{"errors": [], "numProtoSuccess": 100}' |
There was a problem hiding this comment.
[nit] libs/testserver/handlers.go:612 already models POST /telemetry-ext (it accepts every log and returns numProtoSuccess = len(protoLogs)), and the sibling acceptance/cmd/aitools/telemetry tests rely on it. This stub replaces it with a static numProtoSuccess: 100. .agents/rules/testing.md says to reserve [[Server]] for routes the testserver does not model. Drop the stub; the goldens do not depend on it, since print_requests.py filters /telemetry-ext out.
There was a problem hiding this comment.
Dropped the stub; the list tests now use the testserver's POST /telemetry-ext handler. The goldens are unchanged. 8643a8e
| // act on" apart from a failure. | ||
| var ErrNothingInstalled = errors.New("no skills or plugins installed") | ||
|
|
||
| // SkillError reports that a skill named via --skills could not be resolved from |
There was a problem hiding this comment.
[nit] uninstall --skills foo now returns SkillError{Reason: ReasonSkillNotFound} for a skill missing from the install state. The type comment ("could not be resolved from the manifest") and ReasonSkillNotFound ("absent from the resolved manifest", line 26) still describe only the manifest case, so a reader would take SKILL_NOT_FOUND to always mean a manifest problem. Update both comments to cover "not installed".
There was a problem hiding this comment.
Updated both comments: SkillError now covers a skill "not resolved from the manifest or, for uninstall, not installed", and ReasonSkillNotFound covers "absent from the resolved manifest or, for uninstall, from the install state". 8643a8e
| func TestInstrumentedCommandsResolveAuth(t *testing.T) { | ||
| for _, cmd := range []*cobra.Command{NewInstallCmd(), NewUpdateCmd(), NewListCmd(), NewVersionCmd(), NewUninstallCmd()} { | ||
| assert.NotNil(t, cmd.PreRunE, cmd.Name()) | ||
| } | ||
| } |
There was a problem hiding this comment.
[nit] Any PreRunE passes this test. If a later change replaced withTelemetryAuth's hook on list or version, their events would stop uploading and no test would fail; TestUpdateAndUninstallWorkWaitsForTelemetryAuth covers only update and uninstall. Run each command with a recording resolver (setTelemetryAuth) and assert the resolver was called.
There was a problem hiding this comment.
TestInstrumentedCommandsResolveAuth now runs install, update, list, version and uninstall with a recording resolver (setTelemetryAuth) and fails if the resolver never runs. Each run ends early (a flag error, or version itself), because the resolution starts in PreRunE before RunE. With PreRunE cleared in instrumentCommand, it fails for update, list, version and uninstall. 8643a8e
| // funnelCancelled records a run the user cancelled at a prompt while the | ||
| // command still returns nil (e.g. answering "no" to uninstall's confirm), so | ||
| // withCommandFunnel does not report it as SUCCESS. No-op outside a funnel. | ||
| func funnelCancelled(ctx context.Context) { |
There was a problem hiding this comment.
[nit] funnelCancelled, funnelAllAttemptsFailed, funnelNothingToDo, funnelSetScope and funnelSetAgents each repeat the same ctx.Value(funnelKey{}) assertion plus a no-op branch. Every caller runs inside instrumentCommand, and only the NotPanics test (funnel_test.go:500) uses the fallback. AGENTS.md asks to avoid speculative fallbacks. Replace them with one funnelFrom(ctx) *commandFunnel accessor and call the existing methods (finishWith, setScope, setAgents) on it.
There was a problem hiding this comment.
Replaced the five helpers with one funnelFrom(ctx) *commandFunnel accessor. Callers use finishWith, setScope and setAgents on it directly, and uninstall builds the agentPlanItems in the loop that already collects the agent names. The no-op fallback and its NotPanics test are gone. 8643a8e
| // We can't tear down an agent we don't know; keep the record (and | ||
| // the state file) rather than silently dropping a live plugin. | ||
| log.Warnf(ctx, "Leaving plugin record for unknown agent %q; remove its plugin manually", name) | ||
| pluginFailures = append(pluginFailures, fmt.Errorf("unknown agent %q in state", name)) |
There was a problem hiding this comment.
[nit] A plugin record for an agent this CLI does not know (for example one written by a newer CLI) is recorded as a plain fmt.Errorf("unknown agent %q in state"). A run whose only record is that one finishes FAILURE:UNCATEGORIZED, although UNKNOWN_AGENT exists. The same literal is repeated in UpdateInstalledPlugins (update.go:460). Add one typed or sentinel installer error for it, and map it to UNKNOWN_AGENT in classifyInstallError.
There was a problem hiding this comment.
Added installer.ErrUnknownAgent, which both UninstallSkillsWithResult and UpdateInstalledPlugins use through one unknownAgentInStateError(name) (same text: unknown agent "<name>" in state). classifyInstallError maps it to UNKNOWN_AGENT. It has a TestClassifyInstallError case and ErrorIs assertions in TestPluginOperationsReportUnknownAgent. 8643a8e
| } | ||
| // The scope enum has no "both"; a run over both scopes reports none. | ||
| if len(scopes) == 1 { | ||
| funnelSetScope(ctx, scopes[0]) |
There was a problem hiding this comment.
[nit] funnelSetScope runs only after resolveScopeForUpdate/resolveScopeForUninstall succeed. update --scope=project with no project install returns nothingInstalledInScopeError first, so its FINISHED NOTHING_TO_DO has no scope, while update --scope=global in the same situation reports GLOBAL. Splitting nothing-to-do runs by scope therefore undercounts project. list never sets a scope even when --scope picks one. Set the scope from an explicit --scope/--project/--global before resolving, and in list once it picks one.
There was a problem hiding this comment.
update and uninstall now record a single scope chosen by --scope/--project/--global right after parseScopeFlag, before resolving, and then the resolved scope. So update --scope=project with no project install reports PROJECT (asserted in TestNothingInstalledFinishesNothingToDo). list records the scope it picks. The single-scope switch that list had is now explicitScope(projectFlag, globalFlag), which all three commands share; TestListFunnelFailure asserts GLOBAL for --scope=global. 8643a8e
| // runWithBlockedTelemetryAuth runs cmd with args, with telemetry enabled and | ||
| // its events buffered, while the auth resolution is blocked, and returns its error, how long it took and the context it ended | ||
| // with. |
There was a problem hiding this comment.
[nit] The runWithBlockedTelemetryAuth comment has a line about 130 characters long followed by a one-word line ("with."). Rewrap it to match the surrounding comments.
There was a problem hiding this comment.
Rewrapped. The helper is now runWithTelemetry, since the resolve-auth test also uses it without a blocked resolver. 8643a8e
6cd41f6 to
8643a8e
Compare
|
Replies to the review body, in 8643a8e: Outside the diff ( Question 1: I'm keeping Question 2: I'm keeping the current shape ( |
3b60b60 to
54514a6
Compare
8643a8e to
4653c00
Compare
54514a6 to
f6a8dd1
Compare
4653c00 to
cf15bb3
Compare
f6a8dd1 to
8ac74ab
Compare
9dac556 to
e5b0edd
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 e5b0edd. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR extends the aitools_command_event funnel and best-effort background auth from install to update, list, version and uninstall. It also adds installer results so that runs that only warned are reported as FAILURE and runs with nothing installed as NOTHING_TO_DO. Every fix from the earlier rounds is in this head, CI is green, and nothing is blocking. The main open point is that runs with no single scope send "scope":"TYPE_UNSPECIFIED" on FINISHED instead of omitting it; the rest are small outcome-classification and comment nits, and the two earlier design questions are restated for a final answer.
Questions
aitools listandaitools versionnow start a background workspace auth resolution on every run. Before this PR neither contacted the workspace. Their events upload only when auth has already resolved, and in that case exit can take up to 3 s longer on a slow workspace. Your earlier replies keep both instrumented. Is that final for this PR, or would you drop the funnel fromversion, whose FINISHED event is almost always SUCCESS, or from both?- Part of this diff changes the installer API:
UninstallSkillsWithResultreplacesUninstallSkills/UninstallSkillsOpts, and the PR addsPluginUpdate.Err,UpdateResult.CheckErr, theremoveAllFnseam and a sorted full-uninstall order. All of these exist to classify funnel outcomes. Your earlier replies keep them here under the new name. Is that final, or would you move them into a small PR below this one?
Comments outside the changed lines
- [nit]
libs/telemetry/protos/aitools_events.go:146: The struct comment now covers all five commands, butAgentsandScopestill say "Install only", andScopesays it is set "after the scope is resolved".uninstallnow setsAgentsfrom--agents.update,listanduninstallsetScope, andupdateanduninstallset it from the flag before resolving. Update both field comments. - [nit]
libs/aitools/installer/plugin_test.go:244:UninstallSkillsOptsis gone, butTestUninstallSkillsOptsTearsDownPlugin(here),TestUninstallSkillsOptsTargetsPluginAgents(line 275) andTestUninstallSkillsOptsUsesRecordedClaudeScope(line 577) still carry its name. A search for the function therefore finds tests of something else. Rename them afterUninstallSkillsWithResult, or drop the function name. - [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary,## Evidenceand## Merge Danger..agents/skills/pr-checklist/SKILL.mdasks for the template's## Changes,## Whyand## Testsheadings, in that order, with no new sections; most merged PRs here, including #7442, use them. Move the content under those headings; the Merge Danger notes fit under## Why, and the evidence under## Tests.
Agreeing with existing threads
- [nit] Targeted uninstall still deletes the canonical dir after a failed entry removal: #7470 (comment)
| scope = installer.ScopeGlobal | ||
| } | ||
| scope := explicitScope(projectFlag, globalFlag) | ||
| funnelFrom(cmd.Context()).setScope(scope) |
There was a problem hiding this comment.
[nice to have] setScope("") stores scopeType(""), which is the non-empty sentinel TYPE_UNSPECIFIED, so omitempty keeps it. A plain aitools list or list --scope=both, an update over both scopes, and update/uninstall runs that fail before a scope resolves (update.go:74, uninstall.go:38) all send "scope":"TYPE_UNSPECIFIED" on FINISHED, while STARTED in the same run has no scope. Running the head with no flags gives {"command":"LIST","stage":"FINISHED","outcome":"SUCCESS","interactive":false,"scope":"TYPE_UNSPECIFIED"}. That contradicts the comment at update.go:80 ("reports none") and the way finishWith drops the matching error-category sentinel. Leave f.scope empty when scope == "" in setScope (telemetry.go:330), and add a plain-list case to the funnel tests.
There was a problem hiding this comment.
setScope("") now leaves the field unset, so plain list, list --scope=both, an update over both scopes and the runs that fail before a scope resolves omit scope on FINISHED, as STARTED does. TestListFunnelFailure now covers --scope=global (GLOBAL), no flag and --scope=both (no scope), and fails without the change. e1c95af
| removeSymlinksFromAgents(ctx, name, canonicalDir, scope, cwd) | ||
| } | ||
| if removeCanonical { | ||
| // RemoveAll succeeds on a missing dir; only an existing one counts. |
There was a problem hiding this comment.
[nit] On a full uninstall, a skill counts in SkillsRemoved only if its canonical dir existed before removeAllFn. Take a state that lists skills whose dirs are already gone (the TestUninstallHandlesMissingDirectories setup). The run removes their agent links and the state file and prints Uninstalled 2 skills., but aitools uninstall finishes NOTHING_TO_DO, because both counts are 0 and there are no failures. In the non-targeted path, count the skill whenever removeAllFn succeeds, since its state record is dropped either way.
There was a problem hiding this comment.
A full uninstall now counts a skill whenever removeAllFn succeeds, since its state record is dropped either way. With --agents only an existing canonical dir or entry counts, as before. The stale-state case in TestUninstallCountsRemovedSkills now expects 1. e1c95af
| // confirm above do not, nor does a run that only reports that nothing | ||
| // (or a legacy install) is there. | ||
| if state != nil { | ||
| waitForTelemetryAuth(ctx) |
There was a problem hiding this comment.
[nit] waitForTelemetryAuth is armed once the scope has state, before UninstallSkillsWithResult checks --skills against that state. So aitools uninstall --skills=typo with something installed waits for the auth budget (up to 3 s, plus up to 3 s for the upload if auth resolves late) before it prints skill "typo" is not installed. With a blocked resolver and a 2 s budget, the head takes 2.0 s. libs/telemetry/protos/README.md says runs that end at a flag error do not wait. Check that every opts.Skills name is in state.Skills before calling waitForTelemetryAuth; the installer still returns the error itself.
There was a problem hiding this comment.
uninstall now calls waitForTelemetryAuth only when every --skills name is in the loaded state (new skillsInstalled). The installer still returns the skill "typo" is not installed error itself. New case in TestInstrumentedCommandsDoNotWaitForTelemetryAuth with one skill recorded and --skills=typo; it fails without the check. e1c95af
| if state == nil { | ||
| if scope == ScopeGlobal && hasLegacyInstall(ctx, baseDir) { | ||
| return errors.New("found skills from a previous install without state tracking; run 'databricks aitools install' first, then uninstall") | ||
| return nil, errors.New("found skills from a previous install without state tracking; run 'databricks aitools install' first, then uninstall") |
There was a problem hiding this comment.
[nit] On a pre-state (legacy) install, update and uninstall return a plain errors.New here and at update.go:87. Their FINISHED events are therefore FAILURE/UNCATEGORIZED, which is the bucket for failures the CLI has not classified. This is a known, deterministic outcome that the new funnels will see often during migration, while the sibling no-state case got ErrNothingInstalled. Tag both errors with a category, for example withCategory(…, STATE_INVALID) (no code uses that category yet) or a sentinel the command layer maps.
There was a problem hiding this comment.
Added installer.ErrLegacyInstall, which both errors wrap (fmt.Errorf("%w; run ..."), same text). classifyInstallError maps it to STATE_INVALID, since the install state these commands need is missing. Covered by a TestClassifyInstallError case and EqualError + ErrorIs in TestUpdateLegacyInstallDetected and a new TestUninstallLegacyInstallDetected. e1c95af
| // resolution within telemetryAuthBudget, as install does past its plan | ||
| // confirmation. No-op outside an instrumented command. | ||
| func waitForTelemetryAuth(ctx context.Context) { | ||
| if auth, ok := ctx.Value(telemetryAuthKey{}).(*telemetryAuth); ok { |
There was a problem hiding this comment.
[nit] Both callers (update.go:116, uninstall.go:100) run inside instrumentCommand and have already called funnelFrom(ctx), which panics outside it, so the !ok path never runs. The last round removed the same no-op fallback from the funnel helpers, per AGENTS.md ("Avoid speculative fallbacks and default values"). Assert the type the way funnelFrom does, and drop "No-op outside an instrumented command" from the doc comment.
There was a problem hiding this comment.
waitForTelemetryAuth now asserts the type directly, like funnelFrom, and its doc says only an instrumented command's RunE calls it. e1c95af
|
|
||
| // SkillError reports that a skill named via --skills could not be resolved from | ||
| // the manifest. Reason drives telemetry categorization (the command layer maps | ||
| // the manifest or, for uninstall, is not installed. Reason drives telemetry categorization (the command layer maps |
There was a problem hiding this comment.
[nit] The inserted text leaves this line at 115 characters in the middle of the paragraph. Rewrap the SkillError doc comment to the width of the lines around it.
There was a problem hiding this comment.
Rewrapped the SkillError doc comment to the width of the surrounding lines. e1c95af
e5b0edd to
e1c95af
Compare
8ac74ab to
a8819ec
Compare
|
Replies to the review body, in e1c95af: Question 1: Final for this PR: Question 2: Final: the installer changes stay in this PR under the current names. Outside the diff:
|
a8819ec to
9f73e89
Compare
e1c95af to
1a67570
Compare
9f73e89 to
2241742
Compare
`aitools update`, `list`, `version` and `uninstall` now log the same `aitools_command_event` funnel as `aitools install` (STARTED, then FINISHED with the outcome and error category) and resolve auth best-effort in the background so the events can upload at exit. `list` and `version` never wait for that resolution at exit, and neither do `update` and `uninstall` runs that end before their work (flag and scope errors, a declined confirm, nothing installed, a `--skills` name that is not installed), as for `install` before its plan is confirmed; their events upload only if auth has already resolved.
```diff
NewUpdateCmd / NewListCmd / NewVersionCmd / NewUninstallCmd
- return cmd
+ return instrumentCommand(cmd, protos.AitoolsCommand<X>)
+ cmd.RunE = withCommandFunnel(...) // STARTED; FINISHED from RunE's error
+ withTelemetryAuth(cmd) // background auth; no wait at exit unless
+ // RunE calls waitForTelemetryAuth (update and
+ // uninstall, once a scope has state)
```
```text
finish(err)
nil -> SUCCESS
errors.Is(err, ErrNothingInstalled) -> NOTHING_TO_DO (still exits 1 with the same text)
prompt aborted / no agents picked -> CANCELLED
otherwise -> FAILURE, classifyInstallError(err)
(scope flag errors tagged INVALID_FLAGS, as in install)
RunE returns nil, but
uninstall confirm answered "no" -> CANCELLED
uninstall removed nothing and nothing failed -> NOTHING_TO_DO
every plugin/skill update or removal attempted -> FAILURE with the first error's category
failed and nothing else succeeded (still exits 0)
(via funnelFrom(ctx).finishWith)
```
Installer additions: `ErrNothingInstalled` (same error text as before), `PluginUpdate.Err` (failed plugins are returned after the updated ones; `update` prints only the updated ones), `UpdateResult.CheckErr` (the manifest fetch error a `--check` run only warns about), and `UninstallSkillsWithResult` returning `UninstallResult{PluginsRemoved, SkillsRemoved, PluginFailures, SkillFailures}`, which replaces `UninstallSkills` and `UninstallSkillsOpts`. A plugin record for an agent this CLI does not know counts as a failed update or removal (`ErrUnknownAgent`, UNKNOWN_AGENT), and a pre-state (legacy) install returns `ErrLegacyInstall` (same text, STATE_INVALID). `uninstall --skills` with a skill that is not installed returns a `SkillError` (same text). Full uninstall now removes skills in sorted order.
Changelog: `.nextchanges/cli/aitools-telemetry-funnel.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
1a67570 to
f379423
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at f379423, 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 (e5b0edd → f379423) address its comments.
Stacked on #7469.
Stacked on: aitools: add install funnel telemetry
Summary
aitools update,list,versionanduninstallnow log the sameaitools_command_eventfunnel asaitools install(STARTED, then FINISHED with the outcome and error category) and resolve auth best-effort in the background so the events can upload at exit.listandversionnever wait for that resolution at exit, and neither doupdateanduninstallruns that end before their work (flag and scope errors, a declined confirm, nothing installed in the scope, a--skillsname that is not installed), the same rule #7469 applies toinstallbefore its plan is confirmed; their events upload only if auth has already resolved.Installer additions:
ErrNothingInstalled(same error text as before),PluginUpdate.Err(failed plugins are returned after the updated ones;updateprints only the updated ones), andUpdateResult.CheckErr(the manifest fetch error a--checkrun only warns about), andUninstallSkillsWithResultreturningUninstallResult{PluginsRemoved, SkillsRemoved, PluginFailures, SkillFailures}, which replacesUninstallSkillsandUninstallSkillsOpts. A plugin record for an agent this CLI does not know counts as a failed update or removal (ErrUnknownAgent, UNKNOWN_AGENT), and a pre-state (legacy) install returnsErrLegacyInstall(same text, STATE_INVALID).uninstall --skillswith a skill that is not installed returns aSkillError(same text). Full uninstall now removes skills in sorted order.Changelog:
.nextchanges/cli/aitools-telemetry-funnel.md.Evidence
return instrumentCommand(cmd, protos.AitoolsCommandList)inlist.goreverted toreturn cmd):instrumentCommandforced to wait,TestInstrumentedCommandsDoNotWaitForTelemetryAuthfails for version, list, the update/uninstall flag errors and update/uninstall with nothing installed (10 s each, budget set to 10 s); withwaitForTelemetryAutha no-op,TestUpdateAndUninstallWorkWaitsForTelemetryAuthfails for both commands. WithPreRunEcleared ininstrumentCommand,TestInstrumentedCommandsResolveAuthfails for update, list, version and uninstall.DATABRICKS_HOST=https://127.0.0.1:1(refused):aitools version18 ms,update --scope=bogus17 ms,updateanduninstallwith nothing installed 17-18 ms,uninstallwith one skill recorded 3.0 s.cmd/aitools/telemetry/update-uninstallpins the uploaded STARTED/FINISHED events ofupdateanduninstallafter an install.list,version, andupdate/uninstallwith nothing installed upload only if auth has already resolved, which depends on timing, so their events are pinned by unit tests instead.workspace-unreachablechecks that the background resolution prints nothing on install, update and uninstall runs that wait for it (with the handler's level clamp removed, it shows the hostmetadata warning).go build ./...,./task test-exp-aitools,cmd/aitools,libs/aitoolsandlibs/telemetryunit tests, thecmd/aitoolsacceptance tests,./task lint-q,./task fmt-q,./task checks, andgo vetfor windows and darwin: all pass.Merge Danger
Door: two-way
Blast Radius: latency
updateanduninstallruns on a scope with something installed now contact the configured workspace: up to about 6 s longer on a slow workspace (3 s auth wait + 3 s upload), at least 3 s per run while connections are refused or time out.list,version, andupdate/uninstallruns that end at a flag or scope error, a declined confirm, with nothing installed or at a--skillsname that is not installed never wait for auth. When the resolution has already finished (e.g. a PAT or a valid cached token, with host metadata cached), they still upload and can take up to 3 s longer (the upload timeout) on a slow workspace; otherwise their events are dropped.install.This pull request and its description were written by Isaac.