fix: pre-check CVE recommendation improvements#225
Draft
agullon wants to merge 11 commits into
Draft
Conversation
CVEs whose fixes haven't landed yet (status other than Done, Done-Errata, or Not a Bug) are not actionable for a z-stream release — skip them instead of flagging NEEDS REVIEW. Also include the OCPBUGS tracker key alongside CVE IDs in the recommendation output for clarity. Fixes: USHIFT-7291, USHIFT-7292 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: agullon 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
|
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:
WalkthroughThe MicroShift release precheck now retrieves advisory data through Python, deduplicates CVEs, distinguishes actionable, non-actionable, and pending Jira states, supports CVE enrichment output, updates Jira MCP workflow instructions, and bumps plugin versions to 1.4.2. ChangesMicroShift release precheck
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
![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/microshift-release/scripts/precheck_xyz.py (1)
203-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate tests for the removed
needs_reviewcontract.
test_cve_needs_reviewstill expects"needs_review"although in-progress resolutions now return"none".test_needs_review_cve_in_progressalso passes the removed impact and will now receiveSKIP. Update or remove those cases and add explicitDone,Done-Errata,Not a Bug, and in-progress coverage.Per CONTRIBUTING.md: parsing and interpretation changes require positive and negative tests.
🤖 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/microshift-release/scripts/precheck_xyz.py` around lines 203 - 205, Update the tests covering CVE impact interpretation to remove the obsolete "needs_review" expectation and input, including test_cve_needs_review and test_needs_review_cve_in_progress. Add positive and negative coverage for Done, Done-Errata, Not a Bug, and in-progress states, asserting in-progress cases return "none" and removed impacts produce SKIP as required.Source: Path instructions
🤖 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/microshift-release/scripts/precheck_xyz.py`:
- Around line 164-170: Add regression tests for the must-release path through
compute_recommendation and _build_reason, using CVE details with a Jira value
and asserting the reason includes the formatted “CVE (Jira)” text. Also add a
no-Jira case asserting the reason falls back to the CVE alone, with positive and
negative coverage so both formatting behaviors are protected.
- Around line 90-101: Update the precheck function’s contract docstring to
include the "unknown" impact outcome for skipped reports and replace “any other
status” with “any other resolution.” Remove the unused status assignment from
the CVE processing logic, including any now-unnecessary references, so the
Python code passes Ruff.
---
Outside diff comments:
In `@plugins/microshift-release/scripts/precheck_xyz.py`:
- Around line 203-205: Update the tests covering CVE impact interpretation to
remove the obsolete "needs_review" expectation and input, including
test_cve_needs_review and test_needs_review_cve_in_progress. Add positive and
negative coverage for Done, Done-Errata, Not a Bug, and in-progress states,
asserting in-progress cases return "none" and removed impacts produce SKIP as
required.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ef8b0206-9c72-4036-a0fc-41dd31f32138
📒 Files selected for processing (1)
plugins/microshift-release/scripts/precheck_xyz.py
coderabbitai
Bot
added
the
ready-for-human-review
- Add explicit else branch with debug logging for CVEs with non-terminal resolutions so the skip is intentional and auditable - Track skipped_in_progress count so output distinguishes "no CVEs" from "no actionable CVEs (N in progress)" - Update 3 unit tests to assert the new behavior instead of the removed needs_review CVE path Fixes: USHIFT-7291, USHIFT-7292 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
coderabbitai
Bot
removed
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/microshift-release/scripts/unit_tests/test_logic.py`:
- Around line 247-254: Update test_in_progress_cves_skip to capture the
recommendation reason returned by compute_recommendation and assert the expected
skipped-in-progress-CVE reason, distinguishing it from the generic SKIP path
covered by test_skip_commits_no_cves.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3cf236e8-5fc5-4840-a480-866dd39e1d79
📒 Files selected for processing (2)
plugins/microshift-release/scripts/precheck_xyz.pyplugins/microshift-release/scripts/unit_tests/test_logic.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/microshift-release/scripts/precheck_xyz.py
…tness - Dedup CVEs across advisories with seen_cves set (same CVE in extras and image advisories was double-counted) - Rename skipped_in_progress to skipped_not_actionable (terminal Jira resolutions like Won't Do are not "in progress") - Make compute_recommendation aware of skipped CVEs so verbose mode shows the same detail as short format - Update interpret_cves docstring to document all return keys - Use brace-depth matching in run_advisory_report JSON extraction to handle warnings containing braces before the JSON payload - Add 7 new tests: CVE deduplication, mixed must_release+skipped, jira formatting in compute_recommendation and _build_reason, skipped_not_actionable display, reason string assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/microshift-release/scripts/precheck_xyz.py`:
- Around line 145-146: Update the preceding terminal-status branch handling
Done-Errata and Not a Bug to increment skipped_not_actionable before continuing,
so these CVEs are counted as skipped rather than omitted. Extend
test_cve_done_errata_skipped to assert the updated skipped count and resulting
no actionable CVEs output.
- Around line 107-108: Update the skipped-result branch in the advisory report
handling around advisory_report so it returns the documented default payload
shape: use an empty details list and include advisory_types and
skipped_not_actionable with their documented default values, while preserving
impact as unknown.
- Around line 66-77: Replace the manual brace-depth extraction in the advisory
parsing flow with a JSON-aware decoder that can locate and parse the valid
payload despite warning-prefixed stdout and braces inside quoted string values.
Preserve missing/invalid-payload handling, and add positive and negative
coverage for quoted-brace output and warning-prefixed output.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4cb1b8b1-ba7c-4ae4-8603-372465249e7f
📒 Files selected for processing (2)
plugins/microshift-release/scripts/precheck_xyz.pyplugins/microshift-release/scripts/unit_tests/test_logic.py
…EN dependency
The advisory publication report was shelled out to the MicroShift repo
clone, which required ATLASSIAN_API_TOKEN + ATLASSIAN_EMAIL (Jira PAT)
for CVE ticket lookups. These env vars are unavailable when running via
Claude Code (which uses Jira MCP with OAuth instead).
Split the advisory logic into two phases:
- Stage 1 (Python): Extract advisory names, types, and CVE IDs from OCP
shipment MR data (GitHub releases.yml + GitLab ocp-shipment-data).
Runs locally in lib/advisory.py with no Jira auth needed.
- Stage 2 (MCP): Jira CVE lookup deferred to the skill layer via MCP,
following the same pattern as OCPBUGS enrichment.
CVEs are marked with {"pending": True} until enriched, with a new
"pending_enrichment" impact value that flows through the recommendation
and display logic.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pre-commit.check-secrets: ENABLED
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/microshift-release/scripts/precheck_xyz.py (1)
103-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTerminal non-actionable resolutions (
Done-Errata,Not a Bug) still aren't counted as skipped.Only the catch-all
elsebranch incrementsskipped_not_actionable;Done-Errata/Not a Bugfall through thecontinueat line 110 uncounted. As a result, a CVE list containing only these terminal resolutions renders as "no CVEs" instead of "no actionable CVEs (n not actionable)" incompute_recommendation/_build_reason. This mirrors a prior review comment that has no recorded fix commit.🤖 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/microshift-release/scripts/precheck_xyz.py` around lines 103 - 116, Count terminal non-actionable resolutions in the resolution handling around the CVE classification logic: before continuing for “Done-Errata” or “Not a Bug”, increment skipped_not_actionable. Preserve the existing continue behavior and ensure compute_recommendation/_build_reason receives the correct non-actionable count.
♻️ Duplicate comments (1)
plugins/microshift-release/scripts/precheck_xyz.py (1)
70-71: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSkipped advisory-report result still omits documented keys.
This early return only provides
impact/details, but the docstring (lines 62-68) documentsadvisory_types,skipped_not_actionable, andpending_cve_enrichmentas part of the return contract. Downstream code (compute_recommendation,_build_reason) uses.get(..., default)so it won't crash, but the shape is inconsistent with what callers/tests expect for an "unknown" impact.🩹 Proposed fix
if not advisory_report or advisory_report.get("skipped"): - return {"impact": "unknown", "details": ["Advisory report was skipped"]} + return { + "impact": "unknown", + "details": ["Advisory report was skipped"], + "advisory_types": [], + "skipped_not_actionable": 0, + "pending_cve_enrichment": [], + }🤖 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/microshift-release/scripts/precheck_xyz.py` around lines 70 - 71, Update the skipped or missing advisory-report branch in the relevant precheck function to return every key documented by its return contract: advisory_types, skipped_not_actionable, and pending_cve_enrichment, alongside impact and details. Use the documented empty/default values for these fields while preserving the existing unknown impact and skipped-report details.
🤖 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/microshift-release/scripts/enrich_cves.py`:
- Around line 60-63: Update the summary extraction in the enrichment render to
treat an explicit null jira_ticket.summary the same as a missing value before
slicing and replacing text. Preserve the existing truncation and escaping
behavior for non-null summaries, using the fallback already established by the
surrounding jira fields.
- Around line 37-48: 添加针对 classify_cve 的单元测试，覆盖无 jira_ticket、resolution 为
Done、Done-Errata、Not a Bug 以及未知或缺失 resolution 的正反向场景；断言返回结果中的 release_action 和
actionable，同时确认原始 entry 字段被保留。
In `@plugins/microshift-release/scripts/lib/advisory.py`:
- Line 46: Update the external HTTPS request in _get_shipment_mr_url to use the
default TLS certificate verification by removing verify=False from requests.get,
while preserving the existing URL and timeout behavior.
- Around line 206-217: Update the token check in the advisory flow before
_get_advisories so GITLAB_API_TOKEN is required only when the parsed minor
version meets the documented 4.20+ threshold, comparing parts[1] numerically
rather than merely checking that it is digits. Preserve the existing
missing-token response and avoid gating versions below 4.20.
In `@plugins/microshift-release/skills/pre-check/SKILL.md`:
- Line 6: Update the allowed-tools declaration in the pre-check skill to include
mcp__atlassian__searchJiraIssuesUsingJql, which is still invoked by the Step 2
and Step 3 ART-ticket resolution flows, while retaining the existing tools and
keeping the list limited to tools actually used.
---
Outside diff comments:
In `@plugins/microshift-release/scripts/precheck_xyz.py`:
- Around line 103-116: Count terminal non-actionable resolutions in the
resolution handling around the CVE classification logic: before continuing for
“Done-Errata” or “Not a Bug”, increment skipped_not_actionable. Preserve the
existing continue behavior and ensure compute_recommendation/_build_reason
receives the correct non-actionable count.
---
Duplicate comments:
In `@plugins/microshift-release/scripts/precheck_xyz.py`:
- Around line 70-71: Update the skipped or missing advisory-report branch in the
relevant precheck function to return every key documented by its return
contract: advisory_types, skipped_not_actionable, and pending_cve_enrichment,
alongside impact and details. Use the documented empty/default values for these
fields while preserving the existing unknown impact and skipped-report details.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: abd7dc69-16f9-4eb3-849a-c493904155e4
📒 Files selected for processing (7)
plugins/microshift-release/scripts/enrich_cves.pyplugins/microshift-release/scripts/lib/advisory.pyplugins/microshift-release/scripts/lib/git_ops.pyplugins/microshift-release/scripts/precheck.shplugins/microshift-release/scripts/precheck_xyz.pyplugins/microshift-release/scripts/unit_tests/test_logic.pyplugins/microshift-release/skills/pre-check/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/microshift-release/scripts/unit_tests/test_logic.py
Comment on lines
+60
to
+63
| jira = c.get("jira_ticket") or {} | ||
| jid = jira.get("id", "--") | ||
| resolution = jira.get("resolution", "--") | ||
| summary = jira.get("summary", "--")[:80].replace("|", "\\|").replace("\n", " ") |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against explicit null summary crashing the render.
If jira_ticket.summary is explicitly null (key present, value None), .get("summary", "--") returns None, and None[:80] raises TypeError, aborting the whole enrichment render for all CVEs in the batch.
🛡️ Proposed fix
- summary = jira.get("summary", "--")[:80].replace("|", "\\|").replace("\n", " ")
+ summary = (jira.get("summary") or "--")[:80].replace("|", "\\|").replace("\n", " ")📝 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
| jira = c.get("jira_ticket") or {} | |
| jid = jira.get("id", "--") | |
| resolution = jira.get("resolution", "--") | |
| summary = jira.get("summary", "--")[:80].replace("|", "\\|").replace("\n", " ") | |
| jira = c.get("jira_ticket") or {} | |
| jid = jira.get("id", "--") | |
| resolution = jira.get("resolution", "--") | |
| summary = (jira.get("summary") or "--")[:80].replace("|", "\\|").replace("\n", " ") |
🤖 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/microshift-release/scripts/enrich_cves.py` around lines 60 - 63,
Update the summary extraction in the enrichment render to treat an explicit null
jira_ticket.summary the same as a missing value before slicing and replacing
text. Preserve the existing truncation and escaping behavior for non-null
summaries, using the fallback already established by the surrounding jira
fields.
| minor = ".".join(version.split(".")[:2]) | ||
| url = _OCP_BUILD_DATA.format(minor=minor) | ||
| try: | ||
| resp = requests.get(url, verify=False, timeout=15) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
TLS certificate verification disabled on external HTTPS fetch.
requests.get(url, verify=False, timeout=15) disables certificate validation when fetching releases.yml from GitHub. Any on-path attacker could tamper with the response, redirecting _get_shipment_mr_url to an arbitrary GitLab MR and poisoning the CVEs later reported by the precheck. Static analysis flags this too (ruff S501, ast-grep request-verify/CWE-295). The host itself is hardcoded, so SSRF risk is low, but disabling TLS verification for a real network call is unnecessary and should be fixed.
🔒 Proposed fix
- resp = requests.get(url, verify=False, timeout=15)
+ resp = requests.get(url, timeout=15)📝 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
| resp = requests.get(url, verify=False, timeout=15) | |
| resp = requests.get(url, timeout=15) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 46-46: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
🤖 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/microshift-release/scripts/lib/advisory.py` at line 46, Update the
external HTTPS request in _get_shipment_mr_url to use the default TLS
certificate verification by removing verify=False from requests.get, while
preserving the existing URL and timeout behavior.
Source: Linters/SAST tools
The two-phase CVE enrichment (pending markers + skill-layer MCP lookup) adds unnecessary complexity. CVE data from shipment MRs is already surfaced — Jira lookup can be done inline later via Kerberos if needed. Removes enrich_cves.py, enrich-cves command, and SKILL.md Step 7. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Clarify that "this week" and "next week" always use Monday as the first day of the week per ISO 8601, including the edge case where today is Sunday. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Since CVE enrichment via MCP was removed, the "pending Jira lookup" text implied a resolution step that no longer exists. Now shows "N advisory CVEs" to indicate CVEs were found in the shipment MR without implying a follow-up action. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Log YAML parse failures at warning level instead of silently dropping - Distinguish GitLab API failures from legitimate "no data" with warning logs - Promote key failure diagnostics from debug to warning level - Narrow JSON catch blocks to ValueError (remove overly broad AttributeError) - Restore minor >= 20 guard for GITLAB_API_TOKEN requirement - Add missing keys to interpret_cves early return for consistency - Fix _extract_cves docstring accuracy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Adds Step 7 to search Jira for each advisory CVE to determine if it affects MicroShift. Done entirely at the skill layer via MCP — no extra scripts needed. Updates recommendations if any CVE has resolution "Done" (fix landed but not shipped). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
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
1 participant
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
Done(fix landed but not yet shipped) trigger a release recommendationTest plan
/microshift-release:pre-checkagainst a version with a Done-Errata CVE — should SKIP, not ASK ARTCVE-XXXX (OCPBUGS-XXXXX)format🤖 Generated with Claude Code
Summary by CodeRabbit
enrich-cvescommand to generate a CVE advisory table and recommendation counts.