Release 2.4.0 — Gemini 3.6 Flash, multi-user isolation & session config - #12
Merged
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: 13 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 (12)
📝 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, provider fallback handling, 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
Comment on lines
+198
to
+200
| # 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.
…ation, confidence logging, model validation, silent failure
- verify.check_claims: unpack the faithfulness tuple correctly - providers/gemini: retry without thinking_config when a model 400s on thinking_budget=0; streaming only retries if nothing was emitted yet - rag: retrieval cache never populated — deep-copy entries on put as well as on read, and decide cache eligibility before materializing the collection (explicit collection/filter_dict bypasses the cache) - rag: tags filter returned nothing — over-fetch by TAGS_OVERFETCH (capped at TAGS_OVERFETCH_MAX) since tags are a Python post-filter - llm_client: cross-vendor failover looped back to Google because OpenRouter rewrites a bare Gemini name to google/<model>; _fallback_model_for now picks the first "/"-shaped slug - agent/nodes/answer_generator: honour the model requested by the caller - embeddings: thread-safety fix for the lazy singleton - routes/ingest: process-wide collision lock hardening - static/index.html: expose depth, scope, tags, metadata edit, BibTeX export, saved reports and index-health surfaces - config/.env.example: default model gemini-3.6-flash, multi-key docs 253 tests pass (4 deselected: integration/network).
Audited every live doc against actual routes, config defaults and repo layout rather than memory. - README: endpoint table matched to the real route inventory, per-view Web UI section, config table with defaults read from config.py, corrected LLM_SELECTABLE_MODELS default, v2.4 fix notes - docs/ARCHITECTURE: "offline-first" corrected to local-first retrieval (generation is remote), fabricated model lists and 20-30GB memory figures replaced, new caching/filtering and dispatch/failover sections - docs/QUICKSTART: rewritten — d:/RAG paths, dead file:// links and root-level example scripts were all wrong - docs/DEPLOY: collapsed to a pointer; it duplicated DEPLOYMENT.md - docs/DEPLOYMENT: key setup incl. the API_KEYS startup gate, Docker section, deep health checks, pricing links instead of a stale figure - docs/PRODUCTION + PRODUCTION.md: port 8000 -> 8080, /docs -> /api/docs, service rag-api -> indicrag, compose sample matched to the real file - docs/GEMINI_SETUP: covers both providers, Nov-2024 pricing table and invented model tiers replaced with the real routing rule - docs/CONTRIBUTING: Python 3.11+, pytest instead of test_pipeline.py, dropped four "wanted" features that already shipped - docs/evaluation: evaluate.py and its inputs live in docs/Eval/ - PROJECT_STRUCTURE: regenerated — routes/, providers/, llm_client.py, persistence.py and the Docker files were all missing - removed docs/PDF_UPLOAD_NOTE.md (claimed the upload UI did not exist) Verified: no d:/RAG, localhost:8000, test_pipeline.py or root-level example_*.py references remain; all relative Markdown links resolve.
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/PRODUCTION.md (1)
47-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAuthenticate the quick-start requests.
These commands omit
X-API-Key, despite Lines 84-87 stating every endpoint requires it in production. Add the header to the query, health, and stats examples.🤖 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/PRODUCTION.md` around lines 47 - 75, Update the curl examples in the production documentation for the English query, Hindi query, health check, and statistics endpoints to include the required X-API-Key authentication header, matching the production requirement stated later in the document.config.py (1)
281-284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
gemini-3.5-flashas the primary default.The release changes the configured and documented primary model to
gemini-3.6-flash, contrary to the required failover contract.
config.py#L281-L284: restoregemini-3.5-flashasLLM_MODEL_NAME.config.py#L321-L321: keepgemini-3.5-flashas the first selectable default.docs/DEPLOYMENT.md#L154-L158: document the required primary default.docs/GEMINI_SETUP.md#L30-L34: update the quick-start.envexample.docs/GEMINI_SETUP.md#L129-L135: update the configuration example.docs/PRODUCTION.md#L107-L115: update the production.envexample.docs/PRODUCTION.md#L175-L188: update the model-selection example and text.tests/test_openrouter_config.py#L13-L13: assert the required default.As per coding guidelines,
generate_with_failover()must usegemini-3.5-flashas its primary 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 `@config.py` around lines 281 - 284, Restore gemini-3.5-flash as the primary default used by LLM_MODEL_NAME and generate_with_failover(), while retaining it as the first selectable model in config.py; update the documented defaults and examples in config.py (281-284, 321-321), docs/DEPLOYMENT.md (154-158), docs/GEMINI_SETUP.md (30-34, 129-135), and docs/PRODUCTION.md (107-115, 175-188), and update tests/test_openrouter_config.py (13-13) to assert gemini-3.5-flash.Source: Coding guidelines
🧹 Nitpick comments (5)
routes/ingest.py (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a top-level
import threadingover__import__("threading").Purely cosmetic, but the inline
__import__is harder to grep and defeats tooling.♻️ Proposed tidy-up
-_ingest_lock = __import__("threading").Lock() +_ingest_lock = threading.Lock()Add
import threadingto the module imports at the top of the file.🤖 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 24 - 32, Replace the inline __import__("threading") usage for _ingest_lock with a standard top-level import threading, then initialize the lock through that imported module while leaving _inflight_paper_ids unchanged.static/index.html (2)
1642-1642: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead stub —
window.removeSelectedis redefined at Line 2114.This "coming soon" placeholder is silently overwritten by the real implementation later in the same IIFE. Remove 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 `@static/index.html` at line 1642, Remove the earlier placeholder definition of window.removeSelected that displays the “coming soon” toast, leaving the real implementation later in the same IIFE unchanged.
11-14: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd SRI hashes to the CDN scripts.
marked,katex, anddompurifyare pinned by version but loaded withoutintegrity. DOMPurify in particular is the sanitizer guarding all rendered LLM/corpus markdown, so a compromised CDN response silently disables XSS protection.🔒 Suggested change
-<script src="https://cdn.jsdelivr.net/npm/marked@18.0.5/lib/marked.umd.min.js" crossorigin="anonymous"></script> +<script src="https://cdn.jsdelivr.net/npm/marked@18.0.5/lib/marked.umd.min.js" integrity="sha384-..." crossorigin="anonymous"></script>(apply the same for katex css/js and purify.min.js)
🤖 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 11 - 14, Add SRI integrity hashes to the pinned marked, KaTeX CSS/JS, and DOMPurify CDN resources in the static HTML, using the correct SHA-384 hashes for each exact asset and retaining crossorigin="anonymous".providers/gemini.py (1)
78-128: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSynchronize the zero-budget capability cache.
_zero_budget_rejectedis shared mutable state, but_prep_config()and_remember_zero_budget_rejection()read and mutate it without a lock. Make the cache instance-owned and guard all accesses to prevent concurrent requests from racing into redundant failing calls.As per coding guidelines, “Thread safety matters for shared singletons such as embeddings, vector store, BM25 index, and cache; use locks where needed.”
🤖 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 78 - 128, The zero-budget rejection cache is shared and unsynchronized, allowing concurrent requests to race. Make _zero_budget_rejected an instance attribute initialized per provider instance, add an instance lock, and guard every read or mutation in _prep_config and _remember_zero_budget_rejection, including the check-and-add sequence, so only one request performs the fallback discovery.Source: Coding guidelines
tests/test_embeddings_concurrency.py (1)
49-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the waiter-recovery path deterministic.
The owner fails immediately, so this can pass without any thread observing the in-flight event. Block the owner after it registers, wait until a waiter has joined that entry, then release the failure.
🤖 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_embeddings_concurrency.py` around lines 49 - 83, Update test_waiter_recovers_when_owner_fails so the owner blocks after registering its in-flight entry, allowing at least one waiter to join before the owner raises. Coordinate this with threading synchronization primitives and release the owner only after confirming a waiter is observing the same entry, while preserving the assertions that only the owner may fail, waiters recover, and _in_flight is cleared.
🤖 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 `@agent/nodes/answer_generator.py`:
- Around line 68-75: Extend rag.llm_generate() to accept the structured contents
used by the answer node, then update answer_generator’s generation path to call
rag.llm_generate() with the requested model and provider instead of
rag.generate_with_failover(), preserving the existing format_context() and
build_prompt() reuse. In tests/test_agent.py lines 282-327, update assertions to
verify the canonical helper receives the requested model/provider; no direct
failover assertion should remain.
In `@docs/DEPLOYMENT.md`:
- Around line 14-17: Replace the Windows setup command in docs/DEPLOYMENT.md
lines 14-17 with the Command Prompt command “copy .env.example .env” or an
explicit PowerShell alternative, while retaining the Unix command; make the same
platform-specific correction in docs/GEMINI_SETUP.md lines 25-28.
In `@embeddings.py`:
- Around line 145-152: Update the in-flight ownership loop around owns_compute
and wait_event so a waiter whose owner failed re-registers for the same key
before retrying the embed. After the owner removes and signals its entry, have
waiters repeat registration and ownership election, ensuring exactly one
recovery computation proceeds while other waiters continue waiting.
In `@llm_client.py`:
- Around line 93-96: The OpenRouter fallback must not reuse the bare Gemini
model when no selectable OpenRouter slug exists. In llm_client.py lines 93-96,
update the fallback selection to return no model, and adjust _attempts() to omit
that cross-provider attempt; in tests/test_llm_client_dispatch.py lines 68-70,
update the assertion to expect the invalid OpenRouter attempt to be omitted.
In `@PRODUCTION.md`:
- Around line 30-37: Update the Windows service example’s start_server.py
arguments to use port 8080, matching the documented metrics/API endpoint and
start_server.py default; preserve the existing service configuration otherwise.
In `@PROJECT_STRUCTURE.md`:
- Line 3: Update the fenced code block in PROJECT_STRUCTURE.md to specify the
text language by changing the unlabeled fence to ```text, including the matching
closing fence.
In `@providers/gemini.py`:
- Around line 86-88: Restrict _is_invalid_argument and the zero-budget rejection
handling around providers/gemini.py lines 117-121 to errors whose message or
details specifically indicate an unsupported or invalid thinking-budget
configuration, rather than any 400 or INVALID_ARGUMENT response. Preserve
unrelated validation errors as ordinary request failures, and update the
corresponding tests in tests/test_providers_gemini.py lines 72-86 to cover both
thinking-budget rejection and unrelated invalid-argument errors.
In `@routes/agent.py`:
- Around line 341-353: The persistence failure handler around
persistence.log_query in the stream completion path currently suppresses all
errors. Replace the silent except block with the same error logging behavior
used by the non-stream handler around lines 216-217, while preserving the
existing response and query_id flow.
In `@routes/ingest.py`:
- Around line 563-588: Update the filename construction in the permanent-copy
block before `dest` is created so every title-derived `safe_name` includes
`paper_id` as a unique suffix, including after truncation. Preserve the `.pdf`
extension and ensure the fallback path in the exception handler remains
meaningful and distinct.
In `@static/index.html`:
- Around line 2244-2246: Update the `/cache/stats` handling in
`loadDiagnostics()` so a 401 from `handle401(r)` renders the authentication
error in `cacheEl` instead of returning from the function. Then continue
executing the quality and feedback sections so their UI is still rendered and
their spinners are cleared.
- Around line 1595-1611: Remove inline handler interpolation from all three
renderers: static/index.html lines 1595-1611 should use data-file/data-pid with
one delegated listener on `#papersTableBody`; lines 1000-1006 should use data-sid
with a delegated listener on `#wsList`; and lines 1915-1921 should use data-id
with a delegated listener on `#watchGrid`. Update the listeners to dispatch to the
existing ingestOne, dryRunPaper, openEditPaper, deletePaper, openSession,
deleteSession, runWatch, viewDigest, and deleteWatch handlers using dataset
values.
- Around line 1523-1531: Update the fetch call in loadModels to include the
Authorization headers returned by authHeaders(), matching the authentication
used by the other API requests. Preserve the existing model assignment and catch
behavior.
- Around line 2030-2031: Update window.confirmPurge to choose confirmation text
based on the type argument: for type === 'papers', warn that all uploaded PDFs
will be permanently deleted from disk; retain the existing indexed-chunks
message for other purge types. Keep the confirmation cancellation behavior
unchanged.
- Around line 2099-2111: Update deletePaper so the button restoration currently
performed in the catch path also runs when handle401(r) returns early. Move the
btn.disabled and btn.innerHTML restoration into a finally block, preserving the
existing error toast and delete behavior.
- Around line 1828-1831: Update the CSV construction in the dimensions.forEach
export flow to normalize each matrix cell to a string before calling replace,
matching the table renderer’s tolerant handling for numeric or object values.
Preserve the existing N/A fallback and CSV escaping for all cell types.
- Around line 1468-1471: Assign the turn start timestamp to turnDiv._t0 when the
turn is created or begins processing, before the latency calculation in the
asst-meta update. Preserve the existing Date.now()-based elapsed-seconds display
so standard-mode latency reflects the actual turn duration.
- Around line 2266-2267: Replace the inline onclick construction for the “View
full report” button in the evaluation report rendering with a stable id such as
evalFullBtn, then bind its click listener directly using openDocModal. Build the
markdown payload inside the handler from d so quotes or apostrophes in the
report cannot break HTML attributes, while preserving the existing title and
JSON formatting.
In `@tests/test_rag.py`:
- Around line 428-449: Update rag._copy_retrieval_result() to deep-copy nested
metadata dictionaries so caller mutations cannot alter cached results, then
extend tests/test_rag.py lines 428-449 to mutate a nested metadata value and
assert the next cache hit preserves the original; retain the deep-copy claim in
docs/ARCHITECTURE.md lines 388-394 because the implementation will satisfy it.
---
Outside diff comments:
In `@config.py`:
- Around line 281-284: Restore gemini-3.5-flash as the primary default used by
LLM_MODEL_NAME and generate_with_failover(), while retaining it as the first
selectable model in config.py; update the documented defaults and examples in
config.py (281-284, 321-321), docs/DEPLOYMENT.md (154-158), docs/GEMINI_SETUP.md
(30-34, 129-135), and docs/PRODUCTION.md (107-115, 175-188), and update
tests/test_openrouter_config.py (13-13) to assert gemini-3.5-flash.
In `@docs/PRODUCTION.md`:
- Around line 47-75: Update the curl examples in the production documentation
for the English query, Hindi query, health check, and statistics endpoints to
include the required X-API-Key authentication header, matching the production
requirement stated later in the document.
---
Nitpick comments:
In `@providers/gemini.py`:
- Around line 78-128: The zero-budget rejection cache is shared and
unsynchronized, allowing concurrent requests to race. Make _zero_budget_rejected
an instance attribute initialized per provider instance, add an instance lock,
and guard every read or mutation in _prep_config and
_remember_zero_budget_rejection, including the check-and-add sequence, so only
one request performs the fallback discovery.
In `@routes/ingest.py`:
- Around line 24-32: Replace the inline __import__("threading") usage for
_ingest_lock with a standard top-level import threading, then initialize the
lock through that imported module while leaving _inflight_paper_ids unchanged.
In `@static/index.html`:
- Line 1642: Remove the earlier placeholder definition of window.removeSelected
that displays the “coming soon” toast, leaving the real implementation later in
the same IIFE unchanged.
- Around line 11-14: Add SRI integrity hashes to the pinned marked, KaTeX
CSS/JS, and DOMPurify CDN resources in the static HTML, using the correct
SHA-384 hashes for each exact asset and retaining crossorigin="anonymous".
In `@tests/test_embeddings_concurrency.py`:
- Around line 49-83: Update test_waiter_recovers_when_owner_fails so the owner
blocks after registering its in-flight entry, allowing at least one waiter to
join before the owner raises. Coordinate this with threading synchronization
primitives and release the owner only after confirming a waiter is observing the
same entry, while preserving the assertions that only the owner may fail,
waiters recover, and _in_flight is cleared.
🪄 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: 3ab3707f-465d-4715-8fbc-d0a871842dd8
📒 Files selected for processing (37)
.env.examplePRODUCTION.mdPROJECT_STRUCTURE.mdREADME.mdagent/nodes/answer_generator.pyagent/tool_executor.pycache_refresh.pyconfig.pydocs/ARCHITECTURE.mddocs/CONTRIBUTING.mddocs/DEPLOY.mddocs/DEPLOYMENT.mddocs/Eval/evaluate.pydocs/GEMINI_SETUP.mddocs/PDF_UPLOAD_NOTE.mddocs/PRODUCTION.mddocs/QUICKSTART.mddocs/evaluation.mdembeddings.pyllm_client.pypersistence.pyproviders/gemini.pyrag.pyroutes/agent.pyroutes/ingest.pyroutes/query.pystatic/index.htmltests/test_agent.pytests/test_api.pytests/test_embeddings_concurrency.pytests/test_ingest.pytests/test_llm_client_dispatch.pytests/test_openrouter_config.pytests/test_providers_gemini.pytests/test_rag.pyverify.pywatch_runner.py
💤 Files with no reviewable changes (1)
- docs/PDF_UPLOAD_NOTE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- verify.py
- docs/Eval/evaluate.py
- watch_runner.py
- persistence.py
- rag.py
- routes/query.py
Comment on lines
+68
to
+75
| # The user's model choice reaches every other node (planner, selector, | ||
| # evaluator) via state — the node that actually writes the answer must | ||
| # honour it too, or /agent/* reports a model that never produced the text. | ||
| model = state.get("requested_model") or config.LLM_MODEL_NAME | ||
| provider = state.get("requested_provider") | ||
|
|
||
| try: | ||
| resp = rag.generate_with_failover(config.LLM_MODEL_NAME, contents, gen_config) | ||
| resp = rag.generate_with_failover(model, contents, gen_config, provider=provider) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Route agent generation through the shared LLM helper.
The answer node bypasses rag.llm_generate(), and the new tests lock that lower-level call in place. Extend the shared helper to support the structured contents required here, then have the node use it so agent calls share cache and generation behavior.
agent/nodes/answer_generator.py#L68-L75: replace the directrag.generate_with_failover()call with the canonicalrag.llm_generate()path.tests/test_agent.py#L282-L327: assert the canonical helper receives the requested model/provider instead of asserting the direct failover call.
As per coding guidelines, “The agent answer generator must reuse existing rag.format_context(), rag.build_prompt(), and rag.llm_generate() instead of duplicating prompt/context generation logic.”
📍 Affects 2 files
agent/nodes/answer_generator.py#L68-L75(this comment)tests/test_agent.py#L282-L327
🤖 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 68 - 75, Extend
rag.llm_generate() to accept the structured contents used by the answer node,
then update answer_generator’s generation path to call rag.llm_generate() with
the requested model and provider instead of rag.generate_with_failover(),
preserving the existing format_context() and build_prompt() reuse. In
tests/test_agent.py lines 282-327, update assertions to verify the canonical
helper receives the requested model/provider; no direct failover assertion
should remain.
Source: Coding guidelines
Comment on lines
14
to
+17
| ```bash | ||
| # Copy example | ||
| copy .env.example .env | ||
| cp .env.example .env # copy .env.example .env on Windows | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a real Windows copy command.
cp is not a Command Prompt command, so the documented Windows setup fails.
docs/DEPLOYMENT.md#L14-L17: showcopy .env.example .envfor Command Prompt or an explicit PowerShell alternative.docs/GEMINI_SETUP.md#L25-L28: show the same platform-specific command.
📍 Affects 2 files
docs/DEPLOYMENT.md#L14-L17(this comment)docs/GEMINI_SETUP.md#L25-L28
🤖 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/DEPLOYMENT.md` around lines 14 - 17, Replace the Windows setup command
in docs/DEPLOYMENT.md lines 14-17 with the Command Prompt command “copy
.env.example .env” or an explicit PowerShell alternative, while retaining the
Unix command; make the same platform-specific correction in docs/GEMINI_SETUP.md
lines 25-28.
Comment on lines
+145
to
+152
| # Only the owning thread clears the registration. A waiter that fell | ||
| # through after an owner failure must not deregister the owner's entry: | ||
| # doing so lets a later thread start a third redundant embed of the same | ||
| # query, which is the most expensive call on the retrieval path. | ||
| if owns_compute: | ||
| with _in_flight_lock: | ||
| _in_flight.pop(key, None) | ||
| wait_event.set() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Re-register waiters before retrying a failed embed.
After the owner removes and signals its event, every waiter falls through unregistered; a late caller can become a new owner while those waiters also embed the same query. Retry the ownership loop after a failed wait so exactly one recovery computation is elected and the rest wait on 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 `@embeddings.py` around lines 145 - 152, Update the in-flight ownership loop
around owns_compute and wait_event so a waiter whose owner failed re-registers
for the same key before retrying the embed. After the owner removes and signals
its entry, have waiters repeat registration and ownership election, ensuring
exactly one recovery computation proceeds while other waiters continue waiting.
Comment on lines
+93
to
+96
| for model in _config.LLM_SELECTABLE_MODELS: | ||
| if "/" in model: # slug shape == OpenRouter | ||
| return model | ||
| return _config.LLM_MODEL_NAME |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip OpenRouter fallback when no OpenRouter slug exists.
Returning LLM_MODEL_NAME for an OpenRouter fallback creates an OpenRouter attempt with a bare Gemini model, contradicting this function’s contract and routing back to the failed vendor.
llm_client.py#L93-L96: return no fallback model when no/slug exists, and have_attempts()omit that cross-provider attempt.tests/test_llm_client_dispatch.py#L68-L70: assert that the invalid OpenRouter attempt is omitted rather than expecting a bare Gemini fallback.
📍 Affects 2 files
llm_client.py#L93-L96(this comment)tests/test_llm_client_dispatch.py#L68-L70
🤖 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 93 - 96, The OpenRouter fallback must not reuse
the bare Gemini model when no selectable OpenRouter slug exists. In
llm_client.py lines 93-96, update the fallback selection to return no model, and
adjust _attempts() to omit that cross-provider attempt; in
tests/test_llm_client_dispatch.py lines 68-70, update the assertion to expect
the invalid OpenRouter attempt to be omitted.
Comment on lines
+30
to
+37
| - Access the metrics endpoint at: `http://localhost:8080/metrics` | ||
| - Configure your centralized Prometheus scraper to pull from this `/metrics` endpoint. | ||
| - When `API_KEYS` is set, `/metrics` requires the same `X-API-Key` header. Configure the scraper accordingly, e.g.: | ||
| ```yaml | ||
| scrape_configs: | ||
| - job_name: indicrag | ||
| metrics_path: /metrics | ||
| static_configs: [{ targets: ["localhost:8000"] }] | ||
| static_configs: [{ targets: ["localhost:8080"] }] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)PRODUCTION\.md$|api_server\.py$|\.md$' | sed -n '1,120p'
echo
echo "== PRODUCTION relevant sections =="
if [ -f PRODUCTION.md ]; then
sed -n '1,140p' PRODUCTION.md | cat -n
fi
echo
echo "== api_server port references =="
if [ -f api_server.py ]; then
rg -n "port|8080|8099|App\(" api_server.py -C 3
fiRepository: DNSdecoded/IndicRAG
Length of output: 8550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== port definitions in Python =="
rg -n "port[=:\s]|PORT|8080|8000|uvicorn\.run|expose\(" -g '*.py' . | sed -n '1,200p'
echo
echo "== start_server outline and port handling =="
if [ -f start_server.py ]; then
ast-grep outline start_server.py --view compact || true
echo
rg -n "8080|8000|port|uvicorn|api_server|app" start_server.py -C 4
fi
echo
echo "== deploy/service configs port references =="
rg -n "8080|8000|localhost:/127\.0\.0\.1|metrics|/metrics|server_addr|address" -g '*.md' -g '*.yaml' -g '*.yml' -g '*.conf' -g '*.ini' -g '*.service' . | sed -n '1,240p'Repository: DNSdecoded/IndicRAG
Length of output: 20290
Keep the Windows service port aligned with 8080.
start_server.py defaults to 8080, and this production guide now points metrics and the API examples at 8080. The Windows service example still starts with start_server.py --port 8000, so scrape targets using the same production commands would miss the server. Update the service arguments to default to 8080 or document the port mapping explicitly.
🤖 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 `@PRODUCTION.md` around lines 30 - 37, Update the Windows service example’s
start_server.py arguments to use port 8080, matching the documented metrics/API
endpoint and start_server.py default; preserve the existing service
configuration otherwise.
Comment on lines
+2030
to
+2031
| window.confirmPurge = async function(type) { | ||
| if (!confirm('This will delete all indexed chunks. Documents stay. Continue?')) return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Purge confirmation text is wrong for type === 'papers'.
The dialog always says "This will delete all indexed chunks. Documents stay.", but Line 673 calls confirmPurge('papers'), which per the UI copy at Line 672 permanently removes every uploaded PDF from disk. Users confirm an irreversible file deletion while being told their documents are safe.
🛠 Proposed fix
-window.confirmPurge = async function(type) {
- if (!confirm('This will delete all indexed chunks. Documents stay. Continue?')) return;
+window.confirmPurge = async function(type) {
+ const msg = type === 'papers'
+ ? 'This permanently deletes every uploaded PDF from disk. This cannot be undone. Continue?'
+ : 'This will delete all indexed chunks. Documents stay. Continue?';
+ if (!confirm(msg)) return;📝 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.
Suggested change
| window.confirmPurge = async function(type) { | |
| if (!confirm('This will delete all indexed chunks. Documents stay. Continue?')) return; | |
| window.confirmPurge = async function(type) { | |
| const msg = type === 'papers' | |
| ? 'This permanently deletes every uploaded PDF from disk. This cannot be undone. Continue?' | |
| : 'This will delete all indexed chunks. Documents stay. Continue?'; | |
| if (!confirm(msg)) return; |
🤖 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 2030 - 2031, Update window.confirmPurge to
choose confirmation text based on the type argument: for type === 'papers', warn
that all uploaded PDFs will be permanently deleted from disk; retain the
existing indexed-chunks message for other purge types. Keep the confirmation
cancellation behavior unchanged.
Comment on lines
+2099
to
+2111
| window.deletePaper = async function (paperId, btn) { | ||
| if (!confirm('Delete all indexed chunks for "' + paperId + '"? The PDF file stays on disk.')) return; | ||
| const orig = btn ? btn.innerHTML : ''; | ||
| if (btn) { btn.disabled = true; btn.innerHTML = '<span class="spinner"></span>'; } | ||
| try { | ||
| const r = await fetch(API + '/papers/' + encodeURIComponent(paperId), { method: 'DELETE', headers: authHeaders() }); | ||
| if (handle401(r)) return; | ||
| const d = await r.json().catch(() => ({})); | ||
| if (!r.ok) throw new Error(d?.detail || 'Delete failed'); | ||
| showToast('Removed ' + (d.chunks_deleted || 0) + ' chunks', 'success'); | ||
| paperScope = paperScope.filter(p => p !== paperId); updateScopeBar(); | ||
| refreshKB(); | ||
| } catch (e) { showToast(e.message, 'error'); if (btn) { btn.disabled = false; btn.innerHTML = orig; } } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
On a 401 the delete button stays disabled with a spinner forever.
handle401(r) returns early without restoring btn.disabled/btn.innerHTML; only the catch path restores them. Move the restore into a finally (as dryRunPaper and savePaperMeta already do).
🛠 Proposed fix
- } catch (e) { showToast(e.message, 'error'); if (btn) { btn.disabled = false; btn.innerHTML = orig; } }
+ } catch (e) { showToast(e.message, 'error'); }
+ finally { if (btn) { btn.disabled = false; btn.innerHTML = orig; } }📝 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.
Suggested change
| window.deletePaper = async function (paperId, btn) { | |
| if (!confirm('Delete all indexed chunks for "' + paperId + '"? The PDF file stays on disk.')) return; | |
| const orig = btn ? btn.innerHTML : ''; | |
| if (btn) { btn.disabled = true; btn.innerHTML = '<span class="spinner"></span>'; } | |
| try { | |
| const r = await fetch(API + '/papers/' + encodeURIComponent(paperId), { method: 'DELETE', headers: authHeaders() }); | |
| if (handle401(r)) return; | |
| const d = await r.json().catch(() => ({})); | |
| if (!r.ok) throw new Error(d?.detail || 'Delete failed'); | |
| showToast('Removed ' + (d.chunks_deleted || 0) + ' chunks', 'success'); | |
| paperScope = paperScope.filter(p => p !== paperId); updateScopeBar(); | |
| refreshKB(); | |
| } catch (e) { showToast(e.message, 'error'); if (btn) { btn.disabled = false; btn.innerHTML = orig; } } | |
| window.deletePaper = async function (paperId, btn) { | |
| if (!confirm('Delete all indexed chunks for "' + paperId + '"? The PDF file stays on disk.')) return; | |
| const orig = btn ? btn.innerHTML : ''; | |
| if (btn) { btn.disabled = true; btn.innerHTML = '<span class="spinner"></span>'; } | |
| try { | |
| const r = await fetch(API + '/papers/' + encodeURIComponent(paperId), { method: 'DELETE', headers: authHeaders() }); | |
| if (handle401(r)) return; | |
| const d = await r.json().catch(() => ({})); | |
| if (!r.ok) throw new Error(d?.detail || 'Delete failed'); | |
| showToast('Removed ' + (d.chunks_deleted || 0) + ' chunks', 'success'); | |
| paperScope = paperScope.filter(p => p !== paperId); updateScopeBar(); | |
| refreshKB(); | |
| } catch (e) { showToast(e.message, 'error'); } | |
| finally { if (btn) { btn.disabled = false; btn.innerHTML = orig; } } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 2110-2110: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: btn.innerHTML = orig
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(inner-outer-html)
🤖 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 2099 - 2111, Update deletePaper so the button
restoration currently performed in the catch path also runs when handle401(r)
returns early. Move the btn.disabled and btn.innerHTML restoration into a
finally block, preserving the existing error toast and delete behavior.
Comment on lines
+2244
to
+2246
| try { | ||
| const r = await fetch(API + '/cache/stats', { headers: authHeaders() }); | ||
| if (handle401(r)) return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A 401 on /cache/stats aborts the rest of loadDiagnostics().
return here skips the quality and feedback sections, leaving their spinners/empty containers behind. Prefer rendering an error into cacheEl and continuing.
🤖 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 2244 - 2246, Update the `/cache/stats`
handling in `loadDiagnostics()` so a 401 from `handle401(r)` renders the
authentication error in `cacheEl` instead of returning from the function. Then
continue executing the quality and feedback sections so their UI is still
rendered and their spinners are cleared.
Comment on lines
+2266
to
+2267
| '<div class="stat"><span class="sv mono">' + esc(typeof v === 'number' ? (Number.isInteger(v) ? v : v.toFixed(3)) : v) + '</span><span class="sl">' + esc(k) + '</span></div>').join('') + '</div>' + | ||
| '<div style="margin-top:8px"><button class="btn" onclick=\'openDocModal("Eval report", "```json\\n" + ' + JSON.stringify(JSON.stringify(d, null, 2)) + ' + "\\n```")\'>View full report</button></div>'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"View full report" button is broken — openDocModal is not exposed globally.
openDocModal is a plain function inside the IIFE (Line 2043); only closeDocModal and downloadDocModal are attached to window. Inline onclick attributes execute in global scope, so this throws ReferenceError: openDocModal is not defined. Building the handler by string-embedding JSON into a single-quoted attribute is also fragile — a ' in the eval payload breaks the attribute.
🛠 Proposed fix — bind the handler instead of stringifying it
- '<div style="margin-top:8px"><button class="btn" onclick=\'openDocModal("Eval report", "```json\\n" + ' + JSON.stringify(JSON.stringify(d, null, 2)) + ' + "\\n```")\'>View full report</button></div>';
+ '<div style="margin-top:8px"><button class="btn" id="evalFullBtn">View full report</button></div>';
+ $('`#evalFullBtn`').addEventListener('click', () =>
+ openDocModal('Eval report', '```json\n' + JSON.stringify(d, null, 2) + '\n```'));🤖 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 2266 - 2267, Replace the inline onclick
construction for the “View full report” button in the evaluation report
rendering with a stable id such as evalFullBtn, then bind its click listener
directly using openDocModal. Build the markdown payload inside the handler from
d so quotes or apostrophes in the report cannot break HTML attributes, while
preserving the existing title and JSON formatting.
Comment on lines
+428
to
+449
| def test_retrieval_cache_hit_returns_independent_lists(): | ||
| """A caller trimming the returned lists must not corrupt the cached entry.""" | ||
| import config | ||
| import rag | ||
| from cache import retrieval_cache | ||
|
|
||
| retrieval_cache.invalidate() | ||
| # side_effect (not return_value): a shared mock dict would let the first | ||
| # caller's mutation come back through the mock itself, hiding the real | ||
| # question — whether the *cache* handed out an aliased list. | ||
| with patch("vector_store.search", side_effect=lambda **kw: _fake_search_results(4, 0)), \ | ||
| patch("vector_store.get_or_create_collection", return_value=object()), \ | ||
| patch("embeddings.embed_query", return_value=[0.0]), \ | ||
| patch.object(config, "USE_HYBRID_SEARCH", False), \ | ||
| patch.object(config, "USE_RERANKER", False), \ | ||
| patch.object(config, "USE_COLBERT_RERANK", False): | ||
| first = rag.retrieve_context("cache probe", top_k=4) | ||
| del first["chunks"][:] # hostile caller | ||
| second = rag.retrieve_context("cache probe", top_k=4) | ||
|
|
||
| assert len(second["chunks"]) == 4, "cache entry was mutated by a previous caller" | ||
| retrieval_cache.invalidate() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Protect cached metadata from caller mutation.
rag._copy_retrieval_result() copies result lists but leaves each metadata dictionary shared. Mutating result["metadatas"][0] therefore poisons later cache hits, despite the documentation claiming deep-copy isolation.
tests/test_rag.py#L428-L449: mutate a nested metadata value and assert the next cache hit retains the original value.docs/ARCHITECTURE.md#L388-L394: retain the deep-copy claim only after deep-copying cached nested values; otherwise document the actual shallow-copy behavior.
📍 Affects 2 files
tests/test_rag.py#L428-L449(this comment)docs/ARCHITECTURE.md#L388-L394
🤖 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_rag.py` around lines 428 - 449, Update
rag._copy_retrieval_result() to deep-copy nested metadata dictionaries so caller
mutations cannot alter cached results, then extend tests/test_rag.py lines
428-449 to mutate a nested metadata value and assert the next cache hit
preserves the original; retain the deep-copy claim in docs/ARCHITECTURE.md lines
388-394 because the implementation will satisfy it.
Security
- deps.verify_admin_key is now fail-closed. It fell back to verify_api_key,
so any ordinary API_KEYS entry authorized /purge/*, and with API_KEYS
unset (ALLOW_UNAUTHENTICATED) the destructive routes were anonymous.
A dedicated ADMIN_API_KEY is now mandatory; without one they 403.
- Compare jobs are scoped to the submitting key. _jobs is a global store,
so GET /compare/status/{job_id} returned any authenticated caller the
full result of someone else's comparison. Jobs now carry a sha256
fingerprint of the submitter's key (deps.current_owner) and a mismatch
returns 404 rather than 403, so the response doesn't confirm the id.
- static/index.html: SRI integrity hashes on the pinned marked, KaTeX and
DOMPurify CDN assets, plus a fail-closed guard in renderContent — with
SRI a tampered response means DOMPurify never loads, so the renderer
now falls back to escaped text instead of emitting unsanitized HTML.
Correctness / quality
- providers/gemini: guard the shared _zero_budget_rejected cache with a
lock; the check-then-add raced between concurrent requests.
- tests/test_embeddings_concurrency: the owner failed before any waiter
reached the in-flight event, so the recovery path could go unexercised.
Owner now blocks until two waiters are parked on the event.
- routes/ingest: plain `import threading` instead of __import__.
- static/index.html: drop the dead "coming soon" removeSelected stub that
was shadowed by the real implementation later in the same IIFE.
- docs/PRODUCTION: quick-start curl examples now send X-API-Key.
- .env.example / README / start_server: document the fail-closed admin
behavior (the startup warning claimed a fallback that no longer exists).
New tests/test_auth_scoping.py covers both authorization fixes.
258 tests pass, 4 deselected.
Not applied: the request to revert LLM_MODEL_NAME to gemini-3.5-flash.
No guideline file in this repo pins that model, and 3.6-flash is the
deliberate default from e29b72e/761fa60. The failover contract the
finding refers to is satisfied by LLM_SELECTABLE_MODELS, which still
lists gemini-3.5-flash plus a "/" slug for cross-vendor failover.
Owner
Author
|
Addressed the outstanding review findings in 7614c57. Security
Correctness / quality
Not applied
258 tests pass, 4 deselected (integration/network). |
Pre-release audit found two gaps in the deployment docs: - The backup instructions covered chroma_db/ and papers/ but not sessions.db, which holds all user state (sessions, chat history, feedback, watches, saved reports, jobs). Restoring from the documented set would have lost everything except the corpus. Uses `sqlite3 .backup` rather than cp, since the DB runs in WAL mode. - There was no rollback path documented for a release this size. Added one, including what does and does not roll back cleanly: the schema is additive (CREATE TABLE IF NOT EXISTS, no ALTER) so an older release tolerates a newer DB, but there is no schema version marker to rely on. Also documents the one breaking change in 2.4.0: /purge/* is fail-closed and requires a dedicated ADMIN_API_KEY, so automation that purged with an ordinary API_KEYS entry needs updating before deploy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release 2.4.0. Three phases: feature work, a correctness pass over the whole RAG path, and a documentation audit.
Features
config.py,.env.example, and docs.LLM_SELECTABLE_MODELSkeeps 3.5-flash plus at least onevendor/modelslug, which cross-vendor failover depends on.SESSION_MAX_AGE_HOURS(eviction) andCHAT_HISTORY_MAX_TURNS(history pruning).GET /quality.Correctness fixes (
4cef417)Root-caused rather than patched at the call site:
verify.check_claimsfaithfulness tuple was unpacked wrong.thinking_budget=0for some models — now retries once withoutthinking_config; streaming only retries if nothing was emitted yet.collection/filter_dictbypasses it).top_k * TAGS_OVERFETCH(capped atTAGS_OVERFETCH_MAX).google/<model>, so_fallback_model_fornow picks the first/-shaped slug.Security fixes from review (
7614c57)/purge/*is fail-closed.verify_admin_keyfell through toverify_api_key, so any ordinaryAPI_KEYSentry authorized destructive routes, and withAPI_KEYSunset (ALLOW_UNAUTHENTICATED=1) they accepted anonymous requests. A dedicatedADMIN_API_KEYis now mandatory; without one those routes 403 for everyone._jobsis a global store, soGET /compare/status/{job_id}returned any authenticated caller another user's full result. Jobs now carry a sha256 fingerprint of the submitting key; a mismatch returns 404, not 403, so the response doesn't confirm the id exists.renderContent— SRI turns a tampered response into "DOMPurify never loads", which without the guard would mean unsanitized HTML._zero_budget_rejectedcheck-then-add inproviders/gemini.py.Documentation (
780c515)Every live doc was audited against actual routes, config defaults, and repo layout:
config.py; per-view Web UI section.ARCHITECTURE.md— "offline-first" corrected to local-first retrieval (generation is remote); fabricated model lists and 20-30GB memory figures replaced; new caching/filtering and dispatch/failover sections.QUICKSTART.mdrewritten;DEPLOY.mdcollapsed to a pointer (it duplicatedDEPLOYMENT.md);PRODUCTION.md,GEMINI_SETUP.md,CONTRIBUTING.md,evaluation.md,PROJECT_STRUCTURE.mdall corrected.docs/PDF_UPLOAD_NOTE.md, which claimed the upload UI didn't exist.Test plan
pytest tests/ -m "not integration and not network"— 258 passed, 4 deselectedtests/test_auth_scoping.pycovers both authorization fixestests/test_embeddings_concurrency.pypins the in-flight dedup contract, including owner-failure recoveryconfig.pyattribute or anos.getenvread; all relative Markdown links resolve7614c57—ruff check ., unit tests, and the eval gate all pass in GitHub ActionsReview notes
Two CodeRabbit findings were not applied, with reasoning in this comment:
e29b72e.config.py,.env.example, and README all say 20, and no fallback of 10 exists in the tree.