feat(algo): let any model run β litellm-backed context window + base-name capability tables#2508
Conversation
Collapse the provider-prefixed capability lists in pr_agent/algo/__init__.py into base-model-keyed sets, matched via a new base_model_name() normalizer that strips the provider prefix, Bedrock region/vendor infixes, and cloud-version / vertex date suffixes. Adding a model is now one line per capability instead of one per provider x region, removing the silent failure where a missing prefix (e.g. bedrock/eu.) sent an unsupported arg to a model. Add a litellm fallback to get_max_tokens() for models absent from the MAX_TOKENS table (the table is still checked first, so every model already in it keeps its exact value), route the direct MAX_TOKENS[...] readers through get_max_tokens(), and bump litellm 1.84.0 -> 1.91.0 so the fallback covers more models. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqHrpoPezxDkD5dg4VuQL5
PR Summary by Qodorefactor(algo): key model capability tables by base model name
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. MAX_TOKENS uses single quotes
|
β¦litellm get_max_tokens now resolves the input context window as: MAX_TOKENS override (exact or base model name) -> litellm's built-in, self-updating registry (exact or base model name) -> config.custom_model_max_tokens -> a conservative default (DEFAULT_MODEL_MAX_TOKENS) with a warning. It no longer raises on a model that is absent from MAX_TOKENS, so any model runs without a code edit. The MAX_TOKENS table stays as the authoritative, deterministic source for the models it lists (checked first); litellm and the default only cover models that previously errored out. base_model_name lets the litellm lookup resolve provider-prefixed / region / cloud-version spellings (e.g. bedrock/us.anthropic.claude-sonnet-4-6-v1:0 -> claude-sonnet-4-6). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqHrpoPezxDkD5dg4VuQL5
| m = model.split("/", 1)[-1] # drop provider prefix (anthropic/, bedrock/, vertex_ai/, ...) | ||
| m = m.replace("@", "-") # vertex date separator -> '-' (matches anthropic spelling) | ||
| m = re.sub(r"-v\d+:\d+$", "", m) # bedrock cloud-version suffix (-v1:0) | ||
| parts = m.split(".") | ||
| while len(parts) > 1 and parts[0] in _PROVIDER_INFIX: # bedrock region + vendor infixes | ||
| parts.pop(0) | ||
| return ".".join(parts) # dotted model versions (gpt-5.2) are preserved |
There was a problem hiding this comment.
2. Stacked prefix not stripped π Bug β‘ Correctness
base_model_name() strips only the first provider prefix segment, so stacked names like azure/openai/<model> normalize to openai/<model> instead of the intended base model name. As a result, LiteLLMAIHandler capability gates that now rely on base_model_name(model) in <set> can fail to apply (e.g., skipping required streaming or incorrectly adding unsupported params) when stacked prefixes occur.
Agent Prompt
### Issue description
`base_model_name()` currently removes only one leading `<provider>/` segment via `model.split("/", 1)[-1]`. However, the codebase can produce/accept stacked prefixes (e.g., `azure/openai/<model>`), which then normalize incorrectly and cause capability-set membership checks to miss.
### Issue Context
`LiteLLMAIHandler.chat_completion()` can prepend `azure/` to an existing model string, and the file explicitly documents stacked-prefix cases. Capability checks for user-message-only, temperature support, reasoning_effort, extended thinking, and streaming now depend on `base_model_name()`, so incomplete normalization breaks these checks.
### Fix Focus Areas
- pr_agent/algo/__init__.py[9-22]
- tests/unittest/test_base_model_name.py[9-45]
### What to change
1. Update `base_model_name()` to repeatedly strip *known provider prefixes* (e.g., `azure`, `openai`, `anthropic`, `bedrock`, `vertex_ai`, `huggingface`, etc.) while the leftmost segment matches a known provider.
- Use a loop like `prefix, _, rest = m.partition('/')` and only drop `prefix` when itβs in a `KNOWN_PROVIDERS` set.
- This preserves non-provider slashes like HuggingFace org/model (`meta-llama/Llama-2...`) while still handling `huggingface/meta-llama/Llama-2...`.
2. Add a unit test that asserts stacked prefixes normalize correctly, e.g.:
- `base_model_name("azure/openai/qwq-plus") == "qwq-plus"`
- optionally add a regression check that `base_model_name("huggingface/meta-llama/Llama-2-7b-chat-hf") == "meta-llama/Llama-2-7b-chat-hf"` (or whatever behavior you intend for that case).
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 684d529 |
Correct MAX_TOKENS values verified wrong against OpenAI/Anthropic docs and litellm's registry: - gpt-5 / gpt-5.1 200K -> 272K, gpt-5.2 / gpt-5.2-codex / gpt-5.3-codex 400K -> 272K (272K is the enforced input cap; 400K over-budgeted and risked 400 errors), gpt-5.1-chat-latest 200K -> 128K (200K exceeded the model). - o1 / o1-2024-12-17 / o3-mini 204800 -> 200000 (204800 exceeded the real limit). - claude-sonnet-4-6 / claude-opus-4-6 (all provider spellings) 200K -> 1M. Drop retired / no-longer-used models (litellm + the never-fail fallback still serve any stragglers): the whole Claude 3.x line (incl. 3.7), Claude 2 / instant, the retired Opus 4 / Sonnet 4 20250514 snapshots, and dead gpt-3.5 snapshots. Also remove claude-3-7-sonnet from CLAUDE_EXTENDED_THINKING_MODELS. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqHrpoPezxDkD5dg4VuQL5
| NO_SUPPORT_TEMPERATURE_MODELS, | ||
| STREAMING_REQUIRED_MODELS, | ||
| SUPPORT_REASONING_EFFORT_MODELS, | ||
| USER_MESSAGE_ONLY_MODELS) | ||
| USER_MESSAGE_ONLY_MODELS, base_model_name) |
There was a problem hiding this comment.
1. base_model_name import unsorted π Rule violation β Maintainability
The modified from pr_agent.algo import (...) block is not isort-sorted (e.g., base_model_name is appended after the constants). This can violate Ruff/isort checks and causes avoidable churn in future automated formatting runs.
Agent Prompt
## Issue description
The `from pr_agent.algo import (...)` names are not ordered in an isort-compliant way after adding `base_model_name`.
## Issue Context
The repo enables Ruff isort checks (`I001`), so unsorted import members can fail linting.
## Fix Focus Areas
- pr_agent/algo/ai_handlers/litellm_ai_handler.py[13-17]
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| m = model.split("/", 1)[-1] # drop provider prefix (anthropic/, bedrock/, vertex_ai/, ...) | ||
| m = m.replace("@", "-") # vertex date separator -> '-' (matches anthropic spelling) | ||
| m = re.sub(r"-v\d+:\d+$", "", m) # bedrock cloud-version suffix (-v1:0) | ||
| parts = m.split(".") | ||
| while len(parts) > 1 and parts[0] in _PROVIDER_INFIX: # bedrock region + vendor infixes | ||
| parts.pop(0) | ||
| return ".".join(parts) # dotted model versions (gpt-5.2) are preserved |
There was a problem hiding this comment.
2. Bedrock v2 name mangled π Bug β‘ Correctness
base_model_name() strips any trailing -v<digits>:<digits> suffix, which incorrectly removes the -v2:1 portion from Bedrockβs anthropic.claude-v2:1 identifier and collapses it to claude. This can cause base-name matching to miss for plausible regional spellings (e.g. bedrock/us.anthropic.claude-v2:1), leading capability gating and token-window selection to fall back to the wrong model key.
Agent Prompt
### Issue description
`base_model_name()` currently removes any suffix matching `-v\d+:\d+$`. This pattern matches Bedrock Claude v2 identifiers like `anthropic.claude-v2:1`, which are *model identity* (not deploy decoration), causing normalization to incorrectly return `claude`.
### Issue Context
This normalization is used for capability gating and for best-effort model registry lookups. Incorrect normalization can make lookups miss and fall back to defaults.
### Fix Focus Areas
- pr_agent/algo/__init__.py[16-22]
- tests/unittest/test_base_model_name.py[1-45]
### Suggested change
- Restrict the Bedrock deploy-suffix removal to the known deploy pattern (e.g. `-v<digits>:0`), or otherwise ensure `claude-v2:1` is preserved.
- Add a unit test asserting:
- `base_model_name("bedrock/anthropic.claude-v2:1") == "claude-v2:1"`
- `base_model_name("bedrock/us.anthropic.claude-v2:1") == "claude-v2:1"`
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d73a9d1 |
get_max_tokens() resolves via MAX_TOKENS.get(model) or MAX_TOKENS.get(base_model_name(model)), so every provider prefix, Bedrock region+vendor infix, cloud-version (-v1:0) and vertex @Date spelling of a model already collapses to one base key. The Claude and Gemini blocks encoded that single fact ~130 times (90 Claude rows -> 10 keys, 36 Gemini rows -> 18 keys). Dedup to base-name keys. Behavior is identical: the parametrized bedrock/vertex/anthropic tests in test_get_max_tokens.py still pass against the bare keys. Also drops the lone retired Claude 2.1 bedrock row, whose base name is degenerate and which litellm still covers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqHrpoPezxDkD5dg4VuQL5
| 'gpt-5-nano': 272000, # 272K, but may be limited by config.max_model_tokens | ||
| 'gpt-5-mini': 272000, # 272K, but may be limited by config.max_model_tokens | ||
| 'gpt-5': 272000, | ||
| 'gpt-5-2025-08-07': 272000, | ||
| 'gpt-5.1': 272000, | ||
| 'gpt-5.1-2025-11-13': 272000, | ||
| 'gpt-5.1-chat-latest': 128000, | ||
| 'gpt-5.1-codex': 272000, | ||
| 'gpt-5.1-codex-mini': 272000, | ||
| 'gpt-5.2': 272000, # 272K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.2-2025-12-11': 272000, # 272K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.2-chat-latest': 128000, # 128K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.2-codex': 400000, # 400K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.3-codex': 400000, # 400K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.2-codex': 272000, # 272K, but may be limited by config.max_model_tokens | ||
| 'gpt-5.3-codex': 272000, # 272K, but may be limited by config.max_model_tokens |
There was a problem hiding this comment.
1. max_tokens uses single quotes π Rule violation β Maintainability
Modified/added entries in MAX_TOKENS use single-quoted strings, which conflicts with the repo Python formatting standard (double quotes). This can cause Ruff/format checks to fail and introduce style drift across the codebase.
Agent Prompt
## Issue description
Modified lines in `pr_agent/algo/__init__.py` introduce/retain single-quoted string literals in `MAX_TOKENS`, conflicting with the repository Python formatting standard (Ruff formatter prefers double quotes).
## Issue Context
These are modified/added lines in this PR, so they are in-scope for style compliance and may fail CI lint/format checks.
## Fix Focus Areas
- pr_agent/algo/__init__.py[53-66]
β Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 3943da6 |
Goal
Modernize model handling so any model runs without a per-model code edit, and stop the "add a model to the list" PR treadmill β while keeping the explicit, tested context-window values the project relies on.
Changes
1.
get_max_tokensnever fails; litellm is the safety netResolution order:
An unknown model used to raise; now it runs. The table stays the authoritative, deterministic source for the models it lists (checked first); litellm + default only cover models that used to error out β so a brand-new model works with no edit. Direct
MAX_TOKENS[...]readers route throughget_max_tokens()(cap=Falsefor the uncapped-window callers).2. Capability tables keyed by base model name
base_model_name()strips provider prefix, Bedrock region/vendor infixes, and cloud-version/date suffixes, so every spelling collapses to one key. Capability sets drop from ~45β50 provider-prefixed rows to a handful of base names, matched viabase_model_name(model) in <set>β adding a model is one line per capability instead of one per providerΓregion, closing the silent gap where a missing prefix (e.g.bedrock/eu.) sent an unsupported arg. Same normalizer keys the litellm lookup (bedrock/us.anthropic.claude-sonnet-4-6-v1:0βclaude-sonnet-4-6).3. Corrected stale context windows (verified vs OpenAI/Anthropic docs + litellm)
gpt-5/5.1200Kβ272K;gpt-5.2/5.2-codex/5.3-codex400Kβ272K (272K is the enforced input cap β 400K over-budgeted and risked 400s);gpt-5.1-chat-latest200Kβ128K (200K exceeded the model).o1/o1-2024-12-17/o3-mini204800β200000 (204800 exceeded the real limit).claude-sonnet-4-6/claude-opus-4-6(all spellings) 200Kβ1M.4. Dropped retired / unused models
Whole Claude 3.x line (incl. 3.7), Claude 2/instant, retired Opus 4 / Sonnet 4
20250514snapshots, dead gpt-3.5 snapshots. litellm + never-fail still serve any stragglers.claude-3-7-sonnetalso removed fromCLAUDE_EXTENDED_THINKING_MODELS.5. litellm bump 1.84.0 β 1.91.0
Why keep the table instead of fully delegating to litellm?
Verified against litellm's live registry + official docs:
gpt-5.4-mini/nanoat 1.05M (real: 400K; it copied the flagship's number). Kept as explicit overrides.test_get_max_tokens.pypins).temperature, but Anthropic's docs state those reject non-defaulttemperature/top_p/top_kwith a 400 on every request. So the capability lists are correct and stay.litellm is the safety net for unknowns, not the source of truth for knowns β the pattern Aider uses.
Tests
test_base_model_name.py;test_litellm_claude_extended_thinking.pyupdated for set storage;test_get_max_tokens.pyupdated so the unknown-model case asserts the default instead of raising, and sonnet-4-6/opus-4-6 assert the corrected 1M.π€ Generated with Claude Code