Uh oh!
There was an error while loading. Please reload this page.
test(e2e): journey — quiz answer → mastery update (UI + DB) (#393) - #437
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds lazy function-handler module loading, deterministic E2E tutor and quiz handlers, non-real-mode model override bypasses, updated quiz answer payloads, a mastery test ID, and browser/database assertions for all-correct quiz mastery updates. ChangesDeterministic quiz E2E flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 2b0a1c5 | Commit Preview URL Branch Preview URL | Jul 28 2026, 03:35 AM |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/routes/learn.py (1)
90-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated model-mode gate in
learn.pyandquiz.py. Both_resolve_model_prefimplementations add the identical_model_mode() != "real"early-return block to fix the same bypass; one root cause, two copies to keep in sync.
backend/routes/learn.py#L90-L98: extract the_model_mode() != "real"check (and its rationale comment) into a shared helper inbackend/agents/_providers.py(e.g.def block_pref_override_outside_real_mode() -> bool), and call it here.backend/routes/quiz.py#L141-L150: call the same shared helper instead of re-implementing the check.🤖 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/routes/learn.py` around lines 90 - 98, Extract the duplicated non-real model-mode guard and its rationale into a shared helper in backend/agents/_providers.py, such as block_pref_override_outside_real_mode(), returning whether the preference override must be blocked. In backend/routes/learn.py lines 90-98 and backend/routes/quiz.py lines 141-150, replace each local _model_mode() != "real" check and comment with a call to that helper while preserving the existing early-return behavior.frontend/e2e/support/db.ts (1)
147-160: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider enforcing the "SELECT-only" contract at runtime.
The docstring states
queryRawis for parameterized SELECTs only, but nothing stops a future caller from passing a mutating statement through this now-exported helper — bypassing the app-write/DB-read separation the rest of the file is deliberately built around (see the local-host guard inrequireLocalDbUrl).🛡️ Proposed guard
export async function queryRaw( sql: string, params: unknown[] = [], ): Promise<Record<string, unknown>[]> { + if (!/^\s*select\b/i.test(sql)) {+ throw new Error(+ `queryRaw is read-only: refusing non-SELECT statement: ${sql}`,+ );+ } return withDb(async (client) => (await client.query(sql, params)).rows); }🤖 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 147 - 160, Enforce the documented SELECT-only contract in the exported queryRaw helper before calling client.query: validate that the supplied SQL is a single parameterized SELECT statement, reject mutations and other statement types, and preserve the existing withDb local-host guard and readback behavior for valid queries.
🤖 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.
Nitpick comments:
In `@backend/routes/learn.py`:
- Around line 90-98: Extract the duplicated non-real model-mode guard and its
rationale into a shared helper in backend/agents/_providers.py, such as
block_pref_override_outside_real_mode(), returning whether the preference
override must be blocked. In backend/routes/learn.py lines 90-98 and
backend/routes/quiz.py lines 141-150, replace each local _model_mode() != "real"
check and comment with a call to that helper while preserving the existing
early-return behavior.
In `@frontend/e2e/support/db.ts`:
- Around line 147-160: Enforce the documented SELECT-only contract in the
exported queryRaw helper before calling client.query: validate that the supplied
SQL is a single parameterized SELECT statement, reject mutations and other
statement types, and preserve the existing withDb local-host guard and readback
behavior for valid queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f3d5839b-966d-4a82-a89e-6751a46ffce9
📒 Files selected for processing (9)
backend/agents/_providers.pybackend/agents/function_handlers_e2e.pybackend/routes/learn.pybackend/routes/quiz.pybackend/tests/test_e2e_function_handlers.pydocs/frontend-testids.mdfrontend/e2e/quiz.spec.tsfrontend/e2e/support/db.tsfrontend/src/components/QuizPanel.tsx
Browser journey: answer a scripted three-question quiz through the real /quiz UI and assert the mastery change in the UI AND in the database — graph_nodes.mastery_score plus exactly one append-only node_mastery_events row (0023), read back over the raw-SQL queryRaw seam so the assertion never rides the layer that wrote. Monotonicity pinned three ways: a fully correct submission never lowers the rendered score, the persisted score, or the event delta sign. The spec pre-acks the AI disclaimer via addInitScript (the modal intercepts every click for a browser that never acked — found on the first live run) and guards loudly that the stack is in function mode before answering. New quiz-results-mastery testid on the existing quiz surface (inventory row appended; QuizPanel.tsx is already in the eslint files array). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…del_pref bypass (#393) Append the quiz handler to agents/function_handlers_e2e.py per that module's own instruction (fixed three-question quiz; correct wire labels B, C, A exported as E2E_QUIZ_CORRECT_LABELS) and pin the contract in tests/test_e2e_function_handlers.py through the real quiz_agent and the real routes/quiz.py wire mapping — a drift fails hermetic CI instead of mid-browser-run. Also fixes the quiz half of the seam bypass #392's review found in learn.py: routes/quiz.py::_resolve_model_pref built a live GoogleModel for a fast/smart pref regardless of SAPLING_MODEL_MODE. The quiz UI sends no pref today (which is why the journey stayed deterministic regardless), but any client that did would silently dial Gemini in function mode — non-real modes now fall through to the agent's mode-built default, same pattern and tests as learn.py's fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nce the shell revamp Found by the #393 journey (first live run): POST /api/quiz/submit returned 422 because QuizPanel sends {question_id, selected} while backend models.AnswerItem requires selected_label — a wire-format break introduced by the frontend shell revamp (399eae3 removed the old selected_label payload). Every quiz submitted through the UI has failed since; no mocked test on either side could catch it (backend tests build AnswerItem directly, frontend tests mock fetch). Rename the request field to the backend contract — the RESPONSE items keep their `selected` key, which is a different shape and untouched. Tracked as issue #438. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230force-pushed
the
test/393-journey-quiz-mastery
branch
from
July 28, 2026 03:33
aebfbb1 to
2b0a1c5CompareAndresL230
commented
Jul 28, 2026
CollaboratorAuthor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. (Panel verified the #438 fix restores the exact pre-revamp submit contract, the quiz seam handler validates against the real output schema, and the mastery math matches the seeded baseline.) 🤖 Generated with Claude Code |
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.
Summary
Browser journey for the quiz → mastery path (#393): sign in as the seeded
rich-user-active(storageState), deep-link to/quiz?concept=rich-node-cs-recursion, answer a deterministic three-question quiz through the real UI, and assert the mastery change in the UI and in the database:graph_nodes.mastery_scoremoves 0.25 → 0.34 (3 correct × +0.03),times_studiedbumps 0 → 1,last_studied_atset;node_mastery_events(the 0023 table — never amastery_eventscolumn) withdelta ≈ +0.09and reasonQuiz: 3/3 correct;quiz_attemptsrow completes (score=3, total=3, completed_atset; scopedid NOT LIKE 'rich-%'because the seed carries a completed baseline attempt on the same node);quiz-results-mastery) equal the DB state read back over the raw-SQL seam — writes go through the app, assertions never ride the layer that wrote.Monotonicity — the property no mocked test can falsify — is pinned three ways: a fully correct submission must never lower the rendered score (
uiAfter >= uiBefore), the persisted score (masteryAfter >= masteryBefore), or the event delta sign (delta >= 0). If the scoring math inroutes/quiz.pyever regresses to subtract on correct answers, this journey fails.Product bugs found (2, both fixed here)
QuizPanelsubmits{question_id, selected}; backendmodels.AnswerItemrequiresselected_label. Introduced by399eae3("frontend: ship revamp shell, screens, and API client"), which dropped the oldselected_labelpayload; verified live onorigin/main. No mocked test on either side could catch it — backend tests constructAnswerItemdirectly, frontend tests mockfetch. The journey caught it on its first live run (POST /api/quiz/submit -> 422,.e2e/backend.log). Fixed by renaming the request field to the backend contract (the response items keep their separateselectedkey). Distinct from the known [P2] Quiz scoring & idempotency: double-submit double-counts, free point on malformed item #129 scoring bug, which this journey does not exercise (single submit per answer, well-formed items).routes/quiz.py::_resolve_model_prefbuilt a liveGoogleModelfor afast/smartpref regardless ofSAPLING_MODEL_MODE— same bug the test(e2e): journey — tutor conversation persists to messages #392 sibling fixed inroutes/learn.py, deliberately left to this PR. The quiz UI sends no pref today (generateQuizomitsmodel_pref, which is why this journey's runs stayed deterministic regardless), but any client that did would silently dial Gemini in function mode. Fixed with the same pattern + the same test shape as learn's.Seam findings (#391 / ADR 0019 / #392 convergence)
gemini_service.call_gemini_json;/api/quiz/generaterunsagents/quiz.py::quiz_agentbuilt viamodel_for("quiz"). Verified with evidence, not assumption: the spec's mode guard asserts the scripted fixture's question text before answering (a real-Gemini stack fails loudly there), generate round-trips in ~530 ms (.e2e/backend.log), and the only Gemini egress in a full run is the one below-seam embedding call (next bullet).SAPLING_MODEL_MODE=function SAPLING_FUNCTION_HANDLERS=agents.function_handlers_e2e make e2e-upreaches uvicorn by plain env inheritance;load_dotenv(main.py, config.py) never overrides exported vars. No script changes needed._load_env_handlers_moduleand its dispatch-twice regression test), this branch was rebased onto it: the shared seam files (_providers.py,learn.py,db.ts,global-setup.ts) collapsed into main entirely, and the PR diff is down to the quiz-specific surface — the quiz handler appended toagents/function_handlers_e2e.py(per that module's own instruction) and the quiz contract tests appended to its test file. The scripted quiz contract (labels B, C, A =E2E_QUIZ_CORRECT_LABELS) is pinned hermetically through the real agent + real wire mapping./api/quiz/generate's best-effort RAG grounding (_course_material_block→retrieve_chunks→_embed_query) sits below the model seam: with a real key inbackend/.enveach generate makes one livegemini-embedding-001:batchEmbedContentscall even in function mode (observed in.e2e/backend.log). It cannot affect the journey's outcome (the scripted handler ignores prompt content;course_chunksis empty post-reset; failures are swallowed by design) but it is incidental egress and a bounded (60 s) latency source the spec budgets for. Worth a follow-up to short-circuit grounding in non-real modes.quiz_contextdeliberately unregistered:submit_quiz's background context update runs intry/except pass; without a handler it fails fast with no post-response DB write racing the next test's truncate + re-seed.10-run tally
10/10 consecutive local runs green in one lock-held cycle (fresh
SAPLING_MODEL_MODE=function SAPLING_FUNCTION_HANDLERS=agents.function_handlers_e2e make e2e-upboot,make e2e-downafter; cycle exit rc=0):The consecutive greens double as isolation proof: every run asserts the seeded 0.25 baseline before playing, which would fail if the prior run's 0.34 write leaked past the per-test truncate + re-seed.
Post-rebase confirmation (after re-syncing onto main's merged #434 seam, incl. the latch fix): fresh lock-held cycle, 3/3 green — 8.4 s / 6.9 s / 6.8 s.
Harness changes (additive)
frontend/e2e/quiz.spec.ts— the journey. ZerowaitForTimeout; locator/event waits only. Pre-acks the AI disclaimer viaaddInitScript(the modal intercepts every click for a browser that never acked; scoped to the spec soglobal-setup.tsstays byte-identical to the test(e2e): journey — seeded session → dashboard #386/test(e2e): journey — study room with two browser contexts #394 content already on main).frontend/src/components/QuizPanel.tsx+docs/frontend-testids.md— one new testid on the existingquizsurface,quiz-results-mastery(inventory row appended; the file is already in the eslintfilesarray), plus theselected_labelfix above. DB readback uses main'squeryRawhelper (merged with test(e2e): journey — tutor conversation persists to messages (#392) #434) — no harness-file changes remain in this diff.Backend hermetic suite: 1080 passed, 23 skipped on the rebased branch. Frontend: tsc clean, eslint 0 errors, vitest 204 passed.
Part of #402, closes#393
🤖 Generated with Claude Code