Improvements to payload agent - #511
Merged
openshift-merge-bot[bot] merged 27 commits intoJun 3, 2026
Merged
Conversation
Adds a script that downloads all data needed to analyze an OpenShift payload into a local directory tree: release controller data, PR diffs, comments, CI jobs, JUnit test results, and regression tracking. Features: - Chains backwards through payloads to find last all-green baseline - Splits jobs into blocking/informing directories with metadata - Downloads and parses JUnit XML for failed blocking jobs - Tracks test failure regressions across the payload chain - Generates summary.json and summary.md roll-up at stream level - Falls back to git clone for large PR diffs that exceed GitHub API limits - Idempotent: re-running skips existing files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…AGENTS.md Add comprehensive data extraction so summary.json can answer 90% of payload agent questions without live API calls: - GCS browser URLs in job.json and summary for direct artifact access - BuildLogCollector: download build-log.txt from GCS (handles gzip), extract error/warning lines and last 20% of log - JobStreakTracker: per-job consecutive failure count, originating payload, and failure pattern (e.g. "F F F F F S") across the chain - Expanded summary.json: architecture, stream, version, release_url, hours_since_baseline, failed_jobs as detailed objects with streaks, build-log error counts, test failure counts, and file paths - Remove failure_text truncation in regressions (was [:500]) - Generate dynamic AGENTS.md + CLAUDE.md in snapshot output directory so any agent opening the directory gets instant orientation - Drop summary.md (JSON is more useful to LLMs than markdown tables) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update documentation to reflect new capabilities: GCS browser URLs, build-log extraction, per-job failure streaks, comprehensive summary.json schema, and dynamic AGENTS.md/CLAUDE.md generation. Remove references to dropped summary.md. Bump version to 0.0.45. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename skill directory and script to payload-snapshot for consistency. Move script into scripts/ subdirectory per convention. Update all path references in SKILL.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the analyze-payload skill and command with payload-analysis, which reads from a local payload-snapshot instead of making live API calls. This enables: - Time-travel replay: re-analyze historical payloads against frozen data - Model accessibility: less capable models skip multi-skill data gathering - Reproducibility: same snapshot always produces consistent analysis The new skill adds an adversarial review step (Step 9) that launches a dedicated subagent to check conclusions for weak correlations, infrastructure misattribution, and score inflation before finalizing the report. Update all cross-references in payload-revert, payload-experiment, payload-results-yaml, payload-autodl-json, stage-payload-reverts, and payload-snapshot. Delete old analyze-payload skill and command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…orrection Add evaluation infrastructure for the payload-analysis skill with 6 judges (4 deterministic schema checks, 2 LLM quality judges) and 1 test case based on the 5.0.0-0.nightly-2026-05-30-072431 payload. Fix Step 9 adversarial review to act as a methodology reviewer rather than a score overrider — it now validates investigation depth and skill usage without lowering rubric-based confidence scores backed by concrete evidence. This fixes a false negative where the reviewer incorrectly lowered origin#31006 from 100→70, dropping it below the 85 revert threshold. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add SippyClient and SippyPayloadChain to payload_snapshot.py for creating snapshots of historical payloads pruned from the release controller. Sippy provides job runs, changelog diffs, and tag metadata. Extend JUnit and build-log collection to include failed informing jobs (target payload only) so install-analysis-all failures can be traced through their underlying informing jobs. Add 13 prioritized eval cases covering revert detection, false positive rejection, CI config issues, streak detection, and infrastructure classification. Cases are numbered by value — case-001 tests the most complex scenario (CI config change from openshift/release + product regression). Add runner.system_prompt for date-pinning external queries. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add required_skill_invocations judge that parses JSONL transcripts to verify ci:payload-results-yaml and ci:payload-autodl-json are invoked via the Skill tool. Align runner.system_prompt with CI production prompt to enforce skill loading before analysis and mandate use of output formatting skills. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…path Remove <details> check from html_report_structure judge — not useful enough to enforce. Change snapshot extraction from /tmp to .work/ to avoid filling tmpfs during parallel eval runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
approved
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a complete OpenShift CI payload-analysis feature, including a new CLI command, skill implementation for downloading and analyzing payload data offline, a comprehensive evaluation framework with 13 test cases, and validators for structured output formats (YAML/JSON). ChangesPayload Analysis Feature Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
plugins/ci/commands/payload-revert.md (1)
39-42: 💤 Low valueConsider adding a language specifier to the fenced code block.
The error message block could specify a language (e.g.,
text) to satisfy the markdownlint rule and improve consistency.📝 Proposed fix
- ``` + ```text Error: Payload results YAML not found for {payload_tag}. Run `/ci:payload-analysis {payload_tag}` first to generate it. ```🤖 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/commands/payload-revert.md` around lines 39 - 42, Add a language specifier to the fenced code block containing the error message "Error: Payload results YAML not found for {payload_tag}." so it becomes a code fence like ```text; update the opening backticks for the block that includes the "Error: Payload results YAML not found for {payload_tag}." and the "Run `/ci:payload-analysis {payload_tag}` first to generate it." line to satisfy markdownlint and improve consistency.plugins/ci/evals/cases/payload-analysis/case-013-4.20-accepted-with-failures/annotations.yaml (1)
1-11: 💤 Low valueConsider adding
expected_candidatesfield for consistency.This annotations file omits the
expected_candidatesfield, while case-011 includes it explicitly as an empty array. Sincehas_revert_candidates: false, the expected behavior is the same, but explicit presence improves consistency across test cases and may prevent evaluation framework issues if this field is expected.📝 Suggested addition for consistency
has_revert_candidates: false force_accept_expected: false +expected_candidates: [] + 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-013-4.20-accepted-with-failures/annotations.yaml` around lines 1 - 11, Add an explicit expected_candidates field to the annotations YAML (e.g., expected_candidates: []) to mirror case-011 and ensure consistency with has_revert_candidates: false; update the top-level fields in the same document where expected_phase, expected_failed_job_count, and has_revert_candidates are declared so the evaluation framework sees an empty array for expected_candidates rather than omitting the key.plugins/ci/evals/cases/payload-analysis/case-012-4.20-rejected-streak/annotations.yaml (1)
1-12: 💤 Low valueConsider adding
expected_candidatesfield for consistency.This annotations file omits the
expected_candidatesfield, while case-011 includes it explicitly as an empty array. Sincehas_revert_candidates: false, the expected behavior is the same, but explicit presence improves consistency across test cases and may prevent evaluation framework issues if this field is expected.📝 Suggested addition for consistency
has_revert_candidates: false force_accept_expected: false +expected_candidates: [] + 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-012-4.20-rejected-streak/annotations.yaml` around lines 1 - 12, Add an explicit expected_candidates field to the annotations so the file consistently declares expected revert candidates; since has_revert_candidates is false, set expected_candidates to an empty array (expected_candidates: []). Update the top-level keys (e.g., alongside expected_phase, expected_failed_job_count, has_revert_candidates) so consumers that expect the expected_candidates key won't break and test case formatting matches case-011.
🤖 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/commands/payload-analysis.md`:
- Around line 12-14: The Markdown file plugins/ci/commands/payload-analysis.md
has fenced code blocks missing language identifiers (tripping MD040); update the
fences so the usage block uses a neutral language like "text" (for the usage
line `/ci:payload-analysis <payload-tag> [--snapshot-dir DIR]`) and the example
invocation blocks use "bash" (for the example commands such as
`/ci:payload-analysis 4.22.0-0.nightly-2026-02-25-152806`, `/ci:payload-analysis
... --snapshot-dir payload/4.22/nightly`, and the arm64 example); apply the same
change to the other affected fences around lines 76-88 so all triple-backtick
fences include the appropriate language identifiers.
In `@plugins/ci/evals/scripts/extract-payload-analysis-snapshots.sh`:
- Line 33: The tar extraction command tar xzf "$archive" -C "$dest" is
vulnerable to path traversal and link attacks; before extracting, enumerate
members (tar -tzf "$archive") and validate each entry: reject any member with an
absolute path (starts with '/'), any path containing '..' segments, and any
member that is a symlink or hardlink (detect via tar -tzvf or --list with type
flag and reject type 'l'/'L'/'H' entries); only if all members pass, proceed to
extract to "$dest" (tar xzf "$archive" -C "$dest"), otherwise abort with a clear
error.
In `@plugins/ci/skills/payload-analysis/SKILL.md`:
- Around line 137-147: The fenced code blocks in SKILL.md (notably the block
labeled "ANALYSIS_RESULT" and the other plain-output blocks referenced around
the sections showing job summaries) are missing language identifiers which trips
markdown lint; update each ``` fence to include a language token (use ```text
for plain output) so all fenced blocks like the ANALYSIS_RESULT block and the
other plain-output blocks are written as ```text ... ``` to satisfy the linter.
- Around line 1-4: The SKILL.md for payload-analysis is missing the required
4-phase ai-helpers overlap analysis; update the payload-analysis SKILL.md to
include a 4-phase overlap report (Phase 1–4) that classifies similarity as
HIGH/MODERATE/LOW against existing plugins/ci/* payload components and any
overlapping open PRs, include explicit lists of overlapping component names and
PR identifiers/links, state the methodology and metrics used for similarity
scoring, and summarize actionable conclusions (e.g., merge/reuse/avoid,
potential conflicts) so reviewers can see which components/PRs overlap and why.
In `@plugins/ci/skills/payload-snapshot/scripts/payload_snapshot.py`:
- Line 22: The JUnit XML parsing in _parse_junit_xml currently uses
xml.etree.ElementTree (imported as ET) which is vulnerable to XML bomb/entity
expansion; replace it by adding defusedxml as a dependency, change the import at
the top to use defusedxml's ElementTree (e.g., import defusedxml.ElementTree as
ET or from defusedxml import ElementTree as ET), keep using ET.parse(source)
inside _parse_junit_xml, and update the exception handling around the parse to
also catch defusedxml's security exception
(defusedxml.common.DefusedXmlException) in addition to ET.ParseError and OSError
so malicious payloads are rejected safely and logged.
In `@plugins/ci/skills/payload-snapshot/SKILL.md`:
- Around line 120-123: The jq example incorrectly reads component from each
commit's .name; update the query to take the component name from the parent
updatedImages[] object by binding the parent (e.g., using updatedImages[] as
$img) and then iterating $img.commits[] and using $img.name for the component
while keeping .pullURL and .subject for pr and subject; change the expression
that currently uses .changeLogJson.updatedImages[].commits[] and .name to
reference the parent’s name (e.g., $img.name) instead.
---
Nitpick comments:
In `@plugins/ci/commands/payload-revert.md`:
- Around line 39-42: Add a language specifier to the fenced code block
containing the error message "Error: Payload results YAML not found for
{payload_tag}." so it becomes a code fence like ```text; update the opening
backticks for the block that includes the "Error: Payload results YAML not found
for {payload_tag}." and the "Run `/ci:payload-analysis {payload_tag}` first to
generate it." line to satisfy markdownlint and improve consistency.
In
`@plugins/ci/evals/cases/payload-analysis/case-012-4.20-rejected-streak/annotations.yaml`:
- Around line 1-12: Add an explicit expected_candidates field to the annotations
so the file consistently declares expected revert candidates; since
has_revert_candidates is false, set expected_candidates to an empty array
(expected_candidates: []). Update the top-level keys (e.g., alongside
expected_phase, expected_failed_job_count, has_revert_candidates) so consumers
that expect the expected_candidates key won't break and test case formatting
matches case-011.
In
`@plugins/ci/evals/cases/payload-analysis/case-013-4.20-accepted-with-failures/annotations.yaml`:
- Around line 1-11: Add an explicit expected_candidates field to the annotations
YAML (e.g., expected_candidates: []) to mirror case-011 and ensure consistency
with has_revert_candidates: false; update the top-level fields in the same
document where expected_phase, expected_failed_job_count, and
has_revert_candidates are declared so the evaluation framework sees an empty
array for expected_candidates rather than omitting the key.
🪄 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: c23ac0a3-7ee0-4c21-8243-1a4e05eb45d3
⛔ Files ignored due to path filters (14)
plugins/ci/evals/snapshots/payload-analysis/4.18.0-0.nightly-2026-05-14-085133.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.20.0-0.nightly-2026-05-12-225204.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.20.0-0.nightly-2026-05-13-064706.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.20.0-0.nightly-2026-05-14-114051.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.22.0-0.ci-2026-03-31-050515.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.22.0-0.ci-2026-03-31-170515.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.22.0-0.nightly-2026-03-18-161724.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.22.0-0.nightly-2026-03-20-053450.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/4.22.0-0.nightly-2026-03-26-231124.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/5.0.0-0.ci-2026-04-14-085906.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/5.0.0-0.ci-2026-05-07-142711.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/5.0.0-0.ci-2026-05-14-181709.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/5.0.0-0.nightly-2026-04-27-183150.tar.gzis excluded by!**/*.gzplugins/ci/evals/snapshots/payload-analysis/5.0.0-0.nightly-2026-05-30-072431.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (42)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/commands/analyze-payload.mdplugins/ci/commands/payload-analysis.mdplugins/ci/commands/payload-experiment.mdplugins/ci/commands/payload-revert.mdplugins/ci/evals/cases/payload-analysis/case-001-4.22-nightly-ci-config-and-product-regression/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-001-4.22-nightly-ci-config-and-product-regression/input.yamlplugins/ci/evals/cases/payload-analysis/case-002-5.0-nightly-all-new-failures/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-002-5.0-nightly-all-new-failures/input.yamlplugins/ci/evals/cases/payload-analysis/case-003-5.0-ci-cno-networkpolicy-revert/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-003-5.0-ci-cno-networkpolicy-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-004-4.22-two-reverts/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-004-4.22-two-reverts/input.yamlplugins/ci/evals/cases/payload-analysis/case-005-4.22-ci-cco-revert/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-005-4.22-ci-cco-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-006-4.22-ci-hypershift-revert/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-006-4.22-ci-hypershift-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-007-4.22-cvo-revert/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-007-4.22-cvo-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-008-5.0-nightly-cmo-monitoring-revert/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-008-5.0-nightly-cmo-monitoring-revert/input.yamlplugins/ci/evals/cases/payload-analysis/case-009-5.0-ci-hypershift-builder-fp/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-009-5.0-ci-hypershift-builder-fp/input.yamlplugins/ci/evals/cases/payload-analysis/case-010-4.18-rejected-multiple-failures/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-010-4.18-rejected-multiple-failures/input.yamlplugins/ci/evals/cases/payload-analysis/case-011-5.0-ci-infra-only-no-candidates/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-011-5.0-ci-infra-only-no-candidates/input.yamlplugins/ci/evals/cases/payload-analysis/case-012-4.20-rejected-streak/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-012-4.20-rejected-streak/input.yamlplugins/ci/evals/cases/payload-analysis/case-013-4.20-accepted-with-failures/annotations.yamlplugins/ci/evals/cases/payload-analysis/case-013-4.20-accepted-with-failures/input.yamlplugins/ci/evals/eval-payload-analysis.yamlplugins/ci/evals/scripts/extract-payload-analysis-snapshots.shplugins/ci/skills/analyze-payload/SKILL.mdplugins/ci/skills/payload-analysis/SKILL.mdplugins/ci/skills/payload-autodl-json/SKILL.mdplugins/ci/skills/payload-results-yaml/SKILL.mdplugins/ci/skills/payload-snapshot/SKILL.mdplugins/ci/skills/payload-snapshot/scripts/payload_snapshot.pyplugins/ci/skills/stage-payload-reverts/SKILL.md
💤 Files with no reviewable changes (2)
- plugins/ci/commands/analyze-payload.md
- plugins/ci/skills/analyze-payload/SKILL.md
Comment on lines
+137
to
+147
| ``` | ||
| ANALYSIS_RESULT: | ||
| - failure_type: install|test|upgrade|infra | ||
| - root_cause_summary: <one-line summary> | ||
| - affected_components: <comma-separated list of affected operators/components> | ||
| - key_error_patterns: <comma-separated key error strings for matching> | ||
| - known_symptoms: <comma-separated symptom summaries from job_labels, or "none"> | ||
| - underlying_job_name: <for aggregated jobs only, extracted from junit artifacts> | ||
| - retries_consistent: yes|no|no_retries|only_final_examined | ||
| - retry_summary: <brief comparison of failure modes across attempts, e.g. "all 3 attempts failed with same KAS crashloop" or "attempt 1 infra timeout, attempts 2-3 test failure", or "no retries" when there was only a single attempt> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Specify languages for fenced blocks to satisfy markdown lint.
Several fences are missing language identifiers (text is fine for plain output blocks).
Also applies to: 164-166, 471-488
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 137 - 147, The
fenced code blocks in SKILL.md (notably the block labeled "ANALYSIS_RESULT" and
the other plain-output blocks referenced around the sections showing job
summaries) are missing language identifiers which trips markdown lint; update
each ``` fence to include a language token (use ```text for plain output) so all
fenced blocks like the ANALYSIS_RESULT block and the other plain-output blocks
are written as ```text ... ``` to satisfy the linter.
Comment on lines
+120
to
+123
| **List PRs in a payload:** | ||
| ```bash | ||
| jq '.changeLogJson.updatedImages[].commits[] | {component: .name, pr: .pullURL, subject: .subject}' payload/<version>/<stream>/<tag>/changelog.json | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix the jq example: component is read from the wrong level.
The current query reads .name from each commit object, but component name is on the parent updatedImages[] object.
💡 Suggested patch
-jq '.changeLogJson.updatedImages[].commits[] | {component: .name, pr: .pullURL, subject: .subject}' payload/<version>/<stream>/<tag>/changelog.json
+jq '.changeLogJson.updatedImages[] | .name as $component | .commits[] | {component: $component, pr: .pullURL, subject: .subject}' payload/<version>/<stream>/<tag>/changelog.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **List PRs in a payload:** | |
| ```bash | |
| jq '.changeLogJson.updatedImages[].commits[] | {component: .name, pr: .pullURL, subject: .subject}' payload/<version>/<stream>/<tag>/changelog.json | |
| ``` | |
| **List PRs in a payload:** |
🤖 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 120 - 123, The jq
example incorrectly reads component from each commit's .name; update the query
to take the component name from the parent updatedImages[] object by binding the
parent (e.g., using updatedImages[] as $img) and then iterating $img.commits[]
and using $img.name for the component while keeping .pullURL and .subject for pr
and subject; change the expression that currently uses
.changeLogJson.updatedImages[].commits[] and .name to reference the parent’s
name (e.g., $img.name) instead.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-autodl-json/scripts/validate.py`:
- Around line 42-50: The loop over rows is both prematurely breaking (the "if i
> 0: break" stops after the first row) and assumes each row is a dict (using
row.items()), which can crash on malformed input; remove the premature break so
all rows are validated, and before accessing row.items() or checking fields
against REQUIRED_ROW_FIELDS ensure row is a dict (or mapping) — if not, append
an error like "rows[i] is not an object" and continue; only then check missing
fields and non-string values (use the existing REQUIRED_ROW_FIELDS, rows
variable and the enumerate loop) so every row is safely validated without
exceptions.
In `@plugins/ci/skills/payload-results-yaml/scripts/validate.py`:
- Around line 39-43: The loops over data["failing_jobs"] and data["candidates"]
assume each item is a mapping and will crash on non-dict entries; update the
checks in the failing_jobs loop (iterate variable job in validate.py) and the
candidates loop to first verify isinstance(item, dict) and if not append a clear
error like "failing_jobs[i] is not an object" or "candidates[j] is not an
object" then continue, otherwise perform the existing REQUIRED_JOB_FIELDS /
REQUIRED_CANDIDATE_FIELDS membership checks as before.
- Around line 29-33: The validation currently assumes metadata is a dict by
doing meta = data.get("metadata", {}) and then iterating REQUIRED_METADATA,
which allows non-object metadata (e.g., string or list) to pass or produce
confusing messages; update the logic where meta is read (the variable meta
derived from data.get("metadata", {})) to first check that metadata exists and
is a mapping/dict type and, if not, append a clear error like "metadata must be
an object" to errors and skip the REQUIRED_METADATA loop; keep using the same
symbols (data, meta, REQUIRED_METADATA, errors) so callers and tests still find
the check location.
🪄 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: 3d22d286-5c01-48fc-80ac-2112256ece19
📒 Files selected for processing (2)
plugins/ci/skills/payload-autodl-json/scripts/validate.pyplugins/ci/skills/payload-results-yaml/scripts/validate.py
Address CodeRabbit feedback: validate metadata type before checking keys, guard list entries against non-dict values, validate all rows instead of breaking after the first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
stbenjam
commented
Jun 1, 2026
Member
Author
There was a problem hiding this comment.
These are mostly small files, but some are a few megabytes. I am not sure if it makes sense to keep them there, or publish them somewhere else. I was thinking FROM scratch OCI containers. Pre-hooks would help a lot here opendatahub-io/agent-eval-harness#88
13 tests covering valid schemas, missing fields, wrong types, flat schemas (the hallucinated format), non-string JSON values, empty rows, bad syntax, and missing files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-autodl-json/scripts/test_validate.py`:
- Around line 36-38: The temporary file created with
tempfile.NamedTemporaryFile(..., delete=False) in test_validate.py is never
removed; update the test to ensure the file is cleaned up after use by removing
the file (os.unlink or pathlib.Path.unlink) once the test case is registered or
in test teardown, or change to delete=True and capture the name before closing
if necessary; locate the NamedTemporaryFile usage and add explicit cleanup
(e.g., try/finally or fixture teardown) to avoid leaving orphaned files.
In `@plugins/ci/skills/payload-results-yaml/scripts/test_validate.py`:
- Around line 38-40: The temp file created with tempfile.NamedTemporaryFile(...,
delete=False) in the test is never removed; update the test in test_validate.py
so the temporary YAML is deleted after use (either use delete=True or explicitly
unlink the file), e.g. create the temp file, append the case to cases, then
ensure os.unlink(f.name) in a finally block or use pytest's tmp_path fixture to
manage cleanup; refer to the NamedTemporaryFile call and the variable f and the
cases list when making the change.
🪄 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: d4b914a7-1661-443f-b653-954dc7c7ad98
📒 Files selected for processing (11)
plugins/ci/skills/payload-autodl-json/scripts/test_validate.pyplugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_empty_rows.jsonplugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_no_table_name.jsonplugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_non_string_values.jsonplugins/ci/skills/payload-autodl-json/scripts/testdata/valid.jsonplugins/ci/skills/payload-results-yaml/scripts/test_validate.pyplugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_flat_schema.yamlplugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_metadata_string.yamlplugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_missing_job_fields.yamlplugins/ci/skills/payload-results-yaml/scripts/testdata/valid.yamlplugins/ci/skills/payload-results-yaml/scripts/testdata/valid_no_candidates.yaml
✅ Files skipped from review due to trivial changes (8)
- plugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_empty_rows.json
- plugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_metadata_string.yaml
- plugins/ci/skills/payload-autodl-json/scripts/testdata/valid.json
- plugins/ci/skills/payload-results-yaml/scripts/testdata/valid.yaml
- plugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_non_string_values.json
- plugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_flat_schema.yaml
- plugins/ci/skills/payload-autodl-json/scripts/testdata/invalid_no_table_name.json
- plugins/ci/skills/payload-results-yaml/scripts/testdata/invalid_missing_job_fields.yaml
# Conflicts: # docs/index.html
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix markdown fence languages, jq component query, and XML parser trust annotation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 179eb84.
Keep analyze-payload in place alongside payload-analysis until the release repo migration is validated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous docs were generated with node_modules present, which included dotenv, dotenvx, and playwright skills as standalone_skills. CI runs on a clean checkout without node_modules and sees the mismatch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Packages like dotenv and playwright-core ship SKILL.md files that skillsaw picks up as standalone skills, causing docs drift between local (with node_modules) and CI (clean checkout). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace 32MB of tarballs with a shallow clone from stbenjam/historical-payload-data. The extraction script now fetches snapshot data on demand instead of bundling it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/test eval-payload-analysis |
2 similar comments
Member
Author
|
/test eval-payload-analysis |
Member
Author
|
/test eval-payload-analysis |
Member
Author
|
build13 taken out of rotation finally openshift/release#79962 /test eval-payload-analysis |
# Conflicts: # .claude-plugin/marketplace.json # docs/index.html # plugins/ci/.claude-plugin/plugin.json
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/test eval-payload-analysis-minimal |
2 similar comments
Member
Author
|
/test eval-payload-analysis-minimal |
Member
Author
|
/test eval-payload-analysis-minimal |
Member
Author
|
/test eval-payload-analysis |
The adversarial review was causing the agent to second-guess correct rubric scores — talking itself down from 92 to 60 with speculative counter-arguments like "if this were the sole cause, ALL jobs would fail." Three fixes: - Step 6.1: enforce mechanical rubric application, no speculative score adjustments - Step 9: rename to completeness review, refocus on catching lazy agents (missing skills, shallow analysis) not challenging correct conclusions - Step 4: require affirmative evidence for infrastructure classification, default to product regression Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/test eval-payload-analysis |
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
neisw
reviewed
Jun 2, 2026
|
|
||
| set -euo pipefail | ||
|
|
||
| REPO_URL="https://github.com/stbenjam/historical-payload-data.git" |
There was a problem hiding this comment.
Presuming this is intentional / temporary but double checking the use of personal repo. No objections just confirming.
Member
Author
There was a problem hiding this comment.
Yes, transitional. I'll request a repo from cyborg
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, 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 |
Member
Author
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces agent-eval-harness for systematic evaluation of the Payload Agent with reproducible test cases and multi-judge scoring. Restructures the analyze-payload skill to separate deterministic data collection (snapshot creation) from AI-driven analysis.
I will remove the "payload-anaylze" command once this merges and I validate the
openshift/releaseside.Key changes:
JUnit, logs, PRs, Sippy regressions) into hermetic archives before
analysis, eliminating non-deterministic data fetching during agent runs (primary driver of cost reduction)
regressions, false positives, and rejection streaks with annotated
expected outcomes
required file generation) + LLM judges (analysis quality, revert scoring)
Results from 117 evaluation runs:
ad-hoc hallucination of output formats)
Summary by CodeRabbit
New Features
ci:payload-analysiscommand for comprehensive payload failure analysispayload-snapshotskill for downloading and analyzing OpenShift payload data offlineDocumentation
Chores