From a74bd8656ec9d257ebc05dfcf99a44834955e2ef Mon Sep 17 00:00:00 2001 From: ThrownLemon <4219774+ThrownLemon@users.noreply.github.com> Date: Thu, 8 Jan 2026 20:02:28 +1000 Subject: [PATCH] feat: add PR creation workflow for task worktrees (#677) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: add PR creation workflow for task worktrees Adds the ability to push a worktree branch and create a GitHub Pull Request directly from the Auto-Claude UI, instead of manually merging changes locally. ## User Flow 1. User completes a task build in an isolated worktree 2. Instead of clicking "Merge", user can click "Create PR" button 3. A dialog shows source branch → target branch (default: develop) 4. User confirms, system pushes branch and creates GitHub PR via `gh` CLI 5. PR URL is displayed and can be opened in browser ## Changes ### Backend (Python) - Added `push_branch()` with timeout (120s) for git push - Added `create_pull_request()` with timeout (60s) for gh CLI - Added `push_and_create_pr()` orchestrator - Added `--create-pr` CLI argument with handler - Added BRANCH and LINK icons with unique ASCII fallbacks ### Frontend (TypeScript) - Added `WorktreeCreatePRResult` type - Added `TASK_WORKTREE_CREATE_PR` IPC channel - Added IPC handler with 2-min timeout and EAFP pattern - Added `createWorktreePR` preload API method - Created reusable `CreatePRDialog` component - Integrated PR button in `WorkspaceStatus` - Added i18n translations (EN + FR) ## Code Review Fixes (from PR #606) - All subprocess calls have timeouts (TimeoutExpired handled) - EAFP pattern for file existence checks (no TOCTOU) - IPC handler has timeout with process cleanup - Icon ASCII fallbacks are unique (`[BR]` for BRANCH, `[L]` for LINK) - All user-facing strings use i18n translation keys - Translations added to BOTH en/*.json AND fr/*.json - CreatePRDialog component is reusable - Proper typed objects (no type assertions) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address PR review comments and add PR status persistence Review comment fixes: - Fix NameError: use args.base_branch instead of undefined base_branch (main.py) - Add JSON output for frontend IPC consumption (main.py) - Narrow exception handling in _extract_spec_summary to (OSError, UnicodeDecodeError) - Narrow exception handling in _get_existing_pr_url to subprocess-specific exceptions - Add debug logging for exception cases in worktree.py - Add 'exit' event handler to IPC handler for robustness (worktree-handlers.ts) Additional improvements: - Persist PR status to both main and worktree locations - Add CreatePR button to Worktrees page with i18n support - Add CreatePRDialog tests (11 test cases) - Fix i18n compliance for all new strings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(a11y): use button instead of anchor for PR link action Addresses review comment: anchor elements should only be used for navigation, not for triggering actions. Using a button improves accessibility for screen readers and keyboard users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * refactor: address nitpick review comments Backend (worktree.py): - Add TypedDict types (PushBranchResult, PullRequestResult) for better type safety - Add retry logic with exponential backoff (3 attempts) for transient network failures - Retries on: connection errors, network issues, timeouts, reset connections Frontend: - Fix checkbox accessibility: add explicit id/htmlFor for draft PR checkbox - Normalize return type in TaskDetailModal.handleCreatePR to include all fields - Add message field to WorktreeCreatePRResult for consistency with other result types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: remove duplicate JSON output in create-pr command The JSON was being printed twice: 1. In workspace_commands.py handle_create_pr_command() 2. In main.py after calling handle_create_pr_command() This caused JSON.parse to fail with "Unexpected non-whitespace character after JSON" when the frontend tried to parse the output. Removed the duplicate print from main.py since workspace_commands.py already handles JSON output for frontend parsing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: make IPC handler debug logging conditional Debug output for MERGE and CREATE_PR handlers now only appears when: - process.env.DEBUG === 'true', OR - process.env.NODE_ENV === 'development' This matches the pattern used elsewhere in the codebase (project-initializer.ts, terminal-name-generator.ts, etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address code review feedback on JSON parsing and status persistence - Use non-greedy regex pattern to extract last complete JSON object from stdout, avoiding issues with multiple JSON objects or garbage - Add validation that parsed JSON has expected shape before using (typeof checks for success, pr_url, already_exists, error fields) - Await persistPlanStatus calls instead of fire-and-forget to ensure status is persisted before resolving the IPC handler 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: ensure parent directory exists before writing metadata Add mkdirSync with recursive:true before writeFileSync in updateTaskMetadataPrUrl to prevent write failures when the parent directory doesn't exist. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * refactor: add TypedDict for push_and_create_pr return type Add PushAndCreatePRResult TypedDict with all fields (success, pushed, remote, branch, pr_url, already_exists, error) for static type safety. Update push_and_create_pr method signature and return statements to use the TypedDict constructor. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(i18n): use feminine form for PR in French translation Change "PR créé" to "PR créée" to match French grammatical gender (PR is feminine: "la PR"). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(i18n): use translation key for Open PR button Replace hardcoded "Open PR" label with i18n key common:buttons.openPR in Worktrees.tsx. Add translation keys to en/common.json and fr/common.json. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(a11y): use semantic button for PR link in TaskMetadata Replace anchor element with semantic button for better accessibility. Screen readers now properly announce this as an interactive control. The visible URL text provides an accessible label. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(a11y,i18n): use semantic button and i18n for PR status in TaskDetailModal - Replace anchor element with semantic button for PR link - Replace hardcoded "PR Created" with t('tasks:status.prCreated') - Apply fix to both the completion state link and the badge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(i18n): use translation keys for PR button in WorkspaceStatus Add useTranslation hook and replace hardcoded strings: - "Creating PR..." → t('taskReview:pr.actions.creating') - "Create PR" → t('common:buttons.createPR') 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * test: scope numeric assertions to stats container in CreatePRDialog Use within() to scope commit count and changes assertions to the stats container, avoiding accidental matches elsewhere in the dialog. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: handle success results without prUrl in CreatePRDialog Allow success state to render even without a URL (e.g., from the "no JSON in output, assuming success" fallback). The PR link button is now conditionally rendered only when prUrl is present. This prevents the dialog from showing an empty body when the backend returns { success: true, prUrl: undefined }. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address PR review findings for PR creation feature Backend (worktree.py): - Validate PR URL extraction - set pr_url to None if no valid URL found - Add message field to TypedDicts for informative feedback - Handle missing URL gracefully for existing PRs with message Frontend (worktree-handlers.ts): - Add GIT_BRANCH_REGEX and PR_CREATION_TIMEOUT_MS as module-level constants - Add input validation for targetBranch parameter - Add branch name validation in getTaskBaseBranch - Fix inconsistent JSON regex pattern between success/error paths Tests (CreatePRDialog.test.tsx): - Add test for draft PR checkbox functionality - Add test for 'already exists' PR state - Add test for success without prUrl Constants (task.ts): - Add pr_created to TASK_STATUS_LABELS and TASK_STATUS_COLORS 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * refactor: extract helper functions from TASK_WORKTREE_CREATE_PR handler - Extract parsePRJsonOutput() for JSON parsing with snake_case/camelCase - Extract updateTaskStatusAfterPRCreation() for metadata updates - Extract buildCreatePRArgs() for argument construction with validation - Extract initializePythonEnvForPR() for Python environment setup - Add generic withRetry() helper with exponential backoff - Refactor inline updatePlanWithRetry() to use withRetry() helper Addresses HIGH priority review finding about handler complexity and MEDIUM priority finding about duplicated retry logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address additional PR review findings Backend (worktree.py): - Update PullRequestResult.pr_url and PushAndCreatePRResult.pr_url to allow None (str | None) for cases where PR was created but URL couldn't be extracted Frontend (CreatePRDialog): - Add data-testid="pr-stats-container" for stable test targeting - Update test to use getByTestId instead of brittle CSS class selector Frontend (TaskDetailModal): - Remove hardcoded English error strings from handleCreatePR - Propagate IPC errors directly, let CreatePRDialog use i18n fallbacks Frontend (TaskMetadata): - Add i18n support for "Pull Request" header label - Add translation keys to en/tasks.json and fr/tasks.json 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: handle success default and retry validation in PR handlers - Default success to false in parsePRJsonOutput to avoid masking failures when the field is missing from the JSON response - Add validation to withRetry to ensure at least one attempt is made by clamping maxRetries to a minimum of 1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(i18n): remove hardcoded error strings from Worktrees handleCreatePR Let CreatePRDialog handle i18n fallback for undefined error values instead of hardcoding 'Failed to create PR' and 'Unknown error'. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: reset isCreating flag when CreatePRDialog opens Prevents stale loading state when reopening the dialog after a previous PR creation attempt. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix(test): use os.tmpdir() for cross-platform temp path matching Tests were hardcoded to expect /tmp/ but macOS uses /var/folders/.../T/ for temp files. Now dynamically uses os.tmpdir() for platform-independent path matching. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * style: apply pre-commit auto-fixes - Remove trailing whitespace from 20 files - Fix ruff lint errors in Python files - Apply ruff formatting 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address CodeQL and code review findings - Extract escapeForRegex helper in claude-integration-handler.test.ts to deduplicate regex-escaping logic and avoid ReDoS false-positive - Anchor regex pattern in CreatePRDialog.test.tsx to prevent arbitrary host matching (CodeQL security alert) - Remove unused ExternalLink import from TaskCard.tsx - Add defensive window.electronAPI check in CreatePRDialog handleOpenPR to avoid runtime errors in test/misconfigured environments 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address CodeQL and code review findings Frontend: - Fix CodeQL regex anchor issue in CreatePRDialog.test.tsx by using data-testid="pr-link-button" instead of URL regex pattern - Add data-testid to PR link button in CreatePRDialog.tsx - Add defensive window.electronAPI?.openExternal check in TaskCard.tsx Backend: - Add CreatePRResult TypedDict for type-safe return values - Wrap push_and_create_pr call in try/except for clean JSON output on exceptions instead of unhandled tracebacks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * feat: add frontend validation for PR creation form - Add client-side validation for branch names and PR titles - Validate git branch name format (alphanumeric, hyphens, underscores, slashes) - Ensure PR title is not empty - Provide immediate user feedback before backend submission - Add localized error messages in English and French * refactor: improve error handling and import organization in PR creation - Clean up CreatePRResult error structure: separate user-friendly 'message' from technical 'error' field - Move get_existing_build_worktree import to module-level imports for consistency - Remove redundant local import inside handle_create_pr_command function - Improve API clarity by providing both user messages and technical error details * refactor: properly convert PushAndCreatePRResult to CreatePRResult in CLI handler - Convert raw PushAndCreatePRResult to expected CreatePRResult shape - Map fields appropriately: success, pr_url, already_exists, error, message - Maintain type safety by returning declared CreatePRResult instead of raw result - Preserve all essential information while conforming to API contract - Improve code maintainability and type correctness * feat: include push and branch details in CreatePRResult - Add pushed, remote, and branch fields to CreatePRResult type - Include push status, remote name, and branch name in CLI result - Provide complete operation details for frontend consumption - Enhance API with comprehensive PR creation status information - Maintain backward compatibility while adding useful metadata * fix: improve type safety and i18n consistency for task status - Add isValidDropColumn type guard in KanbanBoard.tsx to preserve literal types from TASK_STATUS_COLUMNS instead of using unsafe cast - Replace duplicate CheckCircle2 with GitPullRequest icon in TaskDetailModal PR button for visual consistency with TaskCard - Normalize pr_created i18n key to columns.pr_created namespace - Add pr_created translation keys to en/fr tasks.json columns section - Update all hardcoded status.prCreated references to use mapping 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: remove duplicate PR Created badges and unused import - Remove unused ExternalLink import from TaskDetailModal.tsx - Fix duplicate badge rendering for pr_created status in both TaskCard and TaskDetailModal - Consolidate to single badge showing 'PR Created' for completed PR tasks * refactor: extract status badge variant logic and use i18n for completion text - Extract complex badge variant ternary into getStatusBadgeVariant helper function in TaskDetailModal - Replace hardcoded 'Task completed' with i18n translation t('tasks:status.complete') - Update getStatusBadgeVariant in TaskCard to return 'success' for pr_created status - Use getStatusBadgeVariant consistently instead of hardcoded variant in pr_created conditional * fix: use optional chaining for electronAPI in PR URL button - Update TaskDetailModal PR URL button onClick to use window.electronAPI?.openExternal - Matches the pattern used in TaskCard.tsx handleViewPR function - Prevents runtime errors when electronAPI is undefined * fix: add URL validation for parsed PR URLs Add isValidGitHubUrl() helper to validate PR URLs are valid https://github.com or *.github.com URLs before using them. This improves robustness by filtering out invalid URLs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * refactor: extract WorktreeCreatePROptions into named exported type Extract the inline options object from createWorktreePR signature into a reusable named type. Updated all callers and related declarations to use the new type for consistency across components. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: use WorktreeCreatePROptions type and add defensive optional chaining - Update createWorktreePR implementation to use WorktreeCreatePROptions instead of inline type (matches interface declaration) - Add optional chaining for window.electronAPI?.openExternal in Worktrees - Remove unused ExternalLink import from Worktrees component 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address PR review findings for code quality - Extract retry helper functions in worktree.py for DRY network error handling - Fix broad 'http' retry condition to exclude auth errors (401, 403) - Add Windows taskkill fallback for forceful process termination - Import CreatePRResult from worktree.py instead of duplicating TypedDict - Move import to top of worktree.py following Python conventions - Return result object from updateTaskStatusAfterPRCreation for better state tracking - Add PR title validation (printable chars, 256 char max) - Use WorktreeCreatePROptions type consistently in handler 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 * fix: address PR review findings - dedupe retry logic and support GH Enterprise URLs - Refactor push_branch and create_pull_request to use _with_retry helper instead of duplicated retry loops (addresses code duplication issue) - Update isValidGitHubUrl to accept any HTTPS URL with /pull/\d+ path to support GitHub Enterprise instances with custom domains Co-Authored-By: Claude Opus 4.5 * fix(ui): relax isValidGitHubUrl validation for GH Enterprise support - Remove /pull/\d+ path requirement that was too strict - Only require HTTPS protocol and non-empty hostname - Allows GitHub Enterprise URLs with custom domains to be parsed correctly Co-Authored-By: Claude Opus 4.5 * fix: address CodeRabbit feedback for PR creation - Fix undefined base_branch variable in CLI main.py with proper auto-detection - Improve event handling in worktree-handlers.ts with comprehensive exit event support - Fix dynamic retry count in error messages instead of hardcoded '3 attempts' - Use get_git_executable() and handle FileNotFoundError in push_branch method - Move debug_warning import to module level for better performance - Ensure all error messages reflect actual retry counts used * fix: address additional PR review feedback - main.py: Simplify PR creation by passing pr_target directly to handler, letting WorktreeManager._detect_base_branch handle detection internally - worktree.py: Fix _with_retry type signature to match actual tuple return, use get_git_executable() for proper git path resolution, move debug_warning import to top of file - worktree-handlers.ts: Extract duplicated close/exit callback logic into handleCreatePRProcessExit helper function - workspace_commands.py: Remove redundant json import (CodeQL fix) Co-Authored-By: Claude Opus 4.5 * fix(test): clear GIT_INDEX_FILE in temp_git_repo fixture Pre-commit sets GIT_INDEX_FILE to a relative path (.git/index.pre-commit) which causes git commands in temp repos to fail with "index file open failed: Not a directory" because the relative path resolves against the main repo instead of the temp repo. The fix saves and clears GIT_INDEX_FILE before creating the temp repo, then restores it in a finally block to ensure cleanup. Co-Authored-By: Claude Opus 4.5 * fix: use proper base branch fallback for PR creation target The worktree status handlers were incorrectly determining baseBranch by checking the current HEAD branch in the main project directory. This caused the PR creation dialog to pre-populate the target branch with the user's current feature branch instead of main/develop. Added getEffectiveBaseBranch() helper that properly determines the base branch using this priority: 1. Task metadata baseBranch (from task_metadata.json) 2. Project settings mainBranch 3. Git detection (main/master branch existence) 4. Fallback to 'main' Fixed three handlers: - TASK_WORKTREE_STATUS - TASK_WORKTREE_DIFF - List worktrees helper Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com> --- apps/backend/cli/main.py | 40 + apps/backend/cli/workspace_commands.py | 116 ++- apps/backend/core/worktree.py | 481 +++++++++++- apps/backend/ui/icons.py | 3 +- .../integration/subprocess-spawn.test.ts | 2 +- .../src/main/agent/agent-process.test.ts | 8 +- apps/frontend/src/main/agent/env-utils.ts | 8 +- .../src/main/ipc-handlers/context/utils.ts | 2 +- .../main/ipc-handlers/github/pr-handlers.ts | 24 +- .../github/utils/subprocess-runner.test.ts | 8 +- .../main/ipc-handlers/task/plan-file-utils.ts | 40 + .../ipc-handlers/task/worktree-handlers.ts | 741 ++++++++++++++++-- apps/frontend/src/main/memory-service.ts | 10 +- apps/frontend/src/main/project-store.ts | 6 + .../main/services/profile/profile-manager.ts | 26 +- .../main/services/profile/profile-service.ts | 14 +- .../claude-integration-handler.test.ts | 10 +- apps/frontend/src/preload/api/profile-api.ts | 4 +- apps/frontend/src/preload/api/task-api.ts | 8 +- .../src/renderer/components/KanbanBoard.tsx | 23 +- .../src/renderer/components/TaskCard.tsx | 68 +- .../src/renderer/components/Worktrees.tsx | 85 +- .../components/context/MemoriesTab.tsx | 4 +- .../components/context/MemoryCard.tsx | 6 +- .../components/context/PRReviewCard.tsx | 6 +- .../github-prs/hooks/useGitHubPRs.ts | 4 +- .../onboarding/OllamaModelSelector.tsx | 6 +- .../settings/ModelSearchableSelect.tsx | 6 +- .../task-detail/TaskDetailModal.tsx | 86 +- .../components/task-detail/TaskMetadata.tsx | 26 +- .../components/task-detail/TaskReview.tsx | 27 +- .../task-detail/hooks/useTaskDetail.ts | 6 + .../task-review/CreatePRDialog.test.tsx | 409 ++++++++++ .../task-review/CreatePRDialog.tsx | 274 +++++++ .../task-review/WorkspaceStatus.tsx | 31 +- .../task-detail/task-review/index.ts | 1 + .../components/terminal/usePtyProcess.ts | 2 +- .../src/renderer/components/ui/button.tsx | 2 + .../src/renderer/lib/mocks/workspace-mock.ts | 8 + apps/frontend/src/shared/constants/ipc.ts | 1 + apps/frontend/src/shared/constants/task.ts | 8 +- .../src/shared/i18n/locales/en/common.json | 2 + .../shared/i18n/locales/en/taskReview.json | 31 + .../src/shared/i18n/locales/en/tasks.json | 11 +- .../src/shared/i18n/locales/fr/common.json | 2 + .../shared/i18n/locales/fr/taskReview.json | 31 + .../src/shared/i18n/locales/fr/tasks.json | 11 +- apps/frontend/src/shared/types/ipc.ts | 3 + apps/frontend/src/shared/types/project.ts | 12 +- apps/frontend/src/shared/types/task.ts | 23 +- tests/test_pr_worktree_manager.py | 145 ++-- tests/test_security_cache.py | 10 +- 52 files changed, 2643 insertions(+), 278 deletions(-) create mode 100644 apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.test.tsx create mode 100644 apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.tsx diff --git a/apps/backend/cli/main.py b/apps/backend/cli/main.py index 9b910b53..cfb6a6a4 100644 --- a/apps/backend/cli/main.py +++ b/apps/backend/cli/main.py @@ -38,6 +38,7 @@ from .utils import ( ) from .workspace_commands import ( handle_cleanup_worktrees_command, + handle_create_pr_command, handle_discard_command, handle_list_worktrees_command, handle_merge_command, @@ -153,6 +154,30 @@ Environment Variables: action="store_true", help="Discard an existing build (requires confirmation)", ) + build_group.add_argument( + "--create-pr", + action="store_true", + help="Push branch and create a GitHub Pull Request", + ) + + # PR options + parser.add_argument( + "--pr-target", + type=str, + metavar="BRANCH", + help="With --create-pr: target branch for PR (default: auto-detect)", + ) + parser.add_argument( + "--pr-title", + type=str, + metavar="TITLE", + help="With --create-pr: custom PR title (default: generated from spec name)", + ) + parser.add_argument( + "--pr-draft", + action="store_true", + help="With --create-pr: create as draft PR", + ) # Merge options parser.add_argument( @@ -365,6 +390,21 @@ def main() -> None: handle_discard_command(project_dir, spec_dir.name) return + if args.create_pr: + # Pass args.pr_target directly - WorktreeManager._detect_base_branch + # handles base branch detection internally when target_branch is None + result = handle_create_pr_command( + project_dir=project_dir, + spec_name=spec_dir.name, + target_branch=args.pr_target, + title=args.pr_title, + draft=args.pr_draft, + ) + # JSON output is already printed by handle_create_pr_command + if not result.get("success"): + sys.exit(1) + return + # Handle QA commands if args.qa_status: handle_qa_status_command(spec_dir) diff --git a/apps/backend/cli/workspace_commands.py b/apps/backend/cli/workspace_commands.py index e6f25091..85f9f732 100644 --- a/apps/backend/cli/workspace_commands.py +++ b/apps/backend/cli/workspace_commands.py @@ -5,6 +5,7 @@ Workspace Commands CLI commands for workspace management (merge, review, discard, list, cleanup) """ +import json import subprocess import sys from pathlib import Path @@ -22,6 +23,7 @@ from core.workspace.git_utils import ( get_merge_base, is_lock_file, ) +from core.worktree import PushAndCreatePRResult as CreatePRResult from core.worktree import WorktreeManager from debug import debug_warning from ui import ( @@ -31,6 +33,7 @@ from ui import ( from workspace import ( cleanup_all_worktrees, discard_existing_build, + get_existing_build_worktree, list_all_worktrees, merge_existing_build, review_existing_build, @@ -175,8 +178,6 @@ def _detect_worktree_base_branch( Returns: The detected base branch name, or None if unable to detect """ - import json - # Strategy 1: Check for worktree config file config_path = worktree_path / ".auto-claude" / "worktree-config.json" if config_path.exists(): @@ -1002,6 +1003,117 @@ def handle_merge_preview_command( } +def handle_create_pr_command( + project_dir: Path, + spec_name: str, + target_branch: str | None = None, + title: str | None = None, + draft: bool = False, +) -> CreatePRResult: + """ + Handle the --create-pr command: push branch and create a GitHub PR. + + Args: + project_dir: Path to the project directory + spec_name: Name of the spec (e.g., "001-feature-name") + target_branch: Target branch for PR (defaults to base branch) + title: Custom PR title (defaults to spec name) + draft: Whether to create as draft PR + + Returns: + CreatePRResult with success status, pr_url, and any errors + """ + from core.worktree import WorktreeManager + + print_banner() + print("\n" + "=" * 70) + print(" CREATE PULL REQUEST") + print("=" * 70) + + # Check if worktree exists + worktree_path = get_existing_build_worktree(project_dir, spec_name) + if not worktree_path: + print(f"\n{icon(Icons.ERROR)} No build found for spec: {spec_name}") + print("\nA completed build worktree is required to create a PR.") + print("Run your build first, then use --create-pr.") + error_result: CreatePRResult = { + "success": False, + "error": "No build found for this spec", + } + return error_result + + # Create worktree manager + manager = WorktreeManager(project_dir, base_branch=target_branch) + + print(f"\n{icon(Icons.BRANCH)} Pushing branch and creating PR...") + print(f" Spec: {spec_name}") + print(f" Target: {target_branch or manager.base_branch}") + if title: + print(f" Title: {title}") + if draft: + print(" Mode: Draft PR") + + # Push and create PR with exception handling for clean JSON output + try: + raw_result = manager.push_and_create_pr( + spec_name=spec_name, + target_branch=target_branch, + title=title, + draft=draft, + ) + except Exception as e: + debug_error(MODULE, f"Exception during PR creation: {e}") + error_result: CreatePRResult = { + "success": False, + "error": str(e), + "message": "Failed to create PR", + } + print(f"\n{icon(Icons.ERROR)} Failed to create PR: {e}") + print(json.dumps(error_result)) + return error_result + + # Convert PushAndCreatePRResult to CreatePRResult + result: CreatePRResult = { + "success": raw_result.get("success", False), + "pr_url": raw_result.get("pr_url"), + "already_exists": raw_result.get("already_exists", False), + "error": raw_result.get("error"), + "message": raw_result.get("message"), + "pushed": raw_result.get("pushed", False), + "remote": raw_result.get("remote", ""), + "branch": raw_result.get("branch", ""), + } + + if result.get("success"): + pr_url = result.get("pr_url") + already_exists = result.get("already_exists", False) + + if already_exists: + print(f"\n{icon(Icons.SUCCESS)} PR already exists!") + else: + print(f"\n{icon(Icons.SUCCESS)} PR created successfully!") + + if pr_url: + print(f"\n{icon(Icons.LINK)} {pr_url}") + else: + print(f"\n{icon(Icons.INFO)} Check GitHub for the PR URL") + + print("\nNext steps:") + print(" 1. Review the PR on GitHub") + print(" 2. Request reviews from your team") + print(" 3. Merge when approved") + + # Output JSON for frontend parsing + print(json.dumps(result)) + return result + else: + error = result.get("error", "Unknown error") + print(f"\n{icon(Icons.ERROR)} Failed to create PR: {error}") + # Output JSON for frontend parsing + print(json.dumps(result)) + return result + + def cleanup_old_worktrees_command( project_dir: Path, days: int = 30, dry_run: bool = False ) -> dict: diff --git a/apps/backend/core/worktree.py b/apps/backend/core/worktree.py index a8e19498..eb4870dd 100644 --- a/apps/backend/core/worktree.py +++ b/apps/backend/core/worktree.py @@ -19,11 +19,126 @@ import os import re import shutil import subprocess +import time +from collections.abc import Callable from dataclasses import dataclass from datetime import datetime from pathlib import Path +from typing import TypedDict, TypeVar -from core.git_executable import run_git +from core.git_executable import get_git_executable, run_git +from debug import debug_warning + +T = TypeVar("T") + + +def _is_retryable_network_error(stderr: str) -> bool: + """Check if an error is a retryable network/connection issue.""" + stderr_lower = stderr.lower() + return any( + term in stderr_lower + for term in ["connection", "network", "timeout", "reset", "refused"] + ) + + +def _is_retryable_http_error(stderr: str) -> bool: + """ + Check if an HTTP error is retryable (5xx errors, timeouts). + Excludes auth errors (401, 403) and client errors (404, 422). + """ + stderr_lower = stderr.lower() + # Check for HTTP 5xx errors (server errors are retryable) + if re.search(r"http[s]?\s*5\d{2}", stderr_lower): + return True + # Check for HTTP timeout patterns + if "http" in stderr_lower and "timeout" in stderr_lower: + return True + return False + + +def _with_retry( + operation: Callable[[], tuple[bool, T | None, str]], + max_retries: int = 3, + is_retryable: Callable[[str], bool] | None = None, + on_retry: Callable[[int, str], None] | None = None, +) -> tuple[T | None, str]: + """ + Execute an operation with retry logic. + + Args: + operation: Function that returns a tuple of (success: bool, result: T | None, error: str). + On success (success=True), result contains the value and error is empty. + On failure (success=False), result is None and error contains the message. + max_retries: Maximum number of retry attempts + is_retryable: Function to check if error is retryable based on error message + on_retry: Optional callback called before each retry with (attempt, error) + + Returns: + Tuple of (result, last_error) where result is T on success, None on failure + """ + last_error = "" + + for attempt in range(1, max_retries + 1): + try: + success, result, error = operation() + if success: + return result, "" + + last_error = error + + # Check if error is retryable + if is_retryable and attempt < max_retries and is_retryable(error): + if on_retry: + on_retry(attempt, error) + backoff = 2 ** (attempt - 1) + time.sleep(backoff) + continue + + break + + except subprocess.TimeoutExpired: + last_error = "Operation timed out" + if attempt < max_retries: + if on_retry: + on_retry(attempt, last_error) + backoff = 2 ** (attempt - 1) + time.sleep(backoff) + continue + break + + return None, last_error + + +class PushBranchResult(TypedDict, total=False): + """Result of pushing a branch to remote.""" + + success: bool + branch: str + remote: str + error: str + + +class PullRequestResult(TypedDict, total=False): + """Result of creating a pull request.""" + + success: bool + pr_url: str | None # None when PR was created but URL couldn't be extracted + already_exists: bool + error: str + message: str + + +class PushAndCreatePRResult(TypedDict, total=False): + """Result of push_and_create_pr operation.""" + + success: bool + pushed: bool + remote: str + branch: str + pr_url: str | None # None when PR was created but URL couldn't be extracted + already_exists: bool + error: str + message: str class WorktreeError(Exception): @@ -57,6 +172,11 @@ class WorktreeManager: a corresponding branch auto-claude/{spec-name}. """ + # Timeout constants for subprocess operations + GIT_PUSH_TIMEOUT = 120 # 2 minutes for git push (network operations) + GH_CLI_TIMEOUT = 60 # 1 minute for gh CLI commands + GH_QUERY_TIMEOUT = 30 # 30 seconds for gh CLI queries + def __init__(self, project_dir: Path, base_branch: str | None = None): self.project_dir = project_dir self.base_branch = base_branch or self._detect_base_branch() @@ -194,7 +314,7 @@ class WorktreeManager: def get_worktree_path(self, spec_name: str) -> Path: """Get the worktree path for a spec (checks new and legacy locations).""" - # New path first + # New path first (.auto-claude/worktrees/tasks/) new_path = self.worktrees_dir / spec_name if new_path.exists(): return new_path @@ -684,6 +804,363 @@ class WorktreeManager: result = self._run_git(["status", "--porcelain"], cwd=cwd) return bool(result.stdout.strip()) + # ==================== PR Creation Methods ==================== + + def push_branch(self, spec_name: str, force: bool = False) -> PushBranchResult: + """ + Push a spec's branch to the remote origin with retry logic. + + Args: + spec_name: The spec folder name + force: Whether to force push (use with caution) + + Returns: + PushBranchResult with keys: + - success: bool + - branch: str (branch name) + - remote: str (if successful) + - error: str (if failed) + """ + info = self.get_worktree_info(spec_name) + if not info: + return PushBranchResult( + success=False, + error=f"No worktree found for spec: {spec_name}", + ) + + # Push the branch to origin + push_args = ["push", "-u", "origin", info.branch] + if force: + push_args.insert(1, "--force") + + def do_push() -> tuple[bool, PushBranchResult | None, str]: + """Execute push operation for retry wrapper.""" + try: + git_executable = get_git_executable() + result = subprocess.run( + [git_executable] + push_args, + cwd=info.path, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=self.GIT_PUSH_TIMEOUT, + ) + + if result.returncode == 0: + return ( + True, + PushBranchResult( + success=True, + branch=info.branch, + remote="origin", + ), + "", + ) + return (False, None, result.stderr) + except FileNotFoundError: + return (False, None, "git executable not found") + + max_retries = 3 + result, last_error = _with_retry( + operation=do_push, + max_retries=max_retries, + is_retryable=_is_retryable_network_error, + ) + + if result: + return result + + # Handle timeout error message + if last_error == "Operation timed out": + return PushBranchResult( + success=False, + branch=info.branch, + error=f"Push timed out after {max_retries} attempts.", + ) + + return PushBranchResult( + success=False, + branch=info.branch, + error=f"Failed to push branch: {last_error}", + ) + + def create_pull_request( + self, + spec_name: str, + target_branch: str | None = None, + title: str | None = None, + draft: bool = False, + ) -> PullRequestResult: + """ + Create a GitHub pull request for a spec's branch using gh CLI with retry logic. + + Args: + spec_name: The spec folder name + target_branch: Target branch for PR (defaults to base_branch) + title: PR title (defaults to spec name) + draft: Whether to create as draft PR + + Returns: + PullRequestResult with keys: + - success: bool + - pr_url: str (if created) + - already_exists: bool (if PR already exists) + - error: str (if failed) + """ + info = self.get_worktree_info(spec_name) + if not info: + return PullRequestResult( + success=False, + error=f"No worktree found for spec: {spec_name}", + ) + + target = target_branch or self.base_branch + pr_title = title or f"auto-claude: {spec_name}" + + # Get PR body from spec.md if available + pr_body = self._extract_spec_summary(spec_name) + + # Build gh pr create command + gh_args = [ + "gh", + "pr", + "create", + "--base", + target, + "--head", + info.branch, + "--title", + pr_title, + "--body", + pr_body, + ] + if draft: + gh_args.append("--draft") + + def is_pr_retryable(stderr: str) -> bool: + """Check if PR creation error is retryable (network or HTTP 5xx).""" + return _is_retryable_network_error(stderr) or _is_retryable_http_error( + stderr + ) + + def do_create_pr() -> tuple[bool, PullRequestResult | None, str]: + """Execute PR creation for retry wrapper.""" + try: + result = subprocess.run( + gh_args, + cwd=info.path, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=self.GH_CLI_TIMEOUT, + ) + + # Check for "already exists" case (success, no retry needed) + if result.returncode != 0 and "already exists" in result.stderr.lower(): + existing_url = self._get_existing_pr_url(spec_name, target) + result_dict = PullRequestResult( + success=True, + pr_url=existing_url, + already_exists=True, + ) + if existing_url is None: + result_dict["message"] = ( + "PR already exists but URL could not be retrieved" + ) + return (True, result_dict, "") + + if result.returncode == 0: + # Extract PR URL from output + pr_url: str | None = result.stdout.strip() + if not pr_url.startswith("http"): + # Try to find URL in output + # Use general pattern to support GitHub Enterprise instances + # Matches any HTTPS URL with /pull/ path + match = re.search(r"https://[^\s]+/pull/\d+", result.stdout) + if match: + pr_url = match.group(0) + else: + # Invalid output - no valid URL found + pr_url = None + + return ( + True, + PullRequestResult( + success=True, + pr_url=pr_url, + already_exists=False, + ), + "", + ) + + return (False, None, result.stderr) + + except FileNotFoundError: + # gh CLI not installed - not retryable, raise to exit retry loop + raise + + max_retries = 3 + try: + result, last_error = _with_retry( + operation=do_create_pr, + max_retries=max_retries, + is_retryable=is_pr_retryable, + ) + + if result: + return result + + # Handle timeout error message + if last_error == "Operation timed out": + return PullRequestResult( + success=False, + error=f"PR creation timed out after {max_retries} attempts.", + ) + + return PullRequestResult( + success=False, + error=f"Failed to create PR: {last_error}", + ) + + except FileNotFoundError: + # gh CLI not installed + return PullRequestResult( + success=False, + error="gh CLI not found. Install from https://cli.github.com/", + ) + + def _extract_spec_summary(self, spec_name: str) -> str: + """Extract a summary from spec.md for PR body.""" + worktree_path = self.get_worktree_path(spec_name) + spec_path = worktree_path / ".auto-claude" / "specs" / spec_name / "spec.md" + + if not spec_path.exists(): + # Try project spec path + spec_path = ( + self.project_dir / ".auto-claude" / "specs" / spec_name / "spec.md" + ) + + if not spec_path.exists(): + return "Auto-generated PR from Auto-Claude build." + + try: + content = spec_path.read_text(encoding="utf-8") + # Extract first few paragraphs (skip title, get overview) + lines = content.split("\n") + summary_lines = [] + in_content = False + + for line in lines: + # Skip title headers + if line.startswith("# "): + continue + # Start capturing after first content line + if line.strip() and not line.startswith("#"): + in_content = True + if in_content: + if line.startswith("## ") and summary_lines: + break # Stop at next section + summary_lines.append(line) + if len(summary_lines) >= 10: # Limit to ~10 lines + break + + summary = "\n".join(summary_lines).strip() + if summary: + return summary + except (OSError, UnicodeDecodeError) as e: + # Silently fall back to default - file read errors shouldn't block PR creation + debug_warning( + "worktree", f"Could not extract spec summary for PR body: {e}" + ) + + return "Auto-generated PR from Auto-Claude build." + + def _get_existing_pr_url(self, spec_name: str, target_branch: str) -> str | None: + """Get the URL of an existing PR for this branch.""" + info = self.get_worktree_info(spec_name) + if not info: + return None + + try: + result = subprocess.run( + ["gh", "pr", "view", info.branch, "--json", "url", "--jq", ".url"], + cwd=info.path, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=self.GH_QUERY_TIMEOUT, + ) + if result.returncode == 0: + return result.stdout.strip() + except ( + subprocess.TimeoutExpired, + FileNotFoundError, + subprocess.SubprocessError, + ) as e: + # Silently ignore errors when fetching existing PR URL - this is a best-effort + # lookup that may fail due to network issues, missing gh CLI, or auth problems. + # Returning None allows the caller to handle missing URLs gracefully. + debug_warning("worktree", f"Could not get existing PR URL: {e}") + + return None + + def push_and_create_pr( + self, + spec_name: str, + target_branch: str | None = None, + title: str | None = None, + draft: bool = False, + force_push: bool = False, + ) -> PushAndCreatePRResult: + """ + Push branch and create a pull request in one operation. + + Args: + spec_name: The spec folder name + target_branch: Target branch for PR (defaults to base_branch) + title: PR title (defaults to spec name) + draft: Whether to create as draft PR + force_push: Whether to force push the branch + + Returns: + PushAndCreatePRResult with keys: + - success: bool + - pr_url: str (if created) + - pushed: bool (if push succeeded) + - already_exists: bool (if PR already exists) + - error: str (if failed) + """ + # Step 1: Push the branch + push_result = self.push_branch(spec_name, force=force_push) + if not push_result.get("success"): + return PushAndCreatePRResult( + success=False, + pushed=False, + error=push_result.get("error", "Push failed"), + ) + + # Step 2: Create the PR + pr_result = self.create_pull_request( + spec_name=spec_name, + target_branch=target_branch, + title=title, + draft=draft, + ) + + # Combine results + return PushAndCreatePRResult( + success=pr_result.get("success", False), + pushed=True, + remote=push_result.get("remote"), + branch=push_result.get("branch"), + pr_url=pr_result.get("pr_url"), + already_exists=pr_result.get("already_exists", False), + error=pr_result.get("error"), + ) + # ==================== Worktree Cleanup Methods ==================== def get_old_worktrees( diff --git a/apps/backend/ui/icons.py b/apps/backend/ui/icons.py index 2f274961..13675eb3 100644 --- a/apps/backend/ui/icons.py +++ b/apps/backend/ui/icons.py @@ -39,9 +39,10 @@ class Icons: FILE = ("📄", "[F]") GEAR = ("⚙", "[*]") SEARCH = ("🔍", "[?]") - BRANCH = ("", "[B]") + BRANCH = ("🌿", "[BR]") # [BR] to avoid collision with BLOCKED [B] COMMIT = ("◉", "(@)") LIGHTNING = ("⚡", "!") + LINK = ("🔗", "[L]") # For PR URLs # Progress SUBTASK = ("▣", "#") diff --git a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts index 1d9e0540..f3ca37d4 100644 --- a/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts +++ b/apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts @@ -302,7 +302,7 @@ describe('Subprocess Spawn Integration', () => { await manager.startTaskExecution('task-2', TEST_PROJECT_PATH, 'spec-001'); expect(manager.getRunningTasks()).toHaveLength(2); - }); + }, 15000); it('should use configured Python path', async () => { const { spawn } = await import('child_process'); diff --git a/apps/frontend/src/main/agent/agent-process.test.ts b/apps/frontend/src/main/agent/agent-process.test.ts index db992bb5..c06b8f68 100644 --- a/apps/frontend/src/main/agent/agent-process.test.ts +++ b/apps/frontend/src/main/agent/agent-process.test.ts @@ -268,10 +268,10 @@ describe('AgentProcessManager - API Profile Env Injection (Story 2.3)', () => { await processManager.spawnProcess('task-1', '/fake/cwd', ['run.py'], {}, 'task-execution'); const envArg = spawnCalls[0].options.env as Record; - + // OAuth token should be present expect(envArg.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token-456'); - + // Stale ANTHROPIC_* vars should be cleared (empty string overrides process.env) expect(envArg.ANTHROPIC_AUTH_TOKEN).toBe(''); expect(envArg.ANTHROPIC_BASE_URL).toBe(''); @@ -292,7 +292,7 @@ describe('AgentProcessManager - API Profile Env Injection (Story 2.3)', () => { await processManager.spawnProcess('task-1', '/fake/cwd', ['run.py'], {}, 'task-execution'); const envArg = spawnCalls[0].options.env as Record; - + // Should clear the base URL (so Python uses default api.anthropic.com) expect(envArg.ANTHROPIC_BASE_URL).toBe(''); expect(envArg.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token-789'); @@ -314,7 +314,7 @@ describe('AgentProcessManager - API Profile Env Injection (Story 2.3)', () => { await processManager.spawnProcess('task-1', '/fake/cwd', ['run.py'], {}, 'task-execution'); const envArg = spawnCalls[0].options.env as Record; - + // Should use API profile vars, NOT clear them expect(envArg.ANTHROPIC_AUTH_TOKEN).toBe('sk-profile-active'); expect(envArg.ANTHROPIC_BASE_URL).toBe('https://active-profile.com'); diff --git a/apps/frontend/src/main/agent/env-utils.ts b/apps/frontend/src/main/agent/env-utils.ts index ba384dfa..3c479e60 100644 --- a/apps/frontend/src/main/agent/env-utils.ts +++ b/apps/frontend/src/main/agent/env-utils.ts @@ -4,18 +4,18 @@ /** * Get environment variables to clear ANTHROPIC_* vars when in OAuth mode - * - * When switching from API Profile mode to OAuth mode, residual ANTHROPIC_* + * + * When switching from API Profile mode to OAuth mode, residual ANTHROPIC_* * environment variables from process.env can cause authentication failures. * This function returns an object with empty strings for these vars when * no API profile is active, ensuring OAuth tokens are used correctly. - * + * * **Why empty strings?** Setting environment variables to empty strings (rather than * undefined) ensures they override any stale values from process.env. Python's SDK * treats empty strings as falsy in conditional checks like `if token:`, so empty * strings effectively disable these authentication parameters without leaving * undefined values that might be ignored during object spreading. - * + * * @param apiProfileEnv - Environment variables from getAPIProfileEnv() * @returns Object with empty ANTHROPIC_* vars if in OAuth mode, empty object otherwise */ diff --git a/apps/frontend/src/main/ipc-handlers/context/utils.ts b/apps/frontend/src/main/ipc-handlers/context/utils.ts index c8157517..6611e997 100644 --- a/apps/frontend/src/main/ipc-handlers/context/utils.ts +++ b/apps/frontend/src/main/ipc-handlers/context/utils.ts @@ -131,7 +131,7 @@ export interface EmbeddingValidationResult { /** * Validate embedding configuration based on the configured provider * Supports: openai, ollama, google, voyage, azure_openai - * + * * @returns validation result with provider info and reason if invalid */ export function validateEmbeddingConfiguration( diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index a8fea6d4..04fbd64a 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -181,10 +181,10 @@ export interface PRReviewMemory { /** * Save PR review insights to the Electron memory layer (LadybugDB) - * + * * Called after a PR review completes to persist learnings for cross-session context. * Extracts key findings, patterns, and gotchas from the review result. - * + * * @param result The completed PR review result * @param repo Repository name (owner/repo) * @param isFollowup Whether this is a follow-up review @@ -208,14 +208,14 @@ async function savePRReviewToMemory( // Build the memory content with comprehensive insights // We want to capture ALL meaningful findings so the AI can learn from patterns - + // Prioritize findings: critical > high > medium > low // Include all critical/high, top 5 medium, top 3 low const criticalFindings = result.findings.filter(f => f.severity === 'critical'); const highFindings = result.findings.filter(f => f.severity === 'high'); const mediumFindings = result.findings.filter(f => f.severity === 'medium').slice(0, 5); const lowFindings = result.findings.filter(f => f.severity === 'low').slice(0, 3); - + const keyFindingsToSave = [ ...criticalFindings, ...highFindings, @@ -233,8 +233,8 @@ async function savePRReviewToMemory( // Extract gotchas: security issues, critical bugs, and common mistakes const gotchaCategories = ['security', 'error_handling', 'data_validation', 'race_condition']; const gotchasToSave = result.findings - .filter(f => - f.severity === 'critical' || + .filter(f => + f.severity === 'critical' || f.severity === 'high' || gotchaCategories.includes(f.category?.toLowerCase() || '') ) @@ -246,7 +246,7 @@ async function savePRReviewToMemory( acc[cat] = (acc[cat] || 0) + 1; return acc; }, {} as Record); - + // Patterns are categories that appear multiple times (indicates a systematic issue) const patternsToSave = Object.entries(categoryGroups) .filter(([_, count]) => count >= 2) @@ -275,7 +275,7 @@ async function savePRReviewToMemory( // Add follow-up specific info if applicable if (isFollowup && result.resolvedFindings && result.unresolvedFindings) { - memoryContent.summary.verdict_reasoning = + memoryContent.summary.verdict_reasoning = `Resolved: ${result.resolvedFindings.length}, Unresolved: ${result.unresolvedFindings.length}`; } @@ -296,8 +296,8 @@ async function savePRReviewToMemory( } catch (error) { // Don't fail the review if memory save fails - debugLog('Error saving PR review to memory', { - error: error instanceof Error ? error.message : error + debugLog('Error saving PR review to memory', { + error: error instanceof Error ? error.message : error }); } } @@ -1728,7 +1728,7 @@ export function registerPRHandlers( const hasPending = checkRuns.check_runs.some( cr => cr.status !== 'completed' ); - + if (hasFailing) { ciStatus = 'failing'; } else if (hasPending) { @@ -2124,7 +2124,7 @@ export function registerPRHandlers( const reviewPath = path.join(memoryDir, `pr_${entry.pr_number}_review.json`); if (fs.existsSync(reviewPath)) { const reviewContent = fs.readFileSync(reviewPath, 'utf-8'); - + // Check if content matches query if (reviewContent.toLowerCase().includes(queryLower)) { const memory = JSON.parse(sanitizeNetworkData(reviewContent)); diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts index 49c504b0..8e22bc28 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts @@ -49,12 +49,12 @@ describe('runPythonSubprocess', () => { // Arrange const pythonPath = '/path/with spaces/python'; const mockArgs = ['-c', 'print("hello")']; - - // Mock parsePythonCommand to return the path split logic if needed, - // or just rely on the mock above. + + // Mock parsePythonCommand to return the path split logic if needed, + // or just rely on the mock above. // Let's make sure our mock enables the scenario we want. vi.mocked(parsePythonCommand).mockReturnValue(['/path/with spaces/python', []]); - + // Act runPythonSubprocess({ pythonPath, diff --git a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts index 933d0c5a..f90a3231 100644 --- a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts +++ b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts @@ -81,6 +81,8 @@ export function mapStatusToPlanStatus(status: TaskStatus): string { case 'ai_review': case 'human_review': return 'review'; + case 'pr_created': + return 'pr_created'; case 'done': return 'completed'; default: @@ -262,3 +264,41 @@ export async function createPlanIfNotExists( writeFileSync(planPath, JSON.stringify(plan, null, 2)); }); } + +/** + * Update task_metadata.json to add PR URL. + * This is a simple JSON file update (no locking needed as it's rarely updated concurrently). + * + * @param metadataPath - Path to the task_metadata.json file + * @param prUrl - The PR URL to add to metadata + * @returns true if metadata was updated, false if file doesn't exist or failed + */ +export function updateTaskMetadataPrUrl(metadataPath: string, prUrl: string): boolean { + try { + let metadata: Record = {}; + + // Try to read existing metadata + try { + const content = readFileSync(metadataPath, 'utf-8'); + metadata = JSON.parse(content); + } catch (err) { + if (!isFileNotFoundError(err)) { + throw err; + } + // File doesn't exist, will create new one + } + + // Update with prUrl + metadata.prUrl = prUrl; + + // Ensure parent directory exists before writing + mkdirSync(path.dirname(metadataPath), { recursive: true }); + + // Write back + writeFileSync(metadataPath, JSON.stringify(metadata, null, 2)); + return true; + } catch (err) { + console.warn(`[plan-file-utils] Could not update metadata at ${metadataPath}:`, err); + return false; + } +} diff --git a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts index 8c97ae76..30e8ca52 100644 --- a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts @@ -1,10 +1,12 @@ import { ipcMain, BrowserWindow, shell, app } from 'electron'; -import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP } from '../../../shared/constants'; -import type { IPCResult, WorktreeStatus, WorktreeDiff, WorktreeDiffFile, WorktreeMergeResult, WorktreeDiscardResult, WorktreeListResult, WorktreeListItem, SupportedIDE, SupportedTerminal, AppSettings } from '../../../shared/types'; +import { IPC_CHANNELS, AUTO_BUILD_PATHS, DEFAULT_APP_SETTINGS, DEFAULT_FEATURE_MODELS, DEFAULT_FEATURE_THINKING, MODEL_ID_MAP, THINKING_BUDGET_MAP, getSpecsDir } from '../../../shared/constants'; +import type { IPCResult, WorktreeStatus, WorktreeDiff, WorktreeDiffFile, WorktreeMergeResult, WorktreeDiscardResult, WorktreeListResult, WorktreeListItem, WorktreeCreatePROptions, WorktreeCreatePRResult, SupportedIDE, SupportedTerminal, AppSettings } from '../../../shared/types'; import path from 'path'; import { existsSync, readdirSync, statSync, readFileSync } from 'fs'; import { execSync, execFileSync, spawn, spawnSync, exec, execFile } from 'child_process'; -import { minimatch } from 'minimatch'; +import { createRequire } from 'module'; +const require = createRequire(import.meta.url); +const { minimatch } = require('minimatch'); import { projectStore } from '../../project-store'; import { getConfiguredPythonPath, PythonEnvManager, pythonEnvManager as pythonEnvManagerSingleton } from '../../python-env-manager'; import { getEffectiveSourcePath } from '../../updater/path-resolver'; @@ -17,6 +19,19 @@ import { getTaskWorktreeDir, findTaskWorktree, } from '../../worktree-paths'; +import { persistPlanStatus, updateTaskMetadataPrUrl } from './plan-file-utils'; + +// Regex pattern for validating git branch names +const GIT_BRANCH_REGEX = /^[a-zA-Z0-9][a-zA-Z0-9._/-]*[a-zA-Z0-9]$|^[a-zA-Z0-9]$/; + +// Maximum PR title length (GitHub's limit is 256 characters) +const MAX_PR_TITLE_LENGTH = 256; + +// Regex for validating PR title contains only printable characters +const PRINTABLE_CHARS_REGEX = /^[\x20-\x7E\u00A0-\uFFFF]*$/; + +// Timeout for PR creation operations (2 minutes for network operations) +const PR_CREATION_TIMEOUT_MS = 120000; /** * Read utility feature settings (for commit message, merge resolver) from settings file @@ -1285,7 +1300,10 @@ function getTaskBaseBranch(specDir: string): string | undefined { if (existsSync(metadataPath)) { const metadata = JSON.parse(readFileSync(metadataPath, 'utf-8')); // Return baseBranch if explicitly set (not the __project_default__ marker) - if (metadata.baseBranch && metadata.baseBranch !== '__project_default__') { + // Also validate it's a valid branch name to prevent malformed git commands + if (metadata.baseBranch && + metadata.baseBranch !== '__project_default__' && + GIT_BRANCH_REGEX.test(metadata.baseBranch)) { return metadata.baseBranch; } } @@ -1295,6 +1313,309 @@ function getTaskBaseBranch(specDir: string): string | undefined { return undefined; } +/** + * Get the effective base branch for a task with proper fallback chain. + * Priority: + * 1. Task metadata baseBranch (explicit task-level override from task_metadata.json) + * 2. Project settings mainBranch (project-level default) + * 3. Git default branch detection (main/master) + * 4. Fallback to 'main' + * + * This should be used instead of getting the current HEAD branch, + * as the user may be on a feature branch when viewing worktree status. + */ +function getEffectiveBaseBranch(projectPath: string, specId: string, projectMainBranch?: string): string { + // 1. Try task metadata baseBranch + const specDir = path.join(projectPath, '.auto-claude', 'specs', specId); + const taskBaseBranch = getTaskBaseBranch(specDir); + if (taskBaseBranch) { + return taskBaseBranch; + } + + // 2. Try project settings mainBranch + if (projectMainBranch && GIT_BRANCH_REGEX.test(projectMainBranch)) { + return projectMainBranch; + } + + // 3. Try to detect main/master branch + for (const branch of ['main', 'master']) { + try { + execFileSync(getToolPath('git'), ['rev-parse', '--verify', branch], { + cwd: projectPath, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'pipe'], + }); + return branch; + } catch { + // Branch doesn't exist, try next + } + } + + // 4. Fallback to 'main' + return 'main'; +} + +// ============================================ +// Helper functions for TASK_WORKTREE_CREATE_PR +// ============================================ + +/** + * Result of parsing JSON output from the create-pr Python script + */ +interface ParsedPRResult { + success: boolean; + prUrl?: string; + alreadyExists?: boolean; + error?: string; +} + +/** + * Validate that a URL is a valid GitHub PR URL. + * Supports both github.com and GitHub Enterprise instances (custom domains). + * Only requires HTTPS protocol and non-empty hostname to allow any GH Enterprise URL. + * @returns true if the URL is a valid HTTPS URL with a non-empty hostname + */ +function isValidGitHubUrl(url: string): boolean { + try { + const parsed = new URL(url); + // Only require HTTPS with non-empty hostname + // This supports GH Enterprise instances with custom domains + // The URL comes from gh CLI output which we trust to be valid + return parsed.protocol === 'https:' && parsed.hostname.length > 0; + } catch { + return false; + } +} + +/** + * Parse JSON output from the create-pr Python script + * Handles both snake_case and camelCase field names + * @returns ParsedPRResult if valid JSON found, null otherwise + */ +function parsePRJsonOutput(stdout: string): ParsedPRResult | null { + // Find the last complete JSON object in stdout (non-greedy, handles multiple objects) + const jsonMatches = stdout.match(/\{[^{}]*(?:\{[^{}]*\}[^{}]*)*\}/g); + const jsonMatch = jsonMatches && jsonMatches.length > 0 ? jsonMatches[jsonMatches.length - 1] : null; + + if (!jsonMatch) { + return null; + } + + try { + const parsed = JSON.parse(jsonMatch); + + // Validate parsed JSON has expected shape + if (typeof parsed !== 'object' || parsed === null) { + return null; + } + + // Extract and validate fields with proper type checking + // Handle both snake_case (from Python) and camelCase field names + // Default success to false to avoid masking failures when field is missing + const rawPrUrl = typeof parsed.pr_url === 'string' ? parsed.pr_url : + typeof parsed.prUrl === 'string' ? parsed.prUrl : undefined; + + // Validate PR URL is a valid GitHub URL for robustness + const validatedPrUrl = rawPrUrl && isValidGitHubUrl(rawPrUrl) ? rawPrUrl : undefined; + + return { + success: typeof parsed.success === 'boolean' ? parsed.success : false, + prUrl: validatedPrUrl, + alreadyExists: typeof parsed.already_exists === 'boolean' ? parsed.already_exists : + typeof parsed.alreadyExists === 'boolean' ? parsed.alreadyExists : undefined, + error: typeof parsed.error === 'string' ? parsed.error : undefined + }; + } catch { + return null; + } +} + +/** + * Result of updating task status after PR creation + */ +interface TaskStatusUpdateResult { + mainProjectStatus: boolean; + mainProjectMetadata: boolean; + worktreeStatus: boolean; + worktreeMetadata: boolean; +} + +/** + * Update task status and metadata after PR creation + * Updates both main project and worktree locations + * @returns Result object indicating which updates succeeded/failed + */ +async function updateTaskStatusAfterPRCreation( + specDir: string, + worktreePath: string | null, + prUrl: string, + autoBuildPath: string | undefined, + specId: string, + debug: (...args: unknown[]) => void +): Promise { + const result: TaskStatusUpdateResult = { + mainProjectStatus: false, + mainProjectMetadata: false, + worktreeStatus: false, + worktreeMetadata: false + }; + + const planPath = path.join(specDir, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); + const metadataPath = path.join(specDir, 'task_metadata.json'); + + // Await status persistence to ensure completion before resolving + try { + const persisted = await persistPlanStatus(planPath, 'pr_created'); + result.mainProjectStatus = persisted; + debug('Main project status persisted to pr_created:', persisted); + } catch (err) { + debug('Failed to persist main project status:', err); + } + + // Update metadata with prUrl in main project + result.mainProjectMetadata = updateTaskMetadataPrUrl(metadataPath, prUrl); + debug('Main project metadata updated with prUrl:', result.mainProjectMetadata); + + // Also persist to WORKTREE location (worktree takes priority when loading tasks) + // This ensures the status persists after refresh since getTasks() prefers worktree version + if (worktreePath) { + const specsBaseDir = getSpecsDir(autoBuildPath); + const worktreePlanPath = path.join(worktreePath, specsBaseDir, specId, AUTO_BUILD_PATHS.IMPLEMENTATION_PLAN); + const worktreeMetadataPath = path.join(worktreePath, specsBaseDir, specId, 'task_metadata.json'); + + try { + const persisted = await persistPlanStatus(worktreePlanPath, 'pr_created'); + result.worktreeStatus = persisted; + debug('Worktree status persisted to pr_created:', persisted); + } catch (err) { + debug('Failed to persist worktree status:', err); + } + + result.worktreeMetadata = updateTaskMetadataPrUrl(worktreeMetadataPath, prUrl); + debug('Worktree metadata updated with prUrl:', result.worktreeMetadata); + } + + return result; +} + +/** + * Build arguments for the create-pr Python script + */ +function buildCreatePRArgs( + runScript: string, + specId: string, + projectPath: string, + options: WorktreeCreatePROptions | undefined, + taskBaseBranch: string | undefined +): { args: string[]; validationError?: string } { + const args = [ + runScript, + '--spec', specId, + '--project-dir', projectPath, + '--create-pr' + ]; + + // Add optional arguments with validation + if (options?.targetBranch) { + // Validate branch name to prevent malformed git commands + if (!GIT_BRANCH_REGEX.test(options.targetBranch)) { + return { args: [], validationError: 'Invalid target branch name' }; + } + args.push('--pr-target', options.targetBranch); + } + if (options?.title) { + // Validate title for printable characters and length limit + if (options.title.length > MAX_PR_TITLE_LENGTH) { + return { args: [], validationError: `PR title exceeds maximum length of ${MAX_PR_TITLE_LENGTH} characters` }; + } + if (!PRINTABLE_CHARS_REGEX.test(options.title)) { + return { args: [], validationError: 'PR title contains invalid characters' }; + } + args.push('--pr-title', options.title); + } + if (options?.draft) { + args.push('--pr-draft'); + } + + // Add --base-branch if task was created with a specific base branch + if (taskBaseBranch) { + args.push('--base-branch', taskBaseBranch); + } + + return { args }; +} + +/** + * Initialize Python environment for PR creation + * @returns Error message if initialization fails, undefined on success + */ +async function initializePythonEnvForPR( + pythonEnvManager: PythonEnvManager +): Promise { + if (pythonEnvManager.isEnvReady()) { + return undefined; + } + + const autoBuildSource = getEffectiveSourcePath(); + if (!autoBuildSource) { + return 'Python environment not ready and Auto Claude source not found'; + } + + const status = await pythonEnvManager.initialize(autoBuildSource); + if (!status.ready) { + return `Python environment not ready: ${status.error || 'Unknown error'}`; + } + + return undefined; +} + +/** + * Generic retry wrapper with exponential backoff + * @param operation - Async function to execute with retry + * @param options - Retry configuration options + * @returns Result of the operation or throws after all retries + */ +async function withRetry( + operation: () => Promise, + options: { + maxRetries?: number; + baseDelayMs?: number; + onRetry?: (attempt: number, error: unknown) => void; + shouldRetry?: (error: unknown) => boolean; + } = {} +): Promise { + const { maxRetries: rawMaxRetries = 3, baseDelayMs = 100, onRetry, shouldRetry } = options; + + // Ensure at least one attempt is made (clamp to minimum of 1) + const maxRetries = Math.max(1, rawMaxRetries); + + for (let attempt = 1; attempt <= maxRetries; attempt++) { + try { + return await operation(); + } catch (error) { + const isLastAttempt = attempt === maxRetries; + + // Check if we should retry this error + if (shouldRetry && !shouldRetry(error)) { + throw error; + } + + if (isLastAttempt) { + throw error; + } + + // Notify about retry + onRetry?.(attempt, error); + + // Wait before retry (exponential backoff) + await new Promise(r => setTimeout(r, baseDelayMs * Math.pow(2, attempt - 1))); + } + } + + // This should never be reached, but TypeScript needs it + throw new Error('Retry loop exited unexpectedly'); +} + /** * Register worktree management handlers */ @@ -1333,17 +1654,10 @@ export function registerWorktreeHandlers( encoding: 'utf-8' }).trim(); - // Get base branch - the current branch in the main project (where changes will be merged) - // This matches the Python merge logic which merges into the user's current branch - let baseBranch = 'main'; - try { - baseBranch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: project.path, - encoding: 'utf-8' - }).trim(); - } catch { - baseBranch = 'main'; - } + // Get base branch using proper fallback chain: + // 1. Task metadata baseBranch, 2. Project settings mainBranch, 3. main/master detection + // Note: We do NOT use current HEAD as that may be a feature branch + const baseBranch = getEffectiveBaseBranch(project.path, task.specId, project.settings?.mainBranch); // Get commit count (cross-platform - no shell syntax) let commitCount = 0; @@ -1432,16 +1746,10 @@ export function registerWorktreeHandlers( return { success: false, error: 'No worktree found for this task' }; } - // Get base branch - the current branch in the main project (where changes will be merged) - let baseBranch = 'main'; - try { - baseBranch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: project.path, - encoding: 'utf-8' - }).trim(); - } catch { - baseBranch = 'main'; - } + // Get base branch using proper fallback chain: + // 1. Task metadata baseBranch, 2. Project settings mainBranch, 3. main/master detection + // Note: We do NOT use current HEAD as that may be a feature branch + const baseBranch = getEffectiveBaseBranch(project.path, task.specId, project.settings?.mainBranch); // Get the diff with file stats const files: WorktreeDiffFile[] = []; @@ -1516,14 +1824,15 @@ export function registerWorktreeHandlers( ipcMain.handle( IPC_CHANNELS.TASK_WORKTREE_MERGE, async (_, taskId: string, options?: { noCommit?: boolean }): Promise> => { - // Always log merge operations for debugging + const isDebugMode = process.env.DEBUG === 'true' || process.env.NODE_ENV === 'development'; const debug = (...args: unknown[]) => { - console.warn('[MERGE DEBUG]', ...args); + if (isDebugMode) { + console.warn('[MERGE DEBUG]', ...args); + } }; try { - console.warn('[MERGE] Handler called with taskId:', taskId, 'options:', options); - debug('Starting merge for taskId:', taskId, 'options:', options); + debug('Handler called with taskId:', taskId, 'options:', options); // Ensure Python environment is ready if (!pythonEnvManager.isEnvReady()) { @@ -1628,10 +1937,10 @@ export function registerWorktreeHandlers( const taskBaseBranch = getTaskBaseBranch(specDir); const projectMainBranch = project.settings?.mainBranch; const effectiveBaseBranch = taskBaseBranch || projectMainBranch; - + if (effectiveBaseBranch) { args.push('--base-branch', effectiveBaseBranch); - debug('Using base branch:', effectiveBaseBranch, + debug('Using base branch:', effectiveBaseBranch, `(source: ${taskBaseBranch ? 'task metadata' : 'project settings'})`); } @@ -1940,50 +2249,54 @@ export function registerWorktreeHandlers( const { promises: fsPromises } = require('fs'); - // Fire and forget - don't block the response on file writes - // But add retry logic for transient failures and verification + // Update plan file with retry logic for transient failures // Uses EAFP pattern (try/catch) instead of LBYL (existsSync check) to avoid TOCTOU race conditions - const updatePlanWithRetry = async (planPath: string, isMain: boolean, maxRetries = 3) => { - for (let attempt = 1; attempt <= maxRetries; attempt++) { - try { - const planContent = await fsPromises.readFile(planPath, 'utf-8'); - const plan = JSON.parse(planContent); - plan.status = newStatus; - plan.planStatus = planStatus; - plan.updated_at = new Date().toISOString(); - if (staged) { - plan.stagedAt = new Date().toISOString(); - plan.stagedInMainProject = true; - } - await fsPromises.writeFile(planPath, JSON.stringify(plan, null, 2)); + const updatePlanWithRetry = async (planPath: string, isMain: boolean): Promise => { + // Helper to check if error is ENOENT (file not found) + const isFileNotFound = (err: unknown): boolean => + !!(err && typeof err === 'object' && 'code' in err && err.code === 'ENOENT'); - // Verify the write succeeded by reading back - const verifyContent = await fsPromises.readFile(planPath, 'utf-8'); - const verifyPlan = JSON.parse(verifyContent); - if (verifyPlan.status === newStatus && verifyPlan.planStatus === planStatus) { - return true; // Write verified - } - throw new Error('Write verification failed - status mismatch'); - } catch (persistError: unknown) { - // File doesn't exist - nothing to update (not an error) - if (persistError && typeof persistError === 'object' && 'code' in persistError && persistError.code === 'ENOENT') { - return true; - } - const isLastAttempt = attempt === maxRetries; - if (isLastAttempt) { - // Only log error if main plan fails; worktree plan might legitimately be missing or read-only - if (isMain) { - console.error('Failed to persist task status to main plan after retries:', persistError); - } else { - debug('Failed to persist task status to worktree plan (non-critical):', persistError); + try { + await withRetry( + async () => { + const planContent = await fsPromises.readFile(planPath, 'utf-8'); + const plan = JSON.parse(planContent); + plan.status = newStatus; + plan.planStatus = planStatus; + plan.updated_at = new Date().toISOString(); + if (staged) { + plan.stagedAt = new Date().toISOString(); + plan.stagedInMainProject = true; } - return false; + await fsPromises.writeFile(planPath, JSON.stringify(plan, null, 2)); + + // Verify the write succeeded by reading back + const verifyContent = await fsPromises.readFile(planPath, 'utf-8'); + const verifyPlan = JSON.parse(verifyContent); + if (verifyPlan.status !== newStatus || verifyPlan.planStatus !== planStatus) { + throw new Error('Write verification failed - status mismatch'); + } + }, + { + maxRetries: 3, + baseDelayMs: 100, + shouldRetry: (err) => !isFileNotFound(err) // Don't retry if file doesn't exist } - // Wait before retry (exponential backoff: 100ms, 200ms, 400ms) - await new Promise(r => setTimeout(r, 100 * Math.pow(2, attempt - 1))); + ); + return true; + } catch (err) { + // File doesn't exist - nothing to update (not an error) + if (isFileNotFound(err)) { + return true; } + // Only log error if main plan fails; worktree plan might legitimately be missing or read-only + if (isMain) { + console.error('Failed to persist task status to main plan after retries:', err); + } else { + debug('Failed to persist task status to worktree plan (non-critical):', err); + } + return false; } - return false; }; const updatePlans = async () => { @@ -2160,10 +2473,10 @@ export function registerWorktreeHandlers( const taskBaseBranch = getTaskBaseBranch(specDir); const projectMainBranch = project.settings?.mainBranch; const effectiveBaseBranch = taskBaseBranch || projectMainBranch; - + if (effectiveBaseBranch) { args.push('--base-branch', effectiveBaseBranch); - console.warn('[IPC] Using base branch for preview:', effectiveBaseBranch, + console.warn('[IPC] Using base branch for preview:', effectiveBaseBranch, `(source: ${taskBaseBranch ? 'task metadata' : 'project settings'})`); } @@ -2373,16 +2686,10 @@ export function registerWorktreeHandlers( encoding: 'utf-8' }).trim(); - // Get base branch - the current branch in the main project (where changes will be merged) - let baseBranch = 'main'; - try { - baseBranch = execFileSync(getToolPath('git'), ['rev-parse', '--abbrev-ref', 'HEAD'], { - cwd: project.path, - encoding: 'utf-8' - }).trim(); - } catch { - baseBranch = 'main'; - } + // Get base branch using proper fallback chain: + // 1. Task metadata baseBranch, 2. Project settings mainBranch, 3. main/master detection + // Note: We do NOT use current HEAD as that may be a feature branch + const baseBranch = getEffectiveBaseBranch(project.path, entry, project.settings?.mainBranch); // Get commit count (cross-platform - no shell syntax) let commitCount = 0; @@ -2536,4 +2843,276 @@ export function registerWorktreeHandlers( } } ); + + /** + * Create a Pull Request from the worktree branch + * Pushes the branch to origin and creates a GitHub PR using gh CLI + */ + ipcMain.handle( + IPC_CHANNELS.TASK_WORKTREE_CREATE_PR, + async (_, taskId: string, options?: WorktreeCreatePROptions): Promise> => { + const isDebugMode = process.env.DEBUG === 'true' || process.env.NODE_ENV === 'development'; + const debug = (...args: unknown[]) => { + if (isDebugMode) { + console.warn('[CREATE_PR DEBUG]', ...args); + } + }; + + try { + debug('Handler called with taskId:', taskId, 'options:', options); + + // Ensure Python environment is ready + const pythonEnvError = await initializePythonEnvForPR(pythonEnvManager); + if (pythonEnvError) { + return { success: false, error: pythonEnvError }; + } + + const { task, project } = findTaskAndProject(taskId); + if (!task || !project) { + debug('Task or project not found'); + return { success: false, error: 'Task not found' }; + } + + debug('Found task:', task.specId, 'project:', project.path); + + // Use run.py --create-pr to handle the PR creation + const sourcePath = getEffectiveSourcePath(); + if (!sourcePath) { + return { success: false, error: 'Auto Claude source not found' }; + } + + const runScript = path.join(sourcePath, 'run.py'); + const specDir = path.join(project.path, project.autoBuildPath || '.auto-claude', 'specs', task.specId); + + // Use EAFP pattern - try to read specDir and catch ENOENT + try { + statSync(specDir); + } catch (err) { + if (err && typeof err === 'object' && 'code' in err && err.code === 'ENOENT') { + debug('Spec directory not found:', specDir); + return { success: false, error: 'Spec directory not found' }; + } + throw err; // Re-throw unexpected errors + } + + // Check worktree exists before creating PR + const worktreePath = findTaskWorktree(project.path, task.specId); + if (!worktreePath) { + debug('No worktree found for spec:', task.specId); + return { success: false, error: 'No worktree found for this task' }; + } + debug('Worktree path:', worktreePath); + + // Build arguments using helper function + const taskBaseBranch = getTaskBaseBranch(specDir); + const { args, validationError } = buildCreatePRArgs( + runScript, + task.specId, + project.path, + options, + taskBaseBranch + ); + if (validationError) { + return { success: false, error: validationError }; + } + if (taskBaseBranch) { + debug('Using stored base branch:', taskBaseBranch); + } + + // Use configured Python path + const pythonPath = getConfiguredPythonPath(); + debug('Running command:', pythonPath, args.join(' ')); + debug('Working directory:', sourcePath); + + // Get profile environment with OAuth token + const profileEnv = getProfileEnv(); + + return new Promise((resolve) => { + let timeoutId: NodeJS.Timeout | null = null; + let resolved = false; + + // Get Python environment for bundled packages + const pythonEnv = pythonEnvManagerSingleton.getPythonEnv(); + + // Parse Python command to handle space-separated commands like "py -3" + const [pythonCommand, pythonBaseArgs] = parsePythonCommand(pythonPath); + const createPRProcess = spawn(pythonCommand, [...pythonBaseArgs, ...args], { + cwd: sourcePath, + env: { + ...process.env, + ...pythonEnv, + ...profileEnv, + PYTHONUNBUFFERED: '1', + PYTHONUTF8: '1' + }, + stdio: ['ignore', 'pipe', 'pipe'] + }); + + let stdout = ''; + let stderr = ''; + + // Set up timeout to kill hung processes + timeoutId = setTimeout(() => { + if (!resolved) { + debug('TIMEOUT: Create PR process exceeded', PR_CREATION_TIMEOUT_MS, 'ms, killing...'); + resolved = true; + + // Platform-specific process termination with fallback + if (process.platform === 'win32') { + try { + createPRProcess.kill(); + // Fallback: forcefully kill with taskkill if process ignores initial kill + if (createPRProcess.pid) { + setTimeout(() => { + try { + spawn('taskkill', ['/pid', createPRProcess.pid!.toString(), '/f', '/t'], { + stdio: 'ignore', + detached: true + }).unref(); + } catch { + // Process may already be dead + } + }, 5000); + } + } catch { + // Process may already be dead + } + } else { + createPRProcess.kill('SIGTERM'); + setTimeout(() => { + try { + createPRProcess.kill('SIGKILL'); + } catch { + // Process may already be dead + } + }, 5000); + } + + resolve({ + success: false, + error: 'PR creation timed out. Check if the PR was created on GitHub.' + }); + } + }, PR_CREATION_TIMEOUT_MS); + + createPRProcess.stdout.on('data', (data: Buffer) => { + const chunk = data.toString(); + stdout += chunk; + debug('STDOUT:', chunk); + }); + + createPRProcess.stderr.on('data', (data: Buffer) => { + const chunk = data.toString(); + stderr += chunk; + debug('STDERR:', chunk); + }); + + /** + * Handle process exit - shared logic for both 'close' and 'exit' events. + * Parses JSON output, updates task status if PR was created, and resolves the promise. + * + * @param code - Process exit code (0 = success, non-zero = failure) + * @param eventSource - Which event triggered this ('close' or 'exit') for debug logging + */ + const handleCreatePRProcessExit = async (code: number | null, eventSource: 'close' | 'exit'): Promise => { + if (resolved) return; + resolved = true; + if (timeoutId) clearTimeout(timeoutId); + + debug(`Process exited via ${eventSource} event with code:`, code); + debug('Full stdout:', stdout); + debug('Full stderr:', stderr); + + if (code === 0) { + // Parse JSON output using helper function + const result = parsePRJsonOutput(stdout); + if (result) { + debug('Parsed result:', result); + + // Only update task status if a NEW PR was created (not if it already exists) + if (result.success !== false && result.prUrl && !result.alreadyExists) { + await updateTaskStatusAfterPRCreation( + specDir, + worktreePath, + result.prUrl, + project.autoBuildPath, + task.specId, + debug + ); + } else if (result.alreadyExists) { + debug('PR already exists, not updating task status'); + } + + resolve({ + success: true, + data: { + success: result.success, + prUrl: result.prUrl, + error: result.error, + alreadyExists: result.alreadyExists + } + }); + } else { + // No JSON found, but process succeeded + debug('No JSON in output, assuming success'); + resolve({ + success: true, + data: { + success: true, + prUrl: undefined + } + }); + } + } else { + debug('Process failed with code:', code); + + // Try to parse JSON from stdout even on failure + const result = parsePRJsonOutput(stdout); + if (result) { + debug('Parsed error result:', result); + resolve({ + success: false, + error: result.error || 'Failed to create PR' + }); + } else { + // Fallback to raw output if JSON parsing fails + // Prefer stdout over stderr since stderr often contains debug messages + resolve({ + success: false, + error: stdout || stderr || 'Failed to create PR' + }); + } + } + }; + + createPRProcess.on('close', (code: number | null) => { + handleCreatePRProcessExit(code, 'close'); + }); + + // Also listen to 'exit' event in case 'close' doesn't fire + createPRProcess.on('exit', (code: number | null) => { + // Give close event a chance to fire first with complete output + setTimeout(() => handleCreatePRProcessExit(code, 'exit'), 100); + }); + + createPRProcess.on('error', (err: Error) => { + if (resolved) return; + resolved = true; + if (timeoutId) clearTimeout(timeoutId); + debug('Process spawn error:', err); + resolve({ + success: false, + error: `Failed to run create-pr: ${err.message}` + }); + }); + }); + } catch (error) { + console.error('[CREATE_PR] Exception in handler:', error); + return { + success: false, + error: error instanceof Error ? error.message : 'Failed to create PR' + }; + } + } + ); } diff --git a/apps/frontend/src/main/memory-service.ts b/apps/frontend/src/main/memory-service.ts index c7dd4ccd..9e419cc8 100644 --- a/apps/frontend/src/main/memory-service.ts +++ b/apps/frontend/src/main/memory-service.ts @@ -139,7 +139,7 @@ function getBackendPythonPath(): string { const venvPython = process.platform === 'win32' ? path.join(backendPath, '.venv', 'Scripts', 'python.exe') : path.join(backendPath, '.venv', 'bin', 'python'); - + if (fs.existsSync(venvPython)) { console.log(`[MemoryService] Using backend venv Python: ${venvPython}`); return venvPython; @@ -159,7 +159,7 @@ function getBackendPythonPath(): string { function getMemoryPythonEnv(): Record { // Start with the standard Python environment from the manager const baseEnv = pythonEnvManager.getPythonEnv(); - + // For packaged apps, ensure PYTHONPATH includes bundled site-packages // even if the manager hasn't been fully initialized if (app.isPackaged) { @@ -172,7 +172,7 @@ function getMemoryPythonEnv(): Record { : bundledSitePackages; } } - + return baseEnv; } @@ -627,10 +627,10 @@ export class MemoryService { /** * Add an episode to the memory database - * + * * This allows the Electron app to save memories (like PR review insights) * directly to LadybugDB without going through the full Graphiti system. - * + * * @param name Episode name/title * @param content Episode content (will be JSON stringified if object) * @param episodeType Type of episode (session_insight, pattern, gotcha, task_outcome, pr_review) diff --git a/apps/frontend/src/main/project-store.ts b/apps/frontend/src/main/project-store.ts index dedc374f..a3153211 100644 --- a/apps/frontend/src/main/project-store.ts +++ b/apps/frontend/src/main/project-store.ts @@ -564,6 +564,7 @@ export class ProjectStore { 'done': 'done', 'human_review': 'human_review', 'ai_review': 'ai_review', + 'pr_created': 'pr_created', // PR has been created for this task 'backlog': 'backlog' }; const storedStatus = statusMap[plan.status]; @@ -573,6 +574,11 @@ export class ProjectStore { return { status: 'done' }; } + // If task has a PR created, always respect that status + if (storedStatus === 'pr_created') { + return { status: 'pr_created' }; + } + // For other stored statuses, validate against calculated status if (storedStatus) { // Planning/coding status from the backend should be respected even if subtasks aren't in progress yet diff --git a/apps/frontend/src/main/services/profile/profile-manager.ts b/apps/frontend/src/main/services/profile/profile-manager.ts index 83029f4b..6cb1bf1f 100644 --- a/apps/frontend/src/main/services/profile/profile-manager.ts +++ b/apps/frontend/src/main/services/profile/profile-manager.ts @@ -124,7 +124,7 @@ export async function saveProfilesFile(data: ProfilesFile): Promise { // Write file with formatted JSON const content = JSON.stringify(data, null, 2); await fs.writeFile(filePath, content, 'utf-8'); - + // Set secure file permissions (user read/write only - 0600) const permissionsValid = await validateFilePermissions(filePath); if (!permissionsValid) { @@ -167,17 +167,17 @@ export async function validateFilePermissions(filePath: string): Promise(fn: () => Promise): Promise { const filePath = getProfilesFilePath(); const dir = path.dirname(filePath); - + // Ensure directory and file exist before trying to lock await fs.mkdir(dir, { recursive: true }); - + // Create file if it doesn't exist (needed for lockfile to work) try { await fs.access(filePath); @@ -194,7 +194,7 @@ export async function withProfilesLock(fn: () => Promise): Promise { // EEXIST means another process won the race, proceed normally } } - + // Acquire lock with reasonable timeout let release: (() => Promise) | undefined; try { @@ -205,7 +205,7 @@ export async function withProfilesLock(fn: () => Promise): Promise { maxTimeout: 500 } }); - + // Execute the function while holding the lock return await fn(); } finally { @@ -219,7 +219,7 @@ export async function withProfilesLock(fn: () => Promise): Promise { /** * Atomically modify the profiles file * Loads, modifies, and saves the file within an exclusive lock - * + * * @param modifier Function that modifies the ProfilesFile * @returns The modified ProfilesFile */ @@ -229,25 +229,25 @@ export async function atomicModifyProfiles( return await withProfilesLock(async () => { // Load current state const file = await loadProfilesFile(); - + // Apply modification const modifiedFile = await modifier(file); - + // Save atomically (write to temp file and rename) const filePath = getProfilesFilePath(); const tempPath = `${filePath}.tmp`; - + try { // Write to temp file const content = JSON.stringify(modifiedFile, null, 2); await fs.writeFile(tempPath, content, 'utf-8'); - + // Set permissions on temp file await fs.chmod(tempPath, 0o600); - + // Atomically replace original file await fs.rename(tempPath, filePath); - + return modifiedFile; } catch (error) { // Clean up temp file on error diff --git a/apps/frontend/src/main/services/profile/profile-service.ts b/apps/frontend/src/main/services/profile/profile-service.ts index f3902049..c57f60ce 100644 --- a/apps/frontend/src/main/services/profile/profile-service.ts +++ b/apps/frontend/src/main/services/profile/profile-service.ts @@ -71,7 +71,7 @@ export function validateApiKey(apiKey: string): boolean { /** * Validate that profile name is unique (case-insensitive, trimmed) - * + * * WARNING: This is for UX feedback only. Do NOT rely on this for correctness. * The actual uniqueness check happens atomically inside create/update operations * to prevent TOCTOU race conditions. @@ -144,7 +144,7 @@ export async function createProfile(input: CreateProfileInput): Promise p.name.trim().toLowerCase() === trimmed ); - + if (exists) { throw new Error('A profile with this name already exists'); } @@ -420,10 +420,10 @@ export async function testConnection( const messagesErrorName = messagesError instanceof Error ? messagesError.name : ''; // 400/422 errors mean the endpoint is valid, just our test request was invalid // This is expected - we're just testing connectivity - if (messagesErrorName === 'BadRequestError' || + if (messagesErrorName === 'BadRequestError' || messagesErrorName === 'InvalidRequestError' || - (messagesError instanceof Error && 'status' in messagesError && - ((messagesError as { status?: number }).status === 400 || + (messagesError instanceof Error && 'status' in messagesError && + ((messagesError as { status?: number }).status === 400 || (messagesError as { status?: number }).status === 422))) { // Endpoint is valid, connection successful return { @@ -452,7 +452,7 @@ export async function testConnection( // Map SDK errors to TestConnectionResult error types // Use error.name for instanceof-like checks (works with mocks that set this.name) const errorName = error instanceof Error ? error.name : ''; - + if (errorName === 'AuthenticationError' || error instanceof AuthenticationError) { return { success: false, @@ -580,7 +580,7 @@ export async function discoverModels( // Map SDK errors to thrown errors with errorType property // Use error.name for instanceof-like checks (works with mocks that set this.name) const errorName = error instanceof Error ? error.name : ''; - + if (errorName === 'AuthenticationError' || error instanceof AuthenticationError) { const authError: Error & { errorType?: string } = new Error('Authentication failed. Please check your API key.'); authError.errorType = 'auth'; 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 739b58bd..92090e0b 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 @@ -1,8 +1,12 @@ import { writeFileSync } from 'fs'; +import { tmpdir } from 'os'; import { describe, expect, it, vi, beforeEach } from 'vitest'; import type * as pty from '@lydell/node-pty'; import type { TerminalProcess } from '../types'; +/** Escape special regex characters in a string for safe use in RegExp constructor */ +const escapeForRegex = (str: string): string => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const mockGetClaudeCliInvocation = vi.fn(); const mockGetClaudeProfileManager = vi.fn(); const mockPersistSession = vi.fn(); @@ -229,7 +233,8 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - expect(tokenPath).toMatch(/^\/tmp\/\.claude-token-1234-[0-9a-f]{16}$/); + const tmpDir = escapeForRegex(tmpdir()); + expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-1234-[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 "); @@ -271,7 +276,8 @@ describe('claude-integration-handler', () => { const tokenPath = vi.mocked(writeFileSync).mock.calls[0]?.[0] as string; const tokenContents = vi.mocked(writeFileSync).mock.calls[0]?.[1] as string; - expect(tokenPath).toMatch(/^\/tmp\/\.claude-token-5678-[0-9a-f]{16}$/); + const tmpDir = escapeForRegex(tmpdir()); + expect(tokenPath).toMatch(new RegExp(`^${tmpDir}/\\.claude-token-5678-[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}'`); diff --git a/apps/frontend/src/preload/api/profile-api.ts b/apps/frontend/src/preload/api/profile-api.ts index e285c6f1..69b27890 100644 --- a/apps/frontend/src/preload/api/profile-api.ts +++ b/apps/frontend/src/preload/api/profile-api.ts @@ -84,7 +84,7 @@ export const createProfileAPI = (): ProfileAPI => ({ if (signal && signal.aborted) { return Promise.reject(new DOMException('The operation was aborted.', 'AbortError')); } - + // Setup abort listener AFTER checking aborted status to avoid race condition if (signal && typeof signal.addEventListener === 'function') { try { @@ -97,7 +97,7 @@ export const createProfileAPI = (): ProfileAPI => ({ } else if (signal) { console.warn('[preload/profile-api] signal provided but addEventListener not available - signal may have been serialized'); } - + return ipcRenderer.invoke(IPC_CHANNELS.PROFILES_TEST_CONNECTION, baseUrl, apiKey, requestId); }, diff --git a/apps/frontend/src/preload/api/task-api.ts b/apps/frontend/src/preload/api/task-api.ts index c2f3a678..27e55dd1 100644 --- a/apps/frontend/src/preload/api/task-api.ts +++ b/apps/frontend/src/preload/api/task-api.ts @@ -11,7 +11,9 @@ import type { TaskLogs, TaskLogStreamChunk, SupportedIDE, - SupportedTerminal + SupportedTerminal, + WorktreeCreatePROptions, + WorktreeCreatePRResult } from '../../shared/types'; export interface TaskAPI { @@ -57,6 +59,7 @@ export interface TaskAPI { worktreeDetectTools: () => Promise; terminals: Array<{ id: string; name: string; path: string; installed: boolean }> }>>; archiveTasks: (projectId: string, taskIds: string[], version?: string) => Promise>; unarchiveTasks: (projectId: string, taskIds: string[]) => Promise>; + createWorktreePR: (taskId: string, options?: WorktreeCreatePROptions) => Promise>; // Task Event Listeners // Note: projectId is optional for backward compatibility - events without projectId will still work @@ -160,6 +163,9 @@ export const createTaskAPI = (): TaskAPI => ({ unarchiveTasks: (projectId: string, taskIds: string[]): Promise> => ipcRenderer.invoke(IPC_CHANNELS.TASK_UNARCHIVE, projectId, taskIds), + createWorktreePR: (taskId: string, options?: WorktreeCreatePROptions): Promise> => + ipcRenderer.invoke(IPC_CHANNELS.TASK_WORKTREE_CREATE_PR, taskId, options), + // Task Event Listeners onTaskProgress: ( callback: (taskId: string, plan: ImplementationPlan, projectId?: string) => void diff --git a/apps/frontend/src/renderer/components/KanbanBoard.tsx b/apps/frontend/src/renderer/components/KanbanBoard.tsx index 30fc3c10..69a4612c 100644 --- a/apps/frontend/src/renderer/components/KanbanBoard.tsx +++ b/apps/frontend/src/renderer/components/KanbanBoard.tsx @@ -30,6 +30,12 @@ import { cn } from '../lib/utils'; import { persistTaskStatus, archiveTasks } from '../stores/task-store'; import type { Task, TaskStatus } from '../../shared/types'; +// Type guard for valid drop column targets - preserves literal type from TASK_STATUS_COLUMNS +const VALID_DROP_COLUMNS = new Set(TASK_STATUS_COLUMNS); +function isValidDropColumn(id: string): id is typeof TASK_STATUS_COLUMNS[number] { + return VALID_DROP_COLUMNS.has(id); +} + interface KanbanBoardProps { tasks: Task[]; onTaskClick: (task: Task) => void; @@ -356,7 +362,8 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR ); const tasksByStatus = useMemo(() => { - const grouped: Record = { + // Note: pr_created tasks are shown in the 'done' column since they're essentially complete + const grouped: Record = { backlog: [], in_progress: [], ai_review: [], @@ -365,14 +372,16 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR }; filteredTasks.forEach((task) => { - if (grouped[task.status]) { - grouped[task.status].push(task); + // Map pr_created tasks to the done column + const targetColumn = task.status === 'pr_created' ? 'done' : task.status; + if (grouped[targetColumn]) { + grouped[targetColumn].push(task); } }); // Sort tasks within each column by createdAt (newest first) Object.keys(grouped).forEach((status) => { - grouped[status as TaskStatus].sort((a, b) => { + grouped[status as typeof TASK_STATUS_COLUMNS[number]].sort((a, b) => { const dateA = new Date(a.createdAt).getTime(); const dateB = new Date(b.createdAt).getTime(); return dateB - dateA; // Descending order (newest first) @@ -418,7 +427,7 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR const overId = over.id as string; // Check if over a column - if (TASK_STATUS_COLUMNS.includes(overId as TaskStatus)) { + if (isValidDropColumn(overId)) { setOverColumnId(overId); return; } @@ -441,8 +450,8 @@ export function KanbanBoard({ tasks, onTaskClick, onNewTaskClick, onRefresh, isR const overId = over.id as string; // Check if dropped on a column - if (TASK_STATUS_COLUMNS.includes(overId as TaskStatus)) { - const newStatus = overId as TaskStatus; + if (isValidDropColumn(overId)) { + const newStatus = overId; const task = tasks.find((t) => t.id === activeTaskId); if (task && task.status !== newStatus) { diff --git a/apps/frontend/src/renderer/components/TaskCard.tsx b/apps/frontend/src/renderer/components/TaskCard.tsx index f07db15b..9e941a7d 100644 --- a/apps/frontend/src/renderer/components/TaskCard.tsx +++ b/apps/frontend/src/renderer/components/TaskCard.tsx @@ -1,6 +1,6 @@ import { useState, useEffect, useRef, useCallback, memo, useMemo } from 'react'; import { useTranslation } from 'react-i18next'; -import { Play, Square, Clock, Zap, Target, Shield, Gauge, Palette, FileCode, Bug, Wrench, Loader2, AlertTriangle, RotateCcw, Archive, MoreVertical } from 'lucide-react'; +import { Play, Square, Clock, Zap, Target, Shield, Gauge, Palette, FileCode, Bug, Wrench, Loader2, AlertTriangle, RotateCcw, Archive, GitPullRequest, MoreVertical } from 'lucide-react'; import { Card, CardContent } from './ui/card'; import { Badge } from './ui/badge'; import { Button } from './ui/button'; @@ -74,6 +74,7 @@ function taskCardPropsAreEqual(prevProps: TaskCardProps, nextProps: TaskCardProp prevTask.metadata?.category === nextTask.metadata?.category && prevTask.metadata?.complexity === nextTask.metadata?.complexity && prevTask.metadata?.archivedAt === nextTask.metadata?.archivedAt && + prevTask.metadata?.prUrl === nextTask.metadata?.prUrl && // Check if any subtask statuses changed (compare all subtasks) prevTask.subtasks.every((s, i) => s.status === nextTask.subtasks[i]?.status) ); @@ -247,6 +248,13 @@ export const TaskCard = memo(function TaskCard({ task, onClick, onStatusChange } } }; + const handleViewPR = (e: React.MouseEvent) => { + e.stopPropagation(); + if (task.metadata?.prUrl && window.electronAPI?.openExternal) { + window.electronAPI.openExternal(task.metadata.prUrl); + } + }; + const getStatusBadgeVariant = (status: string) => { switch (status) { case 'in_progress': @@ -255,6 +263,8 @@ export const TaskCard = memo(function TaskCard({ task, onClick, onStatusChange } return 'warning'; case 'human_review': return 'purple'; + case 'pr_created': + return 'success'; case 'done': return 'success'; default: @@ -270,6 +280,8 @@ export const TaskCard = memo(function TaskCard({ task, onClick, onStatusChange } return t('labels.aiReview'); case 'human_review': return t('labels.needsReview'); + case 'pr_created': + return t('columns.pr_created'); case 'done': return t('status.complete'); default: @@ -369,15 +381,26 @@ export const TaskCard = memo(function TaskCard({ task, onClick, onStatusChange } {EXECUTION_PHASE_LABELS[executionPhase]} )} - {/* Status badge - hide when execution phase badge is showing */} - {!hasActiveExecution && ( - - {isStuck ? t('labels.needsRecovery') : isIncomplete ? t('labels.needsResume') : getStatusLabel(task.status)} - - )} + {/* Status badge - hide when execution phase badge is showing */} + {!hasActiveExecution && ( + <> + {task.status === 'pr_created' ? ( + + {getStatusLabel(task.status)} + + ) : ( + + {isStuck ? t('labels.needsRecovery') : isIncomplete ? t('labels.needsResume') : getStatusLabel(task.status)} + + )} + + )} {/* Review reason badge - explains why task needs human review */} {reviewReasonInfo && !isStuck && !isIncomplete && ( {t('actions.resume')} + ) : task.status === 'pr_created' ? ( +
+ {task.metadata?.prUrl && ( + + )} + {!task.metadata?.archivedAt && ( + + )} +
) : task.status === 'done' && !task.metadata?.archivedAt ? ( + {task && ( + + )} + {task?.status === 'pr_created' && task.metadata?.prUrl && ( + + )} + )} ); } @@ -295,12 +357,12 @@ function TaskDetailModalContent({ open, task, onOpenChange, onSwitchToTerminals, ) : ( <> - - {t(TASK_STATUS_LABELS[task.status])} - + + {t(TASK_STATUS_LABELS[task.status])} + {task.status === 'human_review' && task.reviewReason && ( )} diff --git a/apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx b/apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx index 34e5180d..cfd0c1d0 100644 --- a/apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx +++ b/apps/frontend/src/renderer/components/task-detail/TaskMetadata.tsx @@ -1,3 +1,4 @@ +import { useTranslation } from 'react-i18next'; import { Target, Bug, @@ -9,8 +10,10 @@ import { Lightbulb, Users, GitBranch, + GitPullRequest, ListChecks, - Clock + Clock, + ExternalLink } from 'lucide-react'; import ReactMarkdown from 'react-markdown'; import remarkGfm from 'remark-gfm'; @@ -48,6 +51,7 @@ interface TaskMetadataProps { } export function TaskMetadata({ task }: TaskMetadataProps) { + const { t } = useTranslation(['tasks']); const hasClassification = task.metadata && ( task.metadata.category || task.metadata.priority || @@ -139,7 +143,7 @@ export function TaskMetadata({ task }: TaskMetadataProps) { {/* Description - Primary Content */} {task.description && (
-
@@ -201,6 +205,24 @@ export function TaskMetadata({ task }: TaskMetadataProps) {
)} + {/* Pull Request */} + {task.metadata.prUrl && ( +
+

+ + {t('tasks:metadata.pullRequest')} +

+ +
+ )} + {/* Acceptance Criteria */} {task.metadata.acceptanceCriteria && task.metadata.acceptanceCriteria.length > 0 && (
diff --git a/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx b/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx index 7dd2d1ca..de60d8d0 100644 --- a/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx +++ b/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx @@ -1,4 +1,4 @@ -import type { Task, WorktreeStatus, WorktreeDiff, MergeConflict, MergeStats, GitConflictInfo } from '../../../shared/types'; +import type { Task, WorktreeStatus, WorktreeDiff, MergeConflict, MergeStats, GitConflictInfo, WorktreeCreatePRResult } from '../../../shared/types'; import { StagedSuccessMessage, WorkspaceStatus, @@ -8,7 +8,8 @@ import { ConflictDetailsDialog, LoadingMessage, NoWorkspaceMessage, - StagedInProjectMessage + StagedInProjectMessage, + CreatePRDialog } from './task-review'; interface TaskReviewProps { @@ -42,6 +43,11 @@ interface TaskReviewProps { onClose?: () => void; onSwitchToTerminals?: () => void; onOpenInbuiltTerminal?: (id: string, cwd: string) => void; + // PR creation + showPRDialog: boolean; + isCreatingPR: boolean; + onShowPRDialog: (show: boolean) => void; + onCreatePR: (options: { targetBranch?: string; title?: string; draft?: boolean }) => Promise; } /** @@ -83,7 +89,11 @@ export function TaskReview({ onLoadMergePreview, onClose, onSwitchToTerminals, - onOpenInbuiltTerminal + onOpenInbuiltTerminal, + showPRDialog, + isCreatingPR, + onShowPRDialog, + onCreatePR }: TaskReviewProps) { return (
@@ -110,12 +120,14 @@ export function TaskReview({ isLoadingPreview={isLoadingPreview} isMerging={isMerging} isDiscarding={isDiscarding} + isCreatingPR={isCreatingPR} onShowDiffDialog={onShowDiffDialog} onShowDiscardDialog={onShowDiscardDialog} onShowConflictDialog={onShowConflictDialog} onLoadMergePreview={onLoadMergePreview} onStageOnlyChange={onStageOnlyChange} onMerge={onMerge} + onShowPRDialog={onShowPRDialog} onClose={onClose} onSwitchToTerminals={onSwitchToTerminals} onOpenInbuiltTerminal={onOpenInbuiltTerminal} @@ -164,6 +176,15 @@ export function TaskReview({ onOpenChange={onShowConflictDialog} onMerge={onMerge} /> + + {/* Create PR Dialog */} +
); } diff --git a/apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts b/apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts index 5ff22412..b4f3b5df 100644 --- a/apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts +++ b/apps/frontend/src/renderer/components/task-detail/hooks/useTaskDetail.ts @@ -46,6 +46,8 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { } | null>(null); const [isLoadingPreview, setIsLoadingPreview] = useState(false); const [showConflictDialog, setShowConflictDialog] = useState(false); + const [showPRDialog, setShowPRDialog] = useState(false); + const [isCreatingPR, setIsCreatingPR] = useState(false); const selectedProject = useProjectStore((state) => state.getSelectedProject()); const isRunning = task.status === 'in_progress'; @@ -282,6 +284,8 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { mergePreview, isLoadingPreview, showConflictDialog, + showPRDialog, + isCreatingPR, // Setters setFeedback, @@ -313,6 +317,8 @@ export function useTaskDetail({ task }: UseTaskDetailOptions) { setMergePreview, setIsLoadingPreview, setShowConflictDialog, + setShowPRDialog, + setIsCreatingPR, // Handlers handleLogsScroll, diff --git a/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.test.tsx b/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.test.tsx new file mode 100644 index 00000000..17add18f --- /dev/null +++ b/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.test.tsx @@ -0,0 +1,409 @@ +/** + * @vitest-environment jsdom + */ +/** + * CreatePRDialog Tests + * + * Tests the Create PR dialog component functionality. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import '../../../../shared/i18n'; +import { CreatePRDialog } from './CreatePRDialog'; +import type { Task, WorktreeStatus } from '../../../../shared/types'; + +// Mock electronAPI +vi.mock('../../../../preload/api', () => ({})); + +// Mock window.electronAPI +const mockOpenExternal = vi.fn(); +Object.defineProperty(window, 'electronAPI', { + value: { + openExternal: mockOpenExternal + }, + writable: true +}); + +describe('CreatePRDialog', () => { + const mockOnOpenChange = vi.fn(); + const mockOnCreatePR = vi.fn(); + + const mockTask: Task = { + id: 'task-123', + specId: 'spec-123', + projectId: 'project-123', + title: 'Implement user authentication', + description: 'Add login and registration functionality', + status: 'human_review', + subtasks: [], + logs: [], + createdAt: new Date(), + updatedAt: new Date() + }; + + const mockWorktreeStatus: WorktreeStatus = { + exists: true, + worktreePath: '/path/to/worktree', + branch: 'auto-claude/implement-user-authentication', + baseBranch: 'develop', + commitCount: 5, + filesChanged: 10, + additions: 200, + deletions: 50 + }; + + beforeEach(() => { + vi.clearAllMocks(); + mockOnCreatePR.mockResolvedValue({ success: true, prUrl: 'https://github.com/test/pr/1' }); + }); + + it('should render dialog when open', async () => { + render( + + ); + + await waitFor(() => { + // Check for the dialog title (h2 element) + expect(screen.getByRole('heading', { name: /create pull request/i })).toBeInTheDocument(); + }); + }); + + it('should default PR title to task title', async () => { + render( + + ); + + await waitFor(() => { + const titleInput = screen.getByLabelText(/pr title/i); + expect(titleInput).toHaveValue('Implement user authentication'); + }); + }); + + it('should default target branch to worktree base branch', async () => { + render( + + ); + + await waitFor(() => { + const branchInput = screen.getByLabelText(/target branch/i); + expect(branchInput).toHaveValue('develop'); + }); + }); + + it('should display source branch info', async () => { + render( + + ); + + await waitFor(() => { + expect(screen.getByText('auto-claude/implement-user-authentication')).toBeInTheDocument(); + }); + }); + + it('should display commit count and changes', async () => { + render( + + ); + + await waitFor(() => { + // Use data-testid for stable test targeting + const statsContainer = screen.getByTestId('pr-stats-container'); + expect(statsContainer).toBeInTheDocument(); + + // Scope assertions to the stats container to avoid accidental matches elsewhere + const stats = within(statsContainer); + expect(stats.getByText('5')).toBeInTheDocument(); // commit count + expect(stats.getByText('+200')).toBeInTheDocument(); // additions + expect(stats.getByText('-50')).toBeInTheDocument(); // deletions + }); + }); + + it('should call onCreatePR with form values when Create PR is clicked', async () => { + render( + + ); + + await waitFor(() => { + expect(screen.getByLabelText(/pr title/i)).toHaveValue('Implement user authentication'); + }); + + // Find the submit button (not the heading) - it's the one with "Create Pull Request" text inside a button + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(mockOnCreatePR).toHaveBeenCalledWith({ + targetBranch: 'develop', + title: 'Implement user authentication', + draft: false + }); + }); + }); + + it('should allow modifying PR title before creating', async () => { + render( + + ); + + await waitFor(() => { + expect(screen.getByLabelText(/pr title/i)).toHaveValue('Implement user authentication'); + }); + + const titleInput = screen.getByLabelText(/pr title/i); + fireEvent.change(titleInput, { target: { value: 'Custom PR Title' } }); + + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(mockOnCreatePR).toHaveBeenCalledWith({ + targetBranch: 'develop', + title: 'Custom PR Title', + draft: false + }); + }); + }); + + it('should close dialog when Cancel is clicked', async () => { + render( + + ); + + const cancelButton = screen.getByRole('button', { name: /cancel/i }); + fireEvent.click(cancelButton); + + await waitFor(() => { + expect(mockOnOpenChange).toHaveBeenCalledWith(false); + }); + }); + + it('should show success state after PR is created', async () => { + mockOnCreatePR.mockResolvedValue({ + success: true, + prUrl: 'https://github.com/test/repo/pull/123' + }); + + render( + + ); + + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(screen.getByText('https://github.com/test/repo/pull/123')).toBeInTheDocument(); + }); + }); + + it('should show error state when PR creation fails', async () => { + mockOnCreatePR.mockResolvedValue({ + success: false, + error: 'Failed to push branch to remote' + }); + + render( + + ); + + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(screen.getByText('Failed to push branch to remote')).toBeInTheDocument(); + }); + }); + + it('should reset form when dialog is reopened', async () => { + const { rerender } = render( + + ); + + // Modify the title + await waitFor(() => { + expect(screen.getByLabelText(/pr title/i)).toHaveValue('Implement user authentication'); + }); + + const titleInput = screen.getByLabelText(/pr title/i); + fireEvent.change(titleInput, { target: { value: 'Modified Title' } }); + expect(titleInput).toHaveValue('Modified Title'); + + // Close dialog + rerender( + + ); + + // Reopen dialog + rerender( + + ); + + // Should reset to task title + await waitFor(() => { + expect(screen.getByLabelText(/pr title/i)).toHaveValue('Implement user authentication'); + }); + }); + + it('should call onCreatePR with draft: true when draft checkbox is checked', async () => { + render( + + ); + + await waitFor(() => { + expect(screen.getByLabelText(/pr title/i)).toHaveValue('Implement user authentication'); + }); + + // Find and click the draft checkbox + const draftCheckbox = screen.getByRole('checkbox'); + fireEvent.click(draftCheckbox); + + // Create the PR + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(mockOnCreatePR).toHaveBeenCalledWith({ + targetBranch: 'develop', + title: 'Implement user authentication', + draft: true + }); + }); + }); + + it('should show already exists message when PR already exists', async () => { + mockOnCreatePR.mockResolvedValue({ + success: true, + prUrl: 'https://github.com/test/repo/pull/456', + alreadyExists: true + }); + + render( + + ); + + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + expect(screen.getByText(/already exists/i)).toBeInTheDocument(); + expect(screen.getByText('https://github.com/test/repo/pull/456')).toBeInTheDocument(); + }); + }); + + it('should show success state without link when prUrl is undefined', async () => { + mockOnCreatePR.mockResolvedValue({ + success: true, + prUrl: undefined + }); + + render( + + ); + + const createButton = screen.getByRole('button', { name: /create pull request/i }); + fireEvent.click(createButton); + + await waitFor(() => { + // Should show success message but no link + expect(screen.getByText(/created/i)).toBeInTheDocument(); + // Should not have any PR link button (no prUrl to display) + expect(screen.queryByTestId('pr-link-button')).not.toBeInTheDocument(); + }); + }); +}); diff --git a/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.tsx b/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.tsx new file mode 100644 index 00000000..da524957 --- /dev/null +++ b/apps/frontend/src/renderer/components/task-detail/task-review/CreatePRDialog.tsx @@ -0,0 +1,274 @@ +import { useState, useEffect } from 'react'; +import { GitPullRequest, Loader2, ExternalLink } from 'lucide-react'; +import { useTranslation } from 'react-i18next'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '../../ui/dialog'; +import { Button } from '../../ui/button'; +import { Input } from '../../ui/input'; +import { Label } from '../../ui/label'; +import { Checkbox } from '../../ui/checkbox'; +import type { Task, WorktreeStatus, WorktreeCreatePRResult } from '../../../../shared/types'; + +interface CreatePRDialogProps { + open: boolean; + task: Task; + worktreeStatus: WorktreeStatus | null; + onOpenChange: (open: boolean) => void; + onCreatePR: (options: { targetBranch?: string; title?: string; draft?: boolean }) => Promise; +} + +/** + * Dialog for creating a Pull Request from a worktree branch + * Allows user to specify target branch, PR title, and draft status + */ +export function CreatePRDialog({ + open, + task, + worktreeStatus, + onOpenChange, + onCreatePR +}: CreatePRDialogProps) { + const { t } = useTranslation(['taskReview', 'common']); + const [targetBranch, setTargetBranch] = useState(''); + const [prTitle, setPrTitle] = useState(''); + const [isDraft, setIsDraft] = useState(false); + const [isCreating, setIsCreating] = useState(false); + const [result, setResult] = useState(null); + const [error, setError] = useState(null); + + // Reset state when dialog opens + useEffect(() => { + if (open) { + setTargetBranch(worktreeStatus?.baseBranch || ''); + setPrTitle(task.title); + setIsDraft(false); + setIsCreating(false); + setResult(null); + setError(null); + } + }, [open, worktreeStatus?.baseBranch, task.title]); + + // Frontend validation functions + const validateBranchName = (branch: string): string | null => { + if (!branch.trim()) return null; // Empty is OK, will use default + // Basic git branch name rules: no spaces, .., @{, \, etc. + if (!/^[a-zA-Z0-9/_-]+$/.test(branch)) { + return t('taskReview:pr.errors.invalidBranchName'); + } + return null; + }; + + const validatePRTitle = (title: string): string | null => { + if (!title.trim()) { + return t('taskReview:pr.errors.emptyTitle'); + } + return null; + }; + + const handleCreatePR = async () => { + // Frontend validation before submitting + const branchError = validateBranchName(targetBranch); + if (branchError) { + setError(branchError); + return; + } + + const titleError = validatePRTitle(prTitle); + if (titleError) { + setError(titleError); + return; + } + + setIsCreating(true); + setError(null); + setResult(null); + + try { + const prResult = await onCreatePR({ + targetBranch: targetBranch || undefined, + title: prTitle || undefined, + draft: isDraft + }); + + if (prResult) { + if (prResult.success) { + setResult(prResult); + } else { + setError(prResult.error || t('taskReview:pr.errors.unknown')); + } + } else { + setError(t('taskReview:pr.errors.unknown')); + } + } catch (err) { + setError(err instanceof Error ? err.message : t('taskReview:pr.errors.unknown')); + } finally { + setIsCreating(false); + } + }; + + const handleClose = () => { + onOpenChange(false); + }; + + const handleOpenPR = () => { + if (result?.prUrl && window.electronAPI?.openExternal) { + window.electronAPI.openExternal(result.prUrl); + } + }; + + return ( + + + + + + {t('taskReview:pr.title')} + + + {t('taskReview:pr.description', { taskTitle: task.title })} + + + + {/* Success State */} + {result?.success && ( +
+
+

+ {result.alreadyExists + ? t('taskReview:pr.success.alreadyExists') + : t('taskReview:pr.success.created')} +

+ {result.prUrl && ( + + )} +
+ + + +
+ )} + + {/* Error State */} + {error && !result?.success && ( +
+
+

{error}

+
+ + + + +
+ )} + + {/* Form State */} + {!result?.success && !error && ( +
+ {/* Branch Info */} +
+
+ {t('taskReview:pr.labels.sourceBranch')}: + {worktreeStatus?.branch || t('taskReview:pr.labels.unknown')} +
+ {worktreeStatus?.exists && ( + <> +
+ {t('taskReview:pr.labels.commits')}: + {worktreeStatus.commitCount || 0} +
+
+ {t('taskReview:pr.labels.changes')}: + + +{worktreeStatus.additions || 0} + {' / '} + -{worktreeStatus.deletions || 0} + +
+ + )} +
+ + {/* Target Branch */} +
+ + setTargetBranch(e.target.value)} + placeholder={worktreeStatus?.baseBranch || 'main'} + /> +

+ {t('taskReview:pr.hints.targetBranch')} +

+
+ + {/* PR Title (optional) */} +
+ + setPrTitle(e.target.value)} + placeholder={task.title} + /> +

+ {t('taskReview:pr.hints.prTitle')} +

+
+ + {/* Draft PR Checkbox */} +
+ setIsDraft(checked === true)} + /> + +
+ + + + + +
+ )} + + + ); +} diff --git a/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx b/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx index b474c914..a68de379 100644 --- a/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx +++ b/apps/frontend/src/renderer/components/task-detail/task-review/WorkspaceStatus.tsx @@ -5,6 +5,7 @@ import { Minus, Eye, GitMerge, + GitPullRequest, FolderX, Loader2, RotateCcw, @@ -14,6 +15,7 @@ import { Code, Terminal } from 'lucide-react'; +import { useTranslation } from 'react-i18next'; import { Button } from '../../ui/button'; import { Checkbox } from '../../ui/checkbox'; import { cn } from '../../../lib/utils'; @@ -28,12 +30,14 @@ interface WorkspaceStatusProps { isLoadingPreview: boolean; isMerging: boolean; isDiscarding: boolean; + isCreatingPR?: boolean; onShowDiffDialog: (show: boolean) => void; onShowDiscardDialog: (show: boolean) => void; onShowConflictDialog: (show: boolean) => void; onLoadMergePreview: () => void; onStageOnlyChange: (value: boolean) => void; onMerge: () => void; + onShowPRDialog?: (show: boolean) => void; onClose?: () => void; onSwitchToTerminals?: () => void; onOpenInbuiltTerminal?: (id: string, cwd: string) => void; @@ -84,16 +88,19 @@ export function WorkspaceStatus({ isLoadingPreview, isMerging, isDiscarding, + isCreatingPR, onShowDiffDialog, onShowDiscardDialog, onShowConflictDialog, onLoadMergePreview, onStageOnlyChange, onMerge, + onShowPRDialog, onClose, onSwitchToTerminals, onOpenInbuiltTerminal }: WorkspaceStatusProps) { + const { t } = useTranslation(['taskReview', 'common']); const { settings } = useSettingsStore(); const preferredIDE = settings.preferredIDE || 'vscode'; const preferredTerminal = settings.preferredTerminal || 'system'; @@ -433,11 +440,33 @@ export function WorkspaceStatus({ )} + {/* Create PR Button */} + {onShowPRDialog && ( + + )} +