Uh oh!
There was an error while loading. Please reload this page.
fix(agents): guard run_agent_sync against running event loops (#354 follow-up) - #358
Merged
Merged
Conversation
Warning Review limit reached
Next review available in:51 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR centralizes the Gemini health-probe model name and updates the health agent to use it. It also hardens ChangesAgent runtime hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
7 tasks
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 10372df | Commit Preview URL Branch Preview URL | Jul 29 2026, 07:52 AM |
Darkest-Teddy
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the full diff, all PR comments (CodeRabbit ran Free-plan summary-only; no line findings), and ran the 3 affected test files locally in an isolated worktree — 48 passed, and CI is green on this head. The #354 fresh-client sweep, the run_agent_sync loop-guard, and the #355 subject-root dedup are all correct and well-covered (the AST guard in test_fresh_client_sweep.py is a nice revert-proof check). No bugs found; the two notes below are minor and non-blocking.
| live request loop, sidesteps the cross-loop connection reuse. | ||
| """ | ||
| provider = GoogleProvider(api_key=GEMINI_API_KEY or "dummy-key-for-import") | ||
| return GoogleModel(name, provider=provider) |
Collaborator
There was a problem hiding this comment.
Nit (resource hygiene, non-blocking): each call builds a brand-new GoogleProvider -> google.genai/httpx client + connection pool that is never explicitly closed. For the 7 run_agent_sync sites the asyncio.run loop teardown reclaims the connections, but the one awaited path (routes/documents.py::_extend_via_agent, concept_scan) leaves an unclosed AsyncClient on the live request loop for GC to reap. These are low-frequency ops so real-world impact is small and the correctness win clearly justifies losing connection reuse — just flagging that under load this can accumulate unclosed clients / socket-cleanup churn. No change required to merge.
| health_probe_agent.run('Reply with exactly the text: Gemini OK') | ||
| health_probe_agent.run( | ||
| 'Reply with exactly the text: Gemini OK', | ||
| model=fresh_google_model("gemini-2.5-flash-lite"), |
Collaborator
There was a problem hiding this comment.
Nit (DRY, non-blocking): the model name "gemini-2.5-flash-lite" is hardcoded here and again in agents/health.py where health_probe_agent is constructed. They must stay in lockstep by hand; a shared constant (or a fresh_model_for-style helper for the probe) would prevent drift if the probe model ever changes.
Darkest-Teddy added a commit
that referenced
this pull request
Jul 22, 2026
Hoist the admin gemini-test probe's model name into a single HEALTH_PROBE_MODEL constant in agents/_providers.py, referenced by both the agent default (agents/health.py) and the fresh-client override (main.py). Previously "gemini-2.5-flash-lite" was hardcoded in both places and had to be kept in lockstep by hand. Addresses PR #358 review nit. No behavior change; resolved model name is identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Darkest-Teddy
commented
Jul 22, 2026
Collaborator
Follow-up on my two review nits: Nit #2 (DRY) — fixed in 08a2430. Hoisted the probe model name into a single Nit #1 (client hygiene) — retracted, leaving as-is. On closer inspection I overstated this. I claimed the concept_scan path leaves an unclosed client on the live request loop, but Both items resolved. 🤖 Generated with Claude Code |
AndresL230 added a commit
that referenced
this pull request
Jul 28, 2026
…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>
This was referenced Jul 28, 2026
AndresL230 added a commit
that referenced
this pull request
Jul 29, 2026
…ed' flake (#436, #354) (#453) * fix(agents): make the shared Gemini provider loop-safe, not a sweep (#436, #354) test_ocr_pipeline.py::test_save_to_db errored with "RuntimeError: Event loop is closed" on main — the #354 root cause: agents/_providers.py's module-level GoogleProvider eagerly builds an httpx.AsyncClient whose connection pool binds internal asyncio primitives to whichever event loop is running the first time a request goes out over it. Every agent is built once at import time sharing that one provider, so run_agent_sync's per-call asyncio.run() (and, just as much, any test calling asyncio.run() more than once against the same shared agent in one process, as test_agent_parse then the parsed_assignments fixture do here) trips the stale-loop reuse on every second real call. PR #358 (never merged; reviewed clean, just fell through the cracks) fixed this by sweeping eight run_agent_sync call sites to pass a fresh-provider model= override per call. Adapting it verbatim wouldn't have fixed#436: calendar_service's syllabus_extraction_agent, the actual caller behind this test, isn't on that sweep list — and agents/ocr_vision.py already needed its own ad hoc copy of the same idea for a path #358 didn't cover, proof the sweep needs rediscovering at every new call site. The issue itself named this "the tactical sweep" with "make the shared provider loop-safe" as the strategic fix. Since every agent gets its model from model_for()/google_model() exactly once, fixing those two functions fixes every caller — present and future — with no sweep required. _LoopSafeGoogleModel (a GoogleModel subclass) keeps one GoogleProvider per currently-running event loop (a WeakKeyDictionary keyed by the loop object, self-cleaning once a throwaway loop is GC'd), rebuilding only when a NEW loop calls it and reusing the cached one for as long as that loop lives — identical connection-pooling behavior to the old singleton for FastAPI's one persistent per-process loop, and no stale-loop reuse for run_agent_sync's or a test's disposable ones. This also makes ocr_vision.py's ad hoc fresh_ocr_vision_model() workaround redundant; removed it and its call-site override in gemini_vision_backend.py. Proof: tests/test_loop_safe_google_model.py (new, hermetic) pins the per-loop cache directly. tests/test_ocr_pipeline.py run 3x in one process (pytest.main() loop, since repeated identical CLI paths dedup to a single collection) put 6 asyncio.run() cycles through the same shared agent — 33/33 green, zero "Event loop is closed". Full suite green twice back-to-back (1161 passed, 26 skipped, 0 errors both times). ruff check clean. Fixes#436Fixes#354 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agents): eliminate the loop-safe provider's shared-pointer race (#436, #354) PR review on the prior fix found a Critical concurrency bug, backed by an empirical repro: 6 threads x 20 sequential asyncio.run calls against one shared _LoopSafeGoogleModel, with an asyncio.sleep standing in for the real await gap, produced 95/120 mismatches between the provider bound for a call and the one actually read back. Root cause: _bind_to_current_loop() resolved the right provider under a lock, but handed it off via `self._provider = provider` — a single shared mutable attribute on a Model instance that is itself a process-wide singleton (every agent is built once at import time). GoogleModel._generate_ content reads self.client (-> self._provider.client) only AFTER an await (self._build_content_and_config(...)); in that window, a different thread running a different event loop — exactly what happens under concurrent requests, since every sync-def route drives run_agent_sync on a fresh thread + throwaway loop, and gemini_vision_backend._run_from_anywhere does the same for OCR — could rebind that same shared attribute. The first call would then resume and read a provider bound to someone else's, possibly already-closed, loop: a deterministic every-second-call flake became a probabilistic, load-dependent one. Fix: remove the hand-off entirely. self._provider (the base GoogleModel/ Model attribute) is now a fixed template, set once and never reassigned, backing only the reads that are genuinely loop-independent (system/base_url, and the bare .name/.base_url pydantic-ai's own count_tokens/usage-metadata code reads directly) — safe because every provider this module constructs uses identical arguments. .client — the one read that IS loop-affine — is now a property that resolves asyncio.get_running_loop() -> the loop-keyed WeakKeyDictionary fresh, at the exact moment of every access, with no instance-attribute write in between "resolve" and "use". Every inherited GoogleModel method reads self.client through ordinary attribute lookup, so this one override covers all of them — request/count_tokens/request_stream no longer need (and no longer have) their own overrides. __aenter__/ __aexit__ still act on the current loop's provider explicitly. Verified standalone: the buggy hand-off shape reproduces 119/120 mismatches; the fixed property-based design shows 0/120, both under the same 6x20 stress. Added TestConcurrentAccessIsRaceFree to tests/test_loop_safe_google_model.py: a minimal stand-in of the original hand-off design proves the test shape itself would have caught the round-0 bug (asserts it DOES mismatch), then the same shape run against the real _LoopSafeGoogleModel asserts zero mismatches. Deterministic and hermetic — no network. agents/ocr_vision.py and gemini_vision_backend.py needed no further changes: they already call through model_for("ocr_vision")'s default model, so the OCR per-page-loop concurrency concern the review raised falls out of this fix directly. Re-verified: threaded test suite 5x back-to-back (9/9 every time), the OCR pipeline module 3x in one process (pytest.main() loop, 33/33 green), full hermetic suite once (1164 passed, 26 skipped, 0 errors). ruff check clean. Fixes#436Fixes#354 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ollow-up) Reworked from the original fresh-client sweep: the cross-loop client problem is now solved by _LoopSafeGoogleModel (#453), so the per-call fresh-client plumbing and the subject_root dedup (landed separately, #355) are dropped. What remains is the piece main still lacks: - run_agent_sync detects a running event loop, closes the handed coroutine (no 'never awaited' warning) and raises a clear error the try/except-guarded sync-from-async callers can degrade on, instead of letting asyncio.run raise opaquely. - HEALTH_PROBE_MODEL constant so probe sites can't drift. - Regression tests for both loop paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230force-pushed
the
claude/sapling-pr-349-followups-ypjqh3
branch
from
July 29, 2026 07:40
08a2430 to
b349bf6CompareThere was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_run_agent_sync_loop.py`:
- Around line 35-36: Update the pytest.raises assertion around
run_agent_sync(coro) to match the actionable guidance text “must await the agent
directly” instead of the generic “running event loop” phrase, preserving the
RuntimeError expectation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9f2c526-8b7b-4645-ab5e-9455bef59978
📒 Files selected for processing (4)
backend/agents/_providers.pybackend/agents/_run.pybackend/agents/health.pybackend/tests/test_run_agent_sync_loop.py
Comment on lines
+35
to
+36
| with pytest.raises(RuntimeError, match="running event loop"): | ||
| run_agent_sync(coro) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actionable error contract.
Matching only "running event loop" would also accept asyncio’s generic nested-loop error. Match "must await the agent directly" so this regression test protects the intended caller guidance.
Suggested assertion
- with pytest.raises(RuntimeError, match="running event loop"):+ with pytest.raises(RuntimeError, match="must await the agent directly"):📝 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
| withpytest.raises(RuntimeError, match="running event loop"): | |
| run_agent_sync(coro) | |
| withpytest.raises(RuntimeError, match="must await the agent directly"): | |
| run_agent_sync(coro) |
🤖 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 `@backend/tests/test_run_agent_sync_loop.py` around lines 35 - 36, Update the
pytest.raises assertion around run_agent_sync(coro) to match the actionable
guidance text “must await the agent directly” instead of the generic “running
event loop” phrase, preserving the RuntimeError expectation.
…rationale Review found the cited example chain (build_system_prompt -> get_course_context) never reaches run_agent_sync; the reachable chain is _legacy_chat -> apply_graph_update -> update_course_context -> _generate_summary_with_gemini -> run_agent_sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
AndresL230 added a commit
that referenced
this pull request
Jul 29, 2026
…455) * feat(evals): complete extraction-accuracy harness + baselines (#148) Finish the agent eval harness so migrated agents are validated on accuracy, not just smoke-tested — the evidence base the gemini_service cutover (#151) needs. - Record 80 cassettes across the five offline datasets (classification, summary, concepts, syllabus, quiz); replay is now deterministic + keyless. - Gate on regression below a committed baseline (baselines.json) instead of "< 1.0" — the harness measures accuracy, it doesn't assume perfection. - Retry transient 503/429 while recording; force UTF-8 output so non-ASCII cases don't crash rich on a Windows cp1252 console. - Add run_all.py (one combined scored run) and enable evals.yml to run it in replay mode on PRs touching backend/agents/** or the harness. - Document the record/refresh workflow (tests/evals/README.md, README) and the design + baselines (ADR 0020). - Exclude chat_tutor: its retrieval tool reads a live Supabase and can't run offline; folded into the graph-grounded tutor work (#149). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(rag): gate below-seam embedding calls on SAPLING_MODEL_MODE (#439) (#454) services/rag_service.py's _embed_query/_embed_document/_embed_documents_batch and routes/documents.py::_index_document_chunks's catalog-relevance gate construct raw google.genai.Client objects directly, predating the #391 SAPLING_MODEL_MODE seam — so function mode (the hermetic E2E default) still fired live gemini-embedding-001 calls on every document upload, quiz generate, and tutor turn with a course_code, silently billing whenever a real key was present. Add agents/_providers.py::model_mode() as the one sanctioned public read of the seam for call sites outside agents/, and gate every embed call site on it. rag_service.py's client is now built lazily (_get_client()) and only ever reached after a model_mode() == "real" check; in non-real mode the _embed_* helpers raise before touching the client, which the existing broad try/except in retrieve_chunks/index_document_chunks already catches — reusing that path makes the deterministic empty/no-op result the designed behavior instead of an accident of a swallowed exception. Same pattern for the documents.py relevance-gate client, scoped tightly to that block. Interim mitigations (scripts/explore.sh, e2e.yml dummy-key forcing, the e2e_oracles logscan allowlist) are untouched — now defense in depth. * fix(documents): resolve abstract course_id in /api/documents/user (#435) (#451) * fix(documents): resolve abstract course_id in /api/documents/user/{id} Library.tsx filters and labels documents on d.course_id, but the route only ever returned offering_id — every upload silently fell into "Uncategorized" and never matched a course filter. Resolve each row's course_id via services.academics.offering_course_id, batching per unique offering_id (mirrors routes/learn.py::list_sessions) rather than once per row. Adds backend coverage for the enriched response shape (single/batched/ missing offering_id) and extends the #387 upload journey with a library-filter assertion: after upload, filtering by the seeded course (resolved from the persisted row's offering_id) must show the document, and the "Uncategorized" filter must not. Fixes#435. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(documents): PR review round 1 — nullable type, honest tests, safe decrypt Address review findings on the #435 course_id fix before merge: - frontend/src/lib/types.ts: Document.course_id is string | null (the fix's own tests pin course_id: null as a real response shape). Library.tsx already treated it as possibly-falsy; tsc surfaced one spot assuming non-null (courseLookup[d.course_id]) — guarded with `?? ""`. - backend/tests/test_documents_routes.py: relabeled the offering_id=None test as defensive-code coverage (schema-unreachable — 0025 makes documents.offering_id NOT NULL) and added the actually-reachable null branch: a present offering_id that offering_course_id fails to resolve. - backend/routes/documents.py: wrap the concept_notes decrypt_json call in list_documents in try/except, matching the established pattern at _existing_doc_by_request_id and scan_document_concepts — decrypt_json re-raises when both decrypt and plaintext-parse fail, so one corrupted row no longer 500s the whole list. Added a regression test (red-first) proving the corrupted row degrades to concept_notes: [] while sibling rows still return. - frontend/e2e/upload.spec.ts: header now notes the #435 library-course-filter regression coverage the journey carries. Refs #435. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(library): dedupe course filter pills by course_id (#435) Stack verification of the #435 library-filter journey caught a real duplicate-render bug: GET /api/graph/{userId}/courses returns one row per enrollment, so a course with two offerings (e.g. CS101 fall + spring) surfaced as two rows sharing the same course_id. Library.tsx built its filter pills directly off that per-enrollment list, so the same course rendered two identical `library-course-filter-{courseId}` pills — a strict-mode Playwright locator violation, and a real UX bug (duplicate rows in the sidebar). Root cause is #449 (get_courses one-row-per-enrollment), which stays out of scope here — the gradebook depends on the per-enrollment shape. This is the frontend render-side fix: dedupe by course_id via a Map at the two derivation points that render per-course UI (the filter pills and the course label lookup), keeping the first enrollment's row as the stable representative. The raw per-enrollment `courses` list is otherwise untouched (course-scan lookup, upload-button disabled check, and the upload modal's course list still see every enrollment). The e2e library-filter assertion (fronten/e2e/upload.spec.ts, #435) was left as strict-mode (no .first() masking) per review — it should now pass because the pills are unique, not because the locator was weakened. Refs #435, #449. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(agents): loop-safe Gemini provider — kill the 'Event loop is closed' flake (#436, #354) (#453) * fix(agents): make the shared Gemini provider loop-safe, not a sweep (#436, #354) test_ocr_pipeline.py::test_save_to_db errored with "RuntimeError: Event loop is closed" on main — the #354 root cause: agents/_providers.py's module-level GoogleProvider eagerly builds an httpx.AsyncClient whose connection pool binds internal asyncio primitives to whichever event loop is running the first time a request goes out over it. Every agent is built once at import time sharing that one provider, so run_agent_sync's per-call asyncio.run() (and, just as much, any test calling asyncio.run() more than once against the same shared agent in one process, as test_agent_parse then the parsed_assignments fixture do here) trips the stale-loop reuse on every second real call. PR #358 (never merged; reviewed clean, just fell through the cracks) fixed this by sweeping eight run_agent_sync call sites to pass a fresh-provider model= override per call. Adapting it verbatim wouldn't have fixed#436: calendar_service's syllabus_extraction_agent, the actual caller behind this test, isn't on that sweep list — and agents/ocr_vision.py already needed its own ad hoc copy of the same idea for a path #358 didn't cover, proof the sweep needs rediscovering at every new call site. The issue itself named this "the tactical sweep" with "make the shared provider loop-safe" as the strategic fix. Since every agent gets its model from model_for()/google_model() exactly once, fixing those two functions fixes every caller — present and future — with no sweep required. _LoopSafeGoogleModel (a GoogleModel subclass) keeps one GoogleProvider per currently-running event loop (a WeakKeyDictionary keyed by the loop object, self-cleaning once a throwaway loop is GC'd), rebuilding only when a NEW loop calls it and reusing the cached one for as long as that loop lives — identical connection-pooling behavior to the old singleton for FastAPI's one persistent per-process loop, and no stale-loop reuse for run_agent_sync's or a test's disposable ones. This also makes ocr_vision.py's ad hoc fresh_ocr_vision_model() workaround redundant; removed it and its call-site override in gemini_vision_backend.py. Proof: tests/test_loop_safe_google_model.py (new, hermetic) pins the per-loop cache directly. tests/test_ocr_pipeline.py run 3x in one process (pytest.main() loop, since repeated identical CLI paths dedup to a single collection) put 6 asyncio.run() cycles through the same shared agent — 33/33 green, zero "Event loop is closed". Full suite green twice back-to-back (1161 passed, 26 skipped, 0 errors both times). ruff check clean. Fixes#436Fixes#354 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agents): eliminate the loop-safe provider's shared-pointer race (#436, #354) PR review on the prior fix found a Critical concurrency bug, backed by an empirical repro: 6 threads x 20 sequential asyncio.run calls against one shared _LoopSafeGoogleModel, with an asyncio.sleep standing in for the real await gap, produced 95/120 mismatches between the provider bound for a call and the one actually read back. Root cause: _bind_to_current_loop() resolved the right provider under a lock, but handed it off via `self._provider = provider` — a single shared mutable attribute on a Model instance that is itself a process-wide singleton (every agent is built once at import time). GoogleModel._generate_ content reads self.client (-> self._provider.client) only AFTER an await (self._build_content_and_config(...)); in that window, a different thread running a different event loop — exactly what happens under concurrent requests, since every sync-def route drives run_agent_sync on a fresh thread + throwaway loop, and gemini_vision_backend._run_from_anywhere does the same for OCR — could rebind that same shared attribute. The first call would then resume and read a provider bound to someone else's, possibly already-closed, loop: a deterministic every-second-call flake became a probabilistic, load-dependent one. Fix: remove the hand-off entirely. self._provider (the base GoogleModel/ Model attribute) is now a fixed template, set once and never reassigned, backing only the reads that are genuinely loop-independent (system/base_url, and the bare .name/.base_url pydantic-ai's own count_tokens/usage-metadata code reads directly) — safe because every provider this module constructs uses identical arguments. .client — the one read that IS loop-affine — is now a property that resolves asyncio.get_running_loop() -> the loop-keyed WeakKeyDictionary fresh, at the exact moment of every access, with no instance-attribute write in between "resolve" and "use". Every inherited GoogleModel method reads self.client through ordinary attribute lookup, so this one override covers all of them — request/count_tokens/request_stream no longer need (and no longer have) their own overrides. __aenter__/ __aexit__ still act on the current loop's provider explicitly. Verified standalone: the buggy hand-off shape reproduces 119/120 mismatches; the fixed property-based design shows 0/120, both under the same 6x20 stress. Added TestConcurrentAccessIsRaceFree to tests/test_loop_safe_google_model.py: a minimal stand-in of the original hand-off design proves the test shape itself would have caught the round-0 bug (asserts it DOES mismatch), then the same shape run against the real _LoopSafeGoogleModel asserts zero mismatches. Deterministic and hermetic — no network. agents/ocr_vision.py and gemini_vision_backend.py needed no further changes: they already call through model_for("ocr_vision")'s default model, so the OCR per-page-loop concurrency concern the review raised falls out of this fix directly. Re-verified: threaded test suite 5x back-to-back (9/9 every time), the OCR pipeline module 3x in one process (pytest.main() loop, 33/33 green), full hermetic suite once (1164 passed, 26 skipped, 0 errors). ruff check clean. Fixes#436Fixes#354 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci(db): pin local/CI Postgres to 15, matching staging/prod (#441) (#452) * ci(e2e): pin local/CI Postgres to 15, matching staging/prod (#441) supabase/config.toml's major_version pins the local/CI Postgres (via supabase start) independently of hosted staging/prod; PR #440 set it to 17 for local-dev/CI consistency, which drifted the deterministic lane away from what production actually runs. Pin it down to 15 instead — hosted staging/prod stay untouched (bumping them is a separate, outward-facing ops action), and local/CI consistency is preserved since both still follow the same config.toml pin, just at 15. - supabase/config.toml: major_version 17 -> 15 (also the Supabase CLI's own documented default for this key). - .github/workflows/e2e.yml: update the header comment that previously explained "deliberately going against" the PG15 leaning. - backend/tests/integration/test_postgres_version.py: assert the real server's major version via SHOW server_version_num, so drift is loud. Lives in the opt-in integration suite because .github/workflows/integration.yml boots the local stack from empty and runs `RUN_INTEGRATION=1 pytest -m integration` on every push to main — this actually executes in CI, against the same config.toml-pinned Postgres the browser lane (e2e.yml) also boots. - docs/local-supabase.md: update the "Postgres version" troubleshooting entry and add a reset note for stacks provisioned before this pin (the CLI does not swap a running container's image just because config.toml changed). Migration chain audited for PG16+-only constructs (MERGE, JSON_TABLE, REGEXP_*, EXCLUDE, etc.) — none found; UNIQUE NULLS NOT DISTINCT (0023_graph_integrity.sql) is itself a PG15 feature, so it's fine on the target version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(e2e): fix#441 PR-review findings — history + reset guidance Two documentation-accuracy fixes from PR review, no behavior change: 1. backend/tests/integration/test_postgres_version.py docstring misattributed the PG17 pin's origin to PR #440. Git history shows the pin was born with the local Supabase stack itself (9f54739, config.toml created with major_version = 17) — #440 only added the e2e.yml comment rationalizing the already-existing PG17 against epic #402 decision 2's PG15 leaning, and noted the skew was tracked in #441. Rewrote the causal narrative to match; updated the assert failure message's reset guidance to match fix 2 below. 2. docs/local-supabase.md's "Postgres version" troubleshooting entry was self-contradictory: it said the CLI "does not swap a running container's image just because config.toml changed," then claimed scripts/local-db-reset.sh (supabase db reset) "recreates the Postgres container against the pinned version." Reading local-db-reset.sh: it never calls supabase stop/start, so it can't replace a running container's major version — confirmed against supabase/cli issues #5555 and #4522 (db reset does not reliably pick up a changed major_version and can leave a half-upgraded, broken container). Restructured so the reliable path is primary for a version change: `supabase stop --no-backup` + `supabase start` (fresh container, correct major, no app schema yet), then local-db-reset.sh / make e2e-up for their actual job — migrate + seed. Static-only per the controller's instructions (no stack boot); hermetic backend suite re-run clean (1155 passed, same pre-existing unrelated test_ocr_pipeline.py event-loop flake, #354). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * docs(e2e): refresh known-bugs catalogs after the #402 follow-up batch (#456) * docs(e2e): refresh known-bugs catalogs after the #402 follow-up batch Fixed and closed: #355, #430, #435, #436 (+root cause #354), #439, #446 (merged via #447/#448/#450/#451/#453/#454). #441's fix (PR #452, PG15 pin) is in final verification, merging imminently. Known-open remaining: #449 (get_courses per-enrollment fan-out produces duplicate course_id rows; Library's instance fixed render-side in #451, Tree/Dashboard/etc. and the DocumentUploadModal picker still exposed). Updates docs/e2e-exploration.md (§6 logscan allowlist note, §7 triage category 3 + worked examples, §8 fixme lifecycle example) and scripts/explore/explorer-prompt.md's known-bugs section to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(e2e): #441 landed — move it into the recently-fixed list Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * docs: CLAUDE.md — E2E lanes, stack-lock protocol, function-mode seam conventions (#457) Every agent session on this repo now learns the E2E system up front: the deterministic stack commands (e2e-up/down, Playwright lane, oracles, explore), the pre-merge three-way verification expectation, the fix→promoted-journey pairing, the machine-singleton flock protocol, and the function-mode seam rules (fixed constants, handler registration, no raw genai clients below the seam). Follows the #402/#403 epics and the 2026-07-28 bug-queue batch. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(agents): guard run_agent_sync against running event loops (#354 follow-up) (#358) * fix(agents): guard run_agent_sync against running event loops (#354 follow-up) Reworked from the original fresh-client sweep: the cross-loop client problem is now solved by _LoopSafeGoogleModel (#453), so the per-call fresh-client plumbing and the subject_root dedup (landed separately, #355) are dropped. What remains is the piece main still lacks: - run_agent_sync detects a running event loop, closes the handed coroutine (no 'never awaited' warning) and raises a clear error the try/except-guarded sync-from-async callers can degrade on, instead of letting asyncio.run raise opaquely. - HEALTH_PROBE_MODEL constant so probe sites can't drift. - Regression tests for both loop paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(agents): name the real async->sync call chain in the loop-guard rationale Review found the cited example chain (build_system_prompt -> get_course_context) never reaches run_agent_sync; the reachable chain is _legacy_chat -> apply_graph_update -> update_course_context -> _generate_summary_with_gemini -> run_agent_sync. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * feat(learn): stream tutor replies over SSE with live graph deltas (#70, #74) (#349) * feat(learn): stream tutor replies over SSE with live graph deltas (#70, #74) — rebased onto main Net rework of feat/streaming-tutor against current main: - Ported: services/chat_stream.py (stream_agent_turn rung ladder), agent_events.py chat-event vocabulary, /chat/stream + /start-session/stream SSE routes, frontend sse.ts + api.ts stream consumers, Learn.tsx/ChatPanel wiring + tests, ADR as 0020. - Dropped the fresh-client plumbing (_fresh_stream_model, fresh_google_model, per-call model= overrides): superseded by _LoopSafeGoogleModel (#453). The streamed routes now inherit the agent's own model so the SAPLING_MODEL_MODE seam applies. - NEW: function-mode seam serves streamed runs — _function_model_for gains a stream_function that replays the registered handler's ModelResponse as deltas (text + DeltaToolCall), keeping E2E_* constants byte-identical across JSON and SSE lanes; covered by two new seam tests. - Learn.tsx edgeKey NUL byte rewritten as \u0000 escape (text-clean). - tutor-stop testid added (e2e-surface lint #382) + docs entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(learn): harden stream persistence + concurrent-stream guard (review findings) - stream_agent_turn: on_complete failures after a fully-streamed reply now yield the structured ADR-0020 error event instead of aborting the SSE response uncaught (headers are already flushed at that point). Regression test added. - Learn.tsx: abort any in-flight stream controller before starting a new one — a graph-node click could begin a session while a reply streamed, interleaving two streams into shared state. - Docstrings: the on_complete/legacy_fallback invariant is 'at most one, never both' (error rungs run neither), not 'exactly one'; PENDING_SESSIONS wording updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * docs(evals): renumber harness ADR to 0021 — 0020 was taken by the streaming-tutor ADR (#349) Also merges current main (clean; #349's frontend/stream files and this harness touch disjoint trees). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(evals): fail closed on missing baselines — an ungated dataset or evaluator must not PASS CI Review finding: the regression gate iterated only committed baseline keys, so a dataset absent from baselines.json (or an evaluator added without refreshing it) reported PASS with zero protection — right as evals.yml becomes a PR gate. Both paths now FAIL with a pointed message; verified empirically (removed dataset + evaluator entries -> exit 1, restored -> exit 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Andres Lopez <190146319+AndresL230@users.noreply.github.com>
AndresL230 added a commit
that referenced
this pull request
Jul 29, 2026
… (fixes staging session_expired) (#409) * feat(errors): extract FastAPI detail from thrown API errors (#361) `fetchJSON` rejects with `new Error(await res.text())`, so a FastAPI failure surfaces as an Error whose message is the raw JSON body. Add a dependency-free helper that reads the `detail` back out of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover FastAPI detail extraction (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): recover the HTTP status off a thrown error (#361) `fetchJSON` only spells the status out (`HTTP 404`) when the response body is empty, so read it from an attached `status`/`statusCode`, the parsed body, or the `HTTP <code>` message as available. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover HTTP status recovery (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): map HTTP statuses to friendly copy (#361) Add humanizeError: status-driven sentences for the cases users can act on (auth, missing, rate limit, 5xx), falling back to caller-supplied copy so it can never surface a raw body. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(api): declare the term label on EnrolledCourse (#140) /api/graph/{user_id}/courses has always returned the offering's term label; the client type never declared it, so every consumer had to cast through any to reach it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover the status-to-copy mapping (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(api): add getSemesters() for GET /api/semesters (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ui): add responsive layout primitives to globals.css (#109) Inline styles can't carry a media query, so the app's fixed multi-column shells (Admin's master/detail panes and metric row, Settings' profile field rows) get class hooks here instead. Driving them from CSS rather than `useIsMobile` also makes the first paint correct, since the hook can only flip after hydration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): prefer a human-readable server detail (#361) A FastAPI detail like "Exam not found." is better copy than generic status text, so surface it — but only when it reads like a sentence, so a serialized payload, markup or a stack can never reach the UI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin roles pane on mobile (#109) The role editor rail was pinned at `minmax(280px, 360px) 1fr` with no mobile branch, so the pane overflowed the viewport below ~640px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): scaffold the shared term helper module (#140) termRankFromLabel mirrors the sort_key formula from migration 0019 so a label-only fallback orders identically to the server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin achievements pane on mobile (#109) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): assert no raw body, markup or stack ever reaches the UI (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin cosmetics pane on mobile (#109) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): resolve the current term by date (#140) Mirrors services/academics.py::current_term — today within [start_date, end_date], else the highest sort_key — so client and server never disagree about which semester is current. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): reflow the Admin overview metric row on mobile (#109) Four fixed metric cards squeezed to ~75px each at 375px. Drops to a 2x2 grid below 900px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): add an isNotFound predicate (#361) Lets callers branch on "that thing is gone" without string-matching a response body at the call site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Settings profile rows on mobile (#109) The username row and the display-name/bio/location/website rows were both hard-coded to `180px 1fr`, leaving ~150px for the input at 375px. They now share the `.settings-field-row` class and collapse to a label-above-control stack below 600px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover current-term date resolution and the gap fallback (#140) Fixtures are the four terms seeded by migration 0019 verbatim, so a drift between this rule and the backend's shows up here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover isNotFound detection (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the exam-load failure toast (#361) `String(err)` rendered the stringified FastAPI body straight into the toast. Keep the real error on the console and show a sentence instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ui): let Dialog consumers pick the initially focused element (#109) Dialog focuses the first focusable node in the panel, which is always the close button. Form dialogs need their first field instead, and `autoFocus` loses that race — React fires it at mount, before Dialog's focus pass. Opt-in and additive; existing consumers are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): group courses by term label, most recent first (#140) Ordering keys on sort_key when the semesters payload is available and degrades to the label-derived rank otherwise. Courses with no term go to an 'Other' bucket rather than being dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the guide-load failure toast (#361) Also clear the stale guide so a failed load can't leave the previous exam's content on screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover term grouping, ordering and the unknown bucket (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): land on inline guidance when the exam is gone (#361) A missing exam is a normal state — a deleted assignment, or a stale "recent guides" entry — not a failure. Show the user where to go next instead of firing a red toast at them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move LetterScaleEditor onto the shared Dialog (#109) Drops the hand-rolled portal and its `minWidth: 360` — which overflowed a 360px viewport once the overlay's gutters were counted — for Dialog's `min(420px, 100vw - 32px)` panel. Also picks up the focus trap, Escape handling and scroll lock the hand-rolled version never had. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): partition courses into current and archive (#140) Only courses that rank strictly below the current term are archived. Undatable courses — and every course when /api/semesters gives us nothing — stay in the default list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover partition ordering and the no-semesters fallback (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(study): offer a retry when a guide genuinely fails to build (#361) Generation failures (502) are usually transient, so keep the message on screen next to a retry instead of leaving the user on a blank panel after the toast times out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): keep regenerate unreachable without a selected exam (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): derive ordered term labels for the gradebook chips (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the regenerate failure toast (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move EditWeightsModal onto the shared Dialog (#109) `minWidth: 520` made this the worst overflow of the four gradebook modals; it now sits in Dialog's `min(640px, 100vw - 32px)` panel. The footer wraps rather than crushing the "Total: n%" readout against the buttons on narrow screens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the flashcard delete and generate toasts (#361) Last two raw-error toasts on this screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(study): sharpen the no-exam empty-state copy (#361) Say why an exam is needed, not just that none exist — that's the whole question a user lands on this state with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gradebook): read term (not semester) off the courses payload (#140) /api/graph/{user_id}/courses emits `term`; the landing read `(c as any).semester`, which is always undefined. `distinct` was therefore always empty and every signed-in user silently fell through to the hardcoded SAMPLE_SEMESTERS demo chips. The sample chips are now the logged-out preview only — a signed-in user with no terms gets their own empty state instead of another student's fake grades. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move SyllabusUploadFlow onto the shared Dialog (#109) Replaces `minWidth: 460` with Dialog's fluid panel, and lets the category/assignment rows shrink (`minWidth: 0` on the flex text inputs, wrapping on the assignment rows) so the date picker can't push them past the panel edge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study-guide): make the exam-not-found detail actionable (#361) The frontend now renders a FastAPI detail verbatim when it reads like a sentence, so tell the user what to do next instead of just naming the condition. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(study-guide): pin the 404 detail as user-facing copy (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(gradebook): pin the landing chips to the courses payload term (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(gradebook): type the CourseCard test stub instead of any (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): let the guide problem outrank the generic empty hints (#361) Opening a recent guide clears the exam selection, so a missing exam would otherwise stack "No exams for this course yet" on top of the guidance explaining what actually happened. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move AssignmentModal onto the shared Dialog (#109) `minWidth: 420` overflowed any phone viewport, and the panel had no max-height at all — with the bell-curve section expanded the footer ran off-screen with nothing to scroll. Dialog fixes both and adds the focus trap, Escape handling and scroll lock. `autoFocus` is swapped for Dialog's `initialFocusRef` so the Title field still takes focus on open rather than the close button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): retry the guide that actually failed (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(a11y): 44px touch targets for SideNav rows (#110) `8px` vertical padding around a 15px icon left the nav links ~31px tall. Collapsed, the rail is 64px wide minus 6px padding, so the `width: 100%` link already clears 44px horizontally — only the height needed a floor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(a11y): 44px collapse/expand controls in SideNav (#110) The collapse chevron was a 24x24 target and the expand bar 28px tall. Both now match Dialog's 44x44 close button. `flexShrink: 0` keeps the collapse button square when the account name is long — the name block beside it already ellipsizes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(study): cover the missing-exam guidance and retry paths (#361) Drives the screen through the recent-guides rail — the real path to a stale exam id — and asserts a missing exam produces guidance with no toast, while a genuine failure toasts a sentence and keeps a retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ui): make useIsMobile hydration-safe via useSyncExternalStore (#110) `useState(false)` + a `matchMedia` effect meant the value was stale for one paint after every mount, and each consumer registered its own listener. `useSyncExternalStore` pins the SSR/hydration snapshot to `false` (so server and first client render still agree, as React 19 requires) while sharing one `MediaQueryList` per breakpoint and updating as early as React allows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(gradebook): order the semester chips by the real term calendar (#140) Chips now sort by sort_key from /api/semesters and default to the date-derived current term instead of whichever term the courses payload happened to list first. A failed semesters fetch degrades to the label-derived order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(gradebook): open the term named by ?semester= (#140) Gives the dashboard archive somewhere to land: selecting an archived class opens that semester's gradebook rather than the current one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): load the term calendar alongside the graph payload (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * polish(study): stop the failure card restating its own title (#361) When no server detail survives, the body falls back to "Couldn't build that study guide" — which was the title too. Give the card a heading that pairs with any reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ui): cover the useIsMobile SSR/hydration contract (#110) Seven cases: the server render reports desktop on a mobile viewport, hydration produces no recoverable error either way, the value flips after commit and tracks later changes, and the queried width matches the `max-width: 767px` rules globals.css relies on. Verified against a naive `useState(matchMedia(...).matches)` implementation — it fails three of them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): partition course progress into current and archive (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): hide the desktop rail pre-hydration on mobile (#110) The SSR shell always assumes desktop, so a phone painted a 232px SideNav rail until hydration swapped in TopNav. A width-based `@media` rule applies to that first frame, which no amount of hook work can reach. Pairs with the useIsMobile breakpoint, asserted in its test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(images): lazy-load and size the remote avatar images (#111) `Avatar` and `AvatarFrame` render user-supplied URLs with no intrinsic dimensions, so every one of them reserved zero space until it decoded. Explicit width/height give the browser the aspect ratio up front; the CSS `100%` sizing still wins for layout. The two `/sapling-icon.svg` logos in TopNav/SideNav are deliberately left eager — they're local, above-the-fold brand marks already sized by inline styles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(dashboard): extract CourseProgressRow from the courses panel (#140) Same markup, lifted so the current-term list, the archive and the graph overlay can all render a course line without a third copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): group the my-courses panel by semester with an archive (#140) Current-term courses show by default; earlier terms collapse behind an Archive toggle, grouped by label most recent first. Also covers the mobile 'My Courses' tab, which renders the same panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): gate the hero canvas RAF behind prefers-reduced-motion (#111) The hero projects and sorts 226 nodes and runs an O(n^2) edge pass every frame, forever. globals.css only neutralizes CSS animation, so a reduced-motion visitor was still paying for all of it. Now it paints one static frame and parks, repainting on resize (which clears the backing store) and re-arming if the preference flips mid-session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): scope the graph courses key to the current term (#140) The floating course key now lists only current-term courses and offers past terms as a compact Archive that deep-links into each semester's gradebook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): hoist the floating-card DOM and dataset reads out of the RAF (#111) The tick re-ran `querySelectorAll('.floating-card')` and re-parsed three `dataset` floats per card on every frame. Both are static, so they move to effect setup. The loop also parks under prefers-reduced-motion, keeping each card's resting tilt but dropping the drift, mouse tilt and scroll parallax. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): cache spotlight card rects instead of measuring per mousemove (#111) `getBoundingClientRect()` on every pointer sample forces a layout flush. The rect is now taken on `mouseenter` and dropped on scroll/resize — the only things that can move a card relative to the viewport — so a sweep across a card costs one measurement, not one per sample. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api): carry the HTTP status on failed requests (#361) fetchJSON discarded the status, so a FastAPI failure — which always has a JSON body — reached callers with no status at all. isNotFound had to infer "missing" from the words "not found", which would silently regress into a red toast the day someone reworded a server message. ApiError keeps `message` as the raw body, so existing callers that stringify or read `.message` are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(courses): group the manage-courses list by semester (#140) Headings only appear once a student has courses in more than one term. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): rAF-throttle the landing scroll handler (#111) `onScroll` wrote inline styles on the hero, the nav and the ambient glow on every scroll event, which fire well above frame rate. Coalesced to one write per frame; the mousemove and scroll listeners are also marked passive since neither calls preventDefault. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): scope the graph legend chips to the current term (#140) Keeps the top-nav legend consistent with the courses key overlay, which already lists only the current semester. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(dashboard): cover semester grouping, archive routing and degradation (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): wire the archive toggle to its region for assistive tech (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(gradebook): smoke-cover the four modals moved onto Dialog (#109) The migration is invisible to tsc — a modal that stops opening, loses its Cancel handler, or drops its accessible name still typechecks. These four had no tests at all, so the swap was landing unverified. Also pins initial focus landing on the title field rather than Dialog's close button, which is the specific reason initialFocusRef exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(lint): prune the suppression the Landing fix made stale (#140) Reading `term` instead of `(c as any).semester` removed the only no-explicit-any in Landing.tsx, so its suppression entry no longer matches anything. eslint exits 2 on a stale suppression even with zero errors, which fails the CI lint gate — `main` exits 0, this branch did not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(frontend): make DEPLOY_ENV the single source of truth for env config Staging login bounced to /?error=session_expired: the worker serving staging.saplinglearn.com ran with production config (BACKEND_URL= api.saplinglearn.com), so sign-in round-tripped through the prod backend and came back as a prod-signed .saplinglearn.com cookie that staging's middleware rejected under its own SESSION_SECRET. The deployGuard check that would catch a consistent-but-wrong-target build only arms when DEPLOY_ENV is set, and it wasn't set on either Workers Build. - deployGuard: add resolveFrontendEnv (derive apiUrl/cookieDomain from FRONTEND_ENVS when DEPLOY_ENV is set; fall back to explicit vars otherwise) plus expectedEnvForHost/detectHostConfigMismatch. Unit-tested. - middleware: derive API_URL via the resolver; on a protected route, flag a host/backend mismatch with a loud log + distinct `env_misconfig` code instead of the misleading `session_expired`. - session route: derive cookie Domain from the resolver. - next.config: derive build-time BACKEND_URL/NEXT_PUBLIC_API_URL/COOKIE_DOMAIN from DEPLOY_ENV. - wrangler.toml: set DEPLOY_ENV for [vars] and [env.staging.vars]. - SignInModal: user copy for env_misconfig. - docs: ADR 0020 (root cause + required deploy follow-up). Note: this hardens the repo but does not fix the running deployment — that needs a staging redeploy with DEPLOY_ENV=staging + `wrangler deploy --env staging` and the correct route binding (see ADR 0020). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(deploy): stop the build-command footgun that took staging down ADR 0020's operational follow-up told operators to set a `wrangler deploy --env staging` line and a DEPLOY_ENV build variable, but never said to keep the Build command as `npm run cf:build`. Wiring that up, the frontend-staging Workers Build's *build-command* field got overwritten with `npx wrangler deploy --env staging` — a deploy command in the build slot. That skips `opennextjs-cloudflare build`, so `.open-next/` is never produced and every build failed with "Could not find compiled Open Next config" (~16 red builds across all branches since 2026-07-20). Verified locally: `npm run cf:build` produces `.open-next/worker.js` (the `main` wrangler deploys); `npx wrangler deploy --env staging` alone does not. - ADR 0020: split the two Workers Builds fields explicitly, mandate the Build command stay `npm run cf:build`, and forbid putting a deploy command in it. - wrangler.toml: document the same Build vs Deploy field distinction at the point of configuration. The live fix is still a one-field dashboard revert (Build command back to `npm run cf:build`); this stops the docs from steering anyone into it again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(frontend): data-testid convention on six core E2E surfaces (#382) (#410) The browser suite (#385) needs stable selectors. Today shipped code has zero data-testid attributes, so Playwright would have to anchor on CSS classes (utility-ish, non-unique) or copy — both churn on every design pass. Adds a kebab-case `<surface>-<element>` convention, applies it to the six surfaces Chapter-1 drives (sign-in, approval gate, upload modal, tutor composer, quiz answer flow, graph container), and gates drift with a per-file ESLint rule. - docs/frontend-testids.md documents the naming rules, how repeated/list items are disambiguated (stable domain id first, render index as the fallback), the full current inventory, and how to onboard a new surface. - Testids land on the file that actually renders the element, which is not always the screen file: the tutor composer lives in ChatPanel.tsx (single consumer: screens/Learn.tsx) and every quiz control lives in QuizPanel.tsx (screens/Quiz.tsx only mounts it). - eslint.config.mjs gets a `no-restricted-syntax` block scoped to those six files: any <button>/<input>/<textarea> there without a data-testid is an error. Deliberately not repo-wide — the rest of the app has no browser coverage to protect. Attributes and lint config only; no behavior, styling, or logic changes. The SignInModal.tsx edit is strictly additive (open PRs #409/#359 touch that file). Closes #382 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(backend): keyless rag_service import + hermetic LLM egress guard (#411) #378 — services/rag_service.py built a module-level genai.Client with api_key=os.getenv("GEMINI_API_KEY", ""), and genai.Client(api_key="") raises ValueError at construction. That broke `import main` outright without a key (routes/quiz.py and routes/learn.py both pull the module in). Fall back to "dummy-key-for-import" the way services/gemini_service.py and agents/_providers.py already do: imports stay clean and the failure moves to call time, where it is actionable. No behaviour change when a real key is present. #379 — add the autouse `_hermetic_llm_transport` fixture to tests/conftest.py, the LLM sibling of `_hermetic_supabase_client`. It patches the google-genai transport CLASS (google.genai._api_client.BaseApiClient) rather than client instances, so every already-constructed module-level client is covered: gemini_service, rag_service, and pydantic-ai's GoogleProvider. Unstubbed calls now raise UnstubbedLLMEgress("unstubbed LLM egress: ...") instead of making a real, billable request. Unary, streaming, sync, async and the File API side channels are all blocked, and the fixture fails loudly if google-genai ever moves the seam rather than silently degrading to a no-op. Exemptions mirror the existing guards (e2e_staging, integration) plus a new `live_llm` marker for the three deliberately-live tests in test_ocr_pipeline.py. Their existing `_requires_gemini` skipif is invisible to `get_closest_marker`, so a real marker was required; the skipif still keeps them from running without a key. Verified: full suite 987 passed / 5 skipped / 1 pre-existing error (test_ocr_pipeline::test_save_to_db, unchanged from main); CI-equivalent lane 929 passed / 5 skipped; ruff clean; keyless `import main` succeeds. Closes #378 Closes #379 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test(backend): cookie-minting test-auth endpoint for local/test envs (#381) (#412) * test(backend): cookie-minting test-auth endpoint for local/test envs (#381) `GET /api/auth/dev-login` was removed and real Google OAuth is not headless-automatable, so pytest and Playwright had no sanctioned way to obtain an authenticated session. Unify the duplicated minter: - New `backend/services/session_tokens.py` owns the one implementation of the `<payload_b64>.<sig_b64>` format `auth_guard._decode_session` verifies, plus the canonical `SESSION_COOKIE_NAME`. - `db/e2e_staging_http.py` and `tests/integration/conftest.py` now use it instead of carrying verbatim copies; the OAuth-callback redirect handoff token in `routes/auth.py` uses it too (byte-identical output, TTL passed explicitly). `auth_guard` reads the cookie name from it. - `tests/test_auth_session_contract.py::_mint` stays an independent re-implementation on purpose: it pins the wire format from the outside. Add `POST /api/auth/test-login`: - Sets the `sapling_session` cookie with the same attributes as the real session AND returns the token in the body, so Playwright global setup can inject it via `context.addCookies()`. - Hard-gated on `APP_ENV in {"local", "test"}` — narrower than `config.IS_LOCAL`, which also covers `development`/`dev`. - The gate is evaluated per request off the live `config` module attribute and returns a stock 404 `{"detail": "Not Found"}` everywhere else, for every request shape (the body is parsed by hand so FastAPI's pre-handler 422 cannot disclose the route). `include_in_schema=False` keeps it out of /openapi.json in all environments. - No DB access: it does not create users or grant approval/roles. 47 new tests cover the production 404, the request-time gate, the real auth_guard round-trip, and byte-identical minting. Closes #381 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(auth): assert test-login mounting via router.routes, not app.routes `test_route_exists_but_is_gated` walked `client.app.routes` looking for `/api/auth/test-login`. How an included APIRouter flattens into the composed app's route list is not a stable API: under the pinned fastapi 0.138 / starlette 1.3 (CI) the sub-router contributes no `.path` entries there, so the set comprehension silently found nothing and the assertion failed — while every behavioural test against the same endpoint passed, because the route itself was mounted and serving correctly. Assert against `auth_module.router.routes` instead, which is a flat list of APIRoute objects with stable `.path` values across both versions. This keeps the test's original purpose: proving the 404 comes from the environment gate rather than from a route that was never mounted. Caught by CI; the local venv runs fastapi 0.136 / starlette 1.0, where the old introspection happened to work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): wave-2 handoff for epic #402 subcutaneous lane (#414) Session prompt for the next wave (#391, #397, #398), committed so a cloud session can pick it up from the repo rather than needing it pasted in. Records what wave 1 established and what it cost to learn: the baseline test counts and the one pre-existing OCR error not to chase, the shadowed grep/find, the missing venv/.env in fresh worktrees, why `env -u GEMINI_API_KEY pytest` can never work, and the local-vs-requirements.lock version skew that made a locally-green test fail CI. Also states the engineering constraints this lane turns on -- assert through a different layer than the one that wrote, make a test fail before trusting it, never weaken a hermetic guard to get green, and treat #398's findings as the deliverable rather than a blocker. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): add skills + autonomy guidance to the wave-2 handoff (#415) * docs(e2e): add skills + autonomy guidance to the wave-2 handoff The handoff covered environment traps and engineering constraints but said nothing about which skills to reach for or how independently to run, so a session picking it up would default to neither. Splits the tooling by what actually resolves where: /sync-context, the context-curator agent, /recall, /log-decision and /log-attempt are committed under .claude/ and work anywhere, while the superpowers and code-review skills are local plugins that may not exist in a cloud session -- those are listed conditionally with a manual fallback for the review fan-out. Calls out that CLAUDE.md already requires /sync-context before agent-building work, which #391 is, and that context-curator is meant to run before touching LLM integration. Adds an autonomy section: execute the wave without asking permission for reversible work, own CI failures rather than reporting a red PR as done, and never end a turn on a plan instead of doing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): make code review gate the merge, not trail it The handoff put /code-review at the end of the wave, after every PR had already merged. That ordering cannot prevent a bad change from landing -- it can only document one after the fact. Wave 1 was run this way and got lucky: the review found nothing above threshold, but anything it had found would already have been on main. Makes review a per-PR merge gate alongside CI, with every finding addressed or explicitly dismissed with a reason. Keeps a wave-end pass, but reframes it as covering interactions between merged PRs rather than as the only review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): fix false test claim + add destructive-truncate guardrail Review of PR #415 surfaced two real defects in the handoff: - Claimed all four tests in test_local_stack.py assert via table(); only two do. The other two assert on the app's HTTP response. Corrected so an agent doing find-and-replace isn't misled about the current shape. - #397's autouse truncate runs on a direct psycopg connection over SUPABASE_DB_URL, but the only local guard checks SUPABASE_URL, a separate var. .env.staging and .env.production both hold live direct-Postgres strings. Added a non-negotiable requirement to assert SUPABASE_DB_URL is local and fail loudly before any truncate, so an unsupervised run can't silently wipe real data. Same guardrail added to issue #397 and its acceptance criteria. Also flags the psycopg-in-tests pattern as a deliberate test-only exception to the table()-only rule, so a literal reader doesn't stall on the conflict or treat it as licence for psycopg in app code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(agents): SAPLING_MODEL_MODE FunctionModel test seam (#391) (#416) * feat(agents): SAPLING_MODEL_MODE FunctionModel test seam (#391) model_for() now dispatches on SAPLING_MODEL_MODE (default 'real', so production and the hermetic unit lane are unchanged): - real → GoogleModel, still honoring the per-task SAPLING_MODEL_<TASK> override from ADR 0008. - function → pydantic-ai FunctionModel bound to a per-task handler tests register via register_function_handler(). Scripted tool calls run through the real tool registration, arg-schema validation, and retry loop. - cassette → reserved (issue scope) but raises NotImplementedError. - anything else → ValueError (a typo'd mode never silently bills Gemini). The FunctionModel substitutes ABOVE the #379 transport guard: it never builds a google.genai request, so a function-mode run needs no hermetic exemption and runs clean in the default lane. Tests pin that invariant (rides-above-guard + the real-mode counter-check that still trips it). AC: an integration-style test drives note_chat_agent with a FunctionModel and asserts on the LLM-chosen search_course_materials_tool arguments after schema validation; a classifier test proves the retry loop runs for real. +13 tests, no regressions (976 → 989 passed in the CI-ignore lane). ADR 0019 records the decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX * refactor(agents): review polish on the model-mode seam (#391) Self-review follow-ups, no behavior change: - annotate model_for/_function_model_for as -> Model (the pydantic-ai base) instead of GoogleModel + type: ignore — function mode genuinely returns a FunctionModel, so the honest supertype removes the type lie. - drop the unused unregister_function_handler and ModelMode alias to keep the seam's public surface to just register/clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX --------- Co-authored-by: Claude <noreply@anthropic.com> * test(backend): integration fixtures — psycopg raw-SQL seam, truncate isolation, seeded users (#397) (#417) The integration lane existed but only round-tripped through PostgREST both ways (testing the echo, not the DB) or asserted on the app's own JSON. This adds the raw-SQL seam the lane was missing and the fixtures #398 builds on: - db_conn: session-scoped psycopg connection on SUPABASE_DB_URL (dict rows, autocommit) — the raw-SQL assertion seam. Writes go through the app; reads come back through this, never through table(). - _require_local_db_url: the non-negotiable safety gate. SUPABASE_DB_URL is independent of the SUPABASE_URL that _require_local_stack checks, and .env.staging/.env.production hold live direct-Postgres strings, so the truncate could wipe a real project. The gate parses the host (strict, so 127.0.0.1.evil.com is rejected) and RAISES — never skips — on non-local. - _reset_between_tests: autouse truncate of every mutable table + reseed of the rich baseline before each test, making the suite order-independent. The denylist preserves the migration-seeded reference layer + catalog hierarchy (verified to carry no FK to users, so no CASCADE can reach it). - seeded_user factory (distinct approved users) and authed_client / other_user_client, replacing the per-test cookies.set boilerplate. test_local_stack.py is refactored onto the fixtures: the flagship test POSTs a note through the app and asserts the ciphertext at rest via raw SQL; a truncate -isolation pair proves ordering-independence; a distinct-users test and a seeded_user test cover the new fixtures. The safety gate is proven in the DEFAULT hermetic lane (tests/test_integration_ db_guard.py, pure URL logic, no DB) so it gates every PR: +13 tests there (976 → 989), the 9 DB-backed tests skip without RUN_INTEGRATION. No regressions. Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX Co-authored-by: Claude <noreply@anthropic.com> * test(backend): migration order pins, encryption round-trip suite, e2e→subcutaneous rename (#398) (#418) Partial delivery of the subcutaneous write-path suite — the pieces provable or low-risk without a running stack: - test_migrations.py (default lane, VERIFIED): pins the runner's apply order. The 0021 pair is load-bearing — 0021_gradebook.sql CREATEs `assignments` and 0021_gradebook_curve.sql ALTERs it to add curve_* columns, so gradebook MUST apply first. sorted(glob()) does exactly that ('.' 0x2E < '_' 0x5F). This also corrects the issue comment, which claimed the sort yields "gradebook_curve before gradebook" — it does not; the pin guards against a rename flipping it. - tests/integration/test_encryption_roundtrip.py: reads every encrypted column from the seeded baseline via the #397 raw-SQL seam and asserts ciphertext at rest + decrypt round-trip across text (decrypt_if_present), numeric (decrypt_numeric, assignments.points_*), and JSON (decrypt_json, sessions.summary_json) — the "silent decrypt regression" sentinel. - tests/integration/test_migrations_ledger.py: the DB-backed half of the migration check (schema_migrations records every file on disk). - Renamed test_e2e_staging.py → test_subcutaneous_staging.py (it drives HTTP routes below the UI; not a browser E2E). Marker `e2e_staging` unchanged. Default lane: +5 verified migration tests (1002 → 1007), no regressions. The integration files are marked `integration` and skip without RUN_INTEGRATION. Remaining #398 scope (test_postgrest_semantics, test_constraints, test_authz_real_rows, and the actual run-to-find-bugs) needs the local stack and is tracked as a follow-up — #398 stays open. Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX Co-authored-by: Claude <noreply@anthropic.com> * feat(ocr): transcribe text-layer-less pages with Gemini vision Scanned and photographed handwritten coursework carries no text layer, so there are no characters to copy out. Docling's OCR is meant to cover this but crashes on such documents -- `Stage preprocess failed for run 1, pages [13]: std::bad_alloc` -- and the error is swallowed, so the page comes back empty. Docling *does* flag those pages in `fallback_pages`, but the only consumer of that signal was gated behind `OCR_ENGINE=auto` + `GOT_OCR_ENABLED`, and the default engine is `docling`. So in practice the signal was computed and discarded, and a 13-page handwritten practice final extracted to "" -- which then reached the classify prompt as an empty `Content:` block and was answered with an invented summary. Add a Gemini-vision backend that transcribes a rendered page image, and wire it to that existing signal. Deliberately NOT gated on `OCR_ENGINE=auto`, since that gate is precisely why the rescue never fired for real uploads. Chosen over the alternatives for handwritten maths specifically: Tesseract is poor at handwriting, and GOT-OCR needs a ~2GB weight download and is impractical CPU-only. Gemini already backs every other AI path here, and returns LaTeX. Verified end to end on the document that triggered this, with OCR_ENGINE at its default: 0 chars -> 4,507 chars, including row reductions, characteristic polynomials, and \boxed answers. Off by default (`GEMINI_VISION_OCR_ENABLED`): it costs one LLM call per flagged page. Pages with a normal text layer are never flagged, so a text PDF costs nothing. Per-page failures keep whatever Docling produced for that page -- a partial document beats none -- while an unavailability error aborts the loop rather than burning a failed call for every page of a long scan. Also: - extract the OCR cache key into `_ocr_cache_key` and include the new flag, so enabling vision cannot serve the empty string cached from before it was on - correct the comment claiming OCR is deterministic. It no longer is, which matters for content-addressed chunk ids (ADR 0019): two students uploading the same scan only dedup to one embedding if they transcribe identically. Persisting OCR output content-addressed rather than merely caching it is the real fix, and is not attempted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(observability): activate Logfire ops/error/LLM tracing (#119) (#406) * feat(observability): activate Logfire ops/error/LLM tracing (#119) Turn on Logfire safely and document it. The SDK was already configured (logfire.configure + instrument_pydantic_ai + the scrub_value scrubber), but two gaps kept the success criteria unmet: - instrument_fastapi was never called, so no FastAPI request traces would appear even with a token set. Wire it in main.py. - Enabling FastAPI instrumentation introduces a content-egress path the scrubber cannot reach: OTel records parsed endpoint arguments (request body + params) under `fastapi.arguments.values`, which Logfire does not route through scrub_value (a field named e.g. `body` matches no risky pattern). Drop those arguments at the source via a request_attributes_mapper that returns None, keep headers off (capture_headers=False), and keep the extra argument/endpoint spans off (extra_spans=False). No prompts, completions, chat messages, note bodies, quiz answers, or uploaded document text leave the process on request spans. Also: - Add LOGFIRE_TOKEN to .env.example (optional; dormant when unset via send_to_logfire="if-token-present") and surface it through config.py. - Document Logfire in docs/observability-logging-tracking.md: what it captures vs the owned Supabase events/llm_usage tables (independent, no double-count), how to enable, what is scrubbed, and the in-scope query-string caveat. - Tests: AST guards that fail if the argument-dropping mapper / header / span flags regress, plus an end-to-end test asserting a request body never lands in any exported span. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(flashcards): stop rate-limit retry-after overshooting the window check_rate_limit computed `int(_RATE_WINDOW_SEC - elapsed) + 1`, which returns 61 when the limited calls land in the same clock tick (elapsed == 0) — one second past the 60s window, and it tripped test_sixth_call_returns_retry_after (`assert 61 <= 60`). Use math.ceil of the true remaining time instead: it still rounds a sub-second remainder up to 1 (never 0) but is bounded by the window, so retry-after is always in [1, _RATE_WINDOW_SEC]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(agents): bound note_chat orchestrator + remaining worker agents with usage limits (#345) * fix(agents): bound note_chat + remaining worker agents with usage limits (#329) Residual from #327/#243: three run-sites still executed without usage_limits, defaulting to library maximums. - note_chat now runs under ORCHESTRATOR_LIMITS; guardrail trips (UsageLimitExceeded / UnexpectedModelBehavior) degrade to an in-band reply with degraded=true instead of an uncaught 500 (no legacy fallback exists for this path per ADR 0017). - note_summary / note_concepts run under WORKER_LIMITS via a shared _run_note_worker helper that converts guardrail trips to 503. - syllabus_extraction in calendar_service now passes WORKER_LIMITS; its caller already degrades gracefully. - Tests pin the usage_limits kwarg at all four run-sites and the new degrade/503 behavior. Closes #329 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes): use noun form in summarize 503 detail (CodeRabbit nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes,calendar): separate budget trips from model bugs in agent guardrails (#329) Review fixes for the usage-limit guardrails so a deterministic budget trip and a genuine model bug are no longer conflated: - notes worker (_run_note_worker): UsageLimitExceeded -> 413 with an honest "note too long, shortening may help" detail (no transient "try again" wording); UnexpectedModelBehavior -> 500 + logger.exception so a real bug pages us with a traceback instead of hiding behind a 503/WARNING. - note_chat: UsageLimitExceeded keeps the in-band degrade (its budget wording is now accurate); UnexpectedModelBehavior -> 500. Success path now returns degraded: false for schema symmetry with the degrade path. - calendar (extract_assignments_from_file): UsageLimitExceeded degrades with an honest "syllabus too long / split it" warning; model hiccups and bare exceptions keep the generic degrade. _degraded_result gains a `warning=` override. - tests: rewrite the guardrail tests to the new contract and dedup the fake-note fixture into one module-level factory. Note: this revises behavior previously asserted by test_503_when_guardrails_trip and the parametrized note_chat degrade test — UnexpectedModelBehavior is intentionally no longer treated as a budget trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com> * feat(frontend): test environment profile with same-origin API proxy (#380) (#421) Add build:test / start:test npm scripts that produce a production Next build targeting the local stack with ALL API traffic same-origin through the Next /api/:path* rewrite to the local FastAPI on :5000: - NEXT_PUBLIC_API_URL is set explicitly EMPTY so every client fetch is same-origin and the sapling_session cookie always rides along (the landing page falls back to cross-origin http://localhost:5000 when the var is merely unset). - BACKEND_URL=http://localhost:5000 bakes the rewrite destination and satisfies next.config.ts's production-build guard. - Local Supabase URL + demo anon key are inlined so the lazy lib/supabase.ts client initializes instead of throwing. - start:test supplies the runtime side: BACKEND_URL for the middleware session check and the fixed local SESSION_SECRET for the session route. All values are the committed-safe local defaults from .env.local.example, inlined in the scripts (real process env beats .env* files in Next, so the profile is deterministic regardless of a dev's .env.local). Zero new dependencies; middleware.ts and the production `npm run build` are untouched. Recipe documented in docs/local-supabase.md. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * refactor(ocr): route vision transcription through a Pydantic AI agent 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> * fix(ocr): cache key, cost ceiling, sequential rescuers, accurate docs 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> * feat(frontend): NEXT_PUBLIC_TEST_MODE determinism flag (#383) (#422) New src/lib/testMode.ts exports IS_TEST_MODE (build-time inlined), random() (mulberry32-seeded drop-in for Math.random), and now() (frozen 2026-03-11T12:00:00Z clock seam, overridable via globalThis.__SAPLING_TEST_NOW__). With the flag on: - KnowledgeGraph2D seeds its initial node positions and takes the reduced-motion path (synchronous fixed-tick settle) so two loads render identical coordinates. - KnowledgeGraph3D forces cooldownTicks=0 (the reduced-motion seam). - Landing page point cloud + floating cards park their rAF loops on a deterministic static frame; the frame's time read goes through now(). - AtmosphericBackdrop paints one still frame with seeded orbs. - HowItWorks/Study set framer-motion MotionGlobalConfig.skipAnimations. - Dashboard freezes the quote to index 0 and routes greeting, week strip, and relative labels through now(); Calendar (dueLabel, cursor, today) and Notetaker (relTime) do the same. Flag off, every seam passes through to Math.random()/Date.now() and no rAF/motion gate changes: production behavior is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * test(infra): one-command local stack boot — make e2e-up / e2e-down (#384) (#423) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: run the integration lane on every push to main (#402) (#427) The subcutaneous suite (#396–#398) currently runs only on manual workflow_dispatch — a real-DB lane that never runs protects nothing. Per epic #402's open decision 3 (lean: main-only first, promote to a PR gate once #388's stability bar holds), trigger it on every push to main while keeping manual dispatch. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: gate test_extraction_service.py — it needs none of the OCR stack 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> * test(e2e): Playwright harness and fixtures (#385) (#428) * test(e2e): Playwright harness and fixtures (#385) Browser-lane foundation for epic #402 — #386/#387/#392–#395 build on this. - frontend/playwright.config.ts: chromium-only, workers=1 (serial to start), retries=2 gated on CI, trace/video/screenshot on failure, JSON reporter (e2e/results/last-run.json) with per-attempt retry indices for #390 flake tracking, timezoneId pinned to America/New_York for the frozen #383 clock. No webServer block: the boot contract belongs to make e2e-up (#384); global-setup fails fast with the exact fix when the stack is down. - e2e/global-setup.ts: health-check the stack, mint a session for rich-user-active via POST /api/auth/test-login (#381) through the same-origin proxy, persist as storageState. - e2e/support/db.ts: the single DB seam — pg over 127.0.0.1:54322 (loopback-exact guard, mirroring #397), TRUNCATE mutable tables RESTART IDENTITY CASCADE with the #397 denylist, re-seed via the canonical db/seed_local_rich.py. - e2e/support/fixtures.ts: auto fixture resets the DB before each test; specs import test/expect from here. - e2e/smoke.spec.ts: one harness proof (authed /dashboard renders app-shell), deliberately not a journey. - build:test now bakes NEXT_PUBLIC_TEST_MODE=1 (the #383 flag; this composition is what it was built for). - ShellFrame: data-testid="app-shell" on both layout variants — the stable authed-shell anchor per the #382 convention. Verified against a cold make e2e-up boot: npx playwright test green twice in a row (truncate/re-seed isolation holds), tsc --noEmit, eslint, vitest (204 passed), and a plain production build all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): review fixes — testid process + comment accuracy (#385) - Follow docs/frontend-testids.md 'Adding a surface' for app-shell (missed in the initial commit): App shell row in the owning-files table, an `app` inventory section noting ShellFrame.tsx and the smoke-spec anchor role, and ShellFrame.tsx added to the eslint no-restricted-syntax scope (passes clean — the frame renders no intrinsic button/input/textarea). Doc's 'six files' phrasing generalized now that the list has seven. - global-setup.ts: correct the cookie-flags comment — auth.py only sets Secure under an https FRONTEND_URL (config.py), so the local cookie is HttpOnly/Lax; we mint secure:true and Chromium accepts it on http://localhost. - smoke.spec.ts: correct both redirect comments — unauthed /dashboard goes to ${BACKEND_URL}/api/auth/google via the middleware (BACKEND_URL is always set under start:test), not to the landing page. Verified: npx tsc --noEmit clean; npx eslint . 0 errors with ShellFrame.tsx newly in scope (scoped run at --max-warnings=0 clean); vitest 204/204. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(ocr): send the transcription prompt in the user turn, not as system 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> * test(e2e): journey — study room with two browser contexts (#394) (#431) Two signed-in contexts (rich-user-active + rich-user-second), one seeded room. Both contexts assert receipt of the other's message through the real propagation path — Supabase Realtime postgres_changes signal + decrypting REST re-fetch (#124) — and both users' knowledge graphs render. Zero waitForTimeout: cross-context sends only happen after each context's postgres_changes subscription is server-confirmed ("Subscribed to PostgreSQL" frame). Unblocking migrations (both verified-needed at runtime on the local migrations-only schema): - 0032: add the rooms columns routes/social.py already selects (topic/course/owner_id/updated_at/is_public) — bug #405 made every room listing endpoint 500 (verified: PostgREST 42703); columns stay nullable/unpopulated, the create_room semantics remain open in #405. - 0033: publish room_messages on supabase_realtime (guarded, idempotent) — verified empty publication locally; without it postgres_changes never fire, and the chat has no polling fallback. Harness additions (additive): e2e/support/session.ts mints a second user's storageState (cookie + the sapling_user localStorage identity that UserContext requires) via POST /api/auth/test-login; USER_SECOND joins stack.ts; global-setup.ts takes the #386 branch's localStorage fix verbatim so sibling PRs converge on identical content. Social.tsx joins the #382 data-testid convention (social-* inventory in docs/frontend-testids.md, eslint files array). Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(ocr): per-run provider — every second vision call died on a closed 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> * test(e2e): journey — seeded session → dashboard (#386) (#429) * test(e2e): journey — seeded session → dashboard (#386) Co-Authored-By: Claude Fable 5 <norepl…
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworked 2026-07-29: the original fresh-client sweep + subject_root dedup were superseded on main by the #447–#454 batch (
_LoopSafeGoogleModelin #453 solved cross-loop client safety properly; the #355 dedup landed with its own regression test). This PR now carries only the piece main still lacks:run_agent_syncrunning-loop guard — called from an async handler's sync call chain (e.g.build_system_prompt→get_course_context), it used to letasyncio.runraise opaquely and leak a "coroutine was never awaited" warning. Now it closes the coroutine and raises a clearRuntimeErrorthe try/except-guarded callers degrade on.HEALTH_PROBE_MODELconstant inagents/_providers.py, used byagents/health.py, so probe sites can't drift.tests/test_run_agent_sync_loop.py).The streaming-tutor interrupt/retry ADR that was on this branch moves to #349 (main's 0019 slot is now taken by the SAPLING_MODEL_MODE seam ADR).
Testing
pytest tests/ -q: 1181 passed, 27 skipped.ruff check .: clean (one pre-existing hit in an untracked local ops script only).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests