build: stop git apply from silently skipping dep patches with a diff --git header - #35099
build: stop git apply from silently skipping dep patches with a diff --git header#35099robobun wants to merge 2 commits into
git apply from silently skipping dep patches with a diff --git header#35099Conversation
…--git header fetch-cli's applyPatch runs git apply --no-index with cwd=vendor/<dep>/, which is a subdirectory of the bun repo. git still discovers the enclosing repo, and for a patch with a 'diff --git a/... b/...' header treats the paths as toplevel-relative (git-apply(1): 'When running from a subdirectory in a repository, patched paths outside the directory are ignored'). The dep's src/foo.c is outside the vendor/<dep>/ prefix, so git prints 'Skipped patch ...' only under -v and exits 0 having changed nothing. applyPatch checked only the exit status, so the .ref stamp certified an unpatched tree. Set GIT_CEILING_DIRECTORIES=dirname(dest) so repo discovery stops above the dep dir (and drop inherited GIT_DIR/GIT_WORK_TREE which would bypass the ceiling). Add -v + LC_ALL=C and throw on 'Skipped patch' in stderr as a safety net. patches/libuv/win-poll-abort-with-disconnect.patch (added in #32488) has a diff --git header and was silently not being applied; with this change it applies.
|
Updated 5:41 AM PT - Jul 22nd, 2026
❌ @robobun, your commit a6cd153 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35099That installs a local version of the PR into your bun-35099 --bun |
WalkthroughChanges
Patch application safeguards
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build/fetch-cli.ts`:
- Around line 245-260: Condense the comments to three lines or fewer while
preserving the durable behavioral contract. In scripts/build/fetch-cli.ts lines
245-260, retain only the concise git apply invariant; in
test/internal/dep-patch-apply.test.ts lines 1-16, 50-54, 72-75, and 98-101,
shorten the header, fixture-helper documentation, pre-fix behavior explanation,
and inherited-environment explanation respectively, moving historical detail
elsewhere if needed.
In `@test/internal/dep-patch-apply.test.ts`:
- Line 78: Strengthen the assertions in test/internal/dep-patch-apply.test.ts at
lines 78, 87, and 110 by replacing each PATCHED_LINE containment check with an
exact equality assertion against ORIGINAL.replace("line two", PATCHED_LINE),
ensuring the complete patched file contents match the intended replacement at
all three sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6765408d-bcdc-47c2-934b-b1aeef5bc2a3
📒 Files selected for processing (2)
scripts/build/fetch-cli.tstest/internal/dep-patch-apply.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Strip the diff --git / index header from win-poll-abort-with-disconnect.patch. The header change alters the patch-content hash, so computeSourceIdentity() produces a new .ref identity for libuv and existing vendor/libuv/ trees that were stamped against the silently-skipped patch are re-fetched. Also brings the patch in line with the other header-less entries under patches/. - Tighten the applyPatch tests to assert exact file contents with toBe() instead of toContain(), and trim the comment headers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/dep-patch-apply.test.ts`:
- Around line 2-5: Condense the regression-test comment to three lines or fewer
while preserving the non-obvious behavior that `git apply` from a repository
subdirectory treats `diff --git` paths as top-level-relative and may silently
skip them with exit 0; retain the reference to `applyPatch` and its doc comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c74dd610-59b7-470d-b4ca-c0747020ecf7
📒 Files selected for processing (2)
patches/libuv/win-poll-abort-with-disconnect.patchtest/internal/dep-patch-apply.test.ts
💤 Files with no reviewable changes (1)
- patches/libuv/win-poll-abort-with-disconnect.patch
There was a problem hiding this comment.
LGTM — a6cd153 addresses the stale-.ref invalidation concern by stripping the diff --git header from the libuv patch (changes its content hash, so existing vendor/libuv/ trees re-fetch), and tightens the test assertions to exact toBe().
What was reviewed:
- Verified the patch-content change flows through
computeSourceIdentity()→ new.refidentity →fetchDep()no longer short-circuits on previously-stamped trees. - Checked
patches/*/for otherdiff --git-header patches — none remain;patches/ncrypto.patchhas one but is not routed throughapplyPatch(). GIT_CEILING_DIRECTORIES+GIT_DIR/GIT_WORK_TREEscrubbing +-v/LC_ALL=Cskip-detection are correctly layered; env destructuring drops the inherited vars rather than passing empty strings.
Extended reasoning...
Overview
Build-tooling fix in scripts/build/fetch-cli.ts::applyPatch(): git apply --no-index from a repo subdirectory silently skips diff --git-header patches (paths resolved toplevel-relative, exit 0). The fix sets GIT_CEILING_DIRECTORIES=dirname(dest), scrubs inherited GIT_DIR/GIT_WORK_TREE, and adds a belt-and-suspenders Skipped patch stderr check under -v+LC_ALL=C. The follow-up commit strips the diff --git/index header from patches/libuv/win-poll-abort-with-disconnect.patch, which both aligns it with the other header-less dep patches and — critically — changes its content hash so computeSourceIdentity() produces a new .ref identity and stale vendor/libuv/ trees re-fetch. New tests in test/internal/dep-patch-apply.test.ts cover git-header diffs, plain diffs, apply-failure, and inherited git env.
Security risks
None. This is local build tooling that shells out to git apply with a fixed argv and controlled env; no user-controlled input, no network beyond the existing tarball fetch (unchanged here).
Level of scrutiny
Moderate — build infrastructure, not shipped runtime. A regression here fails the build loudly (patch-apply error or compile error), not silently at runtime. The one silent-failure risk (stale .ref certifying an unpatched tree) was my prior review's finding and is now resolved by the patch-file content change.
Other factors
- My previous 🔴 finding (fix doesn't invalidate existing stamped trees) is addressed exactly as suggested — targeted content change to the affected patch file, so only libuv re-fetches.
- CodeRabbit's
toContain→toBeassertion tightening is addressed; the comment-length nit is a style preference and the remaining doc comment carries genuinely non-obvious git semantics worth keeping. - Grepped
patches/*/*.patchfor remainingdiff --githeaders — none; the whole affected class is covered.patches/ncrypto.patch(top-level) has a header but is unreferenced byscripts/so does not flow throughapplyPatch(). - Tests use
tempDir+using, restore mutatedprocess.envinfinally, and assert exact file contents.
|
Self-review complete; both concerns it raised are addressed:
Remaining reds on 77662, none related to this diff:
Ready for review. |
1 participant
What
applyPatch()inscripts/build/fetch-cli.tsrunsgit apply --no-indexwithcwd=vendor/<dep>/. That directory is a subdirectory of the bun repo's worktree, and--no-indexdoesn't suppress repo discovery:git applyfinds the enclosing repo, and for a patch with adiff --git a/... b/...header treats its paths as toplevel-relative. From git-apply(1):The dep's
src/foo.cis outside thevendor/<dep>/prefix, so git printsSkipped patch 'src/foo.c'.(only under-v; normal verbosity prints nothing) and exits 0 having changed nothing.applyPatchonly checked the exit status, so the.refstamp certified an unpatched source tree.Plain unified diffs (
--- a/X/+++ b/Xwith nodiff --gitline) are resolved cwd-relative instead, which is why most ofpatches/was unaffected.Repro
Fix
GIT_CEILING_DIRECTORIES=dirname(dest)so repo discovery stops above the dep dir.GIT_DIR/GIT_WORK_TREE(git hooks, some CI wrappers) which would bypass the ceiling.-v+LC_ALL=Cand throw when stderr containsSkipped patch, so any remaining exit-0-but-skipped case is a build error instead of a silent no-op.diff --git/indexheader frompatches/libuv/win-poll-abort-with-disconnect.patch. The patch-content hash feedscomputeSourceIdentity(), so existingvendor/libuv/.refstamps written against the silently-skipped patch now mismatch and the tree is re-fetched instead of reporting "up to date". Also brings this patch in line with the other header-less entries underpatches/.Consequence on main
patches/libuv/win-poll-abort-with-disconnect.patch(added in #32488) was being silently skipped and has never been applied to the Windows build since it landed. This PR activates it:uv__fast_poll_submit_poll_req/uv__fast_poll_process_poll_reqinsrc/win/poll.cnow subscribe and reportAFD_POLL_ABORTasUV_DISCONNECT, which is a runtime behavior change for every bun-usockets socket on Windows (bun-usockets armsUV_DISCONNECTunconditionally atpackages/bun-usockets/src/eventing/libuv.c). Verified:bun run build --target=libuvon a Windows checkout that still had the stale.refre-fetched, applied both patches, and compiledpoll.ccleanly.node:netreset tests that node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed #32488 un-quarantined and the fetch/http2 suites exercise the affected path.#32761, #33094 and #33018 each carry an ad-hoc copy of the
applyPatchfix as part of unrelated work; landing it standalone lets those drop their copies.Tests
test/internal/dep-patch-apply.test.tsdrivesapplyPatchdirectly against a temp git repo with an extracted dep tree under it. Fail-before with the original function body (export retained for the import):All four pass after. Verified on Linux and Windows.
The fix is in
scripts/andpatches/, notsrc/orpackages/, so the gate'sgit stash push -- src/ packages/leaves it in place; the fail-before above is the manual equivalent.[stamp-90s] gate passed · iteration 0 · 3 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file