[No QA] Fix failureNotifier linking to wrong PR when open PRs merge main#82073
Merged
roryabraham merged 20 commits intomainfrom Feb 17, 2026
Merged
[No QA] Fix failureNotifier linking to wrong PR when open PRs merge main#82073roryabraham merged 20 commits intomainfrom
roryabraham merged 20 commits intomainfrom
Conversation
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. |
Contributor
|
Sorry I missed this. Ironically it's happened to me a couple times today. Will review tomorrow |
Contributor
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
roryabraham
previously approved these changes
Feb 13, 2026
Contributor
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
Overall looks good. No blockers
…file level - Move getMergedPR and PullRequest type from separate failureNotifierUtils.ts into the main failureNotifier.ts action file - Use if (require.main === module) pattern so the action can be imported directly in tests - Disable @typescript-eslint/naming-convention at the file level instead of per-line - Delete the now-unnecessary .github/libs/failureNotifierUtils.ts Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
@neil-marcellini CI is failing |
Co-authored-by: Cursor <cursoragent@cursor.com>
roryabraham
requested changes
Feb 17, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
…ename test - Use RestEndpointMethodTypes from @octokit/plugin-rest-endpoint-methods instead of custom type definitions - Use dedent template string for the issue body - Move failureNotifier entry to alphabetical position in buildActions.sh - Rename FailureNotifierUtilsTest.ts to FailureNotifierTest.ts Co-authored-by: Cursor <cursoragent@cursor.com>
…tests Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@roryabraham this is ready for another round of review. I wasn't able to get dedent working because it couldn't find type declarations, but given that that was a non-blocking comment, I think we can just go ahead with this. |
roryabraham
reviewed
Feb 17, 2026
Contributor
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
@neil-marcellini is there some reason this didn't work?
diff --git a/.github/actions/javascript/failureNotifier/failureNotifier.ts b/.github/actions/javascript/failureNotifier/failureNotifier.ts
index fdaa21d8772..00a53f69232 100644
--- a/.github/actions/javascript/failureNotifier/failureNotifier.ts
+++ b/.github/actions/javascript/failureNotifier/failureNotifier.ts
@@ -1,6 +1,7 @@
/* eslint-disable @typescript-eslint/naming-convention */
import * as core from '@actions/core';
import * as github from '@actions/github';
+import dedent from '@libs/StringUtils/dedent';
type WorkflowRun = {
id: number;
@@ -113,21 +114,27 @@ async function run() {
}
const issueTitle = `Investigate workflow job failing on main: ${job.name}`;
- const issueBody =
- `🚨 **Failure Summary** 🚨:\n\n` +
- `- **📋 Job Name**: [${job.name}](${job.html_url})\n` +
- `- **🔧 Failure in Workflow**: Process new code merged to main\n` +
- `- **🔗 Triggered by PR**: [PR Link](${prLink})\n` +
- `- **👤 PR Author**: @${prAuthor}\n` +
- `- **🤝 Merged by**: @${prMerger}\n` +
- `- **🐛 Error Message**: \n ${errorMessage}\n\n` +
- `⚠️ **Action Required** ⚠️:\n\n` +
- `🛠️ A recent merge appears to have caused a failure in the job named [${job.name}](${job.html_url}).\n` +
- `This issue has been automatically created and labeled with \`${failureLabel}\` for investigation. \n\n` +
- `👀 **Please look into the following**:\n` +
- `1. **Why the PR caused the job to fail?**\n` +
- `2. **Address any underlying issues.**\n\n` +
- `🐛 We appreciate your help in squashing this bug!`;
+ const issueBody = dedent(`
+ 🚨 **Failure Summary** 🚨:
+
+ - **📋 Job Name**: [${job.name}](${job.html_url})
+ - **🔧 Failure in Workflow**: Process new code merged to main
+ - **🔗 Triggered by PR**: [PR Link](${prLink})
+ - **👤 PR Author**: @${prAuthor}
+ - **🤝 Merged by**: @${prMerger}
+ - **🐛 Error Message**: \n ${errorMessage}
+
+ ⚠️ **Action Required** ⚠️:
+
+ 🛠️ A recent merge appears to have caused a failure in the job named [${job.name}](${job.html_url}).
+ 🔍 This issue has been automatically created and labeled with \`${failureLabel}\` for investigation.
+
+ **👀 Please look into the following:**
+ 1. **Why the PR caused the job to fail?**
+ 2. **Address any underlying issues.**
+
+ **🐛 We appreciate your help in squashing this bug!**
+ `);
await octokit.rest.issues.create({
owner,Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Thanks for the clarification on that, Rory. Cursor thought it was the npm package of the same name, and I didn't know any better / check myself. Now it works fine. |
roryabraham
approved these changes
Feb 17, 2026
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
93 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.21-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.21-4 🚀
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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