Skip to content

Commit 80fb159

Browse files
roomote[bot]edelauna
roomote[bot]
and
edelauna
authored
ci: improve PR label reconciliation with CI gating and event triggers (#228)
* feat(label-pr-review-state): updating logic for awaiting-review * feat(label-pr-review-state): check only required steps --------- Co-authored-by: Elliott de Launay <edelauna@gmail.com>
1 parent 8849f1a commit 80fb159

1 file changed

Lines changed: 169 additions & 41 deletions

File tree

‎.github/workflows/label-pr-review-state.yml‎

Lines changed: 169 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,16 @@ name: Label PR review state
22

33
on:
44
schedule:
5-
- cron: '0 * * * *' # hourly
5+
- cron: '0 * * * *' # hourly fallback
66
workflow_dispatch:
7+
pull_request:
8+
types: [opened, reopened, ready_for_review, synchronize, review_requested]
9+
pull_request_review:
10+
types: [submitted, dismissed]
711

812
permissions:
913
pull-requests: write
14+
checks: read
1015

1116
concurrency:
1217
group: label-pr-review-state
@@ -22,70 +27,193 @@ jobs:
2227
script: |
2328
const { owner, repo } = context.repo;
2429
const stateLabels = ['awaiting-author', 'awaiting-review'];
25-
const failures = [];
2630
27-
const prs = await github.paginate(github.rest.pulls.list, {
28-
owner, repo, state: 'open', per_page: 100,
29-
});
31+
// When triggered by a single PR event, only reconcile that PR.
32+
// The hourly schedule and workflow_dispatch reconcile all open PRs.
33+
let prs;
34+
const prNumber = context.payload.pull_request?.number;
35+
if (prNumber) {
36+
const { data: pr } = await github.rest.pulls.get({
37+
owner, repo, pull_number: prNumber,
38+
});
39+
prs = [pr];
40+
} else {
41+
prs = await github.paginate(github.rest.pulls.list, {
42+
owner, repo, state: 'open', per_page: 100,
43+
});
44+
}
45+
46+
// Strips stateLabels from a PR, optionally keeping one.
47+
// Also removes stale-awaiting-author when not keeping awaiting-author.
48+
async function reconcileLabels(pr, desiredLabel) {
49+
const currentLabels = new Set(pr.labels.map(l => l.name));
50+
for (const label of stateLabels) {
51+
if (label !== desiredLabel && currentLabels.has(label)) {
52+
try {
53+
await github.rest.issues.removeLabel({
54+
owner, repo, issue_number: pr.number, name: label,
55+
});
56+
} catch (err) {
57+
if (err.status !== 404) throw err; // 404 = already gone, benign
58+
}
59+
}
60+
}
61+
if (desiredLabel && !currentLabels.has(desiredLabel)) {
62+
await github.rest.issues.addLabels({
63+
owner, repo, issue_number: pr.number, labels: [desiredLabel],
64+
});
65+
}
66+
if (desiredLabel !== 'awaiting-author' && currentLabels.has('stale-awaiting-author')) {
67+
try {
68+
await github.rest.issues.removeLabel({
69+
owner, repo, issue_number: pr.number, name: 'stale-awaiting-author',
70+
});
71+
} catch (err) {
72+
if (err.status !== 404) throw err;
73+
}
74+
}
75+
}
76+
77+
// Fetch required status check names from the branch ruleset.
78+
// Uses the public /rules/branches endpoint — no admin token needed.
79+
// Falls back to blocking on all checks if the endpoint is unavailable.
80+
let requiredCheckNames = null;
81+
try {
82+
const { data: rules } = await github.request(
83+
'GET /repos/{owner}/{repo}/rules/branches/{branch}',
84+
{ owner, repo, branch: 'main' },
85+
);
86+
const statusRule = rules.find(r => r.type === 'required_status_checks');
87+
if (statusRule) {
88+
requiredCheckNames = new Set(
89+
statusRule.parameters.required_status_checks.map(c => c.context),
90+
);
91+
core.info(`Required checks: ${[...requiredCheckNames].join(', ')}`);
92+
}
93+
} catch (err) {
94+
core.warning(`Could not fetch branch rules, falling back to all checks: ${err.message}`);
95+
}
96+
97+
const failures = [];
3098
3199
for (const pr of prs) {
32100
try {
101+
// Draft PRs never get a state label.
102+
if (pr.draft) {
103+
core.info(`PR #${pr.number}: draft — stripping state labels`);
104+
await reconcileLabels(pr, null);
105+
continue;
106+
}
107+
108+
// Check CI status for required checks on the PR's head commit only.
109+
// Scoping to required checks avoids advisory checks (e.g. codecov/patch)
110+
// incorrectly blocking label assignment on otherwise-ready PRs.
111+
const [checkRuns, commitStatusRes] = await Promise.all([
112+
github.paginate(github.rest.checks.listForRef, {
113+
owner, repo, ref: pr.head.sha, per_page: 100,
114+
}),
115+
github.rest.repos.getCombinedStatusForRef({
116+
owner, repo, ref: pr.head.sha,
117+
}),
118+
]);
119+
120+
// Filter to required checks only (or all checks if rules unavailable).
121+
// Always exclude this workflow's own run to avoid self-referential loops.
122+
const relevantRuns = checkRuns.filter(run => {
123+
if (run.name === 'Reconcile PR review state labels') return false;
124+
return requiredCheckNames ? requiredCheckNames.has(run.name) : true;
125+
});
126+
127+
// For commit statuses (external CIs), there's no per-status name filtering
128+
// available from getCombinedStatusForRef — it aggregates all statuses.
129+
// If required checks are known, we only use commitStatus as a signal when
130+
// no required check runs exist for this ref (i.e. pure status-based CI).
131+
const useCommitStatus = !requiredCheckNames || relevantRuns.length === 0;
132+
133+
core.debug(`PR #${pr.number}: ${relevantRuns.length} required check run(s), commit status=${commitStatusRes.data.state} (used=${useCommitStatus})`);
134+
for (const run of relevantRuns) {
135+
core.debug(` check: "${run.name}" status=${run.status} conclusion=${run.conclusion}`);
136+
}
137+
138+
const ciPending = relevantRuns.some(
139+
run => run.status === 'queued' || run.status === 'in_progress',
140+
) || (useCommitStatus && commitStatusRes.data.state === 'pending');
141+
142+
const ciFailed = !ciPending && (
143+
relevantRuns.some(
144+
run => run.status === 'completed' &&
145+
run.conclusion !== 'success' &&
146+
run.conclusion !== 'skipped' &&
147+
run.conclusion !== 'neutral',
148+
) || (useCommitStatus && (
149+
commitStatusRes.data.state === 'failure' ||
150+
commitStatusRes.data.state === 'error'
151+
))
152+
);
153+
154+
// While CI is running or has failed, remove state labels and move on.
155+
// CI failure is its own signal; the label would add noise, not clarity.
156+
if (ciPending || ciFailed) {
157+
core.info(`PR #${pr.number}: CI ${ciPending ? 'pending' : 'failed'} — stripping state labels`);
158+
await reconcileLabels(pr, null);
159+
continue;
160+
}
161+
162+
// CI is passing. Now determine review state.
33163
const reviews = await github.paginate(github.rest.pulls.listReviews, {
34164
owner, repo, pull_number: pr.number, per_page: 100,
35165
});
36166
37-
// Reviews are returned chronologically, so later entries replace
38-
// each reviewer's earlier decision.
167+
// Reduce to each reviewer's latest meaningful state.
168+
// Reviews are returned oldest-first, so last-write-wins yields the latest state.
169+
// COMMENTED and DISMISSED are treated as neutral — they do not
170+
// block the PR or indicate the author needs to act.
39171
const latest = new Map();
40172
for (const r of reviews) {
41-
if (r.state !== 'COMMENTED') {
173+
if (r.state !== 'COMMENTED' && r.state !== 'DISMISSED') {
42174
latest.set(r.user.login, r);
43175
}
44176
}
45177
46-
const changeRequestReviewers = [...latest.entries()]
47-
.filter(([, review]) => review.state === 'CHANGES_REQUESTED')
48-
.map(([login]) => login);
49178
const requestedReviewers = new Set(
50-
pr.requested_reviewers.map(reviewer => reviewer.login),
179+
pr.requested_reviewers.map(r => r.login),
51180
);
52181
53-
let desiredLabel = null;
54-
if (changeRequestReviewers.length > 0) {
55-
desiredLabel = changeRequestReviewers.every(
56-
reviewer => requestedReviewers.has(reviewer),
57-
)
182+
const changeRequesters = [...latest.entries()]
183+
.filter(([, r]) => r.state === 'CHANGES_REQUESTED')
184+
.map(([login]) => login);
185+
186+
let desiredLabel;
187+
if (changeRequesters.length > 0) {
188+
// If every change-requester has been re-requested for review,
189+
// the author has addressed feedback and re-opened it for review.
190+
desiredLabel = changeRequesters.every(login => requestedReviewers.has(login))
58191
? 'awaiting-review'
59192
: 'awaiting-author';
60-
}
193+
} else {
194+
// No outstanding change requests: awaiting first review, or all approved.
195+
// awaiting-review if: there are pending requested reviewers, or nobody
196+
// has given a meaningful review yet. null (approved) if everyone approved.
197+
const allApproved = latest.size > 0 &&
198+
[...latest.values()].every(r => r.state === 'APPROVED') &&
199+
requestedReviewers.size === 0;
61200
62-
const currentLabels = new Set(pr.labels.map(label => label.name));
63-
for (const label of stateLabels) {
64-
if (label !== desiredLabel && currentLabels.has(label)) {
65-
await github.rest.issues.removeLabel({
66-
owner, repo, issue_number: pr.number, name: label,
67-
});
68-
}
201+
desiredLabel = allApproved ? null : 'awaiting-review';
69202
}
70203
71-
if (desiredLabel && !currentLabels.has(desiredLabel)) {
72-
await github.rest.issues.addLabels({
73-
owner, repo, issue_number: pr.number, labels: [desiredLabel],
74-
});
75-
}
204+
core.info(
205+
`PR #${pr.number}: CI passing, reviews=${latest.size}, ` +
206+
`changeRequesters=[${changeRequesters.join(',')}], ` +
207+
`requestedReviewers=[${[...requestedReviewers].join(',')}] → ${desiredLabel ?? '(none)'}`
208+
);
76209
77-
if (
78-
desiredLabel !== 'awaiting-author' &&
79-
currentLabels.has('stale-awaiting-author')
80-
) {
81-
await github.rest.issues.removeLabel({
82-
owner, repo, issue_number: pr.number,
83-
name: 'stale-awaiting-author',
84-
});
85-
}
210+
await reconcileLabels(pr, desiredLabel);
86211
} catch (error) {
87-
failures.push(`#${pr.number}: ${error.message}`);
88-
core.error(`Failed to reconcile PR #${pr.number}: ${error.message}`);
212+
const detail = error.status
213+
? `${error.message} (HTTP ${error.status}${error.response?.data?.message ? `: ${error.response.data.message}` : ''})`
214+
: error.message;
215+
failures.push(`#${pr.number}: ${detail}`);
216+
core.error(`Failed to reconcile PR #${pr.number}: ${detail}`);
89217
}
90218
}
91219

0 commit comments

Comments
 (0)