Skip to content

Commit ccb988f

Browse files
committed
fix: address review feedback on revision-guard draft conversion flow
- fix: add defensive guard on context.payload and context.repo before dereferencing (CodeRabbit major) - fix: parseManagedLabels now merges custom labels with DEFAULT_MANAGED_LABELS instead of silently discarding defaults (darshit2308 blocking) - test: add graphqlCalls assertion in configurable-labels test to catch draft-conversion regressions (CodeRabbit trivial) - fix: checkout uses trusted base SHA instead of PR head/merge ref to prevent fork PRs from running untrusted code with write-scoped token (CodeRabbit critical) Signed-off-by: Gourav NSS <gourav341111@gmail.com>
1 parent c537a55 commit ccb988f

4 files changed

Lines changed: 30 additions & 8 deletions

File tree

.github/scripts/revision-guard/helpers/constants.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,15 @@ function parseManagedLabels(value) {
1212
return [...DEFAULT_MANAGED_LABELS];
1313
}
1414

15-
return value
15+
const customLabels = value
1616
.split(',')
1717
.map(label => label.trim())
1818
.filter(Boolean);
19+
20+
// Merge custom labels with defaults so that setting REVISION_GUARD_MANAGED_LABELS
21+
// adds to the managed set rather than silently discarding the default labels.
22+
const merged = new Set([...DEFAULT_MANAGED_LABELS, ...customLabels]);
23+
return [...merged];
1924
}
2025

2126
module.exports = {

.github/scripts/revision-guard/index.js

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,21 @@ const {
77
} = require('./helpers');
88

99
module.exports = async function revisionGuard({ github, context, core }) {
10-
const reviewState = context.payload.review?.state;
11-
const pr = context.payload.pull_request;
10+
const payload = context?.payload;
11+
const repo = context?.repo;
12+
const reviewState = payload?.review?.state;
13+
const pr = payload?.pull_request;
1214

13-
if (!pr || reviewState !== 'changes_requested') {
15+
if (
16+
!payload ||
17+
!repo?.owner ||
18+
!repo?.repo ||
19+
!pr ||
20+
typeof pr.number !== 'number' ||
21+
!pr.node_id ||
22+
reviewState !== 'changes_requested'
23+
) {
24+
core?.info?.('Skipping revision guard due to missing or non-matching payload data.');
1425
return;
1526
}
1627

@@ -34,8 +45,8 @@ module.exports = async function revisionGuard({ github, context, core }) {
3445
}
3546

3647
await removeManagedLabels(github, {
37-
owner: context.repo.owner,
38-
repo: context.repo.repo,
48+
owner: repo.owner,
49+
repo: repo.repo,
3950
issueNumber: pr.number,
4051
labels: labelsToRemove,
4152
});

.github/scripts/revision-guard/index.test.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ describe('revision-guard index', () => {
116116
assert.equal(github.removedLabels.length, 0);
117117
});
118118

119-
it('uses configurable managed labels', async () => {
119+
it('uses configurable managed labels and still removes defaults', async () => {
120120
process.env.REVISION_GUARD_MANAGED_LABELS = 'custom: one, custom: two';
121121
const handler = freshRequire();
122122
const github = createGithubMock();
@@ -136,6 +136,9 @@ describe('revision-guard index', () => {
136136

137137
await handler({ github, context, core: { info() {} } });
138138

139-
assert.deepEqual(github.removedLabels, ['custom: one', 'custom: two']);
139+
// Draft conversion must also fire for configurable-label scenarios.
140+
assert.deepEqual(github.graphqlCalls, [{ pullRequestId: 'PR_node_45' }]);
141+
// Custom labels AND the matching default (queue:committers) must both be removed.
142+
assert.deepEqual(github.removedLabels, ['queue:committers', 'custom: one', 'custom: two']);
140143
});
141144
});

.github/workflows/revision-guard.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ jobs:
2727
- name: Checkout repository
2828
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
2929
with:
30+
# Use trusted base commit, not PR head/merge ref, to prevent fork PRs
31+
# from executing untrusted changes with a write-scoped token.
32+
ref: ${{ github.event.pull_request.base.sha }}
3033
sparse-checkout: .github/scripts
3134

3235
- name: Handle changes requested

0 commit comments

Comments
 (0)