Add unreviewed merge detector for SOC 2 compliance#14146
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow named "Detect Unreviewed Merge" that triggers on pushes to master, sets SHA-based concurrency, and defines a single job which calls the reusable workflow at Comfy-Org/github-workflows with input 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 @.github/workflows/detect-unreviewed-merge.yml:
- Around line 3-5: The workflow is hardcoded to trigger on branch "main" so it
never runs for this repo's target branch; update all branch filters from "main"
to "master" (specifically the on: push: branches array near the top and every
other occurrence of branches: [main] / pull_request branch filters found later
in the file, including the sections around the markers mentioned in the review)
so the detector triggers for merges into master.
- Around line 21-35: The current logic only checks the single tip SHA
(context.sha) when calling
github.rest.repos.listPullRequestsAssociatedWithCommit, which misses merged PRs
referenced by earlier commits in a push; change the code to iterate over
github.event.commits (or github.event.head_commit when appropriate), and for
each commit SHA call listPullRequestsAssociatedWithCommit, then apply the same
filtering (prs.find(p => p.merged_at && p.base.ref === 'main')) to detect any
merged PRs in the push; stop or aggregate results as needed once a matching PR
is found. Ensure you still reference context.repo.owner/repo when calling
listPullRequestsAssociatedWithCommit.
- Around line 39-49: The current check uses reviews.some(r => r.state ===
'APPROVED') which can be true for historical/dismissed approvals; instead
determine the effective current approval by either querying the aggregated
GraphQL field pullRequest.reviewDecision or computing the latest non-dismissed
review per reviewer from the list returned by github.rest.pulls.listReviews:
after github.paginate(github.rest.pulls.listReviews...) build a map keyed by
reviewer (e.g., r.user.login) keeping only the review with the latest
submitted_at and ignoring state 'DISMISSED' or superseded entries, then check if
any remaining latest review.state === 'APPROVED'; update the conditional to use
that result (or replace with a GraphQL call for pullRequest.reviewDecision) and
remove the old reviews.some(...) check.
🪄 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: Pro
Run ID: 69bdeddf-ec9b-487b-ae38-060147f2ba3f
📒 Files selected for processing (1)
.github/workflows/detect-unreviewed-merge.yml
luke-mino-altherr
left a comment
luke-mino-altherr
left a comment
There was a problem hiding this comment.
Review: Detector workflow
This is one of three near-identical detector copies. Full review is on the central repo PR (Comfy-Org/unreviewed-merges#1) — three findings carry over to this file:
🔴 pr.merged_by will always be null here
listPullRequestsAssociatedWithCommit returns the pull-request-simple schema, which omits merged_by (confirmed against a recent commit's API response). The "Merged by" row in every tracking issue will say "unknown."
Fix: after locating the PR, await github.rest.pulls.get({ owner, repo, pull_number: pr.number }) and use merged_by from that.
🔴 concurrency block does not prevent duplicate issues
The constant group key with cancel-in-progress: false just serializes runs — re-running this workflow on the same SHA will create a duplicate tracking issue. Add an idempotency check (search the tracking repo for an existing issue referencing PR #${pr.number} with the right repo: label before creating).
🟠 c.user is null for deleted accounts
if (c.user.login === pr.user.login) will throw if a commenter's GitHub account was deleted. Guard with if (c.user && c.user.login === pr.user.login).
Diff vs reference (sanity check)
This file should be byte-identical to detector/detect-unreviewed-merge.yml in the central repo, modulo the branch trigger and (for cloud) the approval logic. Verified ✅:
- ComfyUI:
branches: [master]only (single-branch repo). Approval logic identical to OSS reference. - ComfyUI_frontend: byte-identical to the central reference.
- Cloud:
branches: [main, master], plus simplereviews.some(r => r.state === 'APPROVED')since stale-dismissal is not enabled. Correct for the private-repo context.
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 @.github/workflows/detect-unreviewed-merge.yml:
- Around line 95-97: Cache the PR author's login once (e.g., const prAuthorLogin
= pr.user?.login) and use that cached value everywhere you currently dereference
pr.user.login (including inside the comment loop using c.user/login comparison
and when composing the tracking issue/issue payload), fall back to a neutral
display name when prAuthorLogin is undefined, and only include assignees in the
issue payload when prAuthorLogin is truthy; update all occurrences referenced in
the comment (the comment-processing loop variables c and pr, the tracking-issue
composition logic, and any assignees array construction) to use prAuthorLogin to
avoid throwing for deleted/ghosted users.
🪄 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: Pro
Run ID: ceea8b20-92f9-44cf-a60e-09326e0cefab
📒 Files selected for processing (1)
.github/workflows/detect-unreviewed-merge.yml
luke-mino-altherr
left a comment
luke-mino-altherr
left a comment
There was a problem hiding this comment.
Re-review — feedback addressed ✅
All three findings from the previous review on this file are resolved:
| Finding | Status |
|---|---|
🔴 pr.merged_by always null |
✅ Added pulls.get to fetch the full PR; issue body now uses fullPr.merged_by |
🔴 concurrency did not dedupe |
✅ Group is now detect-unreviewed-merge-${{ github.sha }} (parallel commits, dedupe re-runs) plus an explicit idempotency search before issues.create |
🟠 c.user null on deleted accounts |
✅ Guarded: if (c.user && pr.user && c.user.login === pr.user.login) |
Plus a couple of bonus hardening fixes carried over from the central PR review:
- ✅
issues.create422 fallback retries withoutassignees(handles deleted/suspended PR authors). - ✅ Policy text in the issue body aligned to "3 days" (matches the calendar-day implementation in the escalation cron).
Branch trigger and approval logic verified for this repo:
- ComfyUI #14146:
branches: [master]only; latest-per-reviewer approval logic (correct for OSS with stale-dismissal). - ComfyUI_frontend #12497:
branches: [main, master]; latest-per-reviewer approval logic (correct for OSS with stale-dismissal). - cloud #3906:
branches: [main, master]; simplereviews.some(r => r.state === 'APPROVED')(correct for private repo without stale-dismissal).
Ready for merge from my side.
luke-mino-altherr
requested review from
Kosinkadink,
alexisrolland,
guill,
kijai and
rattus128
as code owners
luke-mino-altherr
force-pushed
the
unreviewed-merge-tracking
branch
4 times, most recently
from
7a23a10 to
77d71ec
Compare
luke-mino-altherr
force-pushed
the
unreviewed-merge-tracking
branch
from
77d71ec to
6238a31
Compare
3 participants
Summary
mainwithout an approving reviewComfy-Org/unreviewed-merges(private) for SOC 2 audit purposesJustification: <reason>in PR body or commentsHow it works
Triggers on
pushtomain. Uses the GitHub API to find the associated PR and check for approving reviews. If none found, creates a tracking issue with theunreviewed-mergelabel. No code checkout required — API calls only.Test plan
unreviewed-mergesrepo🤖 Generated with Claude Code