Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

Specs: text-extraction-eml (#1438)#1495

Merged
rjzondervan merged 2 commits into
developmentfrom
feat/1438/text-extraction-eml
May 12, 2026
Merged

Specs: text-extraction-eml (#1438)#1495
rjzondervan merged 2 commits into
developmentfrom
feat/1438/text-extraction-eml

Conversation

@rjzondervan

Copy link
Copy Markdown
Member

Summary

Adds the OpenSpec change directory for text-extraction-eml — extending TextExtractionService with EML support via zbateson/mail-mime-parser. Two outputs share the parser:

  • Flat plain-text extraction (headers + body + recursively-extracted attachment text inline) — used for entity detection.
  • A new public parseEmlStructured() returning headers + both text/plain and text/html body parts + attachments-with-bytes (recursive nestedEml capped at depth 3) — used for downstream PDF assembly.

Tightly scoped — this PR is specs only, no implementation. It also does not retrofit the broader TextExtractionService surface, which is currently uncovered by an OpenSpec capability.

Refs: #1438
Pair: ConductionNL/docudesk#135 — DocuDesk's eml-pdf-assembly (hard dep) and anonymise-output-as-pdf-by-default (soft dep) depend on this.

Test plan

  • Review openspec/changes/text-extraction-eml/{proposal,design,tasks}.md and the text-extraction-eml/spec.md delta
  • Confirm parseEmlStructured() return shape matches the DocuDesk-side consumer expectations in PR check isset items type #135 (EmlPdfAssemblyService)
  • Confirm depth-3 nested-EML cap is acceptable
  • openspec validate (when the implementation PR lands)

Adds the spec set for extending TextExtractionService with EML support
via zbateson/mail-mime-parser. Two outputs share the parser:

- Flat plain-text extraction (headers + body + recursively-extracted
  attachment text inline) for entity detection.
- A new public parseEmlStructured() returning headers + both text/plain
  and text/html body parts + attachments-with-bytes (recursive nestedEml
  capped at depth 3) for downstream PDF assembly.

Tightly scoped: does not retrofit the broader TextExtractionService
surface (currently uncovered by an OpenSpec capability).

Refs: #1438
Pair: ConductionNL/docudesk#135 (eml-pdf-assembly hard dep,
anonymise-output-as-pdf-by-default soft dep).
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 2b3042c

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 153/153
npm ✅ 598/598
PHPUnit
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-12 09:22 UTC

Download the full PDF report from the workflow artifacts.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(inline-only comment review — verdict follows in a separate REQUEST_CHANGES review)

Comment thread openspec/changes/text-extraction-eml/specs/text-extraction-eml/spec.md Outdated
Comment thread openspec/changes/text-extraction-eml/specs/text-extraction-eml/spec.md Outdated
Comment thread openspec/changes/text-extraction-eml/specs/text-extraction-eml/spec.md Outdated

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 blocker requires a fix — the content encoding contract is ambiguous and will corrupt DocuDesk PDF/A-3 attachments. Five concerns (body-as-array vs value-object, MAY throw vs downstream contract, missing multipart/alternative scenario, PII logging not specified, and spec format vs writing-specs.md) — the format concern is repo-wide drift, surfaced here for visibility rather than as a PR blocker. Three minor polish suggestions. Spec design overall is solid: capability scope is tightly bounded, the two-path architecture (flat + structured) is well-justified, scenarios are comprehensive for the headers/recursion paths. CI Vue Quality (eslint) is failing but unrelated to this spec-only PR (no Vue files touched).

Blocker:
- B1: tighten EmlAttachment.content contract — MUST hold decoded binary
  bytes (via $part->getContent(), not getRawContent()); consumers MUST
  NOT base64-decode again. Prevents downstream PDF/A-3 attachment
  corruption in DocuDesk's eml-pdf-assembly.

Concerns:
- C1: fix scenario notation — body accessors switched from array form
  result.body['plainText'] to value-object form result.body->plainText
  to match the typed EmlBody declaration.
- C2: parseEmlStructured changed from MAY throw to MUST throw on
  irrecoverable malformed input — so DocuDesk's exception-driven
  fallback (eml-pdf-assembly → extractEml flat text) actually fires.
- C3: add multipart/alternative scenario for the flat path — confirms
  text/plain is preferred over text/html when both are present.
- C4: add no-PII-in-logs Requirement (ADR-005) with two Scenarios —
  parser-failure log lines and sanitised exception-message logging
  MUST NOT leak addresses / names / subjects / body content /
  attachment filenames.

Minors:
- M1: clarify depth-3 recursion semantics — depth is measured as the
  number of recursive parseEmlStructured calls from the root (root =
  depth 0, limit of 3 allows parses at depths 0, 1, 2, 3).
- M2: add SHOULD Requirement + Scenario for non-UTF-8 body charset
  transcoding (mb_detect_encoding + mb_convert_encoding to UTF-8).
- M3: state the missing-filename fallback format (attachment-<n>,
  1-indexed) on EmlAttachment, with a new Scenario covering it.

Tasks: update 3.4 (filename fallback + decoded-bytes), 3.9 (MUST
throw), and add 3.10 (PII-sanitised logging) + 3.11 (explicit
multipart/alternative preference). Add tests 5.6 (multipart pref),
5.7 (filename fallback), 5.8 (content encoding), 5.9 (PII log
redaction), 5.10 (MUST throw assertion).

Skipped: C5 (spec-format vs writing-specs.md) — repo-wide drift that
Wilco explicitly says does not block merge and needs maintainer
coordination.
@rjzondervan

Copy link
Copy Markdown
Member Author

@WilcoLouwerse — thanks for the thorough review. Addressed in dabba58cd:

Blocker

  • 🔴 B1EmlAttachment.content contract tightened: MUST be the decoded binary content (via \$part->getContent(), NOT getRawContent()); consumers MUST NOT base64-decode again. Explicitly calls out DocuDesk's PDF/A-3 attachment + data: URI consumers so the no-double-encode invariant is clear. tasks.md 3.4 mirrors this.

Concerns

  • 🟡 C1 — body accessors switched from array form (result.body['plainText']) to value-object form (result.body->plainText) across all four Scenarios on lines 88, 89, 96, 97 to match the declared typed EmlBody.
  • 🟡 C2 — Requirement reworded: parseEmlStructured MUST throw EmlParseException on irrecoverable malformed input (not MAY, not return null, not return a partial structure). Body explicitly cites DocuDesk's exception-driven fallback so the rationale is recorded. tasks.md 3.9 updated to MUST throw.
  • 🟡 C3 — added Scenario "multipart/alternative — text/plain is preferred when both parts are present" on the flat path. New tasks.md 3.11 makes the preference explicit in flatten(); test 5.6 covers it.
  • 🟡 C4 — added Requirement "extractEml and parseEmlStructured MUST NOT log PII (ADR-005)" with two Scenarios. Specifies what is forbidden (addresses, display names, Subject, body content, attachment filenames) and what is permitted (file ID, MIME type, exception class, sanitised structural detail). tasks.md 3.10 wires sanitisation in the catch/rethrow paths; test 5.9 verifies.

Minors

  • 🟢 M1 — depth-3 semantics clarified. Depth is the number of recursive parseEmlStructured calls from the root: root = depth 0; the limit of 3 allows parses at depths 0, 1, 2, 3, after which further nested EMLs have nestedEml: null. Scenario rewritten to match.
  • 🟢 M2 — added Requirement "Non-UTF-8 body parts SHOULD be transcoded to UTF-8" with an ISO-8859-1 → UTF-8 Scenario. Mirrors tasks.md 5.5.
  • 🟢 M3EmlAttachment.filename resolution order explicitly stated: Content-Disposition filename → Content-Type name → generated attachment-<n> (1-indexed). New Scenario covering the third fallback; test 5.7 added.

Skipped (intentionally)

  • 🟡 C5 (spec format vs writing-specs.md) — repo-wide drift that you marked as not blocking and needing maintainer coordination. Happy to address in a follow-up once OpenRegister decides whether to adopt the global convention.

Test coverage 5.6–5.10 added for multipart preference, filename fallback, content encoding integrity, PII-redacted logging, and the MUST-throw guarantee. Ready for re-review.

@WilcoLouwerse WilcoLouwerse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 9 prior findings resolved in dabba58B1 content-encoding, C1 body notation, C2 MUST throw, C3 multipart scenario, C4 PII/ADR-005, C5 deferred, M1 depth labels, M2 non-UTF-8 SHOULD, M3 filename fallback. Spec is clean; Newman failure is unrelated (docs-only change).

@rjzondervan rjzondervan merged commit fcb7f0f into development May 12, 2026
1 check failed
@rjzondervan rjzondervan deleted the feat/1438/text-extraction-eml branch May 12, 2026 10:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants