Merge pull request #16 from DrBaher/feat/docx-numbering

feat: resolve .docx automatic list numbering during extraction

Author: DrBaher

AGENTS.md

@@ -13,9 +13,12 @@ Every review carries a `coverage` block. `decision: "approve"` with `coverage.ru
 
 ```json
 "decision": "approve",
-"coverage": {"rules_evaluated": 11, "rules_matched": 6, "extraction_status": "ok:docx-xml", "text_chars": 4213}
+"coverage": {"rules_evaluated": 11, "rules_matched": 6, "extraction_status": "ok:docx-xml",
+             "numbering_resolved": true, "text_chars": 4213}
 ```
 
+`numbering_resolved` is `true` when a `.docx`'s automatic list numbering was resolved to the numbers Word displays, `false` when it could not be (missing `numbering.xml`, an unsupported `numFmt`, or the `textutil` fallback), and `null` where numbering isn't a concept (`.txt`, `.pdf`, stdin). **When it is `false`, don't reason about clause numbers** — "Section 7" may exist in the document even though no `7.` appears in the text you were given.
+
 | `decision` | Means |
 |---|---|
 | `approve` / `escalate` / `block` | Rules ran, matched, and scored. |
@@ -138,7 +141,7 @@ result = review_nda(text=nda_text, policy=policy)
 
 `load_policy` accepts either playbook shape (a `clause_rules` object or a built `policy` list). `review_nda` raises `EmptyDocumentError` on blank text and `PlaybookError` when the policy yields no keyword-bearing rules — both are cases where a silent `approve` would be a lie. Check `result["coverage"]` before trusting `result["decision"]`.
 
-To read a `.docx` / `.pdf`, use `read_nda_input(path)` → `(text, extraction_status)`; reading those with `Path.read_text` yields zip/binary noise that no keyword can match.
+To read a `.docx` / `.pdf`, use `read_nda_input(path)` → `(text, extraction)`, where `extraction` carries `extraction_status` and `numbering_resolved`. Reading those files with `Path.read_text` yields zip/binary noise that no keyword can match, and a `.docx` read without resolving `w:numPr` loses every list number Word displays. See [docs/reference/docx-extraction.md](docs/reference/docx-extraction.md).
 
 Stdlib-only at runtime. LLM augmentation requires only `urllib`; no `anthropic` / `openai` SDK dependency.
 

CHANGELOG.md

@@ -103,7 +103,35 @@ and the fixture's `approve` case is a genuinely clean NDA.
 
 - **The documented library API now exists.** `AGENTS.md` told importers to
   `from nda_review_cli import review_nda, load_policy` — neither function was defined. Both are
-  now real, alongside `read_nda_input(path) -> (text, extraction_status)` for `.docx`/`.pdf`.
+  now real, alongside `read_nda_input(path) -> (text, extraction)` for `.docx`/`.pdf`.
+
+- **`.docx` automatic list numbering is resolved.** Word stores a numbered paragraph's visible
+  number nowhere in its text — the paragraph carries only a `w:pPr/w:numPr` pointer, and the
+  number is computed at render time from `word/numbering.xml` (`numId` → `abstractNumId` →
+  per-level `numFmt`/`lvlText`/`start`). Reading only `w:t` nodes therefore produced a silently
+  *unnumbered* document: a contract whose clauses are numbered 1–20 in Word extracted with no
+  numbers at all, and an in-body cross-reference to "Section 7" had no target.
+
+  Extraction now walks `w:body` in document order and prepends each paragraph's computed number.
+  Supports `decimal`, `decimalZero`, `lowerLetter`, `upperLetter`, `lowerRoman`, `upperRoman`,
+  and `none`; `%1`–`%9` substitution into `lvlText` (so multi-level `"%1.%2"` renders as `2.1`);
+  `start`, `w:startOverride`, and per-level `w:lvlOverride`; deeper-level resets; and numbering
+  inherited through a `w:pStyle` → `w:basedOn` chain in `styles.xml`. Bullets are skipped.
+  Counters are keyed by `(numId, ilvl)`, so a nested `(a) (b)` list under its own `numId` does
+  not disturb the outer clause numbering. Stdlib-only.
+
+- **`.docx` paragraphs are newline-joined, not space-flattened.** `" ".join(root.itertext())`
+  collapsed the whole document into one run-on string, so `locate_clause` (which splits on
+  lines), `extract_clause_snippet` (which splits on blank lines), and `paragraph_index` all
+  operated on a single line.
+
+- **Fail visible, not silent: `coverage.numbering_resolved`.** `true` when every numbered
+  paragraph resolved (and when there were none — nothing to resolve is not a failure), `false`
+  when `numbering.xml` is missing or unparseable, a `numFmt` is unsupported, or the `textutil`
+  fallback was used, and `null` where numbering is not a concept (`.txt`, `.pdf`, stdin).
+  `_read_any_text()` also returns a `numbering` record carrying `numbered_paragraphs` and a
+  human-readable `reason`. Consumers that reason about clause numbers must degrade rather than
+  assert when it is `false`. New: [docs/reference/docx-extraction.md](docs/reference/docx-extraction.md).
 
 - `tests/test_review_coverage.py` (24 tests) and `tests/test_review_golden.py::ClauseRulesPlaybookTests`
   (3 tests). 22 of the 24 fail against the previous source.

docs/reference/README.md

@@ -10,5 +10,6 @@ Concept-level reference for `nda-review-cli`. Each file is the canonical home fo
 | [scoring.md](scoring.md) | Scoring profiles (`balanced` / `strict` / `commercial`), decision thresholds, calibration. |
 | [signer-policy.md](signer-policy.md) | The declarative gate on unattended sign-off. Which clauses an agent may sign, and which escalate to a human. |
 | [state-file.md](state-file.md) | The hash-chained negotiation state file. What it stores, how tamper-detection works. |
+| [docx-extraction.md](docx-extraction.md) | How `.docx` text is extracted, why Word's automatic list numbering must be reconstructed, and what `numbering_resolved` means. |
 | [exit-codes.md](exit-codes.md) | The exit-code map every command honors, plus stable error codes. |
 | [llm-data-flow.md](llm-data-flow.md) | What leaves your machine when you pass `--llm`, and what doesn't. |

docs/reference/docx-extraction.md

@@ -0,0 +1,66 @@
+# `.docx` extraction
+
+What `review --file contract.docx` actually reads, and the two things a naive extractor silently throws away.
+
+## Word does not store list numbers
+
+A paragraph numbered `7.` in Word contains no `7` anywhere in its text. It carries only a pointer:
+
+```xml
+<w:p>
+  <w:pPr><w:numPr><w:ilvl w:val="0"/><w:numId w:val="3"/></w:numPr></w:pPr>
+  <w:r><w:t>Each party acknowledges …</w:t></w:r>
+</w:p>
+```
+
+The visible number is computed at render time by walking `word/numbering.xml`:
+
+```
+numId → abstractNumId → per-level { numFmt, lvlText, start }
+```
+
+An extractor that reads only `w:t` nodes therefore produces a **silently unnumbered document**. A contract whose operative clauses are numbered 1–20 in Word arrives as prose with no numbers, and an in-body cross-reference to "Section 7" has nothing to point at. Nothing errors; the numbers simply never existed in the text being analyzed.
+
+The `textutil` fallback has the same loss — it renders numbered paragraphs as tab-indented bullets.
+
+## What we resolve
+
+`_extract_docx_text` walks `w:body` paragraphs in document order and prepends each one's computed number.
+
+| Supported | Notes |
+|---|---|
+| `numFmt` | `decimal`, `decimalZero`, `lowerLetter`, `upperLetter`, `lowerRoman`, `upperRoman`, `none` |
+| `lvlText` | `%1`–`%9` substitution, so multi-level `"%1.%2"` patterns render as `2.1` |
+| `start` / `w:startOverride` | Honoured per level |
+| `w:lvlOverride` | Per-level `numFmt` / `lvlText` overrides |
+| Level resets | A new item at level *N* restarts every deeper level |
+| Style-inherited numbering | `w:pStyle` → `w:basedOn` chain in `styles.xml`; contract templates frequently number through a style rather than on the paragraph |
+| `bullet` | Skipped — a bullet has no number, and that is not a failure |
+
+Counters are keyed by `(numId, ilvl)`, so two lists in the same document count independently. A nested `(a) (b)` exceptions list under its own `numId` does not disturb the outer `1. 2. 3.` clause numbering.
+
+## Paragraph structure
+
+The extractor joins paragraphs with newlines rather than flattening the document with `" ".join(...)`.
+
+This is not cosmetic. `locate_clause()` splits on lines to find the nearest heading above a match, `extract_clause_snippet()` splits on blank lines to anchor snippets to legal blocks, and `paragraph_index` counts lines. A flattened document reports `clause_heading: ""` and `paragraph_index: 1` for every finding — the `--why` evidence degrades to nothing while still looking well-formed.
+
+## Fail visible, not silent
+
+Every extraction reports whether numbering was resolved. `review` surfaces it as `coverage.numbering_resolved`:
+
+| Value | Meaning |
+|---|---|
+| `true` | Every numbered paragraph was resolved to its visible number. A document with no numbered paragraphs is also `true` — nothing to resolve is not a failure to resolve. |
+| `false` | `numbering.xml` is absent or unparseable, a `numFmt` isn't supported, or the `textutil` fallback was used. **The numbers you see in Word are not in this text.** |
+| `null` | Numbering isn't a concept for this input (`.txt`, `.pdf`, stdin, in-memory text). |
+
+`_read_any_text()` additionally returns a `numbering` record with `resolved`, `numbered_paragraphs`, and a human-readable `reason` when resolution failed.
+
+**Downstream consumers should degrade, not assert.** A tool that checks cross-references or numbering gaps must not report "broken cross-reference to Section 7" as an error when `numbering_resolved` is `false` — it never saw the numbers. Downgrade such findings to warnings and say why.
+
+## See also
+
+- [scoring.md](scoring.md) — the `coverage` block and the `needs_review` decision.
+- [policy.md](policy.md) — the two playbook shapes.
+- [../../SECURITY.md](../../SECURITY.md) — why `.docx` XML is parsed through `_safe_xml_fromstring` (entity-expansion defence).

docs/reference/scoring.md

@@ -59,10 +59,13 @@ Scores only mean something if rules actually ran. Every review therefore carries
   "rules_evaluated": 11,
   "rules_matched": 6,
   "extraction_status": "ok:docx-xml",
+  "numbering_resolved": true,
   "text_chars": 4213
 }
 ```
 
+`numbering_resolved` reports whether a `.docx`'s automatic list numbering was reconstructed (`null` where numbering isn't a concept). When it is `false`, the clause numbers Word displays are absent from the text that was scored — see [docx-extraction.md](docx-extraction.md).
+
 `rules_evaluated` is how many keyword-bearing rules the playbook contributed; `rules_matched` is how many found their clause in the document. A score of `0` with `rules_matched: 0` is not a clean NDA — it is a review that checked nothing.
 
 When `rules_matched == 0` on a non-empty document, the decision is **`needs_review`**, never `approve`. Every real NDA trips at least one clause keyword (`confidential information`, `term`, …), so zero matches means the text is not an NDA or extraction mangled it.