feat(knowledge): carry page provenance as metadata, not marker text (#13894)

[mrveiss]

Thinking Path

The KB upload path stored PDF text carrying ## Page N markers — markers I
introduced in #13893 when unifying the page-marker convention — and that text
goes straight into the embedding.

Those markers are structure, not meaning. They compete with the document's own
words for similarity, and they are a poor provenance mechanism anyway: recovering
a page number by parsing markers back out of retrieved text breaks the moment a
chunker splits between a marker and the page it labels.

What Changed

render_plain(pages) returns the marker-free text and the page spans from
one pure call over the same pages. That pairing is the whole design: the offsets
always index the exact string that was stored, because both come from the same
render. Everything else rests on that invariant, so it is asserted directly rather
than assumed.

Resolution helpers — page_for_offset(), pages_for_span(), chunk_page_map().

Three decisions worth calling out:

• A span crossing a page break reports both pages. A chunk that straddles a
  break genuinely comes from two pages; reporting only the first would silently
  mis-cite half its content.
• chunk_page_map scans forward from the previous chunk's end. A find()
  from zero attributes every recurring header, footer and table scaffold to page 1
  — precisely the content that repeats.
• An offset in the separator between pages resolves to None, and unpaginated
  formats get no page metadata at all. A DOCX has no pages, and page: 1 would be
  a fabricated citation.

Upload path stores marker-free content plus page_count, page_spans and
pages_with_text in fact metadata.

Verification

$ pytest tests/test_page_provenance_13894.py -q
19 passed

$ pytest media/document/ tests/test_page_provenance_13894.py \
         tests/test_document_extraction_canonical_13893.py \
         tests/test_no_text_layer_reporting_13884.py -q
73 passed

Mutation-tested:

### MUTATION 1: forget the separator width (off-by-N in offsets)  → 5 failed, 14 passed
### MUTATION 2: chunk mapping searches from zero                  → 1 failed, 18 passed
### MUTATION 3: upload reverts to storing marker text             → 2 failed, 17 passed
### RESTORED                                                      → 19 passed

Mutation 2 initially passed, and that was a test defect, not a pass. The
boilerplate test used similar chunks ("CONFIDENTIAL one" / "CONFIDENTIAL two"),
which find() from zero still resolves correctly. It now uses byte-identical
chunks across three pages — the only shape that actually distinguishes a forward
cursor from a naive search. Caught because the mutation was run, not because the
test looked right.

Startup imports: 352 passed.

A note on the wider suite

The combined knowledge/ tests/knowledge tests/api run is order-unstable, so
raw failure counts are not comparable between runs:

	failures
base (Dev_new_gui)	9
this branch	7

Every failure on this branch also occurs on base, and each passes in isolation
(tests/api/test_knowledge_grounding.py → 31 passed alone, on both). This branch
introduces none. Filed separately as a finding — a suite that cannot distinguish a
new failure from a scheduling artefact cannot gate anything.

Model Used

Claude Opus 5 (1M context)

Closes #13894. Part of #13892 — Wave 2.

[mrveiss]

Suite instability filed as #14240 — the failure-count comparison in this PR body is only meaningful because the identical command was re-run on base.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Code review

Reviewed from a sweep of green-but-unreviewed PRs, not as the author. One blocking finding,
verified by reproducing it.

page_spans does not survive the write. The retrieval path reads the destroyed copy.

_page_provenance_metadata (autobot-backend/api/knowledge.py:1231-1233) builds a list of
dicts:

[{"page": 1, "start": 0, "end": 13}, {"page": 2, "start": 15, "end": 27}]

sanitize_metadata_for_chromadb (autobot-backend/knowledge/utils.py:36-39) branches on dict
→ json.dumps and on list/tuple → ", ".join(str(v) for v in value). A list of dicts
takes the second branch, so each element becomes a Python repr and the result is:

{'page': 1, 'start': 0, 'end': 13}, {'page': 2, 'start': 15, 'end': 27}

I ran the sanitizer's expression against that shape to confirm. json.loads fails
(Expecting property name enclosed in double quotes), and the structure is unrecoverable,
not merely reformatted — the commas inside each span are indistinguishable from the commas
between spans, so no split can undo it.

That is the copy retrieval uses. knowledge/search.py:398 queries with
include: ["documents", "metadatas", "distances"] and :420 takes
metadata = results_data["metadatas"][0][i] straight from the Chroma result. Nothing
re-hydrates from the correctly JSON-encoded Redis copy at knowledge/facts.py:646.

So the invariant the PR calls "the whole design" — offsets that always index the exact stored
string — never reaches a consumer in usable form. page_count (scalar) and pages_with_text
(list of ints) pass through fine; page_spans is the one genuinely new shape, and every prior
caller of _store_fact_in_kb passed only scalars or lists of strings, so this sanitizer branch
was dormant until now.

Why the suite missed it: test_upload_metadata_offsets_index_the_stored_content
(tests/test_page_provenance_13894.py:383-396) calls _page_provenance_metadata() and asserts
on the raw dict, never going through _store_fact_in_kb → the sanitizer. The mutation testing
covered the producer, not the seam.

Simplest fix consistent with the existing sanitizer: json.dumps(page_spans) at the producer, so
it arrives as a str and takes no lossy branch. The sanitizer's own gap — silently flattening a
list of dicts it cannot represent — is filed separately as #14257, since it will bite the next
caller with a structured list too.

Second, smaller point (already disclosed, flagging for tracking). The connectors still emit
marker text: knowledge/connectors/content_extraction.py:52-59 and gdrive.py:233-236 call
extract_pdf_document(...).text, i.e. render_pages() output with ## Page N baked in, and get
no page metadata at all. The commit message acknowledges this, so the fix is deliberately partial
— worth confirming there is a follow-up issue rather than #13894 being read as closed.

Not a finding: documents already stored with in-text markers read exactly as before, and no
consumer anywhere in the backend parses ## Page markers back out, so stale content does not
degrade.

[mrveiss]

PR #14260 fixes the sanitizer itself (#14257), so page_spans survives the write without this PR needing a workaround at the producer.

Once it lands, sanitize_metadata_for_chromadb JSON-encodes any sequence containing structured elements and leaves lists of scalars in their historical comma-joined form — so pages_with_text and page_count are unaffected and existing stored metadata stays readable.

Worth adding one test on this PR's side that crosses the storage seam rather than asserting on _page_provenance_metadata()'s raw dict — that gap is why the corruption was invisible to a suite that was otherwise mutation-tested.

[mrveiss]

The blocking finding I raised is resolved by base, not by anything needed here.

#14260 fixed sanitize_metadata_for_chromadb itself (#14257): a sequence containing structured elements is now JSON-encoded instead of flattened with str(). Verified by merging current Dev_new_gui into this branch in a throwaway worktree and running the real sanitizer against this PR's page_spans shape:

page_spans round-trips: True

So no producer-side workaround is needed here — update this branch onto current base and the offsets survive the write and come back parseable from the Chroma read at search.py:420.

The smaller point stands and is worth a line before merge: test_upload_metadata_offsets_index_the_stored_content asserts on _page_provenance_metadata()'s raw dict and never crosses the storage seam, which is why the corruption was invisible to an otherwise mutation-tested suite. One test that goes through _store_fact_in_kb would have caught it and would catch the next encoder change too.

The connector gap (Drive/OneDrive still emitting ## Page N markers) is disclosed in the commit message and out of scope here.