Skip to content

Keep the substituted-authority cleanup test portable and leak-free on macOS - #454

Open
vishaltandale00 wants to merge 1 commit into
mainfrom
claude/vigorous-merkle-94b778
Open

Keep the substituted-authority cleanup test portable and leak-free on macOS#454
vishaltandale00 wants to merge 1 commit into
mainfrom
claude/vigorous-merkle-94b778

Conversation

@vishaltandale00

Copy link
Copy Markdown
Owner

Summary

test/evidence-capture-integrity.test.mjs > "does not chmod a directory substituted for captured read-only authority" captured a 0o500 directory's dev/ino, renamed it, and substituted a new directory at the same path. On macOS 15.6 (APFS, internal disk and external SSD alike) rename(2) of a directory the caller cannot write fails with EACCES even within one parent (mkdir d; chmod 500 d; mv d e reproduces from the shell); Linux allows it. Runs from Sep 2 to 4 failed here and leaked 13 relayer-cleanup-substitution-* directories in os.tmpdir(), each holding only an empty read-only source, because the rename ran before the try/finally.

The rename-while-writable step already landed in #404. This change:

  • moves the whole setup (mkdir, chmod, lstat, rename, substitute) inside the try, so any setup failure reaches the finally and removes the temp dir instead of leaking it;
  • guards the restoring chmods on existence, so a partial setup does not throw a second error from cleanup and skip the rmSync;
  • replaces the "this sandbox" comment with the real reason: macOS rename(2) needs write permission on the directory being renamed, Linux does not, and dev/ino survive the rename.

Assertions are unchanged: restoreDirectoryWritesSync returns false for the substitute, and the substitute keeps its mode without the write bit.

Verification

Required plan: the changed seam is the test itself, mapped to running its file directly, plus the repository-required npm run check and npm run build pre-commit gates.

What ran on macOS 15.6 (Darwin 24.6.0, APFS, fresh worktree on an external SSD), against this exact commit:

Run Result
npx vitest run test/evidence-capture-integrity.test.mjs 130 passed, 1 skipped
Substitution test alone, twice 1 passed both times
find os.tmpdir() -name 'relayer-cleanup-substitution-*' after each run 0
npm run check exit 0 (172 vitest files passed, 1 skipped; 2278 tests passed, 3 skipped)
npm run build exit 0

The one skip in the file is accepts a multiply-linked Homebrew Node only as a non-executed copy source, gated on a Homebrew node binary with more than one hard link; this host's has one. A first npm run check attempt failed only because two vitest workers raced to download the Electron binary in the fresh worktree (File exists under node_modules/electron/dist); reinstalling Electron and rerunning gave the clean pass above. No product behavior changed, so no PRD update.

Adversarial subagent review: not run; this handoff is non-certifying on that dimension.

🤖 Generated with Claude Code

… macOS

macOS rename(2) refuses to move a directory the caller cannot write (EACCES
on APFS, even within one parent), while Linux allows it. The test already
renamed while writable; this moves the whole setup inside the try so a setup
failure still reaches the finally, guards the restoring chmods on existence,
and states the actual platform reason in the comment. Assertions unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

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.

1 participant