Repository navigation
aitools: bound best-effort telemetry auth - #7468
Conversation
0d29efd to
a15150a
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/install-host-metadata: branch ✅ | main (0f407e7) ❌ | latest (v1.26.0) ❌main (0f407e7): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (0f407e7) ❌ | latest (v1.26.0) ❌main (0f407e7): latest (v1.26.0): |
✅ Integration test reportCommit: d13de92
Top 11 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 a15150a. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 1 blocking issue(s), tagged [blocking]; 1 open design question.
This PR stops aitools install from blocking on an unreachable workspace. Telemetry auth now resolves in the background on a shadow command, the wait is capped at 3 s from start, and its logs show only with --debug. The approach works as described and CI is green on all platforms. One blocking concern remains: when the wait ends while an OAuth refresh is still running, the CLI exits before saving the new single-use refresh token, so the user can be logged out. Two smaller suggestions are to skip the wait when no event was logged and to add an end-to-end upload test, plus a design question about reusing Upload's existing timeout instead of adding a second one.
Questions
- libs/telemetry.Upload already bounds config resolution and authentication within its own timeout (newAPIClientWithin and authenticate). Did you consider putting an unresolved config (with --profile and env applied) on the context and letting Upload resolve it? Or moving this helper into root or libs/telemetry so other commands can reuse it? Either would remove the shadow command, goroutine handoff, and log handler from cmd/aitools. The trade-offs are less overlap with the install, MustAnyClient's account-client fallback, and where hostmetadata warnings surface. Which approach do you prefer, and why?
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary,## Evidence, and## Merge Danger..agents/skills/pr-checklist/SKILL.mdasks agent-written PRs to follow.github/PULL_REQUEST_TEMPLATE.mdexactly (## Changes,## Why,## Tests, with no invented sections) and to keep the summary concise. Please move the content under those headings and trim it.
| log.Debugf(sctx, "auth for telemetry panicked: %v", r) | ||
| } | ||
| }() | ||
| resolve(shadow, args) |
There was a problem hiding this comment.
[blocking] The background resolution can refresh a U2M token: PersistentAuth.refresh posts to the token endpoint and then stores the result. The process exits without waiting for it once apply gives up. Example: databricks-cli auth, an expired access token, a slow but reachable workspace, and a short --skills-only install where the host-metadata fetch plus the refresh straddle the 3 s deadline. The server rotates the single-use refresh token (cmd/auth/README.md), the new one is never stored, and the next command fails until databricks auth login. Before this PR the refresh always finished in PreRunE. A best-effort telemetry path should not be able to log the user out. Please keep the refresh out of the goroutine that gets abandoned. For example, resolve only the config in the background and authenticate on the main goroutine in apply once it has resolved, so a reachable host makes this one short request. Otherwise, get an explicit sign-off from the auth owners that this risk is acceptable.
There was a problem hiding this comment.
Fixed in e503a62. The background resolution never refreshes a token now. A new root.SkipTokenRefresh context option makes MustWorkspaceClient/MustAccountClient use auth.CredentialsWithoutRefresh, whose databricks-cli strategy passes the new u2m.WithoutRefresh: PersistentAuth.Token returns the cached access token only while it is valid (it skips the proactive refresh too) and returns ErrRefreshDisabled for an expired one. A failed resolution now leaves no config on the command, so Upload does not retry auth (or refresh) at exit either. An expired token therefore just drops the event, and the next command that authenticates refreshes it as usual.
Tests: TestToken_WithoutRefresh (no token request, nothing stored), TestCLICredentialsConfigure_NoRefresh, TestCredentialChainWithoutRefresh, TestTelemetryAuthFailedResolutionUploadsNothing, and acceptance/cmd/aitools/telemetry/install, which installs with an expired cached token and checks that there is no /oidc/v1/token request and no upload, and that the cache is unchanged. Without SkipTokenRefresh that case shows the refresh POST.
| if p == nil { | ||
| return | ||
| } | ||
| if !p.wait() { |
There was a problem hiding this comment.
[nice to have] apply waits up to the budget even when the run logged no telemetry event, and Upload then returns at len(l.logs) == 0. On an unreachable workspace, install --path ./dir, the flag-validation errors, "no agents found", and "Cancelled." all return before logInstallEvent's defer, yet still sit until 3 s after start for nothing. Return early when len(telemetry.BufferedEvents(cmd.Context())) == 0. That check is correct here because the install event is logged in a defer inside runE. Also update BufferedEvents' doc, which says only tests use it. Returning early also exits sooner while a refresh may be in flight, so this is safest once the refresh no longer runs in the background goroutine.
There was a problem hiding this comment.
Done in e503a62. apply returns before waiting when telemetry.BufferedEvents is empty (covered by TestTelemetryAuthNoEventDoesNotWait), and BufferedEvents' doc now mentions this use. Exiting early can no longer cut a refresh short, since the background resolution never refreshes.
| export DATABRICKS_HOST=https://unreachable.test | ||
|
|
||
| title "workspace unreachable: the background auth resolution prints nothing\n" | ||
| trace $CLI aitools install --skills-only --scope=global --agents=claude-code |
There was a problem hiding this comment.
[nice to have] The success path, where the real MustAnyClient runs on the shadow command and the copied config reaches root's Upload, is covered only by a unit test with a fake resolver. If the shadow stopped seeing --profile, or root uploaded from a different context, aitools install would silently stop uploading its event and no test would fail. Please add a case under acceptance/cmd/aitools/telemetry/, which already records /telemetry-ext and has print_telemetry. It would run aitools install --skills-only --scope=global --agents=claude-code against the mock server, with this test's manifest stubs, and print the uploaded aitools_install_event.
There was a problem hiding this comment.
Added acceptance/cmd/aitools/telemetry/install in e503a62. It runs aitools install --skills-only --scope=global --agents=claude-code against the mock server and prints the uploaded aitools_install_event for PAT auth and for a databricks-cli profile passed with --profile and a valid cached token. A third case uses an expired cached token and checks that nothing is refreshed or uploaded.
While there: the new base includes #7003 (title adds its own trailing newline), so I dropped the \n from this script's titles, which had made the test fail on the restacked head.
| // telemetryAuthBudget bounds how long a command waits, at the end of its run, | ||
| // for the best-effort auth resolution that lets its telemetry upload. The | ||
| // resolution runs in the background while the command works, so the command | ||
| // waits only for the part of it that outlasts its run, and only until this | ||
| // budget after its start; a resolution still unfinished at the later of the | ||
| // end of the run and that deadline drops the events. Matches the upload's own | ||
| // default timeout. A resolution that does finish is followed by the upload, | ||
| // which has its own timeout, so after a short run a slow but reachable | ||
| // workspace can add up to about twice this. | ||
| // | ||
| // While the workspace stays unreachable, every run takes at least this long | ||
| // (the budget starts when the command does, so a run that takes longer waits | ||
| // nothing at the end), including runs that buffered no event and upload | ||
| // nothing, such as a flag-validation error: the SDK retries the host-metadata | ||
| // request on connection errors, so the fetch outlasts the budget and the | ||
| // process exits before libs/hostmetadata writes its negative-cache entry, and | ||
| // the next run probes the host again. | ||
| var telemetryAuthBudget = 3 * time.Second |
There was a problem hiding this comment.
[nit] The doc comments on telemetryAuthBudget (lines 21-37) and startTelemetryAuth (lines 70-87) are each about 17 lines and re-derive the timing and risk analysis from the PR description. .agents/rules/comments.md asks for concise doc comments that give the contract and any surprising behavior, not a re-spec. Please trim each to a few lines: what the budget bounds and that an unfinished resolution drops the event, and why the shadow command and the debug-only logger exist. The risk matrix can stay in the PR description.
There was a problem hiding this comment.
Trimmed both in e503a62. telemetryAuthBudget now states what it bounds and that an unfinished resolution drops the events. startTelemetryAuth covers why the shadow command and the debug-only logger exist and that it never refreshes a token. The timing and risk analysis stays in the PR description.
| } | ||
| }() | ||
| resolve(shadow, args) | ||
| if sctx := shadow.Context(); cmdctx.HasConfigUsed(sctx) { |
There was a problem hiding this comment.
[nit] if sctx := shadow.Context(); ... declares a second sctx inside the goroutine, while the deferred recover above logs on the outer one. Two contexts under one name invite a later edit to use the wrong one. Please rename it (e.g. resolved) or call shadow.Context() directly.
2ca5f55 to
0326a2e
Compare
a15150a to
86362f1
Compare
0326a2e to
673bfd5
Compare
86362f1 to
3147276
Compare
673bfd5 to
e1c80b3
Compare
3147276 to
920dbc0
Compare
e1c80b3 to
71b4c1a
Compare
95aeec8 to
e503a62
Compare
|
Thanks for the review. The inline threads have replies; here are the two review-level points. Question 1 (resolving in
Generalizing it into root or PR template nit: this PR is part of a stack whose bodies all use one format (Summary / Evidence / Merge Danger). I'd rather not rewrite all of them, unless you feel strongly about it. |
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 e503a62. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR stops aitools install from blocking on an unreachable workspace. Telemetry auth now runs in the background and is waited for at most 3 s, its logs show only with --debug, and it never refreshes an OAuth token. All earlier review comments are addressed, including the abandoned-refresh risk, and CI is green on Linux, macOS and Windows with the new tests running on all three. Two design questions remain: dropping install events for users whose cached U2M token has expired, and whether a bounded synchronous resolution would be simpler. The rest are small doc and test nits.
Questions
- With SkipTokenRefresh, a databricks-cli user whose cached access token has expired (these last about an hour) uploads no aitools_install_event. The base branch refreshed in PreRunE and uploaded it. Since aitools install often runs on its own, how large and how skewed do you expect that share to be? Would you accept it and state the expected share under Merge Danger, or authenticate with refresh on the main goroutine in apply once the background resolution has resolved the host, so a reachable host costs one token request at exit?
- Your earlier reply explains why you prefer the shadow command over resolving in Upload. Did you also consider keeping the synchronous PreRunE resolution but bounding it with the SDK's HTTPTimeoutSeconds and RetryTimeoutSeconds, as cmd/auth/profiles.go already does for unreachable hosts? The host-metadata fetch goes through the SDK client that honors both. That would keep normal token refresh, and so the expired-token events, and remove the goroutine, shadow command, debug-only handler and the new no-refresh mode. The costs are that auth no longer overlaps with the install and the hostmetadata warning stays visible. Which trade-off do you prefer?
Comments outside the changed lines
- [nit]
cmd/aitools/telemetry.go: PR description: Evidence says "Tests at head, all pass (Linux only)", and Merge Danger lists "Untested: Windows and macOS (the 3 s timing, goroutines at process exit)". CI'stask test (macos, direct)andtask test (windows, direct)jobs ran and passedTestTelemetryAuth*,TestToken_WithoutRefresh,TestAccept/cmd/aitools/install-host-metadataandTestAccept/cmd/aitools/telemetry/install. Please update both sections so reviewers don't count a risk that CI already partly covers.
Agreeing with existing threads
- [nit] PR body does not follow .github/PULL_REQUEST_TEMPLATE.md: #7468 (review)
| return ok && skipLoadBundle | ||
| } | ||
|
|
||
| type skipTokenRefresh int |
There was a problem hiding this comment.
[nit] type skipTokenRefresh int goes against .agents/rules/style-guide-go.md, which says to use empty struct types for context keys and lists type myKeyType int as BAD. The neighbouring skipPrompt and skipLoadBundle keys use the old pattern, so this only affects the new key. Suggest type skipTokenRefreshKey struct{} with context.WithValue(ctx, skipTokenRefreshKey{}, true).
There was a problem hiding this comment.
Changed to type skipTokenRefreshKey struct{} in 76d9ee3. The neighbouring keys are left as they are.
| // SkipTokenRefresh makes [MustWorkspaceClient] and [MustAccountClient] use a | ||
| // cached OAuth token only while it is valid and fail instead of refreshing it. | ||
| // It is for best-effort auth that the command may abandon at exit, where an | ||
| // unfinished refresh could lose the server's rotated single-use refresh token. | ||
| // It does not reach a bundle's workspace client; combine it with | ||
| // [SkipLoadBundle]. |
There was a problem hiding this comment.
[nit] The doc says SkipTokenRefresh makes MustWorkspaceClient and MustAccountClient fail instead of refreshing, and names only the bundle exception. The interactive profile pickers build a fresh databricks.Config{Profile: profile} without CredentialsWithoutRefresh: workspaceClientOrPrompt at cmd/root/auth.go:352 and accountClientOrPrompt at :170. A caller that sets SkipTokenRefresh without SkipPrompt can therefore still refresh, and lose the rotated refresh token, on an interactive terminal. aitools sets SkipPrompt, so nothing is wrong today. Please say in the doc that it must also be combined with SkipPrompt, or apply the no-refresh credentials to the picked-profile config.
There was a problem hiding this comment.
Documented in 76d9ee3. The doc now says SkipTokenRefresh applies only to the config that MustWorkspaceClient/MustAccountClient build, that a bundle's workspace client and an interactively picked profile still refresh, and that callers must combine it with SkipLoadBundle and SkipPrompt. aitools sets both.
| // CredentialsWithoutRefresh returns the CLI credentials chain with a | ||
| // databricks-cli strategy that uses the cached OAuth token only while it is | ||
| // valid and never refreshes it (see [u2m.WithoutRefresh]). For best-effort | ||
| // auth that the command may abandon at exit, such as telemetry's. |
There was a problem hiding this comment.
[nit] With no auth_type (for example DATABRICKS_HOST from the environment, or a hand-written profile), the SDK chain ignores each strategy's dry-run error and tries the next one (credentialsChain.Configure). An expired cached U2M token now fails the databricks-cli dry run with ErrRefreshDisabled, so the chain moves on to the metadata-service, OIDC, Azure and Google strategies. For example, on an Azure host with az logged in, the background goroutine runs az account get-access-token and uploads telemetry as the az identity. The default chain would have refreshed and stopped at databricks-cli. That is acceptable for best-effort telemetry, but please mention it in this doc comment and in Merge Danger so later callers of CredentialsWithoutRefresh know.
There was a problem hiding this comment.
Documented in 76d9ee3 on CredentialsWithoutRefresh. With auth_type = databricks-cli an expired token fails auth. Without an auth_type, the chain skips the failed dry run and moves on to the metadata-service, OIDC, Azure and Google strategies, and the first one that succeeds is used (e.g. az on an Azure host). The default chain would have refreshed and stopped at databricks-cli. The same behavior is now a Merge Danger bullet.
| require.NoError(t, err) | ||
|
|
||
| // WithOAuthArgument, WithTokenStore and WithoutRefresh. | ||
| assert.Len(t, receivedOpts, 3) |
There was a problem hiding this comment.
[nit] assert.Len(t, receivedOpts, 3) still passes if the noRefresh branch appended a different option, or if a refactor reordered the list, so the no-refresh behavior could disappear without this test failing. One way to test the behavior instead: pass receivedOpts plus a u2m.WithTokenStore holding an expired token to u2m.NewPersistentAuth, and assert that Token() returns u2m.ErrRefreshDisabled.
There was a problem hiding this comment.
Strengthened in 76d9ee3. The test now passes the options Configure returned, plus a u2m.WithTokenStore holding an expired token, to u2m.NewPersistentAuth, and asserts that Token() returns u2m.ErrRefreshDisabled. Replacing WithoutRefresh with another option in Configure makes it fail.
e503a62 to
76d9ee3
Compare
|
Thanks. Both questions are answered below; the inline nits are fixed in 76d9ee3. Question 1 (expired cached token uploads no event): I accept that. An install whose cached Question 2 (bounded synchronous PreRunE): I'd keep the background resolution. Bounding PreRunE with Outside-diff nit: updated. Evidence now says the tests pass locally on Linux and in CI on Linux, macOS and Windows, and the "Untested" bullet now says that only the Evidence timings were measured on Linux alone. |
71b4c1a to
29d7bbc
Compare
76d9ee3 to
7e03112
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 7e03112. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR stops aitools install from blocking on an unreachable workspace. Telemetry auth now resolves in the background with a 3 s cap, never refreshes an OAuth token, and logs only at debug level. Every earlier review thread is addressed at this head, the new tests pass under the race detector, and CI is green on Linux, macOS and Windows. What's left is non-blocking: drop the PreRunE/RunE split, fix a test that calls t.Fatal on a background goroutine, close a pre-existing upload gap in the deprecated skills install alias, and three test-convention nits. There are also two follow-up questions on design trade-offs you've already accepted.
Questions
- You've accepted that an install whose cached databricks-cli access token has expired uploads no install event. Databricks OAuth access tokens are short-lived and
aitools installoften runs on its own, so the loss may skew install metrics toward PAT users and people who ran another CLI command recently. Do you have a rough sense of that share? Would you add a note under Merge Danger that metrics built on this event undercount those users, or is leaving it unquantified fine for now? - Your replies explain why you kept the background resolution rather than resolving in
Uploador bounding a synchronousPreRunEwith the SDK's timeouts. That reasoning is only in PR comments right now. Would you add a short note on the rejected alternatives to the description so it stays with the change, or is the current Merge Danger section enough?
| func withTelemetryAuth(cmd *cobra.Command) { | ||
| var pending *pendingAuth | ||
| cmd.PreRunE = func(cmd *cobra.Command, args []string) error { | ||
| pending = startTelemetryAuth(cmd, args) | ||
| return nil | ||
| } | ||
| runE := cmd.RunE | ||
| cmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| err := runE(cmd, args) | ||
| pending.apply(cmd) | ||
| return err | ||
| } |
There was a problem hiding this comment.
[nice to have] withTelemetryAuth starts the resolution in PreRunE, waits for it in the RunE wrapper, and passes pending between them through the closure. Cobra runs only ValidateRequiredFlags/ValidateFlagGroups between the two hooks, so the split gains no overlap. It does overwrite any PreRunE the command has: one added to install later would be silently dropped. It also creates the nil-pending case that TestTelemetryAuthRunEWithoutPreRun exists for. Suggest doing both in the wrapper (p := startTelemetryAuth(cmd, args); err := runE(cmd, args); p.apply(cmd); return err) and dropping the PreRunE assignment and that test.
There was a problem hiding this comment.
I'd rather leave the split in this PR. #7469, stacked on this one, builds on the wrapper's current shape: withTelemetryAuth returns a handle and the exit wait becomes opt-in, and it rewrites exactly these lines. Moving the start into the RunE wrapper here makes #7469 conflict on restack. The behavior is the same either way, since cobra runs only flag validation between the two hooks. This can be folded in on top of the stack in a follow-up.
There was a problem hiding this comment.
Our review found this too: three of our reviewers flagged the replaced PreRunE. One note: #7470 applies this helper to four more commands through instrumentCommand. So a PreRunE that someone later adds to update, uninstall, list or version is dropped with no error. If you keep the split, panic when cmd.PreRunE is already set, so that the conflict fails at once.
There was a problem hiding this comment.
Thanks. I tried the guard: withTelemetryAuth panicking when PreRun or PreRunE is already set. It conflicts with #7469, which rewrites these same lines to return the telemetryAuth handle, so I left it out of this PR. None of the commands that #7470 wraps (install, update, uninstall, list, version) has a PreRun or PreRunE today, so nothing is dropped now. The guard, or moving the start into the RunE wrapper, fits a follow-up on top of the stack, where instrumentCommand applies the helper to all five commands.
| } | ||
|
|
||
| func TestTelemetryAuthRunEWithoutPreRun(t *testing.T) { | ||
| setTelemetryAuth(t, time.Second, func(cmd *cobra.Command, args []string) error { t.Fatal("must not resolve"); return nil }) |
There was a problem hiding this comment.
[nit] This resolver calls t.Fatal, but resolvers only run on the background goroutine. If a change made it run, t.Fatal would fire off the test goroutine, probably after the test returned, since RunE does not wait when pending is nil. The test would then pass, or panic with "Fail in goroutine after TestTelemetryAuthRunEWithoutPreRun has completed", instead of failing cleanly. Record an atomic.Bool in the resolver and assert it on the test goroutine, as TestTelemetryAuthSkippedWhenTelemetryDisabled does with called. This goes away if the PreRunE split is removed.
There was a problem hiding this comment.
Fixed in d25f879. The resolver now records the call on a buffered channel, and the test asserts on the test goroutine (in a t.Cleanup) that nothing was recorded, so a stray call fails the test cleanly instead of calling t.Fatal off-goroutine.
| return | ||
| } | ||
| if p.cfg != nil { | ||
| cmd.SetContext(cmdctx.SetConfigUsed(cmd.Context(), p.cfg)) |
There was a problem hiding this comment.
[nit] This existed before this PR, but it shows here: apply puts the config on the context of the command it wraps. The deprecated experimental aitools skills install alias builds its own NewInstallCmd() and runs installCmd.Execute() (cmd/aitools/legacy_skills.go:45-58), while root uploads from the alias command's context. So the alias always ends with "no auth config available; skipping telemetry upload", and on an unreachable host it first waits up to 3 s for a config it then drops. A one-line fix in the alias: err := installCmd.Execute(); cmd.SetContext(installCmd.Context()); return err.
There was a problem hiding this comment.
Fixed in d25f879. After installCmd.Execute(), the alias now copies installCmd.Context() back onto its own command, so the config resolved for telemetry reaches root's upload. TestLegacySkillsInstallKeepsTelemetryAuthConfig covers it, and it fails without the fix.
| // It applies to that config only. A bundle's workspace client and the config | ||
| // of a profile picked interactively still refresh, so combine it with | ||
| // [SkipLoadBundle] and [SkipPrompt]. | ||
| func SkipTokenRefresh(ctx context.Context) context.Context { |
There was a problem hiding this comment.
[nit] SkipTokenRefresh/shouldSkipTokenRefresh have no test, while SkipPrompt and SkipLoadBundle each have one in cmd/root/auth_options_test.go. .agents/rules/testing.md asks to extend the test file when adding functionality. Add a TestSkipTokenRefresh that mirrors the two existing tests.
There was a problem hiding this comment.
Added TestSkipTokenRefresh in cmd/root/auth_options_test.go in d25f879, mirroring the two existing tests.
| write_token_cache expired-access-token 2000-01-01T00:00:00Z | ||
| trace $CLI aitools install --skills-only --scope=global --agents=claude-code --profile test-profile | ||
| trace print_requests.py //oidc/v1/token //telemetry-ext | ||
| trace jq -r '.tokens["test-profile"].access_token' "$HOME/.databricks/token-cache.json" |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md prefers gron.py | grep <field> over inline jq paths for single-value lookups, because the gron line names the path in output.txt. Suggest trace gron.py "$HOME/.databricks/token-cache.json" | grep access_token, then regenerate the output with -update.
There was a problem hiding this comment.
Switched to trace gron.py "$HOME/.databricks/token-cache.json" | grep access_token and regenerated the output in d25f879.
| @@ -0,0 +1,217 @@ | |||
| package aitools | |||
There was a problem hiding this comment.
[nit] The tests for the new code in cmd/aitools/telemetry.go live in a new telemetry_auth_test.go with no matching source file. .agents/rules/testing.md asks for one test file per source file, extended as the source grows. Either move the auth code (telemetryAuthBudget through debugOnlyHandler) into cmd/aitools/telemetry_auth.go, which keeps telemetry.go about building events, or move these tests into telemetry_test.go.
There was a problem hiding this comment.
I'd rather not move them in this PR. #7469 and #7470, stacked on this one, extend both the auth code in telemetry.go and telemetry_auth_test.go, so moving either here makes both PRs conflict on restack. It's a pure move, so it fits better as a follow-up on top of the stack (e.g. the auth code into telemetry_auth.go, keeping telemetry.go about building events).
7e03112 to
d25f879
Compare
|
Thanks. Answers to the two questions are below; the inline threads have replies, with fixes in d25f879. Question 1: I'm fine leaving it unquantified. Merge Danger already says that an install whose cached Question 2: added. The description now has a short paragraph on the rejected alternatives (resolving in |
29d7bbc to
2c02b39
Compare
d25f879 to
fa4413b
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at fa4413b, 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 (7e03112 → fa4413b) 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): an install whose cached databricks-cli token has expired uploads no install event. Our note: our devil's-advocate reviewer built the alternative that passes the unresolved config to Upload (about 22 lines). It reported the same exit time as this PR with the PR's own repro (3.37 s and 3.38 s against a host that refuses the connection), and it keeps the install event when the cached token has expired.
| # Isolate HOME so the real machine's agents and skills are not seen. | ||
| sethome home | ||
| # Agent detection needs ~/.claude; prefer USERPROFILE on Windows. | ||
| mkdir -p "${USERPROFILE:-$HOME}/.claude" |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md says: "Commit static test inputs into the acceptance test directory; do not create them in script at test time." #7467 fixed the same pattern in apps-init-tip with a committed home/.claude/.gitkeep. This script and acceptance/cmd/aitools/telemetry/install/script:4 still run mkdir -p "${USERPROFILE:-$HOME}/.claude". Commit home/.claude/.gitkeep in both directories and remove the mkdir.
There was a problem hiding this comment.
Done in 03fa288. Both install-host-metadata and telemetry/install now commit home/.claude/.gitkeep, and the mkdir is gone from their scripts.
| title "--debug still shows the hostmetadata line, at debug level" | ||
| # A fresh cache, so the first run's negative-cache entry does not skip the fetch. | ||
| export DATABRICKS_CACHE_DIR="$PWD/debug-cache" | ||
| $CLI aitools install --skills-only --scope=global --agents=claude-code --debug 2>&1 | grep -o 'Debug: \[hostmetadata\] failed to fetch host metadata for [^,]*' |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md asks to use the acceptance/bin/ helpers before inline grep or jq pipelines. This line pipes the --debug run through grep -o without trace. Use trace $CLI ... --debug &> LOG.debug, and then contains.py 'failed to fetch host metadata' < LOG.debug.
There was a problem hiding this comment.
Done in 03fa288. The --debug run is now traced into LOG.debug, and contains.py checks it for Debug: [hostmetadata] failed to fetch host metadata for https://unreachable.test and for no Warn: [hostmetadata] line. I redirect its stdout to /dev/null because contains.py echoes its input, and the full debug log has timestamps and a pid. If the line is missing or still at Warn, the test fails with Exit code: 1; I checked this by inverting the assertion.
19f3090 to
96084b5
Compare
0b368c6 to
03fa288
Compare
|
Re the review summary (expired cached token): see my answer to Question 1 in simonfaltum's review (#7468 (comment)); Merge Danger states the trade-off. Passing the unresolved config to |
`aitools install` resolved auth for telemetry in PreRunE, before doing any work, so an unreachable workspace blocked the install for as long as the SDK retried. It now resolves auth in the background while the install runs and waits for it at most 3 s after the command started. Nothing is resolved when `DATABRICKS_CLI_DISABLE_TELEMETRY` is set (new `telemetry.Disabled`), and a run that logged no event does not wait. ```diff aitools install - PreRunE: tryConfigureAuth - MustAnyClient(cmd) # blocks while the SDK retries host metadata - RunE: install skills + PreRunE: startTelemetryAuth # returns at once + if telemetry.Disabled → skip + go MustAnyClient(shadow cmd) # shares cmd's flags (--profile), not its context + # SkipTokenRefresh: never refreshes an OAuth token + RunE wrapper + runE: install skills # runs while auth resolves + pending.apply(cmd) + no event logged → return + wait until start + 3 s, at most + succeeded → SetConfigUsed(cmd ctx) + failed or not finished → no config, no upload root: telemetry.Upload ``` The background resolution never refreshes an OAuth token. The process may exit before it finishes, and the server rotates the single-use refresh token, so an abandoned refresh could lose the new one and log the user out. The new `root.SkipTokenRefresh` makes `MustWorkspaceClient` and `MustAccountClient` use `auth.CredentialsWithoutRefresh`, whose databricks-cli strategy passes the new `u2m.WithoutRefresh`: the cached token is used only while it is valid, and an expired one fails auth, so nothing is uploaded. The shadow command logs through `debugOnlyHandler`, so a background warning such as `[hostmetadata] failed to fetch host metadata ...` shows only with `--debug` and does not interrupt the install's output or prompts. This relies on root's host-metadata resolver logging through the command's logger (#7390). A panic in the background resolution is recovered and logged at debug level; the install does not fail. Changelog fragment: `.nextchanges/cli/aitools-install-telemetry-auth.md`. Co-authored-by: Isaac <no-reply@databricks.com>
03fa288 to
d13de92
Compare
Stacked on #7467.
Stacked on: aitools: log install-hint telemetry and bound telemetry uploads
Summary
aitools installresolved auth for telemetry in PreRunE, before doing any work, so an unreachable workspace blocked the install for as long as the SDK retried. It now resolves auth in the background while the install runs and waits for it at most 3 s after the command started. Nothing is resolved whenDATABRICKS_CLI_DISABLE_TELEMETRYis set (newtelemetry.Disabled), and a run that logged no event does not wait.The background resolution never refreshes an OAuth token. The process may exit before it finishes, and the server rotates the single-use refresh token, so an abandoned refresh could lose the new one and log the user out. The new
root.SkipTokenRefreshmakesMustWorkspaceClientandMustAccountClientuseauth.CredentialsWithoutRefresh, whosedatabricks-clistrategy passes the newu2m.WithoutRefresh: the cached token is used only while it is valid, and an expired one fails auth, so nothing is uploaded.The shadow command logs through
debugOnlyHandler, so a background warning such as[hostmetadata] failed to fetch host metadata ...shows only with--debugand does not interrupt the install's output or prompts. This relies on root's host-metadata resolver logging through the command's logger (#7390). A panic in the background resolution is recovered and logged at debug level; the install does not fail.Alternatives considered: resolving auth inside
telemetry.Uploadwould serialize it after the install at exit and dropMustAnyClient's account-client fallback and--profilehandling. Bounding a synchronous PreRunE with the SDK'sHTTPTimeoutSeconds/RetryTimeoutSecondswould still add that wait to every install before any work starts and keep the hostmetadata warning in its output; resolving in the background overlaps auth with the install instead.The deprecated
experimental aitools skills installalias runs its own install command, so it now copies that command's context back, and the config resolved for telemetry reaches root's upload (before, the alias never uploaded).Changelog fragment:
.nextchanges/cli/aitools-install-telemetry-auth.md.Evidence
aitools install --skills-only --scope=global --agents=claude-code, skills served locally, fresh HOME and cache per run; before = parent branch binary, after = this branch's head:Tests at head, all pass locally on Linux and in CI on Linux, macOS and Windows:
Merge Danger
Door: two-way
No telemetry schema change, persisted state or config. A revert restores the blocking PreRunE.
Blast Radius:
aitools install.root.SkipTokenRefreshis new and onlyaitools installsets it; without itMustWorkspaceClient,MustAccountClientandPersistentAuth.Tokenbehave as before.DATABRICKS_CLI_TELEMETRY_TIMEOUT, default 3 s), so after a short install the end can take up to about 6 s. No run waits longer for auth than it did before.aitools installrun that logged an event lasts at least 3 s. Runs with nothing to upload (e.g. a flag-validation error) do not wait. The process exits while the SDK is still retrying, before libs/hostmetadata writes its negative-cache entry, so the next run probes the host again.databricks-cliaccess token has expired uploads no install event, because the background resolution does not refresh it. This is on purpose: a refresh that the exit can cut short is what can strand a rotated refresh token. The next command that authenticates refreshes the token as usual. Before, the install refreshed it in PreRunE and uploaded the event.auth_type: with noauth_type(e.g.DATABRICKS_HOSTfrom the environment, or a hand-written profile), the SDK chain skips a strategy whose dry run fails, so an expired token moves on to the strategies afterdatabricks-cli(metadata service, OIDC, Azure, Google). If one succeeds, e.g.azlogged in on an Azure host, the event is uploaded with that identity. The default chain would have refreshed and stopped atdatabricks-cli.databricks-clistrategy,libs/authToOAuthArgumentlooks up host metadata outside the command's logger; SDK warnings logged on its owncontext.Background()(e.g. missingoidc_endpoint) are not filtered either.This pull request and its description were written by Isaac.