From f28d22984d9031015f2256285ef63c2c8e0e94bf Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Thu, 15 Jan 2026 14:29:38 +0100 Subject: [PATCH] fix(pr-review): properly capture structured output from SDK ResultMessage (#1133) * fix(pr-review): properly capture structured output from SDK ResultMessage Fixes critical bug where PR follow-up reviews showed "0 previous findings addressed" despite AI correctly analyzing resolution status. Root Causes Fixed: 1. sdk_utils.py - ResultMessage handling - Added proper check for msg.type == "result" per Anthropic SDK docs - Handle msg.subtype == "success" for structured output capture - Handle error_max_structured_output_retries error case - Added visible logging when structured output is captured 2. parallel_followup_reviewer.py - Silent fallback prevention - Added warning logging when structured output is missing - Added _extract_partial_data() to recover data when Pydantic fails - Prevents complete data loss when schema validation has minor issues 3. parallel_followup_reviewer.py - CI status enforcement - Added code enforcement for failing CI (override to BLOCKED) - Added enforcement for pending CI (downgrade READY_TO_MERGE) - AI prompt compliance is no longer the only safeguard 4. test_dependency_validator.py - macOS compatibility fixes - Fixed symlink comparison issue (/var vs /private/var) - Fixed case-sensitivity comparison for filesystem Impact: Before: AI analysis showed "3/4 resolved" but summary showed "0 resolved" After: Structured output properly captured, fallback extraction if needed Co-Authored-By: Claude Opus 4.5 * fix(pr-review): rescan related files after worktree creation Related files were always returning 0 because context gathering happened BEFORE the worktree was created. For fork PRs or PRs with new files, the files don't exist in the local checkout, so the related files lookup failed. This fix: - Adds `find_related_files_for_root()` static method to ContextGatherer that can search for related files using any project root path - Restructures ParallelOrchestratorReviewer.review() to create the worktree FIRST, then rescan for related files using the worktree path, then build the prompt with the updated context Now the PR review will correctly find related test files, config files, and type definitions that exist in the PR. Co-Authored-By: Claude Opus 4.5 * fix(pr-review): add visible logging for worktree creation and rescan Add always-visible logs (not gated by DEBUG_MODE) to show: - When worktree is created for PR review - Result of related files rescan in worktree This helps verify the fix is working and diagnose issues. Co-Authored-By: Claude Opus 4.5 * feat(pr-review): show model name when invoking specialist agents Add model information to agent invocation logs so users can see which model each agent is using. This helps with debugging and monitoring. Example log output: [ParallelOrchestrator] Invoking agent: logic-reviewer [sonnet-4.5] [ParallelOrchestrator] Invoking agent: quality-reviewer [sonnet-4.5] Added _short_model_name() helper to convert full model names like "claude-sonnet-4-5-20250929" to short display names like "sonnet-4.5". Co-Authored-By: Claude Opus 4.5 * fix(sdk-utils): add model info to AssistantMessage tool invocation logs The agent invocation log was missing the model info when tool calls came through AssistantMessage content blocks (vs standalone ToolUseBlock). Now both code paths show the model name consistently. Co-Authored-By: Claude Opus 4.5 * feat(sdk-utils): add user-visible progress and activity logging Previously, most SDK stream activity was hidden behind DEBUG_MODE, making it hard for users to see what's happening during PR reviews. Changes: - Add periodic progress logs every 10 messages showing agent count - Show tool usage (Read, Grep, etc.) not just Task calls - Show tool completion results with brief preview - Model info now shown for all agent invocation paths Users will now see: - "[ParallelOrchestrator] Processing... (20 messages, 4 agents working)" - "[ParallelOrchestrator] Using tool: Read" - "[ParallelOrchestrator] Tool result [done]: ..." - "[ParallelOrchestrator] Invoking agent: logic-reviewer [opus-4.5]" Co-Authored-By: Claude Opus 4.5 * fix(pr-review): improve worktree visibility and fix log categorization 1. Frontend log categorization: - Add "PRReview" and "ClientCache" to analysisSources - [PRReview] logs now appear in "AI Analysis" section instead of "Synthesis" 2. Enhanced worktree logging: - Show file count in worktree creation log - Display PR branch HEAD SHA for verification - Format: "[PRReview] Created temporary worktree: pr-xxx (1,234 files)" 3. Structured output detection: - Also check for msg_type == "ResultMessage" (SDK class name) - Add diagnostic logging in DEBUG mode to trace ResultMessage handling Co-Authored-By: Claude Opus 4.5 * chore(deps): update claude-agent-sdk to >=0.1.19 Update to latest SDK version for structured output improvements. Previous: >=0.1.16 Latest available: 0.1.19 Co-Authored-By: Claude Opus 4.5 * refactor(sdk-utils): consolidate structured output capture to single location BREAKING: Simplified structured output handling to follow official Python SDK pattern. Before: 5 different capture locations causing "Multiple StructuredOutput blocks" warnings After: 1 capture location using hasattr(msg, 'structured_output') per official docs Changes: - Remove 4 redundant capture paths (ToolUseBlock, AssistantMessage content, legacy, ResultMessage) - Single capture point: if hasattr(msg, 'structured_output') and msg.structured_output - Skip duplicates silently (only capture first one) - Keep error handling for error_max_structured_output_retries - Skip logging StructuredOutput tool calls (handled separately) - Cleaner, more maintainable code following official SDK pattern Reference: https://platform.claude.com/docs/en/agent-sdk/structured-outputs Co-Authored-By: Claude Opus 4.5 * feat(pr-logs): enhance log visibility and organization for agent activities - Introduced a new logging structure to categorize agent logs into groups, improving readability and user experience. - Added functionality to toggle visibility of agent logs and orchestrator tool activities, allowing users to focus on relevant information. - Implemented helper functions to identify tool activity logs and group entries by agent, enhancing log organization. - Updated UI components to support the new log grouping and toggling features, ensuring a seamless user interface. This update aims to provide clearer insights into agent activities during PR reviews, making it easier for users to track progress and actions taken by agents. * fix(pr-review): address PR review findings for reliability and UX - Fix CI pending check asymmetry: check MERGE_WITH_CHANGES verdict - Add file count limit (10k) to prevent slow rglob on large repos - Extract CONFIG_FILE_NAMES constant to fix DRY violation - Fix misleading "agents working" count by tracking completed agents - Add i18n translations for agent activity logs (en/fr) Co-Authored-By: Claude Opus 4.5 * fix(pr-logs): categorize Followup logs to context phase for follow-up reviews The Followup source logs context gathering work (comparing commits, finding changed files, gathering feedback) not analysis. Move from analysisSources to contextSources so follow-up review logs appear in the correct phase. Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- apps/backend/requirements.txt | 2 +- .../runners/github/context_gatherer.py | 90 +++++- .../services/parallel_followup_reviewer.py | 153 ++++++++++ .../parallel_orchestrator_reviewer.py | 65 +++- .../runners/github/services/sdk_utils.py | 260 ++++++++++++---- .../main/ipc-handlers/github/pr-handlers.ts | 10 +- .../github-prs/components/PRLogs.tsx | 282 +++++++++++++++++- .../src/shared/i18n/locales/en/common.json | 7 +- .../src/shared/i18n/locales/fr/common.json | 7 +- tests/test_dependency_validator.py | 11 +- 10 files changed, 786 insertions(+), 101 deletions(-) diff --git a/apps/backend/requirements.txt b/apps/backend/requirements.txt index 13a9e1fb..aa5e2713 100644 --- a/apps/backend/requirements.txt +++ b/apps/backend/requirements.txt @@ -1,5 +1,5 @@ # Auto-Build Framework Dependencies -claude-agent-sdk>=0.1.16 +claude-agent-sdk>=0.1.19 python-dotenv>=1.0.0 # TOML parsing fallback for Python < 3.11 diff --git a/apps/backend/runners/github/context_gatherer.py b/apps/backend/runners/github/context_gatherer.py index 374f11c6..31f5f015 100644 --- a/apps/backend/runners/github/context_gatherer.py +++ b/apps/backend/runners/github/context_gatherer.py @@ -37,6 +37,20 @@ except (ImportError, ValueError, SystemError): SAFE_REF_PATTERN = re.compile(r"^[a-zA-Z0-9._/\-]+$") SAFE_PATH_PATTERN = re.compile(r"^[a-zA-Z0-9._/\-@]+$") +# Common config file names to search for in project directories +# Used by both _find_config_files() and find_related_files_for_root() +CONFIG_FILE_NAMES = [ + "tsconfig.json", + "package.json", + "pyproject.toml", + "setup.py", + ".eslintrc", + ".prettierrc", + "jest.config.js", + "vitest.config.ts", + "vite.config.ts", +] + def _validate_git_ref(ref: str) -> bool: """ @@ -942,20 +956,8 @@ class PRContextGatherer: def _find_config_files(self, directory: Path) -> set[str]: """Find configuration files in a directory.""" - config_names = [ - "tsconfig.json", - "package.json", - "pyproject.toml", - "setup.py", - ".eslintrc", - ".prettierrc", - "jest.config.js", - "vitest.config.ts", - "vite.config.ts", - ] - found = set() - for name in config_names: + for name in CONFIG_FILE_NAMES: config_path = directory / name full_path = self.project_dir / config_path if full_path.exists() and full_path.is_file(): @@ -974,6 +976,68 @@ class PRContextGatherer: return set() + @staticmethod + def find_related_files_for_root( + changed_files: list[ChangedFile], + project_root: Path, + ) -> list[str]: + """ + Find files related to the changes using a specific project root. + + This static method allows finding related files AFTER a worktree + has been created, ensuring files exist in the worktree filesystem. + + Args: + changed_files: List of changed files from the PR + project_root: Path to search for related files (e.g., worktree path) + + Returns: + List of related file paths (relative to project root) + """ + related: set[str] = set() + + for changed_file in changed_files: + path = Path(changed_file.path) + + # Find test files + test_patterns = [ + # Jest/Vitest patterns + path.parent / f"{path.stem}.test{path.suffix}", + path.parent / f"{path.stem}.spec{path.suffix}", + path.parent / "__tests__" / f"{path.name}", + # Python patterns + path.parent / f"test_{path.stem}.py", + path.parent / f"{path.stem}_test.py", + # Go patterns + path.parent / f"{path.stem}_test.go", + ] + + for test_path in test_patterns: + full_path = project_root / test_path + if full_path.exists() and full_path.is_file(): + related.add(str(test_path)) + + # Find config files in same directory + for name in CONFIG_FILE_NAMES: + config_path = path.parent / name + full_path = project_root / config_path + if full_path.exists() and full_path.is_file(): + related.add(str(config_path)) + + # Find type definition files + if path.suffix in [".ts", ".tsx"]: + type_def = path.parent / f"{path.stem}.d.ts" + full_path = project_root / type_def + if full_path.exists() and full_path.is_file(): + related.add(str(type_def)) + + # Remove files that are already in changed_files + changed_paths = {cf.path for cf in changed_files} + related = {r for r in related if r not in changed_paths} + + # Limit to 20 most relevant files + return sorted(related)[:20] + class FollowupContextGatherer: """ diff --git a/apps/backend/runners/github/services/parallel_followup_reviewer.py b/apps/backend/runners/github/services/parallel_followup_reviewer.py index 84e4c8f7..d210801b 100644 --- a/apps/backend/runners/github/services/parallel_followup_reviewer.py +++ b/apps/backend/runners/github/services/parallel_followup_reviewer.py @@ -531,6 +531,7 @@ The SDK will run invoked agents in parallel automatically. stream_result = await process_sdk_stream( client=client, context_name="ParallelFollowup", + model=model, ) # Check for stream processing errors @@ -558,6 +559,17 @@ The SDK will run invoked agents in parallel automatically. if structured_output: result_data = self._parse_structured_output(structured_output, context) else: + # Log when structured output is missing - this shouldn't happen normally + # when output_format is configured, so it indicates a problem + logger.warning( + "[ParallelFollowup] No structured output received from SDK - " + "falling back to text parsing. Resolution data may be incomplete." + ) + safe_print( + "[ParallelFollowup] WARNING: Structured output not captured, " + "using text fallback (resolution tracking may be incomplete)", + flush=True, + ) result_data = self._parse_text_output(result_text, context) # Extract data @@ -623,6 +635,51 @@ The SDK will run invoked agents in parallel automatically. flush=True, ) + # CRITICAL: Enforce CI pending status - cannot approve with pending checks + # This ensures AI compliance with the rule: "Pending CI = NEEDS_REVISION" + ci_status = context.ci_status or {} + pending_ci = ci_status.get("pending", 0) + failing_ci = ci_status.get("failing", 0) + + if failing_ci > 0: + # Failing CI blocks merge + if verdict in ( + MergeVerdict.READY_TO_MERGE, + MergeVerdict.MERGE_WITH_CHANGES, + ): + failed_checks = ci_status.get("failed_checks", []) + checks_str = ( + ", ".join(failed_checks[:3]) if failed_checks else "unknown" + ) + blockers.append( + f"CI Failing: {failing_ci} check(s) failing ({checks_str})" + ) + verdict = MergeVerdict.BLOCKED + verdict_reasoning = ( + f"Blocked: {failing_ci} CI check(s) failing. " + f"Fix CI issues before merge." + ) + safe_print( + f"[ParallelFollowup] ⚠️ CI failing ({failing_ci} checks) - blocking merge", + flush=True, + ) + elif pending_ci > 0: + # Pending CI prevents merge-ready verdicts + if verdict in ( + MergeVerdict.READY_TO_MERGE, + MergeVerdict.MERGE_WITH_CHANGES, + ): + verdict = MergeVerdict.NEEDS_REVISION + verdict_reasoning = ( + f"Ready once CI passes: {pending_ci} check(s) still pending. " + f"All code issues addressed, waiting for CI completion." + ) + safe_print( + f"[ParallelFollowup] ⏳ CI pending ({pending_ci} checks) - " + f"downgrading verdict to NEEDS_REVISION", + flush=True, + ) + for finding in unique_findings: if finding.severity in ( ReviewSeverity.CRITICAL, @@ -928,7 +985,34 @@ The SDK will run invoked agents in parallel automatically. } except Exception as e: + # Log error visibly so users know structured output parsing failed logger.warning(f"[ParallelFollowup] Failed to parse structured output: {e}") + safe_print( + f"[ParallelFollowup] ERROR: Structured output parsing failed: {e}", + flush=True, + ) + safe_print( + "[ParallelFollowup] Attempting to extract partial data from raw output...", + flush=True, + ) + + # Try to extract what we can from the raw dict before giving up + # This handles cases where Pydantic validation fails but data is present + try: + partial_result = self._extract_partial_data(data) + if partial_result: + safe_print( + f"[ParallelFollowup] Recovered partial data: " + f"{len(partial_result.get('resolved_ids', []))} resolved, " + f"{len(partial_result.get('unresolved_ids', []))} unresolved", + flush=True, + ) + return partial_result + except Exception as extract_error: + logger.warning( + f"[ParallelFollowup] Partial extraction also failed: {extract_error}" + ) + return self._create_empty_result() def _parse_text_output(self, text: str, context: FollowupReviewContext) -> dict: @@ -969,6 +1053,75 @@ The SDK will run invoked agents in parallel automatically. "verdict_reasoning": "Unable to parse review results", } + def _extract_partial_data(self, data: dict) -> dict | None: + """ + Extract what data we can from raw output when Pydantic validation fails. + + This handles cases where the AI produced valid data but it doesn't exactly + match the expected schema (missing optional fields, type mismatches, etc.). + """ + if not isinstance(data, dict): + return None + + resolved_ids = [] + unresolved_ids = [] + new_finding_ids = [] + + # Try to extract resolution verifications + resolution_verifications = data.get("resolution_verifications", []) + if isinstance(resolution_verifications, list): + for rv in resolution_verifications: + if isinstance(rv, dict): + finding_id = rv.get("finding_id", "") + status = rv.get("status", "") + if finding_id: + if status == "resolved": + resolved_ids.append(finding_id) + elif status in ( + "unresolved", + "partially_resolved", + "cant_verify", + ): + unresolved_ids.append(finding_id) + + # Try to extract new findings + new_findings = data.get("new_findings", []) + if isinstance(new_findings, list): + for nf in new_findings: + if isinstance(nf, dict): + finding_id = nf.get("id", "") + if finding_id: + new_finding_ids.append(finding_id) + + # Try to extract verdict + verdict_str = data.get("verdict", "NEEDS_REVISION") + verdict_map = { + "READY_TO_MERGE": MergeVerdict.READY_TO_MERGE, + "MERGE_WITH_CHANGES": MergeVerdict.MERGE_WITH_CHANGES, + "NEEDS_REVISION": MergeVerdict.NEEDS_REVISION, + "BLOCKED": MergeVerdict.BLOCKED, + } + verdict = verdict_map.get(verdict_str, MergeVerdict.NEEDS_REVISION) + + verdict_reasoning = data.get("verdict_reasoning", "Extracted from partial data") + + # Only return if we got any useful data + if resolved_ids or unresolved_ids or new_finding_ids: + return { + "findings": [], # Can't reliably extract full findings without validation + "resolved_ids": resolved_ids, + "unresolved_ids": unresolved_ids, + "new_finding_ids": new_finding_ids, + "dismissed_false_positive_ids": [], + "confirmed_valid_count": 0, + "needs_human_review_count": 0, + "verdict": verdict, + "verdict_reasoning": f"[Partial extraction] {verdict_reasoning}", + "agents_invoked": data.get("agents_invoked", []), + } + + return None + def _generate_finding_id(self, file: str, line: int, title: str) -> str: """Generate a unique finding ID.""" content = f"{file}:{line}:{title}" diff --git a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py index 1d88a1ff..eef98688 100644 --- a/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py +++ b/apps/backend/runners/github/services/parallel_orchestrator_reviewer.py @@ -28,7 +28,7 @@ from claude_agent_sdk import AgentDefinition try: from ...core.client import create_client from ...phase_config import get_thinking_budget - from ..context_gatherer import PRContext, _validate_git_ref + from ..context_gatherer import PRContext, PRContextGatherer, _validate_git_ref from ..gh_client import GHClient from ..models import ( BRANCH_BEHIND_BLOCKER_MSG, @@ -45,7 +45,7 @@ try: from .pydantic_models import ParallelOrchestratorResponse from .sdk_utils import process_sdk_stream except (ImportError, ValueError, SystemError): - from context_gatherer import PRContext, _validate_git_ref + from context_gatherer import PRContext, PRContextGatherer, _validate_git_ref from core.client import create_client from gh_client import GHClient from models import ( @@ -468,11 +468,9 @@ The SDK will run invoked agents in parallel automatically. pr_number=context.pr_number, ) - # Build orchestrator prompt - prompt = self._build_orchestrator_prompt(context) - # Create temporary worktree at PR head commit for isolated review - # This ensures agents read from the correct PR state, not the current checkout + # This MUST happen BEFORE building the prompt so we can find related files + # that exist in the PR but not in the current checkout head_sha = context.head_sha or context.head_branch if DEBUG_MODE: @@ -518,12 +516,31 @@ The SDK will run invoked agents in parallel automatically. head_sha, context.pr_number ) project_root = worktree_path - if DEBUG_MODE: - safe_print( - f"[PRReview] DEBUG: Using worktree as " - f"project_root={project_root}", - flush=True, - ) + # Count files in worktree to give user visibility (with limit to avoid slowdown) + MAX_FILE_COUNT = 10000 + try: + file_count = 0 + for f in worktree_path.rglob("*"): + if f.is_file() and ".git" not in f.parts: + file_count += 1 + if file_count >= MAX_FILE_COUNT: + break + except (OSError, PermissionError): + file_count = 0 + file_count_str = ( + f"{file_count:,}+" + if file_count >= MAX_FILE_COUNT + else f"{file_count:,}" + ) + # Always log worktree creation with file count (not gated by DEBUG_MODE) + safe_print( + f"[PRReview] Created temporary worktree: {worktree_path.name} ({file_count_str} files)", + flush=True, + ) + safe_print( + f"[PRReview] Worktree contains PR branch HEAD: {head_sha[:8]}", + flush=True, + ) except (RuntimeError, ValueError) as e: if DEBUG_MODE: safe_print( @@ -541,6 +558,29 @@ The SDK will run invoked agents in parallel automatically. else self.project_dir ) + # Rescan for related files using the worktree/project root + # This fixes the issue where related files were 0 because context gathering + # happened BEFORE the worktree was created (PR files didn't exist locally) + if context.changed_files: + new_related_files = PRContextGatherer.find_related_files_for_root( + context.changed_files, + project_root, + ) + # Always log rescan result (not gated by DEBUG_MODE) + if new_related_files: + context.related_files = new_related_files + safe_print( + f"[PRReview] Rescanned in worktree: found {len(new_related_files)} related files" + ) + else: + safe_print( + f"[PRReview] Rescanned in worktree: found 0 related files " + f"(initial scan found {len(context.related_files)})" + ) + + # Build orchestrator prompt AFTER worktree creation and related files rescan + prompt = self._build_orchestrator_prompt(context) + # Use model and thinking level from config (user settings) model = self.config.model or "claude-sonnet-4-5-20250929" thinking_level = self.config.thinking_level or "medium" @@ -575,6 +615,7 @@ The SDK will run invoked agents in parallel automatically. stream_result = await process_sdk_stream( client=client, context_name="ParallelOrchestrator", + model=model, ) # Check for stream processing errors diff --git a/apps/backend/runners/github/services/sdk_utils.py b/apps/backend/runners/github/services/sdk_utils.py index c6ac7d89..f1d21207 100644 --- a/apps/backend/runners/github/services/sdk_utils.py +++ b/apps/backend/runners/github/services/sdk_utils.py @@ -26,6 +26,104 @@ logger = logging.getLogger(__name__) DEBUG_MODE = os.environ.get("DEBUG", "").lower() in ("true", "1", "yes") +def _short_model_name(model: str | None) -> str: + """Convert full model name to a short display name for logs. + + Examples: + claude-sonnet-4-5-20250929 -> sonnet-4.5 + claude-opus-4-5-20251101 -> opus-4.5 + claude-3-5-sonnet-20241022 -> sonnet-3.5 + """ + if not model: + return "unknown" + + model_lower = model.lower() + + # Handle new model naming (claude-{model}-{version}-{date}) + if "opus-4-5" in model_lower or "opus-4.5" in model_lower: + return "opus-4.5" + if "sonnet-4-5" in model_lower or "sonnet-4.5" in model_lower: + return "sonnet-4.5" + if "haiku-4" in model_lower: + return "haiku-4" + + # Handle older model naming (claude-3-5-{model}) + if "3-5-sonnet" in model_lower or "3.5-sonnet" in model_lower: + return "sonnet-3.5" + if "3-5-haiku" in model_lower or "3.5-haiku" in model_lower: + return "haiku-3.5" + if "3-opus" in model_lower: + return "opus-3" + if "3-sonnet" in model_lower: + return "sonnet-3" + if "3-haiku" in model_lower: + return "haiku-3" + + # Fallback: return last part before date (if matches pattern) + parts = model.split("-") + if len(parts) >= 2: + # Try to find model type (opus, sonnet, haiku) + for i, part in enumerate(parts): + if part.lower() in ("opus", "sonnet", "haiku"): + return part.lower() + + return model[:20] # Truncate if nothing else works + + +def _get_tool_detail(tool_name: str, tool_input: dict[str, Any]) -> str: + """Extract meaningful detail from tool input for user-friendly logging. + + Instead of "Using tool: Read", show "Reading sdk_utils.py" + Instead of "Using tool: Grep", show "Searching for 'pattern'" + """ + if tool_name == "Read": + file_path = tool_input.get("file_path", "") + if file_path: + # Extract just the filename for brevity + filename = file_path.split("/")[-1] if "/" in file_path else file_path + return f"Reading {filename}" + return "Reading file" + + if tool_name == "Grep": + pattern = tool_input.get("pattern", "") + if pattern: + # Truncate long patterns + pattern_preview = pattern[:40] + "..." if len(pattern) > 40 else pattern + return f"Searching for '{pattern_preview}'" + return "Searching codebase" + + if tool_name == "Glob": + pattern = tool_input.get("pattern", "") + if pattern: + return f"Finding files matching '{pattern}'" + return "Finding files" + + if tool_name == "Bash": + command = tool_input.get("command", "") + if command: + # Show first part of command + cmd_preview = command[:50] + "..." if len(command) > 50 else command + return f"Running: {cmd_preview}" + return "Running command" + + if tool_name == "Edit": + file_path = tool_input.get("file_path", "") + if file_path: + filename = file_path.split("/")[-1] if "/" in file_path else file_path + return f"Editing {filename}" + return "Editing file" + + if tool_name == "Write": + file_path = tool_input.get("file_path", "") + if file_path: + filename = file_path.split("/")[-1] if "/" in file_path else file_path + return f"Writing {filename}" + return "Writing file" + + # Default fallback for unknown tools + return f"Using tool: {tool_name}" + + async def process_sdk_stream( client: Any, on_thinking: Callable[[str], None] | None = None, @@ -34,6 +132,7 @@ async def process_sdk_stream( on_text: Callable[[str], None] | None = None, on_structured_output: Callable[[dict[str, Any]], None] | None = None, context_name: str = "SDK", + model: str | None = None, ) -> dict[str, Any]: """ Process SDK response stream with customizable callbacks. @@ -43,7 +142,7 @@ async def process_sdk_stream( - Tracking tool invocations (especially Task/subagent calls) - Tracking tool results - Collecting text output - - Extracting structured output + - Extracting structured output (per official Python SDK pattern) Args: client: Claude SDK client with receive_response() method @@ -53,6 +152,7 @@ async def process_sdk_stream( on_text: Callback for text output - receives text string on_structured_output: Callback for structured output - receives dict context_name: Name for logging (e.g., "ParallelOrchestrator", "ParallelFollowup") + model: Model name for logging (e.g., "claude-sonnet-4-5-20250929") Returns: Dictionary with: @@ -70,17 +170,40 @@ async def process_sdk_stream( stream_error = None # Track subagent tool IDs to log their results subagent_tool_ids: dict[str, str] = {} # tool_id -> agent_name + completed_agent_tool_ids: set[str] = set() # tool_ids of completed agents safe_print(f"[{context_name}] Processing SDK stream...") if DEBUG_MODE: safe_print(f"[DEBUG {context_name}] Awaiting response stream...") + # Track activity for progress logging + last_progress_log = 0 + PROGRESS_LOG_INTERVAL = 10 # Log progress every N messages + try: async for msg in client.receive_response(): try: msg_type = type(msg).__name__ msg_count += 1 + # Log progress periodically so user knows AI is working + if msg_count - last_progress_log >= PROGRESS_LOG_INTERVAL: + if subagent_tool_ids: + pending = len(subagent_tool_ids) - len(completed_agent_tool_ids) + if pending > 0: + safe_print( + f"[{context_name}] Processing... ({msg_count} messages, {pending} agent{'s' if pending > 1 else ''} working)" + ) + else: + safe_print( + f"[{context_name}] Processing... ({msg_count} messages)" + ) + else: + safe_print( + f"[{context_name}] Processing... ({msg_count} messages)" + ) + last_progress_log = msg_count + if DEBUG_MODE: # Log every message type for visibility msg_details = "" @@ -130,23 +253,15 @@ async def process_sdk_stream( agents_invoked.append(agent_name) # Track this tool ID to log its result later subagent_tool_ids[tool_id] = agent_name - safe_print(f"[{context_name}] Invoked agent: {agent_name}") - elif tool_name == "StructuredOutput": - if tool_input: - # Warn if overwriting existing structured output - if structured_output is not None: - logger.warning( - f"[{context_name}] Multiple StructuredOutput blocks received, " - f"overwriting previous output" - ) - structured_output = tool_input - safe_print(f"[{context_name}] Received structured output") - # Invoke callback - if on_structured_output: - on_structured_output(tool_input) - elif DEBUG_MODE: - # Log other tool calls in debug mode - safe_print(f"[DEBUG {context_name}] Other tool: {tool_name}") + # Log with model info if available + model_info = f" [{_short_model_name(model)}]" if model else "" + safe_print( + f"[{context_name}] Invoking agent: {agent_name}{model_info}" + ) + elif tool_name != "StructuredOutput": + # Log meaningful tool info (not just tool name) + tool_detail = _get_tool_detail(tool_name, tool_input) + safe_print(f"[{context_name}] {tool_detail}") # Invoke callback for all tool uses if on_tool_use: @@ -169,6 +284,7 @@ async def process_sdk_stream( # Check if this is a subagent result if tool_id in subagent_tool_ids: agent_name = subagent_tool_ids[tool_id] + completed_agent_tool_ids.add(tool_id) # Mark agent as completed status = "ERROR" if is_error else "complete" result_preview = ( str(result_content)[:600].replace("\n", " ").strip() @@ -176,11 +292,17 @@ async def process_sdk_stream( safe_print( f"[Agent:{agent_name}] {status}: {result_preview}{'...' if len(str(result_content)) > 600 else ''}" ) - elif DEBUG_MODE: - status = "ERROR" if is_error else "OK" - safe_print( - f"[DEBUG {context_name}] Tool result: {tool_id} [{status}]" + else: + # Show tool completion for visibility (not gated by DEBUG) + status = "ERROR" if is_error else "done" + # Show brief preview of result for context + result_preview = ( + str(result_content)[:100].replace("\n", " ").strip() ) + if result_preview: + safe_print( + f"[{context_name}] Tool result [{status}]: {result_preview}{'...' if len(str(result_content)) > 100 else ''}" + ) # Invoke callback if on_tool_result: @@ -205,21 +327,19 @@ async def process_sdk_stream( if agent_name not in agents_invoked: agents_invoked.append(agent_name) subagent_tool_ids[tool_id] = agent_name - safe_print( - f"[{context_name}] Invoking agent: {agent_name}" + # Log with model info if available + model_info = ( + f" [{_short_model_name(model)}]" + if model + else "" ) - elif tool_name == "StructuredOutput": - if tool_input: - # Warn if overwriting existing structured output - if structured_output is not None: - logger.warning( - f"[{context_name}] Multiple StructuredOutput blocks received, " - f"overwriting previous output" - ) - structured_output = tool_input - # Invoke callback - if on_structured_output: - on_structured_output(tool_input) + safe_print( + f"[{context_name}] Invoking agent: {agent_name}{model_info}" + ) + elif tool_name != "StructuredOutput": + # Log meaningful tool info (not just tool name) + tool_detail = _get_tool_detail(tool_name, tool_input) + safe_print(f"[{context_name}] {tool_detail}") # Invoke callback if on_tool_use: @@ -239,33 +359,48 @@ async def process_sdk_stream( if on_text: on_text(block.text) - # Check for StructuredOutput in content (legacy check) - if getattr(block, "name", "") == "StructuredOutput": - structured_data = getattr(block, "input", None) - if structured_data: - # Warn if overwriting existing structured output - if structured_output is not None: - logger.warning( - f"[{context_name}] Multiple StructuredOutput blocks received, " - f"overwriting previous output" - ) - structured_output = structured_data - # Invoke callback - if on_structured_output: - on_structured_output(structured_data) + # ================================================================ + # STRUCTURED OUTPUT CAPTURE (Single, consolidated location) + # Per official Python SDK docs: https://platform.claude.com/docs/en/agent-sdk/structured-outputs + # The Python pattern is: if hasattr(message, 'structured_output') + # ================================================================ - # Check for structured_output attribute - if hasattr(msg, "structured_output") and msg.structured_output: - # Warn if overwriting existing structured output - if structured_output is not None: - logger.warning( - f"[{context_name}] Multiple StructuredOutput blocks received, " - f"overwriting previous output" + # Check for error_max_structured_output_retries first (SDK validation failed) + is_result_msg = msg_type == "ResultMessage" or ( + hasattr(msg, "type") and msg.type == "result" + ) + if is_result_msg: + subtype = getattr(msg, "subtype", None) + if DEBUG_MODE: + safe_print( + f"[DEBUG {context_name}] ResultMessage: subtype={subtype}" + ) + if subtype == "error_max_structured_output_retries": + # SDK failed to produce valid structured output after retries + logger.warning( + f"[{context_name}] Claude could not produce valid structured output " + f"after maximum retries - schema validation failed" + ) + safe_print( + f"[{context_name}] WARNING: Structured output validation failed after retries" + ) + if not stream_error: + stream_error = "structured_output_validation_failed" + + # Capture structured output from ANY message that has it + # This is the official Python SDK pattern - check hasattr() + if hasattr(msg, "structured_output") and msg.structured_output: + # Only capture if we don't already have it (avoid duplicates) + if structured_output is None: + structured_output = msg.structured_output + safe_print(f"[{context_name}] Received structured output") + if on_structured_output: + on_structured_output(msg.structured_output) + elif DEBUG_MODE: + # In debug mode, note that we skipped a duplicate + safe_print( + f"[DEBUG {context_name}] Skipping duplicate structured output" ) - structured_output = msg.structured_output - # Invoke callback - if on_structured_output: - on_structured_output(msg.structured_output) # Check for tool results in UserMessage (subagent results come back here) if msg_type == "UserMessage" and hasattr(msg, "content"): @@ -289,6 +424,9 @@ async def process_sdk_stream( # Check if this is a subagent result if tool_id in subagent_tool_ids: agent_name = subagent_tool_ids[tool_id] + completed_agent_tool_ids.add( + tool_id + ) # Mark agent as completed status = "ERROR" if is_error else "complete" result_preview = ( str(result_content)[:600].replace("\n", " ").strip() 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 d1dacecf..d2c7be05 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -544,15 +544,21 @@ function parseLogLine(line: string): { source: string; content: string; isError: * Determine the phase from source */ function getPhaseFromSource(source: string): PRLogPhase { - const contextSources = ["Context", "BotDetector"]; + // Context phase: gathering PR data, commits, files, feedback + // Note: "Followup" is context gathering for follow-up reviews (comparing commits, finding changes) + const contextSources = ["Context", "BotDetector", "Followup"]; + // Analysis phase: AI agents analyzing code const analysisSources = [ "AI", "Orchestrator", "ParallelOrchestrator", "ParallelFollowup", - "Followup", "orchestrator", + "PRReview", // Worktree creation and PR-specific analysis + "ClientCache", // SDK client cache operations ]; + // Synthesis phase: final summary and results + // Note: "Progress" logs are redundant (shown in progress bar) but kept for completeness const synthesisSources = ["PR Review Engine", "Summary", "Progress"]; if (contextSources.includes(source)) return "context"; diff --git a/apps/frontend/src/renderer/components/github-prs/components/PRLogs.tsx b/apps/frontend/src/renderer/components/github-prs/components/PRLogs.tsx index d7aa3936..c5d5f02c 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRLogs.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRLogs.tsx @@ -1,4 +1,5 @@ import { useState } from 'react'; +import { useTranslation } from 'react-i18next'; import { Terminal, Loader2, @@ -10,7 +11,8 @@ import { ChevronDown, ChevronRight, Info, - Clock + Clock, + Activity } from 'lucide-react'; import { Badge } from '../../ui/badge'; import { Collapsible, CollapsibleTrigger, CollapsibleContent } from '../../ui/collapsible'; @@ -71,8 +73,72 @@ const SOURCE_COLORS: Record = { 'default': 'bg-muted text-muted-foreground' }; +// Helper type for grouped agent entries +interface AgentGroup { + agentName: string; + entries: PRLogEntry[]; +} + +// Patterns that indicate orchestrator tool activity (vs. important messages) +const TOOL_ACTIVITY_PATTERNS = [ + /^Reading /, + /^Searching for /, + /^Finding files /, + /^Running: /, + /^Editing /, + /^Writing /, + /^Using tool: /, + /^Processing\.\.\. \(\d+ messages/, + /^Tool result \[/, +]; + +function isToolActivityLog(content: string): boolean { + return TOOL_ACTIVITY_PATTERNS.some(pattern => pattern.test(content)); +} + +// Group entries by: agents, orchestrator activity, and other entries +function groupEntriesByAgent(entries: PRLogEntry[]): { + agentGroups: AgentGroup[]; + orchestratorActivity: PRLogEntry[]; + otherEntries: PRLogEntry[]; +} { + const agentMap = new Map(); + const orchestratorActivity: PRLogEntry[] = []; + const otherEntries: PRLogEntry[] = []; + + for (const entry of entries) { + if (entry.source?.startsWith('Agent:')) { + // Agent results + const existing = agentMap.get(entry.source) || []; + existing.push(entry); + agentMap.set(entry.source, existing); + } else if ( + (entry.source === 'ParallelOrchestrator' || entry.source === 'ParallelFollowup') && + isToolActivityLog(entry.content) + ) { + // Orchestrator tool activity (verbose logs) + orchestratorActivity.push(entry); + } else { + // Important messages (AI response, Invoking agent, etc.) + otherEntries.push(entry); + } + } + + // Convert map to array of groups, sorted by first entry timestamp + const agentGroups: AgentGroup[] = Array.from(agentMap.entries()) + .map(([agentName, agentEntries]) => ({ agentName, entries: agentEntries })) + .sort((a, b) => { + const aTime = a.entries[0]?.timestamp || ''; + const bTime = b.entries[0]?.timestamp || ''; + return aTime.localeCompare(bTime); + }); + + return { agentGroups, orchestratorActivity, otherEntries }; +} + export function PRLogs({ prNumber, logs, isLoading, isStreaming = false }: PRLogsProps) { const [expandedPhases, setExpandedPhases] = useState>(new Set(['analysis'])); + const [expandedAgents, setExpandedAgents] = useState>(new Set()); const togglePhase = (phase: PRLogPhase) => { setExpandedPhases(prev => { @@ -86,6 +152,18 @@ export function PRLogs({ prNumber, logs, isLoading, isStreaming = false }: PRLog }); }; + const toggleAgent = (agentKey: string) => { + setExpandedAgents(prev => { + const next = new Set(prev); + if (next.has(agentKey)) { + next.delete(agentKey); + } else { + next.add(agentKey); + } + return next; + }); + }; + return (
@@ -122,6 +200,8 @@ export function PRLogs({ prNumber, logs, isLoading, isStreaming = false }: PRLog isExpanded={expandedPhases.has(phase)} onToggle={() => togglePhase(phase)} isStreaming={isStreaming} + expandedAgents={expandedAgents} + onToggleAgent={toggleAgent} /> ))} @@ -150,9 +230,11 @@ interface PhaseLogSectionProps { isExpanded: boolean; onToggle: () => void; isStreaming?: boolean; + expandedAgents: Set; + onToggleAgent: (agentKey: string) => void; } -function PhaseLogSection({ phase, phaseLog, isExpanded, onToggle, isStreaming = false }: PhaseLogSectionProps) { +function PhaseLogSection({ phase, phaseLog, isExpanded, onToggle, isStreaming = false, expandedAgents, onToggleAgent }: PhaseLogSectionProps) { const Icon = PHASE_ICONS[phase]; const status = phaseLog?.status || 'pending'; const hasEntries = (phaseLog?.entries.length || 0) > 0; @@ -235,13 +317,16 @@ function PhaseLogSection({ phase, phaseLog, isExpanded, onToggle, isStreaming = -
+
{!hasEntries ? (

No logs yet

) : ( - phaseLog?.entries.map((entry, idx) => ( - - )) + )}
@@ -249,6 +334,191 @@ function PhaseLogSection({ phase, phaseLog, isExpanded, onToggle, isStreaming = ); } +// Grouped Log Entries Component - renders agents grouped with collapsible sections +interface GroupedLogEntriesProps { + entries: PRLogEntry[]; + phase: PRLogPhase; + expandedAgents: Set; + onToggleAgent: (agentKey: string) => void; +} + +function GroupedLogEntries({ entries, phase, expandedAgents, onToggleAgent }: GroupedLogEntriesProps) { + const { agentGroups, orchestratorActivity, otherEntries } = groupEntriesByAgent(entries); + + return ( +
+ {/* Render important messages first (AI response, Invoking agent, etc.) */} + {otherEntries.length > 0 && ( +
+ {otherEntries.map((entry, idx) => ( + + ))} +
+ )} + + {/* Render orchestrator tool activity in collapsible section */} + {orchestratorActivity.length > 0 && ( + onToggleAgent(`${phase}-orchestrator-activity`)} + /> + )} + + {/* Render agent groups with collapsible sections */} + {agentGroups.map((group) => ( + onToggleAgent(`${phase}-${group.agentName}`)} + /> + ))} +
+ ); +} + +// Orchestrator Activity Section - collapsible section for tool activity logs +interface OrchestratorActivitySectionProps { + entries: PRLogEntry[]; + phase: PRLogPhase; + isExpanded: boolean; + onToggle: () => void; +} + +function OrchestratorActivitySection({ entries, isExpanded, onToggle }: OrchestratorActivitySectionProps) { + const { t } = useTranslation(['common']); + + // Count different types of operations for summary + const readCount = entries.filter(e => e.content.startsWith('Reading ')).length; + const searchCount = entries.filter(e => e.content.startsWith('Searching for ')).length; + const otherCount = entries.length - readCount - searchCount; + + // Build summary text + const summaryParts: string[] = []; + if (readCount > 0) summaryParts.push(`${readCount} file${readCount > 1 ? 's' : ''} read`); + if (searchCount > 0) summaryParts.push(`${searchCount} search${searchCount > 1 ? 'es' : ''}`); + if (otherCount > 0) summaryParts.push(`${otherCount} other`); + const summary = summaryParts.join(', ') || `${entries.length} operations`; + + return ( +
+ + + {isExpanded && ( +
+ {entries.map((entry, idx) => ( +
+ + {new Date(entry.timestamp).toLocaleTimeString('en-US', { hour: '2-digit', minute: '2-digit', second: '2-digit' })} + + {entry.content} +
+ ))} +
+ )} +
+ ); +} + +// Agent Log Group Component - shows first message + expandable section for more +interface AgentLogGroupProps { + group: AgentGroup; + phase: PRLogPhase; + isExpanded: boolean; + onToggle: () => void; +} + +function AgentLogGroup({ group, isExpanded, onToggle }: AgentLogGroupProps) { + const { t } = useTranslation(['common']); + const { agentName, entries } = group; + const hasMultipleEntries = entries.length > 1; + const firstEntry = entries[0]; + const remainingEntries = entries.slice(1); + + // Extract display name from "Agent:logic-reviewer" -> "logic-reviewer" + const displayName = agentName.replace('Agent:', ''); + + const getSourceColor = (source: string) => { + return SOURCE_COLORS[source] || SOURCE_COLORS.default; + }; + + return ( +
+ {/* Agent header with first message always visible */} +
+ {/* Agent badge header */} +
+ + {displayName} + + {hasMultipleEntries && ( + + )} +
+ + {/* First entry (summary) - always visible */} + {firstEntry && ( + + )} +
+ + {/* Collapsible section for remaining entries */} + {hasMultipleEntries && isExpanded && ( +
+ {remainingEntries.map((entry, idx) => ( + + ))} +
+ )} +
+ ); +} + // Log Entry Component interface LogEntryProps { entry: PRLogEntry; diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index aa11f3e2..907cd30c 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -338,7 +338,12 @@ "cleanReviewMessageFooter": "*This automated review found no issues. Generated by Auto Claude.*", "failedPostCleanReview": "Failed to post clean review", "viewErrorDetails": "View details", - "hideErrorDetails": "Hide details" + "hideErrorDetails": "Hide details", + "logs": { + "agentActivity": "Agent Activity", + "showMore": "Show {{count}} more", + "hideMore": "Hide {{count}} more" + } }, "downloads": { "toggleExpand": "Toggle download details", diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index 3861d444..f27975fa 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -338,7 +338,12 @@ "cleanReviewMessageFooter": "*This automated review found no issues. Generated by Auto Claude.*", "failedPostCleanReview": "Échec de la publication de la révision", "viewErrorDetails": "Voir les détails", - "hideErrorDetails": "Masquer les détails" + "hideErrorDetails": "Masquer les détails", + "logs": { + "agentActivity": "Activité des agents", + "showMore": "Afficher {{count}} de plus", + "hideMore": "Masquer {{count}}" + } }, "downloads": { "toggleExpand": "Afficher/masquer les détails", diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index c0e42275..550eb7b2 100644 --- a/tests/test_dependency_validator.py +++ b/tests/test_dependency_validator.py @@ -402,7 +402,8 @@ class TestCliUtilsGetProjectDir: from cli.utils import get_project_dir result = get_project_dir(temp_dir) - assert result == temp_dir + # Resolve symlinks for comparison (macOS /var -> /private/var) + assert result.resolve() == temp_dir.resolve() def test_get_project_dir_auto_detects_backend(self, temp_dir): """Auto-detect when running from apps/backend directory.""" @@ -420,7 +421,8 @@ class TestCliUtilsGetProjectDir: os.chdir(backend_dir) result = get_project_dir(None) # Should go up 2 levels from backend to project root - assert result == temp_dir + # Resolve symlinks for comparison (macOS /var -> /private/var) + assert result.resolve() == temp_dir.resolve() finally: os.chdir(original_cwd) @@ -442,8 +444,9 @@ class TestCliUtilsSetupEnvironment: script_dir = setup_environment() # Verify script_dir is the apps/backend directory - assert script_dir.name == "backend" - assert script_dir.parent.name == "apps" + # Use case-insensitive comparison for macOS filesystem compatibility + assert script_dir.name.lower() == "backend" + assert script_dir.parent.name.lower() == "apps" def test_setup_environment_adds_to_path(self): """Add script directory to sys.path."""