From 17ba2564861076f0e23e550259abb48f55671751 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:16:25 -0400 Subject: [PATCH 1/2] fix(quiz): read misconceptions from the offering keyspace (#553) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `offering_concept_stats.offering_id` holds `course_offerings.id`. The misconceptions tool handed it `ctx.deps.course_id` — the abstract `courses.id` the graph and the HTTP boundary carry. Two disjoint keyspaces, so the read matched nothing for every student since the tool was written, and `use_shared_context` has been a no-op. An empty list is exactly what "this class has no misconceptions yet" looks like, which is why it survived. Verified live before changing anything, as the issue requires: | | stats rows | key on an offering | key on a course | filter by course id | filter by the student's offerings | |---|---|---|---|---|---| | staging | 72 | 72 | 0 | **0** | 68 + 4 | | prod | 73 | 73 | 0 | **0** | 73 | The tool now resolves course -> the student's offerings through `services/academics.py`, which owns that resolution, and filters `offering_id=in.(...)`. Plural throughout: a student can hold more than one offering of the same course (the rich seed's active user has CS in two terms), and scoping to a single "current" offering would silently drop the other class's aggregates. Two things fell out of doing it properly: - **A bare `str` is a `Sequence[str]`.** An unguarded comprehension would iterate the id per CHARACTER and build a well-formed filter that matches nothing — the same shape that had the quiz_history coercer spraying "- r" into prompts earlier in this batch. Guarded explicitly. - **The F5 probe had to get narrower, or this fix would ship a false alarm to every generation.** `COURSE_HAS_AGGREGATES` asked whether any stats row exists. The aggregation writes a row per concept as soon as a class has activity and only fills `common_misconceptions` when it has something to say — 0 of 72 rows on staging and 0 of 73 on prod carry text today. So the moment the keyspace was fixed, every student would trip `quiz.tool_empty` on every quiz. The probe now asks for rows that actually carry text (`neq.{}`, verified against staging PostgREST), which is what the expectation always meant. Tests: the hermetic half pins the filter shape; the real-DB half exists because a mocked `table()` can assert a filter STRING without ever learning that the string selects nothing — the blind spot that let #529 live 51 days. The rich seed gains `offering_concept_stats` rows shaped to tell a fix from a coincidence: two offerings of one course (both the active user's), one row with an empty array, and one belonging to a class they are NOT in whose text must never leak. Hermetic 2129 passed / 9 skipped, integration 7/7 new + 51 total. Co-Authored-By: Claude Opus 5 --- backend/agents/tools/graph_read.py | 120 +++++++++------ backend/db/seed_local_rich.py | 62 ++++++++ backend/services/tool_signals.py | 12 +- .../test_misconceptions_keyspace_db.py | 137 ++++++++++++++++++ .../tests/test_quiz_tool_instrumentation.py | 60 +++++++- 5 files changed, 339 insertions(+), 52 deletions(-) create mode 100644 backend/tests/integration/test_misconceptions_keyspace_db.py diff --git a/backend/agents/tools/graph_read.py b/backend/agents/tools/graph_read.py index df2d6d6d..7889d7ce 100644 --- a/backend/agents/tools/graph_read.py +++ b/backend/agents/tools/graph_read.py @@ -10,6 +10,7 @@ import asyncio import logging +from collections.abc import Sequence from typing import Any from pydantic import BaseModel, Field @@ -367,11 +368,26 @@ class Misconception(BaseModel): async def read_misconceptions_for_course( - offering_id: str | None, + offering_ids: Sequence[str] | None, ) -> list[Misconception]: - """Return aggregated misconception strings for an offering (a class in a - term). Anonymized (sourced from class-wide patterns, not any single student). - Returns [] when offering_id is None or the underlying table is empty. + """Return aggregated misconception strings for one or more offerings (a + class in a term). Anonymized (sourced from class-wide patterns, not any + single student). Returns [] when no offerings are given or the underlying + table has nothing for them. + + Takes OFFERING ids, plural, and the plural is load-bearing twice over. + + Keyspace (#553): `offering_concept_stats.offering_id` holds + `course_offerings.id`, which is a different keyspace from the abstract + `courses.id` the graph and the HTTP boundary carry. This function used to + be handed the latter, so it matched nothing for every student + indefinitely. Callers resolve course -> offerings via + `services/academics.py`; that module owns the resolution. + + Plural: a student can be enrolled in more than one offering of the same + course (a repeat, or a course spanning terms — the rich seed's active user + holds CS in two). Scoping to a single "current" offering would silently + drop the other class's aggregates. Source: `offering_concept_stats` rows for the offering. Each row represents one concept and carries a `common_misconceptions` array @@ -382,7 +398,16 @@ async def read_misconceptions_for_course( The tool contract (returning Misconception[]) is unchanged. """ - if not offering_id: + # A bare `str` IS a Sequence[str], so an un-guarded comprehension would + # iterate it PER CHARACTER and build `in.(c,a,s,-,c,s,...)` — a filter that + # matches nothing while looking entirely well-formed. The same shape + # already bit this batch once (the quiz_history coercer spraying "- r"/ + # "- e"/"- c" into the prompt), and the whole point of #553 is that a + # silently-matching-nothing filter can survive for months. + if isinstance(offering_ids, str): + offering_ids = [offering_ids] + ids = [str(o) for o in (offering_ids or []) if o] + if not ids: return [] def _fetch() -> list[dict[str, Any]]: @@ -390,7 +415,7 @@ def _fetch() -> list[dict[str, Any]]: return ( table("offering_concept_stats").select( "concept_name,common_misconceptions", - filters={"offering_id": f"eq.{offering_id}"}, + filters={"offering_id": f"in.({','.join(ids)})"}, order="updated_at.desc", limit=20, ) @@ -398,8 +423,7 @@ def _fetch() -> list[dict[str, Any]]: ) except Exception: logger.exception( - "read_misconceptions_for_course failed for offering=%s", - offering_id, + "read_misconceptions_for_course failed for offerings=%s", ids, ) return [] @@ -433,49 +457,57 @@ async def read_misconceptions_for_course_tool( """ from services.prompt_safety import neutralize_delimiters - out = await read_misconceptions_for_course(ctx.deps.course_id) - # F5: THE canonical instance of this bug class. This tool passed the - # abstract course id where the query filters `offering_id` — a different - # keyspace, so it returned zero rows for every student, indefinitely, - # and looked exactly like a class that simply had no misconceptions yet. - # (#553 carries the fix; this makes the next one impossible to miss.) - # The probe asks whether aggregates exist for THIS student's offerings of - # THIS course — not merely whether they are enrolled in something. - # "Enrolled somewhere" would fire on every generation in a course whose - # class simply has no aggregated misconceptions yet, which is the normal - # state for the first weeks of any term. - # - # Scoped this way it detects the real failure instead: aggregates exist - # for the class, but this tool's read returned none — the signature of a - # keyspace mismatch, which is precisely how #553 (abstract course id used - # where an offering id is expected) presents. + # #553: resolve course -> the student's offerings BEFORE reading. The + # stats table is keyed on `course_offerings.id`; `ctx.deps.course_id` is + # the abstract `courses.id` the graph carries. Handing the second to a + # filter expecting the first matched nothing for every student since the + # tool was written, and looked exactly like a class with no misconceptions + # yet. Verified live 2026-08-22: staging 72/72 stats rows key on an + # offering id and 0 on a course id (prod 73/73); filtering by course id + # returned 0 in both, filtering by the student's offerings returned 68+4 + # and 73. # - # Gated on the result being EMPTY, not merely on having a course id. - # `user_offering_ids_for_course` is uncached and issues two unbounded - # PostgREST reads (enrollments -> offerings), and this runs on the quiz - # generation request path — resolving it whenever a course id exists made - # every generation pay both round-trips even when the tool returned rows, - # contradicting tool_signals' own documented contract ("one owner-scoped - # indexed read, only on the empty path"). `report_empty_result` would - # short-circuit on a non-zero count anyway, so the work was pure waste. - if not out and ctx.deps.course_id: - offering_ids: list[str] = [] + # The resolution is now unconditional rather than probe-only: it is what + # the READ needs, not merely what the probe needs. It stays a single + # `academics` call whose two reads are the price of asking the right + # question at all. + offering_ids: list[str] = [] + if ctx.deps.course_id: try: offering_ids = await asyncio.to_thread( user_offering_ids_for_course, ctx.deps.user_id, ctx.deps.course_id ) except Exception: - logger.debug("misconceptions probe: offering resolution failed", exc_info=True) - if offering_ids: - await report_empty_result_async( - "read_misconceptions_for_course", - user_id=ctx.deps.user_id, - count=len(out), - expect=Expect.COURSE_HAS_AGGREGATES, - feature=getattr(ctx.deps, "feature", "unknown"), - scope={"offering_id": f"in.({','.join(offering_ids)})"}, - payload={"course_id": ctx.deps.course_id}, + # Degrade to "no offerings" rather than raising: this is one + # optional personalization input, and the agent has others. + logger.warning( + "read_misconceptions_for_course: offering resolution failed; " + "returning no class misconceptions", exc_info=True, ) + + out = await read_misconceptions_for_course(offering_ids) + # F5: THE canonical instance of this bug class. The probe asks whether + # aggregates CARRYING MISCONCEPTION TEXT exist for this student's + # offerings of this course — not merely whether they are enrolled, and + # not merely whether stats rows exist. + # + # The text qualifier matters as much as the scope. Both live environments + # today hold stats rows whose `common_misconceptions` arrays are all + # empty (0 of 72 on staging, 0 of 73 on prod — the aggregation runs, the + # classes just have no misconception text yet). A probe that fired on + # "any stats row exists" would therefore report a discrepancy on EVERY + # generation for EVERY student the moment #553 was fixed — the precise + # alarm-fatigue failure F5 exists to prevent. + if not out and offering_ids: + await report_empty_result_async( + "read_misconceptions_for_course", + user_id=ctx.deps.user_id, + count=len(out), + expect=Expect.COURSE_HAS_AGGREGATES, + feature=getattr(ctx.deps, "feature", "unknown"), + scope={"offering_id": f"in.({','.join(offering_ids)})"}, + payload={"course_id": ctx.deps.course_id}, + ) # F6: this block's contribution to the prompt. prompt_dimensions.record(misconceptions=len(out)) return [ diff --git a/backend/db/seed_local_rich.py b/backend/db/seed_local_rich.py index 3dce2aad..5da6bb4e 100644 --- a/backend/db/seed_local_rich.py +++ b/backend/db/seed_local_rich.py @@ -662,6 +662,66 @@ def seed_room_summaries() -> None: ] +# ── offering_concept_stats (#553) ───────────────────────────────────────── +# +# Class-level aggregates, keyed on `course_offerings.id` — a DIFFERENT +# keyspace from the abstract `courses.id` the graph carries. #553 was the +# quiz's misconceptions tool filtering this table's `offering_id` with a +# course id, which matched nothing for every student indefinitely while +# looking exactly like a class that had no misconceptions yet. +# +# The rows are shaped so a test can tell a real fix from a coincidence: +# +# * OFF_CS_F25 and OFF_CS_S26 are BOTH offerings of the same abstract CS +# course, and the active user is enrolled in both — so a fix that +# resolves only one "current" offering still loses half the rows. +# * OFF_HIST_F25 belongs to a course the active user is NOT enrolled in. +# Its misconception text must never reach them; that is the negative +# half of the assertion, and it is what stops a fix from "working" by +# simply dropping the offering filter altogether. +# * One row carries an EMPTY array: the aggregation writes a stats row per +# concept as soon as a class has activity and only fills the array when +# it has something to say (0 of 72 rows on staging and 0 of 73 on prod +# carried text on 2026-08-22). Seeding that state keeps the empty-vs- +# absent distinction exercised. +# +# (stats_id, offering_id, concept_name, misconceptions) +_OFFERING_CONCEPT_STATS = [ + ("rich-ocs-cs-f25-recursion", OFF_CS_F25, "Recursion", + ["Recursion always costs more memory than a loop", + "A base case is optional if the input shrinks"]), + ("rich-ocs-cs-f25-pointers", OFF_CS_F25, "Pointers and Memory", + ["Freeing a pointer also clears the variable holding it"]), + ("rich-ocs-cs-s26-controlflow", OFF_CS_S26, "Control Flow", + ["`else if` evaluates every branch before choosing one"]), + # Same class, no text yet — a stats row is not the same as a finding. + ("rich-ocs-cs-s26-variables", OFF_CS_S26, "Variables and Types", []), + # A class the active user is NOT in. Must never leak into their prompt. + ("rich-ocs-hist-f25-sources", OFF_HIST_F25, "Primary Sources", + ["A primary source is any source written by a historian"]), +] + + +def seed_offering_concept_stats() -> None: + for stats_id, off_id, concept, misconceptions in _OFFERING_CONCEPT_STATS: + h.insert_if_absent( + "offering_concept_stats", + stats_id, + { + "offering_id": off_id, + "concept_name": concept, + "student_count": 4, + "avg_mastery_score": 0.55, + "pct_mastered": 0.25, + "pct_struggling": 0.5, + "pct_unexplored": 0.25, + "common_misconceptions": misconceptions, + "effective_explanations": [], + "prerequisite_gaps": [], + }, + ) + + def seed_quiz() -> None: for qa_id, node_id, difficulty, score, total, questions, answers, completed_at in _QUIZ_ATTEMPTS: h.insert_if_absent( @@ -785,6 +845,7 @@ def seed_sessions() -> None: "room_summaries", "notes", "documents", "flashcards", "study_guides", "quiz_attempts", "quiz_context", "sessions", "messages", "feedback", "issue_reports", + "offering_concept_stats", ] @@ -804,6 +865,7 @@ def main() -> None: seed_study_guides() seed_room_summaries() seed_quiz() + seed_offering_concept_stats() seed_feedback() seed_sessions() h.print_summary(_SUMMARY_ORDER, "Seed summary (rich local dataset):") diff --git a/backend/services/tool_signals.py b/backend/services/tool_signals.py index 451d74f4..ebcf2d05 100644 --- a/backend/services/tool_signals.py +++ b/backend/services/tool_signals.py @@ -87,7 +87,17 @@ class Expect(str, Enum): # digesting, and counting one would flag every student mid-first-quiz. Expect.HAS_ATTEMPTS: ("quiz_attempts", {"completed_at": "not.is.null"}, True), Expect.HAS_GRAPH: ("graph_nodes", {}, True), - Expect.COURSE_HAS_AGGREGATES: ("offering_concept_stats", {}, False), + # `neq.{}` — rows whose misconception array actually has TEXT in it, not + # merely rows that exist. The aggregation writes a stats row per concept + # as soon as a class has any activity, and leaves `common_misconceptions` + # empty until it has something to say: on 2026-08-22 that was 0 of 72 rows + # on staging and 0 of 73 on prod. Probing for bare existence would then + # report a discrepancy on every generation for every student — the alarm + # fatigue this helper exists to avoid. The expectation is "this class has + # misconceptions to offer", so that is what the probe must ask. + Expect.COURSE_HAS_AGGREGATES: ( + "offering_concept_stats", {"common_misconceptions": "neq.{}"}, False, + ), } diff --git a/backend/tests/integration/test_misconceptions_keyspace_db.py b/backend/tests/integration/test_misconceptions_keyspace_db.py new file mode 100644 index 00000000..18db409b --- /dev/null +++ b/backend/tests/integration/test_misconceptions_keyspace_db.py @@ -0,0 +1,137 @@ +"""#553 (Workstream H1, epic #537) — the misconceptions keyspace, real-DB half. + +The bug: `read_misconceptions_for_course` filters +`offering_concept_stats.offering_id`, which holds `course_offerings.id`, but +the tool handed it `ctx.deps.course_id` — the abstract `courses.id`. Two +disjoint keyspaces, so the read matched nothing for every student since the +tool was written, and an empty list is exactly what "this class has no +misconceptions yet" looks like. + +Verified live on 2026-08-22 before the fix: staging held 72 stats rows, 72 of +which joined `course_offerings` and 0 of which joined `courses`; prod held 73 +with the same split. Filtering by course id returned 0 rows in both; filtering +by the student's offerings returned 68 + 4 (staging) and 73 (prod). + +Why this file has to exist at all: the hermetic suite mocks `table()`, so it +can assert the filter STRING without ever learning that the string selects +nothing. That is the same blind spot that let #529 survive 51 days. These +assertions run against real rows in the real schema. +""" +import asyncio +from types import SimpleNamespace + +import pytest + +pytestmark = pytest.mark.integration + +USER_ACTIVE = "rich-user-active" +USER_SECOND = "rich-user-second" +COURSE_CS = "rich-course-cs101" +OFF_CS_F25 = "rich-off-cs101-f25" +OFF_CS_S26 = "rich-off-cs101-s26" +OFF_HIST_F25 = "rich-off-hist200-f25" + +HIST_MISCONCEPTION = "A primary source is any source written by a historian" + + +def _read(offering_ids): + from agents.tools.graph_read import read_misconceptions_for_course + + return asyncio.run(read_misconceptions_for_course(offering_ids)) + + +def test_seed_stats_key_on_offerings_never_on_courses(db_conn): + """The premise. If this ever inverts, the rest of the file is testing a + keyspace that no longer exists.""" + row = db_conn.execute( + """ + SELECT count(*) FILTER (WHERE o.id IS NOT NULL) AS via_offering, + count(*) FILTER (WHERE c.id IS NOT NULL) AS via_course, + count(*) AS total + FROM offering_concept_stats s + LEFT JOIN course_offerings o ON o.id = s.offering_id + LEFT JOIN courses c ON c.id = s.offering_id + """ + ).fetchone() + assert row["total"] > 0, "rich seed should provide offering_concept_stats rows" + assert row["via_offering"] == row["total"] + assert row["via_course"] == 0 + + +def test_the_abstract_course_id_matches_nothing(db_conn): + """The bug, pinned as a fact about the data rather than about the code: + passing the course id where an offering id belongs selects zero rows. A + future refactor that reintroduces it cannot pass this file.""" + row = db_conn.execute( + "SELECT count(*) AS n FROM offering_concept_stats WHERE offering_id = %s", + (COURSE_CS,), + ).fetchone() + assert row["n"] == 0 + assert _read([COURSE_CS]) == [] + + +def test_reads_misconceptions_across_all_of_the_students_offerings(): + """The fix. Non-zero rows on the rich seed, as #553 requires — and drawn + from BOTH of the student's offerings of the same abstract course, so a + fix that resolves a single "current" offering still fails here.""" + out = _read([OFF_CS_F25, OFF_CS_S26]) + texts = {m.text for m in out} + + assert texts, "the fixed keyspace must return real misconceptions" + assert any("base case is optional" in t for t in texts), "missing F25 offering" + assert any("`else if` evaluates every branch" in t for t in texts), "missing S26 offering" + # Concept attribution survives the flattening — the agent routes + # distractors per-concept. + assert {m.related_concept for m in out} >= {"Recursion", "Control Flow"} + + +def test_another_classs_misconceptions_never_leak(): + """The negative half, and the reason the offering filter can't simply be + dropped: HIST200 has misconception text, and the active user is not in it.""" + out = _read([OFF_CS_F25, OFF_CS_S26]) + assert all(HIST_MISCONCEPTION != m.text for m in out) + + # ...and it IS readable when its own offering is the one asked for, so the + # assertion above is about scoping, not about the row being absent. + assert any(HIST_MISCONCEPTION == m.text for m in _read([OFF_HIST_F25])) + + +def test_empty_arrays_contribute_nothing_but_are_not_an_error(): + """A stats row with no text is the normal early-term state (0 of 72 rows + on staging carried text). It must flatten to nothing without suppressing + its siblings in the same offering.""" + out = _read([OFF_CS_S26]) + assert [m.text for m in out] == ["`else if` evaluates every branch before choosing one"] + + +def test_tool_wrapper_resolves_the_course_through_the_students_enrollments(): + """End to end through the wrapper the agent actually calls: give it the + ABSTRACT course id in deps — the shape `routes/quiz.py` passes — and it + must still come back with the class's misconceptions.""" + from agents.tools.graph_read import read_misconceptions_for_course_tool + + ctx = SimpleNamespace( + deps=SimpleNamespace( + user_id=USER_ACTIVE, course_id=COURSE_CS, feature="quiz", + ) + ) + out = asyncio.run(read_misconceptions_for_course_tool(ctx)) + assert {m.text for m in out}, "wrapper must resolve course -> offerings" + assert all(HIST_MISCONCEPTION != m.text for m in out) + + +def test_a_student_in_a_different_offering_sees_their_own_class(): + """Scoping is per-student, not per-course: the second user holds only the + S26 offering of CS, so the F25 class's misconceptions are not theirs.""" + from agents.tools.graph_read import read_misconceptions_for_course_tool + + ctx = SimpleNamespace( + deps=SimpleNamespace( + user_id=USER_SECOND, course_id=COURSE_CS, feature="quiz", + ) + ) + texts = {m.text for m in asyncio.run(read_misconceptions_for_course_tool(ctx))} + assert any("`else if` evaluates every branch" in t for t in texts) + assert not any("base case is optional" in t for t in texts), ( + "USER_SECOND is not enrolled in the F25 offering" + ) diff --git a/backend/tests/test_quiz_tool_instrumentation.py b/backend/tests/test_quiz_tool_instrumentation.py index 9547ad5e..caed82dc 100644 --- a/backend/tests/test_quiz_tool_instrumentation.py +++ b/backend/tests/test_quiz_tool_instrumentation.py @@ -248,12 +248,14 @@ def test_misconceptions_tool_skips_the_probe_with_no_resolvable_offering(sink): def test_misconceptions_tool_is_silent_when_it_returns_rows(sink): - """And costs nothing: the offering resolution behind the probe is uncached - and issues two unbounded PostgREST reads, so gating it on "a course id - exists" instead of on the result being EMPTY made every quiz generation - pay both round-trips even when the tool had rows to return — - contradicting tool_signals' own contract (one owner-scoped indexed read, - only on the empty path).""" + """No event and no PROBE when the tool has rows to return. + + The offering resolution itself is no longer probe-only: since #553 the + READ needs those ids (the stats table is keyed on `course_offerings.id`), + so it runs exactly once per call, on both paths. What must stay off the + non-empty path is the probe — `report_empty_result` would short-circuit + on a non-zero count anyway, and firing it here would put a discrepancy + event on a tool that just worked.""" resolve = MagicMock(return_value=["off-1"]) with ( _probe(True) as probe, @@ -267,7 +269,8 @@ def test_misconceptions_tool_is_silent_when_it_returns_rows(sink): events_service.flush_now() assert sink == [] assert len(out) == 1 - resolve.assert_not_called() + # Resolved once — the read's own input, not redundant probe work. + assert resolve.call_count == 1 probe.assert_not_called() @@ -343,3 +346,46 @@ def test_tools_work_outside_a_capture_scope(sink): ): asyncio.run(read_misconceptions_for_course_tool(_ctx())) assert prompt_dimensions.snapshot() == {} + + +# ── #553: the misconceptions tool must query the OFFERING keyspace ────────── + +def test_misconceptions_tool_filters_by_the_students_offerings_not_the_course_id(): + """H1/#553. `offering_concept_stats.offering_id` holds `course_offerings.id`; + `ctx.deps.course_id` is the ABSTRACT `courses.id`. Passing the second where + the first is expected matched nothing, for every student, forever — verified + live on 2026-08-22: staging 72/72 stats rows key on an offering id and 0 on a + course id, prod 73/73, and filtering by course id returned 0 rows in both. + + So the tool must resolve the student's offerings of that course and filter on + those. A student can hold more than one offering of the same course (the rich + seed's active user has CS in two terms), so this is a set, not a scalar. + """ + captured: dict = {} + + def fake_table(name): + m = MagicMock() + if name == "offering_concept_stats": + def _select(cols, **kw): + captured["filters"] = kw.get("filters") + return [{"concept_name": "Recursion", + "common_misconceptions": ["All recursion is infinite"]}] + m.select.side_effect = _select + else: + m.select.return_value = [] + return m + + with ( + patch("agents.tools.graph_read.table", side_effect=fake_table), + patch( + "agents.tools.graph_read.user_offering_ids_for_course", + return_value=["off-cs-f25", "off-cs-s26"], + ), + ): + out = asyncio.run(read_misconceptions_for_course_tool(_ctx())) + + assert [m.text for m in out] == ["All recursion is infinite"] + offering_filter = (captured.get("filters") or {}).get("offering_id") + assert offering_filter == "in.(off-cs-f25,off-cs-s26)", ( + f"expected an offering-keyspace IN filter, got {offering_filter!r}" + ) From 16f36c3b9fdf9e742bdd6b3deb61a445ee58b32a Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 03:34:41 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(quiz):=20address=20#553=20review=20?= =?UTF-8?q?=E2=80=94=20the=20fix=20was=20only=20half=20a=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from `/code-review high`. Two changed the shape of the fix. **The read could still return [] for a class that HAS misconceptions.** It kept `updated_at.desc LIMIT 20` with no filter on rows carrying text — and `course_context_service` stamps every row of one aggregation pass with the same timestamp, so ordering within an offering is arbitrary. Text-bearing rows are the rare minority (0 of 72 rows on staging, 0 of 73 on prod carry text today), so the window fills with empty rows and the tool hands back nothing: the exact symptom #553 exists to fix, surviving the fix. The read now filters `common_misconceptions=neq.{}`, which also makes it ask the SAME question the F5 probe asks — otherwise every such class emits a permanent false `quiz.tool_empty` on every generation. **The Class-intel opt-out was never actually enforced.** The tool is registered on quiz_agent unconditionally and system-prompt step 2 tells the model to call it every run; `use_shared_context` only ever APPENDED a routing sentence when true. That looked correct only because the read was keyspace-broken and returned [] for everyone — fixing #553 would have started feeding other students' aggregated misconceptions to students who opted out, while the same run recorded `misconceptions_requested: False`. The consent now rides `SaplingDeps.share_class_context` and the tool returns [] before reading anything. Enforced at the tool, not in the prompt: a system-prompt instruction is a request to a model, and consent is not something to leave to one. Also: - **Per-offering reads.** One shared `LIMIT` over `in.(a,b)` with an arbitrary sort meant an offering with a full window starved its sibling — reintroducing, per offering, the silent drop that taking a LIST was added to prevent. Students hold one or two offerings of a course, so this is one or two indexed reads. - **Cap what reaches the prompt.** The old cap counted ROWS, and each row carries an unbounded array, so the block's real size was never bounded. `_MAX_MISCONCEPTIONS` bounds the unit that costs tokens. - Probe filter pinned by integration tests against real PostgREST (a typo would degrade to "can't tell" and leave the seam inert while looking like "no discrepancies found"), plus one asserting probe and read agree. - `Expect.COURSE_HAS_AGGREGATES` docstring said "aggregates exist" when the probe now means "aggregates carrying text". - The two premise tests the review flagged as FK-guaranteed now say so, rather than presenting as guards they aren't. Hermetic 2131 passed / 9 skipped, ruff clean. Co-Authored-By: Claude Opus 5 --- backend/agents/deps.py | 10 ++ backend/agents/tools/graph_read.py | 87 ++++++++++++++--- backend/routes/quiz.py | 6 ++ backend/services/tool_signals.py | 10 +- .../test_misconceptions_keyspace_db.py | 61 +++++++++++- .../tests/test_quiz_tool_instrumentation.py | 97 +++++++++++++++++-- 6 files changed, 244 insertions(+), 27 deletions(-) diff --git a/backend/agents/deps.py b/backend/agents/deps.py index d83e3a4f..70c04057 100644 --- a/backend/agents/deps.py +++ b/backend/agents/deps.py @@ -47,6 +47,15 @@ class SaplingDeps: identical behavior. Evals inject a FixtureRetrieval here so record/live runs never touch a database. Typed Any to avoid the circular import deps → retrieval → chat_context → deps. + share_class_context: Whether this student consented to class-derived + data (the "Class intel" toggle, migration 0037). Tools that read + OTHER students' aggregated work — `read_misconceptions_for_course` + — must return nothing when it is False. It lives here rather than + in the prompt because a system-prompt instruction is a request to + a model, and consent is not something to leave to one: the tool is + registered on quiz_agent unconditionally and the prompt tells the + model to call it every run. Defaults True to match the column + default; an explicit False is the only thing that suppresses. """ user_id: str @@ -55,6 +64,7 @@ class SaplingDeps: request_id: str session_id: str | None = None feature: str = "unknown" + share_class_context: bool = True graph_updates: list = field(default_factory=list) mastery_changes: list = field(default_factory=list) retrieval: Any = None diff --git a/backend/agents/tools/graph_read.py b/backend/agents/tools/graph_read.py index 7889d7ce..3e153ea3 100644 --- a/backend/agents/tools/graph_read.py +++ b/backend/agents/tools/graph_read.py @@ -367,6 +367,14 @@ class Misconception(BaseModel): related_concept: str | None = None +#: Row budget per offering. Applied per-offering rather than shared, so one +#: class cannot starve another when a student holds two offerings of a course. +_ROWS_PER_OFFERING = 20 +#: Ceiling on the misconception STRINGS handed to the model. Rows are not the +#: unit that costs prompt tokens; entries are. +_MAX_MISCONCEPTIONS = 40 + + async def read_misconceptions_for_course( offering_ids: Sequence[str] | None, ) -> list[Misconception]: @@ -411,21 +419,48 @@ class in a term). Anonymized (sourced from class-wide patterns, not any return [] def _fetch() -> list[dict[str, Any]]: - try: - return ( - table("offering_concept_stats").select( - "concept_name,common_misconceptions", - filters={"offering_id": f"in.({','.join(ids)})"}, - order="updated_at.desc", - limit=20, + rows: list[dict[str, Any]] = [] + # One read PER offering rather than one `in.(...)` read over all of + # them. A single query has to share one LIMIT, and the sort key does + # not break the tie usefully: `course_context_service` stamps every + # row of an aggregation pass with the same `updated_at`, so ordering + # within an offering is arbitrary. An offering with a full window of + # rows would then starve its sibling completely — reintroducing, per + # offering, exactly the silent drop that taking a LIST of offerings + # was meant to prevent. Students hold one or two offerings of a given + # course, so this is one or two indexed reads. + for offering_id in ids: + try: + rows.extend( + table("offering_concept_stats").select( + "concept_name,common_misconceptions", + filters={ + "offering_id": f"eq.{offering_id}", + # Spend the row budget only on rows that actually + # carry text. The aggregation writes a stats row + # per concept as soon as a class has activity and + # fills this array only when it has something to + # say, so text-bearing rows are the rare minority + # (0 of 72 rows on staging, 0 of 73 on prod). + # Unfiltered, the window fills with empty rows and + # the tool returns [] for a class that genuinely + # has misconceptions — the very symptom #553 is + # about. It also keeps this read asking the same + # question the F5 probe asks, so a legitimately + # quiet class cannot look like a broken one. + "common_misconceptions": "neq.{}", + }, + order="updated_at.desc", + limit=_ROWS_PER_OFFERING, + ) + or [] ) - or [] - ) - except Exception: - logger.exception( - "read_misconceptions_for_course failed for offerings=%s", ids, - ) - return [] + except Exception: + logger.exception( + "read_misconceptions_for_course failed for offering=%s", + offering_id, + ) + return rows rows = await asyncio.to_thread(_fetch) out: list[Misconception] = [] @@ -441,6 +476,13 @@ def _fetch() -> list[dict[str, Any]]: continue seen.add(key) out.append(Misconception(text=text, related_concept=concept)) + # Cap what actually reaches the prompt. The old `limit=20` capped + # ROWS, and each row carries an unbounded array — so the block's + # real size was never bounded at all. F6 measured this tool's + # contribution to the prompt; bounding the unit that costs tokens + # is what makes that number hold. + if len(out) >= _MAX_MISCONCEPTIONS: + return out return out @@ -457,6 +499,23 @@ async def read_misconceptions_for_course_tool( """ from services.prompt_safety import neutralize_delimiters + # Class-intel consent, enforced at the tool (#553 review finding 4). + # + # This tool is registered on quiz_agent unconditionally and system-prompt + # step 2 tells the model to call it on EVERY run; `use_shared_context` + # only ever APPENDED an extra routing sentence when true. That looked + # correct for as long as the read was keyspace-broken and returned [] + # for everyone — fixing #553 would have quietly started feeding other + # students' aggregated misconceptions to a student who opted out. + # + # Enforced here rather than by editing the prompt or the toolset: a + # system-prompt instruction is a request to a model, and consent is not + # something to leave to one. Returning [] (not raising) keeps an opted-out + # run identical to a class with nothing to share. + if not getattr(ctx.deps, "share_class_context", True): + prompt_dimensions.record(misconceptions=0) + return [] + # #553: resolve course -> the student's offerings BEFORE reading. The # stats table is keyed on `course_offerings.id`; `ctx.deps.course_id` is # the abstract `courses.id` the graph carries. Handing the second to a diff --git a/backend/routes/quiz.py b/backend/routes/quiz.py index d46560a6..82ce92fb 100644 --- a/backend/routes/quiz.py +++ b/backend/routes/quiz.py @@ -822,6 +822,12 @@ async def _quiz_via_agent( supabase=None, request_id=request_id, feature="quiz", + # The Class-intel opt-out reaches the misconceptions tool through + # deps, not through the prompt: the tool is registered on the agent + # unconditionally and step 2 of the system prompt tells the model to + # call it every run, so the routing sentence below can only ever ADD + # emphasis — it cannot withhold the data (#553 review). + share_class_context=use_shared_context, ) # Keep this message routing-only; the workflow + adaptive rules # live in the system prompt. We just hand the agent the inputs it diff --git a/backend/services/tool_signals.py b/backend/services/tool_signals.py index ebcf2d05..262a0e31 100644 --- a/backend/services/tool_signals.py +++ b/backend/services/tool_signals.py @@ -68,8 +68,14 @@ class Expect(str, Enum): HAS_ATTEMPTS = "has_attempts" #: Has at least one knowledge-graph node — so concept reads could return rows. HAS_GRAPH = "has_graph" - #: Class aggregates exist for the caller-supplied offerings. NOT - #: owner-scoped — `offering_concept_stats` has no user_id, by design + #: Class aggregates CARRYING MISCONCEPTION TEXT exist for the + #: caller-supplied offerings. Not merely "a stats row exists": the + #: aggregation writes a row per concept as soon as a class has any + #: activity and fills `common_misconceptions` only when it has something + #: to say, so bare existence is true for classes that have nothing to + #: offer and would fire on every generation (see the filter below). + #: + #: NOT owner-scoped — `offering_concept_stats` has no user_id, by design #: (it is anonymized class data), so the caller must supply the offering #: scope. This is the probe that catches a KEYSPACE mismatch: aggregates #: exist for this class, but the tool's own query found none — which is diff --git a/backend/tests/integration/test_misconceptions_keyspace_db.py b/backend/tests/integration/test_misconceptions_keyspace_db.py index 18db409b..b6e0f6d2 100644 --- a/backend/tests/integration/test_misconceptions_keyspace_db.py +++ b/backend/tests/integration/test_misconceptions_keyspace_db.py @@ -41,8 +41,12 @@ def _read(offering_ids): def test_seed_stats_key_on_offerings_never_on_courses(db_conn): - """The premise. If this ever inverts, the rest of the file is testing a - keyspace that no longer exists.""" + """The premise, documented rather than defended: migration 0022's FK from + `offering_concept_stats.offering_id` to `course_offerings(id)` already + guarantees this, so it cannot fail while that FK stands. It earns its place + by making the keyspace claim the rest of the file rests on explicit, and it + WOULD fail if that FK were ever dropped — which is the change that would + silently make everything below meaningless.""" row = db_conn.execute( """ SELECT count(*) FILTER (WHERE o.id IS NOT NULL) AS via_offering, @@ -135,3 +139,56 @@ def test_a_student_in_a_different_offering_sees_their_own_class(): assert not any("base case is optional" in t for t in texts), ( "USER_SECOND is not enrolled in the F25 offering" ) + + +def test_the_probe_filter_selects_only_rows_that_carry_text(db_conn): + """#553 review finding 5. The F5 probe's `neq.{}` filter is what keeps a + legitimately quiet class from being reported as a broken one, and the + hermetic suite can only assert the filter STRING — the same blindness that + let #529 and #553 survive. So the filter is exercised against real + PostgREST here, where a typo would show up. + + A broken filter degrades to `logger.warning` + "can't tell", which leaves + the whole seam inert while looking exactly like "no discrepancies found": + the module's own stated bug class, one layer up. + """ + from db.connection import table + + with_text = table("offering_concept_stats").select( + "id", filters={"common_misconceptions": "neq.{}"}, + ) or [] + all_rows = table("offering_concept_stats").select("id") or [] + + assert with_text, "the filter must not reject every row" + assert len(with_text) < len(all_rows), ( + "the seed carries an empty-array row on purpose; if the filter keeps " + "everything it is not discriminating and the probe is inert" + ) + + # And it agrees with the database's own answer. + row = db_conn.execute( + "SELECT count(*) AS n FROM offering_concept_stats " + "WHERE common_misconceptions <> '{}'" + ).fetchone() + assert len(with_text) == row["n"] + + +def test_probe_and_read_ask_the_same_question(db_conn): + """#553 review finding 3. If the probe filters on text-bearing rows and + the read does not, an offering whose text-bearing rows fall outside the + read's row window returns [] while the probe says data exists — a + `quiz.tool_empty` warning on EVERY generation for every student in that + class, indefinitely. They have to be answering the same question.""" + from db.connection import table + from services.tool_signals import _PROBES, Expect + + _table, probe_filters, _owner_scoped = _PROBES[Expect.COURSE_HAS_AGGREGATES] + assert probe_filters.get("common_misconceptions") == "neq.{}" + + # The read applies it too: every offering the seed gives text for must + # come back non-empty through the tool. + assert _read([OFF_CS_F25]), "read must surface an offering the probe counts" + probe_rows = table("offering_concept_stats").select( + "id", filters={"offering_id": f"eq.{OFF_CS_F25}", **probe_filters}, + ) or [] + assert probe_rows diff --git a/backend/tests/test_quiz_tool_instrumentation.py b/backend/tests/test_quiz_tool_instrumentation.py index caed82dc..6d553e28 100644 --- a/backend/tests/test_quiz_tool_instrumentation.py +++ b/backend/tests/test_quiz_tool_instrumentation.py @@ -36,9 +36,14 @@ def _dims(): prompt_dimensions.clear() -def _ctx(user_id="u1", course_id="c1", feature="quiz"): +def _ctx(user_id="u1", course_id="c1", feature="quiz", share_class_context=True): return SimpleNamespace( - deps=SimpleNamespace(user_id=user_id, course_id=course_id, feature=feature) + deps=SimpleNamespace( + user_id=user_id, + course_id=course_id, + feature=feature, + share_class_context=share_class_context, + ) ) @@ -361,15 +366,20 @@ def test_misconceptions_tool_filters_by_the_students_offerings_not_the_course_id those. A student can hold more than one offering of the same course (the rich seed's active user has CS in two terms), so this is a set, not a scalar. """ - captured: dict = {} + seen_filters: list[dict] = [] def fake_table(name): m = MagicMock() if name == "offering_concept_stats": def _select(cols, **kw): - captured["filters"] = kw.get("filters") - return [{"concept_name": "Recursion", - "common_misconceptions": ["All recursion is infinite"]}] + f = kw.get("filters") or {} + seen_filters.append(f) + # Only the first offering has anything to say, which is also + # how a real pair of offerings usually looks. + if f.get("offering_id") == "eq.off-cs-f25": + return [{"concept_name": "Recursion", + "common_misconceptions": ["All recursion is infinite"]}] + return [] m.select.side_effect = _select else: m.select.return_value = [] @@ -385,7 +395,76 @@ def _select(cols, **kw): out = asyncio.run(read_misconceptions_for_course_tool(_ctx())) assert [m.text for m in out] == ["All recursion is infinite"] - offering_filter = (captured.get("filters") or {}).get("offering_id") - assert offering_filter == "in.(off-cs-f25,off-cs-s26)", ( - f"expected an offering-keyspace IN filter, got {offering_filter!r}" + + queried = [f.get("offering_id") for f in seen_filters] + # Every offering asked for, none skipped — a single shared LIMIT would let + # a full first offering starve the second. + assert queried == ["eq.off-cs-f25", "eq.off-cs-s26"], queried + # And the abstract course id never appears in the offering keyspace. + assert not any("eq.c1" == q for q in queried) + + +def test_misconceptions_tool_honors_the_class_intel_opt_out(): + """#553 review finding 4. `read_misconceptions_for_course_tool` is + registered on quiz_agent unconditionally and system-prompt step 2 tells the + model to call it on EVERY run; `use_shared_context` only ever APPENDED an + extra routing sentence when true. That looked fine only because the read + was keyspace-broken and always returned [] — fixing #553 would have started + feeding other students' aggregated misconceptions to a student who + explicitly opted out. + + Enforced at the tool, not in the prompt: a system-prompt instruction is a + request to a model, and consent is not something to leave to one. + """ + with ( + patch("agents.tools.graph_read.table") as t, + patch( + "agents.tools.graph_read.user_offering_ids_for_course", + return_value=["off-1"], + ) as resolve, + ): + out = asyncio.run( + read_misconceptions_for_course_tool(_ctx(share_class_context=False)) + ) + + assert out == [] + # Not merely filtered afterwards — never read at all. + t.assert_not_called() + resolve.assert_not_called() + + +def test_misconceptions_read_asks_only_for_rows_that_carry_text(): + """#553 review finding 1. The read takes `updated_at.desc` LIMIT 20, and + `course_context_service` stamps every row of one aggregation pass with the + same timestamp — so the ordering among an offering's rows is arbitrary. + Text-bearing rows are the rare minority (0 of 72 on staging, 0 of 73 on + prod carried text), so an unfiltered 20-row window can easily contain none + of them and hand back [] for a class that genuinely has misconceptions — + the exact symptom #553 exists to fix, and a permanent false `tool_empty` + besides, since the probe DOES filter on text. + """ + captured: dict = {} + + def fake_table(name): + m = MagicMock() + + def _select(cols, **kw): + captured["filters"] = kw.get("filters") + return [] + + m.select.side_effect = _select + return m + + with ( + patch("agents.tools.graph_read.table", side_effect=fake_table), + patch( + "agents.tools.graph_read.user_offering_ids_for_course", + return_value=["off-1"], + ), + _probe(False), + ): + asyncio.run(read_misconceptions_for_course_tool(_ctx())) + + assert (captured.get("filters") or {}).get("common_misconceptions") == "neq.{}", ( + "the read must spend its row budget on rows that actually carry text" )