Uh oh!
There was an error while loading. Please reload this page.
docs(adr): 0025 — encrypt course_chunks.chunk_text (#484) - #503
Conversation
Records the decision Andres already made, with the reasoning corrected.
The issue's premise ("pgvector similarity can't run over ciphertext") is
wrong: match_course_chunks ranks on 1 - (embedding <=> query_embedding) and
only SELECTs chunk_text as payload, and nothing queries it by content. So
encryption doesn't block retrieval and the decision is cheaper than stated.
But the same fact makes it partial, which is the part worth recording: the
embedding can't be encrypted (pgvector computes distance over it) and is
partially invertible back to its source text. So this restores boundary
consistency with documents.extracted_text — it does not make chunks
confidential, and the ADR says so explicitly rather than letting a future
reader assume otherwise.
Decided uniform (document AND catalog chunks) so the invariant is assertable
by the existing ciphertext oracle, and because decrypt_if_present's
raw-value fallback would make a real decrypt failure indistinguishable from a
legitimately-plaintext catalog row in a mixed table.
Ids stay computed on plaintext: AES-GCM's random nonce means identical text
encrypts differently every time, so ciphertext can never be a dedup key.
Flagged that scripts/dedupe_course_chunks.py:70 re-derives ids from the
STORED text and would destroy content-addressing if run against encrypted
rows.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdded ADR 0025. It defines encryption for ChangesRAG Chunk Text Encryption
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | c7b3c81 | Commit Preview URL Branch Preview URL | Jul 31 2026, 07:44 PM |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/decisions/0025-encrypt-rag-chunk-text.md`:
- Around line 75-86: Update the ADR’s ciphertext-oracle rollout description to
state that the strict “every chunk_text is ciphertext” assertion is enabled only
after encryption backfill completes. Define the mixed-phase behavior explicitly:
existing plaintext remains supported through decrypt_if_present’s raw-value
fallback while new or migrated rows follow the encrypted format, then switch to
strict validation once backfill finishes.
- Around line 58-61: Rewrite the paragraph to state that IDs are derived from
plaintext before encryption and preserve the separate namespaces: document rows
use course::document::text, while catalog rows use course::text. Ensure the
clarification retains stable existing IDs and prevents cross-category
collisions.
- Line 12: Update the paragraph beginning with “#484” to begin with “Issue `#484`
names a real asymmetry.” so Markdownlint no longer interprets it as an invalid
heading, while preserving the rest of the paragraph.
- Around line 52-55: Update decrypt_if_present so decryption authentication,
key, and format failures raise instead of returning the raw value, preventing
dedupe_course_chunks from hashing ciphertext or deleting the wrong
content-addressed row. Retain raw fallback only behind an explicit, positively
identified plaintext migration/backfill path, and preserve normal decryption
behavior for valid encrypted values.
🪄 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: 231e8094-2fdf-4b6f-a05f-6ffafc4f022f
📒 Files selected for processing (1)
docs/decisions/0025-encrypt-rag-chunk-text.md
| ## Context | ||
| #484 names a real asymmetry. `documents.extracted_text` is encrypted (0030) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the issue reference so Markdownlint can parse the paragraph.
Line 12 starts with #484 without a space. Markdownlint reports MD018 because it parses this as an invalid ATX heading. Replace it with Issue #484 names a real asymmetry.
Proposed fix
-#484 names a real asymmetry. `documents.extracted_text` is encrypted (0030)+Issue `#484` names a real asymmetry. `documents.extracted_text` is encrypted (0030)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #484 names a real asymmetry. `documents.extracted_text` is encrypted (0030) | |
| Issue `#484` names a real asymmetry. `documents.extracted_text` is encrypted (0030) |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 12-12: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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 `@docs/decisions/0025-encrypt-rag-chunk-text.md` at line 12, Update the
paragraph beginning with “#484” to begin with “Issue `#484` names a real
asymmetry.” so Markdownlint no longer interprets it as an invalid heading, while
preserving the rest of the paragraph.
Source: Linters/SAST tools
| - `decrypt_if_present` returns the **raw value** when it cannot decrypt. In a | ||
| mixed table that makes a genuine decrypt failure indistinguishable from a | ||
| legitimately-plaintext catalog row, which is precisely the kind of silent | ||
| degradation #482 was about. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n -C 12 \
-e 'def (encrypt_if_present|decrypt_if_present)' \
-e '\bdecrypt_if_present\b' \
-e '\bplan_migration\b' \
-e 'backfill_encryption' \
backend ||trueRepository: SaplingLearn/Sapling
Length of output: 50379
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"== relevant file sizes =="
wc -l backend/services/encryption.py backend/scripts/dedupe_course_chunks.py backend/scripts/backfill_document_chunks.py backend/db/backfill_encryption.py docs/decisions/0025-encrypt-rag-chunk-text.md
echoecho"== backend/services/encryption.py =="
cat -n backend/services/encryption.py
echoecho"== backend/scripts/dedupe_course_chunks.py =="
cat -n backend/scripts/dedupe_course_chunks.py
echoecho"== backend/scripts/backfill_document_chunks.py relevant section =="
cat -n backend/scripts/backfill_document_chunks.py | sed -n '1,180p'echoecho"== backend/db/backfill_encryption.py relevant section =="
cat -n backend/db/backfill_encryption.py | sed -n '1,220p'echoecho"== docs/decisions/0025-encrypt-rag-chunk-text.md relevant section =="
cat -n docs/decisions/0025-encrypt-rag-chunk-text.md | sed -n '1,90p'echoecho"== tests mentioning chunk dedupe or encryption migration =="
rg -n -C 4 'dedupe_course_chunks|decrypt_if_present|backfill_document_chunks|chunk_hash|chunkid|chunk_id' backend/tests backend/scripts backend/db docs ||trueRepository: SaplingLearn/Sapling
Length of output: 50377
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'import base64from cryptography.hazmat.primitives.ciphers.aead import AESGCMkey = bytes.fromhex("00"*32)aesgcm = AESGCM(key)plaintext = "hello world"nonce = b"0" * 12ciphertext = aesgcm.encrypt(nonce, plaintext.encode(), None)stored = base64.b64encode(nonce + ciphertext).decode()wrong_aesgcm = AESGCM(bytes.fromhex("11"*32))try: # This ciphertext is valid-looking base64 + AES nonce length for the current helper wrong_aesgcm.decrypt(nonce, ciphertext, None) decryption_succeeds = Trueexcept Exception as exc: decryption_succeeds = False exc_type = type(exc).__name__is_encrypted_fingerprint = stored.isascii() and base64.b64decode(stored)print({ "wrong_key_decrypt_succeeds": decryption_succeeds, "wrong_key_exception_type": exc_type if not decryption_succeeds else None, "ciphertext_base64_len": len(stored), "ciphertext_base64_is_ascii": stored.isascii(),})PYechoecho"== targeted encryption-related tests and scripts =="
rg -n -C 6 'decrypt_if_present.*fallback|fallback.*raw|ENCRYPTION_KEY|ciphertext|course_chunks|backfill_encryption|dedupe_course_chunks|plan_migration|extracted_text' \
backend tests frontend docs/dev \
--glob '!backend/prompts/**' \
--glob '!backend/db/backfill_encryption.py' \
--glob '!backend/services/encryption.py' \
| head -240Repository: SaplingLearn/Sapling
Length of output: 283
🌐 Web query:
Python cryptography library AESGCM decrypt wrong key exception
💡 Result:
In the Python cryptography library, the AESGCM.decrypt method raises a cryptography.exceptions.InvalidTag exception when decryption fails [1][2]. This exception is not specific only to an incorrect key; it is raised whenever the authentication tag cannot be validated [1][2]. Because AES-GCM is an authenticated encryption mode, the authentication tag is verified during decryption to ensure the integrity and authenticity of the ciphertext [3]. The verification will fail—triggering the InvalidTag exception—if any of the following are incorrect [1][2]: * The decryption key * The nonce (initialization vector) * The associated data (if any was provided during encryption) * The ciphertext itself (if it has been tampered with or corrupted) The library does not provide a distinct exception for an incorrect key because the nature of authenticated encryption means that from the perspective of the decryption process, any mismatch in these parameters results in an invalid authentication tag [3]. If you encounter this exception, ensure that the key, nonce, and associated data used for decryption match exactly those used during encryption [1][2].
Citations:
- 1: https://cryptography.io/en/36.0.2/hazmat/primitives/aead/
- 2: https://cryptography.io/en/latest/hazmat/primitives/aead/
- 3: https://crypto.stackexchange.com/questions/84355/can-aes-gcm-mode-detect-an-incorrect-key-and-refuse-to-decrypt
Make raw fallback fail closed during migration.
decrypt_if_present treats any decryption failure as legacy plaintext. A wrong key, corrupted ciphertext, or malformed AEAD header can be hashed as row["chunk_text"] by backend/scripts/dedupe_course_chunks.py, delete the real content-addressed row, and leave its metadata attached to ciphertext. Keep the fallback only where plaintext is positively identified (for example, during a migration backfill), and raise on encryption auth/key/format failures.
🤖 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 `@docs/decisions/0025-encrypt-rag-chunk-text.md` around lines 52 - 55, Update
decrypt_if_present so decryption authentication, key, and format failures raise
instead of returning the raw value, preventing dedupe_course_chunks from hashing
ciphertext or deleting the wrong content-addressed row. Retain raw fallback only
behind an explicit, positively identified plaintext migration/backfill path, and
preserve normal decryption behavior for valid encrypted values.
| Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the | ||
| same plaintext encrypts to different ciphertext every time, so ciphertext can | ||
| never be a dedup key. `chunk_id(course, text)` remains the merge key, and | ||
| re-uploads of identical content still converge on one row. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the separate document and catalog ID namespaces.
The existing contract is not one chunk_id(course, text) rule for every row. backend/services/rag_service.py:161-177 documents course::document::text for document rows and course::text for catalog rows. Rewrite this paragraph to require plaintext hashing before encryption while preserving both namespaces. Otherwise an implementation could change existing IDs or create cross-category collisions.
Proposed clarification
-Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the-same plaintext encrypts to different ciphertext every time, so ciphertext can-never be a dedup key. `chunk_id(course, text)` remains the merge key, and-re-uploads of identical content still converge on one row.+Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the+same plaintext encrypts to different ciphertext every time, so ciphertext can+never be a dedup key. Preserve the existing per-kind namespaces: document rows+use `chunk_id(course, text)` and catalog rows use `sha256(course::text)`.+Encryption occurs after ID derivation, so identical content still converges+within each row kind.This follows the ID contract documented in backend/services/rag_service.py:161-177.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the | |
| same plaintext encrypts to different ciphertext every time, so ciphertext can | |
| never be a dedup key. `chunk_id(course, text)` remains the merge key, and | |
| re-uploads of identical content still converge on one row. | |
| Ids stay keyed on plaintext because AES-GCM uses a random nonce per call: the | |
| same plaintext encrypts to different ciphertext every time, so ciphertext can | |
| never be a dedup key. Preserve the existing per-kind namespaces: document rows | |
| use `chunk_id(course, text)` and catalog rows use `sha256(course::text)`. | |
| Encryption occurs after ID derivation, so identical content still converges | |
| within each row kind. |
🤖 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 `@docs/decisions/0025-encrypt-rag-chunk-text.md` around lines 58 - 61, Rewrite
the paragraph to state that IDs are derived from plaintext before encryption and
preserve the separate namespaces: document rows use course::document::text,
while catalog rows use course::text. Ensure the clarification retains stable
existing IDs and prevents cross-category collisions.
| - (+) A uniform invariant the `ciphertext` oracle can assert, so a future | ||
| regression fails a lane instead of sitting unnoticed. | ||
| - (−) **Residual exposure: the embedding stays plaintext and is partially | ||
| invertible.** This is defense in depth, not confidentiality. Anyone reading | ||
| this ADR to answer "is chunk content protected?" must read this line. | ||
| - (−) `scripts/dedupe_course_chunks.py:70` re-derives ids from the **stored** | ||
| `chunk_text`. Run against encrypted rows it would hash ciphertext and | ||
| destroy content-addressing. It must decrypt before hashing, or be retired — | ||
| its own docstring calls it a one-time migration. | ||
| - (−) A backfill is required for existing rows (the | ||
| `db/backfill_encryption.py` precedent). Until it completes the table is | ||
| mixed, carried by `decrypt_if_present`'s raw-value fallback. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Define the ciphertext-oracle rollout order.
The ADR says the oracle should assert that every chunk_text is ciphertext. It also says legacy plaintext remains until backfill. State that the strict oracle runs only after backfill, and define the expected behavior during the mixed phase. Otherwise the rollout either fails a valid pre-backfill state or weakens the invariant.
🤖 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 `@docs/decisions/0025-encrypt-rag-chunk-text.md` around lines 75 - 86, Update
the ADR’s ciphertext-oracle rollout description to state that the strict “every
chunk_text is ciphertext” assertion is enabled only after encryption backfill
completes. Define the mixed-phase behavior explicitly: existing plaintext
remains supported through decrypt_if_present’s raw-value fallback while new or
migrated rows follow the encrypted format, then switch to strict validation once
backfill finishes.
Part of #484. Records the decision with the reasoning corrected.
The premise in the issue is wrong
match_course_chunks(migration 0039) computes1 - (c.embedding <=> query_embedding), orders byc.embedding <=> query_embedding, and filtersWHERE c.embedding IS NOT NULL.chunk_textis only SELECTed as payload — the ranking never reads it. Nothing queries it by content either (noILIKE/LIKE/FTS anywhere inservices/,routes/,scripts/).So encryption doesn't block retrieval, and this is cheaper than the issue implies.
But the same fact makes it partial — and that's what the ADR exists to record
The
embeddingcolumn cannot be encrypted, because pgvector must compute distance over it. An embedding is a lossy but real representation of its source text, and embedding-inversion recovers substantial content from vectors alone.So encrypting
chunk_textdoes not make the row opaque. Without that written down, the next person reads "chunk_text is encrypted" and concludes chunk content is confidential. It isn't. The ADR states this as a consequence rather than burying it.What was decided
Encrypt uniformly — document and catalog chunks — with ids still computed on plaintext.
chunk_textis always ciphertext") is assertable by the existingciphertextoracle, and becausedecrypt_if_presentreturns the raw value on failure — in a mixed table that makes a genuine decrypt failure indistinguishable from a legitimately-plaintext catalog row.One trap found while verifying
scripts/dedupe_course_chunks.py:70re-derives chunk ids from the storedchunk_text. Run against encrypted rows it would hash ciphertext and destroy content-addressing. It must decrypt first or be retired — its docstring calls it a one-time migration.Scope
ADR only; no code. Implementation sites are listed at the bottom of the file. #483 is blocked on this decision, so landing the decision unblocks it.
🤖 Generated with Claude Code
Summary by CodeRabbit