Skip to content

feat(ssh): pre-check the host CLI before starting the SSH tunnel - #2127

Merged
misha-db merged 9 commits into
mainfrom
fix/ssh-tunnel-host-cli-precheck
Aug 21, 2026
Merged

feat(ssh): pre-check the host CLI before starting the SSH tunnel#2127
misha-db merged 9 commits into
mainfrom
fix/ssh-tunnel-host-cli-precheck

Conversation

@misha-db

Copy link
Copy Markdown
Contributor

Changes

databricks ssh connect shells out to the host editor's shell command (code/cursor) to open the remote window. When that command is off PATH, the connect only failed deep inside the terminal after a long wait. Fail fast instead: pre-check the command before touching auth, compute, or a terminal, and surface an actionable prompt whose button installs the shell command (falling back to the editor's download page on hosts without the built-in installer).

Adds HostUtils.getHostCliCommand/isHostCliOnPath to resolve and probe the command through a shell so PATH resolves as the terminal would.

Tests

hostUtils.ts

`databricks ssh connect` shells out to the host editor's shell command
(`code`/`cursor`) to open the remote window. When that command is off
PATH, the connect only failed deep inside the terminal after a long
wait. Fail fast instead: pre-check the command before touching auth,
compute, or a terminal, and surface an actionable prompt whose button
installs the shell command (falling back to the editor's download page
on hosts without the built-in installer).

Adds HostUtils.getHostCliCommand/isHostCliOnPath to resolve and probe
the command through a shell so PATH resolves as the terminal would.
@misha-db
misha-db deployed to test-trigger-is August 18, 2026 15:00 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 18, 2026 15:01 — with GitHub Actions Active
@rugpanov

rugpanov commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🤖 Integration tests ✅ all 35 test jobs passed for db9341d7.
View run

@rugpanov

Copy link
Copy Markdown
Contributor

Reviewed across several passes (Isaac Review, Codex, a Claude review agent, and a code-conventions check). Isaac and the conventions pass came back clean; the findings below were each verified against the code and/or the VS Code source.

[must-address] The PATH probe doesn't resolve PATH the way the terminal that runs the connect doeshostUtils.ts (isHostCliOnPath)

isHostCliOnPath() probes with cancellableExecFile(cmd, ["--version"], {shell: true}), which runs a non-interactive, non-login shell (/bin/sh -c on POSIX, cmd.exe /c on Windows) using the extension-host process.env. The tunnel itself runs via window.createTerminal({strictEnv: false}) + terminal.sendText(...) in launchSshTunnel, i.e. the user's configured profile shell, which sources ~/.zshrc / ~/.bash_profile / the PowerShell profile and can extend PATH; getSshConnectEnvVars doesn't pin PATH. So the two environments can genuinely differ, and because the pre-check is a hard gate (return in startTunnelCommand), a false negative blocks a tunnel that would actually have worked — worse than today's late-but-visible terminal error. The doc-comment "Runs through a shell so PATH is resolved the same way the terminal would" isn't accurate. (On macOS/Linux VS Code's resolved shell-env narrows this in many setups; it's most likely to bite on Windows, or when a profile adds the CLI dir.) Consider making the check advisory (warn but still proceed), or probe through the same profile shell the terminal will use — and fix the comment either way.

[should-fix] The probe treats every failure as "not on PATH"hostUtils.ts (isHostCliOnPath, the catch { return false })

A permission error, a non-zero --version exit, or a transient spawn failure all resolve to false and surface the same "isn't on your PATH → install it" prompt, and nothing is logged. The repo already has the right tool: execFileWithShell in AzureCliCheck.ts wraps the same cancellableExecFile(..., {shell: true}) but maps ENOENT to FileNotFoundException and rethrows everything else. Reusing it (and logging unexpected errors via Loggers.Extension) would give the correct remediation and make failures diagnosable.

[should-fix] The "Install shell command" button falls back to a re-download on Windows and LinuxSshCommands.ts (ensureHostCliOnPath)

workbench.action.installCommandLine (InstallShellScriptAction) is registered by VS Code only on macOS — it's guarded by if (isMacintosh) in desktop.contribution.ts. On Windows and Linux the command isn't registered, so executeCommand rejects, the catch runs, and the user is sent to the editor's download page for an editor they already have installed. Gate the button per process.platform (or word the fallback to point at PATH-setup docs) so non-macOS users aren't told to reinstall.

[nit] VS Code Insiders picks the wrong command namehostUtils.ts (getHostCliCommand)

getHostCliCommand() returns "code" for anything that isn't Cursor, but Insiders' shell command is code-insiders (its env.uriScheme is vscode-insiders). An Insiders-only install has code-insiders on PATH, not code, so the probe false-negatives there.

[nit] Unhandled rejection in the detached prompt chainSshCommands.ts (ensureHostCliOnPath)

Returning false immediately while the prompt lives on is intentional and fine, but the .then(...) chain has no terminal .catch, and the fallback await commands.executeCommand("vscode.open", ...) inside the installer catch can itself reject. (ESLint here doesn't enable no-floating-promises, so lint won't catch it.) Cheap to wrap.

[nit] The new logic is untestedhostUtils.test.ts

Tests cover only getHostCliCommand(). isHostCliOnPath() (the probe) and ensureHostCliOnPath() (the prompt/branching) — the parts most likely to regress — are uncovered. Injecting the exec function would make the false-negative and install/fallback branches testable.

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 06:58 — with GitHub Actions Active
@rugpanov

Copy link
Copy Markdown
Contributor

🤖 Integration tests triggered for 7be18f04 — ⏳ running.
View run

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 06:59 — with GitHub Actions Active
* elsewhere we point at the editor's PATH-setup docs instead of telling the
* user to reinstall an editor they already have.
*/
private async warnIfHostCliMissing(): Promise<void> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have tests for this, but not blocking since there aren't tests for the rest of this file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 07:24 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 07:25 — with GitHub Actions Active
@anton-107

Copy link
Copy Markdown
Contributor

Re-reviewed at e2545303. The advisory rework addresses the PATH-divergence, error-classification, non-macOS-fallback and floating-promise findings, and isHostCliOnPath is now injectable and covered. Verified the new probe against the real shell:true failure shape — a missing command exits 127 with command not found on stderr, and the SDK's isFileNotFound catches that, so the "definitively missing" path works as documented. Two things below, then this looks good to me.

[should-fix] code-insiders makes the probe and the CLI disagree, so Insiders now warns on every tunnel start

getHostCliCommand() now returns code-insiders under Insiders, but the value passed to the CLI is unchanged — getSshConnectCommand still sends --ide=vscode (CliWrapper.ts:131), and the CLI's getIDE maps anything that isn't cursor to vsCodeIDE{Command: "code"}. Both LaunchIDE and CheckIDECommand then use code. I confirmed the bundled CLI (v1.11.0) has zero occurrences of code-insiders.

So the extension probes for a command the CLI will never invoke. For an Insiders user with only code-insiders on PATH the probe now reports missing and shows the warning on every tunnel start, even though the connect proceeds and the CLI's own check fails on code regardless. Before the change that user got no warning; now they get a permanently wrong one. Since the tunnel can't actually work through Insiders until the CLI grows an Insiders descriptor, probing code (i.e. matching whatever --ide resolves to) is the more accurate behavior — the warning would then correctly point at the command that's really needed. Worth reverting that hunk and fixing it upstream instead.

[nit] isFileNotFound is a deep /dist/... import

hostUtils.ts:3 imports from @databricks/sdk-experimental/dist/config/execUtils, which CODE_CONVENTIONS §5 calls out ("Never import through deep /dist/... paths ... import from the package root"). AzureCliCheck.ts does the same, but per AGENTS.md that's legacy, not precedent. The root re-exports this as a namespace — import {ExecUtils} from "@databricks/sdk-experimental" then ExecUtils.isFileNotFound(e) — which I typechecked against the vendored SDK. FileNotFoundException is there too, if AzureCliCheck ever gets cleaned up.


Two notes on things I checked and am not asking you to change:

  • utils/hostUtils.ts now imports both ../cli/CliWrapper and ../logger, and CliWrapper imports HostUtils back from ../utils — a module cycle, and the first utils → cli edge in the tree. Not a live bug (neither side touches the other at module scope, and esbuild/TS handle it), but it inverts the layering, since utils is otherwise the leaf. The injectable exec parameter means the default argument is the only thing keeping the edge — a caller-supplied exec would remove it entirely. Your call whether that's worth restructuring now.
  • The premise. The CLI already does this check first: client.Run() calls vscode.CheckIDECommand at line 221, right after Connecting to %s... and before the settings check, EnsureClusterIsRunning, and any tunnel setup — so the "long wait" isn't the current CLI's behavior. Now that the gate is advisory and fires up front, the extension warning is a reasonable belt-and-braces in-editor surface regardless, so I don't think this blocks. But if the original repro was on an older CLI, the version bump alone may have been the fix, and it'd be worth a line in the PR description either way.

For completeness: I couldn't run typecheck/lint/unit tests in my checkout — tsconfig_base.json sets ignoreDeprecations: "6.0", which the workspace's pinned TS 5.3.3 rejects, and root ESLint 8.55 can't read the flat config. Both are pre-existing local tooling drift, not from this PR; CI is green on all three. Note the 35-job integration run and the +310-line setup_local.ucws.e2e.ts came in with the merge from main, not from this change — nothing in CI exercises the new code, so green isn't evidence here.

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 08:10 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 08:10 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 08:33 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 08:33 — with GitHub Actions Active
@rugpanov

rugpanov commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🤖 Integration tests ✅ all 37 test jobs passed for 58ebb7d3.
View run

// Insiders ships its shell command as `code-insiders`, not `code`; an
// Insiders-only install has that on PATH.
if (env.uriScheme === "vscode-insiders") {
return "code-insiders";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[must-fix] The probe checks code-insiders, but the CLI always invokes code — so on Insiders the warning fires (or stays silent) for the wrong command.

getHostCliCommand() returns code-insiders for VS Code Insiders, but that's not the command the tunnel actually needs. getSshConnectCommand() (CliWrapper.ts:131) builds the invocation as --ide=${isCursor() ? "cursor" : "vscode"} — there's no Insiders case, so Insiders sends --ide=vscode, and the CLI maps vscode to the literal command code. The bundled CLI (v1.12.1) has no code-insiders descriptor at all.

So the pre-check and the CLI look at two different commands. Concrete failure for a common Insiders-only setup (code-insiders on PATH, code not):

  1. Probe checks code-insiders → present → no warning.
  2. Tunnel starts; CLI invokes code → missing → fails deep in the terminal after a wait.

That's exactly the slow failure this PR is meant to pre-empt, and the user got no heads-up — the check is silent in the case it was designed to catch. (Mirror case: an Insiders user with neither command is told to install code-insiders, which won't help, since the CLI needs code.)

Fix: probe whatever --ide resolves to. Since the CLI maps everything non-Cursor to code, drop the code-insiders branch and return code for both stable and Insiders:

export function getHostCliCommand(): "code" | "cursor" {
    return isCursor() ? "cursor" : "code";
}

Real Insiders support belongs upstream in the CLI (an Insiders descriptor so --ide can mean code-insiders); until then, probing code-insiders just checks for a command the CLI never calls. The getHostCliCommand() test that asserts "code-insiders" should be updated to match.

@@ -1,4 +1,8 @@
import {env} from "vscode";
import {logging} from "@databricks/sdk-experimental";
import {isFileNotFound} from "@databricks/sdk-experimental/dist/config/execUtils";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Deep /dist/... import — CODE_CONVENTIONS §5.

§5 says never to import through deep /dist/... paths — they aren't a stable entry point; import from the package root. (AzureCliCheck.ts does the same, but per AGENTS.md that's a pre-existing legacy case, not precedent for a new one.)

The root re-exports this as the ExecUtils namespace (verified: ExecUtils.isFileNotFound is exported; isFileNotFound is not available directly off the root):

import {ExecUtils} from "@databricks/sdk-experimental";
// ...
if (ExecUtils.isFileNotFound(e)) { ... }

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 14:06 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 14:07 — with GitHub Actions Active
@rugpanov

Copy link
Copy Markdown
Contributor

🤖 Integration tests triggered for 394edc7c — ⏳ running.
View run

@misha-db
misha-db deployed to test-trigger-is August 19, 2026 14:20 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 14:20 — with GitHub Actions Active
@misha-db
misha-db requested a review from rugpanov August 19, 2026 14:24
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 17:20 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 19, 2026 17:21 — with GitHub Actions Active
@rugpanov

rugpanov commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

🤖 Integration tests ❌ 1 of 41 test jobs failed for cfb4a6b0 (40 passed).
View run

@misha-db
misha-db deployed to test-trigger-is August 21, 2026 08:30 — with GitHub Actions Active
@misha-db
misha-db deployed to test-trigger-is August 21, 2026 08:30 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/vscode

Inputs:

  • PR number: 2127
  • Commit SHA: 73c4ef0e7cfca6cdee2faeb5a48bd41b02b11840

Checks will be approved automatically on success.

@rugpanov rugpanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both earlier findings are addressed:

  • getHostCliCommand() now probes code for Insiders (matching the CLI's --ide=vscodecode), test updated accordingly.
  • The isFileNotFound import now comes through the ExecUtils package-root namespace.

Also reviewed the follow-up rework that resolves the probe against the login/interactive shell env via shell-env on POSIX — verified that execFile resolves the bare command against the passed env.PATH, so it correctly sees the terminal's PATH; new branches are covered by tests. LGTM.

Minor, non-blocking: warnIfHostCliMissing() is awaited on the tunnel-start path, so it spawns a login shell via shell-env on every start — a bounded timeout would guard against a slow/hanging profile.

@rugpanov

rugpanov commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

🤖 Integration tests ✅ all 41 test jobs passed for 73c4ef0e.
View run

@misha-db
misha-db merged commit 5af58f2 into main Aug 21, 2026
8 of 9 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 21, 2026
rugpanov added a commit that referenced this pull request Aug 21, 2026
## packages/databricks-vscode
## (2026-08-21)

* Narrate live progress during Python environment setup (#2139)
([f606d0a](f606d0a))
* Pre-check the host CLI before starting the SSH tunnel (#2127)
([5af58f2](5af58f2))
* Show an info toast on a successful Python environment setup that had
warnings (#2138)
([2acdc44](2acdc44))
* Avoid duplicate profile section creation in `.databrickscfg` during
OAuth setup (#2133)
([6e1c472](6e1c472))

## packages/databricks-vscode-types
## (2026-08-21)

---------

Co-authored-by: releasebot <noreply@github.com>
Co-authored-by: Grigory Panov <grigory.panov@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants