From 651f4c6fcd231d136703867fd53766b89b2871a4 Mon Sep 17 00:00:00 2001 From: Anton Nekipelov <226657+anton-107@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:05:18 +0000 Subject: [PATCH 1/3] Split the SSH extension telemetry category by failure outcome CheckIDESSHExtension reports four distinct outcomes as the single IDE_SSH_EXTENSION_MISSING category: the --list-extensions call failing, a declined prompt, no way to ask for consent, and the install itself being rejected. That category is now the largest source of failed IDE-mode tunnels, and the four call for different fixes -- a list failure means the check never ran, while a rejected install points at the marketplace or a policy blocking it -- so one value cannot tell us which to fix. Give each outcome its own category, attributed through errors.Is sentinels rather than message matching. Only LIST_FAILED and INSTALL_FAILED are reachable under --auto-approve, which the VS Code extension's tunnel button always passes, so the split also separates button traffic from direct CLI use. IDE_SSH_EXTENSION_MISSING is retired rather than reused: rows written before this change still carry it, so a query spanning the release has to accept both. Co-authored-by: Isaac --- experimental/ssh/internal/client/client.go | 22 ++++++- .../internal/client/client_internal_test.go | 45 +++++++++++++ experimental/ssh/internal/vscode/run.go | 28 ++++++-- experimental/ssh/internal/vscode/run_test.go | 66 +++++++++++++++++++ libs/telemetry/protos/ssh_tunnel.go | 23 ++++++- 5 files changed, 174 insertions(+), 10 deletions(-) diff --git a/experimental/ssh/internal/client/client.go b/experimental/ssh/internal/client/client.go index 031bbd4d7af..fcf0fc8ce6f 100644 --- a/experimental/ssh/internal/client/client.go +++ b/experimental/ssh/internal/client/client.go @@ -298,7 +298,7 @@ func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOpt return err } if err := vscode.CheckIDESSHExtension(ctx, opts.IDE, opts.AutoApprove); err != nil { - outcome.errorCategory = protos.SshTunnelErrorCategoryIDESSHExtensionMissing + outcome.errorCategory = sshExtensionErrorCategory(err) return err } } @@ -1247,6 +1247,26 @@ type connectOutcome struct { err error } +// sshExtensionErrorCategory attributes a Remote SSH extension check failure to the outcome that +// caused it. The four are kept apart because they imply different fixes, and because only the +// first two can occur under --auto-approve, which the IDE button always passes -- so a shift +// between them and the consent outcomes distinguishes button traffic from direct CLI use. +func sshExtensionErrorCategory(err error) protos.SshTunnelErrorCategory { + switch { + case errors.Is(err, vscode.ErrSSHExtensionListFailed): + return protos.SshTunnelErrorCategoryIDESSHExtensionListFailed + case errors.Is(err, vscode.ErrSSHExtensionInstallFailed): + return protos.SshTunnelErrorCategoryIDESSHExtensionInstallFailed + case errors.Is(err, vscode.ErrSSHExtensionInstallDeclined): + return protos.SshTunnelErrorCategoryIDESSHExtensionInstallDeclined + case errors.Is(err, vscode.ErrSSHExtensionInstallUnavailable): + return protos.SshTunnelErrorCategoryIDESSHExtensionInstallUnavailable + } + // CheckIDESSHExtension wraps a sentinel on every failure path, so this is only reachable if + // a new one is added without a category. UNKNOWN keeps it countable; see category() below. + return protos.SshTunnelErrorCategoryUnknown +} + // category returns the error category to report. A cancelled context means the user // interrupted the attempt, whichever call happened to observe it first, so it wins over the // category recorded at the failure site. An unattributed failure is reported as UNKNOWN so diff --git a/experimental/ssh/internal/client/client_internal_test.go b/experimental/ssh/internal/client/client_internal_test.go index cb47cd71bb8..7f64774c783 100644 --- a/experimental/ssh/internal/client/client_internal_test.go +++ b/experimental/ssh/internal/client/client_internal_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/databricks/cli/experimental/ssh/internal/sshconfig" + "github.com/databricks/cli/experimental/ssh/internal/vscode" "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/telemetry/protos" "github.com/databricks/databricks-sdk-go/experimental/mocks" @@ -558,6 +559,50 @@ func TestConnectOutcomeCategory(t *testing.T) { } } +// The four Remote SSH extension outcomes were reported as one category until they were split, +// which left the largest IDE-mode failure bucket unattributable. Pin the mapping, including the +// wrapping, since CheckIDESSHExtension returns its sentinels wrapped in a message. +func TestSshExtensionErrorCategory(t *testing.T) { + tests := []struct { + name string + err error + want protos.SshTunnelErrorCategory + }{ + { + name: "list failure", + err: fmt.Errorf("%w in VS Code: %w", vscode.ErrSSHExtensionListFailed, errors.New("exit 4")), + want: protos.SshTunnelErrorCategoryIDESSHExtensionListFailed, + }, + { + name: "install failure", + err: fmt.Errorf("%w: %w", vscode.ErrSSHExtensionInstallFailed, errors.New("exit 3")), + want: protos.SshTunnelErrorCategoryIDESSHExtensionInstallFailed, + }, + { + name: "user declined the install", + err: fmt.Errorf("%w: install it with ...", vscode.ErrSSHExtensionInstallDeclined), + want: protos.SshTunnelErrorCategoryIDESSHExtensionInstallDeclined, + }, + { + name: "no way to ask for consent", + err: fmt.Errorf("%w: install it with ...", vscode.ErrSSHExtensionInstallUnavailable), + want: protos.SshTunnelErrorCategoryIDESSHExtensionInstallUnavailable, + }, + { + // Only reachable if a new failure path forgets its sentinel. + name: "unsentinelled failure falls back to UNKNOWN", + err: errors.New("something else"), + want: protos.SshTunnelErrorCategoryUnknown, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sshExtensionErrorCategory(tt.err)) + }) + } +} + func TestBuildSshTunnelEventReportsErrorCategory(t *testing.T) { got := buildSshTunnelEvent(ClientOptions{ConnectionName: "my-conn", IDE: "vscode"}, connectOutcome{ errorCategory: protos.SshTunnelErrorCategoryIDECommandNotOnPath, diff --git a/experimental/ssh/internal/vscode/run.go b/experimental/ssh/internal/vscode/run.go index db52cff9f5d..08e66f64b02 100644 --- a/experimental/ssh/internal/vscode/run.go +++ b/experimental/ssh/internal/vscode/run.go @@ -2,6 +2,7 @@ package vscode import ( "context" + "errors" "fmt" "os" "os/exec" @@ -118,15 +119,28 @@ func isExtensionVersionAtLeast(version, minVersion string) bool { return semver.IsValid(v) && semver.Compare(v, "v"+minVersion) >= 0 } +// The ways CheckIDESSHExtension can fail. Callers match these with errors.Is to attribute a +// failure without matching on message text. They are separate because they call for different +// fixes: a list failure means the check never ran, an install failure points at the marketplace +// or a policy blocking it, and the two consent outcomes cannot happen under --auto-approve. +var ( + ErrSSHExtensionListFailed = errors.New("could not list installed extensions") + ErrSSHExtensionInstallFailed = errors.New("could not install the Remote SSH extension") + ErrSSHExtensionInstallDeclined = errors.New("install of the Remote SSH extension declined") + ErrSSHExtensionInstallUnavailable = errors.New("cannot prompt to install the Remote SSH extension") +) + // CheckIDESSHExtension verifies that the required Remote SSH extension is installed // with a compatible version, and offers to install/update it if not. // When autoApprove is true, the extension is installed without asking. +// +// Every returned error wraps one of the Err* sentinels above. func CheckIDESSHExtension(ctx context.Context, option string, autoApprove bool) error { ide := getIDE(option) out, err := exec.CommandContext(ctx, ide.Command, "--list-extensions", "--show-versions").Output() if err != nil { - return fmt.Errorf("failed to list %s extensions: %w", ide.Name, err) + return fmt.Errorf("%w in %s: %w", ErrSSHExtensionListFailed, ide.Name, err) } version, found := parseExtensionVersion(string(out), ide.SSHExtensionID) @@ -144,17 +158,17 @@ func CheckIDESSHExtension(ctx context.Context, option string, autoApprove bool) if !autoApprove { if !cmdio.IsPromptSupported(ctx) { - return fmt.Errorf("%s Install it with: %s --install-extension %s, or pass --auto-approve", - msg, ide.Command, ide.SSHExtensionID) + return fmt.Errorf("%w: %s Install it with: %s --install-extension %s, or pass --auto-approve", + ErrSSHExtensionInstallUnavailable, msg, ide.Command, ide.SSHExtensionID) } shouldInstall, err := cmdio.AskYesOrNo(ctx, msg+" Would you like to install it?") if err != nil { - return fmt.Errorf("failed to prompt user: %w", err) + return fmt.Errorf("%w: %w", ErrSSHExtensionInstallUnavailable, err) } if !shouldInstall { - return fmt.Errorf("%s Install it with: %s --install-extension %s", - msg, ide.Command, ide.SSHExtensionID) + return fmt.Errorf("%w: %s Install it with: %s --install-extension %s", + ErrSSHExtensionInstallDeclined, msg, ide.Command, ide.SSHExtensionID) } } else { cmdio.LogString(ctx, msg+" Installing automatically (--auto-approve).") @@ -165,7 +179,7 @@ func CheckIDESSHExtension(ctx context.Context, option string, autoApprove bool) installCmd.Stdout = os.Stdout installCmd.Stderr = os.Stderr if err := installCmd.Run(); err != nil { - return fmt.Errorf("failed to install extension %q: %w", ide.SSHExtensionName, err) + return fmt.Errorf("%w in %s: %w", ErrSSHExtensionInstallFailed, ide.Name, err) } return nil } diff --git a/experimental/ssh/internal/vscode/run_test.go b/experimental/ssh/internal/vscode/run_test.go index 2a33b7b3828..d17385a1ce4 100644 --- a/experimental/ssh/internal/vscode/run_test.go +++ b/experimental/ssh/internal/vscode/run_test.go @@ -232,6 +232,37 @@ func createFakeIDEExecutable(t *testing.T, dir, command, output string) { } } +// createFailingIDEExecutable writes a fake IDE command that exits non-zero for every +// invocation, so "--list-extensions" fails and the check never learns what is installed. +func createFailingIDEExecutable(t *testing.T, dir, command string) { + t.Helper() + if runtime.GOOS == "windows" { + err := os.WriteFile(filepath.Join(dir, command+".cmd"), []byte("@echo off\nexit /b 4\n"), 0o755) + require.NoError(t, err) + } else { + err := os.WriteFile(filepath.Join(dir, command), []byte("#!/bin/sh\nexit 4\n"), 0o755) + require.NoError(t, err) + } +} + +// createIDEExecutableFailingInstall writes a fake IDE command that lists extensions +// successfully but rejects "--install-extension", as a marketplace or policy block would. +func createIDEExecutableFailingInstall(t *testing.T, dir, command, output string) { + t.Helper() + if runtime.GOOS == "windows" { + payloadPath := filepath.Join(dir, command+"-payload.txt") + err := os.WriteFile(payloadPath, []byte(output), 0o644) + require.NoError(t, err) + script := fmt.Sprintf("@echo off\nif \"%%1\"==\"--install-extension\" exit /b 3\ntype \"%s\"\n", payloadPath) + err = os.WriteFile(filepath.Join(dir, command+".cmd"), []byte(script), 0o755) + require.NoError(t, err) + } else { + script := fmt.Sprintf("#!/bin/sh\nfor a in \"$@\"; do\n [ \"$a\" = \"--install-extension\" ] && exit 3\ndone\nprintf '%%s' '%s'\n", output) + err := os.WriteFile(filepath.Join(dir, command), []byte(script), 0o755) + require.NoError(t, err) + } +} + func TestCheckIDESSHExtension_UpToDate(t *testing.T) { tmpDir := t.TempDir() t.Setenv("PATH", tmpDir) @@ -268,6 +299,8 @@ func TestCheckIDESSHExtension_Missing(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), `"Remote - SSH"`) assert.Contains(t, err.Error(), "not installed") + // The test context is not a TTY, so consent cannot be asked for. + assert.ErrorIs(t, err, ErrSSHExtensionInstallUnavailable) } func TestCheckIDESSHExtension_Outdated(t *testing.T) { @@ -282,6 +315,7 @@ func TestCheckIDESSHExtension_Outdated(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "0.100.0") assert.Contains(t, err.Error(), ">= 0.120.0") + assert.ErrorIs(t, err, ErrSSHExtensionInstallUnavailable) } func TestCheckIDESSHExtension_Cursor(t *testing.T) { @@ -319,4 +353,36 @@ func TestCheckIDESSHExtension_NoPrompt_WithoutAutoApprove_Errors(t *testing.T) { err := CheckIDESSHExtension(ctx, VSCodeOption, false) require.Error(t, err) assert.Contains(t, err.Error(), "--install-extension") + assert.ErrorIs(t, err, ErrSSHExtensionInstallUnavailable) +} + +// A command that is on PATH but whose --list-extensions fails is reported separately from a +// missing extension: nothing was learned about what is installed, so it is not an install +// problem. CheckIDECommand passes here, since the command does resolve. +func TestCheckIDESSHExtension_ListFails(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("PATH", tmpDir) + ctx, _ := cmdio.NewTestContextWithStdout(t.Context()) + + createFailingIDEExecutable(t, tmpDir, "code") + + err := CheckIDESSHExtension(ctx, VSCodeOption, true) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSSHExtensionListFailed) + assert.NotErrorIs(t, err, ErrSSHExtensionInstallFailed) +} + +// With --auto-approve there is no prompt, so a missing extension goes straight to an install. +// A rejected install is the one outcome the IDE button can produce on this path. +func TestCheckIDESSHExtension_AutoApprove_InstallFails(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("PATH", tmpDir) + ctx, _ := cmdio.NewTestContextWithStdout(t.Context()) + + createIDEExecutableFailingInstall(t, tmpDir, "code", "ms-python.python@2024.1.1\n") + + err := CheckIDESSHExtension(ctx, VSCodeOption, true) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSSHExtensionInstallFailed) + assert.NotErrorIs(t, err, ErrSSHExtensionInstallUnavailable) } diff --git a/libs/telemetry/protos/ssh_tunnel.go b/libs/telemetry/protos/ssh_tunnel.go index 1c9a5a9e539..76f737dad62 100644 --- a/libs/telemetry/protos/ssh_tunnel.go +++ b/libs/telemetry/protos/ssh_tunnel.go @@ -21,6 +21,10 @@ const ( // The categories name the distinct early-return sites of the connect flow so a failure can // be attributed without logging the error text, which carries cluster names, paths and user // names. +// +// IDE_SSH_EXTENSION_MISSING was retired in favour of the four IDE_SSH_EXTENSION_* categories +// below: it reported all four outcomes as one, and they call for different fixes. Rows written +// before the split still carry it, so a query spanning that release has to accept both. type SshTunnelErrorCategory string const ( @@ -30,8 +34,23 @@ const ( // condition rather than a transient failure, so it is distinguished from the rest. SshTunnelErrorCategoryIDECommandNotOnPath SshTunnelErrorCategory = "IDE_COMMAND_NOT_ON_PATH" - // The required Remote-SSH extension is missing or too old and was not installed. - SshTunnelErrorCategoryIDESSHExtensionMissing SshTunnelErrorCategory = "IDE_SSH_EXTENSION_MISSING" + // The IDE's installed-extension list could not be read, so whether the Remote SSH + // extension was present is unknown. Distinct from the install failures below because it + // says nothing about the extension itself, only that the check could not run. + SshTunnelErrorCategoryIDESSHExtensionListFailed SshTunnelErrorCategory = "IDE_SSH_EXTENSION_LIST_FAILED" + + // The Remote SSH extension was missing or too old, an install was attempted, and the IDE + // rejected it. Points at the marketplace or a policy that forbids the install rather than + // at anything the user chose. + SshTunnelErrorCategoryIDESSHExtensionInstallFailed SshTunnelErrorCategory = "IDE_SSH_EXTENSION_INSTALL_FAILED" + + // The user was asked to install the Remote SSH extension and declined. Unreachable with + // --auto-approve, so absent from IDE-button traffic. + SshTunnelErrorCategoryIDESSHExtensionInstallDeclined SshTunnelErrorCategory = "IDE_SSH_EXTENSION_INSTALL_DECLINED" + + // The Remote SSH extension was missing or too old and consent could not be obtained: no + // --auto-approve and no usable prompt. Also unreachable with --auto-approve. + SshTunnelErrorCategoryIDESSHExtensionInstallUnavailable SshTunnelErrorCategory = "IDE_SSH_EXTENSION_INSTALL_UNAVAILABLE" // IDE settings had to be updated for serverless but the update failed and the user // declined to continue (or --auto-approve turned the failure into an abort). From 1bd44b520af54d18da08013dbe39a48d084ecb84 Mon Sep 17 00:00:00 2001 From: Anton Nekipelov <226657+anton-107@users.noreply.github.com> Date: Fri, 4 Sep 2026 12:59:38 +0000 Subject: [PATCH 2/3] Report an interrupted attempt as USER_ABORTED, keep sentinels out of messages Three fixes from review of the extension-category split: Ctrl-C during the extension check was counted as INSTALL_FAILED or LIST_FAILED. exec.CommandContext kills the child and returns *exec.ExitError, which does not wrap context.Canceled, so category()'s USER_ABORTED branch never fired. Record the connect context's error in the telemetry defer -- registered after `defer cancel()`, so it runs first (LIFO) and sees only genuine cancellation -- and let it win over the site category. Without this, the bucket the split exists to make diagnosable collects users who just gave up. Cover the declined path end to end by driving the real prompt. The mapping test hand-built the sentinel, so nothing checked that CheckIDESSHExtension actually returns ErrSSHExtensionInstallDeclined: swapping it for Unavailable kept both suites green. Keep the consent sentinels out of the message they carry. Prefixed with %w they made the most common path lead with a CLI-internal tag and then restate the problem. consentError renders the message and unwraps to the sentinel, the same shape as bundle/configsync's noMatchingSelectorError, so these two messages are unchanged from before the split. Co-authored-by: Isaac --- experimental/ssh/internal/client/client.go | 19 ++++++++--- .../internal/client/client_internal_test.go | 20 +++++++++++ experimental/ssh/internal/vscode/run.go | 29 +++++++++++++--- experimental/ssh/internal/vscode/run_test.go | 33 ++++++++++++++++++- libs/telemetry/protos/ssh_tunnel.go | 6 ++-- 5 files changed, 95 insertions(+), 12 deletions(-) diff --git a/experimental/ssh/internal/client/client.go b/experimental/ssh/internal/client/client.go index fcf0fc8ce6f..05aa770fe3a 100644 --- a/experimental/ssh/internal/client/client.go +++ b/experimental/ssh/internal/client/client.go @@ -280,6 +280,12 @@ func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOpt outcome := connectOutcome{isReconnect: opts.ServerMetadata != ""} defer func() { outcome.err = retErr + // A cancelled context is the only trace a Ctrl-C leaves: exec.CommandContext kills the + // child and reports *exec.ExitError, which does not wrap context.Canceled, so a step + // that shells out cannot recognise the interruption itself. This defer is registered + // after `defer cancel()` and so runs before it (LIFO), which means ctx is cancelled + // here only by the signal handler or the caller, never by Run's own cleanup. + outcome.interrupted = ctx.Err() != nil logSshTunnelEvent(ctx, opts, outcome) }() @@ -1245,6 +1251,10 @@ type connectOutcome struct { // errorCategory is set at the failure site. Empty means the failure was not attributed. errorCategory protos.SshTunnelErrorCategory err error + // interrupted reports whether the connect context was cancelled, i.e. the user gave up on + // the attempt. Tracked apart from err because a step that shells out reports a killed child + // as *exec.ExitError, which carries no trace of the cancellation. + interrupted bool } // sshExtensionErrorCategory attributes a Remote SSH extension check failure to the outcome that @@ -1267,15 +1277,14 @@ func sshExtensionErrorCategory(err error) protos.SshTunnelErrorCategory { return protos.SshTunnelErrorCategoryUnknown } -// category returns the error category to report. A cancelled context means the user -// interrupted the attempt, whichever call happened to observe it first, so it wins over the -// category recorded at the failure site. An unattributed failure is reported as UNKNOWN so -// that it stays countable. +// category returns the error category to report. An interrupted attempt means the user gave +// up, whichever call happened to observe it first, so it wins over the category recorded at +// the failure site. An unattributed failure is reported as UNKNOWN so that it stays countable. func (o connectOutcome) category() protos.SshTunnelErrorCategory { if o.isSuccess || o.err == nil { return protos.SshTunnelErrorCategoryUnspecified } - if errors.Is(o.err, context.Canceled) { + if o.interrupted || errors.Is(o.err, context.Canceled) { return protos.SshTunnelErrorCategoryUserAborted } if o.errorCategory == "" { diff --git a/experimental/ssh/internal/client/client_internal_test.go b/experimental/ssh/internal/client/client_internal_test.go index 7f64774c783..f883d26df65 100644 --- a/experimental/ssh/internal/client/client_internal_test.go +++ b/experimental/ssh/internal/client/client_internal_test.go @@ -550,6 +550,26 @@ func TestConnectOutcomeCategory(t *testing.T) { }, want: protos.SshTunnelErrorCategoryUserAborted, }, + { + // A step that shells out reports a killed child as *exec.ExitError, which does not + // wrap context.Canceled, so the cancelled context is the only evidence left. Without + // this branch a Ctrl-C during the extension install counts as a rejected install and + // pollutes the bucket that is supposed to mean "the marketplace or a policy blocked + // it" -- one of only two reachable under --auto-approve. + name: "interruption wins over the category set at the failure site", + outcome: connectOutcome{ + interrupted: true, + errorCategory: protos.SshTunnelErrorCategoryIDESSHExtensionInstallFailed, + err: errors.New("signal: killed"), + }, + want: protos.SshTunnelErrorCategoryUserAborted, + }, + { + // Interrupting an established tunnel is not a connection failure. + name: "interruption after a successful connection reports no category", + outcome: connectOutcome{isSuccess: true, interrupted: true, err: errFailed}, + want: protos.SshTunnelErrorCategoryUnspecified, + }, } for _, tt := range tests { diff --git a/experimental/ssh/internal/vscode/run.go b/experimental/ssh/internal/vscode/run.go index 08e66f64b02..188fd48b1a1 100644 --- a/experimental/ssh/internal/vscode/run.go +++ b/experimental/ssh/internal/vscode/run.go @@ -130,6 +130,25 @@ var ( ErrSSHExtensionInstallUnavailable = errors.New("cannot prompt to install the Remote SSH extension") ) +// consentError renders only the message written for the user while still matching its sentinel +// via errors.Is. The two exec failures wrap theirs with %w instead: their message ends in the +// underlying error, so the sentinel reads as a natural prefix. A consent message is a full +// sentence that already states the problem, and prefixing it would lead with an internal tag +// and then restate what follows. +type consentError struct { + msg string + sentinel error +} + +func (e *consentError) Error() string { return e.msg } + +func (e *consentError) Unwrap() error { return e.sentinel } + +// consentErrorf formats a user-facing message and tags it with sentinel. +func consentErrorf(sentinel error, format string, args ...any) error { + return &consentError{msg: fmt.Sprintf(format, args...), sentinel: sentinel} +} + // CheckIDESSHExtension verifies that the required Remote SSH extension is installed // with a compatible version, and offers to install/update it if not. // When autoApprove is true, the extension is installed without asking. @@ -158,8 +177,9 @@ func CheckIDESSHExtension(ctx context.Context, option string, autoApprove bool) if !autoApprove { if !cmdio.IsPromptSupported(ctx) { - return fmt.Errorf("%w: %s Install it with: %s --install-extension %s, or pass --auto-approve", - ErrSSHExtensionInstallUnavailable, msg, ide.Command, ide.SSHExtensionID) + return consentErrorf(ErrSSHExtensionInstallUnavailable, + "%s Install it with: %s --install-extension %s, or pass --auto-approve", + msg, ide.Command, ide.SSHExtensionID) } shouldInstall, err := cmdio.AskYesOrNo(ctx, msg+" Would you like to install it?") @@ -167,8 +187,9 @@ func CheckIDESSHExtension(ctx context.Context, option string, autoApprove bool) return fmt.Errorf("%w: %w", ErrSSHExtensionInstallUnavailable, err) } if !shouldInstall { - return fmt.Errorf("%w: %s Install it with: %s --install-extension %s", - ErrSSHExtensionInstallDeclined, msg, ide.Command, ide.SSHExtensionID) + return consentErrorf(ErrSSHExtensionInstallDeclined, + "%s Install it with: %s --install-extension %s", + msg, ide.Command, ide.SSHExtensionID) } } else { cmdio.LogString(ctx, msg+" Installing automatically (--auto-approve).") diff --git a/experimental/ssh/internal/vscode/run_test.go b/experimental/ssh/internal/vscode/run_test.go index d17385a1ce4..add918950e2 100644 --- a/experimental/ssh/internal/vscode/run_test.go +++ b/experimental/ssh/internal/vscode/run_test.go @@ -2,6 +2,7 @@ package vscode import ( "fmt" + "io" "os" "path/filepath" "runtime" @@ -352,8 +353,38 @@ func TestCheckIDESSHExtension_NoPrompt_WithoutAutoApprove_Errors(t *testing.T) { err := CheckIDESSHExtension(ctx, VSCodeOption, false) require.Error(t, err) - assert.Contains(t, err.Error(), "--install-extension") assert.ErrorIs(t, err, ErrSSHExtensionInstallUnavailable) + // The sentinel is a telemetry tag, so it must stay out of the message: what the user needs + // to read first is the problem, not that the CLI had no way to ask about it. + assert.Equal(t, `Required extension "Remote - SSH" is not installed in VS Code. `+ + "Install it with: code --install-extension ms-vscode-remote.remote-ssh, or pass --auto-approve", + err.Error()) +} + +// Declining at the prompt is the outcome that separates "the user said no" from "there was no +// way to ask", which is the distinction the split exists to make. Drive the real prompt rather +// than asserting against a hand-built error, so the wiring is covered and not just the mapping. +func TestCheckIDESSHExtension_Declined(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("PATH", tmpDir) + ctx, tst := cmdio.SetupTest(t.Context(), cmdio.TestOptions{PromptSupported: true}) + defer tst.Done() + + createFakeIDEExecutable(t, tmpDir, "code", "ms-python.python@2024.1.1\n") + + // Drain stderr, where the prompt is written, or AskYesOrNo blocks on the pipe. + go func() { _, _ = io.Copy(io.Discard, tst.Stderr) }() + go func() { + _, _ = tst.Stdin.WriteString("n\n") + _ = tst.Stdin.Flush() + }() + + err := CheckIDESSHExtension(ctx, VSCodeOption, false) + require.Error(t, err) + assert.ErrorIs(t, err, ErrSSHExtensionInstallDeclined) + assert.NotErrorIs(t, err, ErrSSHExtensionInstallUnavailable) + assert.Equal(t, `Required extension "Remote - SSH" is not installed in VS Code. `+ + "Install it with: code --install-extension ms-vscode-remote.remote-ssh", err.Error()) } // A command that is on PATH but whose --list-extensions fails is reported separately from a diff --git a/libs/telemetry/protos/ssh_tunnel.go b/libs/telemetry/protos/ssh_tunnel.go index 76f737dad62..9fb19a7f702 100644 --- a/libs/telemetry/protos/ssh_tunnel.go +++ b/libs/telemetry/protos/ssh_tunnel.go @@ -41,7 +41,7 @@ const ( // The Remote SSH extension was missing or too old, an install was attempted, and the IDE // rejected it. Points at the marketplace or a policy that forbids the install rather than - // at anything the user chose. + // at anything the user chose: an install the user interrupted lands in USER_ABORTED. SshTunnelErrorCategoryIDESSHExtensionInstallFailed SshTunnelErrorCategory = "IDE_SSH_EXTENSION_INSTALL_FAILED" // The user was asked to install the Remote SSH extension and declined. Unreachable with @@ -75,7 +75,9 @@ const ( // its metadata never appeared before the timeout. SshTunnelErrorCategoryServerStartTimeout SshTunnelErrorCategory = "SERVER_START_TIMEOUT" - // The user interrupted the connection (Ctrl-C or a termination signal). + // The user interrupted the connection (Ctrl-C or a termination signal). Takes precedence + // over the category of whichever step observed the interruption, including steps that shell + // out to the IDE and see only a killed child process. SshTunnelErrorCategoryUserAborted SshTunnelErrorCategory = "USER_ABORTED" // A failure that does not correspond to any of the categories above. The connect path From 0c498db39747610c902c4946c148178e43a5a2a7 Mon Sep 17 00:00:00 2001 From: Anton Nekipelov <226657+anton-107@users.noreply.github.com> Date: Fri, 4 Sep 2026 13:38:54 +0000 Subject: [PATCH 3/3] Match the cancellation cause rather than any context error Follow-up from e2e review of the interrupted-attempt fix. `ctx.Err() != nil` also matches `context.DeadlineExceeded`, so a timeout would have been reported as USER_ABORTED, which means "Ctrl-C or a termination signal". No ancestor of the connect context carries a deadline today -- verified: no WithTimeout or WithDeadline on the command path, and Run never reassigns ctx after its own WithCancel -- so this is unreachable, but the predicate stated the wrong thing and a deadline added upstream later would have poisoned the category silently. Storing the cause instead of a bool also moves the discrimination out of the defer and into category(), where it is unit-testable: the deadline case is a table entry rather than untested glue. Re-verified e2e after the change: the same interrupted extension check on dogfood still reports USER_ABORTED, and an uninterrupted failing list still reports IDE_SSH_EXTENSION_LIST_FAILED. Co-authored-by: Isaac --- experimental/ssh/internal/client/client.go | 13 +++++++------ .../ssh/internal/client/client_internal_test.go | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/experimental/ssh/internal/client/client.go b/experimental/ssh/internal/client/client.go index 05aa770fe3a..97cc5334126 100644 --- a/experimental/ssh/internal/client/client.go +++ b/experimental/ssh/internal/client/client.go @@ -285,7 +285,7 @@ func Run(ctx context.Context, client *databricks.WorkspaceClient, opts ClientOpt // that shells out cannot recognise the interruption itself. This defer is registered // after `defer cancel()` and so runs before it (LIFO), which means ctx is cancelled // here only by the signal handler or the caller, never by Run's own cleanup. - outcome.interrupted = ctx.Err() != nil + outcome.ctxErr = ctx.Err() logSshTunnelEvent(ctx, opts, outcome) }() @@ -1251,10 +1251,11 @@ type connectOutcome struct { // errorCategory is set at the failure site. Empty means the failure was not attributed. errorCategory protos.SshTunnelErrorCategory err error - // interrupted reports whether the connect context was cancelled, i.e. the user gave up on - // the attempt. Tracked apart from err because a step that shells out reports a killed child - // as *exec.ExitError, which carries no trace of the cancellation. - interrupted bool + // ctxErr is the connect context's error when the outcome is logged. Tracked apart from err + // because a step that shells out reports a killed child as *exec.ExitError, which carries no + // trace of the cancellation. Only a cancellation counts as the user giving up: a deadline + // would be a timeout, so category() matches the cause rather than testing for non-nil. + ctxErr error } // sshExtensionErrorCategory attributes a Remote SSH extension check failure to the outcome that @@ -1284,7 +1285,7 @@ func (o connectOutcome) category() protos.SshTunnelErrorCategory { if o.isSuccess || o.err == nil { return protos.SshTunnelErrorCategoryUnspecified } - if o.interrupted || errors.Is(o.err, context.Canceled) { + if errors.Is(o.ctxErr, context.Canceled) || errors.Is(o.err, context.Canceled) { return protos.SshTunnelErrorCategoryUserAborted } if o.errorCategory == "" { diff --git a/experimental/ssh/internal/client/client_internal_test.go b/experimental/ssh/internal/client/client_internal_test.go index f883d26df65..ba15384bb46 100644 --- a/experimental/ssh/internal/client/client_internal_test.go +++ b/experimental/ssh/internal/client/client_internal_test.go @@ -558,16 +558,28 @@ func TestConnectOutcomeCategory(t *testing.T) { // it" -- one of only two reachable under --auto-approve. name: "interruption wins over the category set at the failure site", outcome: connectOutcome{ - interrupted: true, + ctxErr: context.Canceled, errorCategory: protos.SshTunnelErrorCategoryIDESSHExtensionInstallFailed, err: errors.New("signal: killed"), }, want: protos.SshTunnelErrorCategoryUserAborted, }, + { + // Only a cancellation is the user giving up. No ancestor of the connect context + // carries a deadline today, so this is unreachable; matching the cause rather than + // testing ctxErr for non-nil keeps it that way if one is ever added. + name: "an expired deadline is not a user abort", + outcome: connectOutcome{ + ctxErr: context.DeadlineExceeded, + errorCategory: protos.SshTunnelErrorCategoryServerStartTimeout, + err: errFailed, + }, + want: protos.SshTunnelErrorCategoryServerStartTimeout, + }, { // Interrupting an established tunnel is not a connection failure. name: "interruption after a successful connection reports no category", - outcome: connectOutcome{isSuccess: true, interrupted: true, err: errFailed}, + outcome: connectOutcome{isSuccess: true, ctxErr: context.Canceled, err: errFailed}, want: protos.SshTunnelErrorCategoryUnspecified, }, }