v2.3.0 — quality, multimodal, agentic redundancy (Phases 0–8) - #9
Conversation
Query D grounding was a REPLACE-THIS placeholder inflating the baseline (1/1 grounded on a fake claim). Dropped from answers_and_citations.json; report tables mark it not-recorded until regeneration. Version bumped to 2.3.0-dev for traceable eval reports.
…on gate - Add per-language breakdown (lang field on queries, grouped mean-overall in report + terminal). Validates the multilingual USP instead of averaging it away. - Add optional cross-encoder grounding judge (BAAI/bge-reranker-v2-m3) with Jaccard fallback for offline CI (--grounding-judge). - Add per-query regression gate (--baseline / --regression-eps): fail if any query's overall drops vs committed baseline. - Fix nDCG >1.0 bug: dcg_at_k now dedups by first occurrence, so a run that repeats a paper_id can't double-count gain past ideal. - Fix Windows portability: utf-8 on all file I/O (required for Indic-script queries; cp1252 was crashing on report bars). - Add test_evaluate.py (11 plain-assert checks, no framework). nDCG@10 and Recall@20 were already present — plan's Task 3 was stale, skipped.
finalizer_node now aggregates a directional answer_confidence (0.5*faithfulness + 0.3*completeness + 0.2*citation-coverage) from the final reflexion feedback, and emits an explicit insufficient-evidence answer when the corpus is grounded (faithfulness >= 0.75) but incomplete (completeness < ABSTAIN_COMPLETENESS_FLOOR) after the reflexion budget is spent. Low-faithfulness answers are left to the existing reflexion caveat — abstention is an evidence-gap signal, not a hallucination signal. - state.py: add answer_confidence, abstained to AgentState. - config.py + .env.example: ANSWER_CONFIDENCE_ENABLE, ABSTAIN_COMPLETENESS_FLOOR. - routes/agent.py: expose both on AgentQueryResponse (backward-compatible). - tests/test_finalizer.py: 7 checks (coverage, weighting, abstain gates). Confidence weights are uncalibrated until the Phase 1 eval reliability curve exists. SSE streaming + UI badge are the next commit.
Extract figure/table regions (PyMuPDF, CPU-side in the pool worker), have the Gemini VLM describe each, and index caption+description as chunks in the same collection so retrieval and citation are unchanged. - figure_captioner.py: two-stage extract_regions (in-memory PNG bytes) + caption_regions (network, parent-side after dedup). Crops written to disk only for regions that yield an indexed chunk — no orphan PNGs. Self-check. - ingest.py: figures threaded through _build_paper_chunks / ingest_paper / ingest_pdf / _extract_worker / ingest_directory. Captioning runs only after dedup passes, so duplicate/unchanged papers are never captioned. - config.py + .env.example: ENABLE_MULTIMODAL_INGEST=false (off by default), MULTIMODAL_MAX_FIGS_PER_DOC=12, FIGURES_DIR for saved crops. Off by default; per-doc VLM cost bounded by the fig cap.
- api_server.py: mount FIGURES_DIR at /figures (StaticFiles, traversal-safe,
read-only) so crops are fetchable by URL.
- rag.extract_citations: attach the figure/table crops a cited paper
contributed as {page, chunk_type, url}; _crop_url maps a stored crop path to
its /figures URL and rejects anything outside FIGURES_DIR.
- routes/query.py: FigureRef model + optional Citation.figures (additive,
backward compatible). SSE path inherits it via extract_citations.
- static/index.html: render figure thumbnails as clickable links under each
citation row (both non-stream and streaming paths).
Feature stays off by default; citations without figures render unchanged.
esc() used textContent->innerHTML, which escapes &<> but not quotes, yet it's used inside href="..."/title="..." (citeFigs figure links, source hrefs). A value containing a double quote could break out of the attribute. Not currently reachable (crop URLs are sanitized), but fix at the shared escaper so every attribute use is covered. Now encodes & < > " '.
Reuse the faithfulness NLI cross-encoder to run pairwise contradiction scoring over the top retrieved passages. When sources disagree above CONTRADICTION_NLI_THRESHOLD, instruct the answer generator to present both positions with citations instead of silently picking one. - contradiction.py: bounded O(n^2) NLI over top-8 passages, same-paper pairs skipped, deduped by title pair, directional scoring (max of both orderings) - config: CONTRADICTION_DETECT_ENABLE, CONTRADICTION_NLI_THRESHOLD, NLI_CONTRADICTION_INDEX (mirrors NLI_ENTAILMENT_INDEX label order) - answer_generator: best-effort, gated, never blocks the answer - .env.example: documented knobs, off by default
llm_generate_stream had no failover ('primary model only'), so a 503 from
the primary model (gemini-3.5-flash overload) left the chat UI hanging with
no answer, while the non-stream generate_with_failover survived the same
outage. Mirror that logic for streaming: rotate all API keys and fall back
to LLM_FALLBACK_MODEL on transient errors. Failover is pre-first-chunk only;
a mid-stream failure re-raises rather than restarting a partial answer.
Also gate thinking_config on the model — Gemma rejects it with a permanent
400, which previously made any fallback to Gemma abort instead of recover.
Verified live during a real gemini-3.5-flash 503: all 3 keys failed over,
fell back to Gemma, streamed successfully.
generate_with_failover passed the caller's Gemini config (thinking_config set) straight to the fallback model. Gemma rejects thinking_config with a permanent 400, so every agent node (QueryPlanner, ToolSelector, AnswerGenerator, Reflexion) collapsed to 'AI model temporarily unavailable' the moment the primary tripped its circuit. Strip thinking_config per-model inside the failover loop so the fallback actually serves. Complements d0fafc3 which fixed the same gap on the streaming path. Verified live: primary 503 -> failover to Gemma succeeds without 400.
Each sub-query triggers a retrieve + CPU cross-encoder rerank pass; N concurrent passes thrash a CPU-only box rather than parallelize. Hardcoded cap of 6 across planner, tool_selector, and tool_executor is now a single env knob (default 3), the main lever for agent-mode latency. Planner prompt also targets the knob so the LLM stops over-generating sub-queries that get sliced off.
…rence The reranker and NLI faithfulness cross-encoders run at ~10-100s/pass in fp32 on CPU-only hardware. int8 ONNX via optimum's export_dynamic_quantized_onnx_model is ~2-4x faster. Pins optimum>=2.1.0, onnx>=1.22.0, onnxruntime>=1.23.2.
On CPU-only hardware the fp32 cross-encoders dominate agent latency: NLI faithfulness ~10s/pair (200-400s per reflexion pass) and bge-reranker ~35s per 15-pair retrieval. int8 dynamic-quantized ONNX via optimum is ~11x faster for NLI (0.9s/pair) and ~3x for the reranker (11s/15-pairs), with matching scores (entailment discrimination and rerank ordering verified). New onnx_ce.py self-bootstraps: first load exports+quantizes from the local HF snapshot (fully offline) and caches under models/onnx_ce/. verify._load and rerank._load try ONNX first and fall back to fp32 torch, so a box without the optimum/onnx stack never hard-fails. Artifacts are gitignored (rebuilt per box).
Add a 'watches' table + CRUD (save/get/list/due/delete) to persistence.py, following the existing job/prefs pattern (module-level sqlite3 + WAL + lock). The full watch dict lives in a json 'data' column; user_id/next_run are denormalized for per-user listing and due-watch selection by the scheduler. First increment of the watch-a-topic feature (registration, run-one-watch, and the schedule loop follow). Covered by tests/test_watch_persistence.py.
Add /watch CRUD (create/list/get/delete) in routes/watch.py, gated by config.WATCH_ENABLE (404 when off, mirroring /prefs). Register the router in api_server.py; add WATCH_ENABLE + WATCH_DEFAULT_CADENCE knobs to config.py and .env.example. New watches get next_run one cadence interval out; nothing runs them until the run endpoint / schedule loop land in later increments. 8 endpoint tests in tests/test_watch_routes.py.
run_watch(watch_id): arxiv search (open-access fallback) → skip seen arxiv_ids → download pdf_url + ingest_pdf (n_chunks>0 = genuinely new via content-hash dedup; 0 = corpus dup, marked seen but not new) → cited LLM digest via rag.llm_generate → grow seen_ids, store latest_digest, advance next_run. No-PDF/download-fail falls back to abstract-only in the digest. Endpoint runs synchronously in a threadpool (ingest blocks the loop); missing watch → 404. New knob WATCH_MAX_RESULTS (default 10). Tests: tests/test_watch_run.py (6) — dedup, corpus-dup marks-seen-not-new, abstract-only, missing→404, digest persistence, next_run advance. 20 watch tests green.
Backend (routes/chat.py): GET /chat lists saved sessions (recent-first,
120-char preview, turn count) and GET /chat/{session_id} returns a session's
full message history so a conversation can be reopened. Read-only over the
existing in-memory session store; auth-gated. History is global (no per-user
isolation).
UI (static/index.html):
- 🕘 Chat History modal — list, reopen (re-renders turns, restores session_id),
delete.
- 📡 Topic Watches modal — CRUD against the Increment 2 /watch routes
(create/list/delete), per-browser user_id in localStorage, 404 → "disabled"
hint.
Each watch row gets a "Run now" button that POSTs /watch/{id}/run; the returned
cited digest renders inline under the list (markdown via renderContent), and the
list refreshes so seen-count / next-run update. Toast reports new_count.
watch_runner.run_due_watches() runs every watch whose next_run has arrived (each in a worker thread since run_watch blocks; one failure doesn't abort the sweep). watch_loop() polls it every WATCH_POLL_INTERVAL seconds and is started from the FastAPI lifespan only when WATCH_ENABLE is set, cancelled cleanly on shutdown. Single-worker, in-process — no Redis/Celery. New knob WATCH_POLL_INTERVAL (default 3600s). Test: run_due_watches runs each due watch and survives a per-watch failure. 8 watch-run tests green.
GET /watch/{id}/digest returns the stored latest_digest so a digest
survives modal close/reload, not just the moment after a run. UI adds a
Digest button on digest-ready rows; runWatch + viewDigest share showDigest.
POST /report runs an async job that decomposes a topic into sections
(one LLM plan call), synthesizes a cited section per part from the corpus
via rag.answer_question, and stores a downloadable Markdown artifact.
GET /report/status/{id} polls progress; GET /report/{id}/download returns
the .md. Gated by REPORT_ENABLE (404 when off), reuses the deps job store.
UI: Lit Review modal (topic + language, live progress, blob download).
Corpus-only in v1 (sidesteps the corpus/external provenance-merge rule);
external per-section search deferred.
… cross-provider failover
|
Warning Review limit reached
Next review available in: 49 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. 📝 WalkthroughWalkthroughThis update adds multi-provider LLM routing, agent controls, confidence and contradiction handling, multimodal ingestion, topic watches, literature reports, chat history APIs, ONNX fallbacks, evaluation changes, and corresponding API and UI integrations. ChangesIndicRAG v2.3 platform expansion
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routes/agent.py (1)
96-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInitialize
answer_confidenceandabstainedin the AgentState constructor.
AgentStatedeclaresabstained: boolas a non-Optional (required) TypedDict key, but the initialization here omits bothanswer_confidenceandabstained. While the response construction at lines 209–210 safely uses.get()with defaults, any node that readsstate["abstained"]before the finalizer sets it will raise aKeyError, and type checkers will flag the missing required key.🛠️ Proposed fix
strategy=body.strategy, start_time=time.monotonic(), requested_model=body.model, requested_provider=body.provider, + answer_confidence=None, + abstained=False, )🤖 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 `@routes/agent.py` around lines 96 - 113, Update the AgentState constructor in the initial state setup to include initial values for both required keys, answer_confidence and abstained, using the appropriate unset confidence value and false for abstained. Keep the existing response defaults and other state initialization unchanged.
🟡 Minor comments (15)
contradiction.py-61-79 (1)
61-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTwo untitled chunks both default to
"Unknown"and get treated as "the same paper".
_title()falls back to the literal string"Unknown"when no title metadata is present. If two distinct (but both untitled) chunks are compared,ti == tjis true and the pair is skipped via the same-paper guard — even though they may be genuinely different sources that actually contradict. Low practical impact since ingested chunks normally carry titles, but the fallback silently disables detection for that (edge) case.🩹 Proposed fix
def _title(i: int) -> str: m = metas[i] if i < len(metas) else None - return ((m or {}).get("title") or "Unknown").strip() or "Unknown" + return ((m or {}).get("title") or f"Unknown-{i}").strip() or f"Unknown-{i}"🤖 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 `@contradiction.py` around lines 61 - 79, Update the title handling in the comparison loop around _title so missing metadata does not cause distinct untitled chunks to be classified as the same paper. Preserve the same-paper skip for genuinely matching known titles, while allowing pairs whose titles both resolve to the fallback value to proceed through contradiction scoring.agent/nodes/answer_generator.py-37-48 (1)
37-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit contradiction checks to the kept context.
find_contradictions()runs on the fullchunks/metaslists, butrag.format_context()can stop early on chunk/length limits. That can add a contradiction note for sources the model never saw; slice tochunks_usedbefore calling it.🤖 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 `@agent/nodes/answer_generator.py` around lines 37 - 48, The contradiction check currently analyzes all retrieved chunks instead of only the context retained by rag.format_context(). In the CONTRADICTION_DETECT_ENABLE block, update the find_contradictions() call to pass chunks and metas truncated to chunks_used, preserving aligned chunk/metadata pairs and the existing best-effort error handling.providers/openrouter.py-120-130 (1)
120-130: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against empty
choicesingenerate_stream.
chunk.choices[0]will raiseIndexErrorif a streaming chunk has an empty choices array. Some OpenAI-compatible APIs (including OpenRouter) may emit usage-only final chunks withchoices: [].🛡️ Proposed fix: skip empty-choice chunks
def generate_stream(self, model: str, contents, gen_config) -> Iterator[str]: client = self._get_client() emitted = False for chunk in client.chat.completions.create(**self._params(model, contents, gen_config, stream=True)): + if not chunk.choices: + continue delta = chunk.choices[0].delta text = getattr(delta, "content", None) if text: emitted = True yield text if not emitted: raise RuntimeError("No text generated from OpenRouter stream")🤖 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 `@providers/openrouter.py` around lines 120 - 130, Update generate_stream to skip any streaming chunk whose choices collection is empty before accessing chunk.choices[0]. Continue processing populated chunks and preserve the existing no-text RuntimeError when no content is emitted.providers/openrouter.py-25-29 (1)
25-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFlatten structured contents before OpenRouter translation. The repo already passes Gemini
Content/Partlists fromagent/nodes/answer_generator.pyandfigure_captioner.py;str(contents)turns those into reprs and drops the actual prompt payload. [providers/openrouter.py:25-29]🤖 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 `@providers/openrouter.py` around lines 25 - 29, Update the non-string contents handling in the OpenRouter message translation flow to extract and concatenate the actual text from Gemini Content/Part objects instead of using str(contents). Preserve direct string handling, and ensure lists containing structured parts produce the same prompt text passed by answer_generator and figure_captioner.config.py-484-487 (1)
484-487: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate
VERSIONto"2.3.0"for the release.The PR title is "v2.3.0" and the summary says it rolls up Phases 0–8 from
2.3.0-devintomain, butVERSIONis still"2.3.0-dev". As per coding guidelines, "Keep the project version string inconfig.pyasVERSIONand update it for releases."🔧 Proposed fix
-VERSION = "2.3.0-dev" +VERSION = "2.3.0"🤖 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 `@config.py` around lines 484 - 487, Update the VERSION constant in config.py from the development value to the release value "2.3.0", preserving the existing version declaration format.Source: Coding guidelines
routes/chat.py-185-205 (1)
185-205: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse safe access for
m["content"]to preventKeyError.Line 196 uses
m["content"](direct access) alongsidem.get("role")(safe access) on the same expression. If a message dict hasrole == "user"but lacks acontentkey, this raises an unhandledKeyErrorinside the lock.🛡️ Proposed fix
- first_user = next((m["content"] for m in msgs if m.get("role") == "user"), "") + first_user = next((m.get("content", "") for m in msgs if m.get("role") == "user"), "")🤖 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 `@routes/chat.py` around lines 185 - 205, Update the first_user extraction in list_sessions to safely read each user message’s content instead of indexing m["content"], while preserving the existing empty-string fallback when no usable user content exists.routes/query.py-44-45 (1)
44-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
model/providerthrough the query endpoints
QueryRequestaccepts allowlistedmodelandprovider, but both/queryand/query/streamdrop them before calling the RAG layer, so these fields have no effect. Either pass them through torag.answer_question/rag.prepare_query_for_stream, or remove them from this request if query mode should always use the default model.🤖 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 `@routes/query.py` around lines 44 - 45, Thread the QueryRequest model and provider values through both /query and /query/stream handlers into rag.answer_question and rag.prepare_query_for_stream, respectively, preserving omitted values so the RAG layer can apply its defaults; alternatively remove these fields from QueryRequest only if query endpoints must always use the default model.figure_captioner.py-181-195 (1)
181-195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDangling
crop_pathon write failure.If the crop write fails, the chunk still records
crop_pathas if the file exists, leaving a broken reference for any consumer that renders/links it later.🛠️ Proposed fix
out_dir.mkdir(parents=True, exist_ok=True) crop_path = out_dir / f"{r['page']}_{r['idx']}.png" + crop_written = True try: crop_path.write_bytes(r["png"]) except Exception as e: # a missing crop must not lose the text chunk logger.warning("crop write failed %s: %s", crop_path, e) + crop_written = False label = "Table" if r["kind"] == "table" else "Figure" chunks.append({ "text": f"[{label} on page {r['page']}] {body}", "chunk_type": r["kind"], "page": r["page"], - "crop_path": str(crop_path), + "crop_path": str(crop_path) if crop_written else None, "caption": r["caption"], })🤖 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 `@figure_captioner.py` around lines 181 - 195, Update the crop-writing flow before the chunks.append call so crop_path is recorded only when crop_path.write_bytes succeeds; on failure, use the existing missing-value convention for optional crop paths while preserving the warning and chunk creation.static/index.html-1112-1116 (1)
1112-1116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
modelinto the standard chat request body. The non-agent/chat/streampayload still omitsmodel, so the dropdown only affects agent mode.🤖 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 `@static/index.html` around lines 1112 - 1116, Update the standard `/chat/stream` request payload to include the selected model from `modelSelect`, matching the existing `model` field behavior in the agent request while preserving the current null fallback.watch_runner.py-27-37 (1)
27-37: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRestrict PDF downloads to
http/httpsand cap the response size.watch_runner.py:27-33
_download_pdf()will fetch whateverpdf_urlthe search APIs return, with no scheme check and an unboundedresp.read(). Limiting it to expected schemes and a max byte count would avoid unexpected fetch targets and prevent a large response from exhausting memory.🤖 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 `@watch_runner.py` around lines 27 - 37, Update _download_pdf to reject URLs whose scheme is not http or https before opening the request, and enforce a maximum response size while reading instead of using an unbounded resp.read(). Preserve the existing temporary-file cleanup/error behavior and return None for invalid schemes or oversized responses.Source: Linters/SAST tools
rag.py-38-46 (1)
38-46: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse
safeHref()for figure URLs too.esc()only HTML-escapes the value; it doesn’t enforce the same http/https URL check used elsewhere. Apply it tof.urlbefore using it in both thehrefandsrcfor cited figures.🤖 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 `@rag.py` around lines 38 - 46, Apply safeHref() to f.url in the cited-figure rendering path before using it for either the href or src attribute. Reuse the sanitized result for both attributes, preserving the existing figure URL generation and HTML escaping behavior.Source: Coding guidelines
docs/superpowers/specs/2026-07-12-openrouter-provider-design.md-146-152 (1)
146-152: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc overstates what
--model/--provideractually do inevaluate.py.This says the flags are "threaded through to the queries it issues," but the implementation only attaches them as labels on already-generated results (its own inline comment: answers are generated out-of-band and this harness only scores them). Worth tightening the wording so readers don't assume the eval harness can drive a live model run.
🤖 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 `@docs/superpowers/specs/2026-07-12-openrouter-provider-design.md` around lines 146 - 152, Revise the “Per-model eval hook” section to accurately state that evaluate.py only applies --model/--provider as labels to pre-generated results for scoring. Remove the claim that the flags are threaded into queries or drive live answer generation, while preserving the purpose of distinguishing model/provider results in reports.docs/Eval/evaluate.py-184-186 (1)
184-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport methodology text goes stale when a non-default grounding judge is selected.
evaluate()/the returned metrics dict never records which grounding judge (jaccardvscross-encoder) or threshold was actually used.markdown_report's hardcoded description ("Citation grounding uses token Jaccard similarity (threshold 0.15)") is unconditional, so a report generated with--grounding-judge cross-encoderwill misrepresent its own methodology.📝 Proposed fix: propagate the judge label into metrics/report
sim_fn, threshold = make_grounding_scorer(args.grounding_judge) metrics = evaluate(judgments, results, args.k, sim_fn, threshold) + metrics["grounding_judge"] = args.grounding_judgeAnd branch the methodology text in
markdown_reportonmetrics.get("grounding_judge", "jaccard").Also applies to: 397-398
🤖 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 `@docs/Eval/evaluate.py` around lines 184 - 186, Update evaluate() to record the selected grounding judge and threshold in its returned metrics dictionary, preserving the actual sim_fn selection including cross-encoder mode. Update markdown_report to use metrics.get("grounding_judge", "jaccard") and the recorded threshold when generating the methodology text, so reports accurately describe the configured grounding method while retaining the default wording for older metrics.docs/superpowers/specs/2026-07-12-openrouter-provider-design.md-93-101 (1)
93-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFenced code block missing a language tag (MD040).
As per static analysis hints (markdownlint-cli2, MD040 fenced-code-language).📝 Proposed fix
-``` +```dotenv LLM_PROVIDER=gemini # default backend: gemini|openrouter🤖 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 `@docs/superpowers/specs/2026-07-12-openrouter-provider-design.md` around lines 93 - 101, Update the fenced code block in the OpenRouter provider design configuration section to include the dotenv language tag, changing the opening fence to identify the block as dotenv while leaving its configuration contents unchanged.Source: Linters/SAST tools
report_runner.py-27-46 (1)
27-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
languagethrough report planning and metadataplan_sections()ignores itslanguageargument, so section titles are always planned in English, andrun_report()returns the caller-supplied language even though section synthesis infers language from the generated query. Pass the intended language into planning and return the language actually used by the generated report.🤖 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 `@report_runner.py` around lines 27 - 46, Update plan_sections() to include its language argument in the LLM planning prompt so generated section titles use the requested language. In run_report(), pass the intended language through to plan_sections() and set report metadata to the language actually used by query-driven synthesis, rather than blindly returning the caller-supplied value.
🧹 Nitpick comments (11)
rerank.py (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ONNX-then-fallback loading logic vs.
verify.py.This
_load()body (ONNX try / fp32 except, device selection, logging) is line-for-line identical toverify.py's_load(). Consider factoring the common "try onnx_ce.load(), else build fp32 CrossEncoder with device selection" logic into a small helper inonnx_ce.py(e.g.onnx_ce.load_or_fallback(model_name, subdir, cache_folder)) that both callers invoke, so the two loaders can't drift.🤖 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 `@rerank.py` around lines 19 - 30, Factor the duplicated ONNX-loading and fp32 fallback flow from rerank.py and verify.py into a shared helper in onnx_ce.py, such as load_or_fallback(model_name, subdir, cache_folder). Move device selection and associated success/fallback logging into that helper, then update both _load() implementations to invoke it while preserving their current model names, cache directories, and return behavior.contradiction.py (1)
65-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch NLI calls instead of one
model.predict()per pair.Each combination triggers its own
_contradiction_probs()call (a freshmodel.predict()invocation for just 2 rows), up to 28 times for the max 8-item cap. Since all pairs are known upfront, collecting every(premise, hypothesis)tuple across all valid combinations and issuing one batchedmodel.predict()call would cut per-call overhead substantially — relevant given the module's own docstring already flags this as a bounded-but-real O(n²) cost.🤖 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 `@contradiction.py` around lines 65 - 76, Batch contradiction scoring in the loop that builds found instead of calling _contradiction_probs separately for each pair. First collect both directional text pairs for every valid combination, make one model prediction through _contradiction_probs, then map the returned scores back to their corresponding combinations while preserving threshold filtering and found construction.api_server.py (1)
49-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo visibility into a silently-dead watch loop.
watch_taskis created but nothing observes it again until shutdown.watch_runner.watch_loop()only re-raisesCancelledError; any other exception surfacing fromrun_due_watches()mid-loop would terminate the task permanently with nothing logging it — scheduled watches would just stop firing with no operator-visible signal until someone notices watches aren't running.Consider attaching a done-callback that logs unhandled exceptions from
watch_task, for observability:👀 Suggested addition
watch_task = None if config.WATCH_ENABLE: import watch_runner watch_task = asyncio.create_task(watch_runner.watch_loop()) + + def _log_watch_task_result(task: asyncio.Task) -> None: + if not task.cancelled() and task.exception(): + logger.error("[Watch] schedule loop died unexpectedly", exc_info=task.exception()) + + watch_task.add_done_callback(_log_watch_task_result)🤖 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 `@api_server.py` around lines 49 - 64, Attach a completion callback to the task created in the WATCH_ENABLE branch of the application lifespan flow, using the visible watch_task and watch_runner.watch_loop symbols. Have the callback inspect the completed task and log any exception other than cancellation, while preserving the existing shutdown cancellation and awaiting behavior.tests/test_finalizer.py (1)
22-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin config values in abstention tests for isolation.
Tests
test_abstains_when_grounded_but_incomplete,test_no_abstain_when_complete, andtest_low_faithfulness_does_not_abstainrely onconfig.ANSWER_CONFIDENCE_ENABLEbeingTrueandconfig.ABSTAIN_COMPLETENESS_FLOORbeing0.5(both defaults). If the environment overrides either, these tests will fail unexpectedly. Consider accepting amonkeypatchfixture and explicitly setting these values.♻️ Optional refactor
-def test_abstains_when_grounded_but_incomplete(): +def test_abstains_when_grounded_but_incomplete(monkeypatch): + monkeypatch.setattr(config, "ANSWER_CONFIDENCE_ENABLE", True) + monkeypatch.setattr(config, "ABSTAIN_COMPLETENESS_FLOOR", 0.5) state = { "final_answer": "The supported fact is stated here with a citation [1].", "reflexion_history": [{ "faithfulness_score": 0.9, "completeness_score": 0.2, "missing_aspects": ["efficiency numbers"], }], } out = finalizer_node(state) assert out["abstained"] is True assert "Insufficient evidence" in out["final_answer"] assert "efficiency numbers" in out["final_answer"] assert out["answer_confidence"] is not None🤖 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 `@tests/test_finalizer.py` around lines 22 - 63, Update test_abstains_when_grounded_but_incomplete, test_no_abstain_when_complete, and test_low_faithfulness_does_not_abstain to accept the monkeypatch fixture and explicitly set config.ANSWER_CONFIDENCE_ENABLE to True and config.ABSTAIN_COMPLETENESS_FLOOR to 0.5 before calling finalizer_node, isolating their expected behavior from environment overrides.providers/gemini.py (2)
46-48: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
next_client_idx()assumes the pool is already initialized.It reads
self._indexwithout callingself._ensure_pool()first. Currently safe because both call sites (self.pool[self.next_client_idx()]) access.poolfirst, which triggers lazy init — but if this method is ever called standalone (e.g. by future failover code iterating over keys), it will hit an emptyitertools.cycle([])and raiseStopIterationimmediately.🛡️ Proposed defensive fix
def next_client_idx(self) -> int: + self._ensure_pool() with self._lock: return next(self._index)🤖 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 `@providers/gemini.py` around lines 46 - 48, Update next_client_idx() to call _ensure_pool() before advancing self._index, while preserving the existing lock protection and return behavior. Ensure standalone callers initialize the client pool before the cycle is consumed.
72-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent exception swallowing in
_with_cache.Any failure in
gemini_cache.get_or_create()(bad key, network error, malformedsystem_instruction) is swallowed with no log at all, unlike the analogous case ingenerate_streamwhich logs at debug level. This makes cache failures invisible in production.♻️ Proposed fix
- except Exception: + except Exception as exc: + logger.debug("Gemini context caching skipped: %s", exc) return gen_config🤖 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 `@providers/gemini.py` around lines 72 - 86, The broad exception handler in _with_cache currently hides cache failures; log the caught exception at debug level with appropriate context, matching the existing generate_stream behavior, while still returning the original gen_config and preserving the current fallback behavior.docs/Eval/eval_report.json (1)
1-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCommitting a timestamped generated report to the repo will churn on every eval run.
This file is a machine-generated artifact (note the
timestampfield) checked intodocs/Eval/. If it's meant as a live, regenerated report rather than a fixed documentation sample, consider excluding it from version control (or clearly marking it as a static example) to avoid noisy diffs on every evaluation run.🤖 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 `@docs/Eval/eval_report.json` around lines 1 - 134, Remove the timestamped generated eval report from version control and add its artifact name or pattern to the repository’s ignore configuration. If the report is intended as documentation, replace it with a clearly static example rather than regenerating and committing the live report.tests/test_providers_gemini.py (1)
1-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
pool/next_client_idx()round-robin behavior.Classifier and gating logic is well covered, but the round-robin client selection and lazy pool-init (thread-safety-sensitive per coding guidelines) have no test here. A simple test with a mocked
config.LLM_API_KEY_POOL(e.g., 3 fake keys) assertingnext_client_idx()cycles0,1,2,0,1,2...would guard this critical path.As per coding guidelines, "Thread safety matters for shared singletons such as embeddings, vector store, BM25 index, and cache; use locks where needed" — the client pool is exactly such a singleton and currently has no direct test coverage.
🤖 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 `@tests/test_providers_gemini.py` around lines 1 - 25, Add coverage in tests/test_providers_gemini.py for GeminiBackend.next_client_idx() using a mocked config.LLM_API_KEY_POOL with three fake keys. Verify repeated calls return the round-robin sequence 0, 1, 2, 0, 1, 2 and exercise lazy pool initialization; preserve the existing classifier and thinking-gating tests.Source: Coding guidelines
routes/report.py (1)
69-104: 🚀 Performance & Scalability | 🔵 TrivialConsider isolating report jobs from the shared threadpool.
_run_report_jobis dispatched viaBackgroundTasks.add_taskfor a sync function, which Starlette runs on its default threadpool — the same pool backingrun_in_threadpoolcalls elsewhere (e.g./agent). A report can issue several sequential LLM calls (planning + oneanswer_questionper section), so concurrent report jobs could tie up worker threads long enough to delay unrelated request handling. Worth watching threadpool saturation under load, or moving report execution to a dedicated worker/queue if reports become frequent.🤖 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 `@routes/report.py` around lines 69 - 104, Isolate the synchronous report execution in _run_report_job from Starlette’s shared BackgroundTasks threadpool by dispatching reports through a dedicated worker or queue. Preserve the existing job status updates, progress callback, success handling, and failure handling while ensuring concurrent report jobs cannot consume the threadpool used by unrelated run_in_threadpool requests.llm_client.py (1)
21-22: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_circuit_breakerdict mutated across threads without synchronization.Reads/writes to the module-level
_circuit_breakerdict happen from concurrent request threads with no lock. CPython dict ops are individually atomic so this won't corrupt the dict, but interleavings can cause a thread to skip a path another thread just reopened, or vice versa — a soft correctness issue for the breaker's timing guarantees rather than a crash risk.🔒 Optional: guard circuit-breaker reads/writes with a lock
+_circuit_lock = threading.Lock() + def _circuit_key(provider: str, model: str) -> tuple[str, str]: return (provider, model)Then wrap the get/pop/set call sites in
with _circuit_lock:.Also applies to: 101-118, 158-183
🤖 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 `@llm_client.py` around lines 21 - 22, Add a module-level lock dedicated to _circuit_breaker, then wrap every circuit-breaker get, pop, and set operation in the relevant request and retry paths with that lock. Update the call sites around the circuit-breaker logic while preserving the existing cooldown and open/reopen behavior.docs/Eval/evaluate.py (1)
128-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCrossEncoder scored one claim at a time; batching would speed up evaluation.
semantic()callsmodel.predict(...)on a single pair per invocation, andcitation_groundingcallssim_fnonce per claim in a loop.CrossEncoder.predictaccepts a batched list of pairs and abatch_sizefor throughput — batching all claims for a query (or the whole run) before scoring would meaningfully cut eval runtime when--grounding-judge cross-encoderis used on larger claim sets.Also applies to: 135-166
🤖 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 `@docs/Eval/evaluate.py` around lines 128 - 132, Update the cross-encoder scoring flow around semantic and citation_grounding to collect claim/chunk pairs and call model.predict once with the full batch, supplying an appropriate batch_size. Convert each returned logit to its sigmoid score while preserving the existing per-claim scores and sim_fn behavior, rather than invoking predict separately for every claim.
🤖 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 `@docs/Eval/evaluate.py`:
- Around line 112-133: Update make_grounding_scorer so failures while
constructing CrossEncoder, including network, HTTP, and disk-related model-load
exceptions, fall back to jaccard and GROUNDING_THRESHOLD just like a missing
sentence_transformers import. Keep the existing ImportError fallback and
semantic scoring path unchanged when model initialization succeeds.
In `@docs/superpowers/plans/2026-07-12-openrouter-provider.md`:
- Around line 833-897: Update generate_with_failover and its backend invocation
path so each provider/model stage retries every available Gemini pool key before
advancing to the next model or provider. Preserve permanent-error propagation,
pass the selected client for each key, and only set the per-(provider, model)
60-second circuit breaker after all keys for that stage fail; keep non-Gemini
attempts single-client and retain existing success and fallback behavior.
In `@ingest.py`:
- Around line 300-306: Wrap the figure_captioner.extract_regions calls in
ingest_pdf and _extract_worker with localized exception handling so extraction
failures do not propagate to the outer per-paper handler or mark successful text
ingestion as failed. Preserve the existing figures=None/default behavior and
continue processing the extracted text sections when figure extraction fails.
- Around line 140-159: Isolate the optional captioning step in the multimodal
branch of _build_paper_chunks so failures from figure_captioner.caption_regions
do not discard already-built text chunks. Catch captioning/network exceptions
and skip invalid results missing text, chunk_type, page, or crop_path, while
preserving normal metadata and ID appending for valid figures; continue
returning the core chunks when captioning fails.
In `@llm_client.py`:
- Around line 39-49: Make lazy initialization in _init_backends thread-safe
using a module-level lock and double-checked locking: recheck _backends while
holding the lock before constructing GeminiBackend and OpenRouterBackend. Keep
get_backend’s existing lookup and unknown-provider behavior unchanged.
- Around line 63-67: Update _fallback_model_for() so cross-provider fallback
never selects a model incompatible with the requested provider: choose a
provider-matched model from LLM_SELECTABLE_MODELS, and return no fallback when
none exists rather than defaulting to the Gemini model. Adjust its return type
and callers as needed to handle the skipped fallback safely.
In `@onnx_ce.py`:
- Around line 21-23: Update the _QFILE definition to reference the qint8 export
filename produced by export_dynamic_quantized_onnx_model for the _QUANT preset,
using onnx/model_qint8_avx2.onnx or deriving the path from the export output so
the generated quantized model is found before the fp32 fallback.
In `@providers/openrouter.py`:
- Around line 88-105: Update _params to read
gen_config.tool_config.function_calling_config and translate
FunctionCallingConfig(mode="ANY") into the OpenAI-compatible
params["tool_choice"] = "required" when tools are present. Preserve the existing
tool declaration handling and leave tool_choice unset for other modes or when no
tools are declared.
In `@routes/models.py`:
- Around line 22-29: Change the get_models endpoint from async def to a
synchronous def so FastAPI executes its blocking list_models →
model_supports_tools → _catalog → _fetch_openrouter_catalog path in the
threadpool. Leave the existing synchronous httpx.get and catalog behavior
unchanged, and verify OPENROUTER_BASE_URL remains sourced from static
configuration rather than request input.
In `@routes/query.py`:
- Around line 62-66: Update the figure thumbnail rendering in the frontend to
pass f.url through the existing safeHref() helper for both the clickable link
target and image src, matching the protection already used for source and
pdf_url URLs.
In `@routes/watch.py`:
- Around line 100-146: Add per-user ownership enforcement across the watch
endpoints: scope list_watches to the authenticated user and verify ownership
before get_watch, run_watch_now, get_watch_digest, and delete_watch perform or
expose watch data. Reuse the existing authenticated identity and persistence
watch-owner fields/helpers, returning the established not-found or authorization
response for watches owned by another user.
In `@static/index.html`:
- Around line 862-869: Update citeFigs() so both the thumbnail anchor href and
image src pass f.url through safeHref() before HTML escaping or interpolation,
preserving the existing figure rendering and metadata behavior. Ensure only
http/https URLs are emitted for both attributes, including the downstream
callers that rely on citeFigs().
In `@watch_runner.py`:
- Around line 131-143: Update watch_loop so exceptions from each run_due_watches
sweep are caught and logged, allowing the while loop to continue polling after
transient failures. Preserve asyncio.CancelledError handling so cancellation
still logs shutdown and propagates normally.
- Around line 74-99: Update the ingest_pdf handling in run_watch so an exception
for one paper is caught and does not abort the candidate loop or prevent
persistence.save_watch(w) from running. Preserve temporary-file cleanup, skip
adding the failed paper to ingested, and continue processing subsequent passages
while retaining the new_ids progress for the failed arxiv_id.
- Around line 27-37: Update _download_pdf so failures after tempfile.mkstemp
close the returned file descriptor and remove the created temporary path,
including exceptions raised by urllib.request.urlopen before os.fdopen is
entered. Preserve returning the path only after a successful download, and
retain the existing warning and None-on-failure behavior.
---
Outside diff comments:
In `@routes/agent.py`:
- Around line 96-113: Update the AgentState constructor in the initial state
setup to include initial values for both required keys, answer_confidence and
abstained, using the appropriate unset confidence value and false for abstained.
Keep the existing response defaults and other state initialization unchanged.
---
Minor comments:
In `@agent/nodes/answer_generator.py`:
- Around line 37-48: The contradiction check currently analyzes all retrieved
chunks instead of only the context retained by rag.format_context(). In the
CONTRADICTION_DETECT_ENABLE block, update the find_contradictions() call to pass
chunks and metas truncated to chunks_used, preserving aligned chunk/metadata
pairs and the existing best-effort error handling.
In `@config.py`:
- Around line 484-487: Update the VERSION constant in config.py from the
development value to the release value "2.3.0", preserving the existing version
declaration format.
In `@contradiction.py`:
- Around line 61-79: Update the title handling in the comparison loop around
_title so missing metadata does not cause distinct untitled chunks to be
classified as the same paper. Preserve the same-paper skip for genuinely
matching known titles, while allowing pairs whose titles both resolve to the
fallback value to proceed through contradiction scoring.
In `@docs/Eval/evaluate.py`:
- Around line 184-186: Update evaluate() to record the selected grounding judge
and threshold in its returned metrics dictionary, preserving the actual sim_fn
selection including cross-encoder mode. Update markdown_report to use
metrics.get("grounding_judge", "jaccard") and the recorded threshold when
generating the methodology text, so reports accurately describe the configured
grounding method while retaining the default wording for older metrics.
In `@docs/superpowers/specs/2026-07-12-openrouter-provider-design.md`:
- Around line 146-152: Revise the “Per-model eval hook” section to accurately
state that evaluate.py only applies --model/--provider as labels to
pre-generated results for scoring. Remove the claim that the flags are threaded
into queries or drive live answer generation, while preserving the purpose of
distinguishing model/provider results in reports.
- Around line 93-101: Update the fenced code block in the OpenRouter provider
design configuration section to include the dotenv language tag, changing the
opening fence to identify the block as dotenv while leaving its configuration
contents unchanged.
In `@figure_captioner.py`:
- Around line 181-195: Update the crop-writing flow before the chunks.append
call so crop_path is recorded only when crop_path.write_bytes succeeds; on
failure, use the existing missing-value convention for optional crop paths while
preserving the warning and chunk creation.
In `@providers/openrouter.py`:
- Around line 120-130: Update generate_stream to skip any streaming chunk whose
choices collection is empty before accessing chunk.choices[0]. Continue
processing populated chunks and preserve the existing no-text RuntimeError when
no content is emitted.
- Around line 25-29: Update the non-string contents handling in the OpenRouter
message translation flow to extract and concatenate the actual text from Gemini
Content/Part objects instead of using str(contents). Preserve direct string
handling, and ensure lists containing structured parts produce the same prompt
text passed by answer_generator and figure_captioner.
In `@rag.py`:
- Around line 38-46: Apply safeHref() to f.url in the cited-figure rendering
path before using it for either the href or src attribute. Reuse the sanitized
result for both attributes, preserving the existing figure URL generation and
HTML escaping behavior.
In `@report_runner.py`:
- Around line 27-46: Update plan_sections() to include its language argument in
the LLM planning prompt so generated section titles use the requested language.
In run_report(), pass the intended language through to plan_sections() and set
report metadata to the language actually used by query-driven synthesis, rather
than blindly returning the caller-supplied value.
In `@routes/chat.py`:
- Around line 185-205: Update the first_user extraction in list_sessions to
safely read each user message’s content instead of indexing m["content"], while
preserving the existing empty-string fallback when no usable user content
exists.
In `@routes/query.py`:
- Around line 44-45: Thread the QueryRequest model and provider values through
both /query and /query/stream handlers into rag.answer_question and
rag.prepare_query_for_stream, respectively, preserving omitted values so the RAG
layer can apply its defaults; alternatively remove these fields from
QueryRequest only if query endpoints must always use the default model.
In `@static/index.html`:
- Around line 1112-1116: Update the standard `/chat/stream` request payload to
include the selected model from `modelSelect`, matching the existing `model`
field behavior in the agent request while preserving the current null fallback.
In `@watch_runner.py`:
- Around line 27-37: Update _download_pdf to reject URLs whose scheme is not
http or https before opening the request, and enforce a maximum response size
while reading instead of using an unbounded resp.read(). Preserve the existing
temporary-file cleanup/error behavior and return None for invalid schemes or
oversized responses.
---
Nitpick comments:
In `@api_server.py`:
- Around line 49-64: Attach a completion callback to the task created in the
WATCH_ENABLE branch of the application lifespan flow, using the visible
watch_task and watch_runner.watch_loop symbols. Have the callback inspect the
completed task and log any exception other than cancellation, while preserving
the existing shutdown cancellation and awaiting behavior.
In `@contradiction.py`:
- Around line 65-76: Batch contradiction scoring in the loop that builds found
instead of calling _contradiction_probs separately for each pair. First collect
both directional text pairs for every valid combination, make one model
prediction through _contradiction_probs, then map the returned scores back to
their corresponding combinations while preserving threshold filtering and found
construction.
In `@docs/Eval/eval_report.json`:
- Around line 1-134: Remove the timestamped generated eval report from version
control and add its artifact name or pattern to the repository’s ignore
configuration. If the report is intended as documentation, replace it with a
clearly static example rather than regenerating and committing the live report.
In `@docs/Eval/evaluate.py`:
- Around line 128-132: Update the cross-encoder scoring flow around semantic and
citation_grounding to collect claim/chunk pairs and call model.predict once with
the full batch, supplying an appropriate batch_size. Convert each returned logit
to its sigmoid score while preserving the existing per-claim scores and sim_fn
behavior, rather than invoking predict separately for every claim.
In `@llm_client.py`:
- Around line 21-22: Add a module-level lock dedicated to _circuit_breaker, then
wrap every circuit-breaker get, pop, and set operation in the relevant request
and retry paths with that lock. Update the call sites around the circuit-breaker
logic while preserving the existing cooldown and open/reopen behavior.
In `@providers/gemini.py`:
- Around line 46-48: Update next_client_idx() to call _ensure_pool() before
advancing self._index, while preserving the existing lock protection and return
behavior. Ensure standalone callers initialize the client pool before the cycle
is consumed.
- Around line 72-86: The broad exception handler in _with_cache currently hides
cache failures; log the caught exception at debug level with appropriate
context, matching the existing generate_stream behavior, while still returning
the original gen_config and preserving the current fallback behavior.
In `@rerank.py`:
- Around line 19-30: Factor the duplicated ONNX-loading and fp32 fallback flow
from rerank.py and verify.py into a shared helper in onnx_ce.py, such as
load_or_fallback(model_name, subdir, cache_folder). Move device selection and
associated success/fallback logging into that helper, then update both _load()
implementations to invoke it while preserving their current model names, cache
directories, and return behavior.
In `@routes/report.py`:
- Around line 69-104: Isolate the synchronous report execution in
_run_report_job from Starlette’s shared BackgroundTasks threadpool by
dispatching reports through a dedicated worker or queue. Preserve the existing
job status updates, progress callback, success handling, and failure handling
while ensuring concurrent report jobs cannot consume the threadpool used by
unrelated run_in_threadpool requests.
In `@tests/test_finalizer.py`:
- Around line 22-63: Update test_abstains_when_grounded_but_incomplete,
test_no_abstain_when_complete, and test_low_faithfulness_does_not_abstain to
accept the monkeypatch fixture and explicitly set
config.ANSWER_CONFIDENCE_ENABLE to True and config.ABSTAIN_COMPLETENESS_FLOOR to
0.5 before calling finalizer_node, isolating their expected behavior from
environment overrides.
In `@tests/test_providers_gemini.py`:
- Around line 1-25: Add coverage in tests/test_providers_gemini.py for
GeminiBackend.next_client_idx() using a mocked config.LLM_API_KEY_POOL with
three fake keys. Verify repeated calls return the round-robin sequence 0, 1, 2,
0, 1, 2 and exercise lazy pool initialization; preserve the existing classifier
and thinking-gating tests.
🪄 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: 018f94a4-57c2-4dae-ac13-63bd5c8f4ed9
📒 Files selected for processing (55)
.env.exampleagent/json_utils.pyagent/nodes/answer_generator.pyagent/nodes/finalizer.pyagent/nodes/query_planner.pyagent/nodes/reflexion_evaluator.pyagent/nodes/tool_executor_node.pyagent/nodes/tool_selector.pyagent/state.pyapi_server.pyconfig.pycontradiction.pydocs/Eval/answers_and_citations.jsondocs/Eval/eval_report.jsondocs/Eval/eval_report.mddocs/Eval/evaluate.pydocs/Eval/test_evaluate.pydocs/superpowers/plans/2026-07-12-openrouter-provider.mddocs/superpowers/specs/2026-07-12-openrouter-provider-design.mdfigure_captioner.pyingest.pyllm_client.pyonnx_ce.pypersistence.pyproviders/__init__.pyproviders/base.pyproviders/gemini.pyproviders/openrouter.pyrag.pyreport_runner.pyrequirements.txtrerank.pyroutes/agent.pyroutes/chat.pyroutes/models.pyroutes/query.pyroutes/report.pyroutes/watch.pystatic/index.htmltests/test_capability_gating.pytests/test_finalizer.pytests/test_llm_client_dispatch.pytests/test_model_selection_plumbing.pytests/test_models_route.pytests/test_openrouter_config.pytests/test_providers_base.pytests/test_providers_gemini.pytests/test_providers_openrouter.pytests/test_report_routes.pytests/test_structured_output_fallback.pytests/test_watch_persistence.pytests/test_watch_routes.pytests/test_watch_run.pyverify.pywatch_runner.py
💤 Files with no reviewable changes (1)
- docs/Eval/answers_and_citations.json
| def _circuit_key(provider: str, model: str) -> tuple[str, str]: | ||
| return (provider, model) | ||
|
|
||
|
|
||
| def _fallback_model_for(provider: str) -> str: | ||
| """The provider's default model for cross-provider fallback.""" | ||
| if provider == "gemini": | ||
| return _config.LLM_MODEL_NAME | ||
| return _config.LLM_SELECTABLE_MODELS[0] if _config.LLM_SELECTABLE_MODELS else _config.LLM_MODEL_NAME | ||
|
|
||
|
|
||
| def model_supports_tools(provider: str, model: str) -> bool: | ||
| """Gemini always supports tools; OpenRouter is checked against the catalog. | ||
| Default True so a catalog outage doesn't over-block.""" | ||
| if provider == "gemini": | ||
| return True | ||
| try: | ||
| import routes.models as models_route | ||
| return models_route.model_supports_tools(model) | ||
| except Exception: | ||
| return True | ||
|
|
||
|
|
||
| def _attempts(model: str, provider: str) -> list[tuple[str, str]]: | ||
| """Ordered (provider, model) attempts: requested → same-provider fallback → | ||
| cross-provider fallback.""" | ||
| attempts = [(provider, model)] | ||
| if provider == "gemini" and _config.LLM_FALLBACK_MODEL and _config.LLM_FALLBACK_MODEL != model: | ||
| attempts.append((provider, _config.LLM_FALLBACK_MODEL)) | ||
| fb_provider = _config.LLM_FALLBACK_PROVIDER | ||
| if fb_provider and fb_provider != provider: | ||
| attempts.append((fb_provider, _fallback_model_for(fb_provider))) | ||
| return attempts | ||
|
|
||
|
|
||
| def generate_with_failover(model: str, contents, gen_config, provider: str | None = None): | ||
| """Try requested (provider, model), then same-provider then cross-provider | ||
| fallback. Per-(provider,model) circuit breaker skips recently-dead paths.""" | ||
| provider = resolve_provider(model, provider) | ||
| last_exc: Exception | None = None | ||
| any_attempted = False | ||
|
|
||
| for prov, mdl in _attempts(model, provider): | ||
| key = _circuit_key(prov, mdl) | ||
| if time.monotonic() < _circuit_breaker.get(key, 0): | ||
| logger.info(f"[failover] {prov}:{mdl} circuit open, skipping") | ||
| continue | ||
| backend = get_backend(prov) | ||
| any_attempted = True | ||
| try: | ||
| result = backend.generate(mdl, contents, gen_config) | ||
| _circuit_breaker.pop(key, None) | ||
| return result | ||
| except Exception as exc: | ||
| last_exc = exc | ||
| if backend.is_permanent(exc): | ||
| raise | ||
| logger.warning(f"[failover] {prov}:{mdl} failed ({exc!s:.120}) — next path") | ||
| _circuit_breaker[key] = time.monotonic() + _CIRCUIT_COOLDOWN | ||
| continue | ||
|
|
||
| if not any_attempted: | ||
| raise RuntimeError("All configured LLM paths are circuit-open; retry after cooldown.") | ||
| raise last_exc # type: ignore[misc] | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Dispatcher never retries the key pool before falling back to a different model/provider.
_attempts() produces one attempt per (provider, model) stage, and backend.generate()/backend.generate_stream() each pick a single pool client via next_client_idx(). A single transient failure on one key therefore trips the circuit and jumps straight to LLM_FALLBACK_MODEL (or cross-provider) instead of rotating through the rest of config.LLM_API_KEY_POOL on the primary model first, as required.
As per coding guidelines, "generate_with_failover() must try all keys on the primary model (gemini-3.5-flash), then fall back to LLM_FALLBACK_MODEL (gemma-4-26b-a4b-it), and use a 60-second circuit breaker after all keys fail."
🛠️ Sketch of the missing key-level retry loop
for prov, mdl in _attempts(model, provider):
key = _circuit_key(prov, mdl)
if time.monotonic() < _circuit_breaker.get(key, 0):
continue
backend = get_backend(prov)
any_attempted = True
num_keys = len(backend.pool) if prov == "gemini" else 1
for i in range(num_keys):
try:
client = backend.pool[i] if prov == "gemini" else None
result = backend.generate(mdl, contents, gen_config, client=client) if client else backend.generate(mdl, contents, gen_config)
_circuit_breaker.pop(key, None)
return result
except Exception as exc:
last_exc = exc
if backend.is_permanent(exc):
raise
continue # try next key on the SAME model
_circuit_breaker[key] = time.monotonic() + _CIRCUIT_COOLDOWNAlso applies to: 921-961
🤖 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 `@docs/superpowers/plans/2026-07-12-openrouter-provider.md` around lines 833 -
897, Update generate_with_failover and its backend invocation path so each
provider/model stage retries every available Gemini pool key before advancing to
the next model or provider. Preserve permanent-error propagation, pass the
selected client for each key, and only set the per-(provider, model) 60-second
circuit breaker after all keys for that stage fail; keep non-Gemini attempts
single-client and retain existing success and fallback behavior.
Source: Coding guidelines
| @router.get("/watch", response_model=list[WatchResponse], tags=["Watch"]) | ||
| async def list_watches(user_id: Optional[str] = None, authenticated: bool = Depends(verify_api_key)): | ||
| """List all watches, or just one user's when user_id is supplied.""" | ||
| _require_enabled() | ||
| return [_to_response(w) for w in persistence.list_watches(user_id)] | ||
|
|
||
|
|
||
| @router.get("/watch/{watch_id}", response_model=WatchResponse, tags=["Watch"]) | ||
| async def get_watch(watch_id: str, authenticated: bool = Depends(verify_api_key)): | ||
| _require_enabled() | ||
| w = persistence.get_watch(watch_id) | ||
| if w is None: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Watch not found") | ||
| return _to_response(w) | ||
|
|
||
|
|
||
| @router.post("/watch/{watch_id}/run", tags=["Watch"]) | ||
| async def run_watch_now(watch_id: str, authenticated: bool = Depends(verify_api_key)): | ||
| """Run a watch immediately: search the topic, ingest new papers, refresh the digest. | ||
|
|
||
| Synchronous — ingest blocks, so it runs in a threadpool to keep the event loop free. | ||
| """ | ||
| _require_enabled() | ||
| import watch_runner # lazy: keeps arxiv/ingest imports off the request-path cold start | ||
| try: | ||
| return await run_in_threadpool(watch_runner.run_watch, watch_id) | ||
| except KeyError: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Watch not found") | ||
|
|
||
|
|
||
| @router.get("/watch/{watch_id}/digest", tags=["Watch"]) | ||
| async def get_watch_digest(watch_id: str, authenticated: bool = Depends(verify_api_key)): | ||
| """Return the watch's latest stored digest (persists between runs).""" | ||
| _require_enabled() | ||
| w = persistence.get_watch(watch_id) | ||
| if w is None: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Watch not found") | ||
| return {"watch_id": watch_id, "digest": w.get("latest_digest"), "last_run": w.get("last_run")} | ||
|
|
||
|
|
||
| @router.delete("/watch/{watch_id}", tags=["Watch"]) | ||
| async def delete_watch(watch_id: str, authenticated: bool = Depends(verify_api_key)): | ||
| _require_enabled() | ||
| if persistence.get_watch(watch_id) is None: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Watch not found") | ||
| persistence.delete_watch(watch_id) | ||
| return {"status": "deleted", "id": watch_id} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "def verify_api_key" -A 20 deps.pyRepository: DNSdecoded/IndicRAG
Length of output: 947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## routes/watch.py\n'
sed -n '1,220p' routes/watch.py
printf '\n## watch persistence references\n'
rg -n "def (list_watches|get_watch|delete_watch|run_watch|run_watch_now)|list_watches\(|get_watch\(|delete_watch\(" -A 8 -B 4 .Repository: DNSdecoded/IndicRAG
Length of output: 19597
No per-user authorization on watch endpoints routes/watch.py:100-146 lets any caller with a valid API key list every watch, read any digest, run any watch, or delete it by watch_id. If watches are meant to be user-scoped, add an ownership check before exposing these routes.
🤖 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 `@routes/watch.py` around lines 100 - 146, Add per-user ownership enforcement
across the watch endpoints: scope list_watches to the authenticated user and
verify ownership before get_watch, run_watch_now, get_watch_digest, and
delete_watch perform or expose watch data. Reuse the existing authenticated
identity and persistence watch-owner fields/helpers, returning the established
not-found or authorization response for watches owned by another user.
| // Phase 3: render a cited paper's figure/table crops as clickable thumbnails. | ||
| function citeFigs(c) { | ||
| return (c.figures || []).map(f => | ||
| `<a class="cite-fig-link" href="${esc(f.url)}" target="_blank" rel="noopener noreferrer"` | ||
| + ` title="${esc(f.chunk_type)}${f.page ? ' · p'+esc(f.page) : ''}">` | ||
| + `<img class="cite-fig" src="${esc(f.url)}" alt="${esc(f.chunk_type)}" loading="lazy"></a>` | ||
| ).join(''); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
citeFigs() uses esc() instead of safeHref() for href/src.
esc() only HTML-entity-escapes characters — it doesn't validate URL scheme, so a javascript: URI in f.url would pass through unescaped-scheme into a clickable <a href>, executing on click. Downstream uses at lines 978 and 1167 inherit this via the shared helper.
As per coding guidelines, "All URLs rendered in the frontend must go through safeHref() and allow only http/https schemes to prevent XSS."
🔒 Proposed fix
function citeFigs(c) {
return (c.figures || []).map(f =>
- `<a class="cite-fig-link" href="${esc(f.url)}" target="_blank" rel="noopener noreferrer"`
+ `<a class="cite-fig-link" href="${safeHref(f.url)}" target="_blank" rel="noopener noreferrer"`
+ ` title="${esc(f.chunk_type)}${f.page ? ' · p'+esc(f.page) : ''}">`
- + `<img class="cite-fig" src="${esc(f.url)}" alt="${esc(f.chunk_type)}" loading="lazy"></a>`
+ + `<img class="cite-fig" src="${safeHref(f.url)}" alt="${esc(f.chunk_type)}" loading="lazy"></a>`
).join('');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Phase 3: render a cited paper's figure/table crops as clickable thumbnails. | |
| function citeFigs(c) { | |
| return (c.figures || []).map(f => | |
| `<a class="cite-fig-link" href="${esc(f.url)}" target="_blank" rel="noopener noreferrer"` | |
| + ` title="${esc(f.chunk_type)}${f.page ? ' · p'+esc(f.page) : ''}">` | |
| + `<img class="cite-fig" src="${esc(f.url)}" alt="${esc(f.chunk_type)}" loading="lazy"></a>` | |
| ).join(''); | |
| } | |
| // Phase 3: render a cited paper's figure/table crops as clickable thumbnails. | |
| function citeFigs(c) { | |
| return (c.figures || []).map(f => | |
| `<a class="cite-fig-link" href="${safeHref(f.url)}" target="_blank" rel="noopener noreferrer"` | |
| ` title="${esc(f.chunk_type)}${f.page ? ' · p'+esc(f.page) : ''}">` | |
| `<img class="cite-fig" src="${safeHref(f.url)}" alt="${esc(f.chunk_type)}" loading="lazy"></a>` | |
| ).join(''); | |
| } |
🤖 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 `@static/index.html` around lines 862 - 869, Update citeFigs() so both the
thumbnail anchor href and image src pass f.url through safeHref() before HTML
escaping or interpolation, preserving the existing figure rendering and metadata
behavior. Ensure only http/https URLs are emitted for both attributes, including
the downstream callers that rely on citeFigs().
Source: Coding guidelines
| def _download_pdf(url: str) -> str | None: | ||
| """Fetch a PDF to a temp file; return its path, or None on failure.""" | ||
| try: | ||
| req = urllib.request.Request(url, headers={"User-Agent": "IndicRAG/2.0"}) | ||
| fd, path = tempfile.mkstemp(suffix=".pdf") | ||
| with urllib.request.urlopen(req, timeout=30) as resp, os.fdopen(fd, "wb") as f: | ||
| f.write(resp.read()) | ||
| return path | ||
| except Exception as e: | ||
| logger.warning(f"[Watch] PDF download failed {url}: {e}") | ||
| return None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix fd/temp-file leak in _download_pdf on failed downloads.
mkstemp creates path before the with block. Since urlopen(req, ...) is entered before os.fdopen(fd, ...), any exception from urlopen (timeout, DNS failure, HTTP error) leaves fd open and orphans the temp file — path is never returned, so nothing ever cleans it up. Given watch_loop runs forever, every failed download leaks an fd and a stray file.
🔧 Proposed fix
def _download_pdf(url: str) -> str | None:
"""Fetch a PDF to a temp file; return its path, or None on failure."""
+ fd, path = tempfile.mkstemp(suffix=".pdf")
try:
req = urllib.request.Request(url, headers={"User-Agent": "IndicRAG/2.0"})
- fd, path = tempfile.mkstemp(suffix=".pdf")
- with urllib.request.urlopen(req, timeout=30) as resp, os.fdopen(fd, "wb") as f:
- f.write(resp.read())
+ with os.fdopen(fd, "wb") as f, urllib.request.urlopen(req, timeout=30) as resp:
+ f.write(resp.read())
return path
except Exception as e:
logger.warning(f"[Watch] PDF download failed {url}: {e}")
+ try:
+ os.remove(path)
+ except OSError:
+ pass
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _download_pdf(url: str) -> str | None: | |
| """Fetch a PDF to a temp file; return its path, or None on failure.""" | |
| try: | |
| req = urllib.request.Request(url, headers={"User-Agent": "IndicRAG/2.0"}) | |
| fd, path = tempfile.mkstemp(suffix=".pdf") | |
| with urllib.request.urlopen(req, timeout=30) as resp, os.fdopen(fd, "wb") as f: | |
| f.write(resp.read()) | |
| return path | |
| except Exception as e: | |
| logger.warning(f"[Watch] PDF download failed {url}: {e}") | |
| return None | |
| def _download_pdf(url: str) -> str | None: | |
| """Fetch a PDF to a temp file; return its path, or None on failure.""" | |
| fd, path = tempfile.mkstemp(suffix=".pdf") | |
| try: | |
| req = urllib.request.Request(url, headers={"User-Agent": "IndicRAG/2.0"}) | |
| with os.fdopen(fd, "wb") as f, urllib.request.urlopen(req, timeout=30) as resp: | |
| f.write(resp.read()) | |
| return path | |
| except Exception as e: | |
| logger.warning(f"[Watch] PDF download failed {url}: {e}") | |
| try: | |
| os.remove(path) | |
| except OSError: | |
| pass | |
| return None |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 31-31: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(req, timeout=30)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🤖 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 `@watch_runner.py` around lines 27 - 37, Update _download_pdf so failures after
tempfile.mkstemp close the returned file descriptor and remove the created
temporary path, including exceptions raised by urllib.request.urlopen before
os.fdopen is entered. Preserve returning the path only after a successful
download, and retain the existing warning and None-on-failure behavior.
| for p in passages: | ||
| arxiv_id = p.get("arxiv_id") | ||
| if not arxiv_id or arxiv_id in seen: | ||
| continue | ||
| new_ids.append(arxiv_id) # mark seen even if it dupes the corpus, so we stop re-fetching it | ||
|
|
||
| pdf_url = p.get("pdf_url") | ||
| if pdf_url: | ||
| path = _download_pdf(pdf_url) | ||
| if path: | ||
| try: | ||
| n_chunks, title = ingest_pdf( | ||
| path, paper_id=arxiv_id, | ||
| metadata={"title": p.get("title", ""), "source": p.get("source", "")}, | ||
| ) | ||
| finally: | ||
| try: | ||
| os.remove(path) | ||
| except OSError: | ||
| pass | ||
| if n_chunks > 0: # 0 = duplicate/unchanged in corpus → seen, but not "new" | ||
| ingested.append({"arxiv_id": arxiv_id, "title": title or p.get("title", ""), "text": p.get("text", "")}) | ||
| continue | ||
| # ponytail: no PDF (or download failed) → abstract feeds the digest only; | ||
| # indexing an abstract-only chunk is deferred to a later increment. | ||
| ingested.append({"arxiv_id": arxiv_id, "title": p.get("title", ""), "text": p.get("text", "")}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled ingest_pdf exception aborts the whole run and can permanently stall a watch.
arxiv_id is added to new_ids before ingest_pdf runs, but the call is only wrapped in finally (temp-file cleanup), not except. If ingest_pdf raises for one paper, the exception propagates out of run_watch before persistence.save_watch(w) (line 109) runs — so no progress from this run (including earlier successfully-ingested papers in the same loop) is persisted. The next poll re-fetches the same candidates and hits the same failure again, indefinitely blocking the watch on one bad paper.
🔧 Proposed fix
if pdf_url:
path = _download_pdf(pdf_url)
if path:
- try:
- n_chunks, title = ingest_pdf(
- path, paper_id=arxiv_id,
- metadata={"title": p.get("title", ""), "source": p.get("source", "")},
- )
- finally:
+ try:
+ n_chunks, title = ingest_pdf(
+ path, paper_id=arxiv_id,
+ metadata={"title": p.get("title", ""), "source": p.get("source", "")},
+ )
+ except Exception as e:
+ logger.error(f"[Watch] ingest failed for {arxiv_id}: {e}")
+ n_chunks, title = 0, None
+ finally:
try:
os.remove(path)
except OSError:
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for p in passages: | |
| arxiv_id = p.get("arxiv_id") | |
| if not arxiv_id or arxiv_id in seen: | |
| continue | |
| new_ids.append(arxiv_id) # mark seen even if it dupes the corpus, so we stop re-fetching it | |
| pdf_url = p.get("pdf_url") | |
| if pdf_url: | |
| path = _download_pdf(pdf_url) | |
| if path: | |
| try: | |
| n_chunks, title = ingest_pdf( | |
| path, paper_id=arxiv_id, | |
| metadata={"title": p.get("title", ""), "source": p.get("source", "")}, | |
| ) | |
| finally: | |
| try: | |
| os.remove(path) | |
| except OSError: | |
| pass | |
| if n_chunks > 0: # 0 = duplicate/unchanged in corpus → seen, but not "new" | |
| ingested.append({"arxiv_id": arxiv_id, "title": title or p.get("title", ""), "text": p.get("text", "")}) | |
| continue | |
| # ponytail: no PDF (or download failed) → abstract feeds the digest only; | |
| # indexing an abstract-only chunk is deferred to a later increment. | |
| ingested.append({"arxiv_id": arxiv_id, "title": p.get("title", ""), "text": p.get("text", "")}) | |
| for p in passages: | |
| arxiv_id = p.get("arxiv_id") | |
| if not arxiv_id or arxiv_id in seen: | |
| continue | |
| new_ids.append(arxiv_id) # mark seen even if it dupes the corpus, so we stop re-fetching it | |
| pdf_url = p.get("pdf_url") | |
| if pdf_url: | |
| path = _download_pdf(pdf_url) | |
| if path: | |
| try: | |
| n_chunks, title = ingest_pdf( | |
| path, paper_id=arxiv_id, | |
| metadata={"title": p.get("title", ""), "source": p.get("source", "")}, | |
| ) | |
| except Exception as e: | |
| logger.error(f"[Watch] ingest failed for {arxiv_id}: {e}") | |
| n_chunks, title = 0, None | |
| finally: | |
| try: | |
| os.remove(path) | |
| except OSError: | |
| pass | |
| if n_chunks > 0: # 0 = duplicate/unchanged in corpus → seen, but not "new" | |
| ingested.append({"arxiv_id": arxiv_id, "title": title or p.get("title", ""), "text": p.get("text", "")}) | |
| continue | |
| # ponytail: no PDF (or download failed) → abstract feeds the digest only; | |
| # indexing an abstract-only chunk is deferred to a later increment. | |
| ingested.append({"arxiv_id": arxiv_id, "title": p.get("title", ""), "text": p.get("text", "")}) |
🤖 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 `@watch_runner.py` around lines 74 - 99, Update the ingest_pdf handling in
run_watch so an exception for one paper is caught and does not abort the
candidate loop or prevent persistence.save_watch(w) from running. Preserve
temporary-file cleanup, skip adding the failed paper to ingested, and continue
processing subsequent passages while retaining the new_ids progress for the
failed arxiv_id.
| async def watch_loop() -> None: | ||
| """Poll for due watches every WATCH_POLL_INTERVAL seconds until cancelled. | ||
|
|
||
| Started from the FastAPI lifespan only when WATCH_ENABLE is set. | ||
| """ | ||
| logger.info(f"[Watch] schedule loop started (interval {config.WATCH_POLL_INTERVAL}s)") | ||
| try: | ||
| while True: | ||
| await asyncio.sleep(config.WATCH_POLL_INTERVAL) | ||
| await run_due_watches() | ||
| except asyncio.CancelledError: | ||
| logger.info("[Watch] schedule loop stopped") | ||
| raise |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the poll loop against a single failed sweep killing scheduling forever.
run_due_watches() already catches per-watch failures, but a failure inside persistence.due_watches(now) itself (e.g. a transient I/O error) is uncaught here. Since watch_loop only handles asyncio.CancelledError, any other exception ends the loop permanently — the watch feature silently stops scheduling for the remainder of the process lifetime.
🔧 Proposed fix
try:
while True:
await asyncio.sleep(config.WATCH_POLL_INTERVAL)
- await run_due_watches()
+ try:
+ await run_due_watches()
+ except Exception as e:
+ logger.error(f"[Watch] poll iteration failed: {e}")
except asyncio.CancelledError:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def watch_loop() -> None: | |
| """Poll for due watches every WATCH_POLL_INTERVAL seconds until cancelled. | |
| Started from the FastAPI lifespan only when WATCH_ENABLE is set. | |
| """ | |
| logger.info(f"[Watch] schedule loop started (interval {config.WATCH_POLL_INTERVAL}s)") | |
| try: | |
| while True: | |
| await asyncio.sleep(config.WATCH_POLL_INTERVAL) | |
| await run_due_watches() | |
| except asyncio.CancelledError: | |
| logger.info("[Watch] schedule loop stopped") | |
| raise | |
| async def watch_loop() -> None: | |
| """Poll for due watches every WATCH_POLL_INTERVAL seconds until cancelled. | |
| Started from the FastAPI lifespan only when WATCH_ENABLE is set. | |
| """ | |
| logger.info(f"[Watch] schedule loop started (interval {config.WATCH_POLL_INTERVAL}s)") | |
| try: | |
| while True: | |
| await asyncio.sleep(config.WATCH_POLL_INTERVAL) | |
| try: | |
| await run_due_watches() | |
| except Exception as e: | |
| logger.error(f"[Watch] poll iteration failed: {e}") | |
| except asyncio.CancelledError: | |
| logger.info("[Watch] schedule loop stopped") | |
| raise |
🤖 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 `@watch_runner.py` around lines 131 - 143, Update watch_loop so exceptions from
each run_due_watches sweep are caught and logged, allowing the while loop to
continue polling after transient failures. Preserve asyncio.CancelledError
handling so cancellation still logs shutdown and propagates normally.
Ingest resilience (BUG-01/02): guard figure_captioner.caption_regions and
extract_regions so an optional VLM/extraction failure no longer discards a
paper's successfully-built text chunks or marks it failed.
OpenRouter (BUG-03/08): flatten google-genai Content/Part objects to real
prompt text instead of str() repr; skip streaming chunks with empty choices.
Watch runner (BUG-04): reject non-HTTP(S) URLs (SSRF) and stream the PDF
download with a 50 MB cap (OOM).
Concurrency (BUG-05/18/19): double-checked lock on backend init; route
circuit-breaker access through locked helpers; _ensure_pool() before
next_client_idx().
Correctness (BUG-06/07): per-index fallback title so two untitled chunks
aren't skipped as same-paper; run contradiction detection only on the
chunks that made it into the prompt (chunks[:chunks_used]).
Model/provider threading (BUG-09/10/17): thread model/provider end-to-end
through rag.answer_question / answer_with_history / llm_generate / sse_stream
and the /query, /chat, /chat/stream routes + UI chat payload; model included
in llm cache key; safe .get("content") in session listing.
Data/UI (BUG-11/12/13): crop_path=None on write failure; percent-encode
figure URLs; make /models sync so its blocking catalog fetch runs in the
threadpool instead of stalling the event loop.
Eval (BUG-14/15/16/20): use the language param in report section planning;
broaden cross-encoder except to catch load/OSError and fall back to Jaccard;
record the effective grounding judge + threshold in metrics and report;
log swallowed Gemini cache exceptions.
Watch lifecycle (BUG-21): add_done_callback surfaces a crashed schedule loop.
BUG-22 (watch ownership) left as a documented gap: auth is a single shared
key with no per-user identity, so ownership enforcement isn't possible
without per-user keys.
Docs: gitignore generated eval_report.json; dotenv fence tag; correct the
eval-hook description (labels results, does not generate answers).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rag.py (1)
407-449: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider including
providerin the LLM cache key.The cache key at line 433 includes
target_modelbut notprovider. If the same model name is ever used with different providers (e.g., an explicitprovideroverride), responses from one provider could be incorrectly returned for the other. Practical impact is low since model name typically implies provider, but addingproviderto the key would make the cache fully correct.♻️ Optional fix
- cache_key = make_key(prompt, max_tokens, config.LLM_TEMPERATURE, target_model) + cache_key = make_key(prompt, max_tokens, config.LLM_TEMPERATURE, target_model, provider)🤖 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 `@rag.py` around lines 407 - 449, Update the cache-key construction in the LLM response generation flow to include the provider override alongside prompt, token limit, temperature, and target_model. Ensure calls using the same model through different providers produce distinct cache entries, while preserving existing cache-hit behavior.
🤖 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.
Nitpick comments:
In `@rag.py`:
- Around line 407-449: Update the cache-key construction in the LLM response
generation flow to include the provider override alongside prompt, token limit,
temperature, and target_model. Ensure calls using the same model through
different providers produce distinct cache entries, while preserving existing
cache-hit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4856ba90-3a6e-4b6f-9b10-9c1f630decdf
📒 Files selected for processing (28)
.env.example.gitignoreREADME.mdagent/nodes/answer_generator.pyagent/nodes/query_planner.pyagent/nodes/reflexion_evaluator.pyapi_server.pybug_report.mdcontradiction.pydocs/Eval/eval_report.jsondocs/Eval/evaluate.pydocs/Eval/test_evaluate.pydocs/superpowers/specs/2026-07-12-openrouter-provider-design.mdfigure_captioner.pyingest.pyllm_client.pyproviders/gemini.pyproviders/openrouter.pyrag.pyreport_runner.pyroutes/chat.pyroutes/models.pyroutes/query.pysession_context.mdsse_utils.pystatic/index.htmltests/test_finalizer.pywatch_runner.py
💤 Files with no reviewable changes (2)
- tests/test_finalizer.py
- docs/Eval/eval_report.json
🚧 Files skipped from review as they are similar to previous changes (18)
- agent/nodes/answer_generator.py
- docs/Eval/test_evaluate.py
- api_server.py
- routes/models.py
- agent/nodes/reflexion_evaluator.py
- contradiction.py
- docs/superpowers/specs/2026-07-12-openrouter-provider-design.md
- agent/nodes/query_planner.py
- figure_captioner.py
- providers/gemini.py
- providers/openrouter.py
- watch_runner.py
- ingest.py
- .env.example
- report_runner.py
- llm_client.py
- docs/Eval/evaluate.py
- static/index.html
Same model routed through different providers now produces distinct cache entries instead of colliding on model+prompt alone. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
A selected OpenRouter model whose fallback provider is also OpenRouter
had no working fallback and failed outright on a free-tier 429. _attempts
now always appends ("gemini", LLM_MODEL_NAME) when no gemini path exists.
Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
LLMs emit \(...\)/\[...\] math delimiters; the renderer only stashed $...$/$$...$$, so marked stripped the backslash and printed raw TeX. Stash both delimiter styles before marked.parse. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Replace invalid/expiring free model ids with stable free models (gemma-4-31b-it, gemma-4-26b-a4b, nemotron-nano-9b-v2) in README and .env.example; add What's-New rows for the Gemini backstop, KaTeX math rendering, and provider-aware response cache key. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Missing -it suffix made google/gemma-4-26b-a4b:free a 400 invalid model ID. Corrected against the live OpenRouter catalog in README and .env.example (matches .env). Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Stable, function-calling free models only (excludes the July-19 expiries). Adds nvidia/nemotron-3-nano-omni-30b-a3b-reasoning:free alongside gemma-4-31b-it, gemma-4-26b-a4b-it, nemotron-nano-9b-v2. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Bigger models give better tool-use/reasoning on the agent path. Lead with nemotron-3-ultra-550b + super-120b (stable, non-expiring), keep gemma-4-31b as reliable mid-tier and gemini-3.5-flash primary/backstop. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Slug verified against live OpenRouter catalog. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
Each section was synthesized independently and numbered citations from 1, so the same [N] resolved to different papers across sections, and over-numbered inline markers (e.g. a stray [5]) dangled with no source. Add a shared title->number registry across sections, remap each section's local [N] markers to global numbers, drop markers with no matching citation, and emit one document-wide References list. Claude-Session: https://claude.ai/code/session_01KJfTZBK2wYtiiGvuWbnXuv
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@report_runner.py`:
- Around line 72-105: Update the upstream rag.extract_citations marker parser to
recognize comma-separated citation markers such as [1, 2], matching _MARKER_RE
and preserving each valid citation in answer_question results. Keep existing
single-number extraction behavior intact so _remap_markers receives all
referenced local citation keys.
🪄 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: 26e95cb0-b03c-487c-bc8b-27efdfe3438a
📒 Files selected for processing (6)
.env.exampleREADME.mdllm_client.pyrag.pyreport_runner.pystatic/index.html
🚧 Files skipped from review as they are similar to previous changes (4)
- llm_client.py
- rag.py
- static/index.html
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_rag.py`:
- Around line 148-157: Strengthen test_extract_citations_comma_no_spaces by
asserting each resolved citation’s number and title, matching the per-number
assertions in the spaced-variant test. Verify that [1,2] maps to citation 1
“Paper A” and citation 2 “Paper B,” while retaining the existing length
assertion.
🪄 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: 2b42f962-622e-44bd-8214-eca3889c71b6
📒 Files selected for processing (7)
.env.exampleREADME.mdllm_client.pyrag.pyreport_runner.pystatic/index.htmltests/test_rag.py
🚧 Files skipped from review as they are similar to previous changes (5)
- report_runner.py
- llm_client.py
- rag.py
- README.md
- static/index.html
1 participant
v2.3.0 — quality, multimodal, agentic redundancy
Rolls up Phases 0–8 on
2.3.0-dev. Every phase is off by default; existing Gemini-only behavior is unchanged unless the relevant flag/key is set. Full suite green: 170 passed, 0 failed.Phase 8 — Secondary LLM provider (OpenRouter) (latest)
First-class second provider alongside Gemini for cross-vendor failover, model breadth, and per-request model selection. Off unless
OPENROUTER_API_KEYis set.providers/package:LLMBackendABC +ShimResponse(Gemini-shaped shim so agent nodes readresp.candidates[...].function_call/resp.textunchanged);GeminiBackend(verbatim extraction of the old pool/circuit/classifier logic);OpenRouterBackend(OpenAI-compatible, viaopenaiSDK).llm_client.py→ dispatcher:resolve_provider(/in id → openrouter),(provider,model)circuit-breaker key, failover order requested → same-provider (LLM_FALLBACK_MODEL, gemini) → cross-provider (LLM_FALLBACK_PROVIDER). Public names/signatures preserved (rag.pyre-exports; ~52 tests patch them;providerkwarg is additive).GET /modelsallowlist + tool-capability catalog cache; capability gate on the agent path (tool-incapable model → gemini default, no silent degrade); non-Gemini JSON parse Gemini-retry fallback;model/providerrequest fields (/query,/agent/query) + UI dropdown;--model/--providereval label flag.Post-Phase-8 fixes (follow-up on the branch)
thinking_configon failover to a non-thinking model; streaming path key-rotation + model failover.providerin the LLM response cache key (no cross-provider cache collisions).\(...\)/\[...\]LaTeX delimiters via KaTeX.openai/gpt-oss-20b:free, gemma slug fix) + What's New notes.Phases 0–7 (already on the branch)
esc().Test plan
pytest tests/— 170 passed, 0 failedOPENROUTER_API_KEY, exercise UI dropdown in agent mode + cross-provider failoverpython docs/Eval/evaluate.py --model anthropic/claude-haikuNotes / deviations
implementation_plan.mdlives outside the repo (parent dir) — updated on disk, not tracked here./chat/stream) does not carrymodel(itsChatRequesthas no such field); only/agent/queryhonors per-request model.SAFETY_SETTINGSandGEMINI_CACHE_ENABLEDhave no OpenRouter equivalent (dropped on that backend);google-genaistays a hard dependency.Summary by CodeRabbit