Uh oh!
There was an error while loading. Please reload this page.
re-architecture: agentic document upload + AES-256-GCM column encryption + dev-context vault - #67
Conversation
Markdown-based vault per ADR 0002: CLAUDE.md at root, docs/decisions/ (MADR-minimal append-only), docs/attempts/ (failed approaches with "What I'd try next"), docs/architecture.md. Tooling: four slash commands (/log-decision, /log-attempt, /recall, /sync-context) and a read-only context-curator subagent that loads ≤2k tokens of vault context for fresh sessions. Seeds the vault with 5 ADRs (adopt-pydantic-ai, vault-structure, implementation-conventions, graph-service-tool-surface, refactor-2- quiz-generation) and 3 attempts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Refactor #1 of the broader migration off services/gemini_service.py (see docs/decisions/0001-adopt-pydantic-ai.md). Adds backend/agents/: - classifier, summary, concept_extraction, syllabus_extraction — typed workers (Pydantic output models, per-call usage_limits). - document.py — orchestrator: classifier as serial gate, then asyncio.gather of summary+concepts+(optional)syllabus, then a graph-update tool call. - tools/graph.py — apply_graph_update wrapped as a typed tool. - deps.py — SaplingDeps DI shape (user_id, course_id, supabase, request_id) threaded through every agent run. - WORKER_LIMITS / ORCHESTRATOR_LIMITS exported from __init__.py and passed per-call (per ADR 0003 convention 2). Adds backend/services/agent_events.py — SaplingEvent shape + mapper from Pydantic AI's typed events. Switches POST /api/documents/upload to EventSourceResponse, streaming classify/extract/graph-update progress as SSE. The non-streaming /process endpoint is retained alongside the new streaming /upload. Fallback contract: any agent exception (UsageLimitExceeded, UnexpectedModelBehavior, anything else) routes to _legacy_upload_pipeline (services/gemini_service.py-backed). Streaming route emits an error SSE event then yields the legacy result over the same stream. Mechanic documented in ADR 0003. Adds 10-case pydantic-evals set in backend/tests/evals/. Wires Logfire (instrument_pydantic_ai + instrument_fastapi) in main.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integrates the AES-256-GCM column-encryption rollout (origin) with the Pydantic AI agentic refactor (local). Conflicts resolved: - backend/.env.example: kept origin (deletion was a local accident). - CLAUDE.md: kept lean post-ADR-0002 structure; added a Gotchas entry pointing at services/encryption.py + the encrypted columns list and ENCRYPTION_KEY requirement. - backend/routes/documents.py: - Combined imports (BackgroundTasks + Request + SSE/pydantic_ai). - Both new routes (/upload streaming, /upload/sync) gained require_self(user_id, request) before _validate_user. - _persist_document now encrypts summary + concept_notes at the insert boundary and returns the plaintext shape so callers don't re-decrypt for the response. Mirrors the pattern in _legacy_upload_pipeline at lines 749-750. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed Pydantic‑AI agents and evals, an orchestrator for document processing, a graph‑merge tool, refactored sync and SSE upload flows, request correlation and Logfire scrubbing, an optional durable shim, vault/Claude tooling and docs, frontend SSE client/UX, many tests, and dependency updates. ChangesAgent-based document processing + SSE + infra
Sequence DiagramsequenceDiagram
participant Client
participant Route as API Route (/upload or /upload/sync)
participant Orch as Orchestrator (process_document)
participant Classifier as classifier_agent
participant Workers as summary_agent / concept_extraction_agent / syllabus_extraction_agent
participant Graph as apply_concepts_to_graph
participant DB as Database
Client->>Route: POST document (+ optional X-Request-ID)
Route->>Orch: call process_document(text, SaplingDeps)
Orch->>Classifier: run(classify)
Classifier-->>Orch: DocumentClassification
par run workers in parallel
Orch->>Workers: run(summary, concepts[, syllabus])
Workers-->>Orch: Summary, ConceptList[, SyllabusAssignments]
end
Orch->>Graph: apply_concepts_to_graph(user_id, course_id, concept_names)
Graph-->>Orch: merged_count
Orch-->>Route: DocumentProcessingResult (graph_updated flag)
Route->>DB: _persist_document(result, request_id?)
DB-->>Route: persisted row / document_id
Route-->>Client: JSON (sync) or SSE events (progress/result/done)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Uh oh!
There was an error while loading. Please reload this page.
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 95b7112 | Commit Preview URL Branch Preview URL | May 04 2026, 06:50 AM |
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/context-curator.md:
- Around line 21-33: The fenced code block surrounding the "### Relevant
decisions" .. "### Open questions" section is missing a fence language (triple
backticks only), causing MD040 markdown-lint failures; update the opening fence
from ``` to ```markdown (keep the closing ``` unchanged) so the block is
explicitly marked as markdown and linting/CI will pass, and scan for any other
similar fences in context-curator.md to apply the same change if present.
In `@backend/agents/deps.py`:
- Around line 21-31: SaplingDeps currently exposes a raw supabase client via the
supabase attribute; replace that with a constrained DB facade or a table
callable (the table function) instead: change the SaplingDeps type from
supabase: Any to something like table: Callable[[str], Table] or a minimal
DBFacade interface, update SaplingDeps initializer and any consumers (references
to SaplingDeps.supabase) to call the new table callable or facade methods, and
remove direct supabase client usage/imports so all DB access goes through the
table() abstraction.
In `@backend/agents/summary.py`:
- Around line 30-33: The Field for key_points is using list-specific validators
incorrectly and enforces a minimum of 3 which conflicts with the sparse-doc
behavior; update the key_points Field in backend/agents/summary.py to use
min_items (not min_length) and set min_items to 0 (and keep max_items=8) so the
list can be empty when sparse-doc returns fewer points, e.g. change
min_length->min_items and min_items=0 while preserving max (max_items=8) and the
description.
In `@backend/agents/syllabus_extraction.py`:
- Line 38: The code currently constructs _provider =
GoogleProvider(api_key=GEMINI_API_KEY or "dummy-key-for-import") which masks
missing GEMINI_API_KEY; change this to fail fast by validating GEMINI_API_KEY
before creating GoogleProvider: if GEMINI_API_KEY is falsy, raise a clear
configuration error (or exit) referencing GEMINI_API_KEY so deployments fail
loudly, otherwise pass GEMINI_API_KEY into GoogleProvider; update any import or
tests that expect a dummy key to use dependency injection or test fixtures
instead of the "dummy-key-for-import".
In `@backend/agents/tools/graph.py`:
- Around line 52-58: The confirmation message currently uses len(new_nodes)
which may over-report because apply_graph_update performs dedupe/skip logic;
either capture and use an actual merge count returned by apply_graph_update
(call apply_graph_update and store its return value, e.g., merged_count = await
asyncio.to_thread(apply_graph_update, ...), then use merged_count in the
message) or change the text to a neutral wording that does not claim merges
(e.g., "requested" or "submitted") using the existing variables
(apply_graph_update, new_nodes, ctx.deps.course_id) so streamed status cannot
falsely report merged concept counts.
In `@backend/routes/documents.py`:
- Around line 452-454: When the upload falls back to _legacy_upload_pipeline the
code currently schedules update_course_context only on the successful
orchestrator path, so course context isn't refreshed for legacy uploads; ensure
update_course_context(course_id) is also scheduled via background_tasks.add_task
in the fallback/legacy path (where _legacy_upload_pipeline is invoked) and
likewise add the same scheduling to the other fallback block around the 756-763
area so both upload branches always queue update_course_context.
- Around line 638-640: The SSE payload is leaking internal exception text by
calling str(e) in the SaplingEvent; instead, replace the emitted message with a
generic fallback string (e.g., "An internal error occurred during fallback") and
log the full exception server-side using the module logger or processLogger with
stack/exception info; update the yield site that constructs SaplingEvent (the
sapling_event_to_sse(SaplingEvent(...)) call) to use the generic message and
ensure the except block calls logger.error or logger.exception(e) to record the
original exception details.
- Around line 593-597: The final SaplingEvent result is emitted before calling
_persist_document, which means a later persistence failure can trigger
_stream_legacy_fallback and send duplicate result/done sequences; move the yield
sapling_event_to_sse(SaplingEvent(..., type="result", step="finalize", ...)) to
after the call to _persist_document (or alternatively set a local flag like
result_sent and have the outer except avoid calling _stream_legacy_fallback if
result_sent is True) so that post-save failures do not trigger the legacy
fallback; update the same pattern around the other block that currently emits
result at lines ~636-646.
- Around line 694-699: The background task _check_upload_achievements currently
swallows all exceptions; change the except block to capture the exception (e.g.,
except Exception as e) and log it instead of passing so failures leave a trace;
use the project logger or logging.exception (referencing
_check_upload_achievements and check_achievements) to emit a descriptive message
and exception stacktrace while keeping the task best-effort.
In `@backend/scripts/cleanup_classifier_test.py`:
- Around line 23-31: The script currently hardcodes production identifiers
(USER_ID, COURSE_ID, DOC_IDS, SINCE) and accepts a trivial confirmation ("y");
tighten the safety gate by requiring a multi-factor confirmation before any
destructive delete: (1) require an explicit environment variable like
CONFIRM_DELETE="DELETE_PRODUCTION" or a CLI flag --confirm-delete with the exact
value "DELETE_PRODUCTION", (2) require the operator to type the full COURSE_ID
(or full USER_ID) as a second interactive confirmation rather than a single
character, (3) add a --dry-run mode that prints the documents that would be
deleted without performing deletes, and (4) prevent running against production
identifiers unless a new --allow-production flag is set; implement these checks
near the current confirmation logic (the block that reads console input around
the confirmation prompt) and validate against the constants USER_ID, COURSE_ID,
DOC_IDS and SINCE before performing any destructive operations.
In `@CLAUDE.md`:
- Around line 33-36: The markdown fenced command blocks that currently lack a
language tag (the blocks containing "python main.py ... python -m pytest ..."
and the block containing "docker-compose up") are triggering MD040; update each
opening triple-backtick to include "bash" (i.e., ```bash) so the shells are
annotated; ensure both command blocks are changed (the one with the
Python/pytest commands and the one with docker-compose) to resolve the lint
warning.
- Around line 10-19: Update the stale migration notes to reflect that Pydantic
AI is now the chosen agent framework (not "not yet"), that agents live under
backend/agents/, and that the document processing pipeline is implemented rather
than only a refactor target; specifically, replace the "not yet in
`requirements.txt`" language and the "refactor target" phrasing with current
status, mention `Pydantic AI` as the active framework, and keep the repo map
references to backend/main.py, backend/routes/documents.py (`_process_document`
and `upload_document`) and backend/routes/learn.py (`build_system_prompt`) so
readers can find the implemented components.
In `@docs/architecture.md`:
- Around line 11-20: Update the architecture doc to replace the outdated
pre-refactor description of document upload and LLM seam with the new
orchestrator + SSE + legacy-fallback contract: describe that upload_document now
delegates to the document processing orchestrator (instead of a single
`_process_document` Gemini call) which streams progress via SSE to clients,
invokes new agent-based handlers under `backend/agents/` (Pydantic AI agents
replacing direct `call_gemini_json` usage), and falls back to legacy
`backend/services/gemini_service` functions like `call_gemini_json`,
`call_gemini_multiturn`, and `extract_graph_update` only when agents aren’t
available; also note that `backend/agents/` is the intended home for new agent
implementations and that `pydantic-ai` must be added to requirements to complete
the migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d387bcdb-cd39-403f-a0d2-e82866caa414
📒 Files selected for processing (38)
.claude/agents/.gitkeep.claude/agents/context-curator.md.claude/commands/.gitkeep.claude/commands/log-attempt.md.claude/commands/log-decision.md.claude/commands/recall.md.claude/commands/sync-context.md.claude/skills/.gitkeep.gitignoreCLAUDE.mdbackend/agents/__init__.pybackend/agents/classifier.pybackend/agents/concept_extraction.pybackend/agents/deps.pybackend/agents/document.pybackend/agents/summary.pybackend/agents/syllabus_extraction.pybackend/agents/tools/__init__.pybackend/agents/tools/graph.pybackend/main.pybackend/requirements.txtbackend/routes/documents.pybackend/scripts/cleanup_classifier_test.pybackend/services/agent_events.pybackend/tests/evals/__init__.pybackend/tests/evals/document_classification.pydocs/README.mddocs/architecture.mddocs/attempts/.gitkeepdocs/attempts/2026-05-03-mcp-knowledge-server-trial.mddocs/attempts/2026-05-03-orchestrator-schema-complexity.mddocs/attempts/2026-05-03-vault-gap-prompts-13-14.mddocs/decisions/.gitkeepdocs/decisions/0001-adopt-pydantic-ai.mddocs/decisions/0002-vault-structure.mddocs/decisions/0003-implementation-conventions.mddocs/decisions/0004-graph-service-tool-surface.mddocs/decisions/0005-refactor-2-quiz-generation.md
| ``` | ||
| ### Relevant decisions | ||
| - ADR <NNNN>: <one-line summary>. (link) | ||
| ### Relevant prior attempts | ||
| - <date> — <slug>: <what failed in one line>. (link) | ||
| ### Constraints to respect | ||
| - <bullet list of hard rules carried over from ADRs> | ||
| ### Open questions | ||
| - <anything the vault doesn't answer that the parent should know> | ||
| ``` |
There was a problem hiding this comment.
Specify a language for the fenced output-format block.
Add a fence language to satisfy markdown linting (MD040) and keep docs CI-friendly.
Suggested fix
-```+```markdown
### Relevant decisions
- ADR <NNNN>: <one-line summary>. (link)
@@
### Open questions
- <anything the vault doesn't answer that the parent should know></details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/context-curator.md around lines 21 - 33, The fenced code
block surrounding the "### Relevant decisions" .. "### Open questions" section
is missing a fence language (triple backticks only), causing MD040 markdown-lint
failures; update the opening fence from ``` to ```markdown (keep the closing ```
unchanged) so the block is explicitly marked as markdown and linting/CI will
pass, and scan for any other similar fences in context-curator.md to apply the
same change if present.
| supabase: The Supabase client (from db.connection). Typed as Any | ||
| to avoid coupling agent code to a specific Supabase SDK | ||
| version. | ||
| request_id: A correlation ID for tracing across a single | ||
| user-facing request. Used by Logfire spans. | ||
| """ | ||
| user_id: str | ||
| course_id: str | None | ||
| supabase: Any | ||
| request_id: str |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Avoid threading a raw Supabase client through SaplingDeps.
This shared contract makes direct client usage easy in agent code and undermines the repository DB-access boundary. Prefer passing a constrained DB facade (or table callable) instead of a raw client object.
Proposed direction
-from typing import Any+from typing import Any, Callable
@@
- supabase: The Supabase client (from db.connection). Typed as Any- to avoid coupling agent code to a specific Supabase SDK- version.+ table: DB table accessor from db.connection.table, used as the+ only entry point for Supabase/PostgREST operations.
@@
- supabase: Any+ table: Callable[[str], Any]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/agents/deps.py` around lines 21 - 31, SaplingDeps currently exposes a
raw supabase client via the supabase attribute; replace that with a constrained
DB facade or a table callable (the table function) instead: change the
SaplingDeps type from supabase: Any to something like table: Callable[[str],
Table] or a minimal DBFacade interface, update SaplingDeps initializer and any
consumers (references to SaplingDeps.supabase) to call the new table callable or
facade methods, and remove direct supabase client usage/imports so all DB access
goes through the table() abstraction.
| concept_names = [c.name for c in workers.concepts.concepts] | ||
| confirmation = await document_agent.run( | ||
| "Merge these concepts into the student's course graph: " | ||
| f"{concept_names}", | ||
| deps=deps, | ||
| usage_limits=ORCHESTRATOR_LIMITS, | ||
| ) |
There was a problem hiding this comment.
Gate graph writes the same way as the legacy path.
This always sends concepts to apply_graph_update_tool, so a successful orchestrator run mutates the graph for every document category. Both _graph_backstop() and _legacy_upload_pipeline() in backend/routes/documents.py only populate the graph for assignment/syllabus, so agent success vs. fallback changes persisted behavior for the same upload.
Proposed fix
- concept_names = [c.name for c in workers.concepts.concepts]- confirmation = await document_agent.run(- "Merge these concepts into the student's course graph: "- f"{concept_names}",- deps=deps,- usage_limits=ORCHESTRATOR_LIMITS,- )+ graph_updated = False+ if workers.classification.category in {"syllabus", "assignment"}:+ concept_names = [c.name for c in workers.concepts.concepts]+ confirmation = await document_agent.run(+ "Merge these concepts into the student's course graph: "+ f"{concept_names}",+ deps=deps,+ usage_limits=ORCHESTRATOR_LIMITS,+ )+ graph_updated = confirmation.output.graph_updated
return DocumentProcessingResult(
classification=workers.classification,
summary=workers.summary,
concepts=workers.concepts,
syllabus=workers.syllabus,
- graph_updated=confirmation.output.graph_updated,+ graph_updated=graph_updated,
)| key_points: list[str] = Field( | ||
| min_length=3, | ||
| max_length=8, | ||
| description="3-8 most important takeaways, each one sentence.", |
There was a problem hiding this comment.
Align key_points minimum with sparse-document behavior.
min_length=3 conflicts with the sparse-doc instruction (Lines 51-54), which can force padding/hallucination or output validation failure.
Proposed fix
- key_points: list[str] = Field(- min_length=3,+ key_points: list[str] = Field(+ min_length=1,
max_length=8,
- description="3-8 most important takeaways, each one sentence.",+ description="1-8 most important takeaways, each one sentence.",
)
@@
- "prose with no markdown, math, or fenced blocks; and 3-8 key "+ "prose with no markdown, math, or fenced blocks; and 1-8 key "
"takeaways, each one sentence, ordered by importance.\n\n"Also applies to: 51-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/agents/summary.py` around lines 30 - 33, The Field for key_points is
using list-specific validators incorrectly and enforces a minimum of 3 which
conflicts with the sparse-doc behavior; update the key_points Field in
backend/agents/summary.py to use min_items (not min_length) and set min_items to
0 (and keep max_items=8) so the list can be empty when sparse-doc returns fewer
points, e.g. change min_length->min_items and min_items=0 while preserving max
(max_items=8) and the description.
| assignments: list[SyllabusAssignment] = Field(max_length=50) | ||
| _provider = GoogleProvider(api_key=GEMINI_API_KEY or "dummy-key-for-import") |
There was a problem hiding this comment.
Fail fast when GEMINI_API_KEY is missing.
Line 38 currently injects a fake key, which can hide deploy misconfiguration and defer failure into runtime agent calls/fallbacks.
Proposed fix
-_provider = GoogleProvider(api_key=GEMINI_API_KEY or "dummy-key-for-import")+if not GEMINI_API_KEY:+ raise RuntimeError("GEMINI_API_KEY must be set for agent execution")+_provider = GoogleProvider(api_key=GEMINI_API_KEY)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/agents/syllabus_extraction.py` at line 38, The code currently
constructs _provider = GoogleProvider(api_key=GEMINI_API_KEY or
"dummy-key-for-import") which masks missing GEMINI_API_KEY; change this to fail
fast by validating GEMINI_API_KEY before creating GoogleProvider: if
GEMINI_API_KEY is falsy, raise a clear configuration error (or exit) referencing
GEMINI_API_KEY so deployments fail loudly, otherwise pass GEMINI_API_KEY into
GoogleProvider; update any import or tests that expect a dummy key to use
dependency injection or test fixtures instead of the "dummy-key-for-import".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| - Pydantic AI: target agent framework; not yet in `requirements.txt`, agents will live under `backend/agents/`. | ||
| - React frontend: lives in `frontend/` (out of scope for backend sessions). | ||
| - pytest: backend test runner, fixtures in `tests/conftest.py`. | ||
| ## Directory Structure | ||
| ## Repo map | ||
| ``` | ||
| sapling/ | ||
| ├── CLAUDE.md # Claude Code guidelines and project conventions | ||
| ├── README.md # Project overview and setup instructions | ||
| ├── docker-compose.yml # Orchestrates frontend + backend containers | ||
| ├── landingpage.png # Screenshot of the landing page | ||
| ├── .impeccable.md # Impeccable design skill configuration | ||
| │ | ||
| ├── backend/ | ||
| │ ├── main.py # FastAPI app entry point, registers all routers | ||
| │ ├── config.py # Loads and validates env vars (Supabase, Gemini, etc.) | ||
| │ ├── requirements.txt # Python dependencies | ||
| │ ├── Dockerfile # Backend container image definition | ||
| │ ├── .dockerignore # Files excluded from the Docker build context | ||
| │ ├── .env # Local secrets (not committed) | ||
| │ ├── .env.example # Template showing required env vars | ||
| │ │ | ||
| │ ├── db/ | ||
| │ │ ├── connection.py # Creates and exports the Supabase client | ||
| │ │ ├── supabase_schema.sql # Full Supabase table/index schema | ||
| │ │ ├── seed.sql # Sample data for local development | ||
| │ │ ├── migration_google_auth.sql # Migration adding Google OAuth user fields | ||
| │ │ ├── migration_add_is_approved.sql # Migration adding user approval gate flag | ||
| │ │ ├── migration_onboarding_fields.sql # Migration adding onboarding profile columns | ||
| │ │ ├── migration_roles.sql # Migration adding roles and user_roles tables | ||
| │ │ ├── migration_achievements.sql # Migration adding achievements, triggers, and user_achievements | ||
| │ │ ├── migration_cosmetics.sql # Migration adding cosmetics and user_cosmetics tables | ||
| │ │ ├── migration_profile_settings.sql # Migration adding profile and settings fields | ||
| │ │ ├── migration_concept_notes.sql # Migration adding concept_notes column to documents | ||
| │ │ ├── migration_newsletter.sql # Migration adding newsletter_subscribers table | ||
| │ │ ├── migration_flashcard_course_id.sql # Migration adding course_id to flashcards | ||
| │ │ ├── migration_gradebook.sql # Migration adding gradebook tables (categories, assignments, letter scales) | ||
| │ │ ├── migration_drop_legacy_grade_tables.sql # Cleanup migration removing legacy grade_* tables | ||
| │ │ ├── migration_encryption_text_columns.sql # Retypes encrypted columns to TEXT to fit AES-256-GCM ciphertext | ||
| │ │ ├── backfill_encryption.py # One-shot script that walks rows + encrypts existing plaintext | ||
| │ │ ├── dedup_nodes.py # One-off script to deduplicate knowledge graph nodes | ||
| │ │ └── archive/ # Old pre-Supabase init scripts (no longer used) | ||
| │ │ | ||
| │ ├── models/ | ||
| │ │ └── __init__.py # Pydantic request/response models package init | ||
| │ │ | ||
| │ ├── prompts/ | ||
| │ │ ├── preamble.txt # System preamble injected into every AI session | ||
| │ │ ├── socratic.txt # Prompt for Socratic questioning study mode | ||
| │ │ ├── teachback.txt # Prompt for teach-back (explain-it-back) mode | ||
| │ │ ├── expository.txt # Prompt for direct expository explanation mode | ||
| │ │ ├── quiz_generation.txt # Prompt for generating quiz questions from content | ||
| │ │ ├── quiz_context_update.txt # Prompt for updating quiz state after each answer | ||
| │ │ ├── study_match.txt # Prompt for matching students into study groups | ||
| │ │ ├── syllabus_extraction.txt # Prompt for extracting assignments + grading categories from a syllabus | ||
| │ │ └── shared_context.txt # Prompt fragment injected when shared course context is on | ||
| │ │ | ||
| │ ├── routes/ | ||
| │ │ ├── admin.py # Admin endpoints for role, achievement, cosmetic, and user management | ||
| │ │ ├── auth.py # Google OAuth sign-in (popup flow), session tokens, and user upsert | ||
| │ │ ├── calendar.py # Endpoints to read and sync assignment calendar events | ||
| │ │ ├── careers.py # Endpoints for job listings and application submission | ||
| │ │ ├── documents.py # Upload, classify, summarize, and extract from docs | ||
| │ │ ├── extract.py # OCR and text extraction pipeline for uploaded files | ||
| │ │ ├── feedback.py # Endpoints to submit session and general user feedback | ||
| │ │ ├── flashcards.py # CRUD endpoints for user flashcard decks | ||
| │ │ ├── gradebook.py # Gradebook endpoints (courses, categories, assignments, letter scales, syllabus apply) | ||
| │ │ ├── graph.py # Endpoints to build and query the knowledge graph | ||
| │ │ ├── learn.py # Streaming AI tutoring chat endpoint (SSE) | ||
| │ │ ├── newsletter.py # Newsletter / beta-list signup endpoint | ||
| │ │ ├── onboarding.py # Course search and onboarding profile submission | ||
| │ │ ├── profile.py # Public profiles, settings, cosmetics, achievements, account mgmt | ||
| │ │ ├── quiz.py # Quiz session creation, answering, and scoring endpoints | ||
| │ │ ├── social.py # Study room creation, membership, and chat endpoints | ||
| │ │ └── study_guide.py # Endpoint to generate a structured study guide from docs | ||
| │ │ | ||
| │ ├── services/ | ||
| │ │ ├── achievement_service.py # Checks and grants achievements when event thresholds are met | ||
| │ │ ├── assignment_dedupe.py # Deduplicates assignments before inserting into DB | ||
| │ │ ├── auth_guard.py # HMAC session token verification and role-based route guards | ||
| │ │ ├── calendar_service.py # Formats and writes assignments as calendar events | ||
| │ │ ├── course_context_service.py # Fetches and caches shared course context for a session | ||
| │ │ ├── encryption.py # AES-256-GCM helpers (encrypt / decrypt / *_if_present) for column-level encryption | ||
| │ │ ├── extraction_service.py # Thin router selecting an OCR backend based on OCR_ENGINE env var | ||
| │ │ ├── extraction_backends/ # OCR engine implementations (docling, GOT-OCR 2.0, tesseract) | ||
| │ │ ├── flashcard_import_service.py # Parses + AI-extracts flashcards from paste, file, URL, photo | ||
| │ │ ├── gemini_service.py # Wrapper around the Gemini API (chat, streaming, model selection) | ||
| │ │ ├── gradebook_service.py # Grade calculations: category_grade, current_grade, letter_for | ||
| │ │ ├── graph_service.py # Builds knowledge graph nodes and edges from content | ||
| │ │ ├── matching_service.py # Matches students into compatible study groups via AI | ||
| │ │ ├── quiz_context_service.py # Manages per-session quiz state and context window | ||
| │ │ ├── social_cache_service.py # Caches room membership and presence for social features | ||
| │ │ └── storage_service.py # Avatar and asset uploads via Supabase Storage | ||
| │ │ | ||
| │ └── tests/ | ||
| │ ├── conftest.py # Shared pytest fixtures (mock Supabase, Gemini, etc.) | ||
| │ ├── fixtures/ # Test fixture data (sample PDFs, JSON payloads) | ||
| │ ├── README.md # Notes on running and writing backend tests | ||
| │ ├── test_achievement_service.py # Tests for achievement checking and granting | ||
| │ ├── test_admin_routes.py # Tests for admin role, achievement, and cosmetic endpoints | ||
| │ ├── test_assignment_dedupe.py # Tests for assignment deduplication logic | ||
| │ ├── test_calendar_routes.py # Tests for calendar sync endpoints | ||
| │ ├── test_config.py # Tests that config loads env vars correctly | ||
| │ ├── test_docling_integration.py # Integration tests for the Docling OCR backend | ||
| │ ├── test_documents_routes.py # Tests for document upload and processing endpoints | ||
| │ ├── test_encryption.py # Tests for AES-256-GCM helpers and the *_if_present fallbacks | ||
| │ ├── test_extraction_backends.py # Tests for OCR backend selection and fallback chain | ||
| │ ├── test_extraction_service.py # Tests for the OCR extraction router | ||
| │ ├── test_flashcard_import_routes.py # Tests for the flashcard import endpoint | ||
| │ ├── test_flashcard_import_service.py # Tests for parsing/extracting flashcards from each input type | ||
| │ ├── test_gemini_service.py # Tests for Gemini API wrapper behavior | ||
| │ ├── test_gradebook_routes.py # Tests for gradebook endpoints | ||
| │ ├── test_gradebook_service.py # Tests for grade calculation logic | ||
| │ ├── test_graph_service.py # Tests for knowledge graph construction | ||
| │ ├── test_learn_routes.py # Tests for the streaming tutoring chat endpoint | ||
| │ ├── test_ocr_pipeline.py # Tests for end-to-end OCR pipeline | ||
| │ ├── test_onboarding_routes.py # Tests for onboarding endpoint validation | ||
| │ ├── test_profile_routes.py # Tests for profile, settings, and cosmetics endpoints | ||
| │ ├── test_quiz_routes.py # Tests for quiz session endpoints | ||
| │ ├── test_shared_course_context.py # Tests for shared course context injection | ||
| │ ├── test_social_messages.py # Tests for room chat message endpoints | ||
| │ ├── test_storage_service.py # Tests for avatar upload via Supabase Storage | ||
| │ ├── test_study_guide_routes.py # Tests for study guide generation endpoints | ||
| │ └── test_supabase.py # Integration tests against Supabase connection | ||
| │ | ||
| └── frontend/ | ||
| ├── next.config.ts # Next.js build and runtime configuration | ||
| ├── tsconfig.json # TypeScript compiler options | ||
| ├── package.json # Node dependencies and npm scripts | ||
| ├── package-lock.json # Locked dependency tree | ||
| ├── eslint.config.mjs # ESLint rules for the frontend | ||
| ├── postcss.config.mjs # PostCSS config (Tailwind plugin) | ||
| ├── wrangler.toml # Cloudflare Workers config (used by @opennextjs/cloudflare) | ||
| ├── Dockerfile # Frontend container image definition | ||
| ├── .dockerignore # Files excluded from the Docker build context | ||
| ├── .env.local # Local frontend secrets (not committed) | ||
| ├── README.md # Frontend-specific setup notes | ||
| │ | ||
| ├── public/ | ||
| │ ├── sapling-icon.svg # App icon used in favicon and UI | ||
| │ └── sapling-word-icon.png # Full wordmark logo for navbar/branding | ||
| │ | ||
| └── src/ | ||
| ├── middleware.ts # Next.js middleware for auth guards on protected routes | ||
| │ | ||
| ├── app/ | ||
| │ ├── layout.tsx # Root layout: UserContext, providers, global styles | ||
| │ ├── page.tsx # Landing page (sign-in is a modal launched from here) | ||
| │ ├── error.tsx # Global Next.js error boundary page | ||
| │ ├── globals.css # Tailwind base styles and CSS custom properties | ||
| │ ├── about/page.tsx # About page | ||
| │ ├── api/auth/session/route.ts # Next.js API route for session token exchange | ||
| │ ├── auth/callback/page.tsx # OAuth popup callback that posts the code back to opener | ||
| │ ├── careers/ # Careers listing + per-job detail pages with apply form | ||
| │ ├── flashcards/page.tsx # Public flashcard study (entered from the shell) | ||
| │ ├── onboarding/page.tsx # Onboarding entry (renders OnboardingFlow) | ||
| │ ├── pending/page.tsx # Holding page for unapproved users awaiting access | ||
| │ ├── privacy/page.tsx # Privacy policy page | ||
| │ ├── terms/page.tsx # Terms of service page | ||
| │ │ | ||
| │ └── (shell)/ # Route group: every page inside renders inside ShellFrame (SideNav + TopNav) | ||
| │ ├── layout.tsx # Shell layout that wraps children with SideNav and content frame | ||
| │ ├── achievements/page.tsx # Achievements gallery page | ||
| │ ├── admin/page.tsx # Admin panel (role/cosmetic/user management) | ||
| │ ├── calendar/page.tsx # Assignment calendar timeline | ||
| │ ├── course-planner/page.tsx # Course planner tool entry | ||
| │ ├── dashboard/page.tsx # User dashboard | ||
| │ ├── gradebook/page.tsx # Gradebook landing (per-course summaries) | ||
| │ ├── gradebook/[courseId]/page.tsx # Per-course gradebook detail | ||
| │ ├── learn/page.tsx # AI tutoring session entry | ||
| │ ├── library/page.tsx # Document library | ||
| │ ├── profile/[userId]/page.tsx # Public user profile by id | ||
| │ ├── settings/page.tsx # User settings (profile editing, cosmetics, sign out) | ||
| │ ├── social/page.tsx # Study rooms and peer matching | ||
| │ ├── study/page.tsx # Study session shell (rendered with FlashcardsPanel) | ||
| │ └── tree/page.tsx # Knowledge graph tree visualization | ||
| │ | ||
| ├── components/ | ||
| │ ├── AchievementUnlockToast.tsx # Toast shown when an achievement unlocks | ||
| │ ├── AchievementUnlockWatcher.tsx # Polls for newly unlocked achievements and fires toasts | ||
| │ ├── AIDisclaimerChip.tsx # Small chip shown on AI-generated content | ||
| │ ├── AtmosphericBackdrop.tsx # Animated ambient background used on landing/auth surfaces | ||
| │ ├── Avatar.tsx # User avatar with initials fallback | ||
| │ ├── AvatarFrame.tsx # Decorative frame around avatar from equipped cosmetics | ||
| │ ├── ChatPanel.tsx # Chat shell with input + AI disclaimer (renders MarkdownChat inside) | ||
| │ ├── CustomSelect.tsx # Styled dropdown select component | ||
| │ ├── Dialog.tsx # Reusable modal/dialog primitive | ||
| │ ├── DisclaimerModal.tsx # First-use AI disclaimer modal | ||
| │ ├── DocumentUploadModal.tsx # Drag-and-drop upload modal for course documents | ||
| │ ├── ErrorBoundary.tsx # React error boundary wrapper | ||
| │ ├── FeedbackFlow.tsx # Multi-step general feedback submission flow | ||
| │ ├── FloatingActions.tsx # Floating action buttons (feedback, report, etc.) | ||
| │ ├── FunctionPlot.tsx # function-plot.js renderer used by MarkdownChat | ||
| │ ├── HowItWorks.tsx # Landing page section explaining the product | ||
| │ ├── Icon.tsx # Centralized SVG icon component | ||
| │ ├── KnowledgeGraph.tsx # D3-powered interactive knowledge graph | ||
| │ ├── ManageCoursesModal.tsx # Modal for adding/removing courses | ||
| │ ├── MarkdownChat.tsx # Markdown renderer with math (KaTeX), mermaid, plots, theorem callouts | ||
| │ ├── MermaidBlock.tsx # mermaid diagram renderer used by MarkdownChat | ||
| │ ├── MiniStat.tsx # Compact stat tile component | ||
| │ ├── NameColorRenderer.tsx # Renders a username with equipped name-color cosmetic | ||
| │ ├── OnboardingFlow.tsx # Multi-step onboarding flow (school, major, year, courses) | ||
| │ ├── Pill.tsx # Small rounded pill/tag component | ||
| │ ├── ProfileView.tsx # Public profile renderer (used by /profile/[userId]) | ||
| │ ├── QuizPanel.tsx # Quiz UI for answering and reviewing questions | ||
| │ ├── ReportIssueFlow.tsx # Flow for users to report bugs or content issues | ||
| │ ├── RoleBadge.tsx # Badge displaying a user's role | ||
| │ ├── SessionFeedbackFlow.tsx # In-session feedback prompt | ||
| │ ├── SessionFeedbackGlobal.tsx # Global wrapper that triggers session feedback | ||
| │ ├── SessionSummary.tsx # Post-session summary | ||
| │ ├── SharedContextToggle.tsx # Toggle to enable/disable shared course context in chat | ||
| │ ├── ShellFrame.tsx # Layout frame used by the (shell) route group (SideNav + content) | ||
| │ ├── SideNav.tsx # Collapsible left rail with main navigation | ||
| │ ├── SignInModal.tsx # Sign-in modal launched from landing (Google OAuth popup flow) | ||
| │ ├── Skeleton.tsx # Loading skeleton variants used across screens | ||
| │ ├── Sparkline.tsx # Tiny inline sparkline chart | ||
| │ ├── TitleFlair.tsx # Decorative flair rendered next to user titles | ||
| │ ├── ToastProvider.tsx # Global toast notification context and renderer | ||
| │ ├── TopBar.tsx # Header bar within the shell (breadcrumb, actions) | ||
| │ ├── TopNav.tsx # Top navigation bar for non-shell (public) pages | ||
| │ │ | ||
| │ ├── flashcards/ | ||
| │ │ ├── FlashcardImportModal.tsx # Tabbed modal for importing flashcards | ||
| │ │ ├── ParsedCardsTable.tsx # Editable table of parsed cards before saving | ||
| │ │ └── tabs/ # Per-source tabs: AiTab, PasteTab, PhotoTab, UploadTab, UrlTab | ||
| │ │ | ||
| │ ├── Gradebook/ | ||
| │ │ ├── AssignmentList.tsx # List of assignments with grades | ||
| │ │ ├── AssignmentModal.tsx # Edit/create assignment modal | ||
| │ │ ├── CategoryPanel.tsx # Per-category breakdown panel | ||
| │ │ ├── EditWeightsModal.tsx # Modal to edit category weights | ||
| │ │ ├── LetterScaleEditor.tsx # Modal to edit per-course letter-grade thresholds | ||
| │ │ ├── SemesterChips.tsx # Semester filter chips | ||
| │ │ └── SyllabusUploadFlow.tsx # Upload syllabus → preview categories → apply | ||
| │ │ | ||
| │ └── screens/ # Screen-level renderers used by (shell) page.tsx files | ||
| │ ├── Achievements.tsx | ||
| │ ├── Admin.tsx | ||
| │ ├── Calendar.tsx | ||
| │ ├── Dashboard.tsx | ||
| │ ├── Gradebook/Course.tsx # Per-course gradebook detail screen | ||
| │ ├── Gradebook/Landing.tsx # Gradebook landing screen | ||
| │ ├── Learn.tsx | ||
| │ ├── Library.tsx | ||
| │ ├── Onboarding.tsx | ||
| │ ├── Settings.tsx | ||
| │ ├── Social.tsx | ||
| │ ├── Study.tsx | ||
| │ └── Tree.tsx | ||
| │ | ||
| ├── context/ | ||
| │ └── UserContext.tsx # React context providing authenticated user state globally | ||
| │ | ||
| └── lib/ | ||
| ├── api.ts # Typed fetch helpers for every backend API endpoint | ||
| ├── avatarUtils.ts # Avatar initials/colors helpers | ||
| ├── data.ts # Static reference data (constants, enums) | ||
| ├── flashcardParsers.ts # Client-side parsers for paste/file flashcard input | ||
| ├── graphUtils.ts # Helpers for transforming graph data for D3 | ||
| ├── localData.ts # Local-storage-backed offline cache for the demo mode | ||
| ├── sessionToken.ts # HMAC session token creation and verification | ||
| ├── supabase.ts # Supabase browser client singleton | ||
| ├── types.ts # Shared TypeScript types | ||
| ├── useAchievementUnlockWatcher.ts # Hook that polls for unlocked achievements | ||
| ├── useBodyScrollLock.ts # Lock body scroll while a modal is open | ||
| ├── useConfirm.ts # Imperative confirm-dialog hook | ||
| ├── useIsMobile.ts # Viewport size hook | ||
| └── useLayoutPref.ts # Persists layout preferences (e.g. sidenav collapsed) | ||
| ``` | ||
| - backend/main.py:24 — FastAPI app, CORS, and every router mount. | ||
| - backend/routes/documents.py:149 — `_process_document` single-call classify/summarize/extract (refactor target #1). | ||
| - backend/routes/documents.py:265 — `upload_document` POST `/api/documents/upload` pipeline. | ||
| - backend/routes/learn.py:152 — `build_system_prompt` for the streaming tutor (SSE). |
There was a problem hiding this comment.
Update stale migration notes in Stack/Repo map.
Line 10 and Line 17–19 still describe Pydantic AI + document orchestration as “not yet” / future-target state. That now conflicts with this PR’s implemented architecture and will mislead future edits.
Based on learnings: "Migrate from google-genai to Pydantic AI as the target agent framework; agents will live under backend/agents/." and "Document processing pipeline with _process_document ... is marked as a refactor target."
🧰 Tools
🪛 LanguageTool
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mmarize/extract (refactor target #1). - backend/routes/documents.py:265 — `upload_docum...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...OST /api/documents/upload pipeline. - backend/routes/learn.py:152 — `build_system_pro...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 10 - 19, Update the stale migration notes to reflect
that Pydantic AI is now the chosen agent framework (not "not yet"), that agents
live under backend/agents/, and that the document processing pipeline is
implemented rather than only a refactor target; specifically, replace the "not
yet in `requirements.txt`" language and the "refactor target" phrasing with
current status, mention `Pydantic AI` as the active framework, and keep the repo
map references to backend/main.py, backend/routes/documents.py
(`_process_document` and `upload_document`) and backend/routes/learn.py
(`build_system_prompt`) so readers can find the implemented components.
| ``` | ||
| python main.py # uvicorn on PORT (see config.py), reload=True | ||
| python -m pytest tests/ -q # backend test suite | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced command blocks.
Line 33 and Line 40 trigger MD040; annotate these fences as shell/bash.
Lint-only fix
-```+```bash
python main.py # uvicorn on PORT (see config.py), reload=True
python -m pytest tests/ -q # backend test suite@@
- +bash
docker-compose up
Also applies to: 40-42
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 33 - 36, The markdown fenced command blocks that
currently lack a language tag (the blocks containing "python main.py ... python
-m pytest ..." and the block containing "docker-compose up") are triggering
MD040; update each opening triple-backtick to include "bash" (i.e., ```bash) so
the shells are annotated; ensure both command blocks are changed (the one with
the Python/pytest commands and the one with docker-compose) to resolve the lint
warning.
| - **Document upload** — `backend/routes/documents.py:266` `upload_document` runs sequentially: validate → `extraction_service.extract_text_from_file` → `_process_document` (one `call_gemini_json` for category/summary/concepts/assignments) → optional `save_assignments_to_db` (`backend/services/calendar_service.py:62`) for syllabi → optional `apply_graph_update` for syllabus/assignment concepts → insert `documents` row → invalidate `study_guides` cache → `check_achievements("documents_uploaded")`. | ||
| - **Chat with tutor** — `backend/routes/learn.py:311` `chat` rebuilds the system prompt via `build_system_prompt` (`backend/routes/learn.py:152`) using the live graph + course documents + cached `course_context`, calls `call_gemini_multiturn`, splits out `<graph_update>` via `extract_graph_update`, persists the assistant message, then calls `apply_graph_update` which lazy-imports `update_course_context` for any touched course. | ||
| - **Quiz generation** — `backend/routes/quiz.py:26` `generate_quiz` loads the target node + prior `quiz_context`, fills `prompts/quiz_generation.txt`, and (when `use_shared_context`) appends class-wide misconceptions and weak areas from `course_context_service.get_course_context` via `prompt += ...` before `call_gemini_json`. Result is stored in `quiz_attempts`. | ||
| - **Study guide** — `backend/routes/study_guide.py:18` `_generate_and_insert` fetches the exam row + all course `documents`, concatenates `summary` + `concept_notes` into a context block, calls `call_gemini_json`, and inserts into `study_guides`. The `/guide` GET serves cache-first; `upload_document` invalidates by deleting that user+course's rows. | ||
| - **Calendar / syllabus** — covered by the syllabus branch of `upload_document` above (`save_assignments_to_db` deduplicates by trimmed-title + calendar-day). The standalone `backend/services/calendar_service.py:77` `process_and_save_syllabus` exists for direct OCR→Gemini→DB use but is not currently wired to a route. | ||
| ## LLM seam (current) | ||
| Every LLM call in the codebase routes through `backend/services/gemini_service.py`, which holds a single module-level `genai.Client` pointed at `gemini-2.5-flash`. The four public entry points are `call_gemini` (`:62`, plain text), `call_gemini_multiturn` (`:88`, native chat history with system instruction), `call_gemini_json` (`:129`, JSON-mode + tolerant `_extract_json` fallback), and `extract_graph_update` (`:141`, parses the `<graph_update>` block out of tutor replies). This is the legacy seam: new LLM-driven work is intended to land as Pydantic AI agents under `backend/agents/`, replacing call sites incrementally (see `docs/decisions/`). That directory does not exist yet and `pydantic-ai` is not in `requirements.txt`. | ||
There was a problem hiding this comment.
This section still documents the pre-refactor upload architecture.
Line 11 and Line 19 describe the legacy path (_process_document single Gemini call, no backend/agents/, no pydantic-ai in requirements), which conflicts with the architecture introduced in this PR. Please update this block to reflect the orchestrator + SSE + legacy-fallback contract.
Based on learnings: "Document processing pipeline with _process_document ... is marked as a refactor target." and "Migrate from google-genai to Pydantic AI as the target agent framework; agents will live under backend/agents/."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture.md` around lines 11 - 20, Update the architecture doc to
replace the outdated pre-refactor description of document upload and LLM seam
with the new orchestrator + SSE + legacy-fallback contract: describe that
upload_document now delegates to the document processing orchestrator (instead
of a single `_process_document` Gemini call) which streams progress via SSE to
clients, invokes new agent-based handlers under `backend/agents/` (Pydantic AI
agents replacing direct `call_gemini_json` usage), and falls back to legacy
`backend/services/gemini_service` functions like `call_gemini_json`,
`call_gemini_multiturn`, and `extract_graph_update` only when agents aren’t
available; also note that `backend/agents/` is the intended home for new agent
implementations and that `pydantic-ai` must be added to requirements to complete
the migration.
Resolves correctness, observability, and test-coverage gaps surfaced
during /review of the agentic document upload re-architecture.
Routes (backend/routes/documents.py)
- _stream_legacy_fallback now emits a terminal error+done SSE pair when
the legacy path also fails, instead of leaving the client on a
silent EOF.
- _legacy_upload_pipeline schedules update_course_context for parity
with the orchestrator success path; the asymmetry meant fall-back
uploads left course context stale.
- New _spawn_post_roll helper attaches a done-callback so SSE
fire-and-forget tasks log their exceptions instead of disappearing.
- _grading_categories_from maps the orchestrator's grading_categories
to the legacy {name, weight} shape, fixing the categories=[]
regression on /upload/sync.
- SSE error events no longer leak raw exception strings; full detail
remains in logger.exception/logger.warning.
Agents
- New backend/agents/_providers.py with shared google_model() helper;
five agent modules de-duplicate the GoogleProvider boilerplate.
- agents/syllabus_extraction.py adds a GradingCategory model and a
grading_categories field on SyllabusAssignments, with prompt
guidance to extract weight buckets verbatim.
- agents/tools/graph.py drops the unused relationships field from
GraphUpdateInput so the LLM doesn't waste tokens on a discarded
payload.
Observability
- backend/main.py wires logfire.instrument_fastapi(app); requirements
upgraded to logfire[fastapi]>=2.0 to pull in the OpenTelemetry FastAPI
instrumentation deps.
Tests
- tests/test_documents_routes.py:
* _make_upload now targets /upload/sync (the legacy-contract endpoint
the existing assertions were written for).
* Autouse fixture forces the orchestrator to raise so existing tests
exercise _legacy_upload_pipeline as before.
* New TestUploadDocumentOrchestrator (7 tests) covers the
orchestrator success path: persistence, plaintext summary in the
response, grading-category passthrough, syllabus assignment
persistence with no-invent contract, and graph-backstop branching.
- 37/37 tests pass in test_documents_routes; 405/408 in the full
backend suite (the 3 remaining failures hit live Supabase from
unrelated test files and pre-date this branch).
Removed
- backend/scripts/cleanup_classifier_test.py (one-shot dev cleanup
with hardcoded user/document IDs from a personal session).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/routes/documents.py (2)
607-620:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit final
resultonly after persistence succeeds.Line 607 sends the final result before Line 618 persists. If persistence fails, Line 650 fallback can stream another result/done sequence and reprocess the same upload.
Suggested fix
- yield sapling_event_to_sse(SaplingEvent(- type="result", step="finalize",- message="Processing complete.",- data=final_output.model_dump(mode="json"),- ))- # ── Post-roll: side effects + persistence ───────────────────────── _save_orchestrator_syllabus(user_id=user_id, course_id=course_id, filename=filename, result=final_output) _graph_backstop(user_id=user_id, course_id=course_id, filename=filename, result=final_output) doc_id, _ = _persist_document(user_id=user_id, course_id=course_id, filename=filename, result=final_output) ++ yield sapling_event_to_sse(SaplingEvent(+ type="result", step="finalize",+ message="Processing complete.",+ data=final_output.model_dump(mode="json"),+ ))Also applies to: 636-660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 607 - 620, The final SaplingEvent("result", step="finalize") is emitted before persistence; change the flow so you call _save_orchestrator_syllabus, _graph_backstop and _persist_document first (checking _persist_document returns a successful doc_id), and only then yield sapling_event_to_sse(SaplingEvent(... final_output ...)); if persistence fails, catch the exception or check the failure and yield an error/result indicating persistence failure instead of the success finalize event; apply the same reorder/exception-handling change for the analogous block around lines 636-660 as well.
718-723:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t swallow achievement-task failures silently.
Line 723 drops exceptions with
pass, which hides broken achievement updates in production.Suggested fix
def _check_upload_achievements(user_id: str) -> None: """Background task: best-effort achievement check.""" try: check_achievements(user_id, "documents_uploaded", {}) except Exception: - pass+ logger.exception(+ "Achievement check failed after document upload for user=%s",+ user_id,+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 718 - 723, The helper _check_upload_achievements currently swallows all exceptions (except pass) which hides failures; change the except block to catch Exception as e and record the error (including stack trace and user_id context) using the application logger (e.g., logger.exception(...) or current_app.logger.exception(...)) so the failure is visible in logs while still keeping the task best-effort (do not re-raise); ensure the log message references _check_upload_achievements and the call to check_achievements(user_id, "documents_uploaded", {}).
🧹 Nitpick comments (1)
backend/agents/classifier.py (1)
20-29: ⚡ Quick winUse a single source of truth for document categories.
This literal duplicates
VALID_CATEGORIESinbackend/routes/documents.py; drift here can silently coerce valid classifier output to"other".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/agents/classifier.py` around lines 20 - 29, Replace the duplicated Literal in classifier.py with a single source of truth: remove the DocumentCategory Literal from backend/agents/classifier.py and instead import the canonical definitions from backend/routes/documents.py (use the existing VALID_CATEGORIES there and define/export DocumentCategory = Literal[...] in that module as the authoritative type); update documents.py so VALID_CATEGORIES is a tuple/constant and DocumentCategory is declared there, then import DocumentCategory (or VALID_CATEGORIES if you prefer deriving the type in one place) into classifier.py to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/agents/concept_extraction.py`:
- Around line 17-19: The Concept schema currently permits whitespace-only names;
add validation on Concept.name to normalize (trim) and enforce non-empty values
at the model boundary so invalid concepts are rejected early. Implement a
Pydantic validator (or use a constrained type) for the Concept class that strips
surrounding whitespace from name and raises a validation error if the resulting
string is empty, ensuring downstream code never receives whitespace-only concept
names.
In `@backend/agents/syllabus_extraction.py`:
- Line 44: The assignments field is currently required but the prompt allows an
empty list; update the SyllabusAssignment field declaration so it defaults to an
empty list instead of being mandatory — e.g., change the declaration of
assignments: list[SyllabusAssignment] = Field(max_length=50) to use a default
factory (assignments: list[SyllabusAssignment] = Field(default_factory=list,
max_length=50)) so empty assignments validate; apply the same change where this
pattern appears around the Syllabus model (the assignments field at the other
occurrence as noted).
---
Duplicate comments:
In `@backend/routes/documents.py`:
- Around line 607-620: The final SaplingEvent("result", step="finalize") is
emitted before persistence; change the flow so you call
_save_orchestrator_syllabus, _graph_backstop and _persist_document first
(checking _persist_document returns a successful doc_id), and only then yield
sapling_event_to_sse(SaplingEvent(... final_output ...)); if persistence fails,
catch the exception or check the failure and yield an error/result indicating
persistence failure instead of the success finalize event; apply the same
reorder/exception-handling change for the analogous block around lines 636-660
as well.
- Around line 718-723: The helper _check_upload_achievements currently swallows
all exceptions (except pass) which hides failures; change the except block to
catch Exception as e and record the error (including stack trace and user_id
context) using the application logger (e.g., logger.exception(...) or
current_app.logger.exception(...)) so the failure is visible in logs while still
keeping the task best-effort (do not re-raise); ensure the log message
references _check_upload_achievements and the call to
check_achievements(user_id, "documents_uploaded", {}).
---
Nitpick comments:
In `@backend/agents/classifier.py`:
- Around line 20-29: Replace the duplicated Literal in classifier.py with a
single source of truth: remove the DocumentCategory Literal from
backend/agents/classifier.py and instead import the canonical definitions from
backend/routes/documents.py (use the existing VALID_CATEGORIES there and
define/export DocumentCategory = Literal[...] in that module as the
authoritative type); update documents.py so VALID_CATEGORIES is a tuple/constant
and DocumentCategory is declared there, then import DocumentCategory (or
VALID_CATEGORIES if you prefer deriving the type in one place) into
classifier.py to avoid drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8addb596-d8d7-47b2-944e-bdaf28624d80
📒 Files selected for processing (11)
backend/agents/_providers.pybackend/agents/classifier.pybackend/agents/concept_extraction.pybackend/agents/document.pybackend/agents/summary.pybackend/agents/syllabus_extraction.pybackend/agents/tools/graph.pybackend/main.pybackend/requirements.txtbackend/routes/documents.pybackend/tests/test_documents_routes.py
✅ Files skipped from review due to trivial changes (2)
- backend/requirements.txt
- backend/agents/tools/graph.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/agents/summary.py
- backend/agents/document.py
| class Concept(BaseModel): | ||
| name: str = Field(max_length=120, description="Title Case noun phrase.") | ||
| description: str = Field( |
There was a problem hiding this comment.
Enforce non-empty normalized concept names at the schema boundary.
Line 18 allows whitespace-only name, which leaks invalid concepts downstream and relies on later defensive filtering.
Suggested fix
+from pydantic import field_validator+
class Concept(BaseModel):
name: str = Field(max_length=120, description="Title Case noun phrase.")
@@
+ `@field_validator`("name")+ `@classmethod`+ def _validate_name(cls, v: str) -> str:+ v = v.strip()+ if not v:+ raise ValueError("Concept name must be non-empty.")+ return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/agents/concept_extraction.py` around lines 17 - 19, The Concept
schema currently permits whitespace-only names; add validation on Concept.name
to normalize (trim) and enforce non-empty values at the model boundary so
invalid concepts are rejected early. Implement a Pydantic validator (or use a
constrained type) for the Concept class that strips surrounding whitespace from
name and raises a validation error if the resulting string is empty, ensuring
downstream code never receives whitespace-only concept names.
| class SyllabusAssignments(BaseModel): | ||
| course_title: str | None = Field(default=None, max_length=300) | ||
| instructor: str | None = Field(default=None, max_length=200) | ||
| assignments: list[SyllabusAssignment] = Field(max_length=50) |
There was a problem hiding this comment.
Align assignments field default with the prompt contract.
Line 44 makes assignments required, but Line 80 declares empty assignments valid. Missing key currently hard-fails validation unnecessarily.
Suggested fix
- assignments: list[SyllabusAssignment] = Field(max_length=50)+ assignments: list[SyllabusAssignment] = Field(default_factory=list, max_length=50)Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/agents/syllabus_extraction.py` at line 44, The assignments field is
currently required but the prompt allows an empty list; update the
SyllabusAssignment field declaration so it defaults to an empty list instead of
being mandatory — e.g., change the declaration of assignments:
list[SyllabusAssignment] = Field(max_length=50) to use a default factory
(assignments: list[SyllabusAssignment] = Field(default_factory=list,
max_length=50)) so empty assignments validate; apply the same change where this
pattern appears around the Syllabus model (the assignments field at the other
occurrence as noted).
Three follow-ups from the latest /review pass. - TestUploadDocumentStreaming: parses the EventSourceResponse byte stream and asserts on event ordering — status:start → progress:classify → progress:classified → progress:extract → progress:extracted → result:finalize → status:done. Includes a syllabus-path variant and a pre-stream HTTP 400 case. - TestProcessDocumentHelper: extracted the three _process_document harness tests out of TestUploadDocument so they no longer trip the autouse legacy-fallback fixture they don't need. - test_syllabus_grading_categories_pass_through_points_based: confirms weights > 100 (points-based grading) flow through unchanged, matching the "do not normalize" contract. Tests: 41/41 in test_documents_routes; 409/412 in the full backend suite (the 3 remaining failures hit live Supabase from unrelated test files and pre-date this branch). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| from types import SimpleNamespace | ||
| import pytest | ||
| from unittest.mock import MagicMock, patch | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/test_documents_routes.py (2)
807-830: 💤 Low value
_parse_sse_streamoverwrites duplicatedata:fields — minor SSE spec deviationcur[field.strip()] =value.lstrip() # last `data:` line silently winsThe SSE spec requires that multiple
data:lines within a single event block be concatenated with\nbefore JSON-parsing. The current dict-assignment overwrites earlier values, so any future route event that spans multipledata:lines would silently truncate. All current test payloads are single-line JSON so there's no immediate breakage, but the utility will silently misparse if the route ever emits a multi-line data field.♻️ Spec-compliant accumulation
- field, _, value = line.partition(":")- cur[field.strip()] = value.lstrip()+ field, _, value = line.partition(":")+ key = field.strip()+ val = value.lstrip()+ if key == "data" and key in cur:+ cur[key] = cur[key] + "\n" + val+ else:+ cur[key] = val🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_documents_routes.py` around lines 807 - 830, The _parse_sse_stream helper currently overwrites repeated fields (notably multiple "data:" lines) by doing cur[field.strip()] = value.lstrip(); change the logic in _parse_sse_stream so that when field.strip() == "data" you append value.lstrip() to any existing cur["data"] with a "\n" separator (preserving order), while other fields continue to be set/replaced as before; this makes cur and subsequent JSON parsing handle multi-line SSE data blocks per the SSE spec.
840-882: 💤 Low value
_mock_agent_runsreturns a bare tuple — positional destructuring is fragileBoth call-sites (
line 888,line 922) destructure the return value positionally:cls_p, sum_p, cpt_p, syl_p, doc_p=self._mock_agent_runs()Adding or reordering a patch inside
_mock_agent_runssilently misaligns every caller, and a count mismatch only raises at runtime. A simple named container (e.g., adataclassorSimpleNamespace) or unpacking into*patches(and spreading with*patchesin thewith (...)block) would make the coupling explicit.♻️ Example: SimpleNamespace approach
- return (- patch("routes.documents.classifier_agent.run", cls_run),- patch("routes.documents.summary_agent.run", sum_run),- patch("routes.documents.concept_extraction_agent.run", cpt_run),- patch("routes.documents.syllabus_extraction_agent.run", syl_run),- patch("routes.documents.document_agent.run_stream_events", _empty_stream),- )+ return SimpleNamespace(+ classifier=patch("routes.documents.classifier_agent.run", cls_run),+ summary=patch("routes.documents.summary_agent.run", sum_run),+ concept=patch("routes.documents.concept_extraction_agent.run", cpt_run),+ syllabus=patch("routes.documents.syllabus_extraction_agent.run", syl_run),+ document=patch("routes.documents.document_agent.run_stream_events", _empty_stream),+ )Then at call-sites:
p=self._mock_agent_runs() with ( _mock_validate_user(), ..., p.classifier, p.summary, p.concept, p.syllabus, p.document, ... ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_documents_routes.py` around lines 840 - 882, _mock_agent_runs currently returns a positional tuple which callers unpack positionally (e.g. cls_p, sum_p, cpt_p, syl_p, doc_p), making additions/reorders fragile; change _mock_agent_runs to return a named container (SimpleNamespace or small dataclass) with attributes matching each patch (e.g. classifier, summary, concept, syllabus, document) and update callers to retrieve patches via those attributes (e.g. p.classifier, p.summary, p.concept, p.syllabus, p.document) inside the with(...) block so patch ordering is explicit and robust to future edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 807-830: The _parse_sse_stream helper currently overwrites
repeated fields (notably multiple "data:" lines) by doing cur[field.strip()] =
value.lstrip(); change the logic in _parse_sse_stream so that when field.strip()
== "data" you append value.lstrip() to any existing cur["data"] with a "\n"
separator (preserving order), while other fields continue to be set/replaced as
before; this makes cur and subsequent JSON parsing handle multi-line SSE data
blocks per the SSE spec.
- Around line 840-882: _mock_agent_runs currently returns a positional tuple
which callers unpack positionally (e.g. cls_p, sum_p, cpt_p, syl_p, doc_p),
making additions/reorders fragile; change _mock_agent_runs to return a named
container (SimpleNamespace or small dataclass) with attributes matching each
patch (e.g. classifier, summary, concept, syllabus, document) and update callers
to retrieve patches via those attributes (e.g. p.classifier, p.summary,
p.concept, p.syllabus, p.document) inside the with(...) block so patch ordering
is explicit and robust to future edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fb704324-7785-4b1e-ad62-b06a76a41d2f
📒 Files selected for processing (1)
backend/tests/test_documents_routes.py
Wires the new /api/documents/upload SSE route into the document
upload modal so users see live per-phase progress instead of a
spinner that hangs for 8-15s.
Implementation
- frontend/src/lib/sse.ts: minimal streamSSE async generator that
reads a fetch Response body, parses the SSE wire format
(event: + data: + blank-line blocks), and yields typed events.
Uses fetch + ReadableStream because EventSource doesn't support
POST or multipart bodies.
- frontend/src/lib/api.ts:
* uploadDocument now points at /upload/sync (legacy JSON contract)
so existing callers (uploadSyllabus → SyllabusUploadFlow) keep
working without progress events.
* New uploadDocumentStream(formData, onEvent, signal) returns the
final document while invoking onEvent for every status / progress
/ result / error SSE event. Reconciles the document_id off the
final 'done' status when the orchestrator's result event omits it.
- frontend/src/components/DocumentUploadModal.tsx:
* Switches from uploadDocument → uploadDocumentStream.
* UploadItem gains a `progress?: string` field; the row renders
the latest backend message ('Classifying document...' →
'Classified as syllabus.' → 'Extracting summary, concepts and
syllabus in parallel...' → 'Extracted N concept(s).' → tool
call labels → 'Saved.') in an italic aria-live="polite" line
while status='uploading'.
* extractConceptNames helper handles BOTH response shapes:
orchestrator's nested concepts.concepts[].name and the legacy
fallback's flat concept_notes[].name.
* Surfaces classification.category from the orchestrator path,
falling back to legacy `category` when needed.
Verification
- npm run typecheck: passes.
- npm run lint: blocked by a pre-existing path-with-space issue in
`next lint`; not caused by this change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Three review fixes plus a real test suite for the SSE wire-format
parser. Both pieces landed in parallel via sub-agents.
Parser fixes (frontend/src/lib/sse.ts)
- Advance the buffer by the actual separator length: 4 chars on
\r\n\r\n, 2 chars on \n\n. The old code always advanced 2, leaving
a stray \r\n at the head of the next iteration. Downstream parsing
was incidentally tolerant, but the logic is no longer fragile.
- finally block now calls reader.cancel().catch(() => {}) before
releaseLock() so a consumer that breaks out of the for-await early
closes the underlying connection instead of leaking it until GC.
API fix (frontend/src/lib/api.ts)
- Dropped the dead `else if (docIdFromDone && !finalDoc)` branch in
uploadDocumentStream. The post-loop `if (!finalDoc) throw` already
guards that case; the branch could never deliver a usable result.
Vitest scaffold
- npm i -D vitest @vitest/coverage-v8
- Added `test` and `test:watch` scripts to frontend/package.json.
- frontend/vitest.config.ts: node environment, @ → ./src alias,
globs match src/**/*.test.ts(x).
- frontend/src/lib/sse.test.ts: 9 fixture-based tests covering
happy-path, default event="message", multi-line data joins
(JSON + raw), \r\n line endings, comment skip, mid-JSON chunk
split (the buffering case), trailing-block flush without final
blank line, non-2xx throws, and the \r\n\r\n separator edge case.
Verification
- npm run typecheck: passes
- npm test: 9/9 pass (~141ms)
- Front-end has its first test framework. Future SSE consumers
(chat tutor stream per refactor #3) get tests for free.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>…ation IDs V2 of the agentic document upload pipeline. Three independent improvements landed in parallel via sub-agents, plus the seven ADRs that record the decisions (four shipped, three deferred-design). Drop the orchestrator agent (ADR 0007) - backend/agents/document.py: deleted document_agent and GraphUpdateConfirmation. process_document now calls apply_concepts_to_graph directly. - backend/agents/tools/graph.py: split the merge into apply_concepts_to_graph (plain async, callable from anywhere) plus the existing apply_graph_update_tool wrapper for future agents. - backend/routes/documents.py: streaming /upload now emits progress:graph_update / progress:graph_updated events around the direct call instead of iterating document_agent.run_stream_events. - Removes one Gemini Pro round-trip per upload (~1-2s + Pro tokens). The agent had no decision-making — it always called the tool with arguments already produced by the workers. Per-task model routing + cost telemetry (ADR 0008) - backend/agents/_providers.py: new model_for(task) selector. Defaults: classifier and summary on gemini-2.5-flash-lite; concepts and syllabus on gemini-2.5-flash. Operators override via env var (SAPLING_MODEL_CLASSIFIER, _SUMMARY, _CONCEPTS, _SYLLABUS). - backend/agents/classifier|summary|concept_extraction|syllabus_extraction.py: switched to model_for(<task>); google_model retained as back-compat shim. - Cost telemetry: genai-prices is already a transitive dep of pydantic-ai-slim[google]; logfire.instrument_pydantic_ai() picks it up automatically. No code change needed in main.py. Request correlation IDs (ADR 0009) - backend/services/request_context.py (new): RequestIDMiddleware reads or generates X-Request-ID per request, contextvar exposes it to downstream code via current_request_id(). - backend/main.py: middleware registered last (runs outermost). Three global exception handlers (StarletteHTTPException, RequestValidationError, bare Exception) include request_id in error bodies and headers. - backend/routes/documents.py: streaming SSE error events now carry request_id in their data payload so users can correlate a failed upload to a Logfire span. Eval expansion (ADR 0008) - backend/tests/evals/document_classification.py: 10 → 25 cases. - backend/tests/evals/document_summary.py (new): 15 cases, 4 evaluators (abstract length, key-points count, headline length, no-markdown leak). - backend/tests/evals/concept_extraction.py (new): 15 cases, 4 evaluators (count range, no-administrative-names, title-case, importance-ordering). - backend/tests/evals/syllabus_extraction.py (new): 15 cases, 4 evaluators (assignment count, no-invented-dates, grading-categories presence, weights numeric). - Total: 70 eval cases across 4 agents. Run on-demand against live Gemini, not in default pytest collection. Tests - backend/tests/test_documents_routes.py: * Streaming-route fixtures patch apply_concepts_to_graph as AsyncMock and adjust the expected event sequence. * New TestRequestIDPropagation (4 tests): X-Request-ID echo, caller-supplied passthrough, invalid-ID replacement, error-body inclusion. * 45/45 pass in this file. Full backend suite: 413/416 (the 3 failures are pre-existing live-Supabase 409s in unrelated test files). - Frontend: typecheck clean, vitest 9/9. ADRs - 0006 — SSE protocol choice (sse-starlette + custom mapper, not VercelAIAdapter). - 0007 — Drop the orchestrator agent. - 0008 — Per-task model routing. - 0009 — Request correlation IDs. - 0010 — OCR async / two-phase upload (DEFERRED, design only). - 0011 — Durable execution via DBOS (DEFERRED, design only). - 0012 — Concept-by-concept streaming (DEFERRED, design only). Each deferred ADR records the trigger conditions for revisiting and the "what I'd try next" action plan, per the vault discipline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/DocumentUploadModal.tsx (1)
178-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback the optimistic category change if persistence fails.
The UI updates
categorybeforeupdateDocumentCategory(...)succeeds, but the failure path only toasts an error. That leaves the modal showing the new category even though the backend still has the old one.♻️ Proposed fix
const handleCategoryChange = async (item: UploadItem, next: string) => { - setItemField(item.id, prev => ({ ...prev, category: next }));+ const prevCategory = item.category;+ setItemField(item.id, prev => ({ ...prev, category: next })); if (item.docId) { try { await updateDocumentCategory(item.docId, userId, next); toast.success("Category updated"); } catch (err) { + setItemField(item.id, prev => ({ ...prev, category: prevCategory })); toast.error(`Failed: ${String(err)}`); } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/DocumentUploadModal.tsx` around lines 178 - 188, In handleCategoryChange, you're optimistically updating state via setItemField before updateDocumentCategory succeeds; capture the previous category (e.g., read prevCategory from the current item or from the prev callback) before calling setItemField, then call setItemField to apply the optimistic change, and if updateDocumentCategory(item.docId, userId, next) throws, call setItemField again to restore the previous category and show the toast error; reference handleCategoryChange, setItemField, updateDocumentCategory, item.docId and userId to locate where to capture and rollback the prior value.
♻️ Duplicate comments (6)
backend/agents/concept_extraction.py (1)
17-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize and reject blank concept names at the schema boundary.
Whitespace-only names still pass this model and only get trimmed later in the graph helper, which lets invalid concepts leak into downstream prompts and evals.
Suggested fix
-from pydantic import BaseModel, Field+from pydantic import BaseModel, Field, field_validator @@ class Concept(BaseModel): name: str = Field(max_length=120, description="Title Case noun phrase.") @@ importance: float = Field( ge=0.0, le=1.0, description="Centrality to the document; for ranking, not a gate.", ) ++ `@field_validator`("name")+ `@classmethod`+ def _normalize_name(cls, value: str) -> str:+ value = value.strip()+ if not value:+ raise ValueError("Concept name must be non-empty.")+ return value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/agents/concept_extraction.py` around lines 17 - 33, The Concept.name field currently allows whitespace-only values; update the Concept model so names are normalized (trimmed) and rejected if empty at schema validation time by applying a stripped-and-length-checked constraint or validator on Concept.name (e.g., use a constrained string with strip_whitespace=True and min_length=1 or a `@validator` on Concept.name that strips and raises ValueError for empty names); ensure this validation happens in Concept (not later) so ConceptList and downstream code only receive normalized, non-blank names.backend/agents/summary.py (1)
28-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelax
key_pointsfor sparse documents.
min_length=3still conflicts with the sparse-document behavior in the prompt, so near-empty uploads can fail validation or force hallucinated takeaways.Suggested fix
key_points: list[str] = Field( - min_length=3,+ min_length=0, max_length=8, - description="3-8 most important takeaways, each one sentence.",+ description="0-8 most important takeaways, each one sentence.", ) @@ - "prose with no markdown, math, or fenced blocks; and 3-8 key "+ "prose with no markdown, math, or fenced blocks; and 0-8 key " "takeaways, each one sentence, ordered by importance.\n\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/agents/summary.py` around lines 28 - 50, The Summary model's key_points Field currently forces min_length=3 which contradicts the summary_agent system_prompt's allowance for sparse/near-empty documents; update the Field on key_points (and its description) to allow 0–8 items (e.g., min_length=0, max_length=8) so validators won't require fabricated takeaways for sparse uploads, and ensure any downstream code that assumes at least 3 items (if any) gracefully handles shorter lists.backend/agents/tools/graph.py (1)
30-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the actual merge result, not the requested concept count.
apply_graph_updatededuplicates against existing rows, solen(new_nodes)can report success even when nothing was inserted. That makes the SSE confirmation and downstreamgraph_updatedflag overstate what happened.Suggested fix
- await asyncio.to_thread(- apply_graph_update,- user_id,- {"new_nodes": new_nodes},- course_id,- )- return len(new_nodes)+ changes = await asyncio.to_thread(+ apply_graph_update,+ user_id,+ {"new_nodes": new_nodes},+ course_id,+ )+ return len(changes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/agents/tools/graph.py` around lines 30 - 54, apply_concepts_to_graph currently returns len(new_nodes) which can overstate work because apply_graph_update deduplicates; instead capture the return value from apply_graph_update (call it via await asyncio.to_thread) and return the actual merge/insert count it provides. Update apply_concepts_to_graph to assign the result of asyncio.to_thread(apply_graph_update, user_id, {"new_nodes": new_nodes}, course_id) to a variable, then extract an integer merge count from that result (handle cases where the call returns an int, or a dict with keys like "merged", "inserted", or "rows_affected") and return that count (fall back to 0 if nothing present). Ensure references to apply_concepts_to_graph and apply_graph_update are used so the change is easy to locate.backend/agents/document.py (1)
117-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the legacy graph-write gate here.
process_document()now merges concepts for every upload, which changes persisted behavior versus the legacy path that only backstopped assignment/syllabus documents. Keep this branch gated so non-eligible uploads don't mutate the graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/agents/document.py` around lines 117 - 128, process_document is currently calling apply_concepts_to_graph unconditionally which changes legacy behavior; wrap the apply_concepts_to_graph call in the original "graph-write" gate so only eligible uploads mutate the graph. Concretely, in the block that uses workers and deps (workers, concept_names), add a conditional check (e.g., call an existing helper or add a predicate like should_write_graph(deps) / deps.is_backstop_eligible) and only invoke apply_concepts_to_graph(deps.user_id, deps.course_id, concept_names) when that predicate is true; otherwise set merged = 0 (and ensure DocumentProcessingResult.graph_updated is computed from merged > 0). Keep the rest of the returned fields (classification, summary, concepts, syllabus) unchanged.backend/routes/documents.py (2)
603-615:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
resultonly after persistence succeeds.Line 603 emits a final
resultbefore_persist_document(Line 614). If persistence or later post-roll logic fails, the catch block (Line 648+) falls back and can emit anotherresult/done, causing duplicate client completion semantics and possible duplicate processing.Proposed fix
- yield sapling_event_to_sse(SaplingEvent(- type="result", step="finalize",- message="Processing complete.",- data=final_output.model_dump(mode="json"),- ))- # ── Post-roll: side effects + persistence ───────────────────────── _save_orchestrator_syllabus(user_id=user_id, course_id=course_id, filename=filename, result=final_output) _graph_backstop(user_id=user_id, course_id=course_id, filename=filename, result=final_output) doc_id, _ = _persist_document(user_id=user_id, course_id=course_id, filename=filename, result=final_output) ++ yield sapling_event_to_sse(SaplingEvent(+ type="result", step="finalize",+ message="Processing complete.",+ data=final_output.model_dump(mode="json"),+ ))Also applies to: 632-660
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 603 - 615, The final SaplingEvent(result, step="finalize") is emitted before performing post-roll side effects and persistence, which can lead to duplicate/incorrect client completion if those operations fail; move the yield of sapling_event_to_sse(SaplingEvent(..., data=final_output.model_dump(...))) so it runs only after _save_orchestrator_syllabus(user_id, course_id, filename, result=final_output), _graph_backstop(user_id, course_id, filename, result=final_output) and a successful _persist_document(user_id, course_id, filename, result=final_output) return, or alternatively wrap those three calls, check for success, and emit the final SaplingEvent only on success (refer to functions sapling_event_to_sse, SaplingEvent, _save_orchestrator_syllabus, _graph_backstop, _persist_document and variables final_output, user_id, course_id, filename).
722-727:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t swallow background achievement failures silently.
At Line 726-727,
except Exception: passremoves all failure visibility for_check_upload_achievements, making regressions hard to diagnose.Proposed fix
def _check_upload_achievements(user_id: str) -> None: """Background task: best-effort achievement check.""" try: check_achievements(user_id, "documents_uploaded", {}) except Exception: - pass+ logger.exception(+ "Achievement check failed after document upload for user=%s",+ user_id,+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 722 - 727, The try/except in _check_upload_achievements currently swallows all errors; update it to catch Exception and log the failure (including exception details and user_id) via the existing logger or processLogger, e.g., inside the except block call logger.exception or logger.error with the exception info, so failures from check_achievements("documents_uploaded", ...) are visible for debugging; do not rework check_achievements itself—only replace the silent pass in _check_upload_achievements with a logged error that includes context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/main.py`:
- Around line 62-69: The custom http_exception_handler replaces existing HTTP
exception headers (losing exc.headers like
WWW-Authenticate/Retry-After/Location); update the handler
(http_exception_handler for StarletteHTTPException) to merge exc.headers with
the X-Request-ID header instead of overwriting: compute a headers dict by
starting from exc.headers or an empty dict, then add or set "X-Request-ID" when
rid is present, and pass that merged dict into JSONResponse(headers=...). Ensure
you handle exc.headers being None and preserve all original header values.
In `@backend/tests/evals/document_summary.py`:
- Around line 63-75: NoMarkdownLeakEvaluator currently only checks
ctx.output.abstract for markdown markers; update evaluate to scan all textual
output fields (ctx.output.abstract, ctx.output.headline, and each entry in
ctx.output.key_points) and return 0.0 if any of the markers "**", "```", or "$"
appear in any of those fields, otherwise return 1.0; locate the evaluate method
on NoMarkdownLeakEvaluator and replace the single-field checks with a combined
iterable check (e.g., build texts = [ctx.output.abstract, ctx.output.headline,
*ctx.output.key_points] and use any(...) over markers and texts).
In `@backend/tests/evals/syllabus_extraction.py`:
- Around line 88-94: The evaluator currently returns true if any concrete date
exists in the entire input (using _input_has_concrete_date), which lets one real
date mask invented dates on other assignments; update evaluate (the method in
this file) to validate per-assignment: iterate ctx.output.assignments and for
each assignment with a non-None due_date verify that the corresponding source in
ctx.inputs (match by assignment identifier/title/span metadata present on the
output item) contains a concrete date/span that justifies that specific
assignment.due_date; replace the global _input_has_concrete_date check with this
per-item provenance check and return failure if any assignment’s due_date lacks
a matching concrete date in its linked input span.
- Around line 45-62: The _DATE_PATTERNS list currently lacks Spanish month
formats so strings like "10 de febrero de 2026" won't match; update
_DATE_PATTERNS to include a regex that recognizes Spanish month names and the
"de" connectors (e.g., match "10 de febrero de 2026", "10 feb 2026", "10 de
feb.", and "febrero 10, 2026"), by extending the existing month-name patterns:
add Spanish month alternatives (enero, febrero, marzo, abril, mayo, junio,
julio, agosto, septiembre, octubre, noviembre, diciembre and common
abbreviations) into the two month-name regex entries (both the "Month day[,
year]" pattern used with re.IGNORECASE and the "day Month" pattern), and add an
additional pattern to handle the "day de Month de year" structure with optional
abbreviated months and optional year; ensure re.IGNORECASE is set so
capitalization is handled.
---
Outside diff comments:
In `@frontend/src/components/DocumentUploadModal.tsx`:
- Around line 178-188: In handleCategoryChange, you're optimistically updating
state via setItemField before updateDocumentCategory succeeds; capture the
previous category (e.g., read prevCategory from the current item or from the
prev callback) before calling setItemField, then call setItemField to apply the
optimistic change, and if updateDocumentCategory(item.docId, userId, next)
throws, call setItemField again to restore the previous category and show the
toast error; reference handleCategoryChange, setItemField,
updateDocumentCategory, item.docId and userId to locate where to capture and
rollback the prior value.
---
Duplicate comments:
In `@backend/agents/concept_extraction.py`:
- Around line 17-33: The Concept.name field currently allows whitespace-only
values; update the Concept model so names are normalized (trimmed) and rejected
if empty at schema validation time by applying a stripped-and-length-checked
constraint or validator on Concept.name (e.g., use a constrained string with
strip_whitespace=True and min_length=1 or a `@validator` on Concept.name that
strips and raises ValueError for empty names); ensure this validation happens in
Concept (not later) so ConceptList and downstream code only receive normalized,
non-blank names.
In `@backend/agents/document.py`:
- Around line 117-128: process_document is currently calling
apply_concepts_to_graph unconditionally which changes legacy behavior; wrap the
apply_concepts_to_graph call in the original "graph-write" gate so only eligible
uploads mutate the graph. Concretely, in the block that uses workers and deps
(workers, concept_names), add a conditional check (e.g., call an existing helper
or add a predicate like should_write_graph(deps) / deps.is_backstop_eligible)
and only invoke apply_concepts_to_graph(deps.user_id, deps.course_id,
concept_names) when that predicate is true; otherwise set merged = 0 (and ensure
DocumentProcessingResult.graph_updated is computed from merged > 0). Keep the
rest of the returned fields (classification, summary, concepts, syllabus)
unchanged.
In `@backend/agents/summary.py`:
- Around line 28-50: The Summary model's key_points Field currently forces
min_length=3 which contradicts the summary_agent system_prompt's allowance for
sparse/near-empty documents; update the Field on key_points (and its
description) to allow 0–8 items (e.g., min_length=0, max_length=8) so validators
won't require fabricated takeaways for sparse uploads, and ensure any downstream
code that assumes at least 3 items (if any) gracefully handles shorter lists.
In `@backend/agents/tools/graph.py`:
- Around line 30-54: apply_concepts_to_graph currently returns len(new_nodes)
which can overstate work because apply_graph_update deduplicates; instead
capture the return value from apply_graph_update (call it via await
asyncio.to_thread) and return the actual merge/insert count it provides. Update
apply_concepts_to_graph to assign the result of
asyncio.to_thread(apply_graph_update, user_id, {"new_nodes": new_nodes},
course_id) to a variable, then extract an integer merge count from that result
(handle cases where the call returns an int, or a dict with keys like "merged",
"inserted", or "rows_affected") and return that count (fall back to 0 if nothing
present). Ensure references to apply_concepts_to_graph and apply_graph_update
are used so the change is easy to locate.
In `@backend/routes/documents.py`:
- Around line 603-615: The final SaplingEvent(result, step="finalize") is
emitted before performing post-roll side effects and persistence, which can lead
to duplicate/incorrect client completion if those operations fail; move the
yield of sapling_event_to_sse(SaplingEvent(...,
data=final_output.model_dump(...))) so it runs only after
_save_orchestrator_syllabus(user_id, course_id, filename, result=final_output),
_graph_backstop(user_id, course_id, filename, result=final_output) and a
successful _persist_document(user_id, course_id, filename, result=final_output)
return, or alternatively wrap those three calls, check for success, and emit the
final SaplingEvent only on success (refer to functions sapling_event_to_sse,
SaplingEvent, _save_orchestrator_syllabus, _graph_backstop, _persist_document
and variables final_output, user_id, course_id, filename).
- Around line 722-727: The try/except in _check_upload_achievements currently
swallows all errors; update it to catch Exception and log the failure (including
exception details and user_id) via the existing logger or processLogger, e.g.,
inside the except block call logger.exception or logger.error with the exception
info, so failures from check_achievements("documents_uploaded", ...) are visible
for debugging; do not rework check_achievements itself—only replace the silent
pass in _check_upload_achievements with a logged error that includes context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb7f241f-06d8-40fd-84b5-07d19d8cba23
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
backend/agents/_providers.pybackend/agents/classifier.pybackend/agents/concept_extraction.pybackend/agents/document.pybackend/agents/summary.pybackend/agents/syllabus_extraction.pybackend/agents/tools/graph.pybackend/main.pybackend/routes/documents.pybackend/services/request_context.pybackend/tests/evals/concept_extraction.pybackend/tests/evals/document_classification.pybackend/tests/evals/document_summary.pybackend/tests/evals/syllabus_extraction.pybackend/tests/test_documents_routes.pydocs/decisions/0006-sse-protocol-choice.mddocs/decisions/0007-drop-orchestrator-agent.mddocs/decisions/0008-per-task-model-routing.mddocs/decisions/0009-request-correlation-ids.mddocs/decisions/0010-ocr-async-two-phase-upload.mddocs/decisions/0011-durable-execution-dbos.mddocs/decisions/0012-concept-by-concept-streaming.mdfrontend/package.jsonfrontend/src/components/DocumentUploadModal.tsxfrontend/src/lib/api.tsfrontend/src/lib/sse.test.tsfrontend/src/lib/sse.tsfrontend/vitest.config.ts
✅ Files skipped from review due to trivial changes (6)
- frontend/vitest.config.ts
- docs/decisions/0008-per-task-model-routing.md
- docs/decisions/0006-sse-protocol-choice.md
- docs/decisions/0009-request-correlation-ids.md
- docs/decisions/0007-drop-orchestrator-agent.md
- docs/decisions/0012-concept-by-concept-streaming.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/agents/syllabus_extraction.py
- backend/agents/classifier.py
| @app.exception_handler(StarletteHTTPException) | ||
| async def http_exception_handler(request: Request, exc: StarletteHTTPException): | ||
| rid = getattr(request.state, "request_id", None) or current_request_id() | ||
| return JSONResponse( | ||
| status_code=exc.status_code, | ||
| content={"detail": exc.detail, "request_id": rid}, | ||
| headers={"X-Request-ID": rid} if rid else {}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and the specific lines mentioned
cat -n backend/main.py | head -80Repository: SaplingLearn/Sapling
Length of output: 3454
🏁 Script executed:
# Search for any usage of StarletteHTTPException in the codebase
rg "StarletteHTTPException" --type py -B 2 -A 2Repository: SaplingLearn/Sapling
Length of output: 681
🏁 Script executed:
# Look for any places where headers are set on HTTP exceptions
rg "HTTPException.*headers" --type py -A 2 -B 2Repository: SaplingLearn/Sapling
Length of output: 46
🏁 Script executed:
# Check if the exception handler is defined elsewhere or if there are other exception handlers
rg "exception_handler" backend/main.py -A 5Repository: SaplingLearn/Sapling
Length of output: 1196
🌐 Web query:
Starlette HTTPException headers attribute
💡 Result:
Starlette’s HTTPException supports a headers attribute/argument. In Starlette, HTTPException is constructed as HTTPException(status_code, detail=None, headers=None). The headers value is stored on the exception as exc.headers and can be used by exception handling middleware/handlers to set headers on the resulting response (e.g., JSONResponse(..., headers=exc.headers)). Practical usage: - Raise: raise HTTPException(status_code=..., detail=..., headers={"WWW-Authenticate": "Basic ..."}) - Ensure the exception is handled in Starlette/FastAPI in a way that propagates exc.headers to the response (Starlette’s documented exception handler example does so).
Citations:
- 1: https://starlette.dev/exceptions/
- 2: https://github.com/Kludex/starlette/blob/main/docs/exceptions.md
- 3: https://github.com/encode/starlette/blob/master/starlette/exceptions.py
Preserve original HTTP exception headers in the custom handler.
At line 68, the handler replaces headers instead of merging them. Starlette's HTTPException supports a headers attribute (e.g., for WWW-Authenticate, Retry-After, Location), and these will be lost. Merge exc.headers with X-Request-ID:
Proposed fix
`@app.exception_handler`(StarletteHTTPException)
async def http_exception_handler(request: Request, exc: StarletteHTTPException):
rid = getattr(request.state, "request_id", None) or current_request_id()
+ headers = dict(getattr(exc, "headers", {}) or {})+ if rid:+ headers["X-Request-ID"] = rid
return JSONResponse(
status_code=exc.status_code,
content={"detail": exc.detail, "request_id": rid},
- headers={"X-Request-ID": rid} if rid else {},+ headers=headers,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/main.py` around lines 62 - 69, The custom http_exception_handler
replaces existing HTTP exception headers (losing exc.headers like
WWW-Authenticate/Retry-After/Location); update the handler
(http_exception_handler for StarletteHTTPException) to merge exc.headers with
the X-Request-ID header instead of overwriting: compute a headers dict by
starting from exc.headers or an empty dict, then add or set "X-Request-ID" when
rid is present, and pass that merged dict into JSONResponse(headers=...). Ensure
you handle exc.headers being None and preserve all original header values.
| @dataclass | ||
| class NoMarkdownLeakEvaluator(Evaluator[str, Summary]): | ||
| """Fail when the abstract contains markdown bold, fenced code, or $.""" | ||
| def evaluate(self, ctx: EvaluatorContext[str, Summary]) -> float: | ||
| text = ctx.output.abstract | ||
| if "**" in text: | ||
| return 0.0 | ||
| if "```" in text: | ||
| return 0.0 | ||
| if "$" in text: | ||
| return 0.0 | ||
| return 1.0 |
There was a problem hiding this comment.
Broaden the markdown leak check beyond the abstract.
NoMarkdownLeakEvaluator only inspects abstract, so markdown in headline or key_points can still pass even though those fields are rendered too.
♻️ Proposed fix
def evaluate(self, ctx: EvaluatorContext[str, Summary]) -> float:
- text = ctx.output.abstract- if "**" in text:- return 0.0- if "```" in text:- return 0.0- if "$" in text:- return 0.0+ texts = [ctx.output.abstract, ctx.output.headline, *ctx.output.key_points]+ if any(marker in text for text in texts for marker in ("**", "```", "$")):+ return 0.0
return 1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/evals/document_summary.py` around lines 63 - 75,
NoMarkdownLeakEvaluator currently only checks ctx.output.abstract for markdown
markers; update evaluate to scan all textual output fields (ctx.output.abstract,
ctx.output.headline, and each entry in ctx.output.key_points) and return 0.0 if
any of the markers "**", "```", or "$" appear in any of those fields, otherwise
return 1.0; locate the evaluate method on NoMarkdownLeakEvaluator and replace
the single-field checks with a combined iterable check (e.g., build texts =
[ctx.output.abstract, ctx.output.headline, *ctx.output.key_points] and use
any(...) over markers and texts).
| _DATE_PATTERNS = [ | ||
| # 2026-04-01, 2026/04/01 | ||
| re.compile(r"\b\d{4}[-/]\d{1,2}[-/]\d{1,2}\b"), | ||
| # 4/1/2026, 4-1-26, 04/01 | ||
| re.compile(r"\b\d{1,2}[/-]\d{1,2}(?:[/-]\d{2,4})?\b"), | ||
| # April 1, 2026 / April 1 / Apr 1 | ||
| re.compile( | ||
| r"\b(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Sept|Oct|Nov|Dec)" | ||
| r"[a-z]*\.?\s+\d{1,2}(?:,?\s*\d{4})?\b", | ||
| re.IGNORECASE, | ||
| ), | ||
| # 1 April 2026 / 1 Apr | ||
| re.compile( | ||
| r"\b\d{1,2}\s+(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Sept|Oct|Nov|Dec)" | ||
| r"[a-z]*\.?\b", | ||
| re.IGNORECASE, | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Recognize Spanish date formats in the concrete-date check.
The current patterns only cover numeric dates and English month names, so the Spanish case here (10 de febrero de 2026) will be treated as “no concrete date” and a valid due_date will be flagged as invented.
♻️ Proposed fix
re.compile(
r"\b\d{1,2}\s+(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Sept|Oct|Nov|Dec)"
r"[a-z]*\.?\b",
re.IGNORECASE,
),
+ re.compile(+ r"\b\d{1,2}\s+de\s+(?:enero|febrero|marzo|abril|mayo|junio|"+ r"julio|agosto|septiembre|setiembre|octubre|noviembre|diciembre)"+ r"\s+de\s+\d{4}\b",+ re.IGNORECASE,+ ),
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/evals/syllabus_extraction.py` around lines 45 - 62, The
_DATE_PATTERNS list currently lacks Spanish month formats so strings like "10 de
febrero de 2026" won't match; update _DATE_PATTERNS to include a regex that
recognizes Spanish month names and the "de" connectors (e.g., match "10 de
febrero de 2026", "10 feb 2026", "10 de feb.", and "febrero 10, 2026"), by
extending the existing month-name patterns: add Spanish month alternatives
(enero, febrero, marzo, abril, mayo, junio, julio, agosto, septiembre, octubre,
noviembre, diciembre and common abbreviations) into the two month-name regex
entries (both the "Month day[, year]" pattern used with re.IGNORECASE and the
"day Month" pattern), and add an additional pattern to handle the "day de Month
de year" structure with optional abbreviated months and optional year; ensure
re.IGNORECASE is set so capitalization is handled.
| def evaluate( | ||
| self, ctx: EvaluatorContext[str, SyllabusAssignments] | ||
| ) -> float: | ||
| any_due = any(a.due_date is not None for a in ctx.output.assignments) | ||
| if not any_due: | ||
| return 1.0 # vacuously fine | ||
| return 1.0 if _input_has_concrete_date(ctx.inputs) else 0.0 |
There was a problem hiding this comment.
Validate due dates per assignment, not per document.
NoInventedDatesEvaluator passes whenever the input contains any concrete date, so one real date can mask a hallucinated due_date on a different assignment in the same syllabus. The mixed concrete/relative case here still false-passes unless the evaluator ties each output item back to the specific source text/span that justified it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/evals/syllabus_extraction.py` around lines 88 - 94, The
evaluator currently returns true if any concrete date exists in the entire input
(using _input_has_concrete_date), which lets one real date mask invented dates
on other assignments; update evaluate (the method in this file) to validate
per-assignment: iterate ctx.output.assignments and for each assignment with a
non-None due_date verify that the corresponding source in ctx.inputs (match by
assignment identifier/title/span metadata present on the output item) contains a
concrete date/span that justifies that specific assignment.due_date; replace the
global _input_has_concrete_date check with this per-item provenance check and
return failure if any assignment’s due_date lacks a matching concrete date in
its linked input span.
… evals-CI, durable shim
Six independent improvements landed in parallel via four sub-agents
plus a solo phase, addressing every gap surfaced in the latest review.
Observability + safety
- backend/services/logfire_scrubber.py: scrubber callback wired into
logfire.configure(scrubbing=ScrubbingOptions(...)). Truncates +
fingerprints risky attributes (gen_ai.prompt, completion, messages,
user_prompt, etc.) so user document text doesn't leak verbatim to
logfire.pydantic.dev. Defaults still redact secrets/passwords.
- Each worker agent (classifier/summary/concepts/syllabus) extracts
its system prompt to a module-level constant, computes a 12-char
sha256 hash, and passes metadata={"prompt_version": <hash>} to the
Agent constructor — flows into the run span automatically and lets
us answer "which prompt produced this misclassification?" weeks
later via Logfire query.
Idempotency + correlation
- backend/services/request_context.py: middleware already in place;
SaplingDeps.request_id now adopts request.state.request_id (or
current_request_id()) so agent traces and SSE error payloads share
one correlation key.
- backend/routes/documents.py: _existing_doc_by_request_id helper
short-circuits the orchestrator on X-Request-ID replay; both /upload
and /upload/sync write the request_id column on insert and dedupe
retries. Defensive against the schema not being migrated yet.
- backend/db/migration_documents_request_id.sql: ALTER TABLE
documents ADD COLUMN request_id text + partial UNIQUE INDEX. Apply
on staging first; old rows have request_id=NULL.
UX
- backend/routes/documents.py: _stream_legacy_fallback emits a
progress:fallback_processing event before the legacy single-call
pipeline runs, replacing a 14-second blank spinner with a live
status update.
- frontend/src/components/DocumentUploadModal.tsx: SSE error events
now toast (warn for fallback, error for terminal failed),
request_id is captured per attempt and surfaced as a "Reference:
ABCD…" line with a copy button on failed rows. Retry button on
error/aborted rows mints a fresh X-Request-ID so the backend's
idempotency cache doesn't short-circuit retries.
- frontend/src/lib/api.ts: uploadDocumentStream accepts an optional
requestId arg and threads it as X-Request-ID into the streaming
fetch headers. New api.test.ts verifies the header passthrough.
Evals in CI
- backend/tests/evals/_replay.py: SAPLING_EVAL_MODE=record|replay|live
driver. Cassettes under tests/evals/cassettes/<dataset>/<case>.json.
- All 4 eval modules (classification, summary, concept_extraction,
syllabus_extraction) updated to route through run_with_cassette.
- 4 cassettes recorded (one per dataset) as a working-mode proof.
Remaining 66 cassettes recorded by future SAPLING_EVAL_MODE=record
pass before the workflow goes green-on-clean.
- .github/workflows/evals.yml: runs all 4 datasets in replay mode on
PRs touching agents/evals/streaming. cli_main exits 1 if any case
fails or any evaluator scores < 1.0 (pydantic-evals swallows errors
by default; we override).
- backend/requirements.txt: pydantic-evals>=0.0.5 (un-commented).
Durable execution + OCR async (feature-flagged)
- backend/services/durable.py: @workflow / @step decorators activate
as real DBOS when DBOS_ENABLED=true + dbos importable, else no-op
passthroughs. process_document is wrapped in @durable_workflow —
flipping the flag activates checkpointing without further code
changes.
- backend/routes/documents.py: OCR_ASYNC_ENABLED=true moves
extract_text_from_file off the synchronous request path into the
SSE stream context with progress:extracting_text events. Default
off; lightweight version of ADR 0010's two-phase upload (full
version still deferred — needs queue infra).
ADRs
- 0010 updated: feature-flag shipped, full two-phase deferred.
- 0011 updated: optional shim shipped, real DBOS opt-in.
Tests
- Backend: 418/421 pass (3 pre-existing live-Supabase failures
unchanged).
- tests/test_documents_routes.py: 47/47 (45 prior + 2 idempotency).
- tests/test_logfire_scrubber.py: 3/3 (new).
- Frontend: typecheck clean. Vitest: 10/10 (9 prior + 1 X-Request-ID
passthrough).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>| # JsonPath the scrubber walks (e.g. ('attributes', 'gen_ai.prompt'), | ||
| # ('attributes', 'all_messages_events', 0, 'content')). Conservative — | ||
| # easier to add safe attrs to the allowlist than to retract a leak. | ||
| _RISKY_PATH_TOKENS = ( |
| from __future__ import annotations | ||
| import asyncio |
| from __future__ import annotations | ||
| import asyncio |
| from __future__ import annotations | ||
| import asyncio |
| from __future__ import annotations | ||
| import asyncio |
Uh oh!
There was an error while loading. Please reload this page.
1. OCR-async double-fault (correctness) When OCR_ASYNC_ENABLED=true and the threaded extractor raises, the route was falling through to _stream_legacy_fallback with extracted_text=None — the legacy path then crashed inside _process_document on `extracted_text[:12000]`. The streaming route now wraps the asyncio.to_thread call in its own try/except that emits a terminal error+done SSE pair and returns, so the client gets a clean failure instead of a 500-shaped double-fault. 2. DBOS step granularity (correctness vs documented behavior) ADR 0011 promised "resume from the last completed step" on a crash, but @durable_workflow on process_document checkpointed the whole pipeline as one unit — there were no inner steps to resume from. Wrapped each agent call in _run_workers as a @durable_step (_step_classify, _step_summary, _step_concepts, _step_syllabus). When DBOS_ENABLED=true, a worker crash mid-gather resumes at the last completed step instead of re-running every agent. When DBOS is off (default), durable_step is a no-op passthrough — same behavior as before. 3. Evals workflow trigger (operational) Only 4 of 70 cassettes are recorded, so the pull_request trigger would fail every PR until the remaining 66 are filled. Switched to workflow_dispatch only, with the pull_request stanza commented in as a re-enable-when-ready marker. 4. Logfire scrubber test coverage (test gap) Original 3 tests only exercised the pure scrub_attribute helper. Added 6 more (9 total): nested list/dict redaction, deeply nested Pydantic AI all_messages_events shape, and three tests of the actual scrub_value(ScrubMatch) callback shape — including None-return for non-risky paths so Logfire's default password/secret redaction still kicks in. Tests - backend: tests/test_documents_routes.py 48/48 (47 + new test_async_ocr_failure_emits_terminal_error_no_legacy_fallthrough); tests/test_logfire_scrubber.py 9/9; full suite 425/428 (the 3 pre-existing live-Supabase failures unchanged). - frontend: typecheck clean, vitest 10/10. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Three follow-ups from the review of the previous fix commit. Two ran
in parallel via sub-agents, one solo (docs).
Backend — synchronous OCR no longer 500s
- backend/routes/documents.py: new _extract_text_or_400 helper wraps
extract_text_from_file in a try/except that converts any extractor
exception into HTTPException(422) with a friendly detail. Both
upload routes' synchronous call sites updated; the async-OCR path
(already covered) is unchanged. The global StarletteHTTPException
handler in main.py:76 attaches request_id to the body automatically.
- 2 new tests (50/50 in test_documents_routes.py):
* test_sync_ocr_failure_returns_422_not_500 (TestUploadDocument)
* test_sync_ocr_failure_in_streaming_route_returns_422_before_stream
(TestUploadDocumentStreaming, default OCR_ASYNC_ENABLED=false)
Frontend — component tests for upload error UX
- npm i -D jsdom @testing-library/{react,dom,user-event}
- frontend/src/components/DocumentUploadModal.test.tsx (new, 247
lines, 4 tests). Uses per-file `// @vitest-environment jsdom`
directive so the existing node-env lib tests stay fast.
- Tests cover the four UX behaviors added in b20ecf2 with no
coverage:
* toast.error fires on terminal SSE error event (step="failed")
* toast.warn (NOT error) fires on degraded-mode events
(step="fallback")
* Retry button mints a fresh X-Request-ID per attempt (pinning the
backend idempotency-cache contract)
* "Reference: <abbreviated>" line + clipboard copy button surfaces
request_id on failed rows
- vitest 14/14, typecheck clean.
Docs — workflow-internal step contract + streaming asymmetry
- backend/agents/document.py: module docstring now explicitly marks
_step_* as workflow-internal. Calling them outside process_document
is undefined behavior under DBOS.
- docs/decisions/0011-durable-execution-dbos.md: new sections
documenting (a) the step granularity that landed in 918fdba and
(b) the intentional non-durability of the streaming /upload route.
SSE connections are per-process — re-running on the next dedup'd
retry via X-Request-ID is the right semantic, not workflow resume.
Tests
- backend: 427/430 (425 + 2 new sync-OCR tests; 3 pre-existing
live-Supabase failures unchanged).
- frontend: 14/14 (10 + 4 new component tests).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>| """Background task: best-effort achievement check.""" | ||
| try: | ||
| check_achievements(user_id, "documents_uploaded", {}) | ||
| except Exception: |
Three small follow-ups from the latest review pass. Backend - Renamed _extract_text_or_400 -> _extract_text_or_422. The function raises HTTPException(422); the old name lied about the status code. Frontend tests - jest-dom matchers wired up. New frontend/vitest.setup.ts pulls in '@testing-library/jest-dom/vitest' so .toBeInTheDocument / .toHaveTextContent / .toHaveAttribute are available globally; safe for node-env tests because the matchers no-op when there's no DOM. - DocumentUploadModal.test.tsx: * Test 1's terminal-error toast assertion now pins the exact contract (toBe(2) — both the in-band `toast.error` and the catch-block one). Previously a soft `> 0` assertion that would pass even after one half got accidentally suppressed. * Test 2's mock event uses step="finalize" matching the backend's actual SSE wire format (was step="result"). Component branches on ev.type only, so both shapes pass — but the fixture now matches reality. * Test 3 introduces a named REQUEST_ID_ARG_INDEX constant with a comment explaining the positional-arg pin and what to update if uploadDocumentStream's signature ever switches to named options. * Two queryByText / textContent assertions converted to the idiomatic .toBeInTheDocument / .toHaveTextContent forms now that jest-dom is in scope. Tests - backend: 50/50 in test_documents_routes.py; full suite 427/430 (3 pre-existing live-Supabase failures unchanged). - frontend: typecheck clean. vitest 14/14 (3 test files, ~1.0s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/test_documents_routes.py (1)
22-23: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse shared backend fixtures for new route tests instead of bespoke patch stacks.
These new tests introduce direct
TestClient(app)usage and ad-hoc mocks for Supabase/Gemini paths, which will drift from the shared backend test contract and increase maintenance overhead. Please migrate these additions to the canonical fixtures intests/conftest.py.As per coding guidelines
backend/tests/**/*.py: Backend tests should use fixtures fromtests/conftest.pyincluding mock Supabase and mock Gemini implementations.Also applies to: 211-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_documents_routes.py` around lines 22 - 23, Replace direct TestClient(app) construction and ad-hoc Supabase/Gemini mocks in the tests in test_documents_routes.py with the shared fixtures defined in conftest.py: remove the bespoke TestClient(app) and any local patch stacks and instead accept the canonical test client and mock fixtures (e.g., client, mock_supabase, mock_gemini—or whatever the shared fixture names are in conftest.py) as test arguments; update the tests that reference TestClient(app) and the ad-hoc patches (including the block around lines 211-226) to use these fixtures so the tests reuse the centralized mock Supabase and Gemini implementations and conform to the backend test contract.
♻️ Duplicate comments (5)
backend/routes/documents.py (2)
765-769:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmit
resultonly after persistence succeeds.Line 765 emits
type="result"before_persist_document(...)on Line 776. If persistence fails, the outer fallback path on Line 818 can emit another terminal sequence for the same upload.Suggested ordering fix
- yield sapling_event_to_sse(SaplingEvent(- type="result", step="finalize",- message="Processing complete.",- data=final_output.model_dump(mode="json"),- ))- # ── Post-roll: side effects + persistence ───────────────────────── _save_orchestrator_syllabus(...) _graph_backstop(...) doc_id, _ = _persist_document(...) + yield sapling_event_to_sse(SaplingEvent(+ type="result", step="finalize",+ message="Processing complete.",+ data=final_output.model_dump(mode="json"),+ ))Also applies to: 771-779, 811-823
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 765 - 769, The code currently yields a terminal SaplingEvent(type="result", step="finalize", ...) via sapling_event_to_sse before calling _persist_document(...), which can lead to duplicate terminal events if persistence later fails; move the emission of the "result" finalize event to occur only after _persist_document returns successfully and remove any premature yields in the blocks around lines 771-779 and 811-823 so that all success terminal events are emitted exclusively after successful persistence (update the paths that call sapling_event_to_sse and SaplingEvent accordingly to guard on _persist_document success and ensure the fallback/exception paths emit their own distinct terminal events).
893-898:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t silently swallow achievement failures.
Line 897 uses
except Exception: pass, so background failures disappear without diagnostics.Suggested fix
def _check_upload_achievements(user_id: str) -> None: @@ - except Exception:- pass+ except Exception:+ logger.exception(+ "Achievement check failed after document upload for user=%s",+ user_id,+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/documents.py` around lines 893 - 898, The helper _check_upload_achievements currently swallows all exceptions; modify it to catch Exception as e and record the failure (including stack trace) instead of passing silently: wrap the call to check_achievements(user_id, "documents_uploaded", {}) in a try/except that logs the exception (for example via the existing application logger/current_app.logger or a module logger) with a clear message including user_id and the exception details; do not re-raise unless desired, but ensure the error is observable in logs for debugging.backend/tests/evals/document_summary.py (1)
69-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCheck markdown in every output field.
NoMarkdownLeakEvaluatorstill only inspectsabstract, so markdown inheadlineorkey_pointscan pass and skew the eval.♻️ Proposed fix
- text = ctx.output.abstract- if "**" in text:- return 0.0- if "```" in text:- return 0.0- if "$" in text:- return 0.0+ texts = [ctx.output.abstract, ctx.output.headline, *ctx.output.key_points]+ if any(marker in text for text in texts for marker in ("**", "```", "$")):+ return 0.0 return 1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/evals/document_summary.py` around lines 69 - 77, The evaluate method currently only inspects ctx.output.abstract for markdown markers; update it to check all output fields (ctx.output.abstract, ctx.output.headline, and each item in ctx.output.key_points) and return 0.0 if any of them contains any of the markdown/latex markers ("**", "```", "$"); implement this by building a texts list like [ctx.output.abstract, ctx.output.headline, *ctx.output.key_points] and using any(...) to test markers across all texts inside evaluate (the function signifiers: evaluate, EvaluatorContext, ctx.output.abstract, ctx.output.headline, ctx.output.key_points).backend/tests/evals/syllabus_extraction.py (2)
47-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTeach
_DATE_PATTERNSthe Spanish date form.
10 de febrero de 2026will not match the current regex set, so the Spanish syllabus case will look like it has no concrete date.♻️ Proposed fix
re.compile( r"\b\d{1,2}\s+(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Sept|Oct|Nov|Dec)" r"[a-z]*\.?\b", re.IGNORECASE, ), + re.compile(+ r"\b\d{1,2}\s+de\s+(?:enero|febrero|marzo|abril|mayo|junio|"+ r"julio|agosto|septiembre|setiembre|octubre|noviembre|diciembre)"+ r"\s+de\s+\d{4}\b",+ re.IGNORECASE,+ ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/evals/syllabus_extraction.py` around lines 47 - 64, _ADD a Spanish-date regex to the _DATE_PATTERNS list to match forms like "10 de febrero de 2026", "10 de feb 2026", "10 febrero 2026", and variants without the year; specifically add a re.compile that uses a word boundary, \d{1,2}, optional "\s+de\s+" (or just whitespace), the Spanish month names (enero, febrero, mar[ç]o, abril, mayo, junio, julio, agosto, septiembre, octubre, noviembre, diciembre and common 3-letter abbreviations) with optional accent variants, optional "\s+de\s+\d{4}" (or optional year), and a trailing word boundary, using re.IGNORECASE so the existing matching in _DATE_PATTERNS catches Spanish date phrases in syllabus text (refer to the _DATE_PATTERNS symbol to locate where to insert this new compiled regex).
90-96:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftValidate
due_dateper assignment, not per document.A single concrete date anywhere in the input can still mask a hallucinated
due_dateon a different assignment, so this check can false-pass mixed schedules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/evals/syllabus_extraction.py` around lines 90 - 96, The current evaluate method (EvaluatorContext, SyllabusAssignments, ctx.output.assignments) only checks for any concrete due_date and then calls _input_has_concrete_date(ctx.inputs), which can false-pass mixed schedules; update evaluate to validate due_date per assignment: for each assignment in ctx.output.assignments that has a non-None due_date, ensure the inputs contain a matching concrete date for that specific assignment (implement or call a helper like _input_has_concrete_date_for_assignment(ctx.inputs, assignment) or enhance _input_has_concrete_date to accept an assignment identifier), and return 1.0 only if every non-null assignment due_date is supported, otherwise 0.0; keep the vacuous pass (1.0) when no assignments have due_date.
🧹 Nitpick comments (2)
frontend/vitest.config.ts (1)
11-17: The DOM test setup is already correct.DocumentUploadModal.test.tsx—the only TSX test file in the suite—has an explicit//@vitest-environmentjsdomoverride on line 1, allowing React Testing Library tests to run properly despite the globalnodeenvironment setting.While the current approach works,
environmentMatchGlobswould be a cleaner alternative to eliminate the need for per-file environment comments, making the config self-documenting and more maintainable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/vitest.config.ts` around lines 11 - 17, Replace the global environment: 'node' approach with an environmentMatchGlobs entry so TSX tests run under jsdom automatically: add an environmentMatchGlobs mapping that assigns 'jsdom' to patterns matching your TSX tests (e.g., '*.test.tsx') and keeps 'node' (or omits explicit override) for '*.test.ts' tests; update the config object where keys like environment, include, and setupFiles are defined (look for the environment property in vitest.config.ts) to use environmentMatchGlobs instead of relying on per-file // `@vitest-environment` comments.backend/tests/evals/concept_extraction.py (1)
97-102: ⚡ Quick winPrefer
pairwise()for adjacent comparisons.Ruff is already flagging the
zip(importances, importances[1:])pattern here, anditertools.pairwise()avoids the extra slice.♻️ Proposed fix
+from itertools import pairwise+ ... - for prev, cur in zip(importances, importances[1:]):+ for prev, cur in pairwise(importances): if cur > prev: return 0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/evals/concept_extraction.py` around lines 97 - 102, In evaluate, replace the manual adjacent comparison using zip(importances, importances[1:]) with itertools.pairwise(importances): add the import (from itertools import pairwise or import itertools and use itertools.pairwise) and update the loop for prev, cur in pairwise(importances) while keeping the same comparison/return logic in the evaluate method of the EvaluatorContext/ConceptList evaluator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/documents.py`:
- Around line 339-346: The try/except around the table("documents").select (and
the other two similar blocks handling idempotency lookup/legacy insert) is too
broad; change the except Exception to catch only the "missing column" DB error:
catch the DB driver exception (e.g., psycopg2.Error or the library's DBError) as
e and test for SQLSTATE '42703' (undefined_column) or the message containing
'request_id' before falling back to the schema-less behavior; if it's not that
specific error, re-raise the exception so real persistence errors aren't
swallowed. Apply this same narrow-catch pattern to the select call that uses
table("documents").select and to the legacy insert path that currently assumes
missing request_id.
In `@backend/services/durable.py`:
- Around line 30-49: Update the DBOS enablement logic so durability only
activates when both the DBOS flag and DBOS_DATABASE_URL are present: change the
computation of _ENABLED to check os.getenv("DBOS_ENABLED") and that
os.getenv("DBOS_DATABASE_URL") is non-empty, and log a clear warning if
DBOS_ENABLED=true but DBOS_DATABASE_URL is missing; in the import block for
DBOS, narrow the handler to except ImportError when importing from dbos and let
other exceptions (e.g., DBOS initialization errors) propagate so they are not
silently degraded, while still setting _dbos_workflow/_dbos_step and _HAS_DBOS
only when the import succeeds.
In `@backend/services/logfire_scrubber.py`:
- Around line 95-101: The current string scrubber in logfire_scrubber.py returns
plaintext for short strings (value when len(value) <= _PREVIEW_CHARS) and emits
a plaintext prefix for long strings (value[:_PREVIEW_CHARS]), which leaks
sensitive content; modify the string branch that checks isinstance(value, str)
so it never returns any raw substring—both short and long strings should be
replaced with a redaction placeholder that includes only metadata (e.g., length
and the existing _fingerprint(value)), not the original characters; update the
return paths that reference _PREVIEW_CHARS and _fingerprint to produce something
like "[redacted, N chars, sha256:...]" for all strings and remove any use of
value[:_PREVIEW_CHARS].
In `@backend/tests/evals/_replay.py`:
- Around line 23-24: The code reads MODE = os.getenv("SAPLING_EVAL_MODE",
"replay").lower() but does not validate the value, so typos silently fall back
to live; update initialization to validate MODE against an explicit allowed set
(e.g., {"replay", "record", "live"}) and raise a clear exception (or call
sys.exit with an error) if the env value is not in that set; apply the same
validation logic around the related branch code referenced (the block around
lines 118-134) so both the initial MODE variable and any later usage (look for
variable/name MODE and any conditional branches that handle replay/record/live)
enforce allowed values and fail fast on unknown values.
In `@frontend/src/components/DocumentUploadModal.tsx`:
- Around line 136-137: The abort handler currently treats all aborts as
timeouts; change it to distinguish timeout-triggered aborts by adding a boolean
flag (e.g., timeoutTriggered) set to true inside the timeout callback before
calling ac.abort() (where timeout is created with setTimeout(() => {
timeoutTriggered = true; ac.abort(); }, UPLOAD_TIMEOUT_MS)); ensure
user-initiated cancels clear the timeout and call ac.abort() without setting the
flag; then, in the upload error/catch path within DocumentUploadModal (the code
that inspects the AbortError), only show the timeout message when
timeoutTriggered is true and show appropriate user-cancel behavior otherwise,
and remember to clear the timeout on success/failure to avoid leaking timers.
---
Outside diff comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 22-23: Replace direct TestClient(app) construction and ad-hoc
Supabase/Gemini mocks in the tests in test_documents_routes.py with the shared
fixtures defined in conftest.py: remove the bespoke TestClient(app) and any
local patch stacks and instead accept the canonical test client and mock
fixtures (e.g., client, mock_supabase, mock_gemini—or whatever the shared
fixture names are in conftest.py) as test arguments; update the tests that
reference TestClient(app) and the ad-hoc patches (including the block around
lines 211-226) to use these fixtures so the tests reuse the centralized mock
Supabase and Gemini implementations and conform to the backend test contract.
---
Duplicate comments:
In `@backend/routes/documents.py`:
- Around line 765-769: The code currently yields a terminal
SaplingEvent(type="result", step="finalize", ...) via sapling_event_to_sse
before calling _persist_document(...), which can lead to duplicate terminal
events if persistence later fails; move the emission of the "result" finalize
event to occur only after _persist_document returns successfully and remove any
premature yields in the blocks around lines 771-779 and 811-823 so that all
success terminal events are emitted exclusively after successful persistence
(update the paths that call sapling_event_to_sse and SaplingEvent accordingly to
guard on _persist_document success and ensure the fallback/exception paths emit
their own distinct terminal events).
- Around line 893-898: The helper _check_upload_achievements currently swallows
all exceptions; modify it to catch Exception as e and record the failure
(including stack trace) instead of passing silently: wrap the call to
check_achievements(user_id, "documents_uploaded", {}) in a try/except that logs
the exception (for example via the existing application
logger/current_app.logger or a module logger) with a clear message including
user_id and the exception details; do not re-raise unless desired, but ensure
the error is observable in logs for debugging.
In `@backend/tests/evals/document_summary.py`:
- Around line 69-77: The evaluate method currently only inspects
ctx.output.abstract for markdown markers; update it to check all output fields
(ctx.output.abstract, ctx.output.headline, and each item in
ctx.output.key_points) and return 0.0 if any of them contains any of the
markdown/latex markers ("**", "```", "$"); implement this by building a texts
list like [ctx.output.abstract, ctx.output.headline, *ctx.output.key_points] and
using any(...) to test markers across all texts inside evaluate (the function
signifiers: evaluate, EvaluatorContext, ctx.output.abstract,
ctx.output.headline, ctx.output.key_points).
In `@backend/tests/evals/syllabus_extraction.py`:
- Around line 47-64: _ADD a Spanish-date regex to the _DATE_PATTERNS list to
match forms like "10 de febrero de 2026", "10 de feb 2026", "10 febrero 2026",
and variants without the year; specifically add a re.compile that uses a word
boundary, \d{1,2}, optional "\s+de\s+" (or just whitespace), the Spanish month
names (enero, febrero, mar[ç]o, abril, mayo, junio, julio, agosto, septiembre,
octubre, noviembre, diciembre and common 3-letter abbreviations) with optional
accent variants, optional "\s+de\s+\d{4}" (or optional year), and a trailing
word boundary, using re.IGNORECASE so the existing matching in _DATE_PATTERNS
catches Spanish date phrases in syllabus text (refer to the _DATE_PATTERNS
symbol to locate where to insert this new compiled regex).
- Around line 90-96: The current evaluate method (EvaluatorContext,
SyllabusAssignments, ctx.output.assignments) only checks for any concrete
due_date and then calls _input_has_concrete_date(ctx.inputs), which can
false-pass mixed schedules; update evaluate to validate due_date per assignment:
for each assignment in ctx.output.assignments that has a non-None due_date,
ensure the inputs contain a matching concrete date for that specific assignment
(implement or call a helper like
_input_has_concrete_date_for_assignment(ctx.inputs, assignment) or enhance
_input_has_concrete_date to accept an assignment identifier), and return 1.0
only if every non-null assignment due_date is supported, otherwise 0.0; keep the
vacuous pass (1.0) when no assignments have due_date.
---
Nitpick comments:
In `@backend/tests/evals/concept_extraction.py`:
- Around line 97-102: In evaluate, replace the manual adjacent comparison using
zip(importances, importances[1:]) with itertools.pairwise(importances): add the
import (from itertools import pairwise or import itertools and use
itertools.pairwise) and update the loop for prev, cur in pairwise(importances)
while keeping the same comparison/return logic in the evaluate method of the
EvaluatorContext/ConceptList evaluator.
In `@frontend/vitest.config.ts`:
- Around line 11-17: Replace the global environment: 'node' approach with an
environmentMatchGlobs entry so TSX tests run under jsdom automatically: add an
environmentMatchGlobs mapping that assigns 'jsdom' to patterns matching your TSX
tests (e.g., '*.test.tsx') and keeps 'node' (or omits explicit override) for
'*.test.ts' tests; update the config object where keys like environment,
include, and setupFiles are defined (look for the environment property in
vitest.config.ts) to use environmentMatchGlobs instead of relying on per-file //
`@vitest-environment` comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f0e32382-4174-4add-b8bc-7f2328e8105a
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.github/workflows/evals.ymlbackend/agents/classifier.pybackend/agents/concept_extraction.pybackend/agents/document.pybackend/agents/summary.pybackend/agents/syllabus_extraction.pybackend/db/migration_documents_request_id.sqlbackend/main.pybackend/requirements.txtbackend/routes/documents.pybackend/services/durable.pybackend/services/logfire_scrubber.pybackend/tests/evals/__init__.pybackend/tests/evals/_replay.pybackend/tests/evals/cassettes/.gitkeepbackend/tests/evals/cassettes/concept_extraction/long_lecture_neural_networks.jsonbackend/tests/evals/cassettes/document_classification/typical_university_syllabus.jsonbackend/tests/evals/cassettes/document_summary/short_syllabus_excerpt.jsonbackend/tests/evals/cassettes/syllabus_extraction/typical_syllabus_with_dates.jsonbackend/tests/evals/concept_extraction.pybackend/tests/evals/document_classification.pybackend/tests/evals/document_summary.pybackend/tests/evals/syllabus_extraction.pybackend/tests/test_documents_routes.pybackend/tests/test_logfire_scrubber.pydocs/decisions/0010-ocr-async-two-phase-upload.mddocs/decisions/0011-durable-execution-dbos.mdfrontend/package.jsonfrontend/src/components/DocumentUploadModal.test.tsxfrontend/src/components/DocumentUploadModal.tsxfrontend/src/lib/api.test.tsfrontend/src/lib/api.tsfrontend/vitest.config.tsfrontend/vitest.setup.ts
✅ Files skipped from review due to trivial changes (5)
- backend/tests/evals/cassettes/document_summary/short_syllabus_excerpt.json
- frontend/vitest.setup.ts
- backend/db/migration_documents_request_id.sql
- backend/tests/evals/init.py
- backend/tests/evals/cassettes/syllabus_extraction/typical_syllabus_with_dates.json
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/agents/syllabus_extraction.py
- backend/requirements.txt
- backend/agents/summary.py
- backend/agents/concept_extraction.py
- backend/agents/document.py
- backend/tests/evals/document_classification.py
- frontend/src/lib/api.ts
| try: | ||
| rows = table("documents").select( | ||
| "id,user_id,course_id,file_name,category,summary,concept_notes,created_at,processed_at", | ||
| filters={"user_id": f"eq.{user_id}", "request_id": f"eq.{request_id}"}, | ||
| limit=1, | ||
| ) | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
Narrow the request_id schema fallback to only missing-column errors.
On Line 345, Line 401, and Line 981, broad except Exception paths treat any DB failure as “schema missing request_id” and proceed without idempotency metadata. That can mask real persistence errors and create duplicate processing/doc rows.
Suggested hardening
def _existing_doc_by_request_id(user_id: str, request_id: str) -> dict | None:
@@
- except Exception:- return None+ except Exception as err:+ msg = str(err).lower()+ if "request_id" in msg and ("column" in msg or "schema cache" in msg):+ return None+ raise
@@
def _persist_document(...):
@@
- except Exception:+ except Exception as err:
# Schema may not yet have the request_id column; retry without it
# so deployments can ship the code before the migration runs.
- if "request_id" in row:+ msg = str(err).lower()+ missing_request_id_col = "request_id" in msg and ("column" in msg or "schema cache" in msg)+ if "request_id" in row and missing_request_id_col:
row.pop("request_id", None)
inserted = table("documents").insert(row)
else:
raiseApply the same conditional pattern to the Line 981 legacy insert path.
Also applies to: 399-408, 979-988
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 345-345: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/documents.py` around lines 339 - 346, The try/except around
the table("documents").select (and the other two similar blocks handling
idempotency lookup/legacy insert) is too broad; change the except Exception to
catch only the "missing column" DB error: catch the DB driver exception (e.g.,
psycopg2.Error or the library's DBError) as e and test for SQLSTATE '42703'
(undefined_column) or the message containing 'request_id' before falling back to
the schema-less behavior; if it's not that specific error, re-raise the
exception so real persistence errors aren't swallowed. Apply this same
narrow-catch pattern to the select call that uses table("documents").select and
to the legacy insert path that currently assumes missing request_id.
| _ENABLED = os.getenv("DBOS_ENABLED", "false").lower() == "true" | ||
| _HAS_DBOS = False | ||
| _dbos_workflow = None | ||
| _dbos_step = None | ||
| if _ENABLED: | ||
| try: | ||
| from dbos import DBOS # type: ignore[import-not-found] | ||
| # DBOS init must be done by the application entrypoint; we just | ||
| # capture the decorators here and trust that DBOS() was called | ||
| # in main.py BEFORE any decorated function is invoked. | ||
| _dbos_workflow = DBOS.workflow | ||
| _dbos_step = DBOS.step | ||
| _HAS_DBOS = True | ||
| except Exception as e: # ImportError or DBOS init failure | ||
| logger.warning( | ||
| "DBOS_ENABLED=true but DBOS could not be loaded (%s). " | ||
| "Durable decorators will degrade to no-ops.", | ||
| e, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Verify whether DBOS preconditions are enforced before setting _HAS_DBOS=True
rg -n "DBOS_ENABLED|DBOS_DATABASE_URL|_HAS_DBOS|from dbos|DBOS\." backend/services/durable.py backend/main.py backend/agents/document.pyRepository: SaplingLearn/Sapling
Length of output: 1095
Durability can silently degrade when DBOS_DATABASE_URL is missing despite DBOS_ENABLED=true.
The module docstring at line 3–4 documents that durable features require both DBOS_ENABLED=true AND DBOS_DATABASE_URL to be set. However, line 30 checks only the flag, not the database URL, allowing _HAS_DBOS to be set True with incomplete configuration. Additionally, lines 44–49 use a broad except Exception that silently downgrades durability to no-ops on any import or initialization failure, masking configuration errors.
Consider narrowing exception handling to only ImportError (expected when the dbos package is unavailable) while re-raising unexpected failures, and enforce both preconditions before enabling durable decorators:
Suggested approach
- Check both
DBOS_ENABLEDflag andDBOS_DATABASE_URLpresence before setting_ENABLED = True - Change
except Exceptiontoexcept ImportErrorto allow configuration/initialization errors to surface - Add explicit logging when the flag is set but the URL is missing
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 44-44: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/durable.py` around lines 30 - 49, Update the DBOS enablement
logic so durability only activates when both the DBOS flag and DBOS_DATABASE_URL
are present: change the computation of _ENABLED to check
os.getenv("DBOS_ENABLED") and that os.getenv("DBOS_DATABASE_URL") is non-empty,
and log a clear warning if DBOS_ENABLED=true but DBOS_DATABASE_URL is missing;
in the import block for DBOS, narrow the handler to except ImportError when
importing from dbos and let other exceptions (e.g., DBOS initialization errors)
propagate so they are not silently degraded, while still setting
_dbos_workflow/_dbos_step and _HAS_DBOS only when the import succeeds.
| if isinstance(value, str): | ||
| if len(value) <= _PREVIEW_CHARS: | ||
| return value | ||
| return ( | ||
| f"{value[:_PREVIEW_CHARS]}…[redacted, {len(value)} chars, " | ||
| f"sha256:{_fingerprint(value)}]" | ||
| ) |
There was a problem hiding this comment.
Scrubber still emits plaintext user content.
Line 97 returns short risky strings unchanged, and Lines 99–100 emit an 80-char plaintext prefix for long ones. That still leaks prompt/output text off-process.
Suggested redaction behavior
def _sanitize(value: Any, path: tuple[Any, ...] | str) -> Any:
"""Truncate strings, recurse into lists/dicts."""
if isinstance(value, str):
- if len(value) <= _PREVIEW_CHARS:- return value- return (- f"{value[:_PREVIEW_CHARS]}…[redacted, {len(value)} chars, "- f"sha256:{_fingerprint(value)}]"- )+ return f"[redacted, {len(value)} chars, sha256:{_fingerprint(value)}]"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ifisinstance(value, str): | |
| iflen(value) <=_PREVIEW_CHARS: | |
| returnvalue | |
| return ( | |
| f"{value[:_PREVIEW_CHARS]}…[redacted, {len(value)} chars, " | |
| f"sha256:{_fingerprint(value)}]" | |
| ) | |
| ifisinstance(value, str): | |
| returnf"[redacted, {len(value)} chars, sha256:{_fingerprint(value)}]" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/logfire_scrubber.py` around lines 95 - 101, The current
string scrubber in logfire_scrubber.py returns plaintext for short strings
(value when len(value) <= _PREVIEW_CHARS) and emits a plaintext prefix for long
strings (value[:_PREVIEW_CHARS]), which leaks sensitive content; modify the
string branch that checks isinstance(value, str) so it never returns any raw
substring—both short and long strings should be replaced with a redaction
placeholder that includes only metadata (e.g., length and the existing
_fingerprint(value)), not the original characters; update the return paths that
reference _PREVIEW_CHARS and _fingerprint to produce something like "[redacted,
N chars, sha256:...]" for all strings and remove any use of
value[:_PREVIEW_CHARS].
| MODE = os.getenv("SAPLING_EVAL_MODE", "replay").lower() | ||
There was a problem hiding this comment.
Fail fast on unknown SAPLING_EVAL_MODE values.
Right now a typo in the env var silently falls through to the live path, which can unexpectedly hit Gemini instead of failing the eval fast.
🔧 Proposed fix
MODE = os.getenv("SAPLING_EVAL_MODE", "replay").lower()
+if MODE not in {"replay", "record", "live"}:+ raise ValueError(f"Unsupported SAPLING_EVAL_MODE: {MODE!r}")Also applies to: 118-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/evals/_replay.py` around lines 23 - 24, The code reads MODE =
os.getenv("SAPLING_EVAL_MODE", "replay").lower() but does not validate the
value, so typos silently fall back to live; update initialization to validate
MODE against an explicit allowed set (e.g., {"replay", "record", "live"}) and
raise a clear exception (or call sys.exit with an error) if the env value is not
in that set; apply the same validation logic around the related branch code
referenced (the block around lines 118-134) so both the initial MODE variable
and any later usage (look for variable/name MODE and any conditional branches
that handle replay/record/live) enforce allowed values and fail fast on unknown
values.
| const timeout = setTimeout(() => ac.abort(), UPLOAD_TIMEOUT_MS); | ||
| setItems(prev => prev.map(i => i.id === item.id ? { ...i, status: "uploading", abort: ac } : i)); | ||
| // Mint a fresh request_id per attempt so retries don't collide with the |
There was a problem hiding this comment.
Differentiate timeout aborts from user-cancel aborts.
Line 193 currently shows the timeout message for any abort, including user-initiated cancels (e.g., closing modal/removing item), which is misleading.
Suggested fix
- const timeout = setTimeout(() => ac.abort(), UPLOAD_TIMEOUT_MS);+ let timedOut = false;+ const timeout = setTimeout(() => {+ timedOut = true;+ ac.abort();+ }, UPLOAD_TIMEOUT_MS);
@@
- const errorMsg = aborted- ? "Processing took longer than 4 minutes — try a smaller file."+ const errorMsg = aborted+ ? (timedOut+ ? "Processing took longer than 4 minutes — try a smaller file."+ : "Upload canceled.")
: String(err?.message || err);Also applies to: 193-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/DocumentUploadModal.tsx` around lines 136 - 137, The
abort handler currently treats all aborts as timeouts; change it to distinguish
timeout-triggered aborts by adding a boolean flag (e.g., timeoutTriggered) set
to true inside the timeout callback before calling ac.abort() (where timeout is
created with setTimeout(() => { timeoutTriggered = true; ac.abort(); },
UPLOAD_TIMEOUT_MS)); ensure user-initiated cancels clear the timeout and call
ac.abort() without setting the flag; then, in the upload error/catch path within
DocumentUploadModal (the code that inspects the AbortError), only show the
timeout message when timeoutTriggered is true and show appropriate user-cancel
behavior otherwise, and remember to clear the timeout on success/failure to
avoid leaking timers.
Pulls 8 commits from main (auth/cookie fixes, calendar fix, RequestLogMiddleware, /api/users decryption fix). Two real conflict points required reconciliation; everything else auto-merged cleanly. backend/main.py — middleware consolidation - Main added RequestLogMiddleware (8-char rid, duration logging, inline 500 with traceback). Branch had RequestIDMiddleware (caller-supplied IDs accepted, contextvar, three structured exception handlers, no traceback in body). - Resolution: keep RequestIDMiddleware as the single middleware, absorb RequestLogMiddleware's duration-logging behavior into it. Both used to write to request.state.request_id and the response X-Request-ID header — running both would have made the second silently overwrite the first. - Dropped: RequestLogMiddleware class, app.add_middleware( RequestLogMiddleware), the import of BaseHTTPMiddleware in main.py, and the unused time/traceback/uuid imports. - Kept: logging.basicConfig() so every logger inherits the app-wide format/level. Per-request log lines now come from RequestIDMiddleware via the "sapling.request" logger. - Also adopted main's /api/users decryption fix verbatim (real bug: the endpoint was returning ciphertext for user names). backend/services/request_context.py — duration logging - RequestIDMiddleware now records start = time.perf_counter() and emits one logger.log(level, ...) line per request at completion, with severity tracking the response status (>=500 ERROR, >=400 WARNING, else INFO). Format matches what RequestLogMiddleware produced. - contextvar + caller-supplied-ID validation behavior unchanged. frontend/* — auto-merged - src/lib/api.ts: both branches independently arrived at `export const API_URL` + `credentials: 'include'` in fetchJSON (main's intent was the same as branch's). Auto-merge kept both the SSE additions (uploadDocumentStream, UploadEvent) AND main's auth shape. - Other auth-related files (SignInModal, UserContext, session/route, callback/page, sessionToken, wrangler.toml) auto-merged: branch hadn't touched them, so main's auth-fix series landed cleanly. - routes/calendar.py: main's course_code/course_name select fix landed cleanly — branch hadn't touched calendar. Tests - Backend: 427/430 pass (425 + 2 unchanged from b865de1; the 3 pre-existing live-Supabase failures unchanged). - Frontend: typecheck clean. vitest 14/14. PR description should still note that the documents.request_id migration must be applied on staging/prod before the new code's idempotency dedupe takes effect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug surfaced by the merge with origin/main: three direct fetch() calls in api.ts targeted auth-protected endpoints but lacked credentials: 'include'. After main's cross-origin cookie work (SameSite=None; Secure + COOKIE_DOMAIN=.saplinglearn.com), browsers only attach the session cookie when the fetch explicitly opts in. The branch wrote those fetches in commits ccd5345 and earlier — before main's auth refactor — so they never got the opt-in. fetchJSON and uploadDocumentStream already had it; everything else didn't. Affected endpoints (all require_self / require_admin protected): - POST /api/documents/upload/sync (uploadDocument) - POST /api/calendar/extract (extractSyllabus) - POST /api/profile/<id>/avatar (uploadAvatar) POST /api/careers/apply (job application form) is intentionally unauthenticated and stays as-is. Tests - New `credentials: include on auth-protected multipart uploads` block in api.test.ts pins the contract: each of the three uploaders must pass credentials:'include'. Future direct-fetch additions to auth-protected endpoints will fail this test if they drop the attribute. - Also tightened the existing uploadDocumentStream test with an explicit `credentials: 'include'` assertion. - vitest 18/18 (was 14 + 4 new). Typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cloudflare's build runs `npm clean-install --progress=false` with npm 10.9.2 / Node 22.16.0. Local dev had npm 11.6.2 / Node 24, and the lockfile npm 11 produces lays out some transitive entries (emnapi, esbuild peer ranges) in a shape npm 10's strict mode rejects with `Missing: <pkg> from lock file`. Reproduced locally and fixed: $ rm -rf node_modules $ npx -y -p npm@10.9.2 npm install # 91 insertions, 27 deletions in package-lock.json $ rm -rf node_modules $ npx -y -p npm@10.9.2 npm clean-install --progress=false added 1029 packages, exit 0 Also adds frontend/.nvmrc=22 so future contributors and any CI that respects nvmrc default to a Node version with bundled npm 10.x. This is the same Node version Cloudflare Pages picks from environment. No package.json version changes. Frontend tests + typecheck unchanged (18/18 pass, typecheck clean). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
1. All-drift cascade test (TestQuizAgentFallback) New `test_falls_back_to_legacy_when_all_questions_drift` pins the path the 3 contract tests don't cover directly: agent returns a schema-valid Quiz where every question's correct_answer doesn't appear in its options → _quiz_via_agent's wire-format filter drops all of them → raises RuntimeError → bare-Exception catch in generate_quiz routes to _legacy_generate_quiz. Asserts the legacy gemini path actually runs and the legacy fallback question is what reaches the client. 2. Drift warning no longer leaks student content to local logs _agent_question_to_wire's drift warning was using %r to dump the raw correct_answer, options, and concept text. Logfire's egress scrubber (PR #67) handled remote ingestion, but Railway's local stdout still saw the unredacted strings. Now we log: n_options=4, canonical_len=18, fp=<sha256[:12]> The fingerprint is stable across recurrences of the same drift, so we still get correlation; the actual content stays out of stdout. Hashlib import hoisted to module scope. Pre-existing transient: tests/test_ocr_pipeline.py::test_gemini_parse that flickered red in the previous review run cleared on re-run (skipped in isolation, passing in full suite). Confirmed transient live-Gemini hiccup, not caused by this branch. Tests - tests/test_quiz_routes.py: 23/23 (the previous "24" was a miscount; net +1 from the new cascade test). - Full backend suite: 443 passed, 3 pre-existing live-Supabase failures unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cloudflare Workers Builds runs `npm clean-install` with npm 10.9.2. That hit EUSAGE on every build of PR #92: npm error Missing: @emnapi/runtime@1.10.0 from lock file npm error Missing: @emnapi/core@1.10.0 from lock file npm error Missing: esbuild@0.28.0 from lock file Cause: when react-force-graph-3d + three were installed locally, the generating npm version produced a lockfile that omits a few transitive deps that npm 10.9.2's strict `npm ci` requires. Same class of issue PR #67 hit during the docs-readme refresh. Fix: regenerated package-lock.json with `npx -p npm@10.9.2 npm install` so the lockfile matches what Cloudflare's runner expects. Then verified `npm ci` succeeds against the new lockfile (1061 packages, no errors). Local pipeline still clean against the new lockfile: - tsc --noEmit -> clean - vitest -> 36 passed - next build -> all 17 routes succeed - opennextjs-cloudflare build -> Worker saved The build-runtime config (transpilePackages, wrangler nodejs_compat, no engines.npm pin) is otherwise unchanged. The CF failure was purely lockfile-skew between npm versions, not a bundling or runtime issue. Future installs by anyone with npm >=11 should still work because the lockfile is npm-version-tolerant — only `npm ci` strict mode demanded the missing transitives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Description
This PR re-architects the backend around three independent but related workstreams that ship together to keep the merge surface small. The result is a typed, observable, partially-streamed document-upload pipeline; encryption-at-rest for every column that holds PII or generated content; and a markdown-based dev-context vault that lets future Claude Code sessions onboard in seconds instead of relearning the codebase every time.
Why now: the procedural
_process_documentGemini call had grown a per-route output parser, no retries, and no progress signal — every new feature copied the seam. Encryption was overdue once we started persisting Gemini-generated summaries and chat history. The vault is the cheapest tool to keep the next several refactors coherent across sessions.Scope: 80 files changed (+5,373 / −923) across backend agents, encryption rollout, auth hardening, frontend marketing/UX touch-ups, and documentation. No frontend SSE consumer for the new
/uploadroute yet — that's tracked as follow-up; the existing/upload/syncroute preserves the legacy JSON contract for callers that haven't migrated.Changes Made
Agentic refactor (Pydantic AI) — new
backend/agents/layeragents/__init__.py— exportsWORKER_LIMITS(request_limit=2, no tool calls, 50k tokens) andORCHESTRATOR_LIMITS(8 requests, 10 tool calls, 100k tokens). Passed per-.run()call, not on the agent constructor (per ADR 0003).agents/deps.py—SaplingDepsdataclass:user_id,course_id,supabase,request_id. Threaded through every agent run; accessible inside tools viaRunContext[SaplingDeps].agents/classifier.py— typedDocumentClassificationoutput (category enum +is_syllabusbool).agents/summary.py— typedSummaryoutput (abstractfield).agents/concept_extraction.py— typedConceptList(list ofConceptwithname+description).agents/syllabus_extraction.py— typedSyllabusAssignmentswith structureddue_date, no-invent contract.agents/document.py— orchestrator. Classifier as serial gate, thenasyncio.gather(summary, concepts, syllabus?)in parallel, then a graph-update tool call. Output type is intentionally minimal (GraphUpdateConfirmation); the route composes the fullDocumentProcessingResultdeterministically because Gemini rejects rich schemas (logged indocs/attempts/2026-05-03-orchestrator-schema-complexity.md).agents/tools/graph.py—apply_graph_update_toolwrapsservices/graph_service.py::apply_graph_update. Usesasyncio.to_threadso the sync DB call doesn't block the event loop.services/agent_events.py—SaplingEventshape (status / progress / result / error) +map_to_sapling_event(event)mapper from Pydantic AI's typed event union.routes/documents.py— adds streamingPOST /api/documents/upload(EventSourceResponse+agent.run_stream_events()) and renames the original toPOST /api/documents/upload/sync(non-streaming JSON, also orchestrator-backed). Preserves_legacy_upload_pipelineas the fallback target onUsageLimitExceeded,UnexpectedModelBehavior, or any other agent exception. Post-roll work usesasyncio.create_task(notBackgroundTasks) for the streaming route since the stream IS the response.tests/evals/document_classification.py— 10-casepydantic-evalsset covering 4 syllabus variants, 4 non-syllabus, and 2 ambiguous documents.main.py—logfire.instrument_pydantic_ai()andlogfire.instrument_fastapi(app)for free OTel traces.requirements.txt— addspydantic-ai-slim[google]>=0.0.20,logfire>=2.0,pydantic-evals,sse-starlette.Column-level encryption (AES-256-GCM)
services/encryption.py— encryption module:encrypt_if_present,encrypt_json,decrypt_if_present,decrypt_numeric,decrypt_json. ReadsENCRYPTION_KEY(32 bytes hex) from env.tests/test_encryption.py— round-trip + fallback tests.db/migration_encryption_text_columns.sql— retypes encrypted columns to TEXT so AES-256-GCM ciphertext (base64) fits.db/backfill_encryption.py— one-shot script that walks rows and encrypts existing plaintext.services/auth_guard.py— encrypts/decrypts session-derived PII; addsrequire_self/require_adminguards used by sensitive routes.services/gemini_service.py— addsMODEL_DEFAULT/MODEL_LITEconstants andmodel=kwarg threading; quiz + concept_suggestions routed togemini-2.5-flash-lite.routes/auth.py— user PII (name,first_name,last_name) + Google OAuth tokens.routes/profile.py—bio,location; decrypts on/meand public profile reads.routes/onboarding.py— name fields on profile save.routes/admin.py— decrypts user PII for/admin/users.routes/social.py—messages.content,room_messages.text; decrypts user names on room/match/student reads.routes/calendar.py— calendar OAuth tokens, assignment notes.routes/gradebook.py— assignmentnotes+points.routes/documents.py— documentsummary+concept_notes(both at the new orchestrator path AND legacy fallback).routes/learn.py— decrypts student name + document summaries/concept notes for tutor prompts before injection.routes/quiz.py— decrypts student name before injecting into quiz prompts.routes/study_guide.py— decrypts document summaries/concept notes before prompt build.routes/flashcards.py— decrypts document content before card generation.routes/graph.py— preserves graph-touching write paths under encryption.requirements.txt— addscryptography>=42,<46.docker-compose.yml+.env.example— surfaceENCRYPTION_KEY.Dev-context vault for Claude Code
CLAUDE.md— slimmed to ≤ 200 lines (per ADR 0002): project map withfile:linepointers, commands, gotchas (now includes the column-encryption operational note). Pointers todocs/decisions/,docs/attempts/,docs/architecture.md, and/sync-context.docs/architecture.md— current-state architecture overview (37 lines).docs/README.md— vault layout + append-only conventions.docs/decisions/— five accepted ADRs:0001-adopt-pydantic-ai.md— framework choice and migration plan.0002-vault-structure.md— markdown-based vault with slash commands + curator subagent (rejected MCP knowledge server alternative).0003-implementation-conventions.md— bundles four conventions: inline system prompts, per-callusage_limits=,asyncio.create_taskfor SSE post-roll, small orchestrator output schemas.0004-graph-service-tool-surface.md— graph_service is the next agent-tool migration target (read_concepts_for_user,read_misconceptions_for_course).0005-refactor-2-quiz-generation.md— refactor Refine LLM Model selection for each function #2 isroutes/quiz.py::generate_quiz; defer chat tutor (Fix the learning loop for the context #3) and syllabus dedup (Add landing page with liquid glass effects #4).docs/attempts/— three honest "what didn't work" entries with mandatory "What I'd try next":2026-05-03-mcp-knowledge-server-trial.md2026-05-03-orchestrator-schema-complexity.md2026-05-03-vault-gap-prompts-13-14.mddocs/superpowers/plans/2026-05-03-aes-256-gcm-column-encryption.md— encryption rollout plan..claude/commands/— four slash commands:/log-decision,/log-attempt,/recall,/sync-context..claude/agents/context-curator.md— read-only subagent that loads ≤ 2k tokens of vault context for fresh sessions..mcp.json— MCP server config for Claude Code.Frontend / marketing / misc
frontend/src/middleware.ts,app/api/auth/session/route.ts,app/auth/callback/page.tsx— auth flow now fetches/meto hydrate name + avatar (post-encryption, the JWT no longer carries plaintext).frontend/src/components/screens/Learn.tsx,Tree.tsx,ChatPanel.tsx,MarkdownChat.tsx,KnowledgeGraph.tsx— graph color/mastery refactors, breadcrumb, progress + related cards, instant chat open, snappier typing.frontend/src/app/about|privacy|terms/page.tsx— widened marketing pages, careers-style nav, updated legal copy.frontend/src/lib/api.ts— drops 6 lines of dead code.landingpage.png— refreshed screenshot.README.md— updated project title and image.Merge resolution (commit
fddc8c9)CLAUDE.md— kept lean structure; added Gotchas pointer for column encryption.backend/routes/documents.py— combined imports; both upload routes now runrequire_self(user_id, request)before_validate_user;_persist_documentencryptssummary+concept_notesat the insert boundary and returns plaintext to callers, mirroring_legacy_upload_pipeline.backend/.env.example— kept origin's version (local deletion was unintentional).Related Issues
Closes #
Testing
cd backend && python -m pytest tests/ -q./api/documents/upload(SSE): upload a syllabus, confirm progress events fire and the persisted row decrypts cleanly on read./api/documents/upload/sync: same payload, JSON response, plaintextsummary/concept_notesreturned to client.WORKER_LIMITS.request_limit=0) and confirm_legacy_upload_pipelinefallback fires and persists with encryption applied.ENCRYPTION_KEYis set in all environments (dev, staging, prod) before merging.backend/db/backfill_encryption.py) on staging before promoting to prod, perdocs/superpowers/plans/2026-05-03-aes-256-gcm-column-encryption.md.LOGFIRE_TOKEN) for production traces; otherwise local-only viasend_to_logfire="if-token-present".Screenshots (if applicable)
N/A — no new visual surfaces. Marketing page widening is style-only.
Notes for Reviewers
POST /api/documents/uploadworks at the wire level (verifiable viacurl -N), but no React component consumes it yet. Existing upload flows continue to usePOST /api/documents/upload/sync(orchestrator-backed, JSON response). Tracked as follow-up._legacy_upload_pipeline) stays alive until refactor Fix the learning loop for the context #3 ships per ADR 0001. Do not remove it as part of this PR.decrypt_if_present/decrypt_json/decrypt_numericbefore consumption (especially before AI prompt injection). New routes touching encrypted columns must wire this in or they'll silently emit ciphertext.quiz_agentis next week./sync-contextonly reads the 3 most-recent ADRs. Foundational ADRs 0001 and 0002 fall out of that window now that 0003-0005 exist; flagged as a known limitation in ADR 0003 /docs/attempts/2026-05-03-vault-gap-prompts-13-14.md. Future iteration of/sync-contextshould pin foundational ADRs.migration_encryption_text_columns.sqlandbackfill_encryption.pyneed to be executed on each environment before that environment switches to encrypted reads.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores