[aviruthen]

Summary

The Training Recipes clone helper _run_clone_command_silent exists in two places (sagemaker-train and sagemaker-core modules). Its HTTPS failure handling has drifted from sagemaker.core.git_utils._run_clone_command. This change brings both copies in line with git_utils, so a failed recipe clone produces the same URL formatting and exception shape everywhere in the SDK.

Changes

• sagemaker-train/src/sagemaker/train/utils.py and sagemaker-core/src/sagemaker/core/modules/utils.py:
  • Reuse the existing URL helper from sagemaker.core.git_utils when reporting an HTTPS clone failure.
  • Raise a new CalledProcessError built from the formatted URL (from None). This matches git_utils._run_clone_command and keeps tracebacks to a single, clean exception.
  • Error log messages use the same formatted URL as the raised exception.
• Successful clones and the SSH code path are unchanged.

Testing

• Added sagemaker-train/tests/unit/train/test_utils_clone.py and sagemaker-core/tests/unit/modules/test_utils_clone.py, which cover:
  • failure with userinfo in the URL (formatted URL in the log and exception, return code preserved)
  • failure with a plain URL (URL unchanged)
  • success (original URL passed to git clone, GIT_TERMINAL_PROMPT=0 still set)
• Existing suites pass: sagemaker-train recipe tests, sagemaker-core tests/unit/modules, and tests/unit/test_git_utils.py.
• black and flake8 are clean on the changed files.

[github-actions]

🤖 AI code review

This PR aligns the HTTPS failure path of _run_clone_command_silent in both sagemaker-train and sagemaker-core with sagemaker.core.git_utils._run_clone_command: it redacts credentials from the URL via the existing _redact_credentials_from_url helper, reconstructs a CalledProcessError with the sanitized command, logs the safe URL, and re-raises from None. The SSH path is correctly left unchanged, consistent with prior git_utils work (PR #5811/#5812). New unit tests cover sanitized-failure, plain-URL, and success cases in both modules. The change is correct, well-scoped, and adequately tested; I found no issues.

_{Reviewed commit b8cb194. Automated review; verify before acting.}

[github-actions]

🤖 AI code review

This PR aligns the two _run_clone_command_silent copies (sagemaker-train and sagemaker-core modules) with git_utils._run_clone_command: on an HTTPS clone failure it now redacts credentials from the URL, re-raises a reconstructed CalledProcessError ... from None, and logs the redacted URL. It also tightens the shared _redact_credentials_from_url regex so only the authority's userinfo (before the first /, ? or #) is redacted, fixing false-positive redaction of a later @ in a path/query/fragment. The change is correct, consistent with the established credential-redaction precedent, and well covered by new unit tests for both copies plus the regex. No code defects found.

The regex change in git_utils.py alters the behavior of the existing _run_clone_command path too — a correct improvement, but the PR body describes only the two utils.py copies, so reviewers may overlook it.

_{Reviewed commit 2ee095b. Automated review; verify before acting.}

[github-actions]

🟡 Low: This regex change to _redact_credentials_from_url is a behavior change to the shared helper and therefore also affects the existing _run_clone_command HTTPS path, not just the two new _run_clone_command_silent copies described in the PR summary. The new pattern is an improvement (the old greedy [^@]+@ would wrongly redact a URL like https://github.com/user/repo@v1.git up to the @ in the path), and it's covered by the new tests. Consider calling out this git_utils.py change explicitly in the PR description so reviewers don't miss that the canonical redaction behavior changed.

[github-actions]

🤖 AI code review

This PR aligns the Training Recipes _run_clone_command_silent helpers (in sagemaker-train and sagemaker-core) with git_utils._run_clone_command: on an HTTPS clone failure they now redact credentials via _redact_credentials_from_url and re-raise a reconstructed CalledProcessError ... from None. It also tightens the redaction regex to ^(https://)[^/?#]*@ so passwords containing @ are fully redacted and @ signs in the path/query/fragment are not mistaken for userinfo. The change is consistent with prior precedent (Source: #5811, Source: #5812), SSH paths are correctly left unchanged, and the new unit tests cover the userinfo, no-userinfo, and success cases plus the regex edge cases. I found no correctness, security, or test-coverage issues.

_{Reviewed commit 81d6cb6. Automated review; verify before acting.}