From 2b16d3943eaad2f243eccf4dba8676f209ea8bdb Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 09:12:19 -0400 Subject: [PATCH 1/7] payload-snapshot: never report unreadable data as empty A failed `gcloud storage` read returned None indistinguishably from "no such object", and `JUnitCollector.collect()` then wrote `[]` to results.json regardless. The snapshot therefore reported `test_failure_count: 0` for jobs whose test data it had simply been unable to read, and the analysis agent read that as "this job failed for reasons unrelated to its tests". Observed in a real 5.0 snapshot: every gcloud-sourced artifact was missing (0 JUnit XMLs, 0 build_log.json, 15 empty results.json) while every HTTP-sourced artifact was intact. `_check_gcloud()` only ran `gcloud --version`, so an installed-but-unauthenticated gcloud passed preflight and then failed every read silently. Changes: - `_run_gcloud`/`_run_gcloud_bytes` classify failures (auth, timeout, gcloud_missing, command_failed) and record them. A genuine "matched no objects" is still treated as absence, not error. - JUnit collection no longer writes results.json when the data could not be read. The summary omits `test_failure_count` and sets `junit_collection_failed: true`, so absent means unknown and 0 means verified clean. - summary.json gains `data_complete` and `collection_errors[]`. - AGENTS.md gains an INCOMPLETE SNAPSHOT section so the analysis agent sees the gap in its orientation document. - Loud end-of-run warning, plus `--fail-on-incomplete` for automation. - `_check_gcloud_credentials()` warns when gcloud has no active account. Also documents the invariant in both skills, and makes payload-analysis derive a failure mode's originating payload from the per-test `first_failed_in` rather than the job-level streak. The job-level streak merges unrelated modes (infrastructure blips, flakes, real regressions), which causes candidate PRs to be scored from a payload that predates the actual regression. --- .claude-plugin/marketplace.json | 2 +- docs/index.html | 2 +- plugins/ci/.claude-plugin/plugin.json | 2 +- plugins/ci/skills/payload-analysis/SKILL.md | 29 +++ plugins/ci/skills/payload-snapshot/SKILL.md | 20 ++ .../scripts/payload_snapshot.py | 231 +++++++++++++++++- 6 files changed, 279 insertions(+), 7 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 758d11616..12f9dee2c 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -88,7 +88,7 @@ "name": "ci", "source": "./plugins/ci", "description": "A plugin to work with OpenShift CI and analyze Prow job results", - "version": "0.0.71", + "version": "0.0.72", "category": "ci", "keywords": [ "prow", diff --git a/docs/index.html b/docs/index.html index 9b44d6ff2..82e2b4010 100644 --- a/docs/index.html +++ b/docs/index.html @@ -541,7 +541,7 @@

ai-helpers

{ "name": "ci", "description": "Tools for working with OpenShift CI and analyzing Prow job results", - "version": "0.0.71", + "version": "0.0.72", "has_readme": true, "commands": [ { diff --git a/plugins/ci/.claude-plugin/plugin.json b/plugins/ci/.claude-plugin/plugin.json index f6f69c424..3b6cf30e8 100644 --- a/plugins/ci/.claude-plugin/plugin.json +++ b/plugins/ci/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "ci", "description": "Tools for working with OpenShift CI and analyzing Prow job results", - "version": "0.0.71", + "version": "0.0.72", "author": { "name": "github.com/openshift-eng" } diff --git a/plugins/ci/skills/payload-analysis/SKILL.md b/plugins/ci/skills/payload-analysis/SKILL.md index 2b0da9cdd..12322ce9b 100644 --- a/plugins/ci/skills/payload-analysis/SKILL.md +++ b/plugins/ci/skills/payload-analysis/SKILL.md @@ -106,6 +106,24 @@ From `summary.json` top-level fields: **Record `phase` verbatim** from the `summary.json` metadata (`Accepted`, `Rejected`, or `Ready`). Never infer the phase from the job results or from whether failures exist — a payload can be `Accepted` *with* blocking failures (force-accepted) or `Ready` while jobs are still running. The stored phase drives the force-accept decision (Step 6.4) and the executive summary (Step 7.1), so an inferred phase silently corrupts both. +#### 3.1b: Verify the Snapshot Is Complete + +Check `summary.json` → `data_complete`. When it is `false`, read +`collection_errors[]` and treat every affected job's data as **unknown, not +empty**. + +A job entry with `junit_collection_failed: true` has **no** `test_failure_count` +because its JUnit could not be read. An absent `test_failure_count` means +*unknown*; only an explicit `0` means *verified clean*. Never conclude "this job +had no test failures" — and therefore "it must have failed for some other +reason" — from missing data. That inference sends the whole analysis down a +false path. + +If the target payload's test data is incomplete, say so explicitly in the report +and treat any streak or root-cause conclusion that depends on it as +provisional. Prefer re-running the snapshot (see `payload-snapshot`) over +analyzing an incomplete one. + #### 3.2: Failed Blocking Jobs From `summary.json` → `blocking_jobs.failed_jobs[]`, each entry contains: @@ -291,6 +309,17 @@ Compare the subagent's root cause analysis for the target payload against previo If a job fails in two consecutive payloads but for **different reasons**, treat each as a separate streak=1 failure with its own originating payload and candidate PRs. Re-split the streak and re-assign originating payloads before proceeding to scoring. +**The job-level streak is not the originating payload of a failure mode.** `blocking_jobs.failed_jobs[].streak.originating_payload` tracks when the *job* started failing, which silently merges unrelated modes — an infrastructure blip, a flaky test, and a real regression all read as one streak. Scoring candidate PRs from a job-level originating payload that predates the actual regression guarantees misattribution: the causal PR is not even in the candidate set, so the analysis confidently blames whatever else happened to land there. + +Do this mechanically, per failure mode: + +1. Take the **specific failing test** identified in Step 4, and read `test_failures.blocking[]` → the matching entry's **`first_failed_in`** and `payloads_failing`. That is the originating payload for *this* mode. +2. When `first_failed_in` is **later** than the job-level `streak.originating_payload`, the job's earlier failures are a **different mode**. Score candidates from `first_failed_in`, not from the job streak. +3. Verify the boundary. Confirm the test **passed** in the payload immediately before `first_failed_in`. If per-test data is unavailable there (e.g. the aggregation produced none), do **not** assume the test was failing — check the underlying child runs. A job that failed before the upgrade phase even ran cannot have failed this test. +4. An aggregated job can fail with **no per-test signal at all** (too few runs completed, or infrastructure killed the runs). Such a payload is **unclassified**, not a member of the regression streak. Read the underlying child runs' step results before assigning it to any mode. + +Only after the per-mode originating payload is established should you enumerate candidate PRs (Step 6.1). Record the job-level streak and the per-mode onset separately in the report when they differ, and state which one drove scoring. + ### Step 5b: Adjudicate Conflicting Root Causes When two or more investigations reach **contradictory root causes for the same failure signature** (same test, same operation, or same error class — across jobs, across retries, or between a subagent and a previous analysis), the analysis is **UNRESOLVED**. It is *not* a tie to be broken by whichever explanation feels more plausible. Resolve it only with discriminating evidence, applying these rules: diff --git a/plugins/ci/skills/payload-snapshot/SKILL.md b/plugins/ci/skills/payload-snapshot/SKILL.md index c3d276650..2db706bf3 100644 --- a/plugins/ci/skills/payload-snapshot/SKILL.md +++ b/plugins/ci/skills/payload-snapshot/SKILL.md @@ -183,6 +183,26 @@ Comprehensive stream-level triage data — start here. Contains: - `test_failures.blocking[]` — `test_name`, `jobs`, `first_failed_in`, `payloads_failing`, `failure_message`, `failure_text` (full, not truncated) - `payloads[]` — per-payload entries with `tag`, `phase`, relative file paths, `prs[]` with component/diff/comments paths, and `rhcos_changes[]` with RPM diffs per RHCOS variant - `rhcos_rpms[]` — RPMDB metadata for the target payload's RHCOS variants: `tag`, `name`, `pullspec`, `rpmdb` (relative path to rpmdb.sqlite) +- `data_complete` — `true` when every collection step succeeded. `false` means part of the snapshot could not be read. +- `collection_errors[]` — present only when `data_complete` is `false`. Each entry has `reason` (`auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable`, `build_log_unavailable`), `command`, and optionally `detail`, `stage`, `job`. + +#### Missing data is absent, never empty + +When a collection step fails, the affected file is **not written** and the +corresponding summary field is **omitted** — it is never emitted as an empty +list or a zero count. Specifically: + +- If JUnit could not be read for a job, `results.json` is not created, the + job entry has **no** `test_failure_count` and `junit_results`, and instead + carries `junit_collection_failed: true`. +- An absent `test_failure_count` therefore means *unknown*, whereas `0` means + *verified clean*. + +Consumers **must** distinguish these. Treating unreadable data as "no test +failures" makes a broken job look like it failed for some other reason, which +misdirects root-cause analysis. Check `data_complete` before drawing any +conclusion from an absence of test failures. Use `--fail-on-incomplete` in +automation to exit non-zero rather than emit a partial snapshot. ### `AGENTS.md` / `CLAUDE.md` diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index 42941a460..77bfda905 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -716,7 +716,9 @@ def collect(self) -> bool: os.makedirs(self.output_dir, exist_ok=True) all_results: list[_TestResult] = [] + errors_before = _collection_error_count() junit_files = self._list_junit_files() + downloaded = 0 for gcs_uri in junit_files: filename = os.path.basename(gcs_uri) local_path = os.path.join(self.output_dir, filename) @@ -729,9 +731,34 @@ def collect(self) -> bool: f.write(data) if os.path.exists(local_path): + downloaded += 1 results = _parse_junit_xml(local_path, source_name=filename) all_results.extend(results) + gcloud_failed = _collection_error_count() > errors_before + + # Refuse to publish an empty result set that was caused by a read + # failure. Writing `[]` here makes the job look clean, and + # downstream consumers report "0 test failures" for a job that + # actually failed. Leaving results.json absent means "unknown", + # which the summary and the analysis skill treat as such. + if gcloud_failed and downloaded == 0: + _record_collection_error( + "junit_unavailable", + ["gcloud", "storage", "cat", ""], + detail=( + "no JUnit XML could be read; results.json intentionally " + "not written so this is not mistaken for zero failures" + ), + stage="junit", + job=self.job.name, + ) + _log( + f" ERROR: could not read JUnit for {self.job.name} — " + f"leaving results.json absent (unknown, not zero)" + ) + return False + failures = _test_results_to_json(all_results) _write_json(self.output_path, failures) @@ -826,10 +853,19 @@ def _fetch(self) -> Optional[dict]: return None gcs_uri = f"gs://{self.job.gcs_bucket_path}/build-log.txt" + errors_before = _collection_error_count() raw = _run_gcloud_bytes( ["gcloud", "storage", "cat", gcs_uri], timeout=120 ) if not raw: + if _collection_error_count() > errors_before: + _record_collection_error( + "build_log_unavailable", + ["gcloud", "storage", "cat", gcs_uri], + detail="build-log.txt could not be read", + stage="build_log", + job=self.job.name, + ) return None try: @@ -1264,6 +1300,14 @@ def generate(self) -> None: for entry in target_rpms ] + # Surface any data that could not be collected. An empty snapshot + # section must never be silently indistinguishable from a clean one. + if _COLLECTION_ERRORS: + summary["collection_errors"] = list(_COLLECTION_ERRORS) + summary["data_complete"] = False + else: + summary["data_complete"] = True + _write_json(os.path.join(self.base_dir, "summary.json"), summary) self._write_agents_md(summary) _log(" Generated summary.json, AGENTS.md, and CLAUDE.md") @@ -1316,6 +1360,24 @@ def _write_agents_md(self, summary: dict) -> None: "", ] + if not summary.get("data_complete", True): + errs = summary.get("collection_errors", []) + reasons = sorted({e.get("reason", "unknown") for e in errs}) + lines.extend([ + "## ⚠️ INCOMPLETE SNAPSHOT", + "", + f"{len(errs)} collection error(s) occurred: " + f"{', '.join(reasons)}.", + "See `collection_errors` in `summary.json`.", + "", + "Data that could not be read is **absent, not empty**. A job", + "with `junit_collection_failed: true` and no", + "`test_failure_count` has an *unknown* number of test", + "failures — do NOT report it as zero, and do not conclude a", + "job failed for reasons other than its tests on this basis.", + "", + ]) + if failed_names: lines.append("### Failed blocking jobs") lines.append("") @@ -1469,6 +1531,10 @@ def _build_failed_job_details(self, lifecycle: str) -> list[dict]: ) results_data = _read_json(results_path) or [] entry["test_failure_count"] = len(results_data) + elif _job_junit_failed(job_name): + # Data could not be read. Do NOT emit test_failure_count — + # its absence means "unknown", where 0 would mean "clean". + entry["junit_collection_failed"] = True build_log_path = os.path.join( lifecycle_dir, job_name, "build_log.json" @@ -2162,29 +2228,129 @@ def _check_gh_auth() -> bool: return False +# Records every gcloud failure that could hide real data. A "no match" +# result is normal (the object simply does not exist) and is NOT recorded. +# Anything else — a missing binary, a timeout, an auth failure — means the +# snapshot could not read data that may well exist, and callers must be able +# to tell that apart from "there was nothing to find". +_COLLECTION_ERRORS: list[dict] = [] + +# stderr fragments that mean "the object does not exist", not "I failed". +_GCLOUD_NO_MATCH_PATTERNS = ( + "matched no objects", + "matched no such objects", + "not found", +) + +# stderr fragments that indicate a credentials/permission problem. +_GCLOUD_AUTH_PATTERNS = ( + "does not have storage.objects", + "anonymous caller", + "reauthentication", + "credentials", + "unauthorized", + "forbidden", + "403", + "401", +) + + +def _classify_gcloud_stderr(stderr: str) -> str: + """Classify a failed gcloud invocation from its stderr.""" + low = (stderr or "").lower() + if any(p in low for p in _GCLOUD_NO_MATCH_PATTERNS): + return "no_match" + if any(p in low for p in _GCLOUD_AUTH_PATTERNS): + return "auth" + return "command_failed" + + +def _record_collection_error( + reason: str, + command: list[str], + detail: str = "", + stage: str = "", + job: str = "", +) -> None: + """Record a data-collection failure so it can be surfaced, not swallowed. + + Silently returning empty data makes "I could not read this" look + identical to "there is nothing here", which invites downstream + consumers to report zero failures for a job that actually failed. + """ + entry: dict = {"reason": reason, "command": " ".join(command[:4])} + if detail: + entry["detail"] = detail.strip()[:500] + if stage: + entry["stage"] = stage + if job: + entry["job"] = job + _COLLECTION_ERRORS.append(entry) + + +def _collection_error_count() -> int: + """Number of collection errors recorded so far.""" + return len(_COLLECTION_ERRORS) + + +def _job_junit_failed(job_name: str) -> bool: + """True when JUnit collection for this job failed to read its data.""" + return any( + e.get("job") == job_name and e.get("reason") == "junit_unavailable" + for e in _COLLECTION_ERRORS + ) + + def _run_gcloud(args: list[str], timeout: int = 120) -> Optional[str]: - """Run a gcloud CLI command, returning stdout or None on error.""" + """Run a gcloud CLI command, returning stdout or None on error. + + Failures other than "no such object" are recorded in + ``_COLLECTION_ERRORS`` so callers can distinguish an unreadable + source from an empty one. + """ try: result = subprocess.run( args, capture_output=True, text=True, timeout=timeout ) if result.returncode != 0: + reason = _classify_gcloud_stderr(result.stderr) + if reason != "no_match": + _record_collection_error(reason, args, detail=result.stderr) return None return result.stdout - except (subprocess.TimeoutExpired, FileNotFoundError): + except subprocess.TimeoutExpired: + _record_collection_error( + "timeout", args, detail=f"exceeded {timeout}s" + ) + return None + except FileNotFoundError: + _record_collection_error("gcloud_missing", args) return None def _run_gcloud_bytes(args: list[str], timeout: int = 120) -> Optional[bytes]: - """Run a gcloud CLI command, returning raw stdout bytes or None.""" + """Run a gcloud CLI command, returning raw stdout bytes or None. + + Records failures the same way as :func:`_run_gcloud`. + """ try: result = subprocess.run( args, capture_output=True, timeout=timeout ) if result.returncode != 0: + stderr = (result.stderr or b"").decode("utf-8", errors="replace") + reason = _classify_gcloud_stderr(stderr) + if reason != "no_match": + _record_collection_error(reason, args, detail=stderr) return None return result.stdout - except (subprocess.TimeoutExpired, FileNotFoundError): + except subprocess.TimeoutExpired: + _record_collection_error( + "timeout", args, detail=f"exceeded {timeout}s" + ) + return None + except FileNotFoundError: + _record_collection_error("gcloud_missing", args) return None @@ -2200,6 +2366,26 @@ def _check_gcloud() -> bool: return False +def _check_gcloud_credentials() -> bool: + """Report whether gcloud has an active credential. + + ``gcloud --version`` only proves the binary exists. An installed but + unauthenticated gcloud passes that check and then fails every read, + which previously produced a snapshot full of silently empty JUnit + data. Public buckets can still be read anonymously, so a negative + result here is a warning rather than a hard failure. + """ + try: + result = subprocess.run( + ["gcloud", "auth", "list", + "--filter=status:ACTIVE", "--format=value(account)"], + capture_output=True, text=True, timeout=15, + ) + return result.returncode == 0 and bool(result.stdout.strip()) + except (FileNotFoundError, subprocess.TimeoutExpired): + return False + + def _check_podman() -> bool: """Verify that podman is available and functional. @@ -2490,6 +2676,12 @@ def main() -> None: "--no-junit", action="store_true", help="Skip JUnit download, regression tracking", ) + parser.add_argument( + "--fail-on-incomplete", action="store_true", + help=("Exit non-zero if any data could not be collected. Use in " + "automation so an incomplete snapshot is not analyzed as " + "though it were complete."), + ) parser.add_argument( "--no-rpmdb", action="store_true", help="Skip RHCOS RPMDB extraction", @@ -2522,6 +2714,13 @@ def main() -> None: _log("Warning: 'gcloud' CLI not found. JUnit data will not be fetched.") _log("Install gcloud SDK to enable JUnit download and regression tracking.\n") collect_junit = False + elif collect_junit and not _check_gcloud_credentials(): + _log("Warning: 'gcloud' has no active credentials " + "('gcloud auth list' is empty).") + _log("Reads of non-public artifacts will fail. Run " + "'gcloud auth login' to authenticate.") + _log("Collection failures will be recorded in " + "summary.json 'collection_errors'.\n") collect_rpmdb = not args.no_rpmdb if collect_rpmdb and not _check_podman(): @@ -2543,6 +2742,30 @@ def main() -> None: collect_rpmdb=collect_rpmdb, ) snapshotter.run() + + if _COLLECTION_ERRORS: + by_reason: dict[str, int] = {} + for e in _COLLECTION_ERRORS: + r = e.get("reason", "unknown") + by_reason[r] = by_reason.get(r, 0) + 1 + _log("") + _log("=" * 68) + _log(f"WARNING: SNAPSHOT IS INCOMPLETE " + f"({len(_COLLECTION_ERRORS)} collection error(s))") + for reason, count in sorted(by_reason.items()): + _log(f" {reason}: {count}") + affected = sorted({ + e["job"] for e in _COLLECTION_ERRORS if e.get("job") + }) + if affected: + _log(f" affected jobs: {', '.join(affected)}") + _log("") + _log("Missing data is ABSENT, not empty. Do not interpret a job") + _log("without test_failure_count as having zero test failures.") + _log("Details: 'collection_errors' in summary.json") + _log("=" * 68) + if args.fail_on_incomplete: + sys.exit(1) except urllib.error.HTTPError as e: print(f"Error: HTTP {e.code}: {e.reason}", file=sys.stderr) sys.exit(1) From 5a9351b5bf2b43fae57d7187fc0e4d693ffdc31c Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 09:26:09 -0400 Subject: [PATCH 2/7] =?UTF-8?q?payload-snapshot:=20absorb=20#641=20?= =?UTF-8?q?=E2=80=94=20glob=20timeout=20raise=20and=20bounded-depth=20fall?= =?UTF-8?q?back?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Incorporates the JUnit discovery hardening from #641 (closed in favour of this PR), with two fixes and one interaction resolved: - Primary recursive glob timeout raised 30s -> 120s. - Bounded-depth fallback when the recursive glob yields nothing. - Fix: #641's probes checked {step}/, {step}/artifacts/ and {step}/*/artifacts/ only. Aggregated jobs keep junit-aggregated.xml about six levels below artifacts/, so those probes could never find it. The aggregator subtree now gets its own scoped ** probe, which stays fast because it covers one small directory. - Fix: junit_operator.xml lives directly in artifacts/, not inside a step directory, so the fallback missed it. Added a top-level probe. The fallback now recovers the same file set as the glob. - Interaction: a recovered failure must not mark the snapshot incomplete. Errors resolved by a fallback are flagged `recovered: true`, reported separately, and excluded from `data_complete`. Verified against the same payload three ways: - glob fails, fallback recovers: 73 failing tests, data_complete true, 4 errors all recovered, exit 0 — identical results to the glob path - all reads fail: 20 unrecovered errors, no results.json written, junit_collection_failed set, exit 1 - real gcloud: no fallback needed, 73 failing tests, 0 errors, exit 0 --- plugins/ci/skills/payload-snapshot/SKILL.md | 4 +- .../scripts/payload_snapshot.py | 149 ++++++++++++++++-- 2 files changed, 135 insertions(+), 18 deletions(-) diff --git a/plugins/ci/skills/payload-snapshot/SKILL.md b/plugins/ci/skills/payload-snapshot/SKILL.md index 2db706bf3..1bb991086 100644 --- a/plugins/ci/skills/payload-snapshot/SKILL.md +++ b/plugins/ci/skills/payload-snapshot/SKILL.md @@ -184,7 +184,9 @@ Comprehensive stream-level triage data — start here. Contains: - `payloads[]` — per-payload entries with `tag`, `phase`, relative file paths, `prs[]` with component/diff/comments paths, and `rhcos_changes[]` with RPM diffs per RHCOS variant - `rhcos_rpms[]` — RPMDB metadata for the target payload's RHCOS variants: `tag`, `name`, `pullspec`, `rpmdb` (relative path to rpmdb.sqlite) - `data_complete` — `true` when every collection step succeeded. `false` means part of the snapshot could not be read. -- `collection_errors[]` — present only when `data_complete` is `false`. Each entry has `reason` (`auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable`, `build_log_unavailable`), `command`, and optionally `detail`, `stage`, `job`. +- `collection_errors[]` — every read failure encountered. Each entry has `reason` (`auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable`, `build_log_unavailable`), `command`, and optionally `detail`, `stage`, `job`, `recovered`. + - `recovered: true` means a fallback subsequently obtained the data. These entries are diagnostic only (useful for spotting a timeout that needs tuning) and do **not** make `data_complete` false. + - `data_complete` is `false` only when at least one error was **not** recovered. #### Missing data is absent, never empty diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index 77bfda905..b7311e1fa 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -716,7 +716,7 @@ def collect(self) -> bool: os.makedirs(self.output_dir, exist_ok=True) all_results: list[_TestResult] = [] - errors_before = _collection_error_count() + errors_before = _unrecovered_error_count() junit_files = self._list_junit_files() downloaded = 0 for gcs_uri in junit_files: @@ -735,7 +735,7 @@ def collect(self) -> bool: results = _parse_junit_xml(local_path, source_name=filename) all_results.extend(results) - gcloud_failed = _collection_error_count() > errors_before + gcloud_failed = _unrecovered_error_count() > errors_before # Refuse to publish an empty result set that was caused by a read # failure. Writing `[]` here makes the job look clean, and @@ -775,18 +775,39 @@ def collect(self) -> bool: return True def _list_junit_files(self) -> list[str]: - """Discover JUnit XML files in GCS for this job.""" + """Discover JUnit XML files in GCS for this job. + + Tries a recursive glob first, then falls back to bounded-depth + probes. Jobs that emit very large artifact trees (Hypershift e2e + can produce 10,000+ cluster resource dumps) make the ``**`` glob + slow enough to time out, and a timeout here previously produced a + silently empty result set. + """ bucket_path = self.job.gcs_bucket_path base = f"gs://{bucket_path}" + errors_before = _collection_error_count() output = _run_gcloud( ["gcloud", "storage", "ls", f"{base}/artifacts/**/junit*.xml"], - timeout=30, + timeout=120, ) - if not output: - return [] - files = [l.strip() for l in output.strip().splitlines() if l.strip()] + files = [l.strip() for l in (output or "").strip().splitlines() + if l.strip()] + + if not files: + _log( + f" warn: recursive glob found no JUnit for {self.job.name} " + f"— trying bounded-depth fallback" + ) + files = self._list_junit_files_fallback(base) + if files: + # The data was obtained after all; don't report the failed + # first attempt as missing data. + _mark_errors_recovered(errors_before) + + if not files: + return [] if self.job.is_aggregated: # For aggregated jobs, keep junit_operator.xml and the @@ -810,6 +831,71 @@ def _list_junit_files(self) -> list[str]: keep = operator + (preferred if preferred else others[:1]) return keep if keep else files + def _list_junit_files_fallback(self, base: str) -> list[str]: + """Targeted JUnit discovery that avoids a full recursive glob. + + Lists the top-level step directories under ``artifacts/`` and + probes each at the depths JUnit files actually appear, rather + than enumerating every object in the tree: + + - ``{step}/junit*.xml`` + - ``{step}/artifacts/junit*.xml`` + - ``{step}/*/artifacts/junit*.xml`` + + Aggregated jobs keep ``junit-aggregated.xml`` far deeper than + that, so the aggregator subtree gets its own scoped recursive + probe — scoped to one small directory, it stays fast. + """ + dir_output = _run_gcloud( + ["gcloud", "storage", "ls", f"{base}/artifacts/"], + timeout=60, + ) + if not dir_output: + return [] + + # Each line is a directory like gs://bucket/path/artifacts/step-name/ + step_dirs = [ + d.strip().rstrip("/").split("/")[-1] + for d in dir_output.strip().splitlines() + if d.strip() + ] + + # junit_operator.xml sits directly in artifacts/, not in a step dir. + found: list[str] = [] + top = _run_gcloud( + ["gcloud", "storage", "ls", f"{base}/artifacts/junit*.xml"], + timeout=60, + ) + if top: + found.extend(l.strip() for l in top.strip().splitlines() + if l.strip()) + + for step in step_dirs: + patterns = [ + f"{base}/artifacts/{step}/junit*.xml", + f"{base}/artifacts/{step}/artifacts/junit*.xml", + f"{base}/artifacts/{step}/*/artifacts/junit*.xml", + ] + # The aggregated report lives several levels below the + # aggregator step; a scoped ** over that one subtree is cheap. + if "aggregator" in step: + patterns.append(f"{base}/artifacts/{step}/**/junit*.xml") + + for pattern in patterns: + probe = _run_gcloud( + ["gcloud", "storage", "ls", pattern], timeout=60 + ) + if probe: + found.extend( + l.strip() for l in probe.strip().splitlines() + if l.strip() + ) + + found = sorted(set(found)) + if found: + _log(f" fallback found {len(found)} JUnit file(s)") + return found + def _detect_underlying_job_name(self) -> Optional[str]: """For aggregated jobs, extract the underlying job name from GCS paths.""" if not self.job.is_aggregated: @@ -853,12 +939,12 @@ def _fetch(self) -> Optional[dict]: return None gcs_uri = f"gs://{self.job.gcs_bucket_path}/build-log.txt" - errors_before = _collection_error_count() + errors_before = _unrecovered_error_count() raw = _run_gcloud_bytes( ["gcloud", "storage", "cat", gcs_uri], timeout=120 ) if not raw: - if _collection_error_count() > errors_before: + if _unrecovered_error_count() > errors_before: _record_collection_error( "build_log_unavailable", ["gcloud", "storage", "cat", gcs_uri], @@ -1304,9 +1390,7 @@ def generate(self) -> None: # section must never be silently indistinguishable from a clean one. if _COLLECTION_ERRORS: summary["collection_errors"] = list(_COLLECTION_ERRORS) - summary["data_complete"] = False - else: - summary["data_complete"] = True + summary["data_complete"] = not _unrecovered_errors() _write_json(os.path.join(self.base_dir, "summary.json"), summary) self._write_agents_md(summary) @@ -1361,7 +1445,8 @@ def _write_agents_md(self, summary: dict) -> None: ] if not summary.get("data_complete", True): - errs = summary.get("collection_errors", []) + errs = [e for e in summary.get("collection_errors", []) + if not e.get("recovered")] reasons = sorted({e.get("reason", "unknown") for e in errs}) lines.extend([ "## ⚠️ INCOMPLETE SNAPSHOT", @@ -2293,10 +2378,33 @@ def _collection_error_count() -> int: return len(_COLLECTION_ERRORS) +def _mark_errors_recovered(since_index: int) -> None: + """Mark errors recorded since ``since_index`` as recovered. + + A first-choice read can fail (e.g. a glob times out) and a fallback + can then succeed. The failure is still worth reporting — it is how + you learn a timeout needs tuning — but it must not make the snapshot + look incomplete when the data was in fact obtained. + """ + for entry in _COLLECTION_ERRORS[since_index:]: + entry["recovered"] = True + + +def _unrecovered_errors() -> list[dict]: + """Collection errors that were not resolved by a fallback.""" + return [e for e in _COLLECTION_ERRORS if not e.get("recovered")] + + +def _unrecovered_error_count() -> int: + """Number of unrecovered collection errors.""" + return len(_unrecovered_errors()) + + def _job_junit_failed(job_name: str) -> bool: """True when JUnit collection for this job failed to read its data.""" return any( e.get("job") == job_name and e.get("reason") == "junit_unavailable" + and not e.get("recovered") for e in _COLLECTION_ERRORS ) @@ -2743,19 +2851,26 @@ def main() -> None: ) snapshotter.run() - if _COLLECTION_ERRORS: + unrecovered = _unrecovered_errors() + recovered = len(_COLLECTION_ERRORS) - len(unrecovered) + if recovered: + _log("") + _log(f"Note: {recovered} read failure(s) were recovered by a " + f"fallback; data is complete. See 'collection_errors' " + f"(recovered: true) in summary.json.") + if unrecovered: by_reason: dict[str, int] = {} - for e in _COLLECTION_ERRORS: + for e in unrecovered: r = e.get("reason", "unknown") by_reason[r] = by_reason.get(r, 0) + 1 _log("") _log("=" * 68) _log(f"WARNING: SNAPSHOT IS INCOMPLETE " - f"({len(_COLLECTION_ERRORS)} collection error(s))") + f"({len(unrecovered)} unrecovered collection error(s))") for reason, count in sorted(by_reason.items()): _log(f" {reason}: {count}") affected = sorted({ - e["job"] for e in _COLLECTION_ERRORS if e.get("job") + e["job"] for e in unrecovered if e.get("job") }) if affected: _log(f" affected jobs: {', '.join(affected)}") From 6186824e75c5c1f17ec127000db1bdc5a95b71db Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 09:32:58 -0400 Subject: [PATCH 3/7] payload-analysis: tell the agent to collect missing data itself Replaces the guidance to treat incomplete data as provisional. The agent already has the job's gcs_url and the tooling to read it, so on a collection failure it should analyze the job's artifacts directly rather than reason around the gap. Trimmed both additions to what the agent needs to do, dropping the gcloud usage examples and step-by-step procedure. --- plugins/ci/skills/payload-analysis/SKILL.md | 37 +++++++++------------ 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/plugins/ci/skills/payload-analysis/SKILL.md b/plugins/ci/skills/payload-analysis/SKILL.md index 12322ce9b..d3567243c 100644 --- a/plugins/ci/skills/payload-analysis/SKILL.md +++ b/plugins/ci/skills/payload-analysis/SKILL.md @@ -106,23 +106,21 @@ From `summary.json` top-level fields: **Record `phase` verbatim** from the `summary.json` metadata (`Accepted`, `Rejected`, or `Ready`). Never infer the phase from the job results or from whether failures exist — a payload can be `Accepted` *with* blocking failures (force-accepted) or `Ready` while jobs are still running. The stored phase drives the force-accept decision (Step 6.4) and the executive summary (Step 7.1), so an inferred phase silently corrupts both. -#### 3.1b: Verify the Snapshot Is Complete +#### 3.1b: If the Snapshot Is Incomplete, Collect the Data Yourself -Check `summary.json` → `data_complete`. When it is `false`, read -`collection_errors[]` and treat every affected job's data as **unknown, not -empty**. +Check `summary.json` → `data_complete`. An absent `test_failure_count` means +*unknown*, not zero — never conclude a job had no test failures, and therefore +failed for some other reason, from missing data. -A job entry with `junit_collection_failed: true` has **no** `test_failure_count` -because its JUnit could not be read. An absent `test_failure_count` means -*unknown*; only an explicit `0` means *verified clean*. Never conclude "this job -had no test failures" — and therefore "it must have failed for some other -reason" — from missing data. That inference sends the whole analysis down a -false path. +When data is missing, collect it yourself from the job's `gcs_url` artifacts +rather than analyzing around the gap. Do the same for any payload in the chain +whose per-test data is missing. Report a gap as a limitation only when the +artifacts themselves are unreachable. -If the target payload's test data is incomplete, say so explicitly in the report -and treat any streak or root-cause conclusion that depends on it as -provisional. Prefer re-running the snapshot (see `payload-snapshot`) over -analyzing an incomplete one. +An aggregated job with no per-test results at all is **unclassified**, not part +of a regression streak — aggregation also fails when too few child runs +completed or infrastructure killed them. Check the child runs: one that died +before the test phase cannot have failed a test. #### 3.2: Failed Blocking Jobs @@ -309,16 +307,11 @@ Compare the subagent's root cause analysis for the target payload against previo If a job fails in two consecutive payloads but for **different reasons**, treat each as a separate streak=1 failure with its own originating payload and candidate PRs. Re-split the streak and re-assign originating payloads before proceeding to scoring. -**The job-level streak is not the originating payload of a failure mode.** `blocking_jobs.failed_jobs[].streak.originating_payload` tracks when the *job* started failing, which silently merges unrelated modes — an infrastructure blip, a flaky test, and a real regression all read as one streak. Scoring candidate PRs from a job-level originating payload that predates the actual regression guarantees misattribution: the causal PR is not even in the candidate set, so the analysis confidently blames whatever else happened to land there. +**The job-level streak is not a failure mode's originating payload.** `streak.originating_payload` tracks when the *job* started failing, which merges unrelated modes — an infrastructure blip, a flake, and a real regression all read as one streak. Scoring candidates from a payload that predates the actual regression guarantees misattribution: the causal PR is not even in the candidate set. -Do this mechanically, per failure mode: +For each failure mode, take the originating payload from the matching `test_failures.blocking[]` entry's **`first_failed_in`**. When that is later than the job-level streak's, the job's earlier failures are a different mode — score from `first_failed_in`. Confirm the test passed in the preceding payload; where that payload has no per-test data, check its child runs rather than assuming it was failing. -1. Take the **specific failing test** identified in Step 4, and read `test_failures.blocking[]` → the matching entry's **`first_failed_in`** and `payloads_failing`. That is the originating payload for *this* mode. -2. When `first_failed_in` is **later** than the job-level `streak.originating_payload`, the job's earlier failures are a **different mode**. Score candidates from `first_failed_in`, not from the job streak. -3. Verify the boundary. Confirm the test **passed** in the payload immediately before `first_failed_in`. If per-test data is unavailable there (e.g. the aggregation produced none), do **not** assume the test was failing — check the underlying child runs. A job that failed before the upgrade phase even ran cannot have failed this test. -4. An aggregated job can fail with **no per-test signal at all** (too few runs completed, or infrastructure killed the runs). Such a payload is **unclassified**, not a member of the regression streak. Read the underlying child runs' step results before assigning it to any mode. - -Only after the per-mode originating payload is established should you enumerate candidate PRs (Step 6.1). Record the job-level streak and the per-mode onset separately in the report when they differ, and state which one drove scoring. +Establish this before enumerating candidate PRs (Step 6.1). When the two onsets differ, record both and state which drove scoring. ### Step 5b: Adjudicate Conflicting Root Causes From 0be8388615e196ce06cd82a13669132b6d7bbc26 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 09:36:56 -0400 Subject: [PATCH 4/7] payload-snapshot: read public buckets anonymously when gcloud has no creds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The artifact buckets are public, but `gcloud storage` refuses client-side when no account is configured — "You do not currently have an active account selected" — without ever issuing the request. That is why an unauthenticated environment produced a snapshot with zero JUnit XMLs and zero build logs while every HTTPS-sourced artifact was fine. Setting CLOUDSDK_AUTH_DISABLE_CREDENTIALS when no active account exists makes gcloud read public objects anonymously, so the failure is now prevented rather than merely reported. Resolved once under a lock, since collectors run in a thread pool. Verified with an empty CLOUDSDK_CONFIG (no credentials at all): data_complete true, 73 blocking test failures, test_failure_count 68/31, 8 JUnit XMLs, 4 build logs, exit 0 — previously 0/0/15-empty. Authenticated runs are unchanged. --- plugins/ci/skills/payload-snapshot/SKILL.md | 7 +++ .../scripts/payload_snapshot.py | 46 +++++++++++++++---- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/plugins/ci/skills/payload-snapshot/SKILL.md b/plugins/ci/skills/payload-snapshot/SKILL.md index 1bb991086..5be05704f 100644 --- a/plugins/ci/skills/payload-snapshot/SKILL.md +++ b/plugins/ci/skills/payload-snapshot/SKILL.md @@ -188,6 +188,13 @@ Comprehensive stream-level triage data — start here. Contains: - `recovered: true` means a fallback subsequently obtained the data. These entries are diagnostic only (useful for spotting a timeout that needs tuning) and do **not** make `data_complete` false. - `data_complete` is `false` only when at least one error was **not** recovered. +#### gcloud credentials are not required + +The CI artifact buckets are public. When gcloud has no active account it is +run in anonymous mode automatically, so an unauthenticated environment still +produces a complete snapshot. Authenticate only if you also need private +buckets. + #### Missing data is absent, never empty When a collection step fails, the affected file is **not written** and the diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index b7311e1fa..9fa702828 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -17,6 +17,7 @@ import subprocess import sys import tempfile +import threading import time import urllib.error import urllib.parse @@ -2409,6 +2410,37 @@ def _job_junit_failed(job_name: str) -> bool: ) +# Whether gcloud must be run in anonymous mode. Resolved once, on first +# use, under a lock — collectors run in a thread pool. +_GCLOUD_ANONYMOUS: Optional[bool] = None +_GCLOUD_ANONYMOUS_LOCK = threading.Lock() + + +def _gcloud_env() -> dict: + """Environment for gcloud calls, enabling anonymous access if needed. + + The CI artifact buckets are public, but ``gcloud storage`` refuses + client-side when no account is configured ("You do not currently have + an active account selected") — it never even issues the request. + Setting ``CLOUDSDK_AUTH_DISABLE_CREDENTIALS`` makes it read public + objects anonymously, so an unauthenticated environment still gets a + complete snapshot instead of an empty one. + """ + global _GCLOUD_ANONYMOUS + if _GCLOUD_ANONYMOUS is None: + with _GCLOUD_ANONYMOUS_LOCK: + if _GCLOUD_ANONYMOUS is None: + anonymous = not _check_gcloud_credentials() + if anonymous: + _log(" note: no gcloud credentials found — reading " + "public artifact buckets anonymously") + _GCLOUD_ANONYMOUS = anonymous + env = os.environ.copy() + if _GCLOUD_ANONYMOUS: + env["CLOUDSDK_AUTH_DISABLE_CREDENTIALS"] = "true" + return env + + def _run_gcloud(args: list[str], timeout: int = 120) -> Optional[str]: """Run a gcloud CLI command, returning stdout or None on error. @@ -2418,7 +2450,8 @@ def _run_gcloud(args: list[str], timeout: int = 120) -> Optional[str]: """ try: result = subprocess.run( - args, capture_output=True, text=True, timeout=timeout + args, capture_output=True, text=True, timeout=timeout, + env=_gcloud_env(), ) if result.returncode != 0: reason = _classify_gcloud_stderr(result.stderr) @@ -2443,7 +2476,7 @@ def _run_gcloud_bytes(args: list[str], timeout: int = 120) -> Optional[bytes]: """ try: result = subprocess.run( - args, capture_output=True, timeout=timeout + args, capture_output=True, timeout=timeout, env=_gcloud_env() ) if result.returncode != 0: stderr = (result.stderr or b"").decode("utf-8", errors="replace") @@ -2823,12 +2856,9 @@ def main() -> None: _log("Install gcloud SDK to enable JUnit download and regression tracking.\n") collect_junit = False elif collect_junit and not _check_gcloud_credentials(): - _log("Warning: 'gcloud' has no active credentials " - "('gcloud auth list' is empty).") - _log("Reads of non-public artifacts will fail. Run " - "'gcloud auth login' to authenticate.") - _log("Collection failures will be recorded in " - "summary.json 'collection_errors'.\n") + _log("Note: 'gcloud' has no active credentials. CI artifact buckets") + _log("are public, so they will be read anonymously. Run") + _log("'gcloud auth login' if you need private buckets too.\n") collect_rpmdb = not args.no_rpmdb if collect_rpmdb and not _check_podman(): From 30043f931e730bb6bb02430036035d6104640e7d Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 09:48:44 -0400 Subject: [PATCH 5/7] =?UTF-8?q?payload-snapshot:=20address=20review=20?= =?UTF-8?q?=E2=80=94=20partial=20reads,=20error=20scoping,=20recovery=20bo?= =?UTF-8?q?unds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from review, all confirmed against the code and each verified with a targeted run: - Do not recover failed fallback probes. _mark_errors_recovered marked every error since the recursive glob as recovered, including failures the fallback itself hit while probing other step directories. Those are potentially missing data, not recovered data. The range is now bounded to the glob's own errors. Verified: glob fails + one probe fails -> 4 recovered, 12 unrecovered, data_complete false, exit 1 (previously all 16 recovered and data_complete true). - Partial JUnit reads no longer look authoritative. The guard only fired when nothing downloaded, so a job with two JUnit files where one failed published a count derived from half the data. Partial reads now record a junit_partial error and the job entry carries junit_collection_partial: true alongside test_failure_count, which is documented as a lower bound. Verified: count=66 partial=True where a complete read gives 68. - Scope failure lookups by payload. The same job name recurs in every payload of the chain, so matching on job name alone against the global error list could stamp one payload's job entry with another payload's failure. Errors now carry payload_tag and _job_junit_state matches on it. - Rename ambiguous `l` loop variables (Ruff E741) in the fallback. Reduces the file's ruff errors from 4 to 3; the remaining 3 are pre-existing on main and out of scope here. Regression sweep, all unchanged or improved: authenticated (complete, 73 tests), unauthenticated (complete, 73), glob-fails-fallback-recovers (complete, 73), all-reads-fail (incomplete, no results.json, exit 1). --- .../scripts/payload_snapshot.py | 81 +++++++++++++++---- 1 file changed, 65 insertions(+), 16 deletions(-) diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index 9fa702828..e1bb22014 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -720,6 +720,7 @@ def collect(self) -> bool: errors_before = _unrecovered_error_count() junit_files = self._list_junit_files() downloaded = 0 + failed_downloads = 0 for gcs_uri in junit_files: filename = os.path.basename(gcs_uri) local_path = os.path.join(self.output_dir, filename) @@ -735,6 +736,8 @@ def collect(self) -> bool: downloaded += 1 results = _parse_junit_xml(local_path, source_name=filename) all_results.extend(results) + else: + failed_downloads += 1 gcloud_failed = _unrecovered_error_count() > errors_before @@ -753,6 +756,7 @@ def collect(self) -> bool: ), stage="junit", job=self.job.name, + payload_tag=self.payload_tag, ) _log( f" ERROR: could not read JUnit for {self.job.name} — " @@ -760,6 +764,26 @@ def collect(self) -> bool: ) return False + # Some files read, others not. The results are real but partial, so + # publish them and mark them partial — a count derived from half the + # JUnit is not authoritative and must not read as one that is. + if failed_downloads: + _record_collection_error( + "junit_partial", + ["gcloud", "storage", "cat", ""], + detail=( + f"{failed_downloads} of {len(junit_files)} JUnit file(s) " + f"could not be read; test_failure_count is a lower bound" + ), + stage="junit", + job=self.job.name, + payload_tag=self.payload_tag, + ) + _log( + f" WARNING: partial JUnit for {self.job.name} — " + f"{failed_downloads}/{len(junit_files)} file(s) unread" + ) + failures = _test_results_to_json(all_results) _write_json(self.output_path, failures) @@ -792,9 +816,10 @@ def _list_junit_files(self) -> list[str]: ["gcloud", "storage", "ls", f"{base}/artifacts/**/junit*.xml"], timeout=120, ) + glob_errors_end = _collection_error_count() - files = [l.strip() for l in (output or "").strip().splitlines() - if l.strip()] + files = [line.strip() for line in (output or "").strip().splitlines() + if line.strip()] if not files: _log( @@ -803,9 +828,9 @@ def _list_junit_files(self) -> list[str]: ) files = self._list_junit_files_fallback(base) if files: - # The data was obtained after all; don't report the failed - # first attempt as missing data. - _mark_errors_recovered(errors_before) + # The glob's failure is recovered; any probe failures the + # fallback itself hit are NOT — they may be missing data. + _mark_errors_recovered(errors_before, glob_errors_end) if not files: return [] @@ -868,8 +893,8 @@ def _list_junit_files_fallback(self, base: str) -> list[str]: timeout=60, ) if top: - found.extend(l.strip() for l in top.strip().splitlines() - if l.strip()) + found.extend(line.strip() for line in top.strip().splitlines() + if line.strip()) for step in step_dirs: patterns = [ @@ -888,8 +913,8 @@ def _list_junit_files_fallback(self, base: str) -> list[str]: ) if probe: found.extend( - l.strip() for l in probe.strip().splitlines() - if l.strip() + line.strip() for line in probe.strip().splitlines() + if line.strip() ) found = sorted(set(found)) @@ -1617,7 +1642,15 @@ def _build_failed_job_details(self, lifecycle: str) -> list[dict]: ) results_data = _read_json(results_path) or [] entry["test_failure_count"] = len(results_data) - elif _job_junit_failed(job_name): + if _job_junit_state( + job_name, self.target_tag, "junit_partial" + ): + # Real results, but not all of them: the count is a + # lower bound, not an authoritative total. + entry["junit_collection_partial"] = True + elif _job_junit_state( + job_name, self.target_tag, "junit_unavailable" + ): # Data could not be read. Do NOT emit test_failure_count — # its absence means "unknown", where 0 would mean "clean". entry["junit_collection_failed"] = True @@ -2357,6 +2390,7 @@ def _record_collection_error( detail: str = "", stage: str = "", job: str = "", + payload_tag: str = "", ) -> None: """Record a data-collection failure so it can be surfaced, not swallowed. @@ -2371,6 +2405,8 @@ def _record_collection_error( entry["stage"] = stage if job: entry["job"] = job + if payload_tag: + entry["payload_tag"] = payload_tag _COLLECTION_ERRORS.append(entry) @@ -2379,15 +2415,21 @@ def _collection_error_count() -> int: return len(_COLLECTION_ERRORS) -def _mark_errors_recovered(since_index: int) -> None: - """Mark errors recorded since ``since_index`` as recovered. +def _mark_errors_recovered( + since_index: int, until_index: Optional[int] = None +) -> None: + """Mark errors in ``[since_index, until_index)`` as recovered. A first-choice read can fail (e.g. a glob times out) and a fallback can then succeed. The failure is still worth reporting — it is how you learn a timeout needs tuning — but it must not make the snapshot look incomplete when the data was in fact obtained. + + The range is bounded deliberately: a fallback that finds *some* files + may itself have failed other probes, and those failures are real + missing data, not recovered ones. """ - for entry in _COLLECTION_ERRORS[since_index:]: + for entry in _COLLECTION_ERRORS[since_index:until_index]: entry["recovered"] = True @@ -2401,10 +2443,17 @@ def _unrecovered_error_count() -> int: return len(_unrecovered_errors()) -def _job_junit_failed(job_name: str) -> bool: - """True when JUnit collection for this job failed to read its data.""" +def _job_junit_state(job_name: str, payload_tag: str, reason: str) -> bool: + """True when this payload's collection of this job hit ``reason``. + + Scoped by payload tag: the same job name recurs in every payload of + the chain, so an unscoped match would stamp one payload's job entry + with another payload's failure. + """ return any( - e.get("job") == job_name and e.get("reason") == "junit_unavailable" + e.get("job") == job_name + and e.get("payload_tag") == payload_tag + and e.get("reason") == reason and not e.get("recovered") for e in _COLLECTION_ERRORS ) From 6993ef24ece3ee8002d2f8b3f2e95e2a021b6fa7 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Sun, 26 Jul 2026 10:13:18 -0400 Subject: [PATCH 6/7] payload-snapshot: close the remaining completeness holes from deep review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five confirmed paths could still label missing or corrupt data as complete or as an authoritative zero. All five are fixed, each with regression coverage in the new test module. 1. Recovery bookkeeping was unsafe across concurrent collectors. Errors were identified by index into the process-global ledger, so another worker appending between one collector's start and end offsets could have its error marked recovered. Replaced with per-operation error scopes (thread-local stack): a caller now recovers the specific error objects its own calls produced. Global appends take a lock. 2. --fail-on-incomplete succeeded when gcloud was absent. Preflight disabled JUnit collection before any error could be recorded, so the gate saw an empty ledger. Missing gcloud with JUnit requested now records gcloud_missing. 3. Zero discovered JUnit was published as a verified zero. These collectors only run for jobs that failed, so "no JUnit anywhere" is unknown, not clean. Now records junit_missing and writes no results.json. 4. Malformed XML was published as a verified zero. _parse_junit_xml swallowed ET.ParseError and returned [] — indistinguishable from a valid file with no failures. It now returns None, and the collector records junit_unparseable and counts the file as unread. 5. Re-running a partial snapshot promoted it to complete. Errors were process-local while results.json persisted, and collectors skip existing output. The ledger is now persisted to collection_errors.json and, on startup, JUnit output a previous run recorded as incomplete is discarded so it is re-collected and re-judged. Also: sanitize gcloud stderr before persisting it (strips control characters, collapses whitespace, caps length) so shareable summaries cannot carry terminal-spoofing sequences; stop logging "data is complete" when recovered and unrecovered errors coexist; document junit_partial, junit_missing, junit_unparseable, junit_collection_partial, lower-bound count semantics, --fail-on-incomplete, and the resume behaviour; and correct the prerequisite that claimed gcloud authentication is required. Tests: 21 unit tests covering classification, no-match-is-not-an-error, scoped and concurrent recovery, parse failure, all four publish decisions, payload scoping, and resume invalidation. Verified end to end: missing gcloud exits 1; a partial run followed by a healthy rerun re-collects and reaches 68/31 complete rather than inheriting 66/29. --- plugins/ci/skills/payload-snapshot/SKILL.md | 33 +- .../scripts/payload_snapshot.py | 229 +++++++++++-- .../scripts/test_collection_completeness.py | 310 ++++++++++++++++++ 3 files changed, 541 insertions(+), 31 deletions(-) create mode 100644 plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py diff --git a/plugins/ci/skills/payload-snapshot/SKILL.md b/plugins/ci/skills/payload-snapshot/SKILL.md index 5be05704f..93a2432cc 100644 --- a/plugins/ci/skills/payload-snapshot/SKILL.md +++ b/plugins/ci/skills/payload-snapshot/SKILL.md @@ -28,8 +28,10 @@ Use this skill when you need to: 3. **Google Cloud SDK (`gcloud`)** — for JUnit test result download - Install: `brew install google-cloud-sdk` (macOS) or see https://cloud.google.com/sdk - - Authenticate: `gcloud auth login` - - Without `gcloud`, JUnit data is skipped; job directories still created + - **Authentication is not required** — the CI artifact buckets are public and + are read anonymously when no account is configured + - Without `gcloud` entirely, JUnit data is skipped and the snapshot is + reported as incomplete (see [Data completeness](#data-completeness)) 4. **Container runtime (`podman`)** — for RHCOS RPMDB extraction - Install: available in most Linux distributions; `brew install podman` on macOS @@ -165,6 +167,7 @@ Options: --workers N Parallel workers for API calls (default: 8) --no-junit Skip JUnit download and regression tracking --no-rpmdb Skip RHCOS RPMDB extraction + --fail-on-incomplete Exit 1 if any requested data could not be collected ``` ## Output Files @@ -184,10 +187,11 @@ Comprehensive stream-level triage data — start here. Contains: - `payloads[]` — per-payload entries with `tag`, `phase`, relative file paths, `prs[]` with component/diff/comments paths, and `rhcos_changes[]` with RPM diffs per RHCOS variant - `rhcos_rpms[]` — RPMDB metadata for the target payload's RHCOS variants: `tag`, `name`, `pullspec`, `rpmdb` (relative path to rpmdb.sqlite) - `data_complete` — `true` when every collection step succeeded. `false` means part of the snapshot could not be read. -- `collection_errors[]` — every read failure encountered. Each entry has `reason` (`auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable`, `build_log_unavailable`), `command`, and optionally `detail`, `stage`, `job`, `recovered`. +- `collection_errors[]` — every read failure encountered. Each entry has `reason`, `command`, and optionally `detail`, `stage`, `job`, `payload_tag`, `recovered`. Reasons: `auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable` (nothing readable), `junit_missing` (nothing discovered), `junit_unparseable` (corrupt XML), `junit_partial` (some files unread), `build_log_unavailable`. - `recovered: true` means a fallback subsequently obtained the data. These entries are diagnostic only (useful for spotting a timeout that needs tuning) and do **not** make `data_complete` false. - `data_complete` is `false` only when at least one error was **not** recovered. + #### gcloud credentials are not required The CI artifact buckets are public. When gcloud has no active account it is @@ -207,12 +211,24 @@ list or a zero count. Specifically: - An absent `test_failure_count` therefore means *unknown*, whereas `0` means *verified clean*. +- If **some** JUnit files were read and others were not, `results.json` holds + the real results that were obtained and the job entry adds + `junit_collection_partial: true`. `test_failure_count` is then a **lower + bound**, not a total. +- A failed job with **no** JUnit discovered at all, or whose XML would not + parse, is recorded (`junit_missing` / `junit_unparseable`) and left without + a count — a job whose tests may never have run is unknown, not clean. + Consumers **must** distinguish these. Treating unreadable data as "no test failures" makes a broken job look like it failed for some other reason, which misdirects root-cause analysis. Check `data_complete` before drawing any conclusion from an absence of test failures. Use `--fail-on-incomplete` in automation to exit non-zero rather than emit a partial snapshot. +State is persisted to `collection_errors.json` beside `summary.json`, so a +later process can tell that an existing `results.json` came from an incomplete +read. + ### `AGENTS.md` / `CLAUDE.md` Dynamic orientation document generated at snapshot time. Contains the specific payload tag, chain, failed jobs, file layout, key concepts, and summary.json schema. `CLAUDE.md` imports `AGENTS.md` via `@AGENTS.md`. @@ -297,9 +313,14 @@ Aggregated jobs run the same underlying test multiple times with statistical ana - **Tag not found**: Exits with code 2 and a descriptive error - **Release controller unreachable**: Exits with code 1 - **`gh` not authenticated**: Prints a warning and continues without PR data -- **`gcloud` not available**: Prints a warning and skips JUnit download -- **Individual job/PR fetch failure**: Logs a warning and continues -- **Idempotent**: Re-running skips files that already exist +- **`gcloud` not available**: Warns, skips JUnit download, and records + `gcloud_missing` so the snapshot is reported incomplete +- **`gcloud` not authenticated**: Reads the public buckets anonymously +- **Individual job/PR fetch failure**: Logs a warning, records a collection + error, and continues +- **Idempotent**: Re-running skips files that already exist — except JUnit + output a previous run recorded as incomplete, which is discarded and + re-collected so a partial snapshot cannot be inherited as complete ## Notes diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index e1bb22014..581744afb 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -717,8 +717,46 @@ def collect(self) -> bool: os.makedirs(self.output_dir, exist_ok=True) all_results: list[_TestResult] = [] - errors_before = _unrecovered_error_count() - junit_files = self._list_junit_files() + with _error_scope() as own_errors: + junit_files = self._list_junit_files() + downloaded, failed_downloads = self._download_and_parse( + junit_files, all_results + ) + gcloud_failed = any( + not e.get("recovered") for e in own_errors + ) + + # Nothing was discovered at all. These collectors only run for + # jobs that FAILED, so "no JUnit anywhere" is not evidence of a + # clean run — it is an unknown. Publishing [] here would report a + # verified zero for a job whose tests may never have run. + if not junit_files: + _record_collection_error( + "junit_missing", + ["gcloud", "storage", "ls", ""], + detail=( + "no JUnit artifacts discovered for a failed job; " + "test results are unknown, not zero" + ), + stage="junit", + job=self.job.name, + payload_tag=self.payload_tag, + ) + _log( + f" ERROR: no JUnit discovered for {self.job.name} — " + f"leaving results.json absent (unknown, not zero)" + ) + return False + + return self._publish( + junit_files, all_results, downloaded, failed_downloads, + gcloud_failed, + ) + + def _download_and_parse( + self, junit_files: list[str], all_results: list + ) -> tuple[int, int]: + """Fetch each JUnit file and parse it; count successes/failures.""" downloaded = 0 failed_downloads = 0 for gcs_uri in junit_files: @@ -733,14 +771,31 @@ def collect(self) -> bool: f.write(data) if os.path.exists(local_path): - downloaded += 1 results = _parse_junit_xml(local_path, source_name=filename) + if results is None: + # Downloaded but unparseable. Treated as unread, not as + # "no failures" — corrupt XML must never become a zero. + failed_downloads += 1 + _record_collection_error( + "junit_unparseable", + ["parse", filename], + detail=f"{filename} is not valid JUnit XML", + stage="junit", + job=self.job.name, + payload_tag=self.payload_tag, + ) + continue + downloaded += 1 all_results.extend(results) else: failed_downloads += 1 + return downloaded, failed_downloads - gcloud_failed = _unrecovered_error_count() > errors_before - + def _publish( + self, junit_files: list[str], all_results: list, + downloaded: int, failed_downloads: int, gcloud_failed: bool, + ) -> bool: + """Write results.json unless doing so would misreport the data.""" # Refuse to publish an empty result set that was caused by a read # failure. Writing `[]` here makes the job look clean, and # downstream consumers report "0 test failures" for a job that @@ -811,12 +866,12 @@ def _list_junit_files(self) -> list[str]: bucket_path = self.job.gcs_bucket_path base = f"gs://{bucket_path}" - errors_before = _collection_error_count() - output = _run_gcloud( - ["gcloud", "storage", "ls", f"{base}/artifacts/**/junit*.xml"], - timeout=120, - ) - glob_errors_end = _collection_error_count() + with _error_scope() as glob_errors: + output = _run_gcloud( + ["gcloud", "storage", "ls", + f"{base}/artifacts/**/junit*.xml"], + timeout=120, + ) files = [line.strip() for line in (output or "").strip().splitlines() if line.strip()] @@ -830,7 +885,7 @@ def _list_junit_files(self) -> list[str]: if files: # The glob's failure is recovered; any probe failures the # fallback itself hit are NOT — they may be missing data. - _mark_errors_recovered(errors_before, glob_errors_end) + _mark_errors_recovered(glob_errors) if not files: return [] @@ -1419,6 +1474,7 @@ def generate(self) -> None: summary["data_complete"] = not _unrecovered_errors() _write_json(os.path.join(self.base_dir, "summary.json"), summary) + _write_collection_state(self.base_dir) self._write_agents_md(summary) _log(" Generated summary.json, AGENTS.md, and CLAUDE.md") @@ -1806,6 +1862,11 @@ def run(self) -> None: ) os.makedirs(base_dir, exist_ok=True) + # A previous run over this directory may have left JUnit output that + # it recorded as incomplete. Drop it so it is re-collected rather + # than inherited as though it were trustworthy. + _invalidate_suspect_junit(base_dir) + self._collect_streams(base_dir) chain = self._build_chain() @@ -2353,6 +2414,14 @@ def _check_gh_auth() -> bool: # snapshot could not read data that may well exist, and callers must be able # to tell that apart from "there was nothing to find". _COLLECTION_ERRORS: list[dict] = [] +_COLLECTION_ERRORS_LOCK = threading.Lock() + +# Per-thread stack of "operation scopes". Every recorded error is appended +# to each active scope as well as the global ledger, so a caller can act on +# exactly the errors *its own* calls produced. Positional indexes into the +# global list are not safe here: collectors run concurrently, so another +# worker can append between one collector's start and end offsets. +_ERROR_SCOPES = threading.local() # stderr fragments that mean "the object does not exist", not "I failed". _GCLOUD_NO_MATCH_PATTERNS = ( @@ -2374,6 +2443,44 @@ def _check_gh_auth() -> bool: ) +def _error_scope(): + """Collect errors recorded by this thread inside the ``with`` body.""" + return _ErrorScope() + + +class _ErrorScope: + """Context manager yielding the list of errors recorded inside it.""" + + def __init__(self) -> None: + self.entries: list[dict] = [] + + def __enter__(self) -> list[dict]: + stack = getattr(_ERROR_SCOPES, "stack", None) + if stack is None: + stack = [] + _ERROR_SCOPES.stack = stack + stack.append(self.entries) + return self.entries + + def __exit__(self, *exc) -> None: + getattr(_ERROR_SCOPES, "stack", []).pop() + return None + + +def _sanitize_detail(text: str) -> str: + """Make tool stderr safe to persist in a shareable summary. + + Strips control characters (which can spoof terminal output when the + summary is read back) and collapses whitespace. Diagnostics are + truncated: they exist to identify a failure class, not to carry full + tool output. + """ + cleaned = "".join( + ch if ch.isprintable() else " " for ch in (text or "") + ) + return re.sub(r"\s+", " ", cleaned).strip()[:300] + + def _classify_gcloud_stderr(stderr: str) -> str: """Classify a failed gcloud invocation from its stderr.""" low = (stderr or "").lower() @@ -2400,14 +2507,17 @@ def _record_collection_error( """ entry: dict = {"reason": reason, "command": " ".join(command[:4])} if detail: - entry["detail"] = detail.strip()[:500] + entry["detail"] = _sanitize_detail(detail) if stage: entry["stage"] = stage if job: entry["job"] = job if payload_tag: entry["payload_tag"] = payload_tag - _COLLECTION_ERRORS.append(entry) + with _COLLECTION_ERRORS_LOCK: + _COLLECTION_ERRORS.append(entry) + for scope in getattr(_ERROR_SCOPES, "stack", []): + scope.append(entry) def _collection_error_count() -> int: @@ -2415,21 +2525,73 @@ def _collection_error_count() -> int: return len(_COLLECTION_ERRORS) -def _mark_errors_recovered( - since_index: int, until_index: Optional[int] = None -) -> None: - """Mark errors in ``[since_index, until_index)`` as recovered. +COLLECTION_STATE_FILE = "collection_errors.json" + +# Reasons that make a job's persisted JUnit output untrustworthy. +_JUNIT_SUSPECT_REASONS = ( + "junit_unavailable", "junit_partial", "junit_missing", + "junit_unparseable", +) + + +def _invalidate_suspect_junit(base_dir: str) -> int: + """Discard JUnit output that a previous run recorded as incomplete. + + Collection errors live in process memory but ``results.json`` persists, + and collectors skip any job whose output already exists. Without this, + re-running over the same directory regenerates the summary from an empty + ledger and promotes a partial snapshot to "complete". Dropping the + suspect output forces it to be re-collected and re-judged. + """ + state_path = os.path.join(base_dir, COLLECTION_STATE_FILE) + previous = _read_json(state_path) + if not previous: + return 0 + + invalidated = 0 + for entry in previous: + if entry.get("recovered"): + continue + if entry.get("reason") not in _JUNIT_SUSPECT_REASONS: + continue + tag, job = entry.get("payload_tag"), entry.get("job") + if not tag or not job: + continue + for lifecycle in ("blocking", "informing"): + junit_dir = os.path.join( + base_dir, tag, "jobs", lifecycle, job, "junit" + ) + if os.path.isdir(junit_dir): + shutil.rmtree(junit_dir, ignore_errors=True) + invalidated += 1 + if invalidated: + _log(f" re-collecting {invalidated} job(s) whose previous JUnit " + f"collection was incomplete") + return invalidated + + +def _write_collection_state(base_dir: str) -> None: + """Persist the error ledger so a later process can see it.""" + state_path = os.path.join(base_dir, COLLECTION_STATE_FILE) + if _COLLECTION_ERRORS: + _write_json(state_path, list(_COLLECTION_ERRORS)) + elif os.path.exists(state_path): + # Everything was collected this time; drop the stale ledger. + os.remove(state_path) + + +def _mark_errors_recovered(entries: list[dict]) -> None: + """Mark the given error entries as recovered. A first-choice read can fail (e.g. a glob times out) and a fallback can then succeed. The failure is still worth reporting — it is how you learn a timeout needs tuning — but it must not make the snapshot look incomplete when the data was in fact obtained. - The range is bounded deliberately: a fallback that finds *some* files - may itself have failed other probes, and those failures are real - missing data, not recovered ones. + Entries are identified by object, not by position: only the specific + failures handed in are recovered, never a concurrent collector's. """ - for entry in _COLLECTION_ERRORS[since_index:until_index]: + for entry in entries: entry["recovered"] = True @@ -2731,12 +2893,17 @@ def _parse_system_out_yaml(text: str) -> dict: return result -def _parse_junit_xml(source, source_name: str = "") -> list[_TestResult]: +def _parse_junit_xml( + source, source_name: str = "" +) -> Optional[list[_TestResult]]: """Parse JUnit XML, returning a list of _TestResult.""" try: tree = ET.parse(source) except (ET.ParseError, OSError): - return [] + # Return None, not []: an unreadable or corrupt file is not the same + # as a file that legitimately contains no failures. Callers must be + # able to tell those apart or corruption becomes a verified zero. + return None root = tree.getroot() if root.tag == "testsuites": @@ -2904,6 +3071,16 @@ def main() -> None: _log("Warning: 'gcloud' CLI not found. JUnit data will not be fetched.") _log("Install gcloud SDK to enable JUnit download and regression tracking.\n") collect_junit = False + # JUnit was requested and cannot be collected, so the snapshot is + # incomplete by construction. Record it: otherwise the completeness + # gate sees an empty ledger and reports a complete snapshot. + _record_collection_error( + "gcloud_missing", + ["gcloud", "storage"], + detail=("gcloud CLI not available; JUnit, build logs and " + "regression tracking were skipped"), + stage="preflight", + ) elif collect_junit and not _check_gcloud_credentials(): _log("Note: 'gcloud' has no active credentials. CI artifact buckets") _log("are public, so they will be read anonymously. Run") @@ -2934,8 +3111,10 @@ def main() -> None: recovered = len(_COLLECTION_ERRORS) - len(unrecovered) if recovered: _log("") + status = ("data is complete" if not unrecovered + else "see the warning below for what is still missing") _log(f"Note: {recovered} read failure(s) were recovered by a " - f"fallback; data is complete. See 'collection_errors' " + f"fallback; {status}. See 'collection_errors' " f"(recovered: true) in summary.json.") if unrecovered: by_reason: dict[str, int] = {} diff --git a/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py b/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py new file mode 100644 index 000000000..5c6538486 --- /dev/null +++ b/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py @@ -0,0 +1,310 @@ +#!/usr/bin/env python3 +"""Regression tests for the snapshot completeness contract. + +The contract under test: data the snapshot could not read must never be +published as an authoritative zero. Each test below corresponds to a way +that guarantee was previously broken. + +Run with: pytest test_collection_completeness.py +""" + +import importlib.util +import json +import os +import sys +import threading + +import pytest + +_HERE = os.path.dirname(os.path.abspath(__file__)) + + +def _load_module(): + spec = importlib.util.spec_from_file_location( + "payload_snapshot", os.path.join(_HERE, "payload_snapshot.py") + ) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +ps = _load_module() + + +@pytest.fixture(autouse=True) +def clean_ledger(): + """Each test starts with an empty global error ledger.""" + ps._COLLECTION_ERRORS.clear() + ps._GCLOUD_ANONYMOUS = True # never probe real credentials in tests + yield + ps._COLLECTION_ERRORS.clear() + + +def _job(name="job-a", bucket="bucket/path"): + return ps.JobInfo( + name=name, state="Failed", lifecycle="blocking", url="", + retries=0, previous_attempt_urls=[], is_aggregated=False, + gcs_bucket_path=bucket, + ) + + +def _collector(tmp_path, job=None, tag="4.99.0-0.ci-2026-01-01-000000"): + return ps.JUnitCollector(str(tmp_path / "junit"), job or _job(), tag) + + +VALID_JUNIT = ( + '' + 'boom' + "" +) + + +# -------------------------------------------------------------------------- +# Error classification +# -------------------------------------------------------------------------- + +@pytest.mark.parametrize("stderr,expected", [ + ("ERROR: One or more URLs matched no objects.", "no_match"), + ("ERROR: 403 does not have storage.objects.list access", "auth"), + ("ERROR: Anonymous caller does not have access", "auth"), + ("ERROR: something unexpected", "command_failed"), +]) +def test_stderr_classification(stderr, expected): + assert ps._classify_gcloud_stderr(stderr) == expected + + +def test_no_match_is_not_recorded_as_error(monkeypatch): + """A missing object is absence, not failure — probes must stay quiet.""" + monkeypatch.setattr(ps.subprocess, "run", lambda *a, **k: type( + "R", (), {"returncode": 1, "stdout": "", + "stderr": "ERROR: matched no objects"})()) + assert ps._run_gcloud(["gcloud", "storage", "ls", "x"]) is None + assert ps._COLLECTION_ERRORS == [] + + +def test_auth_failure_is_recorded(monkeypatch): + monkeypatch.setattr(ps.subprocess, "run", lambda *a, **k: type( + "R", (), {"returncode": 1, "stdout": "", + "stderr": "ERROR: 403 forbidden"})()) + assert ps._run_gcloud(["gcloud", "storage", "ls", "x"]) is None + assert [e["reason"] for e in ps._COLLECTION_ERRORS] == ["auth"] + + +def test_detail_is_sanitized(): + """Persisted diagnostics must not carry control characters.""" + ps._record_collection_error( + "auth", ["gcloud"], detail="bad\x1b[31mred\x00\nthing" + ) + detail = ps._COLLECTION_ERRORS[0]["detail"] + assert "\x1b" not in detail and "\x00" not in detail + assert "\n" not in detail + + +# -------------------------------------------------------------------------- +# Recovery bookkeeping +# -------------------------------------------------------------------------- + +def test_recovery_is_scoped_to_its_own_operation(): + """A recovered fallback must not clear another operation's error.""" + with ps._error_scope() as mine: + ps._record_collection_error("timeout", ["gcloud"], job="mine") + # A concurrent collector records its own, unrelated failure. + ps._record_collection_error("auth", ["gcloud"], job="theirs") + + # Only the entry captured for *this* operation is recovered. + ps._mark_errors_recovered([e for e in mine if e.get("job") == "mine"]) + + by_job = {e["job"]: e for e in ps._COLLECTION_ERRORS} + assert by_job["mine"].get("recovered") is True + assert by_job["theirs"].get("recovered") is None + assert len(ps._unrecovered_errors()) == 1 + + +def test_concurrent_collectors_do_not_recover_each_others_errors(): + """Threads racing on the shared ledger must stay isolated.""" + started = threading.Barrier(2) + + def worker(tag): + with ps._error_scope() as own: + ps._record_collection_error("timeout", ["gcloud"], job=tag) + started.wait(timeout=5) # force interleaving + if tag == "recovers": + ps._mark_errors_recovered(own) + + threads = [threading.Thread(target=worker, args=(t,)) + for t in ("recovers", "keeps")] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + by_job = {e["job"]: e for e in ps._COLLECTION_ERRORS} + assert by_job["recovers"].get("recovered") is True + assert by_job["keeps"].get("recovered") is None + + +def test_data_complete_ignores_recovered_errors(): + with ps._error_scope() as own: + ps._record_collection_error("timeout", ["gcloud"]) + ps._mark_errors_recovered(own) + assert ps._unrecovered_errors() == [] + + +# -------------------------------------------------------------------------- +# Parsing +# -------------------------------------------------------------------------- + +def test_malformed_xml_returns_none_not_empty(tmp_path): + """Corrupt XML must be distinguishable from "no failures".""" + bad = tmp_path / "junit_bad.xml" + bad.write_text('trunc') + assert ps._parse_junit_xml(str(bad)) is None + + +def test_valid_xml_with_no_failures_returns_empty_list(tmp_path): + good = tmp_path / "junit_ok.xml" + good.write_text('' + "") + results = ps._parse_junit_xml(str(good)) + assert results is not None + assert ps._test_results_to_json(results) == [] + + +# -------------------------------------------------------------------------- +# Publishing decisions +# -------------------------------------------------------------------------- + +def _stub_discovery(monkeypatch, files): + monkeypatch.setattr( + ps.JUnitCollector, "_list_junit_files", lambda self: list(files) + ) + + +def test_unreadable_junit_leaves_results_absent(tmp_path, monkeypatch): + """Read failure => results.json absent (unknown), never [].""" + _stub_discovery(monkeypatch, ["gs://b/junit_operator.xml"]) + monkeypatch.setattr(ps, "_run_gcloud_bytes", lambda *a, **k: ( + ps._record_collection_error("auth", ["gcloud"]) or None)) + + c = _collector(tmp_path) + assert c.collect() is False + assert not os.path.exists(c.output_path) + assert any(e["reason"] == "junit_unavailable" + for e in ps._COLLECTION_ERRORS) + + +def test_no_junit_discovered_is_unknown_not_zero(tmp_path, monkeypatch): + """A failed job with no JUnit at all is unknown, not verified clean.""" + _stub_discovery(monkeypatch, []) + + c = _collector(tmp_path) + assert c.collect() is False + assert not os.path.exists(c.output_path) + assert any(e["reason"] == "junit_missing" + for e in ps._COLLECTION_ERRORS) + + +def test_partial_read_is_published_but_flagged(tmp_path, monkeypatch): + """Half the files read => real results, explicitly marked partial.""" + _stub_discovery(monkeypatch, ["gs://b/junit_a.xml", "gs://b/junit_b.xml"]) + + def fake_cat(args, **kwargs): + if args[-1].endswith("junit_a.xml"): + return VALID_JUNIT.encode() + ps._record_collection_error("auth", ["gcloud"]) + return None + + monkeypatch.setattr(ps, "_run_gcloud_bytes", fake_cat) + + c = _collector(tmp_path) + assert c.collect() is True + assert json.load(open(c.output_path)) # real failures published + assert any(e["reason"] == "junit_partial" + for e in ps._COLLECTION_ERRORS) + + +def test_unparseable_download_counts_as_unread(tmp_path, monkeypatch): + """Corrupt XML must not be published as zero failures.""" + _stub_discovery(monkeypatch, ["gs://b/junit_bad.xml"]) + monkeypatch.setattr( + ps, "_run_gcloud_bytes", lambda *a, **k: b" Date: Sun, 26 Jul 2026 10:28:14 -0400 Subject: [PATCH 7/7] payload-snapshot: fix --no-junit data loss, guard rmtree, redact diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six further review findings, all confirmed. --no-junit over an existing directory was destructive. Invalidation ran unconditionally while re-collection is gated on collect_junit, so the run deleted the suspect junit/ dirs, replaced nothing, and rewrote the ledger from an empty in-memory list — reporting data_complete: true for a snapshot it had just stripped. Gating the delete alone is not enough: the ledger would still be dropped and the stale partial output still counted. Invalidation now runs only when JUnit will be re-collected, and otherwise the unresolved JUnit errors are carried forward so the snapshot stays honestly incomplete. Verified: run 1 partial (4 XMLs, 66/29, incomplete), run 2 with --no-junit keeps all 4 XMLs, stays incomplete, exits 1. The recovery ledger is data read back from disk, so it must not be able to aim a recursive delete. Path components are now rejected unless they are safe single components, and the resolved target must be contained within the snapshot directory. Persisted diagnostics are redacted, not just stripped of control characters: gcloud auth errors routinely name the active account and URLs can carry signed-request parameters, and summary.json exists to be handed to agents and shared. Docs: define data_complete as all requested data being ultimately collected (a recovered failure is not incompleteness); scope the "no count" rule to JUnit that was entirely unreadable, since a parse failure alongside readable files is the partial case; and mark Step 3.3's job-level candidate list as preliminary, requiring per-mode candidates from first_failed_in before scoring — otherwise the causal PR can be excluded from the candidate set entirely. Tests: 25 total, adding coverage for unsafe ledger entries (no delete outside base), carry-forward on --no-junit, and identifier redaction. --- plugins/ci/skills/payload-analysis/SKILL.md | 2 +- plugins/ci/skills/payload-snapshot/SKILL.md | 10 +-- .../scripts/payload_snapshot.py | 72 +++++++++++++++++-- .../scripts/test_collection_completeness.py | 44 ++++++++++++ 4 files changed, 118 insertions(+), 10 deletions(-) diff --git a/plugins/ci/skills/payload-analysis/SKILL.md b/plugins/ci/skills/payload-analysis/SKILL.md index d3567243c..d1d147e9b 100644 --- a/plugins/ci/skills/payload-analysis/SKILL.md +++ b/plugins/ci/skills/payload-analysis/SKILL.md @@ -139,7 +139,7 @@ For each failed job's `streak.originating_payload`, find the matching entry in ` - `url`, `component`, `number`, `description` - Paths to local artifacts: `diff`, `comments`, `jobs` -These PRs are the **candidates** for failures that started in that originating payload. +Treat this as a **preliminary** list only. The job-level streak merges unrelated failure modes, so its originating payload is frequently earlier than the regression being investigated — and candidates gathered from it can omit the causal PR entirely. Before scoring, re-derive the originating payload **per failure mode** from `test_failures.blocking[].first_failed_in` (Step 5) and collect the candidates from *that* payload. #### 3.4: Test Failure Details diff --git a/plugins/ci/skills/payload-snapshot/SKILL.md b/plugins/ci/skills/payload-snapshot/SKILL.md index 93a2432cc..4a13372e7 100644 --- a/plugins/ci/skills/payload-snapshot/SKILL.md +++ b/plugins/ci/skills/payload-snapshot/SKILL.md @@ -186,7 +186,7 @@ Comprehensive stream-level triage data — start here. Contains: - `test_failures.blocking[]` — `test_name`, `jobs`, `first_failed_in`, `payloads_failing`, `failure_message`, `failure_text` (full, not truncated) - `payloads[]` — per-payload entries with `tag`, `phase`, relative file paths, `prs[]` with component/diff/comments paths, and `rhcos_changes[]` with RPM diffs per RHCOS variant - `rhcos_rpms[]` — RPMDB metadata for the target payload's RHCOS variants: `tag`, `name`, `pullspec`, `rpmdb` (relative path to rpmdb.sqlite) -- `data_complete` — `true` when every collection step succeeded. `false` means part of the snapshot could not be read. +- `data_complete` — `true` when all requested data was ultimately collected, including via a fallback after an initial read failed. `false` means some requested data could not be read at all. - `collection_errors[]` — every read failure encountered. Each entry has `reason`, `command`, and optionally `detail`, `stage`, `job`, `payload_tag`, `recovered`. Reasons: `auth`, `timeout`, `gcloud_missing`, `command_failed`, `junit_unavailable` (nothing readable), `junit_missing` (nothing discovered), `junit_unparseable` (corrupt XML), `junit_partial` (some files unread), `build_log_unavailable`. - `recovered: true` means a fallback subsequently obtained the data. These entries are diagnostic only (useful for spotting a timeout that needs tuning) and do **not** make `data_complete` false. - `data_complete` is `false` only when at least one error was **not** recovered. @@ -215,9 +215,11 @@ list or a zero count. Specifically: the real results that were obtained and the job entry adds `junit_collection_partial: true`. `test_failure_count` is then a **lower bound**, not a total. -- A failed job with **no** JUnit discovered at all, or whose XML would not - parse, is recorded (`junit_missing` / `junit_unparseable`) and left without - a count — a job whose tests may never have run is unknown, not clean. +- A failed job with **no** readable JUnit at all — none discovered + (`junit_missing`), or nothing that parsed (`junit_unparseable`) — is left + without a count entirely. A job whose tests may never have run is unknown, + not clean. A parse failure alongside other readable files is the partial + case above, not this one. Consumers **must** distinguish these. Treating unreadable data as "no test failures" makes a broken job look like it failed for some other reason, which diff --git a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py index 581744afb..85fb9282a 100755 --- a/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py +++ b/plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py @@ -1862,10 +1862,15 @@ def run(self) -> None: ) os.makedirs(base_dir, exist_ok=True) - # A previous run over this directory may have left JUnit output that - # it recorded as incomplete. Drop it so it is re-collected rather - # than inherited as though it were trustworthy. - _invalidate_suspect_junit(base_dir) + # A previous run over this directory may have left JUnit output it + # recorded as incomplete. Drop it so it is re-collected rather than + # inherited as trustworthy — but only when we will actually + # re-collect, otherwise we would delete the only copy and then + # report the result as complete. + if self.collect_junit: + _invalidate_suspect_junit(base_dir) + else: + _carry_forward_junit_errors(base_dir) self._collect_streams(base_dir) @@ -2478,6 +2483,14 @@ def _sanitize_detail(text: str) -> str: cleaned = "".join( ch if ch.isprintable() else " " for ch in (text or "") ) + # These diagnostics are persisted into a summary this skill exists to + # hand to agents and share. gcloud auth errors routinely name the + # active account, and URLs can carry signed-request parameters; neither + # should leave the local environment. + cleaned = re.sub( + r"[\w.+-]+@[\w-]+\.[\w.-]+", "", cleaned + ) + cleaned = re.sub(r"\?\S+", "?", cleaned) return re.sub(r"\s+", " ", cleaned).strip()[:300] @@ -2534,6 +2547,20 @@ def _collection_error_count() -> int: ) +def _is_within(base: str, target: str) -> bool: + """True when ``target`` resolves inside ``base``.""" + base_r = os.path.realpath(base) + target_r = os.path.realpath(target) + return target_r == base_r or target_r.startswith(base_r + os.sep) + + +def _safe_component(value: str) -> bool: + """True when ``value`` is usable as a single path component.""" + return bool(value) and value not in (".", "..") and not any( + sep in value for sep in (os.sep, "/", "\\") + ) + + def _invalidate_suspect_junit(base_dir: str) -> int: """Discard JUnit output that a previous run recorded as incomplete. @@ -2555,12 +2582,18 @@ def _invalidate_suspect_junit(base_dir: str) -> int: if entry.get("reason") not in _JUNIT_SUSPECT_REASONS: continue tag, job = entry.get("payload_tag"), entry.get("job") - if not tag or not job: + # The ledger is data read back from disk; never let it aim a + # recursive delete at a path outside the snapshot directory. + if not _safe_component(tag or "") or not _safe_component(job or ""): + _log(f" warn: ignoring unsafe collection-state entry " + f"(tag={tag!r}, job={job!r})") continue for lifecycle in ("blocking", "informing"): junit_dir = os.path.join( base_dir, tag, "jobs", lifecycle, job, "junit" ) + if not _is_within(base_dir, junit_dir): + continue if os.path.isdir(junit_dir): shutil.rmtree(junit_dir, ignore_errors=True) invalidated += 1 @@ -2570,6 +2603,35 @@ def _invalidate_suspect_junit(base_dir: str) -> int: return invalidated +def _carry_forward_junit_errors(base_dir: str) -> int: + """Re-record persisted JUnit failures when JUnit is not re-collected. + + With ``--no-junit`` nothing re-reads the artifacts, so a previous run's + partial or unreadable JUnit is still exactly as incomplete as it was. + Without carrying those errors forward, the ledger would be rewritten + from an empty in-memory list and the stale output would be reported as + a complete snapshot. + """ + previous = _read_json( + os.path.join(base_dir, COLLECTION_STATE_FILE) + ) or [] + carried = 0 + for entry in previous: + if entry.get("recovered"): + continue + if entry.get("reason") not in _JUNIT_SUSPECT_REASONS: + continue + forwarded = dict(entry) + forwarded["carried_forward"] = True + with _COLLECTION_ERRORS_LOCK: + _COLLECTION_ERRORS.append(forwarded) + carried += 1 + if carried: + _log(f" carrying forward {carried} unresolved JUnit collection " + f"error(s) from a previous run (JUnit collection is disabled)") + return carried + + def _write_collection_state(base_dir: str) -> None: """Persist the error ledger so a later process can see it.""" state_path = os.path.join(base_dir, COLLECTION_STATE_FILE) diff --git a/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py b/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py index 5c6538486..a359c76cd 100644 --- a/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py +++ b/plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py @@ -291,6 +291,50 @@ def test_recovered_state_is_not_invalidated_on_rerun(tmp_path): assert junit_dir.exists() +def test_unsafe_ledger_entries_never_delete_outside_base(tmp_path): + """The ledger is data from disk; it must not aim rmtree at a parent.""" + base = tmp_path / "snap" / "5.0" / "ci" + base.mkdir(parents=True) + victim = tmp_path / "victim" + victim.mkdir() + (victim / "keep.txt").write_text("do not delete me") + + (base / ps.COLLECTION_STATE_FILE).write_text(json.dumps([ + {"reason": "junit_partial", "job": "../../../../victim", + "payload_tag": ".."}, + {"reason": "junit_partial", "job": "a/b", "payload_tag": "tag"}, + ])) + + assert ps._invalidate_suspect_junit(str(base)) == 0 + assert (victim / "keep.txt").exists() + + +def test_carry_forward_keeps_unresolved_junit_errors(tmp_path): + """--no-junit must not launder a stale partial snapshot into complete.""" + base = tmp_path / "5.0" / "ci" + base.mkdir(parents=True) + (base / ps.COLLECTION_STATE_FILE).write_text(json.dumps([ + {"reason": "junit_partial", "job": "job-a", "payload_tag": "tag-1"}, + {"reason": "auth", "job": "job-a", "payload_tag": "tag-1"}, + {"reason": "junit_partial", "job": "job-b", "payload_tag": "tag-1", + "recovered": True}, + ])) + + assert ps._carry_forward_junit_errors(str(base)) == 1 + carried = ps._unrecovered_errors() + assert [e["reason"] for e in carried] == ["junit_partial"] + assert carried[0]["carried_forward"] is True + + +@pytest.mark.parametrize("raw,forbidden", [ + ("ERROR: account someone@example.com lacks access", "someone@example.com"), + ("ERROR: GET https://x/o?X-Goog-Signature=abc123", "X-Goog-Signature"), +]) +def test_detail_redacts_identifiers(raw, forbidden): + ps._record_collection_error("auth", ["gcloud"], detail=raw) + assert forbidden not in ps._COLLECTION_ERRORS[0]["detail"] + + def test_collection_state_round_trips(tmp_path): base = tmp_path / "snap" base.mkdir()