-
Notifications
You must be signed in to change notification settings - Fork 1
Add --force-model CLI alias for static routing #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new CLI flag --force-model as an alias for --static-route, integrates it into argument parsing, maps it to the same internal static routing override, updates env var handling, and extends tests. Data snapshots updated for CLI flags and test suite state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Parser
participant Core as apply_cli_args
participant Env as Environment
participant Cfg as Config
User->>CLI: run command with --force-model MODEL
CLI->>Core: args(force_model=MODEL)
Core->>Env: set STATIC_ROUTE=MODEL
Core->>Cfg: set backends.static_route=MODEL
Core-->>User: return configured runtime
note over Core,Cfg: --force-model aliases --static-route (mutually exclusive)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/test_cli.py (1)
226-236: Test coverage should verify functional behavior, not just config assignment.The test verifies that
--force-modelsetsconfig.backends.static_route, but doesn't validate whether the resulting value is correctly parsed by the backend service. Given the semantic ambiguity in the CLI help text (whether--force-modelaccepts "MODEL" or "BACKEND:MODEL"), consider adding an integration test that verifies the backend resolution actually works with the value set by--force-model.Example additional test:
@pytest.mark.asyncio async def test_cli_force_model_backend_resolution(): """Verify that --force-model results in correct backend resolution.""" with patch("src.core.config.app_config.load_config", return_value=AppConfig()): args = parse_cli_args(["--force-model", "gemini-2.5-pro"]) config = _unwrap_config(apply_cli_args(args)) # Verify the backend service can parse the static_route value # This test would need appropriate mocks for BackendService # to validate that the model resolution works correctlytests/unit/test_static_route.py (2)
10-10: Consider moving shared test utilities to a common module.Importing
_unwrap_configfrom another test file (test_cli.py) creates coupling between test modules. If the structure oftest_cli.pychanges, this import could break. Consider moving shared test utilities to a dedicated test utilities module (e.g.,tests/conftest.pyortests/utils.py).Example:
Create
tests/utils.py:"""Shared test utilities.""" from src.core.config.app_config import AppConfig, ParameterResolution def unwrap_config( result: AppConfig | tuple[AppConfig, ParameterResolution], ) -> AppConfig: """Unwrap config from apply_cli_args return value.""" return result[0] if isinstance(result, tuple) else resultThen update imports:
-from tests.unit.test_cli import _unwrap_config +from tests.utils import unwrap_config
211-219: Test should verify the full behavior chain, not just config assignment.Similar to the test in
test_cli.py, this test only verifies that--force-modelsets the config value but doesn't validate whether the resultingstatic_routevalue is correctly parsed and used by the backend service. Consider extending this test to verify the end-to-end behavior using the existing test fixtures in this file.Example extension:
@pytest.mark.asyncio async def test_force_model_alias_routes_correctly( self, mock_session_service, mock_backend_factory, mock_wire_capture, mock_rate_limiter, mock_app_state, ): """Test that --force-model results in correct backend routing.""" from src.core.cli import apply_cli_args, parse_cli_args args = parse_cli_args(["--force-model", "gemini-2.5-pro"]) config = _unwrap_config(apply_cli_args(args)) service = BackendService( factory=mock_backend_factory, rate_limiter=mock_rate_limiter, config=config, session_service=mock_session_service, app_state=mock_app_state, wire_capture=mock_wire_capture, failover_routes={}, ) request = ChatRequest( model="gpt-4", messages=[{"role": "user", "content": "test"}], ) backend_type, effective_model = await service._resolve_backend_and_model(request) # Verify the routing works as expected assert effective_model == "gemini-2.5-pro"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
data/cli_flag_snapshot.txt(1 hunks)data/test_suite_state.json(1 hunks)src/core/cli.py(2 hunks)tests/unit/test_cli.py(2 hunks)tests/unit/test_static_route.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/cli.pytests/unit/test_cli.pytests/unit/test_static_route.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/cli.py
🧬 Code graph analysis (2)
tests/unit/test_cli.py (1)
src/core/cli.py (1)
parse_cli_args(514-517)
tests/unit/test_static_route.py (2)
tests/unit/test_cli.py (1)
_unwrap_config(37-40)src/core/cli.py (2)
apply_cli_args(520-1045)parse_cli_args(514-517)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
tests/unit/test_cli.py (1)
20-20: LGTM!Good practice to include
STATIC_ROUTEin the cleanup to prevent test contamination.src/core/cli.py (1)
96-108: No changes required for --force-model
The CLI already assigns the model-only string tostatic_route, and the backend logic treats a no-colon override as “model only” using the configured default backend, matching the existing help text.Likely an incorrect or invalid review comment.
Summary
--force-modelCLI alias that shares the static route logic and remains mutually exclusive with--static-routeTesting
https://chatgpt.com/codex/tasks/task_e_68ec2580a92883338037a8e0a42c4943
Summary by CodeRabbit
New Features
Tests
Chores