fix(backend): improve gh CLI detection for PR creation (ACS-247) (#1071)
* fix(backend): improve gh CLI detection for PR creation Fixes ACS-247 - Unable to Create PR - gh cli not found error The Python backend was using bare "gh" command which relies on the gh CLI being in the system PATH. On some systems, gh is installed in locations not in PATH (e.g., Homebrew on macOS, Program Files on Windows). Changes: - Add new gh_executable.py module for platform-specific gh CLI detection * Follows same pattern as git_executable.py * Checks GITHUB_CLI_PATH env var (from frontend) * Uses shutil.which() with fallback paths * Supports Homebrew (macOS), Program Files (Windows) - Update worktree.py to use detected gh path - Update frontend to pass GITHUB_CLI_PATH to Python backend This ensures PR creation works reliably even when gh CLI is not in the system PATH but is installed in common locations. Refs: ACS-247 * refactor: address PR review feedback on gh_executable.py - Fix unused global variable: return cached value instead of uncached - Extract repeated subprocess.run pattern into helper functions: * _verify_gh_executable() - validates gh by checking version * _run_where_command() - runs Windows 'where' command - Add explicit encoding='utf-8' to all subprocess.run calls - Add invalidate_gh_cache() function for cache invalidation Addresses review comments on PR #1071: - Unused global variable warning (Code Scanning) - Repeated subprocess.run pattern (Gemini Code Assist) - Missing explicit encoding (Gemini Code Assist) - Cache invalidation for edge cases (CodeRabbit) Refs: ACS-247, PR #1071 * refactor: address remaining PR review feedback - Add explanatory comment to except clause in _run_where_command() - Make _run_where_command() more specific by hardcoding "where gh" * Removes generic command parameter to reduce shell=True risk surface * Uses list argument ["where", "gh"] instead of string command - This addresses Code Scanning alert for empty except clause - This addresses CodeRabbit feedback about shell=True security Addresses additional review comments on PR #1071: - Empty except clause (Code Scanning) - Shell=True risk surface reduction (CodeRabbit) Refs: ACS-247, PR #1071 * fix: correct subprocess.run usage for Windows where command Fix bug where list argument ["where", "gh"] was used with shell=True, which is incorrect on Windows. When using shell=True, the command must be passed as a string, not a list. Changed from: subprocess.run(["where", "gh"], ..., shell=True) Changed to: subprocess.run("where gh", ..., shell=True) This follows the same pattern as git_executable.py and fixes the Sentry/CodeRabbit alerts about incorrect subprocess usage. Addresses additional review comments on PR #1071: - shell=True with list argument (CodeRabbit) - subprocess.run bug (Sentry) Refs: ACS-247, PR #1071 * refactor: remove redundant Windows path checks Remove hardcoded Windows paths that are redundant with the os.path.expandvars() calls. The expandvars calls will resolve to the same values as the hardcoded paths, so keeping both is unnecessary. Removed: - r"C:\Program Files\GitHub CLI\gh.exe" - r"C:\Program Files (x86)\GitHub CLI\gh.exe" Kept: - os.path.expandvars(r"%PROGRAMFILES%\GitHub CLI\gh.exe") - os.path.expandvars(r"%PROGRAMFILES(X86)%\GitHub CLI\gh.exe") - os.path.expandvars(r"%LOCALAPPDATA%\Programs\GitHub CLI\gh.exe") Addresses CodeRabbit review comment on PR #1071: - Minor redundancy in Windows path checks Refs: ACS-247, PR #1071 * fix: address PR review feedback on gh_executable.py Address 6 findings from PR review: - Add cache validation: check cached path still exists before returning - Add run_gh() helper function to match run_git() pattern - Validate _run_where_command() result with _verify_gh_executable() - Add comment explaining shell=True requirement for Windows 'where' builtin - Invalidate cache when FileNotFoundError occurs in worktree.py These changes improve robustness of gh CLI detection and error handling, ensuring stale cache entries are properly handled and the module follows the same patterns as git_executable.py. Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> * docs: fix misleading comment about Windows 'where' command The comment incorrectly stated that 'where' is a Windows shell builtin. It is actually a standalone executable (where.exe). Updated comment to accurately reflect that shell=True is required for proper command execution. Addresses review comment #9 from PR #1071. Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> * docs: clarify cache invalidation comment in FileNotFoundError handler The previous comment suggested the cache was invalidated "in case it was reinstalled", but this handler is reached when the cached path became invalid between get_gh_executable() check and subprocess.run() execution (e.g., file was deleted/moved). Updated comment to accurately reflect the purpose: clear stale cache so next call re-discovers the gh path. Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> * fix: add cache invalidation in _get_existing_pr_url FileNotFoundError handler For consistency with create_pull_request(), invalidate gh cache when FileNotFoundError is caught. This ensures stale cached paths are cleared if the gh executable becomes invalid between get_gh_executable() check and subprocess.run() execution. Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> --------- Signed-off-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
@@ -0,0 +1,191 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
GitHub CLI Executable Finder
|
||||
============================
|
||||
|
||||
Utility to find the gh (GitHub CLI) executable, with platform-specific fallbacks.
|
||||
"""
|
||||
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
|
||||
_cached_gh_path: str | None = None
|
||||
|
||||
|
||||
def invalidate_gh_cache() -> None:
|
||||
"""Invalidate the cached gh executable path.
|
||||
|
||||
Useful when gh may have been uninstalled, updated, or when
|
||||
GITHUB_CLI_PATH environment variable has changed.
|
||||
"""
|
||||
global _cached_gh_path
|
||||
_cached_gh_path = None
|
||||
|
||||
|
||||
def _verify_gh_executable(path: str) -> bool:
|
||||
"""Verify that a path is a valid gh executable by checking version.
|
||||
|
||||
Args:
|
||||
path: Path to the potential gh executable
|
||||
|
||||
Returns:
|
||||
True if the path points to a valid gh executable, False otherwise
|
||||
"""
|
||||
try:
|
||||
result = subprocess.run(
|
||||
[path, "--version"],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
timeout=5,
|
||||
)
|
||||
return result.returncode == 0
|
||||
except (subprocess.TimeoutExpired, OSError):
|
||||
return False
|
||||
|
||||
|
||||
def _run_where_command() -> str | None:
|
||||
"""Run Windows 'where gh' command to find gh executable.
|
||||
|
||||
Returns:
|
||||
First path found, or None if command failed
|
||||
"""
|
||||
try:
|
||||
result = subprocess.run(
|
||||
"where gh",
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
timeout=5,
|
||||
shell=True, # Required: 'where' command must be executed through shell on Windows
|
||||
)
|
||||
if result.returncode == 0 and result.stdout.strip():
|
||||
found_path = result.stdout.strip().split("\n")[0].strip()
|
||||
if (
|
||||
found_path
|
||||
and os.path.isfile(found_path)
|
||||
and _verify_gh_executable(found_path)
|
||||
):
|
||||
return found_path
|
||||
except (subprocess.TimeoutExpired, OSError):
|
||||
# 'where' command failed or timed out - fall through to return None
|
||||
pass
|
||||
return None
|
||||
|
||||
|
||||
def get_gh_executable() -> str | None:
|
||||
"""Find the gh executable, with platform-specific fallbacks.
|
||||
|
||||
Returns the path to gh executable, or None if not found.
|
||||
|
||||
Priority order:
|
||||
1. GITHUB_CLI_PATH env var (user-configured path from frontend)
|
||||
2. shutil.which (if gh is in PATH)
|
||||
3. Homebrew paths on macOS
|
||||
4. Windows Program Files paths
|
||||
5. Windows 'where' command
|
||||
|
||||
Caches the result after first successful find. Use invalidate_gh_cache()
|
||||
to force re-detection (e.g., after gh installation/uninstallation).
|
||||
"""
|
||||
global _cached_gh_path
|
||||
|
||||
# Return cached result if available AND still exists
|
||||
if _cached_gh_path is not None and os.path.isfile(_cached_gh_path):
|
||||
return _cached_gh_path
|
||||
|
||||
_cached_gh_path = _find_gh_executable()
|
||||
return _cached_gh_path
|
||||
|
||||
|
||||
def _find_gh_executable() -> str | None:
|
||||
"""Internal function to find gh executable."""
|
||||
# 1. Check GITHUB_CLI_PATH env var (set by Electron frontend)
|
||||
env_path = os.environ.get("GITHUB_CLI_PATH")
|
||||
if env_path and os.path.isfile(env_path) and _verify_gh_executable(env_path):
|
||||
return env_path
|
||||
|
||||
# 2. Try shutil.which (works if gh is in PATH)
|
||||
gh_path = shutil.which("gh")
|
||||
if gh_path and _verify_gh_executable(gh_path):
|
||||
return gh_path
|
||||
|
||||
# 3. macOS-specific: check Homebrew paths
|
||||
if os.name != "nt": # Unix-like systems (macOS, Linux)
|
||||
homebrew_paths = [
|
||||
"/opt/homebrew/bin/gh", # Apple Silicon
|
||||
"/usr/local/bin/gh", # Intel Mac
|
||||
"/home/linuxbrew/.linuxbrew/bin/gh", # Linux Homebrew
|
||||
]
|
||||
for path in homebrew_paths:
|
||||
if os.path.isfile(path) and _verify_gh_executable(path):
|
||||
return path
|
||||
|
||||
# 4. Windows-specific: check Program Files paths
|
||||
if os.name == "nt":
|
||||
windows_paths = [
|
||||
os.path.expandvars(r"%PROGRAMFILES%\GitHub CLI\gh.exe"),
|
||||
os.path.expandvars(r"%PROGRAMFILES(X86)%\GitHub CLI\gh.exe"),
|
||||
os.path.expandvars(r"%LOCALAPPDATA%\Programs\GitHub CLI\gh.exe"),
|
||||
]
|
||||
for path in windows_paths:
|
||||
if os.path.isfile(path) and _verify_gh_executable(path):
|
||||
return path
|
||||
|
||||
# 5. Try 'where' command with shell=True (more reliable on Windows)
|
||||
return _run_where_command()
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def run_gh(
|
||||
args: list[str],
|
||||
cwd: str | None = None,
|
||||
timeout: int = 60,
|
||||
input_data: str | None = None,
|
||||
) -> subprocess.CompletedProcess:
|
||||
"""Run a gh command with proper executable finding.
|
||||
|
||||
Args:
|
||||
args: gh command arguments (without 'gh' prefix)
|
||||
cwd: Working directory for the command
|
||||
timeout: Command timeout in seconds (default: 60)
|
||||
input_data: Optional string data to pass to stdin
|
||||
|
||||
Returns:
|
||||
CompletedProcess with command results.
|
||||
"""
|
||||
gh = get_gh_executable()
|
||||
if not gh:
|
||||
return subprocess.CompletedProcess(
|
||||
args=["gh"] + args,
|
||||
returncode=-1,
|
||||
stdout="",
|
||||
stderr="GitHub CLI (gh) not found. Install from https://cli.github.com/",
|
||||
)
|
||||
try:
|
||||
return subprocess.run(
|
||||
[gh] + args,
|
||||
cwd=cwd,
|
||||
input=input_data,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
errors="replace",
|
||||
timeout=timeout,
|
||||
)
|
||||
except subprocess.TimeoutExpired:
|
||||
return subprocess.CompletedProcess(
|
||||
args=[gh] + args,
|
||||
returncode=-1,
|
||||
stdout="",
|
||||
stderr=f"Command timed out after {timeout} seconds",
|
||||
)
|
||||
except FileNotFoundError:
|
||||
return subprocess.CompletedProcess(
|
||||
args=[gh] + args,
|
||||
returncode=-1,
|
||||
stdout="",
|
||||
stderr="GitHub CLI (gh) executable not found. Install from https://cli.github.com/",
|
||||
)
|
||||
@@ -26,6 +26,7 @@ from datetime import datetime
|
||||
from pathlib import Path
|
||||
from typing import TypedDict, TypeVar
|
||||
|
||||
from core.gh_executable import get_gh_executable, invalidate_gh_cache
|
||||
from core.git_executable import get_git_executable, run_git
|
||||
from debug import debug_warning
|
||||
|
||||
@@ -959,9 +960,17 @@ class WorktreeManager:
|
||||
# Get PR body from spec.md if available
|
||||
pr_body = self._extract_spec_summary(spec_name)
|
||||
|
||||
# Find gh executable before attempting PR creation
|
||||
gh_executable = get_gh_executable()
|
||||
if not gh_executable:
|
||||
return PullRequestResult(
|
||||
success=False,
|
||||
error="GitHub CLI (gh) not found. Install from https://cli.github.com/",
|
||||
)
|
||||
|
||||
# Build gh pr create command
|
||||
gh_args = [
|
||||
"gh",
|
||||
gh_executable,
|
||||
"pr",
|
||||
"create",
|
||||
"--base",
|
||||
@@ -1063,7 +1072,8 @@ class WorktreeManager:
|
||||
)
|
||||
|
||||
except FileNotFoundError:
|
||||
# gh CLI not installed
|
||||
# Cached gh path became invalid - clear cache so next call re-discovers
|
||||
invalidate_gh_cache()
|
||||
return PullRequestResult(
|
||||
success=False,
|
||||
error="gh CLI not found. Install from https://cli.github.com/",
|
||||
@@ -1121,9 +1131,23 @@ class WorktreeManager:
|
||||
if not info:
|
||||
return None
|
||||
|
||||
gh_executable = get_gh_executable()
|
||||
if not gh_executable:
|
||||
# gh CLI not found - return None and let caller handle it
|
||||
return None
|
||||
|
||||
try:
|
||||
result = subprocess.run(
|
||||
["gh", "pr", "view", info.branch, "--json", "url", "--jq", ".url"],
|
||||
[
|
||||
gh_executable,
|
||||
"pr",
|
||||
"view",
|
||||
info.branch,
|
||||
"--json",
|
||||
"url",
|
||||
"--jq",
|
||||
".url",
|
||||
],
|
||||
cwd=info.path,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
@@ -1141,6 +1165,8 @@ class WorktreeManager:
|
||||
# Silently ignore errors when fetching existing PR URL - this is a best-effort
|
||||
# lookup that may fail due to network issues, missing gh CLI, or auth problems.
|
||||
# Returning None allows the caller to handle missing URLs gracefully.
|
||||
if isinstance(e, FileNotFoundError):
|
||||
invalidate_gh_cache()
|
||||
debug_warning("worktree", f"Could not get existing PR URL: {e}")
|
||||
|
||||
return None
|
||||
|
||||
@@ -3002,6 +3002,9 @@ export function registerWorktreeHandlers(
|
||||
// Get Python environment for bundled packages
|
||||
const pythonEnv = pythonEnvManagerSingleton.getPythonEnv();
|
||||
|
||||
// Get gh CLI path to pass to Python backend
|
||||
const ghCliPath = getToolPath('gh');
|
||||
|
||||
// Parse Python command to handle space-separated commands like "py -3"
|
||||
const [pythonCommand, pythonBaseArgs] = parsePythonCommand(pythonPath);
|
||||
const createPRProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], {
|
||||
@@ -3010,6 +3013,7 @@ export function registerWorktreeHandlers(
|
||||
...process.env,
|
||||
...pythonEnv,
|
||||
...profileEnv,
|
||||
GITHUB_CLI_PATH: ghCliPath,
|
||||
PYTHONUNBUFFERED: '1',
|
||||
PYTHONUTF8: '1'
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user