feat(email-writing): persist privacy-minimized review evidence - #1328
feat(email-writing): persist privacy-minimized review evidence#1328seonghobae wants to merge 16 commits into
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Limit details: You’ve used all 1 included review currently available under your plan. 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?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds privacy-minimized email-writing evidence models and an Alembic migration. Adds contract tests for schema behavior, serialization, constraints, cascades, and migration parity. Adds CI checks for tests, coverage, dependency hashes, and Ruff linting. ChangesEmail-writing evidence
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/tests/test_email_writing_migration.py (2)
54-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the resolved metadata instead of the
env.pysource text.This test matches a literal source line. Any equivalent refactor of
env.pybreaks it, and the test proves nothing about the metadata that Alembic actually receives. Import the module and check the resolved table set.♻️ Proposed refactor
def test_alembic_environment_registers_review_evidence_metadata() -> None: - """Autogenerate sees the modular evidence models without editing the legacy file.""" - environment_source = (BACKEND_ROOT / "alembic" / "env.py").read_text( - encoding="utf-8" - ) - assert "email_writing_evidence" in environment_source - assert ( - "target_metadata = EmailReviewSession.__table__.metadata" - in environment_source - ) + """Autogenerate sees the evidence tables in the target metadata.""" + target_metadata = EmailReviewSession.__table__.metadata + assert set(NEW_TABLE_NAMES).issubset(target_metadata.tables.keys())🤖 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_email_writing_migration.py` around lines 54 - 63, Update test_alembic_environment_registers_review_evidence_metadata to import the Alembic environment module and inspect its resolved target_metadata rather than searching env.py source text. Assert that the metadata contains the expected email_writing_evidence table, preserving verification of the metadata Alembic actually receives.
66-88: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare column types and nullability, not only names.
The parity check compares column names, constraint names, and index names. A drift in type or length between the ORM model and the migration passes this test. For example, if the model changes
prompt_hashtoString(96)and the migration keepsString(71), the assertion still succeeds and the deployed schema rejects valid rows.♻️ Proposed addition
for table_name in NEW_TABLE_NAMES: assert set(migration_metadata.tables[table_name].columns.keys()) == set( orm_tables[table_name].columns.keys() ) + migration_columns = { + column.name: (str(column.type), column.nullable) + for column in migration_metadata.tables[table_name].columns + } + orm_columns = { + column.name: (str(column.type), column.nullable) + for column in orm_tables[table_name].columns + } + assert migration_columns == orm_columns assert {🤖 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_email_writing_migration.py` around lines 66 - 88, Extend test_migration_revision_and_metadata_match_orm_contract to compare each matched column’s type and nullable attributes between migration_metadata and the corresponding ORM table, while retaining the existing name, constraint, and index checks. Ensure differences such as String length cause the assertion to fail.backend/tests/test_email_writing_models.py (1)
121-127: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfirm the stub
email_recordstable matches the realThe fixture creates a one-column
email_recordstable instead of the mappedEmail.id. IfEmail.idchanges type or name, these tests still pass and the migration breaks in PostgreSQL. Consider creatingEmail.__table__in the fixture instead.#!/bin/bash # Description: Inspect the real Email model table name and primary key definition. fd -t f 'models.py' backend/db | while read -r f; do rg -n -C4 '__tablename__\s*=\s*"email_records"' "$f" done rg -nP -C6 'class\s+Email\b' backend/db/models.py🤖 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_email_writing_models.py` around lines 121 - 127, Update the fixture setup around the in-memory engine and foreign-key PRAGMA to create the mapped Email.__table__ metadata instead of manually defining a one-column email_records table. Insert the required record through the real table definition, preserving the existing test data while ensuring the foreign key uses Email.id’s actual name and type.backend/alembic/env.py (1)
10-17: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer
Base.metadataand keep the evidence import for registration only.
EmailReviewSessioninherits fromdb.models.Base, soEmailReviewSession.__table__.metadatais the same object asBase.metadatatoday. The behavior is correct, but the expression is indirect and fragile. If a future change moves the evidence models onto a separate declarative base,target_metadatasilently shrinks to the evidence tables only, and autogenerate then proposesdrop_tablefor every other table in the database.State the metadata source directly and import the evidence module for model registration.
♻️ Proposed refactor
-from db.email_writing_evidence import EmailReviewSession +import db.email_writing_evidence # noqa: F401 # register evidence tables on Base.metadata +from db.models import Base config = context.config if config.config_file_name is not None: fileConfig(config.config_file_name) -target_metadata = EmailReviewSession.__table__.metadata +target_metadata = Base.metadata
backend/tests/test_email_writing_migration.pylines 60-63 assert this exact source string, so update that assertion in the same change.🤖 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/alembic/env.py` around lines 10 - 17, Update Alembic’s metadata setup in env.py to import and use db.models.Base.metadata directly for target_metadata, while retaining the EmailReviewSession import solely to register the evidence model. Update the exact-source assertion in test_email_writing_migration.py to expect the new metadata expression.
🤖 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 @.github/workflows/email-writing-evidence-tdd.yml:
- Around line 53-67: Update both pytest commands in the “Run privacy-minimized
model and migration tests” and “Verify migration statement and branch coverage”
steps to run with PYTHONWARNINGS=error and DISABLE_BACKGROUND_WORKERS=1. Ensure
each test invocation fails if output contains Timeout, Fatal, Warn, or Denied,
while preserving the existing test selections and coverage settings.
In `@backend/db/email_writing_evidence.py`:
- Around line 168-196: Update EmailWritingEvidence.to_evidence_dict to stop
serializing source_email_id, owner_user_id, and owner_organization_id; use an
existing opaque source identifier such as source_email_uid if the model supports
public serialization, otherwise remove these identifiers and rename the method
to clearly indicate internal scope. Update the corresponding test assertion in
test_email_writing_models.py to match the privacy-safe payload.
In `@backend/tests/test_email_writing_models.py`:
- Around line 119-132: Ensure engine cleanup runs on every path in both
fixtures: in backend/tests/test_email_writing_models.py lines 119-132, wrap the
schema setup and yield in evidence_session with try/finally and dispose the
engine in finally; in backend/tests/test_email_writing_migration.py lines
91-144, wrap the engine.begin() setup in try/finally and dispose the engine in
finally.
---
Nitpick comments:
In `@backend/alembic/env.py`:
- Around line 10-17: Update Alembic’s metadata setup in env.py to import and use
db.models.Base.metadata directly for target_metadata, while retaining the
EmailReviewSession import solely to register the evidence model. Update the
exact-source assertion in test_email_writing_migration.py to expect the new
metadata expression.
In `@backend/tests/test_email_writing_migration.py`:
- Around line 54-63: Update
test_alembic_environment_registers_review_evidence_metadata to import the
Alembic environment module and inspect its resolved target_metadata rather than
searching env.py source text. Assert that the metadata contains the expected
email_writing_evidence table, preserving verification of the metadata Alembic
actually receives.
- Around line 66-88: Extend
test_migration_revision_and_metadata_match_orm_contract to compare each matched
column’s type and nullable attributes between migration_metadata and the
corresponding ORM table, while retaining the existing name, constraint, and
index checks. Ensure differences such as String length cause the assertion to
fail.
In `@backend/tests/test_email_writing_models.py`:
- Around line 121-127: Update the fixture setup around the in-memory engine and
foreign-key PRAGMA to create the mapped Email.__table__ metadata instead of
manually defining a one-column email_records table. Insert the required record
through the real table definition, preserving the existing test data while
ensuring the foreign key uses Email.id’s actual name and type.
🪄 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: be6fb858-2cfb-49f9-9f22-35123b988e1b
📒 Files selected for processing (6)
.github/workflows/email-writing-evidence-tdd.ymlbackend/alembic/env.pybackend/alembic/versions/20260812_0001_add_email_writing_review_evidence.pybackend/db/email_writing_evidence.pybackend/tests/test_email_writing_migration.pybackend/tests/test_email_writing_models.py
|
PR governance metadata gate is not ready for
|
…-task2' into feat/llm-email-writing-review-evidence-task3
…-task2' into feat/llm-email-writing-review-evidence-task3 Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
@coderabbitai review Please review the unchanged exact current head |
|
|
…ask4' into feat/llm-email-writing-orchestrator-task5 Retarget Task 5 onto live #1329 head 4570747 (merged onto live #1328 51fb5e8 / #1327 fb7c406 / #1322 bfc2df1 / develop@dd8d1519). Preserve the hardened contextual-orchestrator boundary. Do not restore write-capable Task 5 promotion/finalize workflows. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
opencode-agent
Bot
added
status: draft
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
2 participants
Stack dependency
This Draft PR is stacked on #1327 (
feat/llm-email-writing-contracts-task2) and implements Task 3 only from the committed LLM email-writing implementation plan.Customer next action
This PR is persistence-only. Customers should continue to write and send email with the current editor and send path. No writing-guidance feature, language profile, or model is available in product. Writing and sending stay on the current path.
Included
email_review_sessionfor tenant ownership, source-email reference, immutable document revision/projection, review mode, language profile, status, workflow/model/rubric/policy versions, prompt hash, bounded timing/cost/token buckets, and evidence expiry;writing_diagnostic_recordfor opaque diagnostic identity, category/priority, exact selector positions, candidate/replacement/explanation hashes, criterion categories, Judge score, and admission outcome;diagnostic_feedback_eventfor owner-scoped idempotent Apply/Ignore/Dismiss/Explain feedback, reviewed/resulting revisions, conflict/stale reason codes, and event time;snake_casetables, columns, indexes, constraints, and relationships;to_evidence_dict()and__repr__surfaces containing no authored or provider plaintext.Modular model boundary
The cohesive models live in
backend/db/email_writing_evidence.py, reuse the canonicalBaseandEmail, and supply the shared metadata to Alembic. This avoids a second metadata registry while keeping the evidence aggregate independently testable.Privacy boundary
The schema and ORM deliberately do not contain source mail bodies, authored drafts, replacement or explanation text, prompts, raw model output, provider credentials, or complete orchestration traces. Public evidence serialization also omits tenant-owner identifiers and the sequential
source_email_id; those remain persistence-only authorization and lineage fields.Exact-head verification
Previous head (exact current PR head at launch):
GitHub last showed base
feat/llm-email-writing-contracts-task2at stale:Current head (normal merge commit, not squash, not rebase, not force-push):
Live parent #1327 (
feat/llm-email-writing-contracts-task2) merged as the second parent:Normal merge commit parents:
6ab000efd8ad262e3f51462ae5cfddd57d849c53fb7c406ee1328a6ac42dbaf54bb6852c199d8b0aLive parent #1327 already contains #1322
bfc2df112136bb9fe358778d701e78bf9e78b685/ develop@dd8d15191338b841f9e6f3a06507c6a5643b95d0.ADR numbering follows the live parent:
0004-status-weighted-calendar-conflicts.md);Alembic after merge:
0017_merge_newsdom_carddav_heads(no0018_email_date_provenance; feat(email): deterministic dedupe provenance — gate strong fingerprints on genuine Date (naruon#1086) #1195 is not merged);20260812_email_writing_evidencewith down_revision0017_merge_newsdom_carddav_heads;20260812_email_writing_evidence;Local validation counts on
51fb5e8543247b1e5c790f3fdf98424c8fbed669:tests/test_email_writing_models.py)tests/test_email_writing_migration.py)34statements,4branches,fail-under=100)compileallfor evidence/Alembic files: okgit diff --checkon Task 3 evidence files: okTimeout/Fatal/Warn/DeniedPredecessor evidence does not transfer. Checks, reviews, and security evidence recorded for
6ab000ef,b10f8dcc, or any earlier head are non-passing for this head.Merge boundary
Keep this PR Draft while #1327 and its parent design PR remain unmerged. This slice does not add email/thread authorization, contextual-orchestrator transport, model or Judge calls, API routes, editor UI integration, sending, publishing, or release changes. Independent review, zero actionable threads, branch protection, and exact-current-head required checks remain mandatory before merge. This update does not approve, merge, squash, empty-requeue, force-cancel, or mark Ready.