Skip to content

Add system-wide bunfig.toml support - #28727

Open
robobun wants to merge 1 commit into
mainfrom
farm/e658f71f/system-wide-bunfig
Open

Add system-wide bunfig.toml support#28727
robobun wants to merge 1 commit into
mainfrom
farm/e658f71f/system-wide-bunfig

Conversation

@robobun

@robobun robobun commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator

Closes #28726

Problem

Corporate environments need to enforce bunfig settings (e.g. minimumReleaseAge) system-wide without modifying per-user or per-project config files.

Solution

Add a system-wide bunfig.toml that is loaded before user and project configs. The config merge order is:

  1. System config (lowest priority)
  2. User/home config (~/.bunfig.toml)
  3. Project config (./bunfig.toml) (highest priority)

Later values override earlier ones.

Default paths

  • POSIX: /etc/bunfig.toml
  • Windows: %ALLUSERSPROFILE%\bunfig.toml (typically C:\ProgramData\bunfig.toml)

Auto-discovery of the default path is scoped to package-manager commands (bun install, bun add, bunx, etc.) so ordinary bun run / script invocations don't probe it on every call.

Override

Set the BUN_SYSTEM_CONFIG environment variable to an absolute path. Unlike the default-path auto-discovery, an explicit BUN_SYSTEM_CONFIG is honored on every command path, including compiled standalone binaries. Relative paths are rejected; an empty string is treated as unset.

Failure behavior

An explicit BUN_SYSTEM_CONFIG that is missing or malformed fails loudly (so admin typos surface immediately). An auto-discovered default path that is malformed warns and continues, so a broken /etc/bunfig.toml can't brick every invocation on the host.

Changes

  • src/bunfig/arguments.rs: get_system_config_path(), load_system_bunfig(), and the SystemConfigResult type; load_global_bunfig() / load_config() load the system config first. The config path is interned in the process-lifetime FilenameStore so ctx.log can borrow it without a stack-use-after-return or a leak.
  • src/runtime/cli/run_command.rs: load the system config in boot_standalone() so compiled standalone binaries honor an explicit BUN_SYSTEM_CONFIG.
  • src/bun_core/env_var.rs: BUN_SYSTEM_CONFIG and ALLUSERSPROFILE env-var accessors.
  • src/options_types/context.rs: has_loaded_system_config guard field.
  • docs/runtime/bunfig.mdx, docs/runtime/environment-variables.mdx: documentation.

Verification

test/config/bunfig/system-config.test.ts (12 cases): merge order, project-overrides-system, explicit-path fail-loud, malformed fail-loud, [define] applied, bun run still loads project bunfig, relative-path rejection, empty-string-as-unset, package-manager system→home merge, compiled standalone binary honors BUN_SYSTEM_CONFIG, and two Windows-only auto-discovery cases.

bun bd test test/config/bunfig/system-config.test.ts  → 10 pass, 2 skip (Windows-only)

Rebase note

Rebased onto latest main. The conflict this time was src/bunfig/arguments.rs against #33909, which replaced bun_core::Error with per-crate thiserror enums: migrated load_bunfig, load_system_bunfig, load_global_bunfig, and load_config to return crate::Error (the new bunfig::Error), and mapped the FilenameStore::append_parts failure through bun_alloc::AllocError into Error::Alloc. The branch history was also squashed to a single commit to keep future rebases simple. Verified with cargo check/clippy on bun_bunfig + bun_runtime and the full test suite (10 pass, 2 Windows-only skip).


no test proof · iteration 106 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/config/bunfig/system-config.test.ts

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.

Once credits are available, push a new commit or reopen this pull request to trigger a review.

@robobun

robobun commented Mar 31, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:05 PM PT - Aug 14th, 2026

@robobun, your commit c670c1d has some failures in Build #97293 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 28727

That installs a local version of the PR into your bun-28727 executable, so you can run:

bun-28727 --bun

@coderabbitai

coderabbitai Bot commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds support for a system-wide bunfig.toml (auto-discovered or via BUN_SYSTEM_CONFIG), changes global config load order to load system config first, introduces a per-process flag tracking system-config load, adds environment variable constants, and includes regression tests for precedence and failure modes.

Changes

Cohort / File(s) Summary
Configuration State Tracking
src/cli.zig
Added has_loaded_system_config: bool to Command.ContextData to record whether a system config was loaded in the process.
System Config Loading Logic
src/cli/Arguments.zig
Added getSystemConfigPath(...), loadSystemBunfig(...), and SystemConfigResult; updated loadGlobalBunfig(...) and loadConfig(...) to load system config first, treat explicit BUN_SYSTEM_CONFIG as mandatory, use platform defaults (%ALLUSERSPROFILE%\bunfig.toml on Windows, /etc/bunfig.toml on POSIX) as optional, and adjust error/reporting behavior.
Environment Variables
src/env_var.zig
Exported new env var constants: ALLUSERSPROFILE and BUN_SYSTEM_CONFIG.
Regression Tests
test/regression/issue/28726.test.ts
Added tests exercising system config preload execution, precedence vs project bunfig.toml, explicit-missing BUN_SYSTEM_CONFIG failure, [define] substitutions from system config, and system→home merge during bun install.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code changes successfully implement the requested feature: system-wide bunfig.toml support with proper precedence (system < user < project) and environment variable override [#28726].
Out of Scope Changes check ✅ Passed All changes directly support system-wide bunfig.toml functionality: config loading logic, environment variable definitions, guard field, and comprehensive regression tests within scope.
Title check ✅ Passed The title clearly and concisely describes the primary change: adding system-wide bunfig.toml support.
Description check ✅ Passed The description explains the change, configuration precedence, failure behavior, implementation scope, and verification results in sufficient detail.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/Arguments.zig`:
- Around line 330-345: The getSystemConfigPath function currently calls
bun.getenvZ("BUN_SYSTEM_CONFIG") and bun.getenvZ("ALLUSERSPROFILE") directly;
add cached, type-safe accessors for these names in src/env_var.zig (following
the existing HOME/XDG_CONFIG_HOME PlatformSpecificNew pattern) as
bun.env_var.BUN_SYSTEM_CONFIG and bun.env_var.ALLUSERSPROFILE, then update
getSystemConfigPath to call bun.env_var.BUN_SYSTEM_CONFIG.get() and
bun.env_var.ALLUSERSPROFILE.get() instead of bun.getenvZ() so the environment
lookups are cached and consistent with the rest of the codebase.
- Around line 248-255: The function loadSystemBunfig currently marks every
discovered system config as auto_loaded (calling loadBunfig with auto_loaded =
true), which hides failures for an explicit BUN_SYSTEM_CONFIG override; change
getSystemConfigPath to return both the PathBuffer and a flag indicating whether
the path was provided via BUN_SYSTEM_CONFIG, then update loadSystemBunfig to
call loadBunfig with auto_loaded = false when the path came from the environment
(and true only for the OS default path), adjusting the same pattern in the
analogous code block around the 330-357 range so explicit overrides fail loudly
while the auto-discovered default remains optional.

In `@test/regression/issue/28726.test.ts`:
- Around line 4-87: Add a new test in test/regression/issue/28726.test.ts that
actually exercises the system→home merge path by spawning Bun with a command
that bypasses AutoCommand (e.g., a package-manager or bunx-style invocation) so
readGlobalConfig()/loadGlobalBunfig() runs; create a tempDir, write a home-level
bunfig (or XDG config) plus a system-level system-bunfig.toml, set env vars HOME
or XDG_CONFIG_HOME and BUN_SYSTEM_CONFIG appropriately, spawn Bun with cmd like
[bunExe(), "package-manager", "some-args"] (or the bunx equivalent) and assert
that values from the home config override/merge as expected; reference the
existing tests for patterns (tempDir usage, Bun.spawn) and ensure the new test
name mentions "system→home merge" so it’s clear what is being exercised.
- Around line 25-45: Replace the fragile "from:project" check with a proof that
the system preload never produced an irreversible side effect: change
"system-preload.ts" to perform an irreversible action (for example, write a file
like "system-ran.txt" into the temp dir) and keep "project-preload.ts" writing
the global FROM value; after process exit assert stdout is "from:project" and
also assert that the irreversible artifact from system-preload (e.g., the file
"system-ran.txt") does NOT exist in the temp dir (use the existing dir/tempDir
variable and the proc/exitCode results to locate where to check), ensuring
overrides truly replace earlier state rather than merely running later.
🪄 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: bf722b42-b784-4f21-b389-f4205fe3608b

📥 Commits

Reviewing files that changed from the base of the PR and between 5c59842 and 9f80bf2.

📒 Files selected for processing (3)
  • src/cli.zig
  • src/cli/Arguments.zig
  • test/regression/issue/28726.test.ts

Comment thread src/cli/Arguments.zig Outdated
Comment thread src/cli/Arguments.zig Outdated
Comment thread test/config/bunfig/system-config.test.ts
Comment thread test/regression/issue/28726.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
test/regression/issue/28726.test.ts (2)

1-2: ⚠️ Potential issue | 🟠 Major

Strengthen the preload override test to prove replacement, not just final assignment.

Line 44 can still pass if both preloads run and project executes last. Add an irreversible side effect in system-preload.ts (e.g., create a file) and assert it never occurs.

Suggested diff
+import { existsSync } from "node:fs";
 import { describe, expect, test } from "bun:test";
 import { bunEnv, bunExe, tempDir } from "harness";
@@
-      "system-preload.ts": `(globalThis as any).FROM = "system";`,
+      "system-preload.ts": `import { writeFileSync } from "node:fs"; writeFileSync("system-ran.txt", "1"); (globalThis as any).FROM = "system";`,
       "project-preload.ts": `(globalThis as any).FROM = "project";`,
@@
     // Project-level bunfig overrides system-level preload
     expect(stdout.trim()).toBe("from:project");
+    expect(existsSync(`${dir}/system-ran.txt`)).toBe(false);
     expect(exitCode).toBe(0);

Also applies to: 25-45

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/regression/issue/28726.test.ts` around lines 1 - 2, The current test can
pass even if both preloads run; modify the preload override test so
system-preload.ts performs an irreversible side effect (e.g., write a sentinel
file) and assert that sentinel never exists after the process runs to prove the
system preload was replaced. Concretely: update system-preload.ts to create a
file (use tempDir/bunEnv helpers or fs.writeFileSync) and update
test/regression/issue/28726.test.ts to check for that sentinel (fs.existsSync or
similar) and fail if it exists; reference the system-preload.ts sender and the
test function in the file (the test that currently asserts on Line 44) so the
assertion checks absence of the sentinel after running with the project preload
override. Ensure any created temp paths are unique and cleaned up in test
setup/teardown.

4-87: ⚠️ Potential issue | 🟠 Major

Add a regression case that exercises the system→home merge path.

Current coverage validates system-only and system→project behavior, but not system→home precedence (a core requirement from Issue #28726 / PR objective). Please add a case with a temporary home config (HOME/XDG_CONFIG_HOME) plus BUN_SYSTEM_CONFIG, and assert home values override system values.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/regression/issue/28726.test.ts` around lines 4 - 87, Add a new test that
exercises the system→home merge path similar to the existing tests: create a
temp dir for system config (e.g., "system-bunfig-home-override") with
"system-bunfig.toml" containing preload/define or a value (e.g., set
FROM="system"), create a separate temp dir to act as HOME (or XDG_CONFIG_HOME)
containing "bunfig.toml" that sets the same key to a different value (e.g.,
FROM="home"), then spawn Bun via Bun.spawn (as in other tests) with env {
...bunEnv, BUN_SYSTEM_CONFIG: `${systemDir}/system-bunfig.toml`, HOME:
String(homeDir) } (or XDG_CONFIG_HOME) and assert stdout shows the home value
(e.g., expect(stdout.trim()).toBe("from:home") and exitCode 0); add this new
test case in the same describe block using the same patterns and helpers
(tempDir, Bun.spawn) so home-level bunfig overrides the system-level bunfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@test/regression/issue/28726.test.ts`:
- Around line 1-2: The current test can pass even if both preloads run; modify
the preload override test so system-preload.ts performs an irreversible side
effect (e.g., write a sentinel file) and assert that sentinel never exists after
the process runs to prove the system preload was replaced. Concretely: update
system-preload.ts to create a file (use tempDir/bunEnv helpers or
fs.writeFileSync) and update test/regression/issue/28726.test.ts to check for
that sentinel (fs.existsSync or similar) and fail if it exists; reference the
system-preload.ts sender and the test function in the file (the test that
currently asserts on Line 44) so the assertion checks absence of the sentinel
after running with the project preload override. Ensure any created temp paths
are unique and cleaned up in test setup/teardown.
- Around line 4-87: Add a new test that exercises the system→home merge path
similar to the existing tests: create a temp dir for system config (e.g.,
"system-bunfig-home-override") with "system-bunfig.toml" containing
preload/define or a value (e.g., set FROM="system"), create a separate temp dir
to act as HOME (or XDG_CONFIG_HOME) containing "bunfig.toml" that sets the same
key to a different value (e.g., FROM="home"), then spawn Bun via Bun.spawn (as
in other tests) with env { ...bunEnv, BUN_SYSTEM_CONFIG:
`${systemDir}/system-bunfig.toml`, HOME: String(homeDir) } (or XDG_CONFIG_HOME)
and assert stdout shows the home value (e.g.,
expect(stdout.trim()).toBe("from:home") and exitCode 0); add this new test case
in the same describe block using the same patterns and helpers (tempDir,
Bun.spawn) so home-level bunfig overrides the system-level bunfig.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 746e082e-d072-46a4-9062-43afb7b97b74

📥 Commits

Reviewing files that changed from the base of the PR and between 9f80bf2 and ccb7d0f.

📒 Files selected for processing (1)
  • test/regression/issue/28726.test.ts

Comment thread src/cli/Arguments.zig Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/cli/Arguments.zig`:
- Around line 248-257: The code currently treats an explicit BUN_SYSTEM_CONFIG
that couldn't be stored in the PathBuffer as "no system config" and silently
continues; in loadSystemBunfig (and similarly where getSystemConfigPath is used)
detect the case where result.is_explicit is true but result.path is null and
fail loudly (return an error or call ctx.logger/error and propagate failure)
instead of skipping loadBunfig; update loadSystemBunfig to check "if
(result.is_explicit and result.path == null) return error.InvalidConfigPath" (or
an appropriate error) so explicit overrides behave like other explicit-override
failures, referencing getSystemConfigPath, loadSystemBunfig, and loadBunfig to
locate the change.

In `@test/regression/issue/28726.test.ts`:
- Around line 94-128: The test "system→home config merge via readGlobalConfig
path" can pass without loading the system config because home dryRun=true alone
prevents node_modules creation; fix by making the system config produce a
visible effect independent of dryRun or by removing dryRun from the home config
so both files' settings are observable. Update the test setup that writes
"system-bunfig.toml" (and/or "home-bunfig.toml") used by the test and change the
system config to include an observable, read-only setting (for example a logging
level or other config inspected at runtime such as install.logLevel = "debug")
and then assert on that observable (e.g., check stderr/stdout for the expected
log) or alternatively remove dryRun from the home file and keep system exact =
true so the node_modules behavior verifies the merge; refer to the test name and
loadGlobalBunfig/loadGlobalConfig path to locate where to change the config
strings.
🪄 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: 0ab460d1-43e6-43ab-aec1-7466de099215

📥 Commits

Reviewing files that changed from the base of the PR and between ccb7d0f and 0906577.

📒 Files selected for processing (3)
  • src/cli/Arguments.zig
  • src/env_var.zig
  • test/regression/issue/28726.test.ts

Comment thread src/cli/Arguments.zig Outdated
Comment thread test/regression/issue/28726.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/cli/Arguments.zig`:
- Around line 383-391: The code calls loadSystemBunfig only after the standalone
autoload early return, so an explicit BUN_SYSTEM_CONFIG is ignored when
disable_autoload_bunfig causes the function to return early; change the flow so
loadSystemBunfig(allocator, ctx, cmd) is invoked when an explicit
BUN_SYSTEM_CONFIG is present or move the call above the disable_autoload_bunfig
early return—i.e., detect env BUN_SYSTEM_CONFIG (or equivalent flag) and always
call loadSystemBunfig even if disable_autoload_bunfig is set, preserving the
existing error handling around ctx.log, Output.err, and Global.crash.
🪄 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: f673e059-db5c-4fa6-a5c2-6b6d99fcde0b

📥 Commits

Reviewing files that changed from the base of the PR and between 0906577 and bbefad3.

📒 Files selected for processing (2)
  • src/cli/Arguments.zig
  • test/regression/issue/28726.test.ts

Comment thread src/cli/Arguments.zig Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/cli/Arguments.zig (1)

371-379: ⚠️ Potential issue | 🟠 Major

Move explicit system-config detection above the standalone autoload check.

Lines 373-378 still return before Lines 383-385 run, so a standalone binary with disable_autoload_bunfig never honors BUN_SYSTEM_CONFIG. Also, get() != null treats BUN_SYSTEM_CONFIG="" as explicit, which incorrectly falls back to the default system path on commands that should skip global config.

Suggested fix
 pub fn loadConfig(allocator: std.mem.Allocator, user_config_path_: ?string, ctx: Command.Context, comptime cmd: Command.Tag) OOM!void {
+    const has_explicit_system_config = brk: {
+        if (bun.env_var.BUN_SYSTEM_CONFIG.get()) |path| break :brk path.len > 0;
+        break :brk false;
+    };
+
     // If running as a standalone executable with autoloadBunfig disabled, skip config loading
-    // unless an explicit config path was provided via --config
-    if (user_config_path_ == null) {
+    // unless an explicit config path was provided via --config or BUN_SYSTEM_CONFIG
+    if (user_config_path_ == null and !has_explicit_system_config) {
         if (bun.StandaloneModuleGraph.get()) |graph| {
             if (graph.flags.disable_autoload_bunfig) {
                 return;
             }
         }
@@
-    if (bun.env_var.BUN_SYSTEM_CONFIG.get() != null or comptime cmd.readGlobalConfig()) {
+    if (has_explicit_system_config or comptime cmd.readGlobalConfig()) {
         loadSystemBunfig(allocator, ctx, cmd) catch |err| {
             if (ctx.log.hasAny()) {
                 ctx.log.print(Output.errorWriter()) catch {};
             }

Also applies to: 383-385

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli/Arguments.zig` around lines 371 - 379, The standalone-autoload early
return currently runs before honoring an explicitly provided system config and
treats an empty BUN_SYSTEM_CONFIG as explicit; move the logic that detects an
explicit system config (the environment variable BUN_SYSTEM_CONFIG) above the
bun.StandaloneModuleGraph.get() / graph.flags.disable_autoload_bunfig early
return and only consider the system-config explicit when the env value is
non-empty (i.e., treat "" as not provided). Update both places referenced around
user_config_path_ and the later duplicate block so the code checks for a
non-empty BUN_SYSTEM_CONFIG first, and only if there is no explicit system
config then allow the disable_autoload_bunfig early return to return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cli/Arguments.zig`:
- Around line 343-348: The BUN_SYSTEM_CONFIG env var is accepted verbatim which
allows relative values to make the “system” config cwd-dependent; update the
branch that handles bun.env_var.BUN_SYSTEM_CONFIG to either (A) canonicalize the
value to an absolute path before returning (use the std.fs APIs to join with cwd
and normalize/realpath) or (B) reject non-absolute values with an explicit
error; specifically, in the block that reads custom_path and returns .{ .path =
buf[...], .is_explicit = true }, check whether custom_path is absolute (e.g.,
starts with the platform path separator or otherwise detect absolute on
Windows), and if not either construct an absolute path using std.fs.cwd()/path
normalization or fail loudly and return an error so loadBunfig only ever
receives absolute paths.

---

Duplicate comments:
In `@src/cli/Arguments.zig`:
- Around line 371-379: The standalone-autoload early return currently runs
before honoring an explicitly provided system config and treats an empty
BUN_SYSTEM_CONFIG as explicit; move the logic that detects an explicit system
config (the environment variable BUN_SYSTEM_CONFIG) above the
bun.StandaloneModuleGraph.get() / graph.flags.disable_autoload_bunfig early
return and only consider the system-config explicit when the env value is
non-empty (i.e., treat "" as not provided). Update both places referenced around
user_config_path_ and the later duplicate block so the code checks for a
non-empty BUN_SYSTEM_CONFIG first, and only if there is no explicit system
config then allow the disable_autoload_bunfig early return to return.
🪄 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: 1c3535a3-aa2c-4162-895f-9bc9e24b1ae2

📥 Commits

Reviewing files that changed from the base of the PR and between bbefad3 and aa9b01a.

📒 Files selected for processing (1)
  • src/cli/Arguments.zig

Comment thread src/cli/Arguments.zig Outdated
Comment thread src/cli/Arguments.zig Outdated
Comment thread src/runtime/cli/Arguments.zig Outdated
Comment thread src/cli/Arguments.zig Outdated
Comment thread src/cli/Arguments.zig Outdated
Comment thread src/runtime/cli/Arguments.zig Outdated
Comment thread src/cli/Arguments.zig Outdated
Comment thread src/runtime/cli/Arguments.zig Outdated
Comment thread src/bun.js.zig Outdated
Comment thread test/config/bunfig/system-config.test.ts
Comment thread src/runtime/cli/Arguments.zig Outdated
Comment thread src/runtime/cli/Arguments.zig Outdated
@robobun
robobun force-pushed the farm/e658f71f/system-wide-bunfig branch from bab4460 to ae74e1d Compare April 6, 2026 09:14
@robobun
robobun force-pushed the farm/e658f71f/system-wide-bunfig branch 2 times, most recently from fe519e6 to 6fbb731 Compare May 4, 2026 21:29
Comment thread src/bun_core/env_var.zig Outdated
Comment thread src/bun_core/env_var.zig Outdated
Comment thread test/config/bunfig/system-config.test.ts
Comment thread src/js/internal/sql/errors.ts Outdated
Comment thread src/runtime/cli/Arguments.zig Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 test/regression/issue/28726.test.ts:100-102 — nit: expect(stderr).not.toContain("\xaa") is a no-op — proc.stderr.text() UTF-8-decodes the stream, so raw 0xAA poison bytes become U+FFFD, while the JS literal "\xaa" is code point U+00AA, and the two can never match. The regex on line 99 is the only load-bearing check here; either change this to .not.toContain("\uFFFD") or just drop it and the comment.

    Extended reasoning...

    What the assertion is trying to do

    This test guards the stack-use-after-return fix from commit 6c7bdf5: before the fix, when BUN_SYSTEM_CONFIG pointed at a malformed TOML file, the error printer read Location.file from a dead PathBuffer stack frame, and under ASAN that memory is poisoned with 0xAA bytes. The intent of line 102 is belt-and-suspenders: in addition to asserting the path prints correctly (line 99), also assert that no raw poison bytes leaked into stderr.

    Why the assertion can never fail

    There are two layers of indirection that make this check ineffective:

    1. .text() does UTF-8 decoding, not Latin-1. proc.stderr.text() applies WHATWG "utf-8" decoding to the byte stream. A standalone 0xAA byte is a UTF-8 continuation byte (10xxxxxx) with no preceding lead byte, which is an encoding error. Per the WHATWG spec, each such byte is replaced with U+FFFD REPLACEMENT CHARACTER. So a buggy build that writes at \xAA\xAA\xAA…:1:8 to stderr produces the JS string "at \uFFFD\uFFFD\uFFFD…:1:8" after .text().

    2. The literal "\xaa" is a code point, not a byte. In JavaScript, "\xaa" is shorthand for "\u00aa" — code point U+00AA FEMININE ORDINAL INDICATOR (ª). It is not the byte 0xAA. After UTF-8 decoding, U+00AA could only appear in the string if the subprocess had written the two-byte UTF-8 sequence 0xC2 0xAA, which the UAF regression never does.

    So even on a build where the regression has resurfaced and stderr contains raw 0xAA bytes, stderr.includes("\xaa") is false and .not.toContain("\xaa") passes.

    Step-by-step proof

    Empirically verified with Bun:

    const proc = Bun.spawn({ cmd: ["bash", "-c", "printf 'at \\xaa\\xaa\\xaa.toml:1:5' >&2"], stderr: "pipe" });
    const stderr = await proc.stderr.text();
    stderr                          // "at ���.toml:1:5"
    stderr.charCodeAt(3)            // 0xFFFD
    stderr.includes("\xaa")         // false  ← assertion at line 102 passes
    stderr.includes("\uFFFD")       // true
    /at [^\n]*system-bunfig\.toml:1:\d+/.test(stderr)  // false   line 99 catches it

    The decoded string contains U+FFFD, not U+00AA, so .not.toContain("\xaa") is vacuously satisfied.

    Why this is only a nit

    The test as a whole is not broken. Line 99's expect(stderr).toMatch(/at [^\n]*system-bunfig\.toml:1:\d+/) is the load-bearing assertion: if the path were replaced by poison bytes (→ U+FFFD after decoding), the literal substring system-bunfig.toml would not appear and the regex would fail. So the regression is still caught — line 102 is just dead weight with a misleading comment ("the stderr must not contain any of the ASAN poison bytes") that suggests it does something it cannot.

    How to fix

    Either of:

    • Check for the replacement character instead: expect(stderr).not.toContain("\uFFFD") — this does fire if any invalid-UTF-8 byte (including 0xAA) reaches stderr, and keeps the belt-and-suspenders intent.
    • Check raw bytes: read await proc.stderr.bytes() into a Uint8Array and assert !bytes.includes(0xAA).
    • Just delete lines 100-102 — the regex on line 99 already provides full coverage for this regression, so the extra assertion adds no signal.

@robobun

robobun commented May 6, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed the vacuous "\xaa" assertion in cf065a3. You are right on both counts — proc.stderr.text() UTF-8-decodes invalid bytes to U+FFFD, and the JS literal "\xaa" is U+00AA, so the check could never match the raw 0xAA poison byte it was trying to catch. I verified empirically: even after restoring the UAF, the .includes("\xaa") check passed. Dropped lines 100-102 — the regex on line 99 (at [^\n]*system-bunfig\.toml:1:\d+) is the load-bearing check and fail-before still confirms it rejects any garbled filename (stale heap, pointers, or poison bytes).

Comment thread docs/runtime/bunfig.mdx
Comment thread docs/runtime/bunfig.mdx Outdated
Comment thread test/config/bunfig/system-config.test.ts
Comment thread docs/runtime/environment-variables.mdx Outdated
Comment thread test/config/bunfig/system-config.test.ts Outdated
Comment thread test/config/bunfig/system-config.test.ts
Comment thread src/bun_core/env_var.rs Outdated
Comment thread src/bunfig/arguments.rs
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs Outdated
Comment thread src/bunfig/arguments.rs
Comment thread src/runtime/cli/run_command.rs Outdated
Comment thread src/runtime/cli/run_command.rs
Comment thread src/bunfig/arguments.rs
Comment thread src/bunfig/arguments.rs
Comment thread src/bunfig/arguments.rs
Comment thread src/bunfig/arguments.rs
Comment thread src/runtime/cli/run_command.rs
Comment thread test/config/bunfig/system-config.test.ts Outdated
Comment thread src/runtime/cli/run_command.rs
System config (/etc/bunfig.toml on POSIX, %ALLUSERSPROFILE%\bunfig.toml
on Windows, or explicit BUN_SYSTEM_CONFIG) loads before home and project
configs; later tiers override earlier ones. Auto-discovery of the default
path is scoped to package-manager commands; an explicit BUN_SYSTEM_CONFIG
applies on every command path including compiled standalone binaries, is
required to be absolute, and fails loudly when missing or malformed while
auto-discovered defaults warn and continue.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this revision and the bug-hunting pass found nothing new — the two nits from earlier today (weak stderr assertion in the bad-path test; open-coded report_bunfig_load_failure in boot_standalone) are both addressed in c670c1d. Because this adds a new user-facing config tier with policy implications (system→home→project precedence, auto-discovery scoped to package-manager commands, warn-and-continue on a broken /etc/bunfig.toml), a human sign-off on the design would still be worthwhile.

What was reviewed:

  • Config load ordering across load_config / load_global_bunfig / boot_standalone, including the has_loaded_system_config guard against double-load and that DISABLE_AUTOLOAD_BUNFIG still blocks project autoload after an explicit system config.
  • The FilenameStore interning of the config path so ctx.log diagnostics don't borrow a freed PathBuffer frame — checked against the malformed-TOML test that asserts the readable path in the caret diagnostic.
  • Explicit-vs-auto-discovered error handling: explicit BUN_SYSTEM_CONFIG fails loud on ENOENT/parse error; auto-discovered warns, prints accumulated log, resets, and continues.
  • Test hermeticity: every case sets BUN_SYSTEM_CONFIG (or unsets it and overrides ALLUSERSPROFILE) so CI hosts with a real /etc/bunfig.toml don't leak in.
Extended reasoning...

Overview

This PR adds a third bunfig tier — a system-wide config loaded before the home and project bunfigs — driven by BUN_SYSTEM_CONFIG (explicit, honored on every command including compiled standalone binaries) or a platform default (/etc/bunfig.toml on POSIX, %ALLUSERSPROFILE%\bunfig.toml on Windows) probed only for package-manager commands. It touches src/bunfig/arguments.rs (new get_system_config_path/load_system_bunfig, an is_project parameter on load_bunfig, path interning via FilenameStore, and a now-pub report_bunfig_load_failure taking Context), src/runtime/cli/run_command.rs::boot_standalone, two env-var declarations in env_var.rs, a new has_loaded_system_config guard on ContextData, docs for both the bunfig page and the env-var table, and a 12-case test file.

Security risks

The feature is explicitly aimed at corporate policy enforcement, but the merge order is system→home→project with later-overrides-earlier, so it provides defaults, not enforcement — a project bunfig can override anything the system config sets. The docs are consistent about this ("lowest priority, overridden by global and project configs"), so it's not a bug, but it is a design property a human should confirm matches the intent in #28726. The auto-discovered path is read-only and root-owned on POSIX, so no new privilege surface. The one fail-open concern raised earlier in review (standalone binaries silently ignoring BUN_SYSTEM_CONFIG) is now closed with a dedicated --compile test.

Level of scrutiny

High. This runs on the CLI startup path for every invocation (an extra env-var read on every command, plus a filesystem probe of /etc/bunfig.toml on package-manager commands), adds a new unsafe ZStr::from_raw over an interned buffer, and introduces user-facing API surface (a documented env var and platform default paths) that will be hard to change once shipped. It is not a mechanical or obvious change.

Other factors

The PR has been through ~106 iterations over four months; every earlier automated finding — the standalone-binary bypass, loaded_bunfig poisoning, the ALLUSERSPROFILE empty-string case, the stack-use-after-return on the log-borrowed path, the missing SAFETY comment, the undrained build stdout, the --compile test timeout, today's weak-assertion and dedup nits, and the comment-cop verbosity flags — is resolved in the current diff. Test coverage is solid (12 cases including the standalone-binary path and two Windows-only auto-discovery cases). Given no human has signed off on the API shape yet, I'm deferring rather than approving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System Wide bunfig support

1 participant