OCPQUAL-30: Pull Request Risk Analysis Agent - #602
Conversation
…int verification jobs
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin 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
|
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 change adds CI skills for PR risk assessment, PR test-result analysis, job reporting, and Prow cost estimation, along with a risk analyst agent, evaluation cases and judges, marketplace registration, and ignored evaluation output paths. ChangesCI assessment toolkit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Analyst as pr-risk-analyst
participant Skill as assess-pr-risk
participant GitHub
participant Release as openshift/release
participant Sippy
participant Cost as prow-job-cost-estimator
Analyst->>Skill: assess PR URL
Skill->>GitHub: fetch PR metadata, diff, history, and comments
Skill->>Release: fetch presubmit job configuration
Skill->>Sippy: fetch CI metrics and durations
Skill->>Cost: estimate selected job costs
Skill-->>Analyst: produce report and JSON state
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
plugins/ci/skills/fetch-jobs/SKILL.md (1)
97-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocumented API defaults don't match the script's actual defaults.
This table documents
sortFielddefaulting tocurrent_pass_percentageandsortdefaulting toascat the raw Sippy API level. Butfetch_jobs.pyalways sends explicit values (current_runs/descper its argparse defaults, lines 199 and 206) and never omits these query params, so a user relying on this doc to predict the script's default ordering would be misled.Consider clarifying that this table reflects the raw API's behavior when params are omitted, and separately documenting the script's own CLI defaults.
🤖 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/fetch-jobs/SKILL.md` around lines 97 - 104, The documented defaults in the fetch-jobs API table do not match the behavior of fetch_jobs.py, which always passes its own argparse defaults for sortField and sort instead of omitting them. Update the SKILL.md documentation near the parameters table to clearly separate the raw Sippy API defaults from the script’s CLI defaults, and mention the fetch_jobs.py defaults for sortField and sort so users can predict the actual ordering.plugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.py (1)
147-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop variable
run_id.Flagged by ruff (B007):
run_idis never used inside the loop body.♻️ Proposed fix
- for i, ((run_id, job_name), tests) in enumerate( + for i, ((_run_id, job_name), tests) in enumerate( sorted(by_job.items(), key=lambda x: x[1][0].get("prow_job_start", ""), reverse=True) ):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.py` around lines 147 - 150, The loop in fetch_pr_test_results.py binds run_id but never uses it, triggering ruff B007. Update the iteration over by_job.items() so the unused name is removed or replaced with a placeholder in the enumerate loop, and keep the logic that sorts by prow_job_start and uses job_name/tests unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yaml`:
- Around line 6-9: The score band in the `annotations.yaml` case is inconsistent
with the `expected_tier` mapping used by `tier_matches_score`. Update the
`expected_score_range` for the `critical` case so it matches the `critical` tier
defined in `eval-assess-pr-risk.yaml` (71-100), and keep the `expected_tier`
value unchanged.
In `@plugins/ci/evals/eval-assess-pr-risk.yaml`:
- Around line 224-259: Add a check for annotations.must_skip_jobs in
eval-assess-pr-risk.yaml so cases fail when an expected skip is not present, and
update must_recommend_jobs to inspect only actual /test and /payload-job command
lines rather than any substring in the full conversation. Reuse the existing
command extraction approach from must_not_recommend_jobs, and apply the same
parsing in must_recommend_jobs so skip explanations no longer count as
recommendations.
In `@plugins/ci/skills/assess-pr-risk/SKILL.md`:
- Around line 26-29: The recent-reverts and merged-PR history commands in
SKILL.md rely on a macOS-only cutoff and fixed result limits, which can truncate
data. Update the gh pr list usage to use a portable 6-month date calculation and
fetch all pages for both the revert search and the merged PR query so
assess-pr-risk gets complete history. Keep the changes within the documented
command snippets around gh pr list and the merged: search qualifier.
- Around line 327-331: The Step 5 state-file write in assess-pr-risk should
ensure `.work/pr-risk` starts empty before saving the new assessment, since
reruns can leave stale JSON that breaks the “exactly one file” requirement.
Update the state-file writing flow to clear/reset `.work/pr-risk` first, then
write only `.work/pr-risk/<org>-<repo>-<pr_number>.json`, keeping any
intermediate artifacts in `/tmp/` and preserving the one-file contract.
In `@plugins/ci/skills/prow-job-cost-estimator/estimate_cost.py`:
- Around line 42-48: The aggregations in estimate_cost.py are silently dropping
jobs with unknown decision or ci_status values, which can distort the PR risk
report. Update the logic around the run_jobs/skip_jobs filtering and the
recommended_cost, savings, and added calculations to explicitly validate each
job’s decision and ci_status before summing. If any job has an unexpected value,
raise an error instead of excluding it so malformed upstream input is surfaced
immediately.
---
Nitpick comments:
In `@plugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.py`:
- Around line 147-150: The loop in fetch_pr_test_results.py binds run_id but
never uses it, triggering ruff B007. Update the iteration over by_job.items() so
the unused name is removed or replaced with a placeholder in the enumerate loop,
and keep the logic that sorts by prow_job_start and uses job_name/tests
unchanged.
In `@plugins/ci/skills/fetch-jobs/SKILL.md`:
- Around line 97-104: The documented defaults in the fetch-jobs API table do not
match the behavior of fetch_jobs.py, which always passes its own argparse
defaults for sortField and sort instead of omitting them. Update the SKILL.md
documentation near the parameters table to clearly separate the raw Sippy API
defaults from the script’s CLI defaults, and mention the fetch_jobs.py defaults
for sortField and sort so users can predict the actual ordering.
🪄 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: 1cec3ba1-f4bf-4c37-8515-1d1c57755d99
📒 Files selected for processing (19)
.gitignoredocs/data.jsondocs/index.htmlplugins/ci/agents/pr-risk-analyst.mdplugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yamlplugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/input.yamlplugins/ci/evals/cases/assess-pr-risk/case-002-origin-bug-id-swap/annotations.yamlplugins/ci/evals/cases/assess-pr-risk/case-002-origin-bug-id-swap/input.yamlplugins/ci/evals/cases/assess-pr-risk/case-003-origin-ote-bump/annotations.yamlplugins/ci/evals/cases/assess-pr-risk/case-003-origin-ote-bump/input.yamlplugins/ci/evals/eval-assess-pr-risk.mdplugins/ci/evals/eval-assess-pr-risk.yamlplugins/ci/skills/assess-pr-risk/SKILL.mdplugins/ci/skills/assess-pr-test-results/SKILL.mdplugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.pyplugins/ci/skills/fetch-jobs/SKILL.mdplugins/ci/skills/fetch-jobs/fetch_jobs.pyplugins/ci/skills/prow-job-cost-estimator/SKILL.mdplugins/ci/skills/prow-job-cost-estimator/estimate_cost.py
| # Recent reverts in the same repo (last 6 months) | ||
| # Calculate the date 6 months ago as YYYY-MM-DD and use GitHub's merged: search qualifier | ||
| gh pr list --repo <org>/<repo> --search "revert in:title merged:>$(date -v-6m +%Y-%m-%d)" --state merged --limit 50 --json number,title,mergedAt | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and surrounding lines
git ls-files plugins/ci/skills/assess-pr-risk/SKILL.md
wc -l plugins/ci/skills/assess-pr-risk/SKILL.md
cat -n plugins/ci/skills/assess-pr-risk/SKILL.md | sed -n '1,220p'
# Search for any other references to the same GH command pattern
rg -n "date -v-6m|--limit 50|gh pr list" plugins/ci/skills/assess-pr-risk/SKILL.md plugins -g '*.md'Repository: openshift-eng/ai-helpers
Length of output: 19860
Make the historical cutoff portable and paginate all merged PRs.
date -v-6m is macOS-only, and the fixed --limit values here can silently truncate results on active repos. That undercounts reverts/merges and skews the historical-risk score. Use a portable cutoff and fetch all pages in both places.
🧰 Tools
🪛 SkillSpector (2.3.7)
[warning] 84: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 86: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 87: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 88: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 89: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 90: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 91: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 92: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 140: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
🤖 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/assess-pr-risk/SKILL.md` around lines 26 - 29, The
recent-reverts and merged-PR history commands in SKILL.md rely on a macOS-only
cutoff and fixed result limits, which can truncate data. Update the gh pr list
usage to use a portable 6-month date calculation and fetch all pages for both
the revert search and the merged PR query so assess-pr-risk gets complete
history. Keep the changes within the documented command snippets around gh pr
list and the merged: search qualifier.
|
|
||
| This skill fetches job reports from the Sippy jobs API. It returns job metadata including pass rates, run counts, and trend data comparing the current 7-day window to the previous 7-day window. It supports both release jobs (e.g., `4.19`, `5.0`) and presubmit (pull request) jobs. | ||
|
|
||
| ## When to Use This Skill |
There was a problem hiding this comment.
technically when to use doesn't have effect in body. It belongs to description or its own metadata https://code.claude.com/docs/en/skills.md
openshift-ci
Bot
removed
the
needs-rebase
The expected_tier is critical (71-100) but the score range started at 55, which maps to the high tier. This internal inconsistency could pass the score-range check while failing tier_matches_score. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
must_recommend_jobs was matching any substring in the full conversation, so a job mentioned in a skip explanation could satisfy the check. Now it only matches actual /test and /payload-job command lines, consistent with how must_not_recommend_jobs already works. Also adds a must_skip_jobs judge to enforce the skip contract declared in the dataset schema but previously unchecked. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Unknown values were silently excluded from cost aggregations, which could skew the PR risk report if upstream input was malformed. Now raises ValueError on unexpected values so issues surface immediately. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The "When to Use" section in the SKILL.md body has no effect on skill triggering. Move the key use cases into the description field in the frontmatter where the framework actually reads them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The parameter table documents the raw Sippy API defaults, but fetch_jobs.py always sends its own values (current_runs/desc). Note the distinction so users can predict the script's actual ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
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/evals/cases/assess-pr-risk/eval.yaml`:
- Around line 73-97: Update the state_file_valid check to require exactly one
JSON artifact instead of selecting the first from json_files. Expand top-level
and assessment validation to include every documented schema field, including
coverage_gaps, key_risks, cost_estimate, and notes, while preserving the
existing JSON parsing and failure reporting.
- Around line 140-168: Update the job_recommendations_correct check to validate
structured state only: compare must_recommend_jobs and must_not_recommend_jobs
against assessment.jobs_to_run, and compare must_skip_jobs against
assessment.jobs_to_skip. Remove conversation text from recommendation matching,
and report missing required skips alongside existing recommendation issues while
preserving the current success behavior.
🪄 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: f0693fbe-881d-4e3e-9dfc-ec9d0f016ae4
📒 Files selected for processing (9)
docs/index.htmlplugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yamlplugins/ci/evals/cases/assess-pr-risk/eval.mdplugins/ci/evals/cases/assess-pr-risk/eval.yamlplugins/ci/evals/eval-assess-pr-risk.yamlplugins/ci/skills/assess-pr-risk/SKILL.mdplugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.pyplugins/ci/skills/fetch-jobs/SKILL.mdplugins/ci/skills/prow-job-cost-estimator/estimate_cost.py
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yaml
- plugins/ci/skills/prow-job-cost-estimator/estimate_cost.py
- plugins/ci/skills/fetch-jobs/SKILL.md
- plugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.py
- plugins/ci/evals/eval-assess-pr-risk.yaml
- docs/index.html
dgoodwin
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Require exactly one JSON artifact (not just pick the first) and validate all documented assessment fields including coverage_gaps, key_risks, cost_estimate, and notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…text The job_recommendations_correct judge was matching against the full conversation text, causing false positives when job names appeared in skip justifications. Now checks only assessment.jobs_to_run for must_recommend and must_not_recommend, and validates must_skip_jobs against assessment.jobs_to_skip. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
plugins/ci/skills/assess-pr-risk/SKILL.md (1)
28-28:⚠️ Potential issue | 🟠 MajorPaginate historical PR queries instead of using fixed limits.
The portable date calculation is fixed, but
--limit 50,--limit 100, and--limit 1000still silently truncate results for active repositories. Count all matching pages before calculating revert rate and historical risk.🤖 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/assess-pr-risk/SKILL.md` at line 28, Update the historical PR queries in plugins/ci/skills/assess-pr-risk/SKILL.md lines 28-28 and 110-113 to paginate through all matching pages instead of relying on fixed --limit values of 50, 100, or 1000. Ensure the complete result set is counted before calculating revert rates and historical risk.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/evals/cases/assess-pr-risk/eval.md`:
- Around line 38-40: Update the test-case documentation to require that each
referenced GitHub PR is merged, matching the requirement in eval.yaml; keep the
existing accessibility and annotation requirements unchanged.
In `@plugins/ci/evals/cases/assess-pr-risk/eval.yaml`:
- Around line 40-47: Update the eval schema documentation and assessment flow to
add a structured assessment.hotspots field, then implement a deterministic
hotspots_correct judge with an explicit pass threshold that verifies every
expected_hotspots category. Apply the same changes to the related sections and
ensure the overall quality check includes this judge rather than relying only on
the LLM threshold.
- Around line 73-100: Update the state_file_valid check to require the sole JSON
file’s name to match <org>-<repo>-<pr_number>.json using the parsed data fields,
after validating the JSON and required fields. Reject any filename that does not
match this expected name while preserving the existing single-file and schema
validation behavior.
---
Duplicate comments:
In `@plugins/ci/skills/assess-pr-risk/SKILL.md`:
- Line 28: Update the historical PR queries in
plugins/ci/skills/assess-pr-risk/SKILL.md lines 28-28 and 110-113 to paginate
through all matching pages instead of relying on fixed --limit values of 50,
100, or 1000. Ensure the complete result set is counted before calculating
revert rates and historical risk.
🪄 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: 5d72f2e6-b181-4cd6-8392-d72c996a6e58
📒 Files selected for processing (10)
docs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yamlplugins/ci/evals/cases/assess-pr-risk/eval.mdplugins/ci/evals/cases/assess-pr-risk/eval.yamlplugins/ci/evals/eval-assess-pr-risk.yamlplugins/ci/skills/assess-pr-risk/SKILL.mdplugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.pyplugins/ci/skills/fetch-jobs/SKILL.mdplugins/ci/skills/prow-job-cost-estimator/estimate_cost.py
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/ci/.claude-plugin/plugin.json
- plugins/ci/evals/cases/assess-pr-risk/case-001-olm-hashstructure-upgrade/annotations.yaml
- docs/index.html
- plugins/ci/evals/eval-assess-pr-risk.yaml
- plugins/ci/skills/assess-pr-test-results/fetch_pr_test_results.py
- plugins/ci/skills/prow-job-cost-estimator/estimate_cost.py
- plugins/ci/skills/fetch-jobs/SKILL.md
| Each test case has: | ||
| - `input.yaml`: Single field `pr_url` — a real GitHub PR URL (e.g., `https://github.com/openshift/origin/pull/31350`). The PR must exist and be accessible via `gh`. | ||
| - `annotations.yaml`: Expected outcomes for scoring — `expected_tier`, `expected_score_range` (min/max), `must_recommend_jobs`, `must_not_recommend_jobs`, `must_skip_jobs`, `expected_hotspots`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require merged PRs here as well.
This permits open PRs, while eval.yaml requires merged PRs. Mutable inputs can invalidate fixed score and job annotations; document the same merged-PR requirement.
🤖 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/assess-pr-risk/eval.md` around lines 38 - 40, Update
the test-case documentation to require that each referenced GitHub PR is merged,
matching the requirement in eval.yaml; keep the existing accessibility and
annotation requirements unchanged.
| - annotations.yaml: Expected outcomes for scoring, with fields: | ||
| 'description' (what the PR does and why it's a good test case), | ||
| 'expected_tier' (low|medium|high|critical), | ||
| 'expected_score_range' (min/max integers 0-100), | ||
| 'must_recommend_jobs' (list of job name substrings that must appear in recommendations), | ||
| 'must_not_recommend_jobs' (list of job name substrings that must NOT appear), | ||
| 'must_skip_jobs' (list of jobs that must be explicitly skipped), | ||
| 'expected_hotspots' (list of hotspot categories the skill should flag). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make expected hotspots deterministically testable.
expected_hotspots has no corresponding structured state field or deterministic judge. The LLM quality threshold can pass without verifying every expected category. Add a documented assessment.hotspots field and a hotspots_correct judge with a threshold.
Also applies to: 52-59, 181-230
🤖 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/assess-pr-risk/eval.yaml` around lines 40 - 47, Update
the eval schema documentation and assessment flow to add a structured
assessment.hotspots field, then implement a deterministic hotspots_correct judge
with an explicit pass threshold that verifies every expected_hotspots category.
Apply the same changes to the related sections and ensure the overall quality
check includes this judge rather than relying only on the LLM threshold.
| - name: state_file_valid | ||
| description: | | ||
| Checks that the skill produced exactly one valid JSON state file in | ||
| .work/pr-risk/ with all required top-level and assessment fields. | ||
| check: | | ||
| import json | ||
| files = outputs.get("files", {}) | ||
| json_files = {k: v for k, v in files.items() if k.endswith(".json")} | ||
| if not json_files: | ||
| return (False, "No JSON state file found in .work/pr-risk/") | ||
| if len(json_files) > 1: | ||
| return (False, f"Expected 1 JSON file, found {len(json_files)}: {list(json_files.keys())}") | ||
| fname, content = next(iter(json_files.items())) | ||
| try: | ||
| data = json.loads(content) | ||
| except json.JSONDecodeError as e: | ||
| return (False, f"Invalid JSON: {e}") | ||
| required = ["pr_url", "org", "repo", "pr_number", "assessment"] | ||
| missing = [f for f in required if f not in data] | ||
| if missing: | ||
| return (False, f"Missing top-level fields: {missing}") | ||
| assess = data["assessment"] | ||
| a_required = ["risk_score", "risk_tier", "risk_breakdown", "jobs_to_run", | ||
| "jobs_to_skip", "coverage_gaps", "key_risks", "cost_estimate", "notes"] | ||
| a_missing = [f for f in a_required if f not in assess] | ||
| if a_missing: | ||
| return (False, f"Missing assessment fields: {a_missing}") | ||
| return (True, f"Valid state file: {fname}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the documented state-file name.
The check accepts any single JSON filename, including stale or unrelated artifacts. Require <org>-<repo>-<pr_number>.json after parsing the state data.
Proposed fix
if missing:
return (False, f"Missing top-level fields: {missing}")
+ expected_name = f"{data['org']}-{data['repo']}-{data['pr_number']}.json"
+ actual_name = fname.rsplit("/", 1)[-1]
+ if actual_name != expected_name:
+ return (False, f"Expected state file '{expected_name}', got '{actual_name}'")
assess = data["assessment"]📝 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.
| - name: state_file_valid | |
| description: | | |
| Checks that the skill produced exactly one valid JSON state file in | |
| .work/pr-risk/ with all required top-level and assessment fields. | |
| check: | | |
| import json | |
| files = outputs.get("files", {}) | |
| json_files = {k: v for k, v in files.items() if k.endswith(".json")} | |
| if not json_files: | |
| return (False, "No JSON state file found in .work/pr-risk/") | |
| if len(json_files) > 1: | |
| return (False, f"Expected 1 JSON file, found {len(json_files)}: {list(json_files.keys())}") | |
| fname, content = next(iter(json_files.items())) | |
| try: | |
| data = json.loads(content) | |
| except json.JSONDecodeError as e: | |
| return (False, f"Invalid JSON: {e}") | |
| required = ["pr_url", "org", "repo", "pr_number", "assessment"] | |
| missing = [f for f in required if f not in data] | |
| if missing: | |
| return (False, f"Missing top-level fields: {missing}") | |
| assess = data["assessment"] | |
| a_required = ["risk_score", "risk_tier", "risk_breakdown", "jobs_to_run", | |
| "jobs_to_skip", "coverage_gaps", "key_risks", "cost_estimate", "notes"] | |
| a_missing = [f for f in a_required if f not in assess] | |
| if a_missing: | |
| return (False, f"Missing assessment fields: {a_missing}") | |
| return (True, f"Valid state file: {fname}") | |
| - name: state_file_valid | |
| description: | | |
| Checks that the skill produced exactly one valid JSON state file in | |
| .work/pr-risk/ with all required top-level and assessment fields. | |
| check: | | |
| import json | |
| files = outputs.get("files", {}) | |
| json_files = {k: v for k, v in files.items() if k.endswith(".json")} | |
| if not json_files: | |
| return (False, "No JSON state file found in .work/pr-risk/") | |
| if len(json_files) > 1: | |
| return (False, f"Expected 1 JSON file, found {len(json_files)}: {list(json_files.keys())}") | |
| fname, content = next(iter(json_files.items())) | |
| try: | |
| data = json.loads(content) | |
| except json.JSONDecodeError as e: | |
| return (False, f"Invalid JSON: {e}") | |
| required = ["pr_url", "org", "repo", "pr_number", "assessment"] | |
| missing = [f for f in required if f not in data] | |
| if missing: | |
| return (False, f"Missing top-level fields: {missing}") | |
| expected_name = f"{data['org']}-{data['repo']}-{data['pr_number']}.json" | |
| actual_name = fname.rsplit("/", 1)[-1] | |
| if actual_name != expected_name: | |
| return (False, f"Expected state file '{expected_name}', got '{actual_name}'") | |
| assess = data["assessment"] | |
| a_required = ["risk_score", "risk_tier", "risk_breakdown", "jobs_to_run", | |
| "jobs_to_skip", "coverage_gaps", "key_risks", "cost_estimate", "notes"] | |
| a_missing = [f for f in a_required if f not in assess] | |
| if a_missing: | |
| return (False, f"Missing assessment fields: {a_missing}") | |
| return (True, f"Valid state file: {fname}") |
🤖 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/assess-pr-risk/eval.yaml` around lines 73 - 100,
Update the state_file_valid check to require the sole JSON file’s name to match
<org>-<repo>-<pr_number>.json using the parsed data fields, after validating the
JSON and required fields. Reject any filename that does not match this expected
name while preserving the existing single-file and schema validation behavior.
| import urllib.request | ||
| from datetime import datetime, timedelta, timezone | ||
|
|
||
| SIPPY_API_BASE = "http://localhost:8080/api" |
| @@ -0,0 +1,278 @@ | |||
| #!/usr/bin/env python3 | |||
| """Fetch test results for a pull request from the Sippy API. | |||
There was a problem hiding this comment.
The data lag on sippy is sort of unfortunate, I have mostly stopped using it in AI skills since they're perfectly capable of just grabbing the data from the PR itself (both the entire test matrix for the given sha via https://prow.ci.openshift.org/pr-history/?org=openshift&repo=origin&pr=31404, and payload jobs in comments)
There was a problem hiding this comment.
That is surprising, do you think that's the route this should go? I'm nervous about /payload, possibly with aggregated jobs, and flake vs fail parsing, test names, other things that might go wrong. It seems like it might be best to rely on the formalized parsing. It does however lag and requires a PR in sippy that's still outstanding.
There was a problem hiding this comment.
The main deciding factor is how you want to invoke these skills. If I were building this, I would want these skills invoked by an analysis presubmit job (either always run, or invoked ad hoc) on a PR that:
- Waits for all presubmits and /payload jobs on a PR to finish
- Analyzes the results with this skill
- Leaves a comment on the PR (eventually allowing the job to inform a merge decision - possibly overriding jobs, applying a label, or deciding more tests should run on the PR)
It can still be formalized with a python script. You could also use Sippy API, but against live data (bigquery? but I guess there's cost problems...)
The delay in Sippy’s data makes that not really feasible. We've had cases recently where TRT went to 4+ hours for sync time (but I think its basically back to an hour now)
| gh pr view <url> --json number,title,body,author,files,labels,state,additions,deletions,changedFiles,baseRefName,headRefName,statusCheckRollup | ||
|
|
||
| # Diff summary (always fetch this first for large PRs) | ||
| gh pr diff <url> --stat |
There was a problem hiding this comment.
I'm not finding a --stat option, here and line 31
| gh pr diff <url> | ||
|
|
||
| # PR comments — look for reviewer feedback and CodeRabbit analysis | ||
| gh api repos/<org>/<repo>/pulls/<pr_number>/comments --paginate --jq '.[] | {user: .user.login, body: .body}' 2>/dev/null |
There was a problem hiding this comment.
I think gh pr view <pr number> --comments might be a lot more efficient
| branch="<baseRefName from PR metadata>" | ||
|
|
||
| # Fetch the CI config YAML | ||
| gh api repos/openshift/release/contents/ci-operator/config/<org>/<repo>/<org>-<repo>-${branch}.yaml --jq '.content' | base64 -d > /tmp/ci-config.yaml |
There was a problem hiding this comment.
if the config file doesn't exist the empty file that gets created seems like it will break the python down below. need a check in that that file is non-empty
|
I think the skills look pretty good! I only had time to just comment on some practical considerations on the risk assessment skill |
| Examine the diff for these patterns. Points are additive but capped at 30: | ||
|
|
||
| | Factor | Points | What to Look For | | ||
| | ---------------------------------------- | ------ | --------------------------------------------------------------------------------------------------------------------- | |
There was a problem hiding this comment.
these factors seem very specific to operator repos. There are some repos like ironic-agent-image, ovn-kubernetes for example where many of the categories will never be met and this value becomes inaccurate - should we have some generic factors that can cover non operator/controller repos as well?
Deep Review VerdictDisposition: REQUEST_CHANGES — 4 confirmed bugs, all verified with runtime reproducers
|
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | BLOCKING | fetch_pr_test_results.py:16 |
Hardcoded localhost:8080 URL — script non-functional |
| 2 | SUGGESTION | fetch_jobs.py:138 |
format_summary TypeError on API null values |
Adversarial (13 findings: 4 blocking, 5 suggestions, 4 notes)
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | BLOCKING | fetch_jobs.py:138 |
format_summary TypeError on null fields |
| 2 | BLOCKING | fetch_jobs.py:35 |
get_latest_release assumes sorted API response |
| 3 | BLOCKING | fetch_pr_test_results.py:16 |
localhost:8080 URL — connection refused |
| 4 | BLOCKING | fetch_pr_test_results.py:127 |
format_summary KeyError on missing dict keys |
| 5 | SUGGESTION | fetch_pr_test_results.py:226 |
Malformed URLs produce garbage org/repo |
| 6 | SUGGESTION | estimate_cost.py:39 |
estimate() mutates input in-place |
| 7 | SUGGESTION | estimate_cost.py:47 |
Missing validation for name/duration_minutes |
| 8 | SUGGESTION | estimate_cost.py:50 |
Negative duration_minutes produces negative costs |
| 9 | SUGGESTION | estimate_cost.py:27 |
Platform detection order-dependent substring matching |
| 10 | SUGGESTION | SKILL.md:69 |
Section B scoring can exceed stated 0-25 max |
| 11 | NOTE | SKILL.md:69 |
File count tier boundary overlap at 30 |
| 12 | NOTE | SKILL.md:110 |
Revert rate inflated by --limit caps on active repos |
| 13 | NOTE | fetch_jobs.py:29 |
get_latest_release crashes if API returns null releases |
Security & Supply Chain (7 findings, no blocking)
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | SUGGESTION | fetch_pr_test_results.py:16 |
Plaintext HTTP for Sippy API |
| 2 | SUGGESTION | SKILL.md:157 |
Unsanitized PR metadata in shell commands |
| 3 | SUGGESTION | eval.yaml:21 |
Overly broad Bash permission in eval config |
| 4-7 | NOTE | various | API URL in errors, arbitrary file read, yaml.safe_load on remote content, eval judge safety |
Architecture (8 findings: 1 blocking, 4 suggestions, 3 notes)
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | BLOCKING | fetch_pr_test_results.py:16 |
SIPPY_API_BASE localhost vs production |
| 2 | SUGGESTION | eval.yaml:1 |
Duplicate divergent eval definitions |
| 3 | SUGGESTION | eval.yaml:67 |
Critical eval judges dropped without replacement |
| 4 | SUGGESTION | pr-risk-analyst.md:16 |
Agent claims test skill is "planned" but it ships now |
| 5 | SUGGESTION | eval.yaml:21 |
Blanket Bash permission less safe than granular |
| 6-8 | NOTE | various | Input mutation, agent color format, unusual query string construction |
Codebase Consistency (7 findings: 1 blocking, 3 suggestions, 3 notes)
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | BLOCKING | fetch_pr_test_results.py:16 |
localhost URL inconsistent with all other scripts |
| 2 | SUGGESTION | eval-assess-pr-risk.yaml:3 |
Skill ref missing ci: prefix |
| 3 | SUGGESTION | eval.yaml:1 |
Duplicate eval config in two locations |
| 4 | SUGGESTION | pr-risk-analyst.md:18 |
Agent text contradicts co-shipped skill |
| 5-7 | NOTE | various | Missing See Also section, duplicated get_latest_release, permission style inconsistency |
QA Engineer (12 findings, no blocking)
| # | Severity | File | Finding |
|---|---|---|---|
| 1-3 | SUGGESTION | estimate_cost.py, fetch_jobs.py, fetch_pr_test_results.py |
No unit tests for any Python script |
| 4 | SUGGESTION | annotations | Overly broad 'e2e' substring in eval must_not_recommend |
| 5 | SUGGESTION | annotations | Case 001 missing must_skip_jobs entries |
| 6 | SUGGESTION | evals | Only 3 eval cases, missing high-risk tier coverage |
| 7-12 | NOTE | various | Duplicate eval, sys.exit vs exceptions, dict access inconsistency, negative duration, input mutation |
Technical Writer (3 findings: 1 blocking, 1 suggestion, 1 note)
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | BLOCKING | fetch_pr_test_results.py:16 |
Script hardcodes localhost but SKILL.md implies public API |
| 2 | SUGGESTION | pr-risk-analyst.md:18 |
Agent says skill is "planned" but it ships in this PR |
| 3 | NOTE | SKILL.md:21 |
Prerequisites don't document localhost requirement |
Panel Synthesis
Strong consensus across all 7 specialists on the localhost URL issue — every reviewer independently flagged it. fetch_pr_test_results.py is the only script in 15+ Sippy clients that uses localhost:8080 instead of the production URL. Reproducer confirmed connection refused, and verified the same query succeeds against the public endpoint with 277 results.
Three additional bugs were confirmed by reproducers:
-
format_summaryTypeError (fetch_jobs.py:138) —.get('field', 0)does not protect against JSON null values where the key exists withNone. The author already handles this correctly forduration_minutesat line 157 with an explicitis not Nonecheck, but missed the same pattern for pass percentages. Fix: use(j.get('field') or 0). -
get_latest_releaseunsorted assumption (fetch_jobs.py:35) — Returnsocp_releases[0]without sorting. Currently works because the Sippy API happens to return descending order, but no sort is applied. Reproducer showed wrong results with non-descending input. Fix: addsorted(..., reverse=True)with version-aware key. -
format_summaryKeyError (fetch_pr_test_results.py:127) — Direct dict access (r['prow_job_run_id'],r['prow_job_name'],f['test_name']) crashes on missing keys, while other fields in the same function correctly use.get(). All three crash paths confirmed.
Required Actions Before Merge
-
Fix
SIPPY_API_BASEinfetch_pr_test_results.py:16— Change fromhttp://localhost:8080/apitohttps://sippy.dptools.openshift.org/api. -
Fix null handling in
fetch_jobs.py:138,148,166— Use(j.get('current_pass_percentage') or 0)instead ofj.get('current_pass_percentage', 0)for all float-formatted fields. -
Sort releases in
fetch_jobs.py:35— Add version-aware sort before taking[0]. -
Fix KeyError risk in
fetch_pr_test_results.py:127,162— Use.get()with defaults forprow_job_run_id,prow_job_name, andtest_name.
Optional Follow-ups
- Update
pr-risk-analyst.md:18to referenceci:assess-pr-test-resultsinstead of saying it's "planned for a future iteration" (3 specialists flagged) - Consolidate the two
eval.yamlfiles — decide which is canonical and remove the other (3 specialists flagged) - Fix
ci:prefix ineval-assess-pr-risk.yaml:3skill reference - Add explicit cap to Section B scoring in
SKILL.md:69("capped at 25") - Add unit tests for the three Python scripts
- Add more eval cases to cover high-risk tier and docs-only PRs
Stats: 52 raw findings from 7 specialists. Kept: 4 blocking (4 reproduced), 16 suggestions, 14 notes. Dropped: 18 duplicates.
Generated by /deep-review
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
dgoodwin
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dgoodwin: This pull request references OCPQUAL-30 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
7 participants
Adds two new skills to be part of a future PR pipeline.
assess-pr-risk attempts to determine a risk level for a PR (given code complexity, historical revert risk of the repo), determine what testing should be run against it. The goal here is to improve quality and pre-merge issue detection, while also reducing cost by skipping jobs that are not required. The skill works with the current set of presubmits, and a small set of optional /payload jobs it knows about. If successful, the savings on not running unnecessary jobs may outweigh the cost of performing the analysis on all PRs. The plugin makes a rough guess on how much it spent/saved on CI testing.
assess-pr-test-results attempts to determine if the testing results (across presubmits and /payload jobs) look suspicious, or if we are seeing any failures that look suspicious.
Includes a start on a few evals: /eval-run plugins/ci/evals/cases/assess-pr-risk/
Summary by CodeRabbit
assess-pr-riskevaluation definitions and new scenarios for risk scoring/job recommendations..gitignoreto exclude evaluation run outputs.