Uh oh!
There was an error while loading. Please reload this page.
fix(tutor): stop the tutor repeating itself, and let it actually read course materials - #562
fix(tutor): stop the tutor repeating itself, and let it actually read course materials#562Darkest-Teddy wants to merge 5 commits into
Conversation
`stream_agent_turn` preferred `run_result.output` over the streamed chunks. That output resolves out of the run's message list, and that list includes `message_history` — so a turn whose model response carries no text part (the model ended its turn after tool calls) handed back the PREVIOUS turn's assistant message: fully formed, non-blank, and therefore invisible to the blank-reply ladder below it. The route then persisted it, so the tutor answered a follow-up with a byte-identical copy of its own last answer — same sha256, same length, including a 1757-char reply reproduced verbatim against a completely different question. Trust `final_output` only when this turn actually streamed text. Text always arrives as PartStart/PartDelta events, so "nothing streamed" means "this turn produced no text": degrade through the existing blank-reply ladder instead of replaying history. Measured on gemini-2.5-flash-lite by replaying a real session turn: the correlation is exact — `token/reply == 0` repeated (2/8), any streamed text answered correctly (6/8). After the fix, 0 repeats in 10 runs. Note this does not stop the model from ending a turn without text; those turns now take the Rung-1 fallback or a visible interrupted-error rather than a silent duplicate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n't exist `search_course_materials` filtered `documents` on `course_id`. That table keys on `offering_id` (0025) and has no `course_id` column, so PostgREST returned 400 on every call — and the tool's deliberate degrade-silently `except` turned that into `[]`. Net effect: the tutor never read a single course document, with no error surfaced anywhere. It answered from base knowledge alone, which reads as "the tutor is generic about my class" rather than as a bug. Resolve the abstract course to the user's offerings via `academics.user_offering_ids_for_course`, per the convention that the API boundary keeps the abstract course while documents key on the offering. The #125 user_id scope is unchanged: documents stay user-scoped WITHIN a shared offering. The existing unit tests missed this because they mock `table` loosely enough to accept any filter; the new test uses a schema-faithful fake that rejects a column the table does not have, exactly as PostgREST does. The two older test classes now stub the offering lookup, which is a real dependency of this function for the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:8 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (8)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change scopes course-material searches to the user’s offerings and updates streamed-turn reply selection to avoid stale history output. Tests cover offering resolution, user scoping, document lookup, and tool-only turns. ChangesChat behavior corrections
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score:⚪ Minimal · up to The changes address two localized tutor-chat defects, with regression coverage and passing checks reported; no actionable merge-blocking risk remains beyond normal review. Possibly related PRs
Suggested reviewers: 🚥 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 4e2949d | Commit Preview URL Branch Preview URL | Aug 19 2026, 09:02 PM |
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Code review — tutor repetition + course-material retrievalTwo genuinely independent bugs, both diagnosed properly, both with regression tests written first. The FindingsP0[P0] CI is red — Not this PR's fault. The identical failure is on P1[P1] Soft-deleted documents now reach the tutor — return (
table("documents").select(
"id,file_name,summary,concept_notes",
filters={
# #125 user scope is preserved: documents are# user-scoped WITHIN a shared offering."offering_id": f"in.({','.join(offering_ids)})",
"user_id": f"eq.{user_id}",
},
order="created_at.desc",
)
[P1] The stale result=record_agent_usage(
awaitagent.run(user_message, **run_kwargs),
feature="chat_tutor", task="chat_tutor", user_id=deps.user_id,
)
reply=result.output# str — chat_tutor agents return plain Markdown.ifnotreply.strip():
[P1] Offering resolution is narrower than the writer's and than every sibling reader's — offering_ids=user_offering_ids_for_course(user_id, course_id)
ifnotoffering_ids:
return []
P2[P2] The empty-offering short-circuit is silent — the exact failure mode this PR exists to remove — ifnotoffering_ids:
return []The PR body's own indictment of the old bug is "The tutor answered from base knowledge alone, which presents as 'it's generic about my class' rather than as a failure. Nothing was logged at the user's level." This early return reproduces that precisely: no log, no metric, indistinguishable from "this course genuinely has no materials". The P3[P3] Two extra uncached PostgREST round-trips per tool call, in the streaming hot path — offs=table("course_offerings").select(
"id", filters={"course_id": f"eq.{course_id}"}
) or []
off_ids= {o["id"] foroinoffs}
ifnotoff_ids:
return []
enr=table("enrollments").select(
"offering_id", filters={"user_id": f"eq.{user_id}"}
) or []
[P3] No test pins the newly-reachable textless-turn-with-writes branch — agent=FakeAgent([
FunctionToolCallEvent("read_graph_neighborhood"),
FunctionToolResultEvent(), # no writes landedAgentRunResultEvent(PRIOR), # stale: from message_history
])The new test only covers the no-writes path (Rung-1 fallback). But the trigger it describes is "the model ended its turn after tool calls", and every tutor agent registers What's good
Verdict: request changes — the soft-delete regression and the unguarded Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy |
…le degrade Three findings on the course-materials read, all made reachable by the offering fix in b8aa904 — before it the query 400'd and returned [] on every call, so none of them could be observed. 1. `documents` is soft-deleted. routes/documents.py stamps `deleted_at` and every other reader filters on it (study_guide.py, flashcards.py); this query did not, so a file the student deleted from their Library kept getting its `summary` + `concept_notes` decrypted into LLM context forever. Adds `deleted_at is.null`, which also makes the filter set identical to PR #534's fix of the same bug — the eventual merge conflict is now trivial. 2. `user_offering_ids_for_course` is narrower than the WRITER. Documents are written with `resolve_offering(course_id, create=True)` — current term, `enrollments` never consulted — and the sibling readers use the writer's resolver too. Across a term boundary a student enrolled in Fall-26 who uploads next term gets `documents.offering_id` = the new offering, has no enrollment row for it, and the tutor silently returned [] while the Library still listed the file. The intersection bought no security either: `user_id` is the access boundary on `documents` (#125), so dropping offerings can only hide the student's OWN uploads. Widened to the union of both resolvers, order-stable for the `in.(...)` list. 3. The empty-offering short-circuit was silent — no log, no metric, indistinguishable from "this course has no materials", which is exactly the failure mode the offering fix exists to remove. It logs now, without a raw student id. Also bounds the read. The select was unbounded while every returned row gets AES-decrypted before Python truncates to `limit`, on the latency-critical SSE path. The bound is a multiple of `limit`, not `limit` itself: ranking happens after the fetch, so limiting to exactly `limit` would silently turn "most relevant" into "most recent". The new tests use a schema-faithful `table()` fake that rejects filter columns `documents` does not have. The older mocks in that file accept any filter and return a canned list, which is precisely how a query against a non-existent column survived review.
… path 5092a83 narrowed `run_result.output` inside `stream_agent_turn`, but routes/learn.py reads it in three more places and two of them run with `message_history` in `run_kwargs` — so `.output` resolves out of the same history-bearing message list, and a textless turn hands back the PREVIOUS turn's assistant message: fully formed, non-blank, and therefore invisible to the `if not reply.strip()` guard sitting right below it. That relocated the repeat rather than removing it, onto a path taking MORE traffic: `_chat_turn_json` is both POST /api/learn/chat and the streamed route's Rung-1 `nonstream_fallback`, which is exactly where the streaming fix now sends textless turns. `_action_turn` is the third reader and persists with `save_message` directly. `new_messages()` excludes the history that was passed in, so joining the TextParts of its model responses is exactly "what this turn said" — the non-streaming twin of the `joined.strip()` check in chat_stream.py. `_start_session_agent` passes `message_history=[]`, so it has no prior message to resolve back to and keeps reading `.output` directly; the comment there names why. Also adds the missing coverage for the newly-reachable streamed branch. Every tutor agent registers `apply_graph_update_tool` and `update_mastery_tool`, so a model that ends its turn after tool calls usually arrives with `deps.graph_updates` / `deps.mastery_changes` already populated — the textless-WITH-writes shape, which lands on the terminal `retryable: False` rung rather than Rung 1. That was untested. The agent-run fakes had to become shape-faithful (tests/agent_run_fakes.py): `SimpleNamespace(output=...)` has no `new_messages`, and a bare MagicMock iterates EMPTY on it — which would have silently turned every mocked turn into a "textless" one while the assertions still went green through the 502 catch-all. That is the same class of blind spot as the mock that accepted a filter on a column PostgREST does not have.
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Review fixes appliedEvery outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed. Major
Minor
Not fixed hereThe red Verification — Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate. |
Jose-Gael-Cruz-Lopez
commented
Aug 19, 2026
Verify against the real database before mergingI could not reach a live database while working on this — there are no credentials on this machine (only These are the checks that need a real connection. No schema change — but the behaviour change is data-dependentTwo fixes here only show their value against real rows, so they are worth confirming on staging. 1. How much was actually leakingThe SELECTcount(*) FROM documents WHERE deleted_at IS NOT NULL;That count is exactly how many soft-deleted documents the tutor would otherwise have decrypted into LLM context. If it is non-zero, this fix is load-bearing rather than theoretical. 2. The cross-term offering gap is real or it isn'tThe offering set is now the union of -- documents sitting on an offering the owner has no enrollment row for:-- these are exactly the files the tutor could not see before this changeSELECTcount(*) FROM documents d
WHEREd.deleted_at IS NULLAND NOT EXISTS (
SELECT1FROM enrollments e
WHEREe.user_id=d.user_idANDe.offering_id=d.offering_id);Non-zero means the divergence is already live, not just reachable at the next term boundary. 3. Sanity-check the filter set on the wireThe final query filters on exactly Static verification only — no live database was reachable from this environment. Schema model built by replaying |
Two independent bugs in the tutor chat, found while debugging "why does the AI repeat the message instead of responding to it". One commit each.
1. The tutor replayed its previous reply verbatim
stream_agent_turnpreferredrun_result.outputover the streamed chunks:.outputresolves out of the run's message list, and that list includesmessage_history. So a turn whose model response carries no text part — the model ended its turn after tool calls — handed back the previous turn's assistant message. Fully formed and non-blank, so the blank-reply ladder immediately below never saw it. The route persisted it, and the student got a byte-identical copy of the last answer.Confirmed in two live sessions two days apart: same sha256, same length, including a 1757-character reply reproduced exactly against a completely different question.
Fix: trust
final_outputonly when this turn actually streamed text. Text always arrives asPartStart/PartDeltaevents, so "nothing streamed" means "this turn produced no text" — degrade through the existing blank-reply ladder rather than replaying history.Evidence. Replaying a real session turn against
gemini-2.5-flash-lite, the correlation is exact:token/replyAfter the fix: 0 repeats in 10 runs.
What this does not fix: the model still ends ~40% of these turns without text. Those now take the Rung-1 JSON fallback (a real answer), or — if graph writes already landed — show the honest "interrupted, please retry". A silent wrong answer becomes a correct one or a visible error. Why the model does this at all is unexplored and worth a follow-up.
2. The tutor never read any course document
search_course_materialsfiltereddocumentsoncourse_id. That table keys onoffering_id(0025) and has nocourse_idcolumn, so PostgREST 400s on every call — and the tool's deliberate degrade-silentlyexceptturned that into[].The tutor answered from base knowledge alone, which presents as "it's generic about my class" rather than as a failure. Nothing was logged at the user's level.
Fix: resolve the abstract course to the user's offerings via
academics.user_offering_ids_for_course, matching the convention that the API boundary keeps the abstract course while documents key on the offering. The#125user_id scope is unchanged — documents stay user-scoped within a shared offering.Tests
Both regression tests were written first and watched fail for the right reason:
test_textless_turn_never_replays_the_previous_turns_reply— failed withon_completereceiving the stale prior-turn text.test_documents_are_fetched_by_offering_not_course_id— failed withcolumn documents.course_id does not exist.The second one needed a schema-faithful fake. The existing mocks accept any filter, which is precisely how bug 2 survived them. The two older
search_course_materialstest classes now stub the offering lookup, which became a real dependency of that function.Full backend suite: 1999 passed, 56 skipped, exit 0.
ruff checkclean on all four files.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests