feat(documents): file-level dedup — skip OCR, agents, and re-indexing on duplicate uploads

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}

[Darkest-Teddy]

Why

Sapling's RAG corpus is shared per course, so the same lecture deck arrives from many students under many different filenames. rag_service.chunk_id already collapses identical passages to one row — but only at the end of the pipeline. By then OCR, the agent pipeline, and the embedding batch have all been paid for, and the duplicate chunks are simply upserted onto rows that already exist.

ADR 0019 names embedding spend as the main win of content-addressed ids. As written the code never delivered it: nothing checks for an existing chunk before embedding.

This catches the duplicate at the door, keyed on sha256 of the raw uploaded bytes. The fingerprint covers file contents only, never the filename, so lec3.pdf and Lecture 3 Slides.pdf are recognised as the same upload.

What a duplicate upload now costs

Step	Before	After
OCR	full run	skipped
Classifier / summary / concepts / syllabus	3–4 LLM calls	skipped
Chunking + embedding	full batch	skipped (same course)
Calendar, graph, achievements	ran	still run, per-student
documents row	created	created

Design notes

Each uploader still gets their own documents row. The dedup targets the shared, expensive layer — OCR and embeddings — not the personal library. A student whose upload silently vanished would read that as a bug.

Two scopes, for different reasons. Text and agent-result reuse are global: extraction and the agents are pure functions of the bytes (static system prompts, no user context), so a twin from any course is valid. The indexing skip is course-scoped: chunk ids hash the course code, so the same file uploaded to a different course genuinely needs its own embeddings. Skipping there would leave that course with no retrievable material.

Why the whole result is persisted rather than a few columns. Rebuilding a DocumentProcessingResult from the row is impossible without inventing data: Summary.headline isn't stored, Summary.key_points isn't stored and requires at least 3 entries, Concept.importance isn't stored, and syllabus.assignments — the calendar import's only source — isn't stored anywhere. One encrypted JSON column round-trips losslessly (verified including date-typed due_dates, ~430 bytes) and covers syllabus assignments and grading categories for free.

Syllabus uploads are safe to short-circuit. The calendar write rides on the replayed result and takes the uploader's user_id, so the second student to upload a syllabus gets their own assignments. All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph — they just no longer need a fresh agent run to feed them. A test asserts save_assignments_to_db is called with the new user id.

Degrades rather than fails.decode_result treats a missing payload and one that no longer validates identically: run the agents. Model drift falls back to today's behaviour instead of failing an upload. Both new columns are nullable, and find_duplicate returns "no duplicate" if the columns are absent, so the code can ship ahead of the migrations.

Columns are non-unique by design — the same file legitimately recurs once per uploader and once per course.

Changes

File
services/document_dedup.py	new — file_sha256, find_duplicate, decode_result, chunks_already_exist
db/migrations/0043_documents_file_sha256.sql	new — nullable column + partial index
db/migrations/0044_documents_agent_result.sql	new — nullable encrypted JSON column
routes/documents.py	both upload routes wired; workers extracted to _run_document_workers
tests/test_document_dedup.py	new — 16 tests
tests/test_documents_routes.py	8 route tests incl. the streaming path

The streaming route's parallel workers moved into _run_document_workers so the replay path can bypass them wholesale. The client-visible SSE event sequence is unchanged — a test pins the exact nine-step sequence — so a replayed upload is indistinguishable to the frontend apart from latency.

Verification

• Full backend suite: 1555 passed, 32 skipped, 0 failures
• ruff check services/ routes/ tests/ clean
• Both migrations applied and verified against a local Supabase PG15 instance
• Migration-order pins green after renumbering

Note for reviewers

0043/0044 were originally 0042/0043; main landed 0042_assignments_source_gradescope.sql while this branch was open. Renumbered in fe2f685 — no SQL changed. Anyone who applied the earlier numbering locally has stale 0042 rows in their ledger; the columns are identical, so re-running db.migrate against a fresh database is the clean path.

Not included

Near-duplicate detection (same material, different bytes — a re-export or re-scan) is out of scope. Neither a file hash nor a chunk hash catches it; that needs MinHash/SimHash or content-defined chunking, and belongs with the chunking overhaul that would change chunk boundaries anyway.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Duplicate document uploads are detected automatically using file content.
  • Previously extracted text and available processing results can be reused, reducing unnecessary processing.
  • Streaming uploads now replay reusable results while avoiding duplicate course indexing.
  • Existing documents without deduplication data continue to process normally.

[supabase]

This pull request has been ignored for the connected project ybgqdonkoqftwrmweuyv because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, you've reached your PR review limit, so we couldn't start this review.

Next review available in:8 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ea8f2b7-51b8-4446-9027-00a8266ec647

📥 Commits

Reviewing files that changed from the base of the PR and between f64ff2c and 79f726b.

📒 Files selected for processing (10)

• backend/db/migrations/20260802012500_documents_file_sha256.sql
• backend/db/migrations/20260802012600_documents_agent_result.sql
• backend/e2e_oracles/gather.py
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/services/events_service.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py
• backend/tests/test_e2e_oracles_cli.py
• backend/tests/test_event_capture_seams.py

📝 Walkthrough

Walkthrough

This change adds SHA-256 document deduplication for synchronous and streaming uploads. It persists extracted text and encrypted pipeline results, replays reusable results, preserves per-user effects, and skips duplicate course chunk indexing.

Changes

Document deduplication

Layer / File(s)	Summary
Persistence contract backend/db/migrations/*, backend/routes/documents.py	Adds nullable document fingerprint and agent-result fields. Persistence stores extracted text, hashes, and encrypted results with compatibility fallback.
Duplicate lookup and result validation backend/services/document_dedup.py, backend/tests/test_document_dedup.py	Adds SHA-256 hashing, duplicate lookup, encrypted field decoding, stored-result validation, and course-scoped chunk reuse checks.
Synchronous upload deduplication backend/routes/documents.py, backend/tests/test_documents_routes.py	Reuses extracted text and stored pipeline results for duplicate uploads. New and legacy documents continue through agent processing.
Streaming upload replay backend/routes/documents.py, backend/tests/test_documents_routes.py	Replays duplicate results during streaming uploads, preserves SSE events, and skips chunk indexing when same-course chunks already exist.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
participant UploadEndpoint
participant document_dedup
participant DocumentsDB
participant AgentWorkers
participant SSEClient
UploadEndpoint->>document_dedup: compute file_sha256
document_dedup->>DocumentsDB: find_duplicate
DocumentsDB-->>document_dedup: reusable document and stored result
document_dedup-->>UploadEndpoint: extracted text and replay data
UploadEndpoint->>AgentWorkers: invoke agents when replay data is unavailable
AgentWorkers-->>UploadEndpoint: processing outputs
UploadEndpoint-->>SSEClient: preserve streaming event sequence

Loading

Possibly related PRs

• SaplingLearn/Sapling#67: Both changes modify document upload handling in backend/routes/documents.py; this PR adds content deduplication, while PR #67 adds request-ID idempotency.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes file-level deduplication and the processing steps skipped for duplicate uploads.
Description check	✅ Passed	The description thoroughly covers the rationale, implementation, testing, migration notes, reviewer guidance, and scope of the changes.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/file-level-dedup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	frontend-staging	79f726b	Commit Preview URL Branch Preview URL	Aug 19 2026, 09:13 PM

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

backend/tests/test_document_dedup.py (1)

41-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a test for multiple twin candidates.

TestFindDuplicate only ever mocks a single returned row. Add a case where select returns two rows for the same file_sha256 — one with extracted_text populated and one without — to verify find_duplicate reliably selects the usable row rather than depending on incidental result order. This directly guards the ordering concern raised on services/document_dedup.py lines 99-124.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_document_dedup.py` around lines 41 - 118, Add a
TestFindDuplicate case where the mocked select returns two matching rows in both
orders: one with extracted_text populated and one without. Assert
find_duplicate("cafe1234") returns the usable row in each order, verifying
selection does not depend on result ordering while preserving the existing
incomplete-twin behavior.

backend/tests/test_documents_routes.py (1)

1520-1557: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add coverage for the chunk-indexing skip and its effect on extracted_text.

This test asserts no agent is called and the SSE step sequence is unchanged, but it does not assert whether _index_document_chunks runs or whether the persisted document row retains extracted_text. Since self._TWIN["offering_id"] is "off-original" and this upload targets course_id="c-1", this specific test likely exercises the cross-course path, not the same-offering skip path. Add a variant where the twin's offering_id matches the upload's resolved offering, and assert on the persisted row's extracted_text, to catch the gap raised on backend/routes/documents.py lines 1057-1083.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1520 - 1557, Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0043_documents_file_sha256.sql`:
- Around line 27-32: Update the migration handling for idx_documents_file_sha256
so the partial index is created with CONCURRENTLY and executes outside the
transaction wrapper used by backend/db/migrate.py. Split or otherwise mark the
CREATE INDEX statement for non-transactional execution while preserving its
existing partial condition and IF NOT EXISTS behavior.
In `@backend/routes/documents.py`:
- Around line 1057-1083: Duplicate detection loses future matches because
extracted_text is only saved during chunk indexing. In
backend/routes/documents.py lines 1057-1083, update _persist_document to persist
encrypted extracted_text directly so rows that skip _index_document_chunks
retain it; in backend/services/document_dedup.py lines 99-124, update
find_duplicate’s limit-one query with an explicit ordering that prefers rows
where extracted_text is non-null.
---
Nitpick comments:
In `@backend/tests/test_document_dedup.py`:
- Around line 41-118: Add a TestFindDuplicate case where the mocked select
returns two matching rows in both orders: one with extracted_text populated and
one without. Assert find_duplicate("cafe1234") returns the usable row in each
order, verifying selection does not depend on result ordering while preserving
the existing incomplete-twin behavior.
In `@backend/tests/test_documents_routes.py`:
- Around line 1520-1557: Extend
test_streaming_duplicate_replays_without_calling_any_agent with a same-offering
duplicate variant by making the twin offering_id match the upload’s resolved
offering. Mock and assert _index_document_chunks is skipped, then inspect the
row passed to the mocked table insert and verify extracted_text retains the
duplicate’s stored text while preserving the existing SSE sequence and no-agent
assertions.

🪄 Autofix (Beta)

✅ Autofix completed

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 706f26a5-424e-4600-90d1-355844b0d103

📥 Commits

Reviewing files that changed from the base of the PR and between 37328d6 and fe2f685.

📒 Files selected for processing (6)

• backend/db/migrations/0043_documents_file_sha256.sql
• backend/db/migrations/0044_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

[Darkest-Teddy]

[Darkest-Teddy]

[coderabbitai]

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 2 file(s) based on 1 unresolved review comment.

Files modified:

• backend/db/migrate.py
• backend/db/migrations/0045_documents_file_sha256.sql

Commit:81037dd0fc5bb8000928df5de9c1f7d844322ace

The changes have been pushed to the feat/file-level-dedup branch.

Time taken:5m 18s

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_documents_routes.py (1)

1559-1604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock all three agents for full isolation in the same-course dedup test.

This test patches only classifier_agent.run (Line 1577) and asserts cls_run.assert_not_called() (Line 1595). It leaves summary_agent.run and concept_extraction_agent.run unpatched. The sibling test test_streaming_duplicate_replays_without_calling_any_agent (Lines 1526-1551) and the new _stream_dup helper (Lines 1651-1661) both mock all three agents for the same "stored result replay" scenario.

If a regression in the same-course replay path causes the agents to actually run, this test calls real, unmocked summary_agent.run and concept_extraction_agent.run. That produces a network call attempt or an unrelated exception, not a clear assertion failure. The test's own assertions also do not verify that the summary and concept agents are skipped, so a regression there would go undetected here.

Add the two missing patches and assertions to match the established pattern in this file.

🧪 Proposed fix to mock all agents and verify none run

 with (
_mock_validate_user(),
patch("routes.documents.extract_text_from_file") as extract,
patch("routes.documents.find_duplicate", return_value=twin),
patch("routes.documents.resolve_offering", return_value="off-same"),
patch("routes.documents.classifier_agent.run", AsyncMock()) as cls_run,
+ patch("routes.documents.summary_agent.run", AsyncMock()) as sum_run,+ patch("routes.documents.concept_extraction_agent.run", AsyncMock()) as cpt_run,
patch("routes.documents.apply_concepts_to_graph", AsyncMock(return_value=0)),
patch("routes.documents.table") as t,
patch("routes.documents._spawn_post_roll") as post_roll,
):

 extract.assert_not_called()
cls_run.assert_not_called()
+ sum_run.assert_not_called()+ cpt_run.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/test_documents_routes.py` around lines 1559 - 1604, Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/db/migrations/0045_documents_file_sha256.sql`:
- Line 1: Rename backend/db/migrations/0045_documents_file_sha256.sql to a
unique UTC timestamp-prefixed filename while preserving its order before the
agent-result migration; likewise rename
backend/db/migrations/0046_documents_agent_result.sql to a unique UTC
timestamp-prefixed filename ordered after the fingerprint migration, using the
YYYYMMDDHHMMSS_description.sql format.
---
Nitpick comments:
In `@backend/tests/test_documents_routes.py`:
- Around line 1559-1604: Update
test_streaming_duplicate_in_the_same_course_skips_the_chunk_index to patch
routes.documents.summary_agent.run and
routes.documents.concept_extraction_agent.run alongside classifier_agent.run,
using AsyncMock instances. Add assertions that both new mocks are not called,
matching the established stored-result replay tests while preserving the
existing behavior checks.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7dd3b933-a153-4235-b3bb-6103fd372af2

📥 Commits

Reviewing files that changed from the base of the PR and between fe2f685 and f64ff2c.

📒 Files selected for processing (6)

• backend/db/migrations/0045_documents_file_sha256.sql
• backend/db/migrations/0046_documents_agent_result.sql
• backend/routes/documents.py
• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/tests/test_documents_routes.py

🚧 Files skipped from review as they are similar to previous changes (3)

• backend/services/document_dedup.py
• backend/tests/test_document_dedup.py
• backend/routes/documents.py

[Jose-Gael-Cruz-Lopez]

Code review — file-level document dedup

This PR fingerprints the raw uploaded bytes with SHA-256 and, on a hit, reuses the twin's extracted_text, replays the twin's whole encrypted DocumentProcessingResult instead of re-running the four ingestion agents, and skips RAG re-indexing when the twin is in the same offering. The shape is right and the reasoning in the migration comments is unusually careful. Two things do not hold: on /upload/sync a replay drops the knowledge-graph merge entirely, and chunks_already_exist infers chunk presence from a documents row that may never have been indexed.

Verdict on the two load-bearing questions

Tenant scoping — NOT a cross-tenant leak.find_duplicate (document_dedup.py:99-150) filters only on file_sha256, deleted_at is.null, extracted_text not.is.null — no user_id, no offering_id. Global by design. The four agents whose output this reuses — classifier_agent, summary_agent, concept_extraction_agent, syllabus_extraction_agent — are all tool-less Agent(...) instances with a static module-level _SYSTEM_PROMPT and no tools= registration, so their outputs are genuinely pure functions of the extracted text. Every value handed back to user B is derived from bytes B just supplied; B learns nothing they did not already possess. agent_result is AES-GCM encrypted at rest. The only residual is a weak existence/timing oracle — not worth a finding.

Reuse of failed/partial artifacts — mostly guarded, one hole.find_duplicate guards the OCR artifact (extracted_text not.is.null in the query at line 114 plus the post-fetch if not extracted: return None at 127-132) and excludes soft-deleted twins. decode_result degrades a stale or invalid agent_result to None → run the agents. That covers text and agent results. It does not cover the third derived artifact — chunk indexing. See P1.

Other checks that came back clean: dedup adds no OOM/DoS surface (await file.read() and the 100 MB cap are pre-existing; file_sha256 hashes an already-buffered bytes). Concurrent identical uploads race benignly — both process, both insert, no unique constraint by design, and index_document_chunks upserts on conflict id. Migrations are nullable + IF NOT EXISTS; pre-existing rows get NULL and are excluded from the partial index, so no NULL collision. Deletion is a soft delete that never touches course_chunks, and find_duplicate excludes soft-deleted twins — no dangling storage. All Supabase access goes through db/connection.py::table(), HTTPException codes are explicit, and post-response work still goes through _spawn_post_roll/create_task, not BackgroundTasks.

Findings

P0

[P0] Replay on /upload/sync never merges concepts into the new uploader's knowledge graph — backend/routes/documents.py:694-727, 566-572

# routes/documents.py:566-572def_graph_backstop(*, user_id: str, course_id: str, filename: str,
result: DocumentProcessingResult) ->None:
"""Apply graph update if the orchestrator skipped its tool call."""ifresult.graph_updated:
returnifresult.classification.categorynotin ("syllabus", "assignment"):
return

The sync route has exactly one graph write path: process_document → _step_apply_graph → apply_concepts_to_graph (agents/document.py:207). apply_concepts_to_graph is imported at routes/documents.py:58 but called only once, at line 997, inside the streaming route. The replay branch at 694-700 skips process_document, leaving _graph_backstop (invoked at 724-727) as the only candidate — and it returns immediately, because graph_updated is a per-run, per-user flag (agents/document.py:207-213 sets graph_updated=merged > 0 for the original uploader) that _persist_document:484 bakes into the cached agent_result. Even with graph_updated=False, the second guard restricts the backstop to syllabus/assignment, so a duplicate slides/lecture_notes/reading upload gets nothing either way.

This is a live path, not a legacy one: frontend/src/components/Gradebook/SyllabusUploadFlow.tsx:54 → uploadSyllabus → uploadDocument → POST /api/documents/upload/sync (frontend/src/lib/api.ts:683, 1498). The second student to upload the same syllabus silently loses their graph seeding. It also contradicts the description directly: "All per-student side effects still fire — _save_orchestrator_syllabus, _graph_backstop, apply_concepts_to_graph". apply_concepts_to_graph does not fire on this route. The streaming route is fine — it recomputes merged at line 997 for the current user; the asymmetry is the bug. Run-scoped state does not belong inside a content-addressed cache payload.

No test covers this — test_duplicate_syllabus_populates_the_calendar_without_rerunning_agents patches routes.documents.apply_graph_update but never asserts on it.

P1

[P1] chunks_already_exist checks for a document row, not for chunks — backend/services/document_dedup.py:70-81, consumed at backend/routes/documents.py:1101-1109

# document_dedup.py:79-81ifnottwin:
returnFalsereturnbool(offering_id) andtwin.get("offering_id") ==offering_id

The function never touches course_chunks. It infers "the chunks are in the shared corpus" from "a documents row with these bytes exists in this offering", and there are four ways that is false — the first of which this PR creates:

1. /upload/sync never indexes. Its side effects are _invalidate_study_guide_cache, update_course_context, _check_upload_achievements (routes/documents.py:734-736); _index_document_chunks is called only from the streaming post-roll at line 1108. Before this PR a sync row had neither file_sha256 nor extracted_text and so could never be a twin — _persist_document:470-484 now writes both. So: A uploads a syllabus via SyllabusUploadFlow (/upload/sync, no chunks), B uploads the same bytes to the same course via the streaming modal, chunks_already_exist → True, indexing skipped, and the corpus has zero chunks for that material, permanently — every later upload of that file to that offering matches the same twin and skips too.
2. _index_document_chunks swallows every exception (routes/documents.py:1256-1257), so a failed prior index leaves the same false-positive row.
3. It returns early on if not chunks: return and on the relevance gate if dot < MIN_COURSE_RELEVANCE: return, both of which persist a document row with no chunks.
4. It is fire-and-forget via _spawn_post_roll → asyncio.create_task, so a duplicate arriving seconds later sees the row before the task has run.

The description names exactly this failure mode as the thing to avoid ("Skipping there would leave that course with no retrievable material"), so this is a gap against the PR's own invariant. A course_chunks lookup on chunk_id(course_code, chunks[0]), or a persisted indexed_at, is the sound signal.

Secondary, same function: chunk ids hash the course code (rag_service.chunk_id:161-176, resolved from courses.course_code inside _index_document_chunks), not the offering — so two offerings of the same course share the keyspace and the offering-equality test also misses a large share of the reuse this PR set out to capture. That direction is merely wasteful.

P2

[P2] find_duplicate is LIMIT 1 with no ORDER BY — backend/services/document_dedup.py:101-117

rows=table("documents").select(
_TWIN_COLUMNS,
filters={
"file_sha256": f"eq.{file_hash}",
"deleted_at": "is.null",
"extracted_text": "not.is.null",
},
limit=1,
)

The inline comment correctly identifies the unordered-LIMIT 1 hazard and fixes it for extracted_text, but the same hazard applies to the two other things callers depend on, and neither is filtered or ordered: a row with agent_result IS NULL can come back while a row with a stored result sits behind it (all four agents re-run for nothing), and an arbitrary-offering twin can come back while a same-offering one exists — which also feeds the chunk-skip decision above.

[P2] A permanently broken dedup lookup is invisible — backend/services/document_dedup.py:118-120

exceptException:
logger.debug("file_sha256 duplicate lookup unavailable", exc_info=True)
returnNone

Degrading to "no duplicate" is right, but None is also the normal result and DEBUG sits below production log level, so a dropped column, a PostgREST 400 from a filter typo, or sustained timeouts leave the feature silently never firing while every upload looks healthy. rag_service.retrieve_chunks:150-163 already handles this identical ambiguity one module over with a WARNING plus a countable rag.retrieval_failed event (#482) — worth matching.

[P2] The insert-retry now silently discards the dedup columns on any insert error, unlogged — backend/routes/documents.py:487-499

exceptException:
if"request_id"inrowor"file_sha256"inrow:
row.pop("request_id", None)
row.pop("file_sha256", None)
row.pop("agent_result", None)
inserted=table("documents").insert(row)

The bare-except retry predates this PR, but it now also strips the two columns the feature depends on, and it is not narrowed to a missing-column error. A transient PostgREST failure or an unrelated constraint violation permanently writes a document invisible to dedup with no replayable result — and nothing logs it.

[P2] New encrypted column documents.agent_result is not in the e2e ciphertext manifest — backend/e2e_oracles/gather.py:174-184

_CIPHERTEXT_MANIFEST lists ("documents", "id", "summary"), ("documents", "id", "concept_notes"), ("documents", "id", "extracted_text") but not agent_result, even though _persist_document:482-484 encrypts it precisely because "it carries the summary, concepts, and syllabus contents". Per the Canopy Infrastructure doc the encrypted-column set is enforced at rest by this manifest on every lane run; leaving the new column out means a regression that writes it in plaintext ships undetected.

P3

[P3] find_duplicate decrypts three fields no caller reads — backend/services/document_dedup.py:25-27, 134-146. _TWIN_COLUMNS selects category, summary, concept_notes and the return dict decrypts all three (including a decrypt_json round-trip), but both routes only read twin["extracted_text"], twin.get("result"), twin.get("offering_id") and twin.get("id").

[P3] Untyped signatures — backend/routes/documents.py:393async def _run_document_workers(extracted_text: str, deps, classification): and backend/services/document_dedup.py:39def decode_result(raw: str | None):. The Canopy Engineering Style Guide requires full typing; _run_document_workers is a straight extraction of inline code, so SaplingDeps / DocumentClassification / DocumentProcessingResult | None are free.

[P3] Description is stale about the migrations — the Changes table and the whole "Note for reviewers" paragraph describe 0043_documents_file_sha256.sql / 0044_documents_agent_result.sql and a renumbering from 0042/0043. At HEAD the files are 20260802012500_… and 20260802012600_…, correctly following the UTC-timestamp scheme (#509). Worth updating so the merge record matches what landed.

What's good

• SHA-256 over raw bytes with the filename deliberately excluded is the correct key, and file_sha256's docstring says why. No MD5 anywhere in the change.
• The forward/backward-compat story is genuinely complete: both columns nullable, IF NOT EXISTS, a partial index that excludes pre-migration rows, find_duplicate degrading to "no duplicate" when the column is absent, and decode_result treating model drift and a missing payload identically. The code can ship ahead of the migrations.
• The argument for persisting the whole DocumentProcessingResult rather than rebuilding it from summary/concept_notes (Summary.key_points has min_length=3, syllabus.assignments exists nowhere else on the row) is correct and well-documented in the migration header.
• Pinning the exact nine-step SSE sequence in test_streaming_duplicate_replays_without_calling_any_agent is the right way to prove the replay is client-invisible.

Verdict: request changes — the P0 graph-merge loss on /upload/sync and the P1 chunk-existence inference both cause silent, permanent data loss and should be fixed before merge. Dedup tenant scoping itself is sound.

---

_{Review-only pass — no code changed and nothing fixed. Conventions checked against the Canopy live docs (Engineering Style Guide, Architecture, Infrastructure, Backend & AI Agents). Every finding cites a snippet re-read at this PR's head SHA; severity: P0 blocker · P1 major · P2 minor · P3 nit.}

[Jose-Gael-Cruz-Lopez]

Review fixes applied

Every outstanding finding on this PR (human review + CodeRabbit) has been addressed and pushed.

Blocker

• The /upload/sync replay never merged concepts into the new uploader's graph.apply_concepts_to_graph is called only in the streaming route, and _graph_backstop returned early because graph_updated is a per-run flag baked into the cached agent_result — so the second student to upload a syllabus (a live path via SyllabusUploadFlow) silently lost their graph seeding, contradicting the PR description. The replay branch now runs the merge for the current user, and decode_result clears graph_updated so run-scoped state can never be served out of a content-addressed cache.

Major

• chunks_already_exist checked for a document row, not for chunks. It never touched course_chunks, and this PR created the worst path: /upload/sync now writes file_sha256 + extracted_text but never indexes, so a sync upload followed by a streaming upload of the same bytes would leave that course permanently without retrievable material — every later upload matching the same twin. It now queries course_chunks by real chunk ids (first and last, since batches drop before the upsert), keyed on course code to match the actual keyspace, and degrades to "index it" on failure because re-indexing is an idempotent content-addressed upsert while a wrong skip costs the course its material.

Minor

• find_duplicate selects a small ordered window and ranks candidates, so a twin with no agent_result can no longer win over one with a stored result (four wasted LLM calls) and same-offering is a deterministic tiebreak.
• A permanently broken dedup lookup is no longer invisible — warning plus a countable event, matching rag_service.retrieve_chunks.
• The insert-retry is narrowed to the missing-column case and logs, instead of silently stripping the dedup columns on any error.
• documents.agent_result added to the e2e ciphertext manifest — it was encrypted but unenforced.

Nits

Unread decrypts dropped · signatures typed · the three duplicate-path tests now mock all three agents and cover multi-candidate ordering and the same-offering skip.

Verification — ruff check . clean · 1574 passed, 38 skipped

_{Fixes applied and verified locally against this branch head; each figure above is a command I ran, not an estimate.}

[Jose-Gael-Cruz-Lopez]

Verify against the real database before merging

I could not reach a live database while working on this — there are no credentials on this machine (only .env.example files) and the local Supabase stack needs a container runtime that wouldn't start. So everything below was verified statically, by replaying every migration in ledger order to build a schema model and checking this PR's DB access against it. That model found 0 schema mismatches here, and it is trustworthy enough to have independently reproduced the documents.course_id absence that caused #562/#534 — but it cannot see data, and it cannot see whether staging's ledger actually matches the repo.

These are the checks that need a real connection.

1. Both migrations before the code

SELECT column_name, is_nullable FROMinformation_schema.columnsWHERE table_name ='documents'AND column_name IN ('file_sha256','agent_result');
SELECT indexname, indexdef FROM pg_indexes
WHERE tablename ='documents'AND indexdef ILIKE '%file_sha256%';

Both columns nullable, and the partial index present. find_duplicate returns "no duplicate" when the columns are absent, so the code is safe ahead of the DDL — but the feature is simply inert until it lands.

2. Pre-existing rows are deliberately invisible to dedup

SELECTcount(*) FILTER (WHERE file_sha256 IS NULL) AS no_fingerprint,
count(*) FILTER (WHERE file_sha256 IS NOT NULL) AS fingerprinted
FROM documents WHERE deleted_at IS NULL;

Everything uploaded before this ships has file_sha256 IS NULL, is excluded from the partial index, and can never be a twin. That is by design — please confirm no backfill is expected, because a backfill would need to re-read every stored file.

3. The chunk-existence check is the one that needs real data

The review found chunks_already_exist was inferring "chunks are indexed" from "a documents row exists in this offering", which is false four ways — most importantly because /upload/sync writes a row and never indexes. It now asks course_chunks directly, keyed on course code (course_chunks.id = chunk_id(course_code, text)), not offering.

Worth confirming against real rows that the skip can actually fire, i.e. that ids in course_chunks match what rag_service.chunk_id computes today:

SELECTcount(*) AS chunk_rows, count(DISTINCT course_id) AS courses FROM course_chunks;

Then upload a known-duplicate file into a course that already has chunks and confirm the embedding batch is skipped. If ids ever drifted from the current hash format, the check degrades to "index it" (safe, just no saving) — that is the intended failure direction, but it would mean the feature never pays off.

4. Encryption at rest

documents.agent_result is encrypted and was missing from the e2e ciphertext manifest; this PR adds it. Run the oracle against a real database so the column is actually asserted:

cd backend && venv/bin/python -m e2e_oracles

_{Static verification only — no live database was reachable from this environment. Schema model built by replaying backend/db/migrations/ in ledger order.}