Repository navigation
ci: keep the Multi-Arch summary JSON in one shell argument - #13268
Conversation
A ci:* pull request label is copied into linux_test_labels as a Python list. The single quote in that list ends the quoted toJSON(needs) argument, and a space in the label splits it. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
github actions
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
🟢 Approval recommended
The environment-variable approach consistently preserves the JSON as a single shell argument across all affected workflows.
0 open findings
What changed in this PR
Moves serialized workflow dependencies into an environment variable to prevent shell tokenization errors.
Changes:
- Safely passes
toJSON(needs)as one quoted argument. - Applies the fix consistently across standard, nightly, and ASAN workflows.
| File | Description |
|---|---|
.github/workflows/therock-multi-arch-ci.yml |
Safely passes summary JSON. |
.github/workflows/therock-multi-arch-ci-nightly.yml |
Applies the nightly workflow fix. |
.github/workflows/therock-multi-arch-ci-asan.yml |
Applies the ASAN workflow fix. |
.github/workflows/therock-multi-arch-ci-asan-nightly.yml |
Applies the nightly ASAN workflow fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@ROCm/rocm-systems-reviewers could you please review? |
|
@ROCm/rocm-systems-reviewers a friendly reminder. Please review the PR |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Thanks. We'll need the same change in each other repository that forked the workflows from TheRock, at least until ROCm/TheRock#8991 and more PRs from @geomin12 and others switch to reusing the workflows more directly (without as much forking).
These changes are similar enough to what was done as part of ROCm/TheRock#8275. Note that zizmor will have flagged these lines and applying security fixes across workflows in the repository will fix this and other issues.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
I think we can get enough signal from this PR by adding |
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Yep looks all good to merge now.
TheRock Submodule Bump ActivityNewest first
|
Summary
The Multi-Arch CI summary step passes
toJSON(needs)inside single quotes. Aci:*label is copied intolinux_test_labelsas a Python list such as['ci: code-coverage']. The quote inside that value ends the shell string, and the space in the label splits the argument, soworkflow_summary.pyexits withunrecognized arguments: code-coverage].Seen on #13027 and #12906 (
Multi-Arch CI Summary).Pass that JSON through the
NEEDS_JSONenvironment variable and expand"$NEEDS_JSON", so the shell does not tokenize it. The same call is updated in the ASAN and nightly workflows.Issue ID: ROCm/TheRock#8995
Test plan
ci: code-coveragelabel and confirmMulti-Arch CI Summarygets past argument parsing. This workflow-only change also selects a full multi-arch build.(AI Asist)