From a3d1d1740eadc9ec778b6c2e6eeec507dde81fde Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:03:19 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(quiz):=20repair=20the=20adaptive=20cont?= =?UTF-8?q?ext=20loop=20=E2=80=94=20restore=20UNIQUE,=20stop=20swallowing,?= =?UTF-8?q?=20consume=20the=20full=20digest=20(#529)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Workstream B of the pre-revamp quiz repair batch (epic #537): B1 — migration 20260812210033 restores UNIQUE (user_id, concept_node_id) on quiz_context (dropped by 0025's table recreate), dedup-guarded and idempotent. Staging/prod measured 0 rows + 0 dupes — the very first upsert already 42P10'd, so nothing ever accumulated. B2 — save_quiz_context's on_conflict target now has a matching constraint again; pinned by test. B3 — the post-submit background write is loud on failure: ERROR log with attempt id + request id, a quiz.context_write_failed analytics event (added to the pinned EVENT_TAXONOMY), and a re-raise under config.IS_LOCAL so this regression class fails CI. The E2E seam's UnregisteredHandlerError stays a single WARNING with no traceback — quiz_context is deliberately unregistered in function mode and the logscan oracle treats tracebacks as findings. B4 — the read side now consumes the WHOLE QuizContext shape: _coerce_summary previously returned `notes` alone and dropped weak_areas / common_mistakes / questions_seen_summary (its list fallback also looked for `common_errors`, a key QuizContext never writes). Tool-level round-trip test covers ciphertext row → decrypt → digest in QuizHistory.summary. B5 — real-DB integration tests (tests/integration/ test_quiz_context_repair_db.py): constraint present, double-upsert keeps one row, raw column is ciphertext, app read round-trips. Co-Authored-By: Claude Fable 5 --- backend/agents/tools/quiz_history.py | 53 +++- ...0812210033_restore_quiz_context_unique.sql | 32 ++ backend/routes/quiz.py | 41 ++- backend/services/events_service.py | 4 + .../test_quiz_context_repair_db.py | 71 +++++ backend/tests/test_event_capture_seams.py | 3 +- backend/tests/test_quiz_context_repair.py | 297 ++++++++++++++++++ 7 files changed, 481 insertions(+), 20 deletions(-) create mode 100644 backend/db/migrations/20260812210033_restore_quiz_context_unique.sql create mode 100644 backend/tests/integration/test_quiz_context_repair_db.py create mode 100644 backend/tests/test_quiz_context_repair.py diff --git a/backend/agents/tools/quiz_history.py b/backend/agents/tools/quiz_history.py index 14eb7466..e81f0b9b 100644 --- a/backend/agents/tools/quiz_history.py +++ b/backend/agents/tools/quiz_history.py @@ -57,32 +57,53 @@ class QuizHistory(BaseModel): recent_attempts: list[RecentQuizAttempt] = Field(default_factory=list) +# String fields worth surfacing, in display order. `questions_seen_summary` +# and `notes` are what agents/quiz_context.py::QuizContext actually writes; +# summary/context/digest cover older free-form rows. +_SUMMARY_STRING_KEYS = ("summary", "questions_seen_summary", "notes", "context", "digest") + +# List-of-strings fields, with a label so the agent knows what each block is. +# `weak_areas`/`common_mistakes` are the live QuizContext field names; +# misconceptions/common_errors cover older rows. +_SUMMARY_LIST_KEYS = ( + ("weak_areas", "Weak areas"), + ("common_mistakes", "Common mistakes"), + ("misconceptions", "Misconceptions"), + ("common_errors", "Common errors"), +) + + def _coerce_summary(ctx: Any) -> str | None: """quiz_context.context_json is free-form (whatever the post-submit - LLM produced). Different prompt versions have stored either a flat - string or a small dict. Coerce to a single string the agent can - reason over, or None if there's nothing useful.""" + LLM produced). Coerce to a single string the agent can reason over, + or None if there's nothing useful. + + #529/B4: this must consume the WHOLE QuizContext shape. The old + version returned the first matching string key — for a live + QuizContext row that was `notes` alone, silently dropping + weak_areas / common_mistakes / questions_seen_summary (and its list + fallback looked for `common_errors`, a key QuizContext never writes). + """ if not ctx: return None if isinstance(ctx, str): text = ctx.strip() return text or None if isinstance(ctx, dict): - # Common shapes: {"summary": "..."}, {"notes": "..."}, - # {"misconceptions": [...], "weak_areas": [...]}. - for key in ("summary", "notes", "context", "digest"): + parts: list[str] = [] + for key in _SUMMARY_STRING_KEYS: v = ctx.get(key) if isinstance(v, str) and v.strip(): - return v.strip() - # Fall back to flattening list-of-strings entries so the agent - # at least sees the misconceptions/weak_areas the prior job - # extracted, even when no top-level summary string exists. - parts: list[str] = [] - for key in ("misconceptions", "weak_areas", "common_errors"): - for item in ctx.get(key) or []: - if isinstance(item, str) and item.strip(): - parts.append(f"- {item.strip()}") - return "\n".join(parts) or None + parts.append(v.strip()) + for key, label in _SUMMARY_LIST_KEYS: + items = [ + item.strip() + for item in (ctx.get(key) or []) + if isinstance(item, str) and item.strip() + ] + if items: + parts.append(label + ":\n" + "\n".join(f"- {i}" for i in items)) + return "\n\n".join(parts) or None return None diff --git a/backend/db/migrations/20260812210033_restore_quiz_context_unique.sql b/backend/db/migrations/20260812210033_restore_quiz_context_unique.sql new file mode 100644 index 00000000..1096579b --- /dev/null +++ b/backend/db/migrations/20260812210033_restore_quiz_context_unique.sql @@ -0,0 +1,32 @@ +-- Repairs 0025 (#529): quiz_context lost UNIQUE (user_id, concept_node_id). +-- +-- 0001_baseline_schema.sql created the table with that UNIQUE inline; +-- 0025_study_integrity.sql dropped and recreated quiz_context (lines 108-114) +-- WITHOUT it. services/quiz_context_service.py's upsert names those columns in +-- on_conflict, so PostgREST rejected every write with 42P10 — and because the +-- caller swallowed the exception, the adaptive-context loop was silently dead +-- from 2026-06-23 until this repair. Staging and prod both measured 0 rows and +-- 0 duplicate pairs on 2026-08-12 (the very first write already failed, so +-- nothing accumulated), but local replicas replay independently — dedup anyway. + +-- Keep the newest row per (user_id, concept_node_id); report what was removed. +DO $$ +DECLARE removed integer; +BEGIN + DELETE FROM quiz_context qc + USING quiz_context newer + WHERE qc.user_id = newer.user_id + AND qc.concept_node_id = newer.concept_node_id + AND (qc.updated_at < newer.updated_at + OR (qc.updated_at = newer.updated_at AND qc.id < newer.id)); + GET DIAGNOSTICS removed = ROW_COUNT; + RAISE NOTICE 'quiz_context dedup before UNIQUE restore: % duplicate row(s) removed', removed; +END $$; + +-- Idempotent restore. Named explicitly (0001's inline UNIQUE got the default +-- name quiz_context_user_id_concept_node_id_key; this repair gets its own so +-- its origin is greppable). +ALTER TABLE quiz_context + DROP CONSTRAINT IF EXISTS quiz_context_user_concept_key; +ALTER TABLE quiz_context + ADD CONSTRAINT quiz_context_user_concept_key UNIQUE (user_id, concept_node_id); diff --git a/backend/routes/quiz.py b/backend/routes/quiz.py index 7cc66144..a5a90d0b 100644 --- a/backend/routes/quiz.py +++ b/backend/routes/quiz.py @@ -9,7 +9,9 @@ from pydantic_ai.exceptions import UsageLimitExceeded, UnexpectedModelBehavior +import config from agents import ORCHESTRATOR_LIMITS +from agents._providers import UnregisteredHandlerError from agents.quiz import quiz_agent, Quiz, QuizQuestion from agents.deps import SaplingDeps from agents._run import run_agent_sync @@ -603,17 +605,50 @@ def submit_quiz(body: SubmitQuizBody, background_tasks: BackgroundTasks, request .replace("{quiz_results_json}", json.dumps(results, indent=2)) ) - def _update_context(prompt: str, uid: str, node_id: str): + # Correlate the background write with this request's trace. + ctx_request_id = getattr(request.state, "request_id", None) or current_request_id() + + def _update_context(prompt: str, uid: str, node_id: str, quiz_id: str, + request_id: str | None): + # #529/B3: this write was `except Exception: pass` for months while + # every attempt 42P10'd — the adaptive loop died silently. Failures + # are loud now: ERROR log with the attempt id + request id, a + # `quiz.context_write_failed` analytics event, and a re-raise in + # local/test envs so a regression fails CI instead of going quiet. try: result = record_agent_usage( run_agent_sync(quiz_context_agent.run(prompt)), feature="quiz", task="quiz_context", user_id=uid, ) save_quiz_context(uid, node_id, result.output.model_dump()) + except UnregisteredHandlerError: + # E2E function mode leaves quiz_context deliberately + # unregistered (agents/function_handlers_e2e.py) so no + # post-response DB write races the next test's re-seed. One + # WARNING, no traceback: the logscan oracle reports tracebacks. + logger.warning( + "quiz: context update skipped — quiz_context handler " + "unregistered (function-mode seam) quiz_id=%s", quiz_id, + ) except Exception: - pass + logger.exception( + "quiz: context update failed quiz_id=%s concept=%s " + "request_id=%s", quiz_id, node_id, request_id, + ) + events_service.log_event( + "quiz.context_write_failed", + category="error", + user_id=uid, + request_id=request_id, + payload={"quiz_id": quiz_id, "concept_node_id": node_id}, + ) + if config.IS_LOCAL: + raise - background_tasks.add_task(_update_context, ctx_prompt, user_id, concept_node_id) + background_tasks.add_task( + _update_context, ctx_prompt, user_id, concept_node_id, + body.quiz_id, ctx_request_id, + ) # XP + achievements: after the attempt row (score/total/answers_json) is # persisted above (the atomic completed_at claim + the update at :486-494 diff --git a/backend/services/events_service.py b/backend/services/events_service.py index c3b4230c..fce2dff4 100644 --- a/backend/services/events_service.py +++ b/backend/services/events_service.py @@ -82,6 +82,10 @@ "document.processed", "quiz.started", "quiz.completed", + # #529/B3: the post-submit context write failed. category="error" so it + # surfaces in admin analytics — this failure was invisible for months + # precisely because nothing emitted when the background task died. + "quiz.context_write_failed", "chat.message_sent", "note.created", "session.started", diff --git a/backend/tests/integration/test_quiz_context_repair_db.py b/backend/tests/integration/test_quiz_context_repair_db.py new file mode 100644 index 00000000..bab07482 --- /dev/null +++ b/backend/tests/integration/test_quiz_context_repair_db.py @@ -0,0 +1,71 @@ +"""#529 repair, real-DB half (Workstream B, epic #537). + +The class of bug this file exists to catch: the hermetic suite mocked +`table()` and so never saw that quiz_context's UNIQUE was gone — the +upsert 42P10'd in every real environment for ~7.5 weeks while tests +stayed green. These assertions run against the local Supabase stack +(#397 seam: writes through the app, raw reads through psycopg). +""" +import pytest + +pytestmark = pytest.mark.integration + +USER = "rich-user-active" + + +def _seeded_node_id(db_conn) -> str: + row = db_conn.execute( + "SELECT id FROM graph_nodes WHERE user_id = %s ORDER BY id LIMIT 1", + (USER,), + ).fetchone() + assert row is not None, "rich seed should provide graph nodes for the active user" + return row["id"] + + +def test_quiz_context_unique_constraint_is_restored(db_conn): + """The #529 repair migration must leave a UNIQUE covering exactly + (user_id, concept_node_id) — the columns save_quiz_context's + on_conflict names.""" + rows = db_conn.execute( + """ + SELECT c.conname, + array_agg(a.attname ORDER BY k.ord) AS cols + FROM pg_constraint c + CROSS JOIN LATERAL unnest(c.conkey) WITH ORDINALITY AS k(attnum, ord) + JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = k.attnum + WHERE c.conrelid = 'quiz_context'::regclass AND c.contype = 'u' + GROUP BY c.conname + """ + ).fetchall() + col_sets = [tuple(r["cols"]) for r in rows] + assert ("user_id", "concept_node_id") in col_sets, ( + f"no UNIQUE on (user_id, concept_node_id); found: {col_sets!r} — " + "the 0025 regression (#529) is back" + ) + + +def test_save_quiz_context_upserts_one_row_and_encrypts(db_conn): + """Two writes for the same (user, concept): before the repair the FIRST + write already failed with 42P10; after it, the second must replace the + first (one row), the raw column must be ciphertext, and the app read + must round-trip the latest payload.""" + from services.quiz_context_service import get_quiz_context, save_quiz_context + + node_id = _seeded_node_id(db_conn) + save_quiz_context(USER, node_id, {"weak_areas": ["first write"]}) + save_quiz_context(USER, node_id, {"weak_areas": ["second write"]}) + + rows = db_conn.execute( + "SELECT context_json FROM quiz_context " + "WHERE user_id = %s AND concept_node_id = %s", + (USER, node_id), + ).fetchall() + assert len(rows) == 1, f"upsert must keep exactly one row, found {len(rows)}" + + raw = rows[0]["context_json"] + # #521: ciphertext stored as a JSONB string scalar — a dict here means + # the encrypt-at-write path regressed to plaintext. + assert isinstance(raw, str), f"context_json at rest should be ciphertext str, got {type(raw)}" + assert "second write" not in raw + + assert get_quiz_context(USER, node_id) == {"weak_areas": ["second write"]} diff --git a/backend/tests/test_event_capture_seams.py b/backend/tests/test_event_capture_seams.py index e7ba5594..9edbd03c 100644 --- a/backend/tests/test_event_capture_seams.py +++ b/backend/tests/test_event_capture_seams.py @@ -80,7 +80,7 @@ def _make_request(path: str = "/api/thing", query: str = "") -> Request: # ── Taxonomy pin ───────────────────────────────────────────────────────────── -def test_event_taxonomy_is_pinned_to_twelve_names(): +def test_event_taxonomy_is_pinned(): """Shared constant so a rename breaks loudly — every seam below asserts its exact event name, and this pins the full set in one place.""" assert events_service.EVENT_TAXONOMY == frozenset({ @@ -92,6 +92,7 @@ def test_event_taxonomy_is_pinned_to_twelve_names(): "document.processed", "quiz.started", "quiz.completed", + "quiz.context_write_failed", # #529/B3 "chat.message_sent", "note.created", "session.started", diff --git a/backend/tests/test_quiz_context_repair.py b/backend/tests/test_quiz_context_repair.py new file mode 100644 index 00000000..87116685 --- /dev/null +++ b/backend/tests/test_quiz_context_repair.py @@ -0,0 +1,297 @@ +""" +Workstream B of the pre-revamp quiz repair batch (#529, epic #537). + +The adaptive-context write has been dead since migration 0025 dropped +quiz_context's UNIQUE (user_id, concept_node_id): the upsert 42P10s and +routes/quiz.py swallowed it (`except Exception: pass`). These tests pin: + +- B2: save_quiz_context targets exactly the restored constraint's columns. +- B3: the background context update is loud on failure — ERROR log + + `quiz.context_write_failed` analytics event + re-raise in local/test + envs so a regression fails CI instead of going quiet for months. The + one deliberate quiet path is the E2E function-mode seam's + UnregisteredHandlerError (quiz_context stays unregistered by design). +- B4: the read side consumes the FULL QuizContext shape — before this + fix `_coerce_summary` returned only `notes` and silently dropped + weak_areas / common_mistakes / questions_seen_summary (and its list + fallback looked for `common_errors`, which QuizContext never writes). + +The real-DB constraint restore + ciphertext round-trip lives in +tests/integration/test_quiz_context_repair_db.py. +""" +import pytest +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch +from fastapi.testclient import TestClient + +from main import app +from agents._providers import UnregisteredHandlerError +from agents.quiz_context import QuizContext + +client = TestClient(app) + + +SAMPLE_QUESTIONS = [ + { + "id": 1, + "text": "What does a for-loop do?", + "options": [ + {"label": "A", "correct": True}, + {"label": "B", "correct": False}, + ], + "explanation": "A is correct.", + }, +] + + +def _submit_factory(): + def factory(name): + mock = MagicMock() + if name == "quiz_attempts": + mock.select.return_value = [{ + "id": "quiz1", + "user_id": "user_andres", + "concept_node_id": "node1", + "difficulty": "medium", + "questions_json": SAMPLE_QUESTIONS, + }] + elif name == "graph_nodes": + mock.select.return_value = [{ + "mastery_score": 0.5, + "concept_name": "Loops", + "course_id": "course1", + }] + else: + mock.select.return_value = [] + mock.update.return_value = [{"id": "updated"}] + return mock + + return factory + + +def _ok_ctx_agent(): + return AsyncMock( + return_value=SimpleNamespace(output=SimpleNamespace(model_dump=lambda: {})) + ) + + +def _submit(): + return client.post("/api/quiz/submit", json={ + "quiz_id": "quiz1", + "answers": [{"question_id": 1, "selected_label": "A"}], + }) + + +# ── B2: the upsert target is pinned to the restored constraint ────────────── + + +class TestSaveQuizContextUpsertTarget: + def test_upserts_on_user_id_concept_node_id(self): + from services.quiz_context_service import save_quiz_context + + captured = {} + fake = MagicMock() + + def _upsert(payload, on_conflict=None): + captured["payload"] = payload + captured["on_conflict"] = on_conflict + return [payload] + + fake.upsert.side_effect = _upsert + with patch("services.quiz_context_service.table", return_value=fake): + save_quiz_context("u1", "n1", {"weak_areas": ["x"]}) + + # Must match the UNIQUE restored by the #529 repair migration + # (quiz_context_user_concept_key) column-for-column. + assert captured["on_conflict"] == "user_id,concept_node_id" + # #521: ciphertext at rest. + assert isinstance(captured["payload"]["context_json"], str) + + +# ── B3: failures are loud ─────────────────────────────────────────────────── + + +class TestContextWriteFailureSurfaces: + def test_write_failure_emits_event_and_reraises_in_test_env(self): + """conftest sets APP_ENV=test → config.IS_LOCAL is True → the + background task re-raises, so this exact regression class fails + CI instead of passing silently for months (#529).""" + events = [] + with ( + patch("routes.quiz.table", side_effect=_submit_factory()), + patch("routes.quiz.apply_graph_update"), + patch("routes.quiz.get_quiz_context", return_value={}), + patch("routes.quiz.quiz_context_agent.run", new=_ok_ctx_agent()), + patch( + "routes.quiz.save_quiz_context", + side_effect=RuntimeError("42P10: no unique constraint"), + ), + patch( + "routes.quiz.events_service.log_event", + side_effect=lambda *a, **k: events.append((a, k)), + ), + ): + with pytest.raises(RuntimeError, match="42P10"): + _submit() + + failed = [ + (a, k) for a, k in events if a and a[0] == "quiz.context_write_failed" + ] + assert len(failed) == 1, ( + "a quiz-context write failure must emit quiz.context_write_failed " + "so it shows up in admin analytics" + ) + _, kwargs = failed[0] + assert kwargs.get("category") == "error" + assert kwargs.get("user_id") == "user_andres" + payload = kwargs.get("payload") or {} + assert payload.get("quiz_id") == "quiz1" + assert payload.get("concept_node_id") == "node1" + + def test_write_failure_does_not_break_submit_in_production(self, caplog): + """In production the response already went out; the task logs at + ERROR (with traceback) + emits the event, but must not raise.""" + import config + + events = [] + with ( + patch("routes.quiz.table", side_effect=_submit_factory()), + patch("routes.quiz.apply_graph_update"), + patch("routes.quiz.get_quiz_context", return_value={}), + patch("routes.quiz.quiz_context_agent.run", new=_ok_ctx_agent()), + patch( + "routes.quiz.save_quiz_context", + side_effect=RuntimeError("boom"), + ), + patch( + "routes.quiz.events_service.log_event", + side_effect=lambda *a, **k: events.append((a, k)), + ), + patch.object(config, "IS_LOCAL", False), + ): + with caplog.at_level("ERROR", logger="routes.quiz"): + r = _submit() + + assert r.status_code == 200 + assert any(a[0] == "quiz.context_write_failed" for a, _ in events) + assert any( + "context update failed" in rec.message and rec.exc_info + for rec in caplog.records + ), "the failure must be logged at ERROR with the traceback" + + def test_unregistered_seam_handler_is_quiet_by_design(self, caplog): + """E2E function mode leaves quiz_context deliberately unregistered + (agents/function_handlers_e2e.py) — that fail-fast is the seam + working, not a bug: one WARNING, no traceback, no analytics event, + no re-raise (a raise would put tracebacks in .e2e/backend.log and + turn the logscan oracle red on every quiz journey).""" + events = [] + with ( + patch("routes.quiz.table", side_effect=_submit_factory()), + patch("routes.quiz.apply_graph_update"), + patch("routes.quiz.get_quiz_context", return_value={}), + patch( + "routes.quiz.quiz_context_agent.run", + new=AsyncMock( + side_effect=UnregisteredHandlerError( + "no function-mode handler registered for task 'quiz_context'" + ) + ), + ), + patch("routes.quiz.save_quiz_context") as save_mock, + patch( + "routes.quiz.events_service.log_event", + side_effect=lambda *a, **k: events.append((a, k)), + ), + ): + with caplog.at_level("WARNING", logger="routes.quiz"): + r = _submit() + + assert r.status_code == 200 + save_mock.assert_not_called() + assert not any(a[0] == "quiz.context_write_failed" for a, _ in events) + warnings = [ + rec for rec in caplog.records + if rec.levelname == "WARNING" and "unregistered" in rec.message.lower() + ] + assert warnings, "the seam skip should leave one WARNING breadcrumb" + assert all(not rec.exc_info for rec in warnings), ( + "no traceback — the logscan oracle treats tracebacks as findings" + ) + + +# ── B4: the read side consumes the whole QuizContext shape ────────────────── + + +class TestCoerceSummaryConsumesQuizContext: + def test_full_quiz_context_shape_survives_coercion(self): + from agents.tools.quiz_history import _coerce_summary + + ctx = QuizContext( + weak_areas=["recursion base case"], + common_mistakes=["off-by-one in loop bounds"], + questions_seen_summary="loops and recursion basics", + recommended_difficulty="hard", + notes="solid on iteration", + ) + s = _coerce_summary(ctx.model_dump()) + assert s is not None + for fragment in ( + "recursion base case", + "off-by-one in loop bounds", + "loops and recursion basics", + "solid on iteration", + ): + assert fragment in s, f"digest dropped: {fragment!r}" + + def test_legacy_shapes_still_coerce(self): + from agents.tools.quiz_history import _coerce_summary + + assert _coerce_summary("plain digest") == "plain digest" + assert _coerce_summary({"summary": "s"}) == "s" + assert _coerce_summary({"notes": "n"}) == "n" + assert ( + "wrong base case" + in _coerce_summary({"misconceptions": ["wrong base case"]}) + ) + assert _coerce_summary(None) is None + assert _coerce_summary({}) is None + + def test_encrypted_context_row_reaches_the_agent_tool_summary(self): + """The full read wire: a ciphertext context_json row (what + save_quiz_context writes, #521) decrypts inside + read_recent_quiz_attempts and its digest lands in the + QuizHistory.summary the quiz agent consumes.""" + import asyncio + + from agents.tools.quiz_history import read_recent_quiz_attempts + from services.encryption import encrypt_json + + stored = QuizContext( + weak_areas=["recursion base case"], + common_mistakes=["off-by-one in loop bounds"], + questions_seen_summary="loops and recursion basics", + recommended_difficulty="hard", + notes="solid on iteration", + ).model_dump() + + def factory(name): + mock = MagicMock() + if name == "quiz_context": + mock.select.return_value = [ + {"context_json": encrypt_json(stored)} + ] + else: + mock.select.return_value = [] + return mock + + with patch("agents.tools.quiz_history.table", side_effect=factory): + history = asyncio.run(read_recent_quiz_attempts("u1", "n1")) + + assert history.summary is not None + for fragment in ( + "recursion base case", + "off-by-one in loop bounds", + "loops and recursion basics", + ): + assert fragment in history.summary From bdc95529ff5edfb45ea4b80c2b9351c1373109c1 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:26:33 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(quiz):=20address=20#548=20review=20?= =?UTF-8?q?=E2=80=94=20coercion=20guards,=20consume=20recommended=5Fdiffic?= =?UTF-8?q?ulty,=20stable=20upsert=20id,=20error-category=20admin=20feed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings (xhigh, 4 confirmed): - _coerce_summary guards list-shaped keys with isinstance(list) so legacy rows holding a string/dict there no longer explode into per-character bullets in the agent's prompt digest. - recommended_difficulty is surfaced in the digest ("Recommended next difficulty: …") — previously computed, encrypted, persisted, and never consumed anywhere. - save_quiz_context no longer sends a client-generated id: with merge-duplicates live again, the payload id rewrote the row's PRIMARY KEY on every refresh. Fresh inserts use the column's DB default; integration test now pins id stability across refreshes. - /api/admin/analytics/errors filters by category=error instead of the error.* name prefix, so quiz.context_write_failed (and #482's rag.* events) actually appear in the feed the B3 comments promised — non-HTTP rows null their payload fields, which ErrorEvent already models as Optional. Co-Authored-By: Claude Fable 5 --- backend/agents/tools/quiz_history.py | 14 +++++- backend/routes/admin_analytics.py | 12 +++-- backend/services/quiz_context_service.py | 6 ++- .../test_quiz_context_repair_db.py | 9 +++- backend/tests/test_admin_analytics_routes.py | 45 ++++++++++++++----- backend/tests/test_quiz_context_repair.py | 20 +++++++++ 6 files changed, 88 insertions(+), 18 deletions(-) diff --git a/backend/agents/tools/quiz_history.py b/backend/agents/tools/quiz_history.py index e81f0b9b..7b7c4a14 100644 --- a/backend/agents/tools/quiz_history.py +++ b/backend/agents/tools/quiz_history.py @@ -96,13 +96,25 @@ def _coerce_summary(ctx: Any) -> str | None: if isinstance(v, str) and v.strip(): parts.append(v.strip()) for key, label in _SUMMARY_LIST_KEYS: + raw = ctx.get(key) + if not isinstance(raw, list): + # Legacy free-form rows can hold a string (or dict) under a + # list-shaped key; iterating those element-wise would spray + # per-character bullets / dict keys into the agent's prompt. + continue items = [ item.strip() - for item in (ctx.get(key) or []) + for item in raw if isinstance(item, str) and item.strip() ] if items: parts.append(label + ":\n" + "\n".join(f"- {i}" for i in items)) + rec = ctx.get("recommended_difficulty") + if isinstance(rec, str) and rec.strip(): + # The post-submit agent's difficulty recommendation — surfaced + # here or it rots encrypted-and-unread (its only other mention + # is the dead legacy prompt template). + parts.append(f"Recommended next difficulty: {rec.strip()}") return "\n\n".join(parts) or None return None diff --git a/backend/routes/admin_analytics.py b/backend/routes/admin_analytics.py index bd1db4f0..792e1c8f 100644 --- a/backend/routes/admin_analytics.py +++ b/backend/routes/admin_analytics.py @@ -441,14 +441,18 @@ def errors( offset: int = Query(0, ge=0), bucket: Bucket | None = Query(None), ) -> ErrorsPage: - """Paginated error.* event feed; `?bucket=day` adds a per-day series (its own capped scan).""" + """Paginated error-category event feed; `?bucket=day` adds a per-day series (its own capped scan).""" require_admin(request) response.headers["Cache-Control"] = "private" from_iso, to_iso = _resolve_range(from_, to) - # error.* events, newest first — paginated server-side (no aggregation). + # ALL category="error" events, newest first — not just the error.* HTTP + # names. Backend failures like quiz.context_write_failed (#529/B3) and + # the rag.* pair (#482) carry the error category without the name + # prefix; filtering by name hid exactly the events whose invisibility + # they were added to end. Non-HTTP rows simply null the payload fields. rows, total = table("events").select_with_count( "created_at,event_type,request_id,user_id,payload", - filters={"created_at": [f"gte.{from_iso}", f"lte.{to_iso}"], "event_type": "like.error.*"}, + filters={"created_at": [f"gte.{from_iso}", f"lte.{to_iso}"], "category": "eq.error"}, order="created_at.desc", limit=limit, offset=offset, ) series = None @@ -456,7 +460,7 @@ def errors( if bucket: scan_rows, series_truncated = _scan_range( "events", "created_at", from_iso, to_iso, - extra_filters={"event_type": "like.error.*"}, + extra_filters={"category": "eq.error"}, ) series = _count_series(scan_rows) items = [] diff --git a/backend/services/quiz_context_service.py b/backend/services/quiz_context_service.py index 11d775e9..6caae71f 100644 --- a/backend/services/quiz_context_service.py +++ b/backend/services/quiz_context_service.py @@ -1,5 +1,4 @@ import logging -import uuid from datetime import datetime, timezone from db.connection import table @@ -26,9 +25,12 @@ def get_quiz_context(user_id: str, concept_node_id: str): def save_quiz_context(user_id: str, concept_node_id: str, context: dict): + # No client-generated id: PostgREST's merge-duplicates upsert updates + # every column in the payload on conflict, so an id here would rewrite + # the existing row's PRIMARY KEY on each refresh. Fresh inserts get the + # column's DB default (gen_random_uuid). table("quiz_context").upsert( { - "id": str(uuid.uuid4()), "user_id": user_id, "concept_node_id": concept_node_id, "context_json": encrypt_json(context), diff --git a/backend/tests/integration/test_quiz_context_repair_db.py b/backend/tests/integration/test_quiz_context_repair_db.py index bab07482..23f14717 100644 --- a/backend/tests/integration/test_quiz_context_repair_db.py +++ b/backend/tests/integration/test_quiz_context_repair_db.py @@ -53,14 +53,21 @@ def test_save_quiz_context_upserts_one_row_and_encrypts(db_conn): node_id = _seeded_node_id(db_conn) save_quiz_context(USER, node_id, {"weak_areas": ["first write"]}) + first = db_conn.execute( + "SELECT id FROM quiz_context WHERE user_id = %s AND concept_node_id = %s", + (USER, node_id), + ).fetchone() save_quiz_context(USER, node_id, {"weak_areas": ["second write"]}) rows = db_conn.execute( - "SELECT context_json FROM quiz_context " + "SELECT id, context_json FROM quiz_context " "WHERE user_id = %s AND concept_node_id = %s", (USER, node_id), ).fetchall() assert len(rows) == 1, f"upsert must keep exactly one row, found {len(rows)}" + # The refresh must not rewrite the row's PRIMARY KEY (merge-duplicates + # updates every payload column — an id in the payload would churn here). + assert rows[0]["id"] == first["id"] raw = rows[0]["context_json"] # #521: ciphertext stored as a JSONB string scalar — a dict here means diff --git a/backend/tests/test_admin_analytics_routes.py b/backend/tests/test_admin_analytics_routes.py index 552938ea..141cc66e 100644 --- a/backend/tests/test_admin_analytics_routes.py +++ b/backend/tests/test_admin_analytics_routes.py @@ -36,6 +36,12 @@ def _seed(): {"event_type": "quiz.completed", "category": "usage", "user_id": "u2", "request_id": "r3", "payload": {}, "created_at": IN3}, {"event_type": "error.5xx", "category": "error", "user_id": "u2", "request_id": "r4", "payload": {"path": "/api/quiz", "method": "POST", "status_code": 500, "duration_ms": 12.3}, "created_at": IN3}, + # A category="error" event whose NAME doesn't start with error.* — + # the #529/B3 backend-failure class. The /errors feed keys on the + # category, so this must appear there (the old like.error.* name + # filter hid exactly these, recreating the invisibility it fixed). + {"event_type": "quiz.context_write_failed", "category": "error", "user_id": "u1", "request_id": "r5", + "payload": {"quiz_id": "q9", "concept_node_id": "n9"}, "created_at": IN2}, {"event_type": "auth.login", "category": "audit", "user_id": "u1", "request_id": "r0", "payload": {}, "created_at": OUT}, ] llm = [ @@ -116,7 +122,7 @@ def test_usage_summary_counts(seeded): r = client.get(f"{BASE}/usage/summary", params=RANGE) assert r.status_code == 200 body = r.json() - assert body["total_events"] == 4 # the 2020 auth.login is excluded + assert body["total_events"] == 5 # the 2020 auth.login is excluded assert body["distinct_active_users"] == 2 assert body["truncated"] is False # nowhere near the scan cap by_type = {row["event_type"]: row["count"] for row in body["by_event_type"]} @@ -134,7 +140,7 @@ def test_usage_by_user_totals(seeded): body = r.json() users = {u["user_id"]: u for u in body["users"]} assert body["total_users"] == 2 - assert users["u1"]["event_count"] == 2 + assert users["u1"]["event_count"] == 3 assert users["u1"]["llm_cost_usd"] == pytest.approx(0.06) assert users["u1"]["total_tokens"] == 450 assert users["u2"]["llm_cost_usd"] == pytest.approx(0.02) @@ -187,15 +193,31 @@ def test_errors_returns_error_events_with_payload_fields(seeded): r = client.get(f"{BASE}/errors", params=RANGE) assert r.status_code == 200 body = r.json() - assert body["total"] == 1 - err = body["errors"][0] - assert err["event_type"] == "error.5xx" + assert body["total"] == 2 + by_type = {e["event_type"]: e for e in body["errors"]} + err = by_type["error.5xx"] assert err["path"] == "/api/quiz" assert err["method"] == "POST" assert err["status_code"] == 500 assert err["duration_ms"] == pytest.approx(12.3) +def test_errors_feed_includes_non_http_error_category_events(seeded): + """quiz.context_write_failed (and the rag.* class from #482) carry + category="error" but not the error.* name prefix — the feed must key + on the category or backend failures stay invisible to admins, which + is the exact #529 failure mode the event exists to end.""" + r = client.get(f"{BASE}/errors", params=RANGE) + body = r.json() + by_type = {e["event_type"]: e for e in body["errors"]} + assert "quiz.context_write_failed" in by_type + row = by_type["quiz.context_write_failed"] + # No HTTP payload fields on a background-task event — they null out. + assert row["path"] is None + assert row["status_code"] is None + assert row["request_id"] == "r5" + + # ── date filtering + defaults ──────────────────────────────────────────────── @@ -218,7 +240,7 @@ def now(cls, tz=None): monkeypatch.setattr(analytics, "datetime", _FrozenDatetime) r = client.get(f"{BASE}/usage/summary") assert r.status_code == 200 - assert r.json()["total_events"] == 4 + assert r.json()["total_events"] == 5 def test_rejects_malformed_from(seeded): @@ -279,9 +301,9 @@ def test_usage_summary_bucket_day_series(seeded): assert r.status_code == 200 body = r.json() assert [p["date"] for p in body["series"]] == ["2026-07-10", "2026-07-12", "2026-07-15"] - assert [p["count"] for p in body["series"]] == [1, 1, 2] + assert [p["count"] for p in body["series"]] == [1, 2, 2] # Bucketing adds the series; it must not change the aggregate fields. - assert body["total_events"] == 4 + assert body["total_events"] == 5 assert body["distinct_active_users"] == 2 @@ -306,8 +328,11 @@ def test_errors_bucket_day_series(seeded): r = client.get(f"{BASE}/errors", params={**RANGE, "bucket": "day"}) assert r.status_code == 200 body = r.json() - assert body["series"] == [{"date": "2026-07-15", "count": 1}] - assert body["total"] == 1 + assert body["series"] == [ + {"date": "2026-07-12", "count": 1}, # quiz.context_write_failed + {"date": "2026-07-15", "count": 1}, # error.5xx + ] + assert body["total"] == 2 assert body["truncated"] is False diff --git a/backend/tests/test_quiz_context_repair.py b/backend/tests/test_quiz_context_repair.py index 87116685..5e041806 100644 --- a/backend/tests/test_quiz_context_repair.py +++ b/backend/tests/test_quiz_context_repair.py @@ -106,6 +106,11 @@ def _upsert(payload, on_conflict=None): assert captured["on_conflict"] == "user_id,concept_node_id" # #521: ciphertext at rest. assert isinstance(captured["payload"]["context_json"], str) + # No client-generated id: with merge-duplicates live again, an id in + # the payload would REWRITE the existing row's primary key on every + # refresh (DO UPDATE SET id = excluded.id). The column's DB default + # covers fresh inserts. + assert "id" not in captured["payload"] # ── B3: failures are loud ─────────────────────────────────────────────────── @@ -241,9 +246,24 @@ def test_full_quiz_context_shape_survives_coercion(self): "off-by-one in loop bounds", "loops and recursion basics", "solid on iteration", + # recommended_difficulty is the write side's whole point — it + # must reach the digest, not rot encrypted-and-unread. + "hard", ): assert fragment in s, f"digest dropped: {fragment!r}" + def test_non_list_values_under_list_keys_are_skipped(self): + """Legacy free-form rows can hold a STRING (or dict) under a + list-shaped key; iterating those element-wise sprays per-character + bullets ('- r', '- e', …) into the agent's prompt digest.""" + from agents.tools.quiz_history import _coerce_summary + + legacy = {"summary": "Focus on recursion", "weak_areas": "recursion"} + assert _coerce_summary(legacy) == "Focus on recursion" + + legacy_dict = {"summary": "Focus", "common_mistakes": {"a": 1}} + assert _coerce_summary(legacy_dict) == "Focus" + def test_legacy_shapes_still_coerce(self): from agents.tools.quiz_history import _coerce_summary