Skip to content

fix(path): support Bun through Rust canonicalization - #324

Merged
steipete merged 4 commits into
mainfrom
codex/bun-stress
Sep 13, 2026
Merged

steipete merged 4 commits into
mainfrom
codex/bun-stress

Conversation

@steipete

@steipete steipete commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where fs-safe consumers on Bun reject restrictive files and sockets, confuse literal POSIX backslashes with path separators, or resolve symlink/parent components in the wrong order. On Windows, Bun also rejects existing relative directories during recursive mkdir, breaking atomic file publication and queued JSON writes.

Why This Change Was Made

All default canonicalization now goes through one internal owner. On Bun macOS/Linux it uses the existing Rust N-API addon: the system realpath for native resolution and a component walk for ordinary resolution; Node and Windows retain their runtime resolvers. Ordinary resolution normalizes both initial paths and expanded symlink targets, while native resolution preserves their order. A documented 1,024-expansion limit rejects fixed and growing lexical cycles with ELOOP. Confinement, pinned descriptors, and post-operation identity checks remain with their existing owners. The Rust resolver never opens the leaf, preserving restrictive permissions, sockets, and POSIX record locks.

Windows recursive mkdir receives an absolute path without collapsing raw components or logical junction spelling. Explicit caller-supplied filesystem adapters keep their contracts. Portable test fixtures stop depending on Node-only module synchronization, async-hook internals, enumeration order, and fs.access return values.

User Impact

Bun 1.4.2 works with the matching addon, including bun --jitless. There is no bun:ffi, dynamic libc discovery, pointer management in TypeScript, new dependency, or public API change.

Native mode off still prevents addon loading. Bun POSIX with native disabled, or without the addon in auto, retains the runtime's path/permission limitations; Node remains the option for full addon-free compatibility. On Bun POSIX, require rejects canonicalization when the addon/capability is unavailable. These limits are documented instead of weakening identity checks or silently enabling native code.

Related upstream PRs: oven-sh/bun#42374 and oven-sh/bun#42635. Neither upstream fix is assumed released.

Evidence

Final commit bdd6055ae20da0a0a6f1402ee87c45135a17cad9: CI and coverage passed, including native Bun and JIT-disabled package proof on Windows/macOS/Linux, Rust checks, package smoke, and Node 22/24 checks. Independent review is clean through P2.

  • Full Node pnpm check: 9,049 passed, 109 platform skips, including build, documentation, and package checks.

  • Bun native qualification: 879 passed, 19 platform skips across 36 test files.

  • Built-package proof under normal and JIT-disabled Bun: auto, require, off, and actual external consumers with native packages omitted. Covers Root reads/writes, confinement, hashing, secrets with modes 000/200, search-only directories, sockets, backslash collisions, and raw symlink/parent paths.

  • Refreshed macOS/APFS stress: 57,600 file hash/metadata checks, 540 Root copies, 543 hashes, 96 cancellation operations, no writes after settlement, descriptors 7 → 7.

  • Linux x64 glibc 2.43 and musl 1.2.5: freshly built final Rust addons each passed 875 Bun tests (23 platform skips), record-lock/cycle regressions, and all normal/JIT-disabled package scenarios.

  • Fixed Bun CI timeouts in large-buffer deep equality: exact Buffer.equals comparisons preserve every byte assertion and reduced the affected local tests from 496/445 ms to 15/6 ms without changing timeouts.

  • Rust workspace tests and Clippy pass. Record-lock regression includes an open/close positive control; nested symlink-target normalization and both fixed/growing lexical cycles have regressions. Review caught the symlink normalization and cycle cases; both were repaired and rechecked.

  • The exhaustive Bun diagnostic deliberately remains separate: 8,919 passed, 130 failed, 109 skipped before the copy-loader fixture adjustment. Failures expose unsupported native-off cases and missing/capability mock assumptions; they are not marked as expected passes. Node CI retains all fallback assertions.

  • Tests added or updated when behavior changed

  • Security and compatibility impact considered

  • CHANGELOG.md updated when release-relevant

  • No credentials, private paths, private hosts, or sensitive contents included

@steipete
steipete requested a review from a team as a code owner September 13, 2026 20:31
@clawsweeper

clawsweeper Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 13, 2026
@clawsweeper

clawsweeper Bot commented Sep 13, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 13, 2026, 4:43 PM ET / 20:43 UTC (Revision 2).

ClawSweeper review

What this changes

Adds Bun compatibility through Rust-backed POSIX path resolution, preserves Windows recursive-directory path spelling, and updates runtime qualification, tests, and documentation.

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains useful work absent from current main, but the previously reported workspace-admission defect is still unfixed. The latest buffer-comparison change and reported timings address the earlier timeout concern.

Priority: P2
Reviewed head: bdd6055ae20da0a0a6f1402ee87c45135a17cad9

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful implementation and substantial runtime evidence remain limited by one concrete, previously reported admission defect.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The captured reported macOS and Linux built-package runs exercise the Rust-backed resolver through real Root, secret, socket, and confinement operations under normal and JIT-disabled Bun. Existing sufficient proof is retained; it does not resolve the separate workspace-admission finding.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured reported macOS and Linux built-package runs exercise the Rust-backed resolver through real Root, secret, socket, and confinement operations under normal and JIT-disabled Bun. Existing sufficient proof is retained; it does not resolve the separate workspace-admission finding.
Evidence reviewed 8 items Repository policy: Read the full root AGENTS.md and verified the origin repository. No nested AGENTS.md or maintainer-notes directory was found. Applied filesystem-boundary and public-contract guidance; did not execute builds, tests, or autoreview helpers.
Prior blocker remains: The only change since the previous reviewed head is in test/root-copy-options.test.ts. Both workspace constructors still catch and discard every canonicalization failure before creating directories.
Required canonicalizer contract: The new resolver throws helper-unavailable for a missing canonicalizer in require mode; getNativeBinding also throws when the addon cannot load. Documentation expressly includes temporary-workspace admission in this guarantee.
Findings 1 actionable finding [P2] Propagate required canonicalization failures during workspace admission
Security Needs attention Workspace admission swallows required-helper rejection: Both constructors continue filesystem creation after the new required canonicalizer rejects admission, contradicting the documented policy and potentially leaving cleanup without its retained parent. No confinement escape is established.

How this fits together

fs-safe turns caller-supplied paths into guarded filesystem operations. Its canonicalization layer feeds root confinement, identity checks, secret handling, and temporary-workspace admission.

flowchart TD
  A[Caller paths] --> B[Runtime and native policy]
  B --> C[Bun POSIX Rust resolver]
  B --> D[Runtime resolver]
  C --> E[Confinement and identity checks]
  D --> E
  E --> F[Guarded filesystem operations]
Loading

Before merge

  • Propagate required canonicalization failures during workspace admission (P2) - The prior finding remains unfixed. On Bun POSIX with native mode require, an absent addon or missing canonicalizePath makes this call throw helper-unavailable, but the catch discards it. Default compatible cleanup also absorbs loader/staging failures, so tempWorkspace() still creates and returns a workspace without a cleanup parent; the synchronous constructor has the same problem at lines 276–278. Propagate this policy failure before directory creation in both constructors, preserving ordinary missing-directory handling, and cover both unavailable-helper cases.
  • Resolve security concern: Workspace admission swallows required-helper rejection - Both constructors continue filesystem creation after the new required canonicalizer rejects admission, contradicting the documented policy and potentially leaving cleanup without its retained parent. No confinement escape is established.
  • Resolve merge risk (P1) - Bun POSIX require-mode callers can receive a workspace without a retained cleanup parent; cleanup can leave that directory behind as indeterminate.
  • Complete next step (P2) - Propagate helper-unavailable before directory creation in both workspace constructors and add missing-addon and missing-canonicalizer admission regressions.

Findings

  • [P2] Propagate required canonicalization failures during workspace admission — src/private-temp-workspace.ts:170-173
  • [medium] Workspace admission swallows required-helper rejection — src/private-temp-workspace.ts:170
Agent review details

Security

Needs attention: Required-helper admission remains bypassable for temporary workspaces; no additional supply-chain concern was found.

Review metrics

Metric Value Why it matters
Production and test growth Production +213; tests +237 net lines, excluding scripts and documentation The source growth implements the stated runtime adapters; tests include the 97-line embedded Rust test module.

Merge-risk options

Maintainer options:

  1. Preserve required admission (recommended)
    Propagate helper-unavailable from both workspace constructors and verify that missing-addon and missing-canonicalizer cases create no directories.

Technical review

Best possible solution:

Reject unavailable required canonicalization before workspace directory creation while preserving Node, native-off, and ordinary missing-directory behavior.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: on Bun POSIX, require mode with an unavailable addon or canonicalizer reaches catch-all workspace admission and continues to mkdir/mkdtemp. This review did not execute the failing path.

Is this the best way to solve the issue?

The shared resolver is a reasonable compatibility boundary, but the patch is incomplete until workspace admission propagates its required-helper failure.

Full review comments:

  • [P2] Propagate required canonicalization failures during workspace admission — src/private-temp-workspace.ts:170-173
    The prior finding remains unfixed. On Bun POSIX with native mode require, an absent addon or missing canonicalizePath makes this call throw helper-unavailable, but the catch discards it. Default compatible cleanup also absorbs loader/staging failures, so tempWorkspace() still creates and returns a workspace without a cleanup parent; the synchronous constructor has the same problem at lines 276–278. Propagate this policy failure before directory creation in both constructors, preserving ordinary missing-directory handling, and cover both unavailable-helper cases.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 5bc1f88d6743.

Labels

Label justifications:

  • P2: Bun compatibility is a bounded improvement with a remaining workspace-admission defect.
  • merge-risk: 🚨 security-boundary: The new required-canonicalizer policy can be silently bypassed during temporary-workspace admission.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The captured reported macOS and Linux built-package runs exercise the Rust-backed resolver through real Root, secret, socket, and confinement operations under normal and JIT-disabled Bun. Existing sufficient proof is retained; it does not resolve the separate workspace-admission finding.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured reported macOS and Linux built-package runs exercise the Rust-backed resolver through real Root, secret, socket, and confinement operations under normal and JIT-disabled Bun. Existing sufficient proof is retained; it does not resolve the separate workspace-admission finding.

Evidence

Security concerns:

  • [medium] Workspace admission swallows required-helper rejection — src/private-temp-workspace.ts:170
    Both constructors continue filesystem creation after the new required canonicalizer rejects admission, contradicting the documented policy and potentially leaving cleanup without its retained parent. No confinement escape is established.
    Confidence: 0.99

What I checked:

  • Repository policy: Read the full root AGENTS.md and verified the origin repository. No nested AGENTS.md or maintainer-notes directory was found. Applied filesystem-boundary and public-contract guidance; did not execute builds, tests, or autoreview helpers. (AGENTS.md:25, bdd6055ae20d)
  • Prior blocker remains: The only change since the previous reviewed head is in test/root-copy-options.test.ts. Both workspace constructors still catch and discard every canonicalization failure before creating directories. (src/private-temp-workspace.ts:170, bdd6055ae20d)
  • Required canonicalizer contract: The new resolver throws helper-unavailable for a missing canonicalizer in require mode; getNativeBinding also throws when the addon cannot load. Documentation expressly includes temporary-workspace admission in this guarantee. (src/realpath.ts:32, bdd6055ae20d)
  • Fallback permits creation without cleanup parent: Compatible cleanup absorbs loader and staged-directory failures, leaving parent undefined. Workspace creation then continues; cleanup without a parent returns indeterminate rather than removing the created directory. (src/temp-workspace-owner.ts:43, bdd6055ae20d)
  • Runtime proof and re-review continuity: The captured body, sourceRevision 15ab630f1379102c17186d24d6778b9a12a7a7859b2ea0358551937e1c15c96b, reports successful normal/JIT-disabled built-package runs on macOS and Linux glibc/musl. The inspected script exercises real Root I/O, traversal rejection, backslash collisions, restrictive permissions, sockets, and native loading policy. Its missing-require scenario checks hashing, not workspace admission. The body attributes copy-test delays to deep buffer comparisons and reports reductions from 496/445 ms to 15/6 ms; the latest diff preserves byte equality using Buffer.equals. (scripts/bun-native-proof.mjs:79, bdd6055ae20d)
  • Still necessary on main: Current main still calls the runtime realpath directly. Tree checks show the new resolver and Windows mkdir adapter absent from both main and v0.10.0. The bounded GitHub pull-request search returned this PR as the only Bun/canonicalization match; no merged replacement was established. (src/path.ts:108, 5bc1f88d6743)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Propagate required-canonicalizer failures in both workspace constructors and verify rejection before directory creation for missing-addon and missing-capability cases.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-13T20:37:06.237Z sha 9acb71b :: blocked before merge. :: [P2] Propagate required canonicalization failures during workspace admission

@steipete
steipete merged commit ced0a6d into main Sep 13, 2026
25 checks passed
steipete added a commit that referenced this pull request Sep 13, 2026
## What Problem This Solves

The Bun resolver comments did not explain why its workaround uses the existing Rust addon or why disabling that addon restores Bun's upstream limitations.

## Why This Change Was Made

Document the N-API choice beside the resolver: it works with JIT disabled and avoids a separate FFI bridge for native loading and memory handling. Clarify that native mode `off` disables the workaround because it belongs to the same optional addon.

## User Impact

Source comments only; runtime behavior and public contracts are unchanged. Follows #324. No changelog entry is needed for this explanatory follow-up.

## Evidence

Every non-comment line is identical to the merged implementation. `git diff --check` passes. The implementation's passing Node/Bun checks, platform CI, and stress evidence remain recorded in #324; this comment-only change does not invalidate that proof.
@vincentkoc
vincentkoc deleted the codex/bun-stress branch September 25, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant