fix(sections): stop numbered index rows fabricating item anchors (GH #918) - #926
Conversation
|
Drafting until #919 clears |
dgunning
left a comment
There was a problem hiding this comment.
Approving. This is the right shape — it answers the objection that blocked the previous attempt by keying on the structure of the index rather than validating each row, and it does so without making correctness depend on a signal many filers don't emit.
I re-verified your claims rather than taking them on trust, and ran everything merged onto current main rather than this branch's base:
- All 73 tests in
test_toc_analyzer_form_aware_bare_item.pypass, including the four accept cases (ten_q_accepts_valid_item_6,ten_k_accepts_item_8,no_form_preserves_default_cap_15,letter_suffix_preserved) that the corroboration approach broke. That was the blocking objection and it is genuinely resolved. - 313 TOC-related tests pass; your 6 new tests and the updated #905 file pass, 12 in total.
- Section-map diff across the 55 committed fixtures: 0 changed, 0 sections lost or gained, reproduced independently. Since
ms-10-k-2025-02-21.htmlis in that corpus and unchanged, the "MS keeps all 19 items" claim holds on the real filing, not only the synthetic fixture. - The exact
len(mda_text) == 51_860assertion survives the text-extraction changes that landed onmaintoday, which I had expected to shift it.
A benefit you did not claim. I benchmarked expecting a regression, since _cell_in_numbered_index runs per bare-number row and materialises the whole row list each time. The opposite is true — rejecting the fabricated rows early skips all the anchor resolution they were triggering:
| rows in index | before | after |
|---|---|---|
| 100 | 0.187s | 0.068s |
| 400 | 2.819s | 0.775s |
| 800 | 10.265s | 1.958s |
| 1600 | 38.102s | 4.921s |
Near-quadratic to roughly n^1.3, and 7.7x faster at 1600 rows. Worth a line in the PR body.
Before merge
Rebase. The branch is 12 commits behind — it predates #919, the 5.44.1 release, and five commits from today. The only conflict is CHANGELOG.md, where both sides append to [Unreleased]; I resolved it locally in seconds and nothing else moved. Also mark the PR ready for review when you rebase, since it is still a draft.
Non-blocking
The [:4] header window is an undocumented magic number with a quiet failure mode: an index with five or more preamble rows before its header slips the guard and the old behaviour returns silently. A one-line comment on why 4 would help whoever reads this next.
Two cheap hardening items in _cell_in_numbered_index: the xpath materialises every row before [:4] slices it, and the result is recomputed for every bare-number cell in the same table rather than being memoised. The benchmark says neither costs anything today, but 4e56e32b on main was an O(rows²) bug of exactly that shape, so an early-breaking loop is worth the one line.
Style: _NUMBERED_INDEX_HEADERS sits between two methods; class constants in this codebase go at the top of the class (see DocumentBuilder.BLOCK_ELEMENTS).
Vocabulary risk: the header set is exact-match English, and note/notes is the member most likely to misfire on a financial-statement index that also carries genuine item rows. I could not construct a realistic filing where it bites, so this is a note for the record rather than a request.
On the cassettes
The two cassettes add ~64 MB net. That is a real cost and it is permanent — the 6.0 test-data work explicitly excludes history rewriting — but it is not yours to solve in a bug-fix PR, and I am not going to hold a correct fix for infrastructure debt we are already tracking separately. Merging as-is.
One thing to be aware of for anything you record in future: cassettes here are stored decompressed, and compressing them saves about 76% on real bodies. That switch is queued as part of the 6.0 test-data work, and once it lands, new recordings will be roughly a quarter of the size for free.
Thanks for taking the review direction seriously and for pinning the MS regression case explicitly — that test is the reason I could approve this quickly.
|
Heard, will update soon ™️ |
…gunning#918) FMCC FY2025 10-K (0001026214-26-000021): every part_*_item_N anchored on an MD&A table caption — obj['Item 11'] returned 30,514 chars starting at "Table 11 - Other Investments Portfolio", at full confidence. FMCC's MD&A "List of Tables" index rows ('11' | 'Other Investments Portfolio' | '18') feed the generic TOC scan, and _extract_preceding_item_label read the bare row-number cell as "Item 11" (the page-number range cap doesn't help: 11 is a valid 10-K item number). Fix by context, not per-row validation: the index is recognisable by its header row, which names the numbering ("Table | Description | Page") — bare numbers in such a table are captions, not item numbers. A genuine TOC either heads its number column "Item" (Morgan Stanley: "Table of Contents | Part | Item | Page" — only an exact-word header cell counts, so "Table of Contents" never triggers the guard) or carries no header at all, so headerless bare-number TOCs keep the legacy behaviour. An earlier attempt (PR dgunning#919 as reviewed) corroborated each bare-number row against its link target instead; that silently dropped real items on filers whose TOC anchors land nowhere near the item headings (MS 10-K: 19 item sections -> 6) and is abandoned. The same guard removes the phantom part_i_item_5/6 on FMCC's 10-Qs that the GH dgunning#905 fix could only flag-and-downgrade — they came from the same List of Tables rows — and the surviving Part I items now anchor on the real content (part_i_item_2 = the actual 52K-char MD&A, part_i_item_3 = Market Risk) instead of table scraps. The dgunning#905 end-to-end test is updated to pin the improved behaviour; its cassette is re-recorded from a cold cache so the quarter-index probes of get_filing_by_accession are on tape and the test now replays standalone (--block-network verified), fixing the replay caveat noted in review. Verification: unit tests on FMCC-shaped, MS-shaped (Item-column), headerless, and Figure-index TOCs (no network); VCR-backed end-to-end tests pinned to the FMCC 10-K and 10-Q. Section-map diff across the 55 committed 10-K/10-Q fixtures: zero changes (MS keeps all 19 item sections). TOC suites pass (155 tests) including test_toc_analyzer_form_aware_bare_item.py. Second half of dgunning#918; closes it together with the first (PR dgunning#919).
d9bd1c0 to
f26297e
Compare
|
Rebased onto main @ b361dbd , should be good to go. Thanks for your patience |
Second half of #918 (closes it together with #919). Replaces the bare-number corroboration approach that was blocked in #919's review.
The defect — numbered-index rows fabricate item anchors (FMCC)
For Freddie Mac's FY2025 10-K (
0001026214-26-000021), everypart_*_item_Nanchored on an MD&A table caption —obj['Item 11']returned 30,514 chars starting at "Table 11 - Other Investments Portfolio", at full confidence. FMCC's MD&A "List of Tables" index rows (11 | Other Investments Portfolio | 18) feed the generic TOC scan, and_extract_preceding_item_labelread the bare row-number cell as "Item 11" (the page-number range cap doesn't help — 11 is a valid 10-K item number).The fix — context, not per-row validation
Per the review's suggested direction: the numbered index is a recognisable structure. Its header row names the numbering —
Table | Description | Page— so bare numbers in such a table are captions, not item numbers. A genuine TOC either heads its number column "Item" (Morgan Stanley:Table of Contents | Part | Item | Page) or carries no header at all, so headerless bare-number TOCs keep the legacy behaviour. Only an exact-word header cell counts ("Table of Contents" is not "Table"), and an "Item" cell in the same header row outranks the index vocabulary (table/figure/chart/exhibit/note, singular and plural).This targets the actual FMCC failure without making correctness conditional on a signal many filers don't emit: the reviewed approach — corroborating each bare-number row against its link target — silently dropped real items on filers whose TOC anchors land nowhere near the item headings (MS 10-K: 19 item sections → 6). With the header guard, MS keeps all 19.
Bonus (same as the abandoned approach delivered): the guard removes the phantom
part_i_item_5/part_i_item_6on FMCC's 10-Qs that the #905 fix could only flag-and-downgrade — they came from the same List of Tables rows — and the surviving Part I items now anchor on the real content (part_i_item_2= the actual 52K-char MD&A,part_i_item_3= Market Risk) instead of table scraps. Onmain, that filing's Part I is entirely table scraps: item 1 opens on "Table 1 - Summary…", item 6 over-captures 165K chars.Performance
Rejecting the fabricated rows turns out to be a speedup, not a cost:
_cell_in_numbered_indexruns per bare-number row, but every row it rejects skips all the anchor resolution the fabricated item used to trigger. On the numbered-index shape this takes the scan from near-quadratic to roughly n^1.3:7.7x faster at 1600 rows.
Verification
tests/issues/regression/test_issue_918_numbered_index_rows.py: unit tests (no network) on FMCC-shaped (List of Tables → no items), MS-shaped (Item-headed column with unreachable heading text → all items survive — the regression case from fix(sections): stop the short-section rescue rebuilding items from cross-references (GH #918) #919's review), headerless (legacy behaviour unchanged), Figure-index, and mixed-header fixtures — plus a VCR-backed end-to-end test pinned to the FMCC 10-K (no item sections; no section opens on a "Table N" caption).test_issue_905_phantom_part_items.pyupdated: phantoms asserted absent instead of present-but-warned; MD&A pinned at 51,860 chars.Filingfromtests/_offline_filings.py(the FMCC FY2025 10-K row is added), the renamed 10-Q cassette is byte-identical tomain's index-trimmed one, and the new 10-K cassette records only the filing's own documents — no quarterly-index interactions. All 12 tests verified green under--block-network.test_section_boundary_corpus.pypass; the anomaly census flags Citigroup's 10-K, but identically on currentmain(pre-existing, unrelated to this branch).test_toc_analyzer_form_aware_bare_item.py— the four accept cases that failed under the corroboration approach pass here.Rebased onto
main@ b361dbd (post-#919, post-5.45.1); the only overlap was the CHANGELOG and the #961 cassette trim, both folded in.