Conversation
The payload analysis skill's Step 6.3 previously only checked revert candidates (score >= 85) for existing revert PRs. This missed a critical scenario: when a breaking PR has already been reverted and the revert is merged, but hasn't been included in the current payload yet. Changes: - Expand Step 6.3 to check ALL scored candidate PRs for existing reverts - Add logic to verify whether a merged revert appears in the current payload's PR list (summary.json → payloads[].prs[]) - Add new "merged_not_in_payload" status and revert_in_payload field to the payload-results-yaml schema - Add "Merged Reverts Pending Inclusion" report section (Step 7.4b) - Add revert-pending line to executive summary - Update Read Candidates operation to exclude merged_not_in_payload - Bump CI plugin version to 0.0.55 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[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
Contributor
WalkthroughUpdates the ci plugin version metadata from 0.0.54 to 0.0.55 and revises payload-analysis and payload-results-yaml documentation to describe merged reverts not yet included in the current payload. ChangesCI plugin version sync
Payload revert reporting docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ci/skills/payload-results-yaml/SKILL.md (1)
70-75: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOmit
revert_in_payloadfrom the staged example.The field is documented as only being set for merged revert PRs, but this example shows it on an open/staged action. That makes the contract look valid on states that should not carry the field.
Suggested fix
- type: "revert" status: "staged" revert_pr_url: "https://github.com/openshift/cno/pull/2038" revert_pr_state: "open" - revert_in_payload: false result_summary: "Revert PR opened and payload jobs triggered"🤖 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-results-yaml/SKILL.md` around lines 70 - 75, The staged revert example in SKILL.md incorrectly includes revert_in_payload, which should only appear for merged revert PRs. Update the example under the payload results YAML documentation to remove that field from the staged revert entry, keeping the rest of the revert_pr_url, revert_pr_state, and result_summary example intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/payload-analysis/SKILL.md`:
- Around line 351-366: Step 6.3 now marks some candidates as status
"merged_not_in_payload", but the later revert-candidate logic still treats them
as eligible for the >= 85 revert path. Update the scoring/reporting flow in
payload-analysis SKILL.md so the revert recommendation branch explicitly
excludes candidates with status "merged_not_in_payload"; use the existing status
handling around the Step 6.3 merged-revert check and the revert-candidate
decision path to ensure these PRs are reported as pending next payload, not
recommended for a duplicate revert.
In `@plugins/ci/skills/payload-results-yaml/SKILL.md`:
- Around line 217-221: The Read Candidates flow in the payload-results YAML
skill currently filters out only candidates with pre-existing revert actions in
the open/merged/merged_not_in_payload states, but it still allows candidates
already staged for revert to be returned. Update the filtering logic in the Read
Candidates section so it also excludes any candidate that has a staged revert
action, or more generally any existing revert action that should prevent
duplicate downstream processing. Use the candidate action handling and the
existing status checks in this same YAML spec as the place to make the change.
---
Outside diff comments:
In `@plugins/ci/skills/payload-results-yaml/SKILL.md`:
- Around line 70-75: The staged revert example in SKILL.md incorrectly includes
revert_in_payload, which should only appear for merged revert PRs. Update the
example under the payload results YAML documentation to remove that field from
the staged revert entry, keeping the rest of the revert_pr_url, revert_pr_state,
and result_summary example intact.
🪄 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: 8b3642a6-7fa5-4a64-a76f-c3cae1b6cdaf
📒 Files selected for processing (5)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/payload-analysis/SKILL.mdplugins/ci/skills/payload-results-yaml/SKILL.md
Comment on lines
+351
to
+366
| #### 6.3: Check if Candidate PRs Were Already Reverted | ||
|
|
||
| For each revert candidate: | ||
| For **every scored candidate PR** (not just revert candidates with score >= 85), check whether a revert PR already exists: | ||
|
|
||
| ```bash | ||
| gh pr list --repo <org>/<repo> --search "revert <pr_number>" --json number,title,url,state,mergedAt --limit 5 | ||
| ``` | ||
|
|
||
| If a revert PR is found: | ||
| - **Merged**: Note when it merged relative to the payload. If after the payload was cut, the fix is expected in the next payload. Do not recommend reverting again. | ||
| - **Open**: Mention the existing revert PR and link to it. | ||
| - **Open**: Record the existing revert PR URL and link to it. For revert candidates (score >= 85), do not open a duplicate revert. | ||
| - **Closed (not merged)**: Ignore. | ||
| - **Merged**: Determine whether the merged revert has been **included in the current payload**. Check the target payload's PR list in `summary.json` → `payloads[0].prs[]` for a matching PR number and repository. Also check subsequent payloads in the chain (`payloads[1]`, `payloads[2]`, etc.) in case the revert landed in a later payload that was also rejected. | ||
| - **Revert in payload** (`revert_in_payload: true`): The revert is already included in this payload. The failure must have a different root cause, or the revert was insufficient. Note this and continue scoring — the revert did not fix the problem. | ||
| - **Revert NOT in payload** (`revert_in_payload: false`): The revert was merged after the payload was cut (or has not yet been picked up by the build system). Record this as `status: "merged_not_in_payload"`. Do **not** recommend opening a new revert for this candidate. Instead, surface this prominently in the report: the fix exists and is expected in the next payload build. | ||
|
|
||
| This check is critical for avoiding redundant work: when a revert has already been merged but simply hasn't been included in the current payload yet, the analysis should clearly communicate that the fix is pending rather than recommending a duplicate revert. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude merged_not_in_payload from the revert-recommendation path.
Step 6.3 records the new status, but this block still leaves the candidate eligible for the >= 85 revert-candidate path. Without an explicit exclusion later, the report can tell operators both to avoid opening a duplicate revert and to recommend a revert for the same PR.
Suggested fix
-For each candidate PR with a rubric score of **>= 85**, mark it as a **revert candidate**.
+For each candidate PR with a rubric score of **>= 85**, mark it as a **revert candidate**, unless Step 6.3 recorded `status: "merged_not_in_payload"` for that PR.🧰 Tools
🪛 LanguageTool
[style] ~363-~363: Consider using a different verb for a more formal wording.
Context: ...d continue scoring — the revert did not fix the problem. - **Revert NOT in payloa...
(FIX_RESOLVE)
🪛 SkillSpector (2.2.3)
[error] 403: [P2] Hidden Instructions: Hidden instructions were detected in comments or invisible text. These could contain malicious directives. Manual review is recommended.
Remediation: Audit all comments and invisible characters. Remove any instructions that direct the agent to perform unauthorized actions. Use plain, reviewable content.
(Prompt Injection (P2))
[error] 482: [P2] Hidden Instructions: Hidden instructions were detected in comments or invisible text. These could contain malicious directives. Manual review is recommended.
Remediation: Audit all comments and invisible characters. Remove any instructions that direct the agent to perform unauthorized actions. Use plain, reviewable content.
(Prompt Injection (P2))
[warning] 68: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 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 351 - 366, Step 6.3
now marks some candidates as status "merged_not_in_payload", but the later
revert-candidate logic still treats them as eligible for the >= 85 revert path.
Update the scoring/reporting flow in payload-analysis SKILL.md so the revert
recommendation branch explicitly excludes candidates with status
"merged_not_in_payload"; use the existing status handling around the Step 6.3
merged-revert check and the revert-candidate decision path to ensure these PRs
are reported as pending next payload, not recommended for a duplicate revert.
Comment on lines
+217
to
+221
| Write a new `payload-results-{tag}.yaml` with `metadata`, `failing_jobs`, `candidates`, and optionally `rhcos_suspects` populated. All failed blocking jobs are recorded in `failing_jobs`. Candidates with no pre-existing revert start with `actions: []`. If a pre-existing revert PR is discovered during analysis, append an action with `type: "revert"` and the appropriate status: `"open"` (revert PR is open), `"merged"` (revert PR merged and included in the current payload), or `"merged_not_in_payload"` (revert PR merged but not yet included in the current payload). When the status is `"merged"` or `"merged_not_in_payload"`, set `revert_in_payload` to `true` or `false` respectively. If RHCOS RPM suspects were identified, include them in `rhcos_suspects[]`. | ||
|
|
||
| ### Read Candidates (used by `payload-revert`, `payload-experiment`) | ||
|
|
||
| Read the file. Filter candidates by `confidence_score` range. Exclude candidates that already have an action with `status` of `"open"` or `"merged"` (pre-existing revert). Return matching candidates. Use the top-level `failing_jobs[]` to look up full job details for each candidate's `failing_jobs` references. | ||
| Read the file. Filter candidates by `confidence_score` range. Exclude candidates that already have an action with `status` of `"open"`, `"merged"`, or `"merged_not_in_payload"` (pre-existing revert). Return matching candidates. Use the top-level `failing_jobs[]` to look up full job details for each candidate's `failing_jobs` references. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude staged revert actions here as well.
Read Candidates can still return a candidate that has already been staged for revert, which makes it eligible for duplicate processing by downstream revert/experiment commands. Add staged to the exclusion set, or filter more broadly by any existing revert action.
Suggested fix
-Exclude candidates that already have an action with status of "open", "merged", or "merged_not_in_payload" (pre-existing revert).
+Exclude candidates that already have an action with status of "open", "merged", "merged_not_in_payload", or "staged" (any revert already in flight or completed).📝 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
| Write a new `payload-results-{tag}.yaml` with `metadata`, `failing_jobs`, `candidates`, and optionally `rhcos_suspects` populated. All failed blocking jobs are recorded in `failing_jobs`. Candidates with no pre-existing revert start with `actions: []`. If a pre-existing revert PR is discovered during analysis, append an action with `type: "revert"` and the appropriate status: `"open"` (revert PR is open), `"merged"` (revert PR merged and included in the current payload), or `"merged_not_in_payload"` (revert PR merged but not yet included in the current payload). When the status is `"merged"` or `"merged_not_in_payload"`, set `revert_in_payload` to `true` or `false` respectively. If RHCOS RPM suspects were identified, include them in `rhcos_suspects[]`. | |
| ### Read Candidates (used by `payload-revert`, `payload-experiment`) | |
| Read the file. Filter candidates by `confidence_score` range. Exclude candidates that already have an action with `status` of `"open"` or `"merged"` (pre-existing revert). Return matching candidates. Use the top-level `failing_jobs[]` to look up full job details for each candidate's `failing_jobs` references. | |
| Read the file. Filter candidates by `confidence_score` range. Exclude candidates that already have an action with `status` of `"open"`, `"merged"`, or `"merged_not_in_payload"` (pre-existing revert). Return matching candidates. Use the top-level `failing_jobs[]` to look up full job details for each candidate's `failing_jobs` references. | |
| Write a new `payload-results-{tag}.yaml` with `metadata`, `failing_jobs`, `candidates`, and optionally `rhcos_suspects` populated. All failed blocking jobs are recorded in `failing_jobs`. Candidates with no pre-existing revert start with `actions: []`. If a pre-existing revert PR is discovered during analysis, append an action with `type: "revert"` and the appropriate status: `"open"` (revert PR is open), `"merged"` (revert PR merged and included in the current payload), or `"merged_not_in_payload"` (revert PR merged but not yet included in the current payload). When the status is `"merged"` or `"merged_not_in_payload"`, set `revert_in_payload` to `true` or `false` respectively. If RHCOS RPM suspects were identified, include them in `rhcos_suspects[]`. | |
| ### Read Candidates (used by `payload-revert`, `payload-experiment`) | |
| Read the file. Filter candidates by `confidence_score` range. Exclude candidates that already have an action with status of `"open"`, `"merged"`, `"merged_not_in_payload"`, or `"staged"` (any revert already in flight or completed). Return matching candidates. Use the top-level `failing_jobs[]` to look up full job details for each candidate's `failing_jobs` references. |
🤖 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-results-yaml/SKILL.md` around lines 217 - 221, The
Read Candidates flow in the payload-results YAML skill currently filters out
only candidates with pre-existing revert actions in the
open/merged/merged_not_in_payload states, but it still allows candidates already
staged for revert to be returned. Update the filtering logic in the Read
Candidates section so it also excludes any candidate that has a staged revert
action, or more generally any existing revert action that should prevent
duplicate downstream processing. Use the candidate action handling and the
existing status checks in this same YAML spec as the place to make the change.
Contributor
|
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. |
Member
Author
|
Closing this — the payload-related plugins have been massively refactored since this was opened, so I don't think this change is needed anymore. |
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.
The payload analysis skill's Step 6.3 previously only checked revert candidates (score >= 85) for existing revert PRs. This missed a critical scenario: when a breaking PR has already been reverted and the revert is merged, but hasn't been included in the current payload yet.
Changes:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Documentation
Chores
ciplugin version to0.0.55.