Skip to content

fix(ng-dev/pr): require enforced statuses to be passing, not just present #3975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ParthShethia25 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ng-dev/pr): require enforced statuses to be passing, not just present #3975

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions ng-dev/pr/common/validation/assert-enforced-statuses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@
*/

import {PullRequestConfig} from '../../config/index.js';
import {getStatusesForPullRequest, PullRequestFromGithub} from '../fetch-pull-request.js';
import {
getStatusesForPullRequest,
PullRequestFromGithub,
PullRequestStatus,
} from '../fetch-pull-request.js';
import {createPullRequestValidation, PullRequestValidation} from './validation-config.js';

/** Assert the pull request has passing enforced statuses. */
Expand All @@ -25,10 +29,15 @@ class Validation extends PullRequestValidation {

const {statuses} = getStatusesForPullRequest(pullRequest);
const missing: string[] = [];
const notPassing: string[] = [];

for (const enforced of config.requiredStatuses) {
if (!statuses.some((s) => s.name === enforced.name && s.type === enforced.type)) {
const status = statuses.find((s) => s.name === enforced.name && s.type === enforced.type);

@gemini-code-assist gemini-code-assist Bot Sep 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since config.requiredStatuses is defined as optional in PullRequestConfig, it can be undefined if not explicitly configured. Iterating over it directly in the for...of loop on line 34 will throw a runtime TypeError (e.g., Cannot read properties of undefined).\n\nTo prevent this, consider safeguarding the loop by defaulting to an empty array:\n\ntypescript\nfor (const enforced of config.requiredStatuses ?? []) {\n

@ParthShethia25 ParthShethia25 Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already an early return for this a few lines up (line 26): if (config.requiredStatuses === undefined) { return; }. The loop isn't reachable when it's undefined, and TypeScript narrows the type there as well, so ?? [] would be dead code. Leaving it as is.


if (status === undefined) {
missing.push(enforced.name);
} else if (status.status !== PullRequestStatus.PASSING) {
notPassing.push(enforced.name);
}
}

Expand All @@ -37,5 +46,11 @@ class Validation extends PullRequestValidation {
`Required statuses are missing on the pull request (${missing.join(', ')}).`,
);
}

if (notPassing.length > 0) {
throw this._createError(
`Required statuses are not passing on the pull request (${notPassing.join(', ')}).`,
);
}
}
}