[NoQA] Fix React Compiler compliance check blocked in reviewer#85067
Conversation
Only the command changes from npx to npm run; the single-line parsing instruction was already clear enough for the reviewer agent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kacper-mikolajczak
changed the title
![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: f4853b15dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
.github/workflows/claude-review.yml
Outdated
| claude_args: | | ||
| --model claude-opus-4-6 | ||
| --allowedTools "Task,Glob,Grep,Read,Bash(gh pr diff:*),Bash(gh pr view:*)" --json-schema '${{ steps.schema.outputs.json }}' | ||
| --allowedTools "Task,Glob,Grep,Read,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(npm run react-compiler-compliance-check:*)" --json-schema '${{ steps.schema.outputs.json }}' |
There was a problem hiding this comment.
Adding Bash(npm run react-compiler-compliance-check:*) in the pull_request_target review job lets the agent execute a script that shells out with interpolated input (execSync(\npx react-compiler-healthcheck ${srcArg} --verbose`)inscripts/react-compiler-compliance-check.ts`), so a PR-controlled filepath containing shell substitution can trigger arbitrary command execution when the reviewer runs the recommended check command. This capability was previously blocked by the tool allowlist, so enabling it here introduces a new high-impact injection path unless file paths are strictly escaped or passed as argv arrays.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@kacper-mikolajczak @adhorodyski I feel like this is a real problem as anyone could update the contents of the script, right?
There was a problem hiding this comment.
I feel like this is a real problem as anyone could update the contents of the script, right?
CI workflow checks out and executes scripts directly from the main branch, so potential bad actor can't simply change the script contents (otherwise our CI wouldn't be secure at all).
However, the Codex remark is about potential shell execution crafted with uncontrolled file paths. Following what we have in other scripts, I added a script wrapper in order to check for vulnerable characters in file paths. I would like to ask you @mountiny to tag anyone you'd like to perform a cross-check with, so we increase our confidence before we push this forward.
Also kindly asking @adhorodyski for review, too ❤️
There was a problem hiding this comment.
Thanks! @roryabraham @AndrewGable Mind giving this one a look too please?
Add check-compiler.sh that validates filepaths with a strict allowlist regex before calling the compliance script. This follows the same pattern as other reviewer proxy scripts (addPrReaction.sh, createInlineComment.sh). Reverts the execFileSync change since input validation at the boundary is sufficient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
This does look better now, thanks
|
Thanks! Let's wait for others to review 🫡 |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
I think this is fine for now, but just FYI the react-compiler-healthcheck tool is deprecated, so long-term I think a better tool call would be to just execute the react compiler babel plugin directly (via babel cli)
|
Thanks for the heads-up, @roryabraham! What is the plan/schedule for the deprecation of Switching to babel plugin directly should also mitigate the discrepancies between health check and actual compilation results, so I am rooting for it! |
|
@mountiny as the PR is now approved, I am handing it over to you! 🫡 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.40-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.40-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
5 participants
Explanation of Change
Background: The Expensify App repo uses
claude-code-actionto run automated code reviews on every PR via.github/workflows/claude-review.yml. The reviewer agent operates in a restricted sandbox where only explicitly whitelisted Bash commands are permitted through theallowedToolsparameter - currently limited togh pr diffandgh pr view. Separately, a coding standards rule (CLEAN-REACT-PATTERNS-0) instructs the reviewer to verify whether a file compiles with React Compiler before flagging manual memoization as redundant.Problem: When the reviewer agent tries to verify React Compiler compliance per the rule's instructions, it cannot execute the
npx react-compiler-healthcheckcommand due to theallowedToolswhitelist, which prevents it from distinguishing files that compile (where manual memoization is redundant) from files that don't (where manual memoization may be necessary).Solution: Replace the
npx react-compiler-healthcheckinstruction withnpm run react-compiler-compliance-check check <filepath>- an existing repo script that wraps the same healthcheck - and addBash(npm run react-compiler-compliance-check:*)to theallowedToolswhitelist so the reviewer can execute it.Changes across 3 files:
.claude/skills/coding-standards/rules/clean-react-0-compiler.md- swappednpx react-compiler-healthcheck --src "<filepath>" --verbosewithnpm run react-compiler-compliance-check check <filepath>.github/workflows/claude-review.yml- addedBash(npm run react-compiler-compliance-check:*)toallowedTools.claude/commands/review-code-pr.md- added the same pattern toallowed-toolsfrontmatterFixed Issues
$ #85070
PROPOSAL:
Tests
npm run react-compiler-compliance-check check src/components/CurrentWalletBalance.tsxnpm run react-compiler-compliance-check check src/hooks/usePaginatedReportActions.ts.github/workflows/claude-review.ymland verifyBash(npm run react-compiler-compliance-check:*)is present inallowedToolson line 77.claude/commands/review-code-pr.mdand verify the same pattern is in theallowed-toolsfrontmatter.claude/skills/coding-standards/rules/clean-react-0-compiler.mdand verify the verification command isnpm run react-compiler-compliance-check check <filepath>Offline tests
N/A - changes are to CI workflow config and reviewer instructions only, no runtime code affected.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
N/A - this is a CI/tooling change with no user-facing impact. Verification is that the reviewer agent can successfully run the compliance check on future PRs.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari