Add AI tools integration - #2025
Conversation
|
🤖 Integration tests ❌ 2 of 35 test jobs failed for |
|
🤖 Integration tests ❌ 2 of 35 test jobs failed for |
|
🤖 Integration tests ✅ all 35 test jobs passed for |
|
🤖 Integration tests ✅ all 35 test jobs passed for |
|
🤖 Integration tests ❌ 1 of 35 test jobs failed for |
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
Review notes on the AI tools integration. Two blockers (the CLI version dependency and the empty-agent-selection fallback) plus two medium items, left as inline comments.
On sequencing: the --agents fix is independent of the CLI release and worth landing now. It also shares a root cause with the typing item — both stem from [] being overloaded to mean two different things — so those two go well together.
|
🤖 Integration tests triggered for |
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
🤖 Integration tests triggered for |
rugpanov
left a comment
rugpanov
left a comment
There was a problem hiding this comment.
LGTM — approving. The mutex serialization and the empty-agent-selection guard both look good now, and the test coverage is genuinely thorough (the concurrency test at AiToolsManager.test.ts:755 would actually fail if serialization broke).
Two things I'd like fixed as soon as possible, neither blocking this merge:
- Cancelling an operation is reported as a failure — cosmetically a wrong toast, but it also records
result: "error"telemetry, so every cancellation inflates the AI-tools error rate. Worth landing before the release that enables this for users, since that data can't be cleaned up retroactively. - No re-detect when the active project changes — narrow repro (multi-root, multiple Databricks projects, explicit "Choose a project"), and recoverable via the reload affordance, but the model and the CLI cwd disagree until something re-detects.
Details inline.
| ); | ||
| } catch (e: any) { | ||
| ctx?.logger?.error("Failed to install Databricks AI tools", e); | ||
| throw new ProcessError(e.message, e.code ?? null); |
There was a problem hiding this comment.
Cancelling an AI tools operation is reported to the user as a failure. (Non-blocking, but please fix asap — see below on the telemetry.)
The progress notifications are cancellable: true (AiToolsCommands.ts:65) and cancellation aborts the child process via AbortController (:73). Node rejects an aborted execFile with an AbortError (code: "ABORT_ERR") — confirmed locally:
name=AbortError code=ABORT_ERR killed=undefined signal=undefined msg="The operation was aborted"
This catch treats any rejection as a CLI failure, so cancelling produces a "Failed to install Databricks AI tools." error toast plus result: "error" telemetry (AiToolsManager.ts:474). Same pattern at :653 (update) and :678 (uninstall).
Most visible on the activation-time auto-update, which applies silently but is cancellable — a user who cancels an update they never requested gets an error toast. The toast is cosmetic, but the telemetry isn't: cancellations will inflate the error rate and that can't be cleaned up after the fact, so I'd like this in before the release that turns the feature on for users.
Suggest distinguishing cancellation before wrapping: rethrow a dedicated cancellation error (or check cancellationToken?.isCancellationRequested, the pattern used in ~10 places in this repo, e.g. sdk-extensions/Cluster.ts:226), then have withProgress swallow it and the manager record it as cancelled rather than error.
| } | ||
| } | ||
|
|
||
| private get projectRoot(): string { |
There was a problem hiding this comment.
No re-detect when the active project changes. (Non-blocking — fine as a follow-up.)
AiToolsManager never subscribes to workspaceFolderManager.onDidChangeActiveProjectFolder, but projectRoot (and therefore cwdForScope("project")) reads activeProjectUri live on every call. So after the active project changes, the model's installLocation / version / agents still describe the previous folder while the cwd now points at the new one.
The trigger isn't casual folder switching — setActiveProjectFolder has only two callers: the explicit "Choose a project" flow (databricks.bundle.selectActiveProjectFolder → activeBundleUtils.ts:79) and the fallback when the active folder is removed (WorkspaceFolderManager.ts:59). So the repro is: a workspace with multiple Databricks projects where A has project-scope AI tools and B doesn't; use "Choose a project" to switch to B. The AI tools row still shows installed (project) with A's version, and Update/Uninstall run against B.
Every other collaborator on this event re-syncs on it — BundleProjectManager.ts:58, BundleWatcher.ts:43, BundleFileSet.ts:104, ConnectionManager.ts:243, WorkspaceFolderComponent.ts:12. Suggest the same here: subscribe in the constructor and call detectInstall() on change (it already reconciles the model and the when-context).
No test covers a project switch, so the current suite wouldn't catch this.
3 participants
Changes
Integrate the
databricks aitoolsfamily of commands into the extension.AI toolsrow to the configuration paneTests
Added tests pass