From 3e7ed40b12e930367a150cc51ea7a2127010a0f8 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Tue, 13 Jan 2026 22:46:29 +0100 Subject: [PATCH 1/5] Version 2.7.4 (#1040) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ci: add Azure auth test workflow * fix(worktree): handle "already up to date" case correctly (ACS-226) (#961) * fix(worktree): handle "already up to date" case correctly (ACS-226) When git merge returns non-zero for "Already up to date", the merge code incorrectly treated this as a conflict and aborted. Now checks git output to distinguish between: - "Already up to date" - treat as success (nothing to merge) - Actual conflicts - abort as before - Other errors - show actual error message Also added comprehensive tests for edge cases: - Already up to date with no_commit=True - Already up to date with delete_after=True - Actual merge conflict detection - Merge conflict with no_commit=True * test: strengthen merge conflict abort verification Improve assertions in conflict detection tests to explicitly verify: - MERGE_HEAD does not exist after merge abort - git status returns clean (no staged/unstaged changes) This is more robust than just checking for absence of "CONFLICT" string, as git status --porcelain uses status codes, not literal words. * test: add git command success assertions and branch deletion verification - Add explicit returncode assertions for all subprocess.run git add/commit calls - Add branch deletion verification in test_merge_worktree_already_up_to_date_with_delete_after - Ensures tests fail early if git commands fail rather than continuing silently --------- Co-authored-by: StillKnotKnown * fix(terminal): add collision detection for terminal drag and drop reordering (#985) * fix(terminal): add collision detection for terminal drag and drop reordering Add closestCenter collision detection to DndContext to fix terminal drag and drop swapping not detecting valid drop targets. The default rectIntersection algorithm required too much overlap for grid layouts. Co-Authored-By: Claude Opus 4.5 * fix(terminal): handle file drops when closestCenter returns sortable ID Address PR review feedback: - Fix file drop handling to work when closestCenter collision detection returns the sortable ID instead of the droppable ID - Add terminals to useCallback dependency array to prevent stale state Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(ACS-181): enable auto-switch on 401 auth errors & OAuth-only profiles (#900) * fix(ACS-181): enable auto-switch for OAuth-only profiles Add OAuth token check at the start of isProfileAuthenticated() so that profiles with only an oauthToken (no configDir) are recognized as authenticated. This allows the profile scorer to consider OAuth-only profiles as valid alternatives for proactive auto-switching. Previously, isProfileAuthenticated() immediately returned false if configDir was missing, causing OAuth-only profiles to receive a -500 penalty in the scorer and never be selected for auto-switch. Fixes: ACS-181 Co-Authored-By: Claude Opus 4.5 Signed-off-by: Black Circle Sentinel * fix(ACS-181): detect 'out of extra usage' rate limit messages The previous patterns only matched "Limit reached · resets ..." but Claude Code also shows "You're out of extra usage · resets ..." which wasn't being detected. This prevented auto-switch from triggering. Added new patterns to both output-parser.ts (terminal) and rate-limit-detector.ts (agent processes) to detect: - "out of extra usage · resets ..." - "You're out of extra usage · resets ..." Co-Authored-By: Claude Opus 4.5 * fix(ACS-181): add real-time rate limit detection and debug logging - Add real-time rate limit detection in agent-process.ts processLog() so rate limits are detected immediately as output appears, not just when the process exits - Add clear warning message when auto-switch is disabled in settings - Add debug logging to profile-scorer.ts to trace profile evaluation - Add debug logging to rate-limit-detector.ts to trace pattern matching This enables immediate detection and auto-switch when rate limits occur during task execution. Co-Authored-By: Claude Opus 4.5 * fix(frontend): enable auto-switch on 401 auth errors - Propagate 401/403 errors from fetchUsageViaAPI to checkUsageAndSwap in UsageMonitor to trigger proactive profile swapping. - Fix usage monitor race condition by ensuring it waits for ClaudeProfileManager initialization. - Fix isProfileAuthenticated to correctly validate OAuth-only profiles. * fix(ACS-181): address PR review feedback - Revert unrelated files (rate-limit-detector, output-parser, agent-process) to upstream state - Gate profile-scorer logging behind DEBUG flag - Fix usage-monitor type safety and correct catch block syntax - Fix misleading indentation in index.ts app updater block * fix(frontend): enforce eslint compliance for logs in profile-scorer - Replace all console.log with console.warn (per linter rules) - Strictly gate all debug logs behind isDebug check to prevent production noise * fix(ACS-181): add swap loop protection for auth failures - Add authFailedProfiles Map to track profiles with recent auth failures - Implement 5-minute cooldown before retrying failed profiles - Exclude failed profiles from swap candidates to prevent infinite loops - Gate TRACE logs behind DEBUG flag to reduce production noise - Change console.log to console.warn for ESLint compliance --------- Signed-off-by: Black Circle Sentinel Co-authored-by: Claude Opus 4.5 * feat(frontend): add Claude Code version rollback feature (#983) * feat(frontend): add Claude Code version rollback feature Add ability for users to switch to any of the last 20 Claude Code CLI versions directly from the Claude Code popup in the sidebar. Changes: - Add IPC channels for fetching available versions and installing specific version - Add backend handlers to fetch versions from npm registry (with 1-hour cache) - Add version selector dropdown in ClaudeCodeStatusBadge component - Add warning dialog before switching versions (warns about closing sessions) - Add i18n support for English and French translations Co-Authored-By: Claude Opus 4.5 * fix: address PR review feedback for Claude Code version rollback - Add validation after semver filtering to handle empty version list - Add error state and UI feedback for installation/version switch failures - Extract magic number 5000ms to VERSION_RECHECK_DELAY_MS constant - Bind Select value prop to selectedVersion state - Normalize version comparison to handle 'v' prefix consistently - Use normalized version comparison in SelectItem disabled check Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(security): inherit security profiles in worktrees and validate shell -c commands (#971) * fix(security): inherit security profiles in worktrees and validate shell -c commands - Add inherited_from field to SecurityProfile to mark profiles copied from parent projects - Skip hash-based re-analysis for inherited profiles (fixes worktrees losing npm/npx etc.) - Add shell_validators.py to validate commands inside bash/sh/zsh -c strings - Register shell validators to close security bypass via bash -c "arbitrary_command" - Add 13 new tests for inherited profiles and shell -c validation Fixes worktree security config not being inherited, which caused agents to be blocked from running npm/npx commands in isolated workspaces. * docs: update README download links to v2.7.3 (#976) - Update all stable download links from 2.7.2 to 2.7.3 - Add Flatpak download link (new in 2.7.3) * fix(security): close shell -c bypass vectors and validate inherited profiles - Fix combined shell flags bypass (-xc, -ec, -ic) in _extract_c_argument() Shell allows combining flags like `bash -xc 'cmd'` which bypassed -c detection - Add recursive validation for nested shell invocations Prevents bypass via `bash -c "bash -c 'evil_cmd'"` - Validate inherited_from path in should_reanalyze() with defense-in-depth - Must exist and be a directory - Must be an ancestor of current project - Must contain valid security profile - Add comprehensive test coverage for all security fixes Co-Authored-By: Claude Opus 4.5 * style: fix import ordering in test_security.py Co-Authored-By: Claude Opus 4.5 * style: format shell_validators.py Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * feat(frontend): add searchable branch combobox to worktree creation dialog (#979) * feat(frontend): add searchable branch combobox to worktree creation dialog - Replace limited Select dropdown with searchable Combobox for branch selection - Add new Combobox UI component with search filtering and scroll support - Remove 15-branch limit - now shows all branches with search - Improve worktree name validation to allow dots and underscores - Better sanitization: spaces become hyphens, preserve valid characters - Add i18n keys for branch search UI in English and French Co-Authored-By: Claude Opus 4.5 * fix(frontend): address PR review feedback for worktree dialog - Extract sanitizeWorktreeName utility function to avoid duplication - Replace invalid chars with hyphens instead of removing them (feat/new → feat-new) - Trim trailing hyphens and dots from sanitized names - Add validation to forbid '..' in names (invalid for Git branch names) - Refactor branchOptions to use map/spread instead of forEach/push - Add ARIA accessibility: listboxId, aria-controls, role="listbox" Co-Authored-By: Claude Opus 4.5 * fix(frontend): align worktree name validation with backend regex - Fix frontend validation to match backend WORKTREE_NAME_REGEX (no dots, must end with alphanumeric) - Update sanitizeWorktreeName to exclude dots from allowed characters - Update i18n messages (en/fr) to remove mention of dots - Add displayName to Combobox component for React DevTools - Export Combobox from UI component index.ts - Add aria-label to Combobox listbox for accessibility Co-Authored-By: Claude Opus 4.5 * fix(frontend): address PR review accessibility and cleanup issues - Add forwardRef pattern to Combobox for consistency with other UI components - Add keyboard navigation (ArrowUp/Down, Enter, Escape, Home, End) - Add aria-activedescendant for screen reader focus tracking - Add unique option IDs for ARIA compliance - Add cleanup for async branch fetching to prevent state updates on unmounted component Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(frontend): sync worktree config to renderer on terminal restoration (#982) * fix(frontend): sync worktree config to renderer on terminal restoration When terminals are restored after app restart, the worktree config was not being synced to the renderer, causing the worktree label to not appear. This adds a new IPC channel to send worktree config during restoration and a listener in useTerminalEvents to update the terminal store. Co-Authored-By: Claude Opus 4.5 * fix(frontend): always sync worktreeConfig to handle deleted worktrees Addresses PR review feedback: send worktreeConfig IPC message unconditionally so the renderer can clear stale worktree labels when a worktree is deleted while the app is closed. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(merge): include files with content changes even when semantic analysis is empty (#986) * fix(merge): include files with content changes even when semantic analysis is empty The merge system was discarding files that had real code changes but no detected semantic changes. This happened because: 1. The semantic analyzer only detects imports and function additions/removals 2. Files with only function body modifications returned semantic_changes=[] 3. The filter used Python truthiness (empty list = False), excluding these files 4. This caused merges to fail with "0 files to merge" despite real changes The fix uses content hash comparison as a fallback check. If the file content actually changed (hash_before != hash_after), include it for merge regardless of whether the semantic analyzer could parse the specific change types. This fixes merging for: - Files with function body modifications (most common case) - Unsupported file types (Rust, Go, etc.) where semantic analysis returns empty - Any file where the analyzer fails to detect the specific change pattern Co-Authored-By: Claude Opus 4.5 * refactor(merge): add TaskSnapshot.has_modifications property and handle DIRECT_COPY Address PR review feedback: 1. DRY improvement: Add `has_modifications` property to TaskSnapshot - Centralizes the modification detection logic - Checks semantic_changes first, falls back to content hash comparison - Handles both complete tasks and in-progress tasks safely 2. Fix for files with empty semantic_changes (Cursor issue #2): - Add DIRECT_COPY MergeDecision for files that were modified but couldn't be semantically analyzed (body changes, unsupported languages) - MergePipeline returns DIRECT_COPY when has_modifications=True but semantic_changes=[] (single task case) - Orchestrator handles DIRECT_COPY by reading file directly from worktree - This prevents silent data loss where apply_single_task_changes would return baseline content unchanged 3. Update _update_stats to count DIRECT_COPY as auto-merged The combination ensures: - Files ARE detected for merge (has_modifications check) - Files ARE properly merged (DIRECT_COPY reads from worktree) - No silent data loss (worktree content used instead of baseline) Co-Authored-By: Claude Opus 4.5 * fix(merge): handle DIRECT_COPY in merge_tasks() and log missing files - Add DIRECT_COPY handling to merge_tasks() for multi-task merges (was only handled in merge_task() for single-task merges) - Add warning logging when worktree file doesn't exist during DIRECT_COPY in both merge_task() and merge_tasks() Co-Authored-By: Claude Opus 4.5 * fix(merge): remove unnecessary f-string prefixes Co-Authored-By: Claude Opus 4.5 * fix(merge): properly fail DIRECT_COPY when worktree file missing - Extract _read_worktree_file_for_direct_copy() helper to DRY up logic - Set decision to FAILED when worktree file not found (was silent success) - Add warning when worktree_path is None in merge_tasks - Use `is not None` check for merged_content to allow empty files - Fix has_modifications for new files with empty hash_before - Add debug_error() to merge_tasks exception handling for consistency Co-Authored-By: Claude Opus 4.5 * style(merge): fix ruff formatting for long line Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(terminal): detect Claude exit and reset label when user closes Claude (#990) * fix(terminal): detect Claude exit and reset label when user closes Claude Previously, the "Claude" label on terminals would persist even after the user closed Claude (via /exit, Ctrl+D, etc.) because the system only reset isClaudeMode when the entire terminal process exited. This change adds robust Claude exit detection by: - Adding shell prompt patterns to detect when Claude exits and returns to shell (output-parser.ts) - Adding new IPC channel TERMINAL_CLAUDE_EXIT for exit notifications - Adding handleClaudeExit() to reset terminal state in main process - Adding onClaudeExit callback in terminal event handler - Adding onTerminalClaudeExit listener in preload API - Handling exit event in renderer to update terminal store Now when a user closes Claude within a terminal, the label is removed immediately while the terminal continues running. Co-Authored-By: Claude Opus 4.5 * fix(terminal): add line-start anchors to exit detection regex patterns Address PR review findings: - Add ^ anchors to CLAUDE_EXIT_PATTERNS to prevent false positive exit detection when Claude outputs paths, array access, or Unicode arrows - Add comprehensive unit tests for detectClaudeExit and related functions - Remove duplicate debugLog call in handleClaudeExit (keep console.warn) Co-Authored-By: Claude Opus 4.5 * fix(terminal): prevent false exit detection for emails and race condition - Update user@host regex to require path indicator after colon, preventing emails like user@example.com: from triggering exit detection - Add test cases for emails at line start to ensure they don't match - Add guard in onTerminalClaudeExit to prevent setting status to 'running' if terminal has already exited (fixes potential race condition) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(app-update): persist downloaded update state for Install button visibility (#992) * fix(app-update): persist downloaded update state for Install button visibility When updates auto-download in background, users miss the update-downloaded event if not on Settings page. This causes "Install and Restart" button to never appear. Changes: - Add downloadedUpdateInfo state in app-updater.ts to persist downloaded info - Add APP_UPDATE_GET_DOWNLOADED IPC handler to query downloaded state - Add getDownloadedAppUpdate API method in preload - Update AdvancedSettings to check for already-downloaded updates on mount Now when user opens Settings after background download, the component queries persisted state and shows "Install and Restart" correctly. Co-Authored-By: Claude Opus 4.5 * fix(app-update): resolve race condition and type safety issues - Fix race condition where checkForAppUpdates() could overwrite downloaded update info with null, causing 'Unknown' version display - Add proper type guard for releaseNotes (can be string | array | null) instead of unsafe type assertion Co-Authored-By: Claude Opus 4.5 * fix(app-update): clear downloaded update state on channel change and add useEffect cleanup - Clear downloadedUpdateInfo when update channel changes to prevent showing Install button for updates from a different channel (e.g., beta update showing after switching to stable channel) - Add isCancelled flag to useEffect async operations in AdvancedSettings to prevent React state updates on unmounted components Addresses CodeRabbit review findings. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(backend): add Sentry integration and fix broken pipe errors (#991) * fix(backend): add Sentry integration and fix broken pipe errors - Add sentry-sdk to Python backend for error tracking - Create safe_print() utility to handle BrokenPipeError gracefully - Initialize Sentry in CLI, GitHub runner, and spec runner entry points - Use same SENTRY_DSN environment variable as Electron frontend - Apply privacy path masking (usernames removed from stack traces) Fixes "Review Failed: [Errno 32] Broken pipe" error in PR review Co-Authored-By: Claude Opus 4.5 * fix(backend): address PR review findings for Sentry integration - Fix ruff linting errors (unused imports, import sorting) - Add path masking to set_context() and set_tag() for privacy - Add defensive path masking to capture_exception() kwargs - Add debug logging for bare except clauses in sentry.py - Add top-level error handler in cli/main.py with Sentry capture - Add error handling with Sentry capture in spec_runner.py - Move safe_print to core/io_utils.py for broader reuse - Migrate GitLab runner files to use safe_print() - Add fallback import pattern in sdk_utils.py Co-Authored-By: Claude Opus 4.5 * style: apply ruff formatting Co-Authored-By: Claude Opus 4.5 * fix(backend): address CodeRabbit review findings for Sentry and io_utils - Add path masking to capture_message() kwargs for privacy consistency - Add recursion depth limit (50) to _mask_object_paths() to prevent stack overflow - Add WSL path masking support (/mnt/[a-z]/Users/...) - Add consistent ImportError debug logging across Sentry wrapper functions - Add ValueError handling in safe_print() for closed stdout scenarios - Improve reset_pipe_state() documentation with usage warnings Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix: improve Claude CLI detection and add installation selector (#1004) * fix: improve Claude CLI detection and add installation selector This PR addresses the "Claude Code not found" error when starting tasks by improving CLI path detection across all platforms. Backend changes: - Add cross-platform `find_claude_cli()` function in `client.py` that checks: - CLAUDE_CLI_PATH environment variable for user override - System PATH via shutil.which() - Homebrew paths on macOS - NVM paths for Node.js version manager installations - Platform-specific standard locations (Windows: AppData, Program Files; Unix: .local/bin) - Pass detected `cli_path` to ClaudeAgentOptions in both `create_client()` and `create_simple_client()` - Improve Windows .cmd/.bat file execution using proper cmd.exe flags (/d, /s, /c) and correct quoting for paths with spaces Frontend changes: - Add IPC handlers for scanning all Claude CLI installations and switching active path - Update ClaudeCodeStatusBadge to show current CLI path and allow selection when multiple installations are detected - Add `writeSettingsFile()` to settings-utils for persisting CLI path selection - Add translation keys for new UI elements (English and French) Closes #1001 * fix: address PR review findings for Claude CLI detection Addresses all 8 findings from Auto Claude PR Review: Security improvements: - Add path sanitization (_is_secure_path) to backend CLI validation to prevent command injection via malicious paths - Add isSecurePath validation in frontend IPC handler before CLI execution - Normalize paths with path.resolve() before execution Architecture improvements: - Refactor scanClaudeInstallations to use getClaudeDetectionPaths() from cli-tool-manager.ts as single source of truth (addresses code duplication) - Add cross-reference comments between backend _get_claude_detection_paths() and frontend getClaudeDetectionPaths() to keep them in sync Bug fixes: - Fix path display truncation to use regex /[/\\]/ for cross-platform compatibility (Windows uses backslashes) - Add null check for version in UI rendering (shows "version unknown" instead of "vnull") - Use DEFAULT_APP_SETTINGS merge pattern for settings persistence Debugging improvements: - Add error logging in validateClaudeCliAsync catch block for better debugging of CLI detection issues Translation additions: - Add "versionUnknown" key to English and French navigation.json * ci(release): move VirusTotal scan to separate post-release workflow (#980) * ci(release): move VirusTotal scan to separate post-release workflow VirusTotal scans were blocking release creation, taking 5+ minutes per file. This change moves the scan to a separate workflow that triggers after the release is published, allowing releases to be available immediately. - Create virustotal-scan.yml workflow triggered on release:published - Remove blocking VirusTotal step from release.yml - Scan results are appended to release notes after completion - Add manual trigger option for rescanning old releases * fix(ci): address PR review issues in VirusTotal scan workflow - Add error checking on gh release view to prevent wiping release notes - Replace || true with proper error handling to distinguish "no assets" from real errors - Use file-based approach for release notes to avoid shell expansion issues - Use env var pattern consistently for secret handling - Remove placeholder text before appending VT results - Document 32MB threshold with named constant - Add HTTP status code validation on all curl requests Co-Authored-By: Claude Opus 4.5 * fix(ci): add concurrency control and remove dead code in VirusTotal workflow - Add concurrency group to prevent TOCTOU race condition when multiple workflow_dispatch runs target the same release tag - Remove unused analysis_failed variable declaration Co-Authored-By: Claude Opus 4.5 * fix(ci): improve error handling in VirusTotal workflow - Fail workflow when download errors occur but scannable assets exist - Add explicit timeout handling for analysis polling loop - Use portable sed approach (works on both GNU and BSD sed) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(ui): display actual base branch name instead of hardcoded main (#969) * fix(ui): display actual base branch name instead of hardcoded "main" The merge conflict UI was showing "Main branch has X new commits" regardless of the actual base branch. Now it correctly displays the dynamic branch name (e.g., "develop branch has 40 new commits") using the baseBranch value from gitConflicts. * docs: update README download links to v2.7.3 (#976) - Update all stable download links from 2.7.2 to 2.7.3 - Add Flatpak download link (new in 2.7.3) * fix(i18n): add translation keys for branch divergence messages - Add merge section to taskReview.json with pluralized translations - Update WorkspaceStatus.tsx to use i18n for branch behind message - Update MergePreviewSummary.tsx to use i18n for branch divergence text - Add French translations for all new keys Co-Authored-By: Claude Opus 4.5 * fix(i18n): add missing translation keys for branch behind details - Add branchHasNewCommitsSinceBuild for build started message - Add filesNeedAIMergeDueToRenames for path-mapped files - Add fileRenamesDetected for rename detection message - Add filesRenamedOrMoved for generic rename/move message - Update WorkspaceStatus.tsx to use all new i18n keys Co-Authored-By: Claude Opus 4.5 * fix(i18n): correct pluralization for rename count in AI merge message The filesNeedAIMergeDueToRenames translation has two values that need independent pluralization (fileCount and renameCount). Since i18next only supports one count parameter, added separate translation keys for singular/plural renames and select the correct key based on renameCount value. Co-Authored-By: Claude Opus 4.5 * fix(i18n): use translation keys for merge button labels with dynamic branch Replace hardcoded 'Stage to Main' and 'Merge to Main' button labels with i18n translation keys that interpolate the actual target branch name. Also adds translations for loading states (Resolving, Staging, Merging). Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * fix(github-prs): prevent preloading of PRs currently under review (#1006) - Updated logic to skip PRs that are currently being reviewed when determining which PRs need preloading. - Enhanced condition to only fetch existing review data from disk if no review is in progress, ensuring that ongoing reviews are not overwritten by stale data. * chore: bump version to 2.7.4 * hotfix/sentry-backend-build * fix(github): resolve circular import issues in context_gatherer and services (#1026) - Updated import statements in context_gatherer.py to import safe_print from core.io_utils to avoid circular dependencies with the services package. - Introduced lazy imports in services/__init__.py to prevent circular import issues, detailing the import chain in comments for clarity. - Added a lazy import handler to load classes on first access, improving module loading efficiency. * feat(sentry): embed Sentry DSN at build time for packaged apps (#1025) * feat(sentry): integrate Sentry configuration into Electron build - Added build-time constants for Sentry DSN and sampling rates in electron.vite.config.ts. - Enhanced environment variable handling in env-utils.ts to include Sentry settings for subprocesses. - Implemented getSentryEnvForSubprocess function in sentry.ts to provide Sentry environment variables for Python backends. - Updated Sentry-related functions to prioritize build-time constants over runtime environment variables for improved reliability. This integration ensures that Sentry is properly configured for both local development and CI environments. * fix(sentry): add typeof guards for build-time constants in tests The __SENTRY_*__ constants are only defined when Vite's define plugin runs during build. In test environments (vitest), these constants are undefined and cause ReferenceError. Added typeof guards to safely handle both cases. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * Fix Duplicate Kanban Task Creation on Rapid Button Clicks (#1021) * auto-claude: subtask-1-1 - Add convertingIdeas state and guard logic to useIdeation hook * auto-claude: subtask-1-2 - Update IdeaDetailPanel to accept isConverting prop * auto-claude: subtask-2-1 - Add idempotency check for linked_task_id in task-c * auto-claude: subtask-3-1 - Manual testing: Verify rapid clicking creates only one task - Fixed missing convertingIdeas prop connection in Ideation.tsx - Added convertingIdeas to destructured hook values - Added isConverting prop to IdeaDetailPanel component - Created detailed manual-test-report.md with code review and E2E testing instructions - All code implementation verified via TypeScript checks (no errors) - Multi-layer protection confirmed: UI disabled, guard check, backend idempotency - Manual E2E testing required for final verification Co-Authored-By: Claude Sonnet 4.5 * fix: address PR review findings for duplicate task prevention - Fix TOCTOU race condition by moving idempotency check inside lock - Fix React state closure by using ref for synchronous tracking - Add i18n translations for ideation UI (EN + FR) - Add error handling with toast notifications for conversion failures Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Sonnet 4.5 * feat(terminal): add YOLO mode to invoke Claude with --dangerously-skip-permissions (#1016) * feat(terminal): add YOLO mode to invoke Claude with --dangerously-skip-permissions Add a toggle in Developer Tools settings that enables "YOLO Mode" which starts Claude with the --dangerously-skip-permissions flag, bypassing all safety prompts. Changes: - Add dangerouslySkipPermissions setting to AppSettings interface - Add translation keys for YOLO mode (en/fr) - Modify claude-integration-handler to accept and append extra flags - Update terminal-manager and terminal-handlers to read and forward the setting - Add Switch toggle with warning styling in DevToolsSettings UI The toggle includes visual warnings (amber colors, AlertTriangle icon) to clearly indicate this is a dangerous option that bypasses Claude's permission system. Co-Authored-By: Claude Opus 4.5 * fix(terminal): address PR review issues for YOLO mode implementation - Add async readSettingsFileAsync to avoid blocking main process during settings read - Extract YOLO_MODE_FLAG constant to eliminate duplicate flag strings - Store dangerouslySkipPermissions on terminal object to persist YOLO mode across profile switches - Update switchClaudeProfile callback to pass stored YOLO mode setting These fixes address: - LOW: Synchronous file I/O in IPC handler - LOW: Flag string duplicated in invokeClaude and invokeClaudeAsync - MEDIUM: YOLO mode not persisting when switching Claude profiles Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 * Make worktree isolation prominent in UI (#1020) * auto-claude: subtask-1-1 - Add i18n translation keys for worktree notice banner and merge tooltip - Added wizard.worktreeNotice.title and wizard.worktreeNotice.description for task creation banner - Added review.mergeTooltip for merge button explanation - Translations added to both en/tasks.json and fr/tasks.json * auto-claude: subtask-1-2 - Add visible info banner to TaskCreationWizard expl * auto-claude: subtask-1-3 - Add tooltip to 'Merge with AI' button in WorkspaceStatus - Import Tooltip components from ui/tooltip - Wrap merge button with Tooltip, TooltipTrigger, TooltipContent - Add contextual tooltip text explaining merge operation: * With AI: explains worktree merge, removal, and AI conflict resolution * Without AI: explains worktree merge and removal - Follows Radix UI tooltip pattern from reference file * fix: use i18n key for merge button tooltip in WorkspaceStatus * fix: clarify merge tooltip - worktree removal is optional (qa-requested) Fixes misleading tooltip text that implied worktree is automatically removed during merge. In reality, after merge, users are shown a dialog where they can choose to keep or remove the worktree. Updated tooltip to reflect this flow. Changes: - Updated en/tasks.json: Changed tooltip to clarify worktree removal is optional - Updated fr/tasks.json: Updated French translation to match QA Feedback: "Its currently saying on the tooltip that it will 'remove the worktree' Please validate if this is the actual logic. As per my understanding, there will be an extra button afterwards that will make sure that the user has access to the work tree if they want to revert anything. The user has to manually accept to remove the work tree." Co-Authored-By: Claude Sonnet 4.5 * fix: use theme-aware colors for worktree info banner Replace hardcoded blue colors with semantic theme classes to support dark mode properly. Uses the same pattern as other info banners in the codebase (bg-info/10, border-info/30, text-info). Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Sonnet 4.5 * fix(terminal): improve worktree name input UX (#1012) * fix(terminal): improve worktree name input to not strip trailing characters while typing - Allow trailing hyphens/underscores during input (only trim on submit) - Add preview name that shows the final sanitized value for branch preview - Remove invalid characters instead of replacing with hyphens - Collapse consecutive underscores in addition to hyphens - Final sanitization happens on submit to match backend WORKTREE_NAME_REGEX Co-Authored-By: Claude Opus 4.5 * fix(terminal): address PR review findings for worktree name validation - Fix submit button disabled check to use sanitized name instead of raw input - Simplify trailing trim logic (apply once after all transformations) - Apply lowercase in handleNameChange to reduce input/preview gap - Internationalize 'name' fallback using existing translation key Co-Authored-By: Claude Opus 4.5 * fix(terminal): improve header responsiveness for multiple terminals - Hide text labels (Claude, Open in IDE) when ≥4 terminals, show icon only - Add dynamic max-width to worktree name badge with truncation - Add tooltips to all icon-only elements for accessibility - Maintain full functionality while reducing header width requirements Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 Co-authored-by: Test User * fix(terminal): enhance terminal recreation logic with retry mechanism (#1013) * fix(terminal): enhance terminal recreation logic with retry mechanism - Introduced a maximum retry limit and delay for terminal recreation when dimensions are not ready. - Added cleanup for retry timers on component unmount to prevent memory leaks. - Improved error handling to report failures after exceeding retry attempts, ensuring better user feedback during terminal setup. * fix(terminal): address PR review feedback for retry mechanism - Fix race condition: clear pending retry timer at START of effect to prevent multiple timers when dependencies change mid-retry - Fix isCreatingRef: keep it true during retry window to prevent duplicate creation attempts from concurrent effect runs - Extract duplicated retry logic into scheduleRetryOrFail helper (consolidated 5 duplicate instances into 1 reusable function) - Add handleSuccess/handleError helpers to reduce code duplication - Reduce file from 295 to 237 lines (~20% reduction) Addresses review feedback from CodeRabbit, Gemini, and Auto Claude. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Test User Co-authored-by: Claude Opus 4.5 * feat(terminal): add task worktrees section and remove terminal limit (#1033) * feat(terminal): add task worktrees section and remove terminal limit - Remove 12 terminal worktree limit (now unlimited) - Add "Task Worktrees" section in worktree dropdown below terminal worktrees - Task worktrees (created by kanban) now accessible for manual work - Update translations for new section labels (EN + FR) Co-Authored-By: Claude Opus 4.5 * fix(terminal): address PR review feedback - Clear taskWorktrees state when project is null or changes - Parallelize API calls with Promise.all for better performance - Use consistent path-based filtering for both worktree types - Add clarifying comment for createdAt timestamp Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 Co-authored-by: Test User * Add file/screenshot upload to QA feedback interface (#1018) * auto-claude: subtask-1-1 - Add feedbackImages state and handlers to useTaskDetail - Add feedbackImages state as ImageAttachment[] for storing feedback images - Add setFeedbackImages setter for direct state updates - Add addFeedbackImage handler for adding a single image - Add addFeedbackImages handler for adding multiple images at once - Add removeFeedbackImage handler for removing an image by ID - Add clearFeedbackImages handler for clearing all images - Import ImageAttachment type from shared/types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-1-2 - Update IPC interface to support images in submitReview - Add ImageAttachment import from ./task types - Update submitReview signature to include optional images parameter 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-1-3 - Update submitReview function in task-store to accept and pass images * auto-claude: subtask-2-1 - Add paste/drop handlers and image thumbnail displa - Add paste event handler for screenshot/image clipboard support - Add drag-over and drag-leave handlers for visual feedback during drag - Add drop handler for image file drops - Add image thumbnail display (64x64) with remove button on hover - Import image utilities from ImageUpload.tsx (generateImageId, blobToBase64, etc.) - Add i18n support for all new UI text - Make new props optional for backward compatibility during incremental rollout - Allow submission with either text feedback or images (not both required) - Add visual drag feedback with border/background color change * auto-claude: subtask-2-2 - Update TaskReview to pass image props to QAFeedbackSection * auto-claude: subtask-2-3 - Update TaskDetailModal to manage image state and pass to TaskReview - Pass feedbackImages and setFeedbackImages from useTaskDetail hook to TaskReview - Update handleReject to include images in submitReview call - Allow submission with images only (no text required) - Clear images after successful submission * auto-claude: subtask-3-1 - Add English translations for feedback image UI * auto-claude: subtask-3-2 - Add French translations for feedback image UI * fix(security): sanitize image filename to prevent path traversal - Use path.basename() to strip directory components from filenames - Validate sanitized filename is not empty, '.', or '..' - Add defense-in-depth check verifying resolved path stays within target directory - Fix base64 data URL regex to handle complex MIME types (e.g., svg+xml) Co-Authored-By: Claude Opus 4.5 * fix: add MIME type validation and fix SVG file extension - Add server-side MIME type validation for image uploads (defense in depth) - Fix SVG file extension: map 'image/svg+xml' to '.svg' instead of '.svg+xml' - Add MIME-to-extension mapping for all allowed image types Co-Authored-By: Claude Opus 4.5 * fix: require mimeType and apply SVG extension fix to drop handler - Change MIME validation to reject missing mimeType (prevents bypass) - Add 'image/jpg' to server-side allowlist for consistency - Apply mimeToExtension mapping to drop handler (was only in paste handler) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 Co-authored-by: Test User * fix(auth): await profile manager initialization before auth check (#1010) * fix(auth): await profile manager initialization before auth check Fixes race condition where hasValidAuth() was called before the ClaudeProfileManager finished async initialization from disk. The getClaudeProfileManager() returns a singleton immediately with default profile data (no OAuth token). When hasValidAuth() runs before initialization completes, it returns false even when valid credentials exist. Changed all pre-flight auth checks to use await initializeClaudeProfileManager() which ensures initialization completes via promise caching. Signed-off-by: StillKnotKnown * fix(auth): add error handling for profile manager initialization Prevents unhandled promise rejections when initializeClaudeProfileManager() throws due to filesystem errors (permissions, disk full, corrupt JSON). The ipcMain.on handler for TASK_START doesn't await promises, so unhandled rejections could crash the main process. Wrapped all await initializeClaudeProfileManager() calls in try-catch blocks. Found via automated code review. Signed-off-by: StillKnotKnown * test: mock initializeClaudeProfileManager in subprocess tests The test mock was only mocking getClaudeProfileManager, but now we also use initializeClaudeProfileManager which wasn't mocked, causing test failures. Signed-off-by: StillKnotKnown * fix(auth): add try-catch for initializeClaudeProfileManager in remaining handlers Addresses PR review feedback - TASK_UPDATE_STATUS and TASK_RECOVER_STUCK handlers were missing try-catch blocks for initializeClaudeProfileManager(), inconsistent with TASK_START handler. If initialization fails, users now get specific file permissions guidance instead of generic error messages. Signed-off-by: StillKnotKnown * refactor(auth): extract profile manager initialization into helper Extract the repeated initializeClaudeProfileManager() + try/catch pattern into a helper function ensureProfileManagerInitialized() that returns a discriminated union for type-safe error handling. This reduces code duplication across TASK_START, TASK_UPDATE_STATUS, and TASK_RECOVER_STUCK handlers while preserving context-specific error handling behavior. The helper returns: - { success: true, profileManager } on success - { success: false, error } on failure Signed-off-by: StillKnotKnown * fix(auth): improve error details and allow retry after transient failures Two improvements to profile manager initialization: 1. Include actual error details in failure response for better debugging. Previously, only a generic message was returned to users, making it hard to diagnose the root cause. Now the error message is appended. 2. Reset cached promise on failure to allow retries after transient errors. Previously, if initialize() failed (e.g., EACCES, ENOSPC), the rejected promise was cached forever, requiring app restart to recover. Now the cached promise is reset on failure, allowing subsequent calls to retry. Signed-off-by: StillKnotKnown --------- Signed-off-by: StillKnotKnown Co-authored-by: StillKnotKnown Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> * fix(frontend): validate Windows claude.cmd reliably in GUI (#1023) * fix: use absolute cmd.exe for Claude CLI validation * fix: make cmd.exe validation type-safe for tests * fix: satisfy frontend typecheck for cli tool tests Signed-off-by: Umaru * test: mock windows-paths exports for isSecurePath Signed-off-by: Umaru * test: make cli env tests platform-aware Signed-off-by: Umaru * test: cover isSecurePath guard in claude detection Signed-off-by: Umaru * test: align env-utils mocks with shouldUseShell Signed-off-by: Umaru * test: assert isSecurePath for cmd path * fix(frontend): handle quoted claude.cmd paths in validation --------- Signed-off-by: Umaru Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> * 2.7.4 release * changelog 2.7.4 --------- Signed-off-by: Black Circle Sentinel Signed-off-by: StillKnotKnown Signed-off-by: Umaru Co-authored-by: StillKnotKnown <192589389+StillKnotKnown@users.noreply.github.com> Co-authored-by: StillKnotKnown Co-authored-by: Claude Opus 4.5 Co-authored-by: Michael Ludlow Co-authored-by: Test User Co-authored-by: Umaru --- apps/backend/core/client.py | 97 +++++- apps/backend/core/simple_client.py | 5 +- apps/backend/runners/github/orchestrator.py | 104 ------ .../parallel_orchestrator_reviewer.py | 31 +- .../github/services/pr_review_engine.py | 1 - .../runners/github/services/sdk_utils.py | 47 ++- apps/backend/security/shell_validators.py | 19 +- apps/frontend/src/main/cli-tool-manager.ts | 6 +- apps/frontend/src/main/env-utils.ts | 1 - .../main/ipc-handlers/claude-code-handlers.ts | 13 +- .../claude-integration-handler.test.ts | 250 +++++++++++++- .../terminal/claude-integration-handler.ts | 311 ++++++------------ .../components/terminal/WorktreeSelector.tsx | 143 ++++---- .../src/shared/i18n/locales/en/common.json | 5 - .../src/shared/i18n/locales/fr/common.json | 5 - 15 files changed, 529 insertions(+), 509 deletions(-) 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/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/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/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" } } From 6332566bf089709cb43825613a485ce179079b4a Mon Sep 17 00:00:00 2001 From: StillKnotKnown Date: Fri, 16 Jan 2026 20:36:06 +0200 Subject: [PATCH 2/5] fix(linux): add secretstorage to platform-critical packages (ACS-310) Linux binary installations were missing the `secretstorage` package, causing OAuth token storage via Freedesktop.org Secret Service to fail silently. The build script cache was created before secretstorage was added to requirements.txt (Jan 16, 2026), so the package was not being validated during bundling. Changes: - Add platform-aware critical packages validation in download-python.cjs - Add platform-aware critical packages validation in python-env-manager.ts - Add Linux secretstorage warning in dependency_validator.py - Add comprehensive tests for Linux secretstorage validation The fix follows the same pattern as the Windows pywin32 fix (ACS-306): - Platform-specific packages are only validated on their target platform - Linux: secretstorage (OAuth token storage via keyring) - Windows: pywintypes (MCP library dependency) - macOS: No platform-specific packages The Linux validation emits a warning (not blocking error) since the app gracefully falls back to .env file storage when secretstorage is unavailable. Refs: ACS-310 --- apps/backend/core/dependency_validator.py | 50 +++- apps/frontend/scripts/download-python.cjs | 24 +- apps/frontend/src/main/python-env-manager.ts | 23 +- tests/test_dependency_validator.py | 243 +++++++++++++++---- 4 files changed, 267 insertions(+), 73 deletions(-) diff --git a/apps/backend/core/dependency_validator.py b/apps/backend/core/dependency_validator.py index fdad64a759..761e2cfa41 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: + _exit_with_secretstorage_warning() + def _exit_with_pywin32_error() -> None: """Exit with helpful error message for missing pywin32.""" @@ -76,3 +85,38 @@ def _exit_with_pywin32_error() -> None: "\n" f"Current Python: {sys.executable}\n" ) + + +def _exit_with_secretstorage_warning() -> None: + """Exit with helpful 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" + ) + # Continue execution - this is a warning, not a blocking error diff --git a/apps/frontend/scripts/download-python.cjs b/apps/frontend/scripts/download-python.cjs index e626f3e753..fa4443771a 100644 --- a/apps/frontend/scripts/download-python.cjs +++ b/apps/frontend/scripts/download-python.cjs @@ -710,17 +710,19 @@ 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'); // 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(moduleFile); }); if (missingPackages.length > 0) { @@ -819,17 +821,19 @@ 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'); // 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(moduleFile); }); if (postInstallMissing.length > 0) { diff --git a/apps/frontend/src/main/python-env-manager.ts b/apps/frontend/src/main/python-env-manager.ts index 510dbf566c..88564f5503 100644 --- a/apps/frontend/src/main/python-env-manager.ts +++ b/apps/frontend/src/main/python-env-manager.ts @@ -128,21 +128,26 @@ 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', + ...(platformCriticalPackages[process.platform] || []) + ]; + + // 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(moduleFile); }); // Log missing packages for debugging diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index b857794bfe..a71b141604 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, + _exit_with_secretstorage_warning, + 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._exit_with_secretstorage_warning") 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,241 @@ 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._exit_with_secretstorage_warning") 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._exit_with_secretstorage_warning") 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._exit_with_secretstorage_warning") 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._exit_with_secretstorage_warning") 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._exit_with_secretstorage_warning") 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_validates_pywin32(self): + 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._exit_with_secretstorage_warning") 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() + + +# ============================================================================= +# 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_linux", return_value=True), \ + patch("core.dependency_validator._exit_with_secretstorage_warning") 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_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.""" + with patch("core.dependency_validator.is_linux", return_value=False), \ + patch("builtins.__import__") as mock_import: + # Even if secretstorage is not available, should not warn + mock_import.side_effect = ImportError("No module named 'secretstorage'") + + # Should not call warning function + validate_platform_dependencies() + + def test_macos_skips_secretstorage_validation(self): + """macOS should skip secretstorage validation.""" + with patch("core.dependency_validator.is_linux", return_value=False), \ + patch("builtins.__import__") as mock_import: + # Even if secretstorage is not available, should not warn + mock_import.side_effect = ImportError("No module named 'secretstorage'") + + # Should not call warning function + validate_platform_dependencies() + + +# ============================================================================= +# TESTS FOR _exit_with_secretstorage_warning (ACS-310) +# ============================================================================= + + +class TestExitWithSecretstorageWarning: + """Tests for _exit_with_secretstorage_warning function (ACS-310).""" + + def test_warning_message_contains_helpful_instructions(self, capsys): + """Warning message should include installation instructions.""" + _exit_with_secretstorage_warning() + + # 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.""" + _exit_with_secretstorage_warning() + + 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"): + _exit_with_secretstorage_warning() + + 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: + _exit_with_secretstorage_warning() + + # 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 # ============================================================================= From 2d7a0b7058a33061f8cfa2c20296cc8d5532eaab Mon Sep 17 00:00:00 2001 From: StillKnotKnown Date: Fri, 16 Jan 2026 20:43:01 +0200 Subject: [PATCH 3/5] fix(tests): mock pywintypes import in Windows/macOS secretstorage tests The tests test_windows_skips_secretstorage_validation and test_macos_skips_secretstorage_validation were failing on Windows CI because they didn't mock the pywintypes import. When running on actual Windows in CI, the pywin32 validation runs first and exits because pywin32 isn't installed in the test environment. The fix mocks pywintypes to succeed so we can properly test that secretstorage validation is skipped on non-Linux platforms. --- tests/test_dependency_validator.py | 38 +++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index a71b141604..020cd35ba8 100644 --- a/tests/test_dependency_validator.py +++ b/tests/test_dependency_validator.py @@ -233,23 +233,45 @@ def selective_mock(name, *args, **kwargs): def test_windows_skips_secretstorage_validation(self): """Windows should skip secretstorage validation.""" - with patch("core.dependency_validator.is_linux", return_value=False), \ - patch("builtins.__import__") as mock_import: - # Even if secretstorage is not available, should not warn - mock_import.side_effect = ImportError("No module named 'secretstorage'") + 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._exit_with_secretstorage_warning") 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.""" - with patch("core.dependency_validator.is_linux", return_value=False), \ - patch("builtins.__import__") as mock_import: - # Even if secretstorage is not available, should not warn - mock_import.side_effect = ImportError("No module named 'secretstorage'") + 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._exit_with_secretstorage_warning") as mock_warning, \ + patch("builtins.__import__", side_effect=mock_import): # Should not call warning function validate_platform_dependencies() + mock_warning.assert_not_called() # ============================================================================= From a9831a9cd09085174c8c116fed030460c791fc04 Mon Sep 17 00:00:00 2001 From: StillKnotKnown Date: Fri, 16 Jan 2026 20:55:13 +0200 Subject: [PATCH 4/5] fix(linux): address CodeRabbit review comments for ACS-310 Changes made based on CodeRabbit AI review: Backend (dependency_validator.py): - Rename _exit_with_secretstorage_warning to _warn_missing_secretstorage to accurately reflect that it doesn't exit (it only emits a warning) - Add sys.stderr.flush() to ensure warning is immediately visible Frontend (download-python.cjs): - Fix critical __init__.py validation logic - packages are now only considered valid if directory+__init__.py exists OR single-file module exists Frontend (python-env-manager.ts): - Use platform abstraction (isWindows(), isLinux()) instead of process.platform - Fix critical packages validation to match download-python.cjs logic Tests (test_dependency_validator.py): - Update function name to _warn_missing_secretstorage - Add is_windows patches to Linux tests to prevent pywin32 validation from running on Windows CI Refs: ACS-310 --- apps/backend/core/dependency_validator.py | 7 ++-- apps/frontend/scripts/download-python.cjs | 6 ++- apps/frontend/src/main/python-env-manager.ts | 7 ++-- tests/test_dependency_validator.py | 40 ++++++++++---------- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/apps/backend/core/dependency_validator.py b/apps/backend/core/dependency_validator.py index 761e2cfa41..1e55c1b362 100644 --- a/apps/backend/core/dependency_validator.py +++ b/apps/backend/core/dependency_validator.py @@ -33,7 +33,7 @@ def validate_platform_dependencies() -> None: try: import secretstorage # noqa: F401 except ImportError: - _exit_with_secretstorage_warning() + _warn_missing_secretstorage() def _exit_with_pywin32_error() -> None: @@ -87,8 +87,8 @@ def _exit_with_pywin32_error() -> None: ) -def _exit_with_secretstorage_warning() -> None: - """Exit with helpful warning message for missing secretstorage. +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 @@ -119,4 +119,5 @@ def _exit_with_secretstorage_warning() -> None: "\n" f"Current Python: {sys.executable}\n" ) + sys.stderr.flush() # Continue execution - this is a warning, not a blocking error diff --git a/apps/frontend/scripts/download-python.cjs b/apps/frontend/scripts/download-python.cjs index fa4443771a..71edd52e1f 100644 --- a/apps/frontend/scripts/download-python.cjs +++ b/apps/frontend/scripts/download-python.cjs @@ -719,10 +719,11 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { .concat(platformCriticalPackages[info.nodePlatform] || []); const missingPackages = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); + 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(pkgPath) && !fs.existsSync(moduleFile); + return !(fs.existsSync(pkgPath) && fs.existsSync(initPath)) && !fs.existsSync(moduleFile); }); if (missingPackages.length > 0) { @@ -830,10 +831,11 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { .concat(platformCriticalPackages[info.nodePlatform] || []); const postInstallMissing = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); + 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(pkgPath) && !fs.existsSync(moduleFile); + return !(fs.existsSync(pkgPath) && fs.existsSync(initPath)) && !fs.existsSync(moduleFile); }); if (postInstallMissing.length > 0) { diff --git a/apps/frontend/src/main/python-env-manager.ts b/apps/frontend/src/main/python-env-manager.ts index 88564f5503..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; @@ -137,7 +137,8 @@ export class PythonEnvManager extends EventEmitter { 'claude_agent_sdk', 'dotenv', 'pydantic_core', - ...(platformCriticalPackages[process.platform] || []) + ...(isWindows() ? platformCriticalPackages.win32 : []), + ...(isLinux() ? platformCriticalPackages.linux : []) ]; // Check each package exists with valid structure (directory + __init__.py or single-file module) @@ -147,7 +148,7 @@ export class PythonEnvManager extends EventEmitter { // 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(pkgPath) && !existsSync(moduleFile); + return !(existsSync(pkgPath) && existsSync(initPath)) && !existsSync(moduleFile); }); // Log missing packages for debugging diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index 020cd35ba8..d2fc34b873 100644 --- a/tests/test_dependency_validator.py +++ b/tests/test_dependency_validator.py @@ -20,7 +20,7 @@ from core.dependency_validator import ( _exit_with_pywin32_error, - _exit_with_secretstorage_warning, + _warn_missing_secretstorage, validate_platform_dependencies, ) @@ -45,7 +45,7 @@ def test_windows_python_312_with_pywin32_missing_exits(self): 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_secretstorage_warning") as mock_warning: + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning: # Mock pywintypes import to raise ImportError original_import = builtins.__import__ @@ -82,7 +82,7 @@ def selective_mock(name, *args, **kwargs): 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_secretstorage_warning") as mock_warning, \ + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning, \ patch("builtins.__import__", side_effect=selective_mock): # Should not raise SystemExit validate_platform_dependencies() @@ -94,7 +94,7 @@ def test_windows_python_311_skips_validation(self): 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_secretstorage_warning") as mock_warning, \ + 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'") @@ -118,7 +118,7 @@ def mock_import(name, *args, **kwargs): 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("core.dependency_validator._exit_with_secretstorage_warning") as mock_warning, \ + 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() @@ -129,7 +129,7 @@ def test_macos_skips_pywin32_validation(self): 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._exit_with_secretstorage_warning") as mock_warning, \ + 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'") @@ -147,7 +147,7 @@ def test_windows_python_313_validates(self): 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_secretstorage_warning") as mock_warning: + patch("core.dependency_validator._warn_missing_secretstorage") as mock_warning: original_import = builtins.__import__ @@ -170,7 +170,7 @@ def test_windows_python_310_skips_validation(self): 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._exit_with_secretstorage_warning") as mock_warning, \ + 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'") @@ -198,8 +198,9 @@ def test_linux_with_secretstorage_missing_warns(self): """ import builtins - with patch("core.dependency_validator.is_linux", return_value=True), \ - patch("core.dependency_validator._exit_with_secretstorage_warning") as mock_warning: + 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__ @@ -226,7 +227,8 @@ def selective_mock(name, *args, **kwargs): return MagicMock() return original_import(name, *args, **kwargs) - with patch("core.dependency_validator.is_linux", return_value=True), \ + 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() @@ -247,7 +249,7 @@ def mock_import(name, *args, **kwargs): return original_import(name, *args, **kwargs) with patch("core.dependency_validator.is_linux", return_value=False), \ - patch("core.dependency_validator._exit_with_secretstorage_warning") as mock_warning, \ + 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() @@ -267,7 +269,7 @@ def mock_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._exit_with_secretstorage_warning") as mock_warning, \ + 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() @@ -275,16 +277,16 @@ def mock_import(name, *args, **kwargs): # ============================================================================= -# TESTS FOR _exit_with_secretstorage_warning (ACS-310) +# TESTS FOR _warn_missing_secretstorage (ACS-310) # ============================================================================= class TestExitWithSecretstorageWarning: - """Tests for _exit_with_secretstorage_warning function (ACS-310).""" + """Tests for _warn_missing_secretstorage function (ACS-310).""" def test_warning_message_contains_helpful_instructions(self, capsys): """Warning message should include installation instructions.""" - _exit_with_secretstorage_warning() + _warn_missing_secretstorage() # Get stderr output captured = capsys.readouterr() @@ -298,7 +300,7 @@ def test_warning_message_contains_helpful_instructions(self, capsys): def test_warning_message_mentions_fallback_behavior(self, capsys): """Warning should explain that app continues with .env fallback.""" - _exit_with_secretstorage_warning() + _warn_missing_secretstorage() captured = capsys.readouterr() message = captured.err @@ -310,7 +312,7 @@ def test_warning_message_mentions_fallback_behavior(self, capsys): def test_warning_message_contains_venv_path(self, capsys): """Warning message should include the virtual environment path.""" with patch("sys.prefix", "/path/to/venv"): - _exit_with_secretstorage_warning() + _warn_missing_secretstorage() captured = capsys.readouterr() message = captured.err @@ -324,7 +326,7 @@ 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: - _exit_with_secretstorage_warning() + _warn_missing_secretstorage() # Should NOT have called sys.exit mock_exit.assert_not_called() From 6a709a8725f9a6693cbd9d71923cb247eac2020b Mon Sep 17 00:00:00 2001 From: StillKnotKnown Date: Fri, 16 Jan 2026 21:59:30 +0200 Subject: [PATCH 5/5] fix(tests): mock requestAnimationFrame for xterm integration tests --- .../src/__tests__/integration/terminal-copy-paste.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) 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),