Uh oh!
There was an error while loading. Please reload this page.
feat(tutor): graph-grounded retrieval + tool-use loop (#149) - #469
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
📝 WalkthroughWalkthroughThe chat tutor now uses compact course-scoped graph context, exposes bounded read-only graph tools, supports injectable retrieval implementations, and runs offline evaluations against fixture data with structured tool-call cassettes and new behavioral evaluators. ChangesTutor graph grounding
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
| async def course_materials( | ||
| self, course_id: str | None, query: str, limit: int, *, user_id: str | ||
| ) -> list["CourseMaterial"]: ... |
| concepts: list[str], | ||
| *, | ||
| limit: int = 20, | ||
| ) -> "GraphNeighborhood": ... |
| async def concept_mastery( | ||
| self, user_id: str, course_id: str | None | ||
| ) -> list["ConceptMastery"]: ... |
| async def progress( | ||
| self, user_id: str, course_id: str | None | ||
| ) -> "CourseProgress": ... |
| async def session_history( | ||
| self, session_id: str, last_n: int | ||
| ) -> list["SessionMessage"]: ... |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 220d6a8 | Commit Preview URL Branch Preview URL | Jul 30 2026, 10:23 AM |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
backend/tests/evals/chat_tutor.py (1)
459-478: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
isinstanceover string-based class-name matching for tool-call parts.
type(part).__name__ != "ToolCallPart"only matches parts of the exactToolCallPartclass. pydantic_ai's message types also include a distinctNativeToolCallPartfor native/provider tool-calling paths; if any of the tutor's tools were ever surfaced that way, this check would silently drop them from the cassette, undercounting tool usage thatGraphToolUsedEvaluator/MasteryUpdateEmittedEvaluatorrely on.♻️ Suggested fix
+from pydantic_ai.messages import ToolCallPart+ def _extract_tool_calls(result) -> list[ToolCall]: ... for message in messages: for part in getattr(message, "parts", []) or []: - if type(part).__name__ != "ToolCallPart":+ if not isinstance(part, ToolCallPart): continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/evals/chat_tutor.py` around lines 459 - 478, Update _extract_tool_calls to use isinstance-based recognition of pydantic_ai tool-call part types, including both ToolCallPart and NativeToolCallPart, instead of comparing type(part).__name__. Preserve the existing argument extraction fallback and ToolCall construction for every recognized tool-call part.backend/agents/tools/graph_read.py (2)
181-189: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDuplicate normalized concept names silently collapse.
by_norm[_normalize_concept(name)] = nis last-write-wins, so if the graph holds two rows normalizing to the same key (e.g. pre-dedup legacy rows), a seed resolves to whichever row came back last from PostgREST — non-deterministic across reads. Preferring the first occurrence (setdefault) at least makes it stable given a stable row order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/agents/tools/graph_read.py` around lines 181 - 189, Update the by_norm construction in the node-processing loop to preserve the first node for each normalized concept name instead of overwriting it with later duplicates. Replace the last-write-wins assignment associated with _normalize_concept(name) while leaving by_id behavior unchanged.
104-105: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClamp the LLM-chosen
limiton the upper end too.Both sites only floor at 0. A model that passes
limit=5000gets every tracked concept (and, for the neighborhood, every kept edge) back into the context window — the whole point of the cap. A module-level max (e.g. 50) keeps the tool's prompt cost bounded regardless of what the model asks for.♻️ Suggested clamp
+_MAX_LIMIT = 50+ ... - n = max(0, int(limit))+ n = min(_MAX_LIMIT, max(0, int(limit))) return rows[:n] ... - cap = max(0, int(limit))+ cap = min(_MAX_LIMIT, max(0, int(limit)))Also applies to: 244-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/agents/tools/graph_read.py` around lines 104 - 105, Clamp the LLM-provided limit to a defined module-level maximum in both row-slicing sites, including the neighborhood path referenced by the comment. Update the limit normalization around `n = max(0, int(limit))` so values above the cap are reduced while negative values still produce zero, keeping prompt results bounded.backend/agents/chat_tutor.py (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value"Hard cap" is prompt-advisory only.
Nothing enforces two graph reads per turn; the real ceiling is
TUTOR_LIMITS. Either soften the wording or enforce it in the tool wrapper (e.g. a per-run counter onSaplingDepsthat returns an empty neighborhood past the second call) so cost is bounded even when the model ignores the instruction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/agents/chat_tutor.py` around lines 70 - 73, Update the graph-read enforcement around SaplingDeps and its tool wrapper so each tutor run permits no more than two graph reads, returning an empty neighborhood after the second call; otherwise soften the prompt’s “Hard cap” wording to match the actual TUTOR_LIMITS behavior.backend/tests/test_model_mode_seam.py (1)
304-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegistered function handler is never unregistered.
register_function_handlermutates a module-level registry thatmonkeypatchwon't restore, so this handler leaks into any later test that runschat_tutorin function mode. If the file doesn't already have an autouse reset fixture, worth adding one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_model_mode_seam.py` around lines 304 - 307, Add cleanup for the module-level handler registry after tests using register_function_handler, including the test around socratic_agent.run_sync and the "chat_tutor" handler. Prefer an autouse fixture if the file lacks one, ensuring the registry is reset after each test so handlers do not leak into later function-mode tests.backend/services/graph_context.py (1)
105-111: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBudget trim rejoins the whole block each iteration.
Minor: with ≤12 lines this is irrelevant, but a running length counter (or trimming from a precomputed cumulative sum) avoids the O(n²) rejoin and reads clearer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/services/graph_context.py` around lines 105 - 111, Update the budget-trimming logic in the graph-context builder around _HEADER, lines, and GRAPH_CONTEXT_CHAR_BUDGET to track the assembled character length incrementally instead of rejoining the entire block on every loop iteration. Preserve the existing behavior of removing trailing lines until the budget fits and returning an empty string when no lines remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/routes/learn.py`:
- Around line 520-523: Update the graph-context construction at the referenced
legacy-turn paths, including the flows around lines 520, 763, and 1328, so
course-less sessions retain an appropriate user-wide compact graph fallback
instead of receiving an empty block. Avoid calling get_graph when no course_id
is available unless the fallback requires it, and ensure the serializer’s
selection remains bounded and weakest-first.
In `@backend/tests/test_live_tutor_tools.py`:
- Around line 36-38: Update
test_real_model_chooses_a_graph_reader_for_a_graph_question to accept pytest’s
monkeypatch fixture and replace the direct sys.path.insert mutation with
monkeypatch.syspath_prepend(...), ensuring the evals path is restored after the
test.
In `@docs/decisions/0021-agent-eval-harness-baselines.md`:
- Around line 46-48: The ADRs retain obsolete pre-change descriptions of
chat_tutor. In docs/decisions/0021-agent-eval-harness-baselines.md lines 46-48,
update the five-dataset wording, baseline table, and “unmeasured” consequence to
include chat_tutor; in docs/decisions/0015-refactor-3-chat-tutor-shipped.md
lines 28-33, replace the remaining four-tool and four-registration descriptions
or explicitly mark them as historical.
---
Nitpick comments:
In `@backend/agents/chat_tutor.py`:
- Around line 70-73: Update the graph-read enforcement around SaplingDeps and
its tool wrapper so each tutor run permits no more than two graph reads,
returning an empty neighborhood after the second call; otherwise soften the
prompt’s “Hard cap” wording to match the actual TUTOR_LIMITS behavior.
In `@backend/agents/tools/graph_read.py`:
- Around line 181-189: Update the by_norm construction in the node-processing
loop to preserve the first node for each normalized concept name instead of
overwriting it with later duplicates. Replace the last-write-wins assignment
associated with _normalize_concept(name) while leaving by_id behavior unchanged.
- Around line 104-105: Clamp the LLM-provided limit to a defined module-level
maximum in both row-slicing sites, including the neighborhood path referenced by
the comment. Update the limit normalization around `n = max(0, int(limit))` so
values above the cap are reduced while negative values still produce zero,
keeping prompt results bounded.
In `@backend/services/graph_context.py`:
- Around line 105-111: Update the budget-trimming logic in the graph-context
builder around _HEADER, lines, and GRAPH_CONTEXT_CHAR_BUDGET to track the
assembled character length incrementally instead of rejoining the entire block
on every loop iteration. Preserve the existing behavior of removing trailing
lines until the budget fits and returning an empty string when no lines remain.
In `@backend/tests/evals/chat_tutor.py`:
- Around line 459-478: Update _extract_tool_calls to use isinstance-based
recognition of pydantic_ai tool-call part types, including both ToolCallPart and
NativeToolCallPart, instead of comparing type(part).__name__. Preserve the
existing argument extraction fallback and ToolCall construction for every
recognized tool-call part.
In `@backend/tests/test_model_mode_seam.py`:
- Around line 304-307: Add cleanup for the module-level handler registry after
tests using register_function_handler, including the test around
socratic_agent.run_sync and the "chat_tutor" handler. Prefer an autouse fixture
if the file lacks one, ensuring the registry is reset after each test so
handlers do not leak into later function-mode tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 10240725-b7f7-4f3d-8294-03d828114d0c
📒 Files selected for processing (44)
.github/workflows/evals.ymlbackend/agents/__init__.pybackend/agents/chat_tutor.pybackend/agents/deps.pybackend/agents/tools/chat_context.pybackend/agents/tools/graph_read.pybackend/agents/tools/retrieval.pybackend/routes/learn.pybackend/services/graph_context.pybackend/services/token_overlap.pybackend/tests/evals/README.mdbackend/tests/evals/_retrieval_fixture.pybackend/tests/evals/baselines.jsonbackend/tests/evals/cassettes/chat_tutor/expository_explain_big_o.jsonbackend/tests/evals/cassettes/chat_tutor/expository_explain_dependency_injection.jsonbackend/tests/evals/cassettes/chat_tutor/expository_explain_kantian_ethics.jsonbackend/tests/evals/cassettes/chat_tutor/expository_explain_photosynthesis.jsonbackend/tests/evals/cassettes/chat_tutor/expository_explain_supply_demand.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_chemistry_balancing.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_history_themes.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_intro_calculus.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_open_followup.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_python_recursion.jsonbackend/tests/evals/cassettes/chat_tutor/socratic_stale_concept_review.jsonbackend/tests/evals/cassettes/chat_tutor/teachback_advanced.jsonbackend/tests/evals/cassettes/chat_tutor/teachback_correct_concept.jsonbackend/tests/evals/cassettes/chat_tutor/teachback_minimal.jsonbackend/tests/evals/cassettes/chat_tutor/teachback_misconception.jsonbackend/tests/evals/cassettes/chat_tutor/teachback_partial_correct.jsonbackend/tests/evals/chat_tutor.pybackend/tests/evals/fixtures/tutor_course.jsonbackend/tests/evals/run_all.pybackend/tests/test_chat_stream.pybackend/tests/test_chat_tutor_imports.pybackend/tests/test_e2e_function_handlers.pybackend/tests/test_graph_context_block.pybackend/tests/test_graph_neighborhood_tool.pybackend/tests/test_graph_tools_bugs.pybackend/tests/test_live_tutor_tools.pybackend/tests/test_model_mode_seam.pybackend/tests/test_tutor_retrieval.pydocs/decisions/0015-refactor-3-chat-tutor-shipped.mddocs/decisions/0021-agent-eval-harness-baselines.mddocs/decisions/0023-tutor-graph-retrieval-seam.md
Uh oh!
There was an error while loading. Please reload this page.
| def test_real_model_chooses_a_graph_reader_for_a_graph_question(): | ||
| sys.path.insert(0, str(Path(__file__).parent / "evals")) | ||
| from _retrieval_fixture import FixtureRetrieval |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Files matching test name:"
fd -a 'test_live_tutor_tools\.py$'.||true
test_file="$(fd 'test_live_tutor_tools\.py$'.| head -n1 || true)"if [ -n"${test_file:-}" ];thenechoecho"Relevant test file:"echo"$test_file"echoecho"Line count:"
wc -l "$test_file"echoecho"First 120 lines:"
sed -n '1,120p'"$test_file"| cat -n
fiechoecho"Search for sys.path usage in file:"if [ -n"${test_file:-}" ];then
rg -n "sys\.path|syspath_prepend|Path\(__file__\).parent""$test_file"||truefiechoecho"Search for global sys.path mutations in backend/tests:"
rg -n "sys\.path\.insert|sys\.path\.append|sys\.path\.extend|sys\.path\.remove|syspath_prepend" backend/tests ||trueechoecho"pytest availability:"
python3 - <<'PY'try: import pytest print("pytest:", pytest.__version__)except Exception as e: print("pytest import failed:", repr(e))try: import _retrieval_fixture print("module _retrieval_fixture found")except Exception as e: print("_retrieval_fixture import failed:", repr(e))PYRepository: SaplingLearn/Sapling
Length of output: 6296
Scope the import-path mutation to this test.
sys.path.insert(...) mutates the pytest process state for any following tests. Use pytest’s scoped monkeypatch.syspath_prepend(...) so the path is restored automatically after this test.
Proposed fix
-def test_real_model_chooses_a_graph_reader_for_a_graph_question():- sys.path.insert(0, str(Path(__file__).parent / "evals"))+def test_real_model_chooses_a_graph_reader_for_a_graph_question(monkeypatch):+ monkeypatch.syspath_prepend(str(Path(__file__).parent / "evals"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deftest_real_model_chooses_a_graph_reader_for_a_graph_question(): | |
| sys.path.insert(0, str(Path(__file__).parent/"evals")) | |
| from_retrieval_fixtureimportFixtureRetrieval | |
| deftest_real_model_chooses_a_graph_reader_for_a_graph_question(monkeypatch): | |
| monkeypatch.syspath_prepend(str(Path(__file__).parent/"evals")) | |
| from_retrieval_fixtureimportFixtureRetrieval |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_live_tutor_tools.py` around lines 36 - 38, Update
test_real_model_chooses_a_graph_reader_for_a_graph_question to accept pytest’s
monkeypatch fixture and replace the direct sys.path.insert mutation with
monkeypatch.syspath_prepend(...), ensuring the evals path is restored after the
test.
Uh oh!
There was an error while loading. Please reload this page.
…ion test, tool-count comment, ADR 0015 prose Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 30, 2026
Review pass complete: four reviewers, two fully clean (one empirically re-verified the before/after eval baselines by resurrecting the deleted pre-change cassettes and replaying them — exact match). Three sub-80 findings, all fixed: a /chat/stream twin of the raw-body.message persistence regression test (the invariant was correct but only the JSON route was pinned), the stale 'all five tools' comment, and the ADR 0015 blockquote that had swallowed a sentence. Backend 1441 + ruff green. e2e cycle next. |
…n the next commit; kept in PR history so the before→after delta is auditable)
The agent path inlined NO graph context (the full-graph JSON dump was legacy-only — and user-wide, leaking other courses' concepts); the tutor had write tools (#127) but no graph read surface. Now: - Tools 5 → 7: read_graph_neighborhood (course-scoped, seed-matched via _normalize_concept, depth-1 edges via two merged in.() reads with a both-endpoints-in-course rule, ids never leave the tool, truncated flag, degrade-to-empty) + read_concepts_for_user (existing reader, capped wrapper). Preamble gains the graph-tools paragraph with a hard two-reads-per-turn cap. read_misconceptions_for_course deliberately deferred (shared-context opt-out isn't enforced yet — ADR 0023). - Deterministic GRAPH CONTEXT seed block on every agent turn (services/graph_context.py: overlap-first/weakest-fill selection, depth-1 edges, ≤1.5k chars, no ids) — turn-1 graph awareness without a tool round-trip on the streaming UI; raw body.message persistence unchanged. - Legacy prompt de-dumped: all three call sites now use the same compact course-scoped serializer (also fixes the cross-course concept leak). - TUTOR_LIMITS (12/12/100k) so a legitimate multi-tool turn can't trip UsageLimitExceeded into a silent rung-1 legacy fallback. - The ADR-0021 retrieval seam: TutorRetrieval protocol on SaplingDeps (production None → byte-identical Supabase path), FixtureRetrieval + committed synthetic course, cassettes now carry tool-call traces, three new evaluators, chat_tutor joins the offline harness (16 cases; the previous commit holds the pre-change cassettes). Eval deltas (before → after): ExpositoryHasStructure 0.812→1.000, GraphToolUsed 0.938→1.000, GroundedConcept 0.812→0.875, MasteryUpdateEmitted 0.688→1.000; legacy four stay 1.000. Suites: backend 1440 passed + ruff clean (seam/stream files green under lock-pinned pydantic-ai 1.107); evals replay green across all 6 datasets; frontend untouched (tsc clean). ADR 0023; ADR 0021/0015 updated. Closes#149. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion test, tool-count comment, ADR 0015 prose Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6e1cf19 to
220d6a8CompareAndresL230
commented
Jul 30, 2026
Pre-merge gate at the rebased head (over B6's #468): full lane 28/28 passed + oracles clean. Merging. |
What
First link of the B7 seam-endgame chain (#149 → #153 → #150 → #151 → #154). The scoping pass corrected the issue's premise: the agent path inlined no graph context at all (the full-graph dump was legacy-only — and user-wide, leaking other courses' concepts), and the mid-turn write half already shipped with #127. Full detail in the commit message; the shape:
read_graph_neighborhood(course-scoped, name-seeded, depth-1, ids never leave the tool) +read_concepts_for_user, registered on the tutor (5 → 7 tools) with a hard two-reads-per-turn prompt cap. The misconceptions reader is deliberately deferred until the shared-context opt-out is enforced (ADR 0023).call_gemini*prompt sites use the same compact serializer (kills the raw JSON dump and its cross-course leak) — de-risking the rung-1 fallback ahead of [P1] Agent migration: retire call_gemini* + gemini_service.py (final cutover) #151.TUTOR_LIMITS(12/12/100k): a legitimate multi-tool turn can no longer tripUsageLimitExceededinto a silent legacy fallback.TutorRetrievalprotocol on deps (productionNone→ byte-identical), fixture retrieval + synthetic course, cassettes now record tool-call traces, and chat_tutor joins the offline eval harness (16 cases). The first commit on this branch holds the pre-change cassettes so the delta is auditable.Eval deltas (before → after): ExpositoryHasStructure 0.812→1.000 · GraphToolUsed 0.938→1.000 · GroundedConcept 0.812→0.875 · MasteryUpdateEmitted 0.688→1.000 · the four legacy evaluators hold 1.000.
Verification
Closes#149.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation