Distinguish incorporation-by-reference from truncation in undersize section warnings (#927) - #959
Conversation
dgunning
left a comment
There was a problem hiding this comment.
Thanks — this is careful work, and the diagnosis is right. The warning genuinely was misattributed on every undersized Item 8 in the corpus, and splitting the two causes on the undersize side only, keeping evaluate_size pure length-in/string-out, is the right shape. The negative cases in the test table (bare heading, PART header, heading + page number) are exactly the ones I'd have asked for.
One defect to fix before this can land, plus a scoping correction that I think is good news for you.
Blocker: the predicate is unbounded, and misfires on a large section
is_cross_reference is an unanchored search() over the whole section text, with no constraint on how long the text is or where the match falls. For a 200-character stub that's fine. For a section of several thousand characters, one deferral clause anywhere in the body flips the diagnosis — and the message it flips to makes a specific factual claim that is then false.
ExxonMobil's 10-K Item 1, already in the fixture corpus:
tests/fixtures/html/xom/10k/xom-10-k-2025-02-19.html
Item 1 band: {'low': 8034, 'high': 321384} actual: 7,208 chars
match at char 2423 (33% into the section): "contained in the Financial Section"
The matched sentence is ordinary Item 1 prose:
Operating data and industry segment information for the Corporation are contained in the Financial Section of this report under the following: "Management's Discussion and Analysis…" and Note 18.
That's a complete Business section — it runs to the standard "The SEC maintains an internet site…" boilerplate that normally closes Item 1, and it sits 10% under a floor tuned to large-caps. With this PR the caller is told:
The extraction is faithful to the document; the returned text is the pointer, not the item's substance.
7,208 characters of Business narrative are the substance. This is the inverse of the bug you're fixing, and I'd argue the more expensive direction: the old message sent someone to debug a parser that worked, but this one tells them to stop looking at a section that has real content in it.
Suggested fix — bound the predicate on the two properties every true positive has: a pointer is short, and it defers at the top. All five of your Item 8 cases are ≤268 chars with the match starting immediately after the heading; XOM (7,208 chars, 33% in) and KO (12,718 chars, 7% in) both drop out under either bound. Something like:
# A pointer is short and defers at the top: it is the item's whole answer, not a
# sentence inside a real body. Bounding on both keeps an ordinary cross-reference
# sentence in a genuine (merely undersized) section from reading as a pointer.
_MAX_POINTER_CHARS = 1_500
_MAX_POINTER_OFFSET = 400 # past the item heading, before any real body
def is_cross_reference(text: Optional[str]) -> bool:
if not text or len(text) > _MAX_POINTER_CHARS:
return False
match = _CROSS_REFERENCE_RE.search(text)
return match is not None and match.start() <= _MAX_POINTER_OFFSETBoth constants want a comment saying they're calibrated against the corpus, and XOM wants a negative test alongside the six you already have — it's the case that would have caught this.
Your sweep was narrower than you thought — and you're doing better than you claimed
The PR says 39 10-K fixtures, five filers' messages change. Sweeping 10-K and 10-Q, 12 sections change diagnosis. Your "flag state is identical" claim holds exactly; the "only five messages differ" one doesn't.
Seven undisclosed changes, five of which are correct and worth keeping:
| filer | form | item | chars | verdict |
|---|---|---|---|---|
| ibm | 10-K | 7 | 212 | ✅ correct — IBM incorporates MD&A by reference too, not just Item 8 |
| ba | 10-Q | II-1 | 258 | ✅ correct — Legal Proceedings pointer |
| jnj | 10-Q | II-1 | 221 | ✅ correct |
| jpm | 10-Q | II-1 | 252 | ✅ correct |
| nflx | 10-Q | II-1 | 223 | ✅ correct |
| xom | 10-K | 1 | 7,208 | ❌ false positive (above) |
| ko | 10-Q | II-1 | 12,718 | ❌ false positive — opens with a real pointer sentence, then carries 12k of content |
So the change generalises past Item 8 on its own, which is a better result than the PR claims. Please widen the sweep to 10-Q and pull IBM Item 7 and one of the 10-Q Legal Proceedings filers into the parametrized ground-truth test — right now nothing pins the behaviour you're actually shipping.
Reproduction for both, if useful:
from pathlib import Path
from edgar.documents.config import ParserConfig
from edgar.documents.parser import HTMLParser
for form_dir, form in (("10k", "10-K"), ("10q", "10-Q")):
for p in sorted(Path("tests/fixtures/html").glob(f"*/{form_dir}/*.html")):
doc = HTMLParser(ParserConfig(form=form, detect_sections=True)).parse(p.read_text())
for s in doc.sections.values():
for w in (s.warnings or []):
if "incorporation by reference" in w:
print(f"{form} {p.parts[3]:8} {s.name:20} {len(s.text()):>8,}")Not yours to fix — but it explains the 10-Q rows
While checking the above I found a pre-existing bug that this PR touches the blast radius of: 13 of 16 10-Q fixtures have part_ii_item_1 (Legal Proceedings) flagged against Part I Item 1's (Financial Statements) 18,009-char floor. The bands are keyed on the bare item number and ignore the part, so a form where "Item 1" means two different things gets one band for both. Nearly every 10-Q Legal Proceedings section in the corpus is flagged as anomalous today.
That's not this PR's doing and I don't want it in scope — I'm filing it separately. Worth knowing because it means several of the 10-Q rows above are sections that shouldn't be flagged at all; your change is making their message better rather than making them right.
CI
I approved the workflow run — this PR had never had CI, which is on me. It failed twice on a GitHub Actions infrastructure outage ("Failed to resolve action download info. Error: Service Unavailable") in the cassette gate before any test ran, so there's still no signal. I'll re-approve after your next push.
Two things worth knowing about what CI will and won't tell you here:
test-slowis skipped on PRs by design, and the regression workflow only runs on pushes tomain. So your@pytest.mark.slowground-truth parametrization and both end-to-end regression tests do not run on this PR — only the fast unit tests do. That's a repo property, not a problem with your tests, but it's why I ran the full set locally rather than waiting on the gate. For the record, all 32 pass, and the six fixtures you rely on are tracked in git (I checked — untracked fixtures silently skipping in CI has bitten us recently).- Your branch is 25 commits behind
main. On the PR branch in isolation,test_section_boundary_corpus.py::test_anomaly_census_has_not_grownfails on('c', '10-K')— that's Citigroup, added toANOMALY_BASELINEon main in #985, nothing to do with you. It resolves on rebase, and neither file you touched has moved on main since your merge base, so it'll be clean.
Your two questions
1. Confidence for a pointer. Keep ANOMALOUS_CONFIDENCE. Your reasoning is the one I'd use — a pointer isn't the item's content, so a caller filtering on confidence should still not treat it as the financial statements. "Correct but deferred" is a real third state, but it belongs in the confidence-signalling work as a deliberate design, not smuggled in here. Don't change it in this PR.
2. Reachability. Agreed it's out of scope, and thank you for leaving it out. When we do it: an accessor, not warnings.warn and not a log warning. An incorporation-by-reference Item 8 is a correctly filed document, and emitting to stderr every time someone reads one would train users to ignore the channel. I'll open a follow-up for the accessor design.
Once the predicate is bounded and the sweep is widened, this is ready. Nice piece of work — the failure analysis in the description is the reason this was quick to review.
…ize section warnings The size guardrail flags a section whose content falls below its band, and tells the caller the anchor probably landed on a heading and the extraction was likely truncated. For an Item 8 that a filer answered with a cross-reference, that is the wrong diagnosis: the extraction is faithful to the document and the statements are filed elsewhere (NVDA files them under Item 15). The caller is sent to debug a parser that did its job. Every undersized Item 8 in the fixture corpus is one of these — NVDA (207 chars), NFLX (268), IBM (250), ORCL (158), CIK 915358 (112) — so the warning is currently misattributed in all five cases, and none are truncated extractions. On the undersize side only, test the section text for a deferral before writing the warning. A pointer gets an incorporation-by-reference warning naming where the content actually lives; a section with no deferral keeps the truncation warning. Both keep the reduced confidence: a pointer is still not the item's substance, so what callers receive is unchanged — only what they are told about it. The text is extracted only for sections the bands already flagged, so a healthy filing does no extra work, and the oversize path never pays for it. Closes dgunning#927
c816340 to
af61de3
Compare
|
Thanks — the XOM case is a fair hit and the failure mode you describe is the right way round: The predicate is now bounded on both length and match offset, as you suggested. Measuring
Longest true pointer is 268 chars against the 1,500 cap; furthest true match is offset 232 One deviation from your suggestion, and I want to flag it rather than slip it past you. Your Worth naming the asymmetry too, since it drove the values: these bounds fail toward the old Sweep widened to both forms. You were right that mine was too narrow, and the description Tests. XOM is in as a negative on the real fixture, asserting 7,208 chars, Rebased onto current main. One number in my original description I cannot stand behind. I claimed 134 passing across Leaving confidence at |
dgunning
left a comment
There was a problem hiding this comment.
Approving. This is careful work, and the second round answered the XOM point properly rather than papering over it.
I re-verified independently rather than reading:
Merges cleanly with current main. The branch is 92 commits behind, so this was the thing most likely to have rotted, and it hasn't — no conflicts, and the full fast suite comes back 5,421 passed on the merged result. Your 37 pass both before and after the merge.
The predicate does what the docstring says. Tested is_cross_reference directly against the pointer shapes and against the things that must not match:
| classified as pointer | rejected |
|---|---|
| NVDA, NFLX, IBM, ORCL, CIK 915358, JNJ 10-Q | bare Item 8. heading |
| narrative prose containing "included" | |
| XOM-shaped long body with a mid-text deferral | |
| KO-shaped long body with an opening deferral |
6/6 and 4/4. Both bounds earn their place, exactly as your table argues.
Two things I want to note for the record, because they're the reason this is an easy approval:
You flagged a deviation instead of slipping it past. The earlier review said a pointer "defers at the top"; you measured and found BA at offset 232 of 258 and NFLX 10-Q at 190 of 223, and rewrote the comment to describe what the bound actually does rather than what it was asked to do. A comment that claims something the fixtures contradict is a trap for the next reader, and you removed it.
You reasoned about which way to fail. An unusually verbose pointer keeps the old truncation message — wrong, but wrong in the way it already was. A false positive tells someone to stop reading a section that has real content. Those aren't equally expensive and the bounds prefer the cheaper mistake. That asymmetry is the right instinct, and it's worth saying out loud that it's why the conservative constants are correct rather than timid.
One nit, not blocking, take it or leave it:
except Exception:
logger.debug("Section %s: cross-reference test failed; keeping the size warning", ...)The fallback behaviour is right — keep the size warning, degrade to the previous diagnosis. But at debug level, a section.text() that starts failing systematically would be invisible. Naming the exception you expect, or lifting it to info, would make a persistent failure findable without changing the behaviour. Entirely your call; I'm not holding the merge on it.
Thanks for the sweep table in particular — having the offsets and lengths written down is what made this reviewable at all.
Minor, not patch: edgar.settings is a new public module. Nothing is removed or renamed -- edgar.core re-exports the same objects -- and dropping the legacy parser fallback from TenK/TenQ/TwentyF.items changed the item list on zero of 115 corpus filings. Seventeen entries fold into ## [5.52.0] - 2026-08-22. The headline is one story: the modern parser now answers every item lookup that used to need the deprecated ChunkedDocument, back to 1996 filings, which is the gate for removing edgar.files in 6.0. Alongside it two data-correctness fixes that each returned a plausible wrong answer rather than an error -- get_operating_cash_flow() returning None for Apple (#1083), and every fund series and class name degrading to its bare identifier after SEC moved a dataset page (#1077). Four merged PRs had no changelog entry and were added while scoping: #1077, #1079, #1080, #959. The section was then trimmed from 185 words per entry to 115, back inside the range the rest of the file uses. Schedule 14D-9 (#940) is deliberately not in this release: still a draft, no review, no CI run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Targets the warning path for #927 (
edgartools-xrs0), per your steer — direction (1) only. No fallback/resolution work here; happy to have the design conversation on (2) separately.The gap
The size guardrail already catches NVDA's Item 8 — 207 chars against a 26,136 floor, confidence dropped to 0.5, warning attached. It just names the wrong cause:
Nothing was truncated. NVDA answered Item 8 with one sentence and filed the statements under Item 15 (107,787 chars in the same parse). The extraction is faithful; the warning sends the caller to debug a parser that did its job.
This isn't specific to NVDA, or even to Item 8. Sweeping every fixture in
tests/fixtures/html/across both forms (39 10-K, 16 10-Q), ten sections carry the new diagnosis, and every one is a genuine pointer. The five undersized Item 8s that motivated the change:Beyond those five, the same predicate catches IBM's 10-K Item 7 (212 chars — IBM incorporates its MD&A by reference too, not just Item 8) and four 10-Q Part II Item 1 Legal Proceedings pointers (BA, JNJ, JPM, NFLX, 221–258 chars), so the mechanism generalises past Item 8 rather than being an Item 8 special case. Two near misses are why the predicate is bounded on length and match offset: XOM's 10-K Item 1 (7,208 chars, deferral sentence 2,423 chars in) and KO's 10-Q Part II Item 1 (12,718 chars, a pointer opening sentence with 12k of real content behind it) both contain deferral language inside genuine bodies, and both fall outside the bounds. Flag state is identical before and after on every section in the sweep; only the message changes.
None of the ten are truncated extractions, so the low-side warning is currently misattributed every time it fires on them.
section_size_bands's module docstring andtest_nflx_undersized_item8_is_flaggedboth encode the same misreading; this PR corrects them.The change
On the undersize side only, test the section text for a deferral before writing the warning:
cross_reference_warning, naming where the content actually livesConfidence still drops to
ANOMALOUS_CONFIDENCEin both cases — a pointer is not the item's substance either — so callers receive exactly what they received before. Only the diagnosis changes. Non-breaking.The pattern needs both halves of a deferral (a verb like "set forth" / "submitted" / "listed", plus a target like "Item 15" / "Part IV" / "Annual Report" / "separate section"), matched inside one clause, or an outright "incorporated herein by reference". A bare "included" or a bare "Item 15" is ordinary prose and doesn't match. It's a tie-breaker between two diagnoses on sections the bands already flagged, not a general-purpose classifier — which is what keeps its false-positive surface small. The predicate is also bounded on size and position: a pointer must be under 1,500 characters with its match inside the first 400. Both constants are calibrated against the corpus and both are load-bearing — XOM's 10-K Item 1 fails both, KO's 10-Q Part II Item 1 fails only the length bound.
Cost:
section.text()runs only for sections already flagged undersize. Healthy filings do no extra work, and there's a test asserting the oversize path never triggers the extraction.I kept this out of
evaluate_sizeso that function stays pure length-in/string-out and its existing tests stay meaningful; the new predicates sit alongside it and the detector composes them.Verification
tests/test_section_size_guardrail.pyandtests/issues/regression/test_issue_927_item8_cross_reference.py(the 32 originals plus 5 new, slow marks included, nothing deselected).test_anomaly_census_has_not_grownpasses.ruff checkclean on the changed files (no new findings).New coverage: the five real Item 8 bodies recognised; eight negatives rejected (bare heading, PART header, heading + page number, ordinary prose, empty,
None, plus two bound-shaped cases — a body over the length cap, and one under it with the deferral past offset 400); a slow negative on the real XOM fixture asserting 7,208 chars and the truncation wording; a detector-level test that a pointer replaces the warning while a non-pointer keeps it; a cost test that oversize sections are never re-extracted; and an NVDA end-to-end regression test with ground-truth assertions (207 chars, exact text, Item 15 present and >50k). The regression test is offline (checked-in fixture, no network), so it runs under the PR gating in #958.Two things I'd like your read on
ANOMALOUS_CONFIDENCE, on the grounds that a cross-reference is still not the item's content. But it's arguably a correct extraction of an unusual filing, so a separate signal ("correct but deferred") might belong in the confidence-signaling work rather than reusing the anomaly value. Your call — happy to change it.Section.warnings, but the accessor most callers use —tenk["Item 8"]— returns bare text, which is why my harness never saw a signal at all. Surfacing it there touches public API, so I've deliberately left it out of this PR. Want it as a follow-up, and if so should it be a log warning, awarnings.warn, or an accessor for a section's warnings?Closes #927