Uh oh!
There was an error while loading. Please reload this page.
docs: CLAUDE.md — E2E lanes, lock protocol, and seam conventions for agent sessions - #457
Merged
Merged
Conversation
…conventions 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>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | e97edfc | Commit Preview URL Branch Preview URL | Jul 29 2026, 04:16 AM |
Uh oh!
There was an error while loading. Please reload this page.
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCLAUDE.md adds a local E2E workflow covering stack startup, Playwright and backend checks, teardown, exploration, machine serialization, and function-mode integration guidance. ChangesE2E guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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>
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs-only. Makes the E2E system first-class in the project instructions every agent session loads:
make e2e-up/e2e-down, the Chapter 1 Playwright lane,python -m e2e_oracles,make explore(+ pointers todocs/e2e-exploration.md/docs/local-supabase.md).E2E_*constants are the seam not a bug, handler registration contract,UnregisteredHandlerError, no rawgenai.Clientbelow the seam without amodel_mode()gate).docs/e2e-exploration.mdadded.🤖 Generated with Claude Code
Summary by CodeRabbit