[No QA] Fix failureNotifier linking to wrong PR when open PRs merge main #82073
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The failureNotifier workflow uses listPullRequestsAssociatedWithCommit to find which PR caused a failure on main. However, this API returns ALL PRs containing the commit, including open PRs that have merged main into their feature branch. Taking data[0] could pick an unmerged PR, creating misleading workflow failure issues. Fix: Filter for PRs that are actually merged into the target branch before falling back to the first result. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename html_url/merged_at to htmlUrl/mergedAt in TS type and tests - Use .at(0) instead of [0] per prefer-at rule - Replace real usernames with generic test-user in test data Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 169cbeccc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Convert utility from TS to JS (CommonJS) so the workflow can require() it directly. Update test to import the JS module and use GitHub API field names (merged_at, base.ref) to match what the workflow passes through from the API response. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
- Fix eslint-disable rule: no-relative-import -> no-relative-parent-imports - Add no-unsafe-assignment to eslint-disable for require() - Use context.payload.workflow_run.head_branch instead of YAML interpolation to avoid script injection from branch names containing quotes (addresses Codex review) Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
(Neil's AI agent) Good catch on the YAML interpolation injection risk. Fixed in 99bbffc — now using |
Codecov Report✅ All modified and coverable lines are covered by tests. |
The typecheck CI rejects new .js files in .github/libs/. Convert the utility to .ts for clean test imports, and inline the same logic in the workflow (which can only run raw JS). The TS module serves as the tested reference implementation, with a comment in the workflow pointing to it. Co-authored-by: Cursor <cursoragent@cursor.com>
Add eslint-disable for naming-convention on the PullRequest type (matching GitHub API field names). Run Prettier on test file. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract the inline github-script logic into a proper custom action at .github/actions/javascript/failureNotifier/ that: - Imports and uses getMergedPR from .github/libs/failureNotifierUtils.ts - Is compiled via ncc into index.js (same pattern as all other actions) - The workflow now uses the custom action instead of inline scripts - The tested utility is the SAME code the action actually executes This ensures the unit tests in FailureNotifierUtilsTest.ts are testing the real logic used in production. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add WorkflowRun type to fix unsafe-any TypeScript errors from github.context.payload.workflow_run - Rename annotations/annotation to checkResults/checkResult to avoid triggering the no-negated-variables rule (matches "not" substring) - Rebuild index.js Co-authored-by: Cursor <cursoragent@cursor.com>
The optional chain on head_commit?.id produces string | undefined, but commit_sha requires string. Default to empty string. Co-authored-by: Cursor <cursoragent@cursor.com>
- Handle missing head_commit by early-returning instead of defaulting to empty string (fixes no-default-id-values ESLint rule) - Use immutable SHA for actions/checkout (fixes validateImmutableActionRefs) - Rebuild compiled index.js bundle Co-authored-by: Cursor <cursoragent@cursor.com>
The local ncc build produced slightly different output for template literals. Rebuilding via the official script ensures the bundle matches what the verify CI job expects. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
No C+ needed. Requesting a review from @roryabraham because I think he has experience with GH actions and I don't have a ton of experience with them myself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Problem: The
failureNotifier.ymlworkflow creates GitHub issues when CI fails on main, but it sometimes links to the wrong PR. This happens because the GitHub APIlistPullRequestsAssociatedWithCommitreturns ALL PRs containing a commit — including open PRs that have merged main into their feature branch. The old code tookdata[0](the first result), which could be an unrelated open PR.Example: Issue #82067 was incorrectly attributed to an open, unmerged PR.
Fix: Added a
getMergedPR()utility that filters the API results to find the PR that was actually merged (merged_at !== null) into the target branch (base.ref === targetBranch), falling back to the first result only if no merged PR is found.Refactor: Converted the workflow from inline
github-scriptJavaScript steps to a proper custom GitHub Action (failureNotifier.ts), following the same pattern as all other JS actions in.github/actions/javascript/. This makes the logic testable and lintable. The core filtering logic lives in a separate utility file (.github/libs/failureNotifierUtils.ts) with unit tests.Fixed Issues
$ #82067
Tests
npx jest tests/unit/FailureNotifierUtilsTest.ts— all 5 tests pass:Offline tests
N/A — CI workflow change only
QA Steps
[No QA] — this is an internal CI workflow fix with no user-facing impact.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A — CI workflow fix, no UI changes
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari