Uh oh!
There was an error while loading. Please reload this page.
fix(backend): close the #136 contract-audit tail + #340 + #72 - #464
Merged
Conversation
Warning Review limit reached
Next review available in:54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughChangesThe PR fixes AI tool naming, Class Intel preference persistence and filtering, profile privacy, document upload failure handling, feedback authentication, admin allowlist listing, quiz idempotency and grading, and flashcard content-filter error propagation. Backend, frontend, unit, integration, and E2E tests are updated accordingly. Agent tool contracts
Class context and profile privacy
Document upload pipeline
Authentication and admin allowlist
Quiz submission correctness
Flashcard content-filter errors
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant UploadRoute
participant DocumentPipeline
participant Persistence
Client->>UploadRoute: upload document
UploadRoute->>DocumentPipeline: stream processing result
DocumentPipeline->>Persistence: persist document and graph updates
Persistence-->>DocumentPipeline: success or failure
DocumentPipeline-->>Client: done or error and done
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| """Integration tests for /api/flashcards/import/* routes.""" | ||
| import base64 | ||
| from unittest.mock import MagicMock, patch | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 8f2694a | Commit Preview URL Branch Preview URL | Jul 30 2026, 05:39 AM |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/test_flashcard_import_routes.py (1)
205-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize flashcard agent content-filter mocks through the shared gemini fixture.
backend/tests/conftest.pyonly has the hardBaseApiClienttransport guard; the ContentFilterError behavior still needs an explicit shared fixture/parameterized helper soroute::test_content_filter_block_returns_502,service::test_content_filter_propagatesgeneration, and cleanup content-filter smoke cases don’t each define a separateflashcard_agent.runmonkeypatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_flashcard_import_routes.py` around lines 205 - 207, Centralize the ContentFilterError behavior in a shared gemini fixture or parameterized helper in backend/tests/conftest.py, then update the flashcard agent run mocks in backend/tests/test_flashcard_import_routes.py:205-207 and backend/tests/test_flashcard_import_service.py:320 and :397 to use it instead of defining separate flashcard_agent.run monkeypatches; preserve the existing route, service-generation, and cleanup smoke-test expectations.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/services/flashcard_import_service.py`:
- Around line 271-276: Update the ContentFilterError handler in the flashcard
import flow to log only a sanitized event/category, replacing logger.exception
so UnexpectedModelBehavior.body and other raw provider details are not emitted.
Preserve the existing re-raise behavior so the route’s 502 handling remains
unchanged.
In `@frontend/src/components/SharedContextToggle.tsx`:
- Around line 18-70: Reset or re-scope dirtyRef in useSharedContext whenever
userId changes, so a toggle by one user does not suppress server hydration for
another user. Ensure the new user’s fetchSettings result can update enabled and
localStorage unless that same user has toggled locally.
---
Nitpick comments:
In `@backend/tests/test_flashcard_import_routes.py`:
- Around line 205-207: Centralize the ContentFilterError behavior in a shared
gemini fixture or parameterized helper in backend/tests/conftest.py, then update
the flashcard agent run mocks in
backend/tests/test_flashcard_import_routes.py:205-207 and
backend/tests/test_flashcard_import_service.py:320 and :397 to use it instead of
defining separate flashcard_agent.run monkeypatches; preserve the existing
route, service-generation, and cleanup smoke-test expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 732941d8-6d0b-401e-a8af-976530a519e3
📒 Files selected for processing (25)
backend/agents/chat_tutor.pybackend/agents/note_chat.pybackend/db/migrations/0037_share_class_context.sqlbackend/models/__init__.pybackend/routes/admin.pybackend/routes/documents.pybackend/routes/feedback.pybackend/routes/profile.pybackend/routes/quiz.pybackend/services/course_context_service.pybackend/services/flashcard_import_service.pybackend/tests/test_admin_routes.pybackend/tests/test_chat_stream.pybackend/tests/test_chat_tutor_imports.pybackend/tests/test_documents_routes.pybackend/tests/test_feedback_routes.pybackend/tests/test_flashcard_import_routes.pybackend/tests/test_flashcard_import_service.pybackend/tests/test_model_mode_seam.pybackend/tests/test_profile_routes.pybackend/tests/test_quiz_routes.pybackend/tests/test_shared_course_context.pyfrontend/e2e/quiz.spec.tsfrontend/src/components/SharedContextToggle.test.tsxfrontend/src/components/SharedContextToggle.tsx
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
18
to
70
| export function useSharedContext(): [boolean, (v: boolean) => void] { | ||
| const { userId, userReady } = useUser(); | ||
| const [enabled, setEnabled] = useState(true); | ||
| // Once the user toggles locally, a late-arriving server hydration must not | ||
| // clobber their fresh choice. | ||
| const dirtyRef = useRef(false); | ||
| useEffect(() => { | ||
| const raw = localStorage.getItem(STORAGE_KEY); | ||
| if (raw === "false") setEnabled(false); | ||
| }, []); | ||
| // Best-effort server hydration (#72): the preference persists on | ||
| // user_settings so it follows the user across devices and gates the WRITE | ||
| // path server-side. On any failure — offline, signed out, or a server that | ||
| // does not serve the column yet — the localStorage value above stands. | ||
| useEffect(() => { | ||
| if (!userReady || !userId) return; | ||
| let cancelled = false; | ||
| fetchSettings(userId) | ||
| .then((settings) => { | ||
| if (cancelled || dirtyRef.current) return; | ||
| const server = (settings as ShareClassContextSettings).share_class_context; | ||
| if (typeof server === "boolean") { | ||
| setEnabled(server); | ||
| localStorage.setItem(STORAGE_KEY, String(server)); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| /* keep the localStorage value */ | ||
| }); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [userReady, userId]); | ||
| const update = (v: boolean) => { | ||
| dirtyRef.current = true; | ||
| setEnabled(v); | ||
| localStorage.setItem(STORAGE_KEY, String(v)); | ||
| // Best-effort write-through (#72). Swallow failures: the local toggle | ||
| // still gates this client's read path when offline or when the server | ||
| // does not accept the field yet. | ||
| if (userId) { | ||
| const patch: ShareClassContextSettings = { share_class_context: v }; | ||
| updateSettings(userId, patch).catch((err) => { | ||
| console.warn( | ||
| "Failed to persist share_class_context; toggle applied locally only", | ||
| err, | ||
| ); | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== locate file =="
fd -a 'SharedContextToggle\.tsx$'.||trueecho"== git diff stat =="
git diff --stat ||trueecho"== outline =="
ast-grep outline frontend/src/components/SharedContextToggle.tsx --view expanded ||trueecho"== source lines =="
cat -n frontend/src/components/SharedContextToggle.tsx | sed -n '1,140p'echo"== related files/usages =="
rg -n "useSharedContext|SharedContextToggle|share_class_context|fetchSettings|updateSettings|useUser" -S frontend/src ||trueRepository: SaplingLearn/Sapling
Length of output: 19062
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== locate login/user/account switch references =="
rg -n "logout|sign(out|out)|sign in|login|account|userId|userReady|App|Router|routes|route|useRoute|useLocation|useNavigate|navigate\\(" -S frontend/src ||trueecho"== files =="
git ls-files frontend/src | sed -n '1,240p'Repository: SaplingLearn/Sapling
Length of output: 50376
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== UserContext outline/sections =="
wc -l frontend/src/context/UserContext.tsx
cat -n frontend/src/context/UserContext.tsx | sed -n '1,260p'echo"== Learn screen hook sections =="
wc -l frontend/src/components/screens/Learn.tsx
ast-grep outline frontend/src/components/screens/Learn.tsx --view expanded ||true
cat -n frontend/src/components/screens/Learn.tsx | sed -n '320,350p'
cat -n frontend/src/components/screens/Learn.tsx | sed -n '1205,1232p'
cat -n/frontend/src/components/screens/Learn.tsx | sed -n '1270,1285p'2>/dev/null || cat -n frontend/src/components/screens/Learn.tsx | sed -n '1270,1285p'echo"== route files around learn auth check =="
fd -a 'page\.tsx$|layout\.tsx$' frontend/src/app | rg '/app/.*/|app/'||true
rg -n "useSharedContext|SharedContextToggle|userReady|isAuthenticated|signOut|router.replace|router.push|onboarding|dashboard" frontend/src/app frontend/src/components/screens/Learn.tsx frontend/src/context/UserContext.tsx -S
echo"== app shell/public auth-related snippets =="forfin frontend/src/app/\(public\)/page.tsx frontend/src/app/\(shell\)/page.tsx frontend/src/app/\(shell\)/notetaker/page.tsx frontend/src/components/screens/Learn.tsx;do
[ -f"$f" ] && { echo"--- $f"; cat -n "$f"| sed -n '1,140p'; }
doneRepository: SaplingLearn/Sapling
Length of output: 42699
Reset the dirty flag for each user.
update() makes dirtyRef.current permanent for the component instance, so after user A toggles it once, user B’s later server hydration can be skipped silently and B may see A’s stored preference instead of their own share_class_context. Reset the dirty flag when userId changes, or scope it to the current user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/SharedContextToggle.tsx` around lines 18 - 70, Reset
or re-scope dirtyRef in useSharedContext whenever userId changes, so a toggle by
one user does not suppress server hydration for another user. Ensure the new
user’s fetchSettings result can update enabled and localStorage unless that same
user has toggled locally.
AndresL230
commented
Jul 29, 2026
CollaboratorAuthor
Review pass complete: 5-agent review + confidence scoring produced two findings, both at 75 (below the 80 posting bar) and both fixed anyway in the commit above — (1) the #129 guard was check-then-act under concurrency; the gate is now an atomic conditional update (completed_at is.null), loser 409s before any mastery write; (2) flipping share_class_context now schedules update_course_context for every enrolled offering, so opted-out data leaves the aggregates immediately rather than at the next classmate-triggered refresh. Backend 1348 passed + ruff clean. Full e2e cycle queued behind the in-flight B2 cycle. |
AndresL230 added a commit
that referenced
this pull request
Jul 29, 2026
…red aggregate refresh - #129 hardening: the completed_at pre-read was check-then-act — two concurrent submits (double-click) both passed it and double-applied mastery. The gate is now an atomic conditional update (completed_at is.null) whose loser 409s before any mastery write; the final update no longer re-stamps completed_at. Quiz test fixtures updated to model PostgREST's return=representation (a matched update returns rows); two new tests pin the race loser and the is.null claim idiom. - #72 completeness: PATCHing share_class_context now schedules update_course_context for each of the user's enrolled offerings (deduped, BackgroundTasks) — opted-out data drops out of the aggregates immediately instead of lingering until a classmate's activity fires the next refresh. Tests pin per-offering scheduling and that other settings don't trigger it. Backend 1348 passed, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| from typing import Optional | ||
| from fastapi import APIRouter, HTTPException, Request, UploadFile, File, Query | ||
| from fastapi import APIRouter, BackgroundTasks, HTTPException, Request, UploadFile, File, Query |
#129#132#135) Seven verified-still-present findings, each fixed TDD red-first: - #130: GET /api/admin/allowlist existed only in the frontend client — added the admin-gated listing over newsletter_emails (id,email, created_at,approved_at desc), matching the AllowlistEmail shape. - #134: submit_feedback/submit_issue_report now derive user_id from the session (401 unauthenticated; body.user_id accepted but never trusted). get_public_profile now resolves the viewer: private profiles return a minimal stub to non-owners (name/majors/minors/year/school no longer leak), and the 0031 'school' tier — previously identical to public — reveals extended fields only to same-school viewers (academics school-peer resolver, fail-closed). Owners always see everything. - #129: resubmitting a completed quiz now 409s before any scoring — no re-applied mastery, no duplicate node_mastery_events, no achievement re-fire (409 over replay: quiz_attempts stores no mastery_before/after). Malformed items (no correct option) can no longer match a missing answer ('' == '') for a free point. Lane journey pins the 409 + single-event contract by replaying the captured wire body via page.request. - #132 remainder: the streaming upload's post-result persistence block is now its own try — a failure after the result event yields one terminal error, never a second result from a second (billed) legacy run; the streaming size-limit 400 said '15 MB' with a 100 MB cap — both routes now derive the message from MAX_FILE_SIZE; the remaining synchronous PostgREST calls in BOTH async upload paths moved to asyncio.to_thread. - #135: note_chat + chat_tutor register the retrieval tool under the prompt-facing name search_course_materials (explicit Tool(name=…), fresh instances per agent); seam/stream/import tests updated with the wire rename; evaluator semantics preserved (verified, no eval changes). - #340 (P1): ContentFilterError (a UnexpectedModelBehavior subclass) no longer degrades to [] — content-filter blocks propagate to the routes' 502 instead of a 200 {"cards": []} the UI toasts as success. - #72: Class Intel opt-out is now a persisted preference — user_settings.share_class_context (migration 0037, default true), PATCHable via the settings whitelist, honored at the single write chokepoint (update_course_context filters aggregation to opted-in users; all-opted-out purges the aggregates). SharedContextToggle keeps its hook/localStorage API and adds best-effort server persistence + hydration. Suites: backend 1344 passed + ruff clean; frontend 258 passed + tsc + lint clean; agent-adjacent files re-verified green under the lock-pinned pydantic-ai 1.107. Closes#130. Closes#134. Closes#129. Closes#132. Closes#135. Closes#340. Closes#72. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…red aggregate refresh - #129 hardening: the completed_at pre-read was check-then-act — two concurrent submits (double-click) both passed it and double-applied mastery. The gate is now an atomic conditional update (completed_at is.null) whose loser 409s before any mastery write; the final update no longer re-stamps completed_at. Quiz test fixtures updated to model PostgREST's return=representation (a matched update returns rows); two new tests pin the race loser and the is.null claim idiom. - #72 completeness: PATCHing share_class_context now schedules update_course_context for each of the user's enrolled offerings (deduped, BackgroundTasks) — opted-out data drops out of the aggregates immediately instead of lingering until a classmate's activity fires the next refresh. Tests pin per-offering scheduling and that other settings don't trigger it. Backend 1348 passed, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230force-pushed
the
fix/b3-backend-contract-tail
branch
from
July 30, 2026 05:37
37ecc92 to
8f2694aCompareAndresL230
commented
Jul 30, 2026
CollaboratorAuthor
Pre-merge e2e gate: full lane 20/20 passed (including the new #129 resubmit journey; migration 0037 replayed in the stack's from-seed boot) + oracles clean (0 findings, 1 allowlisted). Merging. |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 30, 2026
Closed
Closed
Closed
This was referenced Jul 30, 2026
AndresL230 added a commit
that referenced
this pull request
Jul 30, 2026
…f, validated procedure (#154) (#474) * feat(durability): productionize DBOS — entrypoint wiring, resume proof, validated procedure (#154) ADR 0011 shipped the durable shim off + unvalidated: main.py never constructed/launched DBOS (the shim 'trusted' an init that didn't exist), the DBOS_DATABASE_URL precondition was docstring-only, and nothing tested either mode. - services/durable.py: enforce the DATABASE_URL precondition at activation; new init_dbos()/shutdown_dbos() — construct + DBOS.launch() from the lifespan, fail-loud when the operator explicitly opted in (#174 posture). Decorate-before-construct-before-launch order verified against dbos==2.28.0's registry internals. - main.py: init_dbos() after validate_config(), shutdown_dbos() on teardown. - requirements-durable.txt: the opt-in extra ADR 0011 promised (dbos>=2.28,<3), never in requirements.txt/lock. - tests/test_durable_shim.py: 11 hermetic tests over both modes and every precondition combination (fake dbos module + reload, pristine restore). - tests/test_dbos_resume.py: opt-in (RUN_DBOS_RESUME=1) subprocess crash/ resume proof — step1 runs EXACTLY once across an os._exit crash and the workflow completes on relaunch recovery — plus process_document parity under real DBOS. Exercised against dbos 2.28 + a real Postgres. - test_documents_routes.py: streaming replay now also pins zero re-inserts (the #132 crash-after-result scenario, with the #464 exactly-one-result guard). - ADR 0011 → accepted (shipped + validated, default off): corrected activation procedure (launch() migrates + auto-recovers; 'dbos migrate' was stale), resume monitoring (startup INFO line -> Logfire per #119, workflow_status SQL), streaming-asymmetry reaffirmed. Closes#154 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * review: make durability real for the product + fix fail-loud gap and doc claims - /upload/sync now pins the DBOS workflow id to doc:{user_id}:{request_id} (user-scoped — X-Request-ID is client-supplied), so a client retry attaches to the SAME workflow: completed -> recorded result, crashed -> resume at last completed step. Graph merge wrapped as _step_apply_graph so resume never re-runs the one real side effect. - init_dbos now RAISES on flag-on-but-preconditions-unmet (missing URL / failed import) instead of silently degrading an explicit opt-in; shim tests pin both raise paths. - Doc corrections: pre-#154 flag-on raised DBOSException per call (502s), not 'silently nothing'; step-outside-workflow runs the plain function on dbos 2.28 (not undefined); only database_url is deprecated. - New opt-in proof test_pipeline_crash_resume_via_workflow_id: real process_document crash mid-_run_workers -> same-id retry -> classify step runs EXACTLY once across both phases. All 3 resume tests exercised green against dbos 2.28 + a real Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 30, 2026
AndresL230 added a commit
that referenced
this pull request
Aug 13, 2026
…_responses, answer-key deprecation (#541) (#549) * feat(quiz): server-authoritative grading — per-question answers, quiz_responses, answer-key deprecation (#541) Workstream C of the pre-revamp quiz repair batch (epic #537): C1 — POST /api/quiz/attempts/{attempt_id}/answer grades one question server-side: owner check, 409 after completion, 400 QUIZ_QUESTION_INVALID on out-of-range indexes, malformed items never grade correct (#129 rule). Idempotent on (attempt_id, question_index): re-answering returns the FIRST recorded response with recorded:false — no revision, decided and documented for the #537 flow. Returns is_correct/correct_index/ explanation plus the next question stripped of the answer key. C2 — quiz_responses table (migration 20260812214402): plaintext analytics scalars only (indexes, boolean, time_ms, confidence), UNIQUE (attempt_id, question_index) as the idempotency contract, FK cascade with the attempt. Real-DB integration tests pin the UNIQUE arbitration and the cascade. C3 — include_answer_key on generate, default true so the current QuizPanel keeps working; every keyed response logs a deprecation breadcrumb; false strips per-option correct booleans from the response while storage keeps them for grading. Removal tracked in #546; deleting the key is a hard requirement of #537. C4 — submit prefers recorded quiz_responses per question (a contradicting payload answer is ignored — answer-time grades are the source of truth) and falls back to the payload for questions never answered through C1. The atomic completed_at claim (PR #464) and 409 behaviour are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(quiz): address #549 review — persist the graded answers, guard the index/id mismatch, drop the redundant index Review findings (xhigh, 4 confirmed; 5 reports collapsed to one root cause): - submit now persists the RECONCILED answer set (recorded responses winning over payload) instead of the raw request body. A recorded-only submit previously stored a full score beside answers_json=[], and a contradicted payload answer was stored despite losing to the recorded response — the attempt record disagreed with its own score. - The answer endpoint accepts an optional question_id and rejects a mismatch with question_index, plus echoes both in the response: passing the 1-based wire id as the 0-based index used to silently grade the neighbouring question, which idempotency then locked in. - quiz_responses drops the standalone attempt_id index — the UNIQUE's btree already leads with attempt_id, so it only added a write to the per-answer hot path. - correct_index is resolved once per request instead of re-scanning the options on every _graded call (it never depended on the answer). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
AndresL230 added a commit
that referenced
this pull request
Aug 13, 2026
…ion, concurrency tests (#543) (#551) * feat(quiz): mastery-model seam, honest delivered counts, wire validation, concurrency tests (#543) Workstream E of the pre-revamp quiz repair batch (epic #537): E1 — the mastery model is a named seam: services/quiz_config.py holds MASTERY_DELTA_PER_CORRECT/_PER_WRONG plus mastery_after(), with the pedagogy written down. THE NUMBERS DO NOT CHANGE — the #393 journey's +0.09 is byte-identical and pinned by a new test. The options the revamp gets to choose from (length normalization, difficulty weighting, diminishing returns) are written up in docs/quiz-mastery-model.md, including the constraint that any change updates the journey in the same commit. E2 — generation stops silently short-changing quizzes: the response reports requested_count and delivered_count, and losing more than a third of the requested questions to drift triggers ONE bounded top-up run (a retry loop against a drifting model burns tokens without converging). A failed top-up serves what we have; all-dropped still 502s. E3 — wire-format validation at the route boundary: at least two options, no duplicate option text (a student can otherwise pick "the same" answer and be wrong), exactly one correct option, and no duplicate question stems within one attempt. E4 — concurrency tests: double-answer on one index (the UNIQUE arbitrates; the loser re-reads instead of 500ing) and generate-while-generating for one concept (distinct attempt rows). The double-submit claim was already pinned by #464's tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(quiz): address #551 review — rekey the top-up on actual drops, stop over-rejecting, surface short quizzes The review found the E2 top-up miskeyed at its core, verified by execution. All ten findings addressed: - The trigger keyed on requested-minus-delivered, conflating "we rejected some" with "the agent returned fewer". The Quiz schema lets a run return any count and the E2E seam always returns 3 against a UI default of 5, so the top-up fired a second full generation on EVERY quiz journey — double tokens and ~5s latency for zero extra questions. It now counts questions actually DROPPED and gates on that. - The top-up prompt said "different from the ones already asked" without saying what they were, so a deterministic model re-emitted the same stems and the dedupe discarded the whole retry. It now lists them. - `wire_questions and ...` made total drift the ONE case that never retried — backwards, since that's the case a retry most obviously clears. Total drift now retries once, then 502s (the old assert_called_once in test_quiz_routes pinned the wrong behaviour and is updated with the reasoning). - The retry reused ORCHESTRATOR_LIMITS, handing it a fresh full budget and doubling the per-request cost backstop. It gets its own smaller TOPUP_LIMITS. - The recovery path logged a traceback on a request that deliberately succeeds — the exact pattern that reds the logscan oracle. Now a warning with the exception type/message. - The duplicate-option check casefolded while grading matches case-SENSITIVELY, so questions whose distractors differ only by case (`list` vs `List` — a real question) were dropped. It now compares the way grading does. - requested_count/delivered_count had no consumer: QuizPanel now warns "We could only build N of M questions" instead of quietly serving a short quiz. - The double-answer test never reached the race path (its fake short-circuited on the pre-read); it now models the real interleaving and asserts the loser actually attempted an insert. Left as-is with reasoning: two of _validate_wire_question's checks are unreachable from today's only caller (the agent schema pins 4 options and the caller builds exactly one correct flag) — they are cheap defence-in-depth for the #537 revamp's new call sites, and the unit tests exercise them directly. 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
1 participant
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.
What
Bundle B3 of the backlog clear — the five remaining #136 audit findings plus the two same-shaped siblings, every claim re-verified against
main @ 9b000b5before touching code (all seven still present; line numbers in the issue bodies had drifted). Implemented as five parallel single-surface changes; full detail in the commit message.GET /api/admin/allowlist(admin-gated,AllowlistEmailshape the frontend has been calling into a 404).user_id, wire-compatible); private profiles no longer leakname/majors/minors/year/schoolto non-owners, and theschoolvisibility tier — previously behaving aspublic— is enforced via the school-peer resolver, fail-closed. Owners always see everything.quiz_attemptsstores nomastery_before/after), and malformed items can't earn a free'' == ''point. A lane journey replays the captured wire body viapage.request.postand DB-asserts the single-event contract.resultevent per streamed upload, ever: post-result persistence failures now yield a terminal error instead of falling into the legacy fallback's second (billed) run + secondresult; the streaming route's "15 MB" message now derives from the real 100 MB constant; remaining synchronous PostgREST calls in both async upload paths moved off the event loop.search_course_materialsregistered as the explicit wire name in both agents (prompt ↔ tool-list mismatch gone); seam/stream tests updated; evaluator semantics verified unchanged.ContentFilterErrorpropagates to the routes' 502 instead of degrading to a200 {"cards": []}the UI toasts as "Generated 0 cards." (Import verified present and identically shaped in both pydantic-ai universes.)user_settings.share_class_context(migration 0037, default true, whitelisted for PATCH), enforced at the one write chokepoint (update_course_contextaggregates only opted-in users; all-opted-out purges the aggregates). The toggle keeps its hook/localStorage API and gains best-effort persistence + hydration.Verification
ruff check .clean; frontend 258 passed, tsc clean, lint 0 errors.quiz.spec.ts.Closes#130. Closes#134. Closes#129. Closes#132. Closes#135. Closes#340. Closes#72.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes