Add analyze-payload command and improve CI skills - #341
Merged
openshift-merge-bot[bot] merged 7 commits intoFeb 27, 2026
Merged
Conversation
Sippy only ingests payloads after they complete, so in-progress or very recent payloads are unavailable. The script now tries Sippy first and automatically falls back to the release controller API, which has the data immediately but with shorter retention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aggregated jobs run the same test in parallel and do statistical analysis, which produces different failure modes than normal jobs. Document how to detect aggregation, the three failure modes (statistically significant failure, insufficient completed runs, non-deterministic test presence), and how to find underlying job run URLs via junit-aggregated.xml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When the payload analysis recommends a revert, the HTML report now includes a ready-to-paste block with the PR, affected jobs, evidence summary, and /ci:revert-pr command. Includes a copy button for convenience. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
approved
Contributor
WalkthroughAdds an interactive, copyable revert UI in payload analysis, implements resilient dual-source PR fetching (Sippy first, release-controller fallback) with new helpers, and inserts aggregated-job detection and analysis guidance into prow-job test-failure skill. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Fetch as fetch_new_prs()
participant Sippy as Sippy API
participant RC as Release Controller API
Client->>Fetch: fetch_new_prs(payload_tag)
Fetch->>Sippy: fetch_from_sippy(payload_tag)
alt Sippy returns PRs
Sippy-->>Fetch: PR list
Fetch-->>Client: PR list
else Sippy missing/failure
Fetch->>RC: fetch_from_release_controller(payload_tag)
RC-->>Fetch: PR list (from changelog)
Fetch-->>Client: PR list (with fallback warning)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 (2)
plugins/ci/skills/analyze-payload/SKILL.md (2)
273-273: Document clipboard API requirements.The
navigator.clipboard.writeTextAPI requires a secure context (HTTPS or localhost) and is not supported in older browsers (IE11, older Safari versions). Consider adding a note in the documentation that the HTML report requires a modern browser and secure context for the Copy functionality to work, or provide fallback guidance (e.g., "If the Copy button doesn't work, manually select and copy the text").📋 Suggested documentation note
After line 283 or in the "Notes" section (line 476), add:
**Note**: The Copy button uses the Clipboard API, which requires: - A modern browser (Chrome 63+, Firefox 53+, Safari 13.1+, Edge 79+) - A secure context (HTTPS or localhost) If the Copy button does not work, users can manually select and copy the text from the code block.Also applies to: 302-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/analyze-payload/SKILL.md` at line 273, The documentation lacks a note that the Copy button's onclick handler uses navigator.clipboard.writeText which requires a secure context and modern browser support; update SKILL.md to document these Clipboard API requirements and add fallback guidance: state that the Copy button needs HTTPS or localhost and modern browsers (give brief version constraints or "modern browser"), and add an instruction like "If the Copy button doesn't work, manually select and copy the text" near the existing Copy button examples (the button element with onclick="navigator.clipboard.writeText(...)") and in the Notes section so users know why copying may fail and how to proceed.
285-287: Clarify CSS placement for implementers.The instruction "Add the following styles for the revert prompt block" doesn't specify where in the existing
<style>section (described in Section 7.5) these rules should be placed. While CSS order typically doesn't matter for these selectors, explicit guidance would help implementers.📝 Suggested clarification
-Add the following styles for the revert prompt block: +Add the following styles to the `<style>` section (described in Section 7.5, before the closing `</style>` tag):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/analyze-payload/SKILL.md` around lines 285 - 287, The doc asks to "Add the following styles for the revert prompt block" but doesn't say where to place them; update SKILL.md to explicitly instruct implementers to insert those CSS rules into the existing <style> block referenced in Section 7.5 (preferably alongside other prompt/block styles or appended to the end of that <style> section) so the rules for the "revert prompt block" are colocated with related prompt styles; mention the specific target (the <style> section described in Section 7.5 and the "revert prompt block" selectors) so implementers know exactly where to put the new rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ci/skills/analyze-payload/SKILL.md`:
- Around line 269-283: Update the revert UI block to add an accessible label on
the copy button and clarify the repeated-heading structure: add aria-label="Copy
revert command" (or aria-label="Copy revert command for PR #{pr_number}" when
iterating) to the <button> in the .revert-prompt block so screen readers
announce the action, and ensure only one <h3>Revert Instructions</h3> heading
appears before multiple .revert-prompt blocks (i.e., render one top-level <h3>
and then repeat the .revert-prompt <div> for each revert candidate) rather than
repeating the heading for every candidate; update the template variables
(pr_url, originating_payload_tag, job_name_*) used inside each .revert-prompt
accordingly so each button’s aria-label can reference the specific PR when
available.
In `@plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py`:
- Around line 140-143: The fallback print message in fetch_new_prs_in_payload.py
is too specific ("Sippy does not have payload '{payload_tag}'") although
fetch_from_sippy can fall back for connectivity and unexpected-response cases;
update the print in the fallback path (the print call that references
payload_tag) to use a generic warning such as "Falling back to release
controller for payload '{payload_tag}'" and include context (e.g., the error or
status) when available from fetch_from_sippy so the message correctly covers all
fallback reasons and points to fetch_from_sippy and the payload_tag variable for
where to change it.
- Around line 88-95: The call to _find_previous_tag in fetch_new_prs_in_payload
can raise network/HTTP exceptions (the I/O happens inside _find_previous_tag)
and currently runs outside the controlled exit path; wrap the call to
_find_previous_tag(stream, payload_tag) in a try/except that catches
network-related exceptions (e.g., requests.exceptions.RequestException or a
broad Exception if requests isn't directly imported) and on exception print the
same stderr message and call sys.exit(1) (the same behavior used when prev_tag
is falsy), so both lookup failures and network errors follow the identical
controlled error path.
- Around line 121-123: The fallback currently derives bug_url from only
commit.get("issues", {}) which misses release-controller payloads that use
"bugs"; update the logic in fetch_new_prs_in_payload.py where issues and bug_url
are computed (around the commit handling block) to merge both
commit.get("issues", {}) and commit.get("bugs", {}) (or prefer one over the
other) before calling next(iter(...)) so bug links from either field are
captured when building prs in the prs.append block.
In `@plugins/ci/skills/fetch-new-prs-in-payload/SKILL.md`:
- Line 140: Update the documentation sentence that claims the fallback is
“transparent” to accurately reflect runtime behavior: change the phrasing near
the existing line that contains the word “transparent” to state that a fallback
warning is emitted to stderr when the fallback is used (i.e., it’s automatic but
not silent); ensure the new wording mentions “fallback warning to stderr” so it
matches the actual stderr behavior described in the code.
In `@plugins/ci/skills/prow-job-analyze-test-failure/SKILL.md`:
- Around line 120-122: The fenced code block containing the GCS path string (the
block showing gs://test-platform-results/{bucket-path}/.../junit-aggregated.xml)
is missing a language tag; update that fenced block to include a language
identifier (e.g., add "text" after the opening ```), so the block becomes
```text ... ``` to satisfy Markdown best practices and render correctly in
SKILL.md.
---
Nitpick comments:
In `@plugins/ci/skills/analyze-payload/SKILL.md`:
- Line 273: The documentation lacks a note that the Copy button's onclick
handler uses navigator.clipboard.writeText which requires a secure context and
modern browser support; update SKILL.md to document these Clipboard API
requirements and add fallback guidance: state that the Copy button needs HTTPS
or localhost and modern browsers (give brief version constraints or "modern
browser"), and add an instruction like "If the Copy button doesn't work,
manually select and copy the text" near the existing Copy button examples (the
button element with onclick="navigator.clipboard.writeText(...)") and in the
Notes section so users know why copying may fail and how to proceed.
- Around line 285-287: The doc asks to "Add the following styles for the revert
prompt block" but doesn't say where to place them; update SKILL.md to explicitly
instruct implementers to insert those CSS rules into the existing <style> block
referenced in Section 7.5 (preferably alongside other prompt/block styles or
appended to the end of that <style> section) so the rules for the "revert prompt
block" are colocated with related prompt styles; mention the specific target
(the <style> section described in Section 7.5 and the "revert prompt block"
selectors) so implementers know exactly where to put the new rules.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
plugins/ci/skills/analyze-payload/SKILL.mdplugins/ci/skills/fetch-new-prs-in-payload/SKILL.mdplugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.pyplugins/ci/skills/prow-job-analyze-test-failure/SKILL.md
neisw
reviewed
Feb 27, 2026
|
|
||
| def fetch_from_sippy(payload_tag: str) -> Optional[list]: | ||
| """Try fetching PR diff from Sippy. Returns None if the payload is not available.""" | ||
| url = f"{SIPPY_API_BASE}/payloads/diff?toPayload={urllib.parse.quote(payload_tag)}" |
There was a problem hiding this comment.
Is this correct? I think it needs fromPayload as well, no?
There was a problem hiding this comment.
It looks like https://sippy.dptools.openshift.org/api/payloads/diff?toPayload=4.22.0-0.nightly-2026-01-06-110407 returns the same results
Member
Author
There was a problem hiding this comment.
Yea I guess so, that's existing code here. I only added fallback to the release controller, the diff is indentation changes
Instruct Claude to use target="_blank" on all anchor tags in the generated payload analysis HTML report. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wrap the call to _find_previous_tag in a try/except for urllib HTTP/URL errors so network failures follow the same controlled exit path as a missing previous tag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py (2)
109-116: Consider catchingjson.JSONDecodeErrorhere as well.Similar to the Sippy path, if the release controller returns malformed JSON, this would crash with an unhandled exception rather than providing a controlled error message.
🛡️ Proposed fix
try: data = _http_get_json(url, timeout=60) except urllib.error.HTTPError as e: print(f"Error: HTTP {e.code} from release controller: {e.reason}", file=sys.stderr) sys.exit(1) except urllib.error.URLError as e: print(f"Error: Failed to connect to release controller: {e.reason}", file=sys.stderr) sys.exit(1) + except json.JSONDecodeError as e: + print(f"Error: Invalid JSON response from release controller: {e}", file=sys.stderr) + sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py` around lines 109 - 116, Add handling for malformed JSON in the try/except around the _http_get_json call: catch json.JSONDecodeError in the same block that currently catches urllib.error.HTTPError and urllib.error.URLError, log a clear error message to stderr (similar style to the existing prints) and call sys.exit(1). Ensure the json module is available where this code runs (import json if not already) and reference the existing call/site around _http_get_json(url, timeout=60) to locate where to add the new except branch.
56-61: Consider catchingjson.JSONDecodeErrorfor fallback robustness.If Sippy returns malformed JSON,
json.loadsin_http_get_jsonraisesJSONDecodeError, which isn't caught here. This would crash instead of gracefully falling back to the release controller.🛡️ Proposed fix
try: data = _http_get_json(url) except urllib.error.HTTPError: return None except urllib.error.URLError: return None + except json.JSONDecodeError: + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py` around lines 56 - 61, The try/except around the call to _http_get_json currently only catches urllib.error.HTTPError and urllib.error.URLError; add a handler for json.JSONDecodeError as well so malformed JSON from Sippy is treated the same way (return None) instead of raising; update the except block that references _http_get_json to include json.JSONDecodeError and ensure json is imported if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py`:
- Around line 109-116: Add handling for malformed JSON in the try/except around
the _http_get_json call: catch json.JSONDecodeError in the same block that
currently catches urllib.error.HTTPError and urllib.error.URLError, log a clear
error message to stderr (similar style to the existing prints) and call
sys.exit(1). Ensure the json module is available where this code runs (import
json if not already) and reference the existing call/site around
_http_get_json(url, timeout=60) to locate where to add the new except branch.
- Around line 56-61: The try/except around the call to _http_get_json currently
only catches urllib.error.HTTPError and urllib.error.URLError; add a handler for
json.JSONDecodeError as well so malformed JSON from Sippy is treated the same
way (return None) instead of raising; update the except block that references
_http_get_json to include json.JSONDecodeError and ensure json is imported if
not already.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py
|
/lgtm |
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 |
This was referenced May 15, 2026
This was referenced May 31, 2026
This was referenced Jun 9, 2026
This was referenced Jun 17, 2026
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
2 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.
Summary
fetch-new-prs-in-payloadfallback: Falls back to the release controller API when Sippy hasn't ingested the payload yet (e.g., in-progress or very recent payloads)analyze-test-failurenow understands aggregated jobs and their three failure modes (statistically significant failure, insufficient completed runs, non-deterministic test presence)analyze-payloadrecommends a revert, the HTML report includes a ready-to-paste block with evidence summary and/ci:revert-prcommand🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores