Uh oh!
There was an error while loading. Please reload this page.
refactor(learn): convert chat tutor to chat_tutor_agent (refactor #3) - #78
Merged
Conversation
Closes the third refactor in the migration plan from ADR 0001 (and the last one named in ADR 0005). The legacy `routes/learn.py::chat` path — hand-built `build_system_prompt` + `services/gemini_service.py::call_gemini_multiturn` — is replaced with three mode-specific Pydantic AI agents (Socratic / Expository / TeachBack), each sharing the same four-tool surface, with the established orchestrator-vs-legacy fallback contract. What shipped: - agents/chat_tutor.py — three Agent[SaplingDeps, str] instances built from a shared preamble + per-mode body. Prompt versions: Socratic 57f278a01d2d Expository 8c840f43b6e2 TeachBack 70a34fb09224 - agents/tools/chat_context.py — three new tools: search_course_materials, read_session_history, read_user_progress. All decryption-aware (messages.content, documents.summary, documents.concept_notes are encrypted at rest per CLAUDE.md). - agents/_providers.py — chat_tutor task slot, default gemini-2.5-pro (matches main's chat behavior post-PR #73), env override SAPLING_MODEL_CHAT_TUTOR. - 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. - tests/test_chat_tutor_imports.py (5), test_chat_context_tools.py (15), test_learn_routes.py +10 new in TestChatViaAgent class. 577 pass on this branch (3 pre-existing failures: test_skips_self_edges, test_save_to_db, test_full_pipeline — unchanged from PR #67/#71). - tests/evals/chat_tutor.py — 15 cases (5 per mode) and 5 evaluators (NonEmpty, SocraticEndsWithQuestion, ExpositoryHasStructure, TeachBackProbes, NoToolMisuse). Cassettes recorded out-of-band. Decisions: - Scope split: only `chat` migrated this PR. start_session and action share enough plumbing that bundling would have doubled the diff and bundled three independent rollbacks. Follow-up PR. - gemini_service.py NOT deleted — still alive as the chat fallback target (and the quiz fallback). Separate PR removes it after the agent path proves stable in production AND start_session/action get migrated. - Wire format unchanged: response still returns reply / graph_update / mastery_changes; messages table schema untouched. ADR 0015 captures the full rollback path and what we'd carry forward. Note: original template numbered this 0014 but PR #77's adaptive-quiz iteration claimed that slot, so this is 0015. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR implements a comprehensive refactor of the chat tutor system, replacing a legacy Gemini-based multiturn chat with a typed Pydantic AI agent framework. The change introduces three mode-specific agents (socratic, expository, teachback) backed by on-demand data-access tools, integrates the new agent into the ChangesChat Tutor Agent Refactor
Sequence DiagramsequenceDiagram
participant Client
participant Learn as learn.py<br/>POST /chat
participant Agent as chat_tutor_agent
participant Tools as chat_context tools
participant DB as Supabase
participant Legacy as _legacy_chat<br/>(fallback)
Client->>Learn: User message + mode
Learn->>DB: Load message history
DB-->>Learn: Encrypted messages
Learn->>Learn: Decrypt message history
Learn->>Agent: agent_for_mode(mode).run()
Agent->>Tools: search_course_materials_tool(query)
Tools->>DB: Fetch documents
DB-->>Tools: Encrypted results
Tools->>Tools: Decrypt, score, filter
Tools-->>Agent: Ranked materials
Agent->>Tools: read_session_history_tool()
Tools->>DB: Fetch messages
DB-->>Tools: Encrypted messages
Tools->>Tools: Decrypt, map roles, filter
Tools-->>Agent: Session history
Agent->>Tools: read_user_progress_tool()
Tools->>DB: Fetch mastery scores
DB-->>Tools: Raw scores
Tools->>Tools: Aggregate, clamp, bin
Tools-->>Agent: Progress summary
alt Agent succeeds
Agent-->>Learn: Reply string
Learn->>DB: Save encrypted messages
Learn-->>Client: Reply + empty graph_update
else Guardrail exception
Learn->>Legacy: _legacy_chat()
Legacy->>DB: Call legacy Gemini
DB-->>Legacy: Response
Legacy-->>Learn: Reply
Learn->>DB: Save messages
Learn-->>Client: Reply + empty graph_update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes This PR spans multiple interdependent subsystems (provider registration, agent implementation, tool layer, route integration) with diverse logic density: cryptographic boundary enforcement in tools, prompt-versioning via SHA-256 hashing, keyword-ranking algorithms, mode-based dispatch logic, message-role mapping with legacy fallback, and comprehensive fallback control flow in the route handler. The changes affect 15+ files across distinct concerns (configuration, agent/tool implementations, routes, tests, and documentation), and while individual tool implementations follow similar patterns (decrypt at boundary, filter, aggregate), each has distinct domain logic (scoring vs. decryption vs. aggregation). The test suite is substantial and heterogeneous, covering unit-level tool behavior, import assertions, route integration, and evaluation harness setup. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| import asyncio | ||
| from types import SimpleNamespace | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | be5b842 | Commit Preview URL Branch Preview URL | May 05 2026, 03:21 AM |
…ssion_id Two correctness fixes from the self-review on PR #78. 1. Symmetric model defaults across agent + legacy paths. When body.model_pref is None (the default), the legacy fallback used to return MODEL_DEFAULT (gemini-2.5-flash) while the agent path returned the agent's task-default (gemini-2.5-pro). A user with no explicit pref silently downgraded from Pro -> Flash on fallback. This is the same bug PR #71's commit a2fd5cd fixed for quiz; chat now matches. - routes/learn.py: _resolve_tutor_model -> _resolve_legacy_model. Default fallback flipped from MODEL_DEFAULT to MODEL_SMART. Comment block + three call sites (start_session, _legacy_chat, action) updated. - tests/test_learn_routes.py: TestResolveTutorModel -> TestResolveLegacyModel. Four tests flipped their expected return from MODEL_DEFAULT to MODEL_SMART to pin the new symmetric contract. One new test (test_default_matches_agent_default_for_no_pref) pins agent/legacy parity explicitly. 2. session_id is a declared field on SaplingDeps, not an attribute attach. Before: routes/learn.py constructed SaplingDeps without session_id and then did `deps.session_id = session_id # type: ignore`. The tool wrapper read it via `getattr(..., None)`. Worked at runtime (unfrozen dataclass), but type checkers couldn't validate it, frozen=True would silently break it, and future SaplingDeps consumers had no way to discover the seam. - agents/deps.py: added `session_id: str | None = None` with a docstring entry covering the eval/batch case (legitimately None). - routes/learn.py: pass session_id via the constructor; removed the imperative attach + the obsolete comment + the # type: ignore. - agents/tools/chat_context.py: read_session_history_tool now does a direct `ctx.deps.session_id` read; getattr ceremony gone. - tests/test_chat_context_tools.py: missing-session test sets session_id=None explicitly to mirror the declared default. 55 tests pass across the three touched test files. No regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
This was referenced May 5, 2026
4 tasks
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third and final refactor named in the ADR 0001 migration plan: convert
routes/learn.py::chatfrombuild_system_prompt+call_gemini_multiturnto a typed Pydantic AI agent with three mode-specific instances and a four-tool surface. Same orchestrator-vs-legacy fallback pattern PR #67 (documents) and PR #71 (quiz) established. Wire format unchanged, encryption boundary preserved, legacy path intact per ADR 0001.What shipped
backend/agents/chat_tutor.py— threeAgent[SaplingDeps, str]instances built from a shared preamble + per-mode body. Prompt versions:57f278a01d2d8c840f43b6e270a34fb09224backend/agents/tools/chat_context.py— three new context tools, all decryption-aware:search_course_materials— keyword overlap ondocuments.summary+concept_notesread_session_history— last N messages,messages.contentdecrypted at the boundaryread_user_progress— aggregatesgraph_nodesto mastered / weak / in-progress countsbackend/agents/_providers.py— addedchat_tutortask slot, defaultgemini-2.5-pro(matchesmain's chat behavior post-PR Restore Google sign-in popup + add tutor Fast/Smart model toggle #73), env-var overrideSAPLING_MODEL_CHAT_TUTOR.backend/routes/learn.py—_chat_via_agent(new),_legacy_chat(preserved per ADR 0001),_load_message_history(Pydantic-AIModelMessageadapter with decryption),_resolve_model_pref(mirrors quiz).chatmigrated agent-first;start_sessionandactioncarryTODO(refactor-3 follow-up)comments and remain on the legacy path for this PR.Tests —
test_chat_tutor_imports.py(5),test_chat_context_tools.py(15),test_learn_routes.py+10 new inTestChatViaAgent. 577 pass on this branch; 3 pre-existing live-Supabase failures (test_skips_self_edges,test_save_to_db,test_full_pipeline) unchanged from PR re-architecture: agentic document upload + AES-256-GCM column encryption + dev-context vault #67/refactor(quiz): convert generate_quiz to quiz_agent (refactor #2) #71. No regressions.backend/tests/evals/chat_tutor.py— 15 cases (5 per mode) and 5 evaluators:NonEmpty,SocraticEndsWithQuestion,ExpositoryHasStructure,TeachBackProbes,NoToolMisuse.docs/decisions/0015-refactor-3-chat-tutor-shipped.md— full ADR with surprises, consequences, rollback. Numbered 0015 because PR feat(quiz): adaptive iteration — spaced repetition + history + difficulty #77 (adaptive-quiz iteration) claimed 0014 between refactor Refine LLM Model selection for each function #2 ship and this work.Scope split — flagged for review
This PR migrates only
chat, notstart_sessionoraction. Both share enough plumbing withchat(same prompt assembly, same legacycall_gemini_multiturncall) that migrating all three would have doubled the route diff and bundled three independent rollback decisions. They carryTODO(refactor-3 follow-up)comments and remain on the legacy path. A follow-up PR will migrate them after the chat agent path proves stable in production.What's NOT in this PR (per ADR 0001 migration contract)
services/gemini_service.pyis NOT deleted. Still alive as the chat fallback target (and the quiz fallback). A separate small PR removes it after the agent path proves stable in production ANDstart_session/actionget migrated.Learn.tsxSSE wiring is NOT in this PR. That's sub-agent E and ships separately. The agent path uses non-streamingagent.runfor now; streaming viarun_stream_eventsis a follow-up.SAPLING_EVAL_MODE=recordrun; replay-mode CI continues to fail loudly when a cassette is missing, so neither the new nor existing cases silently no-op.References
Test plan
pytest tests/test_chat_tutor_imports.py -q→ 5 passedpytest tests/test_chat_context_tools.py -q→ 15 passedpytest tests/test_learn_routes.py -q→ 34 passed (24 prior + 10 new)pytest tests/ -q --ignore=tests/evals→ 577 passed, 3 pre-existing failures (unchanged)SAPLING_EVAL_MODE=record python tests/evals/chat_tutor.py) — recommended before mergeRollback
The legacy path is intact. Single revert of the merge commit drops the three new agent modules (
chat_tutor.py,chat_context.pyare pure-leaf), removes thechat_tutortask slot, revertsroutes/learn.py::chatto the legacy path, and themessagestable schema is unchanged so no migration to undo.🤖 Generated with Claude Code
Summary by CodeRabbit