diff --git a/backend/agents/chat_tutor.py b/backend/agents/chat_tutor.py index d4161449..6da3be4f 100644 --- a/backend/agents/chat_tutor.py +++ b/backend/agents/chat_tutor.py @@ -32,7 +32,7 @@ read_user_progress_tool, search_course_materials_tool, ) -from agents.tools.graph import apply_graph_update_tool +from agents.tools.graph import apply_graph_update_tool, update_mastery_tool TutorMode = Literal["socratic", "expository", "teachback"] @@ -50,6 +50,12 @@ "fabricate context.\n\n" "Tone: warm, concise, no filler. Use math/code blocks where helpful " "(LaTeX `$x^2$`, ```mermaid```, ```plot```). Don't over-explain.\n\n" + "Knowledge graph tools:\n" + "- apply_graph_update_tool: register NEW concepts the student hasn't seen before.\n" + "- update_mastery_tool: adjust mastery on EXISTING concepts this turn. " + "Use +0.1 to +0.3 when they answer correctly; −0.05 to −0.1 for gaps. " + "Call this at the END of every turn where the student demonstrated " + "understanding or revealed a misconception.\n\n" ) _SOCRATIC_PROMPT = _SHARED_PREAMBLE + ( @@ -104,6 +110,7 @@ read_session_history_tool, read_user_progress_tool, apply_graph_update_tool, + update_mastery_tool, ] diff --git a/backend/agents/deps.py b/backend/agents/deps.py index bd557257..ad57cd5e 100644 --- a/backend/agents/deps.py +++ b/backend/agents/deps.py @@ -7,7 +7,7 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Any @@ -27,6 +27,12 @@ class SaplingDeps: that need to scope reads to *this* conversation (e.g. read_session_history_tool). Optional — agent runs that don't happen inside a session (eval mode, batch tasks) leave it None. + graph_updates: Accumulates graph update payloads emitted by tools + during a run so the route can persist them in graph_update_json + for concepts_covered derivation in end_session. + mastery_changes: Accumulates the real before/after mastery deltas + returned by apply_graph_update so the route can surface them in + the chat response for parity with the legacy path. """ user_id: str @@ -34,3 +40,5 @@ class SaplingDeps: supabase: Any request_id: str session_id: str | None = None + graph_updates: list = field(default_factory=list) + mastery_changes: list = field(default_factory=list) diff --git a/backend/agents/tools/graph.py b/backend/agents/tools/graph.py index dd56fe21..46502c01 100644 --- a/backend/agents/tools/graph.py +++ b/backend/agents/tools/graph.py @@ -1,21 +1,24 @@ -"""Graph-update helpers and a Pydantic AI tool wrapper. +"""Graph-update helpers and Pydantic AI tool wrappers. -The core merge logic lives in `apply_concepts_to_graph` — a plain async -function callable from routes directly. `apply_graph_update_tool` is a -thin Pydantic AI wrapper around it for future agents that need a tool -to register on an `Agent`. Neither contains LLM-specific logic; that -stays in `services.graph_service`. +Two tools are exposed: +- apply_graph_update_tool — registers new concepts (new_nodes, initial_mastery 0.0) +- update_mastery_tool — adjusts mastery on existing concepts (updated_nodes + delta) + +Both append their payload to ctx.deps.graph_updates so the route can +persist graph_update_json on the assistant message, enabling end_session +to derive concepts_covered correctly for agent-path chats. """ from __future__ import annotations import asyncio +from typing import Literal from pydantic import BaseModel, Field from pydantic_ai import RunContext from agents.deps import SaplingDeps -from services.graph_service import apply_graph_update +from services.graph_service import _normalize_concept, apply_graph_update class GraphUpdateInput(BaseModel): @@ -27,6 +30,41 @@ class GraphUpdateInput(BaseModel): ) +class ConceptMasteryUpdate(BaseModel): + concept_name: str = Field( + description="Exact name of the concept whose mastery score to change." + ) + mastery_delta: float = Field( + ge=-1.0, + le=1.0, + description=( + "Fractional mastery change, −1.0 to +1.0. " + "Use +0.1 to +0.3 when the student answers correctly; " + "−0.05 to −0.1 when they reveal a gap or misconception." + ), + ) + reason: str = Field( + default="", + description="Short phrase shown in the mastery-event log (e.g. 'answered correctly').", + ) + event_type: Literal["interaction", "correction", "quiz"] = Field( + default="interaction", + description="Event category for the mastery-event log.", + ) + + +class MasteryUpdateInput(BaseModel): + """Typed input for the update_mastery tool.""" + + updates: list[ConceptMasteryUpdate] = Field( + description=( + "One entry per concept whose mastery changed this turn. " + "Only include concepts that already exist in the graph " + "(or were just added via apply_graph_update_tool)." + ) + ) + + async def apply_concepts_to_graph( user_id: str, course_id: str | None, @@ -58,13 +96,82 @@ async def apply_graph_update_tool( ctx: RunContext[SaplingDeps], update: GraphUpdateInput, ) -> str: - """Pydantic AI tool wrapper around apply_concepts_to_graph. + """Register new concepts in the student's knowledge graph. - Returns a short summary string for the agent to confirm the operation. + Call this when a new topic comes up that isn't already tracked. + To raise or lower mastery on an existing concept, call update_mastery_tool. """ - count = await apply_concepts_to_graph( - ctx.deps.user_id, ctx.deps.course_id, update.concepts, - ) - if count == 0: + new_nodes = [ + {"concept_name": name.strip(), "initial_mastery": 0.0} + for name in update.concepts + if name and name.strip() + ] + if not new_nodes: return "Graph update skipped: no concepts to add." - return f"Graph updated: {count} concept(s) merged." + await asyncio.to_thread( + apply_graph_update, + ctx.deps.user_id, + {"new_nodes": new_nodes}, + ctx.deps.course_id, + ) + ctx.deps.graph_updates.append({"new_nodes": new_nodes}) + return f"Graph updated: {len(new_nodes)} concept(s) merged." + + +async def update_mastery_tool( + ctx: RunContext[SaplingDeps], + update: MasteryUpdateInput, +) -> str: + """Adjust mastery scores for concepts the student engaged with this turn. + + Positive delta (e.g. +0.15) when they demonstrate understanding; + negative (e.g. −0.08) when they reveal a gap. Concepts must already + exist in the graph — call apply_graph_update_tool first if needed. + """ + updated_nodes = [ + { + "concept_name": u.concept_name.strip(), + "mastery_delta": u.mastery_delta, + "reason": u.reason, + "event_type": u.event_type, + } + for u in update.updates + if u.concept_name and u.concept_name.strip() + ] + if not updated_nodes: + return "Mastery update skipped: no concepts provided." + + changes = await asyncio.to_thread( + apply_graph_update, + ctx.deps.user_id, + {"updated_nodes": updated_nodes}, + ctx.deps.course_id, + ) + + # Only persist concepts that actually produced a change. A concept the + # model named but that doesn't exist in the graph yields no `changes` + # and is never written, so it must not leak into graph_update_json (it + # would over-report concepts_covered in end_session). Rebuild the + # appended updated_nodes from the concepts that genuinely changed. + # + # `changes` carries the *stored* concept_name while `updated_nodes` holds + # the *model-provided* spelling; match on the normalized form (the same + # case/whitespace-insensitive key apply_graph_update dedups on) so a + # casing/spacing drift doesn't drop a genuinely-changed concept. + if changes: + changed_names = {_normalize_concept(c["concept"]) for c in changes} + persisted_nodes = [ + n + for n in updated_nodes + if _normalize_concept(n["concept_name"]) in changed_names + ] + if persisted_nodes: + ctx.deps.graph_updates.append({"updated_nodes": persisted_nodes}) + # Surface the real before/after deltas for parity with the legacy path. + ctx.deps.mastery_changes.extend(changes) + parts = [f"{c['concept']} {c['before']:.2f}→{c['after']:.2f}" for c in changes] + return f"Mastery updated: {', '.join(parts)}." + return ( + f"Mastery update processed ({len(updated_nodes)} concept(s)); " + "no score change — concept may not exist yet. Call apply_graph_update_tool first." + ) diff --git a/backend/routes/learn.py b/backend/routes/learn.py index 475bb1c8..ed73307e 100644 --- a/backend/routes/learn.py +++ b/backend/routes/learn.py @@ -11,6 +11,7 @@ from pydantic_ai.exceptions import UsageLimitExceeded, UnexpectedModelBehavior from pydantic_ai.messages import ModelRequest, ModelResponse, TextPart, UserPromptPart +from agents import ORCHESTRATOR_LIMITS from agents.chat_tutor import agent_for_mode from agents.deps import SaplingDeps from db.connection import table @@ -512,10 +513,12 @@ async def _chat_via_agent( """Run chat_tutor_agent and return the legacy response shape. Returns ``{"reply": str, "graph_update": dict, "mastery_changes": list}``. - `graph_update` and `mastery_changes` come back empty here because - `apply_graph_update_tool` (registered on chat_tutor) already - persisted any graph changes during the agent run. The frontend's - Learn-page reducer accepts empty values gracefully. + Graph changes are persisted in-band during the agent run by + `apply_graph_update_tool` / `update_mastery_tool` (registered on + chat_tutor); the tools also accumulate their payloads on `deps` so the + route can echo `graph_update` (for graph_update_json / concepts_covered) + and the real `mastery_changes` deltas back to the client, matching the + legacy path. Both are empty when nothing changed this turn. `use_shared_context=False` flips the model into "no class-aggregate" mode by appending a constraint instruction to the user message — @@ -563,7 +566,11 @@ async def _chat_via_agent( ) model_override = _resolve_model_pref(model_pref) - run_kwargs: dict = {"deps": deps, "message_history": message_history} + run_kwargs: dict = { + "deps": deps, + "message_history": message_history, + "usage_limits": ORCHESTRATOR_LIMITS, + } if model_override is not None: run_kwargs["model"] = model_override @@ -576,10 +583,21 @@ async def _chat_via_agent( result = await agent.run(user_message, **run_kwargs) reply = result.output # str — chat_tutor agents return plain Markdown. + # Merge all graph update payloads accumulated by tools during this run + # into a single dict so the route can persist graph_update_json and + # end_session can derive concepts_covered correctly. + merged_graph_update: dict = {} + for gu in deps.graph_updates: + for key, items in gu.items(): + merged_graph_update.setdefault(key, []).extend(items) + return { "reply": reply, - "graph_update": {}, - "mastery_changes": [], + "graph_update": merged_graph_update, + # Real before/after deltas accumulated by update_mastery_tool, for + # parity with the legacy path (which returns apply_graph_update's + # changes directly). Empty when no mastery moved this turn. + "mastery_changes": deps.mastery_changes, } @@ -690,7 +708,8 @@ async def chat(body: ChatBody, request: Request): # own writes so a fallback doesn't double-insert. Encryption happens # inside save_message (`encrypt_if_present`). save_message(body.session_id, "user", body.message) - save_message(body.session_id, "assistant", response["reply"]) + graph_update = response.get("graph_update") or None + save_message(body.session_id, "assistant", response["reply"], graph_update) return response diff --git a/backend/routes/quiz.py b/backend/routes/quiz.py index a25c3352..e1f30a33 100644 --- a/backend/routes/quiz.py +++ b/backend/routes/quiz.py @@ -8,6 +8,7 @@ from pydantic_ai.exceptions import UsageLimitExceeded, UnexpectedModelBehavior +from agents import ORCHESTRATOR_LIMITS from agents.quiz import quiz_agent, Quiz, QuizQuestion from agents.deps import SaplingDeps from agents._run import run_agent_sync @@ -186,7 +187,7 @@ async def _quiz_via_agent( ) model_override = _resolve_model_pref(model_pref) - run_kwargs: dict = {"deps": deps} + run_kwargs: dict = {"deps": deps, "usage_limits": ORCHESTRATOR_LIMITS} if model_override is not None: run_kwargs["model"] = model_override result = await quiz_agent.run(user_message, **run_kwargs) diff --git a/backend/tests/test_chat_tutor_imports.py b/backend/tests/test_chat_tutor_imports.py index 19e66f49..77fc325e 100644 --- a/backend/tests/test_chat_tutor_imports.py +++ b/backend/tests/test_chat_tutor_imports.py @@ -34,13 +34,14 @@ def test_unknown_mode_falls_back_to_socratic(): assert agent_for_mode(None) is socratic_agent -def test_all_four_tools_registered(): - """Chat tutor needs three context tools + the graph-update tool.""" +def test_all_tools_registered(): + """Chat tutor needs three context tools + two graph tools (add and update mastery).""" expected = { "search_course_materials_tool", "read_session_history_tool", "read_user_progress_tool", "apply_graph_update_tool", + "update_mastery_tool", } # Pydantic AI 1.89's tool registry is at agent._function_toolset.tools # (dict keyed by tool name) — see commit a850d31 for the gotcha. diff --git a/backend/tests/test_graph_tools_bugs.py b/backend/tests/test_graph_tools_bugs.py new file mode 100644 index 00000000..237bae4e --- /dev/null +++ b/backend/tests/test_graph_tools_bugs.py @@ -0,0 +1,517 @@ +""" +Tests for the three regressions fixed in the Pydantic AI graph-tool layer. + +Bug #5 (HIGH) — update_mastery_tool now emits updated_nodes with mastery_delta + so conversational tutoring actually moves mastery scores. +Bug #13 (MEDIUM) — apply_graph_update_tool and update_mastery_tool append to + deps.graph_updates, enabling end_session to derive + concepts_covered for agent-path chats. +Bug #14 (MEDIUM) — ORCHESTRATOR_LIMITS is passed as usage_limits to every + tool-using agent .run() call in learn.py and quiz.py. +""" +from __future__ import annotations + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from agents.deps import SaplingDeps +from agents.tools.graph import ( + ConceptMasteryUpdate, + GraphUpdateInput, + MasteryUpdateInput, + apply_graph_update_tool, + update_mastery_tool, +) + + +def _run(coro): + return asyncio.run(coro) + + +def _make_ctx(user_id="u1", course_id="c1", graph_updates=None): + """Minimal RunContext stand-in: only .deps is read by the tools.""" + deps = SaplingDeps( + user_id=user_id, + course_id=course_id, + supabase=None, + request_id="req-test", + session_id="sess-test", + graph_updates=graph_updates if graph_updates is not None else [], + ) + return SimpleNamespace(deps=deps) + + +# ── Bug #5: update_mastery_tool emits updated_nodes ────────────────────────── + + +class TestUpdateMasteryTool: + def test_calls_apply_graph_update_with_updated_nodes(self): + """The tool must forward updated_nodes — NOT new_nodes — so the + mastery-delta branch in graph_service.apply_graph_update fires.""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ + ConceptMasteryUpdate( + concept_name="Recursion", + mastery_delta=0.15, + reason="answered correctly", + event_type="interaction", + ) + ] + ) + + mock_changes = [{"concept": "Recursion", "before": 0.4, "after": 0.55}] + + with patch( + "agents.tools.graph.apply_graph_update", return_value=mock_changes + ), patch("agents.tools.graph.asyncio.to_thread", new=AsyncMock(return_value=mock_changes)): + result = _run(update_mastery_tool(ctx, update)) + + assert "0.40→0.55" in result or "Recursion" in result + + def test_forwards_mastery_delta_not_initial_mastery(self): + """Critically: the payload sent to graph_service must use + 'mastery_delta', not 'initial_mastery' — that's the regression.""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ + ConceptMasteryUpdate(concept_name="Heaps", mastery_delta=0.2) + ] + ) + captured = {} + + async def fake_to_thread(fn, *args, **kwargs): + captured["args"] = args + return [] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(update_mastery_tool(ctx, update)) + + graph_update_dict = captured["args"][1] + assert "updated_nodes" in graph_update_dict + assert "new_nodes" not in graph_update_dict + node = graph_update_dict["updated_nodes"][0] + assert node["concept_name"] == "Heaps" + assert node["mastery_delta"] == pytest.approx(0.2) + + def test_skips_empty_concept_names(self): + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ + ConceptMasteryUpdate(concept_name=" ", mastery_delta=0.1), + ConceptMasteryUpdate(concept_name="", mastery_delta=0.1), + ] + ) + + async def fake_to_thread(fn, *args, **kwargs): + return [] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread) as mock_tt: + result = _run(update_mastery_tool(ctx, update)) + + mock_tt.assert_not_called() + assert "skipped" in result.lower() + + def test_returns_human_readable_summary_when_changes_present(self): + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="BFS", mastery_delta=0.1)] + ) + mock_changes = [{"concept": "BFS", "before": 0.3, "after": 0.4}] + + async def fake_to_thread(fn, *args, **kwargs): + return mock_changes + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + result = _run(update_mastery_tool(ctx, update)) + + assert "BFS" in result + assert "0.30" in result or "0.3" in result + + def test_returns_fallback_message_when_no_score_change(self): + """Concept not found in graph → changes=[] → informative message.""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="UnknownTopic", mastery_delta=0.2)] + ) + + async def fake_to_thread(fn, *args, **kwargs): + return [] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + result = _run(update_mastery_tool(ctx, update)) + + assert "no score change" in result.lower() + assert "mastery updated:" not in result.lower() + + +# ── Bug #13: tools append to deps.graph_updates ────────────────────────────── + + +class TestGraphUpdatesAccumulation: + def test_apply_graph_update_tool_appends_new_nodes(self): + """After a successful tool call, deps.graph_updates must contain + the new_nodes payload so the route can persist graph_update_json.""" + ctx = _make_ctx() + update = GraphUpdateInput(concepts=["Binary Search", "Merge Sort"]) + + async def fake_to_thread(fn, *args, **kwargs): + return [] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(apply_graph_update_tool(ctx, update)) + + assert len(ctx.deps.graph_updates) == 1 + payload = ctx.deps.graph_updates[0] + assert "new_nodes" in payload + names = [n["concept_name"] for n in payload["new_nodes"]] + assert "Binary Search" in names + assert "Merge Sort" in names + + def test_apply_graph_update_tool_does_not_append_when_empty(self): + """Empty concepts list → no DB call and no accumulation.""" + ctx = _make_ctx() + update = GraphUpdateInput(concepts=["", " "]) + + with patch("agents.tools.graph.asyncio.to_thread") as mock_tt: + _run(apply_graph_update_tool(ctx, update)) + + mock_tt.assert_not_called() + assert ctx.deps.graph_updates == [] + + def test_update_mastery_tool_appends_updated_nodes(self): + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ + ConceptMasteryUpdate(concept_name="DFS", mastery_delta=0.1, reason="correct"), + ConceptMasteryUpdate(concept_name="BFS", mastery_delta=-0.05, reason="gap"), + ] + ) + + async def fake_to_thread(fn, *args, **kwargs): + return [ + {"concept": "DFS", "before": 0.3, "after": 0.4}, + {"concept": "BFS", "before": 0.5, "after": 0.45}, + ] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(update_mastery_tool(ctx, update)) + + assert len(ctx.deps.graph_updates) == 1 + payload = ctx.deps.graph_updates[0] + assert "updated_nodes" in payload + names = [n["concept_name"] for n in payload["updated_nodes"]] + assert "DFS" in names + assert "BFS" in names + + def test_update_mastery_tool_skips_append_when_no_change(self): + """A concept the model named but that doesn't exist in the graph + yields no `changes` and must NOT be appended to graph_updates — + otherwise end_session would over-report it as concepts_covered.""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="GhostTopic", mastery_delta=0.2)] + ) + + async def fake_to_thread(fn, *args, **kwargs): + return [] # concept not in graph → nothing persisted + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(update_mastery_tool(ctx, update)) + + assert ctx.deps.graph_updates == [] + assert ctx.deps.mastery_changes == [] + + def test_update_mastery_tool_only_appends_changed_concepts(self): + """When only some named concepts actually change, graph_updates must + contain only the persisted ones (built from the returned changes).""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ + ConceptMasteryUpdate(concept_name="Real", mastery_delta=0.1), + ConceptMasteryUpdate(concept_name="Ghost", mastery_delta=0.1), + ] + ) + + async def fake_to_thread(fn, *args, **kwargs): + return [{"concept": "Real", "before": 0.2, "after": 0.3}] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(update_mastery_tool(ctx, update)) + + assert len(ctx.deps.graph_updates) == 1 + names = [n["concept_name"] for n in ctx.deps.graph_updates[0]["updated_nodes"]] + assert names == ["Real"] + assert ctx.deps.mastery_changes == [ + {"concept": "Real", "before": 0.2, "after": 0.3} + ] + + def test_update_mastery_tool_persists_despite_casing_drift(self): + """apply_graph_update matches concepts case/whitespace-insensitively and + returns the *stored* name. When the model's spelling drifts from the + stored node ("linear regression" vs "Linear Regression"), the persisted + gate must still recognize the concept as changed (normalized match) and + not drop it from graph_updates — otherwise concepts_covered under-reports.""" + ctx = _make_ctx() + update = MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="linear regression", mastery_delta=0.1)] + ) + + async def fake_to_thread(fn, *args, **kwargs): + # apply_graph_update echoes the canonical stored name. + return [{"concept": "Linear Regression", "before": 0.2, "after": 0.3}] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(update_mastery_tool(ctx, update)) + + assert len(ctx.deps.graph_updates) == 1 + names = [n["concept_name"] for n in ctx.deps.graph_updates[0]["updated_nodes"]] + assert names == ["linear regression"] + + def test_multiple_tool_calls_accumulate_independently(self): + """Two consecutive tool calls (simulating a multi-turn agent run) + must each append their own payload — not overwrite.""" + ctx = _make_ctx() + + async def fake_to_thread(fn, *args, **kwargs): + # Echo a change for any updated_nodes concept so the persisted- + # only gate in update_mastery_tool sees a real change. + payload = args[1] + return [ + {"concept": n["concept_name"], "before": 0.3, "after": 0.5} + for n in payload.get("updated_nodes", []) + ] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(apply_graph_update_tool(ctx, GraphUpdateInput(concepts=["Heaps"]))) + _run(update_mastery_tool(ctx, MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="Heaps", mastery_delta=0.2)] + ))) + + assert len(ctx.deps.graph_updates) == 2 + keys = [list(gu.keys())[0] for gu in ctx.deps.graph_updates] + assert keys == ["new_nodes", "updated_nodes"] + + def test_graph_updates_merge_logic(self): + """Simulate what learn.py does after agent.run(): merge all accumulated + payloads into a single dict and verify both keys survive.""" + ctx = _make_ctx() + + async def fake_to_thread(fn, *args, **kwargs): + payload = args[1] + return [ + {"concept": n["concept_name"], "before": 0.3, "after": 0.4} + for n in payload.get("updated_nodes", []) + ] + + with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): + _run(apply_graph_update_tool(ctx, GraphUpdateInput(concepts=["A", "B"]))) + _run(apply_graph_update_tool(ctx, GraphUpdateInput(concepts=["C"]))) + _run(update_mastery_tool(ctx, MasteryUpdateInput( + updates=[ConceptMasteryUpdate(concept_name="A", mastery_delta=0.1)] + ))) + + # Merge as learn.py does + merged: dict = {} + for gu in ctx.deps.graph_updates: + for key, items in gu.items(): + merged.setdefault(key, []).extend(items) + + new_names = [n["concept_name"] for n in merged["new_nodes"]] + assert set(new_names) == {"A", "B", "C"} + upd_names = [n["concept_name"] for n in merged["updated_nodes"]] + assert upd_names == ["A"] + + +# ── Bug #14: ORCHESTRATOR_LIMITS wired into .run() calls ───────────────────── + + +class TestOrchestratorLimitsWired: + def test_learn_chat_via_agent_passes_usage_limits(self): + """_chat_via_agent must include usage_limits in the kwargs it passes + to agent.run() — not silently omit it.""" + from agents import ORCHESTRATOR_LIMITS + + mock_agent = MagicMock() + run_result = MagicMock() + run_result.output = "Great question!" + mock_agent.run = AsyncMock(return_value=run_result) + + with ( + patch("routes.learn.agent_for_mode", return_value=mock_agent), + patch("routes.learn._resolve_model_pref", return_value=None), + patch("routes.learn._build_pro_model_settings", return_value={}), + ): + from routes.learn import _chat_via_agent + import asyncio as _asyncio + + _asyncio.run( + _chat_via_agent( + user_id="u1", + session_id="s1", + course_id=None, + mode="socratic", + user_message="What is recursion?", + message_history=[], + use_shared_context=True, + request_id="req-1", + model_pref=None, + ) + ) + + call_kwargs = mock_agent.run.call_args.kwargs + assert "usage_limits" in call_kwargs, ( + "usage_limits not passed to agent.run() — ORCHESTRATOR_LIMITS is dead code" + ) + assert call_kwargs["usage_limits"] is ORCHESTRATOR_LIMITS + + def test_quiz_via_agent_passes_usage_limits(self): + """_quiz_via_agent must also pass usage_limits to quiz_agent.run().""" + from agents import ORCHESTRATOR_LIMITS + + mock_quiz_agent = MagicMock() + quiz_question = MagicMock() + quiz_question.question_text = "Q?" + quiz_question.options = ["A", "B", "C", "D"] + quiz_question.correct_answer = "A" + quiz_question.explanation = "Because A." + quiz_result = MagicMock() + quiz_result.output = MagicMock(questions=[quiz_question]) + mock_quiz_agent.run = AsyncMock(return_value=quiz_result) + + import asyncio as _asyncio + + with ( + patch("routes.quiz.quiz_agent", mock_quiz_agent), + patch("routes.quiz._resolve_model_pref", return_value=None), + ): + from routes.quiz import _quiz_via_agent + + try: + _asyncio.run( + _quiz_via_agent( + user_id="u1", + course_id="c1", + concept_node_id="nid1", + concept_name="Recursion", + num_questions=3, + difficulty="medium", + use_shared_context=False, + request_id="req-q", + model_pref=None, + ) + ) + except Exception: + pass # We only care that run() was called with the right kwargs + + assert mock_quiz_agent.run.called, "quiz_agent.run() was never called" + call_kwargs = mock_quiz_agent.run.call_args.kwargs + assert "usage_limits" in call_kwargs, ( + "usage_limits not passed to quiz_agent.run() — ORCHESTRATOR_LIMITS is dead code" + ) + assert call_kwargs["usage_limits"] is ORCHESTRATOR_LIMITS + + def test_agent_path_save_message_receives_graph_update(self): + """After a successful agent run, the assistant message must be saved + with the merged graph_update so graph_update_json is not NULL and + end_session can derive concepts_covered.""" + + saved_calls = [] + + def mock_save_message(session_id, role, content, graph_update=None): + saved_calls.append({"role": role, "graph_update": graph_update}) + + mock_agent = MagicMock() + run_result = MagicMock() + run_result.output = "Here's the answer." + + # Simulate the agent having called apply_graph_update_tool once + def fake_run_side_effect(msg, **kwargs): + deps = kwargs["deps"] + deps.graph_updates.append({"new_nodes": [{"concept_name": "Recursion", "initial_mastery": 0.0}]}) + return run_result + + mock_agent.run = AsyncMock(side_effect=fake_run_side_effect) + + with ( + patch("routes.learn.agent_for_mode", return_value=mock_agent), + patch("routes.learn._get_session_offering_id", return_value=None), + patch("routes.learn._resolve_model_pref", return_value=None), + patch("routes.learn._build_pro_model_settings", return_value={}), + patch("routes.learn.save_message", side_effect=mock_save_message), + patch("routes.learn._consume_pending"), + patch("routes.learn._load_message_history", return_value=[]), + patch("routes.learn.table") as mock_table, + patch("routes.learn.require_self"), + ): + mock_table.return_value.select.return_value = [] + + from main import app + from fastapi.testclient import TestClient + _client = TestClient(app) + + _client.post("/api/learn/chat", json={ + "session_id": "s1", + "user_id": "u1", + "message": "Explain recursion", + "mode": "socratic", + "use_shared_context": True, + "model_pref": None, + }) + + # The assistant save_message call must carry graph_update + assistant_calls = [c for c in saved_calls if c["role"] == "assistant"] + assert assistant_calls, "No assistant message saved" + graph_update = assistant_calls[0]["graph_update"] + assert graph_update is not None, ( + "graph_update was None — concepts_covered will always be empty in end_session" + ) + assert "new_nodes" in graph_update + + +class TestEndSessionConceptsCovered: + def test_concepts_covered_populated_from_graph_update_json(self): + """end_session must return non-empty concepts_covered when messages + have graph_update_json set — this is the fix for bug #13.""" + from routes.learn import end_session + + graph_update_payload = { + "new_nodes": [{"concept_name": "BFS"}], + "updated_nodes": [{"concept_name": "DFS"}], + } + + session_row = {"user_id": "u1", "started_at": "2026-01-01T10:00:00"} + msg_rows = [{"graph_update_json": graph_update_payload}] + + def table_factory(name): + m = MagicMock() + if name == "sessions": + m.select.return_value = [session_row] + m.update.return_value = None + elif name == "messages": + m.select.return_value = msg_rows + else: + m.select.return_value = [] + m.update.return_value = None + return m + + mock_request = MagicMock() + + with ( + patch("routes.learn.table", side_effect=table_factory), + patch("routes.learn.require_self"), + patch("routes.learn.get_session_user_id", return_value="u1"), + patch("routes.learn.encrypt_json", return_value="{}"), + ): + from models import EndSessionBody + body = EndSessionBody(session_id="s1", user_id="u1") + result = end_session(body, mock_request) + + covered = result["summary"]["concepts_covered"] + assert set(covered) == {"BFS", "DFS"}, ( + f"Expected {{'BFS', 'DFS'}}, got {covered!r} — " + "end_session is not reading graph_update_json correctly" + )