feat(vector-sync): discover office and Outlook documents, not only PDFs

[cbcoutinho]

Tagged-file discovery hard-filtered on application/pdf in two places, so a
tagged .docx was never enqueued no matter which processor could read it. That
filter, not the absence of a converter, was what kept these formats out of the
index.

Which types count is now settings.indexable_mime_types -- PDF plus the
office and Outlook formats by default. It is an explicit setting rather than
"whatever the processor registry can parse" because enabling an optional
processor (unstructured claims pptx, epub and images) would otherwise silently
widen the corpus and its embedding bill behind the operator. Narrowing it back
to PDF alone is a one-line config change.

find_files_by_tag accepts one type or several. Nextcloud's SEARCH takes a
single content type per query, so a tagged folder is expanded once per type;
one type failing no longer discards the types that succeeded, which previously
would have skipped the whole folder.

Verify-on-read needs no change of its own -- it calls the same
_discover_tagged_files, so the read-side filter cannot drift from the
write-side one and drop indexed documents at query time.

The pre-flight size cap now also lands on the new formats. That is deliberate:
it guards bytes, and rendering a huge .doc holds the source and the rendered
PDF at once, so it is more dangerous at size than a PDF. Its wording was
updated from "PDF" to "document" to match, as the code comment there asked of
whoever broadened discovery.

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

---

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

Upgrade impact

This widens the default indexable set from PDF-only to PDF plus the Word,
Excel and Outlook formats. An instance upgrading on default config will start
indexing — and paying to embed — file types it previously ignored, wherever
those carry the vector-index / keyword-index tag.

Nothing is removed and no API changes, so this is not a BREAKING CHANGE:
under the repo's versioning rule. Narrowing back is one line:

VECTOR_SYNC_INDEXABLE_MIME_TYPES=application/pdf

---

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

[claude]

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

---

Review of PR #1266

• Gather context (diff vs origin/feat/office-msg-processors)
• Review changed files for correctness, security, tests, style
• Post review feedback

This PR has already been through seven review rounds (visible in the thread above), and each substantive finding — the sequential per-type SEARCHes, the empty-allowlist inversion bug, the mislabeled size cap, the submodule pointer leak, the undocumented upgrade cost, the stale "PDF" wording — was fixed and tested. I re-read the full diff against origin/feat/office-msg-processors independently rather than trusting that summary, and it holds up. Notes below are what's left, all minor.

Correctness

• _walk_tagged_dir (client/__init__.py:296-346) and its find_files_by_tag caller are solid: results collected by index (deterministic ordering regardless of completion order), walk_failed and not descendants correctly distinguishes "one type failed but others produced results" (proceed with partial data) from "everything failed" (skip). Matches the docstring's stated intent.
• _as_mime_tuple (client/__init__.py:76-92) correctly avoids the classic bug of iterating a bare string char-by-char before feeding it to str.startswith(tuple) — and there's a dedicated test for exactly that regression (test_a_bare_string_is_wrapped_not_iterated).
• The empty-allowlist fix in _discover_tagged_files (vector/scanner.py:492-508) short-circuits before either find_files_by_tag call, so "index nothing" can't accidentally degrade into "no filter" via the two code paths that previously disagreed. Verified against the actual bug described in 13a5bdc.
• I checked the unstructured processor's SUPPORTED_TYPES set directly (document_processors/unstructured.py:61-78) — the PR's claim that pptx/epub/images are only reachable through that optional backend is accurate, so the allowlist's exclusion of them is consistent with the stated rationale, not just asserted.

Test coverage

• Good breadth: concurrency (in-flight counting, not just "it completed"), partial-failure tolerance, deterministic ordering, the directly-tagged-file path (previously untested — correctly identified as the path most office docs will actually take), the empty-allowlist regression, and now (84dccfb) a real-Nextcloud check that the hardcoded MIME strings actually match what the server reports for .docx/.xlsx. That last one closes a real gap — matching logic can be unit-tested against invented strings and still be wrong against the real server.
• Satisfies the repo's e2e+contract gate for API-surface changes: _discover_tagged_files isn't external API surface, but the new integration test exercises the real WebDAV/SEARCH behavior end-to-end, which is the right tier here.

Minor / non-blocking

• DOCUMENT_MAX_PDF_SIZE_MB now guards every indexed format under a PDF-specific name. The PR deliberately keeps the name to avoid silently re-tuning existing deployments, and documents the mismatch in both the code comment and docs/configuration.md — reasonable tradeoff, just flagging that a future rename (with a deprecation window per this repo's versioning rule) would be cleaner long-term.
• docs/configuration.md states the LibreOffice caveat twice (the new "Which file types get indexed" section and the existing env-var table row). Minor duplication, not wrong, and arguably fine since they're read in different contexts.
• The submodule-pointer leak flagged in round 4 is confirmed clean now: git diff origin/feat/office-msg-processors...HEAD -- third_party/ is empty.

No blocking issues found. Nice use of anyio.create_task_group() per CLAUDE.md conventions, and the round-by-round commit history is unusually easy to audit — each fix is scoped to exactly what was flagged.

[cbcoutinho]

Round-1 findings addressed in 566e151a:

• 🟡 Undocumented VECTOR_SYNC_INDEXABLE_MIME_TYPES: added to the environment-variable reference next to its siblings, including the LibreOffice caveat. You were right that the PR's "one-line config change" claim was empty without a discoverable name.
• 🟡 Sequential per-type SEARCHes: extracted _walk_tagged_dir and run the per-type lookups through an anyio task group, keeping the partial-failure tolerance. Results are collected by index rather than appended, so ordering no longer depends on which request returns first and discovery output stays deterministic. Tested by counting requests actually in flight rather than inspecting state afterwards.
• 🟢 Stale "PDF" wording: preflight_oversize_result's docstring and the max_pdf_mb local both say "document" now. The setting name itself is left alone as you noted.
• Sonar python:S8495: _as_mime_tuple returns from one place.

On the 🟢 pptx omission — confirmed deliberate. It is an explicit allowlist, and .pptx has no processor of its own here (only the optional unstructured backend), so adding it would index PowerPoint through a path none of the measurements in #1265 cover.

3384 unit tests pass; ruff/ty green.

[cbcoutinho]

Round-2 findings addressed in 16c458a2:

• Commit title overclaimed — you are right, and it was worth flagging. Only the per-type SEARCHes within one folder overlap; the folders themselves are still walked in sequence. Retitled to "expand each tagged folder's types concurrently" and the body now states the remaining O(folders) sequentiality explicitly rather than leaving it to be discovered in the docstring. I did not make the folder walk concurrent too: that multiplies the two fan-outs together against Nextcloud, which wants a bound rather than another task group, and is a separate change.
• DOCUMENT_MAX_PDF_SIZE_MB guards more than PDFs now, under a PDF name — good catch on the operator-experience angle. Added a callout in docs/configuration.md right where the cap is described, in the words someone would actually be searching with ("if a large .docx is being rejected as oversize, this is the knob, despite the name"), plus a note on the inline reference line. The setting keeps its name so nobody's deployment is silently re-tuned by a rename.
• No test for the directly-tagged-file filter — added two, and this was the right gap to name: it is the path most office documents will actually take, and only the tagged-folder expansion had coverage. One asserts the office/PDF types pass while image/png and text/plain are dropped; the other covers a content type carrying parameters (…document; charset=binary), which is the case startswith exists to handle.

3393 unit tests pass; ruff/ty green.

Note on timing: this round and the CI behind it were both caught by yesterday's GitHub Actions outage (major outage 15:22–~00:10 UTC, runner acquisition failing). CI has been re-run since.

[cbcoutinho]

Round-3 finding addressed in e20df82f:

• 🔴 Empty VECTOR_SYNC_INDEXABLE_MIME_TYPES widened the corpus instead of closing it: real bug, and the reasoning about why it is reachable is what makes it worth fixing rather than documenting. An empty tuple made find_files_by_tag skip the content-type test for directly-tagged files (indexing every type) while if mime_types and tagged_dirs skipped folder expansion entirely (indexing none) — contradictory, and the exact inverse of what the allowlist exists for. And as you note, vector_sync_keyword_tag documents "set empty to disable", so an operator reaching for the same idiom here to pause indexing would instead open discovery to everything in their Nextcloud.

  _discover_tagged_files now returns [] early and warns, naming the variable and how to re-enable it, so emptying it produces an explanation rather than silence. Two tests: nothing is discovered and find_files_by_tag is never called, and the warning names the setting.

Rebased onto the updated lower PRs. 3399 unit tests pass; ruff/ty green.

[cbcoutinho]

Round-4 — the submodule observation was a real defect, not a nice-to-check. Fixed in df4fdb62.

third_party/astrolabe was bumped from ccd2d180 (the pinned v0.39.5 release) to edecf2b3 — which is a commit on my unmerged astrolabe PR branch (cbcoutinho/astrolabe#302), not anything on that repo's main. It got in via a git add -A while the submodule pointer was dirty from working on both repos, and it survived several rebases because a gitlink change is invisible in a --stat unless you look for it.

Merging that would have pinned this repo to a commit that does not exist outside a PR branch, so git submodule update would fail for anyone who ran it. Worth more than the "not a code issue" framing.

Removed by amending the offending commit and replaying the tip; git diff origin/master...feat/office-vector-sync -- third_party/ is now empty, and I checked the three lower PRs in the stack are clean of it too.

Also confirming the two other observations need no change:

• ", ".join(mime_types) in the log lines — agreed, harmless at debug/info, left alone.
• Folder-to-folder discovery still sequential — yes, deliberate and stated; multiplying the two fan-outs against Nextcloud wants a bound, not a second task group.

3401 unit tests pass; ruff/ty green.

[cbcoutinho]

Round-5 — both nits addressed in 3878e476.

• 🟢 Behaviour change on upgrade: this is the one I think you undersold, so I have promoted it rather than just noting it. An instance upgrading on default config starts indexing — and paying to embed — Word, Excel and Outlook files it previously ignored. Nothing is removed and no API changes, so I agree it is not a BREAKING CHANGE: under the repo rule, but "changes your embedding spend on upgrade" belongs somewhere a release manager reads. It now has an Upgrade impact section in the PR body and a paragraph in the commit message, so it lands in the CHANGELOG rather than living in this thread. Narrowing back is one line, quoted in both places.

• 🟢 Remaining "PDF" wording: you were right that the sweep was half-done. vector/scanner.py (four comments + the "Found N tagged PDFs" log line) and _reconcile_tag_event's docstring ("current tagged PDFs", "PDF descendants") all now say files/documents. Comment-only, no behaviour change.

3406 unit tests pass; ruff/ty green.

On CI for the record: the earlier red on this PR was a genuine bug of mine — tests/integration/test_index_mode_discovery.py builds its settings as a SimpleNamespace and I had updated only the unit test's mock when indexable_mime_types went in, so discovery hit an AttributeError. Fixed in 4dc7060c and confirmed by all three single-user lanes going green on the re-run. The one remaining red lane (login-flow-ldap / nc33) died in Run docker compose before any test executed, while login-flow-ldap / nc32 and both ldap lanes passed on the same commit — re-running it.

[cbcoutinho]

Round-6 — both findings addressed in a0dd8cff, and the 🟡 one was a fair hit.

• 🟡 The "note the upgrade cost" commit did not add one: correct, and worth stating plainly — I put the callout in the commit message and the PR body, and neither becomes a released artifact. Commitizen generates the changelog entry from the subject line, so the note would have existed only in git metadata and a web thread while the commit claimed otherwise.

  There is now a "Which file types get indexed" section in docs/configuration.md, next to the tag configuration where someone tuning indexing is already reading. It says outright that upgrading from a PDF-only release makes already-tagged Word/Excel/Outlook files — including everything beneath a tagged folder — eligible on the next scan, gives the one-line opt-out, and warns that an empty value means index nothing rather than no filter.

• 🟢 The line the sweep missed: scanner.py:1044 still said a folder tag "applies to every PDF beneath it" while the two lines around it were edited in the same hunk. Fixed. I also re-grepped the file: the remaining "PDF" mentions are all genuine (the 531 MB PDF anecdote, "PDF plus the office formats", pathological-PDF examples).

CI for the record: the previous run on 3878e476 came back fully green, all lanes — including login-flow-ldap / nc33, which confirms that earlier failure was the Run docker compose flake rather than anything in the diff. All three single-user lanes also passed, verifying the 4dc7060c integration-stub fix.

3406 unit tests pass; ruff/ty green.

[cbcoutinho]

Round-7 addressed in 84dccfb1.

• 🟢→ took this one seriously: you flagged the PDF-only fixture as a nit, but it was hiding an unchecked premise. Matching is content_type.startswith(indexable_mime_types), so every allowlist entry has to be the exact string Nextcloud reports for that extension — and I took those strings from the spec, never compared them against the server that emits them. If Nextcloud labelled a .docx differently, discovery would return nothing and every unit test would still pass, because they assert the matching logic against the same strings the code declares. That is a silent-failure shape worth closing regardless of which label it gets.

  New tests/integration/test_office_discovery.py, two cases, run against the dev stack before committing (both pass):

  1. The content types Nextcloud actually reports for an uploaded .pdf/.docx/.xlsx are each matched by the configured default — with the failure message printing what it reported, so a future mismatch says why.
  2. A tagged folder containing all three plus a .png expands to exactly the three indexable files. The .png is deliberate: without it a green result could come from the filter having been dropped rather than working.

  That also exercises the concurrent per-type fan-out against a real Depth:infinity REPORT rather than a mocked client.

Left as-is, both explicitly non-blocking:

• Long VECTOR_SYNC_INDEXABLE_MIME_TYPES table row — agreed it is unwieldy, but the prose section added last round is where the explanation now lives; the row is the index entry.
• Per-folder debug log not distinguishing full from partial expansion — the per-type failure is already logged with its MIME type by _walk_tagged_dir, so the information exists; co-locating it would mean threading state through purely for a debug line.

On CI: the two red lanes on the previous commit were both nc33 and both environmental — login-flow-ldap died in Run docker compose before any test ran, and multi-user-basic failed seven tests with subprocess.CalledProcessError: docker compose exec, i.e. it could not reach its containers. The commit they ran against changed only docs/configuration.md and one comment, and its parent was fully green on those same lanes. Re-running.

[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]

Round-8: no blocking issues, and I am treating this as the end of the loop for this PR. CI is complete and green — 28/28 checks, MERGEABLE/CLEAN.

Your three notes, none actioned, with reasons:

• DOCUMENT_MAX_PDF_SIZE_MB guarding every format under a PDF name — agreed a rename with a deprecation window is cleaner long-term, and agreed it does not belong in this PR: renaming here would silently re-tune every existing deployment, which is the specific harm the repo's versioning rule exists to prevent. The mismatch is documented in the code comment and in docs/configuration.md where an operator would hunt for it.
• LibreOffice caveat stated twice in the docs — kept. The table row is the index entry and the prose section is the explanation; someone scanning the env-var reference should not have to find the narrative section to learn a format needs a binary.
• Submodule pointer clean — confirmed, and thank you for re-checking it. That was a real defect you caught in round 4 that would have pinned this repo to a commit existing only on an unmerged PR branch.

Summary of what these eight rounds actually caught in my code, since it is worth recording: a layering violation (document_processors importing vector, reintroducing the coupling #877 removed), the stray submodule pin, a config footgun where an empty allowlist widened the corpus instead of closing it, a quadratic regex reachable by ordinary padded table rows, an integration stub left stale by a new settings attribute, a commit that claimed a changelog note it did not add, and an untested premise — that the hardcoded MIME strings match what Nextcloud actually reports. Seven real defects, none of which the unit suite would have surfaced on its own.