fix: include version manager bins in expanded PATH#562
Open
brownmike wants to merge 4 commits intoRunMaestro:mainfrom
Open
fix: include version manager bins in expanded PATH#562brownmike wants to merge 4 commits intoRunMaestro:mainfrom
brownmike wants to merge 4 commits intoRunMaestro:mainfrom
Conversation
Packaged Maestro can detect Node-backed CLIs like Codex in nvm or fnm installations but still fail to launch them. Agent detection probes those locations directly, while child-process PATH expansion previously omitted the detected version manager bin directories. Add detected version manager bins to the shared expanded PATH and preserve the intended precedence when prepending paths. This keeps nvm/current/bin ahead of versioned installs and prevents packaged launches from failing with 'env: node: No such file or directory'. Add regression coverage for both the shared PATH builder and the child process environment builder so packaged-path resolution stays aligned with agent detection.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds detection and inclusion of Node version-manager bin directories into PATH expansion and child-process environment building, fixes prepending order to preserve left-to-right precedence, and adds tests and test-mock adjustments to cover the behavior. Changes
Sequence Diagram(s)(Skipped — changes are limited to PATH construction and tests and do not introduce multi-component sequential flows requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
brownmike
changed the title
Greptile SummaryThis PR fixes a gap where packaged Maestro could detect Node CLIs (e.g. Codex) inside nvm/fnm installations during agent-detection probes, but then fail to launch them because Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["buildExpandedPath(customPaths?)"] --> B["detectNodeVersionManagerBinPaths()"]
B --> C{isWindows?}
C -- Yes --> D["return []"]
C -- No --> E["Probe NVM_DIR → current/bin + versions/node/**/bin"]
E --> F["Probe fnm aliases/default + node-versions/**/bin"]
F --> G["Probe volta, mise, asdf, n shims"]
G --> H["return versionManagerPaths[]"]
H --> I["Build additionalPaths\n[...versionManagerPaths, homebrew, local, ...]"]
D --> J["Build Windows additionalPaths\n[npm, git, dotnet, ...]"]
A --> K["pathParts = process.env.PATH.split(':')"]
I --> L["Reverse-iterate additionalPaths\nunshift each into pathParts"]
J --> L
K --> L
A --> M{customPaths?}
M -- Yes --> N["Reverse-iterate customPaths\nunshift each into pathParts"]
M -- No --> L
N --> L
L --> O["return pathParts.join(':')"]
O --> P["buildChildProcessEnv()\nenv.PATH = buildExpandedPath()"]
O --> Q["buildExpandedEnv()\nenv.PATH = buildExpandedPath()"]
Last reviewed commit: f2ba473 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/shared/pathUtils.ts (1)
391-407: Comment at line 391 is misleading.The comment says "Add custom paths first" but due to the order of operations (customPaths processed before additionalPaths, both using
unshift),additionalPaths(including version manager paths) actually end up beforecustomPathsin the final PATH. This appears intentional based on the test expectations (version manager paths should be first), but the comment doesn't reflect the actual behavior.📝 Suggested comment fix
- // Add custom paths first (if provided) + // Add custom paths (if provided) - these will appear after additionalPaths in final PATH if (customPaths && customPaths.length > 0) { for (let i = customPaths.length - 1; i >= 0; i--) { const p = customPaths[i]; if (!pathParts.includes(p)) { pathParts.unshift(p); } } } - // Add standard additional paths + // Add standard additional paths (these will be prepended, appearing first in final PATH) for (let i = additionalPaths.length - 1; i >= 0; i--) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/pathUtils.ts` around lines 391 - 407, The inline comment "Add custom paths first (if provided)" is misleading because the code processes customPaths then additionalPaths with unshift, causing additionalPaths to appear before customPaths in the final path; update the comment near the loop handling customPaths/additionalPaths in pathUtils.ts to accurately state that additionalPaths (e.g., version manager paths) will take precedence and will be placed before any customPaths in pathParts, or adjust wording to clarify the intended ordering semantics involving customPaths, additionalPaths, and pathParts.src/__tests__/shared/pathUtils.test.ts (1)
281-308: Useos.tmpdir()instead of hardcoded/tmpfor cross-platform compatibility.Line 284 uses a hardcoded
/tmp/maestro-nvm-prefix. While this test runs under a mockeddarwinplatform, usingos.tmpdir()is more consistent with the pattern used inenvBuilder.test.tsand avoids potential issues if the test setup changes.♻️ Suggested fix
it('should prepend detected Node version manager bin paths', () => { process.env.PATH = '/usr/bin'; const originalNvmDir = process.env.NVM_DIR; - const tempNvmDir = fs.mkdtempSync('/tmp/maestro-nvm-'); + const tempNvmDir = fs.mkdtempSync(path.join(os.tmpdir(), 'maestro-nvm-')); process.env.NVM_DIR = tempNvmDir;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/shared/pathUtils.test.ts` around lines 281 - 308, Replace the hardcoded '/tmp/maestro-nvm-' prefix passed to fs.mkdtempSync with a prefix based on os.tmpdir() so the temp directory is created in the OS temp location; specifically import/use os.tmpdir() (or require('os')) and call fs.mkdtempSync(path.join(os.tmpdir(), 'maestro-nvm-')) when creating tempNvmDir in the test that exercises buildExpandedPath(), leaving the rest of the temp cleanup and NVM_DIR restore logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/process-manager/utils/__tests__/envBuilder.test.ts`:
- Around line 309-335: The test in envBuilder.test.ts is platform-dependent
because detectNodeVersionManagerBinPaths (in pathUtils.ts) returns empty on
Windows, causing assertions in the buildChildProcessEnv() test to fail; fix by
either (A) mocking process.platform to a Unix-like value (e.g., set
process.platform to 'darwin' for the duration of the test and restore it
afterward) so detectNodeVersionManagerBinPaths yields the expected paths before
calling buildChildProcessEnv(), or (B) skip the test on Windows by checking
process.platform and using test.skip or conditional logic to not run the
assertions on 'win32'. Restore any mutated state (process.platform,
process.env.NVM_DIR) in finally blocks.
---
Nitpick comments:
In `@src/__tests__/shared/pathUtils.test.ts`:
- Around line 281-308: Replace the hardcoded '/tmp/maestro-nvm-' prefix passed
to fs.mkdtempSync with a prefix based on os.tmpdir() so the temp directory is
created in the OS temp location; specifically import/use os.tmpdir() (or
require('os')) and call fs.mkdtempSync(path.join(os.tmpdir(), 'maestro-nvm-'))
when creating tempNvmDir in the test that exercises buildExpandedPath(), leaving
the rest of the temp cleanup and NVM_DIR restore logic unchanged.
In `@src/shared/pathUtils.ts`:
- Around line 391-407: The inline comment "Add custom paths first (if provided)"
is misleading because the code processes customPaths then additionalPaths with
unshift, causing additionalPaths to appear before customPaths in the final path;
update the comment near the loop handling customPaths/additionalPaths in
pathUtils.ts to accurately state that additionalPaths (e.g., version manager
paths) will take precedence and will be placed before any customPaths in
pathParts, or adjust wording to clarify the intended ordering semantics
involving customPaths, additionalPaths, and pathParts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9fe5869-7a8c-49bc-869a-ad596d347a36
📒 Files selected for processing (3)
src/__tests__/shared/pathUtils.test.tssrc/main/process-manager/utils/__tests__/envBuilder.test.tssrc/shared/pathUtils.ts
Use os.tmpdir() in the pathUtils regression test instead of a hardcoded temporary directory path, and clarify the PATH-prepend comment to explain why the arrays are iterated in reverse. These changes keep the implementation the same while addressing the review notes on portability and code clarity.
Force the regression test to run under a Unix-like platform value so it exercises version manager path detection consistently across local runs and CI, including Windows. This addresses the remaining review concern without changing the production code path.
Collaborator
|
didnt manage to finish #535 yet, so closing it for this PR |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/releases.md (3)
201-201: Minor: Capitalize "GitHub" properly.Per static analysis, "Github" should be "GitHub" (capital H).
📝 Suggested fix
-🌳 Github Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`. +🌳 GitHub Worktree support was added. Any agent bound to a Git repository has the option to enable worktrees, each of which show up as a sub-agent with their own write-lock and Auto Run capability. Now you can truly develop in parallel on the same project and issue PRs when you're ready, all from within Maestro. Huge improvement, major thanks to `@petersilberman`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 201, The release note contains the incorrect capitalization "Github Worktree support"; update the string in docs/releases.md to "GitHub Worktree support" (replace "Github" with "GitHub") so the release entry uses the proper product casing.
22-22: Minor: Hyphenate compound adjectives.Per static analysis, "open source projects" should be "open-source projects" when used as a compound adjective.
📝 Suggested fix
-🎶 **Maestro Symphony** — Contribute to open source with AI assistance! Browse curated issues from projects with the `runmaestro.ai` label, clone repos with one click, and automatically process the relevant Auto Run playbooks. Track your contributions, streaks, and stats. You're contributing CPU and tokens towards your favorite open source projects and features. +🎶 **Maestro Symphony** — Contribute to open-source with AI assistance! Browse curated issues from projects with the `runmaestro.ai` label, clone repos with one click, and automatically process the relevant Auto Run playbooks. Track your contributions, streaks, and stats. You're contributing CPU and tokens towards your favorite open-source projects and features.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 22, In the "Maestro Symphony" release blurb replace the compound adjective "open source projects" with the hyphenated form "open-source projects" in the sentence that begins with "Contribute to open source with AI assistance!" to satisfy the static analysis rule; update that exact phrase in the diffed line so the text reads "Browse curated issues from projects with the `runmaestro.ai` label, clone repos with one click, and automatically process the relevant Auto Run playbooks." using "open-source projects" where applicable.
176-176: Minor: Hyphenate "built-in".Per static analysis, "built in updater" should be "built-in updater".
📝 Suggested fix
-🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built in updater to grab the latest prompts from the repository. We do override `/speckit-implement` (the final step) to create Auto Run docs and guide the user through their execution, which thanks to Wortrees from v0.11.x allows us to run in parallel! +🎯 Added [GitHub Spec-Kit](https://github.com/github/spec-kit) commands into Maestro with a built-in updater to grab the latest prompts from the repository. We do override `/speckit-implement` (the final step) to create Auto Run docs and guide the user through their execution, which thanks to Worktrees from v0.11.x allows us to run in parallel!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 176, Update the phrase "built in updater" to the correct hyphenated form "built-in updater" in the release notes sentence referencing GitHub Spec-Kit so the line now reads that Maestro includes a "built-in updater" to grab the latest prompts; edit the string in the release notes paragraph containing "GitHub Spec-Kit" / "Maestro" / "/speckit-implement" to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/releases.md`:
- Line 201: The release note contains the incorrect capitalization "Github
Worktree support"; update the string in docs/releases.md to "GitHub Worktree
support" (replace "Github" with "GitHub") so the release entry uses the proper
product casing.
- Line 22: In the "Maestro Symphony" release blurb replace the compound
adjective "open source projects" with the hyphenated form "open-source projects"
in the sentence that begins with "Contribute to open source with AI assistance!"
to satisfy the static analysis rule; update that exact phrase in the diffed line
so the text reads "Browse curated issues from projects with the `runmaestro.ai`
label, clone repos with one click, and automatically process the relevant Auto
Run playbooks." using "open-source projects" where applicable.
- Line 176: Update the phrase "built in updater" to the correct hyphenated form
"built-in updater" in the release notes sentence referencing GitHub Spec-Kit so
the line now reads that Maestro includes a "built-in updater" to grab the latest
prompts; edit the string in the release notes paragraph containing "GitHub
Spec-Kit" / "Maestro" / "/speckit-implement" to apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c29fd465-357a-4939-9c6c-85540e245ea8
📒 Files selected for processing (3)
docs/releases.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/__tests__/main/agents/session-storage.test.ts
brownmike
force-pushed
the
fix/codex-version-manager-paths
branch
from
548327e to
2f5be14
Compare
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
|
manual test working, also fixed agents not being able to run |
chr1syy
added a commit
to chr1syy/Maestro
that referenced
this pull request
Mar 13, 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.
Packaged Maestro can detect Node-backed CLIs like Codex in nvm or fnm installations but still fail to launch them. Agent detection probes those locations directly, while child-process PATH expansion previously omitted the detected version manager bin directories.
Add detected version manager bins to the shared expanded PATH and preserve the intended precedence when prepending paths. This keeps nvm/current/bin ahead of versioned installs and prevents packaged launches from failing with 'env: node: No such file or directory'.
Add regression coverage for both the shared PATH builder and the child process environment builder so packaged-path resolution stays aligned with agent detection.
Resolves #561
Summary by CodeRabbit
New Features
Tests