Skip to content

feat(mcp): opt-in limit for edgar_read 13F holdings - #1548

Merged
dgunning merged 2 commits into
dgunning:mainfrom
MaxHaro:fix-1416-edgar-read-limit
Oct 15, 2026
Merged

dgunning merged 2 commits into
dgunning:mainfrom
MaxHaro:fix-1416-edgar-read-limit

Conversation

@MaxHaro

@MaxHaro MaxHaro commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

edgar_read stopped a 13F-HR holdings section at 30 rows and cut every section to 6,000 characters, so a full portfolio could not be read or reconciled to its filed total. It now takes an optional limit (default 30; 0 returns every row). 13F holdings skip the 6,000-character cap, since limit already bounds them; other sections keep it. The default output is unchanged for realistic issuer names.

limit is validated for all forms and applied only to 13F holdings. A negative or non-integer value (including True) returns an INVALID_ARGUMENTS error before any filing is fetched; integral floats like 30.0 are accepted, since the JSON schema counts them as integers. The error is a deliberate divergence from edgar_ownership (no validation) and edgar_fund (clamps), so limit=-1 doesn't quietly return nothing or everything.

Fixes #1416

Verification

  • Ground truth: Contrarius Group Holdings 13F-HR 0001999371-26-017606 (period 2026-06-30), run live on 2026-10-15. Default returns Top holdings (30 of 34); limit=0 returns all 34 rows (34 of 34), matching the filing's 34 holdings ($2,170,182,768 total). The tests use a synthetic 211-row fixture.
  • Regression test in tests/issues/regression/test_issue_1416_edgar_read_limit.py, docstring links edgar_read: opt-in limit so a full 13F portfolio can be read #1416; all 13 cases fail on main's reader.py
  • Silence check: negative, string, fractional and boolean limit return a clear error, not a silent result
  • Docs: docs/ai/mcp-tools.md, MCP_QUICKSTART.md and the server.py examples list limit
  • Changelog: changelog.d/1416.added.md

Working context

Written with AI assistance (Claude Code) and reviewed by me; I'm responsible for it.

Evidence: reproduced the cap offline (a 211-row fixture returned 30 rows). limit=0 is turned into the row count because DataFrame.head(0) is empty. The new tests pass; tests/ai and the related MCP regression tests pass (534 passed). ruff check adds no findings, and the provenance and changelog checks pass. Failures in the full fast suite on Windows are pre-existing on clean main (verified).

edgar_read stopped a 13F-HR holdings section at 30 rows, and every
section was then cut to 6,000 characters, so a full portfolio could not
be read or reconciled to the filing's total value.

Add an optional limit argument (default 30, so existing callers are
unchanged; 0 returns every row). It is threaded through edgar_read,
_extract_sections, _extract_section and _extract_13f_section. 13F
holdings skip the 6,000-character cap because limit already bounds
them; every other section keeps it. A negative or non-integer limit
returns an INVALID_ARGUMENTS error before any filing is fetched.

Fixes dgunning#1416

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@MaxHaro MaxHaro changed the title feat(mcp): opt-in limit for edgar_read 13F holdings (#1416) feat(mcp): opt-in limit for edgar_read 13F holdings Oct 15, 2026
@arhancanli

Copy link
Copy Markdown

Read the diff and checked the synthetic fixture: 211 rows give 188,845,000 in total and the first 30 give 29,565,000, as the test docstring says. The limit=0 path (head(0) is empty, so 0 becomes the row count) is handled correctly. I did not run the suite.

Three things I'd look at:

  1. Top holdings (N of M) assumes the frame is sorted by value. _holding_rows is holdings_table.head(limit), so it returns the first rows in whatever order ThirteenF.holdings has. If that order is the filing's order (not descending by Value), "Top" is wrong for any limit below the row count, and a caller reconciling to the filed total from a partial read gets a misleading subset. The Contrarius check (34 rows) can't show this, since limit=0 returns everything there. A test with an unsorted frame and limit=5 would settle it.

  2. shown can overstate what is returned. The header prints shown, but _holding_rows skips rows with no usable cell, so the line count can be lower than the number in the header. The new tests use clean rows, so they would not catch it. Counting len(parts) for the header is cheap.

  3. limit=0 has no ceiling at all, and these sections are now exempt from the 6,000-character cap. That is intended, but a large manager's 13F can have thousands of rows, and at roughly 60 characters a row that is hundreds of KB returned into a model's context in one tool call. Maybe a high hard maximum (or a note in the tool description about it) so one edgar_read call cannot return an unbounded payload.

The input validation (bool, negative, 30.0) reads well.

dgunning added a commit that referenced this pull request Oct 15, 2026
…ession lookup (#1554)

Opening a filing from an accession asks EDGAR full-text search first and the
quarterly filing index second (edgartools-vx29). #1498 made an EFTS error body
(HTTP 200, {"errorType": ...}) raise TransportError instead of reading as no
hits, and resolve_accession re-raises transport errors (07lk.10). So when SEC's
search backend failed on its own -- `getaddrinfo EBUSY` on 10-14 and 10-15 --
find(accession) raised for filings the index would have found. Five
regression tests failed that way across #1545, #1548 and #1550.

_filing_from_efts, the caller with a correct fallback, now logs a warning and
returns None on a transport error, so the index is asked. resolve_accession
still raises, and search_filings still raises, since there the error is the
answer. If SEC itself is unreachable, the index fetch raises the same error.

#1498 is unreleased, so no released version had this; no changelog fragment.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…t=0 size (dgunning#1416)

Review follow-up on the edgar_read limit.

The "Top holdings (N of M)" header used the number of rows requested, but
_holding_rows skips a row with no usable cell, so N could be larger than
the number of lines listed. N is now the number of rows actually listed.

Document that limit=0 returns every row, which for a large manager can be
thousands of rows and a large payload, in the tool schema, the MCP tools
reference and the quickstart.

No sort is added: ThirteenF.holdings is already sorted by Value
descending, which tests/thirteenf/test_holdings_aggregation.py pins, so
the first rows are the top holdings. A comment in the extractor says so.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@MaxHaro

MaxHaro commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@arhancanli Thanks for the careful review, all three points were useful. (1) ThirteenF.holdings is already sorted by Value descending (there's an existing test for that), so the first N rows are the top N; I left the sorting alone and added a comment saying so. (2) You're right about the header: it now counts the rows actually listed rather than the rows requested, and there's a test with a skipped junk row. (3) limit=0 stays uncapped since the issue asks for the full portfolio, but the tool schema and docs now say it can return thousands of rows for large managers; the follow-up commit is pushed.

@dgunning
dgunning merged commit e6a0b4a into dgunning:main Oct 15, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

edgar_read: opt-in limit so a full 13F portfolio can be read

3 participants