fix(unstructured): keep table structure instead of flattening it to prose

[cbcoutinho]

A Table element's text is every cell run together in one line, so the
row/column association is lost -- a questionnaire becomes an unsearchable
wall of words. The grid only survives in metadata.text_as_html, which the
processor was discarding.

Render that HTML as a markdown table via the existing html_to_markdown
helper (markdownify is already a dependency). Measured on a 4-column
questionnaire: 0 markdown table rows before, 18 after.

parse_mode now reports "markdown" once a table has been rendered as one,
so utils/document_parser labels the result correctly; documents without
tables keep the previous "text_only" path byte for byte.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Claude finished @cbcoutinho's task in 2m 12s —— View job

---

Review complete ✅

• Gather context (diff, files, prior review rounds)
• Review nextcloud_mcp_server/document_processors/unstructured.py
• Review nextcloud_mcp_server/utils/html.py
• Review nextcloud_mcp_server/vector/html_processor.py
• Review tests/unit/test_unstructured_tables.py
• Post final review feedback

This PR is on its third round; the two prior rounds already caught and fixed the two substantive issues (the Table-type gate, and the layering violation from importing vector.html_processor). I re-read the diff against origin/master with that history in mind rather than re-litigating it. No blocking findings.

What the diff does

unstructured.py:19-48 adds _element_text(), which renders a Table element's metadata.text_as_html as a markdown table (via the new utils/html.py:html_to_markdown) instead of using the flattened text field, and reports parse_mode/tables_as_markdown accordingly. vector/html_processor.py now just re-exports the relocated helper so the four existing vector-side call sites (vector/processor.py:1055, vector/mail_content.py:145, search/context.py:719) are untouched.

Correctness — verified by hand

• Non-table path is byte-identical to before. Old: if "text" in element and element["text"]: texts.append(...). New: element.get("text") or "" returned from _element_text, appended only if rendered. Same result for every non-Table element.
• The pipe-check gate (unstructured.py:46) is the right guard. html_to_markdown can silently degrade to regex-stripped prose on a markdownify exception (utils/html.py:50-57); requiring "|" in table_md before counting it as a converted table (rather than "truthy") is what keeps parse_mode honest, and it's exercised directly by test_markdownify_failure_does_not_report_markdown.
• Layering fix holds up. utils/html.py imports only markdownify/re/logging; confirmed via grep that all four downstream html_to_markdown call sites still resolve through vector/html_processor.py's re-export, so nothing else needed to change. test_the_shared_helper_does_not_drag_in_the_vector_layer makes this a regression test rather than a convention that can silently drift.
• element_types counting is unaffected — it still increments once per element regardless of which text branch fired, so existing metadata consumers see no shape change there.

Test coverage

Six unit tests, and they hit the branches that matter: happy-path table→markdown, table without text_as_html, HTML metadata on a non-table element (guards the type gate specifically), the markdownify exception fallback, an unconvertible/empty <table> falling back to text, and the import-boundary structural test. That's the right shape for this change — I don't see a gap worth adding to it.

Minor / non-blocking observations

• _element_text returns plain, False from two separate branches (non-Table, and Table without text_as_html) — could collapse into one guard, but it's purely stylistic and the current form arguably reads clearer with the docstring's reasoning attached to the type check specifically.
• parse_mode becomes "markdown" for the whole document the moment any single table converts, even if everything else is plain-paragraph prose. That's called out explicitly in the code comment (unstructured.py:245-247) as intentional, so flagging only for visibility, not as a defect.
• I couldn't execute uv run pytest / ruff / ty myself in this sandbox (the harness didn't approve running uv), so I verified correctness by manual trace against both the diff and the pre-change code rather than by executing the suite. The PR thread already reports 3335 unit tests green, ruff/ty clean, and SonarCloud's quality gate passed with 0 new issues — consistent with what I found by reading.

No changes requested.

[cbcoutinho]

Round-1 findings addressed in a2d3750:

• text_as_html not gated on element type: added the type == "Table" guard. Re-checked against a live unstructured container — still 18 markdown rows on the questionnaire, so no behaviour change, and HTML appearing on some other element now degrades to its text instead of being misread as a grid.
• No test for a Table without text_as_html: added, plus its mirror (HTML on a non-Table element must keep its own text).

3333 unit tests pass; ruff/ty green. Sonar reports no issues on this PR.

[cbcoutinho]

Round-2 findings addressed in 9ee8d940:

• 🟡 Layering violation — document_processors importing vector: correct, and the most useful catch of the round. Reaching for the existing helper was the right instinct in the wrong direction: importing vector.html_processor runs vector/__init__.py and drags qdrant-client and langchain-text-splitters onto the document stack's import path — the coupling ModuleNotFoundError: No module named 'resource' on Windows since 0.101.2 #877 removed, reintroduced from the other side. Took your suggested fix exactly: the implementation moved to nextcloud_mcp_server/utils/html.py (markdownify only), vector/html_processor.py re-exports it so the four vector-side call sites are untouched, and a test asserts the shared module names no vector import so the boundary fails loudly instead of by review.
• 🟡 parse_mode mislabelled when the exception fallback fires: also real. html_to_markdown returns regex-stripped prose on a markdownify exception, and that string is truthy, so it counted as a converted table and reported parse_mode="markdown" over exactly the flattened output this PR exists to prevent. The branch now requires a pipe in the result and prefers the element's own text otherwise, with a test that patches md to raise.

3335 unit tests pass; ruff/ty green. CI on this branch is green (the earlier red was the GitHub Actions outage, not the code).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cbcoutinho]

Follow-up in 8e725c17 — SonarCloud flagged a CRITICAL python:S3776 after the last round: the table-vs-text fallback chain pushed _make_api_request's cognitive complexity to 18 against a limit of 15. That nesting was mine, accumulated over two rounds (the Table-type gate, then the pipe check for the markdownify fallback), so it is fair.

Extracted _element_text(element, el_type) -> (text, counted_as_table), leaving the request method to accumulate. Behaviour is unchanged: the same 18 markdown rows come back from a live unstructured container, and the seven existing tests pass untouched.

Gate is OK on all four PRs in the stack now.