Repository navigation
🤖 tests: test the socket lock-owner check itself, keep make's dry runs off the host, and refuse a too-long default socket path early - #7913
Conversation
…temp runtime dir, and refuse a default path that does not fit Red: assertLockPathUsable is not exported, and the chooser creates the folder before it checks the length of the default path.
…socket path that does not fit before make creates the folder
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🟢 e2e stories (Haiku): 14 passed22 agent steps · 7 replayed from cache · 15 model calls · 47.1k tokens (23% cached) All 14 tests in 9 files
e2e 0.20.0 · 1m 14s · web · run artifacts 🟢 e2e Electron stories (Haiku): 3 passed3 agent steps · 3 model calls · 8.0k tokens (22% cached) All 3 tests in 3 files
e2e 0.20.0 · 17.9s · electron · run artifacts 🟢 e2e agent repros (Haiku): no tests selectedSpend: $0.00 of the $1.00 cap (proxy ledger) e2e 0.20.0 · 5.9s · 2 targets (web, phone) · run artifacts |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dff9b6e29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…path fits on macOS Codex review on #7913. os.tmpdir() on macOS is a long /var/folders path, and the default socket path under it exceeds the 103-byte limit that the chooser now checks.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
Three small fixes from the advisor and checker review of #7912: a lock-owner test that tests the lock-owner check, make dry runs in the test file that no longer create folders on the host, and make refusing a too-long default socket path before it creates a folder.
Background
#7912 added the socket lock and the fresh-folder choice. Its reviews found three leftovers. None of them changes what the dev server does for a normal start.
Implementation
src/node/utils/privateUnixSocket.test.ts). "refuses a lock file that another user owns" went throughwithPrivateUnixSocketPathwith a foreign uid, and the folder check failed first, so it passed even without the lock-owner check.assertLockPathUsableis now exported and the test calls it on its own: the lock file is ours and the expected owner is not. A second test checks that a missing lock file is accepted. The symlink and folder tests still go throughwithPrivateUnixSocketPath, which shows that the check is wired in.scripts/devServerAuth.test.ts).make -pn dev-serverclaims the default socket folder at parse time (since 🤖 fix: lock the dev-server socket across probe and bind, and use a fresh folder when the default one is not ours #7912), in$XDG_RUNTIME_DIRor/tmp. The test helper now passesXDG_RUNTIME_DIRset to one temp directory for the whole file, removed inafterAll. The directories are made under/tmp, notos.tmpdir(), so that the socket path under them fits the 103 bytes of macOS (itsos.tmpdir()is a long/var/folders/...path). A test asserts that the default socket path is inside it. The tests that set their ownXDG_RUNTIME_DIRuse their own temp directories, which they already removed.src/node/utils/privateUnixSocketFolder.ts).chooseDevSocketPathchecked the length of the fallback path but not of the normal default path, so a too-long default created an empty folder and the backend refused later. It now checks the default path before it creates anything, and make stops with the "too long" message plus the existing hint that namesXUM_BACKEND_SOCKETandXDG_RUNTIME_DIR.Validation
Red first (
tests:commit before the code): the new tests failed before the change (the lock-owner test and the length tests:assertLockPathUsablewas not exported, and make exited 0 for a too-long default path).assertLockPathUsable, the new test fails (1 of 24 tests in the file). The old test, run against the same mutated code, still passes.strace -f -e trace=mkdir,mkdiratoverbun test scripts/devServerAuth.test.ts:main: 12mkdircalls on/tmp/xum-dev-1000itself, plus the test-owned temp folders./tmp/xum-dev-*. The 12 calls are on/tmp/xum-make-base-<random>/xum-dev-1000, and the test-owned folders are as before. Noxum-make-*folder was left in/tmpafter the run.chooseDevSocketPathhas a unit test for the same case.Risks
Tests and one early check in make's parse-time script. The only product change is that a too-long default path now stops make a step earlier, with the message that the backend would have given.
Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh• Cost:$53.21