TRT-2870: A/B/C test C (historical control): pre-#604 CI skills with current payload evals - #664
TRT-2870: A/B/C test C (historical control): pre-#604 CI skills with current payload evals#664stbenjam wants to merge 12 commits into
Conversation
…evert-evals # Conflicts: # .claude-plugin/marketplace.json # docs/index.html # plugins/ci/.claude-plugin/plugin.json
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
WalkthroughThe PR updates the CI plugin to version 0.0.81, adds point-in-time payload-analysis evaluation controls, simplifies payload snapshots, replaces legacy analysis skills with specialized workflows, and adds artifact-search and must-gather extraction tools. ChangesPayload analysis evaluation controls
Payload snapshot simplification
CI analysis skill migration
Must-gather extraction
Plugin metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
/test eval-payload-analysis |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (1)
719-736: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winA failed JUnit listing or parse now writes an empty
results.json.
_list_junit_filesreturns[]when the gcloud listing fails or times out at 30 s (line 755-760)._parse_junit_xmlreturns[]when the XML cannot be read (line 2358-2363). In both casescollectwrites an emptyresults.jsonand returnsTrue. Downstream,_build_failed_job_detailssetstest_failure_countto 0 (line 1471), and the regression tracker sees no failures. A collection failure is then indistinguishable from a job with no test failures.Log a warning when the listing is empty for a failed job, and skip writing
results.jsonwhen no JUnit file was read.🐛 Proposed fix
junit_files = self._list_junit_files() + if not junit_files: + _log(f" Warning: no JUnit files found for {self.job.name}") + return False + + parsed_any = False for gcs_uri in junit_files: filename = os.path.basename(gcs_uri) local_path = os.path.join(self.output_dir, filename) if not os.path.exists(local_path): data = _run_gcloud_bytes( ["gcloud", "storage", "cat", gcs_uri], timeout=60 ) if data: with open(local_path, "wb") as f: f.write(data) if os.path.exists(local_path): results = _parse_junit_xml(local_path, source_name=filename) + parsed_any = True all_results.extend(results) + if not parsed_any: + _log(f" Warning: no JUnit XML read for {self.job.name}") + return False + failures = _test_results_to_json(all_results) _write_json(self.output_path, failures)🤖 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 719 - 736, Update the JUnit collection flow in collect: when _list_junit_files() returns no files for a failed job, log a warning and avoid writing results.json; also track whether any JUnit file was successfully read and skip _write_json when none were read, including cases where _parse_junit_xml returns no results due to unreadable XML. Preserve normal results writing when at least one JUnit file is read.plugins/ci/skills/payload-analysis/SKILL.md (2)
301-345: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake revert eligibility enforce the stated exclusions.
Line 339 marks every score of 85 or higher as a revert candidate. Lines 341-344 also require exact timing and exclusion of infrastructure and other plausible causes. The mechanical rubric in lines 303-314 does not enforce those conditions.
Require explicit evidence that excludes infrastructure and alternative causes before a candidate becomes a revert recommendation. Otherwise, a high score can mandate a revert for a correlated but unproven PR.
🤖 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-analysis/SKILL.md` around lines 301 - 345, Update the “Propose Revert Candidates” rules to require explicit evidence that the failure began immediately after the originating payload and that infrastructure, RHCOS, and other plausible causes have been ruled out, in addition to a score of at least 85 and a clear mapping to the PR. Keep high-scoring but unproven candidates as non-revert recommendations, and preserve RHCOS RPM changes as informational suspects rather than revert targets.
205-215: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve the must-gather interaction contract.
The payload subagent prompt requires full must-gather analysis and prohibits user questions. The test-failure workflow requires a user confirmation before it downloads and analyzes must-gather data. This conflict causes either skipped diagnostics or an instruction violation.
plugins/ci/skills/payload-analysis/SKILL.md#L205-L215: pass an explicit noninteractive, pre-authorized must-gather option when full analysis is required.plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md#L486-L496: define and honor that option so payload-analysis can perform the required diagnostics without an interactive prompt.🤖 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-analysis/SKILL.md` around lines 205 - 215, Resolve the must-gather authorization contract across plugins/ci/skills/payload-analysis/SKILL.md lines 205-215 and plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md lines 486-496: update the payload-analysis instructions to invoke the test-failure workflow with an explicit noninteractive, pre-authorized must-gather option, and update the test-failure skill to define and honor that option by skipping the confirmation prompt while still performing full must-gather download and analysis; preserve interactive confirmation when the option is absent.
🟡 Minor comments (10)
plugins/ci/skills/payload-snapshot/SKILL.md-70-71 (1)
70-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the retained
--sippyoption.The CLI reference no longer lists
--sippy, butpayload_snapshot.pystill defines it (line 2497). Line 208 of this file also still refers to Sippy-based changelogs. Users cannot discover a supported flag from the skill documentation. Restore a short--sippyentry in the CLI reference, or remove the flag from the script.🤖 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/SKILL.md` around lines 70 - 71, Update the CLI reference in the payload snapshot skill documentation to include a concise entry for the existing --sippy option defined by payload_snapshot.py, including its purpose and usage. Keep the current Sippy changelog behavior documented consistently, without removing the supported flag from the script.plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py-377-380 (1)
377-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winChain the re-raised exception.
Ruff flags B904 here. Add
from Noneto mark the re-raise as intentional and keep the traceback clean.♻️ Proposed change
except ValueError: raise ValueError( f"Tag {start_tag} not found in stream {self.stream_name}" - ) + ) from None🤖 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 377 - 380, Update the ValueError re-raise in the surrounding tag lookup method to explicitly suppress exception chaining by adding the intentional `from None` cause. Keep the existing error message and handling unchanged.Source: Linters/SAST tools
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py-762-762 (1)
762-762: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename the loop variable to satisfy E741.
Ruff reports the ambiguous name
l. Useline.♻️ Proposed change
- files = [l.strip() for l in output.strip().splitlines() if l.strip()] + files = [line.strip() for line in output.strip().splitlines() + if line.strip()]🤖 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` at line 762, In the files comprehension, rename the loop variable from l to line and update its strip() reference accordingly to satisfy Ruff E741.Source: Linters/SAST tools
plugins/ci/evals/eval-payload-analysis.yaml-566-576 (1)
566-576: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequired-claim matching is fragile for multi-word phrases. The
case_constraintsjudge compares eachany_ofentry as an exact lowercased substring against the concatenated raw HTML, YAML, and JSON output. Markup, newlines, or extra indentation between words defeat the match, and the judge runs atmin_pass_rate: 1.0, so a correct analysis can fail the gate.
plugins/ci/evals/eval-payload-analysis.yaml#L566-L576: strip HTML tags and collapse whitespace in bothsearchableand eachany_ofphrase before the substring test.plugins/ci/evals/cases/payload-analysis/case-020/annotations.yaml#L13-L18: shorten the sentence-shaped phrases in the first required claim to distinctive fragments, for example"externally unreachable"and"vip reachability".🤖 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/evals/eval-payload-analysis.yaml` around lines 566 - 576, Normalize required-claim matching in the case_constraints judge by stripping HTML tags and collapsing whitespace in both searchable and each any_of phrase before the substring comparison; update plugins/ci/evals/eval-payload-analysis.yaml lines 566-576 accordingly. In plugins/ci/evals/cases/payload-analysis/case-020/annotations.yaml lines 13-18, replace the first required claim’s sentence-shaped alternatives with distinctive fragments such as “externally unreachable” and “vip reachability”.plugins/ci/evals/eval-payload-analysis.yaml-537-555 (1)
537-555: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the confidence conversions against non-integer agent output.
confidence_scorecomes from the agent-generated results YAML.int()raisesValueErrorfor values such as"95.0"or"95/100", and raisesTypeErrorforNone. The judge body has no exception handling around these conversions, so the judge raises instead of returning a failure verdict. Theyaml_results_validjudge checks only thatconfidence_scoreis present, not that it is an integer. A malformed score therefore breaks the eval run instead of being scored as a failure.🛡️ Proposed coercion helper
failures = [] + + def as_confidence(value, default=0): + try: + return int(float(value)) + except (TypeError, ValueError): + return default expected_causes = ( (case_data.get("expected_candidates", []) or []) + (case_data.get("expected_ci_config_issues", []) or []) ) for expected in expected_causes: url = str(expected.get("pr_url", "")).rstrip("/").lower() candidate = by_url.get(url) if candidate is None: failures.append(f"Missing expected candidate {url}") continue - minimum = int(expected.get("min_confidence", 0)) - actual = int(candidate.get("confidence_score", 0)) + minimum = as_confidence(expected.get("min_confidence", 0)) + actual = as_confidence(candidate.get("confidence_score", 0)) if actual < minimum: failures.append( f"Expected candidate {url} scored {actual}, below {minimum}" ) for url in case_data.get("forbidden_revert_candidates", []) or []: normalized = str(url).rstrip("/").lower() candidate = by_url.get(normalized) if candidate is None: continue - confidence = int(candidate.get("confidence_score", 0)) + confidence = as_confidence(candidate.get("confidence_score", 0)) eligible = candidate.get("revert_eligible") is True🤖 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/evals/eval-payload-analysis.yaml` around lines 537 - 555, Guard both confidence_score conversions in the judge body, including the expected-candidate check and the forbidden_revert_candidates loop, against malformed agent values such as decimal strings, fractions, and None. Reuse or add a local coercion helper that returns a safe failure value for invalid scores instead of raising, while preserving normal integer scoring and failure reporting.plugins/ci/evals/eval-payload-analysis.yaml-335-344 (1)
335-344: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd the
githubusercontentPR-diff host to the mutable-URL check.
is_github_urlrecognizes onlygithub.com,api.github.com, andraw.githubusercontent.com.patch-diff.githubusercontent.comserves mutable PR diffs and patches, for examplehttps://patch-diff.githubusercontent.com/raw/<owner>/<repo>/pull/<n>.diff. That host is not matched, socheck_external_urldoes not flag it and the hard gate is bypassed.gist.github.comhas the same gap.🛡️ Proposed host additions
return host in { "github.com", "api.github.com", "raw.githubusercontent.com", + "patch-diff.githubusercontent.com", + "gist.github.com", + "gist.githubusercontent.com", }🤖 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/evals/eval-payload-analysis.yaml` around lines 335 - 344, Update is_github_url to recognize patch-diff.githubusercontent.com and gist.github.com as GitHub hosts, preserving the existing URL parsing and false-on-invalid-URL behavior so check_external_url applies the mutable-URL check to these domains.plugins/ci/evals/eval-payload-analysis.yaml-307-333 (1)
307-333: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail explicitly when
analysis_cutoffis missing or unparsable.
cutoffbecomesNonewhencase_datahas noanalysis_cutoff, when the value is empty, or when the value is not parsable.check_boundthen verifies only that each bound is an absolute timestamp. It never compares the bound against the cutoff, so a lookup dated after the payload completion passes the gate. The judge threshold ismin_pass_rate: 1.0, and this degradation is silent. An explicit failure makes a missing cutoff visible to the case author.🛡️ Proposed guard
cutoff = absolute_time(cutoff_text) + if cutoff is None: + return ( + False, + "Case annotations lack a parsable analysis_cutoff; " + "point-in-time bounds cannot be enforced", + )🤖 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/evals/eval-payload-analysis.yaml` around lines 307 - 333, Validate the analysis_cutoff immediately after computing cutoff in the surrounding payload-analysis flow, and record an explicit violation when cutoff is missing or unparsable instead of allowing check_bound to continue without a comparison. Preserve the existing bound validation and cutoff-exceeded checks for valid cutoffs, while ensuring invalid or empty analysis_cutoff values fail the case visibly.plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py-705-709 (1)
705-709: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
dict.get()fallback never triggers forNonevalues.
file.get('symlink_path', f"logs/{file['path']}")only falls back to the default when the key is missing.create_txt_symlinksalways setssymlink_path(to a path or toNone), so when inline HTML generation fails for a small file,iframe_pathbecomes the literal stringNoneinstead of falling back to the raw file path. The inline viewer then tries to loaddata-iframe-path="None"and fails.Use
file.get('symlink_path') or f"logs/{file['path']}"instead, so aNonevalue also falls back correctly (already reflected in the diff on the escaping comment above).🤖 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/prow-job-extract-must-gather/generate_html_report.py` around lines 705 - 709, Update the iframe_path assignment in the file-processing loop to use the symlink_path value only when it is truthy, falling back to f"logs/{file['path']}" when it is missing or None. Keep original_path and the surrounding HTML generation behavior unchanged.plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py-1277-1279 (1)
1277-1279: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against a trailing slash in
logs_dir.
os.path.dirname(logs_dir)does not strip a trailing slash first. Iflogs_dirends with/,os.path.dirname()returns the same directory instead of its parent, and the report is written to the wrong location. Current documented invocations omit the trailing slash, but the assumption is fragile.Normalize the path before computing the parent directory.
🐛 Proposed fix
# Determine output path - output_dir = os.path.dirname(logs_dir) + output_dir = os.path.dirname(os.path.normpath(logs_dir)) output_file = os.path.join(output_dir, 'must-gather-browser.html')🤖 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/prow-job-extract-must-gather/generate_html_report.py` around lines 1277 - 1279, Normalize logs_dir to remove any trailing path separator before passing it to os.path.dirname in the output-path construction, ensuring output_file is placed in the parent directory even when logs_dir ends with a slash.plugins/ci/skills/prow-job-extract-must-gather/extract_archives.py-122-136 (1)
122-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix inflated file count on
getsizefailure.
total_files += 1runs beforeos.path.getsize(file_path). Ifgetsizeraises, the bareexcept: passdiscards the error, buttotal_fileswas already incremented whiletotal_sizewas not. The final statistics then overcount files relative to size.Move the increment after the successful size lookup, and catch
OSErrorspecifically instead of a bareexcept.🐛 Proposed fix
for root, dirs, files in os.walk(base_path): for filename in files: file_path = os.path.join(root, filename) try: - total_files += 1 - total_size += os.path.getsize(file_path) - except: - pass + size = os.path.getsize(file_path) + total_files += 1 + total_size += size + except OSError: + pass🤖 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/prow-job-extract-must-gather/extract_archives.py` around lines 122 - 136, Update count_files_and_size so os.path.getsize(file_path) succeeds before incrementing total_files, keeping the file count and total size consistent when metadata lookup fails. Replace the bare except with an OSError-specific handler, while preserving the existing traversal and return behavior.Source: Linters/SAST tools
🧹 Nitpick comments (7)
plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py (3)
2358-2363: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider
defusedxmlfor third-party JUnit XML.
ET.parsereads XML downloaded from CI artifact buckets. Ruff reports S314. If you want to remove the class of entity-expansion risks, parse withdefusedxml.ElementTree. This adds a dependency, and the skill states that only the standard library is used, so treat it as optional.🤖 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 2358 - 2363, Optionally update _parse_junit_xml to use defusedxml.ElementTree.parse for JUnit XML obtained from external CI artifacts, replacing the standard-library ET parser while preserving the existing exception handling and empty-list fallback. Add the dependency only if the skill’s standard-library-only constraint is intentionally relaxed.Source: Linters/SAST tools
2165-2188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog gcloud stderr before returning
None.Both helpers discard stderr and the exit code. The preflight credential probe was also removed. An expired credential, a permission error, and a missing object now produce the same silent
None, so the snapshot reports no JUnit data without any reason. Log stderr at warning level.The Ruff S603 and ast-grep injection findings on these lines are false positives: the arguments are a list,
shell=Trueis not used, and the values come from internal GCS paths.♻️ Proposed change
def _run_gcloud(args: list[str], timeout: int = 120) -> Optional[str]: """Run a gcloud CLI command, returning stdout or None on error.""" try: result = subprocess.run( args, capture_output=True, text=True, timeout=timeout ) if result.returncode != 0: + _log(f" Warning: gcloud failed ({result.returncode}): " + f"{result.stderr.strip()[:300]}") return None return result.stdout - except (subprocess.TimeoutExpired, FileNotFoundError): + except (subprocess.TimeoutExpired, FileNotFoundError) as e: + _log(f" Warning: gcloud command failed: {e}") return None🤖 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 2165 - 2188, Update _run_gcloud and _run_gcloud_bytes to log each failed gcloud invocation’s stderr at warning level before returning None, while preserving their existing success and exception behavior. Use the captured stderr from each subprocess result and retain the current safe list-based, non-shell invocation.Source: Linters/SAST tools
289-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the failure instead of silently returning an empty changelog.
fetch_changelogswallows every exception. A transient Sippy error then looks identical to a payload with no PR changes, and the snapshot records an empty changelog. Narrow the exception and log it.♻️ Proposed change
url = f"{self.SIPPY_BASE}/payloads/diff?{params}" try: return fetch_json(url, timeout=60) - except Exception: + except (urllib.error.HTTPError, urllib.error.URLError, + json.JSONDecodeError, TimeoutError) as e: + _log(f" Warning: Sippy changelog fetch failed for {tag_name}: {e}") return []🤖 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 289 - 292, Update fetch_changelog to catch only the expected request/fetch exception rather than every Exception, log the failure with relevant error details, and preserve returning an empty list for that handled failure. Do not let unrelated exceptions be silently converted into an empty changelog.Source: Linters/SAST tools
plugins/ci/evals/eval-payload-analysis.yaml (1)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the remaining scored annotation fields in the schema.
The
analysis_qualityprompt scoresrequired_claim,discriminating signal, andmust_not_concludeat lines 644-645.case-020/annotations.yamlalso definesdistractorsandkey_evidence. The schema block lists onlyrequired_claimsamong these. Case authors read this schema as the contract, so add the missing fields to keep new cases consistent.♻️ Proposed schema additions
- 'required_claims': causal findings that must appear in the analysis; each entry supplies a name and a list of acceptable evidence phrases + - 'must_not_conclude': optional list of conclusions the analysis must + not reach + - 'distractors': optional list of plausible but incorrect signals + - 'key_evidence': optional list of fact/artifacts pairs establishing the + contemporaneous record + - 'discriminating_signal': optional free text describing the reasoning + that separates the true cause from the distractors - 'notes': free text context🤖 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/evals/eval-payload-analysis.yaml` around lines 78 - 80, Update the schema documentation in eval-payload-analysis.yaml to list all scored annotation fields: required_claims, discriminating signal, must_not_conclude, distractors, and key_evidence. Describe each field consistently with the existing required_claims and notes entries so the schema reflects the contract used by analysis_quality and case annotations.plugins/ci/evals/cases/payload-analysis/case-020/annotations.yaml (1)
13-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider shorter evidence phrases for the first required claim.
The
case_constraintsjudge matches eachany_ofentry as a plain lowercased substring across the HTML, YAML, and JSON outputs. Long sentence-shaped phrases such as"external vip reachability precedes"and"vips were externally unreachable"require near-exact wording. HTML rendering can insert markup or line breaks between those words. The judge threshold ismin_pass_rate: 1.0, so a semantically correct report can still fail the gate. The shorter phrases in the other claims, for example"dns 429"and"osprovisioningtimedout", do not have this fragility.♻️ Proposed shorter phrases
- name: "GCP infrastructure trigger precedes fallout" any_of: - - "external vip reachability precedes" - - "vips were externally unreachable" - - "pre-upgrade network reachability loss" + - "externally unreachable" + - "vip reachability" + - "network reachability loss"🤖 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/evals/cases/payload-analysis/case-020/annotations.yaml` around lines 13 - 18, Shorten the first required claim’s any_of evidence phrases in the annotations for “GCP infrastructure trigger precedes fallout” to robust lowercase substrings that preserve the intended reachability evidence while avoiding sentence-shaped wording vulnerable to HTML markup or line breaks. Keep the claim meaning and existing judge configuration unchanged.plugins/ci/evals/test_eval_payload_analysis.py (1)
12-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the new deterministic judges with tests.
This test asserts only on the
revert_scoring_accuracyprompt text. The PR adds two deterministic judges with substantial Python logic in theircheckblocks:point_in_time_trace_hygieneandcase_constraints. Both run atmin_pass_rate: 1.0, so a logic defect in either one blocks every eval run. Thecheckbodies are plain Python strings in the YAML, so a test canexecthem against syntheticoutputsdictionaries. Useful cases include an immutable commit URL, agit logcall without--until, a bound that exceeds the cutoff, and a forbidden candidate at confidence 90.🤖 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/evals/test_eval_payload_analysis.py` around lines 12 - 25, Extend ScoringPromptTest with coverage for the deterministic judges point_in_time_trace_hygiene and case_constraints by loading and executing each YAML check block against synthetic outputs dictionaries. Assert the expected outcomes for an immutable commit URL, git log usage without --until, a bound exceeding the cutoff, and a forbidden candidate at confidence 90, while preserving the existing revert_scoring_accuracy prompt assertions.plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py (1)
985-993: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRestrict inline HTML generation to text-oriented file types.
Every file under 1MB gets an inline HTML view, including certs, archives, and other binary types. Reading these as UTF-8 text with
errors='replace'produces unreadable output while still paying the read/escape/write cost per file, across potentially thousands of files in a must-gather extraction.Restrict this to text-oriented types (log, yaml, json, xml, config) using the
file['type']value already computed byscan_directory.♻️ Proposed fix
+ TEXT_TYPES = {'log', 'yaml', 'json', 'xml', 'config'} for file in files: - if file['size'] < MAX_INLINE_SIZE: + if file['size'] < MAX_INLINE_SIZE and file['type'] in TEXT_TYPES:🤖 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/prow-job-extract-must-gather/generate_html_report.py` around lines 985 - 993, Update the inline HTML generation condition in the file loop to require both a size below MAX_INLINE_SIZE and a text-oriented file['type'] such as log, yaml, json, xml, or config. Reuse the type classification produced by scan_directory and leave non-text files on the existing non-inline path.
🤖 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/evals/cases/payload-analysis/case-019/annotations.yaml`:
- Around line 57-60: Remove all outcomes referring to PRs `#2953` and `#2954` from
the key_evidence and notes entries in the payload-analysis annotation,
preserving evidence from before the 2026-07-25T16:05:26Z cutoff.
In `@plugins/ci/evals/eval-payload-analysis.yaml`:
- Around line 403-414: Extend the leakage checks alongside the existing Read
handling to also process Grep and Glob tool calls. Inspect each tool’s path,
pattern, and glob inputs for the same annotations.yaml, payload-analysis README,
and comments.json patterns, and append the existing violation message when
matched; preserve the current Read, WebFetch, and Bash checks.
- Line 7: Update the ci:payload-analysis skill’s argument handling to accept an
optional --as-of cutoff alongside the existing payload tag and --snapshot-dir
arguments. Parse and validate the cutoff, then constrain all payload analysis
evidence to data at or before that timestamp, while preserving current behavior
when --as-of is omitted.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 418-424: Update _has_blocking_failures to require explicit success
for every blocking job, matching PayloadChain._all_blocking_passed: treat Error,
Aborted, Pending, and any other non-Succeeded state as failure, and treat an
empty runs result as not green. Preserve the existing Sippy fetch flow while
ensuring baseline detection only succeeds when blocking job data is present and
all blocking jobs have state Succeeded.
In `@plugins/ci/skills/prow-job-analyze-install-failure/SKILL.md`:
- Around line 93-97: Update the build_id extraction guidance in “Extract
build_id” to accept IDs followed by either a slash or the end of the URL, while
still requiring at least 10 consecutive digits and a valid path boundary before
the ID. Preserve the existing error message when no matching build ID is found.
In `@plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md`:
- Around line 1248-1255: Update the completion workflow around the “Display
completion message” step to first persist the selected Branch A or Branch B
report to .work/prow-job-analyze-test-failure/{build_id}/analysis.md, creating
the required parent directory as needed. Only display the report path after the
write succeeds, while preserving the existing completion message and
report-selection behavior.
- Around line 643-648: Update the Pattern 2 (dual) management-cluster extraction
command to use the CI plugin path for extract_archives.py, matching the working
path used elsewhere in the document. Keep the archive and output-directory
arguments unchanged.
- Around line 760-781: Update the must-gather script discovery in the “Locate
must-gather-analyzer scripts” step to remove the find ~ fallback and avoid
executing analyzer files from arbitrary home-directory locations. Use only a
declared trusted plugin path or invoke the registered must-gather analysis
skill, while preserving the existing warning and continuation behavior when the
trusted scripts are unavailable.
- Around line 224-241: The installation-failure delegation uses the wrong
identifier form. In plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md
lines 224-241, invoke the canonical ci:prow-job-analyze-install-failure skill,
or explicitly use /ci:analyze-prow-job-install-failure as a command. In
plugins/ci/skills/fetch-regression-details/SKILL.md line 472, label
/ci:analyze-prow-job-test-failure as a related command or replace it with the
canonical ci:prow-job-analyze-test-failure skill.
In `@plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py`:
- Around line 218-257: Update cmd_fetch to reject non-positive max_bytes values
and enforce the limit before downloading object contents. Use GCS metadata to
determine the object size, reject objects exceeding max_bytes before transfer,
and use a range-capable read limited to max_bytes for accepted objects while
preserving the existing response fields and cleanup behavior.
- Line 269: Align the Python version contract across the script and
documentation: update prow_job_artifact_search.py to avoid the Python 3.7-only
add_subparsers(required=True), capture_output=True, and text=True APIs with
Python 3.6-compatible equivalents, including explicit post-parse command
validation; update SKILL.md lines 31-32 to retain the documented Python 3.6+
minimum. Alternatively, raise the documented minimum in SKILL.md lines 31-32 to
Python 3.7 and keep all three APIs unchanged.
In `@plugins/ci/skills/prow-job-extract-must-gather/extract_archives.py`:
- Around line 21-30: Harden extract_tar_archive so every supported Python
version prevents tar path traversal for both top-level and nested archives. Use
tar.extractall’s filter='data' where available, and provide a Python
3-compatible fallback that rejects absolute paths, traversal outside extract_to,
links, and unsafe special files before extraction; do not leave the existing
unfiltered extractall path.
In `@plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py`:
- Around line 705-729: Escape all must-gather and CLI-derived values before
interpolating them into generated HTML. In the file-item loop, apply
html_module.escape to file['name'], file['path'], file['dir'],
file['top_level_dir'], symlink_path, and related metadata used in attributes or
text; apply the same escaping in the header metadata block for prowjob_name,
build_id, target, and gcsweb_url (including its href), and in the directory
filter for directory and display_name. Reuse the existing html_module.escape
pattern without changing the report’s displayed values or behavior.
- Around line 93-106: Update the top_level_dir derivation in the
report-generation logic so files directly under content/ or with no directory
separator receive a non-empty sentinel category instead of ''. Ensure that
sentinel is added to dir_counts and therefore appears in the directory filter
buttons and default active-filter set, while preserving existing
nested-directory categorization.
---
Outside diff comments:
In `@plugins/ci/skills/payload-analysis/SKILL.md`:
- Around line 301-345: Update the “Propose Revert Candidates” rules to require
explicit evidence that the failure began immediately after the originating
payload and that infrastructure, RHCOS, and other plausible causes have been
ruled out, in addition to a score of at least 85 and a clear mapping to the PR.
Keep high-scoring but unproven candidates as non-revert recommendations, and
preserve RHCOS RPM changes as informational suspects rather than revert targets.
- Around line 205-215: Resolve the must-gather authorization contract across
plugins/ci/skills/payload-analysis/SKILL.md lines 205-215 and
plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md lines 486-496: update
the payload-analysis instructions to invoke the test-failure workflow with an
explicit noninteractive, pre-authorized must-gather option, and update the
test-failure skill to define and honor that option by skipping the confirmation
prompt while still performing full must-gather download and analysis; preserve
interactive confirmation when the option is absent.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 719-736: Update the JUnit collection flow in collect: when
_list_junit_files() returns no files for a failed job, log a warning and avoid
writing results.json; also track whether any JUnit file was successfully read
and skip _write_json when none were read, including cases where _parse_junit_xml
returns no results due to unreadable XML. Preserve normal results writing when
at least one JUnit file is read.
---
Minor comments:
In `@plugins/ci/evals/eval-payload-analysis.yaml`:
- Around line 566-576: Normalize required-claim matching in the case_constraints
judge by stripping HTML tags and collapsing whitespace in both searchable and
each any_of phrase before the substring comparison; update
plugins/ci/evals/eval-payload-analysis.yaml lines 566-576 accordingly. In
plugins/ci/evals/cases/payload-analysis/case-020/annotations.yaml lines 13-18,
replace the first required claim’s sentence-shaped alternatives with distinctive
fragments such as “externally unreachable” and “vip reachability”.
- Around line 537-555: Guard both confidence_score conversions in the judge
body, including the expected-candidate check and the forbidden_revert_candidates
loop, against malformed agent values such as decimal strings, fractions, and
None. Reuse or add a local coercion helper that returns a safe failure value for
invalid scores instead of raising, while preserving normal integer scoring and
failure reporting.
- Around line 335-344: Update is_github_url to recognize
patch-diff.githubusercontent.com and gist.github.com as GitHub hosts, preserving
the existing URL parsing and false-on-invalid-URL behavior so check_external_url
applies the mutable-URL check to these domains.
- Around line 307-333: Validate the analysis_cutoff immediately after computing
cutoff in the surrounding payload-analysis flow, and record an explicit
violation when cutoff is missing or unparsable instead of allowing check_bound
to continue without a comparison. Preserve the existing bound validation and
cutoff-exceeded checks for valid cutoffs, while ensuring invalid or empty
analysis_cutoff values fail the case visibly.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 377-380: Update the ValueError re-raise in the surrounding tag
lookup method to explicitly suppress exception chaining by adding the
intentional `from None` cause. Keep the existing error message and handling
unchanged.
- Line 762: In the files comprehension, rename the loop variable from l to line
and update its strip() reference accordingly to satisfy Ruff E741.
In `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 70-71: Update the CLI reference in the payload snapshot skill
documentation to include a concise entry for the existing --sippy option defined
by payload_snapshot.py, including its purpose and usage. Keep the current Sippy
changelog behavior documented consistently, without removing the supported flag
from the script.
In `@plugins/ci/skills/prow-job-extract-must-gather/extract_archives.py`:
- Around line 122-136: Update count_files_and_size so os.path.getsize(file_path)
succeeds before incrementing total_files, keeping the file count and total size
consistent when metadata lookup fails. Replace the bare except with an
OSError-specific handler, while preserving the existing traversal and return
behavior.
In `@plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py`:
- Around line 705-709: Update the iframe_path assignment in the file-processing
loop to use the symlink_path value only when it is truthy, falling back to
f"logs/{file['path']}" when it is missing or None. Keep original_path and the
surrounding HTML generation behavior unchanged.
- Around line 1277-1279: Normalize logs_dir to remove any trailing path
separator before passing it to os.path.dirname in the output-path construction,
ensuring output_file is placed in the parent directory even when logs_dir ends
with a slash.
---
Nitpick comments:
In `@plugins/ci/evals/cases/payload-analysis/case-020/annotations.yaml`:
- Around line 13-18: Shorten the first required claim’s any_of evidence phrases
in the annotations for “GCP infrastructure trigger precedes fallout” to robust
lowercase substrings that preserve the intended reachability evidence while
avoiding sentence-shaped wording vulnerable to HTML markup or line breaks. Keep
the claim meaning and existing judge configuration unchanged.
In `@plugins/ci/evals/eval-payload-analysis.yaml`:
- Around line 78-80: Update the schema documentation in
eval-payload-analysis.yaml to list all scored annotation fields:
required_claims, discriminating signal, must_not_conclude, distractors, and
key_evidence. Describe each field consistently with the existing required_claims
and notes entries so the schema reflects the contract used by analysis_quality
and case annotations.
In `@plugins/ci/evals/test_eval_payload_analysis.py`:
- Around line 12-25: Extend ScoringPromptTest with coverage for the
deterministic judges point_in_time_trace_hygiene and case_constraints by loading
and executing each YAML check block against synthetic outputs dictionaries.
Assert the expected outcomes for an immutable commit URL, git log usage without
--until, a bound exceeding the cutoff, and a forbidden candidate at confidence
90, while preserving the existing revert_scoring_accuracy prompt assertions.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Around line 2358-2363: Optionally update _parse_junit_xml to use
defusedxml.ElementTree.parse for JUnit XML obtained from external CI artifacts,
replacing the standard-library ET parser while preserving the existing exception
handling and empty-list fallback. Add the dependency only if the skill’s
standard-library-only constraint is intentionally relaxed.
- Around line 2165-2188: Update _run_gcloud and _run_gcloud_bytes to log each
failed gcloud invocation’s stderr at warning level before returning None, while
preserving their existing success and exception behavior. Use the captured
stderr from each subprocess result and retain the current safe list-based,
non-shell invocation.
- Around line 289-292: Update fetch_changelog to catch only the expected
request/fetch exception rather than every Exception, log the failure with
relevant error details, and preserve returning an empty list for that handled
failure. Do not let unrelated exceptions be silently converted into an empty
changelog.
In `@plugins/ci/skills/prow-job-extract-must-gather/generate_html_report.py`:
- Around line 985-993: Update the inline HTML generation condition in the file
loop to require both a size below MAX_INLINE_SIZE and a text-oriented
file['type'] such as log, yaml, json, xml, or config. Reuse the type
classification produced by scan_directory and leave non-text files on the
existing non-inline path.
🪄 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: Enterprise
Run ID: 1e4fb4c4-395e-49d8-bc9d-d2675aacf120
📒 Files selected for processing (103)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/evals/cases/payload-analysis/README.mdplugins/ci/evals/cases/payload-analysis/case-001/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-001/input.yamlplugins/ci/evals/cases/payload-analysis/case-002/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-002/input.yamlplugins/ci/evals/cases/payload-analysis/case-003-5.0-ci-cno-networkpolicy-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-003/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-003/input.yamlplugins/ci/evals/cases/payload-analysis/case-004/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-004/input.yamlplugins/ci/evals/cases/payload-analysis/case-005/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-005/input.yamlplugins/ci/evals/cases/payload-analysis/case-006/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-006/input.yamlplugins/ci/evals/cases/payload-analysis/case-007/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-007/input.yamlplugins/ci/evals/cases/payload-analysis/case-008/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-008/input.yamlplugins/ci/evals/cases/payload-analysis/case-009-5.0-ci-hypershift-builder-fp/input.yamlplugins/ci/evals/cases/payload-analysis/case-009/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-009/input.yamlplugins/ci/evals/cases/payload-analysis/case-010-4.18-rejected-multiple-failures/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-010/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-010/input.yamlplugins/ci/evals/cases/payload-analysis/case-011-5.0-ci-infra-only-no-candidates/input.yamlplugins/ci/evals/cases/payload-analysis/case-011/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-011/input.yamlplugins/ci/evals/cases/payload-analysis/case-012/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-012/input.yamlplugins/ci/evals/cases/payload-analysis/case-013/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-013/input.yamlplugins/ci/evals/cases/payload-analysis/case-014/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-014/input.yamlplugins/ci/evals/cases/payload-analysis/case-018/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-018/input.yamlplugins/ci/evals/cases/payload-analysis/case-019/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-019/input.yamlplugins/ci/evals/cases/payload-analysis/case-020/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-020/input.yamlplugins/ci/evals/eval-payload-analysis.yamlplugins/ci/evals/test_eval_payload_analysis.pyplugins/ci/skills/analyze-disruption/SKILL.mdplugins/ci/skills/bulk-triage-regressions/SKILL.mdplugins/ci/skills/detect-permafail/SKILL.mdplugins/ci/skills/diagnose-job-run-symptoms/SKILL.mdplugins/ci/skills/diagnose-job-run-symptoms/diagnose_job_run.pyplugins/ci/skills/diagnose-job-run-symptoms/test_diagnose_job_run.pyplugins/ci/skills/fetch-job-run-summary/SKILL.mdplugins/ci/skills/fetch-prow-job-runs/SKILL.mdplugins/ci/skills/fetch-prow-job-runs/fetch_prow_job_runs.pyplugins/ci/skills/fetch-prow-job-runs/test_fetch_prow_job_runs.pyplugins/ci/skills/fetch-prowjob-json/SKILL.mdplugins/ci/skills/fetch-regression-details/SKILL.mdplugins/ci/skills/list-symptoms/SKILL.mdplugins/ci/skills/list-symptoms/list_symptoms.pyplugins/ci/skills/list-symptoms/test_list_symptoms.pyplugins/ci/skills/manage-labels/SKILL.mdplugins/ci/skills/manage-labels/manage_labels.pyplugins/ci/skills/manage-labels/test_manage_labels.pyplugins/ci/skills/manage-symptoms/SKILL.mdplugins/ci/skills/manage-symptoms/manage_symptoms.pyplugins/ci/skills/manage-symptoms/test_manage_symptoms.pyplugins/ci/skills/payload-analysis/SKILL.mdplugins/ci/skills/payload-autodl-json/SKILL.mdplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.pyplugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.pyplugins/ci/skills/payload-snapshot/scripts/test_sippy_fallback.pyplugins/ci/skills/payload-snapshot/scripts/test_test_classification.pyplugins/ci/skills/prow-job-analysis/SKILL.mdplugins/ci/skills/prow-job-analysis/prow_job_artifact_search.pyplugins/ci/skills/prow-job-analysis/references/aggregated.mdplugins/ci/skills/prow-job-analysis/references/artifacts.mdplugins/ci/skills/prow-job-analysis/references/ci-infrastructure-changes.mdplugins/ci/skills/prow-job-analysis/references/cloud-provider-errors.mdplugins/ci/skills/prow-job-analysis/references/disruption.mdplugins/ci/skills/prow-job-analysis/references/flaky-test-identification.mdplugins/ci/skills/prow-job-analysis/references/hypershift.mdplugins/ci/skills/prow-job-analysis/references/install/general.mdplugins/ci/skills/prow-job-analysis/references/install/metal.mdplugins/ci/skills/prow-job-analysis/references/networking.mdplugins/ci/skills/prow-job-analysis/references/operating-system-changes.mdplugins/ci/skills/prow-job-analysis/references/resource-exhaustion.mdplugins/ci/skills/prow-job-analysis/references/test-extension-binaries.mdplugins/ci/skills/prow-job-analysis/references/test-failure.mdplugins/ci/skills/prow-job-analysis/references/upgrade.mdplugins/ci/skills/prow-job-analyze-install-failure/SKILL.mdplugins/ci/skills/prow-job-analyze-metal-install-failure/SKILL.mdplugins/ci/skills/prow-job-analyze-test-failure/README.mdplugins/ci/skills/prow-job-analyze-test-failure/SKILL.mdplugins/ci/skills/prow-job-artifact-search/SKILL.mdplugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.pyplugins/ci/skills/prow-job-extract-must-gather/CHANGELOG.mdplugins/ci/skills/prow-job-extract-must-gather/README.mdplugins/ci/skills/prow-job-extract-must-gather/SKILL.mdplugins/ci/skills/prow-job-extract-must-gather/extract_archives.pyplugins/ci/skills/prow-job-extract-must-gather/generate_html_report.pyplugins/ci/skills/reevaluate-job-runs/SKILL.mdplugins/ci/skills/reevaluate-job-runs/reevaluate_job_runs.pyplugins/ci/skills/reevaluate-job-runs/test_reevaluate_job_runs.py
💤 Files with no reviewable changes (41)
- plugins/ci/evals/cases/payload-analysis/case-003-5.0-ci-cno-networkpolicy-revert/input.yaml
- plugins/ci/skills/prow-job-analysis/references/cloud-provider-errors.md
- plugins/ci/skills/prow-job-analysis/references/test-extension-binaries.md
- plugins/ci/skills/manage-symptoms/SKILL.md
- plugins/ci/skills/prow-job-analysis/references/test-failure.md
- plugins/ci/skills/diagnose-job-run-symptoms/diagnose_job_run.py
- plugins/ci/skills/prow-job-analysis/references/aggregated.md
- plugins/ci/skills/detect-permafail/SKILL.md
- plugins/ci/skills/bulk-triage-regressions/SKILL.md
- plugins/ci/skills/fetch-prow-job-runs/test_fetch_prow_job_runs.py
- plugins/ci/skills/diagnose-job-run-symptoms/SKILL.md
- plugins/ci/skills/manage-labels/manage_labels.py
- plugins/ci/skills/list-symptoms/test_list_symptoms.py
- plugins/ci/skills/prow-job-analysis/references/networking.md
- plugins/ci/skills/prow-job-analysis/references/operating-system-changes.md
- plugins/ci/skills/manage-labels/SKILL.md
- plugins/ci/skills/manage-symptoms/manage_symptoms.py
- plugins/ci/skills/list-symptoms/SKILL.md
- plugins/ci/skills/prow-job-analysis/references/flaky-test-identification.md
- plugins/ci/skills/prow-job-analysis/references/artifacts.md
- plugins/ci/skills/payload-snapshot/scripts/test_sippy_fallback.py
- plugins/ci/skills/diagnose-job-run-symptoms/test_diagnose_job_run.py
- plugins/ci/skills/list-symptoms/list_symptoms.py
- plugins/ci/evals/cases/payload-analysis/case-011-5.0-ci-infra-only-no-candidates/input.yaml
- plugins/ci/skills/prow-job-analysis/references/upgrade.md
- plugins/ci/skills/payload-snapshot/scripts/test_test_classification.py
- plugins/ci/evals/cases/payload-analysis/case-010-4.18-rejected-multiple-failures/annotations.yaml
- plugins/ci/skills/fetch-prow-job-runs/fetch_prow_job_runs.py
- plugins/ci/skills/prow-job-analysis/references/disruption.md
- plugins/ci/skills/manage-symptoms/test_manage_symptoms.py
- plugins/ci/skills/prow-job-analysis/references/ci-infrastructure-changes.md
- plugins/ci/skills/prow-job-analysis/references/install/general.md
- plugins/ci/skills/prow-job-analysis/prow_job_artifact_search.py
- plugins/ci/skills/payload-snapshot/scripts/test_collection_completeness.py
- plugins/ci/evals/cases/payload-analysis/case-009-5.0-ci-hypershift-builder-fp/input.yaml
- plugins/ci/skills/fetch-prow-job-runs/SKILL.md
- plugins/ci/skills/prow-job-analysis/references/resource-exhaustion.md
- plugins/ci/skills/prow-job-analysis/SKILL.md
- plugins/ci/skills/prow-job-analysis/references/hypershift.md
- plugins/ci/skills/prow-job-analysis/references/install/metal.md
- plugins/ci/skills/manage-labels/test_manage_labels.py
| PR #2920 is a feature-gate promotion and PR #2923 adds a DevPreview BGP | ||
| API for bare metal; neither explains default AWS and Azure operator | ||
| intervals. Experimental revert jobs continued to fail frequently for both | ||
| PRs, directly falsifying the recommendations. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'analysis_cutoff|`#2953`|`#2954`|experimental revert|later reverts|later outcomes|key_evidence' \
plugins/ci/evalsRepository: openshift-eng/ai-helpers
Length of output: 48044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
case_file="plugins/ci/evals/cases/payload-analysis/case-019/annotations.yaml"
printf '%s\n' '--- case annotation ---'
cat -n "$case_file" | sed -n '1,95p'
printf '%s\n' '--- related repository files ---'
git ls-files | rg 'case-019|2953|2954|payload-analysis-5\.0\.0-0\.ci-2026-07-25-102741|annotations\.yaml$' | sed -n '1,120p'
printf '%s\n' '--- GitHub PR timestamps and state ---'
for pr in 2953 2954; do
curl -fsSL "https://api.github.com/repos/openshift/api/pulls/$pr" |
jq -c '{number,state,created_at,updated_at,closed_at,merged_at,merge_commit_sha,head:.head.sha,base:.base.sha}'
doneRepository: openshift-eng/ai-helpers
Length of output: 12174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
summary='https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-main-claude-payload-agent/2080964771632386048/artifacts/claude-payload-agent/openshift-claude-payload-agent/artifacts/payload-analysis-5.0.0-0.ci-2026-07-25-102741-summary.html'
printf '%s\n' '--- linked source analysis references ---'
curl -fsSL --max-time 20 "$summary" |
sed 's/<[^>]*>/ /g' |
tr -s '[:space:]' ' ' |
fold -w 240 |
rg -n -i -C 2 '2953|2954|revert|0/5|1/5|2/5|3/5|completed|timestamp' |
sed -n '1,160p'
printf '%s\n' '--- public PR page metadata ---'
for pr in 2953 2954; do
printf 'PR #%s\n' "$pr"
curl -fsSL -L --max-time 20 -A 'Mozilla/5.0' \
"https://github.com/openshift/api/pull/$pr" |
rg -o 'datetime="[^"]+"|data-.*="[^"]+"' |
rg 'created|updated|closed|merged|2026-07' |
sed -n '1,30p' || true
doneRepository: openshift-eng/ai-helpers
Length of output: 243
🌐 Web query:
Find public timestamped evidence for openshift/api experimental revert PR #2953and PR#2954, including PR creation, closure, merge, and associated CI job completion times.
💡 Result:
There is no public evidence of experimental revert pull requests numbered #2953 or #2954 within the openshift/api repository [1][2]. The PR numbers #2953 and #2954 in the context of OpenShift-related repositories refer to different activities: 1. In the openshift/cluster-monitoring-operator repository, pull request #2953 is titled MON-4523: ClusterMonitoring prometheusConfig [3][4]. It was created on June 8, 2026, and merged on July 13, 2026 [3]. 2. In other repositories, such as openshift/operator-framework-olm, PRs #2953 and #2954 appear as part of unrelated updates [5][6]. 3. PR #2953 is also referenced in the openshift/node_exporter repository for a watchdog module update [7]. No evidence exists in the provided search results to link experimental revert PRs for openshift/api with these specific numbers [1][2].
Citations:
- 1: https://github.com/openshift/api/pulls
- 2: Check all hypershift variants when the featuregate is not platform specific openshift/api#2951
- 3: MON-4523: ClusterMonitoring prometheusConfig openshift/cluster-monitoring-operator#2953
- 4: https://origin-release.apps.ci.l2s4.p1.openshiftapps.com/changelog?from=5.0.0-0.okd-scos-2026-07-13-071329&to=5.0.0-0.okd-scos-2026-07-13-191329
- 5: openshift/operator-framework-olm@81e34cb...169e315
- 6: openshift/operator-framework-olm@4a69934...3563054
- 7: openshift/node_exporter@aed837c...29ba26d
🏁 Script executed:
#!/bin/bash
set -euo pipefail
summary='https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-main-claude-payload-agent/2080964771632386048/artifacts/claude-payload-agent/openshift-claude-payload-agent/artifacts/payload-analysis-5.0.0-0.ci-2026-07-25-102741-summary.html'
python3 - "$summary" <<'PY'
import html
import re
import sys
import urllib.request
url = sys.argv[1]
request = urllib.request.Request(url, headers={"User-Agent": "Mozilla/5.0"})
with urllib.request.urlopen(request, timeout=20) as response:
body = response.read().decode("utf-8", "replace")
text = html.unescape(re.sub(r"<[^>]*>", " ", body))
text = re.sub(r"\s+", " ", text)
terms = re.compile(r"2953|2954|revert|0/5|1/5|2/5|3/5|completed|timestamp", re.I)
matches = list(terms.finditer(text))
print(f"report_bytes={len(body)} matches={len(matches)}")
for match in matches:
start = max(0, match.start() - 300)
end = min(len(text), match.end() + 500)
print(text[start:end])
print("---")
PY
printf '%s\n' '--- GitHub PR page status ---'
for pr in 2953 2954; do
printf 'PR #%s: ' "$pr"
curl -sS -L --max-time 20 -A 'Mozilla/5.0' -o /tmp/api-pr-"$pr".html \
-w '%{http_code}\n' "https://github.com/openshift/api/pull/$pr"
rg -n -i -C 2 'experimental|revert|AWS|Azure|0/5|1/5|2/5|3/5|2026-07|not found' \
/tmp/api-pr-"$pr".html | sed -n '1,80p' || true
doneRepository: openshift-eng/ai-helpers
Length of output: 2448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
summary='https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/logs/periodic-ci-openshift-release-main-claude-payload-agent/2080964771632386048/artifacts/claude-payload-agent/openshift-claude-payload-agent/artifacts/payload-analysis-5.0.0-0.ci-2026-07-25-102741-summary.html'
curl -kfsSL --max-time 20 "$summary" |
python3 -c '
import html, re, sys
body = sys.stdin.read()
text = html.unescape(re.sub(r"<[^>]*>", " ", body))
text = re.sub(r"\s+", " ", text)
terms = re.compile(r"2953|2954|revert|0/5|1/5|2/5|3/5|completed|timestamp", re.I)
matches = list(terms.finditer(text))
print(f"report_chars={len(body)} matches={len(matches)}")
for match in matches:
print(text[max(0, match.start()-300):min(len(text), match.end()+500)])
print("---")
'
printf '%s\n' '--- GitHub PR page status ---'
for pr in 2953 2954; do
printf 'PR #%s: ' "$pr"
curl -sS -L --max-time 20 -A 'Mozilla/5.0' -o "/tmp/api-pr-$pr.html" \
-w '%{http_code}\n' "https://github.com/openshift/api/pull/$pr" || true
rg -n -i -C 2 'experimental|revert|AWS|Azure|0/5|1/5|2/5|3/5|2026-07|not found' \
"/tmp/api-pr-$pr.html" | sed -n '1,80p' || true
doneRepository: openshift-eng/ai-helpers
Length of output: 50380
Remove post-cutoff revert outcomes from the oracle.
PRs #2953 and #2954 were created at 2026-07-25T18:13:21Z, after the 2026-07-25T16:05:26Z cutoff. Remove their outcomes from key_evidence and notes.
🤖 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/evals/cases/payload-analysis/case-019/annotations.yaml` around
lines 57 - 60, Remove all outcomes referring to PRs `#2953` and `#2954` from the
key_evidence and notes entries in the payload-analysis annotation, preserving
evidence from before the 2026-07-25T16:05:26Z cutoff.
| execution: | ||
| mode: case | ||
| arguments: "{payload_tag} --snapshot-dir $EVAL_SNAPSHOT_DIR/{payload_tag}" | ||
| arguments: "{payload_tag} --snapshot-dir $EVAL_SNAPSHOT_DIR/{payload_tag} --as-of {payload_completed_at}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find cases missing payload_completed_at. 2) Confirm --as-of is documented in the ci plugin.
set -uo pipefail
echo "== cases missing payload_completed_at =="
fd 'input.yaml' plugins/ci/evals/cases/payload-analysis | sort | while IFS= read -r f; do
rg -q '^payload_completed_at:' "$f" || echo "MISSING: $f"
done
echo "== --as-of references in plugins/ci =="
rg -n -- '--as-of' plugins/ciRepository: openshift-eng/ai-helpers
Length of output: 522
🏁 Script executed:
set -uo pipefail
echo "== payload-analysis eval definition =="
cat -n plugins/ci/evals/eval-payload-analysis.yaml
echo "== payload-analysis command and skill references =="
rg -n -i --glob '!plugins/ci/evals/eval-payload-analysis.yaml' \
'payload.?analysis|as-of|snapshot-dir|payload_completed_at' .
echo "== candidate command files =="
fd -t f | rg -i 'payload|analysis|ci' | head -200Repository: openshift-eng/ai-helpers
Length of output: 50380
🏁 Script executed:
set -uo pipefail
echo "== candidate payload-analysis files =="
fd -t f -i 'payload-analysis' plugins/ci | sort
echo "== command/skill declarations =="
rg -n -i --glob '*.md' \
'(^#|^description:|arguments|--as-of|snapshot-dir|payload_completed_at|payload tag)' \
plugins/ci/commands plugins/ci/skills | rg -i 'payload-analysis|as-of|snapshot-dir|payload_completed_at' | head -200
echo "== exact --as-of occurrences outside the eval =="
rg -n --glob '!plugins/ci/evals/eval-payload-analysis.yaml' -- '--as-of' plugins/ci || trueRepository: openshift-eng/ai-helpers
Length of output: 6975
🏁 Script executed:
set -uo pipefail
echo "== payload-analysis argument handling =="
cat -n plugins/ci/skills/payload-analysis/SKILL.md | sed -n '1,95p'
echo "== payload-analysis time and cutoff handling =="
rg -n -i 'as-of|cutoff|timestamp|completed_at|point-in-time|historical|before|after' \
plugins/ci/skills/payload-analysis/SKILL.mdRepository: openshift-eng/ai-helpers
Length of output: 9533
Implement --as-of handling in ci:payload-analysis. The skill accepts only <payload-tag> [--snapshot-dir DIR] and has no cutoff logic. The eval passes an unhandled argument, so the analysis can use evidence after payload_completed_at.
🤖 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/evals/eval-payload-analysis.yaml` at line 7, Update the
ci:payload-analysis skill’s argument handling to accept an optional --as-of
cutoff alongside the existing payload tag and --snapshot-dir arguments. Parse
and validate the cutoff, then constrain all payload analysis evidence to data at
or before that timestamp, while preserving current behavior when --as-of is
omitted.
| if name == "Read": | ||
| path = tool_input.get("file_path", "") | ||
| if ( | ||
| path.endswith("annotations.yaml") | ||
| or "/evals/cases/payload-analysis/README.md" in path | ||
| or path.endswith("/comments.json") | ||
| ): | ||
| violations.append(f"Read evaluation/future-leaking file: {path}") | ||
|
|
||
| if name == "WebFetch": | ||
| url = tool_input.get("url", "") | ||
| check_external_url(url, "WebFetch") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Extend the leakage check to the Grep and Glob tools.
The judge inspects only Read, WebFetch, and Bash tool calls. The agent can read expected answers with Grep by passing a case directory as path and an output mode that returns content. Glob can enumerate the case directories to locate those files. Both paths bypass this hard gate. Check the path, pattern, and glob inputs of those tools with the same file patterns.
🛡️ Proposed coverage extension
- if name == "Read":
- path = tool_input.get("file_path", "")
- if (
- path.endswith("annotations.yaml")
- or "/evals/cases/payload-analysis/README.md" in path
- or path.endswith("/comments.json")
- ):
- violations.append(f"Read evaluation/future-leaking file: {path}")
+ leaking = re.compile(
+ r"annotations\.yaml|"
+ r"/evals/cases/payload-analysis/README\.md|"
+ r"/comments\.json|"
+ r"evals/cases/payload-analysis",
+ re.I,
+ )
+
+ if name in {"Read", "Grep", "Glob", "NotebookRead"}:
+ targets = [
+ str(tool_input.get(key, ""))
+ for key in ("file_path", "path", "glob", "pattern", "notebook_path")
+ ]
+ for target in targets:
+ if target and leaking.search(target):
+ violations.append(
+ f"{name} accessed evaluation/future-leaking path: {target}"
+ )
+ break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if name == "Read": | |
| path = tool_input.get("file_path", "") | |
| if ( | |
| path.endswith("annotations.yaml") | |
| or "/evals/cases/payload-analysis/README.md" in path | |
| or path.endswith("/comments.json") | |
| ): | |
| violations.append(f"Read evaluation/future-leaking file: {path}") | |
| if name == "WebFetch": | |
| url = tool_input.get("url", "") | |
| check_external_url(url, "WebFetch") | |
| leaking = re.compile( | |
| r"annotations\.yaml|" | |
| r"/evals/cases/payload-analysis/README\.md|" | |
| r"/comments\.json|" | |
| r"evals/cases/payload-analysis", | |
| re.I, | |
| ) | |
| if name in {"Read", "Grep", "Glob", "NotebookRead"}: | |
| targets = [ | |
| str(tool_input.get(key, "")) | |
| for key in ("file_path", "path", "glob", "pattern", "notebook_path") | |
| ] | |
| for target in targets: | |
| if target and leaking.search(target): | |
| violations.append( | |
| f"{name} accessed evaluation/future-leaking path: {target}" | |
| ) | |
| break | |
| if name == "WebFetch": | |
| url = tool_input.get("url", "") | |
| check_external_url(url, "WebFetch") |
🤖 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/evals/eval-payload-analysis.yaml` around lines 403 - 414, Extend
the leakage checks alongside the existing Read handling to also process Grep and
Glob tool calls. Inspect each tool’s path, pattern, and glob inputs for the same
annotations.yaml, payload-analysis README, and comments.json patterns, and
append the existing violation message when matched; preserve the current Read,
WebFetch, and Bash checks.
| def _has_blocking_failures(self, tag_name: str) -> bool: | ||
| """Check whether a payload has any failed blocking jobs.""" | ||
| runs = self.sippy.fetch_job_runs(tag_name) | ||
| blocking = [r for r in runs if r.get("kind") == "Blocking"] | ||
| if not blocking: | ||
| tag_meta = self.sippy.find_tag(tag_name) | ||
| return bool( | ||
| tag_meta | ||
| and tag_meta.get("phase") == "Accepted" | ||
| and not tag_meta.get("forced", False) | ||
| and tag_meta.get("failed_job_names") == [] | ||
| ) | ||
| return all( | ||
| r.get("state") == "Succeeded" for r in blocking | ||
| return any( | ||
| r.get("kind") == "Blocking" and r.get("state") == "Failed" | ||
| for r in runs | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Baseline detection in Sippy mode accepts non-green payloads.
_has_blocking_failures only treats state == "Failed" as a failure. PayloadChain._all_blocking_passed (lines 393-408) instead requires every blocking job to be Succeeded. Two consequences follow in Sippy mode:
- A payload whose blocking jobs are
Error,Aborted, orPendingcounts as green, sobuildstops there. - A payload with no returned job runs counts as green, so the chain stops at the first tag without Sippy data.
Both cases produce a false baseline. That value propagates to baseline_tag, chain_length, hours_since_baseline, and the streak and regression trackers. Require explicit success instead.
🐛 Proposed fix
- def _has_blocking_failures(self, tag_name: str) -> bool:
- """Check whether a payload has any failed blocking jobs."""
- runs = self.sippy.fetch_job_runs(tag_name)
- return any(
- r.get("kind") == "Blocking" and r.get("state") == "Failed"
- for r in runs
- )
+ def _has_blocking_failures(self, tag_name: str) -> bool:
+ """Check whether any blocking job did not succeed.
+
+ Missing job-run data counts as a failure so the chain does not
+ stop on an unverified payload.
+ """
+ runs = self.sippy.fetch_job_runs(tag_name)
+ blocking = [r for r in runs if r.get("kind") == "Blocking"]
+ if not blocking:
+ return True
+ return any(r.get("state") != "Succeeded" for r in blocking)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _has_blocking_failures(self, tag_name: str) -> bool: | |
| """Check whether a payload has any failed blocking jobs.""" | |
| runs = self.sippy.fetch_job_runs(tag_name) | |
| blocking = [r for r in runs if r.get("kind") == "Blocking"] | |
| if not blocking: | |
| tag_meta = self.sippy.find_tag(tag_name) | |
| return bool( | |
| tag_meta | |
| and tag_meta.get("phase") == "Accepted" | |
| and not tag_meta.get("forced", False) | |
| and tag_meta.get("failed_job_names") == [] | |
| ) | |
| return all( | |
| r.get("state") == "Succeeded" for r in blocking | |
| return any( | |
| r.get("kind") == "Blocking" and r.get("state") == "Failed" | |
| for r in runs | |
| ) | |
| def _has_blocking_failures(self, tag_name: str) -> bool: | |
| """Check whether any blocking job did not succeed. | |
| Missing job-run data counts as a failure so the chain does not | |
| stop on an unverified payload. | |
| """ | |
| runs = self.sippy.fetch_job_runs(tag_name) | |
| blocking = [r for r in runs if r.get("kind") == "Blocking"] | |
| if not blocking: | |
| return True | |
| return any(r.get("state") != "Succeeded" for r in blocking) |
🤖 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
418 - 424, Update _has_blocking_failures to require explicit success for every
blocking job, matching PayloadChain._all_blocking_passed: treat Error, Aborted,
Pending, and any other non-Succeeded state as failure, and treat an empty runs
result as not green. Preserve the existing Sippy fetch flow while ensuring
baseline detection only succeeds when blocking job data is present and all
blocking jobs have state Succeeded.
| 2. **Extract build_id** | ||
| - Search for pattern `/(\d{10,})/` in the bucket path | ||
| - build_id must be at least 10 consecutive decimal digits | ||
| - Handle URLs with or without trailing slash | ||
| - If not found, error: "Could not find build ID (10+ digits) in URL" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept build IDs at the end of the URL.
The pattern /(\d{10,})/ requires a slash after the build ID. The valid example at line 35 ends with the build ID, so this workflow reports an invalid URL.
Use boundaries that accept either a slash or end of input after the ID, for example (?:^|/)(\d{10,})(?:/|$).
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 124: [AS3] Skill Enumeration: Skill enumerates or reads other installed skills. Access to other skills' SKILL.md files or the skills directory reveals prompt instructions, capabilities, and secrets that should be invisible to peer skills.
Remediation: Remove all code or instructions that list or read other skills' files or directories. Skills should operate independently; cross-skill access is a privilege escalation.
(Agent Snooping (AS3))
🤖 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/prow-job-analyze-install-failure/SKILL.md` around lines 93
- 97, Update the build_id extraction guidance in “Extract build_id” to accept
IDs followed by either a slash or the end of the URL, while still requiring at
least 10 consecutive digits and a valid path boundary before the ID. Preserve
the existing error message when no matching build ID is found.
| def cmd_fetch(prefix, filepath, max_bytes=DEFAULT_MAX_BYTES): | ||
| """Fetch contents of a specific file from GCS.""" | ||
| target = gcs_path(prefix, filepath) | ||
|
|
||
| # Download to a temp file, then read | ||
| import tempfile | ||
|
|
||
| with tempfile.NamedTemporaryFile(delete=False, suffix=".artifact") as tmp: | ||
| tmp_path = tmp.name | ||
|
|
||
| try: | ||
| _stdout, stderr, rc = run_gcloud( | ||
| ["storage", "cp", target, tmp_path, "--no-user-output-enabled"], | ||
| timeout=60, | ||
| ) | ||
|
|
||
| if rc != 0: | ||
| return { | ||
| "success": False, | ||
| "error": f"gcloud storage cp failed: {stderr.strip()}", | ||
| "path": target, | ||
| } | ||
|
|
||
| file_size = os.path.getsize(tmp_path) | ||
| truncated = file_size > max_bytes | ||
|
|
||
| with open(tmp_path, "r", errors="replace") as f: | ||
| content = f.read(max_bytes) | ||
|
|
||
| return { | ||
| "success": True, | ||
| "path": target, | ||
| "size_bytes": file_size, | ||
| "truncated": truncated, | ||
| "max_bytes": max_bytes, | ||
| "content": content, | ||
| } | ||
| finally: | ||
| if os.path.exists(tmp_path): | ||
| os.unlink(tmp_path) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce --max-bytes before downloading the object.
gcloud storage cp downloads the complete artifact to disk before line 245 reads only max_bytes. A multi-gigabyte artifact can exhaust disk space or stall the workflow despite the documented 512 KB default. Negative values also bypass the intended read limit.
Reject non-positive limits. Use object metadata plus a range-capable read, or reject objects larger than the requested limit before transfer.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 243-243: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(tmp_path, "r", errors="replace")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/prow-job-artifact-search/prow_job_artifact_search.py`
around lines 218 - 257, Update cmd_fetch to reject non-positive max_bytes values
and enforce the limit before downloading object contents. Use GCS metadata to
determine the object size, reject objects exceeding max_bytes before transfer,
and use a range-capable read limited to max_bytes for accepted objects while
preserving the existing response fields and cleanup behavior.
| help="Prow job URL (https://prow.ci.openshift.org/view/gs/...)", | ||
| ) | ||
|
|
||
| subparsers = parser.add_subparsers(dest="command", required=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files 'plugins/ci/skills/prow-job-artifact-search/*'
printf '%s\n' '--- source outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py
else
wc -l plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py
fi
printf '%s\n' '--- parser and argument flow ---'
rg -n -C 12 'add_subparsers|parse_args|command|subparser|ArgumentParser' \
plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py
printf '%s\n' '--- documented requirements ---'
cat -n plugins/ci/skills/prow-job-artifact-search/SKILL.md | sed -n '20,40p'
printf '%s\n' '--- version constraints and references ---'
rg -n -i -C 3 'python(3)?(\.|\s|$)|python_requires|requires-python|3\.6|3\.7|add_subparsers|required=True' \
plugins/ci/skills/prow-job-artifact-search .github pyproject.toml setup.py setup.cfg tox.ini 2>/dev/null || true
printf '%s\n' '--- tests ---'
rg -n -i -C 3 'prow.?job.?artifact.?search|artifact.?search|parse_args|subparsers' \
--glob '*test*' --glob '*.py' . 2>/dev/null | head -n 300 || trueRepository: openshift-eng/ai-helpers
Length of output: 41405
🌐 Web query:
site:docs.python.org/3.7/library/argparse.html add_subparsers required changed in version 3.7 site:docs.python.org/3.7/library/subprocess.html capture_output text
💡 Result:
In Python 3.7, two notable changes occurred regarding the argparse and subprocess modules: 1. argparse.add_subparsers: The required keyword argument was added in version 3.7 [1]. This parameter allows you to specify whether a subcommand must be provided, defaulting to False [1]. 2. subprocess: The capture_output and text parameters were added in version 3.7 [2]. - capture_output: When set to True, it instructs the subprocess to capture both stdout and stderr [2]. - text: This serves as a more readable alias for universal_newlines=True; when set to True, the returned streams are opened in text mode rather than binary mode [2].
Citations:
- 1: https://docs.python.org/3.7/library/argparse.html
- 2: https://docs.python.org/3.7/library/subprocess.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- source imports and subprocess calls ---'
cat -n plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py | sed -n '1,135p'
rg -n -C 8 'subprocess\.run|capture_output|text=|universal_newlines' \
plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py
printf '%s\n' '--- repository Python version declarations ---'
for f in .python-version pyproject.toml setup.py setup.cfg tox.ini requirements-dev.txt .github/workflows/test.yml; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,100p'
fi
doneRepository: openshift-eng/ai-helpers
Length of output: 7334
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py")
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if not isinstance(node, ast.Call):
continue
if isinstance(node.func, ast.Attribute) and node.func.attr == "add_subparsers":
keywords = {kw.arg: ast.unparse(kw.value) for kw in node.keywords}
print(f"add_subparsers line {node.lineno}: {keywords}")
if isinstance(node.func, ast.Attribute) and node.func.attr == "run":
if isinstance(node.func.value, ast.Name) and node.func.value.id == "subprocess":
keywords = {kw.arg: ast.unparse(kw.value) for kw in node.keywords}
print(f"subprocess.run line {node.lineno}: {keywords}")
parse_args_lines = [
node.lineno for node in ast.walk(tree)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and node.func.attr == "parse_args"
]
print(f"parse_args calls: {parse_args_lines}")
PYRepository: openshift-eng/ai-helpers
Length of output: 411
Align the documented Python minimum with the script APIs.
SKILL.md declares Python 3.6+, but the script uses Python 3.7-only add_subparsers(required=True), capture_output=True, and text=True. Raise the documented minimum to Python 3.7, or replace all three APIs with Python 3.6-compatible code and add post-parse command validation.
📍 Affects 2 files
plugins/ci/skills/prow-job-artifact-search/prow_job_artifact_search.py#L269-L269(this comment)plugins/ci/skills/prow-job-artifact-search/SKILL.md#L31-L32
🤖 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/prow-job-artifact-search/prow_job_artifact_search.py` at
line 269, Align the Python version contract across the script and documentation:
update prow_job_artifact_search.py to avoid the Python 3.7-only
add_subparsers(required=True), capture_output=True, and text=True APIs with
Python 3.6-compatible equivalents, including explicit post-parse command
validation; update SKILL.md lines 31-32 to retain the documented Python 3.6+
minimum. Alternatively, raise the documented minimum in SKILL.md lines 31-32 to
Python 3.7 and keep all three APIs unchanged.
| def extract_tar_archive(tar_path, extract_to): | ||
| """Extract a tar archive (including .tar.gz and .tgz).""" | ||
| try: | ||
| print(f" Extracting: {tar_path}") | ||
| with tarfile.open(tar_path, 'r:*') as tar: | ||
| tar.extractall(path=extract_to) | ||
| return True | ||
| except Exception as e: | ||
| print(f" ERROR: Failed to extract {tar_path}: {e}", file=sys.stderr) | ||
| return False |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
sed -n '1,220p' plugins/ci/skills/prow-job-extract-must-gather/extract_archives.py
printf '%s\n' '--- Python version declarations and CI references ---'
rg -n -i --glob '!node_modules/**' --glob '!dist/**' \
'python(3|:)?[[:space:]]*[0-9]+\.[0-9]+|requires-python|python-version|setup-python|PYTHON_VERSION|python_requires' \
. | head -250
printf '%s\n' '--- extraction call sites and tests ---'
rg -n -C 3 'extract_tar_archive|extractall|must-gather|prow-job-extract-must-gather' \
plugins/ci .github 2>/dev/null | head -300Repository: openshift-eng/ai-helpers
Length of output: 43564
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- analogous tar extraction compatibility code ---'
sed -n '980,1060p' plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py
printf '%s\n' '--- skill documentation around prerequisites and execution ---'
sed -n '1,120p' plugins/ci/skills/prow-job-extract-must-gather/SKILL.md
sed -n '100,155p' plugins/ci/skills/prow-job-extract-must-gather/README.md
printf '%s\n' '--- standalone tarfile behavior probe ---'
python3 - <<'PY'
import io
import os
import sys
import tarfile
import tempfile
from pathlib import Path
print(f"python={sys.version.split()[0]}")
print(f"extractall_accepts_filter={'filter' in __import__('inspect').signature(tarfile.TarFile.extractall).parameters}")
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp) / "root"
root.mkdir()
outside = Path(tmp) / "outside.txt"
archive = io.BytesIO()
with tarfile.open(fileobj=archive, mode="w") as tar:
payload = b"outside"
member = tarfile.TarInfo("../../outside.txt")
member.size = len(payload)
tar.addfile(member, io.BytesIO(payload))
archive.seek(0)
with tarfile.open(fileobj=archive, mode="r:") as tar:
try:
tar.extractall(path=root)
result = "completed"
except Exception as exc:
result = f"{type(exc).__name__}: {exc}"
print(f"unfiltered_result={result}")
print(f"outside_exists={outside.exists()}")
if outside.exists():
print(f"outside_contents={outside.read_text()}")
PYRepository: openshift-eng/ai-helpers
Length of output: 11153
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- analogous tar extraction compatibility code ---'
sed -n '980,1060p' plugins/sosreport/skills/ovs-db-analysis/scripts/analyze_ovs_db.py
printf '%s\n' '--- skill documentation around prerequisites and execution ---'
sed -n '1,120p' plugins/ci/skills/prow-job-extract-must-gather/SKILL.md
sed -n '100,155p' plugins/ci/skills/prow-job-extract-must-gather/README.md
printf '%s\n' '--- standalone tarfile behavior probe ---'
python3 - <<'PY'
import io
import sys
import tarfile
import tempfile
import inspect
from pathlib import Path
print(f"python={sys.version.split()[0]}")
print(f"extractall_accepts_filter={'filter' in inspect.signature(tarfile.TarFile.extractall).parameters}")
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp) / "root"
root.mkdir()
outside = Path(tmp) / "outside.txt"
archive = io.BytesIO()
with tarfile.open(fileobj=archive, mode="w") as tar:
payload = b"outside"
member = tarfile.TarInfo("../../outside.txt")
member.size = len(payload)
tar.addfile(member, io.BytesIO(payload))
archive.seek(0)
with tarfile.open(fileobj=archive, mode="r:") as tar:
try:
tar.extractall(path=root)
result = "completed"
except Exception as exc:
result = f"{type(exc).__name__}: {exc}"
print(f"unfiltered_result={result}")
print(f"outside_exists={outside.exists()}")
if outside.exists():
print(f"outside_contents={outside.read_text()}")
PYRepository: openshift-eng/ai-helpers
Length of output: 11153
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact runtime tarfile API and implementation ---'
python3 - <<'PY'
import inspect
import sys
import tarfile
print(f"python={sys.version}")
print(f"tarfile={tarfile.__file__}")
print(f"has_data_filter={hasattr(tarfile, 'data_filter')}")
print(f"extractall_signature={inspect.signature(tarfile.TarFile.extractall)}")
print("extractall_source:")
print(inspect.getsource(tarfile.TarFile.extractall))
PY
printf '%s\n' '--- corrected path-traversal probe ---'
python3 - <<'PY'
import io
import sys
import tarfile
import tempfile
from pathlib import Path
print(f"python={sys.version.split()[0]}")
with tempfile.TemporaryDirectory() as tmp:
base = Path(tmp)
root = base / "root"
root.mkdir()
targets = {
"parent": base / "outside.txt",
"absolute": Path("/tmp/coderabbit-tarfile-absolute-probe"),
}
archive = io.BytesIO()
with tarfile.open(fileobj=archive, mode="w") as tar:
for name in ("../outside.txt", "../../outside.txt", "/tmp/coderabbit-tarfile-absolute-probe"):
payload = name.encode()
member = tarfile.TarInfo(name)
member.size = len(payload)
tar.addfile(member, io.BytesIO(payload))
archive.seek(0)
with tarfile.open(fileobj=archive, mode="r:") as tar:
try:
tar.extractall(path=root)
result = "completed"
except Exception as exc:
result = f"{type(exc).__name__}: {exc}"
print(f"unfiltered_result={result}")
for label, path in targets.items():
print(f"{label}_exists={path.exists()}")
if path.exists():
path.unlink()
PY
printf '%s\n' '--- runtime image declarations ---'
sed -n '1,55p' images/Dockerfile
sed -n '1,55p' images/Dockerfile.nested-podmanRepository: openshift-eng/ai-helpers
Length of output: 8113
Prevent tar path traversal on all supported Python versions.
tar.extractall(path=extract_to) allows archive members to write outside extract_to. This affects the top-level and nested archives. The repository image installs Python 3.11, and the documentation requires only Python 3. Use filter='data' when supported. Otherwise, validate members and reject traversal, absolute paths, links, and unsafe special files, or require Python 3.12+.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 25-25: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: tar.extractall(path=extract_to)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🪛 Ruff (0.16.0)
[error] 26-26: Uses of tarfile.extractall()
(S202)
[warning] 28-28: Do not catch blind exception: Exception
(BLE001)
🤖 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/prow-job-extract-must-gather/extract_archives.py` around
lines 21 - 30, Harden extract_tar_archive so every supported Python version
prevents tar path traversal for both top-level and nested archives. Use
tar.extractall’s filter='data' where available, and provide a Python
3-compatible fallback that rejects absolute paths, traversal outside extract_to,
links, and unsafe special files before extraction; do not leave the existing
unfiltered extractall path.
| # Get directory path (everything except filename) | ||
| dir_path = os.path.dirname(rel_path) | ||
|
|
||
| # Get top-level directory (first segment after content/) | ||
| top_level_dir = '' | ||
| if dir_path.startswith('content/'): | ||
| path_parts = dir_path.split('/', 2) | ||
| if len(path_parts) >= 2: | ||
| top_level_dir = path_parts[1] | ||
| dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1 | ||
| elif '/' in dir_path: | ||
| # If not under content/, use first directory | ||
| top_level_dir = dir_path.split('/', 1)[0] | ||
| dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Files with no subdirectory are silently hidden from the browser.
When dir_path has no further nesting under content/ (or has no / at all), top_level_dir stays '' and is never added to dir_counts. The directory filter buttons and the default active filter set are both derived from dir_counts.keys(), so files with top_level_dir == '' have no corresponding filter button and are excluded from the default (all-filters-active) view. These files become invisible in the browser with no visible indication.
Assign a sentinel value for files with no subdirectory instead of leaving top_level_dir unset, so they are represented by a real filter entry.
🐛 Proposed fix
# Get top-level directory (first segment after content/)
top_level_dir = ''
if dir_path.startswith('content/'):
path_parts = dir_path.split('/', 2)
if len(path_parts) >= 2:
top_level_dir = path_parts[1]
- dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1
elif '/' in dir_path:
# If not under content/, use first directory
top_level_dir = dir_path.split('/', 1)[0]
- dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1
+ else:
+ # No further nesting: bucket under a visible sentinel
+ top_level_dir = dir_path if dir_path else '(root)'
+ dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get directory path (everything except filename) | |
| dir_path = os.path.dirname(rel_path) | |
| # Get top-level directory (first segment after content/) | |
| top_level_dir = '' | |
| if dir_path.startswith('content/'): | |
| path_parts = dir_path.split('/', 2) | |
| if len(path_parts) >= 2: | |
| top_level_dir = path_parts[1] | |
| dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1 | |
| elif '/' in dir_path: | |
| # If not under content/, use first directory | |
| top_level_dir = dir_path.split('/', 1)[0] | |
| dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1 | |
| # Get directory path (everything except filename) | |
| dir_path = os.path.dirname(rel_path) | |
| # Get top-level directory (first segment after content/) | |
| top_level_dir = '' | |
| if dir_path.startswith('content/'): | |
| path_parts = dir_path.split('/', 2) | |
| if len(path_parts) >= 2: | |
| top_level_dir = path_parts[1] | |
| elif '/' in dir_path: | |
| # If not under content/, use first directory | |
| top_level_dir = dir_path.split('/', 1)[0] | |
| else: | |
| # No further nesting: bucket under a visible sentinel | |
| top_level_dir = dir_path if dir_path else '(root)' | |
| dir_counts[top_level_dir] = dir_counts.get(top_level_dir, 0) + 1 |
🤖 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/prow-job-extract-must-gather/generate_html_report.py`
around lines 93 - 106, Update the top_level_dir derivation in the
report-generation logic so files directly under content/ or with no directory
separator receive a non-empty sentinel category instead of ''. Ensure that
sentinel is added to dir_counts and therefore appears in the directory filter
buttons and default active-filter set, while preserving existing
nested-directory categorization.
| for file in files: | ||
| icon = get_file_icon(file['type']) | ||
| # Use symlink path for iframe if available, otherwise use original | ||
| iframe_path = file.get('symlink_path', f"logs/{file['path']}") | ||
| original_path = f"logs/{file['path']}" | ||
|
|
||
| html += f''' | ||
| <div class="file-item" data-type="{file['type']}" data-path="{file['path']}" data-name="{file['name'].lower()}" data-dir="{file['top_level_dir']}" data-size="{file['size']}"> | ||
| <div class="file-icon">{icon}</div> | ||
| <div class="file-info"> | ||
| <div class="file-name"> | ||
| <a class="file-link" data-iframe-path="{iframe_path}" data-original-path="{original_path}" data-size="{file['size']}">{file['name']}</a> | ||
| <span class="external-link-icon" data-path="{original_path}" title="Open in new tab"> | ||
| <svg viewBox="0 0 16 16"> | ||
| <path d="M3.75 2A1.75 1.75 0 002 3.75v8.5c0 .966.784 1.75 1.75 1.75h8.5A1.75 1.75 0 0014 12.25v-3.5a.75.75 0 00-1.5 0v3.5a.25.25 0 01-.25.25h-8.5a.25.25 0 01-.25-.25v-8.5a.25.25 0 01.25-.25h3.5a.75.75 0 000-1.5h-3.5zM9.5 2.75a.75.75 0 01.75-.75h3.5a.75.75 0 01.75.75v3.5a.75.75 0 01-1.5 0V4.56L8.78 8.78a.75.75 0 01-1.06-1.06l4.22-4.22h-1.69a.75.75 0 01-.75-.75z"/> | ||
| </svg> | ||
| </span> | ||
| </div> | ||
| <div class="file-meta"> | ||
| <span class="file-path">{file['dir']}</span> | ||
| <span class="file-size-badge">{file['size_human']}</span> | ||
| <span class="badge badge-{file['type']}">{file['type']}</span> | ||
| </div> | ||
| </div> | ||
| </div>''' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape file metadata and CLI-derived strings before embedding in HTML.
file['name'], file['path'], file['dir'], and file['top_level_dir'] are interpolated directly into HTML attributes and text (lines 712-726) without escaping. The same problem exists for the header metadata prowjob_name, build_id, target, gcsweb_url (lines 629-636, with gcsweb_url unescaped inside an href attribute) and for the directory filter button's directory/display_name (lines 680-685).
These values originate from must-gather content or from a user-supplied Prow job URL. A name or URL containing ", <, or > can break out of an HTML attribute or inject markup/script into the generated report. create_txt_symlinks already uses html_module.escape() elsewhere in this file; apply the same escaping here.
🛡️ Proposed fix (file-item block; apply the same pattern to the header and directory filter)
for file in files:
icon = get_file_icon(file['type'])
# Use symlink path for iframe if available, otherwise use original
- iframe_path = file.get('symlink_path', f"logs/{file['path']}")
+ iframe_path = file.get('symlink_path') or f"logs/{file['path']}"
original_path = f"logs/{file['path']}"
+ safe_name = html_module.escape(file['name'])
+ safe_path = html_module.escape(file['path'])
+ safe_dir = html_module.escape(file['dir'])
+ safe_top_level_dir = html_module.escape(file['top_level_dir'])
html += f'''
- <div class="file-item" data-type="{file['type']}" data-path="{file['path']}" data-name="{file['name'].lower()}" data-dir="{file['top_level_dir']}" data-size="{file['size']}">
+ <div class="file-item" data-type="{file['type']}" data-path="{safe_path}" data-name="{html_module.escape(file['name'].lower())}" data-dir="{safe_top_level_dir}" data-size="{file['size']}">
<div class="file-icon">{icon}</div>
<div class="file-info">
<div class="file-name">
- <a class="file-link" data-iframe-path="{iframe_path}" data-original-path="{original_path}" data-size="{file['size']}">{file['name']}</a>
+ <a class="file-link" data-iframe-path="{html_module.escape(iframe_path)}" data-original-path="{html_module.escape(original_path)}" data-size="{file['size']}">{safe_name}</a>
<span class="external-link-icon" data-path="{original_path}" title="Open in new tab">
<div class="file-meta">
- <span class="file-path">{file['dir']}</span>
+ <span class="file-path">{safe_dir}</span>🤖 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/prow-job-extract-must-gather/generate_html_report.py`
around lines 705 - 729, Escape all must-gather and CLI-derived values before
interpolating them into generated HTML. In the file-item loop, apply
html_module.escape to file['name'], file['path'], file['dir'],
file['top_level_dir'], symlink_path, and related metadata used in attributes or
text; apply the same escaping in the header metadata block for prowjob_name,
build_id, target, and gcsweb_url (including its href), and in the directory
filter for directory and display_name. Reuse the existing html_module.escape
pattern without changing the report’s displayed values or behavior.
stbenjam
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@stbenjam: This pull request references TRT-2870 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Closing this experiment PR. The hard-case A/B/C evals are very expensive to run (~50% increase in payload agent eval cost) and Opus doesn't succeed on them regardless, so the signal-to-cost ratio isn't there right now. Will revisit when better models are available that can handle these cases. |
2 participants
These bugs and fixes were automatically generated by a payload-agent experiment to improve resilience and diagnostics for infrastructure failures. Please review the PR and either shepherd it to merge or close it. If the work is incorrect or unhelpful, a brief comment would help us improve. Thanks, and apologies if we missed the mark.
Experiment
This is group C, the historical-skill control for the payload-analysis evaluation. It is evaluation-only and is not intended to merge.
plugins/ci/skills/**is restored toe9065e6, the commit immediately before TRT-2613: feat(ci): prow-job-analysis skill + artifact-verified evals (supersedes #597) #604 merged (fd05958).prow-job-analysismegaskill and later CI skills are absent.git diff --checkpasses.This makes the three groups:
Reference change: PR #604.
Failed payload analyses represented by the new cases
Validation
plugins/ci/evals/**is unchanged from group A commit5ec4bab.e9065e6aside from two whitespace-only normalizations.python3 -m pytest -q plugins/ci/evals/test_eval_payload_analysis.py— passed.0.0.80to0.0.81.Summary by CodeRabbit
New Features
Improvements
Removed