[OCPERT-437] Port blocking sec-alerts check command to main for Konflux release flow - #1147
[OCPERT-437] Port blocking sec-alerts check command to main for Konflux release flow#1147jhuttana wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
WalkthroughThe release workflow replaces the prior RHCOS-only security-alert step with a blocking security-alert check. A new CLI command scans RHSA advisories, updates StateBox, creates blocker issues, and is exposed through an MCP tool. ChangesBlocking security alert workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant MCPServer
participant CheckBlockingSecAlerts
participant AdvisoryManager
participant StateBox
ReleaseWorkflow->>MCPServer: oar_check_blocking_sec_alerts(release)
MCPServer->>CheckBlockingSecAlerts: invoke check-blocking-sec-alerts
CheckBlockingSecAlerts->>AdvisoryManager: load and inspect RHSA advisories
AdvisoryManager-->>CheckBlockingSecAlerts: blocking alert results
CheckBlockingSecAlerts->>StateBox: update task status and blocker issues
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.claude/skills/release-workflow/SKILL.md:
- Around line 351-369: Add the `text` language annotation to all three fenced
code blocks in the security alert detection section, including the two behavior
examples and the Success/Failure Detection blocks, without changing their
contents.
In `@oar/cli/cmd_check_blocking_sec_alerts.py`:
- Line 6: Replace the wildcard import in
oar/cli/cmd_check_blocking_sec_alerts.py at lines 6-6 with explicit imports for
the task and status constants used by the module. Also replace the wildcard
import in tests/test_advisory.py at lines 9-9 with only the constants referenced
by that test module; preserve all existing behavior while eliminating Ruff F403.
- Around line 50-54: Update the RHSA result handling in the
check-blocking-alerts command so any non-empty check_errors causes
TASK_STATUS_FAIL, including mixed successful and failed checks, instead of only
failing when rhsa_checked == 0. Preserve known-blocker issue creation where
applicable, and add a regression test covering mixed success and failure.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8e500334-2ae3-436f-b158-8881fb0400d4
📒 Files selected for processing (7)
.claude/skills/release-workflow/SKILL.mdmcp_server/server.pyoar/cli/cmd_check_blocking_sec_alerts.pyoar/cli/cmd_group.pyoar/core/advisory.pyoar/core/const.pytests/test_advisory.py
…ilures in sec-alerts command
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)
.claude/skills/release-workflow/SKILL.md (1)
59-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate all downstream release actions on
check-blocking-sec-alertspassing.The workflow currently allows downstream actions after a task that can report
Failand create a StateBox blocker, undermining the intended blocking behavior.
.claude/skills/release-workflow/SKILL.md#L59-L59: make the task-graph transition conditional onPass..claude/skills/release-workflow/SKILL.md#L89-L89: state that CDN staging starts only after a passing security-alert check..claude/skills/release-workflow/SKILL.md#L374-L376: trigger downstream actions only onPass; otherwise stop and await blocker resolution.🤖 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 @.claude/skills/release-workflow/SKILL.md at line 59, Gate all downstream release actions on a Pass result from check-blocking-sec-alerts: update the task-graph transition at .claude/skills/release-workflow/SKILL.md lines 59-59, document at lines 89-89 that CDN staging begins only after the check passes, and revise lines 374-376 so non-Pass results stop downstream actions and await StateBox blocker resolution.
🧹 Nitpick comments (1)
oar/cli/cmd_check_blocking_sec_alerts.py (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the per-advisory exception handler. Catch only the API/parsing errors expected from
has_blocking_security_alert()here; unexpected exceptions should reach the outerlogger.exception(...)path so tracebacks are preserved and programming bugs don’t look like ordinary advisory failures.🤖 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 `@oar/cli/cmd_check_blocking_sec_alerts.py` around lines 51 - 53, In the per-advisory handling around has_blocking_security_alert(), replace the broad Exception handler with the specific API/parsing exception types that method is expected to raise. Keep recording expected advisory failures in check_errors and logger.error, while allowing unexpected exceptions to propagate to the outer logger.exception(...) path with their tracebacks.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 `@oar/cli/cmd_check_blocking_sec_alerts.py`:
- Around line 55-60: Update the blocking-security-alert check flow around
TASK_CHECK_BLOCKING_SEC_ALERTS so the existing blocker is not resolved before
advisory checks and replacement add_issue persistence succeed. Resolve the prior
blocker only after a clean pass or confirmed replacement creation, and retain it
when checks fail or StateBoxException occurs during persistence.
- Around line 35-39: Update the no-RHSA early-return path in the blocking
security alert check to resolve any existing StateBox blocker before marking
TASK_CHECK_BLOCKING_SEC_ALERTS as PASS and returning. Reuse the existing
blocker-resolution mechanism, or centralize that cleanup so it runs whenever the
check succeeds after advisories disappear.
---
Outside diff comments:
In @.claude/skills/release-workflow/SKILL.md:
- Line 59: Gate all downstream release actions on a Pass result from
check-blocking-sec-alerts: update the task-graph transition at
.claude/skills/release-workflow/SKILL.md lines 59-59, document at lines 89-89
that CDN staging begins only after the check passes, and revise lines 374-376 so
non-Pass results stop downstream actions and await StateBox blocker resolution.
---
Nitpick comments:
In `@oar/cli/cmd_check_blocking_sec_alerts.py`:
- Around line 51-53: In the per-advisory handling around
has_blocking_security_alert(), replace the broad Exception handler with the
specific API/parsing exception types that method is expected to raise. Keep
recording expected advisory failures in check_errors and logger.error, while
allowing unexpected exceptions to propagate to the outer logger.exception(...)
path with their tracebacks.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00bfbd0d-49ea-40a8-9bc0-2934cb724dac
📒 Files selected for processing (3)
.claude/skills/release-workflow/SKILL.mdoar/cli/cmd_check_blocking_sec_alerts.pytests/test_advisory.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_advisory.py
| existing_blocker = statebox.get_task_blocker(TASK_CHECK_BLOCKING_SEC_ALERTS) | ||
| if existing_blocker: | ||
| statebox.resolve_issue( | ||
| issue=existing_blocker["issue"], | ||
| resolution="Re-checked blocking security alerts", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Retain the previous blocker until replacement state is durable.
The existing blocker is resolved before check completion. If any advisory check fails, or add_issue() raises StateBoxException, the old blocker can be gone without a replacement even though the task remains failed. Resolve it only after a clean pass or successful replacement creation; retain it on partial or persistence failures.
Also applies to: 74-83
🤖 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 `@oar/cli/cmd_check_blocking_sec_alerts.py` around lines 55 - 60, Update the
blocking-security-alert check flow around TASK_CHECK_BLOCKING_SEC_ALERTS so the
existing blocker is not resolved before advisory checks and replacement
add_issue persistence succeed. Resolve the prior blocker only after a clean pass
or confirmed replacement creation, and retain it when checks fail or
StateBoxException occurs during persistence.
There was a problem hiding this comment.
Addressed this issue by re-ordering that logic:
Re-run -> Check the issue -> Resolved/Update.
Also, made some more enhancements:
In cmd_check_blocking_sec_alerts.py -- using get_issues(unresolved_only=True, task_name=...) is better than get_task_blocker() because:
- It returns all unresolved issues (blockers + non-blockers) for the task
- The pass path (lines 100-105) resolves every unresolved issue in a loop, not just the first blocker
- The fail path (lines 72-73) extracts the blocker from the list with next(...) for the dedup comparison
In test_advisory.py -- all tests properly updated:
- Existing tests mock get_issues returning [] for the no-prior-issues case
- test_rerun_pass_resolves_existing_blocker -- single blocker resolved on pass
- test_rerun_same_blocker_skips_update -- duplicate blocker left alone
- test_rerun_pass_resolves_all_issues -- both a blocker and a non-blocker resolved, asserts resolve_issue.call_count == 2
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline oar/cli/cmd_check_blocking_sec_alerts.py --items all
ast-grep outline tests/test_advisory.py --match 'TestCheckBlockingSecAlerts' --view expanded
printf '\n--- command implementation ---\n'
sed -n '1,180p' oar/cli/cmd_check_blocking_sec_alerts.py
printf '\n--- relevant rerun tests ---\n'
rg -n -A45 -B8 \
'def test_rerun_(pass_resolves_existing_blocker|same_blocker_skips_update|pass_resolves_all_issues)\b' \
tests/test_advisory.pyLength of output: 10910
@jhuttana, thanks for the broader get_issues() cleanup—resolving all unresolved task issues on the clean-pass path is an improvement. However, the original issue remains in the current implementation.
In oar/cli/cmd_check_blocking_sec_alerts.py, the changed-blocker path still calls resolve_issue() before add_issue(). If add_issue() raises StateBoxException, its exception is caught and the task is marked failed, but the prior blocker has already been removed without a durable replacement.
Please create the replacement first, then resolve the previous blocker only after add_issue() succeeds. A regression test should cover an existing different blocker plus add_issue.side_effect = StateBoxException(...), asserting that resolve_issue() is not called.
…ilures in sec-alerts command
|
@jhuttana: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 participant
JIRA: https://redhat.atlassian.net/browse/OCPERT-437
Summary by CodeRabbit
New Features
Bug Fixes
Tests