Uh oh!
There was an error while loading. Please reload this page.
Promote staging to production — 285 commits (DB reconciled first) - #515
Merged
Conversation
…index_document_chunks
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172GJxVHwgWqMrBcKR1ixhx
…peline; store extracted_text
No existing test called _index_document_chunks directly; every upload test patches _spawn_post_roll instead. Add direct unit tests covering the happy path (relevance gate skipped when no catalog embedding exists) and the empty-chunks short-circuit before index_document_chunks is ever invoked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172GJxVHwgWqMrBcKR1ixhx
Reworked from the task-5 brief after verifying the real schema: documents has no file_path/uploader_id columns and no Supabase Storage bucket holds raw uploaded bytes (files are extracted in-memory at upload time and discarded), so re-downloading and re-extracting is not possible. The script instead chunks and indexes documents that already have extracted_text but no course_chunks rows yet, and reports (without attempting to fix) documents whose extracted_text is unrecoverably NULL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0172GJxVHwgWqMrBcKR1ixhx
…gate, test mock targets, chunker docs
…o the landing route The [P2-I] follow-up described `components/OnboardingFlow.tsx` as dead code "not imported by the active route." It is actually imported (L7) and rendered (L1247, under `onboardingPhase !== 'idle'`) by the public landing page `app/(public)/page.tsx`, on both `refactor/token-unification` and `main`. Deleting it as-is breaks the build. Re-scoped the item to "unwire from the landing page first, then delete." Refs #292 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ired docs(followups): correct #292 — OnboardingFlow.tsx is still wired into the landing route
The hero-canvas orb palettes hardcode #3e6f8a because canvas fillStyle can't resolve CSS var(); the CSS orbit node uses var(--info). Comment records that provenance so the two stay intentionally in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
main's CI (Backend job) has been red since the RAG pipeline scripts landed (~Jul 3): 8 un-baselined ruff errors caused ruff check . to exit 1, skipping pytest. Because main isn't a protected branch, merges proceeded through the red and it went unnoticed — and every open PR's Backend job inherited the failure via the pull_request merge check. Fixes (behavior-preserving): - benchmark_rag.py: drop unused import os; noqa: E402 on the two intentional post-sys.path imports; drop unused chunk_results binding (keeps the side-effecting run_chunk_tests call). - ingest_catalog.py: noqa: E402 on the post-sys.path import; drop stray f-prefix on a placeholder-less string. - scrape_bu_catalog.py: drop unused import sys; drop unused school_courses binding (keeps the side-effecting scrape_school call). Verified locally: ruff 0.15.20 check . -> All checks passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ipts chore(lint): fix ruff violations in RAG scripts unblocking main CI
docs: staging environment design + implementation plan
Per-doc baseline true-up (each doc diffed from its own last-touched commit to origin/main), plus removal of three stale/historical docs. Updated: - README.md — Gradescope Sync feature + 9 /api/gradescope endpoints, APP_ENV env var, migrations range 0028→0030. - CLAUDE.md — repo-map line refs (main.py router block :168→:169, build_system_prompt :261→:288), migration count now-at-0030, add documents.extracted_text (0030) to the encryption gotcha. - docs/README.md — index the new security/ and staging/ subdirs. - frontend/README.md — dir rename new_frontend→frontend, dev port 3000, Build & deploy (Cloudflare Workers/OpenNext, build-time BACKEND_URL), real fonts (Spectral/Playfair/DM Sans), (shell)/(public) route groups, current route list, Tweaks-panel removal. - backend/tests/README.md — Fixtures & conftest (hermetic Supabase + auth bypass), e2e_staging marker + tests/evals, refreshed test-file table. Deleted (stale/historical): - ROADMAP.md - backend/prompts/refactor-3-chat-tutor/README.md - backend/prompts/refactor-4-syllabus-unification/README.md Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imits (#243) * fix(agents): restore mastery updates, persist graph_update_json, wire usage limits Three regressions introduced when routes moved from legacy <graph_update> XML parsing to Pydantic AI tools. Closes#5 — Chat-tutor agent can now raise concept mastery. The only registered graph tool was emitting new_nodes at initial_mastery 0.0 and never calling the updated_nodes branch that actually moves scores. Added update_mastery_tool (ConceptMasteryUpdate + MasteryUpdateInput) which forwards updated_nodes with mastery_delta to apply_graph_update, and registered it on all three chat-tutor agents (socratic/expository/teachback). Updated _SHARED_PREAMBLE to instruct the model when to call each tool. Closes#13 — end_session concepts_covered now populated for agent-path chats. Agent path was calling save_message() with no graph_update argument, leaving graph_update_json NULL in every message row. end_session derives concepts_covered entirely from that column, so it always returned []. Fixed by adding a graph_updates: list accumulator field to SaplingDeps; both graph tools append their payload during a run; _chat_via_agent merges and returns the combined dict; chat() passes it to save_message() so the column is populated. Closes#14 — ORCHESTRATOR_LIMITS is no longer dead code. UsageLimits(8 req / 10 tool-calls / 100k tokens) was defined in agents/__init__.py but never passed to .run(). Tool-using agents ran with no token or call ceiling. Added usage_limits=ORCHESTRATOR_LIMITS to run_kwargs in both _chat_via_agent (learn.py) and _quiz_via_agent (quiz.py). Tests: 140 tests pass (0 new failures). New test file test_graph_tools_bugs.py covers all three bugs with 14 targeted tests; test_chat_tutor_imports.py updated to expect 5 tools; pre-existing test_skips_self_edges KeyError fixed in test_graph_service.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(agents): only persist mastery updates that actually changed the graph update_mastery_tool appended {updated_nodes} to deps.graph_updates unconditionally, so a concept the model named but that does not exist in the graph (no changes returned by apply_graph_update) still leaked into graph_update_json and was over-reported as concepts_covered in end_session. Gate the append on the concepts that genuinely changed, rebuilding updated_nodes from apply_graph_update's returned changes. Also accumulate the real before/after deltas on deps.mastery_changes so the route can surface them for parity with the legacy path. * fix(learn): surface real mastery_changes from agent chat path _chat_via_agent returned mastery_changes: [] even when update_mastery_tool produced real before/after deltas during the run, leaving the agent path asymmetric with _legacy_chat (which returns apply_graph_update's changes). Echo deps.mastery_changes back to the client for parity, and correct the docstring that claimed the empty value was intentional. * style(tests): remove unused imports and vars flagged by ruff (F401/F841) * fix(agents): bound mastery_delta and normalize the persisted-node gate Two review nits on the update_mastery_tool added in this PR: - Constrain ConceptMasteryUpdate.mastery_delta to [-1.0, 1.0] (ge/le). The score is already clamped in apply_graph_update, but the raw model delta is also written verbatim into node_mastery_events and feeds the 14-day mastery-velocity metric — an out-of-range delta would distort it. - Match the persisted-node over-report gate on the normalized concept name. apply_graph_update dedups case/whitespace-insensitively and returns the *stored* name, while updated_nodes carries the model's spelling; the exact-string gate dropped a genuinely-changed concept on casing/spacing drift, under-reporting concepts_covered in end_session. Reuse _normalize_concept on both sides. Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(tests): reconcile onto main — offering rename + CodeRabbit nits Rebased onto current main (f788ed3). Adjustments the 398-commit drift required: - Tests patched routes.learn._get_session_course_id, which main renamed to _get_session_offering_id in the offering redesign. _chat_via_agent takes course_id directly and never calls it, so drop the incidental patch in test_learn_chat_via_agent_passes_usage_limits; repoint the endpoint test test_agent_path_save_message_receives_graph_update to _get_session_offering_id. This was the sole CI (Backend pytest) blocker. CodeRabbit review nits: - Constrain ConceptMasteryUpdate.event_type to Literal[interaction,correction, quiz] so invalid labels can't reach node_mastery_events. - Trim persisted concept names in both graph tools so " DFS " can't reach graph_update_json with surrounding whitespace. - Replace the tautological assertion in test_returns_fallback_message_when_no_score_change with a real check. Full backend suite: 859 passed, 1 skipped (2 pre-existing storage_service failures + 1 OCR event-loop flake are unrelated; they also fail on clean main). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Jose Cruz <jose.colorstack@gmail.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com>
Make the right-hand knowledge-map panel on the active Tutor session collapsible via a draggable edge tab (desktop only): - railOpen state persisted to localStorage (sapling_learn_rail_open), hydrated on mount - rail widened to 400px; width/min-width slide 0<->400 via --dur/--ease, aside kept mounted with overflow:hidden (reduced-motion covered by the existing global rule) - edge tab: knowledge-graph glyph + rotating chevron, brand-forest when open / text-muted when collapsed; click toggles, pointer-drag moves the width live and snaps at the halfway point (<4px press = click) - strip the white card around the graph so it floats on the transparent rail; KnowledgeGraph/physics untouched - hide the shared graph recenter control on the Learn rail only via a scoped .learn-map-rail CSS rule (Dashboard/Tree keep it) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code-review findings on the collapsible knowledge-map rail: - track hydration with state (not a ref) so the persist effect is gated until the stored collapsed state is applied, preventing the initial value from clobbering storage on mount - route pointercancel through a shared drag terminator that verifies the pointer id and aborts without toggling/snapping on cancellation - make the edge tab keyboard-operable: Enter/Space toggle the rail (Space default prevented), pointer drag handlers preserved Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AI Session Interface Update for Knowledge Graph
perf(cache): per-process lru_cache for hot deterministic reads (#98)
…#487) * fix(ui): replace viewport-math residuals with FullHeightScreen (#341) Three screens still sized themselves by subtracting a magic constant from 100vh. That subtraction can only be correct for ONE of ShellFrame's two layouts — the sidebar variant gives `<main>` the full 100dvh, the TopNav variant gives it 100dvh - 56px — and it is blind to the density preference that retunes the padding tokens above it. Tree.tsx is the one with a user-visible consequence, not just a stray scrollbar: the row sized `calc(100vh - 240px)` wraps the element a ResizeObserver watches, and that contentRect is passed straight through as `<KnowledgeGraph width height>`. A mis-measured row therefore renders the graph CANVAS at the wrong size. It now takes `flex: 1` + `minHeight: 0` inside a FullHeightScreen root, so it absorbs exactly what `<main>` has left after the TopBar and filter row, in either layout and at any density. Gradebook Landing/Course used `calc(100vh - var(--row-h))` — subtracting a DENSITY token (40/34/48px) as though it were a nav height. Both now sit in a FullHeightScreen with `flex: 1 0 auto` on their `<main>`: fill the remaining space, keep growing when the content is taller. Adds frontend/e2e/tree-viewport.spec.ts, which asserts the invariant that was silently violated — on /tree the graph canvas fits inside the shell scrollport — in BOTH layouts, driven off the localStorage layout pref. It lives in its own spec because graph.spec.ts declares itself data-only and reads no geometry by design. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): assert fit in both directions, and cover the gradebook (#341) Review follow-ups on the journey added with this PR. Two-sided fit: the spec only guarded overshoot, so a future regression that broke `flex: 1` and left the canvas a few pixels tall would have passed — `canvasHeight > 0` is not a floor. It now asserts the canvas settles FLUSH with the scrollport's bottom edge, which is the actual stated invariant, and polls for it because the canvas size is ResizeObserver-driven and starts at Tree.tsx's placeholder 900x600. Gradebook coverage: Landing/Course got the same class of change as Tree with no geometry assertion behind it — the gap that let the identical `calc(100vh - N)` bug survive review on Settings.tsx during #335, since jsdom has no layout engine and cannot catch it. Adds a second test asserting the seeded gradebook does not manufacture scroll it does not need, in both layouts. Renames tree-viewport.spec.ts -> viewport-fit.spec.ts now that it covers two screens. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): assert the gradebook adds no phantom height, not that it never scrolls (#341) The first version of this test asserted the gradebook page must not scroll at all. That is wrong: under the topnav layout the scrollport is 56px shorter, and the seeded gradebook's content genuinely needs more than it — scrolling there is the correct answer, and the assertion failed the FIXED build for a legitimate reason (16px at topnav, 0 at sidebar). What the fix actually guarantees is narrower: the container contributes no height of its own beyond the space it was given or the height its content needs. That is now what the test measures — <main> must be no taller than max(available, content), and no shorter than the space available (which would mean flex-grow is broken). The unfixed build overshot by exactly 40px at the default density, which is `--row-h` to the pixel — the density-token-as-nav-height bug, measured. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): measure gradebook content from its children, not scrollHeight (#341) The previous revision of this assertion passed on the UNFIXED build, which makes it worthless as a regression test. Caught by running the spec against the base sha before trusting it green. Cause: it derived the content height from `main.scrollHeight`, which is floored at the element's own client height. Any phantom height the box gave itself was therefore mirrored into the "content" it was being compared against, so `mainHeight <= max(available, content)` held by construction and could never fail. It now measures the content extent from the in-flow children (skipping AmbientOrbs, which is position:fixed) plus the bottom padding — a number the box's own sizing cannot influence. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): close three review findings in the viewport-fit spec (#341) Follow-ups from the review of the spec delta. Gate on the LOADED gradebook grid, not the chrome. `gradebook-transcript-open` renders as soon as there is a user, while `loading` is still swapping a six-card skeleton in for the real grid — measuring that transient made the phantom-height assertion depend on fetch timing. Now waits on role="grid"/"Courses", which only exists once the fetch resolved. Count trailing child margins in the content measurement. A margin-bottom on a direct in-flow child raises <main>'s height without appearing in any child's bounding rect, which would have understated the content and turned a real regression into a pass. Latent today; cheap to close. Assert on the reading that settled. The tree test polled for convergence and then measured AGAIN, reopening the window the poll existed to close. A small settledFit() helper now returns the very reading that satisfied the condition, and every assertion runs on that one. It also returns the last reading on timeout, so a failure reports real numbers instead of a bare "timed out". part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(e2e): rename useLayout so eslint stops reading it as a React Hook (#341) CI's react-hooks/rules-of-hooks rejected the helper: anything named use* is treated as a Hook, and the spec calls it inside a for-loop over the two shell layouts. Renamed to switchLayout — behaviour identical. Local `npm run lint` reported 0 errors on the same code; the installed eslint-plugin-react-hooks is older than CI's, so this was only visible there. part of #341 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
) (#488) * feat(ui): give the approval gate a surface and a confirmation beat (#290) Sign-up ENDS on /pending, and the page was 42 lines with no container: a sprout, an h1, a paragraph and a button floating directly on a radial gradient. Beta glow into silence, with no "you're in" moment anywhere in it. The content now sits on a real `.card` surface (which supplies bg/border/ radius/shadow but no padding — that is the caller's job, so the card sets `var(--pad-lg)`), and the screen plays a one-shot confirmation beat: the sapling draws itself, then the message steps in behind it. The beat is CSS, not JS, and that is the point. globals.css already carries a global prefers-reduced-motion reset, so a CSS entrance is automatically safe where a JS-driven one would have to re-implement that guard. It reuses the motion vocabulary that already exists (`fade-in`/`slide-up` keyframes, the `.anim-d*` delay utilities) rather than inventing a parallel one; the new `.pending-*` rules are declared ABOVE `.anim-d*` on purpose, since the animation shorthand resets animation-delay and the delay utilities have to win. The sprout's stroke uses `pathLength={1}` so the draw keyframe can dash it without measuring, and its three subpaths run stem-then-veins so it reads as the sapling growing rather than a generic fade. Everything settles by ~540ms, single pass, no loop. Copy does the confirmation work the layout can't: "Your account is ready" states what actually happened, and "We'll email you" names the channel the old "We'll reach out" left vague. Both E2E anchors — `pending-gate` and `pending-signout` — are unchanged, and the new page.test.tsx pins them, the card surface, and the beat's finiteness so a future pass can't quietly reintroduce an infinite animation. part of #290 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ui): close review findings on the approval gate (#290) Restore the original copy. Issue #290's proposal says "Keep the copy" in so many words, and the previous revision rewrote both the heading and the body. The rewrite may still be worth making, but it is a separate call and not one this PR was scoped to take. Make the sprout actually sequence. The stem and the two leaf veins were three subpaths of ONE path sharing a single dash animation, and a dash pattern restarts its phase at every `M` — so all three drew simultaneously, which is the opposite of what the comment claimed. Verified in headless Chromium during review. They are now two separate paths on staggered delays, so the stem really does lead and the leaves follow. Split animation-fill-mode out of the shorthand. f4ac696 did exactly this to .fade-up ("split animation-fill-mode both so stagger works with CSS var timing") 20 minutes after the stagger utilities shipped, and every rule added since has avoided var()-timing plus a trailing keyword in one shorthand. It resolves correctly in Chromium — which is the only engine the e2e lane runs — but this file's own history says not to write it that way. Beat now settles at ~560ms (was ~540ms), still inside the issue's 600ms cap. part of #290 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* refactor(ui): extract the shared hero-card surface (#288) The warm gradient behind the sign-in and beta modals was inlined as a literal at five sites, and had already drifted at four of them: three different shadow alphas (0.12 / 0.15 / 0.18), two radii (20 / 24), and one card missing the inset highlight entirely. Meanwhile --surface-hero and --surface-hero-shadow sat in globals.css with zero consumers. Adds `.card--hero` (a named variant of .card, used as `card card--hero`) and `.hero-surface` (the gradient alone, for panels nested inside a hero card that must not restate its border, radius or shadow), plus a thin <HeroCard> wrapper that owns those classes and forwards everything else. Both classes are now the tokens' only consumers. This is a VISIBLE change, not a pure refactor, and an intended one: adopting the token moves the shadow hue from cool slate rgba(15,23,42) to the warm rgba(19,38,16) the rest of the app uses, unifies the alphas at 0.12, and gives the beta success modal the 24px radius and inset highlight its two siblings already had. The literal fallbacks in both CSS rules are deliberate. The tokens are scoped to `.public-surface, .landing-page`; every consumer sits inside that subtree today (verified — SignInModal is mounted only from the landing page, and nothing here portals to document.body), so the fallback is defensive rather than load-bearing. For a shared component it is the cheap guard against the first mount that isn't. data-testid="signin-modal" and "signin-close" are unchanged. Deliberately NOT folded in: the close button and the logo/wordmark row. The close buttons are not actually duplicated verbatim — they differ in offset (18 vs 14) and only one carries a testid — and the wordmark appears at 20 sites repo-wide with no shared component, so extracting it for 2 of them would leave a half-migration. #111 touches every icon site anyway; that is the coherent place for it. part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs+comments: close review findings on the hero-card extraction (#288) Tick [P1-E] in the token-unification follow-ups. This PR is what completes it, and the repo's convention is to fold that tick into the same PR — #486 existed purely to backfill a tick missed the same way. Name the shadow hue change for what it is. The comments described the adoption as de-drifting "three shadow alphas, two radii, one missing inset", which is accurate but incomplete in a misleading direction: the shadow's base COLOUR was the one value that had not drifted — all five sites agreed on slate rgba(15,23,42). The token is rgba(19,38,16) = --sap-900, the base the app's other shadows use, so adopting it re-tints these shadows rather than reconciling them. Deliberate, and called out in the PR body, but a reader of the code would have assumed the colour was untouched. Now stated in globals.css and HeroCard.tsx. part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(css): repair the hero-surface comment, and guard globals.css structurally (#288) The review-fix commit closed the hero-surface block comment early: the `*/` that used to end the whole comment stayed put while a new paragraph was inserted above it, so the rest of the original comment became raw stylesheet text. postcss reported it as "Unclosed string" at line 1007 — 780 lines below the actual mistake, at the first quote it happened to reach. It got that far because nothing in the fast lane reads this file as CSS. eslint lints JS/TS, tsc checks types, vitest never imports the stylesheet; CI's `lint + tsc + vitest` job went green on the broken file. The first thing that actually parses globals.css is the Next production build, which is why the local e2e cycle caught it and everything cheaper did not. Adds src/app/globals.test.ts: a dependency-free structural scan (comments, strings, brace depth) that fails on exactly this class of breakage, in the fast lane, pointing at the right line. Hand-rolled rather than importing postcss on purpose — postcss is only a transitive dependency here and the installed copy already drifts from CI's. It also tests itself: a second case feeds it each broken shape to prove it does not silently return "fine". part of #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* perf(frontend): #111 runtime animations + lazy image loading - Landing hero: squared-distance link pass with axis rejects (no more Math.hypot over all ~27k node pairs/frame); framer-motion split out of the landing chunk via next/dynamic (HowItWorks) with a layout-stable placeholder. - AtmosphericBackdrop: orb gradients pre-baked to offscreen sprites (re-baked only on DPR change), RAF throttled to ~30fps with drift speed preserved, loop paused while the tab is hidden. - KnowledgeGraph2D: simulation ticks write node/edge positions directly to the DOM (zero React work per tick), tooltip setState only on open/reseed with direct style writes while shown, id->node Map replaces O(E*N) find(); testmode tests read the new transform position carrier. eslint-suppressions baseline pruned (13->12). - Images: loading="lazy" + decoding="async" on all raw <img> sites; intrinsic width/height added where the rendered box is fixed (TopNav/SideNav logos, Settings avatar/banner) and deliberately omitted for natural-aspect user uploads (Social attachments, Admin screenshots). - Study: framer-motion subtree moved to StudyMotion.tsx behind next/dynamic (ssr:false) with visually identical fallbacks; the #383 skipAnimations test seam moved with it. Verified: 55 files / 399 vitest tests, eslint 0 errors, tsc clean, production build green. Local E2E lane not runnable on this machine (unprovisioned: no backend/.env, supabase CLI/config.toml mismatch, bash 3.2 parser bug in scripts/lib/local-common.sh:99); e2e.yml covers the lane on push to main. Closes#111 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(perf): #490 review round — sprite memory cap, un-gated Study panes, eager nav logos - AtmosphericBackdrop: cap sprite backing resolution at 512px (was full device resolution — ~100 MB of canvas memory at DPR 2 across 14 orbs); the soft gradients upscale indistinguishably. Also repaint the static frame on resize under reduced motion (canvas.width resets cleared it). - Study: the mode transition no longer routes pane content through the lazy motion chunk — next/dynamic's fallback can't carry children, which gated the panes behind the chunk fetch, and un-gating them remounted the pane mid-session, wiping state (caught by Study.test.tsx). Replaced with a keyed CSS enter-fade (`study-mode-enter`) that needs no chunk; AnimatePresence initial={false} semantics preserved via render-phase state derivation. StudyMotion.tsx keeps only the toggle highlight and the #383 test seam. - TopNav/SideNav: drop loading="lazy" from the always-visible nav logos (lazy only delays permanent chrome); keep decoding + dimensions. - Landing: document the ssr:false SEO tradeoff on the HowItWorks split. Verified: 399/399 vitest, eslint 0 errors, tsc clean, production build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(perf): #490 round 2 — revive backdrop on un-reduce, SSR the HowItWorks copy - AtmosphericBackdrop: disabling reduced motion mid-session now restarts the parked RAF loop (tick() parks itself on a still frame; the old always-running loop resumed implicitly, so the backdrop stayed frozen). - Landing: drop ssr:false from the HowItWorks dynamic import (#492 review) — next/dynamic still splits the motion stack into its own chunk, but the section's marketing copy is back in the server-rendered HTML; verified "Upload Your Materials" present in the prerendered index.html. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com>
…series table fallbacks (#491) * feat(analytics): #122 gaps — tokens/day chart, error-rate trend, day-series table fallbacks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(analytics): #122 review round — labeled disclosures, zero-fill, copy, compact-count boundary - DaySeriesTable and the two raw-table disclosures now carry per-chart labels ("View data: errors per day", …) so the five identical summaries are distinguishable to screen-reader users. - Errors panel zero-fills unconditionally: a no-error range renders real 0s and a flat line, not em-dash "unknown" cells. - Rate-chart fallback copy branches on summary error/loaded-empty/loading instead of always promising the series will load. - formatCompactCount promotes 999,950+ to "1M" instead of emitting "1000k"; boundary cases and the errorRateSeries domain-drop contract pinned in unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(analytics): #122 — guard the rate denominator against stale summary ranges The summary hook keeps its last payload while a range change reloads, so the errors panel could zero-fill an old-range series against the new range and fabricate an all-0% rate line. The join now requires day-key range agreement; mismatch renders the waiting state. Also: TruncatedBadge propagates to the rate chart when the summary scan was truncated (the denominator undercounts, so the rate can overshoot), and formatPct floors tiny nonzero rates at "<0.1%" so they never read as "no errors". Regression test pins the stale-range guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: AndresL230 <190146319+AndresL230@users.noreply.github.com>
…too (#493) `frontend/.npmrc` claimed engine-strict was a write-path-only guard because "`npm ci` ignores engine-strict in npm >=7". That is false for modern npm. Probed against a minimal package carrying this exact `>=10.9.0 <11` pin, on npm 12.0.1 / node v26.4.0: npm ci -> exit 1, EBADENGINE npm install -> exit 1, EBADENGINE npm_config_engine_strict=false npm ci -> exit 0 It also matches the field evidence: the #113 frontend audit (2026-07-03, npm 11.6.2) had to run `npm_config_engine_strict=false npm ci` — the bypass was needed on the READ path, which the write-path-only story never explained. That session lost time to an empty node_modules and a silently disabled lint gate, because the failure is quiet: lint and typecheck stop being a real gate instead of erroring. - .npmrc: replace the false mechanism block with what engine-strict actually does, the probe result, and the two real unblocks. - README.md: new "If your npm is 11 or newer" section so contributors hit the answer before the empty node_modules. Both keep the never-commit-an-off-pin-lockfile rule prominent — that is what breaks the Cloudflare Workers deploy and the whole reason for the pin. `.github/workflows/ci.yml`'s comment was already correct and is unchanged. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* refactor(frontend): group components/ by responsibility (#293) The top level of components/ held 58 files with no organising principle. Groups them per the issue's own proposal, as pure moves + import rewrites — zero behaviour change. marketing/ HowItWorks, SignInModal, HeroCard (+test) graph/ KnowledgeGraph, 2D, 3D (+3 tests) chat/ ChatPanel, MarkdownChat, MermaidBlock, FunctionPlot, AIDisclaimerChip, ModelToggle, SharedContextToggle, SessionSummary Top level: 58 -> 38 files. `ui/`, `flashcards/` and `screens/` are unchanged. The proposal also listed study/ and social/ groups; neither had any top-level members (those screens already live in screens/), so creating them would have made empty directories. Deliberately NOT done: renaming Gradebook/ to gradebook/. The proposal wanted "the two Gradebook levels merged", but they are already split cleanly by responsibility — components/Gradebook/ holds the parts, components/screens/ Gradebook/ holds the routes. The only remaining delta is letter case, and a case-only directory rename is a genuine cross-platform hazard for the macOS contributors on this repo. Not worth it for a cosmetic gain. Moves were done with `git mv`, so history follows the files. Four things had to move WITH the files, each of which would have failed silently or confusingly otherwise: - frontend/eslint.config.mjs — the testid-enforcement `files` array is keyed by path; stale entries would have silently stopped enforcing the rule on the exact files it was written to cover. - frontend/eslint-suppressions.json — also path-keyed. Stale keys surfaced as 22 lint errors, since the suppressions no longer matched any file. - docs/frontend-testids.md — the surface table names owning files. This is the other half of the testid convention. - HeroCard.test.tsx resolved src/ as `__dirname/..`, which silently became components/ once the file moved a level deeper. Now `../..`. Also refreshed the live path references in SECURITY.md and the graph journey's header comment. Historical records (docs/frontend-audit/, docs/decisions/, docs/superpowers/plans/) are deliberately left pointing at the old paths — they are dated snapshots of what was true when written, not live indexes. part of #293 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(test): repoint two vi.mock specifiers that the move left behind (#293) Both passed green while silently mocking nothing — the exact failure mode a move-only refactor hides, and the reason tsc/lint/vitest all being green did not prove the move was complete. Dashboard.test.tsx vi.mock("../KnowledgeGraph") -> "../graph/KnowledgeGraph" chat/ChatPanel.test.tsx vi.mock("./Icon") -> "../Icon" vitest does not error on a mock specifier that resolves to nothing; it just declines to intercept, so the real component rendered instead of the stub and the tests stayed green either way. tsc cannot see these because a vi.mock path is a string literal, not an import. Swept every relative vi.mock specifier in src/ against the filesystem: these two were the only unresolvable ones, and there are now zero. part of #293 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…481, part of #482) (#495) `course_chunks`, the `match_course_chunks` RPC and `CREATE EXTENSION vector` appeared in ZERO .sql files in this repo, yet all three are live in staging and production. Any database replayed purely from `python -m db.migrate` — local Supabase, the E2E stack, a fresh environment — therefore had RAG dead end to end, silently: retrieve_chunks swallows the RPC failure into [], _get_catalog_chunk degrades to "", indexing failures vanish into a fire-and-forget log line. The tutor just answers ungrounded, with no error, no metric and no user-visible signal. Migration 0039 codifies the extension, the table, its indexes and the RPC. Shape verified against LIVE production and staging by reading a real row and calling the RPC — columns, the 768-dim embedding, and the RPC's exact parameter and return names — rather than from the design doc, which describes intent while the database is what the code actually talks to. (The code's _OUTPUT_DIM is 768 and matches; a 3072-dim probe is rejected by the live RPC.) Every statement is IF NOT EXISTS / CREATE OR REPLACE, mirroring 0032's reconcile pattern, so it is a no-op where the objects already exist. Adds a `ragstore` oracle asserting the store EXISTS — the gap the issue names ("nothing in the suites or oracles asserts the table exists"). It checks the extension, the table and the RPC, deliberately not their contents: an empty course_chunks is normal on a fresh stack, a missing one is the bug. It also counts rows with a NULL embedding, which led to the write-path half. index_document_chunks upserted records whose embedding never landed — match_course_chunks ranks by vector distance and skips NULLs, so those rows were unretrievable by construction while still counting toward the "indexed N chunks" the caller logs. That is how a total embedding outage read as a complete success. It now drops them, logs how many, and reports only what was really indexed (part of #482). Two tests changed because they pinned that bug rather than a contract: test_index_document_chunks_handles_embedding_failure asserted the NULL rows were upserted, and the function-mode egress test asserted the same shape incidentally — its real subject, the absence of transport egress, is unchanged. part of #481 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…phase 1) (#496) Phase 1 of the landing redesign: dead-code removal only, no visual change. Removed, each verified unreachable rather than assumed: startCounters (18 lines) — its only caller was the `landing-stat-fade-up` branch of the fade-up observer, and that class appears in no markup anywhere. The observer itself stays; `.landing-fade-up` is live at three call sites, so only the stat branch and the dead selector go. the spotlight mouse-follow effect (42 lines) — opens with `if (cards.length === 0) return`, and `.landing-spotlight-card` is applied in no markup, so it early-returned on every mount. 16 orphaned CSS rules across .landing-btn-shimmer, .landing-spotlight-card and .landing-icon-container — zero consumers outside globals.css, in both the unscoped and the .landing-page-scoped copies. Net: 117 lines deleted, 1 added. One consequence worth recording, because it looks alarming in the diff: the eslint suppressions baseline swaps `react-hooks/immutability` for `react-hooks/set-state-in-effect` on (public)/page.tsx. That is not a new violation. The deleted code mutated DOM styles and drove requestAnimationFrame loops, which made React Compiler bail out on the whole component — masking a pre-existing `setState` inside the mount-time URL-error effect at :110. Removing the dead code made the file analyzable again and the latent violation surfaced. Verified by swapping main's page.tsx into this branch: with main's file the full lint is 0 errors, with mine it is 1. The pattern itself (reading window.location on mount) predates this PR and cannot move to lazy state init, since the initializer would also run during SSR where `window` is undefined. Suppression count for the file is unchanged: one out, one in. NOT done in this commit, deliberately — the wider de-dup of the unscoped vs .landing-page-scoped rule blocks. I verified it is safe in principle (all 21 unscoped landing selectors have scoped twins, the two apparent property gaps are comment-parsing artifacts, and no .public-surface content page uses a landing-* class), but the two blocks are interleaved with 11 unrelated rules so it cannot be a range delete, and the phase's own gate is "screenshot diff shows no change" — which I cannot run here, since Turbopack refuses the symlinked node_modules in a worktree. Left for a follow-up that can be screenshotted. part of #344 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* feat(ui): re-home onboarding onto app spacing/type tokens (#289) Onboarding was color-correct but structurally orphaned: 12 hardcoded px values, a bespoke type ramp (32 on welcome, 26 everywhere else — matching neither TopBar's 30 nor Dashboard's 42), and a centred card floating in its own radial void. The flow and steps are untouched; only the frame changed. Introduces the --fs-* type scale (option (b)). It is DERIVED, not invented: every step is a size the app already uses, counted across screens/ and components/ — 13px is the true body size at 107 call sites, 12/11/10 carry chrome and micro-labels, 14-16 body copy, 18-26 headings, 30 is TopBar's screen title, 32 the Dashboard display numeral. Deliberately NOT density-aware, unlike --pad-*. Retuning the whole type ramp with the density preference changes how much text fits on every screen — a real product decision, not a refactor. Additive to do later; shipping it now would hide a behaviour change inside a token introduction. Onboarding is the scale's first and only consumer. The rest of the app keeps its inline sizes until each screen is converted deliberately — a token set with zero consumers is exactly the state #288 just finished cleaning up. Also: every hardcoded padding becomes --pad-* (so onboarding finally responds to the density preference), the radial void becomes the app's own --bg, and minHeight moves 100vh -> 100dvh — the same iOS Safari lesson ShellFrame learned in #331, which onboarding never got because it renders outside (shell). One visible change worth calling out: step headings move 26 -> 30 to match TopBar. That is the alignment the issue asks for, not a side effect. Adds the first e2e journey over /onboarding. There was none — notable, since it is the first screen a newly-approved student sees and it renders bare, outside (shell), with no ShellFrame, nav or <main> padding to inherit; nothing in the suite would have noticed it breaking. It signs in as USER_NEW (the only seeded user with onboarding_completed=False) and asserts the card actually occupies the screen with resolved padding — a mistyped var() computes to 0px and silently collapses the box, which no unit test would catch. That made onboarding a driven E2E surface, so it joins the testid enforcement list, and its ten controls are tagged and registered in docs/frontend-testids.md — both halves. part of #289 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: close four review findings on the onboarding re-home (#289) Unique testids for repeated elements. Six of the ten testids sat on elements that render simultaneously — two TextInputs side by side in StepName, two TagInputs (majors + minors) each with their own add button and chips, five learning-style radios, N course results — so getByTestId could not disambiguate them. docs/frontend-testids.md's "Repeated / list items" rule already required a stable suffix and the codebase already had the precedent (upload-modal-course-result-${c.id}); I missed both. TagInput now takes a `field` prop for the same reason. eslint only checks that a data-testid is PRESENT, not that it is unique, which is why lint stayed green. Restored two-value paddings. Collapsing "40px 36px" to a single --pad-xl and "14px 12px" to a single --pad-md threw away deliberate vertical/horizontal asymmetry on the card every step renders inside. Both are two-value again. The remaining value shifts are inherent to adopting a quantised scale and are listed in the PR body rather than left to be discovered. Corrected a comment that asserted something I had not verified. The USER_NEW docstring claimed "approved so the middleware lets them past the pending gate" — but /onboarding is not in middleware.ts's PROTECTED list or its matcher, and Onboarding.tsx only redirects when unauthenticated, so the route renders for any signed-in user regardless of approval or onboarding state. USER_NEW is the right choice because it is the realistic first-run state, not because the route gates on it. Both the stack.ts comment and the spec docstring now say that. part of #289 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* feat(social): reserve space for chat image attachments (#315) Room-chat attachments rendered into a zero-height box until the image loaded. Combined with the `loading="lazy"` that #312 added, scrolling UP through history expanded each image as it neared the viewport and shifted the transcript mid-scroll. Chrome and Firefox mostly absorb that with scroll anchoring; Safari has none, so the viewport visibly jumps on every load. It also invalidated the loadEarlier scrollTop compensation, which measures scrollHeight before the prepended images have loaded. This could not be fixed in the frontend alone: room_messages stored only image_url, so there was nothing to reserve a box with. 0040 image_width / image_height on room_messages, nullable models the same two fields on SendMessageBody, optional social.py both columns in the select list and the insert api.ts sendRoomMessage takes an optional imageSize Social.tsx measures the file before upload; renders into an aspect-ratio box The measurement happens on the picked File via an object URL, before upload, so the dimensions travel with the message that creates it — history then renders reserved from the first paint rather than after a round trip. Degrades quietly by design, at every layer. The columns are nullable and unbackfilled, so every message written before this keeps NULL; readImageSize resolves undefined on a non-image or a decode failure rather than blocking the send; and the renderer only applies width/height/aspect-ratio when both values are present. Anything without dimensions renders exactly as it does today. Backfilling would mean fetching every historical image to measure it, which is not worth it for a layout hint. part of #315 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(social): bound the client-supplied image dimensions (#315) Review finding, and a real one: image_width/image_height arrive from the client and the transcript renders them directly as an aspect-ratio, so an absurd pair is a layout weapon against every member of the room rather than a bad row for its author. Unbounded, any room member could POST width=1, height=2000000000 — comfortably inside Postgres INTEGER range, so it inserts cleanly — and every viewer of that room gets a ~2-billion-pixel-tall bubble, with no edit path to recover short of a DB fix. That is a genuine escalation in blast radius: before this PR the worst a bogus attachment could do was render a broken-image icon. Bounded at three layers: models Field(gt=0, le=20000) on both, matching the Field(gt=0) pattern this file already uses for client-supplied numerics. 20000 is comfortably past any real image (8K is 7680). migration a CHECK constraint, matching the convention 0021 established. NOT VALID so it governs new writes without scanning existing rows — every pre-existing row is NULL in both columns, and NULL passes a CHECK anyway. Wrapped so a re-run is a no-op. render objectFit: contain and maxHeight, so dimensions that are merely WRONG (rather than absurd) letterbox instead of stretching, and a tall ratio cannot escape the box — maxWidth alone cannot cap height once the ratio drives it. Editing 0040 rather than adding 0041 because it has not been applied anywhere: it is unmerged, and the only database that has seen it is the local throwaway the from-empty replay rebuilds from scratch each run. Adds tests for the bounds — nothing covered these fields before. part of #315 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#499) * fix(study): keep the exam selected when opening a recent guide (#476) Opening a guide from the "Recent guides" rail left Regenerate permanently disabled. The cause is not the open path — openRecent sets courseId AND examId together. It's the courseId-keyed exams effect, which opened with an unconditional setExamId(""): a scope reset that cannot tell "the user switched course" (selection now invalid) from "we just opened a specific guide" (selection deliberate and valid). Both effects run in the same commit, so the loader still saw the intact pair and the guide loaded; only the NEXT render lost the exam. Hence the symptom — a guide on screen above a dead Regenerate button — rather than "nothing opens". It needs a course CHANGE to reproduce, which is why a rail entry for the already-selected course always worked (pinned as a control test). The reset now happens at the two events that mean it: the course picker's onChange, and a term switch. The term case adjusts state during render (the StudyModePanel pattern already in this file) rather than in an effect, because an effect-time reset lands a render late — the loader would commit one read of the old exam under the new term first. That was the same defect's second trigger, and it now has a test. Making Regenerate reachable on the rail path exposed a term hazard: it sent the ACTIVE selector's term, while a recent entry opens under its OWN term (#475 F1). Regenerating a Fall guide as Spring would rebuild against an offering the displayed guide never came from. Regenerate now replays the term the displayed guide was loaded with. Also seeds a CACHED study guide in the rich local dataset so the e2e journey can open the rail without generating (the study_guide agent has no function-mode handler). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(study): only reset the exam when the course actually changes Code review caught a regression this PR introduced. Moving the reset out of the courseId-keyed effect and onto the picker's onChange dropped a guard the effect had for free: setCourseId(sameValue) bails out, so the effect never re-ran. CustomSelect.commit() fires onChange for the already-selected option too, so re-confirming the course you were already on wiped the guide you were reading — the same class of bug as #476 itself. Guard selectCourse on an actual change, and pin it with a test. Also records the known cross-term edge the fix leaves standing: the exam OPTIONS follow the active selector by #475's design, so a rail entry opened under a different term is absent from that list and the picker shows its placeholder while the guide and Regenerate are live and correctly aimed at loadedTerm. Squaring it means tracking "the term I'm viewing" across list and loads, which is a #475 change rather than part of this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… (#500) * fix(e2e): poll for the tutor turn's rows instead of reading once (#477) The persistence assert read `messages` a single time, immediately after the reply text rendered. That treats "the reply is on screen" as "the rows are committed", and they are not the same signal. The server side is fine: stream_agent_turn persists inside on_complete and only THEN yields `done` (services/chat_stream.py:372-393), so the write does precede the end of the turn. But the composer renders off the streamed TOKENS, so both UI assertions can pass while `done` is still in flight — leaving a window where the one-shot read sees only the 4 seeded rows. That is exactly the observed failure (expected 6, received 4, UI assertions green). Replaces the read with a bounded expect.poll on the row count, the idiom events.spec.ts already uses for its fire-and-forget rollup. Root fix rather than a retry, per the #388 zero-flake policy. Note: a ~1-in-6 race is not reproducible on demand, so this is verified by the persist-before-done contract above plus repeated green cycles, not by a watched-red test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(e2e): capture the polled rows inside the predicate Code review: polling the count and then re-querying for the content assertions decoupled two checks the original single read had joined. A duplicate-persistence regression could land a row in that gap and still slice two valid-looking rows off the end — exactly what this journey exists to catch. Capture inside the predicate, matching events.spec.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…) (#501) * fix(rag): put retrieval/indexing failures on the app-logging path (#482) Item 1 of #482. rag_service reported every failure with a bare `print`, so a retrieval that blew up was invisible to app logging and uncountable by any rollup — and since retrieve_chunks degrades to [], which is also what "nothing relevant matched" returns, an ungrounded tutor/quiz turn was indistinguishable from a grounded one at every layer above it. Adds a module logger, moves all three print sites onto it, and emits rag.retrieval_failed / rag.chunks_dropped so the degrade rates are countable. Crucially these separate the DELIBERATE degrade from a real one. The #439 seam raises _EmbeddingDisabled whenever SAPLING_MODEL_MODE != real, which is every function-mode run — so a naive version emitted an error event on every e2e tutor turn and every e2e upload. That noise would have made the new signal worthless. _EmbeddingDisabled now logs at INFO and spends no event; only genuine failures warn and count. The two #439 transport guards caught this and are updated to assert the new channel (and to pin the no-event contract) — the invariant they protect is unchanged, only the output moved off stdout. Item 2 (poisoned embedding:None rows) was already fixed on main — the filter at the upsert drops them. Item 3 (durability inversion) is documented in architecture.md: indexing rides the NON-durable streaming route while DBOS wraps the sync route that never indexes, so a crash loses chunks behind a healthy documents row, and backfill_document_chunks.py is invoked by nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(rag): register the new events in the #117 taxonomy; pin the test's mode Code review, two real findings. 1. The two new event types were emitted OUTSIDE the pinned #117 taxonomy. log_event deliberately doesn't enforce membership (it must never raise), so nothing failed — they were simply undocumented and absent from the frozenset that exists to make a rename break loudly. Registered in EVENT_TAXONOMY, the docstring table, and the exact-set test. Also recorded why they are NOT named error.*: /api/admin/analytics/errors filters on `event_type like error.*` and projects an HTTP shape (path / method / status_code / duration_ms). Renaming would fill an HTTP-request table with null-path rows; these surface via /usage/summary's by_event_type instead. Giving that feed a shape-agnostic projection is the real fix and is out of scope here. 2. test_retrieve_chunks_failure_is_logged_and_counted depended on ambient SAPLING_MODEL_MODE. _require_real_mode() runs before the mocked client is reached, so with function mode exported — which the E2E workflow tells you to export — it took the _EmbeddingDisabled branch and passed vacuously on an unrelated path. Pinned to real mode; verified it now passes with SAPLING_MODEL_MODE=function set AND unset. Six PRE-EXISTING tests in that file share the same latent dependence (task_type, returns_count, chunk_ids, dedupe). Left alone — the hermetic lane runs with the var unset by contract — but worth its own cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Staging has avatars public=false while prod is public=true and 0029 records the intent explicitly. Avatar <img> src values are plain public object URLs, so a private bucket serves 400s and every avatar on staging renders broken. The reason this never self-heals is the interesting part: BOTH mechanisms that "create" the bucket decline to fix an existing one. - 0011_avatars_bucket.sql inserts it with public=true but ends in ON CONFLICT (id) DO NOTHING — a no-op once the row exists. - storage_service.ensure_bucket_exists (lifespan, public=True) treats the Storage API's 409 as success and deliberately does NOT overwrite settings, "in case an admin has intentionally tuned them in the dashboard". So a bucket that came into existence private stays private forever, through any number of deploys and migrations. An UPDATE is the only thing that corrects it — hence a new file rather than a re-run of 0011. This deliberately overrides that "an admin may have tuned it" stance for THIS bucket: the read path is unauthenticated <img src> against /storage/v1/object/public/avatars/..., so private isn't a valid tuning, it's broken avatars. Verified locally: from-empty replay applies the whole chain clean with 0041 in it; and against a bucket forced private to simulate the staging drift, the statement flips it to true, is idempotent on re-run, and is a safe zero-row no-op where the bucket doesn't exist. Does NOT fix staging by itself — that needs `python -m db.migrate` against that project. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Records the decision Andres already made, with the reasoning corrected.
The issue's premise ("pgvector similarity can't run over ciphertext") is
wrong: match_course_chunks ranks on 1 - (embedding <=> query_embedding) and
only SELECTs chunk_text as payload, and nothing queries it by content. So
encryption doesn't block retrieval and the decision is cheaper than stated.
But the same fact makes it partial, which is the part worth recording: the
embedding can't be encrypted (pgvector computes distance over it) and is
partially invertible back to its source text. So this restores boundary
consistency with documents.extracted_text — it does not make chunks
confidential, and the ADR says so explicitly rather than letting a future
reader assume otherwise.
Decided uniform (document AND catalog chunks) so the invariant is assertable
by the existing ciphertext oracle, and because decrypt_if_present's
raw-value fallback would make a real decrypt failure indistinguishable from a
legitimately-plaintext catalog row in a mixed table.
Ids stay computed on plaintext: AES-GCM's random nonce means identical text
encrypts differently every time, so ciphertext can never be a dedup key.
Flagged that scripts/dedupe_course_chunks.py:70 re-derives ids from the
STORED text and would destroy content-addressing if run against encrypted
rows.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>* docs(adr): 0025 — encrypt course_chunks.chunk_text (#484) Records the decision Andres already made, with the reasoning corrected. The issue's premise ("pgvector similarity can't run over ciphertext") is wrong: match_course_chunks ranks on 1 - (embedding <=> query_embedding) and only SELECTs chunk_text as payload, and nothing queries it by content. So encryption doesn't block retrieval and the decision is cheaper than stated. But the same fact makes it partial, which is the part worth recording: the embedding can't be encrypted (pgvector computes distance over it) and is partially invertible back to its source text. So this restores boundary consistency with documents.extracted_text — it does not make chunks confidential, and the ADR says so explicitly rather than letting a future reader assume otherwise. Decided uniform (document AND catalog chunks) so the invariant is assertable by the existing ciphertext oracle, and because decrypt_if_present's raw-value fallback would make a real decrypt failure indistinguishable from a legitimately-plaintext catalog row in a mixed table. Ids stay computed on plaintext: AES-GCM's random nonce means identical text encrypts differently every time, so ciphertext can never be a dedup key. Flagged that scripts/dedupe_course_chunks.py:70 re-derives ids from the STORED text and would destroy content-addressing if run against encrypted rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gradescope): rewire onto the enrollment-keyed schema (#265) routes/gradescope.py was never migrated past the DB redesign. It named FIVE things the schema doesn't have, so every link/sync/delete returned a PostgREST 400 in production — the same failure mode as #405: 1. table user_courses -> renamed to enrollments in 0020 2. gradescope_course_links.user_id / .sapling_course_id -> 0027 keys the table on enrollment_id 3. assignments.user_id / .course_id -> 0021 made assignments enrollment-scoped 4. table course_categories -> renamed gradebook_categories in 0021 5. assignments.source CHECK allowed only {manual,syllabus}, but the sync writes source='gradescope' — so even with the columns fixed there was no legal value to write. Migration 0042 widens it (strict superset, so the constraint is added VALIDATED). The issue asked to confirm the intended UX. Enrollment-keyed is right: grades land on enrollment-scoped assignments, so a link has to name the specific class instance — a retake gives two enrollments and an abstract-course link cannot say which term's assignments to write. The schema already said this; the code hadn't caught up. Per the repo convention the HTTP boundary keeps the abstract course_id and resolves inward via academics.enrollment_id_for, so the FRONTEND CONTRACT IS UNCHANGED — list_links maps enrollment rows back out to course ids. Also fixes the reason this drift survived the lane built to catch it. `RUN_INTEGRATION=1 pytest -m integration` — the documented invocation — silently ran NOTHING and exited 0. Collecting the whole tree imports tests/test_benchmark_quiz.py and tests/test_seed_quiz_fixture.py, which import scripts that call load_dotenv(".env.staging", override=True) at module import; that clobbered SUPABASE_URL for the session, so _require_local_stack skipped all 27 integration tests. The session fixture now re-asserts the local env at run time and RAISES rather than skipping when it is still wrong — the rule the file already applied to SUPABASE_DB_URL, now applied to SUPABASE_URL too. Documented form went from "1 passed, 27 skipped" to "28 passed". Verified: 4 new integration tests exercise the real column lists against real Postgres (the mocked suite structurally cannot — it agrees with whatever the caller asserts, which is why this shipped); from-empty replay for 0042; hermetic 1534 passed; browser 37/37; oracles clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gradescope): follow the existing link, don't re-derive the enrollment Code review found a real bug in the first cut — and it was the exact retake case I used to JUSTIFY enrollment-keying, which is why it's worth spelling out. enrollment_id_for re-derives ONE enrollment from the current term on every call. That's right for CREATING a link and wrong for finding one that already exists: the seeded user holds CS101 in both fall-2025 and spring-2026, so a link attached to the enrollment the heuristic doesn't pick was invisible to every write path. Re-linking created a SECOND row; DELETE removed nothing while still answering ok:true; sync 400'd "no link" for a course GET /links was happily listing. The GET/write asymmetry was the tell — list_links enumerates every enrollment, the writers only ever touched one. _all_enrollments_for() now spans every enrollment in the course. upsert_link deletes across all of them before inserting (so re-linking after a rollover leaves exactly one), remove_link deletes across all of them, and sync_course keys the whole run on the LINK ROW's enrollment_id rather than a fresh guess — grades belong to the class instance the link was made against. Two integration tests cover it, written against the enrollment the resolver does NOT prefer so the fixture is the hostile case. Both verified red against the pre-fix code (2 failed / 4 passed) and green after. Also from review: - list_links drops rows whose course can't be resolved instead of answering sapling_course_id: null — a null there reads as a real link the UI can't act on. - 0042 switches to NOT VALID. The old comment justified skipping it on the data ("nothing to grandfather"), which answered the wrong question: the reason that matters is that a plain ADD CONSTRAINT takes ACCESS EXCLUSIVE on assignments for the whole validation scan. NOT VALID still enforces every new and updated row, which is all a widened set needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* ci: apply pending migrations to staging on merge to main Nothing applied them. Verified all four places it could have happened and none did: the backend image's CMD is a bare uvicorn, there is no Procfile or release step, main.py's lifespan does not migrate, and the Supabase GitHub integration reads `supabase/migrations/` — the CLI convention — which this repo does not have (only config.toml and snippets live under supabase/, and schema_paths is empty). That is why its check reports "skipping" on every PR: it is connected but has nothing it recognises. Migrations here are raw DDL under backend/db/migrations/ applied by db/migrate.py against its own ledger. So a merge shipped code whose schema had not moved, and someone had to remember to run it. #504 is live proof: it merged code that writes source='gradescope' while the CHECK still rejects that value until 0042 is applied. STAGING ONLY, deliberately. main deploys staging; prod is a separate `production` branch promotion, and auto-applying irreversible DDL to prod on merge is a different risk decision. This runner has no down migrations. Two safety properties, both exercised against the real local database rather than assumed: - No secret set -> notice + skip, so adding this file changes nothing until STAGING_SUPABASE_DB_URL exists. - Preflight refuses to apply on a drifted ledger: a missing schema_migrations table (the #317 shape) or any recorded-but-absent filename fails the job with the offending name, instead of pushing more DDL on top of a history the repo and database already disagree about. Tested three ways: healthy (45 on disk / 45 recorded / 0 pending, exit 0), injected drift (exit 1, names the ghost row), and absent ledger (exit 1). The first draft queried a `version` column; the ledger's column is `filename`, which only the real-database test caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: restrict the migrate job to main; pin psycopg range Code review found a real hole. workflow_dispatch lets you pick ANY branch containing the workflow file, so a migration could be applied to shared staging straight from an unmerged branch, bypassing the push-to-main gate the whole design assumes. The bypass is not the worst part. The filename lands in schema_migrations, so if the file is then edited before merging — easy, since it was only "tested" — the merge never re-applies it, and staging silently diverges from the canonical file with NO pending/orphan signal, because the recorded filename still matches. That is precisely the immutability rule CLAUDE.md states, violated without a trace. Job-level `if: github.ref == 'refs/heads/main'` closes it. Also pins psycopg to >=3.2,<4 to match backend/requirements.txt, so the runner can't silently drift onto a major the app has never run against. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…et (#509) Sequential migration numbers are claimed when a branch is WRITTEN but only validated when it MERGES, so concurrent branches routinely pick the same one. PR #507 hit this twice in a single branch lifetime: first against main's 0042, then against an unpushed branch already holding 0043/0044 — invisible on GitHub, and only found because both had been applied to the same local database. New migrations now use a UTC timestamp prefix (YYYYMMDDHHMMSS_description.sql, `date -u +%Y%m%d%H%M%S`). There is no shared counter, so two branches would have to be created in the same second to collide. THE 45 EXISTING FILES ARE NOT RENAMED, AND MUST NEVER BE. `schema_migrations.filename` is the ledger's primary key and `pending_migrations` treats an unrecorded basename as unapplied, so renaming an applied migration makes the runner apply it AGAIN. 0021_gradebook.sql DROPs and re-CREATEs the assignments table — a bulk rename would destroy the gradebook on every environment that has already run it. The two conventions coexist permanently. Ordering holds, but for a narrower reason than "timestamps are longer": comparison is character-by-character, so length decides nothing — a year-1000 timestamp would sort BEFORE a 9999_ prefix. What actually holds is that every legacy file starts with "0" and every timestamp this millennium starts with "2". A test pins that reason, counter-example included, so the next reader does not re-derive the wrong one. (An initial version of this change asserted the length-based claim; its own boundary test falsified it.) Enforcement is a test, not a note: test_migration_naming.py fails if a new NNNN_ file appears. The existing prefix test in test_migrations.py had to be relaxed to accept both shapes — it would otherwise reject every timestamped migration. Full suite: 1542 passed, 38 skipped. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t URI (#508) * ci: the migrate secret must be the session-mode pooler, not the direct URI Found while running the migration by hand: `db.<ref>.supabase.co` publishes ONLY an AAAA record. This machine has no global IPv6 address (link-local only, despite an RA default route), so a direct connection dies with "Network is unreachable" before it authenticates. Canopy already recorded this from the #481 work — "direct psycopg to staging is blocked, IPv6-only endpoint" — but the workflow I merged in #506 told you to use the direct string, which walks straight into it. GitHub-hosted runners have no outbound IPv6 either, so its first real run would have failed the same way. The pooler hosts do publish A records, so the fix is the SESSION-mode pooler (port 5432), not transaction mode (6543) which drops the session-level behaviour psycopg and DDL rely on. db/migrate.py's "NOT the pooler" warning is about transaction mode and predates the IPv6-only endpoint; session mode behaves like a direct connection. The pooler also changes the username to `postgres.<ref>`, which is easy to miss. Corrects both the header rationale and the skip notice, so the thing you read when the secret is missing points at a host that is actually reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ops: pooler-URI builder and a read-only migration drift report Both came out of actually trying to migrate staging, and both encode something that cost real time to rediscover. pooler_url.py builds the SESSION-mode pooler URI from the password already in an env file, so the secret never has to be copied by hand. It takes the pooler host PREFIX rather than a bare region, because Supabase assigns projects to numbered clusters (aws-0-, aws-1-) and the number is not derivable from the region — staging is aws-1-us-west-2, which an aws-0- assumption gets wrong. migration_drift_report.py answers the question you must answer before applying a backlog to an environment that has been touched outside the repo (#317): is the ledger merely BEHIND, or is it LYING? It reports pending files, orphans (recorded here but absent from the repo — flagging filename NUMBER COLLISIONS, the dangerous shape), and any object a pending migration would create that already exists, noting whether that migration is IF NOT EXISTS-safe or would fail the whole run. Object lists are parsed from the migration SQL itself, so there is nothing to keep in sync by hand. Read-only: runs no DDL, safe against production. Verified against a real database both ways — clean (0 pending, 0 orphans, "behind, not lying") and with staging's shape simulated (unrecorded migrations plus a colliding orphan), where it correctly names the collision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ops: drift report also checks pending UNIQUE indexes against live data A UNIQUE index is the one thing IF NOT EXISTS cannot make safe: it still fails if the rows already present violate it. That is a DATA problem, invisible to a schema diff, and it is what turns a clean-looking backlog into a half-applied run partway through. The report now parses pending migrations for CREATE UNIQUE INDEX (including the partial-index WHERE clause) and runs the equivalent GROUP BY ... HAVING count>1 against the live table, naming the offending rows. Generic — it follows whatever happens to be pending rather than hardcoding today's case. Verified both directions against a real database: clean data reports none, and an injected duplicate is caught with the row identified (0036_offering_null_section_unique -> ('rich-course-math210','summer-2026',2)). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ops): stop double-encoding pooler passwords; finish the direct-URI sweep Self-review of this PR found four things, all of which undercut the PR's own premise that the next person shouldn't have to re-derive any of this. pooler_url.py double-encoded the password. urlparse() returns it STILL percent-encoded, and quoting again turned `p%40ss` into `p%2540ss`, so the URI authenticated as the literal escape text. It fails as "password authentication failed" — indistinguishable from simply holding the wrong secret, which is the expensive kind of wrong. Supabase generates passwords with reserved characters, so this was not hypothetical; it was waiting for the next rotation. Decode then re-encode, and pin it with tests, because nothing about the output looks wrong until you try to connect. The workflow's skip-notice hardcoded `aws-0-<region>` while pooler_url.py, added in the same PR, calls that a guess and records that staging is on `aws-1-`. Verified against both live projects: staging answers only on aws-1-us-west-2, production only on aws-0-us-west-2, same region. An operator copying the notice got "Tenant or user not found" — the exact failure class this PR exists to delete. The notice now points at the dashboard and at the builder script. db/migrate.py still told operators the opposite of the PR. Its docstring said "the direct connection string, NOT the pooler" and main()'s unset-variable error routed the reader to Connection string -> Direct. This PR's own repro was running `python -m db.migrate`, so that was the one path left misdocumented. The docstring now explains why the old warning existed (it is still right about transaction mode / 6543) and why it no longer decides the answer (the direct host went IPv6-only). Same correction in CLAUDE.md, README.md, and docs/staging/setup-checklist.md — the checklist being the document someone actually follows to set staging up. Two smaller things while in the same file: the drift report's docstring said "Three sections" after a fourth was added, and main() returned 0 even while printing orphans or data blockers. That second one is a trap for the obvious next refactor — having the workflow call this script instead of duplicating its preflight — which would have silently downgraded a fail-on-orphan gate into a report nobody checks. It now exits 1 on orphans, a non-idempotent collision, or a data blocker; PENDING alone stays clean, since being behind is not drift. Also folds in the one finding from #509's review that cleared review but landed after merge: CLAUDE.md's Commands section still said "add a new numbered file", which #509's own CI guard now rejects. Verification: 1550 passed, 38 skipped (8 new). ruff clean. Drift report re-run against live staging returns exit 1 and correctly names the 3 orphans. No request-path or schema change, so the e2e lanes have nothing to exercise here; the ledger reconciliation that follows will take the full cycle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…igrations (#316, #265) (#510) * fix(db): reconcile staging's ledger by recovering three out-of-band migrations Staging's schema_migrations holds three rows whose filenames exist nowhere in this repo, and `git log --all` finds nothing for any of them — they were applied out-of-band. migrate-staging.yml's preflight refuses to apply anything on top of that, correctly, so all 11 pending migrations are stuck. That includes the fixes for #316 (avatars bucket still private, every avatar renders broken) and #265 (assignments_source_check still rejects 'gradescope', so every synced row would violate it). Neither needs new code; both need this unblocked. filename is the ledger's primary key, which leaves exactly one reconciliation move that doesn't involve hand-editing a live ledger: restore the files under their exact recorded names. Staging then sees them as recorded-and-present, while prod and fresh local databases see them as pending and apply them for real. A timestamped name would leave the orphan in place AND re-run the DDL. All three are idempotent, because environments genuinely disagree about whether they ran. 0019_newsletter_approved_at is a no-op everywhere but the ledger — 0026_ops.sql already carries the column and says so in a comment. 0032_retire_summer_2026 changes data and behaviour, and is worth reading before it reaches an environment that matters. It moves Fall 2026's start_date back to absorb the Summer window, because 0019 seeds deliberately contiguous ranges so exactly one term contains any date; deleting Summer without that leaves a 98-day hole where current_term() returns nothing and resolve_offering can't place an enrollment. Consequence: a date in the old Summer window now resolves to Fall. 0033_offering_section_not_null is the one with an actual design argument. 0036 patched NULL-section duplicates with a partial index, but NULL was never the only way to say "no section" — all three seeders write '' while resolve_offering omitted the key and wrote NULL, so the pair 0036 exists to catch could sit in the table as ('' , NULL) and the index could not see it. Collapsing NULL into '' leaves one representation, covered directly by 0020's existing course_offerings_unique. resolve_offering needs no code change; its docstring did, since it cited 0036 by number for a guarantee that index no longer provides. 0036 stays (already applied elsewhere; applied migrations are immutable) and becomes a no-op, dropped by a timestamped migration so it can't be mistaken for the thing holding the invariant up. _LEGACY_NUMERIC_COUNT 45 -> 48 is the one sanctioned exception to #509's guard: these numbers were already spoken for by rows in a live ledger, so they are recovered history rather than newly claimed. The count stays closed at 48. The e2e lane caught the one real defect here: the rich seed pins an offering to summer-2026, so 0032 broke it with an FK violation surfacing as a 409. Retargeted to fall-2026 — the same thing the migration does to real rows. The su26 ids are deliberately NOT renamed: on an existing local database the old row survives and would collide with the new one on course_offerings_unique. Verification: 1553 passed, 38 skipped; ruff clean. Full flocked e2e cycle with a genuine from-empty replay (supabase db reset --no-seed, then all 49 migrations): up/reset/playwright/oracles all 0, 33 journeys green including semester-scope, oracles 0 findings. Stops short of prod deliberately — 0032 is a product decision, and prod's ledger state needs checking first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(db): refuse to collapse sections or retire Summer when rows would collide Self-review caught two real defects in this PR's own migrations, both invisible to every lane that verified it. course_offerings_unique is UNIQUE (course_id, term_id, section), and Postgres treats NULL as DISTINCT — so (c, t, NULL) and (c, t, '') coexist legally. That is exactly the mixed state 0033's comment describes, since the seeders write '' and resolve_offering wrote NULL. 0033's blind `UPDATE ... SET section = '' WHERE section IS NULL` collapses that pair onto one key and raises a duplicate key error. Two NULL rows for the same course+term collide the same way, and 0036 cannot prevent it because 0036 applies after 0033. 0032's summer->fall repoint has the identical shape: a course with an offering in both terms lands on an occupied key. Not exotic — resolve_offering(create=True) never sets section, so every app-created offering shares the same default. Either failure is worse than one bad statement. apply_migration runs the whole file plus its ledger INSERT in ONE transaction with no per-file recovery, so a collision rolls the migration back AND stops everything queued behind it. Neither verification lane could see this, which is the part worth remembering: the e2e replay starts from `supabase db reset`, so 0033 always ran against a zero-row table, and the hermetic suite mocks the DB layer entirely. "1553 passed, oracles 0 findings" was true and proved nothing here. Both migrations now detect the collision first and RAISE with the offending groups named. Deliberately not auto-merged: the colliding rows are two distinct offerings and enrollments/documents/notes hang off one id or the other, so choosing a survivor is a data call, not something a migration should do quietly. Verified against a scratch Postgres 15 by running the real migration files: 0033 colliding -> aborts, names course_id/term_id/rows, section still NULLABLE 0033 clean -> succeeds, nullable=NO default=''::text, 0 NULL rows 0032 colliding -> aborts, names the collision, summer-2026 still present 0032 clean -> succeeds, summer gone, fall start 2026-05-18, offering moved Also from the review: - CLAUDE.md and db/migrations/README.md still said the legacy NNNN_ set was frozen at 45. It is 48, and the reconciliation exception is now documented as the ONLY sanctioned reason to add one — previously that rationale lived just in a test comment and a PR description. - _PINNED_PAIRS now covers the three duplicate-prefix groups this PR creates, and _RECOVERED_ORPHANS pins all three recovered files by exact name. The count guard could not catch a rename: it would stay at 48 and pass CI while silently reopening the orphan the file exists to close. - The seed comment named the wrong constraint. The upsert conflicts on (course_id, term_id, section), so course_offerings_unique is the conflict TARGET and routes into an UPDATE; a renamed id actually fails on enrollments.offering_id's FK, which has no ON UPDATE clause. - routes/onboarding.py still described resolve_offering as creating a NULL-section offering. 1554 passed, 38 skipped; ruff clean. Full flocked e2e cycle green with the guards in place: from-empty replay applied all 49, 33 journeys passed, oracles 0 findings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Applying the reconciled backlog to staging died seven files in: ProgramLimitExceeded: memory required is 35 MB, maintenance_work_mem is 32 MB 0039_rag_vector_store builds an ivfflat index over VECTOR(768). Supabase defaults maintenance_work_mem to 32 MB, so the build cannot complete — and because apply_migration wraps each file plus its ledger INSERT in one transaction and run() has no per-file recovery, it took the remaining four migrations down with it. The ledger stopped at 44 of 49 with no partial state, which is the one good thing about that failure mode. This is not staging-specific. Any environment on the default hits it the first time 0039 runs, prod included, and prod has not been migrated yet. Set per session, not per environment. `ALTER DATABASE ... SET` only reaches backends started after it, and a pooled connection is frequently already established — observed directly against Supavisor, where a fresh backend picked up the new value while a reused one still reported 32 MB, making the change look like it had silently failed. A session-level SET always lands on the connection actually running the DDL. 128 MB is transient per-operation memory during an index build, not a reservation, and leaves headroom over 0039's ~35 MB without being reckless on a small instance. MIGRATE_MAINTENANCE_WORK_MEM overrides it. The SET is a literal rather than a bound parameter because SET does not accept one; the value is operator config, never request input, and a bad value fails loudly at the start instead of mid-migration. Verified by using it: staging's remaining 5 migrations applied cleanly, and the drift report now reports 49 on disk / 49 recorded / 0 pending / 0 orphans, exit 0. That also closed#316 (avatars bucket now public=true) and #265 (assignments_source_check now admits 'gradescope'). 1557 passed, 38 skipped; ruff clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#344 phase 2) (#513) * docs(spec): landing page below the hero (#344 phase 2) Interactive knowledge graph replaces the 340vh scroll-jacked HowItWorks, then three feature bands and a four-tile bento of real app surfaces. The structural argument is a density rhythm — the graph is the densest thing on the page, bands decompress, the bento re-energizes, and a closing band gives the CTA a run-up rather than ending on a grid tile. The content argument matters more than the visual one. Tutor chat, Notes, Gradebook and Flashcards are shipped surfaces the current six-feature list never mentions, so the page undersells the product more than it under-designs it. That is the likeliest root of #344's 'feels generic'. Live LLM generation on the public page was considered and rejected for now: most convincing option, but it puts an unauthenticated billable endpoint on the most-crawled page on the site. Recorded as a second pass behind a rate-limited endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(plan): landing interactive graph, step 1 (#344) Seven tasks: fixtures, layout+helix math, component, assembly, interaction, page wiring with the HowItWorks/catalog deletions, and the e2e journey. Self-review found one real gap and it is recorded in the plan rather than papered over: the spec lists drag alongside hover and expand, and Task 5 implements hover and the copy fade only. Dragging needs a pointer-capture and SVG coordinate-mapping decision (getScreenCTM) that deserves its own review gate, so it is called out as 5b/5c rather than hidden inside a step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(landing): course graph fixtures for the interactive demo (#344) * feat(landing): deterministic radial layout + helical entry path (#344) * feat(landing): knowledge graph demo — chips and laid-out render (#344) Static, fully laid-out KnowledgeGraphDemo component: a course-picker chip row plus an inline SVG render of the selected course's graph via radialLayout. This is the parked frame reduced-motion visitors and the E2E lane get; the assembly animation is a later task. Registers the landing-graph E2E surface (docs/frontend-testids.md + eslint.config.mjs) and adds the .landing-page-scoped chip/copy CSS. * feat(landing): helical assembly, parked under reduced motion (#344) Adds the RAF-driven helical assembly to KnowledgeGraphDemo, extracted into an AssemblingGraph child keyed by course id so switching courses remounts (fresh progress state, unmount-driven RAF cleanup) instead of resetting state inside an effect — avoids the react-hooks/set-state-in-effect anti-pattern that the brief's literal snippet would have tripped. Parked (progress=1, full opacity, laid out) whenever IS_TEST_MODE or prefers-reduced-motion. Adds a guarded window.matchMedia stub to vitest.setup.ts (jsdom has none) defaulting to reduced-motion=true, so every KnowledgeGraphDemo unit test asserts the parked/complete frame — the correct target for reduced-motion visitors and the E2E lane alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): SSR-safe reduced-motion detection for the graph demo (#344) Fix round 1: reading window.matchMedia directly in KnowledgeGraphDemo's render body computed "no preference" server-side (no window) but the real value client-side, producing a genuine React hydration mismatch for any reduced-motion visitor once this component mounts with SSR on. Adds usePrefersReducedMotion (frontend/src/lib/usePrefersReducedMotion.ts), following the same useSyncExternalStore + fixed getServerSnapshot pattern useIsMobile.ts already established for this bug class. Server snapshot defaults to true (assume reduced motion) rather than useIsMobile's false default: whichever direction is wrong pays a cost, and defaulting true means the cost lands on no-preference visitors (one extra replay of the entrance animation) rather than reduced-motion visitors (who would otherwise see a blank or mid-assembly graph on first paint). Also switches AssemblingGraph's progress from raw state to a value derived from `parked` at render time, so a post-hydration correction of `parked` (without a remount) resolves correctly in both directions instead of potentially getting stuck. Adds a renderToString -> hydrateRoot regression test using different matchMedia values across the two phases, verified to fail against the old render-body-read pattern before confirming it passes against the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(landing): cold-start the reduced-motion store cache per test (#344) Fix round 2: usePrefersReducedMotion's module-level MediaQueryList cache (correct for production) survived across it() blocks within KnowledgeGraphDemo.test.tsx, so the round-0 test 'parks fully assembled when reduced motion is requested' inherited an earlier test's warmed cache instead of its own local matchMedia override. It still passed, but for the wrong reason -- false confidence, not regression protection. Reviewer proof: inverting that test's override to report "no preference" left the whole file green. Adds a file-level beforeEach calling __resetReducedMotionStoreForTests() so every test in the file cold-starts the cache and actually depends on its own window.matchMedia. Removes the now-redundant reset from the round-1 describe block's nested beforeEach (the mid-test reset between its two installReducedMotion() calls stays, since a beforeEach only runs once per test). Verified by the same inversion the reviewer used: fails whole-file with the override flipped, passes again once restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(landing): graph hover blurbs and copy fade on engagement (#344) Hover a node to reveal its concept blurb below the graph; the first interaction fades the instructional copy for the rest of the session. Both `hovered` and `engaged` live in the KnowledgeGraphDemo parent, not in the AssemblingGraph child, since that child remounts (keyed by graph.id) on every course switch. engaged must survive that remount; hovered's home follows since the blurb paragraph renders in the parent too. AssemblingGraph just gets onNodeEnter/onNodeLeave callback props. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(landing): replace HowItWorks and the feature catalog with the graph demo (#344) * test(e2e): journey for the landing knowledge graph (#344) * fix(landing): viewport-gate the graph assembly, never render it blank (#344) Final whole-branch review of the landing knowledge-graph demo turned up six findings. They interact, so this is one wave. #1 The helix played where nobody could see it. The assembly effect fired on mount, so it burned its full 1100ms during the hydration window — under the page's own intro overlay, alongside the hero canvas RAF — and every visitor who scrolled down found progress === 1 and a static picture. Gate the RAF on an IntersectionObserver on the section. The root margin is a NEGATIVE bottom inset, not the positive lead-in that looks natural: the section sits directly after a min-h-screen hero, so its top edge is at exactly 100vh and any positive bottom margin re-creates the bug at scroll 0. #6 No-preference first paint went blank. usePrefersReducedMotion correcting its SSR-safe `true` to the real `false` dropped progress onto a raw animatedProgress of 0 — a committed frame with the whole graph at opacity 0 — and gating #1 would have turned that into a section that stays blank until scrolled to. animatedProgress is now `number | null`, and null (the assembly has never run) reads back as 1. Leaving the viewport mid-assembly settles on the complete frame rather than freezing a half-faded one. #4 The helix threw the outer ring outside the viewBox. `1 + (1 - e) * 0.9` put depth-2 nodes at y = 693 against a 560-unit viewBox, chopped by the svg viewport at opacity ≈ 0.5. It now contracts (0.55x → 1x) instead of stretching, so the whole sweep lives inside the disc radialLayout already fits to the frame. helixEntry(target, centre, 1) === target is untouched. #3 Illegible on phones. One 900x560 viewBox at every width renders at 0.38 scale on a 390px viewport: 4.6 CSS px labels, a 213px-tall smudge. Added a GraphView descriptor and a 360x300 phone view selected by useIsMobile — 0.95 scale, 12.35 CSS px labels, 22.8px dots, 285px tall. #2 The engaged copy fade failed WCAG AA permanently (engaged never resets). --text at 0.35 over the paper bg is 2.20:1 against a 3:1 bar. The fade moves to the headline alone at 0.55 (3.88:1). The eyebrow is not faded: at 0.7rem it needs 4.5:1, and --brand-forest only holds that to alpha 0.86 — the review's suggested 0.75 is 3.58:1 and still fails. #5 The named SSR guard didn't guard. public-seo.spec.ts asserted only Metadata API output, which survives `ssr: false` on the dynamic import. Added a raw-HTML assertion on the graph section's server-rendered copy. Tests: layout.test.ts sweeps every node of every fixture across the whole t range in both views and asserts circle + label extents stay in frame (this is the test that would have caught #4); KnowledgeGraphDemo.test.tsx drives a fake IntersectionObserver and captured rAF to pin "complete before armed, never blank", "no RAF until on screen", and the WCAG ratios computed from the rendered opacity; landing-graph.spec.ts measures label/dot/height in CSS px at 390x844. Each was confirmed to fail against the pre-fix code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): brand-conform the graph section and fit its frame (#344) Five visual/brand defects found by screenshotting the built page against the brand guide. Every automated gate was already green, so none of these were test-detectable. 1. TIER_COLOR was a fourth inlined copy of the mastery palette with four wrong literals — the landing page advertised different mastery colours than the product. Now consumes the canonical --state-* tokens (globals.css:80-89). The hero legend's four swatches get the same treatment (colours only). 2. The course root inherited its fixture tier's amber, so the section's focal point read as a warning. It now paints --brand-forest as an anchor; the fixture tier is unchanged. 3. The SVG used a hardcoded viewBox="0 0 900 560" stretched to a ~1184px container, reserving ~737px of height around content that clustered in the middle. The frame is now DERIVED (fitViewBox) from the drawn content — dot, label, halo — swept across the whole entry animation, unioned over all three fixtures so a chip click can't change the section height, once per breakpoint. Desktop resolves to "159 42 578 498", 36% narrower than the box it replaced. Fitted to the HELIX SWEEP, not the settled positions: helixEntry rotates 1.5 turns, so an outer node passes 0.925*maxRadius above and below the centre at ~83% opacity — 215 units against a settled extent of 58. A settled fit clips the assembly, which is #344 review #4 all over again. The bounds test is retargeted at the derived box, and a new paired test proves the sweep genuinely leaves the settled bounding box so that containment check stays load-bearing. The fit only pays off with a width cap — stretched to 1184px a tighter box renders at 2x and makes the section taller. Capped at md:max-w-[720px]. 4. The root's label lay along the outer-ring -> depth-1 edge, which crosses the root's x at cy + 0.349*ring in every fixture. It moves above the node on desktop; on the phone the ring is too small for that to clear the top child's label, so it stays below (where the diagonal doesn't reach it). All labels gain a paint-order halo in --bg-mesh. Two unreported collisions also fixed: "Hypothesis Tests" overlapped "Distributions" by ~21 CSS px at 390px, and the shipped phone geometry both overlapped and, under the fit, fell under the E2E height gate. New suite pins label/label, label/dot and label/edge clearance for every fixture in both views. 5. Left-aligned the graph to the headline's grid — the same width cap — so a left-aligned copy block no longer sits above a centred diagram. 503 unit tests pass (up 6), tsc and eslint clean. No new CSS, no new dependency, no assertion weakened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): take the outer ring off the horizontal axis (#344) The ring phase was `offset = d % 2 === 0 ? step / 2 : 0` against a −π/2 start. Depth 2 holds exactly two nodes in all three fixtures, so `step = π`, the half slot is `π/2`, and both outer nodes resolved to exactly 0 and π — dead on the horizontal axis through the centre. The settled layout was therefore a flat ellipse (aspect 0.40) inside a near-circular entry sweep (0.86), and since the viewBox is fitted to the sweep (correctly — a settled fit clips the assembly), the graph filled only 45% of its own box height and left a dead band above and below it. Every ring now starts three quarters of a slot back from 3 o'clock, so its angles are odd multiples of `step/4 = π/(2·count)` and can never be a multiple of π: no ring of any size flattens onto the horizontal axis. The half-slot alternation is kept for what it was for — it now applies only when a ring holds the same node count as the one inside it, which is the only case that lines up into radial spokes. Depth 1 is bit-identical to before (−3·(2π/3)/4 is exactly −π/2), so the reviewed triangle and every constraint the mobile geometry was tuned against are untouched. Depth 2 moves to the NW↔SE diagonal; the mirrored diagonal is not equivalent — the fixtures hang their outer nodes off the 12 and 4 o'clock children, so SW↔NE drags an edge through the root label (−7.2 units of overlap, measured). Desktop, re-derived from the new geometry and still fitted to the sweep: `159 42 578 498` → `185 33 526 516`; settled aspect 0.436/0.405/0.418 → 1.006/0.910/0.951; the drawing fills 73% of the frame height, up from 45% (515 of 706 rendered px, up from 277 of 620). Phone: `-19 5 394 319` → `-4 -1 364 330`, aspect 0.485/0.424/0.450 → 1.006/0.844/0.910, fill 48% → 73%, and every E2E legibility bar clears wider than before (12.8px labels, 25.5px dots, 301px tall). Worst label clearance is unchanged at +29.2 units desktop and +0.8 mobile — the new angles introduce no new binding pair. The "sweep leaves the settled box" guard is restated in units per side (59.0/59.3 desktop, 33.2/33.9 mobile) instead of as a ratio: the ratio was a proxy that shrinks precisely when the rest state stops being flat, which is the fix. New tests pin the no-horizontal invariant over ring sizes 1–12, the spoke-breaking branch, and the settled fill for every fixture × view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): lay the graph out as a radial tree, not concentric rings (#344) `radialLayout` placed nodes on global concentric rings: a node's angle came from its index within its depth, with no relationship to where its parent sat. On the shipped 6-node fixtures that scattered the graph — `cs-sorting` landed 171 units from `cs-arrays` on a 232-unit ring, `cs-trees` was flung to the opposite corner — and the picture read as a lopsided diagonal smear. Tuning the ring angles cannot fix it; the previous wave tried. Now: BFS a spanning tree over the undirected `edges` (never `children`, which is dead data that disagrees with them), root at the centre, depth-1 spread evenly around the circle, and every deeper node placed one ring step from ITS OWN PARENT inside a wedge centred on that parent's outward direction. The tree governs position only — every entry in `graph.edges` is still drawn, cross edges included. Tree-edge length, desktop: max 170.9 → 116.0, mean 128.5 → 116.0. The section gets shorter at the 720px cap: 706px → 663px. The depth-1 ring phase moves forward by one slot (`+step/4`, still an odd multiple, so the no-horizontal proof is unchanged) so the two branch-bearing children sit at 4 and 8 o'clock and grow downward, away from the root's label band. `MOBILE_VIEW` is retuned, not optional: in a tree the flattest of three arms is always 30° off horizontal, so a child's dot lands inside its parent's own 13-character label unless `0.866·ring > 3.9·font + halo/2 + nodeR`. The ring grows 132 → 163 and `nodeR`/`fitPad` shrink to buy it; the phone renders at 11.2px labels / 20.9px dots / 285px tall, still over the E2E legibility gate but with thin margins, documented at both ends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): grow the graph upward from its base, not down from the middle (#344) The radial tree put the course root at the frame's centre and spread its concepts around the full circle, so the drawing grew one arm up and two down — an inverted Y, or a root system. The product is called Sapling and the section headline is "Pick a course. Watch it grow." Only the ANGULAR DOMAIN changes. The BFS spanning tree, the parent-relative placement and the drawing of every edge (cross-edges included) are exactly as the previous wave left them. - depth-1 fans across the UPWARD half-plane, taking the interior gridlines of an (n+1)-way split of it: 45/90/135 degrees for three children. Never lands a child on the horizon at any fan size, and strictly further from it than the previous rule at every size. - seats are handed out outside-in, biggest subtree first, so the deep arms get the open sky and the composition stays mirror-symmetric. - the skeleton is laid out with the root at the origin and translated as a rigid body until its own bounding box is centred on the layout centre — the point helixEntry spirals around. The root ends up at the bottom-centre of the content and the sweep-fitted frame stays centred on the drawing. - both views retuned against the new geometry (the budget is now solved from the type scale, the label clearances and the phone's legibility floor, not inherited from the layout box), and the root's label moves below its circle in both: above is now the direction the plant grows in. Measured, desktop at the 720px cap: the <svg> is 637.6px tall against 662.8 (and 737 for the box this shipped with); cs210 fills 0.839 of the frame's width against 0.828; the worst label clearance goes 8.80 -> 30.14 units. Phone: 11.99px labels and 22.48px dots against 11.23/20.85, and the worst label clearance goes 0.99 -> 4.05 CSS px, retiring a margin that was inside the noise of whether Chromium paints a scrollbar. The frame's HEIGHT share does not improve (0.563 -> 0.532): fitViewBox fits to the entry sweep, the sweep is very nearly a disc, so dead vertical space is about (drawing width - drawing height)/2 for any layout and a canopy is wider than tall. Documented at the assertion that was relaxed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): tighten the graph's entry sweep to a quarter turn (#344) The frame is fitted to the helix sweep, not to the settled drawing — it has to be, or the assembly gets clipped mid-flight (#344 review #4). At 1.5 turns every node passed through every direction on the way in, so that sweep was very nearly a DISC and the fitted frame very nearly SQUARE around a canopy twice as wide as it is tall. The difference was dead paper: 162 and 136 CSS px of it above and below the drawing, which the previous wave measured across the whole layout family and logged as its closing concern — the lever is the sweep, not the layout. `helixEntry` now turns 0.25 (90°) and `ENTRY_CONTRACTION` is 1, so a node starts on the centre and the radial term collapses to the straight-line easing: the path is that line, rotated by a decaying quarter turn, and the widest swing happens where the node is nearest the centre. No layout, fixture, colour, copy, testid or component signature moved; every settled coordinate is byte-identical. desktop viewBox 161 17 603 534 -> 161 114 582 334 <svg> 720x637.6 -> 720x413.2 px (-224) band/side 162.2/136.0 -> 48.1/13.4 px phone viewBox -32 -25 443 365 -> -32 40 429 230 <svg> 332x273.5 -> 332x178.0 px (-95) band/side 63.7/54.2 -> 15.5/1.8 px The drawing did not shrink to get there — it grew, because the narrower frame renders every unit bigger: 669x339 -> 694x352 CSS px on desktop, 12.0 -> 12.4 px phone labels, 22.5 -> 23.2 px dots. Worst label clearance is unchanged in units (30.14 desktop, 5.40 phone) and better in pixels (36.0 -> 37.3, 4.05 -> 4.18). Tests: the mid-flight helix assertion is restated as shape rather than distance (>0.1 travel radii off the straight line, >5 deg of bearing swing; measured 0.172 and 11.25 deg) so it bites at 0.146 turns instead of passing anything; the sweep-vs-settled guard now asserts the top overhang with a number and builds the settled-fit box to show it clips, because the overhang is one-sided by construction (the fan's tips lift past the settled top when they rotate through the vertical; nothing can swing below the course code); the frame-height share floor goes 0.50 -> 0.75 (measured 0.851/0.903); and the phone's third bar stops measuring the FRAME's height (it was passing on the band) and measures the drawing's, with a frame floor kept at 170px. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(landing): restore atmospheric continuity between the sections (#344) Deleting #features and HowItWorks took the middle of the page's atmosphere with them, and nothing replaced it. The hero carries mesh-blob--1 and --2. The old #features carried --3 and --2, and HowItWorks carried a dark-green scroll tint. The CTA carries --1 and --2 PLUS a top gradient that started at rgba(20,83,45,0.08) on its very first pixel — because it was designed to blend DOWN out of that dark-green tint. So after the deletions the page ran: atmospheric hero -> a completely flat graph section -> a CTA whose green tint faded in from bare paper. Two hard seams, one on each side of the new section. The graph section now carries its own blobs at lower opacity than the hero's, so the graph itself stays the focus, and the CTA's wash starts transparent and peaks below the boundary instead of on it. Found by spinning up the dev server and looking at the whole page, not the section in isolation — the section screenshots I had been judging could not show a seam, because a seam only exists between two things. 94 graph tests pass; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(landing): three feature bands and a bento of built surfaces (#344) Step 2 of the below-hero redesign. The page now runs hero → graph → Universal Upload → Adaptive Quizzes → bento → Spaced Repetition → CTA. The three bands carry ONE arc — material in → practice → retention — rather than three disconnected pitches, and the closing band (not a grid tile) hands off to the CTA: a grid's last tile is a weak place to ask for a signup. Surfaces alternate sides, derived from position in `featureBands.tsx` rather than written down per band, so inserting a band can't silently put two surfaces in the same gutter. Every tile is a RECREATED PRODUCT SURFACE, never an icon over a heading over a sentence — the brand guide's hard anti-pattern. The seven recreations are faithful to the shipped screens: the upload modal's file rows and status wording, QuizPanel's radiogroup with its `A.` prefix and selected treatment, Study's rating trio, ChatPanel's asymmetric bubbles, the notetaker's linked-concept rail, Social's invite chip and sender names, and the gradebook's letter grade over real assignment rows. Brand constraints that shaped the code rather than just the CSS: - No glassmorphism. `.liquid-glass` is deliberately not reused; surfaces are solid warm paper with hairline borders. - Colour is state. Every mastery mark reads `TIER_COLOR` — the same map the graph section above paints its nodes with — and the only other hues are `--grade-*`. Unit-tested, so a raw hex can't creep back in. - Contrast forced two divergences from the app's own paint: the student bubble takes `--brand-forest` (6.4:1 under white) not `--accent` (4.04:1), and per-row grade letters keep `--text` lettering with the band on the border/dot, since `--grade-b` is 3.39:1 as a glyph. No new dependency, no framer-motion, no JS motion at all: the surfaces are static pictures, so `prefers-reduced-motion` and `IS_TEST_MODE` have no frame to park — there is only the complete one. The entrance is the page's existing `.landing-fade-up` observer, which degrades to "visible" rather than "invisible" when it never fires. All new CSS is inside the `.landing-page` scope and defines no tokens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(landing): give the knowledge graph product chrome, a dial and a rail (#344) The graph section was the least convincing thing on a page whose bands and bento each recreate a whole product screen. It was six flat circles and six words, floating naked on the page background, left-aligned in a 1184px container with the right 40% of the field empty, and its entire hover payoff was one line of body text swapping under the drawing. It is now one surface, full container width, split the way the app's own Tree screen is split. - CHROME. `KNOWLEDGE GRAPH` in the bento's mono micro-label, and the selected course's real numbers on the right — `MA 242 · 19 concepts · 50% mastery`, all read off the fixture. - THE NODES ARE DIALS. A tier-tinted disc at the full node radius, a neutral track ring, the mastery arc swept clockwise from 12 o'clock, and a solid core. Every dimension is measured INWARD from `nodeRadius`, so the drawn footprint is byte-identical to the flat disc it replaces: `fitViewBox`, `labelBaselineY` and the phone's 5.40-unit label clearance are untouched, and a node group's first `<circle>` is still the tier-painted disc the E2E legibility gate measures. - A LEGEND, which is a comprehension fix rather than decoration: four `--state-*` hues were carrying the whole meaning of the picture and nothing on the page said what any of them meant. It carries counts, so it reads as a readout, and it names the tiers in the app's own words. - AN INSPECTOR RAIL replaces the bare hover line: name, tier as a labelled chip, blurb, mastery meter, and the neighbours listed with their own scores, with the hovered node's edges lit in the canvas beside it. Never empty — at rest it shows the course. - AMBIENT DRIFT, ~3 units on a 13–22s per-node cycle with a negative delay so nothing starts in phase. A CSS animation, not a rAF loop: it stays out of the assembly's frame budget (which the suite counts to prove the helix neither fires early nor replays), and rides the same `parked` switch, so reduced-motion visitors and the E2E lane get nodes exactly on their laid-out points. Off below the mobile breakpoint, where the frame pad is 2 units and the worst label clearance 5.40. The fixtures gain a numeric `mastery` per node and a `conceptCount` per course, held to the product's own cutoffs by a port of `backend/config.py::get_mastery_tier`, so a ring can never say 90% while the paint says "struggling" and the chrome's percentage can never contradict the root's own dial. The assembly, its viewport gate, the negative bottom rootMargin and the never-blank derivation are untouched. The phone gate's three bars improve (12.68 / 23.78 / 182.3 against 11 / 20 / 170) because the canvas is full-bleed horizontally — deliberately, and the CSS says why. 576 tests green (+16), tsc clean, eslint clean on src/ and e2e/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | ec34bf1 | Aug 02 2026, 08:10 AM |
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Updates to Preview Branch (main) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 2, 2026
AndresL230 added a commit
that referenced
this pull request
Aug 12, 2026
* docs(plan): promotion runner implementation plan (#516) * feat(health): report the build commit so promotions can verify the deploy (#516) * docs(plan): fix contradictory nothing-to-promote test fixture (#516) * feat(promotion): read-only preflight guards for prod promotion (#516) Add backend/promotion/ package with preflight.py: project_ref, ledger_diff, staging_gap, scan_destructive (comment-stripping so the repo's explanatory migration headers don't trip false positives) and evaluate, which aggregates all guards into blocking findings before production is ever touched. The plan's original test_evaluate_reports_nothing_to_promote fixture left a migration pending while asserting the "nothing to promote" case, contradicting evaluate()'s own guard (commits_ahead == 0 and not pending). Fixed the test fixture rather than the implementation: "nothing to promote" must mean no commits AND no pending migrations, and a migration-only promotion (code level, schema behind) is real, distinct, and must still proceed. Added test_evaluate_allows_a_migration_only_promotion to pin that distinction. * fix(promotion): scan_destructive matches statements, not lines (#516) Line-oriented matching evaded detection whenever a destructive DDL statement's keywords were split across lines — this repo's own house style wraps ALTER TABLE/ALTER COLUMN clauses (see db/migrations/0012_gradebook.sql), so a wrapped ALTER COLUMN ... TYPE ... USING ... would have passed the guard silently. scan_destructive now splits comment-stripped text on ';' and whitespace-collapses each statement before matching, so wrapped and single-line statements match identically. Findings still report the line where the statement begins and the original (uncollapsed) source line. Added tests for a wrapped ALTER COLUMN ... TYPE, a wrapped DROP COLUMN, the real benign wrapped DROP NOT NULL shape from 0012_gradebook.sql (must stay clean — this is what keeps the guard from crying wolf), and that a wrapped statement's finding reports its starting line. Re-ran the migration sanity check against 0012/0013/0021/0030 — unchanged results plus 0012 now clean. * test(promotion): make wrapped DROP COLUMN test a genuine revert guard (#516) test_scan_destructive_flags_wrapped_drop_column previously wrapped "ALTER TABLE t" and "DROP COLUMN old;" onto separate lines, but DROP COLUMN itself sat wholly on one line — the pre-fix line-oriented scanner would have matched it directly, so the test didn't actually exercise the statement-split fix. Changed the fixture so DROP and COLUMN straddle the line break themselves ("ALTER TABLE t\n DROP\n COLUMN old;"), which the line-oriented scanner cannot match. Verified by hand: reverting scan_destructive to its pre-fix line-oriented form makes this test fail; restoring the statement-based version makes it pass again. * docs(plan): quote identifiers in Task 3 fake-cursor fragments (#516) * feat(promotion): prod snapshot capture and diff (#516) Adds capture()/diff()/format_diff() so an operator confirming a staging→production promotion sees exactly what a migration changed (new/dropped tables, row-count deltas, newly-applied ledger entries) instead of terminal scrollback. SELECTs only, nothing writes. * feat(promotion): durable post-deploy smoke checks (#516) * fix(promotion): drop unused pathlib.Path import in preflight test (#516) ruff check . flagged F401 — tests get their Path objects from pytest's tmp_path fixture, so the explicit import was dead weight. Scoping ruff to promotion/ during earlier fix passes never caught this since it never lints the test file. * docs(plan): fix Task 5 infinite wait loop, JSON spacing, and premature-PR assertion (#516) * feat(promotion): stage sequencing, confirmation gate and CLI (#516) Adds promotion/runner.py (Ports/Options + run(): preflight -> snapshot -> migrate -> snapshot -> ensure_pr -> the one confirm prompt -> merge with 502-retry -> wait-for-deploy -> smoke) and promotion/__main__.py (the real psycopg/git/gh/httpx ports). Test suite is fully hermetic: every side effect is an injected port. * docs(plan): align Task 6 runbook with the Task 5 fixes; fix nested fences (#516) * fix(promotion): confirmation-gate bypass, wrong deploy SHA, staging-check lie (#516) Code review on Task 5 found defects in the brief it was transcribed from: - ensure_pr queried --state all, so it could find a PREVIOUS promotion's already-merged PR and skip the confirmation prompt entirely. Now queries --state open only, creates + re-queries once (no recursion) if none found. - The deploy wait compared /api/health's reported commit against origin/main's tip, but `gh pr merge --merge` creates a merge commit ON production and Railway deploys production — every successful promotion would time out. Now re-fetches and waits on origin/production's tip. - A missing STAGING_SUPABASE_DB_URL (the default: no .env* ships it) made _staging_recorded() return an empty set, which preflight read as "staging ran nothing" and flagged every pending migration as a staging gap. Now returns None ("unknown") and preflight emits one honest staging-unknown finding instead of guessing. - subprocess calls swallowed real git/gh stderr behind "non-zero exit status"; added a _run() helper that surfaces it, and wrapped main()'s run() call so a RuntimeError prints cleanly instead of a traceback. - The "already-merged PR resumes at wait+smoke" resume path was unreachable (a real re-run sees commits_ahead == 0 and exits via nothing-to-promote first). Replaced with an explicit --verify-only flag that skips straight to wait+smoke against production's current tip. Also: FakeGit now returns distinct SHAs per ref (was returning the same SHA for every ref, which is why the tests didn't catch the wrong-SHA bug); FakeGh gained a revert() the runner must never call, making test_smoke_failure_does_not_revert_anything a real guard instead of an assertion that could never fail. * fix(promotion): honest merge-outcome-unknown message; --verify-only needs no DB (#516) Re-review of the prior fix wave found one new Important defect it introduced, plus three small follow-ups: - The merge-retry loop's new state-read except couldn't distinguish "the read failed" from "the read succeeded and said not-merged", so 5 consecutive gh pr view failures (a real GitHub API flake mid-promotion) would exhaust the loop and print "Production code unchanged" even though gh.merge may have landed on one of the attempts. Track whether any read actually succeeded; only claim "unchanged" when one did, otherwise print an explicit UNKNOWN-outcome message that tells the operator to check `gh pr view <N>` by hand and that the migrations are already applied either way. - --verify-only touches no database (it skips preflight/snapshot/migrate entirely) but main() was still rejecting it without SUPABASE_DB_URL. Scoped the credential check to the paths that actually need it. - _staging_recorded() could leak a raw psycopg traceback past main()'s RuntimeError handler on a bad STAGING_SUPABASE_DB_URL; wrapped it the same way _run() already wraps subprocess failures. - Strengthened the --verify-only test to assert the deploy wait and smoke checks actually executed (fetch called for /api/health and for a smoke-only path), not just that connect/migrate/gh were untouched. * fix(promotion): merge-retry state check must use only the latest read (#516) Second re-review found the previous fix's state_confirmed flag was sticky: set True by ANY successful post-merge gh pr view across the 5 retry attempts and never reset, so "read 1 succeeds (OPEN), reads 2-5 all fail" still printed "Production code unchanged" even though 4 more merge() attempts happened after the last confirmed state. That's the same false-claim class the round was meant to close; all-five-failed was just its narrowest instance. Replaced the sticky flag with the single current_state variable, which is reassigned every loop iteration (None on a failed read) and therefore reflects only the MOST RECENT read by the time the loop exhausts. Added test_merge_state_stale_confirmation_does_not_claim_production_unchanged for the mixed case, verified live to fail against the reintroduced sticky implementation and pass against the fix. Also strengthened test_verify_only_skips_promotion_and_just_waits_and_smokes: /api/health is also one of smoke's own CHECKS, so asserting it was fetched didn't prove the wait loop ran (a short-circuit straight to _run_smoke would pass too). Now forces the wait to iterate via a non-matching first health poll and spies on sleep(), asserting it was called at least once. Verified live to fail when the wait is bypassed and pass when restored. * feat(promotion): make promote target, runbook, and #515 artifact cleanup (#516) Wires backend/promotion (Task 5's __main__) to a `make promote` target, documents the CLI in a runbook and CLAUDE.md, and deletes the untracked hand-run #515 promotion artifacts (prod_snapshot.py, prod_db_check.py, smoke_prod_app.sh, smoke_prod_promotion.sh, prod_snapshot_*.json) now superseded by the package. apply_graph_edges_fix.py and graph_edges_fix.sql are a separate, unrelated production reconciliation and are left untouched. * fix(promotion): honest merge/migrate reporting, target line, level-DB path (#516) Whole-branch review found one more Critical (read staleness, not read failure) plus four Important gaps, two Minors, and the previously-deferred __main__.py coverage item. Critical: the merge-retry exhaustion message asserted "Production code unchanged" as fact even when every gh pr view read succeeded and said OPEN — but gh's own documented squash/merge 502 wedge is "error returned, merge lands anyway", so a merge triggered by this run can land seconds after the last successful read. Reworded to report what was observed ("as of the last check, it was OPEN... may still be landing... check gh pr view by hand") instead of asserting a fact this run cannot know. Verified live: reverting to the old wording makes the new test fail, restoring it passes. Important 1: a mid-migration failure (apply_migration commits per file, so partial progress is real and durable) used to propagate as a raw traceback, skipping the after-snapshot and partial-state warning entirely. Now catches it, reopens a fresh connection (the failed one may be left with an aborted transaction), re-captures, and reports exactly how many migrations landed and which one failed via the ledger diff. main()'s handler broadened from except RuntimeError to except Exception so no path exits as a stack trace. Important 2: prints "Target: project <ref> (<host>)" before the migrate stage — SUPABASE_DB_URL/SUPABASE_URL matching each other proves nothing if a whole .env file points at the wrong project. Important 3: commits_ahead == 0 with pending migrations used to apply the migrations successfully then die on `gh pr create` ("No commits between production and main"), reporting the whole run as failed. Now skips PR/merge/deploy-wait and goes straight to smoke (a migration that broke the running app is exactly what that stage exists to catch). Important 4: README step 5 called the prompt "the only irreversible step" when the actual irreversible step (the migration, step 3) already happened by then, contradicting the README's own line 44. Minors: __main__.py's _preflight_data now strips SUPABASE_DB_URL/ SUPABASE_URL the same way main() does before connecting (unstripped values silently no-op the target-mismatch guard); the dead already_merged branch (a real gh call that could abort the run before the operator ever saw the confirm prompt) is deleted now that ensure_pr is --state open-only. Coverage: new tests/test_promotion_main.py locks ensure_pr's `--state open` argv shape down with a faked _run, modeling this repo's real regression (a MERGED PR #515 must never come back as the PR to merge). Verified live: reverting to --state all makes the test fail, restoring it passes. * fix(promotion): honest revert text on the migrations-only path (#516) Final round on Task 5: the migrations-only path (commits_ahead == 0 with pending migrations, added last round) routed smoke failures through the shared _run_smoke, whose default text tells the operator to `git revert -m 1 HEAD` on production. On that path nothing was merged this run, so production's HEAD is a PREVIOUS promotion's merge commit — following that instruction would revert and force-push away an unrelated, previously working deploy. _run_smoke now takes merged_this_run: bool = True; the migrations-only call site passes False and gets its own honest failure text (schema moved, code untouched, no code revert applies, inspect the migration that landed). The normal post-merge path and --verify-only are unchanged and keep the original text, where it is correct. Per the coordinator, the same recipe reachable under --verify-only after a migrations-only promotion is parked, not fixed, this round. Also: the migration-failure message no longer says "PARTIALLY migrated" when 0 of N landed (says schema is UNCHANGED instead), and no longer blames pending[0] by name when nothing landed at all — db.migrate.run()'s prologue (SET maintenance_work_mem / ensure_tracking_table) can fail before touching any file, indistinguishable from a first-file failure from the runner's side, so it now says the run failed before applying anything and points at the Error line instead of guessing a filename. * fix(promotion): thread verify-only revert text, unstall subprocess, degrade staging reads (#516) PR #517 CodeRabbit review, including one item I'd previously parked on a factually wrong premise: --verify-only does not predate this diff (added in f3ab42f, which is part of this PR), so it needed the same fix as the migrations-only path. - runner.py: _wait_then_smoke now forwards merged_this_run to _run_smoke; --verify-only passes False, so a smoke failure there no longer hands out the git-revert-production's-HEAD recipe for a merge this invocation never made. _run_smoke's False-branch message generalized to cover both the migrations-only path and --verify-only accurately. - __main__.py: _run gains a timeout (120s default) so a stalled git/gh network call can no longer hang forever, possibly after the migration has already applied; a timeout now surfaces as a clean RuntimeError naming the command. _staging_recorded no longer re-raises on a staging connection/query failure (stale URI, paused project, transient fault) — it prints a warning and returns None so preflight produces its documented staging-unknown finding instead of aborting the whole run before the operator sees any report. _confirm is now a module-level, directly-testable function that treats EOFError from input() (no controlling terminal / CI without a tty) as a declined confirmation instead of letting main() print a blank "ERROR: " right after the migration landed. - config.py: build_commit() now strips RAILWAY_GIT_COMMIT_SHA and GIT_COMMIT_SHA independently before choosing between them — `A or B` picked a whitespace-only A over a valid B, since whitespace is truthy, silently reporting "unknown" and disabling deploy verification. Also folds in two CodeRabbit nits: test_promotion_main.py's duplicate promotion.__main__ import, and a lambda in test_promotion_runner.py that only wrapped a zero-arg callable. Declined per the coordinator: snapshot's count(*) full scans (already triaged) and CLAUDE.md's bare fence blocks (matches the file's own convention). * test(promotion): hermetic coverage for _run's timeout and _staging_recorded's degrade path (#516) The coordinator rejected "real IO, no dedicated test" for these two Major fixes from the PR #517 review — both are reachable hermetically by monkeypatching the one library call at the boundary, same pattern already used for Gh.ensure_pr's --state open test: - test_run_passes_a_timeout_to_subprocess / test_run_converts_timeout_expired_to_a_clean_runtime_error: fake promotion_main.subprocess.run, no process spawned. Verified live that dropping _run's timeout kwarg fails both (missing kwarg; a raw TimeoutExpired escaping uncaught). - test_staging_recorded_degrades_to_none_on_connection_failure: fake promotion_main.psycopg.connect to raise OperationalError, no database contacted. Verified live that reverting the except branch to re-raise fails it (RuntimeError propagates instead of None being returned). No production code changed — _run and _staging_recorded were already correct; only their test coverage was missing. * fix(promotion): close the 15 review findings — fail-closed guards, pinned merge, honest partial-state reporting (#516) Preflight now fails closed: unparseable project refs block instead of skipping the target-mismatch guard; the destructive-DDL scan gains ALTER TABLE ... RENAME and DROP VIEW, string-literal-aware comment stripping, and a TYPE pattern that no longer false-positives on columns named 'type'; new blocking guards require the local migrations dir to match origin/main and origin/production to be an ancestor of origin/main. The merge is pinned to the SHA preflight audited (--match-head-commit), with a deterministic fail-fast when main moved instead of burning the transient-502 retry loop. The deploy wait is tri-state — 'unknown' never satisfies it on first poll; an all-unknown window degrades explicitly to an UNVERIFIED report — and its timeout budgets wall-clock time via an injected monotonic port. Every post-migrate failure path (gh failure at ensure_pr, dead-DB recovery, post-merge git blip, Ctrl-C at the confirm prompt) now emits the 'migrations ALREADY APPLIED / schema is ahead of code' partial-state report instead of a bare traceback, and ensure_pr parses the PR number from gh pr create's own stdout with the list re-query as a bounded fallback. Ledger-diff primitives are extracted to promotion.preflight and consumed by scripts/migration_drift_report.py (byte-identical output) so the two can no longer drift apart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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
4 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.
Promotes the accumulated staging (
main) work to production: 285 commits, 630 files, +77,426/-8,215.Pre-merge verification (on
main@ ec34bf1)Full cycle run under the stack lock (
up=0 playwright=0 oracles=0).Production DB reconciled FIRST (expand-then-deploy)
Prod's schema had been built out-of-band and carried no
schema_migrationsledger, so it was missing objects this code depends on. Rather than replay 49 migrations blind, the ledger was reconciled the same way PR #510 fixed staging:Both data-migration guards (
0032,0033) were run read-only against prod beforehand and reported 0 offending groups.Data integrity: pre/post snapshots diff to zero row-count change on every table except the intended ones —
terms4→3 (summer-2026 retired), new emptyevents/llm_usage, ledger 0→49. All 8 user accounts, OAuth tokens, rooms, job applications and the 8,192-row course catalog are untouched.Schema now matches fully-migrated staging apart from cosmetic index/constraint name drift (prod's out-of-band build named PKs/indexes differently — functionally equivalent).
Post-merge
Railway (backend) and the Cloudflare Worker (frontend) redeploy from
production; both are smoke-tested after the deploys settle.🤖 Generated with Claude Code