fix(symphony): use resolveGhPath() for gh CLI detection#687
Merged
pedramamini merged 2 commits intoRunMaestro:rcfrom Mar 29, 2026
Merged
fix(symphony): use resolveGhPath() for gh CLI detection#687pedramamini merged 2 commits intoRunMaestro:rcfrom
pedramamini merged 2 commits intoRunMaestro:rcfrom
Conversation
Symphony features called `execFileNoThrow('gh', ...)` directly without
using the existing `resolveGhPath()` utility. This meant gh CLI was not
found when installed in non-standard locations (official MSI installer,
Scoop, Chocolatey, Winget) because:
1. `gh` was missing from `knownExeCommands` in execFile.ts, forcing
unnecessary shell execution on Windows
2. No known installation paths existed for `gh` in the path prober
3. Symphony files bypassed the `resolveGhPath()` detection that other
features (git handlers, Cue poller) already used correctly
Changes:
- Add `gh` to `knownExeCommands` for direct execution on Windows
- Add `gh` known paths for Windows (MSI, Winget, Scoop, Chocolatey)
and Unix (Homebrew, local bin, Linuxbrew)
- Add GitHub CLI MSI install dir to expanded env PATH
- Replace all bare `'gh'` calls in symphony-fork.ts, symphony-runner.ts,
and symphony.ts IPC handler with `resolveGhPath()`
- Update test mocks to include cliDetection
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaced hardcoded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR correctly threads Key changes:
Issues found:
Confidence Score: 4/5Safe to merge with one targeted fix: add C:\Program Files\GitHub CLI to buildExpandedPath() in shared/pathUtils.ts to close the detection gap for MSI-installed gh in packaged Electron apps. The P1 finding describes a present, real-world gap: resolveGhPath() uses isGhInstalled() backed by buildExpandedEnv() from shared/pathUtils.ts, which doesn't include the GitHub CLI MSI directory. For symphony-runner.ts callers that pass no env to execFileNoThrow, this can cause ENOENT when gh is installed via MSI in an Electron environment that doesn't inherit the full system PATH. All other changes are mechanically correct and well-tested, so a single small addition to buildExpandedPath() would bring the score to 5. src/main/agents/path-prober.ts and src/shared/pathUtils.ts (not in diff) — the GitHub CLI MSI path needs to be added to buildExpandedPath() in pathUtils.ts to match what was added to path-prober.ts's getExpandedEnv(). Important Files Changed
Sequence DiagramsequenceDiagram
participant S as symphony.ts / symphony-runner.ts / symphony-fork.ts
participant R as resolveGhPath()<br/>(cliDetection.ts)
participant D as isGhInstalled()<br/>(cliDetection.ts)
participant PU as buildExpandedEnv()<br/>(shared/pathUtils.ts)
participant PP as getExpandedEnv()<br/>(path-prober.ts)
participant E as execFileNoThrow()<br/>(execFile.ts)
S->>R: await resolveGhPath()
R->>D: await isGhInstalled()
D->>PU: getExpandedEnv() → buildExpandedEnv()
Note over PU: ⚠️ Does NOT include C:\Program Files\GitHub CLI
PU-->>D: expanded PATH (missing GH CLI dir)
D->>E: execFile('where', ['gh'], env)
alt gh found in PATH
E-->>D: exitCode=0, stdout=full path
D-->>R: ghPathCache = full path
R-->>S: full path (e.g. C:\Program Files\GitHub CLI\gh.exe)
else gh NOT found (MSI install, Electron no PATH inherit)
E-->>D: exitCode≠0
D-->>R: ghPathCache = null
R-->>S: fallback 'gh'
end
alt symphony.ts (passes getExpandedEnv from path-prober)
S->>PP: getExpandedEnv()
Note over PP: ✅ Includes C:\Program Files\GitHub CLI
PP-->>S: expanded env with GH CLI dir
S->>E: execFileNoThrow('gh', args, cwd, expandedEnv)
Note over E: Resolves 'gh' from env PATH ✅
else symphony-runner.ts (no env passed)
S->>E: execFileNoThrow('gh', args, cwd)
Note over E: ⚠️ Uses process.env.PATH only — may fail if GH CLI dir not inherited
end
Reviews (1): Last reviewed commit: "fix(symphony): use resolveGhPath() for g..." | Re-trigger Greptile |
![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/__tests__/main/services/symphony-runner.test.ts (1)
41-44: Add one assertion thatresolveGhPath()is actually used.Right now the mock keeps existing
execFileNoThrow('gh', ...)assertions passing, but tests would still pass if production regressed to a hardcoded'gh'. Add at least oneexpect(resolveGhPath).toHaveBeenCalled()in a GH workflow test.Example assertion addition
+import { resolveGhPath } from '../../../main/utils/cliDetection'; @@ it('calls gh pr create with --draft flag', async () => { mockSuccessfulWorkflow(); @@ expect(execFileNoThrow).toHaveBeenCalledWith( 'gh', expect.arrayContaining(['pr', 'create', '--draft']), '/tmp/test-repo' ); + expect(resolveGhPath).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/services/symphony-runner.test.ts` around lines 41 - 44, Add an assertion to the GH workflow test that the mocked resolveGhPath was invoked: after the test exercise that triggers GitHub CLI usage, add expect(resolveGhPath).toHaveBeenCalled() (or toHaveBeenCalledTimes(1)) to ensure the mock is actually used; locate the mock defined by vi.mock('../../../main/utils/cliDetection') and place the assertion in the corresponding GH workflow test case that currently checks execFileNoThrow('gh', ...).src/__tests__/main/utils/symphony-fork.test.ts (1)
23-25: Consider assertingresolveGhPath()usage in oneensureForkSetuptest.The mock is correct, but a direct invocation assertion would ensure the code path doesn’t silently drift back to hardcoded
'gh'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/utils/symphony-fork.test.ts` around lines 23 - 25, Add an assertion in one of the ensureForkSetup tests to verify the mocked resolveGhPath() is actually called (e.g., expect(resolveGhPath).toHaveBeenCalled()/toHaveBeenCalledTimes(1)) so the test ensures the code uses the CLI-detection path instead of a hardcoded 'gh'; update the test that invokes ensureForkSetup to import or reference the mocked resolveGhPath and assert its invocation after calling ensureForkSetup.
🤖 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/agents/path-prober.ts`:
- Around line 321-333: resolveGhPath/isGhInstalled fails to detect gh installed
via Linuxbrew because getExpandedEnv (used by resolveGhPath) doesn't include the
Linuxbrew prefix; add the Linuxbrew bin (/home/linuxbrew/.linuxbrew/bin and
optionally /home/linuxbrew/.linuxbrew/sbin) to the expanded PATH returned by
getExpandedEnv (or include those entries in the same PATH-expansion helper used
by resolveGhPath/isGhInstalled) so the probe entries for 'gh' (the gh array,
localBin, and the Linuxbrew probe rows) are actually reachable when which/where
runs.
---
Nitpick comments:
In `@src/__tests__/main/services/symphony-runner.test.ts`:
- Around line 41-44: Add an assertion to the GH workflow test that the mocked
resolveGhPath was invoked: after the test exercise that triggers GitHub CLI
usage, add expect(resolveGhPath).toHaveBeenCalled() (or
toHaveBeenCalledTimes(1)) to ensure the mock is actually used; locate the mock
defined by vi.mock('../../../main/utils/cliDetection') and place the assertion
in the corresponding GH workflow test case that currently checks
execFileNoThrow('gh', ...).
In `@src/__tests__/main/utils/symphony-fork.test.ts`:
- Around line 23-25: Add an assertion in one of the ensureForkSetup tests to
verify the mocked resolveGhPath() is actually called (e.g.,
expect(resolveGhPath).toHaveBeenCalled()/toHaveBeenCalledTimes(1)) so the test
ensures the code uses the CLI-detection path instead of a hardcoded 'gh'; update
the test that invokes ensureForkSetup to import or reference the mocked
resolveGhPath and assert its invocation after calling ensureForkSetup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df960896-78bd-48df-9ade-08feb35e311a
📒 Files selected for processing (8)
src/__tests__/main/ipc/handlers/symphony.test.tssrc/__tests__/main/services/symphony-runner.test.tssrc/__tests__/main/utils/symphony-fork.test.tssrc/main/agents/path-prober.tssrc/main/ipc/handlers/symphony.tssrc/main/services/symphony-runner.tssrc/main/utils/execFile.tssrc/main/utils/symphony-fork.ts
Address review feedback: - Add GitHub CLI MSI dir to buildExpandedPath() in shared/pathUtils.ts so resolveGhPath() can detect gh via isGhInstalled() (P1 - Greptile) - Add Linuxbrew bin dir to buildExpandedPath() Unix paths so gh installed via Linuxbrew is discoverable (P1 - CodeRabbit) - Fix inaccurate "Conda / Mamba" comment on ~/bin to "User bin directory" (P2 - Greptile) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
ghtoknownExeCommandsinexecFile.tsso Windows doesn't force unnecessary shell executionghknown installation paths for Windows (MSI installer, Winget, Scoop, Chocolatey) and Unix (Homebrew, local bin, Linuxbrew) to the path prober'gh'calls insymphony-fork.ts,symphony-runner.ts, andsymphony.tsIPC handler withresolveGhPath()— matching the pattern already used bygit.tsandcue-github-poller.tscliDetectionin affected test filesContext
Symphony features called
execFileNoThrow('gh', ...)directly, bypassing the existingresolveGhPath()utility fromcliDetection.ts. This meantghCLI was not found when installed in non-standard locations because:ghwas missing fromknownExeCommands, forcing shell execution on Windowsghin the path probercliDetection.tswas unused by SymphonyOther features like
git.tsIPC handlers and the Cue GitHub poller already usedresolveGhPath()correctly.Test plan
execFile.tstests pass (27/27)path-prober.tstests pass (24/24)symphony-fork.tstests pass (9/9)symphony-runner.tstests pass (71/71)symphony.tsIPC handler tests pass (219/219)tsconfig.main.json,tsconfig.lint.json,tsconfig.cli.json)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests