node:fs: preserve special POSIX mode bits in file modes - #33042
node:fs: preserve special POSIX mode bits in file modes#33042FuPeiJiang wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesSpecial mode bits fix and tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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/js/node/test/parallel/test-fs-chmod-mask.js`:
- Around line 91-141: This change adds Bun-specific special-bit assertions into
a vendored Node compatibility test, which should remain an upstream-matching
port. Remove the `testSpecialMode` block from the `fs.chmod`/`fs.fchmod` test in
`test-fs-chmod-mask` and keep that file unchanged as the Node reference. Move
the special-mode coverage into the Bun-owned `fs.test.ts` suite instead, using
the existing `fs`/`fchmod` behavior tests there to host the extra assertions.
🪄 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: 03f5e2d0-cf8c-4a20-8c6e-a3bd1af37402
📒 Files selected for processing (3)
src/runtime/node/types.rstest/js/node/fs/fs.test.tstest/js/node/test/parallel/test-fs-chmod-mask.js
8d86391 to
9489175
Compare
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/js/node/fs/fs.test.ts`:
- Around line 1736-1745: Add a `writeFileSync({ mode })` test case for the
octal-string input alongside the existing `writeFileSyncSyncWithSpecialMode`
coverage in `fs.test.ts`. The current `it.skipIf(isWindows)` test only verifies
the numeric mode branch, so extend that same test area to call `writeFileSync`
with a string mode value and assert the preserved POSIX bits via
`fs.statSync(...).mode & 0o7777`. Keep the test near the existing special-mode
case so the `writeFileSync` parser behavior is covered by the same suite.
🪄 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: a4b758e9-ad6f-498f-9a2e-7704bdec3731
📒 Files selected for processing (2)
src/runtime/node/types.rstest/js/node/fs/fs.test.ts
9489175 to
ef751c6
Compare
|
Thank you for catching this and sending a fix! The Closing as already fixed on main. Thanks again for the careful report and patch, and we hope to see more contributions from you! |
What
Change
node:fsmode normalization frommode & 0o777tomode & 0o7777.Why
Bun currently masks file modes to
0o777. That preserves the common rwx permission bits and strips file-type bits from values likestat.mode, e.g.0o100644 -> 0o644.However,
0o777also drops POSIX special mode bits:0o40000o20000o1000Node preserves these bits for
chmod/fchmodand creation modes. Masking to0o7777keeps all POSIX permission bits while still stripping file-type bits such asS_IFREG(0o100000).This keeps the existing stat-mode compatibility behavior while avoiding accidental loss of special mode bits.
Note
Node appears to validate file modes as uint32 values and pass them through without applying this permission mask. This PR keeps Bun's existing behavior of stripping higher file-type bits from values like
stat.mode, but changes the mask from0o777to0o7777so POSIX special mode bits are preserved.If exact Node compatibility is preferred, removing the mask entirely is another option.
Tests
Added Bun-owned coverage for:
fs.chmodfs.chmodSyncfs.fchmodfs.fchmodSyncfs.writeFileSync({ mode })Local verification: