Skip to content

Commit a2fd5cd

Browse files
fix(quiz): symmetric "fast" semantic across agent + legacy paths
Closes the asymmetry I flagged in the most recent review of PR #71. Was: agent path "fast" → gemini-2.5-flash (upgrade), legacy path "fast" → gemini-2.5-flash-lite (no-op). Same body field, different models depending on which path ran. The kind of inconsistency that surfaces six months later as "why does Fast sometimes feel slower than Default?" Now: agent path AND legacy fallback both use the same mapping ("fast" → flash, "smart" → pro, else → lite). _PREF_MODEL_NAMES is the single source of truth on the agent side; the legacy elif-chain in _legacy_generate_quiz mirrors it exactly. Also: - backend/routes/quiz.py: re-imported MODEL_DEFAULT alongside MODEL_LITE/MODEL_SMART. Tightened the lazy-import comment in _resolve_model_pref to describe the actual benefit (deferring GoogleProvider construction to the first override request, not import-path isolation — agents.quiz is already imported at module top). - docs/decisions/0013-refactor-2-quiz-shipped.md: addendum reworded to "two independent layers, not multiplicative" — the env var sets the agent's startup baseline; the body field, when present, fully replaces it for the current call. Adds a paragraph documenting the agent/legacy symmetry and pointing at _PREF_MODEL_NAMES as the single source of truth. Tests (TestQuizModelPref grew to 7) - test_legacy_fallback_uses_default_when_pref_fast (NEW): pins the symmetry — legacy "fast" must call gemini with MODEL_DEFAULT, not MODEL_LITE. Asserts a clear failure message if anyone undoes the fix. - test_legacy_fallback_uses_lite_when_no_pref (NEW): pins the default — no model_pref still uses MODEL_LITE (the cheap baseline that's been there since the route shipped). - Existing 5 tests unchanged. Tests - tests/test_quiz_routes.py: 30/30 (was 28, +2). - Full backend suite: 533 passed, 3 pre-existing live-Supabase failures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ddd109b commit a2fd5cd

3 files changed

Lines changed: 87 additions & 12 deletions

File tree

backend/routes/quiz.py

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
from models import GenerateQuizBody, SubmitQuizBody
1616
from services.auth_guard import require_self
1717
from services.encryption import decrypt_if_present
18-
from services.gemini_service import MODEL_LITE, MODEL_SMART, call_gemini_json
18+
from services.gemini_service import MODEL_DEFAULT, MODEL_LITE, MODEL_SMART, call_gemini_json
1919
from services.graph_service import get_graph, update_streak
2020
from services.quiz_context_service import get_quiz_context, save_quiz_context
2121
from services.fingerprint import fingerprint
@@ -121,9 +121,13 @@ def _resolve_model_pref(model_pref: str | None):
121121
"""Build a GoogleModel override for the per-request fast/smart
122122
preference, or return None to use the agent's default.
123123
124-
Lazy import keeps the agents module out of the route's import path
125-
until the override is actually requested — keeps test fixtures that
126-
patch routes.quiz.quiz_agent.run from re-instantiating providers.
124+
`google_model` is imported lazily so that constructing a
125+
GoogleProvider (which reads GEMINI_API_KEY at call time) only
126+
happens when an override is actually requested — not at module
127+
import. agents.quiz is already in this route's import graph, so
128+
this isn't about import-path isolation; it's about deferring the
129+
one runtime side-effect (the provider build) to the request that
130+
needs it.
127131
"""
128132
if not model_pref:
129133
return None
@@ -265,10 +269,18 @@ async def _legacy_generate_quiz(body: GenerateQuizBody, request: Request) -> lis
265269
)
266270
prompt += "\n\n" + "\n\n".join(addendum_parts)
267271

268-
# Honor the same fast/smart toggle the agent path uses. "smart"
269-
# routes to gemini-2.5-pro; anything else (including "fast" and
270-
# None) stays on the existing flash-lite default.
271-
legacy_model = MODEL_SMART if body.model_pref == "smart" else MODEL_LITE
272+
# Honor the same fast/smart toggle the agent path uses. The mapping
273+
# mirrors _PREF_MODEL_NAMES exactly, so a user choosing "fast" gets
274+
# the same upgraded model whether the agent succeeded or fell back
275+
# here. Without this, "fast" was silently a no-op on the legacy
276+
# path (still MODEL_LITE) — the kind of inconsistency that surfaces
277+
# six months later as "why does Fast sometimes feel slower?"
278+
if body.model_pref == "smart":
279+
legacy_model = MODEL_SMART # gemini-2.5-pro
280+
elif body.model_pref == "fast":
281+
legacy_model = MODEL_DEFAULT # gemini-2.5-flash, matches _PREF_MODEL_NAMES
282+
else:
283+
legacy_model = MODEL_LITE # gemini-2.5-flash-lite, agent default per ADR 0008
272284
try:
273285
result = call_gemini_json(prompt, model=legacy_model)
274286
except Exception as e:

backend/tests/test_quiz_routes.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,3 +731,55 @@ def test_legacy_fallback_uses_smart_when_pref_smart(self):
731731
# call_gemini_json was called with model=MODEL_SMART, not MODEL_LITE.
732732
gemini_mock.assert_called_once()
733733
assert gemini_mock.call_args.kwargs.get("model") == MODEL_SMART
734+
735+
def test_legacy_fallback_uses_default_when_pref_fast(self):
736+
"""Symmetry contract: legacy "fast" must upgrade to MODEL_DEFAULT
737+
(gemini-2.5-flash) just like the agent path does. Without this,
738+
a Fast request that trips the agent silently downgrades to
739+
MODEL_LITE — the same kind of inconsistency users would feel
740+
as "Fast sometimes gives worse quizzes than Default."
741+
"""
742+
from services.gemini_service import MODEL_DEFAULT, MODEL_LITE
743+
run_mock = AsyncMock(side_effect=RuntimeError("agent boom"))
744+
gemini_mock = MagicMock(return_value={"questions": [{
745+
"id": 1, "question": "Q?",
746+
"options": [{"label": "A", "text": "x", "correct": True}],
747+
"explanation": ".", "concept_tested": "X", "difficulty": "easy",
748+
}]})
749+
with (
750+
patch("routes.quiz.table", side_effect=_generate_table_factory()),
751+
patch("routes.quiz.quiz_agent.run", new=run_mock),
752+
patch("routes.quiz.get_graph", return_value={"nodes": [], "edges": []}),
753+
patch("routes.quiz.get_quiz_context", return_value={}),
754+
patch("routes.quiz.call_gemini_json", new=gemini_mock),
755+
):
756+
r = self._post({"model_pref": "fast"})
757+
assert r.status_code == 200
758+
gemini_mock.assert_called_once()
759+
chosen = gemini_mock.call_args.kwargs.get("model")
760+
assert chosen == MODEL_DEFAULT, (
761+
f"Legacy fast→{chosen!r} expected MODEL_DEFAULT (gemini-2.5-flash); "
762+
f"do NOT downgrade to MODEL_LITE (={MODEL_LITE!r})."
763+
)
764+
765+
def test_legacy_fallback_uses_lite_when_no_pref(self):
766+
"""Default path (no model_pref) keeps using MODEL_LITE — the
767+
cheap baseline that's been in place since the route shipped."""
768+
from services.gemini_service import MODEL_LITE
769+
run_mock = AsyncMock(side_effect=RuntimeError("agent boom"))
770+
gemini_mock = MagicMock(return_value={"questions": [{
771+
"id": 1, "question": "Q?",
772+
"options": [{"label": "A", "text": "x", "correct": True}],
773+
"explanation": ".", "concept_tested": "X", "difficulty": "easy",
774+
}]})
775+
with (
776+
patch("routes.quiz.table", side_effect=_generate_table_factory()),
777+
patch("routes.quiz.quiz_agent.run", new=run_mock),
778+
patch("routes.quiz.get_graph", return_value={"nodes": [], "edges": []}),
779+
patch("routes.quiz.get_quiz_context", return_value={}),
780+
patch("routes.quiz.call_gemini_json", new=gemini_mock),
781+
):
782+
r = self._post({}) # no model_pref
783+
assert r.status_code == 200
784+
gemini_mock.assert_called_once()
785+
assert gemini_mock.call_args.kwargs.get("model") == MODEL_LITE

docs/decisions/0013-refactor-2-quiz-shipped.md

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,10 +182,21 @@ fast → flash-flash override, no-pref falls through, unknown pref
182182
falls through, and legacy fallback honors smart.
183183

184184
Decision rationale: keep the env var (`SAPLING_MODEL_QUIZ`) for ops
185-
defaults, AND accept `model_pref` for per-request overrides. They
186-
compose — the env var sets the agent's startup default; the body
187-
field overrides per call. Same shape as the chat tutor on main, so
188-
the two AI-driven routes are now symmetric.
185+
defaults AND accept `model_pref` for per-request overrides. They are
186+
two independent layers, not multiplicative — the env var sets the
187+
agent's startup baseline (`model_for("quiz")` reads it at process
188+
start); the body field, when present, fully replaces that default
189+
for the current call by passing `model=...` to `quiz_agent.run`.
190+
Same shape as the chat tutor on main, so the two AI-driven routes
191+
are now symmetric.
192+
193+
`fast` and `smart` resolve identically across the agent path AND the
194+
legacy fallback (commit ddd109b initially missed this; it was fixed
195+
in the same iteration so a `fast` request that trips the agent
196+
guardrails still gets `gemini-2.5-flash`, not silently downgraded to
197+
`gemini-2.5-flash-lite`). The route's `_PREF_MODEL_NAMES` table is
198+
the single source of truth; the legacy `else`/`elif` chain mirrors
199+
it exactly.
189200

190201
## Pre-existing test failures (not caused by this refactor)
191202

0 commit comments

Comments
 (0)