Uh oh!
There was an error while loading. Please reload this page.
fix(documents): reject empty text extraction instead of fabricating a summary - #419
Merged
Merged
Conversation
Warning Review limit reached
Next review available in:7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds minimum extracted-text validation to document uploads, introduces a resumable multi-step onboarding experience on the landing page, and centralizes frontend deployment environment resolution through ChangesDocument processing safeguards
Landing-page onboarding flow
Deployment environment resolution
Migration file decoding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LandingPage
participant OnboardingFlow
participant ProfileAPI
User->>LandingPage: start or resume onboarding
LandingPage->>OnboardingFlow: show active step
OnboardingFlow-->>LandingPage: submit completed answers
LandingPage->>ProfileAPI: submit onboarding profile
ProfileAPI-->>LandingPage: persistence response
LandingPage-->>User: show outro and navigate to dashboard
sequenceDiagram
participant Browser
participant Middleware
participant DeployGuard
participant SessionRoute
Browser->>Middleware: request protected route
Middleware->>DeployGuard: resolve environment and check host/backend
DeployGuard-->>Middleware: match or env_misconfig
Middleware->>SessionRoute: use resolved cookie domain for session operations
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 4f06eba | Commit Preview URL Branch Preview URL | Jul 29 2026, 09:40 AM |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
backend/tests/test_documents_routes.py (1)
1261-1291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the exact 49/50-character boundary.
The tests reject a short sample and accept a long document, but do not lock the advertised cutoff. Add route tests for 49 stripped characters returning 422 and exactly 50 returning success.
🤖 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_documents_routes.py` around lines 1261 - 1291, The document route tests currently miss the exact minimum extraction-length boundary. Add tests alongside test_rejects_extraction_below_minimum that submit 49 stripped characters and assert status 422, then submit exactly 50 characters and assert a successful response, reusing the existing upload and mocking helpers.frontend/src/app/auth/callback/page.tsx (1)
97-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared
'sapling_onboarding_pending'literal is hardcoded in four places. The onboarding handoff contract is expressed as a bare string at every read/write site, so a rename or typo silently breaks resume with no type or build error. Export a single constant (e.g.ONBOARDING_PENDING_KEYfrom@/lib/…) and import it everywhere.
frontend/src/app/auth/callback/page.tsx#L97-L98: replace the literal insessionStorage.setItemwith the shared constant.frontend/src/components/SignInModal.tsx#L169-L174: replace the literal in the not-onboardedsetItembranch.frontend/src/context/UserContext.tsx#L168-L170: replace the literal in thesignOutremoveItemcall.frontend/src/app/(public)/page.tsx#L577-L584: replace the literal instartOnboarding, and in the resume effect'sgetItem/removeItempair.🤖 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/app/auth/callback/page.tsx` around lines 97 - 98, Define and export a shared ONBOARDING_PENDING_KEY constant, then import and use it for every onboarding pending sessionStorage access. Update frontend/src/app/auth/callback/page.tsx:97-98, frontend/src/components/SignInModal.tsx:169-174, frontend/src/context/UserContext.tsx:168-170, and frontend/src/app/(public)/page.tsx:577-584; replace each literal in setItem, getItem, and removeItem while preserving the existing onboarding flow.
🤖 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 `@frontend/src/app/`(public)/page.tsx:
- Around line 577-584: Clear the `sapling_onboarding_pending` session-storage
flag when the sign-in modal is dismissed without authentication. Update the
sign-in modal close/cancel handler, rather than `startOnboarding`, and preserve
the existing pending flow when sign-in succeeds.
- Around line 627-656: Update the submitOnboardingProfile error path in the
onboarding completion handler so a failed save is surfaced to the user and exits
or pauses the completion flow before the outro animation and dashboard redirect
begin. Preserve the existing success animation and redirect only after the
profile submission resolves successfully, and allow the user to remain in or
retry the onboarding flow.
In `@frontend/src/app/globals.css`:
- Around line 661-690: Remove the duplicate `@keyframes` definitions for
ob-pulse-outer, ob-pulse-inner, and ob-card-in in globals.css, retaining the
later intended definitions as the single source of truth and preserving their
transform and opacity behavior.
In `@frontend/src/components/OnboardingFlow.tsx`:
- Around line 202-219: Update the OnboardingFlow component’s final-step
submission path to track a local submitting state, set it before invoking
onFinish, and gate handleNext so repeated clicks are ignored while submission is
in flight. Bind the same state to the Launch Sapling button’s disabled property,
while preserving normal navigation behavior for earlier steps.
- Around line 277-298: Update the full-screen overlay in OnboardingFlow’s
returned JSX to include dialog semantics with role="dialog" and aria-modal, and
add an accessible aria-label to the icon-only close button using onClose. When
visible is false, mark the overlay inert and aria-hidden so its form and
controls are not keyboard-reachable, while preserving the existing visibility
and pointer-event behavior.
In `@frontend/src/components/SignInModal.tsx`:
- Around line 169-174: Update the non-onboarded branch in the sign-in completion
flow to navigate to "/" after setting "sapling_onboarding_pending" and before
closing the modal. Keep the existing "/dashboard" redirect for completed
onboarding unchanged, ensuring sign-in from any page reaches the landing-page
resume flow.
In `@frontend/src/lib/deployGuard.ts`:
- Around line 62-85: Replace the prototype-traversing `deployEnv in
FRONTEND_ENVS` check in `resolveFrontendEnv` with an own-property check, while
preserving the existing derived configuration behavior for recognized frontend
environments and fallback behavior for all other values.
In `@frontend/src/middleware.ts`:
- Around line 53-69: Update the misconfiguration message in the middleware
mismatch branch guarded by detectHostConfigMismatch to recommend the canonical
npx wrangler versions upload workflow established by the deployment
configuration and ADR, rather than wrangler deploy --env. Preserve the existing
diagnostic context and route-binding guidance.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1261-1291: The document route tests currently miss the exact
minimum extraction-length boundary. Add tests alongside
test_rejects_extraction_below_minimum that submit 49 stripped characters and
assert status 422, then submit exactly 50 characters and assert a successful
response, reusing the existing upload and mocking helpers.
In `@frontend/src/app/auth/callback/page.tsx`:
- Around line 97-98: Define and export a shared ONBOARDING_PENDING_KEY constant,
then import and use it for every onboarding pending sessionStorage access.
Update frontend/src/app/auth/callback/page.tsx:97-98,
frontend/src/components/SignInModal.tsx:169-174,
frontend/src/context/UserContext.tsx:168-170, and
frontend/src/app/(public)/page.tsx:577-584; replace each literal in setItem,
getItem, and removeItem while preserving the existing onboarding flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53ec3e56-3abd-4eaf-b342-d1aa2034845d
📒 Files selected for processing (19)
backend/db/migrate.pybackend/routes/documents.pybackend/tests/test_documents_routes.pydocs/decisions/0020-deploy-env-single-source-of-truth.mddocs/superpowers/followups/2026-06-30-token-unification-followups.mddocs/superpowers/specs/2026-07-16-onboarding-cutover-design.mdfrontend/Dockerfilefrontend/next.config.tsfrontend/src/app/(public)/page.tsxfrontend/src/app/api/auth/session/route.tsfrontend/src/app/auth/callback/page.tsxfrontend/src/app/globals.cssfrontend/src/components/OnboardingFlow.tsxfrontend/src/components/SignInModal.tsxfrontend/src/context/UserContext.tsxfrontend/src/lib/deployGuard.test.tsfrontend/src/lib/deployGuard.tsfrontend/src/middleware.tsfrontend/wrangler.toml
💤 Files with no reviewable changes (1)
- docs/superpowers/specs/2026-07-16-onboarding-cutover-design.md
Comment on lines
577
to
584
| function startOnboarding() { | ||
| if (!userReady) return; | ||
| if (!isAuthenticated) { | ||
| setSignInError(null); | ||
| setSignInOpen(true); | ||
| sessionStorage.setItem('sapling_onboarding_pending', '1'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sapling_onboarding_pending leaks when the user dismisses sign-in.
Clicking "Get Started" while signed out writes the flag unconditionally. If the user closes the modal without authenticating, the flag survives in the tab and a later sign-in (e.g. via the navbar, for an already-onboarded account) will auto-launch onboarding on the landing page. Clear it when the sign-in modal closes without success.
♻️ Proposed fix
<SignInModal
open={signInOpen}
- onClose={() => { setSignInOpen(false); setSignInError(null); }}+ onClose={() => {+ setSignInOpen(false);+ setSignInError(null);+ if (!isAuthenticated) sessionStorage.removeItem('sapling_onboarding_pending');+ }}
errorCode={signInError}
/>🤖 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/app/`(public)/page.tsx around lines 577 - 584, Clear the
`sapling_onboarding_pending` session-storage flag when the sign-in modal is
dismissed without authentication. Update the sign-in modal close/cancel handler,
rather than `startOnboarding`, and preserve the existing pending flow when
sign-in succeeds.
Comment on lines
+627
to
+656
| try { | ||
| await submitOnboardingProfile({ | ||
| user_id: userId, | ||
| first_name: formData.firstName, | ||
| last_name: formData.lastName, | ||
| year: formData.year, | ||
| majors: formData.majors, | ||
| minors: formData.minors, | ||
| course_ids: formData.course_ids, | ||
| learning_style: formData.style as OnboardingProfilePayload['learning_style'], | ||
| }); | ||
| } catch (e) { | ||
| console.error('Failed to save onboarding profile:', e); | ||
| } | ||
| introTimeoutsRef.current.forEach(clearTimeout); | ||
| zoomActiveRef.current = true; | ||
| zoomOutroRef.current = false; | ||
| setOutroText('hidden'); | ||
| setOutroOverlay(false); | ||
| setOnboardingPhase('complete'); | ||
| introTimeoutsRef.current = [ | ||
| setTimeout(() => setOutroText('in'), 1400), | ||
| setTimeout(() => { | ||
| setOutroText('out'); | ||
| zoomOutroRef.current = true; | ||
| }, 3050), | ||
| setTimeout(() => setOutroOverlay(true), 3450), | ||
| setTimeout(() => { router.replace('/dashboard'); }, 4250), | ||
| ]; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A failed profile save still runs the success outro and redirects to /dashboard.
submitOnboardingProfile errors are swallowed into console.error, then the completion animation runs and the user lands on the dashboard with onboarding_completed still false — the exact trap the comment above says this change was meant to fix, just silently. Surface the failure and keep the user in the flow (or retry) instead of proceeding.
🛡️ Proposed fix sketch
} catch (e) {
console.error('Failed to save onboarding profile:', e);
+ setOnboardingError('We could not save your profile. Please try again.');+ return; // stay in `active`; let the user retry the final step
}📝 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
| try{ | |
| awaitsubmitOnboardingProfile({ | |
| user_id: userId, | |
| first_name: formData.firstName, | |
| last_name: formData.lastName, | |
| year: formData.year, | |
| majors: formData.majors, | |
| minors: formData.minors, | |
| course_ids: formData.course_ids, | |
| learning_style: formData.styleasOnboardingProfilePayload['learning_style'], | |
| }); | |
| }catch(e){ | |
| console.error('Failed to save onboarding profile:',e); | |
| } | |
| introTimeoutsRef.current.forEach(clearTimeout); | |
| zoomActiveRef.current=true; | |
| zoomOutroRef.current=false; | |
| setOutroText('hidden'); | |
| setOutroOverlay(false); | |
| setOnboardingPhase('complete'); | |
| introTimeoutsRef.current=[ | |
| setTimeout(()=>setOutroText('in'),1400), | |
| setTimeout(()=>{ | |
| setOutroText('out'); | |
| zoomOutroRef.current=true; | |
| },3050), | |
| setTimeout(()=>setOutroOverlay(true),3450), | |
| setTimeout(()=>{router.replace('/dashboard');},4250), | |
| ]; | |
| try{ | |
| awaitsubmitOnboardingProfile({ | |
| user_id: userId, | |
| first_name: formData.firstName, | |
| last_name: formData.lastName, | |
| year: formData.year, | |
| majors: formData.majors, | |
| minors: formData.minors, | |
| course_ids: formData.course_ids, | |
| learning_style: formData.styleasOnboardingProfilePayload['learning_style'], | |
| }); | |
| }catch(e){ | |
| console.error('Failed to save onboarding profile:',e); | |
| setOnboardingError('We could not save your profile. Please try again.'); | |
| return;// stay in `active`; let the user retry the final step | |
| } | |
| introTimeoutsRef.current.forEach(clearTimeout); | |
| zoomActiveRef.current=true; | |
| zoomOutroRef.current=false; | |
| setOutroText('hidden'); | |
| setOutroOverlay(false); | |
| setOnboardingPhase('complete'); | |
| introTimeoutsRef.current=[ | |
| setTimeout(()=>setOutroText('in'),1400), | |
| setTimeout(()=>{ | |
| setOutroText('out'); | |
| zoomOutroRef.current=true; | |
| },3050), | |
| setTimeout(()=>setOutroOverlay(true),3450), | |
| setTimeout(()=>{router.replace('/dashboard');},4250), | |
| ]; |
🤖 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/app/`(public)/page.tsx around lines 627 - 656, Update the
submitOnboardingProfile error path in the onboarding completion handler so a
failed save is surfaced to the user and exits or pauses the completion flow
before the outro animation and dashboard redirect begin. Preserve the existing
success animation and redirect only after the profile submission resolves
successfully, and allow the user to remain in or retry the onboarding flow.
Comment on lines
+661
to
+690
| .landing-modal-panel { | ||
| opacity: 0; | ||
| transform: translateY(24px) scale(0.94); | ||
| transition: opacity 420ms var(--ease), transform 420ms var(--ease); | ||
| } | ||
| .landing-modal-panel--visible { | ||
| opacity: 1; | ||
| transform: translateY(0) scale(1); | ||
| } | ||
| /* ════════════════════════════════════════════════════════════════════ | ||
| ONBOARDING FLOW ANIMATIONS | ||
| ════════════════════════════════════════════════════════════════════ */ | ||
| @keyframes ob-pulse-outer { | ||
| 0%, 100% { opacity: 0.1; transform: scale(1); } | ||
| 50% { opacity: 0.22; transform: scale(1.06); } | ||
| } | ||
| @keyframes ob-pulse-inner { | ||
| 0%, 100% { opacity: 0.3; transform: scale(1); } | ||
| 50% { opacity: 0.55; transform: scale(1.04); } | ||
| } | ||
| @keyframes ob-card-in { | ||
| from { transform: translateX(10px); } | ||
| to { transform: translateX(0); } | ||
| } | ||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Confirm duplicate keyframe/class definitions and check whether the pulse classes are used anywhere.
rg -n 'ob-pulse-outer|ob-pulse-inner|ob-card-in|landing-modal-panel' frontend/srcRepository: SaplingLearn/Sapling
Length of output: 1325
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"globals.css relevant sections:"printf'\n--- lines 650-690 ---\n'
sed -n '650,690p' frontend/src/app/globals.css | nl -ba -v650
printf'\n--- lines 910-945 ---\n'
sed -n '910,945p' frontend/src/app/globals.css | nl -ba -v910
echoecho"OnboardingFlow relevant section:"
sed -n '380,410p' frontend/src/app/globals.css | nl -ba -v380 2>/dev/null ||trueprintf'\n--- OnboardingFlow around card-in usage ---\n'
sed -n '380,410p' frontend/src/components/OnboardingFlow.tsx | nl -ba -v380
echoecho"Search for ob-card-in declarations in tracked files:"
rg -n '`@keyframes` ob-card-in|animation:.*ob-card-in\.|\.ob-card-in|ob-card-in' frontend/src .Repository: SaplingLearn/Sapling
Length of output: 252
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"globals.css relevant sections:"printf'\n--- lines 650-690 ---\n'
sed -n '650,690p' frontend/src/app/globals.css | awk '{printf "%5d:%s\n", NR+649, $0}'printf'\n--- lines 910-945 ---\n'
sed -n '910,945p' frontend/src/app/globals.css | awk '{printf "%5d:%s\n", NR+909, $0}'echoecho"OnboardingFlow around card-in usage:"
sed -n '380,410p' frontend/src/components/OnboardingFlow.tsx | awk '{printf "%5d:%s\n", NR+379, $0}'echoecho"Search for ob-card-in declarations in tracked files:"
rg -n '`@keyframes` ob-card-in|animation:.*ob-card-in|\.ob-card-in|ob-card-in|\.ob-pulse|ob-pulse-outer|ob-pulse-inner|landing-modal-panel' frontend/src ||trueRepository: SaplingLearn/Sapling
Length of output: 5843
Remove the duplicate onboarding animation definitions.
frontend/src/app/globals.css declares @keyframes ob-pulse-outer, ob-pulse-inner, and ob-card-in first and then redeclares them later with different transform/opacity values. Keep one source of truth for onboarding animations; otherwise the earlier definitions are dead and the intended effect is misleading (for example, the earlier ob-card-in translates horizontally, while the second declaration translates vertically).
🤖 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/app/globals.css` around lines 661 - 690, Remove the duplicate
`@keyframes` definitions for ob-pulse-outer, ob-pulse-inner, and ob-card-in in
globals.css, retaining the later intended definitions as the single source of
truth and preserving their transform and opacity behavior.
Comment on lines
+202
to
+219
| function handleNext() { | ||
| if (!canAdvance()) return; | ||
| setCompleted(new Set([...completed, activeStep])); | ||
| if (activeStep < STEPS.length - 1) { | ||
| setActiveStep(activeStep + 1); | ||
| } else { | ||
| onFinish({ | ||
| firstName: formData.firstName, | ||
| lastName: formData.lastName, | ||
| school: formData.school, | ||
| year: formData.year, | ||
| majors: formData.majors, | ||
| minors: formData.minors, | ||
| course_ids: formData.courses.map(c => c.id), | ||
| style: formData.style, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Final step can be submitted multiple times.
onFinish on the last step triggers an async profile POST in the parent, but nothing disables the button while it is in flight — repeated clicks on "Launch Sapling" fire duplicate submitOnboardingProfile writes before onboardingPhase flips to complete. Track a local submitting flag and gate both handleNext and the button's disabled.
🛡️ Proposed fix
+ const [submitting, setSubmitting] = useState(false);+
function handleNext() {
- if (!canAdvance()) return;+ if (!canAdvance() || submitting) return;
setCompleted(new Set([...completed, activeStep]));
if (activeStep < STEPS.length - 1) {
setActiveStep(activeStep + 1);
} else {
+ setSubmitting(true);
onFinish({- <button onClick={handleNext} disabled={!canAdvance()} style={{+ <button onClick={handleNext} disabled={!canAdvance() || submitting} style={{Also applies to: 722-739
🤖 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/OnboardingFlow.tsx` around lines 202 - 219, Update
the OnboardingFlow component’s final-step submission path to track a local
submitting state, set it before invoking onFinish, and gate handleNext so
repeated clicks are ignored while submission is in flight. Bind the same state
to the Launch Sapling button’s disabled property, while preserving normal
navigation behavior for earlier steps.
Comment on lines
+277
to
+298
| return ( | ||
| <div | ||
| style={{ | ||
| position: 'fixed', inset: 0, zIndex: 80, | ||
| opacity: visible ? 1 : 0, | ||
| transition: 'opacity 600ms cubic-bezier(0.22,1,0.36,1)', | ||
| pointerEvents: visible ? 'auto' : 'none', | ||
| overflowY: 'auto', | ||
| }} | ||
| > | ||
| {/* ── Close ── */} | ||
| <button | ||
| onClick={onClose} | ||
| style={{ | ||
| position: 'fixed', top: '28px', right: '32px', zIndex: 10, | ||
| color: 'rgba(0,0,0,0.28)', background: 'none', border: 'none', | ||
| padding: '8px', display: 'flex', cursor: 'pointer', | ||
| transition: 'color 0.2s ease', | ||
| }} | ||
| > | ||
| <X style={{ width: '22px', height: '22px' }} strokeWidth={1.5} /> | ||
| </button> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Modal lacks dialog semantics and an accessible close label.
The overlay is a full-screen modal but has no role="dialog"/aria-modal, and the close button renders only an icon with no accessible name. SignInModal.tsx already does this correctly (role, aria-modal, aria-label, focus trap) — worth mirroring at least the labeling here.
Also, while visible is false the form stays in the DOM and remains tab-reachable despite pointerEvents: 'none'; consider inert/aria-hidden in that state.
♿ Proposed fix
<div
+ role="dialog"+ aria-modal="true"+ aria-label="Onboarding"+ aria-hidden={!visible}
style={{
position: 'fixed', inset: 0, zIndex: 80,
@@
<button
onClick={onClose}
+ type="button"+ aria-label="Close onboarding"
style={{📝 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
| return( | |
| <div | |
| style={{ | |
| position: 'fixed',inset: 0,zIndex: 80, | |
| opacity: visible ? 1 : 0, | |
| transition: 'opacity 600ms cubic-bezier(0.22,1,0.36,1)', | |
| pointerEvents: visible ? 'auto' : 'none', | |
| overflowY: 'auto', | |
| }} | |
| > | |
| {/* ── Close ── */} | |
| <button | |
| onClick={onClose} | |
| style={{ | |
| position: 'fixed',top: '28px',right: '32px',zIndex: 10, | |
| color: 'rgba(0,0,0,0.28)',background: 'none',border: 'none', | |
| padding: '8px',display: 'flex',cursor: 'pointer', | |
| transition: 'color 0.2s ease', | |
| }} | |
| > | |
| <Xstyle={{width: '22px',height: '22px'}}strokeWidth={1.5}/> | |
| </button> | |
| return( | |
| <div | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label="Onboarding" | |
| aria-hidden={!visible} | |
| style={{ | |
| position: 'fixed',inset: 0,zIndex: 80, | |
| opacity: visible ? 1 : 0, | |
| transition: 'opacity 600ms cubic-bezier(0.22,1,0.36,1)', | |
| pointerEvents: visible ? 'auto' : 'none', | |
| overflowY: 'auto', | |
| }} | |
| > | |
| {/* ── Close ── */} | |
| <button | |
| onClick={onClose} | |
| type="button" | |
| aria-label="Close onboarding" | |
| style={{ | |
| position: 'fixed',top: '28px',right: '32px',zIndex: 10, | |
| color: 'rgba(0,0,0,0.28)',background: 'none',border: 'none', | |
| padding: '8px',display: 'flex',cursor: 'pointer', | |
| transition: 'color 0.2s ease', | |
| }} | |
| > | |
| <Xstyle={{width: '22px',height: '22px'}}strokeWidth={1.5}/> | |
| </button> |
🤖 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/OnboardingFlow.tsx` around lines 277 - 298, Update
the full-screen overlay in OnboardingFlow’s returned JSX to include dialog
semantics with role="dialog" and aria-modal, and add an accessible aria-label to
the icon-only close button using onClose. When visible is false, mark the
overlay inert and aria-hidden so its form and controls are not
keyboard-reachable, while preserving the existing visibility and pointer-event
behavior.
Comment on lines
169
to
174
| if (data.onboardingCompleted) { | ||
| router.replace("/dashboard"); | ||
| } else { | ||
| router.replace("/onboarding"); | ||
| sessionStorage.setItem("sapling_onboarding_pending", "1"); | ||
| } | ||
| onClose(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Where is SignInModal rendered? Any non-landing mount points?
rg -nP --type=tsx -C2 '<SignInModal' frontend/src || rg -nP -C2 '<SignInModal' frontend/srcRepository: SaplingLearn/Sapling
Length of output: 540
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== locate relevant files =="
fd -a 'SignInModal\.tsx$|page\.tsx$|callback|auth' frontend/src | sed 's#^\./##'| head -100
echoecho"== SignInModal outline =="
ast-grep outline frontend/src/components/SignInModal.tsx --view concise ||trueechoecho"== SignInModal relevant code =="
sed -n '1,240p' frontend/src/components/SignInModal.tsx | cat -n
echoecho"== public page relevant SignInModal area =="
sed -n '1220,1285p''frontend/src/app/(public)/page.tsx'| cat -n
echoecho"== callback/auth route files =="
fd -a '.*' frontend/src/app | grep -E 'auth/callback|api/auth|callback'||trueRepository: SaplingLearn/Sapling
Length of output: 15052
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== auth callback page =="
sed -n '1,260p' frontend/src/app/auth/callback/page.tsx | cat -n
echoecho"== onboarding page =="
sed -n '1,260p' frontend/src/app/onboarding/page.tsx | cat -n
echoecho"== public page imports and onboarding state/effect area =="
sed -n '1,180p''frontend/src/app/(public)/page.tsx'| cat -n
echoecho"== session storage / onboarding pending usages =="
rg -n -C2 'sapling_onboarding_pending|onboardingCompleted|onboardingPhase|auth/callback|replace\('\''/'| frontend/src ||trueechoecho"== behavioral shape check: SignInModal exists under app tree and callback behavior =="
python3 - <<'PY'from pathlib import Pathroot = Path('frontend/src')files = {p.relative_to(root) for p in root.rglob('*')}print('SignInModal.tsx present:', Path('components/SignInModal.tsx') in files)print('(public)/page.tsx present:', Path('(public)/page.tsx') in files)print('auth/callback/page.tsx present:', Path('app/auth/callback/page.tsx') in files)for rel in [Path('app/auth/callback/page.tsx'), Path('(public)/page.tsx')]: text = (root/rel).read_text() print(f'{rel}: SignInModal component name =', rel in files and '<SignInModal' in text) print(f'{rel}: router.replace("/") in file =', 'router.replace("/")' in text or "router.replace('/')" in text)PYRepository: SaplingLearn/Sapling
Length of output: 14018
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== SignInModal imports and mount points =="
rg -n --glob '*.tsx' -C3 '<SignInModal\b|import .*SignInModal|from '`@/components/SignInModal`|from "`@/components/SignInModal`|from "`@/components/SignInModal`' frontend/src || trueechoecho "== routing/middleware files =="fd -a '(middleware|layout|globals|next.config|auth|login|signin|sign-in)' frontend/src frontend | sed 's#^\./##' | head -120echoecho"== shell/public layouts for SignInModal import/usage =="forfin$(fd -a 'layout\.tsx$' frontend/src/app | sed 's#^\./##');doecho"-- $f"
rg -n -C2 '<SignInModal\b|SignInModal|sapling_onboarding|router.replace\("'\''/'|"$f"||truedoneechoecho"== middleware routing around auth/callback/landing =="forfin$(fd -a 'middleware\.(ts|tsx)$'.| sed 's#^\./##');doecho"-- $f"
sed -n '1,260p'"$f"| cat -n
doneRepository: SaplingLearn/Sapling
Length of output: 254
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== SignInModal uses/imports =="
rg -n --glob '*.{tsx,ts}' -C3 '<SignInModal\b|SignInModal|sapling_onboarding_pending|onboardingCompleted' frontend/src ||trueechoecho"== layout files under app =="
fd -a 'layout\.tsx$' frontend/src/app -x sh -c 'echo "-- $1"; rg -n -C2 "SignInModal|sapling_onboarding_pending|router.replace\('\'\'/ |sapling_onboarding_pending""$1" || true' sh {}echoecho "== middleware files =="fd -a 'middleware\.(ts|tsx)$' . -x sh -c 'echo "-- $1"; sed -n "1,240p""$1" | cat -n' sh {}echoecho "== behavioral check =="python3 - <<'PY'from pathlib import Pathroot = Path('frontend/src')for file in root.rglob('*'): if file.is_file() and file.suffix in {'.tsx', '.ts'}: text = file.read_text(errors='ignore') if 'SignInModal' in text: print(f'{file}: SignInModal_mentions={text.count(\"SignInModal\")}')print('SignInModal.tsx has onClose(); after pending flag:', "sessionStorage.setItem(\"sapling_onboarding_pending\", \"1\");" in Path('components/SignInModal.tsx').read_text() and '}, [open], onClose' in Path('components/SignInModal.tsx').read_text())print('auth callback redirects pending to /:', "router.replace('/');" in Path('app/auth/callback/page.tsx').read_text())PYRepository: SaplingLearn/Sapling
Length of output: 7604
🏁 Script executed:
#!/bin/bashshopt -s globstar
set -euo pipefail
echo"== layout files under app (if any) =="if fd -a 'layout\.tsx$' frontend/src/app >/tmp/layouts.txt 2>/dev/null;thenwhile IFS= read -r file;doecho"-- $file"
rg -n -C2 "SignInModal|sapling_onboarding_pending|router.replace\('/')|router.replace\(\"\/\"""$file"||truedone< /tmp/layouts.txt
elseecho"No layout files found"fiechoecho"== middleware files =="if fd -a 'middleware\.(ts|tsx)$'.>/tmp/middleware.txt 2>/dev/null;thenwhile IFS= read -r file;doecho"-- $file"
sed -n '1,240p'"$file"| cat -n
done< /tmp/middleware.txt
elseecho"No middleware files found"fiechoecho"== behavior probe: current branches =="
python3 - <<'PY'from pathlib import Pathtext = Path('frontend/src/components/SignInModal.tsx').read_text()start = text.index('if (data.onboardingCompleted) {')block = text[start:text.find('});', start) + 3]has_replaced_dashboard = 'router.replace("/dashboard");' in blockhas_stashed_then_closed = 'sessionStorage.setItem("sapling_onboarding_pending", "1");' in block and 'onClose();' in blockprint(block.strip())print('dashboard branch:', has_replaced_dashboard)print('pending branch stashes then closes:', has_stashed_then_closed)callback = Path('frontend/src/app/auth/callback/page.tsx').read_text()print('callback fallback does router.replace("/");:', 'router.replace("/");' in callback)PYRepository: SaplingLearn/Sapling
Length of output: 6736
No navigation on the not-onboarded branch — only works if the modal is mounted on /.
The completed branch redirects to /dashboard, but the not-completed branch only writes sapling_onboarding_pending and closes. That resume flow depends on the landing-page SignInModal handling the flag, while auth/callback sends non-popup users to / in the same case. Redirect to / here as well so sign-in does not leave non-landing pages after completion is not ready.
🤖 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/SignInModal.tsx` around lines 169 - 174, Update the
non-onboarded branch in the sign-in completion flow to navigate to "/" after
setting "sapling_onboarding_pending" and before closing the modal. Keep the
existing "/dashboard" redirect for completed onboarding unchanged, ensuring
sign-in from any page reaches the landing-page resume flow.
Comment on lines
+62
to
+85
| /** | ||
| * Resolve the effective frontend config from an env bag. | ||
| * | ||
| * `DEPLOY_ENV` is the single source of truth: when it names a known environment | ||
| * the API origin and cookie domain are DERIVED from `FRONTEND_ENVS`, so they | ||
| * cannot drift, be half-set, or be leaked from a stray explicit var. When | ||
| * `DEPLOY_ENV` is unset (local/dev, docker, or a legacy build that sets the | ||
| * vars explicitly) this falls back to the explicit env vars — preserving prior | ||
| * behaviour, including the middleware's `BACKEND_URL`-before-`NEXT_PUBLIC_API_URL` | ||
| * preference (BACKEND_URL is the server-reachable origin; see middleware.ts). | ||
| */ | ||
| export function resolveFrontendEnv(env: EnvSource): ResolvedFrontendEnv { | ||
| const deployEnv = (env.DEPLOY_ENV ?? '').trim().toLowerCase(); | ||
| if (deployEnv && deployEnv in FRONTEND_ENVS) { | ||
| const c = FRONTEND_ENVS[deployEnv as FrontendEnv]; | ||
| return { env: deployEnv as FrontendEnv, apiUrl: c.apiUrl, cookieDomain: c.cookieDomain, derived: true }; | ||
| } | ||
| return { | ||
| env: null, | ||
| apiUrl: (env.BACKEND_URL ?? '').trim() || (env.NEXT_PUBLIC_API_URL ?? '').trim(), | ||
| cookieDomain: (env.COOKIE_DOMAIN ?? '').trim() || undefined, | ||
| derived: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard DEPLOY_ENV in FRONTEND_ENVS against prototype keys.
in traverses the prototype chain, so a DEPLOY_ENV value like constructor, toString, or hasOwnProperty would pass this check and resolve to Object.prototype's member, yielding { apiUrl: undefined, cookieDomain: undefined, derived: true } — a broken config silently marked as trusted/derived. Low likelihood in practice, but this is exactly the class of silent-misconfiguration this guard exists to prevent, and the fix is trivial.
🛡️ Proposed fix
- if (deployEnv && deployEnv in FRONTEND_ENVS) {+ if (deployEnv && Object.prototype.hasOwnProperty.call(FRONTEND_ENVS, deployEnv)) {
const c = FRONTEND_ENVS[deployEnv as FrontendEnv];📝 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
| /** | |
| *Resolvetheeffectivefrontendconfigfromanenvbag. | |
| * | |
| *`DEPLOY_ENV`isthesinglesourceof truth: whenitnamesaknownenvironment | |
| *theAPIoriginandcookiedomainareDERIVEDfrom`FRONTEND_ENVS`,sothey | |
| *cannotdrift,behalf-set,orbeleakedfromastrayexplicitvar.When | |
| *`DEPLOY_ENV`isunset(local/dev,docker,oralegacybuildthatsetsthe | |
| *varsexplicitly)thisfallsbacktotheexplicitenvvars—preservingprior | |
| *behaviour,includingthemiddleware's`BACKEND_URL`-before-`NEXT_PUBLIC_API_URL` | |
| *preference(BACKEND_URListheserver-reachableorigin;seemiddleware.ts). | |
| */ | |
| exportfunctionresolveFrontendEnv(env: EnvSource): ResolvedFrontendEnv{ | |
| constdeployEnv=(env.DEPLOY_ENV??'').trim().toLowerCase(); | |
| if(deployEnv&&deployEnvinFRONTEND_ENVS){ | |
| constc=FRONTEND_ENVS[deployEnvasFrontendEnv]; | |
| return{env: deployEnvasFrontendEnv,apiUrl: c.apiUrl,cookieDomain: c.cookieDomain,derived: true}; | |
| } | |
| return{ | |
| env: null, | |
| apiUrl: (env.BACKEND_URL??'').trim()||(env.NEXT_PUBLIC_API_URL??'').trim(), | |
| cookieDomain: (env.COOKIE_DOMAIN??'').trim()||undefined, | |
| derived: false, | |
| }; | |
| } | |
| /** | |
| *Resolvetheeffectivefrontendconfigfromanenvbag. | |
| * | |
| *`DEPLOY_ENV`isthesinglesourceof truth: whenitnamesaknownenvironment | |
| *theAPIoriginandcookiedomainareDERIVEDfrom`FRONTEND_ENVS`,sothey | |
| *cannotdrift,behalf-set,orbeleakedfromastrayexplicitvar.When | |
| *`DEPLOY_ENV`isunset(local/dev,docker,oralegacybuildthatsetsthe | |
| *varsexplicitly)thisfallsbacktotheexplicitenvvars—preservingprior | |
| *behaviour,includingthemiddleware's`BACKEND_URL`-before-`NEXT_PUBLIC_API_URL` | |
| *preference(BACKEND_URListheserver-reachableorigin;seemiddleware.ts). | |
| */ | |
| exportfunctionresolveFrontendEnv(env: EnvSource): ResolvedFrontendEnv{ | |
| constdeployEnv=(env.DEPLOY_ENV??'').trim().toLowerCase(); | |
| if(deployEnv&&Object.prototype.hasOwnProperty.call(FRONTEND_ENVS,deployEnv)){ | |
| constc=FRONTEND_ENVS[deployEnvasFrontendEnv]; | |
| return{env: deployEnvasFrontendEnv,apiUrl: c.apiUrl,cookieDomain: c.cookieDomain,derived: true}; | |
| } | |
| return{ | |
| env: null, | |
| apiUrl: (env.BACKEND_URL??'').trim()||(env.NEXT_PUBLIC_API_URL??'').trim(), | |
| cookieDomain: (env.COOKIE_DOMAIN??'').trim()||undefined, | |
| derived: false, | |
| }; | |
| } |
🤖 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/lib/deployGuard.ts` around lines 62 - 85, Replace the
prototype-traversing `deployEnv in FRONTEND_ENVS` check in `resolveFrontendEnv`
with an own-property check, while preserving the existing derived configuration
behavior for recognized frontend environments and fallback behavior for all
other values.
Comment on lines
+53
to
+69
| // Defence-in-depth for the "wrong environment on this worker" deploy footgun: | ||
| // if the host we're serving belongs to one environment (e.g. staging.*) but | ||
| // API_URL points at another's backend (e.g. prod api.*), sign-in silently | ||
| // fails — the backend can't validate a session cookie signed with the other | ||
| // env's SESSION_SECRET, which surfaced as a mystery `session_expired` on | ||
| // staging. Fail with a distinct, greppable code and a loud server log instead. | ||
| const mismatch = detectHostConfigMismatch(request.nextUrl.hostname, API_URL) | ||
| if (mismatch) { | ||
| console.error( | ||
| `[sapling] deploy misconfiguration: ${mismatch}. This worker is serving the ` + | ||
| 'wrong environment — redeploy with the correct DEPLOY_ENV build variable and ' + | ||
| '`wrangler deploy --env <env>`, and confirm the custom-domain route binding. ' + | ||
| 'See docs/decisions/0018-session-token-lifecycle.md.', | ||
| ) | ||
| return redirectToSignin(request, 'env_misconfig') | ||
| } | ||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Error message recommends a different deploy command than the one this PR establishes.
The env_misconfig log tells operators to run wrangler deploy --env <env>, but docs/decisions/0020 and wrangler.toml both explicitly establish npx wrangler versions upload as the canonical Deploy command, warning against conflating the two. Recommending wrangler deploy here during an incident risks bypassing the team's versioned-rollout workflow — the exact class of confusion this ADR was written to prevent.
📝 Proposed fix
console.error(
`[sapling] deploy misconfiguration: ${mismatch}. This worker is serving the ` +
'wrong environment — redeploy with the correct DEPLOY_ENV build variable and ' +
- '`wrangler deploy --env <env>`, and confirm the custom-domain route binding. ' ++ '`npx wrangler versions upload` (with the correct DEPLOY_ENV Build variable and, ' ++ 'for staging, `--env staging`), and confirm the custom-domain route binding. ' +
'See docs/decisions/0018-session-token-lifecycle.md.',
)📝 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
| // Defence-in-depth for the "wrong environment on this worker" deploy footgun: | |
| // if the host we're serving belongs to one environment (e.g. staging.*) but | |
| // API_URL points at another's backend (e.g. prod api.*), sign-in silently | |
| // fails — the backend can't validate a session cookie signed with the other | |
| // env's SESSION_SECRET, which surfaced as a mystery `session_expired` on | |
| // staging. Fail with a distinct, greppable code and a loud server log instead. | |
| constmismatch=detectHostConfigMismatch(request.nextUrl.hostname,API_URL) | |
| if(mismatch){ | |
| console.error( | |
| `[sapling] deploy misconfiguration: ${mismatch}. This worker is serving the `+ | |
| 'wrong environment — redeploy with the correct DEPLOY_ENV build variable and '+ | |
| '`wrangler deploy --env <env>`, and confirm the custom-domain route binding. '+ | |
| 'See docs/decisions/0018-session-token-lifecycle.md.', | |
| ) | |
| returnredirectToSignin(request,'env_misconfig') | |
| } | |
| // Defence-in-depth for the "wrong environment on this worker" deploy footgun: | |
| // if the host we're serving belongs to one environment (e.g. staging.*) but | |
| // API_URL points at another's backend (e.g. prod api.*), sign-in silently | |
| // fails — the backend can't validate a session cookie signed with the other | |
| // env's SESSION_SECRET, which surfaced as a mystery `session_expired` on | |
| // staging. Fail with a distinct, greppable code and a loud server log instead. | |
| constmismatch=detectHostConfigMismatch(request.nextUrl.hostname,API_URL) | |
| if(mismatch){ | |
| console.error( | |
| `[sapling] deploy misconfiguration: ${mismatch}. This worker is serving the `+ | |
| 'wrong environment — redeploy with the correct DEPLOY_ENV build variable and '+ | |
| '`npx wrangler versions upload` (with the correct DEPLOY_ENV Build variable and, '+ | |
| 'for staging, `--env staging`), and confirm the custom-domain route binding. '+ | |
| 'See docs/decisions/0018-session-token-lifecycle.md.', | |
| ) | |
| returnredirectToSignin(request,'env_misconfig') | |
| } |
🤖 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/middleware.ts` around lines 53 - 69, Update the misconfiguration
message in the middleware mismatch branch guarded by detectHostConfigMismatch to
recommend the canonical npx wrangler versions upload workflow established by the
deployment configuration and ADR, rather than wrangler deploy --env. Preserve
the existing diagnostic context and route-binding guidance.
AndresL230
marked this pull request as draft
July 27, 2026 23:50
AndresL230
marked this pull request as ready for review
July 29, 2026 09:12
AndresL230force-pushed
the
fix/reject-empty-document-extraction
branch
from
July 29, 2026 09:12
09e23b1 to
2790afdCompare… summary A rasterized PDF has no text layer, so extraction returns "" without raising. `_extract_text_or_422` only caught exceptions, so the empty string flowed straight into the classify/summarize prompt as `Content: ` -- and because that prompt requires a summary plus a concept list with no "insufficient content" escape hatch, the model invented a document instead of failing. Observed on a CS 132 (linear algebra) practice final: the stored summary described the 1964 Berkeley Free Speech Movement and the extracted concepts were CNNs, RNNs, Transformers, and Attention. Those concepts were persisted and bound for the course knowledge graph, which is shared by every enrolled student -- so one unreadable upload would have seeded neural-network topics into a linear algebra course for the whole class. Docling already detects this (it flags low-char pages in `fallback_pages`), but that signal is only acted on when `OCR_ENGINE=auto`, and nothing downstream checked the text at all. Guard both upload paths against near-empty extraction: - `_extract_text_or_422` now raises 422 (covers /upload/sync, and /upload when OCR_ASYNC_ENABLED is off) - the async-OCR branch inside the SSE stream emits the same terminal error+done pair it already uses for extraction failures, so clients need no new case Threshold is 50 stripped chars, matching the floor `extraction_service._extract_text_from_file_uncached` already applies to native PDF text. Emptiness alone would be too weak: a scanned page often yields a few stray characters (a page number, a watermark), which is still enough to trigger fabrication. Happy-path upload fixtures previously returned strings as short as "t", which the guard correctly rejects. They now go through a `_doc_text()` helper so a fixture is no longer indistinguishable from a failed extraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230force-pushed
the
fix/reject-empty-document-extraction
branch
from
July 29, 2026 09:36
9c99345 to
4f06ebaCompareUh oh!
There was an error while loading. Please reload this page.
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 was referenced Jul 29, 2026
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
Rejects near-empty text extraction instead of letting the classify/summarize prompt fabricate a document (observed: a scanned linear-algebra practice final stored as a Berkeley Free Speech Movement summary with CNN/RNN/Transformer concepts — which would have seeded the course's SHARED knowledge graph for every enrolled student).
_extract_text_or_422now 422s on < 50 stripped chars (MIN_EXTRACTED_CHARS, matchingextraction_service's native-PDF floor) — covers/upload/syncand/uploadwith async OCR off.error+donepair it already uses for extraction failures — no new client case._doc_text()helper so a happy-path fixture can't be mistaken for a failed extraction.Rebased 2026-07-29: the branch previously stacked on the DEPLOY_ENV-hardening work; it now carries ONLY this fix, cherry-picked onto current main. The deploy-env commits stay with #409.
Testing
pytest tests/test_documents_routes.py: 61 passed.🤖 Generated with Claude Code