Skip to content

Commit ecdf571

Browse files
committed
fix: Address some more issues
Signed-off-by: darshit2308 <darshit2308@gmail.com>
1 parent 34eb7b5 commit ecdf571

4 files changed

Lines changed: 26 additions & 6 deletions

File tree

.github/scripts/review-sync/helpers/labels.js

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,18 +55,36 @@ async function ensureLabel(github, owner, repo, label, dryRun) {
5555

5656
/**
5757
* Check if the latest CI runs for a given commit have any failures.
58-
* Returns true if any check run conclusion is 'failure' or 'timed_out'.
58+
*
59+
* We intentionally treat the following as failures:
60+
* - 'failure', 'timed_out' (explicit test failures)
61+
* - 'startup_failure' (e.g., invalid workflow YAML)
62+
* - 'action_required' (e.g., waiting for maintainer approval for first-time contributors)
63+
*
64+
* We intentionally EXCLUDE 'cancelled':
65+
* When a developer pushes a new commit, GitHub automatically cancels the currently
66+
* running workflows. If we treated 'cancelled' as a failure, every re-push would
67+
* instantly demote the PR to queue:junior-committer, frustrating contributors.
68+
*
69+
* @returns {boolean} true if any check run conclusion is a blocking failure.
5970
*/
6071
async function hasCIFailures(github, owner, repo, sha) {
6172
try {
62-
const { data } = await github.rest.checks.listForRef({
73+
// We MUST use paginate, otherwise it silently truncates at 30 runs.
74+
// Matrix builds often exceed 30 checks.
75+
const checkRuns = await github.paginate(github.rest.checks.listForRef, {
6376
owner,
6477
repo,
6578
ref: sha,
6679
filter: 'latest'
6780
});
68-
return data.check_runs.some(
69-
run => run.conclusion === 'failure' || run.conclusion === 'timed_out'
81+
82+
return checkRuns.some(
83+
run =>
84+
run.conclusion === 'failure' ||
85+
run.conclusion === 'timed_out' ||
86+
run.conclusion === 'startup_failure' ||
87+
run.conclusion === 'action_required'
7088
);
7189
} catch (error) {
7290
// Fail securely: do not assume CI is passing if we cannot verify it.

.github/scripts/review-sync/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
"name": "review-sync",
33
"version": "1.0.0",
44
"description": "Automated PR review queue label sync scripts and tests",
5+
"private": true,
56
"main": "index.js",
67
"scripts": {
78
"test": "node tests/test-labels.js && node tests/test-permissions.js && node tests/test-reviews.js"

.github/scripts/review-sync/tests/test-labels.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
const { runTestSuite, createMockGithub } = require('./test-utils');
99
const { determineLabel, ensureLabel, syncLabel } = require('../helpers/labels');
10-
const { QUEUE_LABELS } = require('../helpers/constants');
10+
const { QUEUE_LABELS, COMMUNITY_REVIEW } = require('../helpers/constants');
1111

1212
const unitTests = [
1313
{
@@ -167,7 +167,7 @@ const unitTests = [
167167
const mock = createMockGithub({ roles: {}, reviews: [] });
168168
const pr = { number: 1, labels: [{ name: 'queue:junior-committer' }], head: { sha: '123' }, user: { type: 'User' } };
169169
const changed = await syncLabel(mock, 'o', 'r', pr, false);
170-
return changed === true && mock.calls.labelsAdded.includes('open to community review');
170+
return changed === true && mock.calls.labelsAdded.includes(COMMUNITY_REVIEW.name);
171171
},
172172
},
173173
];

.github/workflows/test-review-sync.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ on:
1212

1313
jobs:
1414
test:
15+
if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.fork == false
1516
runs-on: hl-sdk-py-lin-md
1617
permissions:
1718
contents: read

0 commit comments

Comments
 (0)