Repository navigation
fix(service): keep shell-local multishell paths out of service launchers - #6168
Conversation
…efinitions Normalize and compare full launchd plists before PATH reuse, then reload changed definitions through the existing guarded lifecycle. Preserve bundled Bun and non-PATH settings; report temporary launcher repair in the present tense. Co-authored-by: Andrew <shiyuanwang@ucsb.edu>
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. |
|
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 (13)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughService launcher selection and generated service definitions now exclude shell-local multishell paths. Repair reports stale recorded launchers and reloads launchd when plist content changes. ChangesService path handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No identified issue blocks merging. Real launchd and systemd lifecycle testing after the installing shell exits remains open. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A failure during macOS service repair can make a cleaned service definition appear active while the running service still has its old PATH. The affected scope is the installing user's background service; the review did not establish a new cross-user privilege path. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 8 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
![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: 8325639ce8
ℹ️ 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".
| Service launchers reject recorded and newly discovered paths inside shell-local `fnm`, `nvm`, | ||
| `mise`, `asdf`, or `volta` multishell directories. Systemd, launchd, and Windows definitions | ||
| remove those entries from their rendered PATH while keeping other environment values. Launchd |
There was a problem hiding this comment.
When a Windows user installs the opt-in native backend with ocx service install --native, this contract does not hold: buildWinswXml() still writes env.PATH verbatim in src/lib/winsw.ts:109, and native repair regenerates that XML through installWinswService(). Only the Task Scheduler batch script was updated, so a native service installed or repaired from a multishell still retains the disappearing directory and can fail after logout or reboot. Apply the same filtering to the WinSW XML and cover that backend with a regression test.
AGENTS.md reference: structure/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6f4c3943c4: buildWinswXml() now filters the saved Windows PATH through the same transient-directory filter, so native install and native repair (which regenerates the XML through installWinswService()) drop *_multishells entries too. The pure predicate and filter moved to src/lib/transient-service-path.ts to avoid a winsw ↔ service/state import cycle, and state.ts re-exports them. tests/service/service-multishell-path.test.ts adds the WinSW case: a mixed-case transient directory is removed while durable entries and XML escaping are kept. An audit of every service definition that bakes PATH found launchd, systemd and Task Scheduler already filtered.
|
✅ Deterministic PR hygiene checks passed. |
|
Exact-head source review found no P0-P2 defect: the transient-path classifier, per-platform delimiters, launcher fallback, full-plist comparison, non-PATH preservation, and launchd reload decision are coherent, and all three service-lifecycle jobs are green. I am still holding approval on the explicit acceptance criterion carried from #6030: the current tests mock the transient-PATH repair and launchctl verbs, while the PR itself says install from a real fnm shell → close that shell → repair/restart/reboot has not been exercised. Please attach one disposable real systemd proof and one real launchd proof (or a hosted job that creates the transient PATH, installs, removes it, and verifies the live job after repair). No production/live service was touched during this review. |
Co-authored-by: Andrew <shiyuanwang@ucsb.edu>
리뷰 · 우선순위 58 / 80이 PR은 컴퓨터를 껐다 켜면 배경 서비스가 죽던 일을 고칩니다. fnm 같은 도구는 터미널이 열려 있는 동안만 있는 폴더를 PATH 맨 앞에 넣습니다. 이름에 이제는 경로의 한 칸이 이미 저장된 실행 파일이 임시 폴더이면 macOS repair는 plist 전체를 비교합니다. PATH만 다르면, 고치는 셸의 짧은 PATH로 바꾸지 않고 이미 돌고 있는 작업의 PATH를 유지합니다. 그 예전 PATH 안의 임시 폴더만 지웁니다. 파일 내용이 바뀌면 launchd를 내렸다가 다시 올립니다. src/service/state.ts serviceLauncherPathDiagnostic - 상태 검사는 저장된 실행 파일만 봅니다. 실행 파일은 패키지 Bun인데 PATH 안에만 임시 폴더가 있는 예전 서비스는 정상으로 나옵니다. 그 PATH는 src/service/launchd.ts reusePreviousPlistPathVariable - 함수 위 주석은, 이전 PATH를 되돌리면 파일이 같아져서 재시작하지 않는다고 말합니다. 코드는 임시 폴더를 뺀 다음 비교합니다. 그 차이만 있으면 bootout 뒤에 bootstrap 합니다. 주석만 보고 재시작을 빼면, 살아 있는 launchd가 옛 PATH를 계속 쓰는 문제로 돌아갑니다. 메인테이너의 판단이 필요한 지점 머지 전에 실기 확인이 한 번씩 필요합니다. fnm 터미널에서 서비스를 설치하고, 그 터미널을 닫은 다음, repair와 재시작 또는 재부팅 뒤에 서비스가 떠 있는지입니다. 지금 테스트는 launchctl 명령을 흉내 냅니다. PR 본문도 이 확인이 남아 있다고 적었습니다. Linux systemd 하나, macOS launchd 하나면 됩니다. 임시 폴더인지는 이름만으로 정합니다. 오래 쓰는 폴더를 우연히 너의 추천 런처를 임시 폴더 밖으로 빼는 방향은 맞습니다. 실기 확인 전에는 승인만 보류하면 됩니다. #6030은 아직 열려 있습니다. 이 PR이 그중 런처 부분을 대체하고, shim과 이 댓글은 grok-bot이 작성했습니다 |
|
@Ingwannu Understood. The real lifecycle proof is the one remaining item, and this lane can't supply it right now. The maintainers have stopped local test runs and local proxy or service starts for verification in this train, and a hosted job that installs a real systemd unit would need a new workflow change, which crosses the security boundary this PR otherwise stays clear of. I'm leaving this for the coordinator's cross-platform run or a maintainer-run disposable proof. Here is the exact procedure. systemd (disposable Linux VM or user account with a user manager):
launchd (disposable macOS account):
|
|
Maintainer integration into Exact head |
Summary
Reimplements the service-launcher part of #6030 by @andrew05060414 on current
dev, with the fixes from the maintainer review.fnm, nvm, volta, mise and asdf can put a shell-local
*_multishellsdirectory first onPATH. That directory disappears when the installing shell exits.ocx service installorocx service repairrun from such a shell could bake it into the service as its launcher or into itsPATH, and the service then failed to start after logout or reboot. This PR:isTransientServiceLauncherPathandfilterTransientServicePathinsrc/service/state.ts. They match a path component named(asdf|fnm|mise|nvm|volta)_multishell(s), including Windows spellings.stableLauncherEntry, both for the recorded launcher and during thePATHscan, and filters them out of thePATHrendered into the systemd unit, the launchd plist, and the Windows service script. The package-local Bun runtime and every non-PATHvariable are unchanged.src/lib/transient-service-path.tsto avoid an import cycle.ocx service statusand diagnostics report a recorded temporary launcher as stale, and has repair say it is replacing it. The message no longer tells the user to runocx service repairwhile repair is already running.Fixes for the review on the #6030 head (
740b68da54):reusePreviousPlistPathVariablenow compares the whole normalized plist. It reuses the loaded job'sPATHonly whenPATHis the sole difference, so a changedHTTPS_PROXY(or any other field) always installs the new definition.PATHchanges the plist bytes, so repair goes through the existing guarded bootout/bootstrap path and reportsreloaded: true. Tests assert both the bytes and the launchctl verbs for the cleanup-only case and for cleanup combined with a proxy change.This PR leaves out the Codex shim and
ocx connectreadiness changes from #6030. The review found that the shim part could wrap an unresolved temporary target, and it would also exceed the file-size caps ofsrc/codex/shim.tsandtests/codex-integration/codex-shim.test.ts. It should be reviewed separately.Supersedes the launcher part of #6030.
Co-authored-by: Andrew shiyuanwang@ucsb.edu
Verification
bun test tests/service/service-multishell-path.test.ts(new): 7 pass.tests/service/launchd-repair.test.ts: 60 pass.tests/service/service.test.ts: 210 pass.tests/service/systemd-install-cleanup-hardening.test.ts: 1 pass.Other suites that render or diagnose service definitions:
bun test tests/codex-integration/codex-service-manager-probe.test.ts tests/codex-integration/doctor.test.ts tests/service/autostart-health.test.ts tests/service/service-ownership-handover.test.ts tests/service/service-wsl-home-ownership.test.ts tests/service/standalone-service.test.ts tests/windows/windows-service-wrappers.test.ts: 211 pass, 1 skip, 0 fail.bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 0 fail.bun run typecheck,bun run structure:check,bun run privacy:scan: pass.Still open: a real launchd and systemd lifecycle on a machine where the service was installed from an fnm shell, with the shell closed afterwards. The maintainer asked for this proof, and unit tests do not provide it.
Local tests after the review round were skipped by maintainer instruction (no local
bun test/typecheck/test:changed). The GitHub PR CI on the current head is the verification evidence for the latest commits. The local results above predate that instruction.Checklist
docs-site/.../getting-started/installation.md,structure/ops/service-and-sidecars.md, andstructure/runtime.md)PATHentries a service may bake in. It adds no tokens, credentials or workflow changes.