diff --git a/backend/agents/tools/chat_context.py b/backend/agents/tools/chat_context.py index f5068aff..cd167d3e 100644 --- a/backend/agents/tools/chat_context.py +++ b/backend/agents/tools/chat_context.py @@ -44,6 +44,7 @@ from pydantic_ai import RunContext from agents.deps import SaplingDeps +from config import is_mastered, is_weak from db.connection import table from services.encryption import decrypt_if_present, decrypt_json @@ -388,12 +389,13 @@ async def read_session_history_tool( # read_user_progress -# Mastery thresholds — duplicated here (rather than imported from -# graph_service) so the tool stays self-contained and the agent's -# definitions of 'mastered' / 'weak' can evolve independently from the -# spaced-repetition scheduling logic. -_MASTERED_THRESHOLD = 0.7 -_WEAK_THRESHOLD = 0.4 +# Mastery thresholds come from config (#557). They used to be duplicated +# here — 0.7/0.4 against the canonical 0.75/0.45 — on the stated rationale +# that "the agent's definitions can evolve independently". They didn't +# evolve; they drifted, and the result was a student reading "Struggling" +# on the Tree while this tool counted them as in-progress in the same +# session. If the tutor ever needs a genuinely different cut, it gets a +# named constant in config.py, not a literal here. class CourseProgress(BaseModel): @@ -402,9 +404,11 @@ class CourseProgress(BaseModel): clamped to [0, 1] and is 0.0 when there are no concepts.""" total_concepts: int = Field(ge=0) - mastered_count: int = Field(ge=0) # mastery >= 0.7 - weak_count: int = Field(ge=0) # mastery < 0.4 - in_progress_count: int = Field(ge=0) # 0.4 <= mastery < 0.7 + # Tiers per config.get_mastery_tier (#557): "mastered", "struggling" + + # "unexplored" (together: weak), and "learning" (in progress). + mastered_count: int = Field(ge=0) + weak_count: int = Field(ge=0) + in_progress_count: int = Field(ge=0) avg_mastery: float = Field(ge=0.0, le=1.0) @@ -470,9 +474,9 @@ def _fetch() -> list[dict[str, Any]]: m = max(0.0, min(1.0, m)) total += 1 mastery_sum += m - if m >= _MASTERED_THRESHOLD: + if is_mastered(m): mastered += 1 - elif m < _WEAK_THRESHOLD: + elif is_weak(m): weak += 1 else: in_progress += 1 diff --git a/backend/config.py b/backend/config.py index 636984dc..ab7073a5 100644 --- a/backend/config.py +++ b/backend/config.py @@ -92,16 +92,55 @@ def validate_config() -> None: ) +# ── Mastery tiers (#557) ──────────────────────────────────────────────────── +# +# THE thresholds. Every Python surface that classifies a mastery score reads +# them from here — the graph writes that denormalize `mastery_tier`, the +# tutor's progress tool, flashcard selection, the seeds. Three sets used to +# exist (this one, the tutor's 0.7/0.4, and flashcards' ad-hoc <0.4), which +# meant a student could read "Struggling" on the Tree and be counted as +# in-progress by the tutor in the same session. +# +# ONE mirror is unavoidable and is therefore pinned by test rather than by +# hope: `frontend/src/components/screens/Learn.tsx::tierForScore` re-declares +# these to classify a STREAMED mastery delta client-side, so the live Tree +# agrees with the refetch that follows it. It cannot import from here, so +# `tests/test_mastery_tier_unification.py` reads that file and asserts the +# numbers match. Change these and that test tells you what else to change. +# +# If a surface ever needs a genuinely different cut, name it HERE as its own +# constant with the reason. A local literal is how the last three diverged. +MASTERY_MASTERED_MIN = 0.75 +MASTERY_LEARNING_MIN = 0.45 +MASTERY_STRUGGLING_MIN = 0.1 + + def get_mastery_tier(score: float) -> str: - if score >= 0.75: + if score >= MASTERY_MASTERED_MIN: return "mastered" - elif score >= 0.45: + elif score >= MASTERY_LEARNING_MIN: return "learning" - elif score >= 0.1: + elif score >= MASTERY_STRUGGLING_MIN: return "struggling" return "unexplored" +def is_mastered(score: float) -> bool: + """The top tier — the same one the Tree labels "mastered".""" + return score >= MASTERY_MASTERED_MIN + + +def is_weak(score: float) -> bool: + """Below the learning floor: "struggling" OR "unexplored". + + Both mean "not yet learning this", which is the question every caller is + actually asking — which concepts need work (weak counts, flashcard drills, + quiz focus). Splitting them here would just push the union back out to the + call sites, which is where the drift came from. + """ + return score < MASTERY_LEARNING_MIN + + def build_commit() -> str: """Short git SHA of the running build, or "unknown". diff --git a/backend/routes/flashcards.py b/backend/routes/flashcards.py index 95d22c0d..15c2cf28 100644 --- a/backend/routes/flashcards.py +++ b/backend/routes/flashcards.py @@ -11,6 +11,7 @@ from fastapi.responses import JSONResponse from pydantic import BaseModel +from config import is_weak from db.connection import table from services.academics import resolve_offering, term_id_for_label from services.auth_guard import require_self, get_session_user_id @@ -175,8 +176,13 @@ def _get_course_documents( def _get_weak_concepts(user_id: str, course_name: str) -> list[str]: """ - Return concept names where the student has low mastery (score < 0.4) - for the given course/subject. + Return concept names the student is weak on — below the "learning" floor + in `config.get_mastery_tier`, i.e. "struggling" or "unexplored". + + Was a local `< 0.4` (#557), which is not any tier boundary: concepts in + [0.4, 0.45) read as "struggling" on the Tree but were never offered for + practice here — the surface whose entire job is drilling weak concepts + silently skipped a slice of them. """ try: rows = table("graph_nodes").select( @@ -189,12 +195,17 @@ def _get_weak_concepts(user_id: str, course_name: str) -> list[str]: "concept_name,mastery_score", filters={"user_id": f"eq.{user_id}"}, ) - weak = [ - r["concept_name"] - for r in (rows or []) - if (r.get("mastery_score") or 0) < 0.4 - ] - return weak[:15] # cap to keep prompt reasonable + weak = sorted( + (r for r in (rows or []) if is_weak(r.get("mastery_score") or 0)), + key=lambda r: r.get("mastery_score") or 0, + ) + # Weakest first, THEN cap. The cap used to truncate in PostgREST row + # order, which was survivable while the floor was 0.4 and is not now + # that #557 widened it to 0.45: the newly-admitted [0.4, 0.45) + # concepts could displace 0.0-0.1 ones purely on row order, leaving + # the surface whose job is drilling the weakest concepts drilling the + # least-weak of the weak. + return [r["concept_name"] for r in weak[:15]] except Exception: return [] diff --git a/backend/tests/test_chat_context_tools.py b/backend/tests/test_chat_context_tools.py index 2945e15c..00922843 100644 --- a/backend/tests/test_chat_context_tools.py +++ b/backend/tests/test_chat_context_tools.py @@ -286,14 +286,19 @@ def test_empty_session_id_short_circuits(self): class TestReadUserProgress: def test_aggregates_mastered_weak_in_progress(self): - # Thresholds: mastered >= 0.7, weak < 0.4, in_progress in [0.4, 0.7). + # Thresholds come from config.get_mastery_tier (#557): mastered + # >= 0.75, learning >= 0.45, below that is weak (struggling or + # unexplored). This tool used to carry its own 0.7/0.4, which is why + # 0.4 counts as WEAK here and used to count as in-progress — the + # divergence a student saw as "Struggling on the Tree, in-progress to + # the tutor". rows = [ {"mastery_score": 0.9}, # mastered - {"mastery_score": 0.75}, # mastered - {"mastery_score": 0.5}, # in_progress - {"mastery_score": 0.4}, # in_progress (boundary) - {"mastery_score": 0.2}, # weak - {"mastery_score": 0.0}, # weak + {"mastery_score": 0.75}, # mastered (boundary) + {"mastery_score": 0.5}, # learning + {"mastery_score": 0.4}, # weak — below the 0.45 learning floor + {"mastery_score": 0.2}, # struggling -> weak + {"mastery_score": 0.0}, # unexplored -> weak ] with patch("agents.tools.chat_context.table") as t: t.return_value.select.return_value = rows @@ -302,8 +307,8 @@ def test_aggregates_mastered_weak_in_progress(self): assert isinstance(result, CourseProgress) assert result.total_concepts == 6 assert result.mastered_count == 2 - assert result.weak_count == 2 - assert result.in_progress_count == 2 + assert result.weak_count == 3 + assert result.in_progress_count == 1 # avg_mastery is rounded to 4dp; sum/6 = 2.75/6 = 0.4583... assert abs(result.avg_mastery - round(2.75 / 6, 4)) < 1e-6 diff --git a/backend/tests/test_mastery_tier_unification.py b/backend/tests/test_mastery_tier_unification.py new file mode 100644 index 00000000..b6e521e3 --- /dev/null +++ b/backend/tests/test_mastery_tier_unification.py @@ -0,0 +1,153 @@ +"""#557 (Workstream H5, epic #537): one set of mastery thresholds. + +Three divergent sets existed — `config.get_mastery_tier`'s canonical +0.75/0.45/0.1, the tutor's 0.7/0.4, and flashcards' ad-hoc <0.4 — so a +student could read "Struggling" on the Tree and be counted as in-progress by +the tutor in the same session. These tests pin the agreement rather than the +numbers, so the thresholds stay movable in ONE place. +""" +import asyncio +from unittest.mock import patch + +import pytest + +from config import get_mastery_tier, is_mastered, is_weak + + +# One value inside every tier plus every boundary, including the ones the old +# tutor thresholds fell between (0.4-0.45 and 0.7-0.75) — the exact band where +# the two vocabularies disagreed. +SCORES = [ + 0.0, 0.05, 0.09, 0.1, 0.25, 0.39, 0.4, 0.42, 0.44, + 0.45, 0.5, 0.69, 0.7, 0.72, 0.74, 0.75, 0.8, 1.0, +] + + +@pytest.mark.parametrize("score", SCORES) +def test_predicates_agree_with_the_tier_they_describe(score): + tier = get_mastery_tier(score) + assert is_mastered(score) is (tier == "mastered") + # "Weak" is everything below the learning floor: struggling AND unexplored. + assert is_weak(score) is (tier in {"struggling", "unexplored"}) + + +@pytest.mark.parametrize("score", SCORES) +def test_the_tutor_classifies_a_concept_the_same_way_the_tree_labels_it(score): + """The user-visible invariant, and the whole point of #557: whatever the + Tree calls a concept, the tutor must count it as the same thing. + + Driven through the real tool rather than through its constants, so + reintroducing a local threshold anywhere in that path fails here. + """ + from agents.tools import chat_context + + with patch.object( + chat_context, "table", + ) as t: + t.return_value.select.return_value = [{"mastery_score": score}] + progress = asyncio.run( + chat_context.read_user_progress("u1", "c1") + ) + + tier = get_mastery_tier(score) + assert progress.total_concepts == 1 + assert progress.mastered_count == (1 if tier == "mastered" else 0) + assert progress.weak_count == (1 if tier in {"struggling", "unexplored"} else 0) + assert progress.in_progress_count == (1 if tier == "learning" else 0) + + +def test_no_module_redefines_the_thresholds_locally(): + """#557's actual failure mode was three copies drifting apart, not one + wrong number. Cite config; don't re-declare.""" + from agents.tools import chat_context + + assert not hasattr(chat_context, "_MASTERED_THRESHOLD") + assert not hasattr(chat_context, "_WEAK_THRESHOLD") + + +def test_flashcards_weak_concepts_use_the_shared_floor(): + """Flashcards drilled `< 0.4`, so concepts in [0.4, 0.45) — struggling on + the Tree — were never offered for practice.""" + from routes import flashcards + + rows = [ + {"concept_name": "just-below-learning", "mastery_score": 0.42}, + {"concept_name": "learning", "mastery_score": 0.5}, + ] + with patch.object(flashcards, "table") as t: + t.return_value.select.return_value = rows + weak = flashcards._get_weak_concepts("u1", "CS101") + + assert weak == ["just-below-learning"] + + +def test_the_frontend_mirror_matches_the_backend_thresholds(): + """The fourth copy, and the one that cannot import. + + `Learn.tsx::tierForScore` classifies a STREAMED mastery delta client-side + so the live Tree matches what a full graph refetch would show. It is a + deliberate cross-language mirror — but a silent one: move a threshold in + config.py and a node landing in the newly-shifted band paints one tier + live and a different tier after the next refetch. That is precisely the + score/label disagreement #557 exists to kill, just across the wire + instead of across two Python modules. + + So the mirror is pinned here rather than trusted to a comment. + """ + import re + from pathlib import Path + + from config import ( + MASTERY_LEARNING_MIN, + MASTERY_MASTERED_MIN, + MASTERY_STRUGGLING_MIN, + ) + + src = ( + Path(__file__).resolve().parents[2] + / "frontend/src/components/screens/Learn.tsx" + ).read_text() + + body = re.search( + r"function tierForScore\(score: number\)[^{]*\{(.*?)\n\}", src, re.S + ) + assert body, "tierForScore moved or was renamed — re-point this guard" + + found = { + tier: float(value) + for value, tier in re.findall( + r'score >= ([0-9.]+)\) return "(\w+)"', body.group(1) + ) + } + assert found == { + "mastered": MASTERY_MASTERED_MIN, + "learning": MASTERY_LEARNING_MIN, + "struggling": MASTERY_STRUGGLING_MIN, + }, ( + "Learn.tsx::tierForScore has drifted from config.py. Update both, or " + "the live Tree will label a streamed delta differently from the " + "refetch that follows it." + ) + + +def test_weak_concepts_are_capped_weakest_first(): + """`_get_weak_concepts` caps at 15. Widening the floor from 0.4 to 0.45 + (#557) admits more rows, so an unsorted cap lets the newly-admitted + [0.4, 0.45) concepts displace 0.0-0.1 ones on arbitrary PostgREST row + order — the surface whose job is drilling the WEAKEST concepts drilling + the least-weak of the weak instead.""" + from unittest.mock import patch as _patch + + from routes import flashcards + + # Deliberately arrives least-weak first, which is what row order can do. + rows = [{"concept_name": f"c{i}", "mastery_score": 0.44 - i * 0.02} for i in range(20)] + with _patch.object(flashcards, "table") as t: + t.return_value.select.return_value = rows + weak = flashcards._get_weak_concepts("u1", "CS101") + + assert len(weak) == 15 + scores = {r["concept_name"]: r["mastery_score"] for r in rows} + assert max(scores[c] for c in weak) < min( + scores[r["concept_name"]] for r in rows if r["concept_name"] not in weak + ), "the 15 returned must be the 15 weakest, not the first 15 rows"