Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe service now accepts the legacy Linux ChangesWSL Codex home matching
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The legacy WSL ownership record is accepted only in its intended compatibility case, while explicit overrides and unrelated ownership records retain strict matching. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 3📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
![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: 114ece2949
ℹ️ 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".
| const env = deps.env ?? process.env; | ||
| if (env.CODEX_HOME?.trim() || !isWslRuntime(deps)) return false; | ||
| const legacyDefault = join((deps.homedir ?? homedir)(), ".codex"); | ||
| return serviceHomeMatches(recordedHome, legacyDefault); |
There was a problem hiding this comment.
When a WSL service was intentionally installed with CODEX_HOME=/home/$USER/.codex, but that home has no config.toml—for example, because Codex integration was disabled—a later shell with CODEX_HOME unset can discover the sole Windows home. This fallback cannot distinguish that explicit install record from legacy implicit metadata and returns true, so repair silently rewrites the unit and state for the Windows home, while stop/uninstall restore the wrong home. Only accept the compatibility case when the installed launcher/state proves the original home was implicit.
Useful? React with 👍 / 👎.
| const expected = normalizePathForCompare(state.codexHome); | ||
| const actual = normalizePathForCompare(actualCodexHome); | ||
| if (expected !== actual) { | ||
| if (!serviceCodexHomeMatchesInstall(state.codexHome)) { |
There was a problem hiding this comment.
In WSL, home discovery depends on live config.toml files and candidate directories, but assertServiceEnvironmentMatchesInstall() resolves actualCodexHome and then this call resolves it again inside serviceCodexHomeMatchesInstall(). If candidates change between those reads, the check can reject a matching record while reporting identical installed/current paths, or accept based on a different snapshot. Pass the already-resolved actualCodexHome into the compatibility comparison so ownership and diagnostics use the same result.
Useful? React with 👍 / 👎.
github-actions
Bot
changed the title
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
luvs01
changed the title
1 participant
Motivation
~/.codex) from being treated as foreign after runtime WSL home discovery changed to a Windows Desktop path.CODEX_HOMEauthoritative.Description
serviceCodexHomeMatchesInstall(recordedHome, deps)which accepts an exact legacy~/.codexrecord when running under WSL withCODEX_HOMEunset and otherwise enforces strict path equality.assertServiceEnvironmentMatchesInstall()to use the compatibility-aware comparator forcodexHomewhile preserving the existingOPENCODEX_HOMEcomparison logic.tests/codex-home-wsl.test.tsthat verifies WSL Windows-home discovery and the legacy-Linux fallback acceptance and non-acceptance cases.structure/02_config-and-codex-home.mdand leave all other foreign ownership records unmigrated.Testing
./node_modules/.bin/bun test tests/codex-home-wsl.test.tswhich passed (6/6)../node_modules/.bin/bun run typecheckand./node_modules/.bin/bun run privacy:scanwhich succeeded../node_modules/.bin/bun test tests/service.test.ts --timeout 15000which showed 112 passing service tests and one unrelated pre-existing assertion failure in this checkout.git diff --checkand committed the change asfix(service): accept legacy WSL ownership state.Codex Task
Summary by CodeRabbit
Bug Fixes
CODEX_HOMEoverrides as authoritative.Documentation