Repository navigation
aitools: add install funnel telemetry - #7469
Conversation
0d29efd to
a15150a
Compare
5637f13 to
21d6b41
Compare
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 2 tests: regression=2 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (e11d32c) ❌ | latest (v1.26.0) ❌main (e11d32c): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (e11d32c) ❌ | latest (v1.26.0) ❌main (e11d32c): latest (v1.26.0): |
✅ Integration test reportCommit: 5a3cc25
Top 19 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 21d6b41. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 open design question.
This PR adds an aitools_command_event funnel to every aitools install run (STARTED, the steps reached, then FINISHED with an outcome and error category) and tags failures with a category where they happen. I traced every way the command can exit and found exactly one FINISHED event each time; the fields and enum values match the Universe proto, and CI is green. Nothing blocking. The main suggestion is to report "no skills delivered" the same way in --path mode and the other modes; the rest are nits on outcome semantics, small simplifications and test conventions. There is also one question about the extra upload latency for runs that stop early.
Questions
- Runs that stop early (a flag error, a cancelled prompt, no agent detected, a
--pathdump) used to upload no telemetry. With the funnel, each of them now makes an upload, so against a slow but reachable workspace a run that fails instantly can wait up to the upload timeout (3 s by default) on top of the auth wait. The Merge Danger section mentions this. Is that latency acceptable for these runs? Or would you rather use a shorter upload timeout for runs that end before PLAN_CONFIRMED, or upload their events only when auth has already resolved without waiting?
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.mdasks PR bodies to "Follow.github/PULL_REQUEST_TEMPLATE.mdexactly", using## Changes,## Whyand## Testsand no invented sections such as## Summary. It also asks for a concise summary. This description uses## Summary/## Evidence/## Merge Danger. Map the content onto Changes / Why / Tests and trim it; the flow diagram and the "not covered" list could stay, shortened, under those headings.
| for _, o := range outcomes { | ||
| if o.status == outcomeInstalled { | ||
| return "", false | ||
| } | ||
| } |
There was a problem hiding this comment.
[nice to have] When every skill in the manifest is filtered out (e.g. only experimental skills, no --experimental), the outcome depends on the mode. A --path dump finishes FAILURE / UNCATEGORIZED (install.go:159). --skills-only --agents=cursor (or the default mode with a skills agent) finishes SKILLS_INSTALLED plus SUCCESS. That happens because InstallSkillsForAgents returns nil, executePlan marks the agents outcomeInstalled, and this check lets it through. The description lists this as a known gap. But the funnel now reports the same "nothing delivered" condition as both a failure and a success, and the --path side files a known, non-error cause under UNCATEGORIZED, the bucket meant to show what the classifier misses. Pick one outcome for both modes. Either carry the resolved skill count from the installer into the outcomes and report the zero case the way --path does, or, until per-agent delivery is reported, make --path match the other modes. In both cases use a specific outcome (e.g. NOTHING_TO_DO) instead of FAILURE / UNCATEGORIZED.
There was a problem hiding this comment.
Done in 61fb77f, partly. A --path dump that writes no skill now finishes NOTHING_TO_DO instead of FAILURE / UNCATEGORIZED (TestInstallFunnelPathDumpWithNoSkillToWrite). The other modes still can't see that case: InstallSkillsForAgents returns only an error. Carrying the count would change its signature and the installSkillsForAgentsFn seam that the PRs stacked above mock, so it's not in this PR. The description lists it under "Not covered".
There was a problem hiding this comment.
Still only the --path part, as of 54514a6. Reporting zero skills for agent installs needs InstallSkillsForAgents to return how many skills it resolved. That changes the signature of its installSkillsForAgentsFn seam, and the PRs stacked above this one add more mocks of that seam, so the change isn't contained to this PR. For now it stays listed under "Not covered" in the description: an agent install where every skill is filtered out still finishes SUCCESS.
There was a problem hiding this comment.
Our review found this too. Note: #7481 fixes it later in the stack. There, InstallSkillsForAgents returns ErrNoSkillsResolved when no skill is left (libs/aitools/installer/installer.go:398), and finish() maps it to NOTHING_TO_DO. So the "Not covered" item in this description is resolved once #7481 lands.
There was a problem hiding this comment.
Thanks, confirmed: in #7481 InstallSkillsForAgents returns installer.ErrNoSkillsResolved and finish() maps it to NOTHING_TO_DO. I updated the "Not covered" item in this description to point there.
| if _, err := resolveScope(projectFlag, globalFlag); err != nil { | ||
| return withCategory(err, protos.AitoolsErrorCategoryInvalidFlags) | ||
| } | ||
| funnel.step(ctx, protos.AitoolsCommandStepFlagsValidated) |
There was a problem hiding this comment.
[nit] funnel.step(FLAGS_VALIDATED) runs before resolveAgentNames (line 206). So --agents , or --agents bogus logs FLAGS_VALIDATED and SCOPE_RESOLVED, then FINISHED FAILURE INVALID_FLAGS / UNKNOWN_AGENT. The step event says the flags were valid, and the final category says they were not. resolveAgentNames does not depend on the scope, so you could validate --agents before this step. Calling it there would also fail an invalid name before the interactive scope prompt instead of after it. If you'd rather keep the prompt order unchanged in this PR, note on AitoolsCommandStepFlagsValidated that --agents is checked after SCOPE_RESOLVED.
There was a problem hiding this comment.
Done in 61fb77f. resolveAgentNames now runs before FLAGS_VALIDATED, so --agents , / --agents bogus finish STARTED -> FAILURE INVALID_FLAGS / UNKNOWN_AGENT with no step events, and fail before the scope prompt. The description notes the order change.
| return "", false | ||
| } | ||
| } | ||
| return firstFailureCategory(outcomes), true |
There was a problem hiding this comment.
[nit] Suppose every detected agent is skipped for the chosen scope, e.g. --skills-only --scope=project with only Cursor detected. On the interactive path, agentChoices drops the skipped agents, selectAgents returns none, and the run finishes NOTHING_TO_DO (install.go:335). The non-interactive path (e.g. CI) keeps the skipped agents in the plan, so this function reports FAILURE / UNSUPPORTED_SCOPE. Both runs exit 0, but the funnel splits one situation into two outcomes depending on the TTY. If that is unintended, report an all-skipped plan with no named agents as NOTHING_TO_DO, or document the split.
There was a problem hiding this comment.
Documented in 61fb77f on nothingInstalledCategory and in the description. Keeping FAILURE / UNSUPPORTED_SCOPE for the non-interactive run is deliberate: it acted on the detected agents and set up none of them. The interactive picker never offers a skipped agent, so that run has nothing to choose and finishes NOTHING_TO_DO before the plan. #7481, further up this stack, makes the non-interactive run exit 1, so its outcome will match its exit code.
| case deliverySkills: | ||
| skills = skills || o.status == outcomeInstalled | ||
| case deliveryPlugin: | ||
| plugins = plugins || o.status == outcomeInstalled |
There was a problem hiding this comment.
[nit] The CLI and the Universe proto now describe three values differently. PLUGINS_INSTALLED: the proto says "The plugin install loop ran (individual agents may still have failed)", but deliverySteps emits it only when at least one plugin install succeeded. FAILURE: the proto says "The command returned an error", but the CLI also emits it for exit-0 runs (a --path dump that writes nothing, a detected agent whose plugin install failed). CANCELLED: the proto says "The user declined the confirm prompt or aborted a prompt", but the CLI also uses it for an empty picker selection, which exits 1. Anyone building dashboards from the proto will misread these runs. Update the Universe enum.proto comments to the CLI's semantics (which the Go comments in aitools_events.go already state), or change the CLI to match.
There was a problem hiding this comment.
The Go comments in aitools_events.go describe the CLI's behavior; I'll align the schema descriptions separately.
| scope, err := resolveScopeWithPrompt(ctx, projectFlag, globalFlag) | ||
| if err != nil { | ||
| return err | ||
| return promptError(err) |
There was a problem hiding this comment.
[nit] promptError wraps everything resolveScopeWithPrompt returns. But defaultPromptScopeSelection calls env.UserHomeDir and os.Getwd before any prompt opens. An interactive run in a deleted working directory therefore finishes FAILURE / PROMPT_FAILED instead of FILESYSTEM. Tag only the error from the huh ... Run() call (inside defaultPromptScopeSelection, and likewise for the picker and confirm prompts), so the earlier errors keep their stdlib classification.
There was a problem hiding this comment.
Done in 61fb77f. promptError now wraps only the huh ... Run() error inside defaultPromptScopeSelection, defaultPromptAgentSelection and defaultPromptProceed. The call sites return errors as they are, so an env.UserHomeDir / os.Getwd failure keeps its stdlib category (FILESYSTEM for the deleted cwd).
| // setScope records the resolved scope (installer.ScopeGlobal/ScopeProject). | ||
| func (f *commandFunnel) setScope(scope string) { f.scope = scopeType(scope) } | ||
|
|
||
| // setAgents records the targeted agents, reported on the FINISHED event. |
There was a problem hiding this comment.
[nit] setAgents wraps each agent in an agentPlanItem only so it can call agentsField. buildPlan has no side effects and runs on the next line of install.go. Build the plan first and record agentsField(plan) (e.g. funnel.setAgents(plan) taking []agentPlanItem). That drops the extra loop, and the funnel and aitools_install_event then derive agents from the same value.
There was a problem hiding this comment.
Done in 61fb77f. The plan is built first, then funnel.setAgents(plan) records agentsField(plan), the same value aitools_install_event uses.
| } | ||
| // --project with --global is a flag error; check it here so it is not | ||
| // reported as a scope prompt failure below. | ||
| if _, err := resolveScope(projectFlag, globalFlag); err != nil { |
There was a problem hiding this comment.
[nit] This resolveScope(projectFlag, globalFlag) pre-check throws its result away, and resolveScopeWithPrompt runs the same check again two lines later. That leaves its conflict error unreachable from install. Keep this tagged check and use its scope when a flag is set, calling the prompt only when neither flag is set. Then the validation and the tagging happen once.
There was a problem hiding this comment.
Done in 61fb77f. Install calls resolveScope once, only when a scope flag is set, tags its error INVALID_FLAGS and uses its scope. resolveScopeWithPrompt(ctx) no longer takes the flags: it prompts when interactive and defaults to global otherwise. Install was its only caller.
| errcode $CLI aitools install --skills-only --path out | ||
| trace print_telemetry | ||
|
|
||
| title "unknown agent fails after the scope step\n" | ||
| errcode $CLI aitools install --skills-only --scope=global --agents=no-such-agent | ||
| trace print_telemetry | ||
|
|
||
| title "a skill missing from the manifest fails after the plan is confirmed\n" | ||
| errcode $CLI aitools install --skills-only --scope=global --agents=cursor --skills=nonexistent | ||
| trace print_telemetry | ||
|
|
||
| title "a manifest the server does not have is MANIFEST_NOT_FOUND\n" | ||
| errcode env DATABRICKS_SKILLS_REF=missing-ref $CLI aitools install --skills-only --scope=global --agents=cursor | ||
| trace print_telemetry | ||
|
|
||
| title "any other HTTP error status from the manifest fetch is not classified yet (the error is untyped)\n" | ||
| errcode env DATABRICKS_SKILLS_REF=broken-ref $CLI aitools install --skills-only --scope=global --agents=cursor | ||
| trace print_telemetry | ||
|
|
||
| title "a named agent skipped for the scope takes its category from the agent's outcome\n" | ||
| errcode $CLI aitools install --skills-only --scope=project --agents=cursor |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md:150 says: "Assert an expected failure with musterr, not ! cmd or a bare errcode". errcode is for tolerated failures. All six failure cases in this script must fail. Also, without trace (testing.md:148), output.txt shows each Error: ... with no >>> line naming the command. Use musterr trace $CLI ... for each case, as other scripts in the repo do.
There was a problem hiding this comment.
Done in 61fb77f. Each case is now musterr trace $CLI .... The cases that end before PLAN_CONFIRMED (invalid flags, unknown agent, empty --path dump) are no longer in this script: those runs now upload only if auth has already resolved, so whether their events appear depends on timing. TestInstallFunnelEarlyExits and TestInstallFunnelPathDumpWithNoSkillToWrite cover them.
| func isolateAgents(t *testing.T, kv ...string) { | ||
| t.Helper() | ||
| testutil.SetAgentEnv(t, kv...) | ||
| for _, v := range []string{"CLAUDE_CONFIG_DIR", "PI_CODING_AGENT_DIR", "GEMINI_CLI_HOME", "GOOSE_PATH_ROOT", "APPDATA"} { |
There was a problem hiding this comment.
[nit] testutil.SetAgentEnv already clears the whole environment (NullEnvironment calls os.Clearenv). So this loop that blanks CLAUDE_CONFIG_DIR, GEMINI_CLI_HOME, APPDATA etc. does nothing, and the helper's comment suggests the loop is what isolates the test. Drop the loop and keep SetAgentEnv. Similarly, runInstall (line 73) takes t but never uses it. Remove the parameter, or call t.Helper().
There was a problem hiding this comment.
Done in 61fb77f. Dropped the loop and fixed the comment. isolateAgents now only calls testutil.SetAgentEnv; I kept it as a thin wrapper because tests in the PRs stacked above call it. runInstall now calls t.Helper() (kept the parameter for the same reason).
| assert.Equal(t, protos.AitoolsInstallModeSkillsOnly, e.InstallMode) | ||
| assert.False(t, e.Interactive) | ||
| } | ||
| finished := events[len(events)-1] |
There was a problem hiding this comment.
[nit] events[len(events)-1] here, and events[0] at lines 274 and 331, follow only an assert on the event sequence. If the funnel emits nothing, the test panics with index out of range instead of failing with the diff. .agents/rules/testing.md says to "Use require for preconditions that would make the rest of the test meaningless on failure", so switch those shape checks to require.Equal.
There was a problem hiding this comment.
Done in 61fb77f. The shape checks that come before events[len(events)-1] / events[0] now use require.Equal.
a15150a to
86362f1
Compare
3147276 to
920dbc0
Compare
21d6b41 to
61fb77f
Compare
920dbc0 to
95aeec8
Compare
|
Thanks for the review. Every inline comment except one has a reply in its thread. Question 1 (upload latency for runs that stop early): changed in 61fb77f. A run that ends before PLAN_CONFIRMED (a flag error, a cancelled prompt, no agent detected, a
PR template nit: I'd rather keep the current body format. Every PR in this stack uses the same layout, and restructuring all of them would be churn. Happy to change it if you feel strongly. |
61fb77f to
8fcfd3e
Compare
95aeec8 to
e503a62
Compare
d5ef5c6 to
3b60b60
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 3b60b60. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR adds an aitools_command_event funnel to databricks aitools install: STARTED, a STEP_COMPLETED for each step reached, and one FINISHED with the outcome and error category. It also tags failures where they happen and stops early exits from waiting for telemetry auth. The fixes from the last round are in and work as described: every exit path emits exactly one FINISHED, CI is green, and the funnel and auth tests pass locally (removing the post-plan wait makes the wait test fail 298 of 300 runs). Nothing is blocking. There are two questions about trade-offs (timing skew in early-exit data, and the bundled --agents reorder, which has no changelog entry) and a few small nits.
Questions
- Runs that end before the plan is confirmed now upload their events only if the background auth has already resolved. A flag error, an unknown
--agentsname or a non-interactive run with no agent detected ends within milliseconds, before the auth resolution (config plus a host-metadata request) can finish, so those outcomes will rarely reach the table. Slower early exits, such as a cancelled prompt, usually will. The funnel will therefore undercount the fastest drop-offs, and STARTED, relative to the slower ones. Is that skew acceptable for the dashboards you plan to build? Options: keep it and note the bias where the dashboards are defined; give early exits a short bounded wait (for example a few hundred milliseconds); or let them use the normal bounded wait. - Validating
--agents(and the--project/--globalconflict) before the scope prompt changes what an interactive user sees: an unknown agent name now errors before the prompt instead of after it. That is a small user-visible change inside a telemetry PR, and the diff has no.nextchanges/clifragment, which the PR checklist asks for when command or flag behavior changes. Would you rather keep it here and add a fragment, or move the reorder into its own small PR?
Agreeing with existing threads
- [nice to have] Zero-skill agent installs finish SUCCESS while --path finishes NOTHING_TO_DO: #7469 (comment)
| if explicit { | ||
| targetAgents, err = resolveAgentNames(ctx, agentsFlag) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| } |
There was a problem hiding this comment.
[nit] With --agents, the targets are known here, but funnel.setAgents runs only after buildPlan (line 235). An interactive databricks aitools install --agents cursor with no --scope that is aborted (or fails) at the scope prompt finishes CANCELLED / USER_ABORTED (or FAILURE / PROMPT_FAILED) with an empty agents, although AitoolsCommandEvent.Agents says it is set "once the targets are known". A scratch test with the scope prompt returning huh.ErrUserAborted gives [STARTED FLAGS_VALIDATED FINISHED:CANCELLED:USER_ABORTED] with agents=[]. Record the explicit agents right after resolveAgentNames succeeds (e.g. a setAgents variant that takes []*agents.Agent), or reword the field comment to say it is set once the plan is built.
There was a problem hiding this comment.
Done in 54514a6. Agents named via --agents are now recorded right after resolveAgentNames succeeds (funnel.setTargets), so a run aborted at the scope prompt reports them. TestInstallFunnelAbortedScopePromptKeepsNamedAgents covers it (STARTED, FLAGS_VALIDATED, FINISHED CANCELLED USER_ABORTED with agents: [CURSOR]). The plan still sets them after buildPlan for the picker path; for --agents that is the same set.
| Env.DATABRICKS_SKILLS_BASE_URL = "$DATABRICKS_HOST" | ||
| Env.DATABRICKS_CLI_GITHUB_API_URL = "$DATABRICKS_HOST" | ||
| Env.DATABRICKS_SKILLS_REF = "test-ref" |
There was a problem hiding this comment.
[nit] This parent config now sets DATABRICKS_SKILLS_BASE_URL / DATABRICKS_SKILLS_REF and serves GET /test-ref/manifest.json, but install/test.toml still sets the same two variables and serves its own manifest on the same route. The leaf stub wins (acceptance/internal/prepare_server.go:248-251), so install runs against test-stable while its siblings use databricks-test, and a change to the parent manifest never reaches install. .agents/rules/testing.md asks to put common test.toml options in the parent. Drop the duplicated env lines and stubs from install/test.toml (its output only says "Installed 1 skill."), or keep the new stubs in the new leaf directories so hint/ and apps-init-tip/ don't inherit them.
There was a problem hiding this comment.
Done in 54514a6. Removed install/test.toml: install now uses the parent's env and manifest stubs like its siblings. Its output is unchanged ("Installed 1 skill.").
| mkdir -p "${USERPROFILE:-$HOME}/.cursor" | ||
|
|
||
| title "install: STARTED, one event per step reached, FINISHED with SUCCESS" | ||
| trace $CLI aitools install --skills-only --scope=global --agents=cursor |
There was a problem hiding this comment.
[nit] This is the same run as the first case of install/ (--skills-only --scope=global --agents=<one agent>), whose updated output.txt already pins STARTED through FINISHED SUCCESS; only the agent differs. .agents/rules/testing.md says to extend the existing acceptance directory when coverage overlaps. Drop install-success/ and rely on install/, or fold anything extra into install/script.
There was a problem hiding this comment.
Done in 54514a6. Dropped install-success/. The first case of install/ pins the same STARTED through FINISHED SUCCESS sequence.
| if err == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[nit] Every caller passes a non-nil error: string literals at the flag checks, and values inside if err != nil at install.go:172/197/422/443 and scope.go:97. So the nil branches here and in promptError (line 43) run only in TestWithCategoryKeepsMessage. AGENTS.md asks not to add speculative fallbacks or defensive nil checks for values callers always provide. Drop both nil branches and the two NoError(...(nil)) asserts, or keep them and say on the doc comment why nil must be accepted.
There was a problem hiding this comment.
Done in 54514a6. Dropped the nil branches from withCategory and promptError and the two NoError(...(nil)) asserts. Both doc comments now say they take a non-nil error.
| }) | ||
| } | ||
|
|
||
| func (f *commandFunnel) log(ctx context.Context, e protos.AitoolsCommandEvent) { |
There was a problem hiding this comment.
[nit] .agents/rules/comments.md asks for a doc comment on Go functions, starting with the name and ending with a period. This is the only new function without one. For example: // log stamps e with the run's command, interactivity, install mode and scope, and buffers it for upload.
There was a problem hiding this comment.
Done in 54514a6, with the doc comment you suggested.
3b60b60 to
54514a6
Compare
|
Thanks. Every inline comment has a reply in its thread. Changes are in 54514a6. Question 1 (early exits under-counted): I'm keeping the no-wait rule. Early exits stay instant by design, and a run that fails in milliseconds shouldn't wait seconds for telemetry. The resulting under-count of the fastest early exits (and their STARTED events) relative to slower ones is now stated in the code comment next to the rule ( Question 2 ( Zero-skill agent installs: answered in that thread. Only the |
e503a62 to
76d9ee3
Compare
54514a6 to
f6a8dd1
Compare
76d9ee3 to
7e03112
Compare
f6a8dd1 to
8ac74ab
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 8ac74ab. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 3 open design questions.
This PR adds an aitools_command_event funnel to databricks aitools install (STARTED, each step reached, one FINISHED with an outcome and error category). It also tags failures with a category where they happen, lets early exits skip the telemetry auth wait, and validates --agents before the scope prompt. Every exit path logs exactly one FINISHED, JSON mode classifies the real error, and CI is green. Nothing blocking was found. The open items are three design questions (early-exit sampling bias, the bundled --agents reorder, and two event families that define the outcome differently), a few missing funnel-sequence tests, and small test and convention nits.
Questions
- You've kept the no-wait rule for runs that end before the plan is confirmed and documented the under-count. That drops most fast flag errors, unknown agents and quick successful
--pathdumps (and their STARTED events), but keeps slow prompt cancels. Will the dashboards built on this funnel note or correct for that? Or would you consider a short bounded wait, for example a few hundred milliseconds, or waiting for a--pathdump, since it does network I/O anyway? - AGENTS.md asks to keep each PR focused on one change, and validating
--agentsbefore the scope prompt is a user-visible change with its own changelog entry. Would you land it as a small PR below this one, with a test that the scope prompt isn't shown for an unknown name, or do you prefer to keep it here? - The same run now gets two outcome definitions. A named agent skipped for the scope reports
aitools_install_event.error_category = TYPE_UNSPECIFIEDwithagent_results, while the funnel says FINISHEDFAILURE/UNSUPPORTED_SCOPE, and an exit-0 run that installed nothing is FAILURE only in the funnel. The outcome logic also lives in three places (the funnel switch,logInstallEvent,buildInstallOutput). Did you consider computing one run result afterexecutePlanfor all three? Alternatively,aitools_install_eventcould be logged on every run, as its doc says, withinstall_modeand a last-step field instead of a separate event family, since all funnel rows upload together at exit without timestamps.
Agreeing with existing threads
- [nice to have] An agent install where every skill is filtered out finishes SUCCESS, while --path finishes NOTHING_TO_DO: #7469 (comment)
- [nit] PR description does not follow the repo PR template: #7469 (review)
| funnel.finishWith(ctx, protos.AitoolsCommandOutcomeNothingToDo, protos.AitoolsErrorCategoryUnspecified) | ||
| return nil | ||
| } | ||
| funnel.step(ctx, protos.AitoolsCommandStepSkillsDumped) |
There was a problem hiding this comment.
[nice to have] No test asserts the sequence for a successful --path dump (FLAGS_VALIDATED, SKILLS_DUMPED, FINISHED SUCCESS). The same goes for a successful plugin install (PLUGINS_INSTALLED is checked only through deliverySteps) and for an accepted interactive confirm (TestInstallFunnelInteractiveCancel has no 'confirmed' row). If one of these branches stopped logging its step or logged the wrong outcome, no test would fail. Add a success case next to TestInstallFunnelPathDumpWithNoSkillToWrite with a non-experimental skill whose SKILL.md is served, a plugin-install run using setupPluginMock, and a confirmed row in TestInstallFunnelInteractiveCancel.
There was a problem hiding this comment.
Done in a8819ec. Added TestInstallFunnelPathDumpSuccess (local server with a stable skill and its SKILL.md: FLAGS_VALIDATED, SKILLS_DUMPED, FINISHED SUCCESS), TestInstallFunnelPluginSuccess (setupPluginMock, --agents codex: ... PLAN_CONFIRMED, PLUGINS_INSTALLED, FINISHED SUCCESS), and a confirmed row in the interactive test, now TestInstallFunnelInteractiveConfirm (PLAN_CONFIRMED, SKILLS_INSTALLED, FINISHED SUCCESS). Dropping the SKILLS_DUMPED or PLUGINS_INSTALLED step makes the matching test fail.
| opts.Scope = scope | ||
|
|
||
| // Resolve the agents to act on. | ||
| // Validate --agents with the other flags, before the scope prompt. |
There was a problem hiding this comment.
[nice to have] The .nextchanges fragment says an unknown --agents name now fails before the scope prompt. The only tests of an unknown name (TestInstallFunnelEarlyExits, TestInstallUnknownAgentErrors) run with cmdio.MockDiscard, where the prompt is never shown, so moving resolveAgentNames back below the prompt would not fail any test. Add a test with prompts supported, --agents bogus and a promptScopeSelection stub that fails the test if it is called.
There was a problem hiding this comment.
Done in a8819ec. TestInstallUnknownAgentFailsBeforeScopePrompt runs with prompts supported, --agents bogus and a promptScopeSelection stub that fails the test if called. It fails when resolveAgentNames is moved back below the prompt.
| start := time.Now() | ||
| require.ErrorContains(t, cmd.Execute(), "cannot use --skills-only with --path") | ||
| assert.Less(t, time.Since(start), 5*time.Second) | ||
| assert.False(t, cmdctx.HasConfigUsed(cmd.Context()), "an unfinished resolution leaves no config, so nothing uploads") |
There was a problem hiding this comment.
[nit] The blocked resolution is released only at test cleanup and has a 10 s budget. So with the wait put back, p.wait() times out and still leaves no config, and assert.False(t, cmdctx.HasConfigUsed(...)) passes either way. Only the < 5s timing check on the line above catches the regression. Drop this assertion, or reword its message so it doesn't read as a second check.
There was a problem hiding this comment.
Done in a8819ec. Dropped that assertion; the < 5s check is the one that catches the regression.
| return protos.AitoolsInstallScopeUnspecified | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[nit] .agents/rules/testing.md says each source file should have a corresponding test file. The funnel code (commandFunnel, installModeType, deliverySteps, nothingInstalledCategory, firstFailureCategory) is in telemetry.go, but its tests are in the new funnel_test.go. Move the funnel code into cmd/aitools/funnel.go, or move the tests into telemetry_test.go.
There was a problem hiding this comment.
Done in a8819ec. Moved the funnel code (commandFunnel, installModeType, deliverySteps, nothingInstalledCategory, firstFailureCategory) unchanged from telemetry.go into cmd/aitools/funnel.go, next to funnel_test.go.
| cmd.PreRunE = func(cmd *cobra.Command, args []string) error { | ||
| pending = startTelemetryAuth(cmd, args) | ||
| auth.pending = startTelemetryAuth(cmd, args) | ||
| auth.waitAtExit = true |
There was a problem hiding this comment.
[nit] Install is the only command that uses withTelemetryAuth, and its RunE sets auth.waitAtExit = false before doing anything (install.go:129), so the PreRunE assignment and the field comment's 'True at the start of every run' never take effect. Drop the PreRunE assignment and default to no-wait, so install only sets it to true at PLAN_CONFIRMED, or note why other commands need the default.
There was a problem hiding this comment.
Done in a8819ec. Dropped the PreRunE assignment. waitAtExit now defaults to false (no wait), and install sets it to true only at PLAN_CONFIRMED. The comment about the early-exit bias moved next to that line. withTelemetryAuth's own tests ask for the wait explicitly.
| }, funnelShape(funnelEvents(ctx))) | ||
| } | ||
|
|
||
| func TestInstallFunnelPathDump(t *testing.T) { |
There was a problem hiding this comment.
[nit] isolateAgents clears the whole environment, and the comment on the next test says Windows sockets need SystemRoot. On Windows the dial to 127.0.0.1:1 therefore fails during Winsock initialisation, not with connection refused. That is still a *net.OpError, so the test passes for the wrong reason. Set only HOME/USERPROFILE as TestInstallFunnelPathDumpWithNoSkillToWrite does, or keep SystemRoot.
There was a problem hiding this comment.
Done in a8819ec. TestInstallFunnelPathDump no longer clears the environment. It shares a setupPathDump helper with the other --path tests that sets only a temp HOME/USERPROFILE and the skills env, and stubs the telemetry auth. SystemRoot stays set, so the dial to 127.0.0.1:1 reaches the connect step on Windows too.
| trace print_telemetry | ||
|
|
||
| title "a manifest the server does not have is MANIFEST_NOT_FOUND" | ||
| musterr trace env DATABRICKS_SKILLS_REF=missing-ref $CLI aitools install --skills-only --scope=global --agents=cursor |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md documents that trace exports leading KEY=value arguments for the command, and that form is the common one in the repo. Use musterr trace DATABRICKS_SKILLS_REF=missing-ref $CLI ... here and on line 19.
There was a problem hiding this comment.
Done in a8819ec. Both cases now use musterr trace DATABRICKS_SKILLS_REF=... $CLI ....
8ac74ab to
a8819ec
Compare
|
Thanks. Every inline comment has a reply in its thread. Changes are in a8819ec. Question 1 (early-exit sampling): I'm keeping the no-wait rule as documented. A run that ends before the plan is confirmed never waits for telemetry auth, so it stays instant. The resulting bias is stated in the comment next to the rule (where install sets Question 2 ( Question 3 (one run result): Computing one run result after Every skill filtered out in an agent install: same answer as in that thread. It needs PR template: I'm keeping the stack's single body format, as before. |
7e03112 to
d25f879
Compare
a8819ec to
9f73e89
Compare
d25f879 to
fa4413b
Compare
9f73e89 to
2241742
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at 2241742, 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 (8ac74ab → 2241742) address its comments.
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Also found by our reviewers, already asked in simonfaltum's review (Question 1): runs that end before the plan is confirmed rarely upload, so the fastest early exits and their STARTED events are under-counted. Two of our reviewers found the same, and one also suggested a short bounded wait for those runs.
| // install run that gets past flag parsing emits a STARTED event, a | ||
| // STEP_COMPLETED event per step reached and a FINISHED event. All events of one | ||
| // run share ExecutionContext.CmdExecID. | ||
| // The calling agent is not a field: the User-Agent of the telemetry upload |
There was a problem hiding this comment.
[Question] This comment and libs/telemetry/protos/README.md say that the calling agent is not a field, because the User-Agent carries it. The Universe proto defines calling_agent = 6 and calling_agent_tooling = 7 on AitoolsCommandEvent. It documents calling_agent_tooling as "Set on STARTED and FINISHED events", and the User-Agent does not carry the tooling state. Will the dashboards leave both columns empty? If yes, write that the CLI leaves these fields empty on purpose, not that they are not fields. The answer matters because queries on these columns get only empty values.
There was a problem hiding this comment.
The CLI does not set a calling-agent field on this event. It reports the calling agent through the User-Agent of the telemetry upload: agent/<name> from the SDK, plus the calling agent's own Databricks tooling as aitools-agent/<plugin|skills|none> (installer.WithCallingAgentToolingInUserAgent, #7442), next to the same cmd-exec-id every event of the run carries. In be1f960 I reworded the comment here and in libs/telemetry/protos/README.md to say the CLI sets no such field, rather than that there is none.
| interactive: cmdio.IsPromptSupported(ctx), | ||
| installMode: installMode, | ||
| } | ||
| f.log(ctx, protos.AitoolsCommandEvent{Stage: protos.AitoolsCommandStageStarted}) |
There was a problem hiding this comment.
[Question] All events stay in memory, and cmd/root uploads them in one batch at exit. So a STARTED event always arrives together with its FINISHED event, and the STEP_COMPLETED events show only the last step reached. What does STARTED tell the dashboards that FINISHED does not? The answer matters because STARTED adds one row to every run of all five commands (#7470), and FINISHED with the last step would carry the same data.
There was a problem hiding this comment.
With the current upload, nothing: every run that logs STARTED also logs exactly one FINISHED, and cmd/root uploads them together at exit. So STARTED adds no information the FINISHED event (with the last STEP_COMPLETED before it) doesn't already carry. It makes each run's event list self-describing, and it would matter if events were ever uploaded before the process exits. Dropping it, or folding the last step into FINISHED, would change the event shape that the PRs stacked above this one also emit, so I'd leave that to a follow-up rather than change it here.
|
|
||
| // setTargets records agents known before the plan is built (named via | ||
| // --agents), so a run that stops at the scope prompt still reports them. | ||
| func (f *commandFunnel) setTargets(targets []*agents.Agent) { |
There was a problem hiding this comment.
[nit] setTargets makes agentPlanItem values with only agent set, so that it can reuse agentsField. If agentsField later reads another field of the plan item, this path reports wrong data. Map the agents to AitoolsAgentType in a small helper, and use that helper in setTargets and in agentsField.
There was a problem hiding this comment.
Done in be1f960. Added agentTypes([]*agents.Agent), which dedupes and sorts the enums. agentsField now collects the plan's agents and calls it, and setTargets calls it directly, so no partial agentPlanItem is built.
| tmp := t.TempDir() | ||
| t.Setenv("HOME", tmp) | ||
| t.Setenv("USERPROFILE", tmp) | ||
| t.Setenv("XDG_CONFIG_HOME", tmp+"/.config") |
There was a problem hiding this comment.
[nit] This line and line 470 build the path with tmp+"/.config". setupTestAgents uses filepath.Join(tmp, ".config"), and AGENTS.md asks for platform-agnostic paths. Use filepath.Join here too.
There was a problem hiding this comment.
Done in be1f960. Both lines now use filepath.Join(tmp, ".config").
2241742 to
be1f960
Compare
fa4413b to
0b368c6
Compare
be1f960 to
e7f6e53
Compare
|
@rugpanov thanks for the review. Every inline comment has a reply in its thread; the code changes are in be1f960. On the early-exit under-count (review body): this is the same point as Question 1 in simonfaltum's review, answered in #7469 (comment). The no-wait rule stays. The bias is stated in the comment next to it (install sets |
0b368c6 to
03fa288
Compare
e7f6e53 to
f4aaa10
Compare
03fa288 to
d13de92
Compare
f4aaa10 to
508718b
Compare
Every `databricks aitools install` run now logs an `aitools_command_event` funnel next to the existing `aitools_install_event`, so the telemetry shows where runs stop:
```text
install RunE
STARTED (every run past flag parsing)
validate flags, --agents names
--> FINISHED FAILURE INVALID_FLAGS | UNKNOWN_AGENT
STEP FLAGS_VALIDATED
--path dump --> STEP SKILLS_DUMPED, FINISHED SUCCESS
(0 skills written: FINISHED NOTHING_TO_DO)
scope prompt --> FINISHED CANCELLED USER_ABORTED | FAILURE PROMPT_FAILED
STEP SCOPE_RESOLVED
picker --> FINISHED CANCELLED (nothing checked)
no agent detected: FINISHED NOTHING_TO_DO
STEP AGENTS_SELECTED
confirm prompt --> FINISHED CANCELLED (declined)
STEP PLAN_CONFIRMED
executePlan
STEP SKILLS_INSTALLED (skills group installed)
STEP PLUGINS_INSTALLED (at least one plugin installed)
FINISHED SUCCESS | FAILURE <category>
```
Each event carries `command`, `stage`, `interactive`, `install_mode` (DEFAULT / SKILLS_ONLY / PATH) and, once known, `scope`. FINISHED also carries `outcome`, `error_category` and the targeted `agents`. Every event of a run shares the entry's `cmd_exec_id`. The CLI sets no calling-agent field on these events: the upload's User-Agent already has `agent/<name>`, the calling agent's Databricks tooling (`aitools-agent/...`) and `cmd-exec-id`.
`error_category` comes from the existing `classifyInstallError`. Failures in flag validation, agent lookup and prompts are tagged with a category where they happen (a prompt's own failure is PROMPT_FAILED whatever its type, except a Ctrl-C abort; an error before the prompt opens keeps its own category). `Error()` / `Unwrap()` pass through, so the user still sees the same message:
```diff
classifyInstallError(err)
*installer.BlockedError / *installer.SkillError -> their category
+ *categorizedError (withCategory, promptError) -> the tagged category
stdlibErrorCategory(err)
```
Some runs get an explicit outcome instead of the one their return value implies:
- Returns nil but installed nothing (e.g. the only detected agent's plugin install failed, which is a warning): FAILURE with the first failing agent's category.
- Fails only because a named agent failed or was skipped (`--agents`): the category comes from that agent's outcome (e.g. UNSUPPORTED_SCOPE), not UNCATEGORIZED.
- `--output json` failure: classifies the real error, not the `ErrAlreadyPrinted` sentinel.
- `--project --global` and `--agents ","`: INVALID_FLAGS, not PROMPT_FAILED / UNKNOWN_AGENT.
Runs that end before PLAN_CONFIRMED (a flag error, a cancelled prompt, no agent detected, a `--path` dump) upload their events only if the background telemetry auth has already resolved; they never wait for it, so they stay instant. The funnel therefore under-counts the fastest early exits relative to slower ones. Runs past PLAN_CONFIRMED keep the bounded wait.
An unknown `--agents` name now fails before the scope prompt instead of after it (changelog fragment `.nextchanges/cli/aitools-install-agents-before-scope-prompt.md`). Otherwise no change to output, prompts, exit codes or error text.
Co-authored-by: Isaac <no-reply@databricks.com>
508718b to
5a3cc25
Compare
Stacked on #7468.
Stacked on: aitools: bound best-effort telemetry auth
Summary
Every
databricks aitools installrun now logs anaitools_command_eventfunnel next to the existingaitools_install_event, so the telemetry shows where runs stop:Each event carries
command,stage,interactive,install_mode(DEFAULT / SKILLS_ONLY / PATH) and, once known,scope. FINISHED also carriesoutcome,error_categoryand the targetedagents. Every event of a run shares the entry'scmd_exec_id. The CLI sets no calling-agent field on these events: the upload's User-Agent already hasagent/<name>, the calling agent's Databricks tooling (aitools-agent/...) andcmd-exec-id.error_categorycomes from the existingclassifyInstallError. Failures in flag validation, agent lookup and prompts are tagged with a category where they happen (a prompt's own failure is PROMPT_FAILED whatever its type, except a Ctrl-C abort; an error before the prompt opens keeps its own category).Error()/Unwrap()pass through, so the user still sees the same message:classifyInstallError(err) *installer.BlockedError / *installer.SkillError -> their category + *categorizedError (withCategory, promptError) -> the tagged category stdlibErrorCategory(err)Some runs get an explicit outcome instead of the one their return value implies:
--agents): the category comes from that agent's outcome (e.g. UNSUPPORTED_SCOPE), not UNCATEGORIZED.--output jsonfailure: classifies the real error, not theErrAlreadyPrintedsentinel.--project --globaland--agents ",": INVALID_FLAGS, not PROMPT_FAILED / UNKNOWN_AGENT.Runs that end before PLAN_CONFIRMED (a flag error, a cancelled prompt, no agent detected, a
--pathdump) upload their events only if the background telemetry auth has already resolved; they never wait for it. Runs past PLAN_CONFIRMED keep the bounded wait.An unknown
--agentsname now fails before the scope prompt instead of after it (changelog fragment.nextchanges/cli/aitools-install-agents-before-scope-prompt.md). Otherwise no change to output, prompts, exit codes or error text.Evidence
cmd/aitools/telemetry/installfails; the upload has onlyaitools_install_event:install-failurepins the failure paths past PLAN_CONFIRMED: missing skill, manifest 404 / 500, named agent skipped for the scope. Runs that end earlier upload only if auth already resolved, which depends on timing, so they are covered by unit tests instead.cmd/aitools/funnel_test.go(for the funnel code infunnel.go) cover every early return and each success path (e.g.TestInstallFunnelEarlyExits,TestInstallFunnelAbortedScopePromptKeepsNamedAgents,TestInstallFunnelPathDumpSuccess,TestInstallFunnelPathDumpWithNoSkillToWrite,TestInstallFunnelPluginSuccess,TestInstallFunnelInteractiveConfirm,TestInstallFunnelDetectedPluginFailureIsNotSuccess,TestInstallFunnelSkillsFailureThroughJSON); they andTestAitoolsEventsWireKeyspass with-count=1.TestInstallUnknownAgentFailsBeforeScopePrompt: with prompts supported,--agents bogusfails without showing the scope prompt (fails ifresolveAgentNamesmoves back below the prompt).TestInstallEarlyExitDoesNotWaitForTelemetryAuth(a flag error returns at once while auth is still resolving; fails after 10 s with the wait put back) andTestInstallPastPlanConfirmedWaitsForTelemetryAuth(auth that finishes only after the install step still reaches the upload; fails without the wait).go build ./...,./task test-exp-aitools,go test ./libs/telemetry/... ./cmd/root/...,./task lint-q,./task fmt-q,./task checks, andgo vetwithGOOS=windows/GOOS=darwinpass.Merge Danger
Door: two-way
Telemetry only. Reverting removes the events; nothing on disk or in the install state changes.
Blast Radius: telemetry
aitools installupload (about eight instead of one), sent at exit and only when auth resolved. Runs that stop before the plan is confirmed (flag errors, a cancelled prompt, no agent detected) and--pathdumps logged no event before, so they made no telemetry request. They now upload only if the background auth has already resolved when they end, and never wait for it, so an unreachable or slow-to-authenticate workspace adds nothing to them. When auth has resolved, the upload itself can still take up to its timeout (DATABRICKS_CLI_TELEMETRY_TIMEOUT, default 3 s) against a slow workspace. The flip side: a run that ends in milliseconds (e.g. a flag error) usually ends before auth resolves, so its events are often dropped; a run that stopped at a prompt usually had time to resolve auth. The funnel therefore under-counts the fastest early exits, and their STARTED events, relative to slower ones; the code comment next to the rule says the same. Runs past the plan confirmation keep the bounded auth wait.--output jsonandaitools_install_eventreport these runs as installed today too. The--pathdump knows how many skills it wrote, so a dump that writes none finishes NOTHING_TO_DO. A run killed by Ctrl-C outside a prompt, or rejected by flag parsing, uploads no funnel events.This pull request and its description were written by Isaac.