edgar_read: list 13F holdings instead of returning None (#1337)

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)

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
 # =============================================================================

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

edgar/ai/mcp/tools/reader.py

@@ -19,6 +19,7 @@
     format_filing_summary,
     get_error_suggestions,
     truncate_text,
+    _holding_rows,
 )
 
 logger = logging.getLogger(__name__)
@@ -473,23 +474,24 @@ 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 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 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 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(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":

tests/ai/test_mcp_intent_tools.py

@@ -412,6 +412,67 @@ 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)."""
+
+    @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_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
+        assert _extract_13f_section(self._fake_13f(None), "holdings") is None
+
+
 # =============================================================================
 # edgar_compare Tool Tests (network required)
 # =============================================================================