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 df2d6d6d..3e153ea3 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 @@ -366,12 +367,35 @@ 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_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,26 +406,61 @@ 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]]: - try: - return ( - table("offering_concept_stats").select( - "concept_name,common_misconceptions", - filters={"offering_id": f"eq.{offering_id}"}, - 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 offering=%s", - offering_id, - ) - 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] = [] @@ -417,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 @@ -433,49 +499,74 @@ 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. + # Class-intel consent, enforced at the tool (#553 review finding 4). # - # 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. + # 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. # - # 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] = [] + # 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 + # 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. + # + # 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/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 451d74f4..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 @@ -87,7 +93,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..b6e0f6d2 --- /dev/null +++ b/backend/tests/integration/test_misconceptions_keyspace_db.py @@ -0,0 +1,194 @@ +"""#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, 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, + 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" + ) + + +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 9547ad5e..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, + ) ) @@ -248,12 +253,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 +274,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 +351,120 @@ 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. + """ + seen_filters: list[dict] = [] + + def fake_table(name): + m = MagicMock() + if name == "offering_concept_stats": + def _select(cols, **kw): + 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 = [] + 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"] + + 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" + )