docs(security-analysis): document OSV-Scanner as merge_group dependency-CVE check (#168)#178
Conversation
📝 WalkthroughWalkthroughAdds a detailed audit handoff document for Issue ChangesDocumentation & Workflow Comments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the documentation comment in the python-security-analysis workflow template to reference the correct tracking issue for the merge_group dependency-CVE coverage gap, keeping the KNOWN LIMITATION guidance accurate for consumer repos using merge queue.
Changes:
williaby
changed the title
PR ReviewThis PR is comment-only YAML plus a new audit document — no logic, security, or CI surface changes. All checks pass; SonarCloud Quality Gate green (0 new issues, 0 new hotspots); branch is BEHIND Critical: none. Important (1):
Suggested (3 — optional, all doc-only edits to the audit file):
Informational: Copilot's review body describes only the first commit ( Strengths: architectural rigor in the reframe; PR body resolves the audit's open question; all cross-references to #168, #154, and #163 verify; CLAUDE.md compliant (no em-dashes, conventional commits, signed, correct branch convention). 🤖 Generated with Claude Code |
… issue (#168) The KNOWN LIMITATION comment block referenced "issue #154 followup" for the osv-scanner-on-merge_group coverage gap. Issue #154 is the parent merge_queue rollout; the dedicated coverage-gap tracker is #168. Updates the comment to reference #168 directly and rewords from "osv-scanner-on-merge_group" to "dedicated dependency-CVE check on merge_group" so the comment reflects what the followup actually needs to do (not just which tool to use). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cy-CVE check (#168) Reframe the merge_group dependency-CVE coverage from "known limitation" to explicit design decision. The runtime behavior is unchanged; the prior CRITICAL-block framing wrongly implied a coverage gap, which risked a future "fix" that wasn't needed. Workflow change (workflow-templates/python-security-analysis.yml): - actions/dependency-review-action remains pull_request-only by design. It requires PR base/head SHAs from a pull_request context and provides PR-time affordances (GPL-2.0/3.0 deny-license enforcement, PR comment summary) that apply at review time, not to speculative merge refs. - OSV-Scanner is documented as the canonical merge_group dependency-CVE check. It already runs on both pull_request and merge_group (no event_name filter on the job) and feeds the security-gate aggregator via needs:, so coverage on the queue ref is intact. - merge_group SAST and dependency coverage is therefore CodeQL, Bandit, OSV-Scanner, and OWASP Dependency-Check. dependency-review adds PR-time license policy and comment summary on top of that baseline. - Adds a one-line role marker above the osv-scanner job so its dual pull_request/merge_group responsibility is visible at the job definition, not only in the trigger block. Resolved open question on license enforcement: python-sbom.yml's license-compliance job runs on push:main only and is fail-on-forbidden-licenses: false (warn-only), so dependency-review's PR-time deny-licenses GPL-2.0/3.0 is the only fail-on license policy in the repo. Replicating it on merge_group refs is not required: license findings are PR-review policy, not exploitable speculative-merge regressions, and the queued PR's deps were already enforced at PR time. Audit trail: Also adds docs/audits/2026-05-26-issue-168-merge-group-security-handoff.md, the handoff that documents the architectural reasoning and the resolved license question. Verification path (deferred to issue #154 rollout): End-to-end validation requires merge_queue enabled on a consumer repo; confirm via gh api repos/<org>/<repo>/commits/<sha>/check-runs that the OSV Vulnerability Scanner check appears on a merge_group ref. Record that confirmation in #154, not here. Closes #168. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Apply doc-only edits to the issue #168 merge_group security handoff in response to /pr-review findings on PR #178: - Frontmatter Status changed from "Open, ready to assign" to "Resolved by PR #178 (Option 1 selected)" so the doc is internally consistent as an archived decision record. - Section 3 prefaced with a callout that cited line numbers reference the pre-PR-178 file state, with post-PR equivalents listed so a future reader can navigate without re-running grep. - Section 6 step 3 marked "(Optional, not implemented in PR #178)" and cross-referenced to the YAML role-marker comment that ships in place of the runtime echo step. Notes a follow-up issue is the path if runtime visibility becomes important. - Section 12 retitled "(resolved in PR #178)" with a new Resolution subsection capturing the GPL-2.0/3.0 license-enforcement answer from the PR body. The doc no longer poses an open question that was actually resolved in the same PR. No content removed; only clarifications, status updates, and cross-references added. Workflow template unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
williaby
force-pushed
the
claude/fix-osv-merge-group-comment-168
branch
from
d7ecaf3 to
9d77b43
Compare
PR Fix SummaryAll 4 findings from the Changes (1 commit, doc-only):
Findings addressed:
Rebase: Branch was BEHIND Verification:
Skipped (none): All 4 review findings resolved in this push. No CI failures, no SonarQube issues, no Copilot/CodeRabbit inline change requests to address. Coverage check N/A (no Python in this PR). CI re-run triggered automatically by the force-push. 🤖 Generated with Claude Code |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@workflow-templates/python-security-analysis.yml`:
- Around line 34-44: Update the comment to clarify that OSV-Scanner is the
canonical dependency-CVE check on merge_group only when the security-files gate
is true: explicitly state that osv-scanner, security-scanning, and
owasp-dependency-check are gated by needs.detect-changes.outputs.security_files
== 'true' and therefore may be skipped when that output is not 'true'; adjust
wording around "OSV-Scanner is the canonical dependency-CVE check on
merge_group" and similar language at the second location (lines ~223-226) to
condition the coverage on the security_files gate and mention that
dependency-review still runs on pull_request for PR-time license/comment
affordances.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98f042e7-66ac-4630-a288-7ef4237da70c
📒 Files selected for processing (2)
docs/audits/2026-05-26-issue-168-merge-group-security-handoff.mdworkflow-templates/python-security-analysis.yml
| # DESIGN DECISION (resolves #168): actions/dependency-review-action runs on | ||
| # pull_request only, by design. It requires PR base/head SHAs from a | ||
| # pull_request context and provides PR-time affordances (GPL-2.0/3.0 | ||
| # deny-license enforcement, PR comment summary) that apply at review time, | ||
| # not to speculative merge refs. OSV-Scanner is the canonical | ||
| # dependency-CVE check on merge_group: it already runs on both | ||
| # pull_request and merge_group (no event_name filter) and feeds the | ||
| # security-gate aggregator via needs:. merge_group coverage is therefore | ||
| # CodeQL, Bandit, OSV-Scanner, and OWASP Dependency-Check (4 SAST and | ||
| # dependency checks); dependency-review adds PR-time license policy and | ||
| # comment summary on top of that baseline. #VERIFY same procedure as |
There was a problem hiding this comment.
Clarify that merge_group dependency-CVE coverage is conditional on security_files.
These comments currently read as unconditional coverage, but osv-scanner, security-scanning, and owasp-dependency-check are all gated by needs.detect-changes.outputs.security_files == 'true'. Please update wording to reflect that OSV is the canonical merge_group dependency-CVE check when the security-files gate is true, otherwise the jobs can be skipped.
Also applies to: 223-226
🤖 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 `@workflow-templates/python-security-analysis.yml` around lines 34 - 44, Update
the comment to clarify that OSV-Scanner is the canonical dependency-CVE check on
merge_group only when the security-files gate is true: explicitly state that
osv-scanner, security-scanning, and owasp-dependency-check are gated by
needs.detect-changes.outputs.security_files == 'true' and therefore may be
skipped when that output is not 'true'; adjust wording around "OSV-Scanner is
the canonical dependency-CVE check on merge_group" and similar language at the
second location (lines ~223-226) to condition the coverage on the security_files
gate and mention that dependency-review still runs on pull_request for PR-time
license/comment affordances.
Summary
Closes #168 by reframing the merge_group dependency-CVE coverage in
workflow-templates/python-security-analysis.ymlfrom "known limitation" to explicit design decision. The runtime behavior is unchanged; the prior framing wrongly implied a coverage gap that a future maintainer might try to "fix."This branch has two commits:
b514fb0Point the CRITICAL block at the correct followup issue (Add osv-scanner dependency check on merge_group events #168 instead of "Enable GitHub merge_queue for repos with auto-merging dep bumps #154 followup").d7ecaf3Document OSV-Scanner as the canonicalmerge_groupdependency-CVE check and add a role marker above theosv-scannerjob. Closes Add osv-scanner dependency check on merge_group events #168.Architectural reframe
actions/dependency-review-actionremainspull_request-only by design. It requires PR base/head SHAs from apull_requestcontext and provides PR-time affordances (GPL-2.0/3.0 deny-license enforcement, PR comment summary) that apply at review time, not to speculative merge refs.merge_groupdependency-CVE check. Theosv-scannerjob has noevent_namefilter, so it already fires on bothpull_requestandmerge_groupand feeds thesecurity-gateaggregator vianeeds:.merge_groupSAST and dependency coverage is therefore CodeQL + Bandit + OSV-Scanner + OWASP Dependency-Check.dependency-reviewlayers PR-time license policy and comment summary on top of that baseline at PR review time.Resolved license-enforcement question
The handoff audit raised one open question: is GPL-2.0/3.0 license enforcement on
merge_groupruns a hard requirement?Answer: no.
python-sbom.yml'slicense-compliancejob isfail-on-forbidden-licenses: false(warn-only) and only runs onpush:main/release/ weekly schedule.dependency-review-action'sdeny-licenses: GPL-2.0, GPL-3.0is therefore the only fail-on license policy in the repo, and it lives at PR review time. The queued PR's deps were already enforced at that gate; replicating the check on the speculative merge ref would add no security signal because license findings are PR-review policy, not exploitable speculative-merge regressions.Verification
Local:
pre-commit run --files workflow-templates/python-security-analysis.ymlpasses (yamllint, no-em-dash, secrets, markdownlint, commitizen, etc.)-S) on both commitsdependency-securityjob'sif:is unchanged,osv-scannerjob'sif:is unchangedDeferred to #154 rollout:
merge_queueenabled on a consumer repo. Confirm viagh api repos/<org>/<repo>/commits/<sha>/check-runsthat the OSV Vulnerability Scanner check appears on amerge_groupref. Record that confirmation in Enable GitHub merge_queue for repos with auto-merging dep bumps #154, not here.Audit trail
Adds
docs/audits/2026-05-26-issue-168-merge-group-security-handoff.md, the handoff document that walks through the three candidate approaches, picks Option 1 (comment-only), and records the architectural reasoning and the license question's resolution.References
merge_group:triggers to 9 templates)Generated with Claude Code
Summary by CodeRabbit