From 3a2e8fd290aabc01e207071877bd6c699e9ebef7 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Fri, 25 Sep 2026 07:10:44 +0800 Subject: [PATCH 1/3] edgar_read: list 13F holdings instead of returning None (#1337) _extract_13f_section tested the ThirteenF.holdings DataFrame for truth, which raises; the broad except returned None at DEBUG. Walk the first 30 rows with iterrows() instead and log extraction failures at warning. Co-Authored-By: Claude Opus 5.5 --- changelog.d/1337.fixed.md | 1 + edgar/ai/mcp/tools/reader.py | 31 ++++++++++++++---------- tests/ai/test_mcp_intent_tools.py | 39 +++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 12 deletions(-) create mode 100644 changelog.d/1337.fixed.md diff --git a/changelog.d/1337.fixed.md b/changelog.d/1337.fixed.md new file mode 100644 index 000000000..7b3063b11 --- /dev/null +++ b/changelog.d/1337.fixed.md @@ -0,0 +1 @@ +**`edgar_read` returned no holdings section for any 13F-HR.** A filing whose summary reported `Total Holdings: 211` came back with `holdings` set to `None`, because the section tested a DataFrame for truth and then looped over its column names. It now lists the top 30 rows by value (issuer, shares, value), and an extraction failure logs at warning level. (GH #1337) diff --git a/edgar/ai/mcp/tools/reader.py b/edgar/ai/mcp/tools/reader.py index dcb0bd849..e6ccad411 100644 --- a/edgar/ai/mcp/tools/reader.py +++ b/edgar/ai/mcp/tools/reader.py @@ -10,6 +10,9 @@ import logging from typing import Any, Optional + +import pandas as pd + from edgar import find from edgar.ai.mcp.tools.base import ( tool, @@ -473,23 +476,27 @@ def _extract_13f_section(obj, section: str) -> Optional[str]: """Extract sections from a 13F-HR (ThirteenF) object.""" if section == "holdings": try: - if hasattr(obj, 'holdings') and obj.holdings: + # ``ThirteenF.holdings`` is a DataFrame: its truth value raises and + # iterating it yields column names, so walk its rows (as #1136 did + # for ``_get_fund_holdings`` in ownership.py). + holdings = getattr(obj, 'holdings', None) + if holdings is not None and not holdings.empty: parts = [] - for h in obj.holdings[:30]: # Limit to top 30 - name = getattr(h, 'name', None) or getattr(h, 'issuer', 'Unknown') - shares = getattr(h, 'shares', None) - value = getattr(h, 'value', None) - line = f" {name}" - if shares: - line += f" | {shares:,} shares" - if value: - line += f" | ${value:,}" + for _, row in holdings.head(30).iterrows(): # Limit to top 30 + name = row.get('Issuer') + shares = row.get('SharesPrnAmount') + value = row.get('Value') + line = f" {name if pd.notna(name) else 'Unknown'}" + if pd.notna(shares) and shares: + line += f" | {int(shares):,} shares" + if pd.notna(value) and value: + line += f" | ${int(value):,}" parts.append(line) - total = len(obj.holdings) + total = len(holdings) header = f"Top holdings ({min(30, total)} of {total}):" return header + "\n" + "\n".join(parts) except Exception as e: - logger.debug(f"Could not extract 13F holdings: {e}") + logger.warning(f"Could not extract 13F holdings: {e}") return None elif section == "summary": diff --git a/tests/ai/test_mcp_intent_tools.py b/tests/ai/test_mcp_intent_tools.py index b7bd419f2..38b73e3d9 100644 --- a/tests/ai/test_mcp_intent_tools.py +++ b/tests/ai/test_mcp_intent_tools.py @@ -412,6 +412,45 @@ async def test_read_no_params_returns_error(self): assert result.success is False +class TestExtract13FHoldings: + """13F holdings section reads rows from the ThirteenF.holdings DataFrame (#1337).""" + + @staticmethod + def _fake_13f(holdings): + class FakeThirteenF: + pass + + obj = FakeThirteenF() + obj.holdings = holdings + return obj + + def test_holdings_section_lists_rows(self): + import pandas as pd + from edgar.ai.mcp.tools.reader import _extract_13f_section + + holdings = pd.DataFrame( + [ + dict(Issuer=f"ISSUER {i}", Class="COM", Cusip=f"C{i:08d}", Ticker=f"T{i}", + SharesPrnAmount=1000 + i, Value=float(100000 - i)) + for i in range(211) + ] + ) + result = _extract_13f_section(self._fake_13f(holdings), "holdings") + + lines = result.splitlines() + assert lines[0] == "Top holdings (30 of 211):" + assert lines[1] == " ISSUER 0 | 1,000 shares | $100,000" + assert lines[30] == " ISSUER 29 | 1,029 shares | $99,971" + assert len(lines) == 31 + + def test_holdings_section_empty_or_missing_returns_none(self): + import pandas as pd + from edgar.ai.mcp.tools.reader import _extract_13f_section + + assert _extract_13f_section(self._fake_13f(pd.DataFrame()), "holdings") is None + assert _extract_13f_section(self._fake_13f(None), "holdings") is None + + # ============================================================================= # edgar_compare Tool Tests (network required) # ============================================================================= From 63dbcb3445b15f4435e830911d9fa01542395d67 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Sat, 26 Sep 2026 18:32:41 +0800 Subject: [PATCH 2/3] edgar_read: share the 13F row reader with edgar_ownership (#1337) _get_fund_holdings already walked ThirteenF.holdings with the _cell_text and _cell_number helpers. Pull that loop into base._holding_rows and use it from both tools instead of keeping a second copy in reader.py. --- edgar/ai/mcp/tools/base.py | 30 ++++++++++++++++++++++++++++++ edgar/ai/mcp/tools/ownership.py | 27 ++------------------------- edgar/ai/mcp/tools/reader.py | 23 +++++++++-------------- tests/ai/test_mcp_intent_tools.py | 21 +++++++++++++++++++++ 4 files changed, 62 insertions(+), 39 deletions(-) diff --git a/edgar/ai/mcp/tools/base.py b/edgar/ai/mcp/tools/base.py index f36db42e9..1a665023d 100644 --- a/edgar/ai/mcp/tools/base.py +++ b/edgar/ai/mcp/tools/base.py @@ -87,6 +87,36 @@ def _cell_number(value: Any, as_int: bool = False) -> Optional[Union[int, float] return value +def _holding_rows(holdings_table: Any, limit: int) -> list[dict[str, Any]]: + """Read the first ``limit`` rows of a ``ThirteenF.holdings`` frame. + + Iterating a DataFrame yields its column names, not its rows, so every tool + that walked the table directly came back empty. Each row becomes a dict + with ``company``, ``cusip``, ``shares`` and ``value``, each key present + only when its cell holds something: an explicit ``"value": None`` reads as + a value to a caller testing ``"value" in holding``. Rows with no usable + cell are skipped. + """ + rows = [] + for _, row in holdings_table.head(limit).iterrows(): + holding: dict[str, Any] = {} + issuer = _cell_text(row.get("Issuer")) + if issuer: + holding["company"] = issuer + cusip = _cell_text(row.get("Cusip")) + if cusip: + holding["cusip"] = cusip + shares = _cell_number(row.get("SharesPrnAmount"), as_int=True) + if shares is not None: + holding["shares"] = shares + value = _cell_number(row.get("Value"), as_int=True) + if value is not None: + holding["value"] = value + if holding: + rows.append(holding) + return rows + + # ============================================================================= # RESPONSE TYPES # ============================================================================= diff --git a/edgar/ai/mcp/tools/ownership.py b/edgar/ai/mcp/tools/ownership.py index e8deece06..2ea97495a 100644 --- a/edgar/ai/mcp/tools/ownership.py +++ b/edgar/ai/mcp/tools/ownership.py @@ -17,6 +17,7 @@ get_error_suggestions, _cell_number, _cell_text, + _holding_rows, ) logger = logging.getLogger(__name__) @@ -228,31 +229,7 @@ async def _get_fund_holdings(identifier: str, limit: int) -> Any: holdings_table = getattr(obj, 'holdings', None) if holdings_table is not None: - # ``ThirteenF.holdings`` is a DataFrame, and iterating one yields - # its column names rather than its rows: every attribute probe - # missed and the list came back empty for every fund. Walk it the - # way ``_get_portfolio_diff`` below already walks its comparison. - holdings = [] - for _, row in holdings_table.head(limit).iterrows(): - holding = {} - issuer = _cell_text(row.get("Issuer")) - if issuer: - holding["company"] = issuer - cusip = _cell_text(row.get("Cusip")) - if cusip: - holding["cusip"] = cusip - # Independently, the way company and cusip are: writing the - # pair together put an explicit `"value": null` in the - # response for a row whose value did not parse, and a caller - # testing `"value" in holding` reads that as a value. - shares = _cell_number(row.get("SharesPrnAmount"), as_int=True) - if shares is not None: - holding["shares"] = shares - value = _cell_number(row.get("Value"), as_int=True) - if value is not None: - holding["value"] = value - if holding: - holdings.append(holding) + holdings = _holding_rows(holdings_table, limit) result["holdings_count"] = len(holdings_table) result["holdings"] = holdings diff --git a/edgar/ai/mcp/tools/reader.py b/edgar/ai/mcp/tools/reader.py index e6ccad411..1c08e597b 100644 --- a/edgar/ai/mcp/tools/reader.py +++ b/edgar/ai/mcp/tools/reader.py @@ -10,9 +10,6 @@ import logging from typing import Any, Optional - -import pandas as pd - from edgar import find from edgar.ai.mcp.tools.base import ( tool, @@ -22,6 +19,7 @@ format_filing_summary, get_error_suggestions, truncate_text, + _holding_rows, ) logger = logging.getLogger(__name__) @@ -477,20 +475,17 @@ def _extract_13f_section(obj, section: str) -> Optional[str]: if section == "holdings": try: # ``ThirteenF.holdings`` is a DataFrame: its truth value raises and - # iterating it yields column names, so walk its rows (as #1136 did - # for ``_get_fund_holdings`` in ownership.py). + # iterating it yields column names, so read its rows the way + # ``_get_fund_holdings`` in ownership.py does. holdings = getattr(obj, 'holdings', None) if holdings is not None and not holdings.empty: parts = [] - for _, row in holdings.head(30).iterrows(): # Limit to top 30 - name = row.get('Issuer') - shares = row.get('SharesPrnAmount') - value = row.get('Value') - line = f" {name if pd.notna(name) else 'Unknown'}" - if pd.notna(shares) and shares: - line += f" | {int(shares):,} shares" - if pd.notna(value) and value: - line += f" | ${int(value):,}" + for h in _holding_rows(holdings, 30): # Limit to top 30 + line = f" {h.get('company', 'Unknown')}" + if "shares" in h: + line += f" | {h['shares']:,} shares" + if "value" in h: + line += f" | ${h['value']:,}" parts.append(line) total = len(holdings) header = f"Top holdings ({min(30, total)} of {total}):" diff --git a/tests/ai/test_mcp_intent_tools.py b/tests/ai/test_mcp_intent_tools.py index 38b73e3d9..2edbe6de4 100644 --- a/tests/ai/test_mcp_intent_tools.py +++ b/tests/ai/test_mcp_intent_tools.py @@ -426,6 +426,7 @@ class FakeThirteenF: def test_holdings_section_lists_rows(self): import pandas as pd + from edgar.ai.mcp.tools.reader import _extract_13f_section holdings = pd.DataFrame( @@ -443,8 +444,28 @@ def test_holdings_section_lists_rows(self): assert lines[30] == " ISSUER 29 | 1,029 shares | $99,971" assert len(lines) == 31 + def test_holdings_section_skips_missing_cells(self): + import pandas as pd + + from edgar.ai.mcp.tools.reader import _extract_13f_section + + holdings = pd.DataFrame( + [ + dict(Issuer="APPLE INC", Cusip="037833100", SharesPrnAmount=10.0, Value=float("nan")), + dict(Issuer=float("nan"), Cusip="594918104", SharesPrnAmount=float("nan"), Value=500.0), + ] + ) + result = _extract_13f_section(self._fake_13f(holdings), "holdings") + + assert result.splitlines() == [ + "Top holdings (2 of 2):", + " APPLE INC | 10 shares", + " Unknown | $500", + ] + def test_holdings_section_empty_or_missing_returns_none(self): import pandas as pd + from edgar.ai.mcp.tools.reader import _extract_13f_section assert _extract_13f_section(self._fake_13f(pd.DataFrame()), "holdings") is None From 1acac03d0ae10eba34df5067533fa5a998106be3 Mon Sep 17 00:00:00 2001 From: wolfgang-aura <169568318+wolfgang-aura@users.noreply.github.com> Date: Sat, 26 Sep 2026 19:41:05 +0800 Subject: [PATCH 3/3] test: mark the 13F holdings tests fast so the pull-request lane runs them (#1337) test_mcp_intent_tools.py is classed network by filename, which put the new offline tests outside the fast lane and left check_offline_audit.py with nothing to audit. They pass with outbound sockets blocked. --- tests/ai/test_mcp_intent_tools.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ai/test_mcp_intent_tools.py b/tests/ai/test_mcp_intent_tools.py index 2edbe6de4..d60d09933 100644 --- a/tests/ai/test_mcp_intent_tools.py +++ b/tests/ai/test_mcp_intent_tools.py @@ -412,6 +412,7 @@ async def test_read_no_params_returns_error(self): assert result.success is False +@pytest.mark.fast class TestExtract13FHoldings: """13F holdings section reads rows from the ThirteenF.holdings DataFrame (#1337)."""