Uh oh!
There was an error while loading. Please reload this page.
feat(quiz): flip include_answer_key default to false (#546) - #590
Merged
Conversation
The #537 client grades every question through POST /attempts/{id}/answer and has sent `include_answer_key: false` on every /api/quiz/generate call since it shipped (frontend/src/lib/quiz/api.ts, tested in api.test.ts and modeled by the e2e quiz specs), so the generate response's per-option answer key is dead weight for the real caller. Flip GenerateQuizBody.include_answer_key's default to false; the field itself stays accepted-but-logged for one release (explicit `include_answer_key: true` still returns `correct` booleans and logs the #546 deprecation breadcrumb) rather than being deleted outright. _INTERNAL_QUESTION_KEYS (provenance, question_hash) already stripped in both flag states, unaffected by this change. Tests: - test_quiz_answers_c.py::TestIncludeAnswerKey: rewritten for the new default (test_default_now_strips_the_key asserts the omitted-flag path is keyless and does NOT log; test_explicit_true_keeps_the_key_and_logs covers the still-accepted opt-in path) + a new hermetic test (test_default_response_is_the_keyless_projection_of_the_real_stored_key) that grounds the "no leak" claim against the server's own _strip_answer_key projection of the real encrypted questions_json, plus a generic recursive key-name walk, rather than just checking `correct` is absent. - test_quiz_routes.py::test_returns_agent_output_in_legacy_wire_shape: now opts in explicitly (`include_answer_key: true`) since it pins the full keyed wire shape, which is no longer the default. - tests/integration/test_quiz_subcutaneous_db.py (#545): added the real-Postgres twin of the hermetic grounding test above (test_generate_default_response_never_reveals_the_correct_option), marked integration; unrun here (needs the local stack + function mode), collection verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…view r1)
Fix round 1 on the task-3 review (task-3-findings-r1.md):
Important 1 — the recursive key-name walk in both the hermetic test
(test_quiz_answers_c.py) and its subcutaneous DB twin
(test_quiz_subcutaneous_db.py) only covered `{"questions": served}`,
missing generate's five other top-level fields (quiz_id,
requested_difficulty, resolved_difficulty, requested_count,
delivered_count). A future sibling key (e.g. `answer_key`) would have
sailed past it. Both now walk the full response body.
Important 2 — `served == _strip_answer_key(stored)` compared the
route's default-path output against the SAME function that produced
it, so it could not catch a leak introduced inside that projection
(or an accidental widening of `_KEYLESS_*_KEYS`) — falsifiable by
widening the allowlist while both sides still move together. Replaced
with a non-circular anchor: each served question's key set must be a
subset of, and each option's key set must equal exactly, a HARD-CODED
literal written in the test (not imported from routes.quiz). Verified
this actually discriminates by temporarily reverting the model
default to True and confirming both tests fail with "unexpected
key(s): {'explanation'}" before restoring it. Docstrings on both
tests rewritten to stop overclaiming ("any leak... shows up as a
diff") and describe the key-name walk as a heuristic backstop, not a
proof.
M4 — test_quiz_subcutaneous_db.py's new test now guards
`_attempt_row(...)` with an `is not None` assertion before
subscripting, matching the neighbouring test's pattern.
M6 — both caplog assertions in TestIncludeAnswerKey now filter
`caplog.records` by `rec.name == "routes.quiz"` before scanning for
the deprecation breadcrumb, since caplog's handler captures every
propagating logger.
M3 (frontend/src/lib/quiz/api.ts:32-33 now overstates #546's tonight
scope) is note-only per the reviewer; not fixed here — noted in the
task report and PR body.
Covering tests: tests/test_quiz_answers_c.py (16 -> unchanged count,
docstrings/assertions only) + tests/test_quiz_provenance_e5_e6.py —
40 passed. tests/integration/test_quiz_subcutaneous_db.py collect-only
verified (needs the real stack to execute). Full hermetic suite:
2225 passed, 80 skipped (unchanged from before this fix round).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | ac13c62 | Commit Preview URL Branch Preview URL | Aug 26 2026, 06:06 AM |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
…w r2) The one-release grace window for `include_answer_key` was gated on a `logger.info`: nothing rolls log lines up, and the breadcrumb only ever fired on an explicit true, so the population the window actually exists for — flag-unaware callers that OMIT the field and silently changed response shape at the flip — had no telemetry at all (F1). Distinguish the three populations by `model_fields_set` (was the field on the wire?) rather than by value, and emit an events_service event per population so the count lands in the #375 admin-analytics by_event_type rollup with no schema or endpoint work (F4, #117 convention): * explicit true -> quiz.answer_key_served (the count that must reach zero before the parameter is deleted; keeps the log line too) * omitted -> quiz.answer_key_flag_omitted (deletion is a no-op for them, but they are the shape-change population) * explicit false -> nothing; every shipped #537 client sends this on every generate and would swamp the rollup Two event types rather than one with a `flag` payload field because by_event_type does not break payloads out: one type would show a single number mixing the population that blocks deletion with the one that doesn't. Also canonicalizes the flag's lifecycle prose on the field declaration (F14) — it was restated at six sites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…review r2)
Test strength:
- Both "keyless projection of the real stored key" tests asserted the
served shape but never that it WAS the projection: add count equality
vs the decrypted stored questions, per-index id/stem/option label+text
correspondence, and delivered_count computed off the projected list
(F2, F3).
- The ~30-line leak check (allowlist + key-set loops + recursive walk)
was duplicated across the hermetic and subcutaneous lanes and had
already drifted; it moves to one `assert_keyless_projection` fixture in
tests/conftest.py, which both lanes use (F11). Sharing a test-side
helper is not the circularity the anchor guards against — the
allowlists are still literals, never imported from routes.quiz.
- Option key drift now reports the symmetric difference, so a MISSING key
reports itself instead of only extras (F9).
- Pin the exact top-level response key set (F12).
- Consolidate the default-{} and explicit-false cases via parametrize,
keeping the caplog-silence assert (F13).
- Cover the three telemetry populations, including the no-event case.
Comments:
- test_quiz_routes: the keyed shape is not "what submit_quiz expects" —
submit grades from stored questions_json and reads only
question_id/selected_label (F5).
- test_quiz_answers_c: the #537 client has ALWAYS sent explicit false;
the omitting population is flag-unaware callers (F6).
- test_quiz_provenance: the keyed branch is no longer the default — this
PR is the flip (F7).
- frontend quiz/api.ts: the default is false now, so the client's
explicit false is belt-and-braces, not load-bearing (F8).
- Fix the vacuity-guard wording: the anchors sit below, not above (F10).
- Parse the subcutaneous response once (F15).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 26, 2026
AndresL230 added a commit
that referenced
this pull request
Aug 28, 2026
Two conflicts, both in the files #590 and this branch both edited. models/__init__.py: #590 flipped `include_answer_key`'s default to false and replaced its comment with the canonical lifecycle account; this branch added `source_attempt_id` / `missed_question_hashes` and the validator that ties them together. Kept both — the flipped default and its prose verbatim, the G5 fields ahead of it, the validator after. routes/quiz.py: this branch relocated the agent-call except ladder out of `generate_quiz` into `_generate_or_502` so the re-serve branch can catch a failed top-up and still serve what it recovered; main (#592) added `has_graph=True` to the same call. Kept the relocation and carried the new argument (and its rationale) into the helper. #590's keyless projection, #591's attempt helpers and #592's hoisted lookups merged cleanly and are untouched — re-served questions still flow through the single `_client_questions` call, which strips `question_hash`/`provenance` on both the keyless and the opt-in keyed branch. Two G5 tests were written against the pre-flip default and are re-pointed: the keyed-projection test now opts in explicitly, and the keyless one adopts main's shared `assert_keyless_projection` fixture, which grounds the check in the answer key actually stored for the attempt. That fixture then caught G5's conditional top-level `source` block, so the shared key literal now names optional fields instead of requiring every response to carry them. The F5 assertions narrow to `quiz_reserve.missed_questions`: #592 makes an unrelated reporter fire on the same request under these mocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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.
Summary
The
#537client grades every question throughPOST /attempts/{id}/answerand already sends
include_answer_key: falseon every/api/quiz/generatecall (
frontend/src/lib/quiz/api.ts, pinned byapi.test.tsand the e2equiz specs), so the generate response's per-option answer key is dead weight
for the real caller. This flips
GenerateQuizBody.include_answer_key'sdefault from
truetofalse.The parameter itself is not deleted — an explicit
include_answer_key: truestill returns the full keyed shape and still logsthe
#546deprecation breadcrumb, kept accepted-but-logged for one releaseper the plan.
_INTERNAL_QUESTION_KEYS(provenance,question_hash) wasalready stripped in both flag states and is unchanged.
Key table (both flag states) and the frontend/e2e audit (no bug found — every
real caller already sends the flag explicitly) are in the task report.
Frontend note (no code change here):
frontend/src/lib/quiz/api.ts:32-33'scomment currently says "Removing the flag entirely is #546" — true of the
issue eventually, but this PR only flips the default; the comment now
overstates what landed tonight. Left as-is (out of this task's scope) — worth
a one-line tidy whenever the follow-up PR deletes the parameter.
Test plan
venv/bin/python -m pytest tests/ -q— 2225 passed, 80 skipped (was2223/79 on main; +1 new hermetic test passing, +1 new integration test
cleanly skipped without
RUN_INTEGRATION=1)alone), and RED confirmed for the new grounded test specifically (by
temporarily reverting the default and rerunning) — see report for both
transcripts
venv/bin/ruff check— clean on all changed filesfrontend/e2e/quiz*.spec.ts+ oracles) — not run bythe implementer per contract; controller's lane run
tests/integration/test_quiz_subcutaneous_db.py(new test included) —needs
RUN_INTEGRATION=1+ local stack + function mode; collectionverified only, controller's lane run
Refs #546
Lane runs (overnight 2026-08-23, local stack under the flock, function mode)
gradebook.spec.ts:35("a course taken in two terms opens the right enrollment from each chip", "Exams" not visible after 7.8 s). Unrelated to this diff (quiz model default + comments + tests); the same spec passed in two other lanes on the same base tonight and 6/6 on a ×3 re-run of this branch. Worth a look for the test(e2e): browser-lane stability gate — 20 consecutive green runs #388 zero-flake gate.tests/integration/test_quiz_subcutaneous_db.py), so the full-response walk + hard-coded key-set allowlist ran against real HTTP under the seam.questionsonly, grounding assertion circular through_strip_answer_key→ both fixed) + scoped re-review clean.Merge-gate review (2026-08-26)
/code-reviewat the merge gate: the flip itself reviewed safe for everyin-repo caller. All 15 findings were telemetry, test strength, or stale
comments; all are fixed in
c412db8a+ac13c627.Deprecation telemetry, made countable (F1, F4)
The grace window was gated on a
logger.info— nothing rolls log lines up,and the breadcrumb only ever fired on an explicit
true, so the populationthe window actually exists for (callers that OMIT the flag, whose response
shape silently changed at the flip) had zero telemetry. The route now
tells the three populations apart by
"include_answer_key" in body.model_fields_set— was the field on the wire at all — rather than byits value, and emits an
events_serviceevent per population (#117), whichlands in the #375 admin-analytics
by_event_typerollup with no schema orendpoint work:
include_answer_key: truequiz.answer_key_served(usage, payload{quiz_id})quiz.answer_key_flag_omitted(usage, payload{quiz_id})include_answer_key: falseTwo event types rather than one carrying a
flagpayload field: theby_event_typerollup does not break payloads out, so a single type wouldsurface one number mixing the population that blocks deletion with the one
that doesn't — exactly the distinction the gate needs. Both are added to
EVENT_TAXONOMY(+ the pin intest_event_capture_seams.py); no migration,no agent/seam change, and the emit is fire-and-forget like
quiz.started25 lines above it.
Projection tests, grounded (F2, F3, F9, F11, F12)
Both "keyless projection of the real stored key" tests asserted the served
shape but never that it actually was the projection: they would have
passed on an empty
questionslist. They now assert count equality againstthe decrypted stored questions, per-index
id/stem/optionlabel+textcorrespondence, and
delivered_countcomputed off the projected list. The~30-line leak block (allowlist + key-set loops + recursive walk) was
duplicated across the hermetic and subcutaneous lanes and had already
drifted; it is now one
assert_keyless_projectionfixture inbackend/tests/conftest.pythat both lanes use (the allowlists stay literalswritten test-side, never imported from
routes.quiz, so the non-circularityargument is unchanged). Option key drift is reported as a symmetric
difference, so a missing key reports itself; the exact top-level response
key set is now pinned.
Comment sweep (F5–F8, F10, F13–F15)
Four docstrings asserted things that stopped being true at the flip (the
keyed shape is not "what
submit_quizexpects" — submit grades from thestored
questions_jsonand consumes onlyquestion_id/selected_label; the#537 client has always sent explicit
false; the keyed branch is no longerthe default). The lifecycle prose was restated at six sites and is now
canonical on
GenerateQuizBody.include_answer_keywith the rest pointing atit. The
frontend/src/lib/quiz/api.tscomment noted-but-not-fixed above isfixed here (F8) — it claimed the default is
trueand framed the client'sexplicit
falseas load-bearing; it is now belt-and-braces, and the commentsays why the client keeps sending it anyway (so it stays out of the
quiz.answer_key_flag_omittedcount). Default-{}and explicit-falseareconsolidated under one
parametrize, keeping the caplog-silence assert.Merge-gate verification
venv/bin/python -m pytest tests/ -q -p no:cacheprovider— 2229passed, 80 skipped (+4 telemetry tests vs. the run above)
change (
quiz.answer_key_served/..._flag_omittedabsent from boththe sink and
EVENT_TAXONOMY), green aftervenv/bin/ruff check .— All checks passednpx tsc --noEmit(only a comment changed inapi.ts) — cleantests/integration/test_quiz_subcutaneous_db.pycollect +--setup-plan— 15 collected,
assert_keyless_projectionresolves in that lane