fix(attachments): index reparsed attachment content into the content graph - #1501
Conversation
…graph apply_reparsed_result landed a fresh classification result onto the Attachment row but never indexed the recognized content into the content graph, unlike the initial email-import path (_append_email_content_graph). A previously-quarantined attachment that later reparses to "parsed" therefore stayed invisible to content-graph-backed search/AI-hub features even after successful recognition. Flagged as informational by Devin Review on naruon#1486, confirmed real but out of scope there, and closed here as the tracked follow-up. apply_reparsed_result now calls a new _append_reparsed_attachment_content_graph whenever the reparse result lands on "parsed". It reuses the same services.content_graph.parse_content helper the import path already calls, plus a newly shared content_graph_source_record_uid (promoted from a private function in email_import_service.py to a public helper in services/content_graph/parser.py that both call sites import) -- one indexing path, one identity convention, two callers. Since a persisted attachment's original position among its email's siblings is not reliably reproducible post-import, the reparse path keys source_record_uid on the attachment's permanent attachment_uid alone instead of the import path's message-id + list-position convention, and sets the new records' email_id directly from the attachment's already-loaded email_id column rather than through a transient Email relationship append. Updates docs/adr/0005-attachment-content-type-quarantine.md's Revisions section and CHANGELOG.md per repo convention.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR indexes successfully reparsed attachments into the content graph, refreshes embeddings from resolved parse text, centralizes source UID generation, hardens the legacy email read-state migration, and adds tests, CI, configuration, changelog, and ADR updates. ChangesAttachment reparse and content-graph flow
Legacy migration handling
Validation and project records
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to Successfully reparsed attachments will now reach content-graph search and AI features, but the current change still needs follow-up before merge because its migration uses nonstandard raw DDL, migration tests may hide execution failures, and concurrent workers on non-PostgreSQL deployments could leave reparsed attachments without indexed content. Sequence Diagram(s)sequenceDiagram
participant AttachmentReparseWorker
participant ContentGraphParser
participant EmailImportService
participant AsyncSession
AttachmentReparseWorker->>ContentGraphParser: Parse reparsed attachment content
ContentGraphParser-->>AttachmentReparseWorker: Return ReparseOutcome
AttachmentReparseWorker->>EmailImportService: Generate embedding from embedding_source_text
AttachmentReparseWorker->>AsyncSession: Refresh relationships and commit graph records
Suggested reviewers: 🚥 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 |
|
PR governance metadata gate update for PR governance metadata gate is waiting on current-head requirements; see the latest check for pending reasons. |
|
The Generated by Claude Code |
|
Draft PR일 때 |
|
맞습니다 — checks/tests가 이미 정상이었으므로 draft를 유지할 이유가 없었습니다. 확인 후 조치했습니다:
Generated by Claude Code |
|
그리고 not ready 라고 띄운 Workflow는 고쳐야지. |
|
@coderabbitai review (Noting for the record: since this PR's base is Generated by Claude Code |
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Re "not ready라고 띄운 Workflow는 고쳐야지": I traced Also picked up and validated
No new checks have posted yet for Generated by Claude Code |
|
@coderabbitai review |
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Exact-head PostgreSQL evidence for |
|
@coderabbitai review |
|
@coderabbitai review Generated by Claude Code |
|
🧠 Learnings used
|
# Conflicts: # pytest.ini
…tants _UPGRADE_SQL/_DOWNGRADE_SQL became f-strings to interpolate the fixed _IS_READ_PROVENANCE_MARKER literal, which Bandit's hardcoded_sql_expressions check flags as a possible SQL injection vector. Neither string interpolates an identifier or external input (only that module-level constant), matching the safety property the module docstring already documents -- mark both false positives with the repo's established inline `# nosec BXXX` convention rather than leaving CI red.
Status update: code/review side is fully done; CI stuck queued 4.5h+ on current head, looks orphaned not just slowSince the last congestion update (10:22 UTC, head
All 18 review threads are resolved, Devin Review reports 0 issues on the current head, CodeRabbit is green (skipped by design — auto-review is disabled for non-default base branches), and The remaining blocker is purely infrastructure: all 10 check runs on Per repo policy I won't push an empty commit or close/reopen to force a re-trigger. If a maintainer has Actions access, manually re-running the stuck Generated by Claude Code |
|
@coderabbitai review (The governance gate just re-evaluated on the current head Generated by Claude Code |
|
Your plan includes PR reviews subject to rate limits. More reviews will be available in 30 minutes. |
|
@coderabbitai review Generated by Claude Code |
|
🧠 Learnings used✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit's fresh review landed — both findings addressed, but its formal review state is now stuck staleCodeRabbit's retried review (triggered after the earlier
Both threads resolved; no code changes needed for either. However, CodeRabbit's formal top-level review (the one that sets Separately and unrelated to any of the above: this is moot for merging right now anyway, since all 10 Generated by Claude Code |
… new downgrade test test_legacy_email_read_state_downgrade_preserves_a_preexisting_column wrapped its whole body -- table setup, migration execution, and assertions -- in a try/except that treats connection-family errors as "PostgreSQL unavailable, skip", matching several older tests in this file. CodeRabbit correctly pointed out its immediate neighbor, test_legacy_email_read_state_real_postgres_smoke, already uses the safer pattern: skip only on an initial `SELECT 1` connectivity probe, then let every later failure propagate and fail the test instead of silently skipping it. Match that pattern here too, so a real migration or assertion bug can't get masked as an unrelated environment skip. Verified against a real PostgreSQL 16 + pgvector database: the 6 migration-related postgres-marked tests pass, and a full-suite run passes (1951 passed, 3 skipped) apart from one pre-existing, already-documented test-ordering artifact unrelated to this change (test_0001_initial_upgrade... drops email_records mid-suite when run before test_attachment_reparse_worker tests in the same process; confirmed unaffected by re-running that test in isolation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4GKADWJyd8NToEAK5SH6Q
|
@coderabbitai review Generated by Claude Code |
|
🧠 Learnings used
|
CodeRabbit approved — review side is now fully clearCodeRabbit's incremental review system auto-approved (pullrequestreview-5083403237) once its one remaining finding was withdrawn, and confirmed the connectivity-probe fix on the follow-up commit ( Devin Review also reports 0 issues on the latest head, and all review threads are resolved. Only remaining blocker: the new push reset the CI queue (as any push does), so all 10 Generated by Claude Code |
…-commercialization-afow1j' into claude/attachment-reparse-content-graph-index # Conflicts: # CHANGELOG.md
Merging origin/claude/noema-contextualwisdomlab-commercialization-afow1j (the cursor/retry-set starvation fix, which added _LiveReparsePendingSession in tests/test_attachment_reparse_worker.py) into this PR's branch (which added the session.refresh(attachment, attribute_names=[...]) call in _sweep_attachments, needed to eager-load relationships before apply_reparsed_result appends content-graph rows through them) surfaced an integration gap neither branch could have caught alone: the merged production code now calls session.refresh() on every sweep, but _LiveReparsePendingSession (used by two multi-sweep scheduling tests) never implemented it, since it predates that call. Add a no-op refresh(), matching the sibling _SequenceSession fake's pattern. Confirmed via RED (test_sweep_does_not_starve_rows_behind_many_failing_rows and test_sweep_rediscovers_a_row_reverted_to_pending_behind_the_cursor both failed with AttributeError before this fix) -> GREEN (full backend suite: 1920 passed, 43 skipped; ruff clean). One unrelated timing-sensitive test (test_main_kills_original_process_group_on_timeout) failed once under full-suite load and passed in isolation and on a second full-suite run -- a pre-existing flake, not caused by this merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4GKADWJyd8NToEAK5SH6Q
Merge conflict resolved (base branch advanced)This PR's base, Resolution: merged the base branch into this PR's head with a normal merge commit ( Integration gap the merge surfaced (fixed in Validation: RED confirmed first ( New head: Generated by Claude Code |
seonghobae
added
documentation
ChatGPT Codex Connector
ff8807a
into
claude/noema-contextualwisdomlab-commercialization-afow1j
2 participants
Description
Follow-up to naruon#1486 (ADR-0005 attachment quarantine/reparse feature), stacked
on that PR's branch since these files don't exist on
developyet.backend/services/attachment_reparse_worker.py::apply_reparsed_resultlands a freshAttachmentParseResultonto an existingAttachmentrow after a successful reparse,but never indexed the recognized content into the content graph — unlike the initial
email-import path (
email_import_service.py::_append_email_content_graph), whichalready builds a
ContentNodeRecord/ContentSegmentRecordgraph for an attachmentthat parses cleanly on first import.
AttachmentParseResultcarries the sameparse_contentfield the import path indexes; the reparse path just never read it.Devin Review flagged this as informational ("confirm this is intended") on naruon#1486.
Verified real but out of scope for that PR to fix inline, and queued as this dedicated
follow-up: a previously-quarantined attachment (e.g. a MIME-mismatched file that
initially failed classification) that later reparses successfully stayed invisible to
content-graph-backed search/AI-hub features even after successful recognition.
apply_reparsed_resultnow calls a new_append_reparsed_attachment_content_graphwhenever the reparse result lands on
"parsed". It reuses the sameservices.content_graph.parse_contenthelper the import path already calls, plus anewly shared
content_graph_source_record_uid(promoted from a private function inemail_import_service.pyto a public helper inservices/content_graph/parser.pythat both call sites now import) — one indexing path, one identity convention, two
callers, not a second path.
Since a persisted attachment's original position among its email's siblings is not
reliably reproducible post-import, the reparse path keys
source_record_uidon theattachment's permanent
attachment_uidalone instead of the import path'smessage-id + list-position convention, and sets the new records'
email_iddirectlyfrom the attachment's already-loaded
email_idcolumn rather than through atransient
Emailrelationship append (the attachment here is already a persistedrow, unlike at import time).
Fixes: none (follow-up to a review comment, not a tracked issue)
Type of change
Checklist:
docs/adr/0005-attachment-content-type-quarantine.mdRevisions section,CHANGELOG.md)backend/tests/test_attachment_reparse_worker.py: 3 new tests — indexes content graph on a"parsed"reparse, skips indexing on blank"parsed"content, skips indexing on a non-"parsed"reparse)PYTHONPATH=. python -m pytest -q→ 1908 passed, 40 skipped;ruff check .cleanDeveloper experience
services/content_graph/parser.pygains one new public function(
content_graph_source_record_uid), re-exported fromservices/content_graph/__init__.py.email_import_service.pynow imports it instead of defining its own private copy —same hash output, same call sites, no behavior change there (covered by the existing
test_email_import_service.pyandtest_content_graph_parser.pysuites, both stillgreen).
attachment_reparse_worker.pygains one new private helper,_append_reparsed_attachment_content_graph, called fromapply_reparsed_result.User experience
A previously-quarantined attachment that a workspace member requests reparse for
(
POST /api/data/attachments/{attachment_uid}/reparse-intent) and that reparsessuccessfully now shows up in content-graph-backed search and AI-hub features, the
same as any attachment that parsed cleanly on first import. No API or schema change.
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation