fix(runners): use resolve_model_id() for model resolution instead of hardcoded fallbacks (ACS-294) (#1170)
* fix(runners): use resolve_model_id() for model resolution instead of hardcoded fallbacks (ACS-294) This fix ensures that custom model configurations via environment variables (ANTHROPIC_DEFAULT_SONNET_MODEL, ANTHROPIC_DEFAULT_OPUS_MODEL, etc.) are properly respected in PR reviewer services, instead of falling back to hardcoded Claude model IDs. Changes: - parallel_orchestrator_reviewer.py: Import and use resolve_model_id() from phase_config, with fallback to "sonnet" shorthand instead of hardcoded model - parallel_followup_reviewer.py: Same pattern as above - models.py: Update GitHubRunnerConfig default model from hardcoded "claude-sonnet-4-20250514" to "sonnet" shorthand (respects env overrides) - batch_validator.py: Change DEFAULT_MODEL to "sonnet" shorthand and add _resolve_model() method to resolve via phase_config.resolve_model_id() - batch_issues.py: Change validation_model default to "sonnet" shorthand - Add comprehensive tests in test_model_resolution.py to verify model resolution behavior This resolves the issue where logs would display hardcoded model IDs (e.g., "claude-opus-4-5-20251101") instead of the actual configured model (e.g., "glm-4.7"), which caused confusion about which model was being used. Refs: ACS-294 * test: extract environment cleanup into pytest fixture to address PR review feedback - Add clean_env pytest fixture to handle environment variable cleanup - Remove duplicated environment backup/restore logic from 3 tests - Fix import: use collections.abc.Generator instead of typing.Generator This addresses review feedback from PR #1170: - CodeRabbit: Extract duplicated environment cleanup into fixture - ruff: Import Generator from collections.abc The pytest fixture approach is the Pythonic way to handle setup/teardown and reduces code duplication while maintaining test isolation. * test: address CodeRabbit PR review feedback - Fix absolute import issue in batch_validator.py: Use importlib.util.find_spec instead of "from phase_config import resolve_model_id" for more robust imports that don't rely on sys.path - Improve test quality: Add behavioral tests for resolve_model_id() function (11 tests with full coverage of environment variable overrides) - Add documentation explaining why source inspection is used for some tests (to avoid complex import dependencies while still verifying critical patterns) - Add test for importlib.util pattern in batch_validator.py - Add negative assertions to verify old hardcoded fallbacks are not present Addresses CodeRabbit feedback from PR #1170: - Comment #5: Absolute import style (fixed with importlib.util) - Comments #7-11: Brittle source code tests (improved with behavioral tests and documentation explaining why source inspection is used where necessary) All 21 tests pass, ruff checks pass. Refs: ACS-294 * fix: add explanatory comment for empty except and broaden exception handling - Add detailed comment explaining why the empty 'pass' is necessary (ensures BatchValidator remains functional even if phase_config has errors) - Change from except (ImportError, AttributeError) to except Exception to catch broader exceptions for robustness - Addresses GitHub Advanced Security alert about empty except clause - Addresses CodeRabbit feedback about broader exception handling All 21 tests pass, ruff checks pass. Refs: ACS-294 * fix: add resolve_model_id to fallback imports in parallel reviewers - Add resolve_model_id to fallback import in parallel_followup_reviewer.py:64 - Add resolve_model_id to fallback import in parallel_orchestrator_reviewer.py:60 - Fix hardcoded fallback in followup_reviewer.py:688 - use shorthand + resolve_model_id This addresses 3 HIGH priority findings from Auto Claude PR Review: - [e4d8064b75ce] Missing resolve_model_id import in fallback block (parallel_followup_reviewer.py) - [f4beb99bb5d1] Missing resolve_model_id import in fallback block (parallel_orchestrator_reviewer.py) - [c059fa0540e0] Missed hardcoded model fallback (followup_reviewer.py) All 21 tests pass, ruff checks pass. Refs: ACS-294 * fix(batch_issues): resolve model shorthand via resolve_model_id() in ClaudeBatchAnalyzer This fixes the last hardcoded model fallback in ACS-294. The ClaudeBatchAnalyzer.analyze_and_batch_issues() method was using a hardcoded 'claude-sonnet-4-5-20250929' model ID instead of resolving the 'sonnet' shorthand via resolve_model_id(), which prevented environment variable overrides from being respected. Changes: - Import resolve_model_id from phase_config - Replace hardcoded model with resolve_model_id('sonnet') - Add tests to verify the fix This completes the fix for all hardcoded model fallbacks in the GitHub runner services. * test: add UTF-8 encoding to read_text() calls for Windows compatibility On Windows, Path.read_text() defaults to the system encoding (cp1252) which can cause UnicodeDecodeError for files with UTF-8 content. This fixes the CI test failure by explicitly specifying encoding='utf-8' for all read_text() calls in the test file. Fixes test failure: - test_parallel_reviewers_use_sonnet_fallback * test: document UTF-8 encoding requirement for Windows compatibility Adds explanatory comment for encoding parameter in source inspection tests. This clarifies why explicit UTF-8 is needed (platform-dependent defaults). * fix: address PR review findings - import pattern consistency and test improvements MEDIUM: Replace importlib.util pattern with established try/except import pattern - batch_validator.py now uses relative imports with absolute fallback - Matches the convention used across runners/github/services/ - Ensures proper module caching in sys.modules LOW: Add debug logging to exception handler - _resolve_model now logs failures at debug level for diagnosis - Helps identify actual bugs vs expected fallbacks LOW: Extract duplicate file paths to pytest fixtures - Added GITHUB_RUNNER_DIR constant and fixtures for common file paths - Reduces 11 duplicate path constructions to reusable fixtures - Easier maintenance if directory structure changes LOW: Add explanatory comment for implementation-detail test - test_uses_try_except_import_pattern now documents why it tests implementation - Explains the guard against circular dependency import patterns All 23 tests pass. * fix: resolve model shorthand in triage_engine and pr_review_engine (CRITICAL) CRITICAL BUG FIX: Model shorthands (e.g., "sonnet") were being passed directly to create_client() without resolving to full model IDs. The Anthropic API does not recognize shorthands, causing runtime errors. Fixed files: - triage_engine.py: Added resolve_model_id() import and resolution - pr_review_engine.py: Added resolve_model_id() import and resolution at 3 call sites - run_review_pass() (main review pass) - _run_structural_pass() (structural analysis) - _run_ai_triage_pass() (AI comment triage) All changes follow the established pattern used in: - parallel_orchestrator_reviewer.py - parallel_followup_reviewer.py - followup_reviewer.py All 23 tests pass. * fix: address PR review findings - correct import paths and hardcoded models MEDIUM: Fix incorrect relative import paths for phase_config - batch_validator.py: Changed .phase_config to ..phase_config (2 dots from runners/github/) - triage_engine.py: Changed ..phase_config to ...phase_config (3 dots from services/) - pr_review_engine.py: Changed ..phase_config to ...phase_config (3 dots from services/) - Updated test to reflect correct import pattern for batch_validator.py MEDIUM: Fix hardcoded model IDs in batch_processor.py - Changed validation_model="claude-sonnet-4-5-20250929" to "sonnet" on lines 97 and 223 - Ensures consistency with model shorthand resolution pattern LOW: Move inline import to module-level in batch_issues.py - Moved resolve_model_id import to module-level try/except block - Matches established codebase convention for consistency All 23 tests pass. * fix: correct import block ordering for ruff I001 compliance Reordered imports in try/except blocks to comply with ruff's I001 rule: - batch_issues.py: Parent directory imports before same-directory - triage_engine.py: Parent directory imports before same-directory - pr_review_engine.py: Parent directory imports before same-directory All 23 tests pass and ruff checks pass. * fix: improve exception handling robustness in BatchValidator._resolve_model Wrap the fallback import in its own try/except block to ensure any exception during the fallback is caught and logged before returning the original model as a final fallback. This prevents unexpected exceptions from propagating when the absolute import fails. All 23 tests pass and ruff checks pass. * style: add blank line after import for ruff format compliance --------- Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
@@ -23,6 +23,7 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
# Import validators
|
||||
try:
|
||||
from ..phase_config import resolve_model_id
|
||||
from .batch_validator import BatchValidator
|
||||
from .duplicates import SIMILAR_THRESHOLD
|
||||
from .file_lock import locked_json_write
|
||||
@@ -30,6 +31,7 @@ except (ImportError, ValueError, SystemError):
|
||||
from batch_validator import BatchValidator
|
||||
from duplicates import SIMILAR_THRESHOLD
|
||||
from file_lock import locked_json_write
|
||||
from phase_config import resolve_model_id
|
||||
|
||||
|
||||
class ClaudeBatchAnalyzer:
|
||||
@@ -150,11 +152,13 @@ Respond with JSON only:
|
||||
)
|
||||
|
||||
# Using Sonnet for better analysis (still just 1 call)
|
||||
# Note: Model shorthand resolved via resolve_model_id() to respect env overrides
|
||||
from core.simple_client import create_simple_client
|
||||
|
||||
model = resolve_model_id("sonnet")
|
||||
client = create_simple_client(
|
||||
agent_type="batch_analysis",
|
||||
model="claude-sonnet-4-5-20250929",
|
||||
model=model,
|
||||
system_prompt="You are an expert at analyzing GitHub issues and grouping related ones. Respond ONLY with valid JSON. Do NOT use any tools.",
|
||||
cwd=self.project_dir,
|
||||
)
|
||||
@@ -408,7 +412,8 @@ class IssueBatcher:
|
||||
api_key: str | None = None,
|
||||
# AI validation settings
|
||||
validate_batches: bool = True,
|
||||
validation_model: str = "claude-sonnet-4-5-20250929",
|
||||
# Note: validation_model uses shorthand which gets resolved via BatchValidator._resolve_model()
|
||||
validation_model: str = "sonnet",
|
||||
validation_thinking_budget: int = 10000, # Medium thinking
|
||||
):
|
||||
self.github_dir = github_dir
|
||||
|
||||
@@ -21,7 +21,9 @@ logger = logging.getLogger(__name__)
|
||||
CLAUDE_SDK_AVAILABLE = importlib.util.find_spec("claude_agent_sdk") is not None
|
||||
|
||||
# Default model and thinking configuration
|
||||
DEFAULT_MODEL = "claude-sonnet-4-5-20250929"
|
||||
# Note: Default uses shorthand "sonnet" which gets resolved via resolve_model_id()
|
||||
# to respect environment variable overrides (e.g., ANTHROPIC_DEFAULT_SONNET_MODEL)
|
||||
DEFAULT_MODEL = "sonnet"
|
||||
DEFAULT_THINKING_BUDGET = 10000 # Medium thinking
|
||||
|
||||
|
||||
@@ -112,7 +114,8 @@ class BatchValidator:
|
||||
model: str = DEFAULT_MODEL,
|
||||
thinking_budget: int = DEFAULT_THINKING_BUDGET,
|
||||
):
|
||||
self.model = model
|
||||
# Resolve model shorthand via environment variable override if configured
|
||||
self.model = self._resolve_model(model)
|
||||
self.thinking_budget = thinking_budget
|
||||
self.project_dir = project_dir or Path.cwd()
|
||||
|
||||
@@ -121,6 +124,35 @@ class BatchValidator:
|
||||
"claude-agent-sdk not available. Batch validation will be skipped."
|
||||
)
|
||||
|
||||
def _resolve_model(self, model: str) -> str:
|
||||
"""Resolve model shorthand via phase_config.resolve_model_id()."""
|
||||
try:
|
||||
# Use the established try/except pattern for imports (matching
|
||||
# parallel_orchestrator_reviewer.py and other files in runners/github/services/)
|
||||
# This ensures consistency across the codebase and proper caching in sys.modules.
|
||||
from ..phase_config import resolve_model_id
|
||||
|
||||
return resolve_model_id(model)
|
||||
except (ImportError, ValueError, SystemError):
|
||||
# Fallback to absolute import - wrap in try/except for safety
|
||||
try:
|
||||
from phase_config import resolve_model_id
|
||||
|
||||
return resolve_model_id(model)
|
||||
except Exception as e:
|
||||
# Log and return original model as final fallback
|
||||
logger.debug(
|
||||
f"Fallback import failed, using original model '{model}': {e}"
|
||||
)
|
||||
return model
|
||||
except Exception as e:
|
||||
# Log at debug level to aid diagnosis without polluting normal output
|
||||
logger.debug(
|
||||
f"Model resolution via phase_config failed, using original model '{model}': {e}"
|
||||
)
|
||||
# Fallback to returning the original model string
|
||||
return model
|
||||
|
||||
def _format_issues(self, issues: list[dict[str, Any]]) -> str:
|
||||
"""Format issues for the prompt."""
|
||||
formatted = []
|
||||
|
||||
@@ -841,7 +841,9 @@ class GitHubRunnerConfig:
|
||||
)
|
||||
|
||||
# Model settings
|
||||
model: str = "claude-sonnet-4-5-20250929"
|
||||
# Note: Default uses shorthand "sonnet" which gets resolved via resolve_model_id()
|
||||
# to respect environment variable overrides (e.g., ANTHROPIC_DEFAULT_SONNET_MODEL)
|
||||
model: str = "sonnet"
|
||||
thinking_level: str = "medium"
|
||||
|
||||
def to_dict(self) -> dict:
|
||||
@@ -915,6 +917,7 @@ class GitHubRunnerConfig:
|
||||
review_own_prs=settings.get("review_own_prs", False),
|
||||
auto_post_reviews=settings.get("auto_post_reviews", False),
|
||||
allow_fix_commits=settings.get("allow_fix_commits", True),
|
||||
model=settings.get("model", "claude-sonnet-4-5-20250929"),
|
||||
# Note: model is stored as shorthand and resolved via resolve_model_id()
|
||||
model=settings.get("model", "sonnet"),
|
||||
thinking_level=settings.get("thinking_level", "medium"),
|
||||
)
|
||||
|
||||
@@ -94,7 +94,7 @@ class BatchProcessor:
|
||||
min_batch_size=1,
|
||||
max_batch_size=5,
|
||||
validate_batches=True,
|
||||
validation_model="claude-sonnet-4-5-20250929",
|
||||
validation_model="sonnet",
|
||||
validation_thinking_budget=10000,
|
||||
)
|
||||
|
||||
@@ -220,7 +220,7 @@ class BatchProcessor:
|
||||
min_batch_size=1,
|
||||
max_batch_size=5,
|
||||
validate_batches=True,
|
||||
validation_model="claude-sonnet-4-5-20250929",
|
||||
validation_model="sonnet",
|
||||
validation_thinking_budget=10000,
|
||||
)
|
||||
|
||||
|
||||
@@ -683,9 +683,10 @@ Analyze this follow-up review context and provide your structured response.
|
||||
# Use Claude Agent SDK query() with structured outputs
|
||||
# Reference: https://platform.claude.com/docs/en/agent-sdk/structured-outputs
|
||||
from claude_agent_sdk import ClaudeAgentOptions, query
|
||||
from phase_config import get_thinking_budget
|
||||
from phase_config import get_thinking_budget, resolve_model_id
|
||||
|
||||
model = self.config.model or "claude-sonnet-4-5-20250929"
|
||||
model_shorthand = self.config.model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
thinking_level = self.config.thinking_level or "medium"
|
||||
thinking_budget = get_thinking_budget(thinking_level)
|
||||
|
||||
|
||||
@@ -31,7 +31,7 @@ from claude_agent_sdk import AgentDefinition
|
||||
|
||||
try:
|
||||
from ...core.client import create_client
|
||||
from ...phase_config import get_thinking_budget
|
||||
from ...phase_config import get_thinking_budget, resolve_model_id
|
||||
from ..context_gatherer import _validate_git_ref
|
||||
from ..gh_client import GHClient
|
||||
from ..models import (
|
||||
@@ -61,7 +61,7 @@ except (ImportError, ValueError, SystemError):
|
||||
PRReviewResult,
|
||||
ReviewSeverity,
|
||||
)
|
||||
from phase_config import get_thinking_budget
|
||||
from phase_config import get_thinking_budget, resolve_model_id
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
from services.pr_worktree_manager import PRWorktreeManager
|
||||
@@ -488,7 +488,9 @@ The SDK will run invoked agents in parallel automatically.
|
||||
)
|
||||
|
||||
# Use model and thinking level from config (user settings)
|
||||
model = self.config.model or "claude-sonnet-4-5-20250929"
|
||||
# Resolve model shorthand via environment variable override if configured
|
||||
model_shorthand = self.config.model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
thinking_level = self.config.thinking_level or "medium"
|
||||
thinking_budget = get_thinking_budget(thinking_level)
|
||||
|
||||
|
||||
@@ -27,7 +27,7 @@ from claude_agent_sdk import AgentDefinition
|
||||
|
||||
try:
|
||||
from ...core.client import create_client
|
||||
from ...phase_config import get_thinking_budget
|
||||
from ...phase_config import get_thinking_budget, resolve_model_id
|
||||
from ..context_gatherer import PRContext, PRContextGatherer, _validate_git_ref
|
||||
from ..gh_client import GHClient
|
||||
from ..models import (
|
||||
@@ -57,7 +57,7 @@ except (ImportError, ValueError, SystemError):
|
||||
PRReviewResult,
|
||||
ReviewSeverity,
|
||||
)
|
||||
from phase_config import get_thinking_budget
|
||||
from phase_config import get_thinking_budget, resolve_model_id
|
||||
from services.category_utils import map_category
|
||||
from services.io_utils import safe_print
|
||||
from services.pr_worktree_manager import PRWorktreeManager
|
||||
@@ -598,7 +598,9 @@ The SDK will run invoked agents in parallel automatically.
|
||||
prompt = self._build_orchestrator_prompt(context)
|
||||
|
||||
# Use model and thinking level from config (user settings)
|
||||
model = self.config.model or "claude-sonnet-4-5-20250929"
|
||||
# Resolve model shorthand via environment variable override if configured
|
||||
model_shorthand = self.config.model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
thinking_level = self.config.thinking_level or "medium"
|
||||
thinking_budget = get_thinking_budget(thinking_level)
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
try:
|
||||
from ...phase_config import resolve_model_id
|
||||
from ..context_gatherer import PRContext
|
||||
from ..models import (
|
||||
AICommentTriage,
|
||||
@@ -33,6 +34,7 @@ except (ImportError, ValueError, SystemError):
|
||||
ReviewPass,
|
||||
StructuralIssue,
|
||||
)
|
||||
from phase_config import resolve_model_id
|
||||
from services.io_utils import safe_print
|
||||
from services.prompt_manager import PromptManager
|
||||
from services.response_parsers import ResponseParser
|
||||
@@ -228,10 +230,12 @@ class PRReviewEngine:
|
||||
else self.project_dir
|
||||
)
|
||||
|
||||
# Resolve model shorthand (e.g., "sonnet") to full model ID for API compatibility
|
||||
model = resolve_model_id(self.config.model or "sonnet")
|
||||
client = create_client(
|
||||
project_dir=project_root,
|
||||
spec_dir=self.github_dir,
|
||||
model=self.config.model,
|
||||
model=model,
|
||||
agent_type="pr_reviewer", # Read-only - no bash, no edits
|
||||
)
|
||||
|
||||
@@ -491,10 +495,12 @@ class PRReviewEngine:
|
||||
else self.project_dir
|
||||
)
|
||||
|
||||
# Resolve model shorthand (e.g., "sonnet") to full model ID for API compatibility
|
||||
model = resolve_model_id(self.config.model or "sonnet")
|
||||
client = create_client(
|
||||
project_dir=project_root,
|
||||
spec_dir=self.github_dir,
|
||||
model=self.config.model,
|
||||
model=model,
|
||||
agent_type="pr_reviewer", # Read-only - no bash, no edits
|
||||
)
|
||||
|
||||
@@ -549,10 +555,12 @@ class PRReviewEngine:
|
||||
else self.project_dir
|
||||
)
|
||||
|
||||
# Resolve model shorthand (e.g., "sonnet") to full model ID for API compatibility
|
||||
model = resolve_model_id(self.config.model or "sonnet")
|
||||
client = create_client(
|
||||
project_dir=project_root,
|
||||
spec_dir=self.github_dir,
|
||||
model=self.config.model,
|
||||
model=model,
|
||||
agent_type="pr_reviewer", # Read-only - no bash, no edits
|
||||
)
|
||||
|
||||
|
||||
@@ -10,11 +10,13 @@ from __future__ import annotations
|
||||
from pathlib import Path
|
||||
|
||||
try:
|
||||
from ...phase_config import resolve_model_id
|
||||
from ..models import GitHubRunnerConfig, TriageCategory, TriageResult
|
||||
from .prompt_manager import PromptManager
|
||||
from .response_parsers import ResponseParser
|
||||
except (ImportError, ValueError, SystemError):
|
||||
from models import GitHubRunnerConfig, TriageCategory, TriageResult
|
||||
from phase_config import resolve_model_id
|
||||
from services.prompt_manager import PromptManager
|
||||
from services.response_parsers import ResponseParser
|
||||
|
||||
@@ -71,10 +73,12 @@ class TriageEngine:
|
||||
full_prompt = prompt + "\n\n---\n\n" + context
|
||||
|
||||
# Run AI
|
||||
# Resolve model shorthand (e.g., "sonnet") to full model ID for API compatibility
|
||||
model = resolve_model_id(self.config.model or "sonnet")
|
||||
client = create_client(
|
||||
project_dir=self.project_dir,
|
||||
spec_dir=self.github_dir,
|
||||
model=self.config.model,
|
||||
model=model,
|
||||
agent_type="qa_reviewer",
|
||||
)
|
||||
|
||||
|
||||
@@ -0,0 +1,324 @@
|
||||
#!/usr/bin/env python3
|
||||
"""
|
||||
Tests for Model Resolution
|
||||
===========================
|
||||
|
||||
Tests the model resolution functionality including:
|
||||
- resolve_model_id() function from phase_config
|
||||
- Environment variable overrides
|
||||
- Model shorthand to full ID mapping
|
||||
- Default model values in GitHub runner services
|
||||
|
||||
This ensures custom model configurations (e.g., ANTHROPIC_DEFAULT_SONNET_MODEL)
|
||||
are properly respected instead of falling back to hardcoded values.
|
||||
|
||||
Note: Some tests use source code inspection to avoid complex import dependencies
|
||||
while still verifying the critical implementation patterns that prevent regression
|
||||
of the hardcoded fallback bug (ACS-294).
|
||||
"""
|
||||
|
||||
import os
|
||||
import sys
|
||||
from collections.abc import Generator
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
# Add backend to path
|
||||
sys.path.insert(0, str(Path(__file__).parent.parent / "apps" / "backend"))
|
||||
|
||||
from phase_config import MODEL_ID_MAP, resolve_model_id
|
||||
|
||||
# Common paths - extracted to avoid duplication and ease maintenance
|
||||
GITHUB_RUNNER_DIR = (
|
||||
Path(__file__).parent.parent / "apps" / "backend" / "runners" / "github"
|
||||
)
|
||||
GITHUB_RUNNER_SERVICES_DIR = GITHUB_RUNNER_DIR / "services"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def models_file() -> Path:
|
||||
"""Path to models.py in GitHub runner directory."""
|
||||
return GITHUB_RUNNER_DIR / "models.py"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def batch_validator_file() -> Path:
|
||||
"""Path to batch_validator.py in GitHub runner directory."""
|
||||
return GITHUB_RUNNER_DIR / "batch_validator.py"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def batch_issues_file() -> Path:
|
||||
"""Path to batch_issues.py in GitHub runner directory."""
|
||||
return GITHUB_RUNNER_DIR / "batch_issues.py"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def orchestrator_file() -> Path:
|
||||
"""Path to parallel_orchestrator_reviewer.py in GitHub runner services."""
|
||||
return GITHUB_RUNNER_SERVICES_DIR / "parallel_orchestrator_reviewer.py"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def followup_file() -> Path:
|
||||
"""Path to parallel_followup_reviewer.py in GitHub runner services."""
|
||||
return GITHUB_RUNNER_SERVICES_DIR / "parallel_followup_reviewer.py"
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def clean_env() -> Generator[None, None, None]:
|
||||
"""Fixture that provides a clean environment without model override variables.
|
||||
|
||||
This fixture clears all ANTHROPIC_DEFAULT_*_MODEL environment variables
|
||||
before each test and restores them afterward. This ensures tests don't
|
||||
interfere with each other when the user has custom model mappings configured.
|
||||
|
||||
Yields:
|
||||
None
|
||||
"""
|
||||
# Clear any environment variables that might interfere
|
||||
env_vars = [
|
||||
"ANTHROPIC_DEFAULT_SONNET_MODEL",
|
||||
"ANTHROPIC_DEFAULT_OPUS_MODEL",
|
||||
"ANTHROPIC_DEFAULT_HAIKU_MODEL",
|
||||
]
|
||||
env_backup = {k: os.environ.pop(k, None) for k in env_vars}
|
||||
|
||||
yield
|
||||
|
||||
# Restore environment variables
|
||||
for k, v in env_backup.items():
|
||||
if v is not None:
|
||||
os.environ[k] = v
|
||||
|
||||
|
||||
class TestResolveModelId:
|
||||
"""Tests for resolve_model_id function - behavioral tests."""
|
||||
|
||||
def test_resolves_sonnet_shorthand_to_full_id(self, clean_env):
|
||||
"""Sonnet shorthand resolves to full model ID."""
|
||||
result = resolve_model_id("sonnet")
|
||||
assert result == MODEL_ID_MAP["sonnet"]
|
||||
|
||||
def test_resolves_opus_shorthand_to_full_id(self, clean_env):
|
||||
"""Opus shorthand resolves to full model ID."""
|
||||
result = resolve_model_id("opus")
|
||||
assert result == MODEL_ID_MAP["opus"]
|
||||
|
||||
def test_resolves_haiku_shorthand_to_full_id(self, clean_env):
|
||||
"""Haiku shorthand resolves to full model ID."""
|
||||
result = resolve_model_id("haiku")
|
||||
assert result == MODEL_ID_MAP["haiku"]
|
||||
|
||||
def test_passes_through_full_model_id(self):
|
||||
"""Full model IDs are passed through unchanged."""
|
||||
custom_model = "glm-4.7"
|
||||
result = resolve_model_id(custom_model)
|
||||
assert result == custom_model
|
||||
|
||||
def test_passes_through_unknown_shorthand(self):
|
||||
"""Unknown shorthands are passed through unchanged."""
|
||||
unknown = "unknown-model"
|
||||
result = resolve_model_id(unknown)
|
||||
assert result == unknown
|
||||
|
||||
def test_environment_variable_override_sonnet(self):
|
||||
"""ANTHROPIC_DEFAULT_SONNET_MODEL overrides sonnet shorthand."""
|
||||
custom_model = "glm-4.7"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_SONNET_MODEL": custom_model}):
|
||||
result = resolve_model_id("sonnet")
|
||||
assert result == custom_model
|
||||
|
||||
def test_environment_variable_override_opus(self):
|
||||
"""ANTHROPIC_DEFAULT_OPUS_MODEL overrides opus shorthand."""
|
||||
custom_model = "glm-4.7"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_OPUS_MODEL": custom_model}):
|
||||
result = resolve_model_id("opus")
|
||||
assert result == custom_model
|
||||
|
||||
def test_environment_variable_override_haiku(self):
|
||||
"""ANTHROPIC_DEFAULT_HAIKU_MODEL overrides haiku shorthand."""
|
||||
custom_model = "glm-4.7"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_HAIKU_MODEL": custom_model}):
|
||||
result = resolve_model_id("haiku")
|
||||
assert result == custom_model
|
||||
|
||||
def test_environment_variable_takes_precedence_over_hardcoded_map(self):
|
||||
"""Environment variable overrides take precedence over MODEL_ID_MAP."""
|
||||
custom_model = "custom-sonnet-model"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_SONNET_MODEL": custom_model}):
|
||||
result = resolve_model_id("sonnet")
|
||||
assert result == custom_model
|
||||
assert result != MODEL_ID_MAP["sonnet"]
|
||||
|
||||
def test_empty_environment_variable_is_ignored(self):
|
||||
"""Empty environment variable is ignored, falls back to MODEL_ID_MAP."""
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_SONNET_MODEL": ""}):
|
||||
result = resolve_model_id("sonnet")
|
||||
assert result == MODEL_ID_MAP["sonnet"]
|
||||
|
||||
def test_full_model_id_not_affected_by_environment_variable(self):
|
||||
"""Full model IDs are not affected by environment variables."""
|
||||
custom_model = "my-custom-model-123"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_SONNET_MODEL": "glm-4.7"}):
|
||||
result = resolve_model_id(custom_model)
|
||||
assert result == custom_model
|
||||
|
||||
|
||||
class TestGitHubRunnerConfigModelDefaults:
|
||||
"""Tests for GitHubRunnerConfig default model values.
|
||||
|
||||
Uses source inspection to avoid complex import dependencies while
|
||||
verifying the critical pattern: default is shorthand "sonnet", not a
|
||||
hardcoded full model ID.
|
||||
"""
|
||||
|
||||
def test_default_model_is_shorthand(self, models_file: Path):
|
||||
"""GitHubRunnerConfig default model uses shorthand 'sonnet'."""
|
||||
# Explicit UTF-8 encoding required for Windows compatibility (default encoding varies by platform)
|
||||
content = models_file.read_text(encoding="utf-8")
|
||||
# Verify the default is "sonnet" (shorthand), not a hardcoded full model ID
|
||||
assert 'model: str = "sonnet"' in content
|
||||
# Verify the old hardcoded fallback is NOT present
|
||||
assert 'model: str = "claude-sonnet-4-5-20250929"' not in content
|
||||
|
||||
def test_load_settings_default_model_is_shorthand(self, models_file: Path):
|
||||
"""GitHubRunnerConfig.load_settings() uses shorthand 'sonnet' as default."""
|
||||
content = models_file.read_text(encoding="utf-8")
|
||||
# Verify load_settings uses "sonnet" (shorthand) as fallback
|
||||
assert 'model=settings.get("model", "sonnet")' in content
|
||||
|
||||
|
||||
class TestBatchValidatorModelResolution:
|
||||
"""Tests for BatchValidator model resolution.
|
||||
|
||||
Tests verify the try/except import pattern (matching the established
|
||||
codebase convention) and that the shorthand "sonnet" is used as default.
|
||||
"""
|
||||
|
||||
def test_default_model_is_shorthand(self, batch_validator_file: Path):
|
||||
"""BatchValidator DEFAULT_MODEL uses shorthand 'sonnet'."""
|
||||
content = batch_validator_file.read_text(encoding="utf-8")
|
||||
# Verify DEFAULT_MODEL is "sonnet" (shorthand)
|
||||
assert 'DEFAULT_MODEL = "sonnet"' in content
|
||||
|
||||
def test_uses_try_except_import_pattern(self, batch_validator_file: Path):
|
||||
"""BatchValidator uses try/except import pattern (established codebase convention).
|
||||
|
||||
This is an implementation-detail test that guards against import patterns
|
||||
causing circular dependencies. The try/except pattern (relative imports
|
||||
falling back to absolute imports) is the established convention across
|
||||
runners/github/ and ensures proper module caching in sys.modules.
|
||||
|
||||
Note: batch_validator.py is in runners/github/ (not services/), so it uses
|
||||
..phase_config (2 dots) to reach apps/backend/phase_config.py.
|
||||
"""
|
||||
content = batch_validator_file.read_text(encoding="utf-8")
|
||||
# Verify the try/except pattern IS present (relative import first)
|
||||
assert "from ..phase_config import resolve_model_id" in content
|
||||
# Verify fallback to absolute import is present
|
||||
assert "except (ImportError, ValueError, SystemError):" in content
|
||||
assert 'from phase_config import resolve_model_id' in content
|
||||
# Verify debug logging is present for error diagnosis
|
||||
assert "logger.debug" in content
|
||||
|
||||
def test_has_resolve_model_method(self, batch_validator_file: Path):
|
||||
"""BatchValidator has _resolve_model method that resolves models."""
|
||||
content = batch_validator_file.read_text(encoding="utf-8")
|
||||
# Verify _resolve_model method exists
|
||||
assert "def _resolve_model(self, model: str)" in content
|
||||
# Verify it calls resolve_model_id
|
||||
assert "return resolve_model_id(model)" in content
|
||||
|
||||
def test_init_calls_resolve_model(self, batch_validator_file: Path):
|
||||
"""BatchValidator.__init__ calls _resolve_model to resolve the model."""
|
||||
content = batch_validator_file.read_text(encoding="utf-8")
|
||||
# Verify __init__ resolves the model
|
||||
assert "self.model = self._resolve_model(model)" in content
|
||||
|
||||
|
||||
class TestBatchIssuesModelResolution:
|
||||
"""Tests for batch_issues.py validation_model default.
|
||||
|
||||
Uses source inspection to verify shorthand "sonnet" is used as default.
|
||||
"""
|
||||
|
||||
def test_validation_model_default_is_shorthand(self, batch_issues_file: Path):
|
||||
"""IssueBatcher validation_model default uses shorthand 'sonnet'."""
|
||||
content = batch_issues_file.read_text(encoding="utf-8")
|
||||
# Verify validation_model default is "sonnet" (shorthand)
|
||||
assert 'validation_model: str = "sonnet"' in content
|
||||
|
||||
|
||||
class TestClaudeBatchAnalyzerModelResolution:
|
||||
"""Tests for ClaudeBatchAnalyzer model resolution in batch_issues.py.
|
||||
|
||||
Verifies that the hardcoded model ID in analyze_and_batch_issues()
|
||||
has been replaced with resolve_model_id() pattern.
|
||||
"""
|
||||
|
||||
def test_batch_analyzer_resolves_model(self, batch_issues_file: Path):
|
||||
"""ClaudeBatchAnalyzer uses resolve_model_id() instead of hardcoded model ID."""
|
||||
content = batch_issues_file.read_text(encoding="utf-8")
|
||||
|
||||
# Verify the old hardcoded model is NOT present
|
||||
assert 'model="claude-sonnet-4-5-20250929"' not in content
|
||||
assert 'model = "claude-sonnet-4-5-20250929"' not in content
|
||||
|
||||
# Verify resolve_model_id is imported and used
|
||||
assert "from phase_config import resolve_model_id" in content
|
||||
assert "model = resolve_model_id" in content
|
||||
|
||||
def test_batch_analyzer_uses_sonnet_shorthand(self, batch_issues_file: Path):
|
||||
"""ClaudeBatchAnalyzer uses 'sonnet' shorthand, not full model ID."""
|
||||
content = batch_issues_file.read_text(encoding="utf-8")
|
||||
|
||||
# Verify the pattern: model = resolve_model_id("sonnet")
|
||||
assert 'model = resolve_model_id("sonnet")' in content
|
||||
|
||||
|
||||
class TestParallelReviewerImportResolution:
|
||||
"""Tests that parallel reviewers use proper model resolution patterns.
|
||||
|
||||
Includes both behavioral tests (simulating the pattern) and source
|
||||
inspection tests (to verify hardcoded fallbacks are not present).
|
||||
"""
|
||||
|
||||
def test_parallel_reviewers_resolve_models(self, clean_env):
|
||||
"""Parallel reviewers correctly resolve model shorthands using resolve_model_id pattern."""
|
||||
# Simulate the pattern used in parallel reviewers
|
||||
config_model = None
|
||||
model_shorthand = config_model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
|
||||
# Should resolve to the full model ID
|
||||
assert model == MODEL_ID_MAP["sonnet"]
|
||||
|
||||
def test_parallel_reviewers_respect_environment_variables(self):
|
||||
"""Parallel reviewers respect environment variable overrides."""
|
||||
custom_model = "glm-4.7"
|
||||
with patch.dict(os.environ, {"ANTHROPIC_DEFAULT_SONNET_MODEL": custom_model}):
|
||||
config_model = None
|
||||
model_shorthand = config_model or "sonnet"
|
||||
model = resolve_model_id(model_shorthand)
|
||||
|
||||
assert model == custom_model
|
||||
|
||||
def test_parallel_reviewers_use_sonnet_fallback(self, orchestrator_file: Path, followup_file: Path):
|
||||
"""Parallel reviewers use 'sonnet' shorthand as fallback, not hardcoded model IDs."""
|
||||
orchestrator_content = orchestrator_file.read_text(encoding="utf-8")
|
||||
followup_content = followup_file.read_text(encoding="utf-8")
|
||||
|
||||
# Verify the old hardcoded fallback is NOT present (negative assertion)
|
||||
assert 'or "claude-sonnet-4-5-20250929"' not in orchestrator_content
|
||||
assert 'or "claude-sonnet-4-5-20250929"' not in followup_content
|
||||
|
||||
# Verify the new pattern IS present (shorthand fallback)
|
||||
assert 'model_shorthand = self.config.model or "sonnet"' in orchestrator_content
|
||||
assert 'model_shorthand = self.config.model or "sonnet"' in followup_content
|
||||
|
||||
# Verify resolve_model_id is imported and used
|
||||
assert "resolve_model_id" in orchestrator_content
|
||||
assert "resolve_model_id" in followup_content
|
||||
Reference in New Issue
Block a user