Skip to content

fix(test): compare install VM paths in one spelling so symlinked tmpdirs pass - #1120

Merged
benvinegar merged 2 commits into
modem-dev:mainfrom
shashank-100:fix/install-vm-symlinked-tmpdir
Sep 23, 2026
Merged

benvinegar merged 2 commits into
modem-dev:mainfrom
shashank-100:fix/install-vm-symlinked-tmpdir

Conversation

@shashank-100

@shashank-100 shashank-100 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Problem

assertSafeInstallVmRuntimePath canonicalizes the repo root but not the target, then compares them:

const physicalRepoRoot = realpathSync(repoRoot);          // physical
const allowedRoot = path.join(physicalRepoRoot, "tmp", "install-vm");
const resolved = path.resolve(target);                    // NOT physical
const relative = path.relative(allowedRoot, resolved);

On macOS the harness builds fixtures under os.tmpdir() = /var/folders/..., where /var is a symlink to /private/var. So allowedRoot is /private/var/.../tmp/install-vm while resolved is /var/.../tmp/install-vm/cache. path.relative between the two spellings yields an absolute escape path, and a harness-owned directory is rejected as foreign:

error: Refusing install VM path outside /private/var/folders/.../tmp/install-vm:
       /var/folders/.../tmp/install-vm/cache

The same asymmetry corrupts the symlink-ancestor walk below it, which bases its cursor on physicalRepoRoot while iterating segments taken relative to the non-physical resolved.

Impact: three tests fail on macOS, so bun run test cannot go green locally on a Mac:

  • install VM contract > allows cleaning only real harness-owned paths and rejects symlink ancestors
  • disposable VM shell > stages only curated examples and deterministic benchmark patches by default
  • disposable VM shell > runs a fresh host build before adding Hunk to the staged shell input

A symlinked $HOME reproduces it on Linux; CI presumably runs where /var is real, which is why it is green there.

Approach

Rewrite the target onto the repo's physical prefix and use that for the containment check and the symlink walk, so both sides are compared in one spelling.

Crucially, only the prefix is canonicalized. Every segment below the repo root is left verbatim, because resolving those would silently disarm the guard this function exists to enforce: a symlink inside the tree would be collapsed by realpathSync before the symlink-ancestor walk could see it. A plain realpathSync(target) fails for that reason and one more — it throws on a leaf that does not exist yet, which callers legitimately pass.

rebaseOnPhysicalRepoRoot tries the caller's spelling of the repo root and then the physical one, and rebases the first that contains the target. A target outside both is returned untouched so the containment check rejects it.

Non-goals: no change to what is considered owned, no new permitted root, and no relaxation of any rejection.

Tests

Three tests added in test/cli/install-vm/contract.test.ts:

  • accepts owned paths when the repo sits under a symlinked ancestor — reaches the repo through a symlinked ancestor and asserts both an existing leaf and a not-yet-created one are accepted and returned in the caller's spelling, while a path outside the runtime tree still throws.
  • rejects a symlink inside the tree even when the full path exists — a symlink redirecting within tmp/install-vm is refused in all three shapes: as the leaf, with an existing tail, and with a missing tail. The middle case is the one a prefix-only canonicalization protects; resolving the full target would accept it.
  • rejects a target reached through a symlink that escapes the harness tree — pins the escape rejections: a child through an escaping symlink, ../.. traversal, and an outside directory all throw, and the file outside survives.

Verified by differential run rather than assertion alone. Against main these fail 4; against the first revision of this branch they fail 2; with the current fix, 19 pass and 0 fail. Each new test therefore fails against at least one broken implementation.

Each rejection now asserts which guard fired ("symlink ancestor" vs "outside") instead of a bare .toThrow(), so a change that swaps one rejection for another is visible.

Commands run

bun run typecheck                        # clean
bun run lint                             # 0 warnings, 0 errors
bun run format                           # applied
bun test test/cli/install-vm/            # 47 pass, 1 skip, 0 fail  (was 3 fail)
bun run test                             # 4485 pass, 52 skip, 2 fail

main fails 6 on this machine; this branch fails 2, neither related:

  • Jujutsu source reading > logs unexpected source failures... — pre-existing, fails on clean main, and packages/hunk-jj is not in this diff.
  • hunk.dev install script > resolves through Hunk and falls back directly to GitHub — a 5s network timeout. Passes in isolation on both this branch and clean main; it imports nothing this diff touches.

Platforms

Fixed and verified on macOS (darwin 27.0.0), which is where the bug reproduces. The change is node:path/node:fs only and adds no platform branch; the new tests build fixtures with path.join and mkdtempSync. Behavior where tmpdir() has no symlinked ancestor is unchanged, since canonicalizing an already-physical path is a no-op — that is the Linux CI case, and its coverage is the existing suite.

Symlink creation is unprivileged on macOS and Linux but needs elevation or Developer Mode on Windows. The two new tests call symlinkSync, matching the existing test directly above them, so this introduces no new Windows constraint.

Changeset

Empty — test-harness only, nothing user-visible.

Review history

The first revision of this branch canonicalized the whole target through its nearest existing ancestor. Code review caught that this collapsed symlinks above that point, so alias/sub (a symlink resolving inside the tree, with every segment existing) was accepted where main rejected it — a caller's rmSync would then have traversed it. It did not permit escaping the tree, since containment still rejected symlinks resolving outside, but it weakened the no-symlink policy. Fixed in 7239571f by canonicalizing only the repo-root prefix, with the case added as a regression test.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@vercel

vercel Bot commented Sep 22, 2026

Copy link
Copy Markdown

@shashank-100 is attempting to deploy a commit to the Modem Team on Vercel.

A member of the Team first needs to authorize it.

@benvinegar

Copy link
Copy Markdown
Member

Thank you!

@benvinegar
benvinegar enabled auto-merge (squash) September 23, 2026 16:52
@benvinegar
benvinegar merged commit 0793793 into modem-dev:main Sep 23, 2026
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants