fix(sagemaker): improve error handling when the Space SSH host check fails

[bhavya2109sharma]

Problem

When attempting to connect to a SageMaker Space, if the user's SSH config had issues (like the old User '%r' directive),
users would see an unhelpful error :

ssh check against host failed: 255

This happened because:

1. SSH validation ran before checking if the config was outdated
2. The outdated config caused SSH validation to fail
3. Users had no opportunity to fix the issue before seeing the error

Solution

Implemented a proactive SSH config validation that checks for outdated config before running SSH validation
Prompts users to update outdated config automatically
Provides helpful error messages with line numbers for external errors
Validates before showing progress dialogs (better UX)

Changes Made

New Files Created

1. packages/core/src/awsService/sagemaker/sshConfig.ts : Created SageMakerSshConfig class that extends the base SshConfig class with SageMaker specific SSH logic.
2. packages/core/src/test/awsService/sagemaker/sshConfig.test.ts- test suite

Modified

1. packages/core/src/awsService/sagemaker/model.ts: Removed ensureValid() from prepareDevEnvConnection() and added comment explaining validation happens in commands.ts

2. packages/core/src/awsService/sagemaker/commands.ts : Added validateSshConfig() which will validate before showing progress dialogs so that users can fix issues before any connection attempt starts

Testing

1. SSH Config Exists, No sm_* Section
2. Happy path : Up-to-Date sm_* Section
3. Outdated or has error sm_* Section (e.g., has User '%r')
4. Syntax Error in SSH Config (Outside sm_* Section)

• All the testing is done for SMUS and SM-AI (with deeplink)
• Tested changes for Platforms : Mac, Win, Linux

---

• Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
• Your code changes must meet the guidelines in CONTRIBUTING.md.
• License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

• This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
  • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
  • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

[vpbhargav]

I think worth testing this change in Widows/Mac and Linux if possible. Hard to validate regex validations and some of these platforms have casing, newline peculiarities.

[dylanraws]

Not necessary, but something to think about: Instead of generating all possible config values (every time), you could store a list of functions here. Instead of iterating the values, you iterate the functions, and for each one, you call the function and then check for the result in the config. If found, then you shouldn't need to proceed to the next function, avoiding generating additional strings. This isn't necessary in this case because these strings are relatively short, and there won't be very many, so the performance impact is negligible, but it's worth being aware of this "lazy" pattern for future situations.

[dylanraws]

Why is it necessary to use these pointers? This feels over-complicated. If we stick to exact string matching (using the list of past expected sm_* sections), we can easily determine the start and end indices if we need to make a replacement.

const startIndex = configContent.indexOf(pastSection)
if (startIndex == -1) {
    // Not found
}

const endIndex = startIndex + pastSection.length

// Then slice and generate the new content string

We can probably simplify even further, as I don't imagine we need the indices.

if (configContent.includes(pastSection)) {
    const newContent = configContent.replace(pastSection, newSection)
    // Then write newContent to the file
}

[dylanraws]

We don't have to tell the user we failed to make an update.

When we tell the user "fix the issue manually", will it be clear what they should fix? For example, will we be displaying the ssh -G sm_test error output? If this applies to the case where the sm_* section needs to be deleted by the user, we can specify that in the message.

[dylanraws]

Can we use a constant or variable in place of sm_*? It's possible we will need to use a different string for other IDEs (since the global storage path for the sagemaker_connect script will be different) so it's better not to hard-code this.

[dylanraws]

A lot of these comments don't specifically mention the SSH Config. Previously, you had manually prefixed them all with SSH Config, but now that we are using a logger with the sagemaker prefix, SSH Config context is not present, making it hard to understand what these log messages are about. To improve, we can either add more context to the logger (e.g., sagemaker SSH config), or update the wording of each log message.

[dylanraws]

I saw a previous comment about defining constants for these error codes, but looks like that feedback wasn't yet implemented. Not required, but I figured I would remind you in case you planned on doing that.

[dylanraws]

"error while handling ... error" sounds odd. Makes me wonder about how this logic is structured.