Compare commits
6 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| ba49fd064e | |||
| f5f521da2a | |||
| 624c15d29b | |||
| 23f712c094 | |||
| 2f4393b7f1 | |||
| 5046d37d62 |
@@ -297,6 +297,41 @@ When multiple agents report on the same area:
|
||||
- **Track consensus**: Note which findings have cross-agent validation
|
||||
- **Evidence-based, not confidence-based**: Multiple agents agreeing doesn't skip validation - all findings still verified
|
||||
|
||||
## STRUCTURED OUTPUT REQUIREMENTS
|
||||
|
||||
**CRITICAL: Use EXACT values below. Schema validation will fail otherwise.**
|
||||
|
||||
### Valid category values (use EXACTLY one of these, lowercase):
|
||||
- `security` - Authentication, authorization, injection, XSS, secrets
|
||||
- `quality` - Code quality, error handling, maintainability
|
||||
- `logic` - Logic errors, edge cases, race conditions
|
||||
- `test` - Test coverage, test quality
|
||||
- `docs` - Documentation issues
|
||||
- `regression` - Regression from previous fix
|
||||
- `incomplete_fix` - Partial fix that needs more work
|
||||
|
||||
### Valid severity values (use EXACTLY one of these, lowercase):
|
||||
- `critical` - Security vulnerabilities, data loss risk
|
||||
- `high` - Significant bugs, breaking changes
|
||||
- `medium` - Quality issues, should fix
|
||||
- `low` - Suggestions, minor improvements
|
||||
|
||||
### Valid verdict values (use EXACTLY one of these, UPPERCASE with underscores):
|
||||
- `READY_TO_MERGE` - All issues resolved, can merge
|
||||
- `MERGE_WITH_CHANGES` - Only LOW severity issues
|
||||
- `NEEDS_REVISION` - HIGH or MEDIUM issues (NOT "NEEDS REVISION")
|
||||
- `BLOCKED` - CRITICAL issues or CI failing
|
||||
|
||||
### Common mistakes to AVOID:
|
||||
| Wrong | Correct |
|
||||
|-------|---------|
|
||||
| `"duplication"` | Use `"quality"` or `"redundancy"` if in scope |
|
||||
| `"NEEDS REVISION"` | `"NEEDS_REVISION"` |
|
||||
| `"needs revision"` | `"NEEDS_REVISION"` |
|
||||
| `"Ready to Merge"` | `"READY_TO_MERGE"` |
|
||||
| `"HIGH"` | `"high"` (lowercase for severity) |
|
||||
| `"CRITICAL"` | `"critical"` (lowercase for severity) |
|
||||
|
||||
## Output Format
|
||||
|
||||
Provide your synthesis as a structured response matching the ParallelFollowupResponse schema:
|
||||
|
||||
@@ -626,6 +626,44 @@ The `agent_agreement` field in structured output tracks:
|
||||
**Note:** Agent agreement data is logged for monitoring. The cross-validation results
|
||||
are reflected in each finding's source_agents, cross_validated, and confidence fields.
|
||||
|
||||
## STRUCTURED OUTPUT REQUIREMENTS
|
||||
|
||||
**CRITICAL: Use EXACT values below. Schema validation will fail otherwise.**
|
||||
|
||||
### Valid category values (use EXACTLY one of these, lowercase):
|
||||
- `security` - Authentication, authorization, injection, XSS, secrets
|
||||
- `quality` - Code quality, error handling, maintainability
|
||||
- `logic` - Logic errors, edge cases, race conditions
|
||||
- `codebase_fit` - Architectural consistency, pattern adherence
|
||||
- `test` - Test coverage, test quality
|
||||
- `docs` - Documentation issues
|
||||
- `redundancy` - Code duplication (NOT "duplication"!)
|
||||
- `pattern` - Anti-patterns, design issues
|
||||
- `performance` - Performance problems
|
||||
|
||||
### Valid severity values (use EXACTLY one of these, lowercase):
|
||||
- `critical` - Security vulnerabilities, data loss risk
|
||||
- `high` - Significant bugs, breaking changes
|
||||
- `medium` - Quality issues, should fix
|
||||
- `low` - Suggestions, minor improvements
|
||||
|
||||
### Valid verdict values (use EXACTLY one of these, UPPERCASE with underscores):
|
||||
- `APPROVE` - No issues found
|
||||
- `COMMENT` - Only LOW severity issues
|
||||
- `NEEDS_REVISION` - HIGH or MEDIUM issues (NOT "NEEDS REVISION")
|
||||
- `BLOCKED` - CRITICAL issues
|
||||
|
||||
### Common mistakes to AVOID:
|
||||
| Wrong | Correct |
|
||||
|-------|---------|
|
||||
| `"duplication"` | `"redundancy"` |
|
||||
| `"NEEDS REVISION"` | `"NEEDS_REVISION"` |
|
||||
| `"needs revision"` | `"NEEDS_REVISION"` |
|
||||
| `"Ready to Merge"` | `"APPROVE"` |
|
||||
| `"READY_TO_MERGE"` | `"APPROVE"` (this schema uses APPROVE) |
|
||||
| `"HIGH"` | `"high"` (lowercase for severity) |
|
||||
| `"CRITICAL"` | `"critical"` (lowercase for severity) |
|
||||
|
||||
## Output Format
|
||||
|
||||
After synthesis and validation, output your final review in this JSON format:
|
||||
|
||||
@@ -818,7 +818,9 @@ class GHClient:
|
||||
return commits[-1].get("oid")
|
||||
return None
|
||||
|
||||
async def get_pr_checks(self, pr_number: int) -> dict[str, Any]:
|
||||
async def get_pr_checks(
|
||||
self, pr_number: int, excluded_checks: list[str] | None = None
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Get CI check runs status for a PR.
|
||||
|
||||
@@ -826,6 +828,7 @@ class GHClient:
|
||||
|
||||
Args:
|
||||
pr_number: PR number
|
||||
excluded_checks: List of check names to exclude from counts (for stuck/broken checks)
|
||||
|
||||
Returns:
|
||||
Dict with:
|
||||
@@ -834,7 +837,9 @@ class GHClient:
|
||||
- failing: Number of failing checks
|
||||
- pending: Number of pending checks
|
||||
- failed_checks: List of failed check names
|
||||
- excluded_checks: List of checks that were excluded from counting
|
||||
"""
|
||||
excluded_set = set(excluded_checks or [])
|
||||
try:
|
||||
# Note: gh pr checks --json only supports: bucket, completedAt, description,
|
||||
# event, link, name, startedAt, state, workflow
|
||||
@@ -849,11 +854,20 @@ class GHClient:
|
||||
failing = 0
|
||||
pending = 0
|
||||
failed_checks = []
|
||||
excluded_from_count = []
|
||||
|
||||
for check in checks:
|
||||
state = check.get("state", "").upper()
|
||||
name = check.get("name", "Unknown")
|
||||
|
||||
# Skip excluded checks (for stuck/broken CI checks)
|
||||
if name in excluded_set:
|
||||
excluded_from_count.append(name)
|
||||
logger.info(
|
||||
f"Excluding CI check '{name}' from counts (user-configured)"
|
||||
)
|
||||
continue
|
||||
|
||||
# gh pr checks 'state' directly contains: SUCCESS, FAILURE, PENDING, NEUTRAL, etc.
|
||||
if state in ("SUCCESS", "NEUTRAL", "SKIPPED"):
|
||||
passing += 1
|
||||
@@ -870,6 +884,7 @@ class GHClient:
|
||||
"failing": failing,
|
||||
"pending": pending,
|
||||
"failed_checks": failed_checks,
|
||||
"excluded_checks": excluded_from_count,
|
||||
}
|
||||
except (GHCommandError, GHTimeoutError, json.JSONDecodeError) as e:
|
||||
logger.warning(f"Failed to get PR checks for #{pr_number}: {e}")
|
||||
@@ -879,6 +894,7 @@ class GHClient:
|
||||
"failing": 0,
|
||||
"pending": 0,
|
||||
"failed_checks": [],
|
||||
"excluded_checks": [],
|
||||
"error": str(e),
|
||||
}
|
||||
|
||||
@@ -973,7 +989,9 @@ class GHClient:
|
||||
logger.warning(f"Failed to approve workflow run {run_id}: {e}")
|
||||
return False
|
||||
|
||||
async def get_pr_checks_comprehensive(self, pr_number: int) -> dict[str, Any]:
|
||||
async def get_pr_checks_comprehensive(
|
||||
self, pr_number: int, excluded_checks: list[str] | None = None
|
||||
) -> dict[str, Any]:
|
||||
"""
|
||||
Get comprehensive CI status including workflows awaiting approval.
|
||||
|
||||
@@ -983,12 +1001,13 @@ class GHClient:
|
||||
|
||||
Args:
|
||||
pr_number: PR number
|
||||
excluded_checks: List of check names to exclude from counts (for stuck/broken checks)
|
||||
|
||||
Returns:
|
||||
Dict with all check information including awaiting_approval count
|
||||
"""
|
||||
# Get standard checks
|
||||
checks = await self.get_pr_checks(pr_number)
|
||||
# Get standard checks (with exclusions applied)
|
||||
checks = await self.get_pr_checks(pr_number, excluded_checks=excluded_checks)
|
||||
|
||||
# Get workflows awaiting approval
|
||||
awaiting = await self.get_workflows_awaiting_approval(pr_number)
|
||||
|
||||
@@ -14,6 +14,7 @@ REFACTORED: Service layer architecture - orchestrator delegates to specialized s
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from collections.abc import Callable
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
@@ -156,6 +157,17 @@ class GitHubOrchestrator:
|
||||
# Initialize rate limiter singleton
|
||||
self.rate_limiter = RateLimiter.get_instance()
|
||||
|
||||
# Load excluded CI checks from environment (for stuck/broken checks)
|
||||
excluded_ci_env = os.environ.get("GITHUB_EXCLUDED_CI_CHECKS", "")
|
||||
self.excluded_ci_checks: list[str] = [
|
||||
check.strip() for check in excluded_ci_env.split(",") if check.strip()
|
||||
]
|
||||
if self.excluded_ci_checks:
|
||||
safe_print(
|
||||
f"[CI] Excluding checks from review: {', '.join(self.excluded_ci_checks)}",
|
||||
flush=True,
|
||||
)
|
||||
|
||||
# Initialize service layer
|
||||
self.pr_review_engine = PRReviewEngine(
|
||||
project_dir=self.project_dir,
|
||||
@@ -431,7 +443,10 @@ class GitHubOrchestrator:
|
||||
)
|
||||
|
||||
# Check CI status (comprehensive - includes workflows awaiting approval)
|
||||
ci_status = await self.gh_client.get_pr_checks_comprehensive(pr_number)
|
||||
# Excluded checks are filtered out based on user configuration
|
||||
ci_status = await self.gh_client.get_pr_checks_comprehensive(
|
||||
pr_number, excluded_checks=self.excluded_ci_checks
|
||||
)
|
||||
|
||||
# Log CI status with awaiting approval info
|
||||
awaiting = ci_status.get("awaiting_approval", 0)
|
||||
@@ -704,7 +719,10 @@ class GitHubOrchestrator:
|
||||
|
||||
# ALWAYS fetch current CI status to detect CI recovery
|
||||
# This must happen BEFORE the early return check to avoid stale CI verdicts
|
||||
ci_status = await self.gh_client.get_pr_checks_comprehensive(pr_number)
|
||||
# Excluded checks are filtered out based on user configuration
|
||||
ci_status = await self.gh_client.get_pr_checks_comprehensive(
|
||||
pr_number, excluded_checks=self.excluded_ci_checks
|
||||
)
|
||||
followup_context.ci_status = ci_status
|
||||
|
||||
if not has_commits and not has_file_changes:
|
||||
@@ -714,11 +732,14 @@ class GitHubOrchestrator:
|
||||
# If CI was failing before but now passes, we need to update the verdict
|
||||
current_failing = ci_status.get("failing", 0)
|
||||
current_awaiting = ci_status.get("awaiting_approval", 0)
|
||||
current_pending = ci_status.get("pending", 0)
|
||||
|
||||
# Helper to detect CI-related blockers (includes workflows pending)
|
||||
# Helper to detect CI-related blockers (includes workflows pending and CI pending)
|
||||
def is_ci_blocker(b: str) -> bool:
|
||||
return b.startswith("CI Failed:") or b.startswith(
|
||||
"Workflows Pending:"
|
||||
return (
|
||||
b.startswith("CI Failed:")
|
||||
or b.startswith("Workflows Pending:")
|
||||
or b.startswith("CI Pending:")
|
||||
)
|
||||
|
||||
previous_blockers = getattr(previous_review, "blockers", [])
|
||||
@@ -728,11 +749,13 @@ class GitHubOrchestrator:
|
||||
)
|
||||
|
||||
# Determine the appropriate verdict based on current CI status
|
||||
# CI/Workflow status check (both block merging)
|
||||
ci_or_workflow_blocking = current_failing > 0 or current_awaiting > 0
|
||||
# CI/Workflow/Pending status check (all block merging)
|
||||
ci_or_workflow_blocking = (
|
||||
current_failing > 0 or current_awaiting > 0 or current_pending > 0
|
||||
)
|
||||
|
||||
if ci_or_workflow_blocking:
|
||||
# CI is still failing or workflows pending - keep blocked verdict
|
||||
# CI is still failing, pending, or workflows pending - keep blocked verdict
|
||||
updated_verdict = MergeVerdict.BLOCKED
|
||||
if current_failing > 0:
|
||||
updated_reasoning = (
|
||||
@@ -749,6 +772,15 @@ class GitHubOrchestrator:
|
||||
f"No new commits since last review. "
|
||||
f"CI status: {current_failing} check(s) failing.{ci_note}"
|
||||
)
|
||||
elif current_pending > 0:
|
||||
updated_reasoning = (
|
||||
f"No code changes since last review. "
|
||||
f"{current_pending} CI check(s) still pending."
|
||||
)
|
||||
no_change_summary = (
|
||||
f"No new commits since last review. "
|
||||
f"CI status: {current_pending} check(s) still running."
|
||||
)
|
||||
else:
|
||||
updated_reasoning = (
|
||||
f"No code changes since last review. "
|
||||
@@ -837,6 +869,14 @@ class GitHubOrchestrator:
|
||||
if blocker_msg not in blockers:
|
||||
blockers.append(blocker_msg)
|
||||
|
||||
# Add pending CI checks to blockers
|
||||
if current_pending > 0:
|
||||
blocker_msg = (
|
||||
f"CI Pending: {current_pending} check(s) still running"
|
||||
)
|
||||
if blocker_msg not in blockers:
|
||||
blockers.append(blocker_msg)
|
||||
|
||||
# Map verdict to overall_status (consistent with rest of codebase)
|
||||
if updated_verdict == MergeVerdict.BLOCKED:
|
||||
overall_status = "request_changes"
|
||||
@@ -959,6 +999,36 @@ class GitHubOrchestrator:
|
||||
if ci_warning not in result.summary:
|
||||
result.summary += ci_warning
|
||||
|
||||
# Also check for pending CI checks
|
||||
pending_checks = followup_context.ci_status.get("pending", 0)
|
||||
if pending_checks > 0:
|
||||
safe_print(
|
||||
f"[Followup] CI checks still pending: {pending_checks}",
|
||||
flush=True,
|
||||
)
|
||||
# Override verdict if CI is pending
|
||||
if result.verdict in (
|
||||
MergeVerdict.READY_TO_MERGE,
|
||||
MergeVerdict.MERGE_WITH_CHANGES,
|
||||
):
|
||||
result.verdict = MergeVerdict.BLOCKED
|
||||
result.verdict_reasoning = (
|
||||
f"Blocked: {pending_checks} CI check(s) still running. "
|
||||
"Wait for CI to complete before merge."
|
||||
)
|
||||
result.overall_status = "request_changes"
|
||||
# Add pending CI to blockers
|
||||
blocker_msg = f"CI Pending: {pending_checks} check(s) still running"
|
||||
if blocker_msg not in result.blockers:
|
||||
result.blockers.append(blocker_msg)
|
||||
# Update summary to reflect CI status
|
||||
ci_pending_warning = (
|
||||
f"\n\n**⏳ CI Status:** {pending_checks} check(s) still running. "
|
||||
"Wait for CI to complete."
|
||||
)
|
||||
if ci_pending_warning not in result.summary:
|
||||
result.summary += ci_pending_warning
|
||||
|
||||
# Save result
|
||||
await result.save(self.github_dir)
|
||||
|
||||
|
||||
@@ -0,0 +1,163 @@
|
||||
"""
|
||||
Markdown Parsing Utilities for PR Reviews
|
||||
==========================================
|
||||
|
||||
Shared utilities for parsing markdown output from AI reviewers.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
import logging
|
||||
import re
|
||||
|
||||
try:
|
||||
from ..models import PRReviewFinding, ReviewCategory, ReviewSeverity
|
||||
from .category_utils import map_category
|
||||
except (ImportError, ValueError, SystemError):
|
||||
from models import PRReviewFinding, ReviewCategory, ReviewSeverity
|
||||
from services.category_utils import map_category
|
||||
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def parse_markdown_findings(
|
||||
output: str, context_name: str = "Review"
|
||||
) -> list[PRReviewFinding]:
|
||||
"""Parse findings from markdown output when JSON extraction fails.
|
||||
|
||||
This handles cases where the AI outputs findings in markdown format
|
||||
after structured output validation fails. Extracts findings from patterns like:
|
||||
- **CRITICAL - Code Duplication**
|
||||
- ### Critical Issues (Must Fix)
|
||||
- 1. **HIGH - Race condition in auth.ts:45**
|
||||
|
||||
Args:
|
||||
output: Markdown text output to parse
|
||||
context_name: Name of the context (e.g., "ParallelOrchestrator", "ParallelFollowup")
|
||||
|
||||
Returns:
|
||||
List of PRReviewFinding instances extracted from markdown
|
||||
"""
|
||||
findings: list[PRReviewFinding] = []
|
||||
|
||||
# Normalize line endings
|
||||
output = output.replace("\r\n", "\n")
|
||||
|
||||
# Pattern: **SEVERITY - Title** or **SEVERITY: Title** or numbered lists
|
||||
numbered_pattern = r"(?:\d+\.\s*)?\*\*(\w+)\s*[-:–]\s*([^*]+)\*\*"
|
||||
|
||||
# Track positions to avoid duplicates
|
||||
found_titles: set[str] = set()
|
||||
|
||||
# Find all severity-title matches
|
||||
for match in re.finditer(numbered_pattern, output, re.IGNORECASE):
|
||||
severity_str = match.group(1).strip().upper()
|
||||
title = match.group(2).strip()
|
||||
|
||||
# Skip if already found this title (avoid duplicates)
|
||||
title_key = title.lower()[:50]
|
||||
if title_key in found_titles:
|
||||
continue
|
||||
found_titles.add(title_key)
|
||||
|
||||
# Map severity string to enum
|
||||
severity_map = {
|
||||
"CRITICAL": ReviewSeverity.CRITICAL,
|
||||
"HIGH": ReviewSeverity.HIGH,
|
||||
"MEDIUM": ReviewSeverity.MEDIUM,
|
||||
"MED": ReviewSeverity.MEDIUM,
|
||||
"LOW": ReviewSeverity.LOW,
|
||||
"SUGGESTION": ReviewSeverity.LOW,
|
||||
}
|
||||
severity = severity_map.get(severity_str, ReviewSeverity.MEDIUM)
|
||||
|
||||
# Try to extract file and line from title or nearby text
|
||||
file_line_match = re.search(
|
||||
r"[`(]?([a-zA-Z0-9_./\-]+\.(?:ts|tsx|js|jsx|py|go|rs|java|rb|c|cpp|h|hpp|swift|kt|scala|php|vue|svelte)):(\d+)[`)]?",
|
||||
title + output[match.end() : match.end() + 200],
|
||||
)
|
||||
|
||||
file_path = "unknown"
|
||||
line_num = 0
|
||||
if file_line_match:
|
||||
file_path = file_line_match.group(1)
|
||||
line_num = int(file_line_match.group(2))
|
||||
|
||||
# Extract description - text following the title until next heading or finding
|
||||
desc_start = match.end()
|
||||
desc_end_patterns = [
|
||||
r"\n\s*\*\*\w+\s*[-:–]", # Next finding
|
||||
r"\n\s*#{1,3}\s", # Next heading
|
||||
r"\n\s*\d+\.\s*\*\*", # Next numbered item
|
||||
r"\n\s*---", # Horizontal rule
|
||||
]
|
||||
desc_end = len(output)
|
||||
for pattern in desc_end_patterns:
|
||||
end_match = re.search(pattern, output[desc_start:])
|
||||
if end_match:
|
||||
desc_end = min(desc_end, desc_start + end_match.start())
|
||||
|
||||
description = output[desc_start:desc_end].strip()
|
||||
description = re.sub(r"^\s*[-*]\s*", "", description, flags=re.MULTILINE)
|
||||
description = description[:500] # Limit length
|
||||
|
||||
# Infer category from title/description
|
||||
category_keywords = {
|
||||
"security": [
|
||||
"security",
|
||||
"injection",
|
||||
"xss",
|
||||
"auth",
|
||||
"credential",
|
||||
"secret",
|
||||
],
|
||||
"redundancy": [
|
||||
"duplication",
|
||||
"redundant",
|
||||
"duplicate",
|
||||
"similar",
|
||||
"copy",
|
||||
],
|
||||
"performance": ["performance", "slow", "optimize", "memory", "leak"],
|
||||
"logic": ["logic", "race", "condition", "edge case", "bug", "error"],
|
||||
"quality": ["quality", "maintainability", "readability", "complexity"],
|
||||
"test": ["test", "coverage", "assertion"],
|
||||
"docs": ["doc", "comment", "readme"],
|
||||
"regression": ["regression", "broke", "revert"],
|
||||
"incomplete_fix": ["incomplete", "partial", "still"],
|
||||
}
|
||||
|
||||
category = ReviewCategory.QUALITY # Default
|
||||
title_desc_lower = (title + " " + description).lower()
|
||||
for cat, keywords in category_keywords.items():
|
||||
if any(kw in title_desc_lower for kw in keywords):
|
||||
category = map_category(cat)
|
||||
break
|
||||
|
||||
# Generate finding ID
|
||||
finding_id = hashlib.md5(
|
||||
f"{file_path}:{line_num}:{title[:50]}".encode(),
|
||||
usedforsecurity=False,
|
||||
).hexdigest()[:12]
|
||||
|
||||
finding = PRReviewFinding(
|
||||
id=finding_id,
|
||||
file=file_path,
|
||||
line=line_num,
|
||||
title=title[:80],
|
||||
description=description if description else title,
|
||||
category=category,
|
||||
severity=severity,
|
||||
suggested_fix="",
|
||||
evidence=None,
|
||||
)
|
||||
findings.append(finding)
|
||||
|
||||
if findings:
|
||||
logger.info(
|
||||
f"[{context_name}] Extracted {len(findings)} findings from markdown output"
|
||||
)
|
||||
|
||||
return findings
|
||||
@@ -46,6 +46,7 @@ try:
|
||||
from .agent_utils import create_working_dir_injector
|
||||
from .category_utils import map_category
|
||||
from .io_utils import safe_print
|
||||
from .markdown_utils import parse_markdown_findings
|
||||
from .pr_worktree_manager import PRWorktreeManager
|
||||
from .pydantic_models import ParallelFollowupResponse
|
||||
from .sdk_utils import process_sdk_stream
|
||||
@@ -66,6 +67,7 @@ except (ImportError, ValueError, SystemError):
|
||||
from services.agent_utils import create_working_dir_injector
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
from services.markdown_utils import parse_markdown_findings
|
||||
from services.pr_worktree_manager import PRWorktreeManager
|
||||
from services.pydantic_models import ParallelFollowupResponse
|
||||
from services.sdk_utils import process_sdk_stream
|
||||
@@ -565,13 +567,27 @@ The SDK will run invoked agents in parallel automatically.
|
||||
)
|
||||
|
||||
# Check for stream processing errors
|
||||
if stream_result.get("error"):
|
||||
logger.error(
|
||||
f"[ParallelFollowup] SDK stream failed: {stream_result['error']}"
|
||||
)
|
||||
raise RuntimeError(
|
||||
f"SDK stream processing failed: {stream_result['error']}"
|
||||
)
|
||||
stream_error = stream_result.get("error")
|
||||
if stream_error:
|
||||
# Don't raise for structured_output_validation_failed - use text fallback instead
|
||||
if stream_error == "structured_output_validation_failed":
|
||||
logger.warning(
|
||||
"[ParallelFollowup] Structured output validation failed after retries. "
|
||||
"Will use text fallback to extract findings."
|
||||
)
|
||||
safe_print(
|
||||
"[ParallelFollowup] Structured output failed - using text fallback",
|
||||
flush=True,
|
||||
)
|
||||
# Clear structured_output to ensure fallback is used
|
||||
stream_result["structured_output"] = None
|
||||
else:
|
||||
logger.error(
|
||||
f"[ParallelFollowup] SDK stream failed: {stream_error}"
|
||||
)
|
||||
raise RuntimeError(
|
||||
f"SDK stream processing failed: {stream_error}"
|
||||
)
|
||||
|
||||
result_text = stream_result["result_text"]
|
||||
structured_output = stream_result["structured_output"]
|
||||
@@ -1047,12 +1063,35 @@ The SDK will run invoked agents in parallel automatically.
|
||||
|
||||
return self._create_empty_result()
|
||||
|
||||
def _parse_markdown_output(self, output: str) -> list[PRReviewFinding]:
|
||||
"""Parse findings from markdown output when JSON extraction fails.
|
||||
|
||||
Delegates to shared markdown parsing utility.
|
||||
|
||||
Args:
|
||||
output: Markdown text output to parse
|
||||
|
||||
Returns:
|
||||
List of PRReviewFinding instances extracted from markdown
|
||||
"""
|
||||
return parse_markdown_findings(output, context_name="ParallelFollowup")
|
||||
|
||||
def _parse_text_output(self, text: str, context: FollowupReviewContext) -> dict:
|
||||
"""Parse text output when structured output fails."""
|
||||
"""Parse text output when structured output fails.
|
||||
|
||||
Attempts markdown parsing to extract findings when the AI outputs
|
||||
findings in markdown format after structured output validation fails.
|
||||
"""
|
||||
logger.warning("[ParallelFollowup] Falling back to text parsing")
|
||||
|
||||
# Simple heuristic parsing
|
||||
findings = []
|
||||
# Try markdown parsing first to extract findings
|
||||
findings = self._parse_markdown_output(text)
|
||||
new_finding_ids = [f.id for f in findings]
|
||||
|
||||
if findings:
|
||||
logger.info(
|
||||
f"[ParallelFollowup] Extracted {len(findings)} findings via markdown fallback"
|
||||
)
|
||||
|
||||
# Look for verdict keywords
|
||||
text_lower = text.lower()
|
||||
@@ -1063,13 +1102,18 @@ The SDK will run invoked agents in parallel automatically.
|
||||
elif "needs revision" in text_lower or "request changes" in text_lower:
|
||||
verdict = MergeVerdict.NEEDS_REVISION
|
||||
else:
|
||||
verdict = MergeVerdict.MERGE_WITH_CHANGES
|
||||
# If we found findings, default to NEEDS_REVISION
|
||||
verdict = (
|
||||
MergeVerdict.NEEDS_REVISION
|
||||
if findings
|
||||
else MergeVerdict.MERGE_WITH_CHANGES
|
||||
)
|
||||
|
||||
return {
|
||||
"findings": findings,
|
||||
"resolved_ids": [],
|
||||
"unresolved_ids": [],
|
||||
"new_finding_ids": [],
|
||||
"new_finding_ids": new_finding_ids,
|
||||
"verdict": verdict,
|
||||
"verdict_reasoning": text[:500] if text else "Unable to parse response",
|
||||
}
|
||||
|
||||
@@ -48,6 +48,7 @@ try:
|
||||
from .agent_utils import create_working_dir_injector
|
||||
from .category_utils import map_category
|
||||
from .io_utils import safe_print
|
||||
from .markdown_utils import parse_markdown_findings
|
||||
from .pr_worktree_manager import PRWorktreeManager
|
||||
from .pydantic_models import (
|
||||
AgentAgreement,
|
||||
@@ -73,6 +74,7 @@ except (ImportError, ValueError, SystemError):
|
||||
from services.agent_utils import create_working_dir_injector
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
from services.markdown_utils import parse_markdown_findings
|
||||
from services.pr_worktree_manager import PRWorktreeManager
|
||||
from services.pydantic_models import (
|
||||
AgentAgreement,
|
||||
@@ -524,14 +526,27 @@ Report findings with specific file paths, line numbers, and code evidence.
|
||||
|
||||
error = stream_result.get("error")
|
||||
if error:
|
||||
logger.error(
|
||||
f"[Specialist:{config.name}] SDK stream failed: {error}"
|
||||
)
|
||||
safe_print(
|
||||
f"[Specialist:{config.name}] Analysis failed: {error}",
|
||||
flush=True,
|
||||
)
|
||||
return (config.name, [])
|
||||
# Don't bail out for structured_output_validation_failed - use text fallback
|
||||
if error == "structured_output_validation_failed":
|
||||
logger.warning(
|
||||
f"[Specialist:{config.name}] Structured output validation failed. "
|
||||
"Using text fallback."
|
||||
)
|
||||
safe_print(
|
||||
f"[Specialist:{config.name}] Structured output failed - using text fallback",
|
||||
flush=True,
|
||||
)
|
||||
# Clear structured_output to ensure fallback is used
|
||||
stream_result["structured_output"] = None
|
||||
else:
|
||||
logger.error(
|
||||
f"[Specialist:{config.name}] SDK stream failed: {error}"
|
||||
)
|
||||
safe_print(
|
||||
f"[Specialist:{config.name}] Analysis failed: {error}",
|
||||
flush=True,
|
||||
)
|
||||
return (config.name, [])
|
||||
|
||||
# Parse structured output
|
||||
structured_output = stream_result.get("structured_output")
|
||||
@@ -1385,6 +1400,19 @@ The SDK will run invoked agents in parallel automatically.
|
||||
|
||||
return None
|
||||
|
||||
def _parse_markdown_output(self, output: str) -> list[PRReviewFinding]:
|
||||
"""Parse findings from markdown output when JSON extraction fails.
|
||||
|
||||
Delegates to shared markdown parsing utility.
|
||||
|
||||
Args:
|
||||
output: Markdown text output to parse
|
||||
|
||||
Returns:
|
||||
List of PRReviewFinding instances extracted from markdown
|
||||
"""
|
||||
return parse_markdown_findings(output, context_name="ParallelOrchestrator")
|
||||
|
||||
def _create_finding_from_dict(self, f_data: dict[str, Any]) -> PRReviewFinding:
|
||||
"""Create a PRReviewFinding from dictionary data.
|
||||
|
||||
@@ -1419,23 +1447,39 @@ The SDK will run invoked agents in parallel automatically.
|
||||
)
|
||||
|
||||
def _parse_text_output(self, output: str) -> list[PRReviewFinding]:
|
||||
"""Parse findings from text output (fallback)."""
|
||||
findings = []
|
||||
"""Parse findings from text output (fallback).
|
||||
|
||||
Attempts JSON extraction first, then falls back to markdown parsing
|
||||
when structured output validation fails and AI outputs findings in
|
||||
markdown format.
|
||||
"""
|
||||
findings: list[PRReviewFinding] = []
|
||||
|
||||
try:
|
||||
# Extract JSON from text
|
||||
# First, try to extract JSON from text
|
||||
data = self._extract_json_from_text(output)
|
||||
if not data:
|
||||
if data:
|
||||
# Get findings array from JSON
|
||||
findings_data = data.get("findings", [])
|
||||
|
||||
# Convert each finding dict to PRReviewFinding
|
||||
for f_data in findings_data:
|
||||
finding = self._create_finding_from_dict(f_data)
|
||||
findings.append(finding)
|
||||
|
||||
if findings:
|
||||
return findings
|
||||
|
||||
# Fallback: Try markdown parsing when JSON extraction fails
|
||||
# This handles the case where structured output validation failed
|
||||
# and Claude output findings in markdown format instead
|
||||
findings = self._parse_markdown_output(output)
|
||||
if findings:
|
||||
logger.info(
|
||||
f"[ParallelOrchestrator] Extracted {len(findings)} findings via markdown fallback"
|
||||
)
|
||||
return findings
|
||||
|
||||
# Get findings array from JSON
|
||||
findings_data = data.get("findings", [])
|
||||
|
||||
# Convert each finding dict to PRReviewFinding
|
||||
for f_data in findings_data:
|
||||
finding = self._create_finding_from_dict(f_data)
|
||||
findings.append(finding)
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"[ParallelOrchestrator] Text parsing failed: {e}")
|
||||
|
||||
@@ -1749,25 +1793,34 @@ For EACH finding above:
|
||||
|
||||
error = stream_result.get("error")
|
||||
if error:
|
||||
# Check for specific error types that warrant retry
|
||||
error_str = str(error).lower()
|
||||
is_retryable = (
|
||||
"400" in error_str
|
||||
or "concurrency" in error_str
|
||||
or "circuit breaker" in error_str
|
||||
or "tool_use" in error_str
|
||||
)
|
||||
|
||||
if is_retryable and attempt < MAX_VALIDATION_RETRIES:
|
||||
# Handle structured output validation failure - use text fallback
|
||||
if error == "structured_output_validation_failed":
|
||||
logger.warning(
|
||||
f"[PRReview] Retryable validation error: {error}"
|
||||
"[PRReview:FindingValidator] Structured output validation failed. "
|
||||
"Will use text fallback to parse validation results."
|
||||
)
|
||||
# Clear structured_output to trigger text parsing below
|
||||
stream_result["structured_output"] = None
|
||||
else:
|
||||
# Check for specific error types that warrant retry
|
||||
error_str = str(error).lower()
|
||||
is_retryable = (
|
||||
"400" in error_str
|
||||
or "concurrency" in error_str
|
||||
or "circuit breaker" in error_str
|
||||
or "tool_use" in error_str
|
||||
)
|
||||
last_error = Exception(error)
|
||||
continue # Retry
|
||||
|
||||
logger.error(f"[PRReview] Validation failed: {error}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
if is_retryable and attempt < MAX_VALIDATION_RETRIES:
|
||||
logger.warning(
|
||||
f"[PRReview] Retryable validation error: {error}"
|
||||
)
|
||||
last_error = Exception(error)
|
||||
continue # Retry
|
||||
|
||||
logger.error(f"[PRReview] Validation failed: {error}")
|
||||
# Fail-safe: return original findings
|
||||
return findings
|
||||
|
||||
structured_output = stream_result.get("structured_output")
|
||||
|
||||
|
||||
@@ -26,7 +26,87 @@ from __future__ import annotations
|
||||
|
||||
from typing import Literal
|
||||
|
||||
from pydantic import BaseModel, Field
|
||||
from pydantic import BaseModel, Field, field_validator
|
||||
|
||||
# =============================================================================
|
||||
# Enum Normalization Helpers
|
||||
# =============================================================================
|
||||
# These help the AI produce valid output by mapping common synonyms/typos to
|
||||
# the exact values required by the schema. This reduces structured output
|
||||
# validation failures where the AI uses natural language like "duplication"
|
||||
# when the schema requires "redundancy".
|
||||
|
||||
|
||||
def normalize_category(value: str) -> str:
|
||||
"""Normalize category value to match schema requirements.
|
||||
|
||||
Maps common synonyms and typos to valid category values.
|
||||
"""
|
||||
if not isinstance(value, str):
|
||||
return value
|
||||
|
||||
# Map synonyms to valid category values
|
||||
synonyms = {
|
||||
# Redundancy synonyms
|
||||
"duplication": "redundancy",
|
||||
"code duplication": "redundancy",
|
||||
"duplicate": "redundancy",
|
||||
"duplicated": "redundancy",
|
||||
"copy": "redundancy",
|
||||
"copied": "redundancy",
|
||||
# Performance synonyms
|
||||
"perf": "performance",
|
||||
"slow": "performance",
|
||||
"optimization": "performance",
|
||||
# Quality synonyms
|
||||
"code quality": "quality",
|
||||
"maintainability": "quality",
|
||||
# Logic synonyms
|
||||
"correctness": "logic",
|
||||
"bug": "logic",
|
||||
# Test synonyms
|
||||
"testing": "test",
|
||||
"tests": "test",
|
||||
# Docs synonyms
|
||||
"documentation": "docs",
|
||||
"doc": "docs",
|
||||
# Security synonyms
|
||||
"sec": "security",
|
||||
"vulnerability": "security",
|
||||
# Style (map to quality or pattern)
|
||||
"style": "quality",
|
||||
"formatting": "quality",
|
||||
}
|
||||
|
||||
normalized = value.lower().strip()
|
||||
return synonyms.get(normalized, normalized)
|
||||
|
||||
|
||||
def normalize_verdict(value: str) -> str:
|
||||
"""Normalize verdict value to match schema requirements.
|
||||
|
||||
Handles common format variations like spaces, hyphens, and case sensitivity.
|
||||
"""
|
||||
if not isinstance(value, str):
|
||||
return value
|
||||
|
||||
# Normalize: uppercase, replace spaces/hyphens with underscores
|
||||
normalized = value.upper().strip().replace(" ", "_").replace("-", "_")
|
||||
|
||||
# Map common variations
|
||||
synonyms = {
|
||||
"READY": "READY_TO_MERGE",
|
||||
"MERGE": "READY_TO_MERGE",
|
||||
"APPROVED": "APPROVE",
|
||||
"NEEDS_CHANGES": "NEEDS_REVISION",
|
||||
"REQUEST_CHANGES": "NEEDS_REVISION",
|
||||
"CHANGES_REQUESTED": "NEEDS_REVISION",
|
||||
"BLOCK": "BLOCKED",
|
||||
"REJECT": "BLOCKED",
|
||||
}
|
||||
|
||||
return synonyms.get(normalized, normalized)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Verification Evidence (Required for All Findings)
|
||||
@@ -112,11 +192,22 @@ class QualityFinding(BaseFinding):
|
||||
|
||||
category: Literal[
|
||||
"redundancy", "quality", "test", "performance", "pattern", "docs"
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. MUST be exactly one of: redundancy, quality, test, "
|
||||
"performance, pattern, docs. Use 'redundancy' for code duplication "
|
||||
"(NOT 'duplication')."
|
||||
)
|
||||
)
|
||||
redundant_with: str | None = Field(
|
||||
None, description="Reference to duplicate code (file:line) if redundant"
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class DeepAnalysisFinding(BaseFinding):
|
||||
"""A finding from deep analysis with verification info."""
|
||||
@@ -128,11 +219,20 @@ class DeepAnalysisFinding(BaseFinding):
|
||||
"pattern",
|
||||
"performance",
|
||||
"logic",
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. Use 'redundancy' for code duplication (NOT 'duplication')."
|
||||
)
|
||||
)
|
||||
verification_note: str | None = Field(
|
||||
None, description="What evidence is missing or couldn't be verified"
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class StructuralIssue(BaseModel):
|
||||
"""A structural issue with the PR."""
|
||||
@@ -194,7 +294,10 @@ class FollowupFinding(BaseModel):
|
||||
description="Issue severity level"
|
||||
)
|
||||
category: Literal["security", "quality", "logic", "test", "docs"] = Field(
|
||||
description="Issue category"
|
||||
description=(
|
||||
"Issue category. MUST be exactly one of: security, quality, logic, "
|
||||
"test, docs."
|
||||
)
|
||||
)
|
||||
title: str = Field(description="Brief issue title")
|
||||
description: str = Field(description="Detailed explanation of the issue")
|
||||
@@ -206,6 +309,11 @@ class FollowupFinding(BaseModel):
|
||||
description="Evidence that this finding was verified against actual code"
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class FollowupReviewResponse(BaseModel):
|
||||
"""Complete response schema for follow-up PR review."""
|
||||
@@ -222,9 +330,19 @@ class FollowupReviewResponse(BaseModel):
|
||||
)
|
||||
verdict: Literal[
|
||||
"READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED"
|
||||
] = Field(description="Overall merge verdict")
|
||||
] = Field(
|
||||
description=(
|
||||
"Overall merge verdict. MUST be exactly one of: READY_TO_MERGE, "
|
||||
"MERGE_WITH_CHANGES, NEEDS_REVISION, BLOCKED. Use underscores, not spaces."
|
||||
)
|
||||
)
|
||||
verdict_reasoning: str = Field(description="Explanation for the verdict")
|
||||
|
||||
@field_validator("verdict", mode="before")
|
||||
@classmethod
|
||||
def _normalize_verdict(cls, v: str) -> str:
|
||||
return normalize_verdict(v)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Initial Review Responses (Multi-Pass)
|
||||
@@ -289,9 +407,19 @@ class StructuralPassResult(BaseModel):
|
||||
)
|
||||
verdict: Literal[
|
||||
"READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED"
|
||||
] = Field(description="Structural verdict")
|
||||
] = Field(
|
||||
description=(
|
||||
"Structural verdict. MUST be exactly one of: READY_TO_MERGE, "
|
||||
"MERGE_WITH_CHANGES, NEEDS_REVISION, BLOCKED."
|
||||
)
|
||||
)
|
||||
verdict_reasoning: str = Field(description="Explanation for the verdict")
|
||||
|
||||
@field_validator("verdict", mode="before")
|
||||
@classmethod
|
||||
def _normalize_verdict(cls, v: str) -> str:
|
||||
return normalize_verdict(v)
|
||||
|
||||
|
||||
class AICommentTriageResult(BaseModel):
|
||||
"""Result from AI comment triage pass."""
|
||||
@@ -366,7 +494,11 @@ class OrchestratorFinding(BaseModel):
|
||||
"performance",
|
||||
"logic",
|
||||
"test",
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. Use 'redundancy' for code duplication (NOT 'duplication')."
|
||||
)
|
||||
)
|
||||
severity: Literal["critical", "high", "medium", "low"] = Field(
|
||||
description="Issue severity level"
|
||||
)
|
||||
@@ -379,19 +511,34 @@ class OrchestratorFinding(BaseModel):
|
||||
description="Evidence that this finding was verified against actual code"
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class OrchestratorReviewResponse(BaseModel):
|
||||
"""Complete response schema for orchestrator PR review."""
|
||||
|
||||
verdict: Literal[
|
||||
"READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED"
|
||||
] = Field(description="Overall merge verdict")
|
||||
] = Field(
|
||||
description=(
|
||||
"Overall merge verdict. MUST be exactly one of: READY_TO_MERGE, "
|
||||
"MERGE_WITH_CHANGES, NEEDS_REVISION, BLOCKED."
|
||||
)
|
||||
)
|
||||
verdict_reasoning: str = Field(description="Explanation for the verdict")
|
||||
findings: list[OrchestratorFinding] = Field(
|
||||
default_factory=list, description="Issues found during review"
|
||||
)
|
||||
summary: str = Field(description="Brief summary of the review")
|
||||
|
||||
@field_validator("verdict", mode="before")
|
||||
@classmethod
|
||||
def _normalize_verdict(cls, v: str) -> str:
|
||||
return normalize_verdict(v)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Parallel Orchestrator Review Response (SDK Subagents)
|
||||
@@ -446,7 +593,13 @@ class ParallelOrchestratorFinding(BaseModel):
|
||||
"redundancy",
|
||||
"pattern",
|
||||
"performance",
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. MUST be exactly one of: security, quality, logic, "
|
||||
"codebase_fit, test, docs, redundancy, pattern, performance. "
|
||||
"Use 'redundancy' for code duplication (NOT 'duplication')."
|
||||
)
|
||||
)
|
||||
severity: Literal["critical", "high", "medium", "low"] = Field(
|
||||
description="Issue severity level"
|
||||
)
|
||||
@@ -482,6 +635,11 @@ class ParallelOrchestratorFinding(BaseModel):
|
||||
False, description="Whether multiple agents agreed on this finding"
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class AgentAgreement(BaseModel):
|
||||
"""Tracks agreement between agents on findings."""
|
||||
@@ -545,7 +703,12 @@ class SpecialistFinding(BaseModel):
|
||||
)
|
||||
category: Literal[
|
||||
"security", "quality", "logic", "performance", "pattern", "test", "docs"
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. MUST be exactly one of: security, quality, logic, "
|
||||
"performance, pattern, test, docs."
|
||||
)
|
||||
)
|
||||
title: str = Field(description="Brief issue title (max 80 chars)")
|
||||
description: str = Field(description="Detailed explanation of the issue")
|
||||
file: str = Field(description="File path where issue was found")
|
||||
@@ -561,6 +724,11 @@ class SpecialistFinding(BaseModel):
|
||||
description="True if this is about affected code outside the PR (callers, dependencies)",
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class SpecialistResponse(BaseModel):
|
||||
"""Response schema for individual specialist agent (parallel SDK sessions).
|
||||
@@ -612,10 +780,18 @@ class ParallelOrchestratorResponse(BaseModel):
|
||||
description="Information about agent agreement on findings",
|
||||
)
|
||||
verdict: Literal["APPROVE", "COMMENT", "NEEDS_REVISION", "BLOCKED"] = Field(
|
||||
description="Overall PR verdict"
|
||||
description=(
|
||||
"Overall PR verdict. MUST be exactly one of: APPROVE, COMMENT, "
|
||||
"NEEDS_REVISION, BLOCKED. Use underscores, not spaces."
|
||||
)
|
||||
)
|
||||
verdict_reasoning: str = Field(description="Explanation for the verdict")
|
||||
|
||||
@field_validator("verdict", mode="before")
|
||||
@classmethod
|
||||
def _normalize_verdict(cls, v: str) -> str:
|
||||
return normalize_verdict(v)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Parallel Follow-up Review Response (SDK Subagents for Follow-up)
|
||||
@@ -655,7 +831,12 @@ class ParallelFollowupFinding(BaseModel):
|
||||
"docs",
|
||||
"regression",
|
||||
"incomplete_fix",
|
||||
] = Field(description="Issue category")
|
||||
] = Field(
|
||||
description=(
|
||||
"Issue category. MUST be exactly one of: security, quality, logic, "
|
||||
"test, docs, regression, incomplete_fix."
|
||||
)
|
||||
)
|
||||
severity: Literal["critical", "high", "medium", "low"] = Field(
|
||||
description="Issue severity level"
|
||||
)
|
||||
@@ -683,6 +864,11 @@ class ParallelFollowupFinding(BaseModel):
|
||||
),
|
||||
)
|
||||
|
||||
@field_validator("category", mode="before")
|
||||
@classmethod
|
||||
def _normalize_category(cls, v: str) -> str:
|
||||
return normalize_category(v)
|
||||
|
||||
|
||||
class CommentAnalysis(BaseModel):
|
||||
"""Analysis of a contributor or AI comment."""
|
||||
@@ -754,9 +940,19 @@ class ParallelFollowupResponse(BaseModel):
|
||||
# Verdict
|
||||
verdict: Literal[
|
||||
"READY_TO_MERGE", "MERGE_WITH_CHANGES", "NEEDS_REVISION", "BLOCKED"
|
||||
] = Field(description="Overall merge verdict")
|
||||
] = Field(
|
||||
description=(
|
||||
"Overall merge verdict. MUST be exactly one of: READY_TO_MERGE, "
|
||||
"MERGE_WITH_CHANGES, NEEDS_REVISION, BLOCKED. Use underscores, not spaces."
|
||||
)
|
||||
)
|
||||
verdict_reasoning: str = Field(description="Explanation for the verdict")
|
||||
|
||||
@field_validator("verdict", mode="before")
|
||||
@classmethod
|
||||
def _normalize_verdict(cls, v: str) -> str:
|
||||
return normalize_verdict(v)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Finding Validation Response (Re-investigation of unresolved findings)
|
||||
|
||||
@@ -460,12 +460,17 @@ async def process_sdk_stream(
|
||||
)
|
||||
if subtype == "error_max_structured_output_retries":
|
||||
# SDK failed to produce valid structured output after retries
|
||||
# Log ALL available fields for debugging - helps diagnose enum mismatches
|
||||
result_detail = getattr(msg, "result", None)
|
||||
is_error = getattr(msg, "is_error", False)
|
||||
logger.warning(
|
||||
f"[{context_name}] Claude could not produce valid structured output "
|
||||
f"after maximum retries - schema validation failed"
|
||||
f"after maximum retries - schema validation failed. "
|
||||
f"is_error={is_error}, result_preview={str(result_detail)[:500] if result_detail else 'None'}"
|
||||
)
|
||||
safe_print(
|
||||
f"[{context_name}] WARNING: Structured output validation failed after retries"
|
||||
f"[{context_name}] WARNING: Structured output validation failed after retries. "
|
||||
f"Result preview: {str(result_detail)[:300] if result_detail else 'None'}"
|
||||
)
|
||||
if not stream_error:
|
||||
stream_error = "structured_output_validation_failed"
|
||||
|
||||
@@ -54,6 +54,24 @@ import {
|
||||
getEmailFromConfigDir
|
||||
} from './claude-profile/profile-utils';
|
||||
|
||||
/**
|
||||
* Debug flag - only log verbose profile operations when DEBUG=true
|
||||
*/
|
||||
const IS_DEBUG = process.env.DEBUG === 'true';
|
||||
|
||||
/**
|
||||
* Debug log helper - only logs when DEBUG=true
|
||||
*/
|
||||
function debugLog(message: string, data?: unknown): void {
|
||||
if (IS_DEBUG) {
|
||||
if (data !== undefined) {
|
||||
console.warn(message, data);
|
||||
} else {
|
||||
console.warn(message);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Manages Claude Code profiles for multi-account support.
|
||||
* Profiles are stored in the app's userData directory.
|
||||
@@ -121,7 +139,7 @@ export class ClaudeProfileManager {
|
||||
const configEmail = getEmailFromConfigDir(profile.configDir);
|
||||
|
||||
if (configEmail && profile.email !== configEmail) {
|
||||
console.warn('[ClaudeProfileManager] Migrating corrupted email for profile:', {
|
||||
debugLog('[ClaudeProfileManager] Migrating corrupted email for profile:', {
|
||||
profileId: profile.id,
|
||||
oldEmail: profile.email,
|
||||
newEmail: configEmail
|
||||
@@ -133,7 +151,7 @@ export class ClaudeProfileManager {
|
||||
|
||||
if (needsSave) {
|
||||
this.save();
|
||||
console.warn('[ClaudeProfileManager] Email migration complete');
|
||||
debugLog('[ClaudeProfileManager] Email migration complete');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -168,7 +186,7 @@ export class ClaudeProfileManager {
|
||||
|
||||
if (result.subscriptionTypeUpdated) {
|
||||
needsSave = true;
|
||||
console.warn('[ClaudeProfileManager] Populated subscriptionType for profile:', {
|
||||
debugLog('[ClaudeProfileManager] Populated subscriptionType for profile:', {
|
||||
profileId: profile.id,
|
||||
subscriptionType: result.subscriptionType
|
||||
});
|
||||
@@ -176,7 +194,7 @@ export class ClaudeProfileManager {
|
||||
|
||||
if (result.rateLimitTierUpdated) {
|
||||
needsSave = true;
|
||||
console.warn('[ClaudeProfileManager] Populated rateLimitTier for profile:', {
|
||||
debugLog('[ClaudeProfileManager] Populated rateLimitTier for profile:', {
|
||||
profileId: profile.id,
|
||||
rateLimitTier: result.rateLimitTier
|
||||
});
|
||||
@@ -185,7 +203,7 @@ export class ClaudeProfileManager {
|
||||
|
||||
if (needsSave) {
|
||||
this.save();
|
||||
console.warn('[ClaudeProfileManager] Subscription metadata population complete');
|
||||
debugLog('[ClaudeProfileManager] Subscription metadata population complete');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -203,7 +221,7 @@ export class ClaudeProfileManager {
|
||||
const loadedData = loadProfileStore(this.storePath);
|
||||
if (loadedData) {
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] Loaded profiles:', {
|
||||
debugLog('[ClaudeProfileManager] Loaded profiles:', {
|
||||
count: loadedData.profiles.length,
|
||||
activeProfileId: loadedData.activeProfileId,
|
||||
profiles: loadedData.profiles.map(p => ({
|
||||
@@ -329,19 +347,12 @@ export class ClaudeProfileManager {
|
||||
// Fallback to default
|
||||
const defaultProfile = this.data.profiles.find(p => p.isDefault);
|
||||
if (defaultProfile) {
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] getActiveProfile - using default:', {
|
||||
id: defaultProfile.id,
|
||||
name: defaultProfile.name,
|
||||
email: defaultProfile.email
|
||||
});
|
||||
}
|
||||
return defaultProfile;
|
||||
}
|
||||
// If somehow no default exists, return first profile
|
||||
const fallback = this.data.profiles[0];
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] getActiveProfile - using fallback:', {
|
||||
debugLog('[ClaudeProfileManager] getActiveProfile - using fallback:', {
|
||||
id: fallback.id,
|
||||
name: fallback.name,
|
||||
email: fallback.email
|
||||
@@ -351,7 +362,7 @@ export class ClaudeProfileManager {
|
||||
}
|
||||
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] getActiveProfile:', {
|
||||
debugLog('[ClaudeProfileManager] getActiveProfile:', {
|
||||
id: active.id,
|
||||
name: active.name,
|
||||
email: active.email
|
||||
@@ -442,12 +453,12 @@ export class ClaudeProfileManager {
|
||||
const previousProfileId = this.data.activeProfileId;
|
||||
const profile = this.getProfile(profileId);
|
||||
if (!profile) {
|
||||
console.warn('[ClaudeProfileManager] setActiveProfile failed - profile not found:', { profileId });
|
||||
debugLog('[ClaudeProfileManager] setActiveProfile failed - profile not found:', { profileId });
|
||||
return false;
|
||||
}
|
||||
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] setActiveProfile:', {
|
||||
debugLog('[ClaudeProfileManager] setActiveProfile:', {
|
||||
from: previousProfileId,
|
||||
to: profileId,
|
||||
profileName: profile.name
|
||||
@@ -562,10 +573,10 @@ export class ClaudeProfileManager {
|
||||
|
||||
env.CLAUDE_CONFIG_DIR = expandedConfigDir;
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] Using CLAUDE_CONFIG_DIR for profile:', profile.name, expandedConfigDir);
|
||||
debugLog('[ClaudeProfileManager] Using CLAUDE_CONFIG_DIR for profile:', { profileName: profile.name, configDir: expandedConfigDir });
|
||||
}
|
||||
} else {
|
||||
console.warn('[ClaudeProfileManager] Profile has no configDir configured:', profile?.name);
|
||||
debugLog('[ClaudeProfileManager] Profile has no configDir configured:', profile?.name);
|
||||
}
|
||||
|
||||
return env;
|
||||
@@ -785,7 +796,7 @@ export class ClaudeProfileManager {
|
||||
);
|
||||
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] getProfileEnv:', {
|
||||
debugLog('[ClaudeProfileManager] getProfileEnv:', {
|
||||
profileId,
|
||||
profileName: profile.name,
|
||||
isDefault: profile.isDefault,
|
||||
@@ -806,7 +817,7 @@ export class ClaudeProfileManager {
|
||||
if (credentials.token) {
|
||||
env.CLAUDE_CODE_OAUTH_TOKEN = credentials.token;
|
||||
if (process.env.DEBUG === 'true') {
|
||||
console.warn('[ClaudeProfileManager] Retrieved OAuth token from Keychain for profile:', profile.name);
|
||||
debugLog('[ClaudeProfileManager] Retrieved OAuth token from Keychain for profile:', profile.name);
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
@@ -862,7 +873,7 @@ export class ClaudeProfileManager {
|
||||
}
|
||||
|
||||
this.save();
|
||||
console.warn('[ClaudeProfileManager] Cleared migrated profile:', profileId);
|
||||
debugLog('[ClaudeProfileManager] Cleared migrated profile:', profileId);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -36,6 +36,25 @@ function getTokenFingerprint(token: string | null | undefined): string {
|
||||
return token.slice(0, 8) + '...' + token.slice(-4);
|
||||
}
|
||||
|
||||
/**
|
||||
* Debug flag - only log verbose credential operations when DEBUG=true
|
||||
*/
|
||||
const IS_DEBUG = process.env.DEBUG === 'true';
|
||||
|
||||
/**
|
||||
* Debug log helper - only logs when DEBUG=true
|
||||
* Uses console.warn to ensure visibility in Electron's main process
|
||||
*/
|
||||
function debugLog(message: string, ...args: unknown[]): void {
|
||||
if (IS_DEBUG) {
|
||||
if (args.length > 0) {
|
||||
console.warn(message, ...args);
|
||||
} else {
|
||||
console.warn(message);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape a string for safe interpolation into PowerShell double-quoted strings.
|
||||
* Escapes all PowerShell special characters to prevent injection attacks.
|
||||
@@ -193,7 +212,7 @@ export function getKeychainServiceName(configDir?: string): string {
|
||||
// No configDir provided - this should not happen with isolated profiles
|
||||
// Fall back to unhashed name for backwards compatibility during migration
|
||||
if (!configDir) {
|
||||
console.warn('[CredentialUtils] getKeychainServiceName called without configDir - using legacy fallback');
|
||||
debugLog('[CredentialUtils] getKeychainServiceName called without configDir - using legacy fallback');
|
||||
return 'Claude Code-credentials';
|
||||
}
|
||||
|
||||
@@ -396,13 +415,13 @@ function parseCredentialJson<T extends PlatformCredentials>(
|
||||
try {
|
||||
data = JSON.parse(credentialsJson);
|
||||
} catch {
|
||||
console.warn(`[CredentialUtils] Failed to parse credential JSON for ${identifier}`);
|
||||
debugLog(`[CredentialUtils] Failed to parse credential JSON for ${identifier}`);
|
||||
return extractFn({}) as T;
|
||||
}
|
||||
|
||||
// Validate JSON structure
|
||||
if (!validateCredentialData(data)) {
|
||||
console.warn(`[CredentialUtils] Invalid credential data structure for ${identifier}`);
|
||||
debugLog(`[CredentialUtils] Invalid credential data structure for ${identifier}`);
|
||||
return extractFn({}) as T;
|
||||
}
|
||||
|
||||
@@ -439,7 +458,7 @@ function getCredentialsFromFile(
|
||||
if ((now - cached.timestamp) < ttl) {
|
||||
if (isDebug) {
|
||||
const cacheAge = now - cached.timestamp;
|
||||
console.warn(`[CredentialUtils:${logPrefix}:CACHE] Returning cached credentials:`, {
|
||||
debugLog(`[CredentialUtils:${logPrefix}:CACHE] Returning cached credentials:`, {
|
||||
credentialsPath,
|
||||
hasToken: !!cached.credentials.token,
|
||||
tokenFingerprint: getTokenFingerprint(cached.credentials.token),
|
||||
@@ -453,7 +472,7 @@ function getCredentialsFromFile(
|
||||
// Defense-in-depth: Validate credentials path is within expected boundaries
|
||||
if (!isValidCredentialsPath(credentialsPath)) {
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid credentials path rejected:`, { credentialsPath });
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid credentials path rejected:`, { credentialsPath });
|
||||
}
|
||||
const invalidResult = { token: null, email: null, error: 'Invalid credentials path' };
|
||||
credentialCache.set(cacheKey, { credentials: invalidResult, timestamp: now });
|
||||
@@ -463,7 +482,7 @@ function getCredentialsFromFile(
|
||||
// Check if credentials file exists
|
||||
if (!existsSync(credentialsPath)) {
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Credentials file not found:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Credentials file not found:`, credentialsPath);
|
||||
}
|
||||
const notFoundResult = { token: null, email: null };
|
||||
credentialCache.set(cacheKey, { credentials: notFoundResult, timestamp: now });
|
||||
@@ -478,7 +497,7 @@ function getCredentialsFromFile(
|
||||
try {
|
||||
data = JSON.parse(content);
|
||||
} catch {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Failed to parse credentials JSON:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Failed to parse credentials JSON:`, credentialsPath);
|
||||
const errorResult = { token: null, email: null };
|
||||
credentialCache.set(cacheKey, { credentials: errorResult, timestamp: now });
|
||||
return errorResult;
|
||||
@@ -486,7 +505,7 @@ function getCredentialsFromFile(
|
||||
|
||||
// Validate JSON structure
|
||||
if (!validateCredentialData(data)) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid credentials data structure:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid credentials data structure:`, credentialsPath);
|
||||
const invalidResult = { token: null, email: null };
|
||||
credentialCache.set(cacheKey, { credentials: invalidResult, timestamp: now });
|
||||
return invalidResult;
|
||||
@@ -496,7 +515,7 @@ function getCredentialsFromFile(
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid token format in:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid token format in:`, credentialsPath);
|
||||
const result = { token: null, email };
|
||||
credentialCache.set(cacheKey, { credentials: result, timestamp: now });
|
||||
return result;
|
||||
@@ -506,7 +525,7 @@ function getCredentialsFromFile(
|
||||
credentialCache.set(cacheKey, { credentials, timestamp: now });
|
||||
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Retrieved credentials from file:`, credentialsPath, {
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Retrieved credentials from file:`, credentialsPath, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
tokenFingerprint: getTokenFingerprint(token),
|
||||
@@ -516,7 +535,7 @@ function getCredentialsFromFile(
|
||||
return credentials;
|
||||
} catch (error) {
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Failed to read credentials file:`, credentialsPath, errorMessage);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Failed to read credentials file:`, credentialsPath, errorMessage);
|
||||
const errorResult = { token: null, email: null, error: `Failed to read credentials: ${errorMessage}` };
|
||||
credentialCache.set(cacheKey, { credentials: errorResult, timestamp: now });
|
||||
return errorResult;
|
||||
@@ -540,7 +559,7 @@ function getFullCredentialsFromFile(
|
||||
// Defense-in-depth: Validate credentials path is within expected boundaries
|
||||
if (!isValidCredentialsPath(credentialsPath)) {
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid credentials path rejected:`, { credentialsPath });
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid credentials path rejected:`, { credentialsPath });
|
||||
}
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: 'Invalid credentials path' };
|
||||
}
|
||||
@@ -548,7 +567,7 @@ function getFullCredentialsFromFile(
|
||||
// Check if credentials file exists
|
||||
if (!existsSync(credentialsPath)) {
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Credentials file not found:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Credentials file not found:`, credentialsPath);
|
||||
}
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null };
|
||||
}
|
||||
@@ -561,13 +580,13 @@ function getFullCredentialsFromFile(
|
||||
try {
|
||||
data = JSON.parse(content);
|
||||
} catch {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Failed to parse credentials JSON:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Failed to parse credentials JSON:`, credentialsPath);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null };
|
||||
}
|
||||
|
||||
// Validate JSON structure
|
||||
if (!validateCredentialData(data)) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid credentials data structure:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid credentials data structure:`, credentialsPath);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null };
|
||||
}
|
||||
|
||||
@@ -575,12 +594,12 @@ function getFullCredentialsFromFile(
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Invalid token format in:`, credentialsPath);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Invalid token format in:`, credentialsPath);
|
||||
return { token: null, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Retrieved full credentials from file:`, credentialsPath, {
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Retrieved full credentials from file:`, credentialsPath, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
hasRefreshToken: !!refreshToken,
|
||||
@@ -593,7 +612,7 @@ function getFullCredentialsFromFile(
|
||||
return { token, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
} catch (error) {
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn(`[CredentialUtils:${logPrefix}] Failed to read credentials file:`, credentialsPath, errorMessage);
|
||||
debugLog(`[CredentialUtils:${logPrefix}] Failed to read credentials file:`, credentialsPath, errorMessage);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: `Failed to read credentials: ${errorMessage}` };
|
||||
}
|
||||
}
|
||||
@@ -616,15 +635,6 @@ function getCredentialsFromMacOSKeychain(configDir?: string, forceRefresh = fals
|
||||
if (!forceRefresh && cached) {
|
||||
const ttl = cached.credentials.error ? ERROR_CACHE_TTL_MS : CACHE_TTL_MS;
|
||||
if ((now - cached.timestamp) < ttl) {
|
||||
if (isDebug) {
|
||||
const cacheAge = now - cached.timestamp;
|
||||
console.warn('[CredentialUtils:macOS:CACHE] Returning cached credentials:', {
|
||||
serviceName,
|
||||
hasToken: !!cached.credentials.token,
|
||||
tokenFingerprint: getTokenFingerprint(cached.credentials.token),
|
||||
cacheAge: Math.round(cacheAge / 1000) + 's'
|
||||
});
|
||||
}
|
||||
return cached.credentials;
|
||||
}
|
||||
}
|
||||
@@ -664,7 +674,7 @@ function getCredentialsFromMacOSKeychain(configDir?: string, forceRefresh = fals
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:macOS] Invalid token format for service:', serviceName);
|
||||
debugLog('[CredentialUtils:macOS] Invalid token format for service:', serviceName);
|
||||
const result = { token: null, email };
|
||||
credentialCache.set(cacheKey, { credentials: result, timestamp: now });
|
||||
return result;
|
||||
@@ -674,7 +684,7 @@ function getCredentialsFromMacOSKeychain(configDir?: string, forceRefresh = fals
|
||||
credentialCache.set(cacheKey, { credentials, timestamp: now });
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:macOS] Retrieved credentials from Keychain for service:', serviceName, {
|
||||
debugLog('[CredentialUtils:macOS] Retrieved credentials from Keychain for service:', serviceName, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
tokenFingerprint: getTokenFingerprint(token),
|
||||
@@ -685,7 +695,7 @@ function getCredentialsFromMacOSKeychain(configDir?: string, forceRefresh = fals
|
||||
} catch (error) {
|
||||
// Unexpected error (executeCredentialRead already handles "not found" cases)
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:macOS] Keychain access failed for service:', serviceName, errorMessage);
|
||||
debugLog('[CredentialUtils:macOS] Keychain access failed for service:', serviceName, errorMessage);
|
||||
const errorResult = { token: null, email: null, error: `Keychain access failed: ${errorMessage}` };
|
||||
// Use shorter TTL for errors
|
||||
credentialCache.set(cacheKey, { credentials: errorResult, timestamp: now });
|
||||
@@ -756,7 +766,7 @@ function getCredentialsFromLinuxSecretService(configDir?: string, forceRefresh =
|
||||
if ((now - cached.timestamp) < ttl) {
|
||||
if (isDebug) {
|
||||
const cacheAge = now - cached.timestamp;
|
||||
console.warn('[CredentialUtils:Linux:SecretService:CACHE] Returning cached credentials:', {
|
||||
debugLog('[CredentialUtils:Linux:SecretService:CACHE] Returning cached credentials:', {
|
||||
attribute,
|
||||
hasToken: !!cached.credentials.token,
|
||||
tokenFingerprint: getTokenFingerprint(cached.credentials.token),
|
||||
@@ -771,7 +781,7 @@ function getCredentialsFromLinuxSecretService(configDir?: string, forceRefresh =
|
||||
const secretToolPath = findSecretToolPath();
|
||||
if (!secretToolPath) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService] secret-tool not found, falling back to file storage');
|
||||
debugLog('[CredentialUtils:Linux:SecretService] secret-tool not found, falling back to file storage');
|
||||
}
|
||||
// Return a special result indicating Secret Service is unavailable
|
||||
return { token: null, email: null, error: 'secret-tool not found' };
|
||||
@@ -795,7 +805,7 @@ function getCredentialsFromLinuxSecretService(configDir?: string, forceRefresh =
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService] Invalid token format for attribute:', attribute);
|
||||
debugLog('[CredentialUtils:Linux:SecretService] Invalid token format for attribute:', attribute);
|
||||
const result = { token: null, email };
|
||||
credentialCache.set(cacheKey, { credentials: result, timestamp: now });
|
||||
return result;
|
||||
@@ -805,7 +815,7 @@ function getCredentialsFromLinuxSecretService(configDir?: string, forceRefresh =
|
||||
credentialCache.set(cacheKey, { credentials, timestamp: now });
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService] Retrieved credentials from Secret Service:', {
|
||||
debugLog('[CredentialUtils:Linux:SecretService] Retrieved credentials from Secret Service:', {
|
||||
attribute,
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
@@ -817,7 +827,7 @@ function getCredentialsFromLinuxSecretService(configDir?: string, forceRefresh =
|
||||
} catch (error) {
|
||||
// Unexpected error (executeCredentialRead already handles "not found" cases)
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:Linux:SecretService] Secret Service access failed:', errorMessage);
|
||||
debugLog('[CredentialUtils:Linux:SecretService] Secret Service access failed:', errorMessage);
|
||||
// Return error to trigger fallback to file storage
|
||||
return { token: null, email: null, error: `Secret Service access failed: ${errorMessage}` };
|
||||
}
|
||||
@@ -840,7 +850,7 @@ function getCredentialsFromLinux(configDir?: string, forceRefresh = false): Plat
|
||||
// If Secret Service had an error (not just "not found"), log it and try file fallback
|
||||
if (secretServiceResult.error && !secretServiceResult.error.includes('not found')) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux] Secret Service unavailable, trying file fallback:', secretServiceResult.error);
|
||||
debugLog('[CredentialUtils:Linux] Secret Service unavailable, trying file fallback:', secretServiceResult.error);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -894,7 +904,7 @@ function getCredentialsFromWindowsCredentialManager(configDir?: string, forceRef
|
||||
if ((now - cached.timestamp) < ttl) {
|
||||
if (isDebug) {
|
||||
const cacheAge = now - cached.timestamp;
|
||||
console.warn('[CredentialUtils:Windows:CACHE] Returning cached credentials:', {
|
||||
debugLog('[CredentialUtils:Windows:CACHE] Returning cached credentials:', {
|
||||
targetName,
|
||||
hasToken: !!cached.credentials.token,
|
||||
tokenFingerprint: getTokenFingerprint(cached.credentials.token),
|
||||
@@ -910,7 +920,7 @@ function getCredentialsFromWindowsCredentialManager(configDir?: string, forceRef
|
||||
const invalidResult = { token: null, email: null, error: 'Invalid credential target name format' };
|
||||
credentialCache.set(cacheKey, { credentials: invalidResult, timestamp: now });
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Invalid target name rejected:', { targetName });
|
||||
debugLog('[CredentialUtils:Windows] Invalid target name rejected:', { targetName });
|
||||
}
|
||||
return invalidResult;
|
||||
}
|
||||
@@ -1017,7 +1027,7 @@ public static extern bool CredFree(IntPtr cred);
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:Windows] Invalid token format for target:', targetName);
|
||||
debugLog('[CredentialUtils:Windows] Invalid token format for target:', targetName);
|
||||
const result = { token: null, email };
|
||||
credentialCache.set(cacheKey, { credentials: result, timestamp: now });
|
||||
return result;
|
||||
@@ -1027,7 +1037,7 @@ public static extern bool CredFree(IntPtr cred);
|
||||
credentialCache.set(cacheKey, { credentials, timestamp: now });
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Retrieved credentials from Credential Manager for target:', targetName, {
|
||||
debugLog('[CredentialUtils:Windows] Retrieved credentials from Credential Manager for target:', targetName, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
tokenFingerprint: getTokenFingerprint(token),
|
||||
@@ -1037,7 +1047,7 @@ public static extern bool CredFree(IntPtr cred);
|
||||
return credentials;
|
||||
} catch (error) {
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:Windows] Credential Manager access failed for target:', targetName, errorMessage);
|
||||
debugLog('[CredentialUtils:Windows] Credential Manager access failed for target:', targetName, errorMessage);
|
||||
const errorResult = { token: null, email: null, error: `Credential Manager access failed: ${errorMessage}` };
|
||||
credentialCache.set(cacheKey, { credentials: errorResult, timestamp: now });
|
||||
return errorResult;
|
||||
@@ -1102,13 +1112,13 @@ function getCredentialsFromWindows(configDir?: string, forceRefresh = false): Pl
|
||||
// If only one has a token, use that one
|
||||
if (fileResult.token && !credManagerResult.token) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Using file credentials (Credential Manager empty)');
|
||||
debugLog('[CredentialUtils:Windows] Using file credentials (Credential Manager empty)');
|
||||
}
|
||||
return fileResult;
|
||||
}
|
||||
if (credManagerResult.token && !fileResult.token) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Using Credential Manager credentials (file empty)');
|
||||
debugLog('[CredentialUtils:Windows] Using Credential Manager credentials (file empty)');
|
||||
}
|
||||
return credManagerResult;
|
||||
}
|
||||
@@ -1120,7 +1130,7 @@ function getCredentialsFromWindows(configDir?: string, forceRefresh = false): Pl
|
||||
|
||||
// Both have tokens - prefer file since Claude CLI writes there after login
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Both sources have tokens, preferring file (Claude CLI primary storage)');
|
||||
debugLog('[CredentialUtils:Windows] Both sources have tokens, preferring file (Claude CLI primary storage)');
|
||||
}
|
||||
return fileResult;
|
||||
}
|
||||
@@ -1242,12 +1252,12 @@ function getFullCredentialsFromMacOSKeychain(configDir?: string): FullOAuthCrede
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:macOS:Full] Invalid token format for service:', serviceName);
|
||||
debugLog('[CredentialUtils:macOS:Full] Invalid token format for service:', serviceName);
|
||||
return { token: null, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:macOS:Full] Retrieved full credentials from Keychain for service:', serviceName, {
|
||||
debugLog('[CredentialUtils:macOS:Full] Retrieved full credentials from Keychain for service:', serviceName, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
hasRefreshToken: !!refreshToken,
|
||||
@@ -1261,7 +1271,7 @@ function getFullCredentialsFromMacOSKeychain(configDir?: string): FullOAuthCrede
|
||||
} catch (error) {
|
||||
// Unexpected error (executeCredentialRead already handles "not found" cases)
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:macOS:Full] Keychain access failed for service:', serviceName, errorMessage);
|
||||
debugLog('[CredentialUtils:macOS:Full] Keychain access failed for service:', serviceName, errorMessage);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: `Keychain access failed: ${errorMessage}` };
|
||||
}
|
||||
}
|
||||
@@ -1277,7 +1287,7 @@ function getFullCredentialsFromLinuxSecretService(configDir?: string): FullOAuth
|
||||
const secretToolPath = findSecretToolPath();
|
||||
if (!secretToolPath) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Full] secret-tool not found');
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Full] secret-tool not found');
|
||||
}
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: 'secret-tool not found' };
|
||||
}
|
||||
@@ -1299,12 +1309,12 @@ function getFullCredentialsFromLinuxSecretService(configDir?: string): FullOAuth
|
||||
);
|
||||
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Full] Invalid token format for attribute:', attribute);
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Full] Invalid token format for attribute:', attribute);
|
||||
return { token: null, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Full] Retrieved full credentials from Secret Service:', {
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Full] Retrieved full credentials from Secret Service:', {
|
||||
attribute,
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
@@ -1319,7 +1329,7 @@ function getFullCredentialsFromLinuxSecretService(configDir?: string): FullOAuth
|
||||
} catch (error) {
|
||||
// Unexpected error (executeCredentialRead already handles "not found" cases)
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Full] Secret Service access failed:', errorMessage);
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Full] Secret Service access failed:', errorMessage);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: `Secret Service access failed: ${errorMessage}` };
|
||||
}
|
||||
}
|
||||
@@ -1339,7 +1349,7 @@ function getFullCredentialsFromLinux(configDir?: string): FullOAuthCredentials {
|
||||
|
||||
if (secretServiceResult.error && !secretServiceResult.error.includes('not found')) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:Full] Secret Service unavailable, trying file fallback:', secretServiceResult.error);
|
||||
debugLog('[CredentialUtils:Linux:Full] Secret Service unavailable, trying file fallback:', secretServiceResult.error);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1366,7 +1376,7 @@ function getFullCredentialsFromWindowsCredentialManager(configDir?: string): Ful
|
||||
if (!isValidTargetName(targetName)) {
|
||||
const invalidResult = { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: 'Invalid credential target name format' };
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Invalid target name rejected:', { targetName });
|
||||
debugLog('[CredentialUtils:Windows:Full] Invalid target name rejected:', { targetName });
|
||||
}
|
||||
return invalidResult;
|
||||
}
|
||||
@@ -1461,12 +1471,12 @@ public static extern bool CredFree(IntPtr cred);
|
||||
|
||||
// Validate token format if present
|
||||
if (token && !isValidTokenFormat(token)) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Invalid token format for target:', targetName);
|
||||
debugLog('[CredentialUtils:Windows:Full] Invalid token format for target:', targetName);
|
||||
return { token: null, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Retrieved full credentials from Credential Manager for target:', targetName, {
|
||||
debugLog('[CredentialUtils:Windows:Full] Retrieved full credentials from Credential Manager for target:', targetName, {
|
||||
hasToken: !!token,
|
||||
hasEmail: !!email,
|
||||
hasRefreshToken: !!refreshToken,
|
||||
@@ -1479,7 +1489,7 @@ public static extern bool CredFree(IntPtr cred);
|
||||
return { token, email, refreshToken, expiresAt, scopes, subscriptionType, rateLimitTier };
|
||||
} catch (error) {
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:Windows:Full] Credential Manager access failed for target:', targetName, errorMessage);
|
||||
debugLog('[CredentialUtils:Windows:Full] Credential Manager access failed for target:', targetName, errorMessage);
|
||||
return { token: null, email: null, refreshToken: null, expiresAt: null, scopes: null, subscriptionType: null, rateLimitTier: null, error: `Credential Manager access failed: ${errorMessage}` };
|
||||
}
|
||||
}
|
||||
@@ -1508,13 +1518,13 @@ function getFullCredentialsFromWindows(configDir?: string): FullOAuthCredentials
|
||||
// If only one has a token, use that one
|
||||
if (fileResult.token && !credManagerResult.token) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Using file credentials (Credential Manager empty)');
|
||||
debugLog('[CredentialUtils:Windows:Full] Using file credentials (Credential Manager empty)');
|
||||
}
|
||||
return fileResult;
|
||||
}
|
||||
if (credManagerResult.token && !fileResult.token) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Using Credential Manager credentials (file empty)');
|
||||
debugLog('[CredentialUtils:Windows:Full] Using Credential Manager credentials (file empty)');
|
||||
}
|
||||
return credManagerResult;
|
||||
}
|
||||
@@ -1529,7 +1539,7 @@ function getFullCredentialsFromWindows(configDir?: string): FullOAuthCredentials
|
||||
// Using file as primary ensures consistency: the same token is returned whether
|
||||
// calling getCredentialsFromKeychain() or getFullCredentialsFromKeychain().
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Full] Both sources have tokens, preferring file (Claude CLI primary storage)');
|
||||
debugLog('[CredentialUtils:Windows:Full] Both sources have tokens, preferring file (Claude CLI primary storage)');
|
||||
}
|
||||
return fileResult;
|
||||
}
|
||||
@@ -1633,12 +1643,12 @@ function updateMacOSKeychainCredentials(
|
||||
}
|
||||
);
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:macOS:Update] Deleted existing Keychain entry for service:', serviceName);
|
||||
debugLog('[CredentialUtils:macOS:Update] Deleted existing Keychain entry for service:', serviceName);
|
||||
}
|
||||
} catch {
|
||||
// Entry didn't exist - that's fine, we'll create it
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:macOS:Update] No existing entry to delete for service:', serviceName);
|
||||
debugLog('[CredentialUtils:macOS:Update] No existing entry to delete for service:', serviceName);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1656,7 +1666,7 @@ function updateMacOSKeychainCredentials(
|
||||
);
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:macOS:Update] Successfully updated Keychain credentials for service:', serviceName);
|
||||
debugLog('[CredentialUtils:macOS:Update] Successfully updated Keychain credentials for service:', serviceName);
|
||||
}
|
||||
|
||||
// Clear cached credentials to ensure fresh values are read
|
||||
@@ -1689,7 +1699,7 @@ function updateLinuxSecretServiceCredentials(
|
||||
const secretToolPath = findSecretToolPath();
|
||||
if (!secretToolPath) {
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Update] secret-tool not found');
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Update] secret-tool not found');
|
||||
}
|
||||
return { success: false, error: 'secret-tool not found' };
|
||||
}
|
||||
@@ -1730,7 +1740,7 @@ function updateLinuxSecretServiceCredentials(
|
||||
);
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:SecretService:Update] Successfully updated Secret Service credentials for attribute:', attribute);
|
||||
debugLog('[CredentialUtils:Linux:SecretService:Update] Successfully updated Secret Service credentials for attribute:', attribute);
|
||||
}
|
||||
|
||||
// Clear cached credentials to ensure fresh values are read
|
||||
@@ -1766,7 +1776,7 @@ function updateLinuxCredentials(
|
||||
return secretServiceResult;
|
||||
}
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:Update] Secret Service update failed, trying file fallback:', secretServiceResult.error);
|
||||
debugLog('[CredentialUtils:Linux:Update] Secret Service update failed, trying file fallback:', secretServiceResult.error);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1827,7 +1837,7 @@ function updateLinuxFileCredentials(
|
||||
writeFileSync(credentialsPath, credentialsJson, { mode: 0o600, encoding: 'utf-8' });
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Linux:Update] Successfully updated credentials file:', credentialsPath);
|
||||
debugLog('[CredentialUtils:Linux:Update] Successfully updated credentials file:', credentialsPath);
|
||||
}
|
||||
|
||||
// Clear cached credentials to ensure fresh values are read
|
||||
@@ -1966,7 +1976,7 @@ function updateWindowsCredentialManagerCredentials(
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Update] Successfully updated Credential Manager for target:', targetName);
|
||||
debugLog('[CredentialUtils:Windows:Update] Successfully updated Credential Manager for target:', targetName);
|
||||
}
|
||||
|
||||
// Clear cached credentials to ensure fresh values are read
|
||||
@@ -2009,13 +2019,13 @@ function restrictWindowsFilePermissions(filePath: string): void {
|
||||
});
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows] Set restrictive permissions on:', filePath);
|
||||
debugLog('[CredentialUtils:Windows] Set restrictive permissions on:', filePath);
|
||||
}
|
||||
} catch (error) {
|
||||
// Non-fatal: log warning but don't fail the operation
|
||||
// The file is still protected by the user's home directory permissions
|
||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
||||
console.warn('[CredentialUtils:Windows] Could not set restrictive file permissions:', errorMessage);
|
||||
debugLog('[CredentialUtils:Windows] Could not set restrictive file permissions:', errorMessage);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2105,7 +2115,7 @@ function updateWindowsFileCredentials(
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Update] Successfully updated credentials file:', credentialsPath);
|
||||
debugLog('[CredentialUtils:Windows:Update] Successfully updated credentials file:', credentialsPath);
|
||||
}
|
||||
|
||||
// Clear cached credentials to ensure fresh values are read
|
||||
@@ -2162,7 +2172,7 @@ function updateWindowsCredentials(
|
||||
// Credential Manager failed but file succeeded - this is acceptable
|
||||
// Claude CLI will use the file, which has the latest tokens
|
||||
if (isDebug) {
|
||||
console.warn('[CredentialUtils:Windows:Update] Credential Manager update failed (file update succeeded):', credManagerResult.error);
|
||||
debugLog('[CredentialUtils:Windows:Update] Credential Manager update failed (file update succeeded):', credManagerResult.error);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -319,12 +319,6 @@ export async function ensureValidToken(
|
||||
? configDir.replace(/^~/, homedir())
|
||||
: configDir;
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[TokenRefresh:ensureValidToken] Checking token validity', {
|
||||
configDir: expandedConfigDir || 'default'
|
||||
});
|
||||
}
|
||||
|
||||
// Step 1: Read full credentials from keychain
|
||||
const creds = getFullCredentialsFromKeychain(expandedConfigDir);
|
||||
|
||||
|
||||
@@ -90,60 +90,22 @@ const PROVIDER_USAGE_ENDPOINTS: readonly ProviderUsageEndpoint[] = [
|
||||
export function getUsageEndpoint(provider: ApiProvider, baseUrl: string): string | null {
|
||||
const isDebug = process.env.DEBUG === 'true';
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:ENDPOINT_CONSTRUCTION] Constructing usage endpoint:', {
|
||||
provider,
|
||||
baseUrl
|
||||
});
|
||||
}
|
||||
|
||||
const endpointConfig = PROVIDER_USAGE_ENDPOINTS.find(e => e.provider === provider);
|
||||
if (!endpointConfig) {
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:ENDPOINT_CONSTRUCTION] Unknown provider - no endpoint configured:', {
|
||||
provider,
|
||||
availableProviders: PROVIDER_USAGE_ENDPOINTS.map(e => e.provider)
|
||||
});
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:ENDPOINT_CONSTRUCTION] Found endpoint config for provider:', {
|
||||
provider,
|
||||
usagePath: endpointConfig.usagePath
|
||||
});
|
||||
}
|
||||
|
||||
try {
|
||||
const url = new URL(baseUrl);
|
||||
const originalPath = url.pathname;
|
||||
// Replace the path with the usage endpoint path
|
||||
url.pathname = endpointConfig.usagePath;
|
||||
|
||||
// Note: quota/limit endpoint doesn't require query parameters
|
||||
// The model-usage and tool-usage endpoints would need time windows, but we're using quota/limit
|
||||
|
||||
const finalUrl = url.toString();
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:ENDPOINT_CONSTRUCTION] Successfully constructed endpoint:', {
|
||||
provider,
|
||||
originalPath,
|
||||
newPath: endpointConfig.usagePath,
|
||||
finalUrl
|
||||
});
|
||||
}
|
||||
|
||||
return finalUrl;
|
||||
return url.toString();
|
||||
} catch (error) {
|
||||
console.error('[UsageMonitor] Invalid baseUrl for usage endpoint:', baseUrl);
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:ENDPOINT_CONSTRUCTION] URL construction failed:', {
|
||||
baseUrl,
|
||||
error: error instanceof Error ? error.message : String(error)
|
||||
});
|
||||
}
|
||||
return null;
|
||||
}
|
||||
}
|
||||
@@ -163,18 +125,7 @@ export function getUsageEndpoint(provider: ApiProvider, baseUrl: string): string
|
||||
*/
|
||||
export function detectProvider(baseUrl: string): ApiProvider {
|
||||
// Wrapper around shared detectProvider with debug logging for main process
|
||||
const isDebug = process.env.DEBUG === 'true';
|
||||
|
||||
const provider = sharedDetectProvider(baseUrl);
|
||||
|
||||
if (isDebug) {
|
||||
console.warn('[UsageMonitor:PROVIDER_DETECTION] Detected provider:', {
|
||||
baseUrl,
|
||||
provider
|
||||
});
|
||||
}
|
||||
|
||||
return provider;
|
||||
return sharedDetectProvider(baseUrl);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -844,18 +795,12 @@ export class UsageMonitor extends EventEmitter {
|
||||
// Fallback: Try direct keychain read (e.g., if refresh token unavailable)
|
||||
const keychainCreds = getCredentialsFromKeychain(activeProfile.configDir);
|
||||
if (keychainCreds.token) {
|
||||
this.debugLog('[UsageMonitor:TRACE] Using fallback OAuth token from Keychain for profile: ' + activeProfile.name, {
|
||||
tokenFingerprint: getCredentialFingerprint(keychainCreds.token)
|
||||
});
|
||||
return keychainCreds.token;
|
||||
}
|
||||
|
||||
// Keychain read also failed
|
||||
if (keychainCreds.error) {
|
||||
this.debugLog('[UsageMonitor] Keychain access failed:', keychainCreds.error);
|
||||
} else {
|
||||
this.debugLog('[UsageMonitor:TRACE] No token in Keychain for profile: ' + activeProfile.name +
|
||||
' - user may need to re-authenticate with claude /login');
|
||||
}
|
||||
|
||||
// Mark profile as needing re-authentication since credentials are missing
|
||||
@@ -863,7 +808,6 @@ export class UsageMonitor extends EventEmitter {
|
||||
}
|
||||
|
||||
// No credential available
|
||||
this.debugLog('[UsageMonitor:TRACE] No credential available (no API or OAuth profile active)');
|
||||
return undefined;
|
||||
}
|
||||
|
||||
|
||||
@@ -118,6 +118,14 @@ export function registerEnvHandlers(
|
||||
if (config.githubAutoSync !== undefined) {
|
||||
existingVars['GITHUB_AUTO_SYNC'] = config.githubAutoSync ? 'true' : 'false';
|
||||
}
|
||||
// GitHub CI check exclusion (comma-separated list of check names to ignore)
|
||||
if (config.githubExcludedCIChecks !== undefined) {
|
||||
if (config.githubExcludedCIChecks.length > 0) {
|
||||
existingVars['GITHUB_EXCLUDED_CI_CHECKS'] = config.githubExcludedCIChecks.join(',');
|
||||
} else {
|
||||
delete existingVars['GITHUB_EXCLUDED_CI_CHECKS'];
|
||||
}
|
||||
}
|
||||
// GitLab Integration
|
||||
if (config.gitlabEnabled !== undefined) {
|
||||
existingVars[GITLAB_ENV_KEYS.ENABLED] = config.gitlabEnabled ? 'true' : 'false';
|
||||
@@ -251,6 +259,9 @@ ${existingVars['LINEAR_REALTIME_SYNC'] !== undefined ? `LINEAR_REALTIME_SYNC=${e
|
||||
${existingVars['GITHUB_TOKEN'] ? `GITHUB_TOKEN=${existingVars['GITHUB_TOKEN']}` : '# GITHUB_TOKEN='}
|
||||
${existingVars['GITHUB_REPO'] ? `GITHUB_REPO=${existingVars['GITHUB_REPO']}` : '# GITHUB_REPO=owner/repo'}
|
||||
${existingVars['GITHUB_AUTO_SYNC'] !== undefined ? `GITHUB_AUTO_SYNC=${existingVars['GITHUB_AUTO_SYNC']}` : '# GITHUB_AUTO_SYNC=false'}
|
||||
# CI check names to exclude from blocking during PR reviews (comma-separated)
|
||||
# Useful for stuck/broken CI checks that never complete
|
||||
${existingVars['GITHUB_EXCLUDED_CI_CHECKS'] ? `GITHUB_EXCLUDED_CI_CHECKS=${existingVars['GITHUB_EXCLUDED_CI_CHECKS']}` : '# GITHUB_EXCLUDED_CI_CHECKS='}
|
||||
|
||||
# =============================================================================
|
||||
# GITLAB INTEGRATION (OPTIONAL)
|
||||
@@ -430,6 +441,13 @@ ${existingVars['GRAPHITI_DB_PATH'] ? `GRAPHITI_DB_PATH=${existingVars['GRAPHITI_
|
||||
if (vars['GITHUB_AUTO_SYNC']?.toLowerCase() === 'true') {
|
||||
config.githubAutoSync = true;
|
||||
}
|
||||
// Parse excluded CI checks (comma-separated list)
|
||||
if (vars['GITHUB_EXCLUDED_CI_CHECKS']) {
|
||||
config.githubExcludedCIChecks = vars['GITHUB_EXCLUDED_CI_CHECKS']
|
||||
.split(',')
|
||||
.map(s => s.trim())
|
||||
.filter(Boolean);
|
||||
}
|
||||
|
||||
// GitLab config
|
||||
if (vars[GITLAB_ENV_KEYS.TOKEN]) {
|
||||
|
||||
@@ -245,6 +245,29 @@ function getClaudeMdEnv(project: Project): Record<string, string> | undefined {
|
||||
return project.settings?.useClaudeMd !== false ? { USE_CLAUDE_MD: "true" } : undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds extra environment variables for PR review subprocess.
|
||||
* Includes Claude.md setting and GitHub-specific config like excluded CI checks.
|
||||
*/
|
||||
function getPRReviewExtraEnv(
|
||||
project: Project,
|
||||
config: { excludedCIChecks?: string[] } | null
|
||||
): Record<string, string> {
|
||||
const env: Record<string, string> = {};
|
||||
|
||||
// Add Claude.md setting
|
||||
if (project.settings?.useClaudeMd !== false) {
|
||||
env.USE_CLAUDE_MD = "true";
|
||||
}
|
||||
|
||||
// Add excluded CI checks (for stuck/broken CI like license/cla)
|
||||
if (config?.excludedCIChecks && config.excludedCIChecks.length > 0) {
|
||||
env.GITHUB_EXCLUDED_CI_CHECKS = config.excludedCIChecks.join(",");
|
||||
}
|
||||
|
||||
return env;
|
||||
}
|
||||
|
||||
/**
|
||||
* PR review finding from AI analysis
|
||||
*/
|
||||
@@ -1304,8 +1327,8 @@ async function runPRReview(
|
||||
const repo = config?.repo || project.name || "unknown";
|
||||
const logCollector = new PRLogCollector(project, prNumber, repo, false);
|
||||
|
||||
// Build environment with project settings
|
||||
const subprocessEnv = await getRunnerEnv(getClaudeMdEnv(project));
|
||||
// Build environment with project settings (including excluded CI checks)
|
||||
const subprocessEnv = await getRunnerEnv(getPRReviewExtraEnv(project, config));
|
||||
|
||||
const { process: childProcess, promise } = runPythonSubprocess<PRReviewResult>({
|
||||
pythonPath: getPythonPath(backendPath),
|
||||
@@ -2709,8 +2732,8 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v
|
||||
const repo = config?.repo || project.name || "unknown";
|
||||
const logCollector = new PRLogCollector(project, prNumber, repo, true);
|
||||
|
||||
// Build environment with project settings
|
||||
const followupEnv = await getRunnerEnv(getClaudeMdEnv(project));
|
||||
// Build environment with project settings (including excluded CI checks)
|
||||
const followupEnv = await getRunnerEnv(getPRReviewExtraEnv(project, config));
|
||||
|
||||
const { process: childProcess, promise } = runPythonSubprocess<PRReviewResult>({
|
||||
pythonPath: getPythonPath(backendPath),
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
export interface GitHubConfig {
|
||||
token: string;
|
||||
repo: string;
|
||||
excludedCIChecks?: string[];
|
||||
}
|
||||
|
||||
export interface GitHubAPIIssue {
|
||||
|
||||
@@ -81,7 +81,14 @@ export function getGitHubConfig(project: Project): GitHubConfig | null {
|
||||
}
|
||||
|
||||
if (!token || !repo) return null;
|
||||
return { token, repo };
|
||||
|
||||
// Parse excluded CI checks (comma-separated list)
|
||||
const excludedCIChecksRaw = vars['GITHUB_EXCLUDED_CI_CHECKS'];
|
||||
const excludedCIChecks = excludedCIChecksRaw
|
||||
? excludedCIChecksRaw.split(',').map((s) => s.trim()).filter(Boolean)
|
||||
: undefined;
|
||||
|
||||
return { token, repo, excludedCIChecks };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
|
||||
@@ -377,23 +377,12 @@ export class TerminalSessionStore {
|
||||
todaySessions[projectPath] = [];
|
||||
}
|
||||
|
||||
// Debug: Log incoming outputBuffer info
|
||||
const incomingBufferLen = session.outputBuffer?.length ?? 0;
|
||||
debugLog('[TerminalSessionStore] Updating session in memory:', session.id,
|
||||
'incoming outputBuffer:', incomingBufferLen, 'bytes',
|
||||
'isClaudeMode:', session.isClaudeMode);
|
||||
|
||||
// Update existing or add new
|
||||
const existingIndex = todaySessions[projectPath].findIndex(s => s.id === session.id);
|
||||
if (existingIndex >= 0) {
|
||||
// Preserve displayOrder from existing session if not provided in incoming session
|
||||
// This prevents periodic saves (which don't include displayOrder) from losing tab order
|
||||
const existingSession = todaySessions[projectPath][existingIndex];
|
||||
const existingBufferLen = existingSession.outputBuffer?.length ?? 0;
|
||||
const truncatedLen = session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).length;
|
||||
debugLog('[TerminalSessionStore] Updating existing session:', session.id,
|
||||
'existing outputBuffer:', existingBufferLen, 'bytes',
|
||||
'new outputBuffer (after truncation):', truncatedLen, 'bytes');
|
||||
|
||||
todaySessions[projectPath][existingIndex] = {
|
||||
...session,
|
||||
@@ -404,10 +393,6 @@ export class TerminalSessionStore {
|
||||
displayOrder: session.displayOrder ?? existingSession.displayOrder,
|
||||
};
|
||||
} else {
|
||||
const truncatedLen = session.outputBuffer.slice(-MAX_OUTPUT_BUFFER).length;
|
||||
debugLog('[TerminalSessionStore] Creating new session:', session.id,
|
||||
'outputBuffer (after truncation):', truncatedLen, 'bytes');
|
||||
|
||||
todaySessions[projectPath].push({
|
||||
...session,
|
||||
outputBuffer: session.outputBuffer.slice(-MAX_OUTPUT_BUFFER),
|
||||
|
||||
+40
-1
@@ -1,5 +1,5 @@
|
||||
import { useState } from 'react';
|
||||
import { Github, RefreshCw, KeyRound, Info, CheckCircle2 } from 'lucide-react';
|
||||
import { Github, RefreshCw, KeyRound, Info, CheckCircle2, AlertTriangle } from 'lucide-react';
|
||||
import { CollapsibleSection } from './CollapsibleSection';
|
||||
import { StatusBadge } from './StatusBadge';
|
||||
import { PasswordInput } from './PasswordInput';
|
||||
@@ -7,6 +7,7 @@ import { ConnectionStatus } from './ConnectionStatus';
|
||||
import { GitHubOAuthFlow } from './GitHubOAuthFlow';
|
||||
import { Label } from '../ui/label';
|
||||
import { Input } from '../ui/input';
|
||||
import { Textarea } from '../ui/textarea';
|
||||
import { Switch } from '../ui/switch';
|
||||
import { Separator } from '../ui/separator';
|
||||
import { Button } from '../ui/button';
|
||||
@@ -214,6 +215,44 @@ export function GitHubIntegrationSection({
|
||||
onCheckedChange={(checked) => onUpdateConfig({ githubAutoSync: checked })}
|
||||
/>
|
||||
</div>
|
||||
|
||||
<Separator />
|
||||
|
||||
{/* CI Check Exclusion */}
|
||||
<div className="space-y-3">
|
||||
<div className="flex items-center gap-2">
|
||||
<AlertTriangle className="h-4 w-4 text-warning" />
|
||||
<Label className="text-sm font-medium text-foreground">Excluded CI Checks</Label>
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
CI checks listed here will be ignored during PR reviews. Use this for broken or stuck checks
|
||||
that never complete. Enter check names separated by commas.
|
||||
</p>
|
||||
<Textarea
|
||||
placeholder="license/cla, flaky-e2e-test, deprecated-scan"
|
||||
value={(envConfig.githubExcludedCIChecks || []).join(', ')}
|
||||
onChange={(e) => {
|
||||
const value = e.target.value;
|
||||
const checks = value
|
||||
.split(',')
|
||||
.map(s => s.trim())
|
||||
.filter(Boolean);
|
||||
onUpdateConfig({ githubExcludedCIChecks: checks });
|
||||
}}
|
||||
className="min-h-[60px] font-mono text-xs"
|
||||
/>
|
||||
{(envConfig.githubExcludedCIChecks?.length ?? 0) > 0 && (
|
||||
<div className="rounded-lg border border-warning/30 bg-warning/5 p-3">
|
||||
<div className="flex items-start gap-2">
|
||||
<AlertTriangle className="h-4 w-4 text-warning mt-0.5 shrink-0" />
|
||||
<p className="text-xs text-muted-foreground">
|
||||
<span className="font-medium text-warning">{envConfig.githubExcludedCIChecks?.length}</span> check(s) will be
|
||||
ignored. PRs may be approved even if these checks fail or remain pending.
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
</>
|
||||
)}
|
||||
</CollapsibleSection>
|
||||
|
||||
@@ -1,8 +1,9 @@
|
||||
import { useState, useEffect, useMemo } from 'react';
|
||||
import { useTranslation } from 'react-i18next';
|
||||
import { Github, RefreshCw, KeyRound, Loader2, CheckCircle2, AlertCircle, User, Lock, Globe, ChevronDown, GitBranch } from 'lucide-react';
|
||||
import { Github, RefreshCw, KeyRound, Loader2, CheckCircle2, AlertCircle, User, Lock, Globe, ChevronDown, GitBranch, AlertTriangle } from 'lucide-react';
|
||||
import { Input } from '../../ui/input';
|
||||
import { Label } from '../../ui/label';
|
||||
import { Textarea } from '../../ui/textarea';
|
||||
import { Switch } from '../../ui/switch';
|
||||
import { Separator } from '../../ui/separator';
|
||||
import { Button } from '../../ui/button';
|
||||
@@ -413,6 +414,14 @@ export function GitHubIntegration({
|
||||
enabled={envConfig.githubAutoSync || false}
|
||||
onToggle={(checked) => updateEnvConfig({ githubAutoSync: checked })}
|
||||
/>
|
||||
|
||||
<Separator />
|
||||
|
||||
{/* CI Check Exclusion */}
|
||||
<ExcludedCIChecks
|
||||
excludedChecks={envConfig.githubExcludedCIChecks || []}
|
||||
onUpdate={(checks) => updateEnvConfig({ githubExcludedCIChecks: checks })}
|
||||
/>
|
||||
</>
|
||||
)}
|
||||
</div>
|
||||
@@ -662,3 +671,49 @@ function AutoSyncToggle({ enabled, onToggle }: AutoSyncToggleProps) {
|
||||
);
|
||||
}
|
||||
|
||||
interface ExcludedCIChecksProps {
|
||||
excludedChecks: string[];
|
||||
onUpdate: (checks: string[]) => void;
|
||||
}
|
||||
|
||||
function ExcludedCIChecks({ excludedChecks, onUpdate }: ExcludedCIChecksProps) {
|
||||
const { t } = useTranslation(['settings']);
|
||||
|
||||
return (
|
||||
<div className="space-y-3">
|
||||
<div className="flex items-center gap-2">
|
||||
<AlertTriangle className="h-4 w-4 text-warning" />
|
||||
<Label className="text-sm font-medium text-foreground">
|
||||
{t('settings:integrations.github.excludedCIChecks.label')}
|
||||
</Label>
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t('settings:integrations.github.excludedCIChecks.description')}
|
||||
</p>
|
||||
<Textarea
|
||||
placeholder={t('settings:integrations.github.excludedCIChecks.placeholder')}
|
||||
value={excludedChecks.join(', ')}
|
||||
onChange={(e) => {
|
||||
const value = e.target.value;
|
||||
const checks = value
|
||||
.split(',')
|
||||
.map(s => s.trim())
|
||||
.filter(Boolean);
|
||||
onUpdate(checks);
|
||||
}}
|
||||
className="min-h-[60px] font-mono text-xs"
|
||||
/>
|
||||
{excludedChecks.length > 0 && (
|
||||
<div className="rounded-lg border border-warning/30 bg-warning/5 p-3">
|
||||
<div className="flex items-start gap-2">
|
||||
<AlertTriangle className="h-4 w-4 text-warning mt-0.5 shrink-0" />
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t('settings:integrations.github.excludedCIChecks.warningCount', { count: excludedChecks.length })}
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -423,6 +423,22 @@
|
||||
"integrations": {
|
||||
"title": "Integrations",
|
||||
"description": "Manage Claude accounts and API keys",
|
||||
"github": {
|
||||
"defaultBranch": {
|
||||
"label": "Default Branch",
|
||||
"description": "Base branch for creating task worktrees",
|
||||
"autoDetect": "Auto-detect (main/master)",
|
||||
"searchPlaceholder": "Search branches...",
|
||||
"noBranchesFound": "No branches found",
|
||||
"selectedBranchHelp": "All new tasks will branch from {{branch}}"
|
||||
},
|
||||
"excludedCIChecks": {
|
||||
"label": "Excluded CI Checks",
|
||||
"description": "CI checks listed here will be ignored during PR reviews. Use this for broken or stuck checks that never complete. Enter check names separated by commas.",
|
||||
"placeholder": "license/cla, flaky-e2e-test, deprecated-scan",
|
||||
"warningCount": "{{count}} check(s) will be ignored. PRs may be approved even if these checks fail or remain pending."
|
||||
}
|
||||
},
|
||||
"claudeAccounts": "Claude Accounts",
|
||||
"claudeAccountsDescription": "Add multiple Claude subscriptions to automatically switch between them when you hit rate limits.",
|
||||
"claudeAccountsWarning": "When authenticating, ensure you're logged into the correct Claude account in your browser. Each profile should use a different subscription.",
|
||||
|
||||
@@ -423,6 +423,22 @@
|
||||
"integrations": {
|
||||
"title": "Intégrations",
|
||||
"description": "Gérer les comptes Claude et les clés API",
|
||||
"github": {
|
||||
"defaultBranch": {
|
||||
"label": "Branche par défaut",
|
||||
"description": "Branche de base pour créer les worktrees de tâches",
|
||||
"autoDetect": "Détection automatique (main/master)",
|
||||
"searchPlaceholder": "Rechercher des branches...",
|
||||
"noBranchesFound": "Aucune branche trouvée",
|
||||
"selectedBranchHelp": "Toutes les nouvelles tâches partiront de {{branch}}"
|
||||
},
|
||||
"excludedCIChecks": {
|
||||
"label": "Vérifications CI exclues",
|
||||
"description": "Les vérifications CI listées ici seront ignorées lors des revues de PR. Utilisez ceci pour les vérifications cassées ou bloquées qui ne se terminent jamais. Entrez les noms des vérifications séparés par des virgules.",
|
||||
"placeholder": "license/cla, flaky-e2e-test, deprecated-scan",
|
||||
"warningCount": "{{count}} vérification(s) sera(ont) ignorée(s). Les PR peuvent être approuvées même si ces vérifications échouent ou restent en attente."
|
||||
}
|
||||
},
|
||||
"claudeAccounts": "Comptes Claude",
|
||||
"claudeAccountsDescription": "Ajoutez plusieurs abonnements Claude pour basculer automatiquement entre eux quand vous atteignez les limites.",
|
||||
"claudeAccountsWarning": "Lors de l'authentification, assurez-vous d'être connecté au bon compte Claude dans votre navigateur. Chaque profil doit utiliser un abonnement différent.",
|
||||
|
||||
@@ -312,6 +312,7 @@ export interface ProjectEnvConfig {
|
||||
githubRepo?: string; // Format: owner/repo
|
||||
githubAutoSync?: boolean; // Auto-sync issues on project load
|
||||
githubAuthMethod?: 'oauth' | 'pat'; // How the token was obtained
|
||||
githubExcludedCIChecks?: string[]; // CI check names to exclude from blocking (e.g., stuck/broken checks)
|
||||
|
||||
// GitLab Integration
|
||||
gitlabEnabled: boolean;
|
||||
|
||||
@@ -1,166 +0,0 @@
|
||||
# Cross-Project Task Contamination: Missing projectId in Agent Event Pipeline
|
||||
|
||||
## Description
|
||||
|
||||
When running multiple projects simultaneously, agent events from one project can corrupt the status, badges, and column placement of tasks in another project. The root cause is that the entire agent event pipeline (from process spawn through XState state machine to disk persistence) identifies tasks by `specId` alone, with no project scoping. Since specIds are derived from task descriptions and are not unique across projects, `findTaskAndProject(taskId)` returns the first match across all loaded projects, routing events to the wrong task.
|
||||
|
||||
## Severity
|
||||
|
||||
**High** - Silent data corruption. Affected tasks show wrong status, wrong badges, land in wrong Kanban columns, and persist corrupted state to disk. On refresh, the corrupted state is reloaded, making the damage permanent until manually fixed.
|
||||
|
||||
## Affected Versions
|
||||
|
||||
All versions using the XState task state machine (PR #1575 and later).
|
||||
|
||||
## Steps to Reproduce
|
||||
|
||||
1. Open Auto Claude and load two projects (e.g., "Project A" and "Project B")
|
||||
2. In Project A, create a task with a specific name (e.g., "write wtf to text file") - this generates specId `016-write-wtf-to-text-file`
|
||||
3. In Project B, create a task with the same name - this generates the same specId `016-write-wtf-to-text-file`
|
||||
4. Start both tasks simultaneously (or start Project A's task first, let it reach QA, then start Project B's task)
|
||||
5. Switch between projects and observe the Kanban board
|
||||
|
||||
## Expected Results
|
||||
|
||||
- Each project's task progresses independently through its own lifecycle
|
||||
- Events from Project A's agent process only affect Project A's task card
|
||||
- Events from Project B's agent process only affect Project B's task card
|
||||
- Refreshing the app preserves the correct status for both tasks
|
||||
- Switching between projects shows each task in its correct column with the correct badge
|
||||
|
||||
## Actual Results
|
||||
|
||||
- Tasks in the non-active project show wrong status badges (e.g., "Coding" badge on a task still in the Planning column)
|
||||
- Tasks snap to the wrong Kanban column after refresh
|
||||
- Tasks get stuck in states they should have transitioned out of (e.g., permanently stuck in "Planning")
|
||||
- "Incomplete" badges appear on tasks that completed their phase successfully
|
||||
- QA tasks appear in "In Progress" column instead of "AI Review" column after switching projects
|
||||
- The corrupted state persists to `implementation_plan.json`, so the damage survives app restart
|
||||
|
||||
## Root Cause
|
||||
|
||||
### Task Identity Collision
|
||||
|
||||
Every task has two identifiers:
|
||||
|
||||
- **`task.id`** - A UUID, unique globally
|
||||
- **`task.specId`** - The spec directory name (e.g., `016-write-wtf-to-text-file`), derived from the task description, **not unique across projects**
|
||||
|
||||
The backend process uses `specId` as the task identifier in stdout markers. All agent event handlers resolve this back to a Task object via `findTaskAndProject(taskId)`, which searches all projects and returns the first match.
|
||||
|
||||
### Missing projectId in Event Pipeline
|
||||
|
||||
The agent event pipeline has no project scoping:
|
||||
|
||||
```
|
||||
Backend process (Python)
|
||||
-> stdout/stderr (phase markers, task events, logs)
|
||||
-> agent-process.ts (parses output, emits typed events)
|
||||
-> agent-manager.ts (EventEmitter relay)
|
||||
-> agent-events-handlers.ts (event handlers)
|
||||
-> findTaskAndProject(taskId) <-- COLLISION POINT
|
||||
-> taskStateManager (XState actor)
|
||||
-> persistPlanStatusAndReasonSync (disk)
|
||||
-> safeSendToRenderer (IPC to UI)
|
||||
```
|
||||
|
||||
None of the `AgentManagerEvents` carry a `projectId`:
|
||||
|
||||
```typescript
|
||||
// BEFORE: no way to scope events to the correct project
|
||||
interface AgentManagerEvents {
|
||||
log: (taskId: string, log: string) => void;
|
||||
error: (taskId: string, error: string) => void;
|
||||
exit: (taskId: string, code: number | null, processType: ProcessType) => void;
|
||||
'execution-progress': (taskId: string, progress: ExecutionProgressData) => void;
|
||||
'task-event': (taskId: string, event: TaskEventPayload) => void;
|
||||
}
|
||||
```
|
||||
|
||||
### Impact on XState
|
||||
|
||||
The `TaskStateManager` maintains one XState actor per taskId and drives column placement, badge display, disk persistence, and renderer notifications. When an event is routed to the wrong project's actor:
|
||||
|
||||
1. The actor receives an event invalid for its current state (e.g., `PLANNING_COMPLETE` sent to an actor in `qa_review`)
|
||||
2. XState either drops the event or transitions to an unexpected state
|
||||
3. The wrong project's `implementation_plan.json` is overwritten with incorrect status fields
|
||||
4. On app refresh, the task loads from the corrupted plan file and appears in the wrong column
|
||||
5. Subsequent legitimate events may be rejected because the actor is in a state that doesn't accept them
|
||||
|
||||
### Contamination Example
|
||||
|
||||
Given:
|
||||
- **Project A**: task `016-write-wtf-to-text-file` in QA (`qa_review` state)
|
||||
- **Project B**: task `016-write-wtf-to-text-file` just started (`planning` state)
|
||||
|
||||
When Project B's planner emits `PLANNING_COMPLETE`:
|
||||
1. `agent-process.ts` emits `task-event` with `taskId = "016-write-wtf-to-text-file"` and no projectId
|
||||
2. `findTaskAndProject("016-write-wtf-to-text-file")` returns **Project A's task** (first match)
|
||||
3. `PLANNING_COMPLETE` is sent to **Project A's XState actor** (which is in `qa_review`)
|
||||
4. Project A's plan file is corrupted; Project B's task never receives the event
|
||||
|
||||
## Observed Symptoms
|
||||
|
||||
| Symptom | Cause |
|
||||
|---------|-------|
|
||||
| "Coding" badge on a task in the Planning column | Project B's `CODING_STARTED` event hit Project A's planning task |
|
||||
| Task snaps to backlog on refresh | Plan file overwritten without XState fields; wrong project looked up for re-stamp |
|
||||
| "Incomplete" badge on a task that just finished planning | `PROCESS_EXITED` event from Project B's process hit Project A's `plan_review` actor |
|
||||
| QA task in "In Progress" column with "AI Review" badge | Execution progress event wrote wrong status to plan file |
|
||||
| Task stuck in "Planning" forever | Events meant for this task were consumed by the duplicate in another project |
|
||||
|
||||
## Fix
|
||||
|
||||
Thread `projectId` from the IPC handler that starts each agent process through the entire event pipeline to the lookup function.
|
||||
|
||||
### Propagation Chain
|
||||
|
||||
```
|
||||
execution-handlers.ts
|
||||
agentManager.startSpecCreation(..., project.id) <- Origin: project.id from IPC handler
|
||||
agentManager.startTaskExecution(..., project.id)
|
||||
agentManager.startQAProcess(..., project.id)
|
||||
|
||||
agent-manager.ts
|
||||
storeTaskContext(..., projectId) <- Stored in execution context
|
||||
processManager.spawnProcess(..., projectId) <- Passed to process spawner
|
||||
|
||||
agent-process.ts
|
||||
this.emitter.emit('log', taskId, ..., projectId) <- Attached to every emitted event
|
||||
this.emitter.emit('task-event', taskId, ..., projectId)
|
||||
this.emitter.emit('execution-progress', ..., projectId)
|
||||
this.emitter.emit('exit', taskId, ..., projectId)
|
||||
this.emitter.emit('error', taskId, ..., projectId)
|
||||
|
||||
agent-events-handlers.ts
|
||||
findTaskAndProject(taskId, projectId) <- Scoped lookup
|
||||
taskStateManager.handleTaskEvent(...) <- Correct actor receives event
|
||||
persistPlanStatusAndReasonSync(...) <- Correct plan file updated
|
||||
safeSendToRenderer(..., projectId) <- Renderer filters by project
|
||||
```
|
||||
|
||||
### Scoped Lookup
|
||||
|
||||
`findTaskAndProject` now accepts an optional `projectId`. When provided, it searches only the target project. Falls back to searching all projects for backward compatibility (file watcher events, renderer-initiated actions).
|
||||
|
||||
## Files Changed
|
||||
|
||||
| File | Change |
|
||||
|------|--------|
|
||||
| `apps/frontend/src/main/agent/types.ts` | Added `projectId?: string` to all event signatures |
|
||||
| `apps/frontend/src/main/agent/agent-manager.ts` | Added `projectId` to context storage, start methods, restart flow |
|
||||
| `apps/frontend/src/main/agent/agent-process.ts` | Added `projectId` to `spawnProcess` and all `emitter.emit()` calls |
|
||||
| `apps/frontend/src/main/ipc-handlers/task/shared.ts` | Scoped `findTaskAndProject` by projectId with fallback |
|
||||
| `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts` | All event handlers receive and forward projectId |
|
||||
| `apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts` | All 9 `agentManager.start*` call sites pass `project.id` |
|
||||
| `apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts` | Updated test expectations for new projectId parameter |
|
||||
|
||||
## Verification
|
||||
|
||||
- [ ] Run two projects simultaneously with tasks that have the same specId
|
||||
- [ ] Verify events from Project A only affect Project A's task cards
|
||||
- [ ] Verify events from Project B only affect Project B's task cards
|
||||
- [ ] Refresh the app during various lifecycle stages - tasks remain in correct columns
|
||||
- [ ] Switch between projects during QA - task stays in AI Review column
|
||||
- [ ] `npm run typecheck` passes
|
||||
- [ ] `npm run test` passes (2639 tests, 0 failures)
|
||||
@@ -1,139 +0,0 @@
|
||||
# PR #1575 Follow-up: XState Status Lifecycle & Cross-Project Contamination Fixes
|
||||
|
||||
## Overview
|
||||
|
||||
After the XState task state machine migration (PR #1575), several interrelated bugs surfaced when running multiple projects simultaneously and during normal task lifecycle transitions. These bugs caused tasks to appear in wrong columns, display incorrect badges, and lose status on refresh.
|
||||
|
||||
## Bug 1: Cross-Project Task Contamination
|
||||
|
||||
### Problem
|
||||
When two projects have tasks with the same specId (e.g., both have a task `016-write-wtf-to-text-file`), events from Project A's task would affect Project B's task card. Tasks in the secondary project would show wrong status badges (e.g., "Coding" badge on a task in the Planning column).
|
||||
|
||||
### Root Cause
|
||||
`findTaskAndProject(taskId)` searches ALL projects by matching `t.id === taskId || t.specId === taskId`, returning the first match. When the backend emits events using the specId as the task identifier, the lookup could match a task in the wrong project.
|
||||
|
||||
Agent events (log, error, exit, execution-progress, task-event) did not carry a `projectId`, so there was no way to scope the lookup to the correct project.
|
||||
|
||||
### Fix
|
||||
- Added `projectId` to all `AgentManagerEvents` type signatures
|
||||
- Pass `project.id` from all 9 `agentManager.start*` call sites in `execution-handlers.ts`
|
||||
- Thread `projectId` through `agent-manager.ts` → `agent-process.ts` → all `emitter.emit()` calls
|
||||
- Updated `findTaskAndProject(taskId, projectId?)` to scope search to the target project when `projectId` is provided, with fallback to searching all projects for backward compatibility
|
||||
- All event handlers in `agent-events-handlers.ts` now receive and use `projectId`
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/main/agent/types.ts`
|
||||
- `apps/frontend/src/main/agent/agent-manager.ts`
|
||||
- `apps/frontend/src/main/agent/agent-process.ts`
|
||||
- `apps/frontend/src/main/ipc-handlers/task/shared.ts`
|
||||
- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`
|
||||
- `apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts`
|
||||
- `apps/frontend/src/__tests__/integration/subprocess-spawn.test.ts`
|
||||
|
||||
## Bug 2: "Incomplete" Badge on Plan Review Tasks
|
||||
|
||||
### Problem
|
||||
Tasks with `requireReviewBeforeCoding=true` would complete planning, correctly transition to `plan_review` state, but then immediately show an "Incomplete" badge instead of "Planning" + "Approve Plan".
|
||||
|
||||
### Root Cause
|
||||
Two issues combined:
|
||||
|
||||
1. **`PLANNING_COMPLETE` was not in the `TERMINAL_EVENTS` set.** When the spec creation process finished normally (exit code 0), `handleProcessExited` was called. Since `PLANNING_COMPLETE` wasn't terminal, the check didn't skip.
|
||||
|
||||
2. **`handleProcessExited` always sent `unexpected: true`**, even for exit code 0. This caused the XState guard `unexpectedExit` to pass, transitioning the task from `plan_review` → `error`, which overwrote the correct `plan_review` reviewReason.
|
||||
|
||||
### Fix
|
||||
- Added `PLANNING_COMPLETE` to the `TERMINAL_EVENTS` set so process exit is skipped when planning has already completed
|
||||
- Changed `handleProcessExited` to only set `unexpected: true` when `exitCode !== 0` — a code-0 exit is normal and should not trigger error transitions
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/main/task-state-manager.ts`
|
||||
|
||||
## Bug 3: Backend qa.py Racing with XState Status
|
||||
|
||||
### Problem
|
||||
Tasks completing QA would sometimes show "Incomplete" instead of "Needs Review" because the `reviewReason` field was missing.
|
||||
|
||||
### Root Cause
|
||||
The backend `qa.py` tool was writing `plan["status"] = "human_review"` directly to the plan file WITHOUT setting `reviewReason`. This raced with the frontend XState state machine's `persistPlanStatusAndReasonSync()` which writes both `status` and `reviewReason` together. When qa.py wrote last, it clobbered the `reviewReason`.
|
||||
|
||||
### Fix
|
||||
Removed the backend's direct status writes from `qa.py`. The frontend XState state machine is now the sole owner of status transitions — the backend only updates `last_updated` timestamps and QA-specific fields.
|
||||
|
||||
### Files Changed
|
||||
- `apps/backend/agents/tools_pkg/tools/qa.py`
|
||||
|
||||
## Bug 4: Plan File Overwrite by Planner Agent
|
||||
|
||||
### Problem
|
||||
After a task started, the frontend would persist XState status fields (`status`, `xstateState`, `executionPhase`) to `implementation_plan.json`. The planner agent would then create the full plan using the Write tool, completely replacing the file and stripping the frontend's status fields. On refresh, the task would snap back to backlog.
|
||||
|
||||
### Root Cause
|
||||
The planner agent writes `implementation_plan.json` via Claude's Write tool, which replaces the entire file. The agent-generated plan does not include frontend status fields (`xstateState`, `executionPhase`), so they are lost.
|
||||
|
||||
### Fix
|
||||
Added a re-stamp mechanism in the file watcher's `progress` event handler. When the file watcher detects a plan file change and the `xstateState` field is missing (indicating the backend overwrote the file), the handler re-persists the current XState state back to the file. This also covers the worktree copy.
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`
|
||||
|
||||
## Bug 5: QA Tasks in Wrong Column After Project Switch
|
||||
|
||||
### Problem
|
||||
A task correctly in the "AI Review" column (status `ai_review`, phase `qa_review`) would snap to "In Progress" column after switching to another project and back. The "AI Review" badge would still show, but the card was in the wrong column.
|
||||
|
||||
### Root Cause
|
||||
`persistPlanPhaseSync()` in `plan-file-utils.ts` mapped execution phases to TaskStatus for column placement. It incorrectly mapped `qa_review` and `qa_fixing` to `in_progress` instead of `ai_review`. Every execution-progress event during QA would overwrite the correct `ai_review` status (set by XState) with `in_progress`. On refresh (reading from disk), the task loaded with `status: 'in_progress'` + `executionPhase: 'qa_review'`, placing it in the In Progress column with an AI Review badge.
|
||||
|
||||
### Fix
|
||||
Changed the phase-to-status mapping in `persistPlanPhaseSync`:
|
||||
- `qa_review` → `ai_review` (was `in_progress`)
|
||||
- `qa_fixing` → `ai_review` (was `in_progress`)
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts`
|
||||
|
||||
## Bug 6: updateTaskStatus Not Applying reviewReason
|
||||
|
||||
### Problem
|
||||
Tasks completing planning with `requireReviewBeforeCoding=true` would show an "Incomplete" badge in the Human Review column instead of "Planning" + "Approve Plan". The persisted plan file had the correct `reviewReason: 'plan_review'`, so refreshing the app would fix it.
|
||||
|
||||
### Root Cause
|
||||
`updateTaskStatus` in `task-store.ts` received `reviewReason` as a parameter but never applied it to the task object. The spread was `{ ...t, status, executionProgress }` — missing `reviewReason`. The skip condition also only checked `status`, not `reviewReason`, so transitions where only `reviewReason` changed (e.g., `human_review` with different reasons) were silently dropped.
|
||||
|
||||
### Fix
|
||||
- Added `reviewReason` to the task spread: `{ ...t, status, reviewReason, executionProgress }`
|
||||
- Updated skip condition to check both `status` AND `reviewReason`
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/renderer/stores/task-store.ts`
|
||||
|
||||
## Bug 7: Task Stuck in "In Progress" After Planning (requireReviewBeforeCoding)
|
||||
|
||||
### Problem
|
||||
Tasks with `requireReviewBeforeCoding=true` would complete planning, XState would correctly transition to `plan_review`, but the task card would remain in the "In Progress" column with `status=in_progress, reviewReason=none, phase=planning`.
|
||||
|
||||
### Root Cause
|
||||
When the process exits with code 1 (expected — the interactive review checkpoint fails in piped mode), `agent-process.ts` emits an `execution-progress` event with `phase: 'failed'` before the `exit` event. The `execution-progress` handler in `agent-events-handlers.ts`:
|
||||
|
||||
1. **Called `persistPlanPhaseSync` with `phase: 'failed'`**, which maps `failed` → `status: 'error'`, overwriting the `status: 'human_review'` that XState had already persisted to the plan file
|
||||
2. **Sent `TASK_EXECUTION_PROGRESS` with `phase: 'failed'` to the renderer**, overwriting the `planning` phase that XState had already emitted via `emitPhaseFromState`
|
||||
|
||||
Both operations bypassed XState's authority as the source of truth for status.
|
||||
|
||||
### Fix
|
||||
Added an XState "settled state" guard in the `execution-progress` handler. When XState has already transitioned to a settled state (`plan_review`, `human_review`, `error`, `creating_pr`, `pr_created`, `done`), the handler:
|
||||
- Skips `persistPlanPhaseSync` to prevent overwriting XState's persisted status
|
||||
- Skips sending `TASK_EXECUTION_PROGRESS` to the renderer to prevent overwriting XState's emitted phase
|
||||
|
||||
XState's own `persistStatus()` and `emitPhaseFromState()` already handle disk and renderer updates correctly when transitioning to these states.
|
||||
|
||||
### Files Changed
|
||||
- `apps/frontend/src/main/ipc-handlers/agent-events-handlers.ts`
|
||||
|
||||
## Testing
|
||||
|
||||
All fixes pass:
|
||||
- `npm run typecheck` — clean
|
||||
- `npm run test` — 2649 tests passing, 0 failures
|
||||
- Manual testing: multi-project with same specIds, review-required tasks, project switching during QA, refresh at all lifecycle stages
|
||||
@@ -0,0 +1 @@
|
||||
"""Test package for runners module."""
|
||||
@@ -0,0 +1 @@
|
||||
"""Test package for GitHub runner module."""
|
||||
@@ -0,0 +1 @@
|
||||
"""Test package for GitHub services module."""
|
||||
@@ -0,0 +1,335 @@
|
||||
"""
|
||||
Unit tests for pydantic model normalization functions.
|
||||
|
||||
Tests the normalize_category() and normalize_verdict() helper functions
|
||||
that map common synonyms and formatting variations to valid schema values.
|
||||
"""
|
||||
|
||||
import importlib.util
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
# Add the backend directory to the path to allow direct module import
|
||||
backend_path = Path(__file__).parent.parent.parent.parent.parent / "apps" / "backend"
|
||||
sys.path.insert(0, str(backend_path))
|
||||
|
||||
# Import the normalization functions directly from the file
|
||||
# This avoids importing the full module hierarchy which has complex dependencies
|
||||
spec = importlib.util.spec_from_file_location(
|
||||
"pydantic_models",
|
||||
backend_path / "runners" / "github" / "services" / "pydantic_models.py"
|
||||
)
|
||||
pydantic_models = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(pydantic_models)
|
||||
|
||||
normalize_category = pydantic_models.normalize_category
|
||||
normalize_verdict = pydantic_models.normalize_verdict
|
||||
|
||||
|
||||
class TestNormalizeCategory:
|
||||
"""Tests for normalize_category() function."""
|
||||
|
||||
def test_synonyms_duplication_to_redundancy(self):
|
||||
"""Test that 'duplication' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("duplication") == "redundancy"
|
||||
|
||||
def test_synonyms_code_duplication_to_redundancy(self):
|
||||
"""Test that 'code duplication' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("code duplication") == "redundancy"
|
||||
|
||||
def test_synonyms_duplicate_to_redundancy(self):
|
||||
"""Test that 'duplicate' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("duplicate") == "redundancy"
|
||||
|
||||
def test_synonyms_duplicated_to_redundancy(self):
|
||||
"""Test that 'duplicated' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("duplicated") == "redundancy"
|
||||
|
||||
def test_synonyms_copy_to_redundancy(self):
|
||||
"""Test that 'copy' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("copy") == "redundancy"
|
||||
|
||||
def test_synonyms_copied_to_redundancy(self):
|
||||
"""Test that 'copied' synonym maps to 'redundancy'."""
|
||||
assert normalize_category("copied") == "redundancy"
|
||||
|
||||
def test_case_insensitivity_security_uppercase(self):
|
||||
"""Test that uppercase 'SECURITY' normalizes to 'security'."""
|
||||
assert normalize_category("SECURITY") == "security"
|
||||
|
||||
def test_case_insensitivity_security_mixed(self):
|
||||
"""Test that mixed case 'Security' normalizes to 'security'."""
|
||||
assert normalize_category("Security") == "security"
|
||||
|
||||
def test_case_insensitivity_quality_uppercase(self):
|
||||
"""Test that uppercase 'QUALITY' normalizes to 'quality'."""
|
||||
assert normalize_category("QUALITY") == "quality"
|
||||
|
||||
def test_pass_through_valid_security(self):
|
||||
"""Test that valid 'security' passes through unchanged."""
|
||||
assert normalize_category("security") == "security"
|
||||
|
||||
def test_pass_through_valid_quality(self):
|
||||
"""Test that valid 'quality' passes through unchanged."""
|
||||
assert normalize_category("quality") == "quality"
|
||||
|
||||
def test_pass_through_valid_redundancy(self):
|
||||
"""Test that valid 'redundancy' passes through unchanged."""
|
||||
assert normalize_category("redundancy") == "redundancy"
|
||||
|
||||
def test_pass_through_valid_performance(self):
|
||||
"""Test that valid 'performance' passes through unchanged."""
|
||||
assert normalize_category("performance") == "performance"
|
||||
|
||||
def test_pass_through_valid_logic(self):
|
||||
"""Test that valid 'logic' passes through unchanged."""
|
||||
assert normalize_category("logic") == "logic"
|
||||
|
||||
def test_pass_through_valid_test(self):
|
||||
"""Test that valid 'test' passes through unchanged."""
|
||||
assert normalize_category("test") == "test"
|
||||
|
||||
def test_pass_through_valid_docs(self):
|
||||
"""Test that valid 'docs' passes through unchanged."""
|
||||
assert normalize_category("docs") == "docs"
|
||||
|
||||
def test_pass_through_valid_pattern(self):
|
||||
"""Test that valid 'pattern' passes through unchanged."""
|
||||
assert normalize_category("pattern") == "pattern"
|
||||
|
||||
def test_whitespace_handling_leading(self):
|
||||
"""Test that leading whitespace is stripped."""
|
||||
assert normalize_category(" quality") == "quality"
|
||||
|
||||
def test_whitespace_handling_trailing(self):
|
||||
"""Test that trailing whitespace is stripped."""
|
||||
assert normalize_category("quality ") == "quality"
|
||||
|
||||
def test_whitespace_handling_both(self):
|
||||
"""Test that both leading and trailing whitespace is stripped."""
|
||||
assert normalize_category(" quality ") == "quality"
|
||||
|
||||
def test_whitespace_handling_synonym(self):
|
||||
"""Test whitespace stripping with synonym mapping."""
|
||||
assert normalize_category(" duplication ") == "redundancy"
|
||||
|
||||
def test_non_string_input_integer(self):
|
||||
"""Test that non-string input (integer) returns unchanged."""
|
||||
assert normalize_category(42) == 42
|
||||
|
||||
def test_non_string_input_none(self):
|
||||
"""Test that non-string input (None) returns unchanged."""
|
||||
assert normalize_category(None) is None
|
||||
|
||||
def test_non_string_input_list(self):
|
||||
"""Test that non-string input (list) returns unchanged."""
|
||||
test_list = ["quality"]
|
||||
assert normalize_category(test_list) == test_list
|
||||
|
||||
def test_performance_synonyms(self):
|
||||
"""Test performance-related synonyms."""
|
||||
assert normalize_category("perf") == "performance"
|
||||
assert normalize_category("slow") == "performance"
|
||||
assert normalize_category("optimization") == "performance"
|
||||
|
||||
def test_quality_synonyms(self):
|
||||
"""Test quality-related synonyms."""
|
||||
assert normalize_category("code quality") == "quality"
|
||||
assert normalize_category("maintainability") == "quality"
|
||||
assert normalize_category("style") == "quality"
|
||||
assert normalize_category("formatting") == "quality"
|
||||
|
||||
def test_logic_synonyms(self):
|
||||
"""Test logic-related synonyms."""
|
||||
assert normalize_category("correctness") == "logic"
|
||||
assert normalize_category("bug") == "logic"
|
||||
|
||||
def test_test_synonyms(self):
|
||||
"""Test test-related synonyms."""
|
||||
assert normalize_category("testing") == "test"
|
||||
assert normalize_category("tests") == "test"
|
||||
|
||||
def test_docs_synonyms(self):
|
||||
"""Test documentation-related synonyms."""
|
||||
assert normalize_category("documentation") == "docs"
|
||||
assert normalize_category("doc") == "docs"
|
||||
|
||||
def test_security_synonyms(self):
|
||||
"""Test security-related synonyms."""
|
||||
assert normalize_category("sec") == "security"
|
||||
assert normalize_category("vulnerability") == "security"
|
||||
|
||||
def test_unknown_category_passes_through(self):
|
||||
"""Test that unknown category values pass through normalized."""
|
||||
assert normalize_category("unknown") == "unknown"
|
||||
|
||||
def test_empty_string(self):
|
||||
"""Test that empty string passes through as empty."""
|
||||
assert normalize_category("") == ""
|
||||
|
||||
def test_whitespace_only_string(self):
|
||||
"""Test that whitespace-only string normalizes to empty."""
|
||||
assert normalize_category(" ") == ""
|
||||
|
||||
|
||||
class TestNormalizeVerdict:
|
||||
"""Tests for normalize_verdict() function."""
|
||||
|
||||
def test_space_to_underscore_needs_revision(self):
|
||||
"""Test that 'NEEDS REVISION' converts spaces to underscores."""
|
||||
assert normalize_verdict("NEEDS REVISION") == "NEEDS_REVISION"
|
||||
|
||||
def test_space_to_underscore_ready_to_merge(self):
|
||||
"""Test that 'READY TO MERGE' converts spaces to underscores."""
|
||||
assert normalize_verdict("READY TO MERGE") == "READY_TO_MERGE"
|
||||
|
||||
def test_space_to_underscore_merge_with_changes(self):
|
||||
"""Test that 'MERGE WITH CHANGES' converts spaces to underscores."""
|
||||
assert normalize_verdict("MERGE WITH CHANGES") == "MERGE_WITH_CHANGES"
|
||||
|
||||
def test_hyphen_to_underscore_needs_revision(self):
|
||||
"""Test that 'NEEDS-REVISION' converts hyphens to underscores."""
|
||||
assert normalize_verdict("NEEDS-REVISION") == "NEEDS_REVISION"
|
||||
|
||||
def test_hyphen_to_underscore_ready_to_merge(self):
|
||||
"""Test that 'READY-TO-MERGE' converts hyphens to underscores."""
|
||||
assert normalize_verdict("READY-TO-MERGE") == "READY_TO_MERGE"
|
||||
|
||||
def test_lowercase_conversion_needs_revision(self):
|
||||
"""Test that lowercase 'needs_revision' converts to uppercase."""
|
||||
assert normalize_verdict("needs_revision") == "NEEDS_REVISION"
|
||||
|
||||
def test_lowercase_conversion_ready_to_merge(self):
|
||||
"""Test that lowercase 'ready_to_merge' converts to uppercase."""
|
||||
assert normalize_verdict("ready_to_merge") == "READY_TO_MERGE"
|
||||
|
||||
def test_lowercase_conversion_blocked(self):
|
||||
"""Test that lowercase 'blocked' converts to uppercase."""
|
||||
assert normalize_verdict("blocked") == "BLOCKED"
|
||||
|
||||
def test_mixed_case_conversion(self):
|
||||
"""Test that mixed case 'Needs_Revision' converts to uppercase."""
|
||||
assert normalize_verdict("Needs_Revision") == "NEEDS_REVISION"
|
||||
|
||||
def test_synonym_ready_to_ready_to_merge(self):
|
||||
"""Test that 'READY' synonym maps to 'READY_TO_MERGE'."""
|
||||
assert normalize_verdict("READY") == "READY_TO_MERGE"
|
||||
|
||||
def test_synonym_ready_case_insensitive(self):
|
||||
"""Test that 'ready' (lowercase) maps to 'READY_TO_MERGE'."""
|
||||
assert normalize_verdict("ready") == "READY_TO_MERGE"
|
||||
|
||||
def test_synonym_merge_to_ready_to_merge(self):
|
||||
"""Test that 'MERGE' synonym maps to 'READY_TO_MERGE'."""
|
||||
assert normalize_verdict("MERGE") == "READY_TO_MERGE"
|
||||
|
||||
def test_synonym_needs_changes_to_needs_revision(self):
|
||||
"""Test that 'NEEDS_CHANGES' synonym maps to 'NEEDS_REVISION'."""
|
||||
assert normalize_verdict("NEEDS_CHANGES") == "NEEDS_REVISION"
|
||||
|
||||
def test_synonym_needs_changes_with_spaces(self):
|
||||
"""Test that 'NEEDS CHANGES' (with spaces) maps to 'NEEDS_REVISION'."""
|
||||
assert normalize_verdict("NEEDS CHANGES") == "NEEDS_REVISION"
|
||||
|
||||
def test_synonym_request_changes_to_needs_revision(self):
|
||||
"""Test that 'REQUEST_CHANGES' synonym maps to 'NEEDS_REVISION'."""
|
||||
assert normalize_verdict("REQUEST_CHANGES") == "NEEDS_REVISION"
|
||||
|
||||
def test_synonym_changes_requested_to_needs_revision(self):
|
||||
"""Test that 'CHANGES_REQUESTED' synonym maps to 'NEEDS_REVISION'."""
|
||||
assert normalize_verdict("CHANGES_REQUESTED") == "NEEDS_REVISION"
|
||||
|
||||
def test_synonym_approved_to_approve(self):
|
||||
"""Test that 'APPROVED' synonym maps to 'APPROVE'."""
|
||||
assert normalize_verdict("APPROVED") == "APPROVE"
|
||||
|
||||
def test_synonym_block_to_blocked(self):
|
||||
"""Test that 'BLOCK' synonym maps to 'BLOCKED'."""
|
||||
assert normalize_verdict("BLOCK") == "BLOCKED"
|
||||
|
||||
def test_synonym_reject_to_blocked(self):
|
||||
"""Test that 'REJECT' synonym maps to 'BLOCKED'."""
|
||||
assert normalize_verdict("REJECT") == "BLOCKED"
|
||||
|
||||
def test_pass_through_ready_to_merge(self):
|
||||
"""Test that valid 'READY_TO_MERGE' passes through unchanged."""
|
||||
assert normalize_verdict("READY_TO_MERGE") == "READY_TO_MERGE"
|
||||
|
||||
def test_pass_through_needs_revision(self):
|
||||
"""Test that valid 'NEEDS_REVISION' passes through unchanged."""
|
||||
assert normalize_verdict("NEEDS_REVISION") == "NEEDS_REVISION"
|
||||
|
||||
def test_pass_through_blocked(self):
|
||||
"""Test that valid 'BLOCKED' passes through unchanged."""
|
||||
assert normalize_verdict("BLOCKED") == "BLOCKED"
|
||||
|
||||
def test_pass_through_approve(self):
|
||||
"""Test that valid 'APPROVE' passes through unchanged."""
|
||||
assert normalize_verdict("APPROVE") == "APPROVE"
|
||||
|
||||
def test_pass_through_comment(self):
|
||||
"""Test that valid 'COMMENT' passes through unchanged."""
|
||||
assert normalize_verdict("COMMENT") == "COMMENT"
|
||||
|
||||
def test_pass_through_merge_with_changes(self):
|
||||
"""Test that valid 'MERGE_WITH_CHANGES' passes through unchanged."""
|
||||
assert normalize_verdict("MERGE_WITH_CHANGES") == "MERGE_WITH_CHANGES"
|
||||
|
||||
def test_whitespace_handling_leading(self):
|
||||
"""Test that leading whitespace is stripped."""
|
||||
assert normalize_verdict(" BLOCKED") == "BLOCKED"
|
||||
|
||||
def test_whitespace_handling_trailing(self):
|
||||
"""Test that trailing whitespace is stripped."""
|
||||
assert normalize_verdict("BLOCKED ") == "BLOCKED"
|
||||
|
||||
def test_whitespace_handling_both(self):
|
||||
"""Test that both leading and trailing whitespace is stripped."""
|
||||
assert normalize_verdict(" BLOCKED ") == "BLOCKED"
|
||||
|
||||
def test_whitespace_handling_with_synonym(self):
|
||||
"""Test whitespace stripping with synonym mapping."""
|
||||
assert normalize_verdict(" ready ") == "READY_TO_MERGE"
|
||||
|
||||
def test_non_string_input_integer(self):
|
||||
"""Test that non-string input (integer) returns unchanged."""
|
||||
assert normalize_verdict(42) == 42
|
||||
|
||||
def test_non_string_input_none(self):
|
||||
"""Test that non-string input (None) returns unchanged."""
|
||||
assert normalize_verdict(None) is None
|
||||
|
||||
def test_non_string_input_list(self):
|
||||
"""Test that non-string input (list) returns unchanged."""
|
||||
test_list = ["BLOCKED"]
|
||||
assert normalize_verdict(test_list) == test_list
|
||||
|
||||
def test_combined_transformations_space_and_lowercase(self):
|
||||
"""Test combined space replacement and case conversion."""
|
||||
assert normalize_verdict("needs revision") == "NEEDS_REVISION"
|
||||
|
||||
def test_combined_transformations_hyphen_and_lowercase(self):
|
||||
"""Test combined hyphen replacement and case conversion."""
|
||||
assert normalize_verdict("ready-to-merge") == "READY_TO_MERGE"
|
||||
|
||||
def test_combined_transformations_mixed_separators(self):
|
||||
"""Test mixed space and hyphen separators."""
|
||||
assert normalize_verdict("ready to-merge") == "READY_TO_MERGE"
|
||||
|
||||
def test_empty_string(self):
|
||||
"""Test that empty string passes through as empty."""
|
||||
assert normalize_verdict("") == ""
|
||||
|
||||
def test_whitespace_only_string(self):
|
||||
"""Test that whitespace-only string normalizes to empty."""
|
||||
assert normalize_verdict(" ") == ""
|
||||
|
||||
def test_unknown_verdict_passes_through_normalized(self):
|
||||
"""Test that unknown verdict values pass through normalized."""
|
||||
assert normalize_verdict("unknown") == "UNKNOWN"
|
||||
|
||||
def test_multiple_spaces_to_single_underscore(self):
|
||||
"""Test that multiple consecutive spaces become single underscore."""
|
||||
assert normalize_verdict("NEEDS REVISION") == "NEEDS__REVISION"
|
||||
@@ -102,6 +102,7 @@ _modules_to_mock = [
|
||||
"phase_config",
|
||||
"services.pr_worktree_manager",
|
||||
"services.sdk_utils",
|
||||
"services.markdown_utils",
|
||||
"claude_agent_sdk",
|
||||
]
|
||||
_original_modules = {name: sys.modules.get(name) for name in _modules_to_mock}
|
||||
|
||||
Reference in New Issue
Block a user