fix(import): preserve attachment source on security owner stack

[seonghobae]

Authority

• exact head: 23ab3653543240c9c194e98530bb793d037489bc
• tree: 525d4c42800f5ac2912385d1b18aff5bd83a19f9
• stacked base / security owner: fix(security): stabilize exception-redaction successor #1612 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7
• protected ancestor: develop@042b0c70531b229af3acbd0421a2f23098d848b3
• product finding: fix(import): preserve attachment content when fixture embedding fails #1697
• predecessor: fix(import): preserve attachments when enrichment fails #1699 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7
• lifecycle: Draft — canonical stacked successor; source/test/doctoring succession present and current-head independent review APPROVED; hosted PostgreSQL/security evidence still unavailable on stacked base

Why this successor exists

#1699 correctly repaired attachment-source loss and transaction ordering, but it was built directly on protected develop while #1612 already owns exception-redaction changes in the same backend/import_fixtures.py file. As a result, #1699's current tree reintroduced raw exception interpolation relative to the canonical #1612 security lineage. That is a single-writer/stacking defect, not a reason to discard #1699's valid data-integrity work.

This successor starts from unchanged #1612 and reapplies only #1699's valid attachment/transaction/test/doctoring delta. It preserves #1612's static, non-secret failure logging. #1699 remains open as predecessor provenance until this successor or an integrated descendant fully proves succession; do not merge both.

Combined causal repair

The root fixture importer now:

1. checks owner-scoped message_id duplication before enrichment;
2. ends the implicit duplicate-read transaction with rollback() before model/provider work;
3. performs body and attachment enrichment with no database transaction held;
4. retains every parsed attachment as authoritative source content, using embedding=None when only attachment enrichment fails;
5. repeats the duplicate guard after external work;
6. performs thread lookup, aggregate staging and commit as the short database-only phase;
7. retains fix(security): stabilize exception-redaction successor #1612's non-secret static error messages instead of restoring raw exception values.

Body embedding remains fail-closed. Commit failure still rolls back.

Verification contract

• Recording-session tests require no enrichment inside the duplicate-read transaction, duplicate short-circuiting before enrichment, source attachment retention, and nullable attachment embedding.
• Existing fixture-import test doubles implement the rollback method now required by the real session protocol.
• A real @pytest.mark.postgres acceptance imports through production import_eml_file(), reloads the persisted Email/Attachment aggregate, requires source filename/content with embedding=None, deletes the Email, and verifies attachment cascade cleanup.
• Doctoring records the source-vs-derived invariant, fix(security): stabilize exception-redaction successor #1612 security-owner ancestry, PostgreSQL evidence boundary, and the distinction between current-model smoke and Alembic migration compatibility.

Current evidence boundary

CodeRabbit formally APPROVED exact 23ab3653543240c9c194e98530bb793d037489bc at 2026-09-15T16:59:10Z. This satisfies the independent-review component for the unchanged current head; it does not substitute for executable database/security evidence.

Commit-SHA workflow lookup still returns zero repository-local PR workflow runs for this exact head because the PR is stacked on #1612 and the repository-local stacked-PR trigger defect remains owned by #1691. The PostgreSQL test being present is therefore not itself a hosted receipt. Do not fabricate evidence via temporary retargeting, copied workflows, synthetic statuses, or no-op commits.

Gate

Keep Draft. Normal forward path is #1612 protected integration → ordinary retarget/reconciliation of this unchanged effective delta onto protected lineage → exact-head PostgreSQL/migration/security checks → re-evaluate whether the existing unchanged-head approval remains admissible under the then-live merge policy. #1697 closes only after normal integration proves complete succession. No force push, destructive rebase, self-approval, gate weakening, duplicate merge, or scanner suppression.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 38a56cc0-7257-4cb0-a9a5-4fe0f690d06b

📥 Commits

Reviewing files that changed from the base of the PR and between 3da3ae8 and 23ab365.

📒 Files selected for processing (4)

• backend/import_fixtures.py
• backend/tests/test_import_fixture_attachment_integrity.py
• backend/tests/test_import_fixtures.py
• docs/doctoring/fixture-import-attachment-enrichment.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The fixture importer now checks duplicates before and after enrichment, ends read transactions before embedding calls, preserves attachments when attachment embedding fails, and assigns prepared attachments to the email. Tests cover transaction behavior and PostgreSQL persistence.

Changes

Fixture import integrity

Layer / File(s)	Summary
Duplicate checks and transaction boundaries backend/import_fixtures.py, docs/doctoring/fixture-import-attachment-enrichment.md	The importer uses an owner-scoped duplicate helper before and after enrichment. It rolls back the read transaction before external embedding work. The design document records the required operation order and failure behavior.
Attachment persistence and regression coverage backend/import_fixtures.py, backend/tests/test_import_fixture_attachment_integrity.py, backend/tests/test_import_fixtures.py	The importer retains parsed attachments with embedding=None when attachment embedding fails, then assigns the prepared attachments to the email. Tests cover duplicate handling, rollback calls, persistence, and cascading deletion.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant import_eml_file
  participant DatabaseSession
  participant EmbeddingService
  participant Email
  import_eml_file->>DatabaseSession: Check for existing message_id
  DatabaseSession-->>import_eml_file: Duplicate status
  import_eml_file->>DatabaseSession: Roll back read transaction
  import_eml_file->>EmbeddingService: Embed body and attachments
  EmbeddingService-->>import_eml_file: Embeddings or attachment failure
  import_eml_file->>DatabaseSession: Re-check for duplicate
  import_eml_file->>Email: Attach prepared attachments
  import_eml_file->>DatabaseSession: Commit fixture

Loading

Merge Risk: ⚪ Minimal · up to 23ab3

No actionable merge-blocking risk remains in the reviewed changes.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: preserving attachment source content during import on the security-owner lineage.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/import-attachment-integrity-security-stack

---

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.}

[seonghobae]

@coderabbitai review exact head 23ab3653543240c9c194e98530bb793d037489bc. Please verify the conflict-resolution contract: #1612's exception-redaction behavior must remain intact while #1699's attachment-source/transaction/PostgreSQL acceptance delta is fully inherited. Treat predecessor reviews as stale and do not mark the PostgreSQL acceptance complete unless the exact test actually executes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== HEAD AND ANCESTRY =='
git rev-parse HEAD
git status --short
git merge-base --is-ancestor 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc && echo 'base-1612-is-ancestor=yes' || echo 'base-1612-is-ancestor=no'
git merge-base --is-ancestor 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7 23ab3653543240c9c194e98530bb793d037489bc && echo 'predecessor-1699-is-ancestor=yes' || echo 'predecessor-1699-is-ancestor=no'
git merge-base 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc
git merge-base 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7 23ab3653543240c9c194e98530bb793d037489bc

echo '== CHANGED PATHS: `#1612` BASE..HEAD =='
git diff --name-status 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc
echo '== CHANGED PATHS: PROTECTED ANCESTOR..#1612 =='
git diff --name-status 042b0c70531b229af3acbd0421a2f23098d848b3 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7
echo '== CHANGED PATHS: PROTECTED ANCESTOR..#1699 =='
git diff --name-status 042b0c70531b229af3acbd0421a2f23098d848b3 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7

echo '== CANDIDATE FILES =='
fd -a -t f 'import_fixtures\.py|.*import.*fixture.*|.*doctor.*|.*fixture.*test.*|.*import.*test.*' . -E .git | sed -n '1,200p'

echo '== IMPORTER OUTLINE =='
ast-grep outline backend/import_fixtures.py --items all 2>/dev/null || true

echo '== IMPORTER DIFF: `#1612` BASE..HEAD =='
git diff --unified=80 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc -- backend/import_fixtures.py

echo '== IMPORTER DIFF: PROTECTED ANCESTOR..#1612 =='
git diff --unified=80 042b0c70531b229af3acbd0421a2f23098d848b3 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 -- backend/import_fixtures.py

echo '== IMPORTER DIFF: PROTECTED ANCESTOR..#1699 =='
git diff --unified=80 042b0c70531b229af3acbd0421a2f23098d848b3 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7 -- backend/import_fixtures.py

echo '== TEST AND DOCTORING DELTA: `#1612` BASE..HEAD =='
git diff --unified=50 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc -- ':(exclude)backend/import_fixtures.py'

echo '== PR AND CHECK EVIDENCE =='
gh pr view 1701 --repo ContextualWisdomLab/naruon --json number,state,isDraft,headRefOid,baseRefName,commits,statusCheckRollup,reviewDecision,latestReviews,url
gh run list --repo ContextualWisdomLab/naruon --commit 23ab3653543240c9c194e98530bb793d037489bc --limit 30 --json databaseId,name,workflowName,event,status,conclusion,headSha,createdAt,updatedAt,url

Length of output: 45705

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== IMPORTER AT EXACT HEAD =='
cat -n backend/import_fixtures.py | sed -n '1,190p'

echo '== ATTACHMENT TEST OUTLINE =='
ast-grep outline backend/tests/test_import_fixture_attachment_integrity.py --items all
echo '== ATTACHMENT TEST REMAINDER =='
cat -n backend/tests/test_import_fixture_attachment_integrity.py | sed -n '1,280p'

echo '== PYTEST CONFIGURATION =='
fd -a -t f 'pyproject\.toml|pytest\.ini|tox\.ini|setup\.cfg|conftest\.py' . -E .git | while read -r file; do
  echo "--- $file"
  rg -n -C 4 'pytest|markers|postgres|addopts' "$file" || true
done

echo '== DATABASE MODEL CANDIDATES =='
fd -a -t f 'models\.py' backend -E .git
echo '== EMAIL / ATTACHMENT MODEL OUTLINE =='
ast-grep outline backend/db/models.py --items all --match 'Email|Attachment' 2>/dev/null || true
echo '== EMAIL / ATTACHMENT MAPPING AND CASCADE =='
rg -n -C 8 'class (Email|Attachment)\b|attachments\s*=|email_id\s*=|ForeignKey|cascade=' backend/db/models.py

echo '== TEST SELECTION REFERENCES =='
rg -n -C 3 'test_import_fixture_attachment_integrity|pytest.*postgres|postgres.*pytest| -m postgres|--marker' . \
  -g '!**/.git/**' -g '!**/node_modules/**' -g '!**/dist/**' | sed -n '1,240p'

echo '== EXACT LOGGING DIFFERENCE: `#1612` BASE VS HEAD =='
git diff --word-diff=plain 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc -- backend/import_fixtures.py | rg -n -C 2 'except|logger\.(error|warning|info)|Exception' || true

echo '== EFFECTIVE FUNCTIONAL DELTA COMPARISON =='
git diff --ignore-space-at-eol --unified=0 042b0c70531b229af3acbd0421a2f23098d848b3 0b83e7062d0829f4b8b6771242b5b6ed0cd57ac7 -- backend/import_fixtures.py > /tmp/predecessor-importer.diff
git diff --ignore-space-at-eol --unified=0 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 23ab3653543240c9c194e98530bb793d037489bc -- backend/import_fixtures.py > /tmp/successor-importer.diff
echo '-- predecessor functional additions/removals excluding exception/log lines --'
rg -n -v '^[+-].*(except Exception as e|logger\.error|Failed to|Fixture .* failed)' /tmp/predecessor-importer.diff | sed -n '1,220p'
echo '-- successor functional additions/removals excluding exception/log lines --'
rg -n -v '^[+-].*(except Exception:|logger\.error|Fixture .* failed)' /tmp/successor-importer.diff | sed -n '1,220p'

Length of output: 60532

---

@seonghobae I reviewed exact head 23ab3653543240c9c194e98530bb793d037489bc.

I found no blocking source-level defect in the conflict resolution.

• #1612 commit 3da3ae8e60e1bb049f59ae86bfe82db12b7e3cc7 is an ancestor of the reviewed head.
• #1699 is not an ancestor. The reviewed head reapplies its effective importer, test, and doctoring delta on the #1612 lineage.
• backend/import_fixtures.py retains #1612 static failure logging. The parsing, body-embedding, attachment-embedding, and commit handlers do not interpolate exception values.
• The importer retains the attachment-source and transaction behavior: owner-scoped duplicate checks, rollback() before enrichment, a second duplicate check after enrichment, attachment persistence with embedding=None after attachment enrichment failure, and rollback on commit failure.
• The recording-session tests cover transaction release, duplicate short-circuiting, and source attachment retention.
• The PostgreSQL test is present and registered with @pytest.mark.postgres. Its cascade assertion matches Email.attachments with cascade="all, delete-orphan".

The PostgreSQL acceptance is not complete. GitHub reports no workflow run for this exact head. I found only CodeRabbit and Devin Review status contexts. I did not execute repository tests in this inspection. A successful exact-head execution of test_attachment_embedding_failure_persists_source_in_real_postgres against PostgreSQL with pgvector is still required.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.