fix: windows #1056
fix: windows #1056
Conversation
github-actions
bot
added
area/fullstack
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds comprehensive frontend (TS) and backend (Py) platform abstraction layers, replaces ad-hoc platform checks with shared helpers, updates CLI/tool detection and terminal spawning to use the abstraction, adds cross-platform docs and tests, and expands CI to run matrix builds on Windows, macOS, and Linux. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend
participant BE as Backend
participant PM as Platform Module (TS/Py)
participant FS as Filesystem/OS
participant VAL as CLI Validator
FE->>PM: request candidate tool paths / detection
BE->>PM: request claude/python/git candidate paths
PM->>FS: probe candidate paths (exists/expand/env)
FS-->>PM: existence & metadata
PM->>VAL: validate candidate path(s)
VAL-->>PM: validation result
PM-->>FE: selected executable path or null
PM-->>BE: selected executable path or null
FE->>PM: request invocation build (requiresShell/build command)
PM-->>FE: command, args, shell decision
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (4)tests/**⚙️ CodeRabbit configuration file
Files:
apps/frontend/src/**/*.{tsx,ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/frontend/**/*.{ts,tsx}⚙️ CodeRabbit configuration file
Files:
apps/backend/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-01-11T10:48:12.867ZApplied to files:
📚 Learning: 2026-01-12T13:47:32.549ZApplied to files:
📚 Learning: 2026-01-12T13:47:32.549ZApplied to files:
🧬 Code graph analysis (1)tests/test_platform.py (1)
🪛 ast-grep (0.40.5)apps/frontend/src/main/platform/paths.ts[warning] 234-234: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns. (regexp-from-variable) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (13)
✏️ Tip: You can disable this entire section by setting 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 |
Summary of ChangesHello @AlexMadera, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural improvement by implementing a comprehensive platform abstraction layer across both the Python backend and the TypeScript frontend. The primary goal is to centralize and standardize how the application interacts with the underlying operating system, moving away from scattered, direct OS checks. This change aims to drastically improve cross-platform compatibility, particularly for Windows, by providing a consistent and secure API for handling paths, detecting executables, and managing shell interactions. The accompanying documentation and updated pull request template reinforce this new approach, guiding developers towards writing more robust and platform-agnostic code. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
🔄 Checking
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
added
🔄 Checking
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and significant refactoring to centralize platform-specific logic for both the Python backend and TypeScript frontend. Creating dedicated platform modules is a great step towards improving maintainability and ensuring cross-platform consistency, which is crucial for a project supporting Windows, macOS, and Linux. The addition of comprehensive documentation in CLAUDE.md and the PR template, along with dedicated test suites for the new modules, is highly commendable.
I've found a couple of minor areas for improvement to fully complete this refactoring, but overall, this is a very strong pull request that addresses a critical architectural concern.
| def get_claude_detection_paths() -> List[str]: | ||
| """ | ||
| Get platform-specific paths for Claude CLI detection. | ||
| Returns: | ||
| List of possible Claude CLI executable paths. | ||
| """ | ||
| home_dir = Path.home() | ||
| paths = [] | ||
|
|
||
| if is_windows(): | ||
| paths.extend([ | ||
| str(home_dir / "AppData" / "Local" / "Programs" / "claude" / "claude.exe"), | ||
| str(home_dir / "AppData" / "Roaming" / "npm" / "claude.cmd"), | ||
| str(home_dir / ".local" / "bin" / "claude.exe"), | ||
| r"C:\Program Files\Claude\claude.exe", | ||
| r"C:\Program Files (x86)\Claude\claude.exe", | ||
| ]) | ||
| else: | ||
| paths.extend([ | ||
| str(home_dir / ".local" / "bin" / "claude"), | ||
| str(home_dir / "bin" / "claude"), | ||
| ]) | ||
|
|
||
| # Add Homebrew path on macOS | ||
| if is_macos(): | ||
| brew_path = get_homebrew_path() | ||
| if brew_path: | ||
| paths.append(os.path.join(brew_path, "claude")) | ||
|
|
||
| return paths |
There was a problem hiding this comment.
There seems to be a duplication of logic for Claude CLI path detection. This function get_claude_detection_paths returns a flat list of paths, but apps/backend/core/client.py still defines and uses its own _get_claude_detection_paths which returns a dictionary of paths. The function in client.py is the one being used, making this implementation unused.
To improve maintainability and create a single source of truth, consider moving the logic from _get_claude_detection_paths in client.py into this platform module and having client.py import it. This would fully centralize the platform-specific detection logic as intended by this refactoring.
| export async function findExecutable( | ||
| name: string, | ||
| additionalPaths: string[] = [] | ||
| ): Promise<string | null> { | ||
| const config = getPathConfig(); | ||
| const searchPaths: string[] = []; | ||
|
|
||
| // Add PATH environment | ||
| const pathEnv = getEnvVar('PATH') || ''; | ||
| searchPaths.push(...pathEnv.split(config.delimiter).filter(Boolean)); | ||
|
|
||
| // Add platform-specific directories | ||
| const bins = getBinaryDirectories(); | ||
| searchPaths.push(...bins.user, ...bins.system); | ||
|
|
||
| // Add custom paths | ||
| searchPaths.push(...additionalPaths); | ||
|
|
||
| // Search with all applicable extensions | ||
| const extensions = [...config.executableExtensions]; | ||
|
|
||
| for (const searchDir of searchPaths) { | ||
| for (const ext of extensions) { | ||
| const fullPath = path.join(searchDir, `${name}${ext}`); | ||
| if (existsSync(fullPath)) { | ||
| return fullPath; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
This findExecutable function is declared as async but doesn't use await and relies on the synchronous existsSync. This is misleading and could cause confusion.
Since this function doesn't perform any asynchronous operations, it would be clearer to make it a synchronous function. Alternatively, if it's intended to be async, it should use asynchronous file system APIs like fs.promises.access.
export function findExecutable(
name: string,
additionalPaths: string[] = []
): string | null {
const config = getPathConfig();
const searchPaths: string[] = [];
// Add PATH environment
const pathEnv = getEnvVar('PATH') || '';
searchPaths.push(...pathEnv.split(config.delimiter).filter(Boolean));
// Add platform-specific directories
const bins = getBinaryDirectories();
searchPaths.push(...bins.user, ...bins.system);
// Add custom paths
searchPaths.push(...additionalPaths);
// Search with all applicable extensions
const extensions = [...config.executableExtensions];
for (const searchDir of searchPaths) {
for (const ext of extensions) {
const fullPath = path.join(searchDir, `${name}${ext}`);
if (existsSync(fullPath)) {
return fullPath;
}
}
}
return null;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/core/client.py (1)
143-188: Consider consolidating with platform module'sget_claude_detection_paths.This local
_get_claude_detection_paths()function duplicates logic from the platform module. The docstring notes it should stay in sync with the frontend, but the platform module'sget_claude_detection_pathsalready provides this functionality.If both functions are intentionally kept separate, ensure the import is removed. Otherwise, consider using the platform module function directly for better maintainability.
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 169-172: The CI step named "Setup Node.js and Python" only invokes
actions/setup-python@v5 (python-version: '3.12'); either rename the step to
"Setup Python" to reflect reality, or actually add Node.js setup by invoking
actions/setup-node@v4 with a node-version (e.g., '20') alongside the existing
actions/setup-python@v5, keeping the step name consistent with what's
configured.
In `@apps/backend/core/client.py`:
- Around line 26-32: Remove the unused imported symbol
get_claude_detection_paths from the import list in client.py and reorder the
remaining imports alphabetically to satisfy isort (e.g., ensure is_macos,
is_windows, get_comspec_path, validate_cli_path are sorted). Do not change or
replace usages of the local _get_claude_detection_paths() function — that local
function returns a different dict structure and must remain as-is.
In `@apps/backend/core/platform/__init__.py`:
- Around line 47-50: The get_current_os function currently calls
OS(platform.system()) which will raise ValueError for unexpected platform
strings; update get_current_os to map common platform.system() return values
(e.g., "Windows", "Darwin", "Linux") to the corresponding OS enum entries and
handle any other value by returning a safe default (e.g., OS.UNKNOWN or
OS.OTHER) instead of constructing the enum directly; implement this by
retrieving platform.system() into a variable, using a dict or if/elif to match
known names, and falling back to the default to avoid raising an exception in
get_current_os.
- Around line 14-21: Replace deprecated typing imports and use PEP 585/PEP 604
built-in generic and union types across this module: remove imports of Dict,
List, Optional from typing and update all annotations such as Dict[str,
List[str]] → dict[str, list[str]], Optional[str] → str | None,
Optional[List[str]] → list[str] | None, and List[str] → list[str]; search for
and update type annotations in functions/classes (e.g., any signatures or
variables using Dict, List, Optional in this file) and adjust any import lines
accordingly so only needed stdlib modules remain.
In `@apps/frontend/src/main/cli-tool-manager.ts`:
- Line 30: The import list in cli-tool-manager.ts includes an unused symbol
getPathDelimiter; remove getPathDelimiter from the import statement (the line
importing isWindows, isMacOS, isUnix, getPathDelimiter, joinPaths,
getExecutableExtension) so only the used functions remain, then run the linter
to confirm no unused-import warnings.
In `@apps/frontend/src/main/platform/__tests__/platform.test.ts`:
- Around line 313-325: The tests for normalizePath and joinPaths are too weak
(they only use toContain); update them to assert exact expected outputs: for
normalizePath('some/path/./to/../file') expect the result toEqual the normalized
path (e.g., 'some/path/file' or use path.posix.normalize to compute expected),
and for joinPaths('home','user','project') expect the result toEqual the
platform-appropriate joined path (use Node's path.join or path.posix.join to
compute the expected value) so the assertions verify full correctness of
normalizePath and joinPaths.
- Line 8: The import list includes unused and inconsistent symbols: remove
unused afterAll and beforeEach from the import statement and either replace test
with it or vice versa to match usage; update the import line that currently
imports { describe, test, expect, beforeEach, afterAll, vi } so it only imports
the symbols actually used in the file (e.g., { describe, it, expect, afterEach,
vi } if the file uses it and afterEach), ensuring the names (test vs it and
beforeEach vs afterEach) are consistent with the test body.
In `@apps/frontend/src/main/platform/index.ts`:
- Around line 339-370: The async function findExecutable currently uses the
synchronous existsSync which is misleading; either convert the filesystem checks
to real async by replacing existsSync with fs.promises.access (await
access(fullPath, fs.constants.F_OK) inside a try/catch) and keep the async
signature, or remove the async keyword and/or add a clear comment above
findExecutable stating that synchronous fs ops are used intentionally; update
references to existsSync and ensure imports/usage match the chosen approach
(e.g., use fs.promises.access or keep existsSync).
- Around line 232-240: getUnixShellConfig currently ignores the preferredShell
parameter; update it so that if preferredShell is provided it is used as the
executable (map or validate ShellType to a path if necessary), otherwise fall
back to process.env.SHELL and finally to '/bin/zsh'; ensure the returned object
from getUnixShellConfig uses that resolved executable and preserves args/env.
Reference: function name getUnixShellConfig and parameter preferredShell.
- Around line 188-206: The Windows paths in the shellPaths map are built with
string 'C:' which yields malformed paths like "C:Program Files..."; update all
occurrences that start with 'C:' (in shellPaths for ShellType.PowerShell and
ShellType.Bash entries and any other 'C:' literals) to use a proper absolute
drive root, e.g. use 'C:\\' or better use path.join(process.env.SystemDrive ||
'C:\\', ...) so path.join produces "C:\\Program Files\\..."—apply this change
for every path.join call that currently begins with 'C:'.
In `@apps/frontend/src/main/platform/paths.ts`:
- Around line 227-230: The code unconditionally appends the npm bin folder by
computing roamingAppData and pushing path.join(roamingAppData, 'npm') into
paths, which can confuse callers when toolName is not npm-related; update the
logic in the function that builds paths (look for the paths array,
roamingAppData variable and where path.join(roamingAppData, 'npm') is pushed) to
only add the npm path when appropriate—either guard the push behind a condition
that checks toolName (e.g., toolName === 'npm' or a new flag indicating
npm-installed CLIs) or add a clear comment/documentation explaining why npm must
always be included. Ensure the new behavior preserves existing callers by
running through any callers of this function and adjust tests or docs
accordingly.
- Around line 33-41: The macOS/Homebrew install locations are missing from the
Claude CLI detection in getClaudeDetectionPaths: inside the branch that
currently pushes joinPaths(homeDir, '.local', 'bin', 'claude') and
joinPaths(homeDir, 'bin', 'claude'), add Homebrew locations used by the backend
by pushing the absolute Homebrew paths (e.g., /opt/homebrew/bin/claude and
/usr/local/bin/claude) using joinPaths or string literals; ensure this is only
added for darwin (process.platform === 'darwin') so the paths array (variable
paths) includes Homebrew locations consistent with the backend.
- Around line 174-188: The current loop builds a RegExp from the keys in envVars
(e.g., '%PROGRAMFILES(X86)%') which contains unescaped special regex characters
like parentheses; update the replacement to avoid regex interpretation by either
escaping regex meta-characters before constructing new RegExp or, simpler,
perform a plain-string replacement (for example replace via
split(pattern).join(value || '') or use String.prototype.replaceAll) when
iterating over envVars so pattern keys are matched literally against
pathPattern/expanded; keep references to the same variables (envVars,
pathPattern, expanded) and ensure case-insensitive behavior is preserved if
needed by normalizing case on both sides rather than using a regex with flags.
In `@apps/frontend/src/main/platform/types.ts`:
- Around line 60-63: The BinaryDirectories interface uses mutable string arrays;
update its property types so both user and system are readonly arrays (change
user: string[] and system: string[] to user: readonly string[] and system:
readonly string[]) to match the other readonly array properties (e.g.,
alternativePaths, args, executableExtensions) and ensure immutability across the
platform types.
In `@tests/test_platform.py`:
- Around line 223-225: The test calls validate_cli_path(None) but the function
is typed as cli_path: str; either broaden the parameter type to accept None
(change signature to cli_path: Optional[str] or str | None and update any
callers/annotations) so the implementation's if not cli_path check is
type-correct, or remove the validate_cli_path(None) assertion from tests if None
should never be passed; update the validate_cli_path declaration and imports
(Optional) if choosing the first option and run type checks to ensure
consistency.
- Around line 336-348: Tests inconsistently patch platform.system across
modules; update the decorators in the Windows test to patch the same target used
in the macOS test (patch 'core.platform.platform.system' instead of
'platform.system') so both tests patch the symbol where get_platform_description
resolves it; ensure the patch decorator order and parameter names for
test_windows_description match the macOS test (mock_machine, mock_system) and
that get_platform_description remains called the same way.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci.ymlCLAUDE.mdapps/backend/core/client.pyapps/backend/core/platform/__init__.pyapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/platform/index.tsapps/frontend/src/main/platform/paths.tsapps/frontend/src/main/platform/types.tsapps/frontend/src/main/terminal/pty-manager.tstests/test_platform.py
🧰 Additional context used
📓 Path-based instructions (5)
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_platform.py
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/platform/types.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/platform/index.tsapps/frontend/src/main/platform/paths.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/platform/types.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/platform/index.tsapps/frontend/src/main/platform/paths.tsapps/frontend/src/main/env-utils.tsapps/frontend/src/main/terminal/pty-manager.ts
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/client.pyapps/backend/core/platform/__init__.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/client.pyapps/backend/core/platform/__init__.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/client.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Use Python 3.12+ for backend development. Use uv package manager for Python dependency management (uv venv, uv pip install).
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2026-01-11T10:48:12.867Z
Learnt from: g1331
Repo: AndyMik90/Auto-Claude PR: 912
File: tests/test_issue_884_plan_schema.py:287-296
Timestamp: 2026-01-11T10:48:12.867Z
Learning: In test files under tests/, prefer keeping small helper classes (e.g., DummyClient) local to individual test functions when duplication is minimal, tests are integration-style and benefit from self-containment, and tests/conftest.py is already large. Extract helpers to conftest only when duplication across tests is significant or when shared setup is truly reusable across many tests.
Applied to files:
tests/test_platform.py
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Applied to files:
apps/backend/core/client.py
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/core/*.py : Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Applied to files:
apps/backend/core/client.py
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/agents/{qa_reviewer,qa_fixer}.py : For QA agents (qa_reviewer and qa_fixer), use Electron MCP tools to perform automated E2E testing when ELECTRON_MCP_ENABLED=true is set. Available commands: click_by_text, click_by_selector, fill_input, select_option, send_keyboard_shortcut, navigate_to_hash, get_page_structure, debug_elements, verify_form_state, eval, take_screenshot.
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/frontend/src/**/*.{tsx,ts} : Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/frontend/package.json : Configure Electron app with --remote-debugging-port=9222 for Chrome DevTools Protocol access during development (npm run dev). This enables E2E testing via Electron MCP.
Applied to files:
CLAUDE.md
🧬 Code graph analysis (8)
tests/test_platform.py (2)
apps/frontend/scripts/download-python.cjs (1)
os(22-22)apps/backend/core/platform/__init__.py (20)
get_current_os(47-49)is_windows(52-54)is_macos(57-59)is_linux(62-64)is_unix(67-69)get_path_delimiter(76-78)get_executable_extension(81-83)with_executable_extension(86-96)get_binary_directories(103-150)get_homebrew_path(153-172)find_executable(179-230)get_claude_detection_paths(233-263)get_python_commands(266-275)validate_cli_path(278-323)requires_shell(330-346)get_comspec_path(349-361)build_windows_command(364-384)get_env_var(391-409)get_platform_description(416-430)OS(26-30)
apps/frontend/src/main/platform/types.ts (1)
apps/backend/core/platform/__init__.py (2)
OS(26-30)ShellType(33-40)
apps/frontend/src/main/platform/__tests__/platform.test.ts (1)
apps/frontend/src/main/platform/index.ts (19)
getCurrentOS(25-27)isWindows(32-34)isMacOS(39-41)isLinux(46-48)isUnix(53-55)getPathConfig(60-74)getPathDelimiter(79-81)getExecutableExtension(86-88)withExecutableExtension(93-99)getBinaryDirectories(104-148)getHomebrewPath(153-168)getShellConfig(173-179)requiresShell(247-252)getNpmCommand(257-259)getNpxCommand(264-266)isSecurePath(274-298)normalizePath(303-305)joinPaths(310-312)getPlatformDescription(375-384)
apps/frontend/src/main/cli-tool-manager.ts (1)
apps/frontend/src/main/platform/index.ts (6)
isWindows(32-34)joinPaths(310-312)getExecutableExtension(86-88)isMacOS(39-41)isSecurePath(274-298)isUnix(53-55)
apps/frontend/src/main/platform/paths.ts (1)
apps/frontend/src/main/platform/index.ts (5)
isWindows(32-34)joinPaths(310-312)getExecutableExtension(86-88)isMacOS(39-41)getHomebrewPath(153-168)
apps/frontend/src/main/env-utils.ts (1)
apps/frontend/src/main/platform/index.ts (3)
isWindows(32-34)getPathDelimiter(79-81)isUnix(53-55)
apps/frontend/src/main/terminal/pty-manager.ts (2)
apps/frontend/src/main/platform/index.ts (2)
getWindowsShellPaths(20-20)isWindows(32-34)apps/frontend/src/main/platform/paths.ts (1)
getWindowsShellPaths(128-161)
apps/backend/core/client.py (1)
apps/backend/core/platform/__init__.py (5)
is_windows(52-54)is_macos(57-59)get_claude_detection_paths(233-263)get_comspec_path(349-361)validate_cli_path(278-323)
🪛 ast-grep (0.40.5)
apps/frontend/src/main/platform/paths.ts
[warning] 186-186: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Lint
apps/backend/core/client.py
[error] 15-15: ruff check failed: Import block is un-sorted or un-formatted (I001).
🪛 GitHub Check: python
apps/backend/core/client.py
[failure] 29-29: Ruff (F401)
apps/backend/core/client.py:29:5: F401 core.platform.get_claude_detection_paths imported but unused
apps/backend/core/platform/__init__.py
[failure] 179-179: Ruff (UP006)
apps/backend/core/platform/init.py:179:59: UP006 Use list instead of List for type annotation
[failure] 179-179: Ruff (UP045)
apps/backend/core/platform/init.py:179:50: UP045 Use X | None for type annotations
[failure] 153-153: Ruff (UP045)
apps/backend/core/platform/init.py:153:28: UP045 Use X | None for type annotations
[failure] 103-103: Ruff (UP006)
apps/backend/core/platform/init.py:103:43: UP006 Use list instead of List for type annotation
[failure] 103-103: Ruff (UP006)
apps/backend/core/platform/init.py:103:33: UP006 Use dict instead of Dict for type annotation
[failure] 20-20: Ruff (UP035)
apps/backend/core/platform/init.py:20:1: UP035 typing.Dict is deprecated, use dict instead
[failure] 20-20: Ruff (UP035)
apps/backend/core/platform/init.py:20:1: UP035 typing.List is deprecated, use list instead
[failure] 14-20: Ruff (I001)
apps/backend/core/platform/init.py:14:1: I001 Import block is un-sorted or un-formatted
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
418-418: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
441-441: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
452-452: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
465-465: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
484-484: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
496-496: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
505-505: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
514-514: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Seer Code Review
- GitHub Check: Python 3.12 on windows-latest
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Python 3.13 on windows-latest
🔇 Additional comments (30)
.github/PULL_REQUEST_TEMPLATE.md (1)
43-54: LGTM! Well-structured platform testing checklist.This addition clearly communicates cross-platform expectations to contributors and references the centralized platform abstraction modules introduced in this PR. The note about CI covering all platforms when developers only have access to one OS is helpful.
CLAUDE.md (1)
404-536: Excellent cross-platform development documentation.This section thoroughly covers the platform abstraction strategy with clear examples of correct vs. incorrect patterns. The API table and code examples will help developers use the centralized platform modules effectively.
.github/workflows/ci.yml (3)
26-96: Well-structured multi-platform Python test job.The OS-aware virtual environment activation logic correctly handles Windows (
Scripts/activate) vs Unix (bin/activate). Thefail-fast: falsesetting ensures all platform results are collected even if one fails early.
150-198: Good sequencing of platform integration tests.The
needs: [test-python, test-frontend]dependency ensures basic tests pass before running integration tests, avoiding wasted CI resources on integration tests when unit tests fail.
116-116: No action required.Node.js 24 is a valid and fully supported release as of January 2026. It was released on May 6, 2025, entered Active LTS on October 28, 2025, and receives active security updates. The
actions/setup-node@v4action fully supports specifying major versions like'24'.apps/frontend/src/main/platform/types.ts (1)
1-72: Well-designed platform abstraction types.The type definitions provide a clean contract for platform-specific operations. Using
readonlyproperties throughout enforces immutability. The OS and ShellType enums align with the backend Python counterparts while using platform-appropriate values (process.platformfor TypeScript vsplatform.system()for Python).apps/frontend/src/main/platform/__tests__/platform.test.ts (2)
264-311: Excellent security test coverage forisSecurePath.The tests cover critical injection vectors: directory traversal (
..), shell metacharacters (;,|, backticks,$()), Windows environment variable expansion (%VAR%), and newline injection. This is essential for preventing command injection vulnerabilities.
36-50: Good platform mocking approach with proper cleanup.The
mockPlatformhelper andafterEachcleanup ensure tests don't leak state. Note that this mocksprocess.platformat runtime, but modules that cachepath.sepat load time won't reflect the mock. For the current tests this works since the platform module likely readsprocess.platformdynamically.apps/frontend/src/main/terminal/pty-manager.ts (1)
10-10: LGTM - Clean platform abstraction refactor.The migration from inline
process.platform === 'win32'checks to the centralizedisWindows()helper andgetWindowsShellPaths()for shell path configuration is well-executed. This improves maintainability by consolidating platform-specific logic in the platform module.Also applies to: 28-30, 57-61
apps/frontend/src/main/cli-tool-manager.ts (1)
166-188: Well-structured cross-platform path configuration.The
getClaudeDetectionPathsfunction effectively usesjoinPathsandgetExecutableExtensionfor platform-aware path construction. The documentation clearly notes the sync requirement with the backend's_get_claude_detection_paths()function, which is good practice for maintaining consistency.tests/test_platform.py (1)
1-38: Comprehensive test coverage for platform abstraction.This test suite provides excellent coverage for the new platform module, including security-critical path validation tests. The use of mocking to simulate different OS behaviors enables testing cross-platform logic on any CI runner.
apps/backend/core/client.py (1)
209-213: Good security improvement: Delegating to centralizedvalidate_cli_path.Replacing the local
_is_secure_pathwith the platform module'svalidate_cli_pathcentralizes security validation, reducing the risk of inconsistent security checks across the codebase.apps/frontend/src/main/env-utils.ts (2)
19-19: LGTM - Clean async utilities and platform abstraction.The introduction of
existsAsyncprovides a proper non-blocking alternative tofs.existsSync, and the migration to platform helpers (isWindows,isUnix,getPathDelimiter) improves consistency across the codebase.Also applies to: 31-38
470-482: Correct platform-aware shell detection.The
shouldUseShellfunction properly usesisUnix()to short-circuit on non-Windows systems. The.cmd/.batextension check for Windows is correctly implemented with case-insensitive regex matching.apps/frontend/src/main/platform/paths.ts (5)
1-12: LGTM!Clean module structure with appropriate imports. The use of centralized platform helpers from
./indexaligns with the abstraction strategy.
50-55: LGTM!Correct platform-specific Python command variations. Using
py -3as the first Windows option is the recommended approach for the Python launcher.
83-100: LGTM!Good implementation with explicit path checking on Windows and PATH-based resolution on Unix.
105-120: LGTM!Simple and correct platform-specific executable name resolution.
64-69: This function is unused in the codebase and contains glob pattern issues in path strings.
getPythonPaths()is not imported or called anywhere. Additionally, the glob patterns (Python3*) in lines 67-68 are literal strings that won't expand automatically—path.join()just concatenates them as-is, resulting in paths likeC:\Program Files\Python3*that won't match actual directories. If this function is planned for future use, either document that glob expansion must be handled by the consumer or remove the wildcards.apps/backend/core/platform/__init__.py (6)
26-41: LGTM!Clean enum definitions with appropriate values matching
platform.system()return values.
76-97: LGTM!Path configuration functions are correctly implemented with proper extension handling.
103-151: LGTM!Comprehensive binary directory configuration for all supported platforms.
179-230: LGTM!Well-structured executable search with proper PATH lookup followed by platform-specific directory search.
278-323: LGTM!Thorough security validation with appropriate checks for shell metacharacters, directory traversal, and command injection patterns. The Windows executable name validation adds an extra layer of protection.
391-409: LGTM!Correct handling of Windows case-insensitive environment variables.
apps/frontend/src/main/platform/index.ts (5)
1-21: LGTM!Clean module structure with proper imports and re-exports for backward compatibility.
25-55: LGTM!Platform detection functions are correctly implemented for the Electron/Node.js context.
60-99: LGTM!Path configuration functions correctly handle platform differences including executable extensions.
274-298: LGTM!Robust security validation with appropriate pattern matching for dangerous characters and proper Windows executable name validation.
375-383: LGTM!Clean platform description function for error messages and logging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| from core.platform import ( | ||
| is_windows, | ||
| is_macos, | ||
| get_claude_detection_paths, | ||
| get_comspec_path, | ||
| validate_cli_path, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Compare local _get_claude_detection_paths with platform module version
echo "=== Local function in client.py ==="
rg -n "def _get_claude_detection_paths" apps/backend/core/client.py -A 45 | head -80
echo ""
echo "=== Platform module function ==="
rg -n "def get_claude_detection_paths" apps/backend/core/platform.py -A 45 | head -80Repository: AndyMik90/Auto-Claude
Length of output: 2032
🏁 Script executed:
# Check actual usage of get_claude_detection_paths in client.py
echo "=== Usages of get_claude_detection_paths in client.py ==="
rg "get_claude_detection_paths" apps/backend/core/client.py
echo ""
echo "=== Usages of _get_claude_detection_paths in client.py ==="
rg "_get_claude_detection_paths" apps/backend/core/client.pyRepository: AndyMik90/Auto-Claude
Length of output: 544
🏁 Script executed:
# Check import order (isort-style)
echo "=== Import block at lines 26-32 ==="
sed -n '26,32p' apps/backend/core/client.py
echo ""
echo "=== First 50 lines of client.py ==="
head -50 apps/backend/core/client.pyRepository: AndyMik90/Auto-Claude
Length of output: 1789
🏁 Script executed:
# Find platform module
echo "=== Looking for platform module/package ==="
find apps/backend/core -name "platform*" -type f
find apps/backend/core -name "platform*" -type d
echo ""
echo "=== Check if platform is a package ==="
ls -la apps/backend/core/platform* 2>/dev/null || echo "Not found as file"Repository: AndyMik90/Auto-Claude
Length of output: 351
🏁 Script executed:
# Check core.platform imports/exports
echo "=== Contents of platform directory/module ==="
if [ -d "apps/backend/core/platform" ]; then
ls -la apps/backend/core/platform/
fi
if [ -f "apps/backend/core/platform.py" ]; then
head -100 apps/backend/core/platform.py
fiRepository: AndyMik90/Auto-Claude
Length of output: 286
🏁 Script executed:
# Search for get_claude_detection_paths definition anywhere in backend
echo "=== Search for get_claude_detection_paths definition ==="
rg "def get_claude_detection_paths" apps/backend/
echo ""
echo "=== Search in platform __init__ if it exists ==="
if [ -f "apps/backend/core/platform/__init__.py" ]; then
rg "get_claude_detection_paths" apps/backend/core/platform/__init__.py
fiRepository: AndyMik90/Auto-Claude
Length of output: 304
🏁 Script executed:
# Get the actual implementation of get_claude_detection_paths from platform module
echo "=== Platform module get_claude_detection_paths ==="
rg "def get_claude_detection_paths" apps/backend/core/platform/__init__.py -A 30Repository: AndyMik90/Auto-Claude
Length of output: 1073
🏁 Script executed:
# Search for where get_claude_detection_paths (imported) is actually used
echo "=== All usages of 'get_claude_detection_paths' (without underscore) ==="
rg "get_claude_detection_paths\(" apps/backend/core/client.pyRepository: AndyMik90/Auto-Claude
Length of output: 237
🏁 Script executed:
# Check import sorting - list all imports in client.py
echo "=== Full import section ==="
head -45 apps/backend/core/client.py | tail -25Repository: AndyMik90/Auto-Claude
Length of output: 884
🏁 Script executed:
# Verify if imports are actually sorted correctly by checking isort rules
echo "=== Check import block sort order ==="
sed -n '25,33p' apps/backend/core/client.py
echo ""
echo "=== All imports in alphabetical order test ==="
cat <<'EOF' > /tmp/check_imports.py
imports = [
"is_windows",
"is_macos",
"get_claude_detection_paths",
"get_comspec_path",
"validate_cli_path",
]
sorted_imports = sorted(imports)
for i, (actual, expected) in enumerate(zip(imports, sorted_imports)):
status = "✓" if actual == expected else "✗"
print(f"{status} {i}: {actual} vs {expected}")
EOF
python /tmp/check_imports.pyRepository: AndyMik90/Auto-Claude
Length of output: 493
Remove unused import and fix import ordering to resolve pipeline failures.
The get_claude_detection_paths import is genuinely unused (F401)—only the local _get_claude_detection_paths() function is called. However, these functions serve different purposes and are not interchangeable:
- The imported
get_claude_detection_paths()returns a flatList[str] - The local
_get_claude_detection_paths()returns adict[str, list[str]]with categorized paths (homebrew, platform, nvm_versions_dir) and explicitly mirrors the frontend implementation inapps/frontend/src/main/cli-tool-manager.ts, which must be kept in sync
Additionally, the import block violates isort ordering rules (I001). Imports should be alphabetically sorted:
🔧 Fix: Remove unused import and sort
from core.platform import (
+ get_claude_detection_paths,
+ get_comspec_path,
is_macos,
is_windows,
- get_claude_detection_paths,
- get_comspec_path,
validate_cli_path,
)Remove the unused get_claude_detection_paths import and reorder the remaining imports alphabetically.
🧰 Tools
🪛 GitHub Check: python
[failure] 29-29: Ruff (F401)
apps/backend/core/client.py:29:5: F401 core.platform.get_claude_detection_paths imported but unused
[failure] 15-32: Ruff (I001)
apps/backend/core/client.py:15:1: I001 Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
In `@apps/backend/core/client.py` around lines 26 - 32, Remove the unused imported
symbol get_claude_detection_paths from the import list in client.py and reorder
the remaining imports alphabetically to satisfy isort (e.g., ensure is_macos,
is_windows, get_comspec_path, validate_cli_path are sorted). Do not change or
replace usages of the local _get_claude_detection_paths() function — that local
function returns a different dict structure and must remain as-is.
| // Roaming AppData (for npm) | ||
| const roamingAppData = process.env.APPDATA || path.join(homeDir, 'AppData', 'Roaming'); | ||
| paths.push(path.join(roamingAppData, 'npm')); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unconditional npm path addition may cause confusion.
The npm path is added regardless of the toolName parameter. If this is intentional (e.g., for npm-installed CLIs), consider documenting this behavior or making it conditional.
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/platform/paths.ts` around lines 227 - 230, The code
unconditionally appends the npm bin folder by computing roamingAppData and
pushing path.join(roamingAppData, 'npm') into paths, which can confuse callers
when toolName is not npm-related; update the logic in the function that builds
paths (look for the paths array, roamingAppData variable and where
path.join(roamingAppData, 'npm') is pushed) to only add the npm path when
appropriate—either guard the push behind a condition that checks toolName (e.g.,
toolName === 'npm' or a new flag indicating npm-installed CLIs) or add a clear
comment/documentation explaining why npm must always be included. Ensure the new
behavior preserves existing callers by running through any callers of this
function and adjust tests or docs accordingly.
| export interface BinaryDirectories { | ||
| readonly user: string[]; | ||
| readonly system: string[]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using readonly string[] for BinaryDirectories properties.
For consistency with other interfaces that use readonly arrays (e.g., alternativePaths, args, executableExtensions), consider making these arrays readonly as well.
Proposed change
export interface BinaryDirectories {
- readonly user: string[];
- readonly system: string[];
+ readonly user: readonly string[];
+ readonly system: readonly string[];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface BinaryDirectories { | |
| readonly user: string[]; | |
| readonly system: string[]; | |
| } | |
| export interface BinaryDirectories { | |
| readonly user: readonly string[]; | |
| readonly system: readonly string[]; | |
| } |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/platform/types.ts` around lines 60 - 63, The
BinaryDirectories interface uses mutable string arrays; update its property
types so both user and system are readonly arrays (change user: string[] and
system: string[] to user: readonly string[] and system: readonly string[]) to
match the other readonly array properties (e.g., alternativePaths, args,
executableExtensions) and ensure immutability across the platform types.
| def test_rejects_empty_path(self): | ||
| assert validate_cli_path('') is False | ||
| assert validate_cli_path(None) is False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the actual function signature and implementation
rg -n "def validate_cli_path" apps/backend/core/platform/__init__.py -A 15Repository: AndyMik90/Auto-Claude
Length of output: 551
🏁 Script executed:
#!/bin/bash
# Also check imports at the top to understand typing
head -30 apps/backend/core/platform/__init__.pyRepository: AndyMik90/Auto-Claude
Length of output: 889
🏁 Script executed:
#!/bin/bash
# Check the full test function to see context
sed -n '223,225p' tests/test_platform.py -A 2Repository: AndyMik90/Auto-Claude
Length of output: 1887
Update type hint to accept None or remove this test case.
The function signature has cli_path: str but the implementation checks if not cli_path:, which handles None. The test at line 225 calls validate_cli_path(None) expecting False, creating a type safety mismatch. Either:
- Update the type hint to
cli_path: str | Noneorcli_path: Optional[str]if callers may passNone - Remove the
validate_cli_path(None)test ifNoneis never a valid input from callers
🤖 Prompt for AI Agents
In `@tests/test_platform.py` around lines 223 - 225, The test calls
validate_cli_path(None) but the function is typed as cli_path: str; either
broaden the parameter type to accept None (change signature to cli_path:
Optional[str] or str | None and update any callers/annotations) so the
implementation's if not cli_path check is type-correct, or remove the
validate_cli_path(None) assertion from tests if None should never be passed;
update the validate_cli_path declaration and imports (Optional) if choosing the
first option and run type checks to ensure consistency.
| @patch('platform.system', return_value='Windows') | ||
| @patch('platform.machine', return_value='AMD64') | ||
| def test_windows_description(self, mock_machine, mock_system): | ||
| desc = get_platform_description() | ||
| assert 'Windows' in desc | ||
| assert 'AMD64' in desc | ||
|
|
||
| @patch('core.platform.platform.system', return_value='Darwin') | ||
| @patch('platform.machine', return_value='arm64') | ||
| def test_macos_description(self, mock_machine, mock_system): | ||
| desc = get_platform_description() | ||
| assert 'macOS' in desc | ||
| assert 'arm64' in desc |
There was a problem hiding this comment.
Inconsistent mock target for platform.system.
Lines 336-337 mock platform.system directly (at the top-level platform module), while line 343 mocks core.platform.platform.system (within the core.platform module namespace). This inconsistency could cause unexpected test behavior if get_platform_description caches or calls platform.system directly.
For consistent behavior, use the same mock target pattern throughout.
🔧 Suggested fix for consistent mocking
class TestPlatformDescription:
"""Tests for platform description."""
- `@patch`('platform.system', return_value='Windows')
+ `@patch`('core.platform.platform.system', return_value='Windows')
`@patch`('platform.machine', return_value='AMD64')
def test_windows_description(self, mock_machine, mock_system):
desc = get_platform_description()
assert 'Windows' in desc
assert 'AMD64' in desc
`@patch`('core.platform.platform.system', return_value='Darwin')
- `@patch`('platform.machine', return_value='arm64')
+ `@patch`('core.platform.platform.machine', return_value='arm64')
def test_macos_description(self, mock_machine, mock_system):
desc = get_platform_description()
assert 'macOS' in desc
assert 'arm64' in desc🤖 Prompt for AI Agents
In `@tests/test_platform.py` around lines 336 - 348, Tests inconsistently patch
platform.system across modules; update the decorators in the Windows test to
patch the same target used in the macOS test (patch
'core.platform.platform.system' instead of 'platform.system') so both tests
patch the symbol where get_platform_description resolves it; ensure the patch
decorator order and parameter names for test_windows_description match the macOS
test (mock_machine, mock_system) and that get_platform_description remains
called the same way.
- Remove unused imports in TypeScript files: - platform.test.ts: remove unused beforeEach, afterAll, test, os, fs, ShellType; add missing afterEach, it - cli-tool-manager.ts: remove unused getPathDelimiter - paths.ts: remove unused homeDir variable - Remove unused imports in Python files: - client.py: remove unused get_claude_detection_paths import - test_platform.py: remove unused pytest, MagicMock, find_executable, get_comspec_path - Fix findExecutable in platform/index.ts: change from async to sync (function uses synchronous existsSync, should not be async) Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
🔄 Checking
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/platform/__tests__/platform.test.ts`:
- Around line 211-227: The failing test mocks process.platform via
mockPlatform('win32') but doesn't stub fs.existsSync, so getWindowsShellConfig
falls back to cmd.exe on CI; update the test that calls getShellConfig() to also
mock existsSync (e.g., stub existsSync to return true for the expected
PowerShell paths like 'pwsh.exe' or 'powershell.exe') so the PowerShell branch
is exercised, or alternatively relax the assertion to accept 'cmd.exe' as a
valid fallback; reference the test helpers mockPlatform and the implementation
functions getShellConfig / getWindowsShellConfig and the fs.existsSync symbol
when making the change.
In `@apps/frontend/src/main/platform/index.ts`:
- Around line 373-383: getPlatformDescription can yield "undefined (arch)" when
getCurrentOS() returns an unexpected value; update the osName resolution in
getPlatformDescription to provide a safe fallback (e.g., use a default like
"Unknown OS" or String(getCurrentOS())) if the lookup object doesn't contain the
key, so the returned template `${osName} (${arch})` never contains undefined.
- Around line 25-27: getCurrentOS currently does an unsafe cast of
process.platform to OS; change it to validate and map process.platform to a
known OS enum member (e.g., via a switch or Object.values(OS) lookup) and return
that mapped value, and for any unknown platform either return a safe default
(such as an OS.Unknown/OS.Other enum member if available) or throw a clear
error; update the logic inside getCurrentOS to perform the explicit
check/mapping rather than using a direct cast so callers never receive an
invalid OS value.
♻️ Duplicate comments (3)
apps/backend/core/client.py (1)
26-31: Fix import ordering to resolve pipeline failure.The imports within the
core.platformblock are not alphabetically sorted, causing the I001 (isort) pipeline failure. The unusedget_claude_detection_pathsimport has been correctly removed, but the remaining imports need reordering.🔧 Proposed fix
from core.platform import ( - is_windows, - is_macos, get_comspec_path, + is_macos, + is_windows, validate_cli_path, )apps/frontend/src/main/platform/paths.ts (1)
21-41: Missing Homebrew path for macOS in Claude detection.The function doesn't add Homebrew paths for macOS, unlike the backend
get_claude_detection_paths()which includes them. This inconsistency may cause Claude CLI detection to fail on macOS when installed via Homebrew.apps/frontend/src/main/platform/__tests__/platform.test.ts (1)
310-322: Weak test assertions for path utilities.These tests only verify the result contains a substring, which doesn't fully validate the functions' behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
apps/backend/core/client.pyapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/platform/index.tsapps/frontend/src/main/platform/paths.tstests/test_platform.py
🧰 Additional context used
📓 Path-based instructions (5)
apps/frontend/src/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/frontend/src/**/*.{tsx,ts}: Use i18n translation keys for all user-facing text in the frontend. All labels, buttons, messages must use translation keys from react-i18next with namespace:section.key format (e.g., 'navigation:items.githubPRs').
Never use hardcoded strings for UI text in JSX/TSX files. Always use translation keys via useTranslation() hook.
Files:
apps/frontend/src/main/platform/paths.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/platform/index.ts
apps/frontend/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
apps/frontend/**/*.{ts,tsx}: Review React patterns and TypeScript type safety.
Check for proper state management and component composition.
Files:
apps/frontend/src/main/platform/paths.tsapps/frontend/src/main/platform/__tests__/platform.test.tsapps/frontend/src/main/cli-tool-manager.tsapps/frontend/src/main/platform/index.ts
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Use Graphiti memory system (mandatory) for cross-session context and knowledge graph management. Access via get_graphiti_memory() from integrations.graphiti.memory module.
Files:
apps/backend/core/client.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/client.py
apps/backend/core/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Files:
apps/backend/core/client.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: Ensure tests are comprehensive and follow pytest conventions.
Check for proper mocking and test isolation.
Files:
tests/test_platform.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: MikeeBuilds
Repo: AndyMik90/Auto-Claude PR: 661
File: apps/frontend/src/renderer/components/onboarding/OllamaModelSelector.tsx:176-189
Timestamp: 2026-01-04T23:59:48.743Z
Learning: In the AndyMik90/Auto-Claude repository, pre-existing i18n issues (hardcoded user-facing strings that should be localized) can be deferred to future i18n cleanup passes rather than requiring immediate fixes in PRs that don't introduce new i18n violations.
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: When submitting PRs to AndyMik90/Auto-Claude upstream, always target the develop branch, NOT main. Commits must include sign-off (git commit -s).
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/**/*.py : Always use the Claude Agent SDK (claude-agent-sdk package) for AI interactions, never use the Anthropic API directly. Use create_client() from core.client module for client setup.
Applied to files:
apps/backend/core/client.py
📚 Learning: 2026-01-12T13:47:32.539Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T13:47:32.539Z
Learning: Applies to apps/backend/core/*.py : Use project stack detection (context/project_analyzer.py) to dynamically determine allowed commands and available tools. Cache security profile in .auto-claude-security.json.
Applied to files:
apps/backend/core/client.py
📚 Learning: 2026-01-11T10:48:12.867Z
Learnt from: g1331
Repo: AndyMik90/Auto-Claude PR: 912
File: tests/test_issue_884_plan_schema.py:287-296
Timestamp: 2026-01-11T10:48:12.867Z
Learning: In test files under tests/, prefer keeping small helper classes (e.g., DummyClient) local to individual test functions when duplication is minimal, tests are integration-style and benefit from self-containment, and tests/conftest.py is already large. Extract helpers to conftest only when duplication across tests is significant or when shared setup is truly reusable across many tests.
Applied to files:
tests/test_platform.py
🧬 Code graph analysis (6)
apps/frontend/src/main/platform/paths.ts (1)
apps/frontend/src/main/platform/index.ts (5)
isWindows(32-34)joinPaths(310-312)getExecutableExtension(86-88)isMacOS(39-41)getHomebrewPath(153-168)
apps/frontend/src/main/platform/__tests__/platform.test.ts (1)
apps/frontend/src/main/platform/index.ts (19)
getCurrentOS(25-27)isWindows(32-34)isMacOS(39-41)isLinux(46-48)isUnix(53-55)getPathConfig(60-74)getPathDelimiter(79-81)getExecutableExtension(86-88)withExecutableExtension(93-99)getBinaryDirectories(104-148)getHomebrewPath(153-168)getShellConfig(173-179)requiresShell(247-252)getNpmCommand(257-259)getNpxCommand(264-266)isSecurePath(274-298)normalizePath(303-305)joinPaths(310-312)getPlatformDescription(375-384)
apps/backend/core/client.py (1)
apps/backend/core/platform/__init__.py (4)
is_windows(52-54)is_macos(57-59)get_comspec_path(349-361)validate_cli_path(278-323)
tests/test_platform.py (1)
apps/backend/core/platform/__init__.py (18)
get_current_os(47-49)is_windows(52-54)is_macos(57-59)is_linux(62-64)is_unix(67-69)get_path_delimiter(76-78)get_executable_extension(81-83)with_executable_extension(86-96)get_binary_directories(103-150)get_homebrew_path(153-172)get_claude_detection_paths(233-263)get_python_commands(266-275)validate_cli_path(278-323)requires_shell(330-346)build_windows_command(364-384)get_env_var(391-409)get_platform_description(416-430)OS(26-30)
apps/frontend/src/main/cli-tool-manager.ts (1)
apps/frontend/src/main/platform/index.ts (6)
isWindows(32-34)joinPaths(310-312)getExecutableExtension(86-88)isMacOS(39-41)isSecurePath(274-298)isUnix(53-55)
apps/frontend/src/main/platform/index.ts (3)
apps/frontend/src/main/platform/types.ts (3)
PathConfig(51-55)BinaryDirectories(60-63)ShellConfig(42-46)apps/frontend/scripts/download-python.cjs (3)
os(22-22)key(206-206)config(207-207)apps/frontend/src/main/env-utils.ts (1)
findExecutable(258-279)
🪛 ast-grep (0.40.5)
apps/frontend/src/main/platform/paths.ts
[warning] 185-185: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Lint
apps/backend/core/client.py
[error] 15-15: Ruff: Import block is unsorted or unformatted. (I001)
🪛 GitHub Check: Frontend on macos-latest
apps/frontend/src/main/platform/__tests__/platform.test.ts
[failure] 218-218: src/main/platform/tests/platform.test.ts > Platform Module > Shell Configuration > returns PowerShell config on Windows by default
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/main/platform/tests/platform.test.ts:218:28
🪛 GitHub Check: Frontend on ubuntu-latest
apps/frontend/src/main/platform/__tests__/platform.test.ts
[failure] 218-218: src/main/platform/tests/platform.test.ts > Platform Module > Shell Configuration > returns PowerShell config on Windows by default
AssertionError: expected false to be true // Object.is equality
- Expected
- Received
- true
- false
❯ src/main/platform/tests/platform.test.ts:218:28
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Seer Code Review
- GitHub Check: Python 3.13 on windows-latest
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Frontend on windows-latest
- GitHub Check: Python 3.12 on macos-latest
- GitHub Check: Python 3.12 on windows-latest
🔇 Additional comments (12)
apps/backend/core/client.py (7)
167-179: LGTM!Correct use of
is_windows()for platform-specific path detection, replacing directplatform.system()calls with the centralized abstraction.
209-212: LGTM!Security validation is now properly delegated to the centralized
validate_cli_path()function from the platform module, which handles shell metacharacters, directory traversal patterns, and Windows-specific executable validation.
225-237: LGTM!Proper Windows batch file handling using
get_comspec_path()from the platform module. The/d /s /cflags are correctly used for secure command execution (disabling AutoRun and handling quoted paths).
238-246: LGTM!Correct platform-conditional usage of
subprocess.CREATE_NO_WINDOWflag, which is only valid on Windows.
307-316: LGTM!Correct use of
is_macos()for Homebrew paths, as Homebrew is primarily a macOS package manager (Intel and Apple Silicon paths are both covered).
318-352: LGTM!Using
not is_windows()for NVM path detection correctly covers both macOS and Linux where NVM (Node Version Manager) is commonly used.
748-752: LGTM!Appropriate use of
is_windows()for the Git Bash path detection warning, which is only relevant on Windows systems.tests/test_platform.py (1)
1-35: LGTM! Comprehensive test coverage for the platform module.The test file provides good coverage of the platform abstraction layer, testing OS detection, path configuration, executable extensions, binary directories, and security validation across Windows/macOS/Linux scenarios.
Note: Several issues (unused imports, type hint for
validate_cli_path(None), inconsistent mock targets inTestPlatformDescription) have been identified in previous review comments and should be addressed.apps/frontend/src/main/cli-tool-manager.ts (3)
30-30: LGTM! Import cleanup.The unused
getPathDelimiterimport noted in previous reviews has been removed. The current imports match actual usage.
96-124: Good platform-aware path validation logic.The
isWrongPlatformPathfunction correctly identifies cross-platform path issues by detecting Unix absolute paths on Windows and Windows-style paths (drive letters, backslashes, Windows-specific directories) on Unix systems.
166-188: Well-structured Claude detection path configuration.The
getClaudeDetectionPathsfunction properly uses platform helpers (isWindows(),joinPaths(),getExecutableExtension()) for cross-platform path construction. The documentation noting sync requirements with the Python backend is helpful for maintainability.apps/frontend/src/main/platform/__tests__/platform.test.ts (1)
43-47: Good test cleanup pattern.The
afterEachproperly restores the original platform and mocks, ensuring test isolation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-actions
bot
added
🔄 Checking
Fixes:
1. Windows path construction - path.join('C:', ...) produces 'C:foo'
(relative to C: drive), not 'C:\foo'. Changed to 'C:\Program Files'
as the first segment to get proper absolute paths.
2. getCurrentOS() now handles unknown platforms (e.g., FreeBSD) by
defaulting to Linux for Unix-like systems.
3. getPlatformDescription() now has a fallback when OS mapping fails.
4. Shell config test now accepts cmd.exe fallback (when PowerShell
paths don't exist in test environments).
5. Python ruff fixes - sorted imports and modernized type annotations
(list instead of List, dict instead of Dict, X | None instead of
Optional[X]).
Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
🔄 Checking
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
✅ Auto Claude Review - APPROVED
Status: Ready to Merge
Summary: ## 🟡 Follow-up Review: Merge With Changes
🟡 Can merge - Minor suggestions noted, no blockers.
Resolution Status
- ✅ Resolved: 7 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 1 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
Verdict
CI Status: ✅ All 26 CI checks passing - not a blocker.
Previous Findings: All 7 findings from the last review have been verified as RESOLVED:
- HIGH: Duplicate Claude detection paths → Now delegates to platform module
- MEDIUM (5): All addressed - argument quoting fixed with subprocess.list2cmdline, empty string checks added, Homebrew paths added, CI step renamed, env var fallbacks added
New Findings: 5 LOW severity issues identified (code quality and minor consistency improvements). The initially-reported medium finding (NEW-002) was dismissed as false positive - it flagged pre-existing code in unchanged files which is out of scope. NEW-005 was confirmed valid but represents a defensive coding suggestion with low actual risk since validation exists and works correctly.
CodeRabbit Comments: All 4 comments were addressed in subsequent commits:
- Commit 12c2b3a: Condensed path construction, expanded Python glob patterns
- Commit 89b7133: Returned Python commands as argument sequences
Recommendation: PR is ready to merge. All blocking issues from the previous review have been resolved, CI passes on all platforms, and the remaining findings are low-severity suggestions that can be addressed in follow-up work. The author has been responsive to feedback and made quality improvements across the platform abstraction layer.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
💡 Suggestions (5)
These are non-blocking suggestions for consideration:
🔵 [NEW-005] [LOW] build_windows_command relies on caller to validate cli_path
📁 apps/backend/core/platform/__init__.py:463
The function builds a command string with cli_path quoted but does not validate it internally. While validate_cli_path() exists and correctly rejects dangerous characters including double quotes, build_windows_command() relies on callers to validate first. This is a defensive coding recommendation - consider either adding validation within the function or documenting the precondition clearly.
Suggested fix:
Add assertion: 'if not validate_cli_path(cli_path): raise ValueError("Invalid CLI path")' or document the precondition in the docstring
🔵 [NEW-001] [LOW] validate_cli_path accepts None but lacks type annotation
📁 apps/backend/core/platform/__init__.py:361
Function handles None correctly at runtime (returns False) but type signature is 'cli_path: str' not 'cli_path: str | None'
Suggested fix:
Change signature to validate_cli_path(cli_path: str | None) -> bool
🔵 [NEW-004] [LOW] Regex metacharacters not escaped in expandDirPattern
📁 apps/frontend/src/main/platform/paths.ts:80
The function converts glob patterns to regex by only replacing '' with '.'. Other regex metacharacters (., +, ?) are not escaped. Current usage only passes 'Python3*' so this is low risk.
Suggested fix:
Escape regex metacharacters before replacing *
🔵 [NEW-003] [LOW] Windows isSecurePath only strips .exe extension before validation
📁 apps/frontend/src/main/platform/index.ts:307
Uses path.basename(candidatePath, getExecutableExtension()) which only strips .exe. Python version strips any extension via os.path.splitext(). Low impact as the regex includes dots.
Suggested fix:
Use path.extname to strip any extension, matching Python behavior
🔵 [NEW-006] [LOW] TypeScript isSecurePath does not verify file existence for absolute paths
📁 apps/frontend/src/main/platform/index.ts:308
Python validate_cli_path checks os.path.isfile() for absolute paths but TypeScript version does not. Minor consistency issue.
Suggested fix:
Add existsSync check for absolute paths if needed
This automated review found no blocking issues. The PR can be safely merged.
Generated by Auto Claude
github-actions
bot
added
🔄 Checking
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| ], | ||
| windowsterminal: [ | ||
| path.join('C:\\Program Files', 'WindowsApps', 'Microsoft.WindowsTerminal_*', 'WindowsTerminal.exe') | ||
| ], |
There was a problem hiding this comment.
Windows Terminal paths use invalid glob pattern
Medium Severity
The windowsterminal path in getWindowsShellPaths() uses a glob pattern (Microsoft.WindowsTerminal_*) that existsSync() cannot match since it treats asterisks as literal characters. Additionally, WindowsTerminal.exe is a terminal emulator, not a shell—node-pty requires an actual shell like PowerShell. The old code correctly pointed windowsterminal to PowerShell executables. Users who select Windows Terminal as their preferred terminal will now incorrectly fall back to cmd.exe instead of PowerShell.
Additional Locations (1)
| path.join('C:\\cygwin64', 'bin', 'bash.exe') | ||
| ], | ||
| msys2: [ | ||
| path.join('C:\\msys64', 'usr', 'bin', 'bash.exe') |
There was a problem hiding this comment.
Missing 32-bit Cygwin and MSYS2 shell paths
Low Severity
The getWindowsShellPaths() function is missing 32-bit installation paths for Cygwin (C:\cygwin\bin\bash.exe) and MSYS2 (C:\msys32\usr\bin\bash.exe). The old WINDOWS_SHELL_PATHS constant included both 64-bit and 32-bit paths, but the new implementation only includes 64-bit. Users with 32-bit Cygwin or MSYS2 installations who select these as their preferred terminal will unexpectedly fall back to cmd.exe.
* fix windows * fix: address code review feedback - unused imports and async function - Remove unused imports in TypeScript files: - platform.test.ts: remove unused beforeEach, afterAll, test, os, fs, ShellType; add missing afterEach, it - cli-tool-manager.ts: remove unused getPathDelimiter - paths.ts: remove unused homeDir variable - Remove unused imports in Python files: - client.py: remove unused get_claude_detection_paths import - test_platform.py: remove unused pytest, MagicMock, find_executable, get_comspec_path - Fix findExecutable in platform/index.ts: change from async to sync (function uses synchronous existsSync, should not be async) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: correct Windows path construction and Python type annotations Fixes: 1. Windows path construction - path.join('C:', ...) produces 'C:foo' (relative to C: drive), not 'C:\foo'. Changed to 'C:\Program Files' as the first segment to get proper absolute paths. 2. getCurrentOS() now handles unknown platforms (e.g., FreeBSD) by defaulting to Linux for Unix-like systems. 3. getPlatformDescription() now has a fallback when OS mapping fails. 4. Shell config test now accepts cmd.exe fallback (when PowerShell paths don't exist in test environments). 5. Python ruff fixes - sorted imports and modernized type annotations (list instead of List, dict instead of Dict, X | None instead of Optional[X]). Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix comment (sentry) and tests * try and fix lint and tests * fix tests * fix remaining * we need to add more tests, i have a PR for this but it has been ignored * fix(tests): normalize path separators in fs.existsSync mock for Windows path.join() uses backslashes on Windows, so the mock now normalizes paths to forward slashes before comparison to ensure tests pass cross-platform. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(platform): address code review findings for cross-platform safety - Fix argument quoting vulnerability in build_windows_command using subprocess.list2cmdline() - Remove duplicate Claude detection paths from client.py by using platform module - Add empty string check in withExecutableExtension (TypeScript) - Add empty string check in isSecurePath (TypeScript) for cross-platform consistency - Add Homebrew paths for macOS in getClaudeExecutablePath (TypeScript) - Fix misleading CI step name (was "Setup Node.js and Python", now "Setup Python") - Fix expandWindowsEnvVars to provide sensible defaults for unset env vars Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): condense path construction and expand Python glob patterns - Condense multi-line path construction in get_claude_detection_paths_structured() to single-line expressions for cleaner ruff formatting - Fix getPythonPaths() to expand glob patterns (Python3*) using readdirSync instead of returning literal glob strings that existsSync can't handle - Add expandDirPattern helper function for safe directory pattern matching Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): return Python commands as argument sequences Change get_python_commands() / getPythonCommands() to return command arguments as sequences (list of lists) instead of space-separated strings. Before: ["py -3", "python"] - broken with subprocess/shutil.which After: [["py", "-3"], ["python"]] - works correctly This allows callers to: - Pass cmd directly to subprocess.run(cmd) - Use cmd[0] with shutil.which() for executable lookup Updated both Python and TypeScript implementations for consistency. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Andy <[email protected]>
* fix windows * fix: address code review feedback - unused imports and async function - Remove unused imports in TypeScript files: - platform.test.ts: remove unused beforeEach, afterAll, test, os, fs, ShellType; add missing afterEach, it - cli-tool-manager.ts: remove unused getPathDelimiter - paths.ts: remove unused homeDir variable - Remove unused imports in Python files: - client.py: remove unused get_claude_detection_paths import - test_platform.py: remove unused pytest, MagicMock, find_executable, get_comspec_path - Fix findExecutable in platform/index.ts: change from async to sync (function uses synchronous existsSync, should not be async) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: correct Windows path construction and Python type annotations Fixes: 1. Windows path construction - path.join('C:', ...) produces 'C:foo' (relative to C: drive), not 'C:\foo'. Changed to 'C:\Program Files' as the first segment to get proper absolute paths. 2. getCurrentOS() now handles unknown platforms (e.g., FreeBSD) by defaulting to Linux for Unix-like systems. 3. getPlatformDescription() now has a fallback when OS mapping fails. 4. Shell config test now accepts cmd.exe fallback (when PowerShell paths don't exist in test environments). 5. Python ruff fixes - sorted imports and modernized type annotations (list instead of List, dict instead of Dict, X | None instead of Optional[X]). Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix comment (sentry) and tests * try and fix lint and tests * fix tests * fix remaining * we need to add more tests, i have a PR for this but it has been ignored * fix(tests): normalize path separators in fs.existsSync mock for Windows path.join() uses backslashes on Windows, so the mock now normalizes paths to forward slashes before comparison to ensure tests pass cross-platform. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(platform): address code review findings for cross-platform safety - Fix argument quoting vulnerability in build_windows_command using subprocess.list2cmdline() - Remove duplicate Claude detection paths from client.py by using platform module - Add empty string check in withExecutableExtension (TypeScript) - Add empty string check in isSecurePath (TypeScript) for cross-platform consistency - Add Homebrew paths for macOS in getClaudeExecutablePath (TypeScript) - Fix misleading CI step name (was "Setup Node.js and Python", now "Setup Python") - Fix expandWindowsEnvVars to provide sensible defaults for unset env vars Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): condense path construction and expand Python glob patterns - Condense multi-line path construction in get_claude_detection_paths_structured() to single-line expressions for cleaner ruff formatting - Fix getPythonPaths() to expand glob patterns (Python3*) using readdirSync instead of returning literal glob strings that existsSync can't handle - Add expandDirPattern helper function for safe directory pattern matching Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): return Python commands as argument sequences Change get_python_commands() / getPythonCommands() to return command arguments as sequences (list of lists) instead of space-separated strings. Before: ["py -3", "python"] - broken with subprocess/shutil.which After: [["py", "-3"], ["python"]] - works correctly This allows callers to: - Pass cmd directly to subprocess.run(cmd) - Use cmd[0] with shutil.which() for executable lookup Updated both Python and TypeScript implementations for consistency. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Andy <[email protected]>
* fix windows * fix: address code review feedback - unused imports and async function - Remove unused imports in TypeScript files: - platform.test.ts: remove unused beforeEach, afterAll, test, os, fs, ShellType; add missing afterEach, it - cli-tool-manager.ts: remove unused getPathDelimiter - paths.ts: remove unused homeDir variable - Remove unused imports in Python files: - client.py: remove unused get_claude_detection_paths import - test_platform.py: remove unused pytest, MagicMock, find_executable, get_comspec_path - Fix findExecutable in platform/index.ts: change from async to sync (function uses synchronous existsSync, should not be async) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: correct Windows path construction and Python type annotations Fixes: 1. Windows path construction - path.join('C:', ...) produces 'C:foo' (relative to C: drive), not 'C:\foo'. Changed to 'C:\Program Files' as the first segment to get proper absolute paths. 2. getCurrentOS() now handles unknown platforms (e.g., FreeBSD) by defaulting to Linux for Unix-like systems. 3. getPlatformDescription() now has a fallback when OS mapping fails. 4. Shell config test now accepts cmd.exe fallback (when PowerShell paths don't exist in test environments). 5. Python ruff fixes - sorted imports and modernized type annotations (list instead of List, dict instead of Dict, X | None instead of Optional[X]). Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix comment (sentry) and tests * try and fix lint and tests * fix tests * fix remaining * we need to add more tests, i have a PR for this but it has been ignored * fix(tests): normalize path separators in fs.existsSync mock for Windows path.join() uses backslashes on Windows, so the mock now normalizes paths to forward slashes before comparison to ensure tests pass cross-platform. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(platform): address code review findings for cross-platform safety - Fix argument quoting vulnerability in build_windows_command using subprocess.list2cmdline() - Remove duplicate Claude detection paths from client.py by using platform module - Add empty string check in withExecutableExtension (TypeScript) - Add empty string check in isSecurePath (TypeScript) for cross-platform consistency - Add Homebrew paths for macOS in getClaudeExecutablePath (TypeScript) - Fix misleading CI step name (was "Setup Node.js and Python", now "Setup Python") - Fix expandWindowsEnvVars to provide sensible defaults for unset env vars Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): condense path construction and expand Python glob patterns - Condense multi-line path construction in get_claude_detection_paths_structured() to single-line expressions for cleaner ruff formatting - Fix getPythonPaths() to expand glob patterns (Python3*) using readdirSync instead of returning literal glob strings that existsSync can't handle - Add expandDirPattern helper function for safe directory pattern matching Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix(platform): return Python commands as argument sequences Change get_python_commands() / getPythonCommands() to return command arguments as sequences (list of lists) instead of space-separated strings. Before: ["py -3", "python"] - broken with subprocess/shutil.which After: [["py", "-3"], ["python"]] - works correctly This allows callers to: - Pass cmd directly to subprocess.run(cmd) - Use cmd[0] with shutil.which() for executable lookup Updated both Python and TypeScript implementations for consistency. Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Andy <[email protected]>
Summary by CodeRabbit
New Features
CI / Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.