Uh oh!
There was an error while loading. Please reload this page.
fix: restore mastery updates, persist graph_update_json, wire usage limits - #243
Merged
Conversation
Deploying with |
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs | frontend | e27c0f34 | Jun 22 2026, 03:37 AM |
Warning Review limit reached
Next review available in:37 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 67281192a959eefb852a2d050c3f34789742dd34 and 15b26a7. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a mastery-adjustment tool ( ChangesAgent Graph Tools and Orchestrator Limits
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/storage_service.py (1)
88-111:⚠️ Potential issue | 🟠 MajorHTTP 409 is the documented Supabase Storage response for duplicate buckets—verify if HTTP 400 fallback is still needed.
According to official Supabase Storage API documentation, a duplicate bucket returns HTTP 409 Conflict, not HTTP 400. The nested
{"statusCode":"409"}response your code anticipates is not documented. While_is_duplicate_bucket()correctly handles the documented 409 case first, the HTTP 400 fallback with nested statusCode appears undocumented. Confirm whether this is legacy behavior or an edge case; if not needed, simplify to only checkstatus_code == 409.🤖 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/storage_service.py` around lines 88 - 111, Verify whether the HTTP 400 fallback logic in the _is_duplicate_bucket function is still needed by checking current Supabase Storage API documentation. If the documented response for duplicate buckets is only HTTP 409 Conflict and the HTTP 400 fallback with nested statusCode checking is legacy or undocumented behavior that is no longer needed, simplify the _is_duplicate_bucket function to return True only when resp.status_code equals 409, removing the HTTP 400 branch entirely. If the fallback is needed for backward compatibility with older Supabase versions or edge cases, add a comment explaining why it's retained.
🧹 Nitpick comments (7)
backend/agents/tools/graph.py (1)
36-42: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using ASCII hyphen-minus in Field descriptions.
The
mastery_deltadescription uses typographic minus signs (−, U+2212) instead of ASCII hyphen-minus (-, U+002D). While visually cleaner, this can cause subtle issues if the text is ever parsed or compared programmatically.♻️ Suggested fix
mastery_delta: float = Field( description=( - "Fractional mastery change, −1.0 to +1.0. "- "Use +0.1 to +0.3 when the student answers correctly; "- "−0.05 to −0.1 when they reveal a gap or misconception."+ "Fractional mastery change, -1.0 to +1.0. "+ "Use +0.1 to +0.3 when the student answers correctly; "+ "-0.05 to -0.1 when they reveal a gap or misconception." ) )🤖 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.py` around lines 36 - 42, The mastery_delta Field description in graph.py contains Unicode minus signs (−, U+2212) instead of standard ASCII hyphen-minus characters (−). Replace all occurrences of the Unicode minus sign (−) with the ASCII hyphen-minus (-) in the description string of the mastery_delta field to ensure the text can be reliably parsed and compared programmatically without issues.Source: Linters/SAST tools
backend/routes/learn.py (1)
456-462: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDocstring is now stale.
The docstring states that
graph_update"come[s] back empty here becauseapply_graph_update_tool... already persisted any graph changes during the agent run." This is no longer accurate—the function now returns the mergedgraph_updatesfromdeps.graph_updates(lines 507-517) specifically so the caller can persistgraph_update_json.♻️ Suggested fix
async def _chat_via_agent( ... ) -> 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.+ `graph_update` contains merged payloads from all graph tools called+ during the agent run (both `new_nodes` and `updated_nodes`), enabling+ the caller to persist `graph_update_json` on the assistant message.+ `mastery_changes` remains empty (mastery deltas are in `graph_update`).🤖 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/routes/learn.py` around lines 456 - 462, The docstring for the function at lines 456-462 incorrectly states that graph_update comes back empty because apply_graph_update_tool persists changes during the agent run. This is no longer accurate since the function now returns merged graph_updates from deps.graph_updates (lines 507-517) so the caller can persist graph_update_json. Update the docstring to accurately reflect that graph_update is no longer empty and explain that the merged graph updates are returned for the caller to handle persistence, rather than being empty due to prior persistence.backend/tests/test_graph_tools_bugs.py (2)
221-221: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueOptional: Prefer
next(iter(...))over single-element slice.Static analysis suggests replacing
list(gu.keys())[0]withnext(iter(gu.keys()))to avoid creating a temporary list.♻️ Suggested refactor
- keys = [list(gu.keys())[0] for gu in ctx.deps.graph_updates]+ keys = [next(iter(gu.keys())) for gu in ctx.deps.graph_updates]🤖 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_graph_tools_bugs.py` at line 221, The list comprehension creating the keys variable uses an inefficient pattern where list(gu.keys())[0] creates a temporary list just to access the first element. Replace list(gu.keys())[0] with next(iter(gu.keys())) in the list comprehension to avoid the unnecessary temporary list creation while still retrieving the first key from the keys view.Source: Linters/SAST tools
330-331: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider narrowing the exception handler scope.
The bare
try-except-passsuppresses all exceptions, which could mask test setup failures. While the comment explains the intent (only caring about kwargs), consider one of these alternatives:
- Check
run.calledbefore the try block completes- Catch only expected exceptions (e.g., attribute errors from incomplete mocks)
- Remove the try-except and ensure mocks are complete enough to avoid exceptions
Line 333's assertion will catch if
run()wasn't called, but other setup issues might be silently suppressed.🤖 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_graph_tools_bugs.py` around lines 330 - 331, The bare `try-except-pass` block at lines 330-331 suppresses all exceptions which could hide test setup failures. Instead of catching all exceptions, either: (1) check the run.called attribute before the try block completes to verify the mock was invoked with correct kwargs, (2) catch only specific expected exceptions like AttributeError that would arise from incomplete mocks, or (3) ensure your mock objects are complete enough to not raise exceptions and remove the try-except block entirely. The goal is to only ignore exceptions directly related to kwargs validation, not all exceptions that might indicate test setup issues.Source: Linters/SAST tools
backend/routes/gradescope.py (1)
458-458: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMove uuid import to module top.
The uuid import should be at the top of the file with other imports rather than inline within the sync loop.
♻️ Proposed fix
Move the import to line 24 (after existing imports):
from datetime import datetime, timezone from typing import Any, Literal +import uuid from fastapi import APIRouter, HTTPException, RequestThen remove the inline import:
else: - import uuid- table("assignments").insert({ "id": str(uuid.uuid4()),🤖 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/routes/gradescope.py` at line 458, The uuid module is being imported inline at line 458 within the sync loop, which violates Python conventions of keeping all imports at the module level. Move the import uuid statement to the top of the file with the other imports around line 24, and then remove the inline import statement from line 458 to ensure all imports are grouped together at the module top.frontend/src/components/Gradebook/CourseCard.tsx (1)
81-105: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider memoizing disc computation for minor performance gain.
The discs array is recomputed on every render. For a small optimization, wrap the computation in
React.useMemo:constDiscsWatermark=({ seed, courseColor }: {seed: string;courseColor: string})=>{constdiscs=React.useMemo(()=>{consth=hashSeed(seed);constresult: Disc[]=[];for(leti=0;i<3;i++){result.push(discFromSeed(h,i,courseColor,result));}returnresult;},[seed,courseColor]);return(<svg ...>{discs.map((d,i)=>(<circlekey={i}cx={d.cx}cy={d.cy}r={d.r}fill={d.fill}/>))}</svg>);};This is not critical since the computation is fast and React may skip re-renders when props are unchanged.
🤖 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 `@frontend/src/components/Gradebook/CourseCard.tsx` around lines 81 - 105, The discs array computation in the DiscsWatermark component is being recalculated on every render, which is inefficient. Wrap the discs computation logic (the hashSeed call and the for loop that builds the discs array) in React.useMemo with [seed, courseColor] as the dependency array. This will memoize the result and only recompute when either of those props change, avoiding unnecessary recalculations during renders where the props remain the same.frontend/src/components/screens/Gradebook/Landing.tsx (1)
187-219: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract duplicated ErrorBanner to a shared component.
This
ErrorBannercomponent is duplicated inCourse.tsxat lines 187-219 (visible in the graph context). Extract it to a shared location such asfrontend/src/components/Gradebook/ErrorBanner.tsxto maintain a single source of truth.♻️ Suggested extraction
Create
frontend/src/components/Gradebook/ErrorBanner.tsx:"use client";importReactfrom"react";interfaceErrorBannerProps{message: string;onRetry: ()=>void;title?: string;}exportfunctionErrorBanner({ message, onRetry, title ="We couldn't load your courses."}: ErrorBannerProps){return(<divrole="alert"style={{padding: "20px 24px",borderRadius: "var(--r-md)",background: "var(--err-soft)",border: "1px solid color-mix(in oklab, var(--err) 20%, transparent)",display: "flex",gap: 16,alignItems: "center",justifyContent: "space-between",flexWrap: "wrap",}}><div><divstyle={{fontWeight: 600,color: "var(--err)",marginBottom: 4}}>{title}</div><divstyle={{fontSize: 13,color: "var(--text-dim)"}}>{message}</div></div><buttontype="button"className="btn btn--primary"onClick={onRetry}style={{padding: "8px 16px"}}>Tryagain</button></div>);}Then import and use in both Landing.tsx and Course.tsx.
🤖 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 `@frontend/src/components/screens/Gradebook/Landing.tsx` around lines 187 - 219, Extract the duplicated ErrorBanner function into a shared component file in the Gradebook components directory. Create the new shared ErrorBanner component with a title prop that defaults to "We couldn't load your courses." to make it reusable across multiple files, then import and use this shared ErrorBanner in both Landing.tsx and Course.tsx instead of defining it locally in each file.
🤖 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 `@docs/superpowers/plans/2026-06-16-bell-curve-grading.md`:
- Around line 90-103: The documentation references outdated backend schema and
route names that do not match the actual implementation. Update all references
to table names from the stale gradebook_* naming convention to use the correct
tables: assignments and user_courses. Additionally, replace any generic
gradebook URL references with the actual endpoint PATCH
/courses/{course_id}/curve. This ensures implementers follow the correct backend
contract for schema and routing.
- Around line 170-173: Update the documentation or comments in the
gradebook_service.py file to clarify that the drop-lowest operation is performed
on the raw items list before the apply_curve() function is executed. Remove or
correct any existing notes or comments that suggest the lowest-scoring item is
selected from curved scores, since the current implementation selects from raw
scores. Ensure any related test documentation in test_gradebook_service.py also
reflects this correct ordering of operations.
- Around line 275-280: The `or` operator used for assigning item_avg and
item_sd_delta treats zero as a falsy value and incorrectly discards valid 0.0
overrides, falling back to course defaults instead. Replace both `or` operators
in the assignments for item_avg and item_sd_delta with explicit None checks
(e.g., checking if the value is not None) to preserve zero-valued overrides
while still falling back to defaults only when the values are actually None or
missing from the item dictionary.
In `@docs/superpowers/specs/2026-06-16-bell-curve-grading-design.md`:
- Around line 162-173: The edge case documentation for "Drop-lowest + curve"
incorrectly describes the order of operations. Update the behaviour description
in the Edge Cases table to reflect that the drop-lowest operation is applied to
raw scores before curving is applied, not after. The note should clarify that
the lowest scores are determined from the uncurved scores, and then the curve is
applied to the remaining scores after the drop operation completes.
- Around line 90-103: The Backend columns (Supabase) section references table
names that don't match the actual implementation. Update the spec to use the
correct table names: replace references to `gradebook_assignments` with
`assignments` and `gradebook_courses` with `user_courses`. Additionally, update
the API endpoint reference in the persistence description to use
`/courses/{course_id}/curve` instead of the generic gradebook URL pattern. This
will align the design spec with the actual implemented schema and API contract.
- Around line 150-154: The applyFinalCurve function uses truthiness checks
(!course.curve_final_mean || !course.curve_final_sd) to determine if fields are
missing, but since these are nullable floats, this will incorrectly treat a
legitimate 0 value as missing and return early. Replace the condition with
explicit null and undefined checks (such as course.curve_final_mean === null ||
course.curve_final_mean === undefined) for both curve_final_mean and
curve_final_sd to properly distinguish between missing values and valid zero
values.
In `@docs/superpowers/specs/2026-06-16-grade-predictor-design.md`:
- Around line 90-93: The specification contains a contradiction: the data-flow
section states that GradeCompositionBar and GradeProjector require no interface
changes and will receive the same assignments prop, but the Files Changed table
later shows an optional isPredicted prop being added to both components. Resolve
this by choosing one approach as authoritative: either remove the isPredicted
prop from the Files Changed table and update that section to reflect no
component interface changes, or remove the "no changes to those components'
interfaces" statement from the data-flow section and clarify that both
components will accept the new optional isPredicted prop. Ensure the contract is
consistent throughout the entire specification document.
In `@frontend/src/components/Gradebook/AssignmentModal.tsx`:
- Around line 74-81: The date validation logic in the dueDateInvalid constant is
timezone-sensitive and can incorrectly reject valid dates. The current approach
of appending "T00:00:00" to the date string, creating a Date object, and then
comparing against the ISO string slice can shift the date across timezone
boundaries, causing false validation failures. Replace this logic with a simpler
approach that validates the YYYY-MM-DD format directly without timezone
conversions. Use a regex pattern to check the format (e.g., matching YYYY-MM-DD)
and then validate that it represents an actual date using a method like
Date.parse or by checking individual date components, avoiding any conversion to
ISO format or UTC.
In `@frontend/src/components/Gradebook/curveUtils.ts`:
- Around line 33-41: The curve calculation logic in the section checking null
values for points_earned, curve_class_mean, and curve_class_sd does not validate
that points_possible is greater than zero before applying the curve. Currently,
both line 37 (rawPct calculation) and line 41 (points_earned assignment) use a
fallback of 1 when points_possible is null or undefined, which can produce
fabricated curved scores for invalid assignments. Add an additional guard
condition to check that points_possible is greater than 0 and return the
original assignment data unchanged if this condition is not met, ensuring curve
math is only applied to valid assignments with positive point totals.
In `@frontend/src/components/Gradebook/EditWeightsModal.tsx`:
- Around line 53-56: The sort_order field assignment in the setDrafts function
uses arr.length which can create duplicate values after deletions or reordering
operations. Replace the sort_order value assignment from arr.length to instead
calculate the maximum existing sort_order value from the array and add 1 to it,
ensuring unique sort_order values are maintained regardless of prior
modifications to the drafts array.
In `@frontend/src/components/Gradebook/GradeProjector.tsx`:
- Around line 75-109: The current grade calculation is including categories with
no graded items as zero, which understates the overall grade. After calculating
catCurrent using dropAndSum, check if the currentItems array actually contains
any graded items (where points_earned is not null) before adding cat.weight to
weightSum and weightedCurrent. Only accumulate the weight and weighted score for
categories that have at least one graded item in the current scenario, while
still calculating floor and ceiling for all items regardless.
---
Outside diff comments:
In `@backend/services/storage_service.py`:
- Around line 88-111: Verify whether the HTTP 400 fallback logic in the
_is_duplicate_bucket function is still needed by checking current Supabase
Storage API documentation. If the documented response for duplicate buckets is
only HTTP 409 Conflict and the HTTP 400 fallback with nested statusCode checking
is legacy or undocumented behavior that is no longer needed, simplify the
_is_duplicate_bucket function to return True only when resp.status_code equals
409, removing the HTTP 400 branch entirely. If the fallback is needed for
backward compatibility with older Supabase versions or edge cases, add a comment
explaining why it's retained.
---
Nitpick comments:
In `@backend/agents/tools/graph.py`:
- Around line 36-42: The mastery_delta Field description in graph.py contains
Unicode minus signs (−, U+2212) instead of standard ASCII hyphen-minus
characters (−). Replace all occurrences of the Unicode minus sign (−) with the
ASCII hyphen-minus (-) in the description string of the mastery_delta field to
ensure the text can be reliably parsed and compared programmatically without
issues.
In `@backend/routes/gradescope.py`:
- Line 458: The uuid module is being imported inline at line 458 within the sync
loop, which violates Python conventions of keeping all imports at the module
level. Move the import uuid statement to the top of the file with the other
imports around line 24, and then remove the inline import statement from line
458 to ensure all imports are grouped together at the module top.
In `@backend/routes/learn.py`:
- Around line 456-462: The docstring for the function at lines 456-462
incorrectly states that graph_update comes back empty because
apply_graph_update_tool persists changes during the agent run. This is no longer
accurate since the function now returns merged graph_updates from
deps.graph_updates (lines 507-517) so the caller can persist graph_update_json.
Update the docstring to accurately reflect that graph_update is no longer empty
and explain that the merged graph updates are returned for the caller to handle
persistence, rather than being empty due to prior persistence.
In `@backend/tests/test_graph_tools_bugs.py`:
- Line 221: The list comprehension creating the keys variable uses an
inefficient pattern where list(gu.keys())[0] creates a temporary list just to
access the first element. Replace list(gu.keys())[0] with next(iter(gu.keys()))
in the list comprehension to avoid the unnecessary temporary list creation while
still retrieving the first key from the keys view.
- Around line 330-331: The bare `try-except-pass` block at lines 330-331
suppresses all exceptions which could hide test setup failures. Instead of
catching all exceptions, either: (1) check the run.called attribute before the
try block completes to verify the mock was invoked with correct kwargs, (2)
catch only specific expected exceptions like AttributeError that would arise
from incomplete mocks, or (3) ensure your mock objects are complete enough to
not raise exceptions and remove the try-except block entirely. The goal is to
only ignore exceptions directly related to kwargs validation, not all exceptions
that might indicate test setup issues.
In `@frontend/src/components/Gradebook/CourseCard.tsx`:
- Around line 81-105: The discs array computation in the DiscsWatermark
component is being recalculated on every render, which is inefficient. Wrap the
discs computation logic (the hashSeed call and the for loop that builds the
discs array) in React.useMemo with [seed, courseColor] as the dependency array.
This will memoize the result and only recompute when either of those props
change, avoiding unnecessary recalculations during renders where the props
remain the same.
In `@frontend/src/components/screens/Gradebook/Landing.tsx`:
- Around line 187-219: Extract the duplicated ErrorBanner function into a shared
component file in the Gradebook components directory. Create the new shared
ErrorBanner component with a title prop that defaults to "We couldn't load your
courses." to make it reusable across multiple files, then import and use this
shared ErrorBanner in both Landing.tsx and Course.tsx instead of defining it
locally in each file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9f64ab9-6cf5-4384-b462-a651963d6855
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (49)
backend/agents/chat_tutor.pybackend/agents/deps.pybackend/agents/tools/graph.pybackend/config.pybackend/db/migration_gradebook_drops.sqlbackend/db/migration_gradescope.sqlbackend/main.pybackend/models/__init__.pybackend/requirements.txtbackend/routes/auth.pybackend/routes/gradebook.pybackend/routes/gradescope.pybackend/routes/learn.pybackend/routes/quiz.pybackend/services/gradebook_service.pybackend/services/gradescope_service.pybackend/services/storage_service.pybackend/tests/test_chat_tutor_imports.pybackend/tests/test_gradebook_service.pybackend/tests/test_graph_service.pybackend/tests/test_graph_tools_bugs.pydocs/superpowers/plans/2026-06-16-bell-curve-grading.mddocs/superpowers/plans/2026-06-16-grade-predictor.mddocs/superpowers/specs/2026-06-16-bell-curve-grading-design.mddocs/superpowers/specs/2026-06-16-grade-predictor-design.mdfrontend/src/app/(shell)/settings/connections/page.tsxfrontend/src/app/api/auth/session/route.tsfrontend/src/app/globals.cssfrontend/src/components/Gradebook/AmbientOrbs.tsxfrontend/src/components/Gradebook/AssignmentList.tsxfrontend/src/components/Gradebook/AssignmentModal.tsxfrontend/src/components/Gradebook/CategoryPanel.tsxfrontend/src/components/Gradebook/CourseCard.tsxfrontend/src/components/Gradebook/EditWeightsModal.tsxfrontend/src/components/Gradebook/GradePredictorPanel.tsxfrontend/src/components/Gradebook/GradeProjector.tsxfrontend/src/components/Gradebook/GradescopeSyncModal.tsxfrontend/src/components/Gradebook/SemesterChips.tsxfrontend/src/components/Gradebook/SyllabusUploadFlow.tsxfrontend/src/components/Gradebook/categoryColor.tsfrontend/src/components/Gradebook/curveUtils.tsfrontend/src/components/ToastProvider.tsxfrontend/src/components/screens/ConnectedAccounts.tsxfrontend/src/components/screens/Gradebook/Course.tsxfrontend/src/components/screens/Gradebook/Landing.tsxfrontend/src/components/screens/Settings.tsxfrontend/src/lib/api.tsfrontend/src/lib/localData.tsfrontend/src/lib/types.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/Gradebook/CategoryPanel.tsx
Comment on lines
+90
to
+103
| def test_one_sd_below_mean(self): | ||
| # z=-1.0 → avg_target - sd_delta | ||
| result = apply_curve(0.56, class_mean=0.68, class_sd=0.12, | ||
| avg_target=0.83, sd_delta=0.10) | ||
| assert abs(result - 0.73) < 1e-9 | ||
| def test_clamp_above_100(self): | ||
| # Very high score should clamp to 1.0 | ||
| result = apply_curve(1.0, class_mean=0.50, class_sd=0.05, | ||
| avg_target=0.83, sd_delta=0.10) | ||
| assert result == 1.0 | ||
| def test_clamp_below_0(self): | ||
| # Very low score should clamp to 0.0 |
There was a problem hiding this comment.
Keep the plan aligned with the actual schema and route names.
The backend contract in this PR uses assignments / user_courses and PATCH /courses/{course_id}/curve; the gradebook_* table names and generic gradebook URL here are stale. That mismatch will send implementers to the wrong table/endpoint.
🤖 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 `@docs/superpowers/plans/2026-06-16-bell-curve-grading.md` around lines 90 -
103, The documentation references outdated backend schema and route names that
do not match the actual implementation. Update all references to table names
from the stale gradebook_* naming convention to use the correct tables:
assignments and user_courses. Additionally, replace any generic gradebook URL
references with the actual endpoint PATCH /courses/{course_id}/curve. This
ensures implementers follow the correct backend contract for schema and routing.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+275
to
+280
| if curve_mode == "curved": | ||
| item_mean = item.get("curve_class_mean") | ||
| item_sd = item.get("curve_class_sd") | ||
| item_avg = item.get("curve_avg_target") or curve_avg_target | ||
| item_sd_delta = item.get("curve_sd_delta") or curve_sd_delta | ||
| if (item_mean is not None and item_sd is not None |
There was a problem hiding this comment.
Preserve explicit zero-valued curve overrides.
Using or here will discard a valid 0.0 override for curve_avg_target / curve_sd_delta and silently fall back to the course default. Please mirror the backend's nullish check instead.
🤖 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 `@docs/superpowers/plans/2026-06-16-bell-curve-grading.md` around lines 275 -
280, The `or` operator used for assigning item_avg and item_sd_delta treats zero
as a falsy value and incorrectly discards valid 0.0 overrides, falling back to
course defaults instead. Replace both `or` operators in the assignments for
item_avg and item_sd_delta with explicit None checks (e.g., checking if the
value is not None) to preserve zero-valued overrides while still falling back to
defaults only when the values are actually None or missing from the item
dictionary.
Comment on lines
+90
to
+103
| ### Backend columns (Supabase) | ||
| - `gradebook_assignments`: add `curve_class_mean`, `curve_class_sd`, `curve_avg_target`, `curve_sd_delta` (all float, nullable) | ||
| - `gradebook_courses`: add `curve_mode` (text, default `'raw'`), `curve_avg_target`, `curve_sd_delta`, `curve_final_mean`, `curve_final_sd` (all float/text, nullable) | ||
| --- | ||
| ## UI Components | ||
| ### Raw ↔ Curved Toggle | ||
| - Sits in the course page header area, next to the Letter Scale button in the TopBar actions. | ||
| - Pill-style toggle: two segments — "Raw" and "Curved". | ||
| - Only visible when at least one assignment has curve data OR a final grade curve is set. | ||
| - Persisted via `PATCH /api/gradebook/:userId/courses/:courseId` (updates `curve_mode`). | ||
| - When switched to Curved: composition bar, grade projector, and assignment rows all re-render with curved values. |
There was a problem hiding this comment.
Align the data model section with the implemented contract.
The backend now stores these fields on assignments and user_courses, and the toggle persists via /courses/{course_id}/curve. Keeping gradebook_* and the generic gradebook URL here makes the spec internally inconsistent and easy to implement against the wrong schema.
🤖 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 `@docs/superpowers/specs/2026-06-16-bell-curve-grading-design.md` around lines
90 - 103, The Backend columns (Supabase) section references table names that
don't match the actual implementation. Update the spec to use the correct table
names: replace references to `gradebook_assignments` with `assignments` and
`gradebook_courses` with `user_courses`. Additionally, update the API endpoint
reference in the persistence description to use `/courses/{course_id}/curve`
instead of the generic gradebook URL pattern. This will align the design spec
with the actual implemented schema and API contract.
Comment on lines
+150
to
+154
| function applyFinalCurve(pct, course): number { | ||
| if (!course.curve_final_mean || !course.curve_final_sd) return pct; | ||
| const z = (pct/100 - course.curve_final_mean) / course.curve_final_sd; | ||
| return Math.max(0, Math.min(100, (course.curve_avg_target + z * course.curve_sd_delta) * 100)); | ||
| } |
There was a problem hiding this comment.
Use null checks in the final-curve pseudocode.
curve_final_mean and curve_final_sd are nullable floats, so !course.curve_final_mean will wrongly treat a legitimate 0 as missing. Spell these conditions out explicitly.
🤖 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 `@docs/superpowers/specs/2026-06-16-bell-curve-grading-design.md` around lines
150 - 154, The applyFinalCurve function uses truthiness checks
(!course.curve_final_mean || !course.curve_final_sd) to determine if fields are
missing, but since these are nullable floats, this will incorrectly treat a
legitimate 0 value as missing and return early. Replace the condition with
explicit null and undefined checks (such as course.curve_final_mean === null ||
course.curve_final_mean === undefined) for both curve_final_mean and
curve_final_sd to properly distinguish between missing values and valid zero
values.
Comment on lines
+90
to
+93
| - `isUngraded(a)` = `a.points_earned === null` | ||
| - `augmentedAssignments` is a derived value, never written to state or the backend | ||
| - `GradeCompositionBar` and `GradeProjector` receive the same `assignments` prop they already accept — no changes to those components' interfaces | ||
There was a problem hiding this comment.
Resolve the isPredicted contract contradiction.
The data-flow section says GradeCompositionBar and GradeProjector need no interface changes, but the Files Changed table later adds an optional isPredicted prop to both. Please make one section authoritative so the spec doesn't describe two different APIs.
Also applies to: 147-154
🤖 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 `@docs/superpowers/specs/2026-06-16-grade-predictor-design.md` around lines 90
- 93, The specification contains a contradiction: the data-flow section states
that GradeCompositionBar and GradeProjector require no interface changes and
will receive the same assignments prop, but the Files Changed table later shows
an optional isPredicted prop being added to both components. Resolve this by
choosing one approach as authoritative: either remove the isPredicted prop from
the Files Changed table and update that section to reflect no component
interface changes, or remove the "no changes to those components' interfaces"
statement from the data-flow section and clarify that both components will
accept the new optional isPredicted prop. Ensure the contract is consistent
throughout the entire specification document.
Comment on lines
+74
to
+81
| const dueDateInvalid = !!draft.due_date && (() => { | ||
| const d = new Date(draft.due_date + "T00:00:00"); | ||
| return isNaN(d.getTime()) || draft.due_date !== d.toISOString().slice(0, 10); | ||
| })(); | ||
| const valid = draft.title.trim() !== "" && | ||
| (draft.points_possible === null || draft.points_possible > 0); | ||
| (draft.points_possible === null || draft.points_possible > 0) && | ||
| !dueDateInvalid; |
There was a problem hiding this comment.
Date validation is timezone-sensitive and can reject valid inputs.
Line 75–77 converts a local date to UTC ISO and compares strings, which can fail for valid dates in some timezones. This can incorrectly disable Save.
Proposed fix
- const dueDateInvalid = !!draft.due_date && (() => {- const d = new Date(draft.due_date + "T00:00:00");- return isNaN(d.getTime()) || draft.due_date !== d.toISOString().slice(0, 10);- })();+ const dueDateInvalid = !!draft.due_date && (() => {+ const m = /^(\d{4})-(\d{2})-(\d{2})$/.exec(draft.due_date);+ if (!m) return true;+ const y = Number(m[1]);+ const mo = Number(m[2]);+ const d = Number(m[3]);+ const dt = new Date(Date.UTC(y, mo - 1, d));+ return (+ dt.getUTCFullYear() !== y ||+ dt.getUTCMonth() + 1 !== mo ||+ dt.getUTCDate() !== d+ );+ })();🤖 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 `@frontend/src/components/Gradebook/AssignmentModal.tsx` around lines 74 - 81,
The date validation logic in the dueDateInvalid constant is timezone-sensitive
and can incorrectly reject valid dates. The current approach of appending
"T00:00:00" to the date string, creating a Date object, and then comparing
against the ISO string slice can shift the date across timezone boundaries,
causing false validation failures. Replace this logic with a simpler approach
that validates the YYYY-MM-DD format directly without timezone conversions. Use
a regex pattern to check the format (e.g., matching YYYY-MM-DD) and then
validate that it represents an actual date using a method like Date.parse or by
checking individual date components, avoiding any conversion to ISO format or
UTC.
Comment on lines
+33
to
+41
| a.points_earned === null || | ||
| a.curve_class_mean == null || | ||
| a.curve_class_sd == null | ||
| ) return a; | ||
| const rawPct = a.points_earned / (a.points_possible ?? 1); | ||
| const avgTarget = a.curve_avg_target ?? coursePolicy.curve_avg_target; | ||
| const sdDelta = a.curve_sd_delta ?? coursePolicy.curve_sd_delta; | ||
| const curved = applyCurve(rawPct, a.curve_class_mean, a.curve_class_sd, avgTarget, sdDelta); | ||
| return { ...a, points_earned: curved * (a.points_possible ?? 1) }; |
There was a problem hiding this comment.
Guard curve math when total points are missing/invalid.
Line 37 and Line 41 currently fall back to 1 for missing totals, which can fabricate curved scores for malformed assignments. Skip curve application unless points_possible > 0.
Proposed fix
export function applyCurveToAssignment(
a: GradedAssignment,
coursePolicy: { curve_avg_target: number; curve_sd_delta: number },
): GradedAssignment {
if (
a.points_earned === null ||
+ a.points_possible == null ||+ a.points_possible <= 0 ||
a.curve_class_mean == null ||
a.curve_class_sd == null
) return a;
- const rawPct = a.points_earned / (a.points_possible ?? 1);+ const pointsPossible = a.points_possible;+ const rawPct = a.points_earned / pointsPossible;
const avgTarget = a.curve_avg_target ?? coursePolicy.curve_avg_target;
const sdDelta = a.curve_sd_delta ?? coursePolicy.curve_sd_delta;
const curved = applyCurve(rawPct, a.curve_class_mean, a.curve_class_sd, avgTarget, sdDelta);
- return { ...a, points_earned: curved * (a.points_possible ?? 1) };+ return { ...a, points_earned: curved * pointsPossible };
}📝 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.
Suggested change
| a.points_earned===null|| | |
| a.curve_class_mean==null|| | |
| a.curve_class_sd==null | |
| )returna; | |
| constrawPct=a.points_earned/(a.points_possible??1); | |
| constavgTarget=a.curve_avg_target??coursePolicy.curve_avg_target; | |
| constsdDelta=a.curve_sd_delta??coursePolicy.curve_sd_delta; | |
| constcurved=applyCurve(rawPct,a.curve_class_mean,a.curve_class_sd,avgTarget,sdDelta); | |
| return{ ...a,points_earned: curved*(a.points_possible??1)}; | |
| a.points_earned===null|| | |
| a.points_possible==null|| | |
| a.points_possible<=0|| | |
| a.curve_class_mean==null|| | |
| a.curve_class_sd==null | |
| )returna; | |
| constpointsPossible=a.points_possible; | |
| constrawPct=a.points_earned/pointsPossible; | |
| constavgTarget=a.curve_avg_target??coursePolicy.curve_avg_target; | |
| constsdDelta=a.curve_sd_delta??coursePolicy.curve_sd_delta; | |
| constcurved=applyCurve(rawPct,a.curve_class_mean,a.curve_class_sd,avgTarget,sdDelta); | |
| return{ ...a,points_earned: curved*pointsPossible}; |
🤖 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 `@frontend/src/components/Gradebook/curveUtils.ts` around lines 33 - 41, The
curve calculation logic in the section checking null values for points_earned,
curve_class_mean, and curve_class_sd does not validate that points_possible is
greater than zero before applying the curve. Currently, both line 37 (rawPct
calculation) and line 41 (points_earned assignment) use a fallback of 1 when
points_possible is null or undefined, which can produce fabricated curved scores
for invalid assignments. Add an additional guard condition to check that
points_possible is greater than 0 and return the original assignment data
unchanged if this condition is not met, ensuring curve math is only applied to
valid assignments with positive point totals.
Comment on lines
+53
to
+56
| setDrafts((arr) => [ | ||
| ...arr, | ||
| { name: "", weight: 0, sort_order: arr.length, drop_lowest: 0 }, | ||
| ]); |
There was a problem hiding this comment.
sort_order can collide after deletions/reordering.
Line 55 uses arr.length, which can duplicate existing sort_order values. Prefer max(sort_order)+1 when adding a new category.
Proposed fix
const add = () =>
- setDrafts((arr) => [- ...arr,- { name: "", weight: 0, sort_order: arr.length, drop_lowest: 0 },- ]);+ setDrafts((arr) => {+ const nextSort = arr.length+ ? Math.max(...arr.map((d) => d.sort_order)) + 1+ : 0;+ return [+ ...arr,+ { name: "", weight: 0, sort_order: nextSort, drop_lowest: 0 },+ ];+ });📝 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.
Suggested change
| setDrafts((arr)=>[ | |
| ...arr, | |
| {name: "",weight: 0,sort_order: arr.length,drop_lowest: 0}, | |
| ]); | |
| setDrafts((arr)=>{ | |
| constnextSort=arr.length | |
| ? Math.max(...arr.map((d)=>d.sort_order))+1 | |
| : 0; | |
| return[ | |
| ...arr, | |
| {name: "",weight: 0,sort_order: nextSort,drop_lowest: 0}, | |
| ]; | |
| }); |
🤖 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 `@frontend/src/components/Gradebook/EditWeightsModal.tsx` around lines 53 - 56,
The sort_order field assignment in the setDrafts function uses arr.length which
can create duplicate values after deletions or reordering operations. Replace
the sort_order value assignment from arr.length to instead calculate the maximum
existing sort_order value from the array and add 1 to it, ensuring unique
sort_order values are maintained regardless of prior modifications to the drafts
array.
Comment on lines
+75
to
+109
| const currentDS = dropAndSum(currentItems, drop); | ||
| const catCurrent = | ||
| currentDS.count > 0 ? currentDS.scoreSum / currentDS.count : 0; | ||
| // Floor scenario — ungraded = 0. | ||
| const floorItems = items.map((a) => { | ||
| const p = a.points_possible as number; | ||
| const e = a.points_earned !== null ? (a.points_earned as number) : 0; | ||
| return { score: e / p, earned: e, possible: p }; | ||
| }); | ||
| const floorDS = dropAndSum(floorItems, drop); | ||
| const catFloor = | ||
| floorDS.count > 0 ? floorDS.scoreSum / floorDS.count : 0; | ||
| // Ceiling scenario — ungraded = full marks. | ||
| const ceilingItems = items.map((a) => { | ||
| const p = a.points_possible as number; | ||
| const e = a.points_earned !== null ? (a.points_earned as number) : p; | ||
| return { score: e / p, earned: e, possible: p }; | ||
| }); | ||
| const ceilingDS = dropAndSum(ceilingItems, drop); | ||
| const catCeiling = | ||
| ceilingDS.count > 0 ? ceilingDS.scoreSum / ceilingDS.count : 0; | ||
| weightSum += cat.weight; | ||
| weightedCurrent += cat.weight * catCurrent; | ||
| weightedFloor += cat.weight * catFloor; | ||
| weightedCeiling += cat.weight * catCeiling; | ||
| } | ||
| if (weightSum === 0) return null; | ||
| return { | ||
| current: (weightedCurrent / weightSum) * 100, | ||
| floor: (weightedFloor / weightSum) * 100, | ||
| ceiling: (weightedCeiling / weightSum) * 100, |
There was a problem hiding this comment.
current grade calculation includes ungraded-only categories as zero.
Line 99 adds category weight regardless of whether any graded item exists, so current is understated versus “graded work only” semantics documented in this function.
Proposed fix
export function projectGrade(
categories: GradeCategory[],
assignments: GradedAssignment[],
): GradeProjection | null {
let weightSum = 0;
+ let currentWeightSum = 0;
let weightedCurrent = 0;
let weightedFloor = 0;
let weightedCeiling = 0;
@@
- weightSum += cat.weight;- weightedCurrent += cat.weight * catCurrent;+ if (currentDS.count > 0) {+ currentWeightSum += cat.weight;+ weightedCurrent += cat.weight * catCurrent;+ }+ weightSum += cat.weight;
weightedFloor += cat.weight * catFloor;
weightedCeiling += cat.weight * catCeiling;
}
@@
return {
- current: (weightedCurrent / weightSum) * 100,+ current: (currentWeightSum > 0 ? weightedCurrent / currentWeightSum : 0) * 100,
floor: (weightedFloor / weightSum) * 100,
ceiling: (weightedCeiling / weightSum) * 100,
};
}🤖 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 `@frontend/src/components/Gradebook/GradeProjector.tsx` around lines 75 - 109,
The current grade calculation is including categories with no graded items as
zero, which understates the overall grade. After calculating catCurrent using
dropAndSum, check if the currentItems array actually contains any graded items
(where points_earned is not null) before adding cat.weight to weightSum and
weightedCurrent. Only accumulate the weight and weighted score for categories
that have at least one graded item in the current scenario, while still
calculating floor and ceiling for all items regardless.
Jose-Gael-Cruz-Lopezforce-pushed
the
fix/graph-mastery-agent-regressions
branch
from
June 22, 2026 03:36
c6fe7a2 to
e27c0f3CompareDeploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 15b26a7 | Commit Preview URL Branch Preview URL | Jul 09 2026, 07:36 AM |
AndresL230
commented
Jul 8, 2026
Collaborator
Re-review against current |
This was referenced Jul 8, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/tests/test_graph_tools_bugs.py (1)
439-448: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winSame
_get_session_course_idpatch failure affects this test too.Line 441 patches the same non-existent
routes.learn._get_session_course_id, causingtest_agent_path_save_message_receives_graph_updateto fail in CI. Fix the patch target here as well once the correct function name is identified.🤖 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_graph_tools_bugs.py` around lines 439 - 448, The test setup is patching a nonexistent routes.learn._get_session_course_id symbol, which causes this case to fail as well. Update the patch target in test_agent_path_save_message_receives_graph_update to use the correct session-course lookup function from routes.learn, matching the actual symbol name used by the code under test, so the mock applies to the real call site.Source: Pipeline failures
🧹 Nitpick comments (2)
backend/tests/test_graph_tools_bugs.py (2)
297-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge-logic test duplicates the exact merge code from
learn.py.This test re-implements the merge loop from
_chat_via_agentrather than importing and calling the real function. If the merge logic inlearn.pychanges, this test will still pass while production breaks. Consider importing the real merge helper or testing through the route itself (astest_agent_path_save_message_receives_graph_updatedoes).🤖 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_graph_tools_bugs.py` around lines 297 - 325, The merge-logic test is duplicating the production merge behavior instead of exercising the real implementation in _chat_via_agent or the same helper used by learn.py. Update the test to call the actual merge path (or route-level flow) so it validates the production logic rather than re-creating it locally. Use the existing symbols apply_graph_update_tool, update_mastery_tool, and _chat_via_agent to locate the relevant code and ensure any future merge changes are covered by the test.
393-408: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBroad
except Exception: passhides real failures in quiz test.Swallowing all exceptions means if
_quiz_via_agentraises before callingquiz_agent.run()(e.g., aKeyErrororTypeErrorin setup), the test would still fail at line 410 (assert mock_quiz_agent.run.called) — but the root cause is obscured. If it raises afterrun()but before the assertion, the test passes silently despite a broken code path.Consider narrowing the catch or removing it if the mock setup is sufficient to prevent post-run errors.
♻️ Proposed fix: narrow the exception scope
try: _asyncio.run( _quiz_via_agent( user_id="u1", course_id="c1", concept_node_id="nid1", concept_name="Recursion", num_questions=3, difficulty="medium", use_shared_context=False, request_id="req-q", model_pref=None, ) ) - except Exception:- pass # We only care that run() was called with the right kwargs+ except (TypeError, AttributeError):+ pass # Post-run result processing may fail with mocked types🤖 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_graph_tools_bugs.py` around lines 393 - 408, The quiz test is swallowing all exceptions with a broad catch, which can hide real failures in the `_quiz_via_agent` flow and let broken paths pass unnoticed. Update the test around `_asyncio.run(_quiz_via_agent(...))` to avoid `except Exception: pass` by either removing the catch entirely or narrowing it to the specific expected exception type. Keep the assertion on `mock_quiz_agent.run` as the primary check, and ensure the setup in `test_graph_tools_bugs.py` prevents unrelated errors from being masked.
🤖 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/agents/tools/graph.py`:
- Around line 103-107: The concept name filtering in apply_graph_update still
keeps the untrimmed string, so whitespace-surrounded names can be persisted.
Update the comprehensions that build new_nodes and the related concept update
payload to normalize each name with strip() first, then filter on the trimmed
value, and use that trimmed value for concept_name everywhere it is passed from
graph.py into apply_graph_update and graph_update_json.
- Around line 49-52: The Graph event schema currently lets event_type accept any
string even though it is documented to only allow interaction, correction, or
quiz. Update the event model in graph.py where event_type is defined to enforce
those three categories, using the existing schema/model definition so invalid
labels are rejected before they reach mastery-event data.
In `@backend/tests/test_graph_tools_bugs.py`:
- Line 147: The assertion in the bug test is tautological because the `or` chain
makes it pass without checking the actual fallback behavior. Update the test
around the `result` assertion to verify the expected fallback message from the
graph tool path directly, using the existing `result` value and the surrounding
`test_graph_tools_bugs` case, so it fails unless the response actually indicates
the no-score-change / concept-may-not-exist outcome.
---
Duplicate comments:
In `@backend/tests/test_graph_tools_bugs.py`:
- Around line 439-448: The test setup is patching a nonexistent
routes.learn._get_session_course_id symbol, which causes this case to fail as
well. Update the patch target in
test_agent_path_save_message_receives_graph_update to use the correct
session-course lookup function from routes.learn, matching the actual symbol
name used by the code under test, so the mock applies to the real call site.
---
Nitpick comments:
In `@backend/tests/test_graph_tools_bugs.py`:
- Around line 297-325: The merge-logic test is duplicating the production merge
behavior instead of exercising the real implementation in _chat_via_agent or the
same helper used by learn.py. Update the test to call the actual merge path (or
route-level flow) so it validates the production logic rather than re-creating
it locally. Use the existing symbols apply_graph_update_tool,
update_mastery_tool, and _chat_via_agent to locate the relevant code and ensure
any future merge changes are covered by the test.
- Around line 393-408: The quiz test is swallowing all exceptions with a broad
catch, which can hide real failures in the `_quiz_via_agent` flow and let broken
paths pass unnoticed. Update the test around
`_asyncio.run(_quiz_via_agent(...))` to avoid `except Exception: pass` by either
removing the catch entirely or narrowing it to the specific expected exception
type. Keep the assertion on `mock_quiz_agent.run` as the primary check, and
ensure the setup in `test_graph_tools_bugs.py` prevents unrelated errors from
being masked.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ba85782-c8b9-474c-bc02-2dbc94ef795b
📥 Commits
Reviewing files that changed from the base of the PR and between c6fe7a2 and 67281192a959eefb852a2d050c3f34789742dd34.
📒 Files selected for processing (7)
backend/agents/chat_tutor.pybackend/agents/deps.pybackend/agents/tools/graph.pybackend/routes/learn.pybackend/routes/quiz.pybackend/tests/test_chat_tutor_imports.pybackend/tests/test_graph_tools_bugs.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/routes/quiz.py
- backend/tests/test_chat_tutor_imports.py
- backend/agents/chat_tutor.py
- backend/agents/deps.py
- backend/routes/learn.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| with patch("agents.tools.graph.asyncio.to_thread", side_effect=fake_to_thread): | ||
| result = _run(update_mastery_tool(ctx, update)) | ||
| assert "UnknownTopic" not in result or "processed" in result.lower() or "not exist" in result.lower() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Tautological assertion — test doesn't verify anything.
"UnknownTopic" not in result is always True because the fallback message ("Mastery update processed (1 concept(s)); no score change — concept may not exist yet...") never echoes the concept name. The or chain short-circuits on the first True, so the assertion passes unconditionally regardless of the actual return value.
🧪 Proposed fix: assert on the actual fallback message content
- assert "UnknownTopic" not in result or "processed" in result.lower() or "not exist" in result.lower()+ assert "processed" in result.lower()+ assert "no score change" in result.lower()📝 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.
Suggested change
| assert"UnknownTopic"notinresultor"processed"inresult.lower() or"not exist"inresult.lower() | |
| assert"processed"inresult.lower() | |
| assert"no score change"inresult.lower() |
🤖 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_graph_tools_bugs.py` at line 147, The assertion in the bug
test is tautological because the `or` chain makes it pass without checking the
actual fallback behavior. Update the test around the `result` assertion to
verify the expected fallback message from the graph tool path directly, using
the existing `result` value and the surrounding `test_graph_tools_bugs` case, so
it fails unless the response actually indicates the no-score-change /
concept-may-not-exist outcome.
… usage limits Three regressions introduced when routes moved from legacy <graph_update> XML parsing to Pydantic AI tools. Closes#5 — Chat-tutor agent can now raise concept mastery. The only registered graph tool was emitting new_nodes at initial_mastery 0.0 and never calling the updated_nodes branch that actually moves scores. Added update_mastery_tool (ConceptMasteryUpdate + MasteryUpdateInput) which forwards updated_nodes with mastery_delta to apply_graph_update, and registered it on all three chat-tutor agents (socratic/expository/teachback). Updated _SHARED_PREAMBLE to instruct the model when to call each tool. Closes#13 — end_session concepts_covered now populated for agent-path chats. Agent path was calling save_message() with no graph_update argument, leaving graph_update_json NULL in every message row. end_session derives concepts_covered entirely from that column, so it always returned []. Fixed by adding a graph_updates: list accumulator field to SaplingDeps; both graph tools append their payload during a run; _chat_via_agent merges and returns the combined dict; chat() passes it to save_message() so the column is populated. Closes#14 — ORCHESTRATOR_LIMITS is no longer dead code. UsageLimits(8 req / 10 tool-calls / 100k tokens) was defined in agents/__init__.py but never passed to .run(). Tool-using agents ran with no token or call ceiling. Added usage_limits=ORCHESTRATOR_LIMITS to run_kwargs in both _chat_via_agent (learn.py) and _quiz_via_agent (quiz.py). Tests: 140 tests pass (0 new failures). New test file test_graph_tools_bugs.py covers all three bugs with 14 targeted tests; test_chat_tutor_imports.py updated to expect 5 tools; pre-existing test_skips_self_edges KeyError fixed in test_graph_service.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…raph
update_mastery_tool appended {updated_nodes} to deps.graph_updates
unconditionally, so a concept the model named but that does not exist
in the graph (no changes returned by apply_graph_update) still leaked
into graph_update_json and was over-reported as concepts_covered in
end_session. Gate the append on the concepts that genuinely changed,
rebuilding updated_nodes from apply_graph_update's returned changes.
Also accumulate the real before/after deltas on deps.mastery_changes
so the route can surface them for parity with the legacy path._chat_via_agent returned mastery_changes: [] even when update_mastery_tool produced real before/after deltas during the run, leaving the agent path asymmetric with _legacy_chat (which returns apply_graph_update's changes). Echo deps.mastery_changes back to the client for parity, and correct the docstring that claimed the empty value was intentional.
Two review nits on the update_mastery_tool added in this PR: - Constrain ConceptMasteryUpdate.mastery_delta to [-1.0, 1.0] (ge/le). The score is already clamped in apply_graph_update, but the raw model delta is also written verbatim into node_mastery_events and feeds the 14-day mastery-velocity metric — an out-of-range delta would distort it. - Match the persisted-node over-report gate on the normalized concept name. apply_graph_update dedups case/whitespace-insensitively and returns the *stored* name, while updated_nodes carries the model's spelling; the exact-string gate dropped a genuinely-changed concept on casing/spacing drift, under-reporting concepts_covered in end_session. Reuse _normalize_concept on both sides. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebased onto current main (f788ed3). Adjustments the 398-commit drift required: - Tests patched routes.learn._get_session_course_id, which main renamed to _get_session_offering_id in the offering redesign. _chat_via_agent takes course_id directly and never calls it, so drop the incidental patch in test_learn_chat_via_agent_passes_usage_limits; repoint the endpoint test test_agent_path_save_message_receives_graph_update to _get_session_offering_id. This was the sole CI (Backend pytest) blocker. CodeRabbit review nits: - Constrain ConceptMasteryUpdate.event_type to Literal[interaction,correction, quiz] so invalid labels can't reach node_mastery_events. - Trim persisted concept names in both graph tools so " DFS " can't reach graph_update_json with surrounding whitespace. - Replace the tautological assertion in test_returns_fallback_message_when_no_score_change with a real check. Full backend suite: 859 passed, 1 skipped (2 pre-existing storage_service failures + 1 OCR event-loop flake are unrelated; they also fail on clean main). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AndresL230force-pushed
the
fix/graph-mastery-agent-regressions
branch
from
July 9, 2026 07:34
6728119 to
15b26a7CompareUh oh!
There was an error while loading. Please reload this page.
2 tasks
Darkest-Teddy added a commit
that referenced
this pull request
Jul 27, 2026
…ith usage limits (#345) * fix(agents): bound note_chat + remaining worker agents with usage limits (#329) Residual from #327/#243: three run-sites still executed without usage_limits, defaulting to library maximums. - note_chat now runs under ORCHESTRATOR_LIMITS; guardrail trips (UsageLimitExceeded / UnexpectedModelBehavior) degrade to an in-band reply with degraded=true instead of an uncaught 500 (no legacy fallback exists for this path per ADR 0017). - note_summary / note_concepts run under WORKER_LIMITS via a shared _run_note_worker helper that converts guardrail trips to 503. - syllabus_extraction in calendar_service now passes WORKER_LIMITS; its caller already degrades gracefully. - Tests pin the usage_limits kwarg at all four run-sites and the new degrade/503 behavior. Closes#329 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes): use noun form in summarize 503 detail (CodeRabbit nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes,calendar): separate budget trips from model bugs in agent guardrails (#329) Review fixes for the usage-limit guardrails so a deterministic budget trip and a genuine model bug are no longer conflated: - notes worker (_run_note_worker): UsageLimitExceeded -> 413 with an honest "note too long, shortening may help" detail (no transient "try again" wording); UnexpectedModelBehavior -> 500 + logger.exception so a real bug pages us with a traceback instead of hiding behind a 503/WARNING. - note_chat: UsageLimitExceeded keeps the in-band degrade (its budget wording is now accurate); UnexpectedModelBehavior -> 500. Success path now returns degraded: false for schema symmetry with the degrade path. - calendar (extract_assignments_from_file): UsageLimitExceeded degrades with an honest "syllabus too long / split it" warning; model hiccups and bare exceptions keep the generic degrade. _degraded_result gains a `warning=` override. - tests: rewrite the guardrail tests to the new contract and dedup the fake-note fixture into one module-level factory. Note: this revises behavior previously asserted by test_503_when_guardrails_trip and the parametrized note_chat degrade test — UnexpectedModelBehavior is intentionally no longer treated as a budget trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com>
AndresL230 added a commit
that referenced
this pull request
Jul 29, 2026
… (fixes staging session_expired) (#409) * feat(errors): extract FastAPI detail from thrown API errors (#361) `fetchJSON` rejects with `new Error(await res.text())`, so a FastAPI failure surfaces as an Error whose message is the raw JSON body. Add a dependency-free helper that reads the `detail` back out of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover FastAPI detail extraction (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): recover the HTTP status off a thrown error (#361) `fetchJSON` only spells the status out (`HTTP 404`) when the response body is empty, so read it from an attached `status`/`statusCode`, the parsed body, or the `HTTP <code>` message as available. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover HTTP status recovery (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): map HTTP statuses to friendly copy (#361) Add humanizeError: status-driven sentences for the cases users can act on (auth, missing, rate limit, 5xx), falling back to caller-supplied copy so it can never surface a raw body. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(api): declare the term label on EnrolledCourse (#140) /api/graph/{user_id}/courses has always returned the offering's term label; the client type never declared it, so every consumer had to cast through any to reach it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover the status-to-copy mapping (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(api): add getSemesters() for GET /api/semesters (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ui): add responsive layout primitives to globals.css (#109) Inline styles can't carry a media query, so the app's fixed multi-column shells (Admin's master/detail panes and metric row, Settings' profile field rows) get class hooks here instead. Driving them from CSS rather than `useIsMobile` also makes the first paint correct, since the hook can only flip after hydration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): prefer a human-readable server detail (#361) A FastAPI detail like "Exam not found." is better copy than generic status text, so surface it — but only when it reads like a sentence, so a serialized payload, markup or a stack can never reach the UI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin roles pane on mobile (#109) The role editor rail was pinned at `minmax(280px, 360px) 1fr` with no mobile branch, so the pane overflowed the viewport below ~640px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): scaffold the shared term helper module (#140) termRankFromLabel mirrors the sort_key formula from migration 0019 so a label-only fallback orders identically to the server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin achievements pane on mobile (#109) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): assert no raw body, markup or stack ever reaches the UI (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Admin cosmetics pane on mobile (#109) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): resolve the current term by date (#140) Mirrors services/academics.py::current_term — today within [start_date, end_date], else the highest sort_key — so client and server never disagree about which semester is current. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): reflow the Admin overview metric row on mobile (#109) Four fixed metric cards squeezed to ~75px each at 375px. Drops to a 2x2 grid below 900px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(errors): add an isNotFound predicate (#361) Lets callers branch on "that thing is gone" without string-matching a response body at the call site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): stack the Settings profile rows on mobile (#109) The username row and the display-name/bio/location/website rows were both hard-coded to `180px 1fr`, leaving ~150px for the input at 375px. They now share the `.settings-field-row` class and collapse to a label-above-control stack below 600px. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover current-term date resolution and the gap fallback (#140) Fixtures are the four terms seeded by migration 0019 verbatim, so a drift between this rule and the backend's shows up here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(errors): cover isNotFound detection (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the exam-load failure toast (#361) `String(err)` rendered the stringified FastAPI body straight into the toast. Keep the real error on the console and show a sentence instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(ui): let Dialog consumers pick the initially focused element (#109) Dialog focuses the first focusable node in the panel, which is always the close button. Form dialogs need their first field instead, and `autoFocus` loses that race — React fires it at mount, before Dialog's focus pass. Opt-in and additive; existing consumers are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): group courses by term label, most recent first (#140) Ordering keys on sort_key when the semesters payload is available and degrades to the label-derived rank otherwise. Courses with no term go to an 'Other' bucket rather than being dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the guide-load failure toast (#361) Also clear the stale guide so a failed load can't leave the previous exam's content on screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover term grouping, ordering and the unknown bucket (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): land on inline guidance when the exam is gone (#361) A missing exam is a normal state — a deleted assignment, or a stale "recent guides" entry — not a failure. Show the user where to go next instead of firing a red toast at them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move LetterScaleEditor onto the shared Dialog (#109) Drops the hand-rolled portal and its `minWidth: 360` — which overflowed a 360px viewport once the overlay's gutters were counted — for Dialog's `min(420px, 100vw - 32px)` panel. Also picks up the focus trap, Escape handling and scroll lock the hand-rolled version never had. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): partition courses into current and archive (#140) Only courses that rank strictly below the current term are archived. Undatable courses — and every course when /api/semesters gives us nothing — stay in the default list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(semesters): cover partition ordering and the no-semesters fallback (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(study): offer a retry when a guide genuinely fails to build (#361) Generation failures (502) are usually transient, so keep the message on screen next to a retry instead of leaving the user on a blank panel after the toast times out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): keep regenerate unreachable without a selected exam (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(semesters): derive ordered term labels for the gradebook chips (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the regenerate failure toast (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move EditWeightsModal onto the shared Dialog (#109) `minWidth: 520` made this the worst overflow of the four gradebook modals; it now sits in Dialog's `min(640px, 100vw - 32px)` panel. The footer wraps rather than crushing the "Total: n%" readout against the buttons on narrow screens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): humanize the flashcard delete and generate toasts (#361) Last two raw-error toasts on this screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(study): sharpen the no-exam empty-state copy (#361) Say why an exam is needed, not just that none exist — that's the whole question a user lands on this state with. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(gradebook): read term (not semester) off the courses payload (#140) /api/graph/{user_id}/courses emits `term`; the landing read `(c as any).semester`, which is always undefined. `distinct` was therefore always empty and every signed-in user silently fell through to the hardcoded SAMPLE_SEMESTERS demo chips. The sample chips are now the logged-out preview only — a signed-in user with no terms gets their own empty state instead of another student's fake grades. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move SyllabusUploadFlow onto the shared Dialog (#109) Replaces `minWidth: 460` with Dialog's fluid panel, and lets the category/assignment rows shrink (`minWidth: 0` on the flex text inputs, wrapping on the assignment rows) so the date picker can't push them past the panel edge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study-guide): make the exam-not-found detail actionable (#361) The frontend now renders a FastAPI detail verbatim when it reads like a sentence, so tell the user what to do next instead of just naming the condition. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(study-guide): pin the 404 detail as user-facing copy (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(gradebook): pin the landing chips to the courses payload term (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(gradebook): type the CourseCard test stub instead of any (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): let the guide problem outrank the generic empty hints (#361) Opening a recent guide clears the exam selection, so a missing exam would otherwise stack "No exams for this course yet" on top of the guidance explaining what actually happened. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): move AssignmentModal onto the shared Dialog (#109) `minWidth: 420` overflowed any phone viewport, and the panel had no max-height at all — with the bell-curve section expanded the footer ran off-screen with nothing to scroll. Dialog fixes both and adds the focus trap, Escape handling and scroll lock. `autoFocus` is swapped for Dialog's `initialFocusRef` so the Title field still takes focus on open rather than the close button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(study): retry the guide that actually failed (#361) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(a11y): 44px touch targets for SideNav rows (#110) `8px` vertical padding around a 15px icon left the nav links ~31px tall. Collapsed, the rail is 64px wide minus 6px padding, so the `width: 100%` link already clears 44px horizontally — only the height needed a floor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(a11y): 44px collapse/expand controls in SideNav (#110) The collapse chevron was a 24x24 target and the expand bar 28px tall. Both now match Dialog's 44x44 close button. `flexShrink: 0` keeps the collapse button square when the account name is long — the name block beside it already ellipsizes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(study): cover the missing-exam guidance and retry paths (#361) Drives the screen through the recent-guides rail — the real path to a stale exam id — and asserts a missing exam produces guidance with no toast, while a genuine failure toasts a sentence and keeps a retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(ui): make useIsMobile hydration-safe via useSyncExternalStore (#110) `useState(false)` + a `matchMedia` effect meant the value was stale for one paint after every mount, and each consumer registered its own listener. `useSyncExternalStore` pins the SSR/hydration snapshot to `false` (so server and first client render still agree, as React 19 requires) while sharing one `MediaQueryList` per breakpoint and updating as early as React allows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(gradebook): order the semester chips by the real term calendar (#140) Chips now sort by sort_key from /api/semesters and default to the date-derived current term instead of whichever term the courses payload happened to list first. A failed semesters fetch degrades to the label-derived order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(gradebook): open the term named by ?semester= (#140) Gives the dashboard archive somewhere to land: selecting an archived class opens that semester's gradebook rather than the current one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): load the term calendar alongside the graph payload (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * polish(study): stop the failure card restating its own title (#361) When no server detail survives, the body falls back to "Couldn't build that study guide" — which was the title too. Give the card a heading that pairs with any reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(ui): cover the useIsMobile SSR/hydration contract (#110) Seven cases: the server render reports desktop on a mobile viewport, hydration produces no recoverable error either way, the value flips after commit and tracks later changes, and the queried width matches the `max-width: 767px` rules globals.css relies on. Verified against a naive `useState(matchMedia(...).matches)` implementation — it fails three of them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): partition course progress into current and archive (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): hide the desktop rail pre-hydration on mobile (#110) The SSR shell always assumes desktop, so a phone painted a 232px SideNav rail until hydration swapped in TopNav. A width-based `@media` rule applies to that first frame, which no amount of hook work can reach. Pairs with the useIsMobile breakpoint, asserted in its test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(images): lazy-load and size the remote avatar images (#111) `Avatar` and `AvatarFrame` render user-supplied URLs with no intrinsic dimensions, so every one of them reserved zero space until it decoded. Explicit width/height give the browser the aspect ratio up front; the CSS `100%` sizing still wins for layout. The two `/sapling-icon.svg` logos in TopNav/SideNav are deliberately left eager — they're local, above-the-fold brand marks already sized by inline styles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(dashboard): extract CourseProgressRow from the courses panel (#140) Same markup, lifted so the current-term list, the archive and the graph overlay can all render a course line without a third copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): group the my-courses panel by semester with an archive (#140) Current-term courses show by default; earlier terms collapse behind an Archive toggle, grouped by label most recent first. Also covers the mobile 'My Courses' tab, which renders the same panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): gate the hero canvas RAF behind prefers-reduced-motion (#111) The hero projects and sorts 226 nodes and runs an O(n^2) edge pass every frame, forever. globals.css only neutralizes CSS animation, so a reduced-motion visitor was still paying for all of it. Now it paints one static frame and parks, repainting on resize (which clears the backing store) and re-arming if the preference flips mid-session. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): scope the graph courses key to the current term (#140) The floating course key now lists only current-term courses and offers past terms as a compact Archive that deep-links into each semester's gradebook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): hoist the floating-card DOM and dataset reads out of the RAF (#111) The tick re-ran `querySelectorAll('.floating-card')` and re-parsed three `dataset` floats per card on every frame. Both are static, so they move to effect setup. The loop also parks under prefers-reduced-motion, keeping each card's resting tilt but dropping the drift, mouse tilt and scroll parallax. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): cache spotlight card rects instead of measuring per mousemove (#111) `getBoundingClientRect()` on every pointer sample forces a layout flush. The rect is now taken on `mouseenter` and dropped on scroll/resize — the only things that can move a card relative to the viewport — so a sweep across a card costs one measurement, not one per sample. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api): carry the HTTP status on failed requests (#361) fetchJSON discarded the status, so a FastAPI failure — which always has a JSON body — reached callers with no status at all. isNotFound had to infer "missing" from the words "not found", which would silently regress into a red toast the day someone reworded a server message. ApiError keeps `message` as the raw body, so existing callers that stringify or read `.message` are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(courses): group the manage-courses list by semester (#140) Headings only appear once a student has courses in more than one term. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(landing): rAF-throttle the landing scroll handler (#111) `onScroll` wrote inline styles on the hero, the nav and the ambient glow on every scroll event, which fire well above frame rate. Coalesced to one write per frame; the mousemove and scroll listeners are also marked passive since neither calls preventDefault. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): scope the graph legend chips to the current term (#140) Keeps the top-nav legend consistent with the courses key overlay, which already lists only the current semester. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(dashboard): cover semester grouping, archive routing and degradation (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(dashboard): wire the archive toggle to its region for assistive tech (#140) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(gradebook): smoke-cover the four modals moved onto Dialog (#109) The migration is invisible to tsc — a modal that stops opening, loses its Cancel handler, or drops its accessible name still typechecks. These four had no tests at all, so the swap was landing unverified. Also pins initial focus landing on the title field rather than Dialog's close button, which is the specific reason initialFocusRef exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(lint): prune the suppression the Landing fix made stale (#140) Reading `term` instead of `(c as any).semester` removed the only no-explicit-any in Landing.tsx, so its suppression entry no longer matches anything. eslint exits 2 on a stale suppression even with zero errors, which fails the CI lint gate — `main` exits 0, this branch did not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(frontend): make DEPLOY_ENV the single source of truth for env config Staging login bounced to /?error=session_expired: the worker serving staging.saplinglearn.com ran with production config (BACKEND_URL= api.saplinglearn.com), so sign-in round-tripped through the prod backend and came back as a prod-signed .saplinglearn.com cookie that staging's middleware rejected under its own SESSION_SECRET. The deployGuard check that would catch a consistent-but-wrong-target build only arms when DEPLOY_ENV is set, and it wasn't set on either Workers Build. - deployGuard: add resolveFrontendEnv (derive apiUrl/cookieDomain from FRONTEND_ENVS when DEPLOY_ENV is set; fall back to explicit vars otherwise) plus expectedEnvForHost/detectHostConfigMismatch. Unit-tested. - middleware: derive API_URL via the resolver; on a protected route, flag a host/backend mismatch with a loud log + distinct `env_misconfig` code instead of the misleading `session_expired`. - session route: derive cookie Domain from the resolver. - next.config: derive build-time BACKEND_URL/NEXT_PUBLIC_API_URL/COOKIE_DOMAIN from DEPLOY_ENV. - wrangler.toml: set DEPLOY_ENV for [vars] and [env.staging.vars]. - SignInModal: user copy for env_misconfig. - docs: ADR 0020 (root cause + required deploy follow-up). Note: this hardens the repo but does not fix the running deployment — that needs a staging redeploy with DEPLOY_ENV=staging + `wrangler deploy --env staging` and the correct route binding (see ADR 0020). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(deploy): stop the build-command footgun that took staging down ADR 0020's operational follow-up told operators to set a `wrangler deploy --env staging` line and a DEPLOY_ENV build variable, but never said to keep the Build command as `npm run cf:build`. Wiring that up, the frontend-staging Workers Build's *build-command* field got overwritten with `npx wrangler deploy --env staging` — a deploy command in the build slot. That skips `opennextjs-cloudflare build`, so `.open-next/` is never produced and every build failed with "Could not find compiled Open Next config" (~16 red builds across all branches since 2026-07-20). Verified locally: `npm run cf:build` produces `.open-next/worker.js` (the `main` wrangler deploys); `npx wrangler deploy --env staging` alone does not. - ADR 0020: split the two Workers Builds fields explicitly, mandate the Build command stay `npm run cf:build`, and forbid putting a deploy command in it. - wrangler.toml: document the same Build vs Deploy field distinction at the point of configuration. The live fix is still a one-field dashboard revert (Build command back to `npm run cf:build`); this stops the docs from steering anyone into it again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(frontend): data-testid convention on six core E2E surfaces (#382) (#410) The browser suite (#385) needs stable selectors. Today shipped code has zero data-testid attributes, so Playwright would have to anchor on CSS classes (utility-ish, non-unique) or copy — both churn on every design pass. Adds a kebab-case `<surface>-<element>` convention, applies it to the six surfaces Chapter-1 drives (sign-in, approval gate, upload modal, tutor composer, quiz answer flow, graph container), and gates drift with a per-file ESLint rule. - docs/frontend-testids.md documents the naming rules, how repeated/list items are disambiguated (stable domain id first, render index as the fallback), the full current inventory, and how to onboard a new surface. - Testids land on the file that actually renders the element, which is not always the screen file: the tutor composer lives in ChatPanel.tsx (single consumer: screens/Learn.tsx) and every quiz control lives in QuizPanel.tsx (screens/Quiz.tsx only mounts it). - eslint.config.mjs gets a `no-restricted-syntax` block scoped to those six files: any <button>/<input>/<textarea> there without a data-testid is an error. Deliberately not repo-wide — the rest of the app has no browser coverage to protect. Attributes and lint config only; no behavior, styling, or logic changes. The SignInModal.tsx edit is strictly additive (open PRs #409/#359 touch that file). Closes #382 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(backend): keyless rag_service import + hermetic LLM egress guard (#411) #378 — services/rag_service.py built a module-level genai.Client with api_key=os.getenv("GEMINI_API_KEY", ""), and genai.Client(api_key="") raises ValueError at construction. That broke `import main` outright without a key (routes/quiz.py and routes/learn.py both pull the module in). Fall back to "dummy-key-for-import" the way services/gemini_service.py and agents/_providers.py already do: imports stay clean and the failure moves to call time, where it is actionable. No behaviour change when a real key is present. #379 — add the autouse `_hermetic_llm_transport` fixture to tests/conftest.py, the LLM sibling of `_hermetic_supabase_client`. It patches the google-genai transport CLASS (google.genai._api_client.BaseApiClient) rather than client instances, so every already-constructed module-level client is covered: gemini_service, rag_service, and pydantic-ai's GoogleProvider. Unstubbed calls now raise UnstubbedLLMEgress("unstubbed LLM egress: ...") instead of making a real, billable request. Unary, streaming, sync, async and the File API side channels are all blocked, and the fixture fails loudly if google-genai ever moves the seam rather than silently degrading to a no-op. Exemptions mirror the existing guards (e2e_staging, integration) plus a new `live_llm` marker for the three deliberately-live tests in test_ocr_pipeline.py. Their existing `_requires_gemini` skipif is invisible to `get_closest_marker`, so a real marker was required; the skipif still keeps them from running without a key. Verified: full suite 987 passed / 5 skipped / 1 pre-existing error (test_ocr_pipeline::test_save_to_db, unchanged from main); CI-equivalent lane 929 passed / 5 skipped; ruff clean; keyless `import main` succeeds. Closes #378 Closes #379 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test(backend): cookie-minting test-auth endpoint for local/test envs (#381) (#412) * test(backend): cookie-minting test-auth endpoint for local/test envs (#381) `GET /api/auth/dev-login` was removed and real Google OAuth is not headless-automatable, so pytest and Playwright had no sanctioned way to obtain an authenticated session. Unify the duplicated minter: - New `backend/services/session_tokens.py` owns the one implementation of the `<payload_b64>.<sig_b64>` format `auth_guard._decode_session` verifies, plus the canonical `SESSION_COOKIE_NAME`. - `db/e2e_staging_http.py` and `tests/integration/conftest.py` now use it instead of carrying verbatim copies; the OAuth-callback redirect handoff token in `routes/auth.py` uses it too (byte-identical output, TTL passed explicitly). `auth_guard` reads the cookie name from it. - `tests/test_auth_session_contract.py::_mint` stays an independent re-implementation on purpose: it pins the wire format from the outside. Add `POST /api/auth/test-login`: - Sets the `sapling_session` cookie with the same attributes as the real session AND returns the token in the body, so Playwright global setup can inject it via `context.addCookies()`. - Hard-gated on `APP_ENV in {"local", "test"}` — narrower than `config.IS_LOCAL`, which also covers `development`/`dev`. - The gate is evaluated per request off the live `config` module attribute and returns a stock 404 `{"detail": "Not Found"}` everywhere else, for every request shape (the body is parsed by hand so FastAPI's pre-handler 422 cannot disclose the route). `include_in_schema=False` keeps it out of /openapi.json in all environments. - No DB access: it does not create users or grant approval/roles. 47 new tests cover the production 404, the request-time gate, the real auth_guard round-trip, and byte-identical minting. Closes #381 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(auth): assert test-login mounting via router.routes, not app.routes `test_route_exists_but_is_gated` walked `client.app.routes` looking for `/api/auth/test-login`. How an included APIRouter flattens into the composed app's route list is not a stable API: under the pinned fastapi 0.138 / starlette 1.3 (CI) the sub-router contributes no `.path` entries there, so the set comprehension silently found nothing and the assertion failed — while every behavioural test against the same endpoint passed, because the route itself was mounted and serving correctly. Assert against `auth_module.router.routes` instead, which is a flat list of APIRoute objects with stable `.path` values across both versions. This keeps the test's original purpose: proving the 404 comes from the environment gate rather than from a route that was never mounted. Caught by CI; the local venv runs fastapi 0.136 / starlette 1.0, where the old introspection happened to work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): wave-2 handoff for epic #402 subcutaneous lane (#414) Session prompt for the next wave (#391, #397, #398), committed so a cloud session can pick it up from the repo rather than needing it pasted in. Records what wave 1 established and what it cost to learn: the baseline test counts and the one pre-existing OCR error not to chase, the shadowed grep/find, the missing venv/.env in fresh worktrees, why `env -u GEMINI_API_KEY pytest` can never work, and the local-vs-requirements.lock version skew that made a locally-green test fail CI. Also states the engineering constraints this lane turns on -- assert through a different layer than the one that wrote, make a test fail before trusting it, never weaken a hermetic guard to get green, and treat #398's findings as the deliverable rather than a blocker. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): add skills + autonomy guidance to the wave-2 handoff (#415) * docs(e2e): add skills + autonomy guidance to the wave-2 handoff The handoff covered environment traps and engineering constraints but said nothing about which skills to reach for or how independently to run, so a session picking it up would default to neither. Splits the tooling by what actually resolves where: /sync-context, the context-curator agent, /recall, /log-decision and /log-attempt are committed under .claude/ and work anywhere, while the superpowers and code-review skills are local plugins that may not exist in a cloud session -- those are listed conditionally with a manual fallback for the review fan-out. Calls out that CLAUDE.md already requires /sync-context before agent-building work, which #391 is, and that context-curator is meant to run before touching LLM integration. Adds an autonomy section: execute the wave without asking permission for reversible work, own CI failures rather than reporting a red PR as done, and never end a turn on a plan instead of doing it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): make code review gate the merge, not trail it The handoff put /code-review at the end of the wave, after every PR had already merged. That ordering cannot prevent a bad change from landing -- it can only document one after the fact. Wave 1 was run this way and got lucky: the review found nothing above threshold, but anything it had found would already have been on main. Makes review a per-PR merge gate alongside CI, with every finding addressed or explicitly dismissed with a reason. Keeps a wave-end pass, but reframes it as covering interactions between merged PRs rather than as the only review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(e2e): fix false test claim + add destructive-truncate guardrail Review of PR #415 surfaced two real defects in the handoff: - Claimed all four tests in test_local_stack.py assert via table(); only two do. The other two assert on the app's HTTP response. Corrected so an agent doing find-and-replace isn't misled about the current shape. - #397's autouse truncate runs on a direct psycopg connection over SUPABASE_DB_URL, but the only local guard checks SUPABASE_URL, a separate var. .env.staging and .env.production both hold live direct-Postgres strings. Added a non-negotiable requirement to assert SUPABASE_DB_URL is local and fail loudly before any truncate, so an unsupervised run can't silently wipe real data. Same guardrail added to issue #397 and its acceptance criteria. Also flags the psycopg-in-tests pattern as a deliberate test-only exception to the table()-only rule, so a literal reader doesn't stall on the conflict or treat it as licence for psycopg in app code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(agents): SAPLING_MODEL_MODE FunctionModel test seam (#391) (#416) * feat(agents): SAPLING_MODEL_MODE FunctionModel test seam (#391) model_for() now dispatches on SAPLING_MODEL_MODE (default 'real', so production and the hermetic unit lane are unchanged): - real → GoogleModel, still honoring the per-task SAPLING_MODEL_<TASK> override from ADR 0008. - function → pydantic-ai FunctionModel bound to a per-task handler tests register via register_function_handler(). Scripted tool calls run through the real tool registration, arg-schema validation, and retry loop. - cassette → reserved (issue scope) but raises NotImplementedError. - anything else → ValueError (a typo'd mode never silently bills Gemini). The FunctionModel substitutes ABOVE the #379 transport guard: it never builds a google.genai request, so a function-mode run needs no hermetic exemption and runs clean in the default lane. Tests pin that invariant (rides-above-guard + the real-mode counter-check that still trips it). AC: an integration-style test drives note_chat_agent with a FunctionModel and asserts on the LLM-chosen search_course_materials_tool arguments after schema validation; a classifier test proves the retry loop runs for real. +13 tests, no regressions (976 → 989 passed in the CI-ignore lane). ADR 0019 records the decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX * refactor(agents): review polish on the model-mode seam (#391) Self-review follow-ups, no behavior change: - annotate model_for/_function_model_for as -> Model (the pydantic-ai base) instead of GoogleModel + type: ignore — function mode genuinely returns a FunctionModel, so the honest supertype removes the type lie. - drop the unused unregister_function_handler and ModelMode alias to keep the seam's public surface to just register/clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX --------- Co-authored-by: Claude <noreply@anthropic.com> * test(backend): integration fixtures — psycopg raw-SQL seam, truncate isolation, seeded users (#397) (#417) The integration lane existed but only round-tripped through PostgREST both ways (testing the echo, not the DB) or asserted on the app's own JSON. This adds the raw-SQL seam the lane was missing and the fixtures #398 builds on: - db_conn: session-scoped psycopg connection on SUPABASE_DB_URL (dict rows, autocommit) — the raw-SQL assertion seam. Writes go through the app; reads come back through this, never through table(). - _require_local_db_url: the non-negotiable safety gate. SUPABASE_DB_URL is independent of the SUPABASE_URL that _require_local_stack checks, and .env.staging/.env.production hold live direct-Postgres strings, so the truncate could wipe a real project. The gate parses the host (strict, so 127.0.0.1.evil.com is rejected) and RAISES — never skips — on non-local. - _reset_between_tests: autouse truncate of every mutable table + reseed of the rich baseline before each test, making the suite order-independent. The denylist preserves the migration-seeded reference layer + catalog hierarchy (verified to carry no FK to users, so no CASCADE can reach it). - seeded_user factory (distinct approved users) and authed_client / other_user_client, replacing the per-test cookies.set boilerplate. test_local_stack.py is refactored onto the fixtures: the flagship test POSTs a note through the app and asserts the ciphertext at rest via raw SQL; a truncate -isolation pair proves ordering-independence; a distinct-users test and a seeded_user test cover the new fixtures. The safety gate is proven in the DEFAULT hermetic lane (tests/test_integration_ db_guard.py, pure URL logic, no DB) so it gates every PR: +13 tests there (976 → 989), the 9 DB-backed tests skip without RUN_INTEGRATION. No regressions. Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX Co-authored-by: Claude <noreply@anthropic.com> * test(backend): migration order pins, encryption round-trip suite, e2e→subcutaneous rename (#398) (#418) Partial delivery of the subcutaneous write-path suite — the pieces provable or low-risk without a running stack: - test_migrations.py (default lane, VERIFIED): pins the runner's apply order. The 0021 pair is load-bearing — 0021_gradebook.sql CREATEs `assignments` and 0021_gradebook_curve.sql ALTERs it to add curve_* columns, so gradebook MUST apply first. sorted(glob()) does exactly that ('.' 0x2E < '_' 0x5F). This also corrects the issue comment, which claimed the sort yields "gradebook_curve before gradebook" — it does not; the pin guards against a rename flipping it. - tests/integration/test_encryption_roundtrip.py: reads every encrypted column from the seeded baseline via the #397 raw-SQL seam and asserts ciphertext at rest + decrypt round-trip across text (decrypt_if_present), numeric (decrypt_numeric, assignments.points_*), and JSON (decrypt_json, sessions.summary_json) — the "silent decrypt regression" sentinel. - tests/integration/test_migrations_ledger.py: the DB-backed half of the migration check (schema_migrations records every file on disk). - Renamed test_e2e_staging.py → test_subcutaneous_staging.py (it drives HTTP routes below the UI; not a browser E2E). Marker `e2e_staging` unchanged. Default lane: +5 verified migration tests (1002 → 1007), no regressions. The integration files are marked `integration` and skip without RUN_INTEGRATION. Remaining #398 scope (test_postgrest_semantics, test_constraints, test_authz_real_rows, and the actual run-to-find-bugs) needs the local stack and is tracked as a follow-up — #398 stays open. Claude-Session: https://claude.ai/code/session_018b3MtMEu2htdGVBGRcrmzX Co-authored-by: Claude <noreply@anthropic.com> * feat(ocr): transcribe text-layer-less pages with Gemini vision Scanned and photographed handwritten coursework carries no text layer, so there are no characters to copy out. Docling's OCR is meant to cover this but crashes on such documents -- `Stage preprocess failed for run 1, pages [13]: std::bad_alloc` -- and the error is swallowed, so the page comes back empty. Docling *does* flag those pages in `fallback_pages`, but the only consumer of that signal was gated behind `OCR_ENGINE=auto` + `GOT_OCR_ENABLED`, and the default engine is `docling`. So in practice the signal was computed and discarded, and a 13-page handwritten practice final extracted to "" -- which then reached the classify prompt as an empty `Content:` block and was answered with an invented summary. Add a Gemini-vision backend that transcribes a rendered page image, and wire it to that existing signal. Deliberately NOT gated on `OCR_ENGINE=auto`, since that gate is precisely why the rescue never fired for real uploads. Chosen over the alternatives for handwritten maths specifically: Tesseract is poor at handwriting, and GOT-OCR needs a ~2GB weight download and is impractical CPU-only. Gemini already backs every other AI path here, and returns LaTeX. Verified end to end on the document that triggered this, with OCR_ENGINE at its default: 0 chars -> 4,507 chars, including row reductions, characteristic polynomials, and \boxed answers. Off by default (`GEMINI_VISION_OCR_ENABLED`): it costs one LLM call per flagged page. Pages with a normal text layer are never flagged, so a text PDF costs nothing. Per-page failures keep whatever Docling produced for that page -- a partial document beats none -- while an unavailability error aborts the loop rather than burning a failed call for every page of a long scan. Also: - extract the OCR cache key into `_ocr_cache_key` and include the new flag, so enabling vision cannot serve the empty string cached from before it was on - correct the comment claiming OCR is deterministic. It no longer is, which matters for content-addressed chunk ids (ADR 0019): two students uploading the same scan only dedup to one embedding if they transcribe identically. Persisting OCR output content-addressed rather than merely caching it is the real fix, and is not attempted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(observability): activate Logfire ops/error/LLM tracing (#119) (#406) * feat(observability): activate Logfire ops/error/LLM tracing (#119) Turn on Logfire safely and document it. The SDK was already configured (logfire.configure + instrument_pydantic_ai + the scrub_value scrubber), but two gaps kept the success criteria unmet: - instrument_fastapi was never called, so no FastAPI request traces would appear even with a token set. Wire it in main.py. - Enabling FastAPI instrumentation introduces a content-egress path the scrubber cannot reach: OTel records parsed endpoint arguments (request body + params) under `fastapi.arguments.values`, which Logfire does not route through scrub_value (a field named e.g. `body` matches no risky pattern). Drop those arguments at the source via a request_attributes_mapper that returns None, keep headers off (capture_headers=False), and keep the extra argument/endpoint spans off (extra_spans=False). No prompts, completions, chat messages, note bodies, quiz answers, or uploaded document text leave the process on request spans. Also: - Add LOGFIRE_TOKEN to .env.example (optional; dormant when unset via send_to_logfire="if-token-present") and surface it through config.py. - Document Logfire in docs/observability-logging-tracking.md: what it captures vs the owned Supabase events/llm_usage tables (independent, no double-count), how to enable, what is scrubbed, and the in-scope query-string caveat. - Tests: AST guards that fail if the argument-dropping mapper / header / span flags regress, plus an end-to-end test asserting a request body never lands in any exported span. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(flashcards): stop rate-limit retry-after overshooting the window check_rate_limit computed `int(_RATE_WINDOW_SEC - elapsed) + 1`, which returns 61 when the limited calls land in the same clock tick (elapsed == 0) — one second past the 60s window, and it tripped test_sixth_call_returns_retry_after (`assert 61 <= 60`). Use math.ceil of the true remaining time instead: it still rounds a sub-second remainder up to 1 (never 0) but is bounded by the window, so retry-after is always in [1, _RATE_WINDOW_SEC]. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(agents): bound note_chat orchestrator + remaining worker agents with usage limits (#345) * fix(agents): bound note_chat + remaining worker agents with usage limits (#329) Residual from #327/#243: three run-sites still executed without usage_limits, defaulting to library maximums. - note_chat now runs under ORCHESTRATOR_LIMITS; guardrail trips (UsageLimitExceeded / UnexpectedModelBehavior) degrade to an in-band reply with degraded=true instead of an uncaught 500 (no legacy fallback exists for this path per ADR 0017). - note_summary / note_concepts run under WORKER_LIMITS via a shared _run_note_worker helper that converts guardrail trips to 503. - syllabus_extraction in calendar_service now passes WORKER_LIMITS; its caller already degrades gracefully. - Tests pin the usage_limits kwarg at all four run-sites and the new degrade/503 behavior. Closes #329 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes): use noun form in summarize 503 detail (CodeRabbit nit) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ori1maMbbFjkpCS7jPgjj * fix(notes,calendar): separate budget trips from model bugs in agent guardrails (#329) Review fixes for the usage-limit guardrails so a deterministic budget trip and a genuine model bug are no longer conflated: - notes worker (_run_note_worker): UsageLimitExceeded -> 413 with an honest "note too long, shortening may help" detail (no transient "try again" wording); UnexpectedModelBehavior -> 500 + logger.exception so a real bug pages us with a traceback instead of hiding behind a 503/WARNING. - note_chat: UsageLimitExceeded keeps the in-band degrade (its budget wording is now accurate); UnexpectedModelBehavior -> 500. Success path now returns degraded: false for schema symmetry with the degrade path. - calendar (extract_assignments_from_file): UsageLimitExceeded degrades with an honest "syllabus too long / split it" warning; model hiccups and bare exceptions keep the generic degrade. _degraded_result gains a `warning=` override. - tests: rewrite the guardrail tests to the new contract and dedup the fake-note fixture into one module-level factory. Note: this revises behavior previously asserted by test_503_when_guardrails_trip and the parametrized note_chat degrade test — UnexpectedModelBehavior is intentionally no longer treated as a budget trip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * ci: re-trigger frontend-staging Workers build Empty commit to re-run CI after the frontend-staging Workers build-command config fix (npm run cf:build restored; the broken wrangler deploy --env staging skipped the OpenNext build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com> * feat(frontend): test environment profile with same-origin API proxy (#380) (#421) Add build:test / start:test npm scripts that produce a production Next build targeting the local stack with ALL API traffic same-origin through the Next /api/:path* rewrite to the local FastAPI on :5000: - NEXT_PUBLIC_API_URL is set explicitly EMPTY so every client fetch is same-origin and the sapling_session cookie always rides along (the landing page falls back to cross-origin http://localhost:5000 when the var is merely unset). - BACKEND_URL=http://localhost:5000 bakes the rewrite destination and satisfies next.config.ts's production-build guard. - Local Supabase URL + demo anon key are inlined so the lazy lib/supabase.ts client initializes instead of throwing. - start:test supplies the runtime side: BACKEND_URL for the middleware session check and the fixed local SESSION_SECRET for the session route. All values are the committed-safe local defaults from .env.local.example, inlined in the scripts (real process env beats .env* files in Next, so the profile is deterministic regardless of a dev's .env.local). Zero new dependencies; middleware.ts and the production `npm run build` are untouched. Recipe documented in docs/local-supabase.md. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * refactor(ocr): route vision transcription through a Pydantic AI agent The vision OCR call built a raw genai.Client and invoked generate_content directly. Three reasons that is wrong here, the third load-bearing: - CLAUDE.md: new LLM-driven code belongs in backend/agents/ as a Pydantic AI agent, not a fresh client. - ADR-0008 made agents/_providers.py::model_for(task) the one place a model is chosen. GEMINI_VISION_OCR_MODEL was a competing knob that bypassed it; the slot is now SAPLING_MODEL_OCR_VISION like every other agent's. - Cost attribution. Logfire's instrument_pydantic_ai() tags every pydantic-ai span with tokens and USD; a raw client call is invisible to it and to the usage capture #118/PR #375 is building. Vision OCR is one metered call per scanned page — plausibly the largest per-document LLM spend in the app, and it would have been the one call the new cost dashboard could not see. The run is bounded by WORKER_LIMITS: it sits in a per-page loop, where an unbounded run multiplies a single runaway page across the whole document. Also fixes a latent bug this refactor surfaced. _extract_text_or_422 is sync but called from both async handlers (routes/documents.py:640, :771), so a bare asyncio.run raises there — and _apply_gemini_vision_fallback's per-page `except Exception: continue` would have swallowed it, silently turning vision OCR into a no-op on the main upload path. _run_from_anywhere hands the coroutine to a worker thread when a loop is already running, copying the context so agent.override and the active span survive. The module contract is unchanged: same function name and signature, same GeminiVisionUnavailableError semantics, GEMINI_VISION_OCR_ENABLED still the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ocr): cache key, cost ceiling, sequential rescuers, accurate docs Four findings from the review of #420. Cache key omitted the model. _ocr_cache_key claimed to include "every flag that changes the output" but not the vision model, so switching models kept serving the old transcription for the full 30-day TTL. Model and page cap are now in the key, mixed in only when vision is enabled so the vision-off majority keeps its existing entries. GOT_OCR_MODEL_PATH has the same pre-existing gap; the docstring now names it instead of overclaiming. No cost ceiling. Each flagged page is one metered call, and nothing upstream bounds the count: routes/extract.py allows min(max_pages, 50) and the upload path has no rate limit at all. The #182 limit (10 req/60s) was sized when a request meant one bounded local OCR run. GEMINI_VISION_OCR_MAX_PAGES caps it per document, default 10, and logs how many pages it left behind — a silent cap reads downstream as a full transcription. if/elif made the rescuers mutually exclusive. Enabling both meant vision never ran, including on pages GOT-OCR failed to fill, recreating the exact "signal computed then dropped" bug this feature exists to fix. They now run in sequence — GOT-OCR first (local, free), then vision over what it could not fill. Both share one driver; GOT-OCR's gate is byte-for-byte unchanged. Three false claims. .env.example said an unreadable scan "is rejected" — it is not on this base; the upload paths convert only extraction *exceptions* to 422, so "" reaches the classify prompt and the model fabricates. That rejection is PR #419, still open. The module docstring said vision applies to "any engine"; it needs Docling to have run and succeeded. And the cache comment cited ADR 0019 (actually the SAPLING_MODEL_MODE test seam) for content-addressed chunk ids, whose dedup claim is untrue on main and becomes true only under PR #352. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(frontend): NEXT_PUBLIC_TEST_MODE determinism flag (#383) (#422) New src/lib/testMode.ts exports IS_TEST_MODE (build-time inlined), random() (mulberry32-seeded drop-in for Math.random), and now() (frozen 2026-03-11T12:00:00Z clock seam, overridable via globalThis.__SAPLING_TEST_NOW__). With the flag on: - KnowledgeGraph2D seeds its initial node positions and takes the reduced-motion path (synchronous fixed-tick settle) so two loads render identical coordinates. - KnowledgeGraph3D forces cooldownTicks=0 (the reduced-motion seam). - Landing page point cloud + floating cards park their rAF loops on a deterministic static frame; the frame's time read goes through now(). - AtmosphericBackdrop paints one still frame with seeded orbs. - HowItWorks/Study set framer-motion MotionGlobalConfig.skipAnimations. - Dashboard freezes the quote to index 0 and routes greeting, week strip, and relative labels through now(); Calendar (dueLabel, cursor, today) and Notetaker (relTime) do the same. Flag off, every seam passes through to Math.random()/Date.now() and no rAF/motion gate changes: production behavior is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * test(infra): one-command local stack boot — make e2e-up / e2e-down (#384) (#423) Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: run the integration lane on every push to main (#402) (#427) The subcutaneous suite (#396–#398) currently runs only on manual workflow_dispatch — a real-DB lane that never runs protects nothing. Per epic #402's open decision 3 (lean: main-only first, promote to a PR gate once #388's stability bar holds), trigger it on every push to main while keeping manual dispatch. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci: gate test_extraction_service.py — it needs none of the OCR stack The CI pytest step ignored four files. Three genuinely need what requirements.lock deliberately excludes: transformers (test_extraction_backends), docling (test_docling_integration), live network (test_ocr_pipeline). test_extraction_service.py needs none of them — it stubs every backend it exercises. It was swept into the list with its heavy neighbours, and the consequence is that nothing in it has ever gated a PR: not the OCR engine gating, not the content-addressed cache key (#97), and not the cost ceiling and rescuer sequencing added alongside this change. #420's own fallback and cache-key tests were ungated for the same reason. Verified against the locked (non-OCR) dependency set CI actually installs, using CI's exact command and env: 1069 passed, 23 skipped, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): Playwright harness and fixtures (#385) (#428) * test(e2e): Playwright harness and fixtures (#385) Browser-lane foundation for epic #402 — #386/#387/#392–#395 build on this. - frontend/playwright.config.ts: chromium-only, workers=1 (serial to start), retries=2 gated on CI, trace/video/screenshot on failure, JSON reporter (e2e/results/last-run.json) with per-attempt retry indices for #390 flake tracking, timezoneId pinned to America/New_York for the frozen #383 clock. No webServer block: the boot contract belongs to make e2e-up (#384); global-setup fails fast with the exact fix when the stack is down. - e2e/global-setup.ts: health-check the stack, mint a session for rich-user-active via POST /api/auth/test-login (#381) through the same-origin proxy, persist as storageState. - e2e/support/db.ts: the single DB seam — pg over 127.0.0.1:54322 (loopback-exact guard, mirroring #397), TRUNCATE mutable tables RESTART IDENTITY CASCADE with the #397 denylist, re-seed via the canonical db/seed_local_rich.py. - e2e/support/fixtures.ts: auto fixture resets the DB before each test; specs import test/expect from here. - e2e/smoke.spec.ts: one harness proof (authed /dashboard renders app-shell), deliberately not a journey. - build:test now bakes NEXT_PUBLIC_TEST_MODE=1 (the #383 flag; this composition is what it was built for). - ShellFrame: data-testid="app-shell" on both layout variants — the stable authed-shell anchor per the #382 convention. Verified against a cold make e2e-up boot: npx playwright test green twice in a row (truncate/re-seed isolation holds), tsc --noEmit, eslint, vitest (204 passed), and a plain production build all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): review fixes — testid process + comment accuracy (#385) - Follow docs/frontend-testids.md 'Adding a surface' for app-shell (missed in the initial commit): App shell row in the owning-files table, an `app` inventory section noting ShellFrame.tsx and the smoke-spec anchor role, and ShellFrame.tsx added to the eslint no-restricted-syntax scope (passes clean — the frame renders no intrinsic button/input/textarea). Doc's 'six files' phrasing generalized now that the list has seven. - global-setup.ts: correct the cookie-flags comment — auth.py only sets Secure under an https FRONTEND_URL (config.py), so the local cookie is HttpOnly/Lax; we mint secure:true and Chromium accepts it on http://localhost. - smoke.spec.ts: correct both redirect comments — unauthed /dashboard goes to ${BACKEND_URL}/api/auth/google via the middleware (BACKEND_URL is always set under start:test), not to the landing page. Verified: npx tsc --noEmit clean; npx eslint . 0 errors with ShellFrame.tsx newly in scope (scoped run at --max-warnings=0 clean); vitest 204/204. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(ocr): send the transcription prompt in the user turn, not as system Caught by the first real Gemini call anyone has made against this feature. Moving the instruction to `system_prompt` during the agent refactor changed what the model produces. Measured on a rasterized syllabus with known ground truth (231 chars of source text, 0-char text layer): prompt as system_prompt -> 743 chars: \documentclass{article}, five \usepackage lines, \begin{document}, a tabular, \end{document} prompt in the user turn -> 359 chars: clean Markdown table Both transcribe the facts correctly — every assignment, date and type matches. The difference is that as a system prompt, "Use LaTeX for mathematics" reads as a document-format directive rather than an instruction about math notation, so the model emits a whole LaTeX file. The preamble is not cosmetic. extracted_text feeds the classify, summary and concept prompts and is chunked into course_chunks for RAG, so "amsmath" and "booktabs" become candidate concepts on a graph shared by every student in the course — the same pollution this feature exists to prevent, arriving by a different door. Restores the wire shape the original raw-client implementation used (contents=[image, prompt]), verified to produce 358 chars of clean Markdown on the same fixture. The agent seam, the ADR-0008 model slot and the cost attribution are all unaffected — only the placement changes. The test now pins placement in the user turn and asserts the instruction is absent from any system prompt. Revert-proof: reintroducing system_prompt fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): journey — study room with two browser contexts (#394) (#431) Two signed-in contexts (rich-user-active + rich-user-second), one seeded room. Both contexts assert receipt of the other's message through the real propagation path — Supabase Realtime postgres_changes signal + decrypting REST re-fetch (#124) — and both users' knowledge graphs render. Zero waitForTimeout: cross-context sends only happen after each context's postgres_changes subscription is server-confirmed ("Subscribed to PostgreSQL" frame). Unblocking migrations (both verified-needed at runtime on the local migrations-only schema): - 0032: add the rooms columns routes/social.py already selects (topic/course/owner_id/updated_at/is_public) — bug #405 made every room listing endpoint 500 (verified: PostgREST 42703); columns stay nullable/unpopulated, the create_room semantics remain open in #405. - 0033: publish room_messages on supabase_realtime (guarded, idempotent) — verified empty publication locally; without it postgres_changes never fire, and the chat has no polling fallback. Harness additions (additive): e2e/support/session.ts mints a second user's storageState (cookie + the sapling_user localStorage identity that UserContext requires) via POST /api/auth/test-login; USER_SECOND joins stack.ts; global-setup.ts takes the #386 branch's localStorage fix verbatim so sibling PRs converge on identical content. Social.tsx joins the #382 data-testid convention (social-* inventory in docs/frontend-testids.md, eslint files array). Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * fix(ocr): per-run provider — every second vision call died on a closed loop Found by the live test added here, which is the only thing that could have found it: every other test in this feature substitutes the model, and a FunctionModel has no client and no event loop. Measured against the live API, calling the seam four times in one process: call 1: OK 302 chars call 2: RuntimeError: Event loop is closed call 3: OK 308 chars call 4: RuntimeError: Event loop is closed `_providers._provider` is a module-level GoogleProvider, so its async httpx client binds to the first loop `asyncio.run` creates and dies when that loop closes. Every `run_agent_sync` caller shares this — it is #354, and the sweep is still open in PR #358. Transcription is the only caller that runs in a LOOP, which turns a latent bug into an unusable feature: a 10-page scan alternates success and failure page by page, and `_apply_gemini_vision_fallback`'s per-page `except Exception: continue` keeps Docling's text without a word. Half a document silently degrades to the mangled OCR this feature exists to replace. So this path does not wait for #358. `fresh_ocr_vision_model()` builds a provider per run and is passed as a per-run `model=` override, leaving the shared `_provider` untouched so it cannot conflict with whatever #358 lands. It returns None outside SAPLING_MODEL_MODE=real, where the FunctionModel has no loop affinity and must not be overridden. Four consecutive live calls now pass. The fixture is an image-only math worksheet. A missing text layer alone is not enough to reach vision — Docling ships RapidOCR and reads rasterized prose fine. This page is reached because `_detect_math_without_latex` flags math-shaped content carrying no LaTeX, the scanned-math case the feature is for. Docling alone drops problem 3 entirely as `<!-- formula-not-decoded -->`; with vision it comes back as `$\sqrt{x^2 + 16} \leq 5$`. Tests live in the `live_llm` lane, not tests/integration/: they need Docling and a real model, not Postgres, and that lane's conftest mandates a running Supabase stack. Opt-in via RUN_LIVE_OCR=1 plus a real key; skipped otherwise, so CI's dummy key is a clean skip. One test guards the premise and fails loudly if Docling ever stops flagging the fixture, since the other two would then pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): journey — seeded session → dashboard (#386) (#429) * test(e2e): journey — seeded session → dashboard (#386) Co-Authored-By: Claude Fable 5 <norepl…
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
3 participants
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
Three regressions introduced when routes moved from legacy
<graph_update>XML parsing to Pydantic AI tools. Allthree are now fixed with tests.
end_sessionconcepts_covered always empty for agent-path chatsORCHESTRATOR_LIMITSis dead code, agents run with no token/call ceilingWhat changed
#5 — Mastery update tool (HIGH)
agents/tools/graph.pyonly had one tool that emittednew_nodeswithinitial_mastery: 0.0. Theupdated_nodesbranch in
graph_service.apply_graph_update— the only code that actually moves a mastery score — was unreachablefrom any agent tool.
Fix: Added
update_mastery_toolwithMasteryUpdateInput/ConceptMasteryUpdatemodels. Registered on allthree chat-tutor agents.
_SHARED_PREAMBLEtells the model to call it after any turn where the student demonstratesunderstanding (+0.1–+0.3) or reveals a gap (−0.05–−0.1).
#13 — graph_update_json persisted for agent path (MEDIUM)
chat()calledsave_message()with nograph_updateargument, leavinggraph_update_json = NULL.end_sessionderives
concepts_coveredentirely from that column, so it always returned[].Fix: Added
graph_updates: listaccumulator toSaplingDeps. Both graph tools append their payload during a run._chat_via_agentmerges and returns the combined dict.chat()passes it tosave_message().#14 — ORCHESTRATOR_LIMITS wired (MEDIUM)
UsageLimits(8 req / 10 tool-calls / 100k tokens)was defined but never passed to.run(). Tool-using agents ranwith no ceiling.
Fix:
usage_limits=ORCHESTRATOR_LIMITSadded torun_kwargsin_chat_via_agentand_quiz_via_agent.Test plan
test_graph_tools_bugs.py— 14 new tests (mastery delta forwarding, accumulator, merge logic, usage_limits inkwargs, graph_update_json persisted, end_session concepts_covered)
test_chat_tutor_imports.py— updated to expect 5 tools (was 4)test_graph_service.py— fixed pre-existingKeyError: 'graph_edges'intest_skips_self_edgesSummary by CodeRabbit
New Features
Bug Fixes