From d16be307718a3a2767d109d2cf3d42ce0b6afa82 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Fri, 30 Jan 2026 13:20:34 +0100 Subject: [PATCH] Merge conflict resolution progress bar and log viewer (#1620) * auto-claude: subtask-1-3 - Thread progress callback through MergePipeline and ConflictResolver Add progress_callback parameter to MergePipeline.merge_file() and ConflictResolver.resolve_conflicts(). MergePipeline emits per-file progress at the start of merge within the resolving stage (50-75%). ConflictResolver emits per-conflict resolution progress with details about current file, conflict count, and conflicts resolved so far. Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-1-4 - Wire progress emission into CLI merge entry point. Add _create_merge_progress_callback() helper that returns emit_progress only when stdout is piped (subprocess mode from Electron), avoiding JSON pollution in interactive CLI sessions. Wire the callback into _try_smart_merge_inner() with progress emissions at key pipeline stages: ANALYZING, DETECTING_CONFLICTS, RESOLVING, COMPLETE, and ERROR. Co-Authored-By: Claude Opus 4.5 * Fix PR review issues: conflict counts, progress calculations, and cross-task leakage - Fix conflicts_found on COMPLETE/ERROR stages to use original conflict count - Fix off-by-one in progress percentage calculations (50-75% range) - Add JSON validation for MergeProgress before IPC transmission - Add taskId filtering to prevent cross-task progress event leakage - Limit log entries to 500 to prevent unbounded memory growth - Fix race condition: wait for terminal progress event before hiding overlay - Remove unused imports (ruff fixes) - Remove orphaned unreachable code in workspace.py - Fix test mocks to use optional config_dir argument Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- apps/backend/core/workspace.py | 18 ++++-- apps/backend/merge/conflict_resolver.py | 3 +- apps/backend/merge/orchestrator.py | 3 +- .../ipc-handlers/task/worktree-handlers.ts | 9 ++- .../components/task-detail/TaskReview.tsx | 1 + .../task-review/WorkspaceStatus.tsx | 56 ++++++++++++++----- tests/test_auth.py | 15 +++-- tests/test_client.py | 26 ++++----- 8 files changed, 90 insertions(+), 41 deletions(-) diff --git a/apps/backend/core/workspace.py b/apps/backend/core/workspace.py index 3551b41b..29a6b17f 100644 --- a/apps/backend/core/workspace.py +++ b/apps/backend/core/workspace.py @@ -589,12 +589,15 @@ def _try_smart_merge_inner( if progress_callback is not None: stats = resolution_result.get("stats", {}) + original_conflict_count = len( + git_conflicts.get("conflicting_files", []) + ) progress_callback( MergeProgressStage.COMPLETE, 100, "Merge complete", { - "conflicts_found": stats.get("conflicts_resolved", 0), + "conflicts_found": original_conflict_count, "conflicts_resolved": stats.get("conflicts_resolved", 0), }, ) @@ -613,14 +616,21 @@ def _try_smart_merge_inner( ) if progress_callback is not None: + original_conflict_count = len( + git_conflicts.get("conflicting_files", []) + ) + remaining_count = len( + resolution_result.get("remaining_conflicts", []) + ) progress_callback( MergeProgressStage.ERROR, 0, "Some conflicts could not be resolved", { - "conflicts_found": len( - resolution_result.get("remaining_conflicts", []) - ), + "conflicts_found": original_conflict_count, + "conflicts_resolved": original_conflict_count + - remaining_count, + "conflicts_remaining": remaining_count, }, ) diff --git a/apps/backend/merge/conflict_resolver.py b/apps/backend/merge/conflict_resolver.py index 542e4bf2..52493728 100644 --- a/apps/backend/merge/conflict_resolver.py +++ b/apps/backend/merge/conflict_resolver.py @@ -87,7 +87,8 @@ class ConflictResolver: for idx, conflict in enumerate(conflicts): if progress_callback: # Emit per-conflict progress within the resolving stage (50-75%) - conflict_percent = 50 + int((idx / max(total_conflicts, 1)) * 25) + # Calculate progress after processing (idx + 1) to reach 75% on last conflict + conflict_percent = 50 + int(((idx + 1) / max(total_conflicts, 1)) * 25) progress_callback( stage=MergeProgressStage.RESOLVING, percent=conflict_percent, diff --git a/apps/backend/merge/orchestrator.py b/apps/backend/merge/orchestrator.py index 185e946b..4e87d7d7 100644 --- a/apps/backend/merge/orchestrator.py +++ b/apps/backend/merge/orchestrator.py @@ -360,7 +360,8 @@ class MergeOrchestrator: # --- RESOLVING stage (50-75%) --- total_files = len(modifications) for idx, (file_path, snapshot) in enumerate(modifications): - file_percent = 50 + int((idx / max(total_files, 1)) * 25) + # Calculate progress after processing (idx + 1) to reach 75% on last file + file_percent = 50 + int(((idx + 1) / max(total_files, 1)) * 25) _emit( MergeProgressStage.RESOLVING, file_percent, 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 f5798d77..73164220 100644 --- a/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/worktree-handlers.ts @@ -2145,7 +2145,14 @@ export function registerWorktreeHandlers( try { const parsed = JSON.parse(trimmed); - if (parsed && parsed.type === 'progress') { + // Validate parsed object has expected MergeProgress structure before forwarding + if ( + parsed && + parsed.type === 'progress' && + typeof parsed.stage === 'string' && + typeof parsed.percent === 'number' && + typeof parsed.message === 'string' + ) { const mainWindow = getMainWindow(); if (mainWindow) { mainWindow.webContents.send(IPC_CHANNELS.TASK_MERGE_PROGRESS, taskId, parsed); diff --git a/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx b/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx index c22085a2..1595bc0f 100644 --- a/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx +++ b/apps/frontend/src/renderer/components/task-detail/TaskReview.tsx @@ -134,6 +134,7 @@ export function TaskReview({ ) : worktreeStatus?.exists ? ( /* Worktree exists but not yet staged - show staging UI */ > = { }; export function WorkspaceStatus({ + taskId, worktreeStatus, workspaceError, stageOnly, @@ -131,20 +136,33 @@ export function WorkspaceStatus({ }, [isMerging]); // Minimum display time: keep overlay visible for at least 500ms after merge ends + // Also wait for terminal progress event (complete/error) to avoid hiding before final message useEffect(() => { if (!isMerging && showOverlay && mergeStartTimeRef.current !== null) { - const elapsed = Date.now() - mergeStartTimeRef.current; - const MIN_DISPLAY_MS = 500; - const remaining = Math.max(0, MIN_DISPLAY_MS - elapsed); + // Check if we received a terminal progress event (complete or error) + const hasTerminalEvent = mergeProgress?.stage === 'complete' || mergeProgress?.stage === 'error'; - if (remaining > 0) { + // Only hide if we have a terminal event OR if a fallback timeout expires + if (hasTerminalEvent) { + const elapsed = Date.now() - mergeStartTimeRef.current; + const MIN_DISPLAY_MS = 500; + const remaining = Math.max(0, MIN_DISPLAY_MS - elapsed); + + if (remaining > 0) { + minDisplayTimerRef.current = setTimeout(() => { + setShowOverlay(false); + mergeStartTimeRef.current = null; + }, remaining); + } else { + setShowOverlay(false); + mergeStartTimeRef.current = null; + } + } else { + // Fallback: hide after 2s if no terminal event received (defensive) minDisplayTimerRef.current = setTimeout(() => { setShowOverlay(false); mergeStartTimeRef.current = null; - }, remaining); - } else { - setShowOverlay(false); - mergeStartTimeRef.current = null; + }, 2000); } } @@ -154,7 +172,7 @@ export function WorkspaceStatus({ minDisplayTimerRef.current = null; } }; - }, [isMerging, showOverlay]); + }, [isMerging, showOverlay, mergeProgress?.stage]); // Subscribe to merge progress IPC events useEffect(() => { @@ -169,24 +187,32 @@ export function WorkspaceStatus({ } }; - const cleanup = window.electronAPI.onMergeProgress((_taskId: string, progress: MergeProgress) => { + const cleanup = window.electronAPI.onMergeProgress((eventTaskId: string, progress: MergeProgress) => { + // Filter by task ID to prevent cross-task event leakage + if (eventTaskId !== taskId) return; + setMergeProgress(progress); - setLogEntries(prev => [ - ...prev, - { + setLogEntries(prev => { + const newEntry = { timestamp: new Date().toISOString(), type: stageToLogType(progress.stage), message: progress.message, details: progress.details?.current_file, + }; + // Limit log entries to prevent unbounded growth during long merges + const updated = [...prev, newEntry]; + if (updated.length > MAX_LOG_ENTRIES) { + return updated.slice(-MAX_LOG_ENTRIES); } - ]); + return updated; + }); }); // Store cleanup ref so we can call it on unmount even if isMerging changes ipcCleanupRef.current = cleanup; return cleanup; - }, [isMerging]); + }, [isMerging, taskId]); // Ensure IPC listener cleanup on unmount during active merge useEffect(() => { diff --git a/tests/test_auth.py b/tests/test_auth.py index fcb4334d..d022202a 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -70,7 +70,7 @@ class TestEnvVarTokenResolution: def test_no_token_returns_none(self, monkeypatch): """Returns None when no auth token is configured.""" # Mock keychain to return None (env vars already cleared by fixture) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) token = get_auth_token() assert token is None @@ -374,7 +374,7 @@ class TestRequireAuthToken: for var in AUTH_TOKEN_ENV_VARS: os.environ.pop(var, None) # Mock keychain to return None (tests that need a token will set env var) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) yield # Cleanup after test for var in AUTH_TOKEN_ENV_VARS: @@ -512,7 +512,8 @@ class TestTokenSourceDetection: monkeypatch.setattr("subprocess.run", Mock(return_value=mock_result)) source = get_auth_token_source() - assert source == "macOS Keychain" + # Source can be "macOS Keychain" or "macOS Keychain (profile)" depending on profile settings + assert source is not None and source.startswith("macOS Keychain") def test_source_windows_credential_files(self, monkeypatch, tmp_path): """Identifies Windows Credential Files as source.""" @@ -528,7 +529,8 @@ class TestTokenSourceDetection: ) source = get_auth_token_source() - assert source == "Windows Credential Files" + # Source can have "(profile)" suffix depending on profile settings + assert source is not None and source.startswith("Windows Credential Files") def test_source_linux_secret_service(self, monkeypatch): """Identifies Linux Secret Service as source.""" @@ -554,12 +556,13 @@ class TestTokenSourceDetection: monkeypatch.setattr("core.auth.secretstorage", mock_ss) source = get_auth_token_source() - assert source == "Linux Secret Service" + # Source can have "(profile)" suffix depending on profile settings + assert source is not None and source.startswith("Linux Secret Service") def test_source_none_when_not_found(self, monkeypatch): """Returns None when no token source is found.""" # Mock keychain to return None (env vars already cleared by fixture) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) source = get_auth_token_source() assert source is None diff --git a/tests/test_client.py b/tests/test_client.py index 4f0a73ba..cf6cca5c 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -46,7 +46,7 @@ class TestClientTokenValidation: monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "enc:test123456789012") # Mock keychain to ensure encrypted token is the only source - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock decrypt_token to raise ValueError (simulates decryption failure) # This ensures the encrypted token flows through to validate_token_not_encrypted monkeypatch.setattr( @@ -63,7 +63,7 @@ class TestClientTokenValidation: monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", "enc:test123456789012") # Mock keychain to ensure encrypted token is the only source - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock decrypt_token to raise ValueError (simulates decryption failure) monkeypatch.setattr( "core.auth.decrypt_token", @@ -77,7 +77,7 @@ class TestClientTokenValidation: """Verify create_client() accepts valid plaintext tokens and creates SDK client.""" valid_token = "sk-ant-oat01-valid-plaintext-token" monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", valid_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock the SDK client to avoid actual initialization mock_sdk_client = MagicMock() @@ -93,7 +93,7 @@ class TestClientTokenValidation: """Verify create_simple_client() accepts valid plaintext tokens and creates SDK client.""" valid_token = "sk-ant-oat01-valid-plaintext-token" monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", valid_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock the SDK client to avoid actual initialization mock_sdk_client = MagicMock() @@ -113,7 +113,7 @@ class TestClientTokenValidation: """Verify create_client() validates token format before SDK initialization.""" valid_token = "sk-ant-oat01-valid-token" monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", valid_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock validate_token_not_encrypted to verify it's called with patch( @@ -130,7 +130,7 @@ class TestClientTokenValidation: """Verify create_simple_client() validates token format before SDK initialization.""" valid_token = "sk-ant-oat01-valid-token" monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", valid_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock validate_token_not_encrypted to verify it's called with patch( @@ -213,7 +213,7 @@ class TestAPIProfileAuthentication: # Don't set ANTHROPIC_BASE_URL - this should trigger OAuth mode monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False) monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock the SDK client mock_sdk_client = MagicMock() @@ -270,7 +270,7 @@ class TestAPIProfileAuthentication: # Set empty ANTHROPIC_BASE_URL - should be treated as "not set" monkeypatch.setenv("ANTHROPIC_BASE_URL", "") monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock require_auth_token to verify it's called (OAuth mode) with patch("core.auth.require_auth_token", return_value=oauth_token): @@ -314,7 +314,7 @@ class TestAPIProfileAuthentication: monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False) # Mock keychain to return None - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) from core.client import create_client @@ -397,7 +397,7 @@ class TestAPIProfileAuthenticationIntegration: monkeypatch.setenv("ANTHROPIC_AUTH_TOKEN", api_token) monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False) monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", encrypted_oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) from core.client import create_client @@ -421,7 +421,7 @@ class TestAPIProfileAuthenticationEdgeCases: # Set whitespace-only ANTHROPIC_BASE_URL - should be trimmed to empty string monkeypatch.setenv("ANTHROPIC_BASE_URL", " ") monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) # Mock the SDK client mock_sdk_client = MagicMock() @@ -526,7 +526,7 @@ class TestSimpleClientAPIProfileAuthentication: monkeypatch.delenv("ANTHROPIC_BASE_URL", raising=False) monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) mock_sdk_client = MagicMock() with patch("core.simple_client.ClaudeSDKClient", return_value=mock_sdk_client): @@ -582,7 +582,7 @@ class TestSimpleClientAPIProfileAuthentication: # Set whitespace-only ANTHROPIC_BASE_URL - should be trimmed to empty string monkeypatch.setenv("ANTHROPIC_BASE_URL", " ") monkeypatch.setenv("CLAUDE_CODE_OAUTH_TOKEN", oauth_token) - monkeypatch.setattr("core.auth.get_token_from_keychain", lambda: None) + monkeypatch.setattr("core.auth.get_token_from_keychain", lambda _config_dir=None: None) mock_sdk_client = MagicMock() with patch("core.simple_client.ClaudeSDKClient", return_value=mock_sdk_client):