fix(code): preserve inherited PATH so husky hooks work in Create PR#2051
Open
fix(code): preserve inherited PATH so husky hooks work in Create PR#2051
Conversation
`fixPath` was replacing `process.env.PATH` with what `zsh -lc` returns. `-lc` only sources `.zprofile`, not `.zshrc`, so version-manager paths (nvm/mise/volta — where `pnpm`, `node`, `husky` typically live) added by `.zshrc` got wiped out. When `getCleanEnv` then snapshotted PATH for git child processes, the husky pre-commit hook ran without those tools and failed with "command not found", blocking the Create PR button even though the same hook ran fine from a terminal. Now merge the inherited PATH with the resolved shell PATH and fallbacks (deduped), instead of replacing. Terminal launches keep their rich PATH; Finder/Spotlight launches still get the shell PATH filled in. Generated-By: PostHog Code Task-Id: cf740c81-d28c-4ce9-9573-c016a5c0e773
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/main/utils/fixPath.test.ts:154-192
**Prefer parameterised tests for repeated structure**
The two new tests ("preserves entries from the inherited PATH that the login shell lacks" and "preserves inherited PATH entries when reading from the cache") have nearly identical structure: set `process.env.PATH` to a rich terminal PATH, configure either the shell-resolution or cache mock, call `fixPath()`, and assert the same inherited entries survive. Per the team's convention, these should be collapsed into a single `it.each` that drives both code paths (live shell and cache hit) from one set of assertions, keeping the inherited-PATH check DRY.
Reviews (1): Last reviewed commit: "fix(code): preserve inherited PATH so hu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+154
to
+192
| it("preserves entries from the inherited PATH that the login shell lacks", async () => { | ||
| // Simulate launching from a terminal where .zshrc has added nvm/mise | ||
| // paths that the -lc resolution (only sources .zprofile) won't see. | ||
| process.env.PATH = | ||
| "/Users/me/.nvm/versions/node/v22.0.0/bin:/Users/me/.local/share/pnpm:/usr/bin:/bin"; | ||
| mockSpawnSync.mockReturnValue({ | ||
| status: 0, | ||
| stdout: shellOutput("/opt/homebrew/bin:/usr/bin:/bin"), | ||
| }); | ||
|
|
||
| const { fixPath } = await import("./fixPath"); | ||
| fixPath(); | ||
|
|
||
| const parts = process.env.PATH?.split(":") ?? []; | ||
| // Inherited entries (added by .zshrc, missing from .zprofile) survive. | ||
| expect(parts).toContain("/Users/me/.nvm/versions/node/v22.0.0/bin"); | ||
| expect(parts).toContain("/Users/me/.local/share/pnpm"); | ||
| // Shell-resolved entries are still merged in. | ||
| expect(parts).toContain("/opt/homebrew/bin"); | ||
| }); | ||
|
|
||
| it("preserves inherited PATH entries when reading from the cache", async () => { | ||
| process.env.PATH = "/Users/me/.nvm/versions/node/v22.0.0/bin:/usr/bin:/bin"; | ||
| mockExistsSync.mockReturnValue(true); | ||
| mockReadFileSync.mockReturnValue( | ||
| JSON.stringify({ | ||
| path: "/opt/homebrew/bin:/usr/bin:/bin", | ||
| timestamp: Date.now(), | ||
| }), | ||
| ); | ||
|
|
||
| const { fixPath } = await import("./fixPath"); | ||
| fixPath(); | ||
|
|
||
| const parts = process.env.PATH?.split(":") ?? []; | ||
| expect(parts).toContain("/Users/me/.nvm/versions/node/v22.0.0/bin"); | ||
| expect(parts).toContain("/opt/homebrew/bin"); | ||
| expect(mockSpawnSync).not.toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
The two new tests ("preserves entries from the inherited PATH that the login shell lacks" and "preserves inherited PATH entries when reading from the cache") have nearly identical structure: set process.env.PATH to a rich terminal PATH, configure either the shell-resolution or cache mock, call fixPath(), and assert the same inherited entries survive. Per the team's convention, these should be collapsed into a single it.each that drives both code paths (live shell and cache hit) from one set of assertions, keeping the inherited-PATH check DRY.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/utils/fixPath.test.ts
Line: 154-192
Comment:
**Prefer parameterised tests for repeated structure**
The two new tests ("preserves entries from the inherited PATH that the login shell lacks" and "preserves inherited PATH entries when reading from the cache") have nearly identical structure: set `process.env.PATH` to a rich terminal PATH, configure either the shell-resolution or cache mock, call `fixPath()`, and assert the same inherited entries survive. Per the team's convention, these should be collapsed into a single `it.each` that drives both code paths (live shell and cache hit) from one set of assertions, keeping the inherited-PATH check DRY.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
charlesvien
approved these changes
May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fixPathwas replacingprocess.env.PATHwith whatzsh -lcreturns.-lconly sources.zprofile, not.zshrc, so version-manager paths added by.zshrc(nvm/mise/volta — wherepnpm,node,huskytypically live) got wiped out. WhengetCleanEnvlater snapshotted PATH for git child processes, the husky hook ran without those tools and failed with "command not found".-ilc(full.zshrc) because of the zombie-process risk called out in #1399, so merging is the safer fix.Test plan
pnpm --filter code test src/main/utils/fixPath.test.ts— 8/8 passing (added 2 new tests for the inherited-PATH scenario, both with shell-resolved and cached paths)pnpm --filter code typecheck— cleanNote: existing users may need to wait up to 1h for the cached PATH to refresh, or delete `~/Library/Application Support/@posthog/posthog-code/shell-env-cache.json`. The cache only stores the shell PATH, which is now merged on top of the inherited PATH on every boot.
Generated-By: PostHog Code
Task-Id: cf740c81-d28c-4ce9-9573-c016a5c0e773