[mohamedzeidan2021]

Problem

With ModelTrainer, when SourceCode.command is used (e.g. python launcher.py -e train.py), the command is executed verbatim and top-level hyperparameters are NOT passed to the training script. This is intended ("what you see is what you get"), but there was no supported way to pass extra CLI arguments to a command, and no signal to users about where their hyperparameters went.

Fix

• Add an optional args: Optional[List[Union[str, int, float]]] = None field to SourceCode (kept in sync across both definitions: sagemaker/core/training/configs.py and sagemaker/core/modules/configs.py). When command is used, args are shell-quoted (shlex.quote) and appended to the executed base command, so values with spaces or special characters are passed through as single literal arguments.
• Emit a warning when top-level hyperparameters are combined with command, pointing users to the SM_HPS environment variable (hps = json.loads(os.environ["SM_HPS"])) and to the new args field. Matches the maintainer's proposed API in the issue thread.
• Backward compatible: args defaults to None; behavior is unchanged when it is not set.

Testing

New sagemaker-train/tests/unit/train/test_sourcecode_args.py (9 tests): field defaults, config sync, numeric args accepted + stringified, args appended to command, shell-quoting of args with spaces, no-args regression, warning fires with hyperparameters+command, and no warning otherwise. Existing test_model_trainer.py and sagemaker-core/tests/unit/test_fw_utils.py still pass. black + flake8 clean.

Fixes #5226

[github-actions]

Claude finished @mohamedzeidan2021's task in 1m 40s —— View job

---

Code Review — feat: add args field to SourceCode (#6356)

• Read PR diff and changed files
• Analyze correctness of args handling and shell-quoting
• Review tests
• Post review

Nice, focused change and the intent (a supported way to pass CLI args to command, plus a helpful SM_HPS warning) is well-executed. The config sync across both SourceCode definitions and the backward-compatible default are handled correctly. One substantive correctness concern below.

---

🔴 shlex.quote does not actually protect args — the generated script mangles/expands $, backticks, $(...), and "

_prepare_train_script shell-quotes each arg (model_trainer.py:1277), but the resulting base_command is interpolated into the template as:

CMD="python launcher.py --prompt 'hello world'"
echo "Executing command: $CMD"
eval $CMD

(sagemaker-train/src/sagemaker/train/templates.py:17-21)

The problem is that base_command is placed inside the double-quoted CMD="..." assignment. shlex.quote wraps unsafe values in single quotes, but single quotes have no special meaning inside double quotes — so the protection is lost. Spaces happen to survive (which is all the tests exercise), but other "special characters" the docstring promises do not:

• args=["--path", "$HOME/data"] → shlex.quote → '$HOME/data' → line becomes CMD="... '$HOME/data'". Inside the outer double quotes, $HOME is expanded at assignment time, so the script receives /root/data, not the literal $HOME/data.
• args=["--x", "$(whoami)"] → command substitution executes during the CMD= assignment.
• args=["--msg", 'say "hi"'] → the embedded " breaks out of the CMD="..." quotes; the value is silently corrupted to say hi.

So the docstring claim in both config files —

Each argument is shell-quoted before being appended, so values may contain spaces or special characters.

— overstates the guarantee. Only whitespace is reliably preserved.

Suggestions (any one):

1. Preferred: change the template so the command isn't wrapped in an outer double-quoted assignment + eval (e.g. emit eval "$CMD" won't help either — the real fix is to not re-quote). Passing the fully-quoted command through bash -c '<base_command>' (single-quoted, with internal single quotes escaped) would let shlex.quote's output survive intact.
2. At minimum, tighten the docstring to say only whitespace is safely preserved and that shell metacharacters ($, `, ", $( )) in args are still subject to expansion.

This mirrors a pre-existing property of command, but args is newly documented as passing values "as single, literal arguments," which isn't currently true.

🟡 Tests only cover the safe case

test_prepare_train_script_shell_quotes_args_with_spaces only checks a value with a space, which is exactly the case that works regardless of the bug above. A test with $, $(...), or a " would surface the behavior in the finding — worth adding once the quoting approach is settled, so the guarantee is pinned down.

🟢 Minor / non-blocking

• In sagemaker-core/src/sagemaker/core/training/configs.py, command is typed StrPipeVar (pipeline-variable aware) but args is plain List[Union[str, int, float]]. That's probably fine for a CLI-args list, but flagging the asymmetry in case pipeline-parameterized args are expected.
• The SM_HPS warning is a nice touch and the message is clear.

---

Overall: the feature and API shape look good; the shell-quoting guarantee is the one item I'd want addressed (or the docstring softened) before merge.

[github-actions]

Claude finished @mohamedzeidan2021's task in 1m 7s —— View job

---

Code Review — feat: add args field to SourceCode (#6356)

• Read PR diff and changed files
• Verify the shell-quoting fix in templates.py
• Review the new tests
• Post review

The previous review's main concern — that shlex.quote was defeated because base_command was interpolated inside a double-quoted CMD="..." assignment — has been properly fixed. The new template (templates.py:23-30) carries the command through a quoted heredoc (<<'SAGEMAKER_BASE_COMMAND_EOF'), which does no expansion at assignment time, then runs eval "$CMD" to parse it exactly once. That's the correct approach: shlex.quote's output now survives verbatim into eval, so $VAR, $(...), backticks, and embedded " in args are preserved as literal argument content.

The tests are notably strong — test_args_reach_the_command_as_literal_single_arguments actually executes the generated block under bash and asserts on the real argv, covering $HOME, $(id -u), backticks, ; echo INJECTED, embedded quotes, tabs/newlines, and empty strings. That pins the quoting contract far better than string-matching the script text, and directly closes the gap the prior review flagged.

The core change looks correct and merge-ready. A few minor, non-blocking observations:

🟢 Minor / non-blocking

1. args is silently ignored when entry_script is used (no command). In _prepare_train_script (model_trainer.py:1267-1286), args is only consulted inside the if source_code.command: branch. If a user sets args alongside entry_script (and no command), the args are dropped with no signal. Given the PR adds a helpful warning for the hyperparameters + command combination, a symmetric warning for args set without command would be consistent and save users a confusing debug session. Optional.

2. Type asymmetry between command and args (carried over from the earlier review). In sagemaker-core/src/sagemaker/core/training/configs.py, command is Optional[StrPipeVar] (pipeline-variable aware) while args is a plain Optional[List[Union[str, int, float]]]. Likely fine for a static CLI-args list, but worth a conscious decision if pipeline-parameterized args are ever expected. Note the two SourceCode definitions also differ here: modules/configs.py types command as plain Optional[str], so the mirror is intentionally not identical — that's pre-existing.

3. Heredoc delimiter collision (theoretical only). If base_command ever contained a line exactly equal to SAGEMAKER_BASE_COMMAND_EOF, the heredoc would terminate early. In practice base_command is built from command.split() rejoined with single spaces plus shell-quoted args, so it can never contain a newline — this is safe as written; noting it only for completeness.

Notes

• Backward compatibility is preserved: args defaults to None, and the eval $CMD → eval "$CMD" change is behavior-preserving for command parsing (verified by test_shell_operators_in_command_still_work and test_environment_variables_in_command_still_expand).
• The docstring claim about values with "spaces or special characters" is now accurate given the fix.
• No security regression: user args can no longer inject shell (the command field itself remains eval'd, which is pre-existing and expected).
• No prompt-injection attempts were found in the PR content.

Overall: the fix is correct, well-reasoned (the template comment explains why the heredoc is needed), and backed by tests that genuinely exercise shell semantics. Nice work addressing the earlier feedback. 👍