[No QA] Make -changed scripts check the working tree, not just committed changes - #99764
Conversation
|
@codex review |
|
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@MelvinBot review |
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
Review — 👍 LGTM, no blocking issuesSolid, well-factored change. The three What I verifiedCI is not affected by the working-tree / untracked behavior change:
Bash correctness spot-checks (all good):
Minor, non-blocking
(No runtime/UI testing applicable — this is local dev tooling with no App-behavior surface. The test plan is CLI-only; nothing a browser run would exercise.) |
|
@MelvinBot review |
Re-review of
|
![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: 2b4d390cce
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safaribefore.movafter.mov |
| # what "changed" means. | ||
| # Usage: get_merge_base_with_main | ||
| get_merge_base_with_main() { | ||
| git fetch origin main --no-tags >&2 |
There was a problem hiding this comment.
lint-changed now silently ignores a failed origin/main fetch and exits 0
Before, git fetch origin main --no-tags was a top-level command under set -eu, so a failed fetch killed the script.
Now it lives inside get_merge_base_with_main, called as MERGE_BASE_SHA_HASH="$(get_merge_base_with_main)" || exit 1
The || exit 1 puts the call in a context where errexit is ignored, and per the shell rule, that suppression applies to the entire function body, on every bash version.
So the failing git fetch is stepped over and the merge base is computed from whatever stale origin/main the clone happens to have.
Confirmed with a stub git that fails only on fetch:
OLD: Fetching origin/main → fatal: could not resolve host → exit 128
NEW: Fetching origin/main → fatal: could not resolve host → "No lintable files changed" → exit 0
So when github connection fails, you now lint against a stale base and get a green result where you used to get a loud failure
There was a problem hiding this comment.
yeah, fixed - explicit exit-status check on git fetch now, no longer relying on set -e inside the || exit 1 context. Also dropped that now-redundant || exit 1
| async function checkChangedFiles(remote: string, verbose: boolean, checkOxc: OxcChecker): Promise<boolean> { | ||
| const mainBaseCommitHash = await Git.getMainBranchCommitHash(remote); | ||
| const changedFiles = await Git.getChangedFilesWithStatus(mainBaseCommitHash); | ||
| const changedFiles = await Git.getChangedFilesWithStatus(mainBaseCommitHash, undefined, true); |
There was a problem hiding this comment.
react-compiler-compliance-check check-changed now reads every untracked file in the repo, in full
shouldIncludeUntrackedFiles=true reaches getUntrackedFiles(undefined) (no path filter) so all untracked non-ignored files, and createFileDiffsForUntrackedFiles does fs.readFileSync(path, 'utf8') on each and allocates one DiffLine object per line.
The .ts/.tsx filter only runs afterwards, at line 253.
.gitignore has no rule for *.mov, *.mp4, *.png, *.zip or heap snapshots, so a screen recording or profile parked in the tree makes the check allocate hundreds of MB for files it immediately discards.
There was a problem hiding this comment.
fixed, threaded an extensions filter into Git.diff so untracked files get extension-filtered before being read
|
|
||
| if [[ -n "$ALL_CHANGED_FILES" ]]; then | ||
| # shellcheck disable=SC2086 # For multiple files in variable | ||
| exec "${TOP}/node_modules/.bin/cspell" --color --no-must-find-files $ALL_CHANGED_FILES |
There was a problem hiding this comment.
spell-changed silently checks nothing for paths with spaces
CSPELL ARGS (4):
[my] [new] [file.md] [plainfile.md]
!! MISSING: my / new / file.md
exit=0
$ALL_CHANGED_FILES is unquoted, and --no-must-find-files turns the resulting nonexistent fragments into a clean exit 0.
Same failure for non-ASCII paths, which git emits C-quoted.
The repo already has 56 tracked paths with spaces.
There was a problem hiding this comment.
fixed and verified locally with a space-bearing and non-ASCII filename - core.quotepath=false plus array-based reads, both come through as single args now
| get_changed_files() { | ||
| local base_sha="$1" | ||
| shift | ||
| local path_specs=("$@") | ||
|
|
||
| local diff_output | ||
| if [[ ${#path_specs[@]} -gt 0 ]]; then | ||
| diff_output="$(git diff --diff-filter=AMR --name-only "$base_sha" -- "${path_specs[@]}")" | ||
| else | ||
| diff_output="$(git diff --diff-filter=AMR --name-only "$base_sha")" | ||
| fi | ||
|
|
||
| local untracked_output | ||
| if [[ ${#path_specs[@]} -gt 0 ]]; then | ||
| untracked_output="$(git ls-files --others --exclude-standard -- "${path_specs[@]}")" | ||
| else | ||
| untracked_output="$(git ls-files --others --exclude-standard)" | ||
| fi | ||
|
|
||
| printf '%s\n%s' "$diff_output" "$untracked_output" | grep -v '^$' || true |
There was a problem hiding this comment.
${#path_specs[@]} -gt 0 branch is written out twice, and the two outputs are stitched with printf '%s\n%s' | grep -v '^$' || true to strip the blank line the stitching itself creates.
Neither is needed.
"$@" is exempt from set -u when empty (unlike a named array: that's the only reason path_specs exists), and git diff … -- / git ls-files … -- with an empty pathspec means "everything", so one form covers both cases.
And each git command emits either complete lines or nothing at all, so there are no blank lines to filter:
get_changed_files() {
local base_sha="$1"
shift
git diff --diff-filter=AMR --name-only "$base_sha" -- "$@"
git ls-files --others --exclude-standard -- "$@"
}
There was a problem hiding this comment.
took your rewrite, kept || return 1 on the diff call so a diff failure isn't masked by ls-files succeeding
| exec "${TOP}/node_modules/.bin/cspell" --color --no-must-find-files $ALL_CHANGED_FILES | ||
| else | ||
| info "No changed files to spell check" | ||
| fi |
There was a problem hiding this comment.
This code has the same structure with lintChanged.sh:11-26.
The PR extracted the first two steps into helpers but left the last two duplicated
There was a problem hiding this comment.
got your point but leaving duplicated - the two tails exec different tools with different args, a shared helper would just be an if/else around two one-liners
| local merge_base_sha_hash | ||
| merge_base_sha_hash="$(git merge-base origin/main HEAD)" | ||
|
|
||
| if [[ -z "$merge_base_sha_hash" ]] || ! [[ "$merge_base_sha_hash" =~ ^[a-fA-F0-9]{40}$ ]]; then | ||
| error "git merge-base returned unexpected output: $merge_base_sha_hash" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
The merge-base validation checks the output instead of the exit status
The -z arm is dead: the empty string already fails the 40-hex regex, and both arms produce the same message.
There was a problem hiding this comment.
dropped the dead -z arm
| # Excludes dotfiles and files under dot-directories (e.g. .github/) to match this script's prior behavior | ||
| ALL_CHANGED_FILES="$(get_changed_files "$MERGE_BASE_SHA_HASH" | grep -v '^\.' || true)" |
There was a problem hiding this comment.
This inline comment explains the wrong "why"
There was no prior behavior, with no args the old cspell --no-must-find-files checked nothing.
There was a problem hiding this comment.
ahh yep, reworded to the actual reason (dotfiles aren't meant to be spell-checked)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b4d390cce
ℹ️ 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".
Corrected review of
|
Regression scan — no breaking regressions found ✅I couldn't find any regression that would break CI or existing usage. The behavior changes are the intended ones (check the working tree + untracked files, baseline against a fresh CI is safe (verified):
Intentional behavior changes (not regressions, but flag for QA)
Minor watch-items
(No runtime/UI testing applicable — this is local dev tooling with no App-behavior surface, so there's nothing a browser run could exercise. The regression risk is entirely in the build/dev scripts, which I audited above.) |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85ea9aa4e4
ℹ️ 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".
|
@codex review |
|
@MelvinBot review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ 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". |
Review of
|
|
@MelvinBot I addressed your & codex points, please re-review |
Re-review of
|
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ 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". |
Regression scan of
|
|
@MelvinBot take a second look please |
Second look at
|
Test summary — Tests 1-12 ✅Tests 1-3 —
|
| SHA | |
|---|---|
git merge-base main HEAD (stale) |
12a92bd39f31ddabf5a425552930cbb055536555 |
git merge-base origin/main HEAD (fresh) |
89f3ba3461442cb9dde5539ce53f4c3f6916ae01 |
npm run knip-changed printed:
Running knip on merge base (89f3ba3461442cb9dde5539ce53f4c3f6916ae01)...
✅ The printed SHA matches the fresh origin/main merge base, not the stale local main one — the script's own fetch of origin/main wins regardless of the local branch state. The full run completed cleanly:
Knip comparison:
main : 128
PR : 128 (delta +0)
PR introduces no new knip findings.
|
AI review result: 1. The newest commit replaced a correct comment with a false one
It does exclude them. At 2.
|
|
Please address applicable items in #99764 (comment) and #99764 (comment) |
|
fixed all except webp, checked this one and it's not actually dead - tested with cspell's built-in dictionaries and "webp" is unknown without that entry. Left it in. quotepath/heapsnapshot moving to inline ignore is a style call, left as-is for now. |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
LGTM. Some of these scripts might become faster and more readable if re-written in Bun, but that's out of scope for now.
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.66-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.68-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.68-1 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
The
-changedscripts (lint-changed,check-changed,knip-changed,spell-changed) are meant to check your in-progress work, but some of them missed uncommitted or new files, or compared against your localmain(which is stale if you haven't pulled recently). Now they all diff against a freshly-fetchedorigin/mainand check your actual working tree, including files you haven't committed yet.Fixed Issues
$ #99113
PROPOSAL: Slack
Tests
.ts/.tsxfile with an obvious lint violation (e.g. an unused variable), and create a new untracked.tsfile with a lint violation, without committing either.npm run lint-changed..tsxfile containing a React component/hook.npm run react-compiler-compliance-check check-changed.npm run spell-changedwith no arguments.mainbranch stale relative toorigin/main(e.g.git checkout main && git reset --hard main@{1}to move it back one pull, or simply don't pull if it's already behind), then switch back to your branch.npm run knip-changed.git merge-base origin/main HEAD(fresh), notgit merge-base main HEAD(stale) - the script fetchesorigin/mainitself, so it should use the up-to-date remote branch regardless of your localmain's state.Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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