feat(email-writing): persist privacy-minimized review evidence - #1328
feat(email-writing): persist privacy-minimized review evidence#1328seonghobae wants to merge 19 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
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Dismissed as addressed predecessor-head review evidence only. This CHANGES_REQUESTED review evaluated 001da508739ff1b49e3d857e593fb27cd4339269; the current Task-3 head is f65974e5d535f9546c1bc09e5f743b9db0d59bbe after ordinary stack reconciliation. The three actionable threads are resolved on current source, including workflow safety settings, privacy-minimized evidence serialization, and engine cleanup; later review on exact current head reported APPROVED. This dismissal does not convert the old review into approval and transfers no predecessor check/review evidence.
|
@coderabbitai review |
|
seonghobae
added
the
enhancement
ChatGPT Codex Connector
2 participants
Scope
This Draft PR implements Task 3 only: privacy-minimized persistence for email-writing review sessions, diagnostic evidence, and feedback events. It excludes authorized thread construction, contextual-orchestrator/model calls, Candidate/Judge execution, review API, editor integration, sending, policy publication, and release behavior.
The feature-owned evidence schema stores authorization/lineage identifiers, revisions/hashes, selector positions, bounded operational buckets, criterion/category outcomes and feedback state. It must not persist raw source email, authored draft, replacement/explanation text, prompts, raw model/Judge output, provider credentials, or complete orchestration traces in ordinary evidence/log surfaces.
Live stack state — 2026-09-01
Direct parent #1327 is
ab74a345e4d03680da32a7eac2cc1fca3005cfb3. This Task-3 head isea61b9de9cd26a26209da36e858510a31486531aand contains that exact parent through an ordinary non-destructive two-parent merge. No force-push or destructive rebase was used.Its unique Task-3 delta remains six files: the evidence TDD workflow, Alembic environment integration, the review-evidence migration, the feature-owned persistence models, and the two focused migration/model test modules. The current email-writing design documents are inherited from the root stack and are not Task-3-owned semantic delta. All predecessor-head evidence is stale after this movement.
Data/privacy contract retained
email_review_session,writing_diagnostic_record, anddiagnostic_feedback_eventremain feature-owned evidence objects;snake_casewith explicit constraints/indexes;Evidence and continuation
Fresh exact-head migration, PostgreSQL/SQLite behavior, Python 3.14, owned coverage/docstrings, security/SAST/dependency/package/provenance and review/thread evidence must be regenerated for
ea61b9de9cd26a26209da36e858510a31486531a. Child #1329 is reconciled onto this current Task-3 head as64d1f746723616b0111eb40b130d6f07ed413a86; later task-owned descendants have likewise been advanced in dependency order. No predecessor check/review result transfers across head movements.Merge boundary
Keep Draft while current-head evidence is regenerated. Merge only on an unchanged head satisfying live rulesets/protection, every applicable exact-head gate, zero valid unresolved findings, and any qualifying independent approval actually required after the last push. Pending/queued/skipped-required/cancelled/neutral/failed/absent/stale/predecessor/synthetic/model-only/status-only/author-only evidence is non-passing.
No writing-guidance feature is shipped by this persistence slice; editing and sending remain available.