diff --git a/apps/backend/core/client.py b/apps/backend/core/client.py index 807d129561..327060619a 100644 --- a/apps/backend/core/client.py +++ b/apps/backend/core/client.py @@ -16,6 +16,7 @@ import json import logging import os +import platform import shutil import subprocess import threading @@ -140,18 +141,83 @@ def invalidate_project_cache(project_dir: Path | None = None) -> None: _CLI_CACHE_LOCK = threading.Lock() -def _get_claude_detection_paths() -> dict[str, list[str] | str]: +def _get_claude_detection_paths() -> dict[str, list[str]]: """ Get all candidate paths for Claude CLI detection. - This is a thin wrapper around the platform module's implementation. - See core/platform/__init__.py:get_claude_detection_paths_structured() - for the canonical implementation. + Returns platform-specific paths where Claude CLI might be installed. + + IMPORTANT: This function mirrors the frontend's getClaudeDetectionPaths() + in apps/frontend/src/main/cli-tool-manager.ts. Both implementations MUST + be kept in sync to ensure consistent detection behavior across the + Python backend and Electron frontend. + + When adding new detection paths, update BOTH: + 1. This function (_get_claude_detection_paths in client.py) + 2. getClaudeDetectionPaths() in cli-tool-manager.ts Returns: - Dict with 'homebrew', 'platform', and 'nvm_versions_dir' keys + Dict with 'homebrew', 'platform', and 'nvm' path lists """ - return get_claude_detection_paths_structured() + home_dir = Path.home() + is_windows = platform.system() == "Windows" + + homebrew_paths = [ + "/opt/homebrew/bin/claude", # Apple Silicon + "/usr/local/bin/claude", # Intel Mac + ] + + if is_windows: + platform_paths = [ + str(home_dir / "AppData" / "Local" / "Programs" / "claude" / "claude.exe"), + str(home_dir / "AppData" / "Roaming" / "npm" / "claude.cmd"), + str(home_dir / ".local" / "bin" / "claude.exe"), + "C:\\Program Files\\Claude\\claude.exe", + "C:\\Program Files (x86)\\Claude\\claude.exe", + ] + else: + platform_paths = [ + str(home_dir / ".local" / "bin" / "claude"), + str(home_dir / "bin" / "claude"), + ] + + nvm_versions_dir = str(home_dir / ".nvm" / "versions" / "node") + + return { + "homebrew": homebrew_paths, + "platform": platform_paths, + "nvm_versions_dir": nvm_versions_dir, + } + + +def _is_secure_path(path_str: str) -> bool: + """ + Validate that a path doesn't contain dangerous characters. + + Prevents command injection attacks by rejecting paths with shell metacharacters, + directory traversal patterns, or environment variable expansion. + + Args: + path_str: Path to validate + + Returns: + True if the path is safe, False otherwise + """ + import re + + dangerous_patterns = [ + r'[;&|`${}[\]<>!"^]', # Shell metacharacters + r"%[^%]+%", # Windows environment variable expansion + r"\.\./", # Unix directory traversal + r"\.\.\\", # Windows directory traversal + r"[\r\n]", # Newlines (command injection) + ] + + for pattern in dangerous_patterns: + if re.search(pattern, path_str): + return False + + return True def _validate_claude_cli(cli_path: str) -> tuple[bool, str | None]: @@ -174,11 +240,13 @@ def _validate_claude_cli(cli_path: str) -> tuple[bool, str | None]: import re # Security validation: reject paths with shell metacharacters or directory traversal - if not validate_cli_path(cli_path): + if not _is_secure_path(cli_path): logger.warning(f"Rejecting insecure Claude CLI path: {cli_path}") return False, None try: + is_windows = platform.system() == "Windows" + # Augment PATH with the CLI directory for proper resolution env = os.environ.copy() cli_dir = os.path.dirname(cli_path) @@ -189,9 +257,11 @@ def _validate_claude_cli(cli_path: str) -> tuple[bool, str | None]: # /d = disable AutoRun registry commands # /s = strip first and last quotes, preserving inner quotes # /c = run command then terminate - if is_windows() and cli_path.lower().endswith((".cmd", ".bat")): - # Get cmd.exe path from platform module - cmd_exe = get_comspec_path() + if is_windows and cli_path.lower().endswith((".cmd", ".bat")): + # Get cmd.exe path from environment or use default + cmd_exe = os.environ.get("ComSpec") or os.path.join( + os.environ.get("SystemRoot", "C:\\Windows"), "System32", "cmd.exe" + ) # Use double-quoted command line for paths with spaces cmd_line = f'""{cli_path}" --version"' result = subprocess.run( @@ -209,7 +279,7 @@ def _validate_claude_cli(cli_path: str) -> tuple[bool, str | None]: text=True, timeout=5, env=env, - creationflags=subprocess.CREATE_NO_WINDOW if is_windows() else 0, + creationflags=subprocess.CREATE_NO_WINDOW if is_windows else 0, ) if result.returncode == 0: @@ -247,6 +317,7 @@ def find_claude_cli() -> str | None: logger.debug(f"Using cached Claude CLI path: {cached}") return cached + is_windows = platform.system() == "Windows" paths = _get_claude_detection_paths() # 1. Check environment variable override @@ -272,7 +343,7 @@ def find_claude_cli() -> str | None: return which_path # 3. Homebrew paths (macOS) - if is_macos(): + if platform.system() == "Darwin": for hb_path in paths["homebrew"]: if Path(hb_path).exists(): valid, version = _validate_claude_cli(hb_path) @@ -283,7 +354,7 @@ def find_claude_cli() -> str | None: return hb_path # 4. NVM paths (Unix only) - check Node.js version manager installations - if not is_windows(): + if not is_windows: nvm_dir = Path(paths["nvm_versions_dir"]) if nvm_dir.exists(): try: diff --git a/apps/backend/core/dependency_validator.py b/apps/backend/core/dependency_validator.py index fdad64a759..1e55c1b362 100644 --- a/apps/backend/core/dependency_validator.py +++ b/apps/backend/core/dependency_validator.py @@ -8,6 +8,8 @@ import sys from pathlib import Path +from core.platform import is_linux, is_windows + def validate_platform_dependencies() -> None: """ @@ -18,14 +20,21 @@ def validate_platform_dependencies() -> None: with helpful installation instructions. """ # Check Windows-specific dependencies - # pywin32 is required on all Python versions on Windows (ACS-306) - # The MCP library unconditionally imports win32api on Windows - if sys.platform == "win32": + if is_windows() and sys.version_info >= (3, 12): try: import pywintypes # noqa: F401 except ImportError: _exit_with_pywin32_error() + # Check Linux-specific dependencies (ACS-310) + # Note: secretstorage is optional for app functionality (falls back to .env), + # but we validate it to ensure proper OAuth token storage via keyring + if is_linux(): + try: + import secretstorage # noqa: F401 + except ImportError: + _warn_missing_secretstorage() + def _exit_with_pywin32_error() -> None: """Exit with helpful error message for missing pywin32.""" @@ -76,3 +85,39 @@ def _exit_with_pywin32_error() -> None: "\n" f"Current Python: {sys.executable}\n" ) + + +def _warn_missing_secretstorage() -> None: + """Emit warning message for missing secretstorage. + + Note: This is a warning, not a hard error - the app will fall back to .env + file storage for OAuth tokens. We warn users to ensure they understand the + security implications. + """ + # Use sys.prefix to detect the virtual environment path + venv_activate = Path(sys.prefix) / "bin" / "activate" + + sys.stderr.write( + "Warning: Linux dependency 'secretstorage' is not installed.\n" + "\n" + "Auto Claude can use secretstorage for secure OAuth token storage via\n" + "the system keyring (gnome-keyring, kwallet, etc.). Without it, tokens\n" + "will be stored in plaintext in your .env file.\n" + "\n" + "To enable keyring integration:\n" + "1. Activate your virtual environment:\n" + f" source {venv_activate}\n" + "\n" + "2. Install secretstorage:\n" + " pip install 'secretstorage>=3.3.3'\n" + "\n" + " Or reinstall all dependencies:\n" + " pip install -r requirements.txt\n" + "\n" + "Note: The app will continue to work, but OAuth tokens will be stored\n" + "in your .env file instead of the system keyring.\n" + "\n" + f"Current Python: {sys.executable}\n" + ) + sys.stderr.flush() + # Continue execution - this is a warning, not a blocking error diff --git a/apps/backend/core/simple_client.py b/apps/backend/core/simple_client.py index 59e88a45d4..4245ba1a2d 100644 --- a/apps/backend/core/simple_client.py +++ b/apps/backend/core/simple_client.py @@ -96,12 +96,9 @@ def create_simple_client( "max_turns": max_turns, "cwd": str(cwd.resolve()) if cwd else None, "env": sdk_env, + "max_thinking_tokens": max_thinking_tokens, } - # Only add max_thinking_tokens if not None (Haiku doesn't support extended thinking) - if max_thinking_tokens is not None: - options_kwargs["max_thinking_tokens"] = max_thinking_tokens - # Add CLI path if found if cli_path: options_kwargs["cli_path"] = cli_path diff --git a/apps/backend/runners/github/orchestrator.py b/apps/backend/runners/github/orchestrator.py index ad67d57542..2e6d546994 100644 --- a/apps/backend/runners/github/orchestrator.py +++ b/apps/backend/runners/github/orchestrator.py @@ -660,110 +660,6 @@ async def followup_review_pr(self, pr_number: int) -> PRReviewResult: if not has_commits and not has_file_changes: base_sha = previous_review.reviewed_commit_sha[:8] - - # Check if CI status has changed since last review - # If CI was failing before but now passes, we need to update the verdict - current_failing = ci_status.get("failing", 0) - current_awaiting = ci_status.get("awaiting_approval", 0) - - # Helper to detect CI-related blockers (includes workflows pending) - def is_ci_blocker(b: str) -> bool: - return b.startswith("CI Failed:") or b.startswith( - "Workflows Pending:" - ) - - previous_blockers = getattr(previous_review, "blockers", []) - previous_was_blocked_by_ci = ( - previous_review.verdict == MergeVerdict.BLOCKED - and any(is_ci_blocker(b) for b in previous_blockers) - ) - - # Determine the appropriate verdict based on current CI status - # CI/Workflow status check (both block merging) - ci_or_workflow_blocking = current_failing > 0 or current_awaiting > 0 - - if ci_or_workflow_blocking: - # CI is still failing or workflows pending - keep blocked verdict - updated_verdict = MergeVerdict.BLOCKED - if current_failing > 0: - updated_reasoning = ( - f"No code changes since last review. " - f"{current_failing} CI check(s) still failing." - ) - failed_checks = ci_status.get("failed_checks", []) - ci_note = ( - f" Failing: {', '.join(failed_checks)}" - if failed_checks - else "" - ) - no_change_summary = ( - f"No new commits since last review. " - f"CI status: {current_failing} check(s) failing.{ci_note}" - ) - else: - updated_reasoning = ( - f"No code changes since last review. " - f"{current_awaiting} workflow(s) awaiting approval." - ) - no_change_summary = ( - f"No new commits since last review. " - f"{current_awaiting} workflow(s) awaiting maintainer approval." - ) - elif previous_was_blocked_by_ci and not ci_or_workflow_blocking: - # CI/Workflows have recovered! Update verdict to reflect this - safe_print( - "[Followup] CI recovered - updating verdict from BLOCKED", - flush=True, - ) - # Check for remaining non-CI blockers (use helper defined above) - non_ci_blockers = [ - b for b in previous_blockers if not is_ci_blocker(b) - ] - - # Determine verdict based on findings AND remaining blockers - if non_ci_blockers: - # There are still non-CI blockers - stay blocked - updated_verdict = MergeVerdict.BLOCKED - updated_reasoning = ( - "CI checks now passing. Non-CI blockers still remain: " - + ", ".join(non_ci_blockers[:3]) - ) - elif previous_review.findings: - # Check finding severity - only low severity is non-blocking - findings = previous_review.findings - high_medium = [ - f - for f in findings - if f.severity - in ( - ReviewSeverity.HIGH, - ReviewSeverity.MEDIUM, - ReviewSeverity.CRITICAL, - ) - ] - if high_medium: - # There are blocking findings - needs revision - updated_verdict = MergeVerdict.NEEDS_REVISION - updated_reasoning = f"CI checks now passing. {len(high_medium)} code finding(s) still require attention." - else: - # Only low-severity findings - safe to merge - updated_verdict = MergeVerdict.READY_TO_MERGE - updated_reasoning = f"CI checks now passing. {len(findings)} non-blocking suggestion(s) to consider." - else: - updated_verdict = MergeVerdict.READY_TO_MERGE - updated_reasoning = ( - "CI checks now passing. No outstanding code issues." - ) - no_change_summary = ( - "No new commits since last review. " - "CI checks are now passing. Previous findings still apply." - ) - else: - # No CI-related changes, keep previous verdict - updated_verdict = previous_review.verdict - updated_reasoning = "No changes since last review." - no_change_summary = "No new commits since last review. Previous findings still apply." - safe_print( f"[Followup] No changes since last review at {base_sha}", flush=True, diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index 6a65dd4780..eda564f69f 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -532,31 +532,12 @@ async def review(self, context: PRContext) -> PRReviewResult: head_sha, context.pr_number ) project_root = worktree_path - # Count files in worktree to give user visibility (with limit to avoid slowdown) - MAX_FILE_COUNT = 10000 - try: - file_count = 0 - for f in worktree_path.rglob("*"): - if f.is_file() and ".git" not in f.parts: - file_count += 1 - if file_count >= MAX_FILE_COUNT: - break - except (OSError, PermissionError): - file_count = 0 - file_count_str = ( - f"{file_count:,}+" - if file_count >= MAX_FILE_COUNT - else f"{file_count:,}" - ) - # Always log worktree creation with file count (not gated by DEBUG_MODE) - safe_print( - f"[PRReview] Created temporary worktree: {worktree_path.name} ({file_count_str} files)", - flush=True, - ) - safe_print( - f"[PRReview] Worktree contains PR branch HEAD: {head_sha[:8]}", - flush=True, - ) + if DEBUG_MODE: + safe_print( + f"[PRReview] DEBUG: Using worktree as " + f"project_root={project_root}", + flush=True, + ) except (RuntimeError, ValueError) as e: if DEBUG_MODE: safe_print( diff --git a/apps/backend/runners/github/services/pr_review_engine.py b/apps/backend/runners/github/services/pr_review_engine.py index 867a8f1918..6f4c0201c2 100644 --- a/apps/backend/runners/github/services/pr_review_engine.py +++ b/apps/backend/runners/github/services/pr_review_engine.py @@ -34,7 +34,6 @@ ReviewPass, StructuralIssue, ) - from phase_config import resolve_model_id from services.io_utils import safe_print from services.prompt_manager import PromptManager from services.response_parsers import ResponseParser diff --git a/apps/backend/runners/github/services/sdk_utils.py b/apps/backend/runners/github/services/sdk_utils.py index f1d212073a..4552f3f5e8 100644 --- a/apps/backend/runners/github/services/sdk_utils.py +++ b/apps/backend/runners/github/services/sdk_utils.py @@ -176,10 +176,6 @@ async def process_sdk_stream( if DEBUG_MODE: safe_print(f"[DEBUG {context_name}] Awaiting response stream...") - # Track activity for progress logging - last_progress_log = 0 - PROGRESS_LOG_INTERVAL = 10 # Log progress every N messages - try: async for msg in client.receive_response(): try: @@ -253,15 +249,23 @@ async def process_sdk_stream( agents_invoked.append(agent_name) # Track this tool ID to log its result later subagent_tool_ids[tool_id] = agent_name - # Log with model info if available - model_info = f" [{_short_model_name(model)}]" if model else "" - safe_print( - f"[{context_name}] Invoking agent: {agent_name}{model_info}" - ) - elif tool_name != "StructuredOutput": - # Log meaningful tool info (not just tool name) - tool_detail = _get_tool_detail(tool_name, tool_input) - safe_print(f"[{context_name}] {tool_detail}") + safe_print(f"[{context_name}] Invoked agent: {agent_name}") + elif tool_name == "StructuredOutput": + if tool_input: + # Warn if overwriting existing structured output + if structured_output is not None: + logger.warning( + f"[{context_name}] Multiple StructuredOutput blocks received, " + f"overwriting previous output" + ) + structured_output = tool_input + safe_print(f"[{context_name}] Received structured output") + # Invoke callback + if on_structured_output: + on_structured_output(tool_input) + elif DEBUG_MODE: + # Log other tool calls in debug mode + safe_print(f"[DEBUG {context_name}] Other tool: {tool_name}") # Invoke callback for all tool uses if on_tool_use: @@ -292,12 +296,10 @@ async def process_sdk_stream( safe_print( f"[Agent:{agent_name}] {status}: {result_preview}{'...' if len(str(result_content)) > 600 else ''}" ) - else: - # Show tool completion for visibility (not gated by DEBUG) - status = "ERROR" if is_error else "done" - # Show brief preview of result for context - result_preview = ( - str(result_content)[:100].replace("\n", " ").strip() + elif DEBUG_MODE: + status = "ERROR" if is_error else "OK" + safe_print( + f"[DEBUG {context_name}] Tool result: {tool_id} [{status}]" ) if result_preview: safe_print( @@ -327,11 +329,8 @@ async def process_sdk_stream( if agent_name not in agents_invoked: agents_invoked.append(agent_name) subagent_tool_ids[tool_id] = agent_name - # Log with model info if available - model_info = ( - f" [{_short_model_name(model)}]" - if model - else "" + safe_print( + f"[{context_name}] Invoking agent: {agent_name}" ) safe_print( f"[{context_name}] Invoking agent: {agent_name}{model_info}" diff --git a/apps/backend/security/shell_validators.py b/apps/backend/security/shell_validators.py index 4b66fc64f9..f5653df2b4 100644 --- a/apps/backend/security/shell_validators.py +++ b/apps/backend/security/shell_validators.py @@ -16,7 +16,7 @@ from project_analyzer import is_command_allowed -from .parser import _cross_platform_basename, extract_commands, split_command_segments +from .parser import extract_commands, split_command_segments from .profile import get_security_profile from .validation_models import ValidationResult @@ -80,18 +80,7 @@ def validate_shell_c_command(command_string: str) -> ValidationResult: inner_command = _extract_c_argument(command_string) if inner_command is None: - # Not a -c invocation (e.g., "bash script.sh") - # Block dangerous shell constructs that could bypass sandbox restrictions: - # - Process substitution: <(...) or >(...) - # - Command substitution in dangerous contexts: $(...) - dangerous_patterns = ["<(", ">("] - for pattern in dangerous_patterns: - if pattern in command_string: - return ( - False, - f"Process substitution '{pattern}' not allowed in shell commands", - ) - # Allow simple shell invocations (e.g., "bash script.sh") + # Not a -c invocation (e.g., "bash script.sh") - allow it # The script itself would need to be in allowed commands return True, "" @@ -137,8 +126,8 @@ def validate_shell_c_command(command_string: str) -> ValidationResult: segment_commands = extract_commands(segment) if segment_commands: first_cmd = segment_commands[0] - # Handle paths like /bin/bash or C:\Windows\System32\bash.exe - base_cmd = _cross_platform_basename(first_cmd) + # Handle paths like /bin/bash + base_cmd = first_cmd.rsplit("/", 1)[-1] if "/" in first_cmd else first_cmd if base_cmd in SHELL_INTERPRETERS: valid, err = validate_shell_c_command(segment) if not valid: diff --git a/apps/frontend/scripts/download-python.cjs b/apps/frontend/scripts/download-python.cjs index e626f3e753..71edd52e1f 100644 --- a/apps/frontend/scripts/download-python.cjs +++ b/apps/frontend/scripts/download-python.cjs @@ -710,17 +710,20 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { // Note: Same list exists in python-env-manager.ts - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library - // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs - // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py + // secretstorage is platform-critical for Linux (ACS-310) - required for OAuth token storage + const platformCriticalPackages = { + 'win32': ['pywintypes'], // Check for 'pywintypes' instead of 'pywin32' (pywin32 installs top-level modules) + 'linux': ['secretstorage'] // Linux OAuth token storage via Freedesktop.org Secret Service + }; const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] - .concat(info.nodePlatform === 'win32' ? ['pywintypes'] : []); + .concat(platformCriticalPackages[info.nodePlatform] || []); const missingPackages = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); - const initFile = path.join(pkgPath, '__init__.py'); + const initPath = path.join(pkgPath, '__init__.py'); // For single-file modules (like pywintypes.py), check for the file directly const moduleFile = path.join(sitePackagesDir, pkg + '.py'); // Package is valid if directory+__init__.py exists OR single-file module exists - return !fs.existsSync(initFile) && !fs.existsSync(moduleFile); + return !(fs.existsSync(pkgPath) && fs.existsSync(initPath)) && !fs.existsSync(moduleFile); }); if (missingPackages.length > 0) { @@ -819,17 +822,20 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { // Note: Same list exists in python-env-manager.ts - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library - // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs - // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py + // secretstorage is platform-critical for Linux (ACS-310) - required for OAuth token storage + const platformCriticalPackages = { + 'win32': ['pywintypes'], // Check for 'pywintypes' instead of 'pywin32' (pywin32 installs top-level modules) + 'linux': ['secretstorage'] // Linux OAuth token storage via Freedesktop.org Secret Service + }; const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] - .concat(info.nodePlatform === 'win32' ? ['pywintypes'] : []); + .concat(platformCriticalPackages[info.nodePlatform] || []); const postInstallMissing = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); - const initFile = path.join(pkgPath, '__init__.py'); + const initPath = path.join(pkgPath, '__init__.py'); // For single-file modules (like pywintypes.py), check for the file directly const moduleFile = path.join(sitePackagesDir, pkg + '.py'); // Package is valid if directory+__init__.py exists OR single-file module exists - return !fs.existsSync(initFile) && !fs.existsSync(moduleFile); + return !(fs.existsSync(pkgPath) && fs.existsSync(initPath)) && !fs.existsSync(moduleFile); }); if (postInstallMissing.length > 0) { diff --git a/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts b/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts index ea4cec57d3..e8cb7faef6 100644 --- a/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts +++ b/apps/frontend/src/__tests__/integration/terminal-copy-paste.test.ts @@ -77,6 +77,13 @@ describe('Terminal copy/paste integration', () => { }; }); + // Mock requestAnimationFrame for xterm.js integration tests + global.requestAnimationFrame = vi.fn((callback: FrameRequestCallback) => { + // Synchronously execute the callback to avoid timing issues in tests + callback.call(window, 0); + return 0; + }) as unknown as Mock; + // Mock navigator.clipboard mockClipboard = { writeText: vi.fn().mockResolvedValue(undefined), diff --git a/apps/frontend/src/main/cli-tool-manager.ts b/apps/frontend/src/main/cli-tool-manager.ts index 442bd438a7..da927c64e4 100644 --- a/apps/frontend/src/main/cli-tool-manager.ts +++ b/apps/frontend/src/main/cli-tool-manager.ts @@ -20,7 +20,7 @@ * - Graceful fallbacks when tools not found */ -import { execFileSync, execFile, type ExecFileOptionsWithStringEncoding, type ExecFileSyncOptions } from 'child_process'; +import { execFileSync, execFile } from 'child_process'; import { existsSync, readdirSync, promises as fsPromises } from 'fs'; import path from 'path'; import os from 'os'; @@ -33,10 +33,10 @@ import { findHomebrewPython as findHomebrewPythonUtil } from './utils/homebrew-p const execFileAsync = promisify(execFile); -export type ExecFileSyncOptionsWithVerbatim = ExecFileSyncOptions & { +type ExecFileSyncOptionsWithVerbatim = import('child_process').ExecFileSyncOptions & { windowsVerbatimArguments?: boolean; }; -export type ExecFileAsyncOptionsWithVerbatim = ExecFileOptionsWithStringEncoding & { +type ExecFileAsyncOptionsWithVerbatim = import('child_process').ExecFileOptionsWithStringEncoding & { windowsVerbatimArguments?: boolean; }; diff --git a/apps/frontend/src/main/env-utils.ts b/apps/frontend/src/main/env-utils.ts index 5f592c0562..bd9765b57f 100644 --- a/apps/frontend/src/main/env-utils.ts +++ b/apps/frontend/src/main/env-utils.ts @@ -16,7 +16,6 @@ import { promises as fsPromises } from 'fs'; import { execFileSync, execFile } from 'child_process'; import { promisify } from 'util'; import { getSentryEnvForSubprocess } from './sentry'; -import { isWindows, isUnix, getPathDelimiter } from './platform'; const execFileAsync = promisify(execFile); diff --git a/apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts b/apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts index e257bd6339..28f912d6c0 100644 --- a/apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/claude-code-handlers.ts @@ -16,7 +16,7 @@ import { promisify } from 'util'; import { IPC_CHANNELS, DEFAULT_APP_SETTINGS } from '../../shared/constants'; import type { IPCResult } from '../../shared/types'; import type { ClaudeCodeVersionInfo, ClaudeInstallationList, ClaudeInstallationInfo } from '../../shared/types/cli'; -import { getToolInfo, configureTools, sortNvmVersionDirs, getClaudeDetectionPaths, type ExecFileAsyncOptionsWithVerbatim } from '../cli-tool-manager'; +import { getToolInfo, configureTools, sortNvmVersionDirs, getClaudeDetectionPaths } from '../cli-tool-manager'; import { readSettingsFile, writeSettingsFile } from '../settings-utils'; import { isSecurePath } from '../utils/windows-paths'; import semver from 'semver'; @@ -38,11 +38,6 @@ async function validateClaudeCliAsync(cliPath: string): Promise<[boolean, string try { const isWindows = process.platform === 'win32'; - // Security validation: reject paths with shell metacharacters or directory traversal - if (isWindows && !isSecurePath(cliPath)) { - throw new Error(`Claude CLI path failed security validation: ${cliPath}`); - } - // Augment PATH with the CLI directory for proper resolution const cliDir = path.dirname(cliPath); const env = { @@ -61,14 +56,12 @@ async function validateClaudeCliAsync(cliPath: string): Promise<[boolean, string || path.join(process.env.SystemRoot || 'C:\\Windows', 'System32', 'cmd.exe'); // Use double-quoted command line for paths with spaces const cmdLine = `""${cliPath}" --version"`; - const execOptions: ExecFileAsyncOptionsWithVerbatim = { + const result = await execFileAsync(cmdExe, ['/d', '/s', '/c', cmdLine], { encoding: 'utf-8', timeout: 5000, windowsHide: true, - windowsVerbatimArguments: true, env, - }; - const result = await execFileAsync(cmdExe, ['/d', '/s', '/c', cmdLine], execOptions); + }); stdout = result.stdout; } else { const result = await execFileAsync(cliPath, ['--version'], { diff --git a/apps/frontend/src/main/python-env-manager.ts b/apps/frontend/src/main/python-env-manager.ts index 510dbf566c..335ca7651a 100644 --- a/apps/frontend/src/main/python-env-manager.ts +++ b/apps/frontend/src/main/python-env-manager.ts @@ -4,7 +4,7 @@ import path from 'path'; import { EventEmitter } from 'events'; import { app } from 'electron'; import { findPythonCommand, getBundledPythonPath } from './python-detector'; -import { isWindows } from './platform'; +import { isLinux, isWindows } from './platform'; export interface PythonEnvStatus { ready: boolean; @@ -128,21 +128,27 @@ export class PythonEnvManager extends EventEmitter { // Note: Same list exists in download-python.cjs - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library - // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs - // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py - const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] - .concat(isWindows() ? ['pywintypes'] : []); - - // Check each package exists with valid structure - // For traditional packages: directory + __init__.py - // For single-file modules (like pywintypes.py): just the .py file + // secretstorage is platform-critical for Linux (ACS-310) - required for OAuth token storage + const platformCriticalPackages: Record = { + win32: ['pywintypes'], // Check for 'pywintypes' instead of 'pywin32' (pywin32 installs top-level modules) + linux: ['secretstorage'] // Linux OAuth token storage via Freedesktop.org Secret Service + }; + const criticalPackages = [ + 'claude_agent_sdk', + 'dotenv', + 'pydantic_core', + ...(isWindows() ? platformCriticalPackages.win32 : []), + ...(isLinux() ? platformCriticalPackages.linux : []) + ]; + + // Check each package exists with valid structure (directory + __init__.py or single-file module) const missingPackages = criticalPackages.filter((pkg) => { const pkgPath = path.join(sitePackagesPath, pkg); const initPath = path.join(pkgPath, '__init__.py'); // For single-file modules (like pywintypes.py), check for the file directly const moduleFile = path.join(sitePackagesPath, `${pkg}.py`); // Package is valid if directory+__init__.py exists OR single-file module exists - return !existsSync(initPath) && !existsSync(moduleFile); + return !(existsSync(pkgPath) && existsSync(initPath)) && !existsSync(moduleFile); }); // Log missing packages for debugging diff --git a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts index 0e985a55f1..2512384100 100644 --- a/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts +++ b/apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts @@ -4,18 +4,7 @@ import path from 'path'; import { describe, expect, it, vi, beforeEach } from 'vitest'; import type * as pty from '@lydell/node-pty'; import type { TerminalProcess } from '../types'; -import { buildCdCommand, escapeShellArg } from '../../../shared/utils/shell-escape'; - -// Mock the platform module (main/platform/index.ts) -vi.mock('../../platform', () => ({ - isWindows: vi.fn(() => false), - isMacOS: vi.fn(() => false), - isLinux: vi.fn(() => false), - isUnix: vi.fn(() => false), - getCurrentOS: vi.fn(() => 'linux'), -})); - -import { isWindows } from '../../platform'; +import { buildCdCommand } from '../../../shared/utils/shell-escape'; /** Escape special regex characters in a string for safe use in RegExp constructor */ const escapeForRegex = (str: string): string => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -207,7 +196,26 @@ describe('claude-integration-handler', () => { mockPlatform(platform); }); - it('uses the resolved CLI path and PATH prefix when invoking Claude', async () => { + const terminal = createMockTerminal(); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', undefined, () => null, vi.fn()); + + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain(buildCdCommand('/tmp/project')); + expect(written).toContain("PATH='/opt/claude/bin:/usr/bin' "); + expect(written).toContain("'/opt/claude bin/claude'\\''s'"); + expect(mockReleaseSessionId).toHaveBeenCalledWith('term-1'); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + expect(profileManager.getActiveProfile).toHaveBeenCalled(); + expect(profileManager.markProfileUsed).toHaveBeenCalledWith('default'); + }); + + it('converts Windows PATH separators to colons for bash invocations', async () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + Object.defineProperty(process, 'platform', { value: 'win32' }); + + try { mockGetClaudeCliInvocation.mockReturnValue({ command: "/opt/claude bin/claude's", env: { PATH: '/opt/claude/bin:/usr/bin' }, @@ -530,6 +538,222 @@ describe('claude-integration-handler', () => { expect(() => invokeClaude(terminal, '/tmp/project', 'prof-err', () => null, vi.fn())).toThrow('disk full'); expect(terminal.pty.write).not.toHaveBeenCalled(); }); + + it('uses the temp token flow when the active profile has an oauth token', async () => { + const command = '/opt/claude/bin/claude'; + const profileManager = { + getActiveProfile: vi.fn(), + getProfile: vi.fn(() => ({ + id: 'prof-1', + name: 'Work', + isDefault: false, + oauthToken: 'token-value', + })), + getProfileToken: vi.fn(() => 'token-value'), + markProfileUsed: vi.fn(), + }; + + mockGetClaudeCliInvocation.mockReturnValue({ + command, + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + mockGetClaudeProfileManager.mockReturnValue(profileManager); + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(1234); + + const terminal = createMockTerminal({ id: 'term-3' }); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', 'prof-1', () => null, vi.fn()); + + const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; + const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; + const tokenPrefix = path.join(tmpdir(), '.claude-token-1234-'); + expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); + expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain("HISTFILE= HISTCONTROL=ignorespace "); + expect(written).toContain(`source '${tokenPath}'`); + expect(written).toContain(`rm -f '${tokenPath}'`); + expect(written).toContain(`exec '${command}'`); + expect(profileManager.getProfile).toHaveBeenCalledWith('prof-1'); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + + nowSpy.mockRestore(); + }); + + it('prefers the temp token flow when profile has both oauth token and config dir', async () => { + const command = '/opt/claude/bin/claude'; + const profileManager = { + getActiveProfile: vi.fn(), + getProfile: vi.fn(() => ({ + id: 'prof-both', + name: 'Work', + isDefault: false, + oauthToken: 'token-value', + configDir: '/tmp/claude-config', + })), + getProfileToken: vi.fn(() => 'token-value'), + markProfileUsed: vi.fn(), + }; + + mockGetClaudeCliInvocation.mockReturnValue({ + command, + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + mockGetClaudeProfileManager.mockReturnValue(profileManager); + const nowSpy = vi.spyOn(Date, 'now').mockReturnValue(5678); + + const terminal = createMockTerminal({ id: 'term-both' }); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', 'prof-both', () => null, vi.fn()); + + const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; + const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; + const tokenPrefix = path.join(tmpdir(), '.claude-token-5678-'); + expect(tokenPath).toMatch(new RegExp(`^${escapeForRegex(tokenPrefix)}[0-9a-f]{16}$`)); + expect(tokenContents).toBe("export CLAUDE_CODE_OAUTH_TOKEN='token-value'\n"); + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain(`source '${tokenPath}'`); + expect(written).toContain(`rm -f '${tokenPath}'`); + expect(written).toContain(`exec '${command}'`); + expect(written).not.toContain('CLAUDE_CONFIG_DIR='); + expect(profileManager.getProfile).toHaveBeenCalledWith('prof-both'); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + expect(profileManager.markProfileUsed).toHaveBeenCalledWith('prof-both'); + + nowSpy.mockRestore(); + }); + + it('handles missing profiles by falling back to the default command', async () => { + const command = '/opt/claude/bin/claude'; + const profileManager = { + getActiveProfile: vi.fn(), + getProfile: vi.fn(() => undefined), + getProfileToken: vi.fn(() => null), + markProfileUsed: vi.fn(), + }; + + mockGetClaudeCliInvocation.mockReturnValue({ + command, + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + mockGetClaudeProfileManager.mockReturnValue(profileManager); + + const terminal = createMockTerminal({ id: 'term-6' }); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', 'missing', () => null, vi.fn()); + + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain(`'${command}'`); + expect(profileManager.getProfile).toHaveBeenCalledWith('missing'); + expect(profileManager.markProfileUsed).not.toHaveBeenCalled(); + }); + + it('uses the config dir flow when the active profile has a config dir', async () => { + const command = '/opt/claude/bin/claude'; + const profileManager = { + getActiveProfile: vi.fn(), + getProfile: vi.fn(() => ({ + id: 'prof-2', + name: 'Work', + isDefault: false, + configDir: '/tmp/claude-config', + })), + getProfileToken: vi.fn(() => null), + markProfileUsed: vi.fn(), + }; + + mockGetClaudeCliInvocation.mockReturnValue({ + command, + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + mockGetClaudeProfileManager.mockReturnValue(profileManager); + + const terminal = createMockTerminal({ id: 'term-4' }); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', 'prof-2', () => null, vi.fn()); + + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain("HISTFILE= HISTCONTROL=ignorespace "); + expect(written).toContain("CLAUDE_CONFIG_DIR='/tmp/claude-config'"); + expect(written).toContain(`exec '${command}'`); + expect(profileManager.getProfile).toHaveBeenCalledWith('prof-2'); + expect(profileManager.markProfileUsed).toHaveBeenCalledWith('prof-2'); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + }); + + it('uses profile switching when a non-default profile is requested', async () => { + const command = '/opt/claude/bin/claude'; + const profileManager = { + getActiveProfile: vi.fn(), + getProfile: vi.fn(() => ({ + id: 'prof-3', + name: 'Team', + isDefault: false, + })), + getProfileToken: vi.fn(() => null), + markProfileUsed: vi.fn(), + }; + + mockGetClaudeCliInvocation.mockReturnValue({ + command, + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + mockGetClaudeProfileManager.mockReturnValue(profileManager); + + const terminal = createMockTerminal({ id: 'term-5' }); + + const { invokeClaude } = await import('../claude-integration-handler'); + invokeClaude(terminal, '/tmp/project', 'prof-3', () => null, vi.fn()); + + const written = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(written).toContain(`'${command}'`); + expect(written).toContain("PATH='/opt/claude/bin:/usr/bin' "); + expect(profileManager.getProfile).toHaveBeenCalledWith('prof-3'); + expect(profileManager.markProfileUsed).toHaveBeenCalledWith('prof-3'); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + }); + + it('uses --continue regardless of sessionId (sessionId is deprecated)', async () => { + mockGetClaudeCliInvocation.mockReturnValue({ + command: '/opt/claude/bin/claude', + env: { PATH: '/opt/claude/bin:/usr/bin' }, + }); + + const terminal = createMockTerminal({ + id: 'term-2', + cwd: undefined, + projectPath: '/tmp/project', + }); + + const { resumeClaude } = await import('../claude-integration-handler'); + + // Even when sessionId is passed, it should be ignored and --continue used + resumeClaude(terminal, 'abc123', () => null); + + const resumeCall = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(resumeCall).toContain("PATH='/opt/claude/bin:/usr/bin' "); + expect(resumeCall).toContain("'/opt/claude/bin/claude' --continue"); + expect(resumeCall).not.toContain('--resume'); + // sessionId is cleared because --continue doesn't track specific sessions + expect(terminal.claudeSessionId).toBeUndefined(); + expect(terminal.isClaudeMode).toBe(true); + expect(mockPersistSession).toHaveBeenCalledWith(terminal); + + vi.mocked(terminal.pty.write).mockClear(); + mockPersistSession.mockClear(); + terminal.projectPath = undefined; + terminal.isClaudeMode = false; + resumeClaude(terminal, undefined, () => null); + const continueCall = vi.mocked(terminal.pty.write).mock.calls[0][0] as string; + expect(continueCall).toContain("'/opt/claude/bin/claude' --continue"); + expect(terminal.isClaudeMode).toBe(true); + expect(terminal.claudeSessionId).toBeUndefined(); + expect(mockPersistSession).not.toHaveBeenCalled(); + }); }); /** diff --git a/apps/frontend/src/main/terminal/claude-integration-handler.ts b/apps/frontend/src/main/terminal/claude-integration-handler.ts index 75257eb0c1..47bccb7203 100644 --- a/apps/frontend/src/main/terminal/claude-integration-handler.ts +++ b/apps/frontend/src/main/terminal/claude-integration-handler.ts @@ -27,88 +27,6 @@ function normalizePathForBash(envPath: string): string { return isWindows() ? envPath.replace(/;/g, ':') : envPath; } -/** - * Generate temp file content for OAuth token based on platform - * - * On Windows, creates a .bat file with set command using double-quote syntax; - * on Unix, creates a shell script with export. - * - * @param token - OAuth token value - * @returns Content string for the temp file - */ -function generateTokenTempFileContent(token: string): string { - if (isWindows()) { - // Windows: Use double-quote syntax for set command to handle special characters - // Format: set "VARNAME=value" - quotes allow spaces and special chars in value - // For values inside double quotes, use escapeForWindowsDoubleQuote() because - // caret is literal inside double quotes in cmd.exe (only " needs escaping). - const escapedToken = escapeForWindowsDoubleQuote(token); - return `@echo off\r\nset "CLAUDE_CODE_OAUTH_TOKEN=${escapedToken}"\r\n`; - } - // Unix/macOS: Use export with single-quoted value - return `export CLAUDE_CODE_OAUTH_TOKEN=${escapeShellArg(token)}\n`; -} - -/** - * Get the file extension for temp files based on platform - * - * @returns File extension including the dot (e.g., '.bat' on Windows, '' on Unix) - */ -function getTempFileExtension(): string { - return isWindows() ? '.bat' : ''; -} - -/** - * Build PATH environment variable prefix for Claude CLI invocation. - * - * On Windows, uses semicolon separators and cmd.exe escaping. - * On Unix/macOS, uses colon separators and bash escaping. - * - * @param pathEnv - PATH environment variable value - * @returns Empty string if no PATH, otherwise platform-specific PATH prefix - */ -function buildPathPrefix(pathEnv: string): string { - if (!pathEnv) { - return ''; - } - - if (isWindows()) { - // Windows: Use semicolon-separated PATH with double-quote escaping - // Format: set "PATH=value" where value uses semicolons - // For values inside double quotes, use escapeForWindowsDoubleQuote() because - // caret is literal inside double quotes in cmd.exe (only " needs escaping). - const escapedPath = escapeForWindowsDoubleQuote(pathEnv); - return `set "PATH=${escapedPath}" && `; - } - - // Unix/macOS: Use colon-separated PATH with bash escaping - // Format: PATH='value' where value uses colons - const normalizedPath = normalizePathForBash(pathEnv); - return `PATH=${escapeShellArg(normalizedPath)} `; -} - -/** - * Escape a command for safe use in shell commands. - * - * On Windows, wraps in double quotes for cmd.exe. Since the value is inside - * double quotes, we use escapeForWindowsDoubleQuote() (only escapes embedded - * double quotes as ""). Caret escaping is NOT used inside double quotes. - * On Unix/macOS, wraps in single quotes for bash. - * - * @param cmd - The command to escape - * @returns The escaped command safe for use in shell commands - */ -function escapeShellCommand(cmd: string): string { - if (isWindows()) { - // Windows: Wrap in double quotes and escape only embedded double quotes - // Inside double quotes, caret is literal, so use escapeForWindowsDoubleQuote() - const escapedCmd = escapeForWindowsDoubleQuote(cmd); - return `"${escapedCmd}"`; - } - // Unix/macOS: Wrap in single quotes for bash - return escapeShellArg(cmd); -} - /** * Flag for YOLO mode (skip all permission prompts) * Extracted as constant to ensure consistency across invokeClaude and invokeClaudeAsync @@ -172,43 +90,12 @@ export function buildClaudeShellCommand( extraFlags?: string ): string { const fullCmd = extraFlags ? `${escapedClaudeCmd}${extraFlags}` : escapedClaudeCmd; - const isWin = isWindows(); - switch (config.method) { case 'temp-file': - if (isWin) { - // Windows: Use batch file approach with 'call' command - // The temp file on Windows is a .bat file that sets CLAUDE_CODE_OAUTH_TOKEN - // We use 'cls' instead of 'clear', and 'call' to execute the batch file - // - // SECURITY: Environment variables set via 'call' persist in memory - // after the batch file is deleted, so we can safely delete the file - // immediately after sourcing it (before running Claude). - // - // For paths inside double quotes (call "..." and del "..."), use - // escapeForWindowsDoubleQuote() instead of escapeShellArgWindows() - // because caret is literal inside double quotes in cmd.exe. - const escapedTempFile = escapeForWindowsDoubleQuote(config.tempFile); - return `cls && ${cwdCommand}${pathPrefix}call "${escapedTempFile}" && del "${escapedTempFile}" && ${fullCmd}\r`; - } else { - // Unix/macOS: Use bash with source command and history-safe prefixes - const escapedTempFile = escapeShellArg(config.tempFile); - return `clear && ${cwdCommand}HISTFILE= HISTCONTROL=ignorespace ${pathPrefix}bash -c "source ${escapedTempFile} && rm -f ${escapedTempFile} && exec ${fullCmd}"\r`; - } + return `clear && ${cwdCommand}HISTFILE= HISTCONTROL=ignorespace ${pathPrefix}bash -c "source ${config.escapedTempFile} && rm -f ${config.escapedTempFile} && exec ${fullCmd}"\r`; case 'config-dir': - if (isWin) { - // Windows: Set environment variable using double-quote syntax - // For values inside double quotes (set "VAR=value"), use - // escapeForWindowsDoubleQuote() because caret is literal inside - // double quotes in cmd.exe (only double quotes need escaping). - const escapedConfigDir = escapeForWindowsDoubleQuote(config.configDir); - return `cls && ${cwdCommand}set "CLAUDE_CONFIG_DIR=${escapedConfigDir}" && ${pathPrefix}${fullCmd}\r`; - } else { - // Unix/macOS: Use bash with config dir and history-safe prefixes - const escapedConfigDir = escapeShellArg(config.configDir); - return `clear && ${cwdCommand}HISTFILE= HISTCONTROL=ignorespace CLAUDE_CONFIG_DIR=${escapedConfigDir} ${pathPrefix}bash -c "exec ${fullCmd}"\r`; - } + return `clear && ${cwdCommand}HISTFILE= HISTCONTROL=ignorespace CLAUDE_CONFIG_DIR=${config.escapedConfigDir} ${pathPrefix}bash -c "exec ${fullCmd}"\r`; default: return `${cwdCommand}${pathPrefix}${fullCmd}\r`; @@ -539,6 +426,20 @@ export function invokeClaude( // Compute extra flags for YOLO mode const extraFlags = dangerouslySkipPermissions ? YOLO_MODE_FLAG : undefined; + terminal.isClaudeMode = true; + // Store YOLO mode setting so it persists across profile switches + terminal.dangerouslySkipPermissions = dangerouslySkipPermissions; + SessionHandler.releaseSessionId(terminal.id); + terminal.claudeSessionId = undefined; + + const startTime = Date.now(); + const projectPath = cwd || terminal.projectPath || terminal.cwd; + + const profileManager = getClaudeProfileManager(); + const activeProfile = profileId + ? profileManager.getProfile(profileId) + : profileManager.getActiveProfile(); + // Track terminal state for cleanup on error const wasClaudeMode = terminal.isClaudeMode; const previousProfileId = terminal.claudeProfileId; @@ -574,47 +475,24 @@ export function invokeClaude( const pathPrefix = buildPathPrefix(claudeEnv.PATH || ''); const needsEnvOverride = profileId && profileId !== previousProfileId; - debugLog('[ClaudeIntegration:invokeClaude] Environment override check:', { - profileIdProvided: !!profileId, - previousProfileId, - needsEnvOverride - }); - - if (needsEnvOverride && activeProfile && !activeProfile.isDefault) { - const token = profileManager.getProfileToken(activeProfile.id); - debugLog('[ClaudeIntegration:invokeClaude] Token retrieval:', { - hasToken: !!token, - tokenLength: token?.length - }); - - if (token) { - const nonce = crypto.randomBytes(8).toString('hex'); - const tempFile = path.join(os.tmpdir(), `.claude-token-${Date.now()}-${nonce}${getTempFileExtension()}`); - debugLog('[ClaudeIntegration:invokeClaude] Writing token to temp file:', tempFile); - fs.writeFileSync( - tempFile, - generateTokenTempFileContent(token), - { mode: 0o600 } - ); - - const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'temp-file', tempFile }, extraFlags); - debugLog('[ClaudeIntegration:invokeClaude] Executing command (temp file method, history-safe)'); - terminal.pty.write(command); - profileManager.markProfileUsed(activeProfile.id); - finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); - debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (temp file) =========='); - return; - } else if (activeProfile.configDir) { - const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'config-dir', configDir: activeProfile.configDir }, extraFlags); - debugLog('[ClaudeIntegration:invokeClaude] Executing command (configDir method, history-safe)'); - terminal.pty.write(command); - profileManager.markProfileUsed(activeProfile.id); - finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); - debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (configDir) =========='); - return; - } else { - debugLog('[ClaudeIntegration:invokeClaude] WARNING: No token or configDir available for non-default profile'); - } + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'temp-file', escapedTempFile }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaude] Executing command (temp file method, history-safe)'); + terminal.pty.write(command); + profileManager.markProfileUsed(activeProfile.id); + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (temp file) =========='); + return; + } else if (activeProfile.configDir) { + const escapedConfigDir = escapeShellArg(activeProfile.configDir); + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'config-dir', escapedConfigDir }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaude] Executing command (configDir method, history-safe)'); + terminal.pty.write(command); + profileManager.markProfileUsed(activeProfile.id); + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (configDir) =========='); + return; + } else { + debugLog('[ClaudeIntegration:invokeClaude] WARNING: No token or configDir available for non-default profile'); } if (activeProfile && !activeProfile.isDefault) { @@ -646,6 +524,21 @@ export function invokeClaude( }); throw error; // Re-throw to allow caller to handle } + + if (activeProfile && !activeProfile.isDefault) { + debugLog('[ClaudeIntegration:invokeClaude] Using terminal environment for non-default profile:', activeProfile.name); + } + + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'default' }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaude] Executing command (default method):', command); + terminal.pty.write(command); + + if (activeProfile) { + profileManager.markProfileUsed(activeProfile.id); + } + + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaude] ========== INVOKE CLAUDE COMPLETE (default) =========='); } /** @@ -734,9 +627,20 @@ export async function invokeClaudeAsync( onSessionCapture: (terminalId: string, projectPath: string, startTime: number) => void, dangerouslySkipPermissions?: boolean ): Promise { - // Track terminal state for cleanup on error - const wasClaudeMode = terminal.isClaudeMode; - const previousProfileId = terminal.claudeProfileId; + debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE START (async) =========='); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Terminal ID:', terminal.id); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Requested profile ID:', profileId); + debugLog('[ClaudeIntegration:invokeClaudeAsync] CWD:', cwd); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Dangerously skip permissions:', dangerouslySkipPermissions); + + // Compute extra flags for YOLO mode + const extraFlags = dangerouslySkipPermissions ? YOLO_MODE_FLAG : undefined; + + terminal.isClaudeMode = true; + // Store YOLO mode setting so it persists across profile switches + terminal.dangerouslySkipPermissions = dangerouslySkipPermissions; + SessionHandler.releaseSessionId(terminal.id); + terminal.claudeSessionId = undefined; const startTime = Date.now(); @@ -777,62 +681,24 @@ export async function invokeClaudeAsync( // Async CLI invocation - non-blocking const cwdCommand = buildCdCommand(cwd); - // Add timeout protection for CLI detection (10s timeout) - const cliInvocationPromise = getClaudeCliInvocationAsync(); - let timeoutId: NodeJS.Timeout | undefined; - const timeoutPromise = new Promise((_, reject) => { - timeoutId = setTimeout(() => reject(new Error('CLI invocation timeout after 10s')), 10000); - }); - const { command: claudeCmd, env: claudeEnv } = await Promise.race([cliInvocationPromise, timeoutPromise]) - .finally(() => { - if (timeoutId) clearTimeout(timeoutId); - }); - - const escapedClaudeCmd = escapeShellCommand(claudeCmd); - const pathPrefix = buildPathPrefix(claudeEnv.PATH || ''); - const needsEnvOverride = profileId && profileId !== previousProfileId; - - debugLog('[ClaudeIntegration:invokeClaudeAsync] Environment override check:', { - profileIdProvided: !!profileId, - previousProfileId, - needsEnvOverride - }); - - if (needsEnvOverride && activeProfile && !activeProfile.isDefault) { - const token = profileManager.getProfileToken(activeProfile.id); - debugLog('[ClaudeIntegration:invokeClaudeAsync] Token retrieval:', { - hasToken: !!token, - tokenLength: token?.length - }); - - if (token) { - const nonce = crypto.randomBytes(8).toString('hex'); - const tempFile = path.join(os.tmpdir(), `.claude-token-${Date.now()}-${nonce}${getTempFileExtension()}`); - debugLog('[ClaudeIntegration:invokeClaudeAsync] Writing token to temp file:', tempFile); - await fsPromises.writeFile( - tempFile, - generateTokenTempFileContent(token), - { mode: 0o600 } - ); - - const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'temp-file', tempFile }, extraFlags); - debugLog('[ClaudeIntegration:invokeClaudeAsync] Executing command (temp file method, history-safe)'); - terminal.pty.write(command); - profileManager.markProfileUsed(activeProfile.id); - finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); - debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (temp file) =========='); - return; - } else if (activeProfile.configDir) { - const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'config-dir', configDir: activeProfile.configDir }, extraFlags); - debugLog('[ClaudeIntegration:invokeClaudeAsync] Executing command (configDir method, history-safe)'); - terminal.pty.write(command); - profileManager.markProfileUsed(activeProfile.id); - finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); - debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (configDir) =========='); - return; - } else { - debugLog('[ClaudeIntegration:invokeClaudeAsync] WARNING: No token or configDir available for non-default profile'); - } + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'temp-file', escapedTempFile }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Executing command (temp file method, history-safe)'); + terminal.pty.write(command); + profileManager.markProfileUsed(activeProfile.id); + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (temp file) =========='); + return; + } else if (activeProfile.configDir) { + const escapedConfigDir = escapeShellArg(activeProfile.configDir); + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'config-dir', escapedConfigDir }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Executing command (configDir method, history-safe)'); + terminal.pty.write(command); + profileManager.markProfileUsed(activeProfile.id); + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (configDir) =========='); + return; + } else { + debugLog('[ClaudeIntegration:invokeClaudeAsync] WARNING: No token or configDir available for non-default profile'); } if (activeProfile && !activeProfile.isDefault) { @@ -866,6 +732,21 @@ export async function invokeClaudeAsync( }); throw error; // Re-throw to allow caller to handle } + + if (activeProfile && !activeProfile.isDefault) { + debugLog('[ClaudeIntegration:invokeClaudeAsync] Using terminal environment for non-default profile:', activeProfile.name); + } + + const command = buildClaudeShellCommand(cwdCommand, pathPrefix, escapedClaudeCmd, { method: 'default' }, extraFlags); + debugLog('[ClaudeIntegration:invokeClaudeAsync] Executing command (default method):', command); + terminal.pty.write(command); + + if (activeProfile) { + profileManager.markProfileUsed(activeProfile.id); + } + + finalizeClaudeInvoke(terminal, activeProfile, projectPath, startTime, getWindow, onSessionCapture); + debugLog('[ClaudeIntegration:invokeClaudeAsync] ========== INVOKE CLAUDE COMPLETE (default) =========='); } /** diff --git a/apps/frontend/src/renderer/components/terminal/WorktreeSelector.tsx b/apps/frontend/src/renderer/components/terminal/WorktreeSelector.tsx index 797a97ab48..493f69df0c 100644 --- a/apps/frontend/src/renderer/components/terminal/WorktreeSelector.tsx +++ b/apps/frontend/src/renderer/components/terminal/WorktreeSelector.tsx @@ -178,82 +178,83 @@ export function WorktreeSelector({
- ) : ( - <> - {/* Terminal Worktrees Section */} - {worktrees.length > 0 && ( - <> -
- {t('terminal:worktree.existing')} -
- {worktrees.map((wt) => ( - + ) : ( + <> + {/* Terminal Worktrees Section */} + {worktrees.length > 0 && ( + <> + +
+ {t('terminal:worktree.existing')} +
+ {worktrees.map((wt) => ( + { + e.stopPropagation(); + setIsOpen(false); + onSelectWorktree(wt); + }} + className="text-xs group" + > + +
+ {wt.name} + {wt.branchName && ( + + {wt.branchName} + + )} +
+ -
- ))} - - )} + + +
+ ))} + + )} - {/* Task Worktrees Section */} - {taskWorktrees.length > 0 && ( - <> - -
- {t('terminal:worktree.taskWorktrees')} -
- {taskWorktrees.map((wt) => ( - { - e.stopPropagation(); - setIsOpen(false); - selectTaskWorktree(wt); - }} - className="text-xs group" - > - -
- {wt.specName} - {wt.branch && ( - - {wt.branch} - - )} -
-
- ))} - - )} - - )} - + {/* Task Worktrees Section */} + {taskWorktrees.length > 0 && ( + <> + +
+ {t('terminal:worktree.taskWorktrees')} +
+ {taskWorktrees.map((wt) => ( + { + e.stopPropagation(); + setIsOpen(false); + selectTaskWorktree(wt); + }} + className="text-xs group" + > + +
+ {wt.specName} + {wt.branch && ( + + {wt.branch} + + )} +
+
+ ))} + + )} + + )} diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index bb2d0a2cfc..00b8996739 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -384,10 +384,5 @@ "conversionFailedDescription": "Failed to convert idea to task", "conversionError": "Conversion error", "conversionErrorDescription": "An error occurred while converting the idea" - }, - "issues": { - "loadingMore": "Loading more...", - "scrollForMore": "Scroll for more", - "allLoaded": "All issues loaded" } } diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index b704f75ec8..ce4a01ba20 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -384,10 +384,5 @@ "conversionFailedDescription": "Impossible de convertir l'idée en tâche", "conversionError": "Erreur de conversion", "conversionErrorDescription": "Une erreur s'est produite lors de la conversion de l'idée" - }, - "issues": { - "loadingMore": "Chargement...", - "scrollForMore": "Défiler pour plus", - "allLoaded": "Toutes les issues chargées" } } diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index b857794bfe..d2fc34b873 100644 --- a/tests/test_dependency_validator.py +++ b/tests/test_dependency_validator.py @@ -18,8 +18,11 @@ # Add apps/backend directory to path for imports sys.path.insert(0, str(Path(__file__).parent.parent / "apps" / "backend")) -from core.dependency_validator import validate_platform_dependencies, _exit_with_pywin32_error - +from core.dependency_validator import ( + _exit_with_pywin32_error, + _warn_missing_secretstorage, + validate_platform_dependencies, +) # ============================================================================= # TESTS FOR validate_platform_dependencies @@ -38,9 +41,11 @@ def test_windows_python_312_with_pywin32_missing_exits(self): """ import builtins - with patch("sys.platform", "win32"), \ + with patch("core.dependency_validator.is_windows", return_value=True), \ + patch("core.dependency_validator.is_linux", return_value=False), \ patch("sys.version_info", (3, 12, 0)), \ - patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: + patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit, \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning: # Mock pywintypes import to raise ImportError original_import = builtins.__import__ @@ -48,13 +53,16 @@ def test_windows_python_312_with_pywin32_missing_exits(self): def mock_import(name, *args, **kwargs): if name == "pywintypes": raise ImportError("No module named 'pywintypes'") + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") return original_import(name, *args, **kwargs) with patch("builtins.__import__", side_effect=mock_import): validate_platform_dependencies() - # Should have called the error exit function + # Should have called the error exit function (not warning) mock_exit.assert_called_once() + mock_warning.assert_not_called() def test_windows_python_312_with_pywin32_installed_continues(self): """Windows + Python 3.12+ with pywin32 installed should continue.""" @@ -67,108 +75,265 @@ def selective_mock(name, *args, **kwargs): """Return mock for pywintypes, delegate everything else to original.""" if name == "pywintypes": return MagicMock() + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") return original_import(name, *args, **kwargs) - with patch("sys.platform", "win32"), \ + with patch("core.dependency_validator.is_windows", return_value=True), \ + patch("core.dependency_validator.is_linux", return_value=False), \ patch("sys.version_info", (3, 12, 0)), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ patch("builtins.__import__", side_effect=selective_mock): # Should not raise SystemExit validate_platform_dependencies() + # Linux warning should not be called on Windows + mock_warning.assert_not_called() - def test_windows_python_311_validates_pywin32(self): - """ - Windows + Python 3.11 should validate pywin32 (ACS-306). - - Previously, validation only happened on Python 3.12+, but the MCP - library requires pywin32 on all Python versions on Windows. - """ - import builtins - - with patch("sys.platform", "win32"), \ + def test_windows_python_311_skips_validation(self): + """Windows + Python < 3.12 should skip pywin32 validation.""" + with patch("core.dependency_validator.is_windows", return_value=True), \ + patch("core.dependency_validator.is_linux", return_value=False), \ patch("sys.version_info", (3, 11, 0)), \ - patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ + patch("builtins.__import__") as mock_import: + # Even if pywintypes is not available, should not exit + mock_import.side_effect = ImportError("No module named 'pywintypes'") - original_import = builtins.__import__ + # Should not raise SystemExit + validate_platform_dependencies() + # Linux warning should not be called on Windows + mock_warning.assert_not_called() - def mock_import(name, *args, **kwargs): - if name == "pywintypes": - raise ImportError("No module named 'pywintypes'") - return original_import(name, *args, **kwargs) + def test_linux_skips_pywin32_validation(self): + """Linux should skip pywin32 validation but warn about secretstorage.""" + import builtins - with patch("builtins.__import__", side_effect=mock_import): - validate_platform_dependencies() + original_import = builtins.__import__ - # Should have called the error exit function (changed from skipping) - mock_exit.assert_called_once() + def mock_import(name, *args, **kwargs): + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") + return original_import(name, *args, **kwargs) - def test_linux_skips_validation(self): - """Non-Windows platforms should skip pywin32 validation.""" - with patch("sys.platform", "linux"), \ + with patch("core.dependency_validator.is_windows", return_value=False), \ + patch("core.dependency_validator.is_linux", return_value=True), \ patch("sys.version_info", (3, 12, 0)), \ - patch("builtins.__import__") as mock_import: - # Even if pywintypes is not available, should not exit - mock_import.side_effect = ImportError("No module named 'pywintypes'") - - # Should not raise SystemExit + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ + patch("builtins.__import__", side_effect=mock_import): + # Should not call pywin32 error, but should call secretstorage warning validate_platform_dependencies() + mock_warning.assert_called_once() - def test_macos_skips_validation(self): - """macOS should skip pywin32 validation.""" - with patch("sys.platform", "darwin"), \ + def test_macos_skips_pywin32_validation(self): + """macOS should skip pywin32 validation and secretstorage warning.""" + with patch("core.dependency_validator.is_windows", return_value=False), \ + patch("core.dependency_validator.is_linux", return_value=False), \ patch("sys.version_info", (3, 12, 0)), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ patch("builtins.__import__") as mock_import: # Even if pywintypes is not available, should not exit mock_import.side_effect = ImportError("No module named 'pywintypes'") # Should not raise SystemExit validate_platform_dependencies() + # Linux warning should not be called on macOS + mock_warning.assert_not_called() def test_windows_python_313_validates(self): """Windows + Python 3.13+ should validate pywin32.""" import builtins - with patch("sys.platform", "win32"), \ + with patch("core.dependency_validator.is_windows", return_value=True), \ + patch("core.dependency_validator.is_linux", return_value=False), \ patch("sys.version_info", (3, 13, 0)), \ - patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: + patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit, \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning: original_import = builtins.__import__ def mock_import(name, *args, **kwargs): if name == "pywintypes": raise ImportError("No module named 'pywintypes'") + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") return original_import(name, *args, **kwargs) with patch("builtins.__import__", side_effect=mock_import): validate_platform_dependencies() - # Should have called the error exit function + # Should have called the error exit function (not warning) mock_exit.assert_called_once() + mock_warning.assert_not_called() + + def test_windows_python_310_skips_validation(self): + """Windows + Python 3.10 should skip pywin32 validation.""" + with patch("core.dependency_validator.is_windows", return_value=True), \ + patch("core.dependency_validator.is_linux", return_value=False), \ + patch("sys.version_info", (3, 10, 0)), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ + patch("builtins.__import__") as mock_import: + mock_import.side_effect = ImportError("No module named 'pywintypes'") + + # Should not raise SystemExit + validate_platform_dependencies() + # Linux warning should not be called on Windows + mock_warning.assert_not_called() + - def test_windows_python_310_validates_pywin32(self): +# ============================================================================= +# TESTS FOR Linux secretstorage validation (ACS-310) +# ============================================================================= + + +class TestLinuxSecretstorageValidation: + """Tests for Linux secretstorage dependency validation (ACS-310).""" + + def test_linux_with_secretstorage_missing_warns(self): """ - Windows + Python 3.10 should validate pywin32 (ACS-306). + Linux without secretstorage should warn but not exit (ACS-310). - Previously, validation only happened on Python 3.12+, but the MCP - library requires pywin32 on all Python versions on Windows. + Unlike Windows pywin32 which is required, secretstorage is optional + and falls back to .env file storage. The warning informs users about + the security implications. """ import builtins - with patch("sys.platform", "win32"), \ - patch("sys.version_info", (3, 10, 0)), \ - patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: + with patch("core.dependency_validator.is_windows", return_value=False), \ + patch("core.dependency_validator.is_linux", return_value=True), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning: original_import = builtins.__import__ def mock_import(name, *args, **kwargs): - if name == "pywintypes": - raise ImportError("No module named 'pywintypes'") + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") return original_import(name, *args, **kwargs) with patch("builtins.__import__", side_effect=mock_import): validate_platform_dependencies() - # Should have called the error exit function (changed from skipping) - mock_exit.assert_called_once() + # Should have called the warning function + mock_warning.assert_called_once() + + def test_linux_with_secretstorage_installed_continues(self): + """Linux with secretstorage installed should continue without warning.""" + import builtins + + original_import = builtins.__import__ + + def selective_mock(name, *args, **kwargs): + """Return mock for secretstorage, delegate everything else to original.""" + if name == "secretstorage": + return MagicMock() + return original_import(name, *args, **kwargs) + + with patch("core.dependency_validator.is_windows", return_value=False), \ + patch("core.dependency_validator.is_linux", return_value=True), \ + patch("builtins.__import__", side_effect=selective_mock): + # Should not call warning function + validate_platform_dependencies() + + def test_windows_skips_secretstorage_validation(self): + """Windows should skip secretstorage validation.""" + import builtins + + original_import = builtins.__import__ + + def mock_import(name, *args, **kwargs): + # Allow pywintypes to succeed (Windows validation passes) + if name == "pywintypes": + return MagicMock() + # secretstorage import fails (but should be skipped on Windows) + if name == "secretstorage": + raise ImportError("No module named 'secretstorage'") + return original_import(name, *args, **kwargs) + + with patch("core.dependency_validator.is_linux", return_value=False), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ + patch("builtins.__import__", side_effect=mock_import): + # Should not call warning function + validate_platform_dependencies() + mock_warning.assert_not_called() + + def test_macos_skips_secretstorage_validation(self): + """macOS should skip secretstorage validation.""" + import builtins + + original_import = builtins.__import__ + + def mock_import(name, *args, **kwargs): + # All platform-specific imports fail (macOS has none required) + if name in ("pywintypes", "secretstorage"): + raise ImportError(f"No module named '{name}'") + return original_import(name, *args, **kwargs) + + with patch("core.dependency_validator.is_linux", return_value=False), \ + patch("core.dependency_validator.is_windows", return_value=False), \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ + patch("builtins.__import__", side_effect=mock_import): + # Should not call warning function + validate_platform_dependencies() + mock_warning.assert_not_called() + + +# ============================================================================= +# TESTS FOR _warn_missing_secretstorage (ACS-310) +# ============================================================================= + + +class TestExitWithSecretstorageWarning: + """Tests for _warn_missing_secretstorage function (ACS-310).""" + + def test_warning_message_contains_helpful_instructions(self, capsys): + """Warning message should include installation instructions.""" + _warn_missing_secretstorage() + + # Get stderr output + captured = capsys.readouterr() + message = captured.err + + # Verify helpful content + assert "secretstorage" in message.lower() + assert "pip install" in message.lower() + assert "linux" in message.lower() + assert "keyring" in message.lower() + + def test_warning_message_mentions_fallback_behavior(self, capsys): + """Warning should explain that app continues with .env fallback.""" + _warn_missing_secretstorage() + + captured = capsys.readouterr() + message = captured.err + + # Should mention the fallback behavior + assert ".env" in message + assert "continue" in message.lower() + + def test_warning_message_contains_venv_path(self, capsys): + """Warning message should include the virtual environment path.""" + with patch("sys.prefix", "/path/to/venv"): + _warn_missing_secretstorage() + + captured = capsys.readouterr() + message = captured.err + + # Should reference the full venv bin/activate path + expected_path = str(Path("/path/to/venv")) + assert expected_path in message or "/path/to/venv" in message + assert "bin" in message + + def test_warning_does_not_exit(self, capsys): + """Warning function should write to stderr but not exit.""" + # This function should NOT call sys.exit + with patch("sys.exit") as mock_exit: + _warn_missing_secretstorage() + + # Should NOT have called sys.exit + mock_exit.assert_not_called() + + # But should have written to stderr + captured = capsys.readouterr() + assert len(captured.err) > 0 # =============================================================================