-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(scm/git): Avoid panic for unanchorable/non-UTF8 git paths , gracefully handle path errors #11106
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.
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
base: main
Are you sure you want to change the base?
Conversation
…fully handle path error
|
@biru-codeastromer is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
anthonyshew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR!
Let's do a hard failure (exit 1) here instead of a warning. My worry is that users will not see the warning, and suddenly start running all of their CI because of one misbehaving file. If that file were to make it to the default branch of the repo, that would result in some dramatic slowdowns in a large enough repo.
Before this PR, the CI run would crash, and the user's code would never be able to make it through CI. So, erroring with a useful message instead of a crash report would be a meaningful DX improvement, while preserving existing behavior.
|
Thanks good call. I updated the change detector so SCM path errors now produce a hard failure (exit 1) with a focused, actionable error message on stderr instead of logging a warning and defaulting to “all packages changed”. This makes the failure visible in CI logs and avoids accidentally running full CI due to a stray problematic filename. |
anthonyshew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read and correct Claude's output before submitting it in a pull request. If you are not able to verify the completeness of the provided implementation because you do not understand it, please do not send a pull request.
The Vercel review is correct. You need to update the PR description.
Additionally, lets add some unit testing in accordance with this fix. Sorry for not mentioning this earlier.
Co-authored-by: Anthony Shew <[email protected]>
Removed outdated documentation for the add_files_from_stdout function.
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
|
I will be adding the unit test soon |
What
Prevent a runtime panic in
crates/turborepo-scm/src/git.rswhen git emits paths that cannot be anchored to the turbo root (e.g. corrupted filenames, non-parent paths) or when git stdout contains non-UTF8 bytes. Instead of unwrap() → crash, errors are surfaced and handled safely by the change detector.Why
GitHub Actions CI was failing for repositories that contain unusual filenames (Cyrillic / corrupted / non-UTF8). The panic originated in add_files_from_stdout and caused --affected CI runs to crash. This change makes the flow fail-safe: we log the problem and conservatively treat all packages as changed rather than crashing CI.
Summary of changes
crates/turborepo-scm/src/git.rs
add_files_from_stdout(...)now returnsResult<(), Error>(propagates non-UTF8 viaString::from_utf8and anchoring errors instead of unwrap()).?to propagate the new Result.crates/turborepo-lib/src/run/scope/change_detector.rs
Result<Result<..., InvalidRange>, Error>fromSCM::changed_files(...).ScmError::Path(...)— log a warning and default to marking all packages changed.all_packages_changed_due_to_errorhelper to deduplicate fallback logic and keep code readable.Behavior (user-facing / CI)
Instead of panicking on unanchorable or invalid paths:
Notes
RelativeUnixPath::new(line)remains unwrap()-ed by design (git output for the commands used is not expected to include absolute paths). If maintainers want stricter defensive handling, we can change that to return an error and handle it the same way.Fix #10403