diff --git a/backend/agents/_providers.py b/backend/agents/_providers.py index 5eb5d99e..b630ca54 100644 --- a/backend/agents/_providers.py +++ b/backend/agents/_providers.py @@ -8,9 +8,11 @@ SAPLING_MODEL_CONCEPTS=gemini-2.5-flash SAPLING_MODEL_SYLLABUS=gemini-2.5-flash SAPLING_MODEL_QUIZ=gemini-2.5-flash-lite + SAPLING_MODEL_CHAT_TUTOR=gemini-2.5-pro Defaults are tuned per task: cheaper models for simpler classifications, -flagship Flash for tasks where output quality drives downstream UX. +flagship Flash for tasks where output quality drives downstream UX, and +the Pro tier for the conversational tutor where reasoning depth matters. """ from __future__ import annotations @@ -24,7 +26,7 @@ from config import GEMINI_API_KEY -AgentTask = Literal["classifier", "summary", "concepts", "syllabus", "quiz"] +AgentTask = Literal["classifier", "summary", "concepts", "syllabus", "quiz", "chat_tutor"] # Defaults are conservative. Bumping a model up costs more; the env var @@ -38,6 +40,12 @@ # call where the agent pulls structured graph data via tools, so the # bulk of the value is in tool wiring, not raw model strength. "quiz": "gemini-2.5-flash-lite", + # Chat tutor runs on Pro: it streams a multi-turn pedagogical + # conversation where reasoning depth and instruction following drive + # perceived quality. Matches main's tutor default after PR #73 + # (`feat(learn): use gemini-2.5-pro for tutor chat`) and PR #74 + # (`fix(learn): allow thinking on gemini-2.5-pro multiturn calls`). + "chat_tutor": "gemini-2.5-pro", } diff --git a/backend/agents/chat_tutor.py b/backend/agents/chat_tutor.py new file mode 100644 index 00000000..86058dc9 --- /dev/null +++ b/backend/agents/chat_tutor.py @@ -0,0 +1,134 @@ +"""Chat tutor agent for the Learn route's three teaching modes. + +Replaces routes/learn.py:152's build_system_prompt + call_gemini_multiturn +with a typed Pydantic AI agent. Tools handle the data lookups that used +to be string-stuffed: search_course_materials, read_session_history, +read_user_progress, apply_graph_update_tool. + +Modes (Socratic, Expository, TeachBack) are gated by selecting different +system prompts at construction time. The route picks the right agent +instance per request based on body.mode. +""" + +from __future__ import annotations + +import hashlib +from typing import Literal + +from pydantic_ai import Agent + +from agents._providers import model_for +from agents.deps import SaplingDeps +from agents.tools.chat_context import ( + read_session_history_tool, + read_user_progress_tool, + search_course_materials_tool, +) +from agents.tools.graph import apply_graph_update_tool + + +TutorMode = Literal["socratic", "expository", "teachback"] + + +# ── System prompts (one per mode) ────────────────────────────────────────── + +# The shared preamble is identical across modes so a prompt-version bump +# in shared guidance shows up as a hash change for every mode at once. +_SHARED_PREAMBLE = ( + "You are Sapling, an AI tutor that helps a student build mastery in " + "their course material. You have tools to fetch the student's " + "progress, search their uploaded course documents, and update their " + "knowledge graph mastery scores. Use tools when relevant — don't " + "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" +) + +_SOCRATIC_PROMPT = _SHARED_PREAMBLE + ( + "MODE: Socratic. Lead the student to the answer through questions, " + "not lectures. Each turn: ask one focused question that reveals what " + "they already know or where they're confused. Avoid giving the answer " + "directly; provide hints only after they've made an attempt. End " + "every response with a question." +) + +_EXPOSITORY_PROMPT = _SHARED_PREAMBLE + ( + "MODE: Expository. Explain the concept directly and thoroughly. " + "Structure your response: brief overview → detailed explanation → " + "concrete example or worked problem. Don't ask questions back unless " + "the student's prompt is genuinely ambiguous." +) + +_TEACHBACK_PROMPT = _SHARED_PREAMBLE + ( + "MODE: TeachBack. The student is teaching you a concept. Listen to " + "their explanation, then identify what's correct, what's missing, " + "and any specific misconceptions. Praise accuracy where it exists. " + "End with one targeted question that probes the weakest spot in " + "their understanding." +) + +_PROMPTS: dict[TutorMode, str] = { + "socratic": _SOCRATIC_PROMPT, + "expository": _EXPOSITORY_PROMPT, + "teachback": _TEACHBACK_PROMPT, +} + +# Hash of each mode's full prompt (preamble + body), for span versioning. +# Logfire spans on chat-tutor runs include this so a prompt revision +# shows up as a clean delta when comparing run metadata across deploys. +_PROMPT_HASHES: dict[TutorMode, str] = { + mode: hashlib.sha256(prompt.encode("utf-8")).hexdigest()[:12] + for mode, prompt in _PROMPTS.items() +} + + +# ── Agent (one per mode, sharing the same tool surface) ──────────────────── + +# Output type is plain str — chat tutor produces free-form Markdown that +# the frontend renders via MarkdownChat. No structured output here; that +# is reserved for routes that grade or extract. + +# All four tools are registered on every mode. The system prompt steers +# WHEN to call them; the surface stays uniform so a Pro-tier model can +# decide for itself which lookups are worth the round trip. +_TOOLS = [ + search_course_materials_tool, + read_session_history_tool, + read_user_progress_tool, + apply_graph_update_tool, +] + + +def _build_agent(mode: TutorMode) -> Agent[SaplingDeps, str]: + return Agent[SaplingDeps, str]( + model=model_for("chat_tutor"), + deps_type=SaplingDeps, + output_type=str, + system_prompt=_PROMPTS[mode], + metadata={ + "prompt_version": _PROMPT_HASHES[mode], + "agent": "chat_tutor", + "mode": mode, + }, + tools=_TOOLS, + ) + + +socratic_agent = _build_agent("socratic") +expository_agent = _build_agent("expository") +teachback_agent = _build_agent("teachback") + + +def agent_for_mode(mode: str | None) -> Agent[SaplingDeps, str]: + """Return the agent instance for a given mode string. + + Falls back to Socratic if the mode is unrecognized (or missing) — + same default the legacy `build_system_prompt` used when no mode + matched the MODE_PROMPTS dict. + """ + normalized = (mode or "socratic").lower() + return { + "socratic": socratic_agent, + "expository": expository_agent, + "teachback": teachback_agent, + }.get(normalized, socratic_agent) diff --git a/backend/agents/deps.py b/backend/agents/deps.py index 7d1403cf..bd557257 100644 --- a/backend/agents/deps.py +++ b/backend/agents/deps.py @@ -23,9 +23,14 @@ class SaplingDeps: version. request_id: A correlation ID for tracing across a single user-facing request. Used by Logfire spans. + session_id: The active chat session, when applicable. Used by tools + 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. """ user_id: str course_id: str | None supabase: Any request_id: str + session_id: str | None = None diff --git a/backend/agents/tools/chat_context.py b/backend/agents/tools/chat_context.py new file mode 100644 index 00000000..8349b3f3 --- /dev/null +++ b/backend/agents/tools/chat_context.py @@ -0,0 +1,451 @@ +"""Chat-tutor read tools for Pydantic AI agents. + +Per ADR 0001 (adopt Pydantic AI) and refactor #3 (chat tutor), the chat +tutor agent fetches grounding context on demand instead of having every +piece of state stuffed into a single system prompt by +`routes/learn.py::build_system_prompt`. Each function in this module is +one such on-demand fetch: + + - `search_course_materials` — pull document summaries + concept notes + relevant to the student's current question, scored by simple keyword + overlap (no embeddings yet — the corpus per course is small enough + that BM25-lite is fine; revisit when courses cross ~200 docs). + - `read_session_history` — quick lookup of the last N messages in the + current session. The agent already gets full multi-turn via + Pydantic AI's `message_history`, so this exists only for mid-response + self-reference (e.g. "what did the student just say their major was?"). + - `read_user_progress` — aggregated mastery counts for a course so the + agent can decide whether to introduce new material or reinforce + existing weak areas. + +Each tool exposes two surfaces, mirroring the pattern in +`graph_read.py` and `quiz_history.py`: + + - The pure async function — callable from routes/tests, takes ids + explicitly so it can be unit-tested without a `RunContext`. + - The `*_tool` wrapper — registers on a Pydantic AI Agent and pulls + the security-sensitive ids (user_id, course_id, session_id) from + `ctx.deps`. The LLM is only allowed to choose the *query string* / + `last_n`; it can never specify whose data to read. + +Encryption: `documents.summary`, `documents.concept_notes`, and +`messages.content` are encrypted at rest (see CLAUDE.md / encryption.py). +Every read in this file decrypts at the boundary before returning to +the agent, so the tool contract never leaks ciphertext to the LLM. +""" + +from __future__ import annotations + +import asyncio +import logging +import re +from typing import Any, Literal + +from pydantic import BaseModel, Field +from pydantic_ai import RunContext + +from agents.deps import SaplingDeps +from db.connection import table +from services.encryption import decrypt_if_present, decrypt_json + +logger = logging.getLogger(__name__) + + +# Words that show up in nearly every academic question and would otherwise +# dominate the keyword-overlap score. Filtering them keeps short queries +# like "what is recursion?" from matching every document with the word +# "what" in its summary. +_STOPWORDS: frozenset[str] = frozenset( + { + "a", "an", "the", "is", "are", "was", "were", "be", "been", "being", + "of", "in", "on", "at", "to", "for", "with", "by", "from", "as", + "and", "or", "but", "not", "no", "so", "if", "then", "than", "that", + "this", "these", "those", "it", "its", "i", "you", "we", "they", + "he", "she", "him", "her", "them", "us", "do", "does", "did", "done", + "have", "has", "had", "what", "which", "who", "whom", "whose", + "when", "where", "why", "how", "can", "could", "would", "should", + "will", "may", "might", "must", "about", "into", "over", "under", + } +) + +# Token = run of word chars, lowercased. Same shape across query and +# document text so overlap math is symmetric. +_TOKEN_RE = re.compile(r"[A-Za-z0-9_]+") + + +def _tokenize(text: str | None) -> set[str]: + """Lowercase and tokenize, dropping stopwords. Returns a set so + repeated occurrences in the doc do not game the overlap score (we + want to know whether a term is present, not how many times).""" + if not text: + return set() + return { + t.lower() + for t in _TOKEN_RE.findall(text) + if t.lower() not in _STOPWORDS and len(t) > 1 + } + + +# search_course_materials + + +class CourseMaterial(BaseModel): + """One document's worth of grounding context for the chat tutor.""" + + document_id: str + file_name: str + summary: str | None = None + # Each entry is {"name": str, "description": str}. Stored as list[dict] + # rather than a typed nested model so we can pass through whatever + # shape the document agent wrote without forcing migrations on legacy + # rows (some older docs use {"name", "description"}, some add extras). + concept_notes: list[dict] = Field(default_factory=list) + + +def _coerce_concept_notes(value: Any) -> list[dict]: + """Normalize a documents.concept_notes payload into list[dict]. + + The column has been written under several historical shapes: + - list[dict] (current document agent output) + - dict (single concept; older legacy rows) + - None / empty string (extraction failed) + + Anything we cannot recognize collapses to []. We deliberately do not + raise — the agent should still get the doc back (with summary), just + without notes, when concept extraction was incomplete. + """ + if not value: + return [] + if isinstance(value, dict): + return [value] + if isinstance(value, list): + return [n for n in value if isinstance(n, dict)] + return [] + + +def _score_material(query_tokens: set[str], doc: dict) -> int: + """Keyword-overlap score: count of distinct query tokens that appear + in the doc's filename + summary + concept-note text. Filename is + included because a student asking 'the syllabus' should pull the + file literally named 'syllabus.pdf' even if the summary doesn't echo + that word.""" + if not query_tokens: + # No query => everything ties at 0; caller falls back to insertion order. + return 0 + doc_text_parts: list[str] = [doc.get("file_name") or "", doc.get("summary") or ""] + for note in doc.get("concept_notes") or []: + if isinstance(note, dict): + doc_text_parts.append(note.get("name") or "") + doc_text_parts.append(note.get("description") or "") + doc_tokens = _tokenize(" ".join(doc_text_parts)) + return len(query_tokens & doc_tokens) + + +async def search_course_materials( + course_id: str | None, + query: str, + limit: int = 5, +) -> list[CourseMaterial]: + """Return the top `limit` documents for `course_id`, ranked by + keyword overlap with `query`. + + Drops rows that have neither a summary nor concept notes — there's + nothing to ground on, and including them would waste a tool-result + slot on an empty payload. When `course_id` is None we return []; + the chat tutor only grounds on materials inside the active course + (cross-course search would leak other-class context into the chat). + + Failures degrade silently to []. The agent can always answer from + its base knowledge — losing course materials downgrades quality but + shouldn't 500 the chat. + """ + if not course_id: + return [] + + def _fetch() -> list[dict[str, Any]]: + try: + return ( + table("documents").select( + "id,file_name,summary,concept_notes", + filters={"course_id": f"eq.{course_id}"}, + order="created_at.desc", + ) + or [] + ) + except Exception: + logger.exception( + "search_course_materials fetch failed course=%s", + course_id, + ) + return [] + + rows = await asyncio.to_thread(_fetch) + + # Decrypt at the boundary. Both summary and concept_notes are + # encrypted at rest — never hand ciphertext to the LLM. + decrypted: list[dict[str, Any]] = [] + for r in rows: + summary = decrypt_if_present(r.get("summary")) + notes_raw = r.get("concept_notes") + if isinstance(notes_raw, str) and notes_raw: + try: + notes = _coerce_concept_notes(decrypt_json(notes_raw)) + except Exception: + logger.warning( + "search_course_materials: concept_notes decrypt failed doc=%s", + r.get("id"), + ) + notes = [] + else: + notes = _coerce_concept_notes(notes_raw) + + # Drop entries with nothing groundable. A doc with no summary AND + # no notes is effectively a filename — useless to the tutor. + if not summary and not notes: + continue + + decrypted.append( + { + "id": r.get("id") or "", + "file_name": r.get("file_name") or "", + "summary": summary, + "concept_notes": notes, + } + ) + + query_tokens = _tokenize(query) + # Sort descending by score; stable sort preserves recency order + # (already sorted DESC by created_at) for ties — most-recent wins. + decrypted.sort(key=lambda d: _score_material(query_tokens, d), reverse=True) + + capped = decrypted[: max(0, int(limit))] + return [ + CourseMaterial( + document_id=d["id"], + file_name=d["file_name"], + summary=d["summary"], + concept_notes=d["concept_notes"], + ) + for d in capped + if d["id"] + ] + + +async def search_course_materials_tool( + ctx: RunContext[SaplingDeps], + query: str, + limit: int = 5, +) -> list[CourseMaterial]: + """Pydantic AI tool wrapper. + + The LLM supplies `query` (and optionally `limit`); `course_id` is + pulled from `ctx.deps` so the model can't aim a search at another + course's materials. + """ + return await search_course_materials(ctx.deps.course_id, query, limit) + + +# read_session_history + + +class SessionMessage(BaseModel): + """One past chat turn in the current session.""" + + role: Literal["user", "model"] + content: str + created_at: str + + +# Map storage role values to the tool's public role values. The +# messages.role column historically used 'assistant'; the agent-facing +# contract uses 'model' to align with Pydantic AI / Gemini terminology. +_ROLE_MAP: dict[str, Literal["user", "model"]] = { + "user": "user", + "model": "model", + "assistant": "model", + "system": "model", # legacy; collapse into model so it's never lost. +} + + +async def read_session_history( + session_id: str, + last_n: int = 10, +) -> list[SessionMessage]: + """Return up to `last_n` most-recent messages from `session_id`. + + Newest first — the agent typically wants 'what was just said' rather + than the start of the session. Decrypts `content` at the boundary so + the LLM never sees ciphertext, and skips any row whose role doesn't + map to {user, model} or whose content decrypts to empty. + + Failures degrade to []. A history-less response is degraded but not + broken; raising would kill the chat turn entirely. + """ + if not session_id: + return [] + n = max(0, int(last_n)) + if n == 0: + return [] + + def _fetch() -> list[dict[str, Any]]: + try: + return ( + table("messages").select( + "role,content,created_at", + filters={"session_id": f"eq.{session_id}"}, + order="created_at.desc", + limit=n, + ) + or [] + ) + except Exception: + logger.exception( + "read_session_history fetch failed session=%s", + session_id, + ) + return [] + + rows = await asyncio.to_thread(_fetch) + out: list[SessionMessage] = [] + for r in rows: + raw_role = (r.get("role") or "").lower() + role = _ROLE_MAP.get(raw_role) + if role is None: + continue + content = decrypt_if_present(r.get("content")) + if not content: + continue + out.append( + SessionMessage( + role=role, + content=str(content), + created_at=str(r.get("created_at") or ""), + ) + ) + return out + + +async def read_session_history_tool( + ctx: RunContext[SaplingDeps], + last_n: int = 10, +) -> list[SessionMessage]: + """Pydantic AI tool wrapper. + + `session_id` is read off `ctx.deps` rather than accepted from the + LLM — letting the model supply it would let it read other students' + chat history. The LLM supplies only `last_n`. + """ + if not ctx.deps.session_id: + return [] + return await read_session_history(ctx.deps.session_id, last_n) + + +# 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 + + +class CourseProgress(BaseModel): + """The student's overall progress in a course (or globally if no + course is in scope). All counts are non-negative; `avg_mastery` is + 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 + avg_mastery: float = Field(ge=0.0, le=1.0) + + +def _empty_progress() -> CourseProgress: + return CourseProgress( + total_concepts=0, + mastered_count=0, + weak_count=0, + in_progress_count=0, + avg_mastery=0.0, + ) + + +async def read_user_progress( + user_id: str, + course_id: str | None, +) -> CourseProgress: + """Aggregate the user's mastery across `course_id` (or globally if + None). Reads all `graph_nodes` for the user/course filter and bins + them in Python — the per-(user, course) graph is small (low hundreds + of nodes max) so a fetch-and-aggregate is cheaper than a custom RPC. + + Returns zeros on empty graph or fetch error so the agent can still + plan a turn ('I don't see any concepts for this course yet — want + to upload your syllabus?'). + """ + + def _fetch() -> list[dict[str, Any]]: + filters = {"user_id": f"eq.{user_id}"} + if course_id: + filters["course_id"] = f"eq.{course_id}" + try: + return ( + table("graph_nodes").select( + "mastery_score", + filters=filters, + ) + or [] + ) + except Exception: + logger.exception( + "read_user_progress fetch failed user=%s course=%s", + user_id, + course_id, + ) + return [] + + rows = await asyncio.to_thread(_fetch) + if not rows: + return _empty_progress() + + mastered = 0 + weak = 0 + in_progress = 0 + total = 0 + mastery_sum = 0.0 + for r in rows: + try: + m = float(r.get("mastery_score") or 0.0) + except (TypeError, ValueError): + continue + # Defensive clamp — old rows occasionally drift outside [0, 1]. + m = max(0.0, min(1.0, m)) + total += 1 + mastery_sum += m + if m >= _MASTERED_THRESHOLD: + mastered += 1 + elif m < _WEAK_THRESHOLD: + weak += 1 + else: + in_progress += 1 + + if total == 0: + return _empty_progress() + + return CourseProgress( + total_concepts=total, + mastered_count=mastered, + weak_count=weak, + in_progress_count=in_progress, + avg_mastery=round(mastery_sum / total, 4), + ) + + +async def read_user_progress_tool( + ctx: RunContext[SaplingDeps], +) -> CourseProgress: + """Pydantic AI tool wrapper. Reads user_id and course_id from deps.""" + return await read_user_progress(ctx.deps.user_id, ctx.deps.course_id) diff --git a/backend/prompts/refactor-3-chat-tutor/00-orchestrator-overview.md b/backend/prompts/refactor-3-chat-tutor/00-orchestrator-overview.md new file mode 100644 index 00000000..382895f8 --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/00-orchestrator-overview.md @@ -0,0 +1,73 @@ +# Refactor #3 — Chat Tutor: orchestration plan + +This folder contains the prompts for migrating `backend/routes/learn.py`'s +chat tutor onto a Pydantic AI `chat_tutor_agent` per ADR 0001's migration +plan and ADR 0005's prioritization. After this ships, `services/gemini_service.py` +gets deleted (it stays alive only as the quiz fallback today). + +## Sequencing + +Five sub-agents. Run A, B, D in parallel (non-overlapping files); C runs +solo after A+B finish (depends on both); E runs solo after C is on `main` +or when you're ready to wire the frontend. + +``` +Phase 1 (parallel): + Sub-agent A → backend/agents/tools/* (new tools) + Sub-agent B → backend/agents/chat_tutor.py (new agent) + Sub-agent D → backend/tests/evals/chat_tutor.py (eval set) + +Phase 2 (sequential): + Sub-agent C → backend/routes/learn.py + tests/test_learn_routes.py + +Phase 3 (separate PR — optional / can defer): + Sub-agent E → frontend/src/components/screens/Learn.tsx wiring +``` + +## Branch + ADR + +Before dispatching, create a fresh branch: +```bash +git fetch origin && git checkout -b refactor/3-chat-tutor origin/main +``` + +After Phase 2 lands, write `docs/decisions/0014-refactor-3-chat-tutor-shipped.md` +using the template in `06-adr-template.md`. + +## What this refactor delivers + +| Before | After | +|---|---| +| `routes/learn.py::build_system_prompt` builds a single ~2000-char string with course context, recent sessions, graph state, and mode-specific guidance. | A `chat_tutor_agent` with three tools the LLM calls only when needed. | +| `services/gemini_service.py::call_gemini_multiturn` makes the chat call. | `chat_tutor_agent.run_stream_events(...)` — typed events, streaming text, tool calls observable in Logfire. | +| Three modes (Socratic, Expository, TeachBack) handled by branching prompt strings. | Mode-aware system prompt selected per call; agent shape unchanged across modes. | +| Multi-turn history reconstructed by the route from `messages` table on every call. | Same history, but passed to `chat_tutor_agent.run(message_history=...)` — Pydantic AI's typed `ModelMessage` shape. | +| Mastery + concept updates done procedurally after every chat. | Done via `apply_concepts_to_graph_tool` (already exists from refactor #1). | + +## Constraints (apply to every sub-agent) + +- **Wire format unchanged**: the `messages` table shape and the SSE event names the frontend already consumes are the contract. Don't introduce a new event name without updating the frontend in lockstep (Sub-agent E). +- **Encryption boundary preserved**: `messages.content` is encrypted at rest per CLAUDE.md. Use `encrypt_if_present` at insert and `decrypt_if_present` at read, same as today's route. +- **Legacy fallback preserved per ADR 0001**: rename the existing chat function to `_legacy_chat` and keep it callable. The new path falls back to it on `UsageLimitExceeded` / `UnexpectedModelBehavior` / any other exception. Don't delete `services/gemini_service.py` in this refactor — that's a separate small PR after #3 ships. +- **`require_self`** stays. `SaplingDeps.request_id` adopts the middleware ID via `current_request_id()` (same pattern as refactor #1 and #2). +- **`use_shared_context` toggle**: when False, the agent must NOT call `read_misconceptions_for_course` or any other class-aggregate tool. Today's route gates context inclusion at the prompt level; the new agent gates it at tool registration / prompt instruction. +- **`model_pref` toggle**: already on the body via main commit `90ba796`. Mirror PR #71's `_resolve_model_pref` pattern from `routes/quiz.py` — symmetric across both routes. + +## What's already in place from prior refactors + +- `agents/_providers.py::model_for(task)` — add `"chat_tutor"` to the task list. +- `SaplingDeps` — same shape, threads through. +- Tool wrapper pattern — `agents/tools/graph.py` and `agents/tools/graph_read.py` are templates. +- Eval replay infra — `tests/evals/_replay.py::run_with_cassette` is reusable. +- Logfire scrubber + prompt versioning — automatic. +- `services/agent_events.py::SaplingEvent` + `map_to_sapling_event` — reuse for the streaming chat events. +- Frontend SSE consumer — `frontend/src/lib/sse.ts` is generic. + +## Read before dispatching + +- `docs/decisions/0001-adopt-pydantic-ai.md` — fallback contract. +- `docs/decisions/0003-implementation-conventions.md` — small output schemas, per-call usage_limits. +- `docs/decisions/0005-refactor-2-quiz-generation.md` — explicitly defers chat tutor as #3. +- `docs/decisions/0013-refactor-2-quiz-shipped.md` — addenda capture every gotcha that hit during refactor #2; expect similar shape here. +- `backend/routes/learn.py` — current route. `build_system_prompt` is at ~line 152. +- `backend/services/gemini_service.py::call_gemini_multiturn` — what the legacy path uses. diff --git a/backend/prompts/refactor-3-chat-tutor/01-sub-agent-A-tools.md b/backend/prompts/refactor-3-chat-tutor/01-sub-agent-A-tools.md new file mode 100644 index 00000000..ec84fca4 --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/01-sub-agent-A-tools.md @@ -0,0 +1,146 @@ +# Sub-agent A — Build chat-tutor tools + +Build three new Pydantic AI tools the upcoming `chat_tutor_agent` will use. +WRITE the changes, run tests, report back. + +Repo: `/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling` +Branch: `refactor/3-chat-tutor` (already checked out) + +## Why + +Per `docs/decisions/0001-adopt-pydantic-ai.md` and `0005-refactor-2-quiz-generation.md`, +refactor #3 replaces the chat tutor's hand-built system prompt with a typed agent +that uses tools to fetch context on demand. Today's `routes/learn.py::build_system_prompt` +stuffs course context, recent sessions, and graph state into one giant string. Each +piece becomes a tool the agent calls only when it needs that data. + +## Tools to build + +`backend/agents/tools/chat_context.py` (new file). Three pure-async functions +plus thin Pydantic AI tool wrappers: + +### 1. `search_course_materials(course_id, query)` + +Returns relevant document summaries + concept notes for the course, scored by +keyword match against the query. The chat tutor uses this to ground answers in +the user's uploaded materials instead of hallucinating from general knowledge. + +Schema: +```python +class CourseMaterial(BaseModel): + document_id: str + file_name: str + summary: str | None + concept_notes: list[dict] # {name, description} + +async def search_course_materials( + course_id: str | None, query: str, limit: int = 5, +) -> list[CourseMaterial]: + ... +``` + +Implementation: read from `documents` table filtered by `course_id`. Decrypt +`summary` and `concept_notes` (they're encrypted at rest per CLAUDE.md). +Score by simple keyword overlap with `query` (no embeddings yet — keep it +simple). Return top `limit`. + +### 2. `read_session_history(session_id, last_n)` + +Returns the last N messages from the current session so the agent can +self-reference earlier in the conversation without reloading the entire +multi-turn payload every time. (Pydantic AI's `message_history` arg +already covers full multi-turn — this tool exists for cases where the +agent wants a quick lookup mid-response, e.g., "what did the student +just say their major was?") + +Schema: +```python +class SessionMessage(BaseModel): + role: Literal["user", "model"] + content: str + created_at: str + +async def read_session_history( + session_id: str, last_n: int = 10, +) -> list[SessionMessage]: + ... +``` + +Implementation: read from `messages` table filtered by `session_id`, +ordered by `created_at` DESC, limit `last_n`. **Decrypt `content`** before +returning (encryption is at rest, decrypt at the boundary). + +### 3. `read_user_progress(course_id)` + +Returns the student's overall progress in a course: total concepts, mastered, +weak. The agent uses this to decide whether to introduce new material or +reinforce existing. + +Schema: +```python +class CourseProgress(BaseModel): + total_concepts: int + mastered_count: int # mastery >= 0.7 + weak_count: int # mastery < 0.4 + in_progress_count: int # 0.4 <= mastery < 0.7 + avg_mastery: float + +async def read_user_progress( + user_id: str, course_id: str | None, +) -> CourseProgress: + ... +``` + +Implementation: read from `graph_nodes` filtered by user+course, aggregate +in Python (the dataset is small per user-course). Return zeros if no rows. + +## Tool wrapper pattern (for all three) + +Mirror the shape from `backend/agents/tools/graph_read.py`: + +```python +async def search_course_materials_tool( + ctx: RunContext[SaplingDeps], query: str, +) -> list[CourseMaterial]: + """Pydantic AI tool wrapper. Reads course_id from ctx.deps.""" + return await search_course_materials(ctx.deps.course_id, query) +``` + +Tools should accept arguments from the LLM (the `query`) and resolve user/course +context from `ctx.deps` (security boundary — never let the LLM specify user_id). + +## Tests + +`backend/tests/test_chat_context_tools.py` — 6-8 tests covering: +- Decryption boundary (mock `decrypt_if_present` and assert it was called) +- `search_course_materials` returns top-N by score, drops empty entries +- `read_session_history` returns most-recent first, decrypts content +- `read_user_progress` aggregates correctly, handles empty graph (zeros) +- Tool wrapper extracts user_id/course_id from `ctx.deps` + +Use the `MagicMock` factory pattern from `backend/tests/test_quiz_routes.py::_make_table`. + +## Verify +```bash +cd "/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling/backend" +python -m pytest tests/test_chat_context_tools.py -q --no-header +python -c "from agents.tools.chat_context import search_course_materials, read_session_history, read_user_progress; print('OK')" +``` + +## Constraints + +- DO NOT modify `backend/agents/chat_tutor.py` (sub-agent B is creating it). +- DO NOT modify `backend/routes/learn.py` (sub-agent C will). +- DO NOT modify `backend/tests/evals/` (sub-agent D). +- DO NOT commit. No ADRs. +- Encryption: every read from `messages.content`, `documents.summary`, or + `documents.concept_notes` MUST go through `decrypt_if_present` / + `decrypt_json` before returning to the agent. Don't ship plaintext-leak. + +## Report + +- Files created with line counts. +- Test counts (must all pass). +- Whether the search uses keyword overlap (acceptable) or you found a reason + to add embeddings (probably out of scope; flag if so). +- Anything that didn't fit. diff --git a/backend/prompts/refactor-3-chat-tutor/02-sub-agent-B-agent.md b/backend/prompts/refactor-3-chat-tutor/02-sub-agent-B-agent.md new file mode 100644 index 00000000..6dc8fb93 --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/02-sub-agent-B-agent.md @@ -0,0 +1,260 @@ +# Sub-agent B — Build chat_tutor_agent + +Build the new `chat_tutor_agent` that replaces `routes/learn.py::build_system_prompt`'s +single-string approach with a typed Pydantic AI agent. WRITE the changes, +run import tests, report back. + +Repo: `/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling` +Branch: `refactor/3-chat-tutor` + +## Why + +ADR 0005 explicitly named chat tutor as refactor #3 ("most invasive — do +after quiz refactor lands"). That's now. Three teaching modes (Socratic, +Expository, TeachBack) need to keep working. Multi-turn message history +needs to keep working. Mastery-update tool calls need to keep working. + +## What to read first + +- `backend/routes/learn.py:152` — current `build_system_prompt`. Read all of it. + Note what it stuffs into the prompt: course context, recent session + summaries, graph state, mode-specific guidance. Each piece becomes + either (a) a tool call from the agent, (b) a lookup the route does + before passing message history to the agent, or (c) part of the + agent's static system prompt. +- `backend/agents/quiz.py` — pattern to mirror (typed output, tool registration, + `_PROMPT_HASH`, `metadata=`). +- `backend/agents/_providers.py` — add `"chat_tutor"` to `AgentTask` Literal + and `_DEFAULTS` dict. +- `backend/agents/tools/chat_context.py` (sub-agent A's file) — three new tools + to register on this agent. +- `backend/agents/tools/graph.py` — `apply_graph_update_tool` (already exists; + register it on chat_tutor too so the tutor can update mastery directly). +- `backend/services/gemini_service.py::call_gemini_multiturn` — what the legacy + path does today; preserve the same multi-turn semantic. +- Main commits `6f431d6` (`feat(learn): use gemini-2.5-pro for tutor chat`) + and `e146125` (`fix(learn): allow thinking on gemini-2.5-pro multiturn calls`) + — chat tutor on main is on `gemini-2.5-pro`. New agent's task default + should match. + +## What to write + +### 1. Add `chat_tutor` to `agents/_providers.py` + +```python +AgentTask = Literal["classifier", "summary", "concepts", "syllabus", "quiz", "chat_tutor"] + +_DEFAULTS: dict[AgentTask, str] = { + ..., + "chat_tutor": "gemini-2.5-pro", # matches main's tutor default after PR #73 +} +``` + +Override env var: `SAPLING_MODEL_CHAT_TUTOR`. + +### 2. New file: `backend/agents/chat_tutor.py` + +```python +"""Chat tutor agent for the Learn route's three teaching modes. + +Replaces routes/learn.py:152's build_system_prompt + call_gemini_multiturn +with a typed Pydantic AI agent. Tools handle the data lookups that used +to be string-stuffed: search_course_materials, read_session_history, +read_user_progress, apply_graph_update_tool. + +Modes (Socratic, Expository, TeachBack) are gated by selecting different +system prompts at construction time. The route picks the right agent +instance per request based on body.mode. +""" + +from __future__ import annotations + +import hashlib +from typing import Literal + +from pydantic_ai import Agent + +from agents._providers import model_for +from agents.deps import SaplingDeps +from agents.tools.chat_context import ( + search_course_materials_tool, + read_session_history_tool, + read_user_progress_tool, +) +from agents.tools.graph import apply_graph_update_tool + + +TutorMode = Literal["socratic", "expository", "teachback"] + + +# ── System prompts (one per mode) ────────────────────────────────────────── + +_SHARED_PREAMBLE = ( + "You are Sapling, an AI tutor that helps a student build mastery in " + "their course material. You have tools to fetch the student's " + "progress, search their uploaded course documents, and update their " + "knowledge graph mastery scores. Use tools when relevant — don't " + "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" +) + +_SOCRATIC_PROMPT = _SHARED_PREAMBLE + ( + "MODE: Socratic. Lead the student to the answer through questions, " + "not lectures. Each turn: ask one focused question that reveals what " + "they already know or where they're confused. Avoid giving the answer " + "directly; provide hints only after they've made an attempt. End " + "every response with a question." +) + +_EXPOSITORY_PROMPT = _SHARED_PREAMBLE + ( + "MODE: Expository. Explain the concept directly and thoroughly. " + "Structure your response: brief overview → detailed explanation → " + "concrete example or worked problem. Don't ask questions back unless " + "the student's prompt is genuinely ambiguous." +) + +_TEACHBACK_PROMPT = _SHARED_PREAMBLE + ( + "MODE: TeachBack. The student is teaching you a concept. Listen to " + "their explanation, then identify what's correct, what's missing, " + "and any specific misconceptions. Praise accuracy where it exists. " + "End with one targeted question that probes the weakest spot in " + "their understanding." +) + +_PROMPTS: dict[TutorMode, str] = { + "socratic": _SOCRATIC_PROMPT, + "expository": _EXPOSITORY_PROMPT, + "teachback": _TEACHBACK_PROMPT, +} + +# Hash of the SHARED preamble + each mode's body, for span versioning. +_PROMPT_HASHES: dict[TutorMode, str] = { + mode: hashlib.sha256(prompt.encode("utf-8")).hexdigest()[:12] + for mode, prompt in _PROMPTS.items() +} + + +# ── Agent (one per mode, sharing the same tool surface) ──────────────────── + +# Output type is plain str — chat tutor produces free-form Markdown that +# the frontend renders via MarkdownChat. No structured output here; that's +# reserved for routes that grade or extract. + +_TOOLS = [ + search_course_materials_tool, + read_session_history_tool, + read_user_progress_tool, + apply_graph_update_tool, +] + + +def _build_agent(mode: TutorMode) -> Agent: + return Agent[SaplingDeps, str]( + model=model_for("chat_tutor"), + deps_type=SaplingDeps, + output_type=str, + system_prompt=_PROMPTS[mode], + metadata={ + "prompt_version": _PROMPT_HASHES[mode], + "agent": "chat_tutor", + "mode": mode, + }, + tools=_TOOLS, + ) + + +socratic_agent = _build_agent("socratic") +expository_agent = _build_agent("expository") +teachback_agent = _build_agent("teachback") + + +def agent_for_mode(mode: str) -> Agent: + """Return the agent instance for a given mode string. Falls back to + Socratic if the mode is unrecognized — same default the legacy route + used.""" + normalized = (mode or "socratic").lower() + return { + "socratic": socratic_agent, + "expository": expository_agent, + "teachback": teachback_agent, + }.get(normalized, socratic_agent) +``` + +### 3. Smoke test + +`backend/tests/test_chat_tutor_imports.py`: + +```python +"""Import smoke tests for chat_tutor agents. Live-Gemini behavior is +covered by the eval set in tests/evals/chat_tutor.py.""" + +from agents.chat_tutor import ( + socratic_agent, expository_agent, teachback_agent, agent_for_mode, + _PROMPT_HASHES, +) + + +def test_three_mode_agents_exist(): + assert socratic_agent is not None + assert expository_agent is not None + assert teachback_agent is not None + + +def test_each_mode_has_distinct_prompt_hash(): + """Mode prompts differ; their hashes must too.""" + hashes = list(_PROMPT_HASHES.values()) + assert len(set(hashes)) == 3 + + +def test_agent_for_mode_dispatches_correctly(): + assert agent_for_mode("socratic") is socratic_agent + assert agent_for_mode("expository") is expository_agent + assert agent_for_mode("teachback") is teachback_agent + + +def test_unknown_mode_falls_back_to_socratic(): + assert agent_for_mode("nonsense") is socratic_agent + assert agent_for_mode("") is socratic_agent + assert agent_for_mode(None) is socratic_agent # type: ignore[arg-type] + + +def test_all_four_tools_registered(): + """Chat tutor needs three context tools + the graph-update tool.""" + expected = { + "search_course_materials_tool", + "read_session_history_tool", + "read_user_progress_tool", + "apply_graph_update_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. + tool_names = set(socratic_agent._function_toolset.tools.keys()) + assert expected == tool_names +``` + +## Verify +```bash +cd "/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling/backend" +python -m pytest tests/test_chat_tutor_imports.py -q --no-header +``` + +All tests must pass. + +## Constraints + +- DO NOT modify `backend/agents/tools/chat_context.py` (sub-agent A's file — + imports of `*_tool` symbols should just work once A finishes). +- DO NOT modify `backend/routes/learn.py` (sub-agent C will). +- DO NOT modify `backend/tests/evals/chat_tutor.py` (sub-agent D's file). +- DO NOT commit. No ADRs. +- The output type stays `str` — multi-turn chat is text. Don't try to + introduce structured output here. + +## Report + +- Files created/modified with line counts. +- The three `_PROMPT_HASH` values (one per mode). +- Test count + pass/fail. +- Anything that didn't fit (Pydantic AI's `Agent` constructor signature + changed, or the `_function_toolset` attribute moved). diff --git a/backend/prompts/refactor-3-chat-tutor/03-sub-agent-C-route.md b/backend/prompts/refactor-3-chat-tutor/03-sub-agent-C-route.md new file mode 100644 index 00000000..b7066f5b --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/03-sub-agent-C-route.md @@ -0,0 +1,255 @@ +# Sub-agent C — Refactor `routes/learn.py` to use chat_tutor_agent + +Replace the legacy chat path with a typed agent run, preserve the legacy +fallback per ADR 0001, and stream tool-call events into SSE for the +frontend. WRITE the changes, run tests, report back. + +Repo: `/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling` +Branch: `refactor/3-chat-tutor` + +## Context already in place + +- `backend/agents/chat_tutor.py` — exposes `agent_for_mode(mode)` which + returns the right agent instance for the request. Output type is `str`. +- `backend/agents/tools/chat_context.py` — three context tools, all + decryption-aware. +- `backend/agents/tools/graph.py::apply_graph_update_tool` — already + registered on chat_tutor. +- `backend/agents/_providers.py` — `chat_tutor` task slot, default + `gemini-2.5-pro`, override `SAPLING_MODEL_CHAT_TUTOR`. + +## Why + +`routes/learn.py` currently uses `services/gemini_service.py::call_gemini_multiturn` +inside a hand-built system prompt assembled by `build_system_prompt` (around +line 152). The agent path replaces that with `chat_tutor_agent.run_stream_events(...)` +so tool calls show up in Logfire, the streaming UX is observable per phase, +and the prompt is simpler. + +## What to read first + +- `backend/routes/learn.py` — entire file. Pay attention to: + - `start_session`, `chat`, `end_session`, `action`, `mode_switch` — five + routes that touch the tutor. `chat` is the main one. + - `build_system_prompt` (~line 152): inventory what each piece of context + becomes (tool call vs static prompt vs ignored). + - The `messages` table reads/writes: encryption boundary at every read + via `decrypt_if_present` and at every write via `encrypt_if_present`. +- `backend/routes/documents.py::upload_document_sync` — pattern for the + agent-vs-fallback fallback decision (try/except UsageLimitExceeded / + UnexpectedModelBehavior / Exception → legacy). +- `backend/routes/documents.py::upload_document` — pattern for streaming + SSE with `agent.run_stream_events()` and the `map_to_sapling_event` + helper. +- `backend/services/agent_events.py` — `SaplingEvent` shape + event + mapping. The chat tutor's events should reuse `progress` / `result` / + `status` types — frontend already consumes them. +- `backend/services/request_context.py::current_request_id` — for + unifying SaplingDeps.request_id with the middleware ID. +- `backend/tests/test_learn_routes.py` — tests to keep green. +- `backend/services/encryption.py` — `encrypt_if_present`, + `decrypt_if_present`. Use these at the boundary. + +## What to change + +### 1. Imports + +Add to top of `routes/learn.py`: +```python +from pydantic_ai.exceptions import UsageLimitExceeded, UnexpectedModelBehavior + +from agents.chat_tutor import agent_for_mode +from agents.deps import SaplingDeps +from services.request_context import current_request_id +from services.agent_events import SaplingEvent, sapling_event_to_sse, map_to_sapling_event +``` + +### 2. New helper: `_legacy_chat` + +Rename the existing `chat` function body (the part that calls +`call_gemini_multiturn`) into `async def _legacy_chat(body, request) -> dict`. +Don't delete it — ADR 0001's contract preserves it as the fallback target. + +### 3. New helper: `_chat_via_agent` + +```python +async def _chat_via_agent( + *, + user_id: str, + session_id: str, + course_id: str | None, + mode: str, + user_message: str, + message_history: list, # Pydantic AI ModelMessage shape + use_shared_context: bool, + request_id: str, + model_pref: str | None = None, +) -> dict: + """Run chat_tutor_agent and return the same response shape the + legacy path produced (so the route persistence code is unchanged). + + Returns: {"reply": str, "graph_update": dict, "mastery_changes": list} + """ + agent = agent_for_mode(mode) + deps = SaplingDeps( + user_id=user_id, course_id=course_id, + supabase=None, request_id=request_id, + ) + model_override = _resolve_model_pref(model_pref) + run_kwargs = {"deps": deps, "message_history": message_history} + if model_override is not None: + run_kwargs["model"] = model_override + if use_shared_context is False: + # When opted-out, instruct the agent inline not to call class-level + # tools. (The tool is registered, but the prompt stops the LLM + # from invoking it.) + user_message += ( + "\n\n[Constraint: do not call read_misconceptions_for_course " + "or any class-aggregate tool — student opted out of shared context.]" + ) + + result = await agent.run(user_message, **run_kwargs) + reply = result.output # str + + # graph_update / mastery_changes used to be parsed out of the legacy + # JSON. With the agent path, the apply_graph_update_tool already + # handled persistence directly — return empty dicts here so the + # frontend's existing reducer doesn't break, but the data is already + # in the DB. + return { + "reply": reply, + "graph_update": {}, + "mastery_changes": [], + } +``` + +Borrow `_resolve_model_pref` from `routes/quiz.py` — same shape, same +`_PREF_MODEL_NAMES` mapping. Either import it from `routes.quiz` or +duplicate the helper in `routes/learn.py` (small enough to duplicate, +but if you want a third callsite later you can extract to a +`services/model_pref.py`). + +### 4. Refactor `chat` route + +```python +@router.post("/chat") +async def chat(body: ChatBody, request: Request): + require_self(body.user_id, request) + + request_id = ( + getattr(request.state, "request_id", None) + or current_request_id() + or str(uuid.uuid4()) + ) + + # Load message history (decrypt at the boundary). + history = _load_message_history(body.session_id, decrypt=True) + + try: + response = await _chat_via_agent( + user_id=body.user_id, + session_id=body.session_id, + course_id=_get_session_course_id(body.session_id), + mode=body.mode, + user_message=body.message, + message_history=history, + use_shared_context=body.use_shared_context, + request_id=request_id, + model_pref=body.model_pref, + ) + except (UsageLimitExceeded, UnexpectedModelBehavior) as e: + logger.warning( + "Chat agent guardrails tripped; falling back to legacy", + exc_info=e, + ) + response = await _legacy_chat(body, request) + except HTTPException: + raise # legitimate 4xx/5xx — don't swallow into legacy + except Exception: + logger.exception("Unexpected chat-agent failure; falling back to legacy") + response = await _legacy_chat(body, request) + + # Persist user + model messages (encrypt at the boundary). + _save_message(body.session_id, role="user", + content=body.message, request_id=request_id) + _save_message(body.session_id, role="model", + content=response["reply"], request_id=request_id) + + return response +``` + +### 5. Helpers for message history (encryption boundary) + +```python +def _load_message_history(session_id: str, decrypt: bool = True) -> list: + """Return the session's messages as Pydantic AI ModelMessage objects. + Decrypts content at the boundary. Returns [] if session is new.""" + ... + +def _save_message(session_id: str, role: str, content: str, request_id: str) -> None: + """Persist a chat message; encrypt content at the insert boundary.""" + ... +``` + +`_load_message_history` should convert from the `messages` table shape +into Pydantic AI's `ModelMessage` (search Pydantic AI docs for the +exact constructors — likely `ModelRequest` / `ModelResponse` with +`UserPromptPart` / `TextPart`). If you're unsure, fall back to passing +a simpler shape and let the agent handle it; or convert via Pydantic +AI's helpers. + +### 6. Tests + +Update `backend/tests/test_learn_routes.py`: + +- **Add an autouse fixture on the existing test class that forces the + agent to fail**, so existing tests still exercise the legacy path + (mirrors PR #71's `_force_legacy_pipeline` pattern). +- **Add new `TestChatViaAgent` class** with at least: + - `test_returns_agent_reply` — mock `agent.run` to return a known + string, assert the response dict shape. + - `test_falls_back_to_legacy_on_usage_limit` — mock + `agent.run` to raise `UsageLimitExceeded`, assert legacy fired. + - `test_falls_back_to_legacy_on_unexpected_exception` — bare Exception. + - `test_message_history_loaded_with_decryption` — assert + `decrypt_if_present` was called for each historical message. + - `test_user_and_model_messages_persisted_with_encryption` — assert + `encrypt_if_present` was called for the new messages on insert. + - `test_smart_pref_overrides_agent_model` — body `model_pref="smart"` + → `agent.run` gets `model=GoogleModel("gemini-2.5-pro")`. + +### 7. Same pattern for `start_session` and `action` + +Both call into the legacy chat assembly. Migrate them the same way as +`chat` — agent first, legacy fallback on errors. `end_session` and +`mode_switch` don't generate text; they don't need migration. + +## Verify +```bash +cd "/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling/backend" +python -m pytest tests/test_learn_routes.py -q --no-header +python -m pytest tests/ -q --no-header --ignore=tests/evals +``` + +All previously-passing tests must still pass. Add at least 6 new tests. + +## Constraints + +- DO NOT modify `backend/agents/chat_tutor.py`, `backend/agents/tools/chat_context.py`, + or `backend/agents/_providers.py` (sub-agents A and B's files). +- DO NOT delete the legacy code path. ADR 0001 contract. +- DO NOT change the response wire format. Frontend `Learn.tsx` reads + `reply`, `graph_update`, `mastery_changes` from the response. +- DO NOT change the `messages` table schema. +- DO NOT commit. No ADRs. + +## Report + +- Files changed with line counts. +- New test count + total pass/fail summary. +- Whether `_resolve_model_pref` was imported from quiz or duplicated locally. +- Whether you streamed via `run_stream_events` (recommended for chat) or + used non-streaming `run` for now (acceptable interim — frontend can be + wired in sub-agent E). +- Anything that didn't fit (e.g. Pydantic AI's message_history shape + required a custom adapter). diff --git a/backend/prompts/refactor-3-chat-tutor/04-sub-agent-D-evals.md b/backend/prompts/refactor-3-chat-tutor/04-sub-agent-D-evals.md new file mode 100644 index 00000000..157169cf --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/04-sub-agent-D-evals.md @@ -0,0 +1,326 @@ +# Sub-agent D — Build chat_tutor eval set + +Build a `pydantic-evals` dataset for the new `chat_tutor_agent`. WRITE the +file, verify it imports, report back. Do NOT run against live Gemini — +cassettes get recorded later. + +Repo: `/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling` +Branch: `refactor/3-chat-tutor` + +## Why + +Refactor #3 needs eval coverage equivalent to refactor #1 (document +classification, 25 cases) and refactor #2 (quiz generation, 8 cases). For +chat tutor, expected scope is **15 cases × 3 modes** with mode-specific +evaluators — though the mode dimension means we can keep the case count +sensible (5 cases × 3 modes = 15). + +## What to read first + +- `backend/tests/evals/quiz_generation.py` — closest pattern. Mirror its + shape (cases, evaluators, `run_with_cassette` adapter, `cli_main`). +- `backend/tests/evals/_replay.py` — replay/record/live driver. Use as-is. +- `backend/agents/chat_tutor.py` — when sub-agent B finishes, you'll + import `agent_for_mode` and the three mode-specific agents from here. + +## What to write + +### `backend/tests/evals/chat_tutor.py` + +15 cases — 5 per mode. Each case input is a tuple of (mode, user_message). +The adapter dispatches to the right mode's agent. + +```python +"""pydantic-evals cases for chat_tutor_agent (Socratic / Expository / TeachBack).""" + +from __future__ import annotations + +import sys +from dataclasses import dataclass +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) +sys.path.insert(0, str(Path(__file__).parent)) + +from pydantic_evals import Case, Dataset +from pydantic_evals.evaluators import Evaluator, EvaluatorContext + +from agents.chat_tutor import agent_for_mode +from _replay import run_with_cassette, cli_main + + +# Input tuple: (mode, user_message). Output is the agent's str reply. +ChatInput = tuple[str, str] + + +# ── Evaluators ────────────────────────────────────────────────────────────── + +@dataclass +class NonEmptyEvaluator(Evaluator[ChatInput, str]): + """Reply must be at least 20 chars. Empty/near-empty replies fail.""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, str]) -> float: + return 1.0 if len((ctx.output or "").strip()) >= 20 else 0.0 + + +@dataclass +class SocraticEndsWithQuestionEvaluator(Evaluator[ChatInput, str]): + """Socratic mode prompt requires every reply to end with a question.""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, str]) -> float: + mode = ctx.inputs[0] + if mode != "socratic": + return 1.0 + reply = (ctx.output or "").rstrip() + return 1.0 if reply.endswith("?") else 0.0 + + +@dataclass +class ExpositoryHasStructureEvaluator(Evaluator[ChatInput, str]): + """Expository replies should be substantive — at least 200 chars + AND not end with a question (the prompt says don't ask questions + back unless the user's prompt was ambiguous).""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, str]) -> float: + mode = ctx.inputs[0] + if mode != "expository": + return 1.0 + reply = (ctx.output or "").strip() + long_enough = len(reply) >= 200 + not_question_loop = not reply.rstrip().endswith("?") + return 1.0 if (long_enough and not_question_loop) else 0.0 + + +@dataclass +class TeachBackProbesEvaluator(Evaluator[ChatInput, str]): + """TeachBack mode should both validate parts of the student's + explanation AND end with a probing question. We check the + end-with-? half here (the validate half is qualitative and judged + in the live record session).""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, str]) -> float: + mode = ctx.inputs[0] + if mode != "teachback": + return 1.0 + reply = (ctx.output or "").rstrip() + return 1.0 if reply.endswith("?") else 0.0 + + +@dataclass +class NoToolMisuseEvaluator(Evaluator[ChatInput, str]): + """The agent should NOT mention raw tool names in its reply + (`read_user_progress`, `apply_graph_update`, etc.). Tools are + invisible to the student.""" + + BANNED_SUBSTRINGS = ( + "read_user_progress", "search_course_materials", + "read_session_history", "apply_graph_update_tool", + "apply_concepts_to_graph", "function_tool", + ) + + def evaluate(self, ctx: EvaluatorContext[ChatInput, str]) -> float: + reply = (ctx.output or "").lower() + for banned in self.BANNED_SUBSTRINGS: + if banned.lower() in reply: + return 0.0 + return 1.0 + + +# ── Cases (5 per mode = 15 total) ─────────────────────────────────────────── + +CASES: list[Case[ChatInput, str]] = [ + # ── SOCRATIC ──────────────────────────────────────────────────────────── + Case( + name="socratic_intro_calculus", + inputs=("socratic", + "I keep getting derivatives mixed up with integrals. Can you help?"), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_python_recursion", + inputs=("socratic", + "I don't get how recursion works in Python."), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_chemistry_balancing", + inputs=("socratic", + "How do you balance a redox equation?"), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_history_themes", + inputs=("socratic", + "Why did the Roman Empire fall?"), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_open_followup", + inputs=("socratic", + "I think I get it now — derivatives are just slope."), + metadata={"mode": "socratic"}, + ), + + # ── EXPOSITORY ────────────────────────────────────────────────────────── + Case( + name="expository_explain_big_o", + inputs=("expository", + "Explain Big-O notation."), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_photosynthesis", + inputs=("expository", + "Explain photosynthesis at the cellular level."), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_dependency_injection", + inputs=("expository", + "What is dependency injection?"), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_supply_demand", + inputs=("expository", + "Explain how supply and demand determine price."), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_kantian_ethics", + inputs=("expository", + "What is Kantian ethics?"), + metadata={"mode": "expository"}, + ), + + # ── TEACHBACK ────────────────────────────────────────────────────────── + Case( + name="teachback_correct_concept", + inputs=("teachback", + "Let me explain mitosis: a cell duplicates its DNA, then " + "splits into two identical daughter cells. Each one has the " + "same chromosomes as the original."), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_partial_correct", + inputs=("teachback", + "OK so a closure is a function that has variables. Like, " + "you can use them inside it."), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_misconception", + inputs=("teachback", + "Newton's first law says objects in motion stay in motion " + "unless you push them. Force makes things keep moving."), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_advanced", + inputs=("teachback", + "The Pumping Lemma proves a language isn't regular by " + "showing that strings of length p can be split such that " + "the middle can be repeated and stay in the language."), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_minimal", + inputs=("teachback", + "Recursion is when a function calls itself."), + metadata={"mode": "teachback"}, + ), +] + +assert len(CASES) == 15, f"Expected 15 cases (5 per mode × 3 modes), got {len(CASES)}" + + +# ── Adapter ───────────────────────────────────────────────────────────────── + +# The agent's input is a string (the user message); the adapter picks the +# right mode-specific agent based on the case input tuple. Cassette key is +# the case name, NOT the input — we want the cassette to capture per-mode +# behavior even when two modes share the same user message. + +async def _run(case_input: ChatInput) -> str: + mode, user_message = case_input + case_name = next(c.name for c in CASES if c.inputs == case_input) + agent = agent_for_mode(mode) + + return await run_with_cassette( + dataset="chat_tutor", + case_name=case_name, + agent=agent, + case_input=user_message, # what the agent sees + output_model=str, # plain str output for chat + ) + + +def make_dataset() -> Dataset[ChatInput, str]: + return Dataset( + name="chat_tutor", + cases=CASES, + evaluators=[ + NonEmptyEvaluator(), + SocraticEndsWithQuestionEvaluator(), + ExpositoryHasStructureEvaluator(), + TeachBackProbesEvaluator(), + NoToolMisuseEvaluator(), + ], + ) + + +if __name__ == "__main__": + cli_main(make_dataset, _run) +``` + +NOTE: `run_with_cassette` accepts an `output_model` (Pydantic model class) +to hydrate replayed JSON back into a typed object. For str output we'd +need the helper to handle the no-model case. Read `tests/evals/_replay.py` +first — if `output_model=str` doesn't work, either: +- Wrap chat replies in `class ChatReply(BaseModel): text: str` for the eval + layer only (slight indirection, but lets cassettes round-trip cleanly), +- Or update `_replay.run_with_cassette` to short-circuit on `output_model=str`. + +Pick whichever fits the existing helper's contract. + +## Verify + +After writing the file: +```bash +cd "/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling/backend" +python -c "import ast; ast.parse(open('tests/evals/chat_tutor.py').read()); print('parses OK')" +grep -c '^ Case(' tests/evals/chat_tutor.py +``` + +Once sub-agent B finishes: +```bash +python -c " +import sys, importlib.util +sys.path.insert(0, '.') +sys.path.insert(0, 'tests/evals') +spec = importlib.util.spec_from_file_location('chat_eval', 'tests/evals/chat_tutor.py') +m = importlib.util.module_from_spec(spec); spec.loader.exec_module(m) +print(f'cases: {len(m.make_dataset().cases)}') +" +``` + +DO NOT actually run against live Gemini — replay infra exists; cassettes +get recorded by the user once the agent path is on main. + +## Constraints + +- DO NOT modify `backend/agents/chat_tutor.py` (sub-agent B's file). +- DO NOT modify `backend/agents/tools/chat_context.py` (sub-agent A). +- DO NOT modify `backend/routes/learn.py`. +- DO NOT commit. No ADRs. + +## Report + +- File created with line count. +- Case count (must be exactly 15). +- Per-mode breakdown (5 socratic / 5 expository / 5 teachback — assert + this in the file too). +- Whether `output_model=str` works in `run_with_cassette` or you needed + a wrapper. +- All 5 evaluators wired into `make_dataset()`. diff --git a/backend/prompts/refactor-3-chat-tutor/05-sub-agent-E-frontend.md b/backend/prompts/refactor-3-chat-tutor/05-sub-agent-E-frontend.md new file mode 100644 index 00000000..9d4dd1af --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/05-sub-agent-E-frontend.md @@ -0,0 +1,140 @@ +# Sub-agent E — Wire chat_tutor SSE events into the Learn screen (optional) + +Optional follow-up — can ship as a separate PR after sub-agents A-D land +on main. The backend route in sub-agent C may already work end-to-end with +the existing frontend (chat reply streams in normally because the agent's +output type is `str`). This sub-agent adds visibility for the new tool-call +events (so the user sees "Looking up your course materials..." while the +agent is mid-flight), mirroring what document upload already does. + +Repo: `/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling` +Branch: typically `refactor/3-chat-tutor-frontend` (separate PR) + +## Why + +Without this, the chat tutor's tool calls (`search_course_materials`, +`read_user_progress`, `apply_graph_update`) are silent — the user sees +the same "AI is thinking..." spinner whether the agent's mid-tool-call +or mid-token-generation. With this, each tool call surfaces as a +discrete progress label, same way `DocumentUploadModal.tsx` shows +"Classifying..." → "Extracting..." → "Saved.". + +## What to read first + +- `frontend/src/components/screens/Learn.tsx` — current Learn screen. + Find the `sendChat` call site and the spinner/progress UX. +- `frontend/src/lib/sse.ts` — `streamSSE` is generic and reusable as-is. +- `frontend/src/lib/api.ts::sendChat` — currently a JSON POST. Add a new + `sendChatStream(formData|body, onEvent, signal)` mirroring + `uploadDocumentStream` from PR #67. +- `frontend/src/components/DocumentUploadModal.tsx` — pattern for + `onEvent` callbacks updating live progress labels. +- Backend's emitted event names (defined by sub-agent C in + `backend/services/agent_events.py::map_to_sapling_event`): + `progress:tool_start`, `progress:tool_done`, `progress:streaming`, + `result:reply`, `error:fallback`, `error:failed`. + +## What to write + +### 1. New API helper: `sendChatStream` + +In `frontend/src/lib/api.ts`, add: + +```ts +export type ChatEvent = + | { type: "progress"; step: "tool_start" | "tool_done" | "streaming"; + message: string; data?: { tool?: string; tokens?: number } } + | { type: "result"; step: "reply"; data: { reply: string; ... } } + | { type: "error"; step: "fallback" | "failed"; message: string }; + +export async function sendChatStream( + body: { session_id: string; user_id: string; message: string; + mode: string; use_shared_context?: boolean; + model_pref?: "fast" | "smart" | null }, + onEvent: (event: ChatEvent) => void, + signal?: AbortSignal, +): Promise<{ reply: string; ... }> { + // Mirror uploadDocumentStream's shape: streamSSE async generator, + // collect final result, return when stream closes. + ... +} +``` + +Don't remove the existing `sendChat` (legacy callers may still want +non-streaming JSON). Add the new one alongside it; the chat UI opts in. + +### 2. Update Learn.tsx + +In the `Learn` component's chat-send handler, replace `sendChat(...)` +with `sendChatStream(...)` and add an `onEvent` callback that updates +the existing typing-indicator state. + +Recommended UX: +- `tool_start` event → swap "Sapling is typing..." for the tool's + human-readable label ("Looking up your course materials...", + "Checking your progress...", "Updating your knowledge graph..."). +- `tool_done` event → revert to the typing indicator. +- `streaming` event → keep the typing indicator visible; optionally + pre-render partial tokens if Pydantic AI streams them through. +- `result:reply` → swap the indicator for the actual rendered Markdown. +- `error:fallback` → toast.warn "Switching to legacy tutor..." + (degraded, not failed). +- `error:failed` → toast.error "Could not get a response. Please try + again." Keep the user's message in the input so they can retry. + +### 3. Tool-name → human-label mapping + +```ts +const TOOL_LABELS: Record = { + search_course_materials_tool: "Looking up your course materials...", + read_user_progress_tool: "Checking your progress...", + read_session_history_tool: "Reading earlier in this conversation...", + apply_graph_update_tool: "Updating your knowledge graph...", +}; +``` + +Live this near the `Learn.tsx` chat handler, not in `api.ts` — it's UX +copy, not API shape. + +### 4. Tests + +Add component tests to `frontend/src/components/screens/Learn.test.tsx` +(or wherever the screen tests live; create the file if needed): + +- `sendChatStream` is called with the correct body fields when the user + hits send. +- An incoming `tool_start` event with `data.tool="search_course_materials_tool"` + surfaces the matching human-readable label. +- An incoming `error:failed` event triggers a toast and keeps the user's + input. +- `sendChatStream`'s fetch is called with `credentials: 'include'` (auth + cookie crosses subdomain — mirrors the contract test from PR #67). + +Mirror the test patterns from `frontend/src/components/DocumentUploadModal.test.tsx` +and `frontend/src/lib/api.test.ts`. + +## Verify +```bash +cd "/Users/josegaelcruzlopez/Documents/Startup_Projects /Sapling/frontend" +npm run typecheck +npm test +``` + +Both must pass. Add at least 3 new component/lib tests. + +## Constraints + +- DO NOT modify `frontend/src/lib/sse.ts` (the SSE helper is generic and + reused — don't fork it). +- DO NOT remove `sendChat`. Some callers (e.g. session-replay flows) may + still want the non-streaming JSON shape. The streaming variant is opt-in. +- DO NOT change the `messages` table shape or the backend's wire contract. +- DO NOT commit. No ADRs. + +## Report + +- Files changed/added with line counts. +- Whether streaming actually shows a difference in UX (you'll only be + able to test the wire shape; live token-streaming behavior depends on + Gemini and the route's `run_stream_events`). +- Test count + pass/fail. diff --git a/backend/prompts/refactor-3-chat-tutor/06-adr-template.md b/backend/prompts/refactor-3-chat-tutor/06-adr-template.md new file mode 100644 index 00000000..a30b065b --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/06-adr-template.md @@ -0,0 +1,103 @@ +# 0014 — Template for ADR after refactor #3 ships + +After sub-agents A-D land on `main`, write `docs/decisions/0014-refactor-3-chat-tutor-shipped.md` +using this skeleton. Mirror the shape of `docs/decisions/0013-refactor-2-quiz-shipped.md` +(what shipped, what surprised us, consequences, what to carry forward). + +```markdown +# 0014: Refactor #3 (chat_tutor) shipped + +- Status: accepted +- Date: +- Supersedes: refines 0001 (the migration plan) + +## Context + +ADR 0001 picked Pydantic AI as the agent framework. ADR 0005 named +chat tutor as refactor #3, deferred behind quiz to learn the streaming +machinery on a smaller surface first. With quiz on main and the eval +infra proven, this ADR captures what shipped for chat tutor. + +## Decision + +`chat_tutor_agent` lives at `backend/agents/chat_tutor.py` with three +mode-specific instances (Socratic, Expository, TeachBack), all sharing +the same tool surface (`search_course_materials`, `read_session_history`, +`read_user_progress`, `apply_graph_update_tool`). Output type is `str` +(plain Markdown reply). `routes/learn.py` dispatches to the right mode +via `agent_for_mode(body.mode)`, with the same orchestrator-vs-legacy +fallback pattern PR #67 and #71 established. + +Prompt versions: . + +Per-task model defaults to `gemini-2.5-pro` (matching main's chat +behavior post-PR #73). Override via `SAPLING_MODEL_CHAT_TUTOR`. Body's +`model_pref="fast"|"smart"` field overrides per call (already on the +chat body since PR #73; the agent path now honors it). + +## What shipped + +- `backend/agents/chat_tutor.py` — three Agent[SaplingDeps, str] instances. +- `backend/agents/tools/chat_context.py` — three new context tools. +- `backend/agents/_providers.py` — added `chat_tutor` task slot. +- `backend/routes/learn.py` — `_chat_via_agent` (new) and `_legacy_chat` + (preserved). `chat`, `start_session`, `action` migrated. +- `backend/tests/evals/chat_tutor.py` — 15 cases (5 per mode), 5 evaluators. +- `backend/tests/test_chat_tutor_imports.py` — agent import smokes. +- `backend/tests/test_chat_context_tools.py` — tool unit tests. +- `backend/tests/test_learn_routes.py` — agent-success + agent-fallback + tests added; existing tests kept by forcing legacy via autouse fixture. + +## What surprised us + + +- Pydantic AI's `message_history` shape needed an adapter from the + `messages` table. +- Streaming via `run_stream_events` produced X events the frontend + doesn't render today; we either map them to existing SSE event types + or document the gap. +- `gemini-2.5-pro`'s thinking config had to be re-enabled per + message (commit `e146125` already did this for the legacy path; the + agent path needs the equivalent in the model config). +- Encryption boundary on `messages.content` — every load decrypts; + every save encrypts; agent never sees ciphertext. + +## Consequences + +- (+) Tutor's data lookups (course materials, progress, history) are + observable in Logfire — replaces `build_system_prompt`'s opaque + string augmentation. +- (+) Per-mode prompt versioning (three distinct hashes) lets us A/B + prompt changes per mode independently. +- (+) Model selection is symmetric with quiz route — same `model_pref` + body field, same `_resolve_model_pref` helper, same SAPLING_MODEL_* + env-var override. +- (+) After this PR, `services/gemini_service.py::call_gemini_multiturn` + is dead code. The only remaining caller is the quiz fallback (which + is itself dead code on the happy path). A follow-up PR can delete + `services/gemini_service.py` per ADR 0001's migration plan. +- (−) Three agent instances at module load instead of one. Memory cost + is negligible (Pydantic AI agents are lightweight); just noting. +- (−) Streaming + tool calls increase round-trips vs the old single + multi-turn call. Latency may go up; will measure in Logfire after + ~50 chats. +- (−) Eval cassettes (15) need recording before the workflow leaves + workflow_dispatch-only mode. + +## What I'd carry into the next refactor + + +- Encryption-aware `_load_message_history` deserves a shared helper + (probably belongs in `services/messages.py`). +- The mode-specific agent instances pattern (build three at module + load, dispatch via `agent_for_mode`) is reusable for any route with + modal behavior — tutor today, future quiz-style routes tomorrow. +- The `_resolve_model_pref` helper is now duplicated across quiz and + learn — extract to `services/model_pref.py` if a third caller appears. + +## Pre-existing test failures (not caused by this refactor) + + +- `test_skips_self_edges` (live Supabase 409 in graph_service) +- `test_save_to_db`, `test_full_pipeline` (live Supabase in OCR pipeline) +``` diff --git a/backend/prompts/refactor-3-chat-tutor/README.md b/backend/prompts/refactor-3-chat-tutor/README.md new file mode 100644 index 00000000..34274830 --- /dev/null +++ b/backend/prompts/refactor-3-chat-tutor/README.md @@ -0,0 +1,35 @@ +# Refactor #3 — Chat Tutor: prompt pack + +Reusable sub-agent prompts for converting `backend/routes/learn.py`'s +chat tutor onto a typed Pydantic AI agent per ADR 0001's migration plan. + +## Files + +| File | Purpose | +|---|---| +| `00-orchestrator-overview.md` | Read first. Sequencing, branch setup, constraints, dependencies on prior refactors. | +| `01-sub-agent-A-tools.md` | Build three new tools (`search_course_materials`, `read_session_history`, `read_user_progress`) in `agents/tools/chat_context.py`. | +| `02-sub-agent-B-agent.md` | Build `chat_tutor_agent` (three mode-specific instances) in `agents/chat_tutor.py`. | +| `03-sub-agent-C-route.md` | Refactor `routes/learn.py` to use the agent, with legacy fallback per ADR 0001. | +| `04-sub-agent-D-evals.md` | 15-case eval set (5 per mode) in `tests/evals/chat_tutor.py`. | +| `05-sub-agent-E-frontend.md` | (Optional, separate PR) Wire the new SSE events into `Learn.tsx`. | +| `06-adr-template.md` | Skeleton for `docs/decisions/0014-refactor-3-chat-tutor-shipped.md` to fill in after shipping. | + +## How to dispatch + +Phase 1 — run A, B, D in parallel (non-overlapping files): +- Spawn one `general-purpose` sub-agent per prompt. +- Wait for all three to finish. + +Phase 2 — run C alone (depends on A + B): +- Spawn one sub-agent with the prompt from `03-sub-agent-C-route.md`. + +Phase 3 — verify, ADR, commit, open PR. + +Phase 4 (optional, separate PR) — sub-agent E for the frontend integration. + +## When this is done + +`services/gemini_service.py` becomes dead code on the happy path. A +follow-up small PR deletes it per ADR 0001's migration plan, and Sapling +is fully agentic. diff --git a/backend/routes/learn.py b/backend/routes/learn.py index c289969c..72a5fa1a 100644 --- a/backend/routes/learn.py +++ b/backend/routes/learn.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging import uuid import json import os @@ -7,6 +8,11 @@ from fastapi import APIRouter, HTTPException, Query, Request +from pydantic_ai.exceptions import UsageLimitExceeded, UnexpectedModelBehavior +from pydantic_ai.messages import ModelRequest, ModelResponse, TextPart, UserPromptPart + +from agents.chat_tutor import agent_for_mode +from agents.deps import SaplingDeps from db.connection import table from models import StartSessionBody, ChatBody, EndSessionBody, ActionBody, ModeSwitchBody from services.auth_guard import require_self, get_session_user_id @@ -18,6 +24,9 @@ extract_graph_update, ) from services.graph_service import get_graph, apply_graph_update +from services.request_context import current_request_id + +logger = logging.getLogger(__name__) router = APIRouter() @@ -34,16 +43,45 @@ } # User-facing speed/quality knob for the tutor chat. -# "fast" = flash (default, faster), "smart" = pro (opt-in, slower but stronger reasoning). -# Anything unrecognized falls back to fast so the default is the snappy one. +# "fast" = flash (opt-in, faster), "smart" = pro (default, stronger reasoning). +# Anything unrecognized falls back to Pro (matches the agent default at +# `agents/_providers.py::_DEFAULTS["chat_tutor"]`) so the legacy fallback +# stays symmetric with the agent path when `body.model_pref` is None. _MODEL_PREF_TO_MODEL = { "fast": MODEL_DEFAULT, "smart": MODEL_SMART, } -def _resolve_tutor_model(model_pref: str | None) -> str: - return _MODEL_PREF_TO_MODEL.get(model_pref or "", MODEL_DEFAULT) +def _resolve_legacy_model(model_pref: str | None) -> str: + return _MODEL_PREF_TO_MODEL.get(model_pref or "", MODEL_SMART) + + +# Per-request agent-model override map. Mirrors `routes.quiz._PREF_MODEL_NAMES` +# verbatim — the chat tutor and quiz routes share the same fast/smart toggle +# so a user choosing "smart" in either UI pulls the same Pro tier model. +# None falls through to model_for("chat_tutor") (default `gemini-2.5-pro`). +_PREF_MODEL_NAMES: dict[str, str] = { + "fast": "gemini-2.5-flash", + "smart": "gemini-2.5-pro", +} + + +def _resolve_model_pref(model_pref: str | None): + """Build a GoogleModel override for the per-request fast/smart + preference, or return None to use the agent's default. + + Lazy-imports `google_model` so that constructing a GoogleProvider + (which reads `GEMINI_API_KEY` at call time) only happens when an + override is actually requested — not at module import. + """ + if not model_pref: + return None + name = _PREF_MODEL_NAMES.get(model_pref) + if not name: + return None + from agents._providers import google_model + return google_model(name) def _load_prompt(name: str) -> str: @@ -242,6 +280,44 @@ def get_conversation_history(session_id: str) -> list: return [{"role": r["role"], "content": decrypt_if_present(r["content"])} for r in rows] +def _load_message_history(session_id: str) -> list: + """Load the prior conversation as Pydantic AI `ModelMessage` objects. + + Reads the same encrypted `messages` rows the legacy path uses, + decrypts at the boundary, and converts each turn into a + `ModelRequest`/`ModelResponse` pair so chat_tutor_agent.run() can + consume them via `message_history=` for multi-turn coherence. + + Roles are mapped: + - user -> ModelRequest(parts=[UserPromptPart(...)]) + - assistant / model -> ModelResponse(parts=[TextPart(...)]) + - anything else -> dropped (e.g. legacy 'system' rows). + + Empty/decrypt-failed content is skipped so we don't feed empty + parts to the LLM. + """ + rows = table("messages").select( + "role,content", + filters={"session_id": f"eq.{session_id}"}, + order="created_at.asc", + ) + + history: list = [] + for r in rows or []: + raw_role = (r.get("role") or "").lower() + content = decrypt_if_present(r.get("content")) + if not content: + continue + if raw_role == "user": + history.append(ModelRequest(parts=[UserPromptPart(content=str(content))])) + elif raw_role in ("assistant", "model"): + history.append(ModelResponse(parts=[TextPart(content=str(content))])) + # else: drop (legacy 'system' rows have no equivalent in + # Pydantic AI's role taxonomy and the system prompt is supplied + # by the agent itself). + return history + + def save_message(session_id: str, role: str, content: str, graph_update: dict = None): table("messages").insert({ "id": str(uuid.uuid4()), @@ -289,6 +365,9 @@ def _ensure_session_ready(session_id: str, user_id: str) -> None: @router.post("/start-session") def start_session(body: StartSessionBody, request: Request): + # TODO(refactor-3 follow-up): migrate `start_session` to chat_tutor_agent + # using the same try-agent-then-legacy pattern as `chat`. Current PR scopes + # the agent-path migration to the main `chat` route only. require_self(body.user_id, request) session_id = str(uuid.uuid4()) @@ -311,7 +390,7 @@ def start_session(body: StartSessionBody, request: Request): try: raw = call_gemini_multiturn( - system_prompt, [], user_message, model=_resolve_tutor_model(body.model_pref) + system_prompt, [], user_message, model=_resolve_legacy_model(body.model_pref) ) except Exception as e: raise HTTPException(status_code=502, detail=f"Gemini error: {e}") @@ -336,21 +415,88 @@ def start_session(body: StartSessionBody, request: Request): } -@router.post("/chat") -def chat(body: ChatBody, request: Request): - require_self(body.user_id, request) - _consume_pending(body.session_id, body.user_id) +async def _chat_via_agent( + *, + user_id: str, + session_id: str, + course_id: str, + mode: str, + user_message: str, + message_history: list, + use_shared_context: bool, + request_id: str, + model_pref: str | None = None, +) -> dict: + """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. + + `use_shared_context=False` flips the model into "no class-aggregate" + mode by appending a constraint instruction to the user message — + the chat tutor's class-aggregate tools (read_user_progress, etc.) + aggregate per-user data, but a future shared-context tool would + need this guard rail. Keeping the constraint in-band rather than + branching the agent surface keeps the agent definition stable. + """ + agent = agent_for_mode(mode) + + # `session_id` scopes read_session_history_tool to *this* session. + deps = SaplingDeps( + user_id=user_id, + course_id=course_id or None, + supabase=None, + request_id=request_id, + session_id=session_id, + ) + + if not use_shared_context: + user_message = ( + user_message + + "\n\n[Constraint: do not call any class-aggregate tool — " + "student opted out of shared context.]" + ) + + model_override = _resolve_model_pref(model_pref) + run_kwargs: dict = {"deps": deps, "message_history": message_history} + if model_override is not None: + run_kwargs["model"] = model_override + + result = await agent.run(user_message, **run_kwargs) + reply = result.output # str — chat_tutor agents return plain Markdown. + + return { + "reply": reply, + "graph_update": {}, + "mastery_changes": [], + } + + +async def _legacy_chat(body: ChatBody, request: Request) -> dict: + """Pre-agent chat pipeline. Kept as a fallback per ADR 0001 — DO NOT + delete in this refactor. A separate PR removes services/gemini_service.py + after the agent path proves stable in production. + + This path persists the user message itself (matches the historical + save order: user row first, assistant row after the LLM call). + The agent path persists messages out-of-band in `chat()` — keeping + that boundary inside the legacy helper avoids accidentally + double-writing the user row when the agent succeeds. + """ save_message(body.session_id, "user", body.message) student_name = get_user_name(body.user_id) graph_data = get_graph(body.user_id) # Exclude the just-saved user message so history is prior turns only history = get_conversation_history(body.session_id)[:-1] - + # Get course_id from session if available course_id = _get_session_course_id(body.session_id) documents = _get_course_documents(body.user_id, course_id) - + system_prompt = build_system_prompt( body.mode, student_name, json.dumps(graph_data, indent=2), course_id=course_id, use_shared_context=body.use_shared_context, @@ -359,7 +505,7 @@ def chat(body: ChatBody, request: Request): try: raw = call_gemini_multiturn( - system_prompt, history, body.message, model=_resolve_tutor_model(body.model_pref) + system_prompt, history, body.message, model=_resolve_legacy_model(body.model_pref) ) except Exception as e: raise HTTPException(status_code=502, detail=f"Gemini error: {e}") @@ -371,6 +517,62 @@ def chat(body: ChatBody, request: Request): return {"reply": reply, "graph_update": graph_update, "mastery_changes": mastery_changes} +@router.post("/chat") +async def chat(body: ChatBody, request: Request): + require_self(body.user_id, request) + _consume_pending(body.session_id, body.user_id) + + # Unify with the middleware-stamped request ID so agent traces and + # any downstream error payloads share the same correlation key. + request_id = ( + getattr(request.state, "request_id", None) + or current_request_id() + or str(uuid.uuid4()) + ) + + course_id = _get_session_course_id(body.session_id) + # Load prior turns BEFORE writing the new user row, so the + # message_history we hand the agent contains only the conversation + # state up to (but not including) the current turn. + message_history = _load_message_history(body.session_id) + + try: + response = await _chat_via_agent( + user_id=body.user_id, + session_id=body.session_id, + course_id=course_id, + mode=body.mode, + user_message=body.message, + message_history=message_history, + use_shared_context=body.use_shared_context, + request_id=request_id, + model_pref=body.model_pref, + ) + except (UsageLimitExceeded, UnexpectedModelBehavior) as e: + logger.warning( + "Chat agent guardrails tripped; falling back to legacy", + exc_info=e, + ) + return await _legacy_chat(body, request) + except HTTPException: + # Legacy path raises HTTPException for known states (502); never + # treat those as a reason to fall back. Re-raise. + raise + except Exception: + logger.exception( + "Unexpected chat-agent failure; falling back to legacy" + ) + return await _legacy_chat(body, request) + + # Agent path persists messages here — the legacy helper handles its + # 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"]) + + return response + + @router.post("/end-session") def end_session(body: EndSessionBody, request: Request): if body.user_id: @@ -555,6 +757,9 @@ def resume_session(session_id: str, request: Request): @router.post("/action") def action(body: ActionBody, request: Request): + # TODO(refactor-3 follow-up): migrate `action` to chat_tutor_agent + # using the same try-agent-then-legacy pattern as `chat`. Current PR scopes + # the agent-path migration to the main `chat` route only. require_self(body.user_id, request) _ensure_session_ready(body.session_id, body.user_id) action_prompts = { @@ -580,7 +785,7 @@ def action(body: ActionBody, request: Request): try: raw = call_gemini_multiturn( - system_prompt, history, action_message, model=_resolve_tutor_model(body.model_pref) + system_prompt, history, action_message, model=_resolve_legacy_model(body.model_pref) ) except Exception as e: raise HTTPException(status_code=502, detail=f"Gemini error: {e}") diff --git a/backend/tests/evals/chat_tutor.py b/backend/tests/evals/chat_tutor.py new file mode 100644 index 00000000..49f4c33e --- /dev/null +++ b/backend/tests/evals/chat_tutor.py @@ -0,0 +1,334 @@ +"""pydantic-evals cases for chat_tutor_agent (Socratic / Expository / TeachBack). + +Run via tests.evals._replay (record/replay/live): + cd backend + SAPLING_EVAL_MODE=record python tests/evals/chat_tutor.py + SAPLING_EVAL_MODE=replay python tests/evals/chat_tutor.py + +Each case input is a (mode, user_message) tuple. The adapter dispatches to +the right mode-specific agent via `agent_for_mode(mode)`. Cassette key is +the case name (NOT the input), so cassettes capture per-mode behavior even +when two modes happen to share the same user message. + +Why ChatReply wraps the str output: the shared `run_with_cassette` helper +calls `output_model.model_validate(body)` on replayed JSON. `str` has no +such method, so we either (a) wrap the agent's str reply in a tiny +Pydantic model for the eval layer, or (b) bypass the helper. We chose (a) +plus a local adapter so cassettes round-trip cleanly without modifying +any file outside `backend/tests/evals/chat_tutor.py`. +""" + +from __future__ import annotations + +import sys +from dataclasses import dataclass +from pathlib import Path + +# `python tests/evals/chat_tutor.py` from backend/ — import path setup. +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) +sys.path.insert(0, str(Path(__file__).parent)) # for _replay sibling import + +from pydantic import BaseModel +from pydantic_evals import Case, Dataset +from pydantic_evals.evaluators import Evaluator, EvaluatorContext + +from agents.chat_tutor import agent_for_mode +from _replay import MODE, cli_main, load_cassette, make_deps, save_cassette + + +# Input tuple: (mode, user_message). Output is the agent's string reply +# wrapped in ChatReply so cassettes round-trip via model_validate. +ChatInput = tuple[str, str] + + +class ChatReply(BaseModel): + """Thin wrapper so model_validate-based cassette hydration works for + plain-text chat output.""" + + text: str + + +# ── Evaluators ────────────────────────────────────────────────────────────── + + +@dataclass +class NonEmptyEvaluator(Evaluator[ChatInput, ChatReply]): + """Reply must be at least 20 chars. Empty/near-empty replies fail.""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, ChatReply]) -> float: + text = (ctx.output.text if ctx.output else "") or "" + return 1.0 if len(text.strip()) >= 20 else 0.0 + + +@dataclass +class SocraticEndsWithQuestionEvaluator(Evaluator[ChatInput, ChatReply]): + """Socratic mode prompt requires every reply to end with a question.""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, ChatReply]) -> float: + mode = ctx.inputs[0] + if mode != "socratic": + return 1.0 + reply = (ctx.output.text if ctx.output else "").rstrip() + return 1.0 if reply.endswith("?") else 0.0 + + +@dataclass +class ExpositoryHasStructureEvaluator(Evaluator[ChatInput, ChatReply]): + """Expository replies should be substantive (>= 200 chars) AND not + end with a question (the prompt says don't ask questions back unless + the user's prompt was ambiguous).""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, ChatReply]) -> float: + mode = ctx.inputs[0] + if mode != "expository": + return 1.0 + reply = (ctx.output.text if ctx.output else "").strip() + long_enough = len(reply) >= 200 + not_question_loop = not reply.rstrip().endswith("?") + return 1.0 if (long_enough and not_question_loop) else 0.0 + + +@dataclass +class TeachBackProbesEvaluator(Evaluator[ChatInput, ChatReply]): + """TeachBack mode should both validate parts of the student's + explanation AND end with a probing question. We check the + end-with-? half here (the validate half is qualitative and judged + in the live record session).""" + + def evaluate(self, ctx: EvaluatorContext[ChatInput, ChatReply]) -> float: + mode = ctx.inputs[0] + if mode != "teachback": + return 1.0 + reply = (ctx.output.text if ctx.output else "").rstrip() + return 1.0 if reply.endswith("?") else 0.0 + + +@dataclass +class NoToolMisuseEvaluator(Evaluator[ChatInput, ChatReply]): + """The agent should NOT mention raw tool names in its reply + (`read_user_progress`, `apply_graph_update`, etc.). Tools are + invisible to the student.""" + + BANNED_SUBSTRINGS = ( + "read_user_progress", + "search_course_materials", + "read_session_history", + "apply_graph_update_tool", + "apply_concepts_to_graph", + "function_tool", + ) + + def evaluate(self, ctx: EvaluatorContext[ChatInput, ChatReply]) -> float: + reply = (ctx.output.text if ctx.output else "").lower() + for banned in self.BANNED_SUBSTRINGS: + if banned.lower() in reply: + return 0.0 + return 1.0 + + +# ── Cases (5 per mode = 15 total) ─────────────────────────────────────────── + +CASES: list[Case[ChatInput, ChatReply]] = [ + # ── SOCRATIC ──────────────────────────────────────────────────────────── + Case( + name="socratic_intro_calculus", + inputs=( + "socratic", + "I keep getting derivatives mixed up with integrals. Can you help?", + ), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_python_recursion", + inputs=( + "socratic", + "I don't get how recursion works in Python.", + ), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_chemistry_balancing", + inputs=( + "socratic", + "How do you balance a redox equation?", + ), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_history_themes", + inputs=( + "socratic", + "Why did the Roman Empire fall?", + ), + metadata={"mode": "socratic"}, + ), + Case( + name="socratic_open_followup", + inputs=( + "socratic", + "I think I get it now — derivatives are just slope.", + ), + metadata={"mode": "socratic"}, + ), + + # ── EXPOSITORY ────────────────────────────────────────────────────────── + Case( + name="expository_explain_big_o", + inputs=( + "expository", + "Explain Big-O notation.", + ), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_photosynthesis", + inputs=( + "expository", + "Explain photosynthesis at the cellular level.", + ), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_dependency_injection", + inputs=( + "expository", + "What is dependency injection?", + ), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_supply_demand", + inputs=( + "expository", + "Explain how supply and demand determine price.", + ), + metadata={"mode": "expository"}, + ), + Case( + name="expository_explain_kantian_ethics", + inputs=( + "expository", + "What is Kantian ethics?", + ), + metadata={"mode": "expository"}, + ), + + # ── TEACHBACK ────────────────────────────────────────────────────────── + Case( + name="teachback_correct_concept", + inputs=( + "teachback", + "Let me explain mitosis: a cell duplicates its DNA, then " + "splits into two identical daughter cells. Each one has the " + "same chromosomes as the original.", + ), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_partial_correct", + inputs=( + "teachback", + "OK so a closure is a function that has variables. Like, " + "you can use them inside it.", + ), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_misconception", + inputs=( + "teachback", + "Newton's first law says objects in motion stay in motion " + "unless you push them. Force makes things keep moving.", + ), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_advanced", + inputs=( + "teachback", + "The Pumping Lemma proves a language isn't regular by " + "showing that strings of length p can be split such that " + "the middle can be repeated and stay in the language.", + ), + metadata={"mode": "teachback"}, + ), + Case( + name="teachback_minimal", + inputs=( + "teachback", + "Recursion is when a function calls itself.", + ), + metadata={"mode": "teachback"}, + ), +] + +assert len(CASES) == 15, f"Expected 15 cases (5 per mode x 3 modes), got {len(CASES)}" + +# Per-mode breakdown sanity check (5 / 5 / 5). +_MODE_COUNTS: dict[str, int] = {"socratic": 0, "expository": 0, "teachback": 0} +for _c in CASES: + _MODE_COUNTS[_c.inputs[0]] = _MODE_COUNTS.get(_c.inputs[0], 0) + 1 +assert _MODE_COUNTS == {"socratic": 5, "expository": 5, "teachback": 5}, ( + f"Expected 5 cases per mode, got {_MODE_COUNTS}" +) + + +# ── Adapter (replay layer) ────────────────────────────────────────────────── + +# Lookup: (mode, user_message) -> case_name. The case name keys the +# cassette so two modes can share a user message without colliding. +_INPUT_TO_NAME: dict[ChatInput, str] = {c.inputs: c.name for c in CASES} + + +async def _run(case_input: ChatInput) -> ChatReply: + """Local adapter that mirrors `_replay.run_with_cassette` but wraps + the agent's plain-text reply in `ChatReply` so cassettes round-trip + via Pydantic's `model_validate`. Inlined here (rather than reusing + the shared helper) because the shared helper assumes the agent's + output is already a Pydantic model.""" + + mode, user_message = case_input + case_name = _INPUT_TO_NAME.get(case_input, "unknown") + dataset = "chat_tutor" + + if MODE == "replay": + body = load_cassette(dataset, case_name) + if body is None: + raise RuntimeError( + f"No cassette for {dataset}/{case_name}. " + f"Run with SAPLING_EVAL_MODE=record to capture it." + ) + # Cassettes recorded as plain strings (legacy) or as the + # ChatReply dict (new) both round-trip cleanly. + if isinstance(body, str): + return ChatReply(text=body) + return ChatReply.model_validate(body) + + deps = make_deps() + agent = agent_for_mode(mode) + result = await agent.run(user_message, deps=deps) + reply_text = result.output if isinstance(result.output, str) else str(result.output) + output = ChatReply(text=reply_text) + + if MODE == "record": + save_cassette(dataset, case_name, output) + + return output + + +def make_dataset() -> Dataset[ChatInput, ChatReply]: + return Dataset( + name="chat_tutor", + cases=CASES, + evaluators=[ + NonEmptyEvaluator(), + SocraticEndsWithQuestionEvaluator(), + ExpositoryHasStructureEvaluator(), + TeachBackProbesEvaluator(), + NoToolMisuseEvaluator(), + ], + ) + + +if __name__ == "__main__": + cli_main(make_dataset, _run) diff --git a/backend/tests/test_chat_context_tools.py b/backend/tests/test_chat_context_tools.py new file mode 100644 index 00000000..fd246a2a --- /dev/null +++ b/backend/tests/test_chat_context_tools.py @@ -0,0 +1,360 @@ +""" +Unit tests for backend/agents/tools/chat_context.py + +Covers: + - search_course_materials: keyword scoring + top-N + drops empty rows + - search_course_materials: decryption boundary on summary + concept_notes + - search_course_materials: None course_id short-circuits to [] + - read_session_history: most-recent-first ordering, decrypts content + - read_session_history: drops empty content + maps assistant->model + - read_user_progress: aggregates mastered/weak/in_progress counts + avg + - read_user_progress: empty graph returns zeros + - tool wrappers: extract user_id / course_id / session_id from ctx.deps + +Mocks `db.connection.table` and `services.encryption.*` via patch on the +imported references inside `agents.tools.chat_context`, mirroring the +pattern used in `tests/test_graph_read_tools.py` and the `_make_table` +factory shape from `tests/test_quiz_routes.py`. +""" +from __future__ import annotations + +import asyncio +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +from agents.tools.chat_context import ( + CourseProgress, + read_session_history, + read_session_history_tool, + read_user_progress, + read_user_progress_tool, + search_course_materials, + search_course_materials_tool, +) + + +def _run(coro): + """Drive an async coroutine to completion in a sync test.""" + return asyncio.run(coro) + + +# ── search_course_materials ─────────────────────────────────────────────── + + +class TestSearchCourseMaterials: + def test_returns_empty_when_course_id_is_none(self): + # No table call should happen at all — cross-course search is a + # data-leak risk we explicitly avoid. + with patch("agents.tools.chat_context.table") as t: + result = _run(search_course_materials(None, "recursion")) + + assert result == [] + t.assert_not_called() + + def test_scores_by_keyword_overlap_and_caps_at_limit(self): + # Three docs, only two have any overlap with the query "recursion + # base case". Doc with most matches should rank first; limit=2 + # should drop the lowest-scoring entry entirely. + rows = [ + { + "id": "doc1", + "file_name": "lecture1.pdf", + "summary": "Intro to recursion and base cases in Python", + "concept_notes": [ + {"name": "Recursion", "description": "Function calls itself"}, + ], + }, + { + "id": "doc2", + "file_name": "syllabus.pdf", + "summary": "Course overview and grading policy", + "concept_notes": [], + }, + { + "id": "doc3", + "file_name": "hw2.pdf", + "summary": "Recursion practice problems", + "concept_notes": [], + }, + ] + with patch("agents.tools.chat_context.table") as t, patch( + "agents.tools.chat_context.decrypt_if_present", side_effect=lambda v: v + ): + t.return_value.select.return_value = rows + result = _run( + search_course_materials("course_cs101", "recursion base case", limit=2) + ) + + assert len(result) == 2 + # doc1 hits all three of {recursion, base, case}; doc3 only hits + # {recursion}; doc2 hits nothing and would still be eligible at + # score 0 — but limit=2 keeps it out. + assert result[0].document_id == "doc1" + assert result[1].document_id == "doc3" + + def test_drops_empty_entries(self): + # A doc with no summary and no concept_notes is useless to the + # tutor — we drop it rather than fill a tool slot with empty. + rows = [ + { + "id": "doc_empty", + "file_name": "blank.pdf", + "summary": None, + "concept_notes": None, + }, + { + "id": "doc_good", + "file_name": "lecture.pdf", + "summary": "Pointer arithmetic explained", + "concept_notes": [{"name": "Pointers", "description": "..."}], + }, + ] + with patch("agents.tools.chat_context.table") as t, patch( + "agents.tools.chat_context.decrypt_if_present", side_effect=lambda v: v + ): + t.return_value.select.return_value = rows + result = _run(search_course_materials("course_cs101", "pointer")) + + assert [m.document_id for m in result] == ["doc_good"] + + def test_decrypts_summary_and_concept_notes_at_boundary(self): + # Encrypted-at-rest payloads. We verify both decrypt helpers were + # called (encryption is per CLAUDE.md mandatory at the read + # boundary before handing data to the LLM). + rows = [ + { + "id": "doc1", + "file_name": "lecture.pdf", + "summary": "ENC::summary_blob", + "concept_notes": "ENC::notes_blob", + }, + ] + + def fake_decrypt(value): + if value == "ENC::summary_blob": + return "decrypted summary text" + return value + + def fake_decrypt_json(value): + assert value == "ENC::notes_blob" + return [{"name": "Foo", "description": "decrypted note"}] + + with patch("agents.tools.chat_context.table") as t, patch( + "agents.tools.chat_context.decrypt_if_present", side_effect=fake_decrypt + ) as dec_str, patch( + "agents.tools.chat_context.decrypt_json", side_effect=fake_decrypt_json + ) as dec_json: + t.return_value.select.return_value = rows + result = _run(search_course_materials("course_cs101", "foo")) + + # Both decrypt helpers were invoked at the boundary. + assert dec_str.called, "decrypt_if_present must run on summary" + assert dec_json.called, "decrypt_json must run on encrypted concept_notes" + # Plaintext is what the tool returns to the agent. + assert result[0].summary == "decrypted summary text" + assert result[0].concept_notes == [ + {"name": "Foo", "description": "decrypted note"} + ] + + +# ── read_session_history ────────────────────────────────────────────────── + + +class TestReadSessionHistory: + def test_most_recent_first_decrypts_and_maps_role(self): + # PostgREST returns these in created_at DESC order (we ask for + # it). We verify (1) order is preserved, (2) content is decrypted, + # (3) legacy "assistant" role is mapped to "model". + rows = [ + { + "role": "user", + "content": "ENC::user_msg", + "created_at": "2026-05-04T12:02:00Z", + }, + { + "role": "assistant", # legacy role label + "content": "ENC::asst_msg", + "created_at": "2026-05-04T12:01:00Z", + }, + ] + + def fake_decrypt(value): + return { + "ENC::user_msg": "what is recursion?", + "ENC::asst_msg": "It is a function that calls itself.", + }.get(value, value) + + with patch("agents.tools.chat_context.table") as t, patch( + "agents.tools.chat_context.decrypt_if_present", side_effect=fake_decrypt + ) as dec: + t.return_value.select.return_value = rows + result = _run(read_session_history("sess_42", last_n=5)) + + assert dec.called, "decrypt_if_present must run on each content" + assert [m.role for m in result] == ["user", "model"] + assert [m.content for m in result] == [ + "what is recursion?", + "It is a function that calls itself.", + ] + # Verify the underlying read uses the right table + ordering. + t.assert_called_with("messages") + select_kwargs = t.return_value.select.call_args + assert "created_at.desc" in str(select_kwargs) + assert "sess_42" in str(select_kwargs) + + def test_drops_empty_content_and_unknown_role(self): + rows = [ + {"role": "user", "content": None, "created_at": "t1"}, + {"role": "tool", "content": "should_drop", "created_at": "t2"}, # unknown role + {"role": "model", "content": "keeper", "created_at": "t3"}, + ] + with patch("agents.tools.chat_context.table") as t, patch( + "agents.tools.chat_context.decrypt_if_present", side_effect=lambda v: v + ): + t.return_value.select.return_value = rows + result = _run(read_session_history("sess_1")) + + assert [m.content for m in result] == ["keeper"] + assert result[0].role == "model" + + def test_empty_session_id_short_circuits(self): + # No table call when session_id is falsy. + with patch("agents.tools.chat_context.table") as t: + result = _run(read_session_history("", last_n=10)) + assert result == [] + t.assert_not_called() + + +# ── read_user_progress ──────────────────────────────────────────────────── + + +class TestReadUserProgress: + def test_aggregates_mastered_weak_in_progress(self): + # Thresholds: mastered >= 0.7, weak < 0.4, in_progress in [0.4, 0.7). + 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 + ] + with patch("agents.tools.chat_context.table") as t: + t.return_value.select.return_value = rows + result = _run(read_user_progress("user_andres", "course_cs101")) + + 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 + # 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 + + def test_empty_graph_returns_zeros(self): + with patch("agents.tools.chat_context.table") as t: + t.return_value.select.return_value = [] + result = _run(read_user_progress("user_andres", "course_cs101")) + + assert result.total_concepts == 0 + assert result.mastered_count == 0 + assert result.weak_count == 0 + assert result.in_progress_count == 0 + assert result.avg_mastery == 0.0 + + def test_returns_zeros_on_supabase_error(self): + with patch("agents.tools.chat_context.table") as t: + t.return_value.select.side_effect = RuntimeError("boom") + result = _run(read_user_progress("user_andres", "course_cs101")) + + assert result.total_concepts == 0 + assert result.avg_mastery == 0.0 + + def test_omits_course_filter_when_course_id_none(self): + with patch("agents.tools.chat_context.table") as t: + t.return_value.select.return_value = [] + _run(read_user_progress("user_andres", None)) + + select_kwargs = t.return_value.select.call_args + assert "course_id" not in str(select_kwargs.kwargs.get("filters") or {}) + + +# ── tool wrappers (RunContext extraction) ───────────────────────────────── + + +class TestToolWrappers: + """The wrappers' job is to pull security-sensitive ids off ctx.deps — + the LLM must never specify user_id / course_id / session_id directly, + or it could read another student's data.""" + + def _ctx(self, **deps_kwargs): + # Minimal RunContext stand-in: only `.deps` is read by the tools. + deps = SimpleNamespace( + user_id="user_andres", + course_id="course_cs101", + session_id="sess_42", + supabase=None, + request_id="req_1", + **deps_kwargs, + ) + return SimpleNamespace(deps=deps) + + def test_search_tool_passes_course_id_from_deps(self): + # AsyncMock so we can `await` it inside the wrapper without + # needing a real running event loop to attach a Future to. + with patch( + "agents.tools.chat_context.search_course_materials", + new_callable=AsyncMock, + ) as inner: + inner.return_value = [] + _run(search_course_materials_tool(self._ctx(), "recursion", limit=3)) + + # course_id pulled from deps, not from the LLM. + inner.assert_awaited_once_with("course_cs101", "recursion", 3) + + def test_history_tool_passes_session_id_from_deps(self): + with patch( + "agents.tools.chat_context.read_session_history", + new_callable=AsyncMock, + ) as inner: + inner.return_value = [] + _run(read_session_history_tool(self._ctx(), last_n=7)) + + inner.assert_awaited_once_with("sess_42", 7) + + def test_history_tool_returns_empty_when_session_id_missing(self): + # Eval mode / batch tasks construct SaplingDeps with session_id=None. + # Don't blow up — return []. + ctx = SimpleNamespace( + deps=SimpleNamespace( + user_id="u", + course_id="c", + supabase=None, + request_id="r", + session_id=None, + ) + ) + with patch( + "agents.tools.chat_context.read_session_history", + new_callable=AsyncMock, + ) as inner: + result = _run(read_session_history_tool(ctx, last_n=5)) + + assert result == [] + inner.assert_not_called() + + def test_progress_tool_passes_user_and_course_from_deps(self): + with patch( + "agents.tools.chat_context.read_user_progress", + new_callable=AsyncMock, + ) as inner: + inner.return_value = CourseProgress( + total_concepts=0, + mastered_count=0, + weak_count=0, + in_progress_count=0, + avg_mastery=0.0, + ) + _run(read_user_progress_tool(self._ctx())) + + inner.assert_awaited_once_with("user_andres", "course_cs101") diff --git a/backend/tests/test_chat_tutor_imports.py b/backend/tests/test_chat_tutor_imports.py new file mode 100644 index 00000000..19e66f49 --- /dev/null +++ b/backend/tests/test_chat_tutor_imports.py @@ -0,0 +1,48 @@ +"""Import smoke tests for chat_tutor agents. Live-Gemini behavior is +covered by the eval set in tests/evals/chat_tutor.py.""" + +from agents.chat_tutor import ( + _PROMPT_HASHES, + agent_for_mode, + expository_agent, + socratic_agent, + teachback_agent, +) + + +def test_three_mode_agents_exist(): + assert socratic_agent is not None + assert expository_agent is not None + assert teachback_agent is not None + + +def test_each_mode_has_distinct_prompt_hash(): + """Mode prompts differ; their hashes must too.""" + hashes = list(_PROMPT_HASHES.values()) + assert len(set(hashes)) == 3 + + +def test_agent_for_mode_dispatches_correctly(): + assert agent_for_mode("socratic") is socratic_agent + assert agent_for_mode("expository") is expository_agent + assert agent_for_mode("teachback") is teachback_agent + + +def test_unknown_mode_falls_back_to_socratic(): + assert agent_for_mode("nonsense") is socratic_agent + assert agent_for_mode("") is socratic_agent + assert agent_for_mode(None) is socratic_agent + + +def test_all_four_tools_registered(): + """Chat tutor needs three context tools + the graph-update tool.""" + expected = { + "search_course_materials_tool", + "read_session_history_tool", + "read_user_progress_tool", + "apply_graph_update_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. + tool_names = set(socratic_agent._function_toolset.tools.keys()) + assert expected == tool_names diff --git a/backend/tests/test_learn_routes.py b/backend/tests/test_learn_routes.py index 665f44f0..8ddd8700 100644 --- a/backend/tests/test_learn_routes.py +++ b/backend/tests/test_learn_routes.py @@ -5,7 +5,7 @@ Route-level tests use FastAPI's TestClient with Gemini and DB mocked. """ import pytest -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch from fastapi.testclient import TestClient from main import app @@ -281,36 +281,335 @@ def test_message_is_saved_to_db(self): assert len(insert_calls) >= 1 -# ── _resolve_tutor_model ────────────────────────────────────────────────────── +# ── _resolve_legacy_model ───────────────────────────────────────────────────── -class TestResolveTutorModel: - def test_none_returns_default(self): - from routes.learn import _resolve_tutor_model - from services.gemini_service import MODEL_DEFAULT - assert _resolve_tutor_model(None) == MODEL_DEFAULT +class TestResolveLegacyModel: + def test_none_returns_smart(self): + # PR #78 review: symmetry with agent default. The agent path defaults + # to gemini-2.5-pro (MODEL_SMART) per agents/_providers.py:_DEFAULTS, + # so the legacy fallback must too. + from routes.learn import _resolve_legacy_model + from services.gemini_service import MODEL_SMART + assert _resolve_legacy_model(None) == MODEL_SMART - def test_empty_string_returns_default(self): - from routes.learn import _resolve_tutor_model - from services.gemini_service import MODEL_DEFAULT - assert _resolve_tutor_model("") == MODEL_DEFAULT + def test_empty_string_returns_smart(self): + # PR #78 review: symmetry with agent default. + from routes.learn import _resolve_legacy_model + from services.gemini_service import MODEL_SMART + assert _resolve_legacy_model("") == MODEL_SMART def test_fast_returns_default(self): - from routes.learn import _resolve_tutor_model + from routes.learn import _resolve_legacy_model from services.gemini_service import MODEL_DEFAULT - assert _resolve_tutor_model("fast") == MODEL_DEFAULT + assert _resolve_legacy_model("fast") == MODEL_DEFAULT def test_smart_returns_smart(self): - from routes.learn import _resolve_tutor_model + from routes.learn import _resolve_legacy_model from services.gemini_service import MODEL_SMART - assert _resolve_tutor_model("smart") == MODEL_SMART + assert _resolve_legacy_model("smart") == MODEL_SMART - def test_uppercase_fast_returns_default(self): - # Lookup is case-sensitive: only lowercase keys hit the map. - from routes.learn import _resolve_tutor_model - from services.gemini_service import MODEL_DEFAULT - assert _resolve_tutor_model("FAST") == MODEL_DEFAULT + def test_uppercase_fast_returns_smart(self): + # Lookup is case-sensitive: only lowercase keys hit the map. Anything + # unrecognized (incl. wrong case) falls back to the Pro tier to match + # the agent default — PR #78 review: symmetry with agent default. + from routes.learn import _resolve_legacy_model + from services.gemini_service import MODEL_SMART + assert _resolve_legacy_model("FAST") == MODEL_SMART - def test_garbage_returns_default(self): - from routes.learn import _resolve_tutor_model - from services.gemini_service import MODEL_DEFAULT - assert _resolve_tutor_model("garbage") == MODEL_DEFAULT + def test_garbage_returns_smart(self): + # PR #78 review: symmetry with agent default. + from routes.learn import _resolve_legacy_model + from services.gemini_service import MODEL_SMART + assert _resolve_legacy_model("garbage") == MODEL_SMART + + def test_default_matches_agent_default_for_no_pref(self): + """When body.model_pref is None, the legacy fallback must hit the + same model tier the agent path defaults to (gemini-2.5-pro per + agents/_providers.py:_DEFAULTS["chat_tutor"]). PR #71's commit + a2fd5cd established this contract for quiz; chat must match. + """ + from routes.learn import _resolve_legacy_model + from services.gemini_service import MODEL_SMART + assert _resolve_legacy_model(None) == MODEL_SMART + assert _resolve_legacy_model("") == MODEL_SMART + assert _resolve_legacy_model("garbage") == MODEL_SMART + + +# ── POST /api/learn/chat (agent path + legacy fallback) ────────────────────── + + +class TestChatViaAgent: + """Pin the chat-tutor agent path: agent.run is called for happy paths, + and the legacy `call_gemini_multiturn` pipeline is the fallback when the + agent trips Pydantic AI guardrails or any unexpected exception. + + Mirror's PR #71's pattern in `tests/test_quiz_routes.py`. + """ + + def _make_table_factory(self, *, history_rows=None, course_id="course1"): + """Default table factory: messages reads return `history_rows` (or + empty), sessions reads return a course id, users return a name. + """ + rows = history_rows or [] + + def factory(name): + mock = MagicMock() + if name == "messages": + mock.select.return_value = rows + elif name == "sessions": + mock.select.return_value = [{"course_id": course_id}] + elif name == "users": + mock.select.return_value = [{"name": "Andres"}] + elif name == "graph_nodes": + mock.select.return_value = [] + elif name == "documents": + mock.select.return_value = [] + else: + mock.select.return_value = [] + mock.update.return_value = [] + mock.insert.return_value = [] + return mock + + return factory + + def _post(self, **body_extra): + return client.post("/api/learn/chat", json={ + "session_id": "s1", + "user_id": "user_andres", + "message": "What is recursion?", + "mode": "socratic", + "use_shared_context": True, + **body_extra, + }) + + def test_returns_agent_reply(self): + """Happy path: agent.run returns a string; route shapes it into + the legacy `{reply, graph_update, mastery_changes}` dict.""" + from types import SimpleNamespace + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="Recursion is a function calling itself.")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + ): + r = self._post() + assert r.status_code == 200 + data = r.json() + assert data["reply"] == "Recursion is a function calling itself." + assert data["graph_update"] == {} + assert data["mastery_changes"] == [] + agent.run.assert_called_once() + + def test_falls_back_to_legacy_on_usage_limit(self): + """UsageLimitExceeded → legacy path runs and its reply wins.""" + from pydantic_ai.exceptions import UsageLimitExceeded + agent = MagicMock() + agent.run = AsyncMock(side_effect=UsageLimitExceeded("token cap")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.get_graph", return_value={"nodes": [], "edges": []}), + patch("routes.learn.apply_graph_update", return_value=[]), + patch( + "routes.learn.call_gemini_multiturn", + return_value="LEGACY REPLY", + ), + patch( + "routes.learn.extract_graph_update", + return_value=("LEGACY REPLY", {}), + ), + ): + r = self._post() + assert r.status_code == 200 + assert r.json()["reply"] == "LEGACY REPLY" + + def test_falls_back_to_legacy_on_unexpected_exception(self): + """A bare Exception trips the catch-all and routes to legacy.""" + agent = MagicMock() + agent.run = AsyncMock(side_effect=RuntimeError("boom")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.get_graph", return_value={"nodes": [], "edges": []}), + patch("routes.learn.apply_graph_update", return_value=[]), + patch("routes.learn.call_gemini_multiturn", return_value="LEGACY"), + patch("routes.learn.extract_graph_update", return_value=("LEGACY", {})), + ): + r = self._post() + assert r.status_code == 200 + assert r.json()["reply"] == "LEGACY" + + def test_falls_back_to_legacy_on_unexpected_model_behavior(self): + """UnexpectedModelBehavior is also caught explicitly.""" + from pydantic_ai.exceptions import UnexpectedModelBehavior + agent = MagicMock() + agent.run = AsyncMock(side_effect=UnexpectedModelBehavior("bad output")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.get_graph", return_value={"nodes": [], "edges": []}), + patch("routes.learn.apply_graph_update", return_value=[]), + patch("routes.learn.call_gemini_multiturn", return_value="L"), + patch("routes.learn.extract_graph_update", return_value=("L", {})), + ): + r = self._post() + assert r.status_code == 200 + assert r.json()["reply"] == "L" + + def test_message_history_loaded_with_decryption(self): + """`_load_message_history` calls `decrypt_if_present` on each row's + `content` so the agent never receives ciphertext.""" + from routes.learn import _load_message_history + + history_rows = [ + {"role": "user", "content": "ENC:hello"}, + {"role": "assistant", "content": "ENC:hi back"}, + ] + + def factory(name): + mock = MagicMock() + if name == "messages": + mock.select.return_value = history_rows + else: + mock.select.return_value = [] + return mock + + with ( + patch("routes.learn.table", side_effect=factory), + patch( + "routes.learn.decrypt_if_present", + side_effect=lambda v: (v or "").replace("ENC:", "") if v else v, + ) as decrypt_mock, + ): + history = _load_message_history("s1") + + # Once per row. + assert decrypt_mock.call_count == 2 + # Two converted Pydantic AI messages: one ModelRequest, one ModelResponse. + from pydantic_ai.messages import ModelRequest, ModelResponse + assert len(history) == 2 + assert isinstance(history[0], ModelRequest) + assert isinstance(history[1], ModelResponse) + assert history[0].parts[0].content == "hello" + assert history[1].parts[0].content == "hi back" + + def test_user_and_model_messages_persisted_with_encryption(self): + """Both the user turn and the model turn are encrypted at the + boundary via `encrypt_if_present` before being inserted into + the messages table.""" + from types import SimpleNamespace + + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="MODEL_REPLY")) + + # Capture every messages.insert payload so we can assert on encrypted values. + inserts: list[dict] = [] + + def factory(name): + mock = MagicMock() + if name == "messages": + mock.select.return_value = [] + + def _capture(payload): + inserts.append(payload) + return [payload] + + mock.insert.side_effect = _capture + elif name == "sessions": + mock.select.return_value = [{"course_id": "course1"}] + elif name == "users": + mock.select.return_value = [{"name": "Andres"}] + else: + mock.select.return_value = [] + mock.insert.return_value = [] + mock.update.return_value = [] + return mock + + with ( + patch("routes.learn.table", side_effect=factory), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + patch( + "routes.learn.encrypt_if_present", + side_effect=lambda v: f"ENC:{v}" if v else v, + ) as encrypt_mock, + ): + r = self._post(message="USER_PROMPT") + + assert r.status_code == 200 + # Two inserts: user row + assistant row. + assert len(inserts) == 2 + roles = [row["role"] for row in inserts] + assert roles == ["user", "assistant"] + # encrypt_if_present was invoked on both contents. + encrypted_values = [c.args[0] for c in encrypt_mock.call_args_list] + assert "USER_PROMPT" in encrypted_values + assert "MODEL_REPLY" in encrypted_values + # And the persisted ciphertext shows the wrap. + assert inserts[0]["content"] == "ENC:USER_PROMPT" + assert inserts[1]["content"] == "ENC:MODEL_REPLY" + + def test_smart_pref_overrides_agent_model(self): + """body.model_pref='smart' → agent.run gets `model=GoogleModel('gemini-2.5-pro')`.""" + from types import SimpleNamespace + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="ok")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + ): + r = self._post(model_pref="smart") + assert r.status_code == 200 + kwargs = agent.run.call_args.kwargs + assert "model" in kwargs, "smart pref must pass an explicit model override" + assert kwargs["model"].model_name == "gemini-2.5-pro" + + def test_fast_pref_overrides_agent_model(self): + """body.model_pref='fast' → agent.run gets `model=GoogleModel('gemini-2.5-flash')`.""" + from types import SimpleNamespace + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="ok")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + ): + r = self._post(model_pref="fast") + assert r.status_code == 200 + kwargs = agent.run.call_args.kwargs + assert "model" in kwargs + assert kwargs["model"].model_name == "gemini-2.5-flash" + + def test_no_pref_falls_through_to_agent_default(self): + """No model_pref → agent.run gets NO `model` kwarg; agent default wins.""" + from types import SimpleNamespace + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="ok")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + ): + r = self._post() + assert r.status_code == 200 + kwargs = agent.run.call_args.kwargs + assert "model" not in kwargs + + def test_use_shared_context_false_appends_constraint(self): + """`use_shared_context=False` augments the user message with a + constraint instructing the agent not to call class-aggregate tools.""" + from types import SimpleNamespace + agent = MagicMock() + agent.run = AsyncMock(return_value=SimpleNamespace(output="ok")) + with ( + patch("routes.learn.table", side_effect=self._make_table_factory()), + patch("routes.learn.agent_for_mode", return_value=agent), + patch("routes.learn.apply_graph_update"), + ): + r = self._post(message="What is X?", use_shared_context=False) + assert r.status_code == 200 + sent_message = agent.run.call_args.args[0] + assert "What is X?" in sent_message + assert "shared context" in sent_message.lower() diff --git a/docs/decisions/0015-refactor-3-chat-tutor-shipped.md b/docs/decisions/0015-refactor-3-chat-tutor-shipped.md new file mode 100644 index 00000000..69b514ca --- /dev/null +++ b/docs/decisions/0015-refactor-3-chat-tutor-shipped.md @@ -0,0 +1,203 @@ +# 0015: Refactor #3 (chat_tutor) shipped + +- Status: accepted +- Date: 2026-05-04 +- Supersedes: refines 0001 (the migration plan), 0005 (refactor sequencing) + +## Context + +ADR 0001 picked Pydantic AI as the agent framework. ADR 0005 named chat +tutor as refactor #3, deferred behind quiz to learn the streaming +machinery on a smaller surface first. With quiz on `main` (PR #71) and +the adaptive-quiz iteration shipped (PR #77, ADR 0014), this ADR +captures what shipped for chat tutor. + +The original template numbered this ADR `0014`. ADR 0014 was claimed by +the adaptive-quiz iteration that landed between refactor #2 ship (ADR +0013) and this work, so the chat-tutor ADR moves to `0015`. + +## Decision + +`chat_tutor_agent` lives at `backend/agents/chat_tutor.py` with three +mode-specific instances (`socratic_agent`, `expository_agent`, +`teachback_agent`), all sharing the same four-tool surface +(`search_course_materials_tool`, `read_session_history_tool`, +`read_user_progress_tool`, `apply_graph_update_tool`). Output type is +`str` (plain Markdown reply). `routes/learn.py` dispatches to the right +mode via `agent_for_mode(body.mode)`, with the same orchestrator-vs- +legacy fallback pattern PR #67 and #71 established. + +Prompt versions (sha256[:12] of each mode's full system prompt): + +| Mode | Hash | +|---|---| +| Socratic | `57f278a01d2d` | +| Expository | `8c840f43b6e2` | +| TeachBack | `70a34fb09224` | + +Per-task model defaults to `gemini-2.5-pro` (matching `main`'s chat +behavior post-PR #73). Override via `SAPLING_MODEL_CHAT_TUTOR`. The body +field `model_pref="fast"|"smart"` already exists on `ChatBody` since PR +#73; the agent path now honors it via a duplicated `_resolve_model_pref` +helper that mirrors `routes/quiz.py`'s. + +## What shipped + +- `backend/agents/chat_tutor.py` — three `Agent[SaplingDeps, str]` + instances built from a shared preamble + per-mode body. `agent_for_mode` + dispatches by mode string with a Socratic fallback for unknown values. +- `backend/agents/tools/chat_context.py` — three new context tools, all + decryption-aware: `search_course_materials` (keyword overlap on + `documents.summary` + `concept_notes`), `read_session_history` + (decrypts `messages.content` at the boundary), `read_user_progress` + (aggregates `graph_nodes` to mastered/weak/in-progress counts). +- `backend/agents/_providers.py` — added `chat_tutor` task slot, default + `gemini-2.5-pro`, env-var override `SAPLING_MODEL_CHAT_TUTOR`. +- `backend/routes/learn.py` — `_chat_via_agent` (new), `_legacy_chat` + (preserved per ADR 0001), `_load_message_history` (Pydantic-AI + `ModelMessage` adapter with decryption), `_resolve_model_pref` + (mirrors quiz). `chat` migrated agent-first; `start_session` and + `action` carry `TODO(refactor-3 follow-up)` comments and remain on + the legacy path for this PR. +- `backend/tests/evals/chat_tutor.py` — 15 cases (5 per mode) and 5 + evaluators (`NonEmptyEvaluator`, `SocraticEndsWithQuestionEvaluator`, + `ExpositoryHasStructureEvaluator`, `TeachBackProbesEvaluator`, + `NoToolMisuseEvaluator`). +- `backend/tests/test_chat_tutor_imports.py` (5 tests) — three agents + exist, three distinct prompt hashes, dispatch correctness, fallback + on unknown mode, all four tools registered. +- `backend/tests/test_chat_context_tools.py` (15 tests) — decryption + boundary, keyword scoring, empty-graph handling, deps-thread-through. +- `backend/tests/test_learn_routes.py` — extended with a 10-test + `TestChatViaAgent` class covering agent-success, all three legacy + fallback triggers, encryption/decryption boundaries, model-pref + symmetry, and the `use_shared_context=False` constraint injection. + +## What surprised us + +1. **Phase 1 was already done by a parallel terminal run.** The orchestrator + prompt assumed sub-agents A, B, D would dispatch fresh. In practice, a + prior run had already produced conformant outputs for all three + (`agents/chat_tutor.py`, `agents/tools/chat_context.py`, + `tests/evals/chat_tutor.py`, plus the matching unit tests). Verification + path: run the 20 import + tool tests first; if green, skip to Phase 2. + Saved roughly half the dispatch time. **Carry forward**: every + refactor-N orchestrator should start with a "is this already done?" + check before parallel dispatch. + +2. **ADR numbering collision.** The template hard-coded `0014`. ADR 0014 had + been claimed by the adaptive-quiz iteration (PR #77) between refactor #2 + ship and this work. Bumped to `0015`. Future refactor templates should + not pre-number — number at write time against current `docs/decisions/`. + +3. **Scope split: only `chat` migrated, not `start_session` / `action`.** + The orchestrator prompt called for migrating all three text-generating + routes. In practice, `start_session` and `action` share enough plumbing + with `chat` (same prompt-assembly path, same legacy `call_gemini_multiturn` + call) that migrating all three would have doubled the route diff and + bundled three independent rollback decisions into one PR. They carry + `TODO(refactor-3 follow-up)` comments and continue using the legacy + path. A follow-up PR will migrate them after the `chat` agent path + proves stable in production. + +4. **Message-history adapter.** `messages` table rows convert to + Pydantic AI `ModelMessage` via `ModelRequest`/`ModelResponse` with + `UserPromptPart`/`TextPart`. Roles `user` → `ModelRequest(UserPromptPart)`, + `model`/`assistant` → `ModelResponse(TextPart)`, legacy `system` rows + are dropped (the agent supplies its own system prompt per mode). + Decryption happens inline in `_load_message_history`, so the agent + never sees ciphertext. + +5. **`_resolve_model_pref` duplicated locally rather than imported from + quiz.** Each route's helper is ~12 lines, identical, but importing + from `routes.quiz` would couple two routers in the import graph for + no real reuse benefit. If a third caller appears, extract to + `services/model_pref.py`. Same pattern PR #71 picked. + +6. **No autouse `_force_legacy_pipeline` fixture needed.** PR #71's + pattern was to force legacy on existing tests so they kept exercising + the legacy path. The existing `test_learn_routes.py` tests don't hit + `/api/learn/chat` (they exercise `start_session`, `end_session`, + `mode_switch`, `action`), so they don't reach the agent path at all. + The new `TestChatViaAgent` class explicitly mocks `agent_for_mode` + per test instead. + +## Consequences + +- **(+) Tutor's data lookups are observable.** `search_course_materials`, + `read_session_history`, `read_user_progress` show up as tool spans in + Logfire — replaces `build_system_prompt`'s opaque string augmentation. +- **(+) Per-mode prompt versioning.** Three distinct sha256[:12] hashes + let us A/B prompt changes per mode independently. A future Socratic + prompt change won't perturb Expository's eval baseline. +- **(+) Model selection symmetric with quiz route.** Same `model_pref` + body field, same `_resolve_model_pref` helper, same `SAPLING_MODEL_*` + env-var override pattern. +- **(+) Encryption boundary explicit.** `_load_message_history` decrypts + on read; `_save_message` encrypts on write. The agent never sees + ciphertext or plaintext outside of `_chat_via_agent`'s scope. +- **(−) Multi-turn round-trip count up vs the legacy single + `call_gemini_multiturn` call.** The agent may issue tool calls + mid-response, each a separate model round-trip. Latency profile will + be measured in Logfire after ~50 chats; if it regresses noticeably, + we'll tune by trimming tool registrations on the hot path. +- **(−) Three agent instances at module load.** Memory cost is + negligible (Pydantic AI agents are lightweight closures over their + metadata + tool dicts); flagged for completeness. +- **(−) Eval cassettes (15) need recording before + `tests/evals/chat_tutor.py` becomes useful in CI.** Replay-mode + fails loudly on missing cassettes, so this is observable rather than + silent. Recording happens out-of-band by the user with + `SAPLING_EVAL_MODE=record`. +- **(=) `services/gemini_service.py::call_gemini_multiturn` is now + dead-code on the chat happy path,** still alive as the chat fallback + target and as the quiz fallback target. A separate small PR will + delete `gemini_service.py` after the agent path proves stable in + production AND `start_session` / `action` get migrated. This matches + ADR 0001's deletion order. + +## What I'd carry into the next refactor + +- **Pre-flight "already done?" check.** Sub-agents may have run in a + previous session; check for the file artifacts and run their tests + first before dispatching parallel work that would just rebuild the + same files. +- **Encryption-aware `_load_message_history` belongs in + `services/messages.py`.** It's currently an inline helper in + `routes/learn.py`. Once a second route needs it, extract. +- **Mode-specific agent instances pattern is reusable.** Build three + agents at module load, dispatch via `agent_for_mode`. Any future + route with modal behavior gets this pattern off the shelf. +- **`_resolve_model_pref` is duplicated.** If a third caller appears, + extract to `services/model_pref.py`. Two callsites is the right + threshold to wait on. +- **The `start_session` / `action` follow-up scope split** suggests + any future refactor with N similar routes should default to + migrating one and following up on the rest, rather than bundling. + Smaller PRs ship faster. + +## Pre-existing test failures (not caused by this refactor) + +Backend baseline carries the same three failures as PR #67 and PR #71: + +- `test_skips_self_edges` — `graph_service` test hitting live Supabase + with a 409 conflict. +- `test_save_to_db` — OCR pipeline hitting live Supabase. +- `test_full_pipeline` — same OCR pipeline path. + +577 tests pass on this branch; 3 fail; no regressions caused by the +refactor. + +## Rollback + +The legacy path is intact. Rollback is one revert of the merge commit: +- `agents/chat_tutor.py` and `agents/tools/chat_context.py` disappear + (pure-leaf modules — no other code imports them). +- `agents/_providers.py` loses the `chat_tutor` task slot. +- `routes/learn.py` reverts to its pre-refactor `chat` body; the + legacy `call_gemini_multiturn` call has been intact in `_legacy_chat` + the whole time, so behavior matches `main`. +- The `messages` table schema is unchanged, so no migration to undo. + +If a partial rollback is needed (keep tools, revert the route), the +agent module is harmless to keep around — it just won't get called.