G2: chunking contract test + the four silent defects it found

.changeset/g2-chunking-contract.md

@@ -0,0 +1,48 @@
+---
+"@smooai/smooth-operator": patch
+---
+
+fix(ingestion): the chunking contract (G2) — and the four ways the chunker was wrong
+
+Gap **G2** was recorded as open ("our knowledge store assumes pre-chunked text"),
+but the `Chunker` the connectors feed shipped with G1. What was actually missing
+was the **contract suite** the gap doc asked for — and writing it found the
+chunker wrong on four counts. Every one of them fails *silently*: no error, no
+panic, just worse retrieval or a chunk the embedding API quietly truncates.
+
+`rust/ingestion/tests/chunking.rs` (16 tests) pins chunk count / dense indices /
+stable ids, overlap as a shared run of words, metadata + title + source + acl
+propagation onto every chunk, oversized spill with no word lost, UTF-8 integrity
+at the boundary, and degenerate configs. Fixed against it:
+
+- **`max_chars` is a hard cap on the emitted chunk, overlap included.** Overlap
+  was prepended on top of a chunk that already filled the cap, so a default
+  500/64 chunker emitted chunks of up to 565 characters. The cap is the contract
+  with the embedding model's input limit — over it, the API truncates rather
+  than rejects, so the tail of a chunk is dropped from the index with nothing
+  logged. Overlap now comes out of the packing budget.
+- **Text without spaces now spills.** Splitting on word boundaries only meant a
+  Chinese, Japanese or Thai document — or a long URL, or a minified blob — had
+  exactly one "word", so it came back as a **single unbounded chunk**. A 50k-char
+  document became one chunk, embedded truncated, retrievable as nothing useful.
+  The fallback cuts on **character** boundaries.
+- **CRLF documents split on paragraphs.** `"\r\n\r\n"` contains no `"\n\n"`, so
+  every Windows-authored file and many HTTP-fetched pages arrived as one giant
+  paragraph and lost all paragraph structure. Content is CRLF-normalized first.
+- **A markdown heading is a hard chunk boundary.** A chunk spanning two sections
+  attributes section A's text to section B's heading at retrieval time.
+
+Characters, never bytes, throughout: slicing to a byte offset to hit a character
+cap cuts an em-dash or an emoji in half — a panic in Rust, silent mojibake in a
+port to the other four languages.
+
+Proof of red: reverting `chunker.rs` in place, leaving the tests, fails 6 of 16.
+The UTF-8 guard needed a second pass and is worth calling out — built from
+space-separated words it passed against a deliberately byte-slicing
+implementation, because spaced text never reaches the character-split path at
+all. Rebuilt on unspaced mixed-width text it panics on that same mutant. A guard
+that cannot fail is the defect, not the reassurance.
+
+Rust only. Chunking runs server-side in the ingestion crate; the other four
+languages have no ingestion pipeline to port it into, so there is nothing to
+port yet. No public API change — `Chunker::new` / `chunk` keep their signatures.

docs/Architecture/Ingestion Pipeline.md

@@ -144,12 +144,27 @@ Chunker::new(max_chars, overlap_chars)   // or Chunker::default() (500 / 64)
 
 Strategy:
 
-1. split content into paragraphs on blank lines (`\n\n`),
-2. greedily pack paragraphs into a chunk up to `max_chars`,
-3. a single paragraph larger than the cap is hard-split on word boundaries,
+1. normalize CRLF, then split content into paragraphs on blank lines,
+2. greedily pack paragraphs into a chunk, **breaking before a markdown heading**
+   so no chunk straddles two sections,
+3. an oversized paragraph spills on word boundaries; a single token longer than
+   the budget — a URL, or any script that does not use spaces (Chinese,
+   Japanese, Thai) — spills on **character** boundaries,
 4. successive chunks overlap by `overlap_chars` of trailing whole words so a
    fact spanning a boundary stays retrievable.
 
+`max_chars` is a **hard cap on the emitted chunk, overlap included** — it is the
+contract with the embedding model's input limit, and a chunk over it is silently
+truncated by the API rather than rejected. Overlap is therefore spent out of the
+packing budget rather than added on top of a chunk that already fills it.
+
+Everything is denominated in **characters, never bytes**. Slicing to a byte
+offset to hit a character cap cuts an em-dash or an emoji in half: a panic in
+Rust, silent mojibake in a port. `tests/chunking.rs` guards this on unspaced
+mixed-width text, which is the only input that reaches the character-split path
+at all — a guard built from space-separated words passes against a byte-slicing
+implementation and protects nothing.
+
 `overlap_chars` is clamped below `max_chars` so chunking always makes forward
 progress. Each `Chunk` gets a **stable id** `"{doc_id}#{index}"` and inherits the
 source document's title, metadata (`title`, `source`, plus any custom keys), and
@@ -216,7 +231,7 @@ keyed and unkeyed paths can never silently mix dimensions.
 
 | Tier        | What                                                  | When           |
 | ----------- | ---------------------------------------------------- | -------------- |
-| `unit`      | chunker, embedder, file connector, web strip/guard, GitHub path/issue/ACL filters + `tests/github_connector.rs` (mock GitHub API → prose/code/issue RawDocuments + ingest→retrieve), `tests/ingestion_contract.rs` (chunk→embed→store→retrieve + idempotency) | every PR, no creds |
+| `unit`      | chunker, embedder, file connector, web strip/guard, GitHub path/issue/ACL filters + `tests/github_connector.rs` (mock GitHub API → prose/code/issue RawDocuments + ingest→retrieve), `tests/ingestion_contract.rs` (chunk→embed→store→retrieve + idempotency), `tests/chunking.rs` (the chunking contract: cap-with-overlap, boundary rules, CRLF, unspaced/multi-byte spill, metadata propagation) | every PR, no creds |
 | `external`  | `WebConnector::live_fetch_example`, `GithubConnector` live pull | gated on `SMOOTH_AGENT_E2E=1`, nightly |
 
 The headline acceptance is `rust/ingestion/tests/ingestion_contract.rs`: it wires
@@ -238,6 +253,7 @@ SMOOTH_AGENT_E2E=1 cargo test -p smooai-smooth-operator-ingestion \
 
 - `rust/ingestion/src/connector.rs` — `Connector` trait, `RawDocument`, `MockConnector`
 - `rust/ingestion/src/chunker.rs` — `Chunker`, `Chunk` (G2)
+- `rust/ingestion/tests/chunking.rs` — the chunking contract (G2)
 - `rust/ingestion/src/embedder.rs` — `Embedder`, `DeterministicEmbedder`
 - `rust/ingestion/src/pipeline.rs` — `ingest`, `IngestOptions`, `IngestLedger`, `IngestReport`
 - `rust/ingestion/src/connectors/file.rs` — `FileConnector`

docs/Planning/Feature Gaps.md

@@ -49,9 +49,12 @@ Mature platforms ship 50+ connectors (confluence, jira, github, gmail, google_dr
 - ⚠️ **Deviation from the sketch above: `pull` returns `Vec<RawDocument>`, not `Stream<Document>`.** A `Vec` materializes the whole source per pull. That is fine for the three shipped connectors and wrong for a large Confluence space or Jira project — which is precisely what the next connectors are. Re-shaping `pull` into a stream (or a paged `pull_page(cursor)`) is a **breaking change to the trait**, so it should land *before* the SaaS connectors, not after them.
 - **Remains**: the SaaS long tail — confluence, jira, notion, slack, zendesk, google_drive, salesforce, sharepoint. Confluence and Jira are the hardest shape (deep pagination, incremental `since`, per-document permissions) and should be designed against first; their per-document permissions must map onto `RawDocument::acl` the way `GithubConnectorConfig::acl_groups` already does, or ingesting them reopens G3.
 
-### G2. Document processing / chunking pipeline
-Mature knowledge platforms have a tested chunking + metadata-extraction pipeline. Our knowledge store assumes pre-chunked text.
+### G2. Document processing / chunking pipeline — ✅ chunker done + contract-tested; format extraction remains
+Mature knowledge platforms have a tested chunking + metadata-extraction pipeline. Our knowledge store assumed pre-chunked text.
 - **TDD**: `tests/chunking.rs` first — feed a long doc + assert chunk count, overlap, boundary rules, metadata propagation, and that oversized items spill correctly. Then implement the chunker the connectors feed.
+- ✅ **Done.** The `Chunker` the connectors feed shipped with G1 (`rust/ingestion/src/chunker.rs`); what was missing was the **contract suite** — and writing it found the chunker wrong on four counts, each of which fails silently rather than loudly. `rust/ingestion/tests/chunking.rs` (17 tests) now pins: chunk count/dense indices/stable ids, overlap as a shared word-run, metadata + title + source + acl propagation on every chunk, oversized spill without word loss, degenerate configs, and **`Chunk.acl` surviving every split path** — packed, word-spill, character-spill and heading-break — because a dropped ACL here fails *open*: the pipeline turns it into the `DocAcl` the adapters filter on, so a chunk arriving with `acl: None` is stored org-public and retrievable by anyone, silently (credit: `g1conn`). Fixed against it: (a) **`max_chars` is now a hard cap on the emitted chunk** — overlap was prepended *on top of* an already-full chunk, so real chunks ran over the cap the embedding API truncates at; it now comes out of the packing budget; (b) **unspaced scripts now spill** — splitting on word boundaries only meant a Chinese/Japanese/Thai document, a long URL, or a minified blob returned as **one unbounded chunk**, no error; the fallback cuts on **character** boundaries; (c) **CRLF documents split on paragraphs** — `"\r\n\r\n"` contains no `"\n\n"`, so every Windows-authored and many HTTP-fetched documents arrived as one giant paragraph; (d) **a markdown heading is a hard boundary**, so a chunk never straddles two sections and attributes section A's text to section B's heading. Proof of red: reverting `chunker.rs` in place fails 6 of the 17; the remaining 11 are regression guards, each mutation-checked to confirm it actually fails when the behaviour it names is broken. The UTF-8 guard needed a second pass — built from space-separated words it passed against a deliberately byte-slicing implementation, i.e. guarded nothing; rebuilt on unspaced mixed-width text it panics on the same mutant, which is the only version worth keeping. Rust only in this PR — but **not a non-event for the ports, as first stated.** Go, TypeScript and Python genuinely never split text (Go's `knowledge_store.go` only *reads* a stored chunk; TS and Python store "a document as a single chunk keyed by its id"), so they are read-only consumers. **.NET is not**: `dotnet/server/src/Chunker.cs` is a second chunker, a line-by-line port of the pre-fix Rust one, and it still carries all four defects — plus a fifth unique to it, because C# `string.Length` counts **UTF-16 code units** where Rust counts `chars()`, so the same document chunks to *different boundaries* in the two implementations for any astral-plane codepoint. Its tests mirror the old Rust unit tests exactly, which is why they are green through all five. Tracked as pearl `th-ed78a9`; deliberately not fixed here to keep this PR to the reference implementation. See [[Ingestion Pipeline]].
+- ⚠️ **The green unit tests were not evidence the chunker was right.** `src/chunker.rs` shipped with a unit test per rule this plan names — count, packing, cap split, oversized spill, overlap carry, overlap clamp, metadata, id stability — all passing, all passing *through* the four defects above. Each test asserted its rule on the one input shape that never triggers the bug: the cap was checked only with overlap **off**, spill only on space-separated ASCII, paragraphs only with LF. A rule-by-rule checklist against the author's own choice of input is not a contract; the contract is the rule asserted across the inputs the connectors actually deliver.
+- **Remains**: **format extraction** (PDF / DOCX / rich HTML → text) ahead of the chunker. The pipeline still takes text a connector already extracted — the web connector strips HTML, and the GitHub connector *skips* binary extensions rather than reading them. Any connector over a document store (Drive, SharePoint, Confluence attachments) needs this first. (Credit: `g1conn`, via #535.)
 
 ### G3. Access control / permissions (document-level) — ✅ enforced on the live chat path
 Mature knowledge platforms sync per-connector permissions and filters retrieval by user entitlement. We filter by `organizationId` only.