Repository navigation
aitools: never prompt a coding agent caller in install - #7477
Conversation
5601cd2 to
2c28258
Compare
3af19e5 to
2711bbe
Compare
✅ Integration test reportCommit: e9edeb2
Top 25 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 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
f6b4ec8 to
3108ec5
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
8c2aca6 to
7a33789
Compare
simonfaltum
left a comment
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.
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| setupAgentCallerInstall(t) | ||
| isolateAgents(t, tc.env...) |
There was a problem hiding this comment.
[nit] setupAgentCallerInstall sets HOME, PATH and DATABRICKS_SKILLS_REF, and then this isolateAgents call clears the whole environment (SetAgentEnv calls os.Clearenv). So each subtest runs with no HOME and no PATH, and no agent is detected. The test passes only because the note prints before agent detection. Pass the agent env pairs to setupAgentCallerInstall, so that isolateAgents runs once and first. Then also assert the install, as TestInstallAgentCallerNoteSilent does with require.Len(t, *plugins, 1).
There was a problem hiding this comment.
Good catch, fixed in 3027224. setupAgentCallerInstall now takes the agent env pairs and calls isolateAgents once, first, so the HOME, PATH and DATABRICKS_SKILLS_REF it sets afterwards survive. TestInstallAgentCallerNoteNamesAgent passes each case's env through the helper, and its cases now also assert the install (require.Len(t, *plugins, 1)). The other callers pass CURSOR_AGENT=1. This is a test-only change; install's behavior is unchanged.
d0cb9df to
1988e05
Compare
7a33789 to
ce44e54
Compare
1988e05 to
735616f
Compare
ce44e54 to
88d00f0
Compare
735616f to
e26c489
Compare
14c2f8d to
3027224
Compare
e26c489 to
b28c0c8
Compare
3027224 to
7586075
Compare
b28c0c8 to
c3363cb
Compare
7586075 to
055533b
Compare
c3363cb to
3daa09d
Compare
055533b to
22ff761
Compare
3daa09d to
7342f80
Compare
22ff761 to
43056d1
Compare
7342f80 to
2326b6b
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>
43056d1 to
e9edeb2
Compare
❌ Integration test reportCommit: db52460
123 interesting tests: 75 FAIL, 36 MISS, 8 RECOVERED, 4 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
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.