Repository navigation
fix(test): isolate tmux servers and guard default sockets - #845
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@codex review |
Codex Review Summary: ❌ FailedThis 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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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. |
|
@codex review |
🛡️ 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: 88ebc8b50e
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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. |
The guard admits `-L name` and `-S <root>/x.sock`, but cleanup only killed the root's default server, so a named or explicit-socket server could outlive its removed root. Close now stops each socket found under the owned root (symlinks not followed) and keeps the root if any stop fails. Regression starts both forms and checks their servers stop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Ready for lead: 4d00738 Took over HACK-2242 from the previous owner (account out of capacity).
Ran on Linux, Bun 1.4.4, real tmux, temp HOME,
Review: persistent Codex reviewer (gpt-6.1-sol, codex-dio) CLEAN at 4d00738 after one fix round. Exact-head hosted CI at 4d00738: all 29 active checks passed (8 test shards, 4 Docker e2e shards and fan-ins, linux-process-lifetime, Candidate core and Config compiler on every host). Optional Proof skipped. GitHub reports mergeable. No further pushes planned after this note. Not verified: macOS (the guard and cleanup run on Darwin paths in the lead's Mac suite), e2e harness changes under Docker, SIGKILL leftovers (follow-up, as before). |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
… guard next's tmux socket guard (#845) admits only registered private socket roots. lifecycle-session-recovery points TMUX_TMPDIR at its own fixture root, so its first tmux call was refused, and its cleanup then threw before restoring TMUX_TMPDIR, which broke later scenarios in the shard with ENOENT on the deleted root. Register the fixture root (mkdtemp, 0700) for the scenario and always restore and unregister in an inner finally. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After merging next, tests/sync-spawn-bound.test.ts failed on two files that landed after this branch's base: tmux-socket-guard.test.ts (12) and nss-read-safety.test.ts (1). Bun.spawnSync sites use spawnSyncBounded; it still calls Bun.spawnSync, so the tmux socket guard under test still applies. The Node execFileSync/spawnSync/execSync calls, and the Bun.spawnSync call that must throw from the guard, keep their API and get an explicit timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tests can invoke tmux directly or through helper children and inherit the user's default server. Test startup now owns a private socket root, removes inherited
TMUX, and stops its server after teardown, including assertion failures. The E2E runner also owns a private server scope closed infinally.Bun's spawn APIs and Node's child-process APIs receive an explicit environment with a registered private
TMUX_TMPDIR. This covers scripts and helper children with omitted or minimal environments. Direct tmux commands additionally validate socket arguments and directory ownership/mode. Cleanup runs after the checkout guard's Git children; the global-state guard retains its fixed read-only native-environment probe.The daemon-session fixture merged in #844 keeps its stricter explicit socket, empty config, finite pane lifetime, per-case cleanup and real session-presence assertions. This PR adds only four socket-registration lines to that file. Other real-tmux fixtures register their roots, and project-view unit tests inject session data.
Seven compiler environment-contract checks use an async-local scope for their owned, pure compiler fixture only. Those fixtures keep the original explicit compiler environment and unchanged PATH-only assertions. Sibling commands and commands outside the scope remain isolated; these pure fixtures never invoke tmux.
Regressions cover indirect real tmux calls reaching the owned server PID, Bun/Node API forms, null/undefined argv preserving options, fixture-only environment scopes, unsafe-socket refusals using a harmless stand-in, and cleanup after a deliberately failing child test.
Validation after a normal merge of frozen
nextat8e7242db94:tmux list-sessions -F '#{session_id}:#{session_name}', withTMUXandTMUX_TMPDIRunset: byte-identical stdout and stderr before/after; 3 sessions before and after. No user session names are included here.1a09a7037; the contributor guide retains all incoming guidance.Scripts that deliberately erase their environment or name an explicit socket must maintain isolation themselves; explicit
Bun.$.env(...)is unwrapped. SIGKILL skips teardown and may leave a private server/root; stale-root recovery remains follow-up work. Lead full-suite/Mac acceptance remains separate from local evidence.Fixes HACK-2242.