Repository navigation
Conversation
5601cd2 to
2c28258
Compare
3af19e5 to
2711bbe
Compare
|
Commit: Regression test reportSelected 12 changed tests (limit=20, 0 not selected) 12 tests: regression=8 unreleased=0 coverage=4 failing=0
TestAccept/cmd/aitools/install-host-metadata: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/apps-init-tip: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/hint: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/install-failure: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/update-uninstall: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/workspace-unreachable: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): TestAccept/cmd/version: branch ✅ | main (8b4f052) ❌ | latest (v1.26.0) ❌main (8b4f052): latest (v1.26.0): |
✅ Integration test reportCommit: 7a33789
Top 6 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 2711bbe. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR stops databricks aitools install from showing its scope select, agent picker and confirm when a coding agent runs it in a pseudo-terminal, so agent runs get the non-interactive defaults instead of hanging. The fix is small and correct for install, and all CI checks pass. Nothing is blocking. The main suggestions are to test the real agent detection through the environment (isolateAgents) instead of a stubbed seam and package-wide TestMain, and to keep the funnel's interactive field in line with the new behavior. Two scope questions are open: whether uninstall, update and apps init should get the same treatment, and what an agent caller should get by default.
Questions
- This makes
installskip prompts for agent callers, butaitools uninstall(theProceed?confirm, uninstall.go:70),update/uninstallwith both scopes installed (scope.go:207 and 257) and theapps initskills prompt (libs/aitools/agents/recommend.go:25) still check onlycmdio.IsPromptSupported, so an agent in a pseudo-terminal can still hang on them. Is install-only the intended scope? If so, could the PR description say so? Or should the check move into a shared helper (or intocmdio's prompt capability) so these prompts behave the same way? - With no
--agents, an agent caller in a pseudo-terminal now gets every detected agent (config dir or CLI on PATH) installed at global scope, with no plan summary or confirm. Agent detection reads inherited env vars, including the genericAGENT/AI_AGENT, so a person in a shell started from an agent session also loses the picker without explanation. Did you consider defaulting to just the calling agent (agents.CallingAgent()), or printing a short note saying why no prompt was shown and pointing to--scope/--agents? Or is installing for every detected agent the intended behavior here?
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md: The description uses## Summary/## Evidence/## Merge Danger, but.agents/skills/pr-checklist/SKILL.mdasks for the.github/PULL_REQUEST_TEMPLATE.mdheadings (## Changes,## Why,## Tests) in that order and names## Summaryas one not to add. The content maps directly: Summary to Changes/Why, Evidence to Tests, and the Merge Danger notes fold into Why.
| origAgent := callerIsAgent | ||
| t.Cleanup(func() { callerIsAgent = origAgent }) | ||
| callerIsAgent = func() bool { return true } |
There was a problem hiding this comment.
[nice to have] The new test overrides callerIsAgent, and TestMain pins it to false for every other test, so the real check, useragent.AgentProvider() != "" (install.go:30), never runs in a test. A flipped comparison, or a change to agents.CallingAgent() != nil (which returns nil for multiple and vscode-agent), would pass. The pin also silently overrides tests that set an agent env on purpose, such as isolateAgents(t, "CURSOR_AGENT", "1") in funnel_test.go:87, which still runs install as a human. Suggest dropping the seam and main_test.go. Call isolateAgents(t, "VSCODE_AGENT", "1") (or "CURSOR_AGENT", "1") at the start of the new test, and isolateAgents(t) in the existing interactive install tests, the way the interactive funnel tests already do. isolateAgents wraps testutil.SetAgentEnv, which also resets the SDK's agent cache.
There was a problem hiding this comment.
Done in d0c5cc6. Dropped the callerIsAgent seam and main_test.go; installPromptsSupported now calls useragent.AgentProvider() == "" directly. The new test starts with isolateAgents(t, "CURSOR_AGENT", "1"), and TestInstallInteractivePickerAndConfirm / TestInstallInteractivePickerErrorPropagates call isolateAgents(t). Checked that flipping the comparison fails all three tests, and that the package passes when run with CLAUDECODE/AI_AGENT set.
| funnel.step(ctx, protos.AitoolsCommandStepFlagsValidated) | ||
|
|
||
| scope, err := resolveScopeWithPrompt(ctx, projectFlag, globalFlag) | ||
| interactive := installPromptsSupported(ctx) |
There was a problem hiding this comment.
[nice to have] An agent caller in a PTY now takes install's non-interactive path, but startFunnel still records interactive: cmdio.IsPromptSupported(ctx) (telemetry.go:311). Those runs show up as interactive installs with a PLAN_CONFIRMED step and no confirm, which skews interactive vs. non-interactive funnel numbers such as picker drop-off and cancel rates. Either record installPromptsSupported(ctx) for install (it does not depend on flags, so it can be computed before startFunnel), or update the proto comment at aitools_events.go:133 to say the field ignores agent detection.
There was a problem hiding this comment.
Done in d0c5cc6. startFunnel takes interactive as a parameter now. Install passes installPromptsSupported(ctx), computed before startFunnel, and the other subcommands still pass cmdio.IsPromptSupported. The new test asserts that the STARTED event has Interactive == false for an agent caller.
| // terminal) where stdin looks like a TTY but nobody can answer a TUI prompt, so | ||
| // an agent caller always gets the documented non-interactive defaults. | ||
| func installPromptsSupported(ctx context.Context) bool { | ||
| return cmdio.IsPromptSupported(ctx) && !callerIsAgent() |
There was a problem hiding this comment.
[nit] uninstall.go:67-69 justifies its prompt rule as "matching how install only prompts on a TTY". That stops being true with this change: install now also skips prompts for agent callers on a TTY. Please update that comment, whichever way the uninstall/update prompts end up being handled.
There was a problem hiding this comment.
Done in d0c5cc6. I removed the "matching how install only prompts on a TTY" clause, so the comment now just says non-interactive runs proceed without a prompt.
| // stubLegacyCleanup skips the raw-skill cleanup that follows a plugin install; | ||
| // the test agents have no config dir for it to inspect. | ||
| func stubLegacyCleanup(t *testing.T) { | ||
| t.Helper() | ||
| orig := cleanupLegacyFn | ||
| t.Cleanup(func() { cleanupLegacyFn = orig }) | ||
| cleanupLegacyFn = func(context.Context, *agents.Agent, string) error { return nil } | ||
| } |
There was a problem hiding this comment.
[nit] The comment on stubLegacyCleanup says the test agents have no config dir, but setupTestAgents creates ~/.claude and ~/.cursor (install_test.go:129-130). The real RemoveLegacyRawSkills only removes entries recorded in the temp HOME's install state, and a failure is only logged at debug level (install.go:623). TestInstallPluginFirstDefault takes the same non-interactive path without a stub. Consider dropping the helper and its call. If it is needed, fix the comment to give the real reason.
There was a problem hiding this comment.
Removed in d0c5cc6. You were right: the test passes without it, because cleanup only removes what the temp HOME's install state recorded.
| origPrompt := promptAgentSelection | ||
| t.Cleanup(func() { promptAgentSelection = origPrompt }) | ||
| promptAgentSelection = func(context.Context, []agentChoice) ([]*agents.Agent, error) { | ||
| t.Fatal("agent picker shown to an agent caller") | ||
| return nil, nil |
There was a problem hiding this comment.
[nit] The test fails fast if the scope prompt or the agent picker runs, but promptProceed stays the real huh.NewConfirm().Run(), which reads the real terminal. If the confirm gate at install.go:241 regressed alone, the test would hit the real widget and fail with an opaque prompt error (or block) instead of naming the regression. Stub promptProceed to t.Fatal("plan confirm shown to an agent caller") too.
There was a problem hiding this comment.
Done in d0c5cc6. The test now stubs promptProceed to t.Fatal("plan confirm shown to an agent caller").
| // installPromptsSupported reports whether install may show its prompts. Agents | ||
| // often run commands in a pseudo-terminal (Cursor, Copilot in the VS Code | ||
| // terminal) where stdin looks like a TTY but nobody can answer a TUI prompt, so | ||
| // an agent caller always gets the documented non-interactive defaults. |
There was a problem hiding this comment.
[nit] .agents/rules/comments.md asks for source reference URLs when detecting environment variables, and the new detection cites none. Please link the SDK's detection list next to callerIsAgent/installPromptsSupported, as libs/aitools/agents/calling.go and hint.go do: https://github.com/databricks/databricks-sdk-go/blob/main/useragent/agent.go. The link also documents which agents are covered; for example, Copilot in VS Code is detected only via VSCODE_AGENT, which VS Code 1.121+ sets.
There was a problem hiding this comment.
Done in d0c5cc6. The installPromptsSupported comment now links the SDK's agent detection list (useragent/agent.go in databricks-sdk-go) as the source of the agent env vars.
2c28258 to
4a70459
Compare
2711bbe to
95f78a5
Compare
4a70459 to
c162e8e
Compare
95f78a5 to
668167b
Compare
c162e8e to
0c0d208
Compare
eef5046 to
d0c5cc6
Compare
|
On the PR template nit: I'm keeping the Summary / Evidence / Merge Danger layout. Every PR in this stack uses it, so the stack reads the same from one PR to the next, and Evidence carries the before/after test output that |
873e0e0 to
eabdd30
Compare
d0c5cc6 to
2e61f20
Compare
|
Answers to the two questions in the review, as of 2e61f20:
Also in this push: a rebase onto the updated #7476. Install now validates flags before the scope prompt, and |
eabdd30 to
e158591
Compare
2e61f20 to
6251610
Compare
e158591 to
f6b4ec8
Compare
6251610 to
bc7b149
Compare
3108ec5 to
38d9620
Compare
ab35f36 to
a475e72
Compare
|
Answers to the two questions in the second review, as of a475e72:
On the PR template nit, the format stays as I explained in my earlier comment. |
38d9620 to
6f93818
Compare
a475e72 to
a1aa2c3
Compare
6f93818 to
71dc5e6
Compare
a1aa2c3 to
4f96c90
Compare
055b9a6 to
d677ec3
Compare
4f96c90 to
a4e1e52
Compare
d677ec3 to
149851e
Compare
a4e1e52 to
42b3163
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 42b3163. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 3 open design questions.
This PR makes databricks aitools install skip its scope select, agent picker and confirm when a coding agent runs it in a pseudo-terminal. Instead it uses the non-interactive defaults and prints a stderr note naming the agent and the missing --scope/--agents flags. The change is small, every earlier review comment is addressed in the code, the aitools tests pass with and without an agent environment, and CI is green. Three small nits remain (the agent name shown in the note, the note's advice under the deprecated skills install alias, and a test-helper goroutine leak), plus three questions about scope, agent-caller defaults and the telemetry field.
Questions
- You've said install-only is deliberate and that uninstall, update and the
apps initskills prompt will follow with a shared helper. Is there an issue tracking that follow-up? Until it lands, an agent in a pseudo-terminal can still block on uninstall'sProceed?confirm (cmd/aitools/uninstall.go:83) and on the both-scopes prompts (cmd/aitools/scope.go:215,:265). Could you link the issue in the description? Or would you rather move the agent check into a shared place, such ascmdio's prompt capability, in this PR? - On a terminal, an agent caller now installs into every detected agent at global scope with no plan summary. For example, the new test has a Cursor caller install the Claude Code plugin. A person whose shell inherited
AGENT,AI_AGENTorKIROgets the same, and can only get the picker back by unsetting the variable. You've described this as the intended trade-off. Should the follow-up add a way to turn prompts back on, or limit the default to the calling agent? Or is the stderr note the intended end state? - This follows an earlier review suggestion, but install's
interactivetelemetry field now reads false for an agent on a terminal. Update and uninstall still report the terminal state, and older CLI versions reported true for the same setup. From this field alone, install funnel queries can no longer tell 'no terminal' apart from 'agent on a terminal'. Is that acceptable? Or would you prefer to keepinteractiveas the terminal state and record agent detection in a separate field?
| // because the caller is a coding agent. It names the detected agent and the | ||
| // flags that would have replaced the skipped prompts. | ||
| func agentCallerNote(missingFlags []string) string { | ||
| return fmt.Sprintf("No prompts shown because a coding agent (%s) was detected. Pass %s to choose what to install.", useragent.AgentProvider(), strings.Join(missingFlags, " and ")) |
There was a problem hiding this comment.
[nit] The note prints the raw SDK product name from useragent.AgentProvider() right before "Pass --agents", and for several agents that name is not a valid --agents value. Copilot CLI shows (copilot-cli) and Gemini CLI (gemini-cli), whose registry names are copilot and gemini (libs/aitools/agents/calling.go:16-19). VS Code shows (vscode-agent), which has no registry entry, and nested agents show (multiple), which names nothing. An agent that retries with --agents copilot-cli gets unknown agent "copilot-cli". Consider naming the registry agent from agents.AgentForProvider when there is one and saying "more than one coding agent" for multiple. A COPILOT_CLI=1 test case would cover it.
There was a problem hiding this comment.
Done in d6c9440. The note now maps the provider through agents.AgentForProvider and shows the registry name, which is a valid --agents value: COPILOT_CLI=1 shows (copilot). A provider with no registry entry, such as vscode-agent, gets no name ("a coding agent was detected"), and multiple reads "more than one coding agent was detected". TestInstallAgentCallerNoteNamesAgent covers all three cases.
| missing = append(missing, "--agents") | ||
| } | ||
| if len(missing) > 0 { | ||
| cmdio.LogString(ctx, agentCallerNote(missing)) |
There was a problem hiding this comment.
[nit] The deprecated databricks experimental aitools skills install alias also runs NewInstallCmd (cmd/aitools/legacy_skills.go:45-58), but it accepts only [skill-name] and --experimental. An agent on a terminal that runs the alias now sees "Pass --scope and --agents to choose what to install.", and retrying the alias with --scope global fails with unknown flag: --scope. The deprecation warning does point at databricks aitools install, so this is minor. Naming the command in the note (for example "Pass --scope and --agents to databricks aitools install ...") makes the advice correct from both entry points.
There was a problem hiding this comment.
Done in d6c9440. The note now names the command: "Pass --scope and --agents to databricks aitools install to choose what to install." That advice is correct from both databricks aitools install and the deprecated experimental aitools skills install alias.
| t.Helper() | ||
| ctx, test := cmdio.SetupTest(t.Context(), cmdio.TestOptions{PromptSupported: promptSupported}) | ||
| go drainReader(test.Stdout) | ||
| stderr := make(chan []byte) |
There was a problem hiding this comment.
[nit] runInstallWithStderr sends on an unbuffered channel, and the only receive comes after require.NoError(t, cmd.Execute()). If install fails, or one of the t.Fatal prompt stubs fires, the test exits before the receive. The reader goroutine then blocks on stderr <- b forever, and the captured stderr is never shown. Use a buffered channel (make(chan []byte, 1)). Also consider calling test.Done() and reading stderr before checking the error, so a failure can print what install wrote.
There was a problem hiding this comment.
Done in d6c9440. The channel is buffered (make(chan []byte, 1)). The helper now calls test.Done() and reads stderr before checking the error, and require.NoError prints the captured stderr when install fails.
42b3163 to
d6c9440
Compare
|
Answers to the three questions in the third review, as of d6c9440:
|
149851e to
4f48110
Compare
d6c9440 to
914a32c
Compare
4f48110 to
538afb9
Compare
914a32c to
7577e8d
Compare
538afb9 to
c9b6d13
Compare
7577e8d to
2f670b2
Compare
c9b6d13 to
6731eb1
Compare
2f670b2 to
c2d7335
Compare
6731eb1 to
a7cd2a4
Compare
c2d7335 to
8c2aca6
Compare
a7cd2a4 to
d0cb9df
Compare
Coding agents often run commands in a pseudo-terminal (Cursor, Copilot in the VS Code terminal). Stdin looks like a TTY, so `databricks aitools install` showed its scope select and agent picker, and nobody was there to answer them. The run blocked. Now a caller that `useragent.AgentProvider()` identifies as an agent always gets the non-interactive defaults: global scope and every detected agent. On a terminal, when a prompt would otherwise have run, it prints a note to stderr that names the detected agent by its `--agents` value (or says more than one was detected) and the missing `--scope`/`--agents` flags of `databricks aitools install`, for example `No prompts shown because a coding agent (cursor) was detected. Pass --scope and --agents to databricks aitools install to choose what to install.` An agent env var inherited from an agent session can reach a person's shell, and the note tells them why the picker is gone. The change is scoped to install. These prompts still check only `cmdio.IsPromptSupported` and are left for a follow-up: the `Proceed?` confirm of `uninstall`, the both-scopes prompts of `update` and `uninstall`, and the `apps init` skills prompt. ```diff install RunE + interactive := cmdio.IsPromptSupported(ctx) && useragent.AgentProvider() == "" - resolveScopeWithPrompt(ctx) // prompted on any TTY + resolveScopeWithPrompt(ctx, interactive) - selectAgents(ctx, scope, skillsOnly) // picker on any TTY + selectAgents(ctx, scope, skillsOnly, interactive) - if !explicit && cmdio.IsPromptSupported(ctx) // plan summary + confirm + if !explicit && interactive ``` - The `install` help text now describes the non-interactive defaults. - The install funnel records `interactive` from the same check, so an agent caller's run is reported as non-interactive. - The interactive install tests clear the agent env with `isolateAgents`, so they keep prompting when the test binary runs under an agent (`CLAUDECODE`, `AI_AGENT`, ...). - Changelog fragment: `.nextchanges/cli/aitools-install-no-prompts-for-agents.md`. Co-authored-by: Isaac <no-reply@databricks.com>
8c2aca6 to
7a33789
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Approval from simonfaltum's review agent at 7a33789, 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 (42b3163 → 7a33789) address its comments.
Stacked on #7476.
Stacked on: aitools: accept a Windows shim probe that printed help but exited non-zero
Summary
Coding agents often run commands in a pseudo-terminal (Cursor, Copilot in the VS Code terminal). Stdin looks like a TTY, so
databricks aitools installshowed its scope select and agent picker, and nobody was there to answer them. The run blocked.Now a caller that
useragent.AgentProvider()identifies as an agent always gets the non-interactive defaults: global scope and every detected agent.On a terminal, when a prompt would otherwise have run, it prints a note to stderr that names the detected agent by its
--agentsvalue (or says more than one was detected) and the missing--scope/--agentsflags ofdatabricks aitools install, for exampleNo prompts shown because a coding agent (cursor) was detected. Pass --scope and --agents to databricks aitools install to choose what to install.An agent env var inherited from an agent session can reach a person's shell, and the note tells them why the picker is gone.The change is scoped to install. Follow-up scope, not in this PR: these prompts still check only
cmdio.IsPromptSupported, so an agent in a pseudo-terminal can still block on them.Proceed?confirm ofaitools uninstallaitools updateandaitools uninstallapps initskills promptinstallhelp text now describes the non-interactive defaults.interactivefrom the same check, so an agent caller's run is reported as non-interactive.isolateAgents, so they keep prompting when the test binary runs under an agent (CLAUDECODE,AI_AGENT, ...). The agent-caller test setsCURSOR_AGENT=1and fails if the scope prompt, the picker or the plan confirm runs. It also checks that the note is printed on stderr. Further tests check that the note lists only the missing flags, shows the--agentsvalue for the detected agent (copilotforCOPILOT_CLI, no name forvscode-agent, "more than one" for nested agents), and stays silent when both flags are passed or there is no terminal..nextchanges/cli/aitools-install-no-prompts-for-agents.md.Evidence
TestInstallAgentCallerInPTYNeverPrompts: prompts are supported, the caller is an agent, and the scope and picker prompts fail the test if they are called.installPromptsSupportedreverted locally tocmdio.IsPromptSupported(ctx)):Build, unit and acceptance tests for the aitools packages, lint, fmt and
go vetfor GOOS=windows/darwin pass on this branch. Nothing was run on a real Windows or macOS machine.Merge Danger
Door: two-way
Reverting is one small commit. Nothing persistent changes.
Blast Radius: small
CLAUDECODE,AI_AGENT), a person typingdatabricks aitools installin a terminal no longer sees prompts. The run installs for every detected agent in global scope and prints a note explaining why. To pick, pass--scope/--agents.useragent.AgentProvider(). Agents it does not recognize still get prompts on a TTY.This pull request and its description were written by Isaac.