Skip to content

fix(attachments): align deferred parse budget with import transport - #1469

Open
seonghobae wants to merge 11 commits into
developfrom
feat/naruon-attachment-parse-64m
Open

fix(attachments): align deferred parse budget with import transport#1469
seonghobae wants to merge 11 commits into
developfrom
feat/naruon-attachment-parse-64m

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Customer outcome

Attachments larger than 20 MiB are no longer rejected by a hidden deferred-parser limit after the authenticated 64 MiB import transport accepts them. Unsupported binary types remain visible as metadata-only evidence with a concrete parser status; this PR does not claim to parse them.

Change

  • Align MAX_ATTACHMENT_PARSE_SOURCE_BYTES with the existing 64 MiB email import budget.
  • Preserve fail-closed unsupported_content_type / unsupported_binary behavior without raw bytes.
  • Add ADR-0006 and doctoring traceability with APA 7 references.
  • Add a regression assertion for the former 20 MiB boundary.

Verification

  • python -m ruff check backend/services/attachment_parser.py backend/tests/test_attachment_parser.py
  • PYTHONPATH=. pytest backend/tests/test_attachment_parser.py -q — 18 passed
  • git diff --check

Scope boundary

This is an ingestion-contract fix. It does not add a new binary parser dependency, change unsupported-format handling, expose raw bytes, or change database schema. A future parser for an unsupported format requires its own sandbox, provenance, dependency, and exact-head regression evidence.

Research


Open in Devin Review

Summary by CodeRabbit

  • Bug Fixes

    • Increased the attachment parsing limit to 64 MiB for larger supported attachments.
    • Preserved fail-closed handling for unsupported binary formats and oversized content.
    • Enforced a 20 MiB limit for direct PDF processing requests.
    • Improved oversized PDF request handling with clear failure statuses and diagnostic logging instead of leaving attachments pending.
  • Documentation

    • Documented attachment size limits, processing outcomes, deferred PDF handling, and supported follow-up actions.

@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 13:16
devin-ai-integration[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Approval pending

CodeRabbit has no unresolved comments, but it could not review the latest commit because the review limit was reached. Follow the review guidance in this comment to continue.

📝 Walkthrough

Walkthrough

The attachment parser source limit is increased from 20 MiB to 64 MiB. The NewsDOM client rejects PDF payloads above 20 MiB before network activity. The worker records provider payload-size failures. ADRs and related documentation define the bounded, fail-closed contract.

Changes

Attachment Parse Contract

Layer / File(s) Summary
Parser limit and admission contract
backend/services/attachment_parser.py, backend/tests/test_attachment_parser.py, docs/doctoring/bounded-attachment-parse-source-contract.md, docs/adr/0006-bounded-attachment-parse-source-contract.md
The parser limit is set to 64 MiB. Tests verify payloads above 20 MiB. Documentation defines bounded PDF retention, size-limit results, and metadata-only handling for unsupported binaries.
NewsDOM upload guard and failure handling
backend/services/newsdom_client.py, backend/services/newsdom_worker.py, backend/api/data.py, backend/tests/test_newsdom_client.py, backend/tests/test_newsdom_worker.py
The NewsDOM client rejects PDF payloads above 20 MiB before network activity. The worker records provider_payload_size_exceeded and marks the attachment as failed. Shared validation uses the NewsDOM upload limit.
Contract decisions and release records
docs/adr/0005-bounded-pdf-dom-upload-contract.md, docs/adr/0006-bounded-attachment-parse-source-contract.md, docs/adr/README.md, CHANGELOG.md
The ADRs, ADR index, and changelog record the separate 20 MiB PDF upload and 64 MiB attachment parse-source contracts.

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

Merge Risk: 🔵 Low · up to 19305

The PR raises the deferred attachment parse limit to match the 64 MiB import transport and preserves explicit unsupported-format statuses. It is mergeable with owner awareness or follow-up for remaining changelog wording corrections and the warning-level output behavior.

Suggested reviewers: cursoragent

Sequence Diagram(s)

sequenceDiagram
  participant AttachmentParser
  participant NewsdomWorker
  participant NewsdomClient
  AttachmentParser->>NewsdomWorker: provide deferred PDF payload
  NewsdomWorker->>NewsdomClient: request_pdf_dom(payload)
  NewsdomClient-->>NewsdomWorker: raise NewsdomPayloadTooLargeError above 20 MiB
  NewsdomWorker->>NewsdomWorker: record provider_payload_size_exceeded and failed status
Loading
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: aligning the deferred attachment parse budget with the authenticated import transport limit.
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.
✨ 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 feat/naruon-attachment-parse-64m

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

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head verification note (dd1fb33c6ca9fba7d94eed2a13eb3933c60149d3): focused parser tests passed (18 passed), Ruff and git diff --check passed. The full backend suite reached 1832 passed, 2 skipped, 2 failed; both failures are the same pre-existing PostgreSQL smoke-seed root cause (email_records.is_read omitted from raw INSERTs), isolated in dependent PR #1468. This PR keeps that unrelated fixture repair out of the attachment contract. Hosted exact-head Checks remain authoritative.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current head 8bd9c0c60ae05b6161c410240aca7a2d5b05a9d2 only corrects ADR wording so the PR is self-contained before #1427 integrates the separate PDF DOM ADR. Parser Ruff, git diff --check, and backend/tests/test_attachment_parser.py pass (18 passed) on this exact head. The prior full-suite result remains code-equivalent because this commit changes documentation only; hosted exact-head Checks remain authoritative.

devin-ai-integration[bot]

This comment was marked as resolved.

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

PR governance metadata gate update for 6d6e499d763dcbde6fb9896d9e866b9691b5206b: no current blocking failures remain.

PR governance metadata gate is waiting on current-head requirements; see the latest check for pending reasons.

@seonghobae

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

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.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Current-head disposition for de6d712: the four Devin threads are informational. The parser bound now matches the authenticated 64 MiB import ceiling; retained base64 expansion and the external NewsDOM capacity are recorded as the object-storage/sidecar integration gaps. The worker maps sidecar request/response errors to visible recognition failure rather than leaving an unusable payload falsely parsed; only an unconfigured provider remains pending for retry. The oversized test is an intentional boundary test. No additional source change is required in this bounded slice.

@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 14:57
@seonghobae

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

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.

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Maintainer exact-head validation

@seonghobae

Copy link
Copy Markdown
Contributor Author

Exact-head disposition for the prior CodeRabbit changelog observation: current head 575b0c2 already states the bounded range explicitly in CHANGELOG.md: attachments larger than 20 MiB and no larger than 64 MiB, with unsupported binaries remaining metadata-only. ADR-0006 and the doctoring contract state the same boundary. No further source or documentation change is warranted for that observation. The remaining merge blockers are current-head hosted review/security evidence and the protected approval requirement.

devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 25, 2026 20:17
@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 20:19
devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 25, 2026 20:39
@seonghobae
seonghobae enabled auto-merge (squash) August 25, 2026 20:43
@seonghobae

Copy link
Copy Markdown
Contributor Author

/coderabbit review

@seonghobae

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

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.

@seonghobae
seonghobae dismissed coderabbitai[bot]’s stale review August 25, 2026 22:37

Superseded by current head 258ac41: the requested change is implemented verbatim — the NewsdomPayloadTooLargeError handler now records via logger.info with the operational-information rationale, preserving parse status, error code provider_payload_size_exceeded, and RESULT_FAILED return. All threads resolved; required checks green.

coderabbitai[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 26, 2026 00:11
CodeRabbit round-2 finding: the entry named only unsupported_content_type;
the contract also defines unsupported_binary. Name both so the changelog
matches the persisted-metadata contract while preserving the size-limit and
parse_size_limit_exceeded fail-closed descriptions.
@seonghobae

Copy link
Copy Markdown
Contributor Author

Addressed the CodeRabbit round-2 finding in 6d6e499: CHANGELOG.md now names both contract statuses (unsupported_content_type and unsupported_binary) for unsupported binaries, preserving the size-limit descriptions and fail-closed parse_size_limit_exceeded behavior.

@seonghobae
seonghobae dismissed coderabbitai[bot]’s stale review August 26, 2026 09:23

Superseded by head 6d6e499: the single requested change is implemented verbatim — CHANGELOG.md now names both contract statuses (unsupported_content_type and unsupported_binary) for unsupported binaries, preserving size-limit descriptions and fail-closed parse_size_limit_exceeded behavior. Thread resolved with the diff reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant