mirror of
https://github.com/comfyanonymous/ComfyUI.git
synced 2026-06-01 20:07:37 +08:00
Address review feedback: merged_by, idempotency, null guards
- Fetch full PR via pulls.get() to get merged_by (not in simple schema) - Add idempotency check before issue creation to prevent duplicates - Use SHA-scoped concurrency group to allow parallel independent runs - Guard c.user null for deleted GitHub accounts - Retry issue creation without assignee on 422 - Align policy text: "3 business days" → "3 days" to match implementation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
09403bd734
commit
e40cc8f286
54
.github/workflows/detect-unreviewed-merge.yml
vendored
54
.github/workflows/detect-unreviewed-merge.yml
vendored
@ -5,7 +5,7 @@ on:
|
|||||||
branches: [master]
|
branches: [master]
|
||||||
|
|
||||||
concurrency:
|
concurrency:
|
||||||
group: detect-unreviewed-merge
|
group: detect-unreviewed-merge-${{ github.sha }}
|
||||||
cancel-in-progress: false
|
cancel-in-progress: false
|
||||||
|
|
||||||
permissions:
|
permissions:
|
||||||
@ -41,6 +41,13 @@ jobs:
|
|||||||
|
|
||||||
core.info(`Found PR #${pr.number}: ${pr.title}`);
|
core.info(`Found PR #${pr.number}: ${pr.title}`);
|
||||||
|
|
||||||
|
// Fetch full PR to get merged_by (not in pull-request-simple schema)
|
||||||
|
const { data: fullPr } = await github.rest.pulls.get({
|
||||||
|
owner,
|
||||||
|
repo,
|
||||||
|
pull_number: pr.number,
|
||||||
|
});
|
||||||
|
|
||||||
// Check for approving reviews using latest review per reviewer.
|
// Check for approving reviews using latest review per reviewer.
|
||||||
// DISMISSED approvals (from "dismiss stale reviews on new commits")
|
// DISMISSED approvals (from "dismiss stale reviews on new commits")
|
||||||
// are intentionally NOT counted — OSS PRs require current approval.
|
// are intentionally NOT counted — OSS PRs require current approval.
|
||||||
@ -86,7 +93,7 @@ jobs:
|
|||||||
});
|
});
|
||||||
|
|
||||||
for (const c of comments) {
|
for (const c of comments) {
|
||||||
if (c.user.login === pr.user.login) {
|
if (c.user && pr.user && c.user.login === pr.user.login) {
|
||||||
const match = c.body.match(pattern);
|
const match = c.body.match(pattern);
|
||||||
if (match) {
|
if (match) {
|
||||||
justification = match[1].trim();
|
justification = match[1].trim();
|
||||||
@ -102,7 +109,7 @@ jobs:
|
|||||||
: 'No inline justification found.');
|
: 'No inline justification found.');
|
||||||
|
|
||||||
// Build issue body
|
// Build issue body
|
||||||
const mergedBy = pr.merged_by ? pr.merged_by.login : 'unknown';
|
const mergedBy = fullPr.merged_by ? fullPr.merged_by.login : 'unknown';
|
||||||
const table = [
|
const table = [
|
||||||
'## Unreviewed Merge Detected',
|
'## Unreviewed Merge Detected',
|
||||||
'',
|
'',
|
||||||
@ -121,7 +128,7 @@ jobs:
|
|||||||
'',
|
'',
|
||||||
'Per the Secure Development Policy (Emergency Change Procedures), changes merged',
|
'Per the Secure Development Policy (Emergency Change Procedures), changes merged',
|
||||||
'without prior approval must be justified and peer-reviewed within 1 business day.',
|
'without prior approval must be justified and peer-reviewed within 1 business day.',
|
||||||
'Unresolved items older than 3 business days are escalated to engineering leadership.',
|
'Unresolved items older than 3 days are escalated to engineering leadership.',
|
||||||
];
|
];
|
||||||
|
|
||||||
let body;
|
let body;
|
||||||
@ -169,13 +176,38 @@ jobs:
|
|||||||
const repoLabel = `repo:${repo.toLowerCase()}`;
|
const repoLabel = `repo:${repo.toLowerCase()}`;
|
||||||
const statusLabel = hasJustification ? 'needs-review' : 'needs-justification';
|
const statusLabel = hasJustification ? 'needs-review' : 'needs-justification';
|
||||||
|
|
||||||
const issue = await tracking.rest.issues.create({
|
// Idempotency check — avoid duplicate issues on workflow re-runs
|
||||||
owner: 'Comfy-Org',
|
const { data: search } = await tracking.rest.search.issuesAndPullRequests({
|
||||||
repo: 'unreviewed-merges',
|
q: `repo:Comfy-Org/unreviewed-merges in:title "PR #${pr.number}" label:${repoLabel}`,
|
||||||
title: `[${repo}] PR #${pr.number} — ${pr.title}`,
|
|
||||||
body,
|
|
||||||
labels: ['unreviewed-merge', statusLabel, repoLabel],
|
|
||||||
assignees: [pr.user.login],
|
|
||||||
});
|
});
|
||||||
|
if (search.total_count > 0) {
|
||||||
|
core.info(`Tracking issue already exists: ${search.items[0].html_url}`);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
let issue;
|
||||||
|
try {
|
||||||
|
issue = await tracking.rest.issues.create({
|
||||||
|
owner: 'Comfy-Org',
|
||||||
|
repo: 'unreviewed-merges',
|
||||||
|
title: `[${repo}] PR #${pr.number} — ${pr.title}`,
|
||||||
|
body,
|
||||||
|
labels: ['unreviewed-merge', statusLabel, repoLabel],
|
||||||
|
assignees: [pr.user.login],
|
||||||
|
});
|
||||||
|
} catch (e) {
|
||||||
|
if (e.status === 422) {
|
||||||
|
core.warning(`Retrying without assignee: ${e.message}`);
|
||||||
|
issue = await tracking.rest.issues.create({
|
||||||
|
owner: 'Comfy-Org',
|
||||||
|
repo: 'unreviewed-merges',
|
||||||
|
title: `[${repo}] PR #${pr.number} — ${pr.title}`,
|
||||||
|
body,
|
||||||
|
labels: ['unreviewed-merge', statusLabel, repoLabel],
|
||||||
|
});
|
||||||
|
} else {
|
||||||
|
throw e;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
core.info(`Created tracking issue: ${issue.data.html_url}`);
|
core.info(`Created tracking issue: ${issue.data.html_url}`);
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user