Skip to content

[wip] Fix payload analysis to detect merged reverts not yet in payload #580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stbenjam wants to merge 1 commit into from
Closed

[wip] Fix payload analysis to detect merged reverts not yet in payload #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude-plugin/marketplace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
"name": "ci",
"source": "./plugins/ci",
"description": "A plugin to work with OpenShift CI and analyze Prow job results",
"version": "0.0.54",
"version": "0.0.55",
"category": "ci",
"keywords": [
"prow",
Expand Down
2 changes: 1 addition & 1 deletion docs/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ <h1>ai-helpers</h1>
{
"name": "ci",
"description": "Tools for working with OpenShift CI and analyzing Prow job results",
"version": "0.0.54",
"version": "0.0.55",
"has_readme": true,
"commands": [
{
Expand Down
2 changes: 1 addition & 1 deletion plugins/ci/.claude-plugin/plugin.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "ci",
"description": "Tools for working with OpenShift CI and analyzing Prow job results",
"version": "0.0.54",
"version": "0.0.55",
"author": {
"name": "github.com/openshift-eng"
}
Expand Down
44 changes: 40 additions & 4 deletions plugins/ci/skills/payload-analysis/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,18 +348,22 @@ For each revert candidate, record: PR URL, description, component, confidence sc

**Do NOT propose reverts for**: Infrastructure failures, flaky tests that also fail on accepted payloads, jobs where analysis is inconclusive.

#### 6.3: Check if Revert Candidates Were Already Reverted
#### 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.
Comment on lines +351 to +366

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


#### 6.4: Determine Force-Accept Recommendation

Expand Down Expand Up @@ -396,9 +400,13 @@ The report must include the following sections:
<p>{total_blocking} blocking jobs: {succeeded} passed, {failed} failed</p>
<p>{new_failures} new failure(s), {persistent_failures} persistent failure(s)</p>
<p>Chain: {chain_length} payloads, {hours_since_baseline}h since baseline</p>
<!-- Include when merged reverts are pending inclusion in the next payload -->
<p class="revert-pending">{N} merged revert(s) pending inclusion in the next payload</p>
</div>
```

The `revert-pending` line should only appear when one or more candidates have `status: "merged_not_in_payload"` from Step 6.3. This is the most important signal for operators monitoring payload health — it tells them the fix exists and a new payload build should resolve the issue.

#### 7.2: Blocking Jobs Summary Table

A table showing ALL blocking jobs with columns:
Expand Down Expand Up @@ -519,6 +527,34 @@ If no revert candidates:
</div>
```

#### 7.4b: Merged Reverts Pending Inclusion

Include this section immediately after the Recommended Reverts section when any candidate has `status: "merged_not_in_payload"` from Step 6.3. This section appears regardless of whether there are also new revert recommendations — a payload can have both new revert candidates and already-merged reverts awaiting inclusion.

```html
<div class="verdict verdict-pending-revert">
<h2>Merged Reverts Pending Inclusion</h2>
<p>The following revert PRs have been merged but are <strong>not yet included</strong> in this payload.
The next payload build is expected to include these fixes.</p>
<table>
<tr><th>Revert PR</th><th>Reverts</th><th>Component</th><th>Merged At</th><th>Affected Jobs</th></tr>
<tr>
<td><a href="{revert_pr_url}" target="_blank">#{revert_pr_number}</a></td>
<td><a href="{original_pr_url}" target="_blank">#{original_pr_number}</a></td>
<td>{component}</td>
<td>{merged_at}</td>
<td>{comma-separated failing job names}</td>
</tr>
</table>
</div>
```

Add this CSS for the pending revert styling:
```css
.verdict-pending-revert { background: rgba(88,166,255,0.1); border-left: 4px solid var(--blue); padding: 0.75rem 1rem; border-radius: 0 0.3rem 0.3rem 0; margin: 0.75rem 0; }
.revert-pending { color: var(--blue); font-weight: 600; }
```

#### 7.5: Force-Accept Recommendation

If recommended (Step 6.4):
Expand Down
24 changes: 21 additions & 3 deletions plugins/ci/skills/payload-results-yaml/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,29 @@ candidates:
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"
jira_key: "TRT-1234"
jira_url: "https://redhat.atlassian.net/browse/TRT-1234"
payload_jobs:
- command: "/payload-job periodic-ci-...-e2e-aws-ovn"
test_url: "https://pr-payload-tests.ci.openshift.org/runs/ci/..."
test_prow_url: "https://prow.ci.openshift.org/view/gs/..."
- pr_url: "https://github.com/openshift/mco/pull/4100"
pr_number: 4100
component: "machine-config-operator"
title: "Update MCO rollout strategy"
confidence_score: 90
rationale: "new failure mode + sole modifier of affected component"
failing_jobs:
- "periodic-ci-...-e2e-aws-ovn-upgrade"
actions:
- type: "revert"
status: "merged_not_in_payload"
revert_pr_url: "https://github.com/openshift/mco/pull/4105"
revert_pr_state: "merged"
revert_in_payload: false
result_summary: "Revert PR #4105 merged 2h after payload was cut — fix pending in next payload"

rhcos_suspects:
- rhcos_tag: "rhel-coreos-10"
Expand Down Expand Up @@ -147,6 +163,7 @@ Actions taken on a candidate. New entries are **appended** by downstream skills.
| `status` | string | See status values below |
| `revert_pr_url` | string | URL of the revert PR (draft or real) |
| `revert_pr_state` | string | `"draft"`, `"open"`, `"merged"`, `"closed"` |
| `revert_in_payload` | bool | `true` if the revert PR is included in the current payload's PR list, `false` if merged but not yet picked up. Only set when `revert_pr_state` is `"merged"`. |
| `result_summary` | string | Brief description of the outcome |
| `jira_key` | string | TRT JIRA key (e.g., `"TRT-1234"`), or `""` |
| `jira_url` | string | TRT JIRA URL, or `""` |
Expand All @@ -157,7 +174,8 @@ Actions taken on a candidate. New entries are **appended** by downstream skills.
| Status | Meaning |
|--------|---------|
| `"open"` | Pre-existing revert PR found open during analysis |
| `"merged"` | Pre-existing revert PR already merged |
| `"merged"` | Pre-existing revert PR already merged and included in the current payload |
| `"merged_not_in_payload"` | Pre-existing revert PR merged but not yet included in the current payload — fix is pending inclusion in the next payload build |
| `"staged"` | Revert PR and JIRA created, payload jobs triggered (used by `type: "revert"`) |
| `"pending"` | Experiment dispatched, payload jobs running, results not yet collected |
| `"passed"` | Payload jobs passed with the revert — candidate confirmed as cause |
Expand Down Expand Up @@ -196,11 +214,11 @@ An empty array or absent key means no RHCOS RPM changes were suspected.

### Create (used by `payload-analysis`)

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 `status: "open"` or `"merged"`. If RHCOS RPM suspects were identified, include them in `rhcos_suspects[]`.
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.
Comment on lines +217 to +221

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


### Append Action (used by `stage-payload-reverts`, `payload-experimental-reverts`)

Expand Down
Loading