fix(attachments): index reparsed attachment content into the content graph

.github/workflows/app-ci.yml

@@ -76,12 +76,14 @@ jobs:
         # tests/ (repo root) holds workflow/YAML contract tests, e.g.
         # test_stacked_pr_workflow_contract.py. The backend job's own pytest
         # invocation above runs from backend/ and never collects this
-        # directory, so it needs its own explicit step.
+        # directory, so it needs its own explicit step. Mirrors that step's
+        # own log scan: a warning-class message pytest prints without
+        # actually failing the run must not be accepted as clean evidence.
         run: |
           set -o pipefail
           python -m pytest -q tests 2>&1 | tee root_pytest_output.log
           if grep -qiE 'timeout|fatal|warn|denied' root_pytest_output.log; then
-            echo "::error::Tests produced Timeout, Fatal, Warn, or Denied outputs"
+            echo "::error::Root governance contract tests produced Timeout, Fatal, Warn, or Denied outputs"
             exit 1
           fi
 

CHANGELOG.md

@@ -1,4 +1,64 @@
 ## [Unreleased]
+- **(CodeRabbit 리뷰 대응, naruon#1501) 첨부파일 reparse content-graph 색인 후속(바로 아래 항목)의
+  전체 리뷰에서 실제 결함 2건이 나와 모두 고쳤습니다.** (1) reparse 임베딩 재생성이
+  resolved parse 소스 텍스트 대신 `attachment.content`에서 값을 읽고 있었습니다.
+  `apply_reparsed_result`는 `result.content`(마크업을 걷어낸 *display* 문자열)가 비어있지
+  않을 때만 `attachment.content`를 덮어쓰는데, `"parsed"` 결과의 display 텍스트는 빈 문자열로
+  스트립되지만 raw `result.parse_content`는 그렇지 않은 경우(예: 보이는 텍스트 노드 없이
+  마크업만 있는 첨부파일) `attachment.content`가 base64로 인코딩된 채 그대로 남아있어,
+  임베딩이 실제 재파싱된 텍스트가 아니라 base64 노이즈로부터 생성됐습니다 — content graph는
+  올바른 텍스트로 색인됐는데(`_append_reparsed_attachment_content_graph`가 이미
+  `result.parse_content or result.content`를 직접 resolve했으므로, import 시점의
+  `email_import_service._extract_and_generate_embeddings`와 동일한 resolve 방식), 임베딩만
+  어긋난 것입니다. `process_reparse_pending_attachment`는 이제 단순 상태 문자열 대신
+  `ReparseOutcome(parse_status, embedding_source_text)`를 반환해, 그 동일한 resolved 텍스트를
+  attachment 행에서 다시(불안정하게) 유도하지 않고 임베딩 재생성으로 명시적으로 전달합니다.
+  신규 테스트:
+  `test_reparse_that_lands_on_parsed_with_markup_only_content_still_embeds_parse_content`.
+  (2) `0011_email_read_state.py`의 `downgrade()`가 legacy `emails` 테이블과 `is_read`
+  컬럼이 둘 다 있으면 무조건 컬럼을 drop했습니다 — 이 리비전보다 먼저 존재했던(그래서 이
+  리비전의 `NOT EXISTS` 가드가 건드리지 않은) 동명의 `is_read` 컬럼까지 데이터째 파괴할 수
+  있었습니다. `upgrade()`가 이제 자신이 만든 컬럼에 `COMMENT ON COLUMN` provenance 마커
+  (`_IS_READ_PROVENANCE_MARKER = "0011_email_read_state:added"`)를 남기고, `downgrade()`는
+  `col_description`으로 그 마커가 정확히 있을 때만 drop합니다 — 이 리비전이 추가한 것만
+  drop하고 그 외에는 손대지 않습니다. 신규 real-Postgres 테스트:
+  `test_legacy_email_read_state_downgrade_preserves_a_preexisting_column`(legacy
+  `emails.is_read` 컬럼에 데이터를 미리 심어두고 upgrade→downgrade를 실행해 컬럼과 데이터가
+  모두 살아남는지 확인). 추가로 `email_import_service._generate_source_embedding`을 공개
+  `generate_source_embedding`으로 개명(CodeRabbit nitpick): `content_graph_source_record_uid`,
+  `append_knowledge_graph_edges`에 이어 `attachment_reparse_worker.py`가 가져다 쓰는 세
+  번째 cross-module 헬퍼이므로, 모든 cross-module 헬퍼가 public일 때 모듈 경계가 일관됩니다.
+  검증: 전체 백엔드 스위트 1911 passed/43 skipped(`DATABASE_URL` 미설정, CI와 동일), 이번
+  수정이 건드린 테스트는 전부 실제 PostgreSQL 16 + pgvector에 대해 단독 실행 시 통과 — 같은
+  실제 DB에 대해 스위트 전체를 한 프로세스로 돌리면 이 PR에서 이미 보고된 기존 cross-file
+  test-ordering 실패 1건(`test_0001_initial_upgrade_succeeds_against_a_fresh_database`가
+  스위트 중간에 `email_records`를 drop·재생성)이 재현되지만, 이번 수정과는 무관합니다. ruff
+  clean.
+- **(Devin 리뷰 대응, naruon#1486 후속) 첨부파일 reparse가 성공적으로 재인식된 콘텐츠를
+  초기 import 경로와 달리 content graph에 색인하지 않던 gap을 고쳤습니다.**
+  `services/email_import_service.py::_append_email_content_graph`는 첨부파일이 첫
+  import에서 정상 파싱되면 `ContentNodeRecord`/`ContentSegmentRecord` 그래프를
+  만들지만, `attachment_reparse_worker.py::apply_reparsed_result`는 `Attachment`
+  행 자체 컬럼만 갱신했습니다 — 격리(quarantine)됐던 첨부파일이 나중에 reparse로
+  `"parsed"`가 되어도 content-graph 기반 검색/AI-hub 기능에는 계속 보이지
+  않았습니다(`AttachmentParseResult`가 import 경로와 동일한 `parse_content` 필드를
+  이미 들고 있었음에도). `apply_reparsed_result`가 결과 `parse_status`가
+  `"parsed"`일 때 새 `_append_reparsed_attachment_content_graph`를 호출하도록
+  추가했습니다 — import 경로가 이미 쓰는 `services.content_graph.parse_content`와,
+  새로 공개 API로 옮긴 `content_graph_source_record_uid`(원래
+  `email_import_service.py`의 private 함수였던 것을
+  `services/content_graph/parser.py`로 옮겨 두 호출부가 공유)를 그대로 재사용해
+  색인 경로를 두 개로 만들지 않았습니다. 영속화된 attachment가 자신이 속한
+  이메일의 첨부파일 목록에서 원래 몇 번째였는지는 신뢰성 있게 재현할 수 없으므로,
+  reparse 경로의 `source_record_uid`는 import 경로의 message-id + 목록 위치
+  조합 대신 attachment의 영구 `attachment_uid` 하나로만 구성하고, 새 레코드의
+  `email_id`는 (import 경로처럼 아직 저장되지 않은 `Email`을 통한 관계 append로
+  간접 설정하는 대신) 이미 영속화된 attachment 행의 `email_id` 컬럼에서 직접
+  가져옵니다. 빈 문자열로만 파싱되는 `"parsed"` 결과(공백만 있는 첨부파일 등)는
+  기존 import 경로와 동일하게 색인을 건너뜁니다. 신규 테스트 3개
+  (`test_reparse_that_lands_on_parsed_indexes_the_content_graph`,
+  blank-content 스킵, non-parsed 스킵). 검증: 전체 백엔드 스위트 1908
+  passed/40 skipped, ruff clean.
 - **(Devin 리뷰 대응, 🔴 실제 결함, `backend/services/attachment_reparse_worker.py:346-352`)
   커서보다 낮은 id(또는 document의 경우 더 이른 `(created_at, document_id)`)를 가진
   행이 나중에 외부에서 다시 pending 상태로 되돌려지면, 커서+재시도-집합 설계로는

backend/alembic/versions/0011_email_read_state.py

@@ -1,29 +1,116 @@
 """Add is_read to emails (IMAP \\Seen read state).
 
 Existing rows default to read so historical/file imports do not surface as unread.
+
+Deliberate exception to this repo's "Alembic migrations use structured
+operations (``op.create_index``, ...), never ``sa.text(f"...")`` DDL" rule
+(``AGENTS.md``/``CLAUDE.md``): ``upgrade()``/``downgrade()`` below use
+``op.execute()`` with the module-level ``_UPGRADE_SQL``/``_DOWNGRADE_SQL``
+constants instead of a structured ``op.*`` call. That rule's actual target is
+DDL built from interpolated identifier strings (an injection-safety concern);
+these constants interpolate only ``_IS_READ_PROVENANCE_MARKER``, a fixed
+module-level literal, never an identifier or a value built from a variable,
+external input, or runtime state -- the same safety property a structured
+call would have. The reason a structured call isn't used is different: this
+migration's behavior must be conditional on whether the legacy ``emails``
+table exists, evaluated at apply time (see the comment on ``_UPGRADE_SQL``
+below for why that check cannot live in Python), and no structured Alembic
+operation expresses "run this DDL only if a runtime condition holds" -- a
+``DO $$ ... $$`` block is the correct primitive for that, not a workaround
+for one.
 """
 
 from alembic import op
-import sqlalchemy as sa
 
 # revision identifiers, used by Alembic.
 revision = "0011_email_read_state"
 down_revision = "0009_project_graph_projection"
 branch_labels = None
 depends_on = None
 
+# Fresh installations materialize the current ``email_records`` model in the
+# 0001 baseline, including ``is_read``. This historical side branch only
+# applies to databases that still carry its legacy ``emails`` table.
+#
+# The condition has to be evaluated in SQL, not Python: offline SQL
+# generation (``alembic upgrade --sql``, a real flag ``scripts/migrate_db.py``
+# exposes) has no live connection to introspect with and no specific target
+# database to ask "does this legacy table exist" at generation time either --
+# the same static script is meant to later be applied by a DBA against
+# whichever database they choose, fresh-install or legacy. A Python-side
+# check (``sa.inspect(op.get_bind())``) can only ever answer that question
+# for one hypothetical target chosen at generation time, so it is wrong for
+# the other: skip unconditionally and the column silently never gets added
+# for a legacy database that applies the generated script (while
+# ``alembic_version`` still advances, permanently hiding the gap); inspect
+# online and bake in one fixed answer and the same script fails outright
+# against the other kind of target. A ``DO $$ ... $$`` block defers the
+# check to apply time instead, so the one generated script is correct
+# against either kind of target, online or offline-then-applied-later alike.
+#
+# ``to_regclass('emails')`` (not ``information_schema.tables`` by bare
+# ``table_name``) deliberately: the unqualified ``ALTER TABLE emails`` below
+# resolves through the connection's ``search_path``, and ``to_regclass``
+# resolves an unqualified name exactly the same way, returning NULL if it
+# doesn't. ``information_schema.tables`` filtered only by ``table_name``
+# ignores ``search_path`` entirely and matches a same-named table in *any*
+# schema the connecting role can see -- on a deployment with more than one
+# accessible schema, that could find an unrelated ``emails`` table outside
+# the search path while the unqualified ``ALTER TABLE emails`` targets a
+# different (or no) table, passing the guard for the wrong relation or
+# aborting the migration outright. Resolving both the check and the DDL
+# through the same name lookup makes that mismatch structurally impossible.
+#
+# ``COMMENT ON COLUMN emails.is_read`` tags the column with a provenance
+# marker (``_IS_READ_PROVENANCE_MARKER``) the moment upgrade() actually adds
+# it. downgrade() only drops the column when that exact marker is present
+# (CodeRabbit, naruon#1501): an ``emails.is_read`` column that already
+# existed before this revision ran -- from some other, unrelated origin --
+# would upgrade()'s ``NOT EXISTS`` guard correctly leave alone, but an
+# unconditional ``DROP COLUMN IF EXISTS`` on downgrade would still destroy it
+# and its data, since a downgrade has no other way to tell "I added this"
+# apart from "this happens to be present". Checking the marker via
+# ``col_description`` makes downgrade drop only what this exact revision's
+# upgrade created.
+_IS_READ_PROVENANCE_MARKER = "0011_email_read_state:added"
+_UPGRADE_SQL = f"""
+DO $$
+BEGIN
+    IF to_regclass('emails') IS NOT NULL AND NOT EXISTS (
+        SELECT 1 FROM pg_attribute
+        WHERE attrelid = to_regclass('emails')
+        AND attname = 'is_read'
+        AND NOT attisdropped
+    ) THEN
+        ALTER TABLE emails ADD COLUMN is_read boolean NOT NULL DEFAULT true;
+        COMMENT ON COLUMN emails.is_read IS '{_IS_READ_PROVENANCE_MARKER}';
+    END IF;
+END $$;
+"""  # nosec B608
+
+_DOWNGRADE_SQL = f"""
+DO $$
+BEGIN
+    IF to_regclass('emails') IS NOT NULL AND EXISTS (
+        SELECT 1 FROM pg_attribute
+        WHERE attrelid = to_regclass('emails')
+        AND attname = 'is_read'
+        AND NOT attisdropped
+    ) AND col_description(to_regclass('emails'), (
+        SELECT attnum FROM pg_attribute
+        WHERE attrelid = to_regclass('emails')
+        AND attname = 'is_read'
+        AND NOT attisdropped
+    )) = '{_IS_READ_PROVENANCE_MARKER}' THEN
+        ALTER TABLE emails DROP COLUMN IF EXISTS is_read;
+    END IF;
+END $$;
+"""  # nosec B608
+
 
 def upgrade() -> None:
-    op.add_column(
-        "emails",
-        sa.Column(
-            "is_read",
-            sa.Boolean(),
-            nullable=False,
-            server_default=sa.text("true"),
-        ),
-    )
+    op.execute(_UPGRADE_SQL)
 
 
 def downgrade() -> None:
-    op.drop_column("emails", "is_read")
+    op.execute(_DOWNGRADE_SQL)