Uh oh!
There was an error while loading. Please reload this page.
test(e2e): journey — tutor conversation persists to messages (#392) - #434
Conversation
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 70441ef | Commit Preview URL Branch Preview URL | Jul 28 2026, 03:25 AM |
Warning Review limit reached
Next review available in:43 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 (11)
📝 WalkthroughWalkthroughThe PR adds environment-based function-handler loading, deterministic tutor responses, non-real model routing, and browser E2E coverage for tutor rendering and encrypted message persistence. ChangesDeterministic tutor E2E flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TutorE2E
participant Learn
participant Backend
participant Database
participant DecryptHelper
TutorE2E->>Learn: Resume seeded session
Learn->>Backend: Submit tutor message
Backend->>Database: Store encrypted messages
Backend->>Learn: Return deterministic tutor reply
TutorE2E->>Database: Read session messages
TutorE2E->>DecryptHelper: Decrypt stored content
DecryptHelper->>TutorE2E: Return plaintext values
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| import pytest | ||
| from pydantic_ai.models.google import GoogleModel | ||
| import agents._providers as providers |
Uh oh!
There was an error while loading. Please reload this page.
…port (#434 review) Review fixes on PR #434: - _providers.py latched _ENV_HANDLERS_LOADED BEFORE importing the env module, permanently caching a failed import as loaded: the first dispatch raised ModuleNotFoundError correctly, every later one silently downgraded to the generic LookupError. Latch now sets only after a successful import (or when the var is unset), so a broken boot fails loudly on EVERY dispatch. Regression test dispatches twice against a bad module path within one latch lifetime (verified red on the old order, green on the fix). - Learn.tsx joins the eslint testid enforcement files array; its 21 pre-existing untagged elements are baselined via npm run lint:baseline (the documented legacy-debt mechanism, matching #429's Dashboard.tsx treatment) instead of the prose lint exception, and the doc's Tutor table row now names the ChatPanel/Learn.tsx split. - Qualified the "pytest lane never sets the var / never imports it" comments to the normal (hermetic) lane — the seam's own tests do both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
83e7951 to
30161b2CompareAndresL230
commented
Jul 28, 2026
Review fixes pushed (
Re-verification: backend 1076 passed, 1 skipped (+1 regression test), |
| module = (os.getenv("SAPLING_FUNCTION_HANDLERS") or "").strip() | ||
| if module: | ||
| importlib.import_module(module) # raises before the latch on failure | ||
| _ENV_HANDLERS_LOADED = True |
There 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 `@frontend/e2e/support/db.ts`:
- Around line 155-159: Update queryRaw to execute client.query(sql, params)
inside a read-only transaction, ensuring the transaction is explicitly
configured as read-only before returning rows. Preserve the existing withDb
usage, parameters, and result shape while preventing mutation statements through
this helper.
🪄 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: 179f9b80-dec5-417c-8b08-827b9a21f412
📒 Files selected for processing (11)
backend/agents/_providers.pybackend/agents/function_handlers_e2e.pybackend/routes/learn.pybackend/tests/test_e2e_function_handlers.pydocs/frontend-testids.mdfrontend/e2e/support/db.tsfrontend/e2e/support/decrypt.tsfrontend/e2e/tutor.spec.tsfrontend/eslint-suppressions.jsonfrontend/eslint.config.mjsfrontend/src/components/screens/Learn.tsx
| export async function queryRaw( | ||
| sql: string, | ||
| params: unknown[] = [], | ||
| ): Promise<Record<string, unknown>[]> { | ||
| return withDb(async (client) => (await client.query(sql, params)).rows); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Enforce the read-only query contract.
This accepts arbitrary SQL despite being documented as SELECT-only, so a future E2E assertion can mutate or truncate the local database through this helper. Execute it in a read-only transaction.
Proposed fix
export async function queryRaw(
sql: string,
params: unknown[] = [],
): Promise<Record<string, unknown>[]> {
- return withDb(async (client) => (await client.query(sql, params)).rows);+ return withDb(async (client) => {+ await client.query("BEGIN READ ONLY");+ try {+ return (await client.query(sql, params)).rows;+ } finally {+ await client.query("ROLLBACK");+ }+ });
}📝 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.
| exportasyncfunctionqueryRaw( | |
| sql: string, | |
| params: unknown[]=[], | |
| ): Promise<Record<string,unknown>[]>{ | |
| returnwithDb(async(client)=>(awaitclient.query(sql,params)).rows); | |
| exportasyncfunctionqueryRaw( | |
| sql: string, | |
| params: unknown[]=[], | |
| ): Promise<Record<string,unknown>[]>{ | |
| returnwithDb(async(client)=>{ | |
| awaitclient.query("BEGIN READ ONLY"); | |
| try{ | |
| return(awaitclient.query(sql,params)).rows; | |
| }finally{ | |
| awaitclient.query("ROLLBACK"); | |
| } | |
| }); | |
| } |
🤖 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 `@frontend/e2e/support/db.ts` around lines 155 - 159, Update queryRaw to
execute client.query(sql, params) inside a read-only transaction, ensuring the
transaction is explicitly configured as read-only before returning rows.
Preserve the existing withDb usage, parameters, and result shape while
preventing mutation statements through this helper.
AndresL230
commented
Jul 28, 2026
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. (Three review findings were fixed in-PR: the env-handlers failure latch now sets only after a successful import — regression-tested red→green with a dispatch-twice test; Learn.tsx joined eslint testid enforcement via the suppressions baseline; comment claims qualified.) 🤖 Generated with Claude Code |
Browser journey: resume the seeded recursion session, send a message through the real composer, assert the deterministic reply renders, then raw-SQL readback of the two new `messages` rows proving the encryption boundary (content is ciphertext at rest and decrypts to the sent text). Deterministic model responses via the #391 seam, extended for out-of-process runs: - agents/_providers.py: SAPLING_FUNCTION_HANDLERS names a module imported lazily (once) on a dispatch miss, so the E2E uvicorn process can register handlers at boot; unset var = #391 behavior unchanged. - agents/function_handlers_e2e.py: fixed-reply chat_tutor handler the E2E stack points the var at. - routes/learn.py::_resolve_model_pref: in any non-real mode, the browser's fast/smart pref no longer builds a live GoogleModel override (ModelToggle defaults to "fast", so every browser turn sends a pref — this silently bypassed the seam and dialed real Gemini). - tutor-session-resume-{sessionId} testid on the Learn screen's session rows (documented under the signin-trigger lint exception): the journey enters the chat by resuming, because start-session still runs on the legacy call_gemini_multiturn path the seam does not cover. - e2e/support/db.ts queryRaw (parameterized SELECT behind the loopback guard) + e2e/support/decrypt.ts (shells the backend venv's decrypt_if_present, mirroring the integration-suite readback pattern). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…port (#434 review) Review fixes on PR #434: - _providers.py latched _ENV_HANDLERS_LOADED BEFORE importing the env module, permanently caching a failed import as loaded: the first dispatch raised ModuleNotFoundError correctly, every later one silently downgraded to the generic LookupError. Latch now sets only after a successful import (or when the var is unset), so a broken boot fails loudly on EVERY dispatch. Regression test dispatches twice against a bad module path within one latch lifetime (verified red on the old order, green on the fix). - Learn.tsx joins the eslint testid enforcement files array; its 21 pre-existing untagged elements are baselined via npm run lint:baseline (the documented legacy-debt mechanism, matching #429's Dashboard.tsx treatment) instead of the prose lint exception, and the doc's Tutor table row now names the ChatPanel/Learn.tsx split. - Qualified the "pytest lane never sets the var / never imports it" comments to the normal (hermetic) lane — the seam's own tests do both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
30161b2 to
70441efCompareUh oh!
There was an error while loading. Please reload this page.
Summary
Browser journey for the tutor chat (#392): resume the seeded
rich-sess-cs-recursionsession on/learn, send a message through the real composer (tutor-input→tutor-send), assert the deterministic reply renders intutor-messages, then raw-SQL readback ofmessagesproving the encryption boundary:contentat rest is ciphertext (≠ the sent plaintext / rendered reply), anddecrypt_if_present, shelled through the backend venv — to exactly the sent text and the deterministic reply. Both directions are asserted becausedecrypt_if_presentechoes plaintext input back (legacy tolerance), so decrypt-equality alone can't prove encryption.No SSE handling and zero
waitForTimeout:sendChatis a plain fetch returning the full{ reply, … }object; assertions wait on the rendered reply locator.Boot contract
scripts/e2e-up.shdoes not scrub the environment — exported vars flow throughmakeinto thesetsid venv/bin/python -m uvicornlaunch, andbackend/.envdoesn't defineSAPLING_*, so no sanctioned-passing workaround was needed. The second var is new (see below).Seam findings (#391 coverage of the tutor path)
POST /api/learn/chatIS covered by the seam — it runschat_tutor(pydantic-ai) via_chat_via_agent, whose model comes frommodel_for("chat_tutor").routes/learn.py::_resolve_model_prefbuilt a liveGoogleModelviagoogle_model(name)for the per-request fast/smart pref, passed asagent.run(model=…)— overriding the FunctionModel. The frontend always sends a pref (ModelToggle'suseModelPrefdefaults to"fast"), so in function mode every browser tutor turn would still have dialed real Gemini. Fixed here: in any non-real mode the pref resolves toNone(the agent default, already mode-correct).realmode is byte-for-byte unchanged.routes/quiz.py::_resolve_model_pref(lines ~127–145) — left untouched (out of scope), flagged for the test(e2e): journey — quiz answer → mastery update (UI + DB) #393 quiz journey.POST /api/learn/start-sessionis NOT covered — it still runs the legacyservices/gemini_service.py::call_gemini_multiturnpath (explicitTODO(refactor-3 follow-up)in the route). The journey therefore enters the chat by resuming a seeded session, never via "Start learning". Same applies to/actionand/mode-switch(both legacy).register_function_handleris in-process only; ADR 0019 anticipated E2E lanes would "set the env at process start, then register per-task handlers" but shipped no mechanism for a separate uvicorn process. Added additively:SAPLING_FUNCTION_HANDLERS=<module>is imported lazily, once, only on a dispatch miss — the pytest lane (env unset, explicit registration) is unchanged, a missing handler still raises the pointedLookupError, explicit registrations always win, and a typo'd module path fails loudly (ImportError) instead of running handler-less.agents/function_handlers_e2e.pyholds the fixed-replychat_tutorhandler; other journeys should append theirs there._chat_via_agentcallsrag_service.retrieve_chunkswhenever the session's course has acourse_code(seeded CS101 does), and_embed_queryhitsgemini-embedding-001outside any seam.retrieve_chunksswallows every exception and returns[], so the journey's outcome does not depend on it — but each E2E tutor turn still attempts one billed embedding call whenbackend/.envcarries a real key. Worth a follow-up (seam or env kill-switch for embeddings).Verification
1 passed, 4.5–5.9s each, zero failed/flaky/retried, clean teardown. Boot line used: the contract above.tests/test_e2e_function_handlers.py: env-module autoload end-to-end through the realsocratic_agent, unset-var LookupError posture, bad-module loud failure, explicit-registration precedence, and_resolve_model_prefmode behavior both ways).ruff checkclean on changed files.tsc --noEmitandeslintclean on changed frontend files (one pre-existing baselined warning inLearn.tsx).ENCRYPTION_KEYbefore the runs.Testid note (deviation, with precedent)
The journey needs one new testid,
tutor-session-resume-{sessionId}, on the Learn screen's session rows — butscreens/Learn.tsxhas ~20 intrinsic interactive elements outside any browser journey, so adding it to the eslintfilesarray would force testids on all of them. Followed the documentedsignin-triggerexception instead: testid added + inventoried indocs/frontend-testids.mdwith the exception noted, file stays out of the lint block.The AI-disclosure
DisclaimerModal(fixed overlay, no testids) is pre-acked viaaddInitScript(models a returning user); dismissing it by copy would violate the no-copy-anchoring rule and tagging it was out of scope.Cross-PR convergence notes
frontend/e2e/global-setup.tsgained thesapling_userlocalStorage entry copied verbatim fromtest/386-journey-dashboard(per the harness-owner heads-up): the cookie-only storageState never populatesUserContext(it bootstraps identity solely from that localStorage key), so the Learn screen's session list — which this journey resumes from — stays empty without it. Identical content on both branches, so the merge is clean whichever lands first.SAPLING_FUNCTION_HANDLERS=agents.function_handlers_e2econtract and will carry its own version of the handlers module /_providers.pyautoload. These were authored in isolated worktrees — whoever merges second should converge to one module (append handlers per task) rather than duplicate the mechanism.Files
frontend/e2e/tutor.spec.ts— the journey (new)frontend/e2e/support/decrypt.ts— backend-venv decrypt seam (new)frontend/e2e/support/db.ts— additivequeryRaw(parameterized SELECTs behind the existing loopback guard)frontend/src/components/screens/Learn.tsx—tutor-session-resume-{sessionId}docs/frontend-testids.md— inventory + lint-exception note (append-only)backend/agents/_providers.py—SAPLING_FUNCTION_HANDLERSlazy autoload on dispatch missbackend/agents/function_handlers_e2e.py— deterministicchat_tutorhandler (new)backend/routes/learn.py—_resolve_model_prefrespectsSAPLING_MODEL_MODEbackend/tests/test_e2e_function_handlers.py— 6 unit tests (new)Part of #402, closes#392
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation