Uh oh!
There was an error while loading. Please reload this page.
docs: newsletter email plaintext-by-decision ADR + encryption-gaps spec (#519) - #526
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 064e44d | Commit Preview URL Branch Preview URL | Aug 05 2026, 10:55 PM |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe change documents ChangesEncryption coverage documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md`:
- Line 1393: Update the expected ciphertext-column count in the oracle
validation text near the “journeys green; oracles exit 0” statement from 13 to
11, matching the documented additions and the count referenced later in the
plan.
- Around line 63-64: Update the pre-PR read-path statement near the
UNIQUE(email) discussion to clarify that, before PR B, no filters or search
operations read these columns; do not claim the application never read the
tables, since the new admin endpoints add application reads.
- Around line 857-862: Update the context decryption flow around
decrypt_json_column to stop converting every exception into an empty context
object. Remove the broad exception handler, or restrict it to documented
compatibility errors while emitting the required telemetry; preserve the
existing fallback for absent or validly empty context_json.
- Around line 494-500: Update the reports rendering in the reports.map block to
implement the screenshot-link contract: validate each r.screenshot_urls entry
and render links to the private screenshot URLs instead of displaying only the
count. If count-only behavior is intended, update the design specification and
response contract consistently instead.
- Around line 99-101: Update the fenced block containing the “Deliberate
exception” text to specify the text language tag, changing the untyped fence to
a text fence while preserving its contents.
- Around line 1380-1389: Update the locked E2E shell command around the make
e2e-up, Playwright, and e2e_oracles steps to guarantee make e2e-down runs on any
failure despite set -e. Install an EXIT trap before make e2e-up that performs
teardown, while preserving the original command failure status as the script’s
exit status.
🪄 Autofix
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: 373b84fa-2ed1-425a-a53e-b549009534fc
📒 Files selected for processing (4)
CLAUDE.mddocs/decisions/0026-newsletter-email-plaintext.mddocs/superpowers/plans/2026-08-05-encryption-gaps.mddocs/superpowers/specs/2026-08-05-encryption-gaps-design.md
| breaks this table's `UNIQUE(email)` constraint, its lookup index, and both | ||
| upserts' conflict detection, silently converting dedupe into duplicate rows. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the pre-PR read-path statement.
Lines 51-60 add application reads through the new admin endpoints, but Line 63 says the app never reads these tables. Change this to “Before PR B, no filters or search read these columns” or equivalent.
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` around lines 63 - 64,
Update the pre-PR read-path statement near the UNIQUE(email) discussion to
clarify that, before PR B, no filters or search operations read these columns;
do not claim the application never read the tables, since the new admin
endpoints add application reads.
| ``` | ||
| Deliberate exception: `newsletter_emails.email` stays plaintext (ADR 0026) — the UNIQUE constraint, lookup index, and both subscribe/allowlist upserts key on the value, and AES-GCM's per-call nonce breaks value equality. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced block.
The block at Line 99 contains plain text. Use a text fence so markdownlint MD040 passes.
Proposed fix
-```+```text
Deliberate exception: `newsletter_emails.email` stays plaintext ...📝 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.
| ``` | |
| Deliberate exception: `newsletter_emails.email` stays plaintext (ADR 0026) — the UNIQUE constraint, lookup index, and both subscribe/allowlist upserts key on the value, and AES-GCM's per-call nonce breaks value equality. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` around lines 99 - 101,
Update the fenced block containing the “Deliberate exception” text to specify
the text language tag, changing the untyped fence to a text fence while
preserving its contents.
Source: Linters/SAST tools
| {reports.map((r) => ( | ||
| <div key={r.id} style={{ padding: "12px 16px", borderBottom: "1px solid var(--border)" }} data-testid="adminfb-issue-row"> | ||
| <div style={{ display: "flex", gap: 12, color: "var(--text-dim)", fontSize: 13 }}> | ||
| <span>{when(r.created_at)}</span> | ||
| <span style={{ color: "var(--text)" }}>{r.user_name || r.user_id}</span> | ||
| <span>{r.topic}</span> | ||
| {r.screenshot_urls.length > 0 && <span>{r.screenshot_urls.length} screenshot(s)</span>} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Implement the screenshot-link contract.
The proposed component displays only the screenshot count at Line 500. The design specification requires links to the private screenshot URLs. Render validated links, or change the specification and response contract to require count-only behavior.
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` around lines 494 - 500,
Update the reports rendering in the reports.map block to implement the
screenshot-link contract: validate each r.screenshot_urls entry and render links
to the private screenshot URLs instead of displaying only the count. If
count-only behavior is intended, update the design specification and response
contract consistently instead.
| from services.encryption import decrypt_json_column | ||
| cj = ctx.get("context_json") or {} | ||
| try: | ||
| cj = decrypt_json_column(cj) or {} | ||
| except Exception: | ||
| cj = {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not hide all decryption failures as empty context.
The broad except Exception converts key mismatches, corrupted ciphertext, and programming errors into {}. Users then lose quiz context while the request appears successful. decrypt_json_column already handles the documented legacy shapes, so remove this catch or catch only documented compatibility errors and emit telemetry.
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` around lines 857 - 862,
Update the context decryption flow around decrypt_json_column to stop converting
every exception into an empty context object. Remove the broad exception
handler, or restrict it to documented compatibility errors while emitting the
required telemetry; preserve the existing fallback for absent or validly empty
context_json.
| flock /tmp/claude-1000/sapling-e2e-stack.lock bash -c ' | ||
| set -e | ||
| export SAPLING_MODEL_MODE=function | ||
| export SAPLING_FUNCTION_HANDLERS=agents.function_handlers_e2e | ||
| make e2e-up | ||
| (cd frontend && npx playwright test) | ||
| (cd backend && venv/bin/python -m e2e_oracles) | ||
| status=$? | ||
| make e2e-down | ||
| exit $status |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guarantee teardown when a test command fails.
With set -e, a Playwright or oracle failure exits before status=$? and before make e2e-down. The local stack can remain running and poison later runs. Install an EXIT trap before make e2e-up, or disable errexit while capturing both command statuses.
Proposed fix
set -e
+ trap 'rc=$?; make e2e-down || true; exit "$rc"' EXIT
export SAPLING_MODEL_MODE=function
export SAPLING_FUNCTION_HANDLERS=agents.function_handlers_e2e
make e2e-up
(cd frontend && npx playwright test)
(cd backend && venv/bin/python -m e2e_oracles)
- status=$?- make e2e-down- exit $status🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` around lines 1380 -
1389, Update the locked E2E shell command around the make e2e-up, Playwright,
and e2e_oracles steps to guarantee make e2e-down runs on any failure despite set
-e. Install an EXIT trap before make e2e-up that performs teardown, while
preserving the original command failure status as the script’s exit status.
| ' | ||
| ``` | ||
| Expected: journeys green; oracles exit 0 — the ciphertext oracle now proves all 13 new columns encrypted at rest in a really-running stack. Testing the tip covers B and C's changes too; if the tip needs a fix, re-run after fixing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the ciphertext-column count.
The listed additions total 11 columns: four feedback/issue columns, three quiz columns, and four derived-content columns. Line 1393 says 13, while Line 1437 says 11. Align the count so operators do not validate the oracle against the wrong coverage target.
🤖 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/superpowers/plans/2026-08-05-encryption-gaps.md` at line 1393, Update
the expected ciphertext-column count in the oracle validation text near the
“journeys green; oracles exit 0” statement from 13 to 11, matching the
documented additions and the count referenced later in the plan.
Closes#519.
Option 2 from the issue:
newsletter_emails.emailstays plaintext, deliberately —the UNIQUE constraint, lookup index, and both upserts (subscribe + admin allowlist)
key on the email value, which AES-GCM's per-call nonce breaks. ADR 0026 records the
decision and the rejected HMAC alternative; CLAUDE.md names the exception so the gap
is not re-filed. Also carries the approved design spec + implementation plan for the #522 stack.
🤖 Generated with Claude Code
Summary by CodeRabbit