USHIFT-7291, USHIFT-7292: move advisory report in-tree and fix CVE recommendations#225
Conversation
|
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 |
|
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 |
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
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
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
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
| jira = c.get("jira_ticket") or {} | ||
| jid = jira.get("id", "--") | ||
| resolution = jira.get("resolution", "--") | ||
| summary = jira.get("summary", "--")[:80].replace("|", "\\|").replace("\n", " ") |
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.
| 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.
- Complete Atlassian → Jira MCP migration in SKILL.md (Steps 2-3, fallback, Notes, Prerequisites still referenced old tool names) - Fix branch ordering in advisory.py: try source_branch before target_branch so unmerged MRs read MR content, not stale base - Add AttributeError guard to chained .get() in _get_advisories, matching the pattern in _get_shipment_mr_url - Use os.path.basename for advisory type matching instead of substring match against full file path - Map 'pending_enrichment' to 'pending' in verbose table display - Remove unused 'import os' from precheck_xyz.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Move advisory report generation from shelling out to MicroShift's advisory_publication_report.sh to an in-tree advisory.py that extracts CVE data from OCP shipment MRs (GitHub releases.yml + GitLab). This removes the ATLASSIAN_API_TOKEN dependency — Jira CVE lookup is deferred to the skill layer via MCP. CVE interpretation changes: - Skip in-progress CVE fixes instead of flagging NEEDS REVIEW - Deduplicate CVEs across advisories (extras + image) - Handle Done-Errata as already-shipped (no action needed) - Include OCPBUGS tracker key in recommendation output - Track skipped/pending counts for visibility Fixes: USHIFT-7291, USHIFT-7292 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
- Rename test_cve_needs_review → test_cve_in_progress_skipped - Rename test_cve_needs_review_status_verified → test_cve_verified_but_unresolved_skipped - Add tests: CVE deduplication across advisories, pending enrichment, mixed must_release+skipped, Jira key in recommendations, skipped_not_actionable display, pending_enrichment reason formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
6a7fac1 to
d2d7063
Compare
|
/label tide/merge-method-squash |
- Replace Atlassian MCP tool references with Jira MCP equivalents (jira_get_issue, jira_search) - Enforce ISO 8601 Monday-based weeks in time range parsing - Add Step 7: advisory CVE enrichment via Jira MCP search - Add Step 8: final recommendations table (one row per version) 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
d2d7063 to
de44312
Compare
Advisory report logic moved in-tree (advisory.py), so the sparse-checkout of scripts/advisory_publication is no longer needed. The --filter=blob:none partial clone already avoids downloading blobs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
|
/lgtm |
Summary
The pre-check advisory report previously shelled out to
advisory_publication_report.shin the MicroShift repo clone, which requiredATLASSIAN_API_TOKEN+ATLASSIAN_EMAILfor Jira CVE lookups. These env vars are unavailable when running via Claude Code (which uses Jira MCP with OAuth).This PR:
lib/advisory.pyfetches CVE data from OCP shipment MRs (GitHubreleases.yml+ GitLabocp-shipment-data) with no Jira auth needed. Jira CVE lookup is deferred to the skill layer via MCP.NEEDS REVIEW; Done-Errata CVEs (already shipped) no longer trigger falseASK ART TO CREATE ARTIFACTS. Only CVEs with resolutionDone(fix landed, not yet shipped) recommend a release.Fixes: USHIFT-7291, USHIFT-7292
Test plan
/microshift-release:pre-check this week— advisory CVEs show aspending, OCPBUGS enriched via MCP, final recommendations table renderedCVE-XXXX (OCPBUGS-XXXXX)formatpython -m pytest)