Uh oh!
There was an error while loading. Please reload this page.
fix(ocr): review follow-ups for #420 — agent seam, cost ceiling, cache key, sequential rescuers - #424
Conversation
The vision OCR call built a raw genai.Client and invoked generate_content directly. Three reasons that is wrong here, the third load-bearing: - CLAUDE.md: new LLM-driven code belongs in backend/agents/ as a Pydantic AI agent, not a fresh client. - ADR-0008 made agents/_providers.py::model_for(task) the one place a model is chosen. GEMINI_VISION_OCR_MODEL was a competing knob that bypassed it; the slot is now SAPLING_MODEL_OCR_VISION like every other agent's. - Cost attribution. Logfire's instrument_pydantic_ai() tags every pydantic-ai span with tokens and USD; a raw client call is invisible to it and to the usage capture #118/PR #375 is building. Vision OCR is one metered call per scanned page — plausibly the largest per-document LLM spend in the app, and it would have been the one call the new cost dashboard could not see. The run is bounded by WORKER_LIMITS: it sits in a per-page loop, where an unbounded run multiplies a single runaway page across the whole document. Also fixes a latent bug this refactor surfaced. _extract_text_or_422 is sync but called from both async handlers (routes/documents.py:640, :771), so a bare asyncio.run raises there — and _apply_gemini_vision_fallback's per-page `except Exception: continue` would have swallowed it, silently turning vision OCR into a no-op on the main upload path. _run_from_anywhere hands the coroutine to a worker thread when a loop is already running, copying the context so agent.override and the active span survive. The module contract is unchanged: same function name and signature, same GeminiVisionUnavailableError semantics, GEMINI_VISION_OCR_ENABLED still the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four findings from the review of #420. Cache key omitted the model. _ocr_cache_key claimed to include "every flag that changes the output" but not the vision model, so switching models kept serving the old transcription for the full 30-day TTL. Model and page cap are now in the key, mixed in only when vision is enabled so the vision-off majority keeps its existing entries. GOT_OCR_MODEL_PATH has the same pre-existing gap; the docstring now names it instead of overclaiming. No cost ceiling. Each flagged page is one metered call, and nothing upstream bounds the count: routes/extract.py allows min(max_pages, 50) and the upload path has no rate limit at all. The #182 limit (10 req/60s) was sized when a request meant one bounded local OCR run. GEMINI_VISION_OCR_MAX_PAGES caps it per document, default 10, and logs how many pages it left behind — a silent cap reads downstream as a full transcription. if/elif made the rescuers mutually exclusive. Enabling both meant vision never ran, including on pages GOT-OCR failed to fill, recreating the exact "signal computed then dropped" bug this feature exists to fix. They now run in sequence — GOT-OCR first (local, free), then vision over what it could not fill. Both share one driver; GOT-OCR's gate is byte-for-byte unchanged. Three false claims. .env.example said an unreadable scan "is rejected" — it is not on this base; the upload paths convert only extraction *exceptions* to 422, so "" reaches the classify prompt and the model fabricates. That rejection is PR #419, still open. The module docstring said vision applies to "any engine"; it needs Docling to have run and succeeded. And the cache comment cited ADR 0019 (actually the SAPLING_MODEL_MODE test seam) for content-addressed chunk ids, whose dedup claim is untrue on main and becomes true only under PR #352. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 6a6c555 | Commit Preview URL Branch Preview URL | Jul 28 2026, 03:57 AM |
The CI pytest step ignored four files. Three genuinely need what requirements.lock deliberately excludes: transformers (test_extraction_backends), docling (test_docling_integration), live network (test_ocr_pipeline). test_extraction_service.py needs none of them — it stubs every backend it exercises. It was swept into the list with its heavy neighbours, and the consequence is that nothing in it has ever gated a PR: not the OCR engine gating, not the content-addressed cache key (#97), and not the cost ceiling and rescuer sequencing added alongside this change. #420's own fallback and cache-key tests were ungated for the same reason. Verified against the locked (non-OCR) dependency set CI actually installs, using CI's exact command and env: 1069 passed, 23 skipped, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230
commented
Jul 28, 2026
Locked-deps verificationRan the branch against a throwaway venv built from The pydantic-ai API surface held. What it did surface: Verified with CI's exact command and env on the locked set: The 4 Still unverifiedNo real Gemini vision call has been made anywhere. The prompt reshape ( |
Caught by the first real Gemini call anyone has made against this feature.
Moving the instruction to `system_prompt` during the agent refactor changed
what the model produces. Measured on a rasterized syllabus with known ground
truth (231 chars of source text, 0-char text layer):
prompt as system_prompt -> 743 chars: \documentclass{article}, five
\usepackage lines, \begin{document}, a
tabular, \end{document}
prompt in the user turn -> 359 chars: clean Markdown table
Both transcribe the facts correctly — every assignment, date and type
matches. The difference is that as a system prompt, "Use LaTeX for
mathematics" reads as a document-format directive rather than an instruction
about math notation, so the model emits a whole LaTeX file.
The preamble is not cosmetic. extracted_text feeds the classify, summary and
concept prompts and is chunked into course_chunks for RAG, so "amsmath" and
"booktabs" become candidate concepts on a graph shared by every student in
the course — the same pollution this feature exists to prevent, arriving by a
different door.
Restores the wire shape the original raw-client implementation used
(contents=[image, prompt]), verified to produce 358 chars of clean Markdown
on the same fixture. The agent seam, the ADR-0008 model slot and the cost
attribution are all unaffected — only the placement changes.
The test now pins placement in the user turn and asserts the instruction is
absent from any system prompt. Revert-proof: reintroducing system_prompt
fails it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>AndresL230
commented
Jul 28, 2026
First real Gemini call found a regression I introducedBuilt a scanned-PDF fixture by rasterizing Then made one real vision call. The facts came back perfect: every assignment, due date and type matched. But the shape did not.
As a system prompt, "Use LaTeX for mathematics" reads as a document-format directive rather than an instruction about math notation, so the model emits an entire LaTeX file. I introduced this in the agent refactor; the original raw-client implementation did not have it. Why the preamble matters: Fixed in 9252497 by restoring 62 passed on locked deps, ruff clean. This is the concrete argument for keeping a scanned-PDF fixture in the repo — the entire unit suite passed against this bug, because a FunctionModel will happily return whatever you tell it to. |
AndresL230
commented
Jul 28, 2026
Live stack results — including a correctionRan the real thing: local Supabase (main's Confirmed working
Correction: that upload did NOT exercise vision OCRDocling ships RapidOCR and read the rasterized page itself — 341 chars, Worth knowing precisely when this feature actually spends money. ifchar_count<LOW_CHAR_THRESHOLDormath_flag: # threshold = 40So vision fires only when RapidOCR also fails (<40 chars) or the page has math-shaped content that isn't LaTeX. That matches #419's evidence, where all 13 pages of a handwritten linear-algebra exam were flagged — RapidOCR handles clean rasterized text fine and fails on handwriting. Consequence: a plain scanned text PDF costs nothing, which is the intended design — but it also means the fixture needed to regression-test this path must contain handwriting or unLaTeXed math, not just a missing text layer. Still openNo end-to-end run has pushed a document through the vision branch of the pipeline. Both halves are proven independently (real model + real loop; Docling flagging + rescue sequencing under unit test), but not composed. That needs a genuinely hard scan as a fixture. |
…d loop Found by the live test added here, which is the only thing that could have found it: every other test in this feature substitutes the model, and a FunctionModel has no client and no event loop. Measured against the live API, calling the seam four times in one process: call 1: OK 302 chars call 2: RuntimeError: Event loop is closed call 3: OK 308 chars call 4: RuntimeError: Event loop is closed `_providers._provider` is a module-level GoogleProvider, so its async httpx client binds to the first loop `asyncio.run` creates and dies when that loop closes. Every `run_agent_sync` caller shares this — it is #354, and the sweep is still open in PR #358. Transcription is the only caller that runs in a LOOP, which turns a latent bug into an unusable feature: a 10-page scan alternates success and failure page by page, and `_apply_gemini_vision_fallback`'s per-page `except Exception: continue` keeps Docling's text without a word. Half a document silently degrades to the mangled OCR this feature exists to replace. So this path does not wait for #358. `fresh_ocr_vision_model()` builds a provider per run and is passed as a per-run `model=` override, leaving the shared `_provider` untouched so it cannot conflict with whatever #358 lands. It returns None outside SAPLING_MODEL_MODE=real, where the FunctionModel has no loop affinity and must not be overridden. Four consecutive live calls now pass. The fixture is an image-only math worksheet. A missing text layer alone is not enough to reach vision — Docling ships RapidOCR and reads rasterized prose fine. This page is reached because `_detect_math_without_latex` flags math-shaped content carrying no LaTeX, the scanned-math case the feature is for. Docling alone drops problem 3 entirely as `<!-- formula-not-decoded -->`; with vision it comes back as `$\sqrt{x^2 + 16} \leq 5$`. Tests live in the `live_llm` lane, not tests/integration/: they need Docling and a real model, not Postgres, and that lane's conftest mandates a running Supabase stack. Opt-in via RUN_LIVE_OCR=1 plus a real key; skipped otherwise, so CI's dummy key is a clean skip. One test guards the premise and fails loudly if Docling ever stops flagging the fixture, since the other two would then pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230
commented
Jul 28, 2026
The live test found a second, worse bugRan the composed path for real. It works — and in doing so it exposed something no mocked test could reach. Calling the vision seam four times in one process:
Transcription is the only caller that runs in a loop, which turns a latent bug into an unusable feature: a 10-page scan alternates success/failure page by page, and Fixed in 6a6c555 with a per-run provider passed as a The composed path, verifiedSame code, same fixture, only the flag differs:
Docling losing an entire problem is the point — content it drops never reaches the concept prompt, so it never reaches the student's graph. A full upload through the async handler classified it About the fixtureA missing text layer is not enough to reach vision. Docling ships RapidOCR and reads rasterized prose perfectly — my first fixture (a rasterized syllabus) came back clean with Tests are in the Verification: live lane |
Uh oh!
There was an error while loading. Please reload this page.
Follow-ups from the review of #420. Targets
worktree-gemini-vision-ocrso you can take them as a discrete diff rather than having your branch rewritten under you.Two commits, disjoint file sets.
1. Route vision transcription through a Pydantic AI agent
The backend built a raw
genai.Clientand calledgenerate_contentdirectly. Three reasons that's wrong here, and the third is the one that matters:backend/agents/."agents/_providers.py::model_for(task)the single place a model is chosen.GEMINI_VISION_OCR_MODELwas a competing knob that bypassed it — the slot is nowSAPLING_MODEL_OCR_VISION, like every other agent.instrument_pydantic_ai()tags every pydantic-ai span with tokens and USD. Logfire went live on main last week ([P2] Observability: activate Logfire for ops/error/LLM tracing #119/feat(observability): activate Logfire ops/error/LLM tracing (#119) #406) and feat(observability): capture LLM token usage + cost (#118) #375 is building usage capture for [P2] Observability: capture LLM token usage + cost (agents + Gemini) #118. A raw client call is invisible to both. Vision OCR is one metered call per scanned page — plausibly the largest per-document LLM spend in the app — so it would have been the single call the new cost dashboard couldn't see.The run is bounded by
WORKER_LIMITS, matching the #329/#345 sweep: an unbounded run inside a per-page loop multiplies one runaway page across the whole document.Latent bug this surfaced.
_extract_text_or_422is sync but called from bothasync defhandlers (routes/documents.py:640,:771), so a bareasyncio.runraises there — and_apply_gemini_vision_fallback's per-pageexcept Exception: continuewould have swallowed it. Vision OCR would have silently done nothing on the main upload path._run_from_anywherehands the coroutine to a worker thread when a loop is already running, copying the context soagent.overrideand the active span survive. Pinned by a test.Module contract unchanged: same function name and signature, same
GeminiVisionUnavailableErrorsemantics,GEMINI_VISION_OCR_ENABLEDstill the switch.2. Cache key, cost ceiling, sequential rescuers, accurate docs
_ocr_cache_keyclaimed to include "every flag that changes the output" but not the vision model, so switching models kept serving the old transcription for the full 30-day TTL. Model and page cap now in the key, mixed in only when vision is enabled so vision-off deployments keep their entries.GOT_OCR_MODEL_PATHhas the same pre-existing gap — the docstring now names it rather than overclaiming.routes/extract.pyallowsmin(max_pages, 50), the upload path has no rate limit at all, and /api/extract/pdf and /api/extract/image are unauthenticated, unbounded OCR endpoints with no rate limit #182's 10 req/60s was sized when a request meant one bounded local OCR run.GEMINI_VISION_OCR_MAX_PAGEScaps it per document (default 10) and logs what it skipped.if/elifmade the rescuers mutually exclusive. Enabling both meant vision never ran — including on pages GOT-OCR failed to fill, recreating the exact "signal computed then dropped" bug this feature exists to fix. They now run in sequence, GOT-OCR first (local, free), then vision over what it couldn't fill. GOT-OCR's gate is byte-for-byte unchanged..env.examplesaid an unreadable scan "is rejected" — not on this base; the upload paths convert only extraction exceptions to 422, so""reaches the classify prompt and the model fabricates. That rejection is fix(documents): reject empty text extraction instead of fabricating a summary #419, still open. The module docstring said vision works on "any engine"; it needs Docling to have run and succeeded. And the cache comment cited ADR 0019 (actually theSAPLING_MODEL_MODEtest seam) for content-addressed chunk ids, whose dedup claim is untrue on main and becomes true only under fix(rag): content-hash chunk ids — dedup identical uploads per course #352.Verification
1099 passed, 23 skippedon the full backend suite;ruff checkclean. The one error (test_ocr_pipeline.py::test_save_to_db, event-loop teardown in a live-network test) is pre-existing — I reproduced it identically on unmodifiedfe1fce6.Worth your call
routes/documents.py'smax_pagesif you'd rather nothing truncates in the default window — at the cost of a 2x worse ceiling on/api/extract.""is now implemented by catchingUnexpectedModelBehavior, sinceoutput_type=strmakes pydantic-ai retry once then raise. Same document-level behaviour as before, but it does swallow that exception class. Quota and network errors still propagate.system_promptwith the image as the user message — a different wire shape thancontents=[image, prompt]. Worth one live-model spot check before enabling in prod.🤖 Generated with Claude Code