refactor(sagemaker-ai): remove --command option from hyperpod-issue-report

[badmin-aws]

Removes the --command/-cmd option from the hyperpod-issue-report skill.

This is a functional removal of a documented, user-facing capability — not a dead-code cleanup. The flag worked end to end when passed: generate_collector_script interpolated each value into the generated bash collector script, and each command's output was written to command_{NN}_<sanitized_name>.txt inside ${OUTPUT_DIR}, tarballed, and uploaded to S3 alongside the built-in diagnostics.

No caller in this repo passed it, but the intended invocation path was agent-mediated rather than hardcoded: SKILL.md step 1 instructed the agent to collect "Additional commands to run on nodes" from the user, SKILL.md advertised --command in the --help list, and references/collection-details.md documented the resulting command_NN_*.txt artifact. So agent-visible behavior changes here, and anyone who previously supplied commands through that prompt will no longer see those files in the bundle.

Why remove it

The diagnostics the skill actually relies on are built into the generated script — sinfo, nvidia-smi, nvidia-bug-report.sh, df, dmesg, systemd unit status, Slurm config and logs, the EKS log collector — which covers the cases the removed --help examples illustrated (df -h, free -h).

It also removes an unquoted interpolation. The deleted line built the executed command as f"{cmd} > ..." with no quoting, while shlex.quote was applied only to the adjacent echo display line. That script is uploaded to S3 and executed on every targeted node, so shell metacharacters in a --command value were shell metacharacters on the fleet. That is inherent to an arbitrary-command feature rather than a defect in it, but deleting the feature does reduce attack surface, and doing so is preferable to bolting validation onto a capability with no callers. A metacharacter allowlist would have had to reject pipes and redirects to mean anything, and an opt-in override flag would just relocate the same path behind a second flag.

Changes

• Remove --command/-cmd, the per-command interpolation loop, and its output-filename sanitization.
• Remove the commands parameter from generate_collector_script, execute_collection_on_node, execute_with_retry, and collect_reports. Two of these were genuinely dead: execute_collection_on_node accepted the parameter and never read it (it builds its SSM invocation from script_s3_uri), and execute_with_retry only threaded it through. generate_collector_script and collect_reports did use it.
• Remove the option from SKILL.md (both the step-1 input list and the --help line) and the command_NN_*.txt section from references/collection-details.md.
• Bump the sagemaker-ai plugin 1.2.1 → 1.3.0 in all three manifests. Minor rather than patch, matching the minor feat(aws-transform): add --mode aws-managed and --resume-incomplete #251 (9545072) took for renaming --resume → --resume-incomplete, since SKILL.md made this agent-visible rather than merely internal.

Known gap

hyperpod-ssm covers ad-hoc command execution on a node, but it is not a complete substitute: it returns stdout to the caller one node at a time with no tar/S3 bundling, so "capture this extra output inside the support bundle" has no direct replacement. Given the built-ins already cover the common diagnostics, the practical gap is small — recording it so the tradeoff is explicit.

Testing

• Generated collector scripts are byte-identical for both cluster types (Slurm 117 lines, EKS 133 lines); every built-in diagnostic, the tarball step, and the S3 upload are unchanged. Verified by generating both and asserting no command_ or Running: residue remains.
• generate_collector_script() now takes no arguments, so the removed path is unreachable programmatically as well as from the CLI.
• --command is now an argparse error (exit 2).
• Verified live against a single-node Slurm HyperPod cluster end to end: cluster describe → Slurm detection → node enumeration → script upload → SSM session → on-node execution → tarball → upload → summary. The resulting bundle contained 66 entries (6.8 MB) of real diagnostics — Slurm config and logs, dmesg, syslog, systemd units, resource_config.json, and 28 HyperPod cluster-agent logs.
• python3 -m py_compile clean; bandit reports no issues; node tools/validate-cross-refs.cjs reports 0 errors.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

[ssuday]

LGTM

[scottschreckengaust]

Code Review: refactor(sagemaker-ai): remove unused --command option from hyperpod-issue-report

Summary

Well-scoped deletion. The removal itself is clean and complete: all four call sites are consistently updated, import shlex is still legitimately used (lines 478-479, 532-534) so no dead import is left behind, python3 -m py_compile passes, docs are updated in both places, and all three version manifests agree. All 22 CI checks are green. I found no correctness defects.

My comments are about framing and blast radius, not code quality. The description characterizes this as removing a dead parameter, but the parameter was live in generate_collector_script — this is a functional removal of a documented user-facing capability. That's a legitimate call to make; it just deserves to be recorded as such rather than as a no-op cleanup, because it affects how the next person reads the history and how the version was bumped.

Findings

#	File	Line	Finding	Severity
1	scripts/hyperpod_issue_report.py	476 (removed)	"dead commands parameter on generate_collector_script" is inaccurate — it was genuinely used to emit command_NN_<name>.txt into the bundle	🟡 Description accuracy
2	SKILL.md	30 (removed)	The skill instructed the agent to actively solicit "Additional commands" from users, so agent-visible behavior changes — not just an unreachable CLI flag	🟡 Description accuracy
3	marketplace.json + 2 manifests	245	Patch bump (1.2.1 → 1.2.2) for a user-facing capability removal; #251 took a minor for a comparable CLI change	🔵 Nit / non-blocking
4	—	—	hyperpod-ssm is not a complete substitute: it has no tar/S3 bundling, so "extra commands captured inside the support bundle" has no replacement	🔵 Discussion

On #3, to be explicit that I'm not blocking on it: docs/DESIGN_GUIDELINES.md:485-507 says breaking changes need a major bump plus a one-minor deprecation window, but that policy has never been exercised in this repo — no plugin has ever taken a major bump, and there are zero deprecation notices anywhere. The nearest precedent, #251 (9545072, merged a day before this PR), renamed --resume → --resume-incomplete and took a minor. So patch is a notch light by that yardstick but well inside observed variance. Your call.

On #4: hyperpod-ssm/scripts/ssm-exec.sh has no s3/tar handling — it returns stdout to the caller one node at a time, subject to SSM output truncation. That covers ad-hoc execution well, but not "capture this extra output into the tarball that goes to AWS Support." Given the built-ins already cover the common cases (df at line 373, nvidia-smi, systemd, dmesg, Slurm logs — the very things the removed --help examples showed), the practical gap is small. Flagging it so the tradeoff is explicit rather than implied.

What Looks Good

• The genuinely-dead code claim holds for 2 of 4 methods.execute_collection_on_node and execute_with_retry really did just thread the parameter through unused — nice catch.
• Security posture improves. The removed code interpolated the raw user string unquoted into a bash script (cmd_line = f"{cmd} > ...") that was uploaded to S3 and executed as root on every node. shlex.quote was applied only to the echo display line, not to the executed command. Intentional for an arbitrary-command feature, but deleting it is a real attack-surface reduction worth noting in the description as a benefit.
• Complete docs cleanup. No orphaned references to --command, -cmd, "Additional commands", or command_01_ remain anywhere in the repo.
• No leftover cruft. Every import still used; no unreachable branches; all three version manifests consistent with no stray 1.2.1.

Verdict

Comment — no changes required, two things worth your judgment. The code is correct and I'd be happy to see it merge. Before you do, consider (a) editing the PR/commit description so it doesn't call a live, documented feature "unused"/"dead" — that framing will mislead whoever bisects this later, and (b) whether 1.3.0 fits better than 1.2.2 for consistency with #251.

_{Two repo-level gaps surfaced during this review that are out of scope here and worth separate issues: the semver/deprecation policy in DESIGN_GUIDELINES.md:481-507 has never been applied in practice, and tools/validate-cross-refs.cjs validates name↔path but not version agreement across the three manifests — nothing in CI would catch a version mismatch.}

[badmin-aws]

Thanks — findings 1 and 2 were both correct and I've rewritten the PR description and force-pushed an amended commit message (f8288d8 → 6d976d0) so the corrected framing lands in history rather than only in the PR body, since bisect was the concern.

#1 — "dead parameter" was wrong for generate_collector_script. Agreed. It consumed commands to emit command_{NN}_<sanitized_name>.txt into ${OUTPUT_DIR}, which was then tarballed and uploaded, and collect_reports printed the list. Both the description and commit message now state plainly that this is a functional removal of a documented user-facing capability, and scope the "genuinely unused" claim to the two methods where it holds — execute_collection_on_node (accepted it, never read it, builds its SSM invocation from script_s3_uri) and execute_with_retry (threaded it through only).

#2 — "unused" overstated it. Also agreed. SKILL.md step 1 instructed the agent to solicit "Additional commands to run on nodes" from the user, so the intended caller was the model at runtime, not a hardcoded call site. The description now says so explicitly, and notes that anyone who previously supplied commands through that prompt will no longer see those files in the bundle. I've dropped "unused" from the title too.

I also took your suggestion on the security framing: the description now records the unquoted interpolation as a benefit of removal rather than leaving the change to read as a no-op cleanup, while noting it was inherent to an arbitrary-command feature rather than a defect in it.

#3 — bumped to 1.3.0. Your reasoning persuaded me over my own: SKILL.md step 1 makes this agent-visible, and #251 (9545072) taking a minor for the --resume → --resume-incomplete rename is the closest real precedent. All three manifests are at 1.3.0 with no stray 1.2.x.

#4 — you're right that hyperpod-ssm isn't a complete substitute.ssm-exec.sh has no tar/S3 handling and returns stdout one node at a time, so "capture this extra output inside the support bundle" genuinely has no replacement. I've recorded that as a "Known gap" section in the description and in the commit message rather than letting the hyperpod-ssm pointer imply full parity. I think the gap is acceptable given the built-ins cover the diagnostics the removed --help examples showed, but it should be explicit rather than implied — thanks for pushing on it.

One extra change in the force-push, unrelated to your findings: the previous revision also deleted an epilog example block (# With custom prefix and additional commands) outright. I've restored it as # With custom S3 prefix minus the flag, which keeps the PR scoped to just the option removal and keeps this file byte-identical to our internal source so future syncs stay clean.

Agreed both repo-level gaps you spotted are out of scope here — thanks for filing them separately.

[scottschreckengaust]

LGTM

[ssuday]

LGTM