From 4754dbc17b574eeb70da4b3737f0b8af1a2074c6 Mon Sep 17 00:00:00 2001 From: Dennis Thiessen Date: Wed, 22 Jul 2026 16:40:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(sec):=20A3=20slice-2a=20review=20=E2=80=94?= =?UTF-8?q?=20shares=20fallback,=20robust=20context,=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Multi-class shares: prefer the single dei:EntityCommonStockSharesOutstanding cover-page fact; else fall back to us-gaap:CommonStockSharesOutstanding at period end (Alphabet has no dei fact). Never sum class facts (companyfacts is non-dimensional) and never use weighted-average/diluted; conflicting values -> null, counted as an "ambiguous shares outstanding" note in validation. Plan's "sum class-specific" wording corrected. Verified live: Alphabet shares now populate (12.1B), Apple still uses its dei cover date. 2. Fiscal context is the majority (fy, fp) among facts ending at reportDate, with ties rejected — no longer the arbitrary first fact. 3. Hardening: catalog selectors require taxonomy == "us-gaap"; indexing drops malformed facts (missing accession/end, non-finite value) so a custom concept or bad date can't be selected. Tests: +8 (dei precedence, us-gaap fallback, conflict->null, no weighted-average, tie-context skip, foreign-taxonomy/malformed ignored, ambiguous-shares note). 14 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 --- app/services/sec_facts_parser.py | 113 +++++++++++++++++++++------- docs/dolt-integration-plan.md | 14 ++-- tests/unit/test_sec_facts_parser.py | 91 ++++++++++++++++++++++ 3 files changed, 183 insertions(+), 35 deletions(-) diff --git a/app/services/sec_facts_parser.py b/app/services/sec_facts_parser.py index 2c7e4d7..8052a9a 100644 --- a/app/services/sec_facts_parser.py +++ b/app/services/sec_facts_parser.py @@ -20,7 +20,8 @@ The load-bearing rules (design Decision 2 + review): from __future__ import annotations import logging -from dataclasses import dataclass, field +import math +from dataclasses import dataclass from datetime import date, datetime from typing import Any, NamedTuple @@ -126,11 +127,13 @@ def parse_snapshots( if meta is None or not facts: skips.append({"accession": accn, "reason": "no facts or filing metadata"}) continue - row = _parse_one(cik, accn, facts, meta) + row, note = _parse_one(cik, accn, facts, meta) if row is None: - skips.append({"accession": accn, "reason": "no usable period identity"}) + skips.append({"accession": accn, "reason": note or "unparseable"}) continue rows.append(row) + if note: # row produced, but a field-level issue to count in validation + skips.append({"accession": accn, "reason": note}) return rows, skips @@ -142,7 +145,12 @@ def _index_by_accession(companyfacts: dict[str, Any]) -> dict[str, list[Fact]]: for unit, facts in body.get("units", {}).items(): for f in facts: accn = f.get("accn") - if not accn: + end = _d(f.get("end")) + val = f.get("val") + # Skip malformed facts so they can't be selected accidentally: + # every usable fact needs an accession, an end date, and a + # finite numeric value. + if not accn or end is None or not _finite(val): continue out.setdefault(accn, []).append( Fact( @@ -150,8 +158,8 @@ def _index_by_accession(companyfacts: dict[str, Any]) -> dict[str, list[Fact]]: concept=concept, unit=unit, start=_d(f.get("start")), - end=_d(f.get("end")), - val=f.get("val"), + end=end, + val=val, fy=f.get("fy"), fp=f.get("fp"), ) @@ -159,10 +167,15 @@ def _index_by_accession(companyfacts: dict[str, Any]) -> dict[str, list[Fact]]: return out -def _parse_one(cik: str, accn: str, facts: list[Fact], meta: FilingMeta) -> SnapshotRow | None: - fy, fp = _fiscal_context(facts) +def _parse_one( + cik: str, accn: str, facts: list[Fact], meta: FilingMeta +) -> tuple[SnapshotRow | None, str | None]: + """Returns (row, note). row is None when there's no usable period identity; + note is a validation reason (row-skip reason when row is None, else a + field-level issue such as ambiguous shares).""" + fy, fp = _fiscal_context(facts, meta.report_date) if fy is None or fp not in _EXPECTED_YTD_DAYS: - return None + return None, "no usable period identity" row = SnapshotRow( cik=cik, @@ -189,16 +202,26 @@ def _parse_one(cik: str, accn: str, facts: list[Fact], meta: FilingMeta) -> Snap # balance-sheet instants at reportDate row.cash_and_st_investments = _compose_cash(facts, meta.report_date) row.total_debt = _compose_debt(facts, meta.report_date) - row.shares_outstanding, row.shares_outstanding_date = _select_shares(facts) - return row + shares, shares_date, ambiguous = _select_shares(facts, meta.report_date) + row.shares_outstanding = shares + row.shares_outstanding_date = shares_date + return row, ("ambiguous shares outstanding" if ambiguous else None) -def _fiscal_context(facts: list[Fact]) -> tuple[int | None, str | None]: - """A filing's own (fy, fp) — shared by all its facts; take the first set.""" +def _fiscal_context(facts: list[Fact], report_date: date) -> tuple[int | None, str | None]: + """The filing's (fy, fp) taken as the majority context among the facts that + end at reportDate (the current-period facts, which share the filing's + context). Reject a tie so a conflicting context is never chosen arbitrarily.""" + counts: dict[tuple[int, str], int] = {} for f in facts: - if f.fy is not None and f.fp: - return f.fy, f.fp - return None, None + if f.end == report_date and f.fy is not None and f.fp: + counts[(f.fy, f.fp)] = counts.get((f.fy, f.fp), 0) + 1 + if not counts: + return None, None + ranked = sorted(counts.items(), key=lambda kv: kv[1], reverse=True) + if len(ranked) > 1 and ranked[0][1] == ranked[1][1]: + return None, None # tie → conflicting contexts, reject + return ranked[0][0] def _select_ytd( @@ -212,11 +235,11 @@ def _select_ytd( best_diff: int | None = None for f in facts: if ( - f.concept != concept + f.taxonomy != "us-gaap" + or f.concept != concept or f.unit != unit or f.start is None or f.end != report_date - or f.val is None ): continue diff = abs((f.end - f.start).days - expected) @@ -232,11 +255,11 @@ def _select_instant(facts: list[Fact], concepts: list[str], report_date: date) - for concept in concepts: for f in facts: if ( - f.concept == concept + f.taxonomy == "us-gaap" + and f.concept == concept and f.unit == "USD" and f.start is None and f.end == report_date - and f.val is not None ): return float(f.val) return None @@ -262,22 +285,49 @@ def _compose_debt(facts: list[Fact], report_date: date) -> float | None: return (long_term or 0.0) + (short_term or 0.0) -def _select_shares(facts: list[Fact]) -> tuple[float | None, date | None]: - """dei:EntityCommonStockSharesOutstanding — cover-page instant. Store its own - end (the cover date, which differs from period_end).""" - candidates = [ +def _select_shares( + facts: list[Fact], report_date: date +) -> tuple[float | None, date | None, bool]: + """Issuer-wide shares outstanding as a single consolidated value (never a + class sum — companyfacts is non-dimensional — and never weighted-average/ + diluted). Returns (value, shares_date, ambiguous). + + 1. Prefer the `dei:EntityCommonStockSharesOutstanding` cover-page instant; + its own end is the shares date (cover date != period_end). + 2. Else fall back to `us-gaap:CommonStockSharesOutstanding` at period end + (e.g. Alphabet has no dei fact); shares date = reportDate. + Conflicting values within the chosen source → (None, None, True) to be + counted in validation. + """ + dei = [ f for f in facts if f.taxonomy == "dei" and f.concept == "EntityCommonStockSharesOutstanding" and f.unit == "shares" and f.start is None - and f.val is not None ] - if not candidates: - return None, None - best = max(candidates, key=lambda f: f.end) - return float(best.val), best.end + if dei: + if len({f.val for f in dei}) > 1: + return None, None, True + best = max(dei, key=lambda f: f.end) + return float(best.val), best.end, False + + gaap = [ + f + for f in facts + if f.taxonomy == "us-gaap" + and f.concept == "CommonStockSharesOutstanding" + and f.unit == "shares" + and f.start is None + and f.end == report_date + ] + if gaap: + if len({f.val for f in gaap}) > 1: + return None, None, True + return float(gaap[0].val), report_date, False + + return None, None, False # simply absent — not a conflict def _d(value: Any) -> date | None: @@ -287,3 +337,8 @@ def _d(value: Any) -> date | None: return date.fromisoformat(str(value)[:10]) except ValueError: return None + + +def _finite(value: Any) -> bool: + """True for a finite numeric value (rejects None, bool, strings, NaN/inf).""" + return isinstance(value, (int, float)) and not isinstance(value, bool) and math.isfinite(value) diff --git a/docs/dolt-integration-plan.md b/docs/dolt-integration-plan.md index 036a077..d29e2c0 100644 --- a/docs/dolt-integration-plan.md +++ b/docs/dolt-integration-plan.md @@ -253,12 +253,14 @@ that scoring already reads, refreshed daily by step (c) after activation. | Earnings surprise history | last 4+ from `earnings_events` | query | **Market cap is an estimate** (issuer-wide shares outstanding × one ticker's price — -approximate for multi-class issuers). For a multi-class issuer, derive the -issuer-wide share count **either** from the consolidated cover-page figure **or** -by summing the class-specific `dei:EntityCommonStockSharesOutstanding` facts -(GOOG + GOOGL) — **never both**, or the count double-counts. Label it "est." in the -UI and round aggressively rather than withholding it; false precision is the failure -mode, not the approximation. +approximate for multi-class issuers). Share count comes from a single consolidated +value, not a class sum: prefer the one `dei:EntityCommonStockSharesOutstanding` +cover-page fact; if absent (e.g. Alphabet) fall back to +`us-gaap:CommonStockSharesOutstanding` at period end. companyfacts is +non-dimensional, so class-specific facts can't be summed reliably — never do that, +and never substitute weighted-average/diluted shares; if conflicting values remain, +store null. Label it "est." in the UI and round aggressively rather than withholding +it; false precision is the failure mode, not the approximation. **Units follow existing app conventions:** percentages are percentage points (21.0 = 21%), P/E and net-debt/EBITDA are multiples, market cap and net debt are diff --git a/tests/unit/test_sec_facts_parser.py b/tests/unit/test_sec_facts_parser.py index 0f326e9..df5e380 100644 --- a/tests/unit/test_sec_facts_parser.py +++ b/tests/unit/test_sec_facts_parser.py @@ -13,6 +13,8 @@ from app.services.sec_facts_parser import ( FilingMeta, _compose_cash, _compose_debt, + _fiscal_context, + _select_shares, parse_snapshots, ) @@ -135,6 +137,95 @@ def test_cash_picks_one_st_investment_source(): assert _compose_cash(facts, rd) == 45000 +RD = date(2026, 3, 28) + + +def _dei(end, val): + return Fact("dei", "EntityCommonStockSharesOutstanding", "shares", None, end, val, 2026, "Q2") + + +def _gaap_shares(end, val): + return Fact("us-gaap", "CommonStockSharesOutstanding", "shares", None, end, val, 2026, "Q2") + + +def test_shares_prefers_dei_cover_page(): + facts = [_dei(date(2026, 4, 17), 100), _gaap_shares(RD, 999)] + assert _select_shares(facts, RD) == (100.0, date(2026, 4, 17), False) + + +def test_shares_falls_back_to_usgaap_at_report_date(): + # Alphabet case: no dei fact; us-gaap current + a prior comparative. + facts = [_gaap_shares(date(2025, 12, 31), 888), _gaap_shares(RD, 12116)] + assert _select_shares(facts, RD) == (12116.0, RD, False) # comparative excluded + + +def test_shares_conflict_returns_null_ambiguous(): + facts = [_dei(RD, 100), _dei(RD, 200)] # two differing consolidated values + assert _select_shares(facts, RD) == (None, None, True) + + +def test_shares_never_uses_weighted_average(): + facts = [Fact("us-gaap", "WeightedAverageNumberOfDilutedSharesOutstanding", "shares", None, RD, 5, 2026, "Q2")] + assert _select_shares(facts, RD) == (None, None, False) # not a shares source + + +def test_conflicting_fiscal_context_is_rejected(): + facts = [ + Fact("us-gaap", "Revenues", "USD", date(2025, 9, 28), RD, 1, 2026, "Q2"), + Fact("us-gaap", "NetIncomeLoss", "USD", date(2025, 9, 28), RD, 2, 2025, "Q3"), + ] # 1-1 tie between two contexts at reportDate + assert _fiscal_context(facts, RD) == (None, None) + # a clear majority wins + facts.append(Fact("us-gaap", "OperatingIncomeLoss", "USD", date(2025, 9, 28), RD, 3, 2026, "Q2")) + assert _fiscal_context(facts, RD) == (2026, "Q2") + + +def test_conflicting_context_skips_row(): + cf = {"cik": 1, "facts": {"us-gaap": { + "Revenues": {"units": {"USD": [_dur("2025-09-28", "2026-03-28", 1, "B", fp="Q2")]}}, + "NetIncomeLoss": {"units": {"USD": [_dur("2025-09-28", "2026-03-28", 2, "B", fy=2025, fp="Q3")]}}, + }}} + filings = {"B": FilingMeta(RD, date(2026, 5, 1), datetime(2026, 5, 1, tzinfo=UTC), "10-Q")} + rows, skips = parse_snapshots(cf, filings, {"B"}) + assert rows == [] and skips == [{"accession": "B", "reason": "no usable period identity"}] + + +def test_foreign_taxonomy_and_malformed_facts_ignored(): + cf = {"cik": 1, "facts": { + "us-gaap": { + "Revenues": {"units": {"USD": [_dur("2025-09-28", "2026-03-28", 500, "B")]}}, + "NetIncomeLoss": {"units": {"USD": [ + {"start": "2025-09-28", "end": "2026-03-28", "val": None, "fy": 2026, "fp": "Q2", "accn": "B"}, + ]}}, + "OperatingIncomeLoss": {"units": {"USD": [ + {"start": "2025-09-28", "end": "2026-03-28", "val": float("nan"), "fy": 2026, "fp": "Q2", "accn": "B"}, + ]}}, + }, + "acme": {"RevenueFromContractWithCustomerExcludingAssessedTax": {"units": {"USD": [ + _dur("2025-09-28", "2026-03-28", 99999, "B"), # custom taxonomy — must be ignored + ]}}}, + }} + filings = {"B": FilingMeta(RD, date(2026, 5, 1), datetime(2026, 5, 1, tzinfo=UTC), "10-Q")} + rows, _ = parse_snapshots(cf, filings, {"B"}) + assert rows[0].revenue == 500 # us-gaap Revenues, not the acme concept + assert rows[0].net_income is None # val None ignored + assert rows[0].operating_income is None # NaN ignored + + +def test_ambiguous_shares_produces_row_plus_note(): + cf = {"cik": 1, "facts": { + "us-gaap": {"Revenues": {"units": {"USD": [_dur("2025-09-28", "2026-03-28", 500, "B")]}}}, + "dei": {"EntityCommonStockSharesOutstanding": {"units": {"shares": [ + {"end": "2026-04-17", "val": 100, "fy": 2026, "fp": "Q2", "accn": "B"}, + {"end": "2026-04-17", "val": 200, "fy": 2026, "fp": "Q2", "accn": "B"}, + ]}}}, + }} + filings = {"B": FilingMeta(RD, date(2026, 5, 1), datetime(2026, 5, 1, tzinfo=UTC), "10-Q")} + rows, skips = parse_snapshots(cf, filings, {"B"}) + assert len(rows) == 1 and rows[0].shares_outstanding is None # row kept, shares null + assert {"accession": "B", "reason": "ambiguous shares outstanding"} in skips + + # Opt-in live check against real Apple companyfacts. Skips unless SEC_LIVE=1 and a # real SEC_USER_AGENT are set (network + fair-access contact email). @pytest.mark.skipif(