payload-snapshot: fix silent data loss on unauthenticated gcloud (supersedes #641) - #644
Conversation
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.
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @not-stbenjam. Thanks for your PR. I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe CI payload snapshot flow now records gcloud collection failures, exposes incomplete snapshots through ChangesPayload snapshot completeness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SnapshotCLI
participant JUnitCollector
participant Gcloud
participant SummaryGenerator
participant SummaryJSON
SnapshotCLI->>Gcloud: check active credentials
JUnitCollector->>Gcloud: discover and download JUnit files
Gcloud-->>JUnitCollector: data or classified collection error
JUnitCollector->>SummaryGenerator: provide collected data and errors
SummaryGenerator->>SummaryJSON: write data_complete and collection_errors
SnapshotCLI->>SnapshotCLI: fail when --fail-on-incomplete detects unrecovered errors
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (2)
2231-2236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModule-level mutable global isn't reset between invocations.
_COLLECTION_ERRORSis a plain module-level list with no reset hook. For a single CLI run this is fine, but the PR description mentions test scenarios covering both "incomplete snapshot" and "successful artifact collection" — if those are exercised in-process (e.g. via pytest calling into this module repeatedly without re-importing it), errors recorded by one test will leak into the next test'sdata_complete/collection_errorsassertions.Consider adding a small
_reset_collection_errors()helper (or making this an attribute passed throughSnapshotter) that test fixtures/main()can call at the start of a run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 2231 - 2236, The module-level _COLLECTION_ERRORS list persists across in-process invocations and leaks failures between runs. Add a small _reset_collection_errors() helper and invoke it at the start of each CLI/run entry point, including main(), so data_complete and collection_errors reflect only the current snapshot collection.
2268-2354: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollection errors are double-recorded for the same underlying failure.
_run_gcloud/_run_gcloud_bytesalready call_record_collection_errorwith a raw reason (auth,timeout,gcloud_missing,command_failed) for every failed invocation.JUnitCollector.collect()(Line 745) andBuildLogCollector._fetch()(Line 862) then add a second, semantic entry (junit_unavailable/build_log_unavailable) for the same event. Worse, inJUnitCollector.collect()this raw error is recorded once per file inside the download loop, so a systemic auth failure with 2 junit files produces 3 entries (2×auth+ 1×junit_unavailable) for what is really a single credential problem. Across dozens of failed jobs in a broken-credentials run,collection_errors[]can balloon with near-duplicate entries (each carrying up to 500 chars of near-identicalstderrindetail), inflatingsummary.jsonsize and the "N collection error(s)" count shown in the CLI warning (Lines 2753-2754) far beyond the actual number of distinct problems.Consider deduplicating identical
(reason, stage, job)entries before appending, or suppressing the raw per-call reason once a semantic collector-level reason has been recorded for the same job/stage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 2268 - 2354, Prevent duplicate collection errors across _run_gcloud, _run_gcloud_bytes, JUnitCollector.collect, and BuildLogCollector._fetch by deduplicating entries using the same reason, stage, and job identity. Ensure repeated per-file or per-call failures for one job/stage produce only one recorded error while preserving distinct problems and the semantic collector-level errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 2296-2302: Scope JUnit collection failures by payload tag. Update
_record_collection_error and _job_junit_failed to accept and match an optional
payload_tag, pass self.payload_tag from JUnitCollector’s collection error call,
and pass self.target_tag from _build_failed_job_details so repeated job names
across payloads cannot be misattributed.
- Around line 733-761: Track whether any JUnit download fails during collection,
rather than only checking whether downloaded equals zero. Update the guard
around _collection_error_count and results.json generation to treat any failed
download as incomplete, record the collection error, and leave results.json
absent so partial data is not reported as authoritative.
---
Nitpick comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 2231-2236: The module-level _COLLECTION_ERRORS list persists
across in-process invocations and leaks failures between runs. Add a small
_reset_collection_errors() helper and invoke it at the start of each CLI/run
entry point, including main(), so data_complete and collection_errors reflect
only the current snapshot collection.
- Around line 2268-2354: Prevent duplicate collection errors across _run_gcloud,
_run_gcloud_bytes, JUnitCollector.collect, and BuildLogCollector._fetch by
deduplicating entries using the same reason, stage, and job identity. Ensure
repeated per-file or per-call failures for one job/stage produce only one
recorded error while preserving distinct problems and the semantic
collector-level errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a8a770d-3479-490e-b983-30b14e82fc6d
📒 Files selected for processing (6)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-analysis/SKILL.mdplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
…ounded-depth fallback Incorporates the JUnit discovery hardening from openshift-eng#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: openshift-eng#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
not-stbenjam
changed the title
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Line 795: Rename the ambiguous single-letter loop variable l in the list
comprehensions around the payload snapshot processing at the referenced
locations to a descriptive name such as line, updating each corresponding
strip() usage while preserving behavior.
- Around line 803-807: Update the fallback handling around
_list_junit_files_fallback so _mark_errors_recovered only receives errors
generated by the initial recursive glob, not errors from fallback probes.
Preserve failed fallback errors so data_complete remains false when any fallback
step may have missing JUnit data, including the corresponding logic at the other
reported occurrence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 186beb90-97d7-4d9c-b942-a1c2253432a1
📒 Files selected for processing (2)
plugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/ci/skills/payload-snapshot/SKILL.md
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.
…creds 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.
not-stbenjam
changed the title
…very bounds 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).
Deep Review VerdictDisposition: REQUEST_CHANGES — five data-integrity failures are confirmed on current head The unauthenticated-gcloud fix is directionally right, and the latest commit correctly addresses the earlier partial-download, payload-scoping, and bounded-fallback findings. However, the remaining paths below can still label missing or corrupt CI data as complete or as an authoritative zero. Specialist FindingsBugs / Adversarial
Security & Supply Chain No injection, unsafe shell composition, dependency, or credential-use blocker found. Suggest sanitizing the raw 500-character gcloud Architecture / Consistency The positional global error ledger is the core structural issue. Returning a structured per-operation result, or assigning stable collector/operation IDs, would make recovery and completeness ownership explicit. A lock around QA There are no automated tests for the roughly 400 lines of snapshot/error-handling changes. Syntax compilation and Technical Writer The skill docs need to describe Panel SynthesisAll functional reviewers converged on the same issue: completeness is inferred from a process-global error list rather than from explicit outcomes for each requested collection stage. Five runtime reproducers confirmed that this still permits silent promotion of missing, corrupt, concurrent, or previously-partial data. The latest commit's partial-read fixes are valid and were not re-raised. Required Actions Before Merge
Optional Follow-ups
StatsArbiter summary: 28 raw findings from 7 specialists. Kept: 5 blocking findings, all runtime-confirmed; 4 grouped suggestions/notes. Dropped: duplicates, findings fixed by Generated by /deep-review |
…view 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.
|
All five blocking findings are fixed in 1. Recovery bookkeeping across concurrent collectors — valid, and the underlying design was wrong, not just the bound. Errors were identified by index into the process-global ledger, so a concurrent worker appending between one collector's offsets could have its error recovered. Bounding the upper end (30043f9) only fixed the same-collector case, as you say. Replaced positional ranges with per-operation error scopes: a thread-local stack of scopes, where every recorded error is appended to each active scope as well as the global ledger. 2. 3. Zero discovered JUnit published as verified zero — valid, and it contradicted the analysis-side rule in the same PR, which is a fair thing to catch. 4. Malformed XML published as verified zero — valid. 5. Rerun promotes a partial snapshot to complete — valid, and the most consequential for how this skill is actually used, since snapshot directories get reused. The ledger is now persisted to Verified end to end: The rerun revalidates rather than inheriting — and reaching Optional follow-ups, also done:
QA / required action 5 — added Two things I did not do, deliberately:
|
|
/test payload-agent |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (1)
1023-1035: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
build_log_unavailableuses a global counter delta, so it misattributes other workers' errors.
_collect_build_logsruns these collectors in aThreadPoolExecutor. Between the two_unrecovered_error_count()reads, a different worker (build-log or JUnit) can append an error, so an emptybuild-log.txthere gets recorded asbuild_log_unavailablefor this job even though its own read never failed. That's the same cross-thread attribution bug_error_scopewas added to eliminate — use it here too.🐛 Proposed fix: scope the error check to this call
gcs_uri = f"gs://{self.job.gcs_bucket_path}/build-log.txt" - errors_before = _unrecovered_error_count() - raw = _run_gcloud_bytes( - ["gcloud", "storage", "cat", gcs_uri], timeout=120 - ) + with _error_scope() as own_errors: + raw = _run_gcloud_bytes( + ["gcloud", "storage", "cat", gcs_uri], timeout=120 + ) if not raw: - if _unrecovered_error_count() > errors_before: + if any(not e.get("recovered") for e in own_errors): _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, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 1023 - 1035, Update the build-log read handling in _collect_build_logs to use _error_scope around the _run_gcloud_bytes call instead of comparing the global _unrecovered_error_count() before and after it. Record build_log_unavailable only when that scoped call reports an error for the current job, preserving the existing error details and stage.
🧹 Nitpick comments (1)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (1)
1529-1546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider mentioning
junit_collection_partialhere too.The block explains
junit_collection_failed(absent count) but not the partial case, wheretest_failure_countis present but is a lower bound — the more likely misread of the two, since the number looks authoritative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py` around lines 1529 - 1546, The incomplete snapshot explanation in the summary-rendering block should also document the `junit_collection_partial` case. Extend the guidance near `junit_collection_failed` to state that when partial collection is indicated, `test_failure_count` is present but only a lower bound and must not be treated as authoritative.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-analysis/SKILL.md`:
- Around line 310-314: Update Step 3.3 to treat candidates from
streak.originating_payload as preliminary only, then require candidate discovery
from each matching test_failures.blocking[] entry’s first_failed_in before
scoring. Ensure per-mode candidates are used when the onsets differ, with both
onsets recorded and the scoring source identified.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 2552-2566: The cleanup loop over previous entries must validate
persisted tag and job values before constructing any deletion path. Reject path
separators, traversal, and non-normalized components, then only call
shutil.rmtree for validated paths contained within base_dir; keep invalid
entries from affecting cleanup.
- Around line 1865-1868: Gate the _invalidate_suspect_junit call in the snapshot
collection flow on self.collect_junit, matching the existing JUnit re-collection
condition. When --no-junit is active, leave existing suspect JUnit data and
collection state untouched so the snapshot is not reported complete after
deleting data without replacement.
- Around line 2470-2481: Update _sanitize_detail to redact sensitive email-like
identifiers and remove or mask URL query strings before persisting the sanitized
detail, while preserving its existing control-character removal, whitespace
collapsing, and 300-character truncation behavior.
In `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 214-220: Update the JUnit result-recording guidance around the
“junit_missing” and “junit_unparseable” cases so a parse failure omits
test_failure_count only when no JUnit XML was successfully parsed. Preserve
partial results.json and the lower-bound test_failure_count when at least one
JUnit file is readable, even if another file is malformed.
- Around line 190-192: Clarify the snapshot contract so data_complete is true
when all requested data is ultimately collected, even if an intermediate
collection failure was recovered by a fallback. Update the surrounding
collection_errors and completeness definitions consistently, preserving
unrecovered failures as the only reason for data_complete: false.
---
Outside diff comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 1023-1035: Update the build-log read handling in
_collect_build_logs to use _error_scope around the _run_gcloud_bytes call
instead of comparing the global _unrecovered_error_count() before and after it.
Record build_log_unavailable only when that scoped call reports an error for the
current job, preserving the existing error details and stage.
---
Nitpick comments:
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 1529-1546: The incomplete snapshot explanation in the
summary-rendering block should also document the `junit_collection_partial`
case. Extend the guidance near `junit_collection_failed` to state that when
partial collection is indicated, `test_failure_count` is present but only a
lower bound and must not be treated as authoritative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89d183d2-4bd3-4c70-b4f1-de0b68a7fcdd
📒 Files selected for processing (4)
plugins/ci/skills/payload-analysis/SKILL.mdplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.pyplugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py
…nostics 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.
|
/test payload-agent |
|
/ok-to-test |
Verified in CI — artifact collection is fixed
The Root cause confirmed in the real environment. The CI container has no gcloud credentials, and The The analysis improved too. With per-test onset data available, the agent caught the exact error class that motivated this PR:
That is job-level streak vs per-test onset being distinguished inline — previously impossible, because there was no per-test data at all. |
|
/lgtm |
|
Extending self-approval rights to my bot /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, not-stbenjam, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
… failures A test result falls into one of three categories, and only one can fail a job and therefore reject a payload: flake - same test, same suite, both failed and passed -> does not gate informing - testcase carries lifecycle="informing" -> does not gate failure - failed everywhere, no informing lifecycle -> gates The parser recognised neither. `grep -ci flake` was 0, and the testcase's `lifecycle` attribute was read and discarded, so `_test_results_to_json` emitted every `<failure>` element as a failure. That overstated what could have rejected a payload and, worse, let a non-cause drive regression onset: `first_failed_in` could be set by an informing test, sending analysis to hunt for a culprit PR behind a test that never gated anything. Real example from the payload chain analysed in openshift-eng#644: one conformance run's JUnit contains 11 failures, *all* of them informing UDN tests being stabilized — 0 gating. It previously reported test_failure_count: 11. Changes: - `_TestResult` carries `test_lifecycle` from the testcase attribute. A missing attribute means the test gates; the attribute exists only to opt out. - `_mark_flakes()` relabels a failure as a flake when the same test also passed in the same suite. Grouping is per (suite, name): the same monitor evaluated in `openshift-tests-upgrade` and `openshift-tests` covers two different phases, and a pass in one does not clear a failure in the other. - `results.json` records all three categories with `status` and `test_lifecycle`, so nothing is hidden. - `test_failure_count` counts gating results only; `test_flake_count` and `test_informing_failure_count` are reported separately. - `summary.json` gains `test_failures.informing[]` and `test_failures.flakes[]`. No onset is tracked for either — an onset implies a culprit to find. - Regression tracking considers gating failures only, and logs what it excluded. - payload-analysis: never score informing failures or flakes as candidates, never derive an originating payload from them, never revert for them; report them in their own section stating they cannot cause a rejection, with the one exception worth investigating — a test that harms the cluster it runs on. Tests: 13 new covering flake detection (including the cross-suite case that must NOT be treated as a flake), informing classification, absent-lifecycle gating, and mixed counting; validated against real aggregated and non-aggregated JUnit.
…ately from failures (#645) * payload-snapshot: classify flakes and informing tests separately from failures A test result falls into one of three categories, and only one can fail a job and therefore reject a payload: flake - same test, same suite, both failed and passed -> does not gate informing - testcase carries lifecycle="informing" -> does not gate failure - failed everywhere, no informing lifecycle -> gates The parser recognised neither. `grep -ci flake` was 0, and the testcase's `lifecycle` attribute was read and discarded, so `_test_results_to_json` emitted every `<failure>` element as a failure. That overstated what could have rejected a payload and, worse, let a non-cause drive regression onset: `first_failed_in` could be set by an informing test, sending analysis to hunt for a culprit PR behind a test that never gated anything. Real example from the payload chain analysed in #644: one conformance run's JUnit contains 11 failures, *all* of them informing UDN tests being stabilized — 0 gating. It previously reported test_failure_count: 11. Changes: - `_TestResult` carries `test_lifecycle` from the testcase attribute. A missing attribute means the test gates; the attribute exists only to opt out. - `_mark_flakes()` relabels a failure as a flake when the same test also passed in the same suite. Grouping is per (suite, name): the same monitor evaluated in `openshift-tests-upgrade` and `openshift-tests` covers two different phases, and a pass in one does not clear a failure in the other. - `results.json` records all three categories with `status` and `test_lifecycle`, so nothing is hidden. - `test_failure_count` counts gating results only; `test_flake_count` and `test_informing_failure_count` are reported separately. - `summary.json` gains `test_failures.informing[]` and `test_failures.flakes[]`. No onset is tracked for either — an onset implies a culprit to find. - Regression tracking considers gating failures only, and logs what it excluded. - payload-analysis: never score informing failures or flakes as candidates, never derive an originating payload from them, never revert for them; report them in their own section stating they cannot cause a rejection, with the one exception worth investigating — a test that harms the cluster it runs on. Tests: 13 new covering flake detection (including the cross-suite case that must NOT be treated as a flake), informing classification, absent-lifecycle gating, and mixed counting; validated against real aggregated and non-aggregated JUnit. * payload-snapshot: drop the flake and informing count fields Per-job entries carry a single failure count again. test_flake_count and test_informing_failure_count are removed: neither is a failure count, and having three numbers next to each other invites summing them back into the inflated total this change exists to remove. test_failure_count remains the gating count. The non-gating results are not lost — every one is still recorded by name in the job's results.json with its status and test_lifecycle, and listed under test_failures.flakes[] and test_failures.informing[] in the summary. * Address review: fix thread-safety, classification gaps, and stale regressions - Use _error_scope() in BuildLogCollector to avoid racing the global error count across concurrent worker threads - Extend junit_collection_failed to cover junit_missing and junit_unparseable, not just junit_unavailable - Remove stale regressions.json when JUnit is invalidated so _track_regressions recomputes from fresh data - Narrow _GCLOUD_NO_MATCH_PATTERNS by removing overly broad "not found" and check auth patterns before no-match to prevent auth errors from being silently classified as benign Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Bump ci plugin version to 0.0.75 and regenerate docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Bump ci plugin version to 0.0.77 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Problem
A real 5.0 snapshot contained no CI artifact data at all:
Every gcloud-sourced artifact was missing; every HTTPS-sourced artifact (release controller, changelog, GitHub PR data) was intact.
Root cause:
gcloud storagerefuses client-side when no account is configured. The artifact buckets are public, but gcloud does not attempt an anonymous read — it fails with "You do not currently have an active account selected" without ever issuing the request._check_gcloud()only rangcloud --version, so an unauthenticated install passed preflight and then failed every read.Demonstrated:
Second defect, which turned that into silent corruption:
_run_gcloud()returnedNoneidentically for a timeout, a missing binary, an auth refusal, and a genuine "no such object", andJUnitCollector.collect()wrote[]regardless. So the snapshot reportedtest_failure_count: 0for jobs whose data it had simply failed to read. Downstream, "0 test failures" on a failed job reads as "it failed for reasons unrelated to its tests", which sends root-cause analysis down a false path.Consequence: four consecutive payload analyses attributed a 50-hour stream outage to the wrong PRs, including an innocent candidate scored 80/100. The real cause was in a payload the analyses had already searched past.
Fix
1. Don't fail in the first place. When gcloud has no active account, set
CLOUDSDK_AUTH_DISABLE_CREDENTIALSso it reads the public buckets anonymously. Resolved once under a lock (collectors run in a thread pool). Authentication is now optional, not a silent prerequisite.2. Make any remaining failure loud, not empty.
_run_gcloud/_run_gcloud_bytesclassify failures (auth,timeout,gcloud_missing,command_failed) and record them. A genuine "matched no objects" stays absence, not error — which matters because the fallback below probes many paths that legitimately don't exist.results.jsonis not written; the summary omitstest_failure_countand setsjunit_collection_failed: true. Absent means unknown;0means verified clean.summary.jsongainsdata_completeandcollection_errors[];AGENTS.mdgains an⚠️ INCOMPLETE SNAPSHOTsection so the analysis agent sees the gap in the document it reads first.--fail-on-incompletefor automation.3. JUnit discovery hardening (absorbed from #641).
{step}/,{step}/artifacts/and{step}/*/artifacts/only. Aggregated jobs keepjunit-aggregated.xmlabout six levels belowartifacts/, so the fallback could never find it — precisely for the jobs in the affected snapshot. Added a**probe scoped to the aggregator subtree.junit_operator.xmlsits directly inartifacts/, not in a step directory. Added a top-level probe; the fallback now recovers the same file set as the glob (73 failing tests either way, vs 71 without).recovered: trueand excluded fromdata_complete, so a tuned-away timeout stays visible as diagnostics without condemning a complete snapshot.4. Analysis side.
payload-analysisnow derives a failure mode's originating payload from the per-testfirst_failed_inrather than the job-level streak, which merges unrelated modes (infra blip, flake, real regression) and so scores candidates from a payload predating the regression. On a collection failure the agent is told to collect the job's artifacts itself rather than reason around the gap.Verification
Same payload, four environments:
CLOUDSDK_CONFIGempty)data_complete: true, 73 blocking test failures,test_failure_count68/31, 8 XMLs, 4 build logs, exit 0 — previously 0 / 0 / 15 emptyrecovered: true,data_complete: true, exit 0 — identical to the glob pathresults.jsonwritten,test_failure_countabsent,junit_collection_failed: true, exit 1Before/after on the same payload: JUnit XMLs 0 → 18,
build_log.json0 → 9, non-emptyresults.json0 → 9, blocking test failures 0 → 73.With the data present, the per-test onset now identifies the true originating payload automatically — the one that took hours of manual archive work to establish by hand:
The job-level streak pointed two payloads earlier, at failures that were CI infrastructure (
pod has not been scheduled in 1h,The node had condition: [DiskPressure]) where the upgrade phase never ran.Note on #641's premise
The glob was not slow for the jobs in the affected snapshot — 0.73 s for the aggregated AWS job, 8 s for
hypershift-e2e-aws— and gcloud failed on every read regardless of timeout. So #641 alone would not have prevented this, and its fallback could not have found the aggregated JUnit even when it ran. Both problems are real, which is why its changes are included here rather than dropped.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--fail-on-incompleteto exit non-zero when unrecovered collection issues remain.Documentation
Tests
Chores