Repository navigation
test: keep the suite runnable from Codex-managed worktrees and beside a live proxy - #6307
Conversation
… a live proxy A checkout under ~/.codex/worktrees/ (a Codex-app worktree) could not clean up the fixture directories forty suites keep beside their test files: the removal guard refused every path inside the real Codex home, 863 failures in one test:changed run. The guard now lifts only the inside-the-tree refusal, only for content of the running checkout, and only when that checkout itself sits inside the same protected tree. The checkout root, its ancestors and siblings stay refused, each path spelling is judged on its own so a link out of the checkout is still caught, and a checkout that contains a protected tree gains nothing. shutdown-launcher started its child with a fresh home whose configured port defaulted to 10100. On a machine running ocx there, the child found that proxy, took the sibling path and never injected Codex config. The test now pins its own port in config.json and pins GROK_HOME and the owner registry into the fixture.
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. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adjusts removal checks for worktrees nested inside protected paths. It also configures the shutdown-launcher test to use its allocated port and test-specific home paths. ChangesLocal test stability
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to This change lets the test suite run from nested worktrees and beside a live proxy. The supplied evidence shows the removal guard's existing refusals are preserved, so the merge risk is minimal. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Cleanup permissions expand only within the running checkout, while protected roots, ancestors, sibling worktrees, and links escaping the checkout remain refused. No production authentication or deletion change was identified. Concurrent filesystem changes remain an unresolved limitation of the cleanup checks. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
리뷰 · 우선순위 14 / 80이 PR은 개발자 노트북에서 테스트가 깨지던 두 가지를 고칩니다. 첫째, Codex 앱이 만든 worktree( 라인 - 메인테이너의 판단이 필요한 지점 너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: e17396cd87
ℹ️ 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".
| // Inherited real state must not make the child a sibling of the host's proxy either | ||
| // (same pinning as tests/cli/sibling-home-client-sync.test.ts). | ||
| GROK_HOME: join(home, "grok"), | ||
| OCX_OWNER_REGISTRY_DIR: join(identity.homeDir, ".opencodex", "ocx-homes"), |
There was a problem hiding this comment.
On macOS with a live proxy in the user's default home, these overrides still do not isolate the child: findCrossHomeOwnerDetailed() separately reads join(homedir(), ".opencodex", "runtime-port.json"), and this repository documents that os.homedir() there ignores the rewritten HOME. OCX_OWNER_REGISTRY_DIR only relocates the registry, so markCrossHomeSibling() can still find the host proxy, mark this child as a sibling, skip Codex config injection, and make all three signal cases time out exactly as before. Add a test-only seam for the default-home lookup and point it at identity.homeDir here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Checked this against the running host rather than the comment in the repo.
findCrossHomeOwnerDetailedresolves the default home asjoin(options.homeDir ?? homedir(), ".opencodex")(src/cli/cross-home-owner.ts:162). On this macOS host both runtimes honorHOME:HOME=/private/tmp/mm31/fakehome bun -e 'console.log(require("os").homedir())'prints the fake home, andnode -eprints the same. The child here getsHOME: identity.homeDir.- Empirically: this host has a real
~/.opencodex/runtime-port.jsonand ocx 2.73.0 answering on 10100, andbun test ./tests/service/shutdown-launcher.test.tspasses all three signal cases from this branch. The same three failed atdev6538001e2fon the same host.
The passwd-database note in the repo applies to the LaunchAgents path comment. It does not hold for homedir() in these runtimes, so no extra seam is needed for this test.
|
Maintainer integration into
|
1 participant
Summary
Two host-dependent failures made the suite unusable on a developer machine.
1. Codex-managed worktrees. When the checkout is a Codex-app worktree under
~/.codex/worktrees/, the test-home removal guard refused every path inside the real Codex home. That included the fixture directories forty suites keep beside their test files (join(import.meta.dir, ".tmp-…")). Onebun run test:changedfrom such a worktree produced 863 failures. For example,model-visibility-management-api.test.tsfailed 23/23 withrefusing to remove a path inside the real Codex home. The same head cloned to/private/tmpproduced 56.The guard now allows one narrow case. It skips the "inside the tree" refusal only for content of the running checkout, and only when that checkout itself sits inside the same protected tree.
$HOMEand the virtual root a compiled build reports.2.
shutdown-launcherbeside a live proxy. The test startedocx startwith a fresh home whose configured port defaulted to 10100.handleStartprobes the configured port even when no state files exist. On a machine running ocx on 10100, the child found that proxy, took the sibling path and by design never injected Codex config. SIGINT, SIGTERM and SIGHUP all failed locally, atdevand at every head since; CI has no proxy on 10100. The test now writes{ port }to the fixtureconfig.json. It also pinsGROK_HOMEandOCX_OWNER_REGISTRY_DIRinto the fixture, assibling-home-client-sync.test.tsalready does.Plan and audit:
devlog/_plan/260930_local_test_stability/.Verification
bun x tsc --noEmit: pass.bun test ./tests/service/shutdown-launcher.test.ts ./tests/ci-workflows/test-home-guard.test.ts ./tests/codex-integration/model-visibility-management-api.test.ts: 50 pass, 0 fail. The launcher's 3 signal cases and model-visibility's 23 cases failed before this change.14-checkout-contentbuilds a sentinel home whose.codexlinks to the checkout's parent. It asserts that a fixture path is allowed while the checkout root, its parent, a sibling and an escaping link are refused.15-checkout-contains-treeusesOCX_REAL_HOME= checkout and asserts that.codexand.opencodexcontent is still refused.fixture: false, so it is not vacuous.bun run test:changedresults:refusing to removeremains./private/tmpclone of this head: 32031 pass, 17 fail. At the feat(minimax): add MiniMax-M3.1-Flash-Preview with its always-on thinking contract #6304 head it was 56 fail, including the 3 launcher cases.native-grok-toggle,native-codex-toggle,service-sqlite-home,service-ownership-state,service.testandrelease-helper.server-live, run together throughbun scripts/test.ts, give 387 pass, 0 fail. That makes the residual cross-file interference in a ~1500-file single run, not this change, and it is left out of scope (recorded in the devlog).Checklist