fix(skills): harden UAT debug-bundle triage against malformed input#1914
Open
njhensley wants to merge 1 commit into
Open
fix(skills): harden UAT debug-bundle triage against malformed input#1914njhensley wants to merge 1 commit into
njhensley wants to merge 1 commit into
Conversation
Review follow-ups from NVIDIA#1913. triage_digest crashed the whole --download-debug loop on a report.json carrying an explicit "tests": null — .get("tests", []) returns None only when the key is absent, so iteration raised TypeError, which the except clause did not list. Confirmed against the merged code. Use `or` fallbacks for null-valued keys and catch TypeError, so one truncated report degrades to "unparseable" for that run instead of aborting the remaining runs. Sanitize the download directory name. Reservation tokens reach the script through the workflow run-name and the title regexes accept any non-whitespace token, so svc/gpu are not guaranteed well-formed; collapse everything outside [a-z0-9-] to strip separators and dot runs before joining onto the output directory. Requires repo write access to reach, so this is defense in depth rather than a live exposure. Give each artifact its own subdirectory when a run carries more than one debug bundle — a shared directory merged two clusters' state and left the digest describing neither. Also correct the debug_artifacts docstring: it returns expired artifacts too, and never sorted. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>
njhensley
added
the
theme/ci-dx
Contributor
📝 WalkthroughWalkthroughThe UAT report workflow now includes expired debug artifacts in listings and adds filesystem-safe directory naming. Multiple active artifacts for a run are downloaded into separate directories. Triage parsing treats null Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/aicr-uat-report/uat_report.py:
- Around line 123-133: Add type annotations to the variadic parts parameter and
return value of slug, using types compatible with the existing str conversion
and filesystem-safe string result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3bb32642-afaa-4520-9036-4616dfebc708
📒 Files selected for processing (1)
.agents/skills/aicr-uat-report/uat_report.py
Comment on lines
+123
to
+133
| def slug(*parts): | ||
| """Build a filesystem-safe directory name from run metadata. | ||
|
|
||
| Reservation names reach us through the workflow's run-name, and the title | ||
| regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed | ||
| to be well-formed. Collapsing everything outside [a-z0-9-] removes both | ||
| separators and dot runs, so no component can traverse out of the download | ||
| directory. | ||
| """ | ||
| raw = "-".join(str(p) for p in parts).lower() | ||
| return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add annotations to slug.
Ruff reports ANN002 for the untyped variadic parameter. Add parameter and return annotations.
Suggested fix
-def slug(*parts):
+def slug(*parts: object) -> str:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def slug(*parts): | |
| """Build a filesystem-safe directory name from run metadata. | |
| Reservation names reach us through the workflow's run-name, and the title | |
| regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed | |
| to be well-formed. Collapsing everything outside [a-z0-9-] removes both | |
| separators and dot runs, so no component can traverse out of the download | |
| directory. | |
| """ | |
| raw = "-".join(str(p) for p in parts).lower() | |
| return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown" | |
| def slug(*parts: object) -> str: | |
| """Build a filesystem-safe directory name from run metadata. | |
| Reservation names reach us through the workflow's run-name, and the title | |
| regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed | |
| to be well-formed. Collapsing everything outside [a-z0-9-] removes both | |
| separators and dot runs, so no component can traverse out of the download | |
| directory. | |
| """ | |
| raw = "-".join(str(p) for p in parts).lower() | |
| return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown" |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 123-123: Missing type annotation for *parts
(ANN002)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/aicr-uat-report/uat_report.py around lines 123 - 133, Add
type annotations to the variadic parts parameter and return value of slug, using
types compatible with the existing str conversion and filesystem-safe string
result.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the three CodeRabbit findings on #1913: a crash in the triage digest, an unsanitized download path, and a directory collision when a run carries more than one debug bundle.
Motivation / Context
All three were raised on #1913 after it merged. The first is a genuine defect that defeats the feature's own error-handling contract; the other two are latent.
Fixes: N/A
Related: #1913
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/).agents/skills/aicr-uat-report/)Implementation Notes
1.
TypeErroraborted the download loop (real defect).report.jsonwith an explicit"tests": nullmade.get("tests", [])returnNone— a default only applies when the key is absent — so iteration raisedTypeError, which theexcept (OSError, ValueError, AttributeError)clause did not list.triage_digestis called per run with no surrounding guard, so one truncated report killed every remaining run. That directly contradicts the contract #1913 introduced, where a bad artifact is reported per run and processing continues.Reproduced against the merged code before fixing:
Fixed with
orfallbacks for null-valued keys plusTypeErrorin the except tuple. A malformed report now degrades toreport.json present but unparseablefor that one run.2. Unsanitized path components. Reservation tokens reach the script via the workflow
run-name, andNEW_TITLE/OLD_TITLEaccept any non-whitespace token, sosvc/gpuare not guaranteed well-formed before being joined onto the output directory. Newslug()collapses everything outside[a-z0-9-], which removes separators and dot runs together. Reaching this requires write access to dispatch the workflow, so it is defense in depth rather than a live exposure — but it is one line and the destination is attacker-influenced in principle.3. Multi-artifact collision. A run carrying more than one debug bundle unpacked them into a shared directory, merging two clusters' state so the printed digest described neither. Each artifact now gets its own subdirectory once there is more than one; the single-bundle path (every run observed so far) is unchanged.
Also corrected the
debug_artifactsdocstring, which claimed to filter expired artifacts and sort newest-first. It does neither — the caller filters deliberately, so that "never uploaded" and "aged out" stay distinguishable.Testing
Live regression run: green, output unchanged from #1913 for the normal path (one bundle fetched, one "no artifact" correctly reported).
Targeted cases, all exercised directly against
triage_digestandslug:{"results": {"tests": null}}0/0 checks failing(crashed pre-fix){"results": null}0/0 checks failing[1,2,3](top-level list)present but unparseablepresent but unparseable1/1 checks failing: x(failed)slug("EKS","GB200","training",id)eks-gb200-training-<id>— unchangedslug("../../etc","H100",…)etc-h100-…, stays under the output dirslug("AWS/../..","..",…)aws----inference-2, stays under the output dirslug("...","","","")unknownNo exception escaped in any case; every traversal attempt resolved inside the download directory.
make qualifynot run — no Go, YAML, or build inputs changed; verified withpy_compileplus the runs above.Risk Assessment
Rollout notes: Agent tooling only. Behaviour on well-formed input is identical to #1913; the changes only affect malformed-input and multi-artifact paths.
Checklist
make testwith-race) — N/A, no Go changes; see Testingmake lint) — N/A, no Go/YAML changes;py_compilecleangit commit -S)