Release 2.4.0 — Gemini 3.6 Flash, multi-user isolation & session config#12
Release 2.4.0 — Gemini 3.6 Flash, multi-user isolation & session config#12DNSdecoded wants to merge 24 commits into
Conversation
watch_runner.run_watch() ingested PDFs directly without calling the route layer's cache/index invalidation, so watch-ingested papers were invisible to search and served stale from the retrieval cache. Extract the shared helper into cache_refresh.py and call it from watch_runner after any paper is actually indexed.
…nguage stats Feedback was write-only: ratings were stored keyed by query_id, but query_id was never persisted alongside the question/answer, so there was no read path and no way to correlate feedback with what was actually asked. Add a query_log table logged from both the standard and agent query paths, and GET /feedback + GET /feedback/stats to read it back with per-language approval rates.
verify.check_claims already scored every answer sentence against its cited chunk via NLI entailment but discarded the winning chunk. Standard RAG ran the same faithfulness check and threw the aggregate score away entirely (logged only), unlike the agent path. Surface both: check_claims now returns the argmax supporting chunk per claim, and /query now returns a confidence score and per-claim evidence.
… fix PATCH /papers stores tags as one unsplit string, so a naive ChromaDB $in filter against split tag names can never match a paper tagged with more than one tag -- it would return zero results for the declared primary use case (multi-tag search). Route tags through a reserved sentinel in filter_dict instead; rag.retrieve_context pulls it out and applies tag matching as a Python-side post-filter after retrieval, for both the dense/hybrid and paper-scoped paths. Wired into the indicrag_retrieval agent tool, /query, /query/stream, /chat, and /chat/stream.
BibTeX export already existed for search results but not for a generated answer's or report's cited sources, so users couldn't pull an answer's bibliography into Zotero/Overleaf. Add POST /export/bibtex accepting a client-supplied source list, reusing the existing bibtex escaping and citation-key conventions.
After a batch ingest, there was no way to see which papers actually indexed successfully -- a corrupted/failed PDF silently shows 0 chunks and shrinks the corpus without anyone noticing until query time. Add GET /ingest/health and a sidebar panel showing per-paper chunk counts and failed papers, wired to re-ingest via the existing POST /ingest/reindex endpoint.
…ardening Building a corpus meant manually downloading PDFs first. The SSRF-guarded downloader already existed inside watches but was private to them; extract it as download_utils and expose it as a first-class ingest path (arxiv_id/doi/url/reading_list -> POST /ingest/from-url). Hardened two real gaps found in review, not just the originally- planned private-IP check: urlopen() auto-follows redirects, so a public URL could 302 to an internal address and fully bypass the guard -- redirects are now re-validated per hop. And the from-url path had no existing-paper_id check, so a crafted id could silently overwrite another paper's chunks -- now skipped with a warning.
No "compare these papers on these dimensions" output existed --
users hand-tabulated. Reuses the paper-scoped exhaustive retrieval
(_retrieve_scoped) to see each paper's full content, then asks one
grounded extraction per dimension. Runs as a background job (N papers
x M dimensions is N*M LLM calls) polled via /compare/status/{job_id},
same pattern as bulk ingest.
UI for this (static/index.html) held back -- a concurrent redesign
is in progress on that file (see DESIGN_NOTES.md); the compare-table
UI needs to be reapplied on top of it rather than committed with a
misleading message bundling both.
Refresh the visual identity per DESIGN_NOTES.md (warm scholarly palette, serif display type for headings, parchment sidebar) and add the "Compare Papers" modal/table UI for the /compare backend (rag.compare_papers, POST /compare) added in the prior commit.
…pare paper_id (the PDF filename stem) is what /compare, /ingest/reindex, and PATCH /papers actually need, but GET /papers only returned filename -- there was no way for the UI (or a user) to know what to send. Add paper_id to PaperInfo, and replace the Compare modal's raw paper_id text input with a checkbox list of ingested papers.
Paper-scoped retrieval already worked end to end (_retrieve_scoped, paper_ids threaded through /chat/stream) via the paper-scope checkboxes in the sidebar, but nothing showed the user their answer was actually being scoped. Add a "Scoped to: X" bar above the chat with a clear button, driven by the existing checkboxes -- no backend change needed.
Engineering half of the eval-hardening task: version each evaluate.py run into docs/Eval/eval_history/ (previously each run overwrote the last report, losing trend history), wire the already-built --ci --threshold gate into CI using the jaccard grounding judge (CI-safe, no LLM/API key -- scores the already-committed eval answers), and add GET /quality to surface the latest report. Expanding the eval set itself to 50+ real multilingual queries is not done here -- it needs actual corpus/domain knowledge to write correct relevance judgments, not something to fabricate from outside the corpus.
… reviews
Reports rode the generic job store (deps._jobs), which prunes
completed entries after 24h -- fine for a one-shot ingest job, silent
data loss for a report meant to be a durable artifact. Add a
dedicated reports table (mirrors watches) plus GET /reports and
GET /reports/{report_id}, and persist on every successful /report job.
A watch that owns a living review (report_id set) now regenerates
that report in place whenever it actually indexes a new paper --
no confirmation step a user could forget to click.
SSE push notification for watch completions (the other half of this
task) is deferred: no pub/sub broadcast infrastructure exists yet
(sse_utils.py only streams a single request/response), and building
one is a separate, larger piece of work than this card's scope.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe v2.4 update adds tag-filtered retrieval, confidence and evidence metadata, secure URL ingestion, comparison jobs, durable query/report persistence, feedback and BibTeX APIs, evaluation history and CI gating, expanded UI workflows, and refreshed configuration and documentation. Changesv2.4 platform expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant IngestRoutes
participant DownloadUtils
participant VectorStore
Browser->>IngestRoutes: POST /ingest/from-url
IngestRoutes->>DownloadUtils: Resolve and download validated PDF
DownloadUtils-->>IngestRoutes: Temporary PDF path
IngestRoutes->>VectorStore: Ingest PDF and check existing paper IDs
VectorStore-->>IngestRoutes: Indexing result and chunk count
IngestRoutes-->>Browser: Job status and progress
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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/Eval/evaluate.py (1)
466-471: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite history snapshots independently of
--json.The snapshot call is inside
if args.json, but.github/workflows/ci.ymlrunspython evaluate.py --ci --threshold 0.85without--json. Consequently, the CI evaluation never records the new history artifact. Move the snapshot call outside that conditional, or pass an explicit JSON output path in CI.🤖 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 466 - 471, Move the write_history_snapshot(metrics) call and its history-path output from inside the if args.json block so snapshots are created for every evaluation, including CI runs without --json; keep JSON file writing and its output message conditional on args.json.routes/ingest.py (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInconsistent
paper_idnormalization between/ingest/from-urland watch ingestion — same paper can be double-indexed.
routes/ingest.py::_bibtex_safe_idsanitizes ids (e.g."2401.07041"→"2401_07041") before using them as the Chromapaper_idin_run_batch_url_ingest, butwatch_runner.py::run_watchingests the same kind of arXiv id raw and unsanitized. A paper ingested once via a watch and later re-submitted via/ingest/from-url(or vice versa) lands under two differentpaper_ids, silently duplicating it in the corpus rather than being recognized as already-present — defeating the collision/dedup intent that_run_batch_url_ingest's existing-id check was built for.
routes/ingest.py#L575-578: this is where the new, divergent normalization was introduced.watch_runner.py#L73-76: apply the same normalization (e.g. reuse_bibtex_safe_idor extract it into a shared helper) topaper_id=arxiv_idhere so both paths agree on onepaper_idfor the same external id.🤖 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/ingest.py` at line 1, Align watch ingestion with URL ingestion by normalizing the arXiv identifier before assigning it as paper_id in watch_runner.py::run_watch. Reuse routes/ingest.py::_bibtex_safe_id or extract the normalization into a shared helper, ensuring _run_batch_url_ingest and run_watch produce the same paper_id for identical external IDs while preserving existing deduplication behavior.watch_runner.py (1)
73-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRaw
arxiv_idused aspaper_id— diverges from/ingest/from-url's sanitized scheme. See consolidated comment.
ingest_pdfis called with the unsanitizedarxiv_id(e.g."2401.07041") aspaper_id, whileroutes/ingest.py::_run_batch_url_ingestruns the same kind of id through_bibtex_safe_id(which strips.) before using it aspaper_id. The same paper ingested via both paths ends up under two differentpaper_ids.🤖 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 73 - 76, Update the watch runner’s ingest_pdf call to sanitize arxiv_id through the existing _bibtex_safe_id scheme before passing it as paper_id. Reuse the established helper or equivalent shared sanitization used by _run_batch_url_ingest, while preserving the raw arxiv_id for metadata or other external identifiers.
🧹 Nitpick comments (9)
static/index.html (2)
726-744: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFocus-ring color doesn't use the theme's primary accent.
.input-bar:focus-withinsetsbox-shadow: 0 0 0 3px rgba(51,65,85,.12)with a hardcoded slate color instead of an alpha ofvar(--primary). In dark mode,--primaryis#58A6FF(gold/blue accent), so the focus ring will visually clash/mismatch instead of reinforcing the accent color used elsewhere (e.g.border-color: var(--primary)on the same rule).🎨 Suggested fix
- .input-bar:focus-within { border-color: var(--primary); box-shadow: 0 0 0 3px rgba(51,65,85,.12); } + .input-bar:focus-within { border-color: var(--primary); box-shadow: 0 0 0 3px rgba(46,80,144,.15); }Better: define a
--focus-ringcustom property per theme so light/dark both get a color-matched ring.🤖 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 726 - 744, Update the .input-bar:focus-within focus ring to use the theme’s --primary accent instead of the hardcoded rgba(51,65,85,.12); preferably define and use a --focus-ring custom property in each theme so the ring remains color-matched in light and dark modes while preserving the existing border-color behavior.
1767-1800: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo busy-state guard on the "+ Add" ingest button.
ingestFromUrl()doesn't disable its triggering button while the fetch + 2s polling loop runs, unlikerunCompare()andreindexPaper()which disable their buttons for the duration of the async operation. Repeated clicks can fire multiple/ingest/from-urlPOSTs and spawn multiple overlappingsetIntervalpolling loops, quickly exhausting the server's5/minuterate limit on that endpoint (perroutes/ingest.py's@limiter.limit("5/minute")oningest_from_url).🛑 Suggested fix
-async function ingestFromUrl() { - const input=document.getElementById('urlIngestInput'); +async function ingestFromUrl(btn) { + const input=document.getElementById('urlIngestInput'); const raw=input.value.trim(); if(!raw){ showToast('Enter an arXiv ID, DOI, or PDF URL','error'); return; } + if(btn){ btn.disabled=true; } ... - }catch(e){ showToast(`✗ ${e.message}`,'error'); } + }catch(e){ showToast(`✗ ${e.message}`,'error'); } + finally{ if(btn){ btn.disabled=false; } } }(and re-enable the button once polling settles to
success/partial/failed, similar torunCompare).🤖 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 1767 - 1800, Update ingestFromUrl() to guard the triggering “+ Add” button for the entire async request and polling lifecycle: disable it before the initial fetch, and re-enable it when polling reaches success, partial, or failed, as well as on timeout or request errors. Ensure all exit paths clear the interval and restore the button so repeated clicks cannot create overlapping ingestion jobs.agent/tool_executor.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTags-sentinel filter parsing/combination is duplicated across
agent/tool_executor.pyandroutes/query.py. Both files independently implement the same_TAGS_SENTINEL-based comma-tag parsing and$andfilter combination logic; consolidating into one shared helper (e.g. inrag.py, which already owns_TAGS_SENTINEL) removes the duplication and the risk of the two copies drifting apart as the contract evolves.
agent/tool_executor.py#L96-126: replace_tags_filter/_combine_filterswith calls to a shared helper.routes/query.py#L38-59: replacebuild_tags_filter/combine_filterswith calls to the same shared helper (and haveroutes/chat.py, which imports these fromroutes/query.py, continue to import from the new shared location).🤖 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/tool_executor.py` at line 1, Consolidate the duplicated _TAGS_SENTINEL tag parsing and $and filter-combination logic into shared helpers in rag.py, which owns the sentinel. Replace _tags_filter and _combine_filters in agent/tool_executor.py and build_tags_filter and combine_filters in routes/query.py with calls to those helpers, then update routes/chat.py imports to use the shared location while preserving existing behavior.rag.py (1)
448-475: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider per-cell error isolation for the comparison matrix.
compare_papersruns N*M sequentialllm_generatecalls; the docstring/_run_compare_jobtreat a single exception as failing the whole job, discarding any cells already computed. Given comparisons can span multiple papers/dimensions, catching per-dimension exceptions and recording e.g."Error: ..."in that cell (instead of aborting the whole matrix) would preserve partial results.🤖 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 448 - 475, Update compare_papers so each llm_generate call inside the per-dimension loop is isolated with exception handling. On failure, store an “Error: …” message in that paper/dimension cell and continue processing remaining dimensions and papers, while preserving successful results and existing not-found handling.routes/query.py (1)
38-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
build_tags_filter/combine_filtersduplicateagent/tool_executor.py's_tags_filter/_combine_filters.Both implement the identical tags-sentinel contract independently. Consider extracting one shared helper (e.g., into
rag.py, which already owns_TAGS_SENTINEL) to avoid the two copies drifting apart.🤖 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 38 - 59, Extract the shared tag-sentinel parsing and filter-combination logic from routes/query.py’s build_tags_filter and combine_filters and agent/tool_executor.py’s _tags_filter and _combine_filters into a common helper, preferably in rag.py alongside _TAGS_SENTINEL. Update both callers to reuse that helper, preserving the existing None handling, tag trimming, and combined-filter behavior.routes/ingest.py (1)
528-532: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull-corpus metadata fetch just to check a handful of ids.
vector_store._chroma_call(collection.get, include=["metadatas"])pulls every chunk's metadata for the whole corpus on every batch job, purely to build a paper_id membership set. This scales linearly with corpus size regardless of how many ids are actually being ingested in this batch.Consider querying only for the specific sanitized ids being ingested (e.g.
collection.get(where={"paper_id": {"$in": [...]}})) instead of pulling the entire corpus's metadata.🤖 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/ingest.py` around lines 528 - 532, Update the existing-paper lookup around collection.get so it queries only the sanitized paper IDs being ingested, using the collection’s paper_id filter and preserving the current metadata-based membership set; avoid fetching the full corpus.cache_refresh.py (1)
13-18: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent failure on BM25 rebuild — add logging.
Unlike the cache-invalidation block right below it, a failure here (
bm25_searchimport/invalidate/rebuild) is swallowed with a barepass. This makes a stale BM25 index invisible to operators — hybrid search would keep serving out-of-date results with no signal anything went wrong.♻️ Proposed fix
try: import bm25_search bm25_search.invalidate() threading.Thread(target=bm25_search.get_or_build_index, daemon=True).start() - except Exception: - pass + except Exception: + logger.warning("Failed to invalidate/rebuild BM25 index after ingestion", exc_info=True)🤖 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 `@cache_refresh.py` around lines 13 - 18, Update the BM25 refresh try/except around bm25_search.invalidate and bm25_search.get_or_build_index to log the caught exception instead of silently passing. Match the logging approach used by the cache-invalidation block below so failures are visible while preserving the existing asynchronous rebuild behavior.watch_runner.py (1)
90-109: 🧹 Nitpick | 🔵 TrivialLiving-review regeneration adds real latency to every indexed watch run.
Each watch with
report_idset now re-synthesizes a full multi-section report (multiple LLM calls viareport_runner.run_report) inline whenever anything is indexed. This runs off the event loop (asyncio.to_thread) and failures are caught, so it's not a correctness risk, but worth keeping an eye onWATCH_POLL_INTERVALvs. total per-watch runtime as the number of "living review" watches grows.🤖 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 90 - 109, Review the indexed-watch path around _post_ingest_refresh and the report_runner.run_report call to prevent living-review regeneration from adding unbounded inline latency to every watch run. Move regeneration to an asynchronous/background execution path while preserving report persistence and existing exception logging, and ensure watch polling remains responsive as multiple report_id watches run.routes/management.py (1)
206-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate BibTeX entry-formatting logic between
export_search_resultsandexport_bibtex.Both endpoints independently build the same
title/author/yearfield list, escape via_bibtex_escape, and assemble the@article{key,\n...\n}template — differing only in how the citation key is derived (_bibtex_key(paper_id)vs_cite_key(authors, year, i)).Consider extracting a shared
_format_bibtex_entry(key, title, authors, year)helper used by both endpoints.Also applies to: 244-267
🤖 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/management.py` around lines 206 - 218, Extract the shared title/author/year escaping and `@article` assembly from export_bibtex and export_search_results into a _format_bibtex_entry(key, title, authors, year) helper. Update both endpoints to call this helper while preserving their existing citation-key derivation through _bibtex_key and _cite_key.
🤖 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 @.env.example:
- Around line 198-200: Align the CHAT_HISTORY_MAX_TURNS default across the
runtime fallback and configuration documentation. Update .env.example lines
198-200 and README.md lines 456-457 to consistently reflect the chosen effective
default, or change the runtime fallback accordingly so all surfaces use the same
value.
- Around line 157-164: The documented authentication configuration must keep
`/purge/*` fail-closed: require a dedicated ADMIN_API_KEY in multi-user mode
instead of falling back to API_KEYS, and ensure ALLOW_UNAUTHENTICATED mode
disables purge routes rather than exposing them anonymously. Update the
ADMIN_API_KEY and ALLOW_UNAUTHENTICATED guidance accordingly.
In `@docs/Eval/evaluate.py`:
- Around line 51-52: Update the timestamp-based filename construction in the
evaluation history flow to prevent collisions between evaluations started within
the same second. Extend the timestamp in the ts value to include microseconds,
or append another unique suffix, while preserving the existing eval_report.json
naming structure.
In `@persistence.py`:
- Around line 257-269: Update the ON CONFLICT(id) DO UPDATE SET clause in
save_report to also assign created_at from the incoming excluded row, while
preserving the existing field updates and insert behavior so regenerated reports
sort newest first.
- Around line 159-173: Update feedback_stats so its by_language aggregation
includes every feedback row counted by total, up, and down. Replace the INNER
JOIN in the aggregation with a LEFT JOIN and bucket feedback without a matching
query_log row under a consistent fallback language key, preserving the existing
count and approval-rate calculations.
In `@routes/ingest.py`:
- Around line 575-578: Remove or bypass _bibtex_safe_id normalization when
assigning the ChromaDB paper_id, and use the original raw identifier
consistently with watch_runner.run_watch. Preserve any separate
filesystem-safety handling where required, but ensure arXiv IDs, DOIs, and URLs
retain the same paper_id across ingestion paths.
- Around line 511-540: Make the collision protection in _run_batch_url_ingest
concurrency-safe by adding a process-wide lock and registry for paper_ids
currently being ingested. Atomically check and reserve each paper_id immediately
before ingest_pdf, skip and count collisions when already reserved or present,
and release the reservation in a finally block so failures do not leave stale
entries; retain the existing corpus check as needed but do not rely on the
startup snapshot alone.
In `@routes/query.py`:
- Around line 323-334: The CompareRequest validation leaves comparison jobs
unbounded because paper_ids and dimensions only enforce minimum lengths. Add
appropriate maximum-length validation to both fields so the resulting N*M LLM
workload has a fixed cap, applying the same limits to any related request model
or comparison endpoint flow around the referenced job handling.
- Around line 323-334: Update CompareRequest.model to use the same
validate_model_allowlisted validator as QueryRequest and ChatRequest, reusing
the existing routes.models validation path so only allowlisted model IDs reach
the comparison flow.
- Around line 250-273: Update the persistence.log_query call in the query
handler to pass the computed result['answer_confidence'] value instead of
hardcoded 0.0, matching the confidence used by the returned QueryResponse.
- Around line 381-387: Update get_compare_status and the compare-job
creation/storage flow to associate each job with the submitting API key, using
the authenticated request’s key for ownership checks. Before returning a job,
reject access when its owner does not match the requester, while preserving the
existing not-found response for unknown job IDs and returning results only to
the owning key.
In `@tests/test_api.py`:
- Around line 164-175: Update test_ingest_from_url_accepts_direct_url to mock
download_utils.download_pdf before posting to /ingest/from-url, matching the
existing ingest test patterns. Configure the mock to return the expected
successful download result so the synchronous BackgroundTasks execution cannot
make a real HTTP request while preserving the 202 response and job payload
assertions.
In `@verify.py`:
- Around line 115-123: The citation-processing flow around _paper_chunk_map must
preserve each chunk’s original position instead of returning the grouped-list
position as supporting_chunk_index. Carry the source index alongside grouped
chunks, use it when selecting supporting_chunk_index and its corresponding
metadata, and add regression coverage for non-first and interleaved citations.
---
Outside diff comments:
In `@docs/Eval/evaluate.py`:
- Around line 466-471: Move the write_history_snapshot(metrics) call and its
history-path output from inside the if args.json block so snapshots are created
for every evaluation, including CI runs without --json; keep JSON file writing
and its output message conditional on args.json.
In `@routes/ingest.py`:
- Line 1: Align watch ingestion with URL ingestion by normalizing the arXiv
identifier before assigning it as paper_id in watch_runner.py::run_watch. Reuse
routes/ingest.py::_bibtex_safe_id or extract the normalization into a shared
helper, ensuring _run_batch_url_ingest and run_watch produce the same paper_id
for identical external IDs while preserving existing deduplication behavior.
In `@watch_runner.py`:
- Around line 73-76: Update the watch runner’s ingest_pdf call to sanitize
arxiv_id through the existing _bibtex_safe_id scheme before passing it as
paper_id. Reuse the established helper or equivalent shared sanitization used by
_run_batch_url_ingest, while preserving the raw arxiv_id for metadata or other
external identifiers.
---
Nitpick comments:
In `@agent/tool_executor.py`:
- Line 1: Consolidate the duplicated _TAGS_SENTINEL tag parsing and $and
filter-combination logic into shared helpers in rag.py, which owns the sentinel.
Replace _tags_filter and _combine_filters in agent/tool_executor.py and
build_tags_filter and combine_filters in routes/query.py with calls to those
helpers, then update routes/chat.py imports to use the shared location while
preserving existing behavior.
In `@cache_refresh.py`:
- Around line 13-18: Update the BM25 refresh try/except around
bm25_search.invalidate and bm25_search.get_or_build_index to log the caught
exception instead of silently passing. Match the logging approach used by the
cache-invalidation block below so failures are visible while preserving the
existing asynchronous rebuild behavior.
In `@rag.py`:
- Around line 448-475: Update compare_papers so each llm_generate call inside
the per-dimension loop is isolated with exception handling. On failure, store an
“Error: …” message in that paper/dimension cell and continue processing
remaining dimensions and papers, while preserving successful results and
existing not-found handling.
In `@routes/ingest.py`:
- Around line 528-532: Update the existing-paper lookup around collection.get so
it queries only the sanitized paper IDs being ingested, using the collection’s
paper_id filter and preserving the current metadata-based membership set; avoid
fetching the full corpus.
In `@routes/management.py`:
- Around line 206-218: Extract the shared title/author/year escaping and
`@article` assembly from export_bibtex and export_search_results into a
_format_bibtex_entry(key, title, authors, year) helper. Update both endpoints to
call this helper while preserving their existing citation-key derivation through
_bibtex_key and _cite_key.
In `@routes/query.py`:
- Around line 38-59: Extract the shared tag-sentinel parsing and
filter-combination logic from routes/query.py’s build_tags_filter and
combine_filters and agent/tool_executor.py’s _tags_filter and _combine_filters
into a common helper, preferably in rag.py alongside _TAGS_SENTINEL. Update both
callers to reuse that helper, preserving the existing None handling, tag
trimming, and combined-filter behavior.
In `@static/index.html`:
- Around line 726-744: Update the .input-bar:focus-within focus ring to use the
theme’s --primary accent instead of the hardcoded rgba(51,65,85,.12); preferably
define and use a --focus-ring custom property in each theme so the ring remains
color-matched in light and dark modes while preserving the existing border-color
behavior.
- Around line 1767-1800: Update ingestFromUrl() to guard the triggering “+ Add”
button for the entire async request and polling lifecycle: disable it before the
initial fetch, and re-enable it when polling reaches success, partial, or
failed, as well as on timeout or request errors. Ensure all exit paths clear the
interval and restore the button so repeated clicks cannot create overlapping
ingestion jobs.
In `@watch_runner.py`:
- Around line 90-109: Review the indexed-watch path around _post_ingest_refresh
and the report_runner.run_report call to prevent living-review regeneration from
adding unbounded inline latency to every watch run. Move regeneration to an
asynchronous/background execution path while preserving report persistence and
existing exception logging, and ensure watch polling remains responsive as
multiple report_id watches run.
🪄 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 Plus
Run ID: f5910212-5019-4810-a928-3d9c9e5046aa
📒 Files selected for processing (35)
.env.example.github/workflows/ci.yml.gitignoreREADME.mdagent/tool_declarations.pyagent/tool_executor.pycache_refresh.pyconfig.pydocs/Eval/evaluate.pydocs/Eval/test_evaluate.pydownload_utils.pygemini_cache.pypersistence.pyrag.pyroutes/agent.pyroutes/chat.pyroutes/feedback.pyroutes/ingest.pyroutes/management.pyroutes/query.pyroutes/report.pystatic/index.htmltests/test_agent.pytests/test_api.pytests/test_download_utils.pytests/test_feedback.pytests/test_ingest.pytests/test_rag.pytests/test_report_routes.pytests/test_vector_store.pytests/test_verify.pytests/test_watch_run.pyvector_store.pyverify.pywatch_runner.py
| # Admin API key for destructive operations (purge, delete). Falls back to | ||
| # API_KEYS validation when not set. Required in production for /purge/* routes. | ||
| # ADMIN_API_KEY= | ||
|
|
||
| # Production startup (start_server.py without --dev) refuses to boot with no | ||
| # API_KEYS. Set to 1 to explicitly allow an unauthenticated server (private | ||
| # hosts only — every endpoint, including DELETE /purge/*, becomes anonymous). | ||
| # ALLOW_UNAUTHENTICATED=1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Keep purge authorization fail-closed.
In multi-user mode, falling back from ADMIN_API_KEY to the ordinary API_KEYS pool lets any user key authorize /purge/*. Setting ALLOW_UNAUTHENTICATED=1 makes those destructive routes anonymous. Require a dedicated admin key, or disable purge routes in unauthenticated 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 @.env.example around lines 157 - 164, The documented authentication
configuration must keep `/purge/*` fail-closed: require a dedicated
ADMIN_API_KEY in multi-user mode instead of falling back to API_KEYS, and ensure
ALLOW_UNAUTHENTICATED mode disables purge routes rather than exposing them
anonymously. Update the ADMIN_API_KEY and ALLOW_UNAUTHENTICATED guidance
accordingly.
| # Maximum number of conversation turns retained per session (each turn = user + | ||
| # assistant message). Older turns are pruned from the front. | ||
| CHAT_HISTORY_MAX_TURNS=20 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the documented chat-history default with the runtime default.
The changed example and README advertise 20 turns, while the supplied runtime fallback is 10 when CHAT_HISTORY_MAX_TURNS is unset. Choose one effective default and update all configuration surfaces consistently.
.env.example#L198-L200: align the example value with the runtime fallback, or update the runtime fallback to 20.README.md#L456-L457: document the same effective default.
📍 Affects 2 files
.env.example#L198-L200(this comment)README.md#L456-L457
🤖 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 @.env.example around lines 198 - 200, Align the CHAT_HISTORY_MAX_TURNS
default across the runtime fallback and configuration documentation. Update
.env.example lines 198-200 and README.md lines 456-457 to consistently reflect
the chosen effective default, or change the runtime fallback accordingly so all
surfaces use the same value.
| @router.get("/compare/status/{job_id}", tags=["Comparison"]) | ||
| async def get_compare_status(job_id: str, authenticated: bool = Depends(verify_api_key)): | ||
| with _jobs_lock: | ||
| job = _jobs.get(job_id) | ||
| if job is None: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=f"Job '{job_id}' not found.") | ||
| return job |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether jobs/_jobs are scoped by API key anywhere in deps.py or ingest routes
rg -n "api_key" deps.py routes/ingest.py -A3 -B3Repository: DNSdecoded/IndicRAG
Length of output: 5761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deps outline =="
ast-grep outline deps.py --view expanded 2>/dev/null | sed -n '1,220p' || true
echo "== deps relevant =="
sed -n '1,160p' deps.py
echo "== ingest/_compare job usage =="
rg -n "_jobs|_jobs_lock|_update_job|compare|job_id" deps.py routes/ingest.py routes -A2 -B2
echo "== query route relevant =="
sed -n '320,410p' routes/query.py
echo "== ingest compare submission/status snippets =="
sed -n '140,330p' routes/ingest.py
sed -n '332,400p' routes/ingest.pyRepository: DNSdecoded/IndicRAG
Length of output: 49608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search api_key/job ownership scoping =="
rg -n "api_key|submitted_by|owner|_jobs|_jobs_lock|compare/status|ingest/status|report/status" routes deps.py -g '*.py' -A2 -B2
echo "== route import/api key usage summary =="
python3 - <<'PY'
from pathlib import Path
import ast
files = [p for p in Path('routes').glob('*') if p.suffix == '.py']
for p in files:
tree = ast.parse(p.read_text())
route_names = []
deps = []
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef):
for decorator in node.decorator_list:
if isinstance(decorator, ast.AsyncFunctionDef):
route_names.append({"path": None, "func": node.name, "line": node.lineno, "depends": []})
elif isinstance(decorator, ast.Call) and isinstance(decorator.func, ast.Attribute) and decorator.func.attr == "get" or isinstance(decorator, ast.Call) and isinstance(decorator.func, ast.Attribute) and decorator.func.attr == "post":
route_names.append({"path": (node.lineno,), "func": node.name, "line": node.lineno})
if isinstance(node, ast.Module):
for imp in node.body:
if isinstance(imp, ast.ImportFrom) and imp.module == "deps":
names = [n.name for n in imp.names]
if "verify_api_key" in names:
deps.append({"file": str(p), "depends_on_api_key": any(isinstance(f, ast.FunctionDef) and any(
isinstance(a, ast.keyword) and a.arg == "authenticated" and isinstance(a.value, ast.Name) and a.value.id == "authenticated"
for a in ast.walk(getattr(f, 'body', []))) for f in tree.body)})
api_dependant = [f["func"] for r in route_names for f in (getattr(ast.find(globals(), "_"), 'not_needed', []))]
PYRepository: DNSdecoded/IndicRAG
Length of output: 25317
Scope compare job results by the submitting API key.
_jobs is a global store, and GET /compare/status/{job_id} returns any authenticated caller that guesses the returned job_id the full completed result (dimensions, matrix). Store a per-key ownership field and reject/status responses for jobs that don’t match the request’s API key.
🤖 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 381 - 387, Update get_compare_status and the
compare-job creation/storage flow to associate each job with the submitting API
key, using the authenticated request’s key for ownership checks. Before
returning a job, reject access when its owner does not match the requester,
while preserving the existing not-found response for unknown job IDs and
returning results only to the owning key.
| entail_probs = probs[:, config.NLI_ENTAILMENT_INDEX] | ||
| best_idx = int(entail_probs.argmax()) | ||
| score = float(entail_probs[best_idx]) | ||
| results.append({ | ||
| "claim": sent, "support": score, | ||
| "grounded": score >= config.FAITHFULNESS_THRESHOLD, | ||
| "supporting_chunk": cited_chunks[best_idx][:500], # truncate for payload size | ||
| "supporting_chunk_index": best_idx, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve the original chunk index.
best_idx indexes the flattened cited_chunks list, but _paper_chunk_map() discards each chunk’s original position and groups chunks by paper. For a citation such as [2], or interleaved chunks from multiple papers, supporting_chunk_index will not match the corresponding index in chunks/metadatas; downstream source resolution can therefore select the wrong metadata. Carry the original index alongside each chunk and return that index, with a regression test for non-first and interleaved citations.
🤖 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 `@verify.py` around lines 115 - 123, The citation-processing flow around
_paper_chunk_map must preserve each chunk’s original position instead of
returning the grouped-list position as supporting_chunk_index. Carry the source
index alongside grouped chunks, use it when selecting supporting_chunk_index and
its corresponding metadata, and add regression coverage for non-first and
interleaved citations.
…ation, confidence logging, model validation, silent failure
1 participant
Summary
SESSION_MAX_AGE_HOURS(eviction) andCHAT_HISTORY_MAX_TURNS(history pruning) now user-configurableTest plan
Summary by CodeRabbit
New Features
Improvements
Quality