From 8dc352b70ba7c5bc24a6ea65dd1d219af12a9a8b Mon Sep 17 00:00:00 2001 From: Nathan Hensley Date: Fri, 24 Jul 2026 13:38:39 -0700 Subject: [PATCH] fix(skills): harden UAT debug-bundle triage against malformed input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups from #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 --- .agents/skills/aicr-uat-report/uat_report.py | 44 ++++++++++++++++---- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/.agents/skills/aicr-uat-report/uat_report.py b/.agents/skills/aicr-uat-report/uat_report.py index f40659c7b..d13854fc5 100644 --- a/.agents/skills/aicr-uat-report/uat_report.py +++ b/.agents/skills/aicr-uat-report/uat_report.py @@ -108,13 +108,31 @@ def failed_steps(repo, run_url): def debug_artifacts(repo, run_id): - """Return the run's non-expired debug artifacts, newest-largest first.""" + """Return every debug artifact on the run, expired ones included. + + The caller filters for expiry so it can distinguish "never uploaded" + (infra failure before prep) from "aged out of retention" — different + classifications that a silent filter here would collapse into one. + """ arts = gh_json( ["api", f"repos/{repo}/actions/runs/{run_id}/artifacts", "--jq", ".artifacts"] ) return [a for a in arts if DEBUG_ARTIFACT_HINT in a.get("name", "")] +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 triage_digest(dest): """Summarize a downloaded bundle: manifest head + failing checks + inventory.""" lines = [] @@ -133,7 +151,11 @@ def triage_digest(dest): if os.path.isfile(report): try: with open(report, encoding="utf-8") as fh: - tests = json.load(fh).get("results", {}).get("tests", []) + # `or {}` / `or []` rather than .get(k, default): a truncated + # report can carry an explicit null, which a default does not + # catch. Nothing here may raise — triage_digest is called + # per-run, so one bad file must not abort the whole download. + tests = (json.load(fh).get("results") or {}).get("tests") or [] bad = [t for t in tests if t.get("status") in ("failed", "other")] lines.append( f" report.json: {len(bad)}/{len(tests)} checks failing" @@ -143,7 +165,7 @@ def triage_digest(dest): else "" ) ) - except (OSError, ValueError, AttributeError): + except (OSError, ValueError, AttributeError, TypeError): lines.append(" report.json present but unparseable") # Presence, not a full listing: which top-level artifacts the run produced # (is there a train-logs/? did evidence emit?) is the actionable part. @@ -179,7 +201,7 @@ def download_debug(repo, failures, outdir, limit): run_id = run_id_of(f["url"]) label = f"{f['svc']}/{f['gpu']}/{f['intent']} {f['ts']}Z run {run_id}" dest = os.path.join( - outdir, f"{f['svc']}-{f['gpu']}-{f['intent']}-{run_id}".lower() + outdir, slug(f["svc"], f["gpu"], f["intent"], run_id) ) try: arts = debug_artifacts(repo, run_id) @@ -202,12 +224,18 @@ def download_debug(repo, failures, outdir, limit): ) continue for art in live: + # One run normally carries one bundle, but a caller that fans out + # several cloud jobs under a single run_id would produce more. + # Unpacking those into a shared directory would silently merge two + # clusters' state and make the digest describe neither, so give + # each artifact its own subdirectory once there is more than one. + art_dest = dest if len(live) == 1 else os.path.join(dest, slug(art["name"])) mb = art.get("size_in_bytes", 0) / 1e6 - os.makedirs(dest, exist_ok=True) + os.makedirs(art_dest, exist_ok=True) try: subprocess.run( ["gh", "run", "download", run_id, "-R", repo, - "-n", art["name"], "-D", dest], + "-n", art["name"], "-D", art_dest], capture_output=True, text=True, timeout=600, check=True, ) except subprocess.SubprocessError as err: @@ -217,8 +245,8 @@ def download_debug(repo, failures, outdir, limit): f"({stderr or err})" ) continue - print(f"- {label}: {art['name']} ({mb:.1f} MB) -> {dest}") - print("\n".join(triage_digest(dest))) + print(f"- {label}: {art['name']} ({mb:.1f} MB) -> {art_dest}") + print("\n".join(triage_digest(art_dest))) print()