Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefined Zig resolver directory-read error handling: classify permission-denied errors, introduce a missing-parent sentinel and explicit parent-result flow to skip inaccessible non-target directories, change parent-index usage, and add a Linux-only Landlock regression test that compiles a helper and verifies sandboxed Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28220.test.ts`:
- Around line 115-157: The test currently builds the landlock helper once into a
shared /tmp/landlock-helper and relies on module-level variables helperPath and
landlockSupported for other tests; change it so each test builds and self-checks
its own helper inside a test-specific tempDir (use tempDir from harness), remove
reliance on the shared helperPath/landlockSupported state, and invoke the
compile+self-check setup at the start of each test (or factor into a per-test
setup helper function referenced by test names like "compile landlock helper"
and the subsequent "bun run ..." tests) so paths are isolated and tests are
self-contained.
🪄 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: 4bafb50e-5d30-44a7-808b-917200be4e6c
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/regression/issue/28220.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28220.test.ts`:
- Around line 135-145: The test reads the Landlock self-check stdout but never
asserts the subprocess exit code; add an explicit assertion on the Bun.spawnSync
result: after computing landlockSupported from check.stdout, assert that when
landlockSupported is true then check.status === 0 (or check.exitCode === 0 if
your environment uses exitCode), and when landlockSupported is false assert
check.status !== 0 (or non-zero exitCode). Use the existing variables check and
landlockSupported to locate where to add these assertions in
issue/28220.test.ts.
🪄 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: 622b25c7-0aa2-4f20-afbf-3e0e97ec2c13
📒 Files selected for processing (1)
test/regression/issue/28220.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28220.test.ts`:
- Around line 155-227: Add a new test case in
test/regression/issue/28220.test.ts that exercises the "unreadable target dir"
scenario: using the same prepareLandlockFixture/helperPath pattern, create a
tempDir (e.g., "issue-28220-unreadable-target-dir"), write a simple index/main
file into testBase, then make testBase itself unreadable (chmod 0 or use the
landlock helper to deny read to the target dir) before spawning Bun via
Bun.spawnSync with the same cmd/env/cwd/stdio settings; assert that stderr
contains the expected failure markers (e.g., "CouldntReadCurrentDirectory" or
"error loading current directory") and that result.exitCode is non-zero to lock
in the invariant that unreadable cwd fails, mirroring the other tests' structure
(reference the existing test names and prepareLandlockFixture/helperPath/bunExe
usage).
- Around line 12-15: The test currently assumes compilation succeeds by
asserting expect(compile.exitCode).toBe(0) after generating LANDLOCK_HELPER_SRC;
instead wrap the compile step (the invocation producing compile and the
assertion on compile.exitCode) in a try/catch or check compile.status and call
the test skip logic so build failures are treated as a skipped test. Locate the
compile invocation that uses LANDLOCK_HELPER_SRC and the
expect(compile.exitCode).toBe(0) assertion and replace it with a guarded block
that catches compilation errors or non-zero exitCode and calls the existing skip
path (the same skip behavior used later for runtime Landlock checks), ensuring
the rest of the test is not executed when the C compiler or <linux/landlock.h>
are unavailable.
🪄 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: b6455ccb-e341-431c-a5c6-fd8db04a4099
📒 Files selected for processing (1)
test/regression/issue/28220.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28220.test.ts`:
- Around line 163-176: The test suite gating currently uses
canCompileLandlockHelper() which only checks C compilation; change it to probe
runtime Landlock support instead: use prepareLandlockFixture() (or call the
compiled helper in a temp fixture) to obtain the runtime flag
prepareResult.landlockSupported (or landlockSupported) and set
landlockRuntimeSupported = prepareResult.landlockSupported; then use
describe.skipIf(!isLinux || !landlockRuntimeSupported) instead of
describe.skipIf(!isLinux || !landlockCompileSupported) so tests are skipped when
the kernel doesn't actually support landlock and the helper won't be invoked
erroneously.
🪄 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: 7a40aaf8-2b9e-4759-a648-cbdee013050c
📒 Files selected for processing (1)
test/regression/issue/28220.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/28220.test.ts`:
- Around line 134-160: prepareLandlockFixture currently returns an object with
compileSupported only on the early-return branch; update the final return to
include compileSupported so both branches return the same shape (include
compileSupported along with helperPath, landlockSupported, and testBase), and
ensure any expectations (e.g., checks using check.exitCode/stdout) still run
before returning.
🪄 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: ed6e5961-4b7d-4240-8971-40a5dd731408
📒 Files selected for processing (1)
test/regression/issue/28220.test.ts
|
Any news on this? Do you want me to update/rebase the pr? |
cee35b0 to
509fc54
Compare
|
Since the rust port is merged, I also ported this fix to rust now, it is ready for another review. |
|
+1 |
509fc54 to
1c282a1
Compare
|
+1 on this as well |
…dCurrentDirectory on Android bun v1.3.14's readDirInfo resolver walks from CWD up to / opening every ancestor with openat(O_DIRECTORY). Android sandbox blocks /data/, /data/data/ (outside com.termux), and / with EACCES. bun treats ANY ancestor-open failure as fatal and throws CouldntReadCurrentDirectory. Instead of returning ENOENT (which bun also treats as fatal), redirect inaccessible ancestor opens to the CWD. The entries read for these redirected ancestors are harmless since they don't carry bun config files. This works around the missing fix from oven-sh/bun#28782, which skips permission-denied ancestors in the resolver but was not included in v1.3.14.
|
Thank you for this contribution! This was fixed in #31938 and #33119: the resolver now treats EPERM/EACCES on ancestor directories as empty and continues. See Closing as already fixed. Thanks again for taking the time to send this! |
4 participants
What does this PR do TLDR?
Allows bun to run in a sandbox like Landlock or Seatbelt.
Closes #28220
What does this PR do?
Fixes
CouldntReadCurrentDirectorywhen Bun can read the current working directory but cannot open one or more ancestor directories while building resolverDirInfo.The resolver now treats permission-denied ancestor directories as an inheritance boundary instead of aborting the whole lookup. If the target directory itself is unreadable, it still fails as before.
This also adds a Linux Landlock regression test for #28220 that reproduces the sandboxed
bun runfailure and verifies the fixed behavior.How did you verify your code works?
env USE_SYSTEM_BUN=1 bun test test/regression/issue/28220.test.tserror loading current directoryCouldntReadCurrentDirectorybun bd test test/regression/issue/28220.test.ts