From be9912fa4529fb035e96b42153534abba275e19c Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 04:23:09 -0400 Subject: [PATCH 1/2] fix(mastery): one set of tier thresholds, cited not copied (#557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three sets existed. `config.get_mastery_tier` said mastered >= 0.75 / learning >= 0.45 / struggling >= 0.1; the tutor's progress tool carried its own 0.7 / 0.4; flashcards drilled an ad-hoc < 0.4. So a concept at 0.72 read "learning" on the Tree while the tutor counted it mastered, and one at 0.42 read "struggling" on the Tree while the tutor counted it in-progress AND flashcards refused to drill it — three surfaces disagreeing about the same number in the same session. `config.py` now owns the thresholds as named constants plus two predicates, `is_mastered` / `is_weak`, and the other two sites call them. `is_weak` is "below the learning floor" — struggling OR unexplored — because that union is what every caller is actually asking for (which concepts need work), and splitting it would push the union back out to the call sites, which is where the drift came from. The tutor's local copy carried a comment claiming the duplication was deliberate, so "the agent's definitions can evolve independently". They did not evolve; they drifted. That rationale is replaced with the rule the issue asks for: a genuinely different cut gets a named constant in `config.py`, never a literal at the call site. Behaviour changes, all of them the point of the issue: * tutor `mastered_count` 0.7 -> 0.75 * tutor `weak_count` 0.4 -> 0.45 * flashcards' weak-concept picker 0.4 -> 0.45, so concepts in [0.4, 0.45) — "struggling" on the Tree — are now offered for practice. The surface whose whole job is drilling weak concepts had been skipping a slice. Tests pin the AGREEMENT, not the numbers, so the thresholds stay movable in one place: a sweep across every tier and boundary asserts the tutor's classification matches `get_mastery_tier` for the same score, driven through the real tool rather than its constants. Plus a guard that the local constants are gone, since the failure mode was three copies drifting, not one wrong number. Hermetic 2169 passed / 9 skipped, ruff clean. Co-Authored-By: Claude Opus 5 --- backend/agents/tools/chat_context.py | 26 +++--- backend/config.py | 37 ++++++++- backend/routes/flashcards.py | 12 ++- backend/tests/test_chat_context_tools.py | 21 +++-- .../tests/test_mastery_tier_unification.py | 81 +++++++++++++++++++ 5 files changed, 152 insertions(+), 25 deletions(-) create mode 100644 backend/tests/test_mastery_tier_unification.py 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..3ee8a980 100644 --- a/backend/config.py +++ b/backend/config.py @@ -92,16 +92,47 @@ def validate_config() -> None: ) +# ── Mastery tiers (#557) ──────────────────────────────────────────────────── +# +# THE thresholds. Every surface that classifies a mastery score reads them +# from here — the Tree, 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. +# +# 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..3934946f 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( @@ -192,7 +198,7 @@ def _get_weak_concepts(user_id: str, course_name: str) -> list[str]: weak = [ r["concept_name"] for r in (rows or []) - if (r.get("mastery_score") or 0) < 0.4 + if is_weak(r.get("mastery_score") or 0) ] return weak[:15] # cap to keep prompt reasonable except Exception: 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..1ae20bc3 --- /dev/null +++ b/backend/tests/test_mastery_tier_unification.py @@ -0,0 +1,81 @@ +"""#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"] From 37e4c154a57e73aa2f5f73234394049083b180b9 Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Sat, 22 Aug 2026 04:32:56 -0400 Subject: [PATCH 2/2] fix(mastery): pin the frontend mirror; cap flashcards weakest-first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both about claims this PR made that weren't yet true. **A fourth copy survived, in TypeScript.** `Learn.tsx::tierForScore` re-declares 0.75/0.45/0.1 to classify a STREAMED mastery delta client-side, so the live Tree matches the refetch that follows. It cannot import from `config.py` — but the header I added asserted "every surface reads them from here", and the guard test only checked that two Python attribute names were absent. So moving a threshold would have painted a node one tier live and a different tier on the next refetch: #557's own bug, across the wire instead of across two modules. The mirror is now pinned by a test that reads the TSX and asserts the numbers match, and the header says the mirror exists. **Widening the floor made an unsorted cap unsafe.** `_get_weak_concepts` takes the first 15 qualifying rows in PostgREST order. That was survivable at `< 0.4`; at `< 0.45` the newly-admitted [0.4, 0.45) concepts can displace 0.0-0.1 ones on arbitrary row order, so the surface whose job is drilling the weakest concepts could drill the least-weak of the weak — a regression created by this PR's own widening. Sorted ascending before the cap, which also makes the truncation deterministic. Hermetic 2171 passed / 9 skipped, ruff clean. Co-Authored-By: Claude Opus 5 --- backend/config.py | 18 +++-- backend/routes/flashcards.py | 17 +++-- .../tests/test_mastery_tier_unification.py | 72 +++++++++++++++++++ 3 files changed, 96 insertions(+), 11 deletions(-) diff --git a/backend/config.py b/backend/config.py index 3ee8a980..ab7073a5 100644 --- a/backend/config.py +++ b/backend/config.py @@ -94,11 +94,19 @@ def validate_config() -> None: # ── Mastery tiers (#557) ──────────────────────────────────────────────────── # -# THE thresholds. Every surface that classifies a mastery score reads them -# from here — the Tree, 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. +# 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. diff --git a/backend/routes/flashcards.py b/backend/routes/flashcards.py index 3934946f..15c2cf28 100644 --- a/backend/routes/flashcards.py +++ b/backend/routes/flashcards.py @@ -195,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 is_weak(r.get("mastery_score") or 0) - ] - 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_mastery_tier_unification.py b/backend/tests/test_mastery_tier_unification.py index 1ae20bc3..b6e521e3 100644 --- a/backend/tests/test_mastery_tier_unification.py +++ b/backend/tests/test_mastery_tier_unification.py @@ -79,3 +79,75 @@ def test_flashcards_weak_concepts_use_the_shared_floor(): 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"