feat(mcp): cap financial-statement-analysis at the columns a filing presents

[jfrench9]

Summary

The second half of the MCP response economy (local/RoboSystems/specs/ai-operators/mcp-response-economy.md §3, change 5), following #1376. financial-statement-analysis was a median 24K characters per call in the Filing Ladder runs because it returned every period its hypercube holds: on the 3M FY2024 10-K income statement, 8 of 11 period keys were the quarterly figures from the notes, and the balance sheet carried 2021 and 2022 instants from the equity roll-forward. Each fact row also repeated the qname as name plus three null fields. The tool now answers with the columns a filing presents, and says what it left out.

Changes

financial-statement-analysis — middleware/mcp/tools/financial_statement_tools.py

• default_period_type: when the caller gives no period_type, an annual form (10-K / 20-F / 40-F, per the resolver's ANNUAL_FORMS) defaults the filter to annual. The balance sheet keeps the query's instant default; a 10-Q is left unfiltered because its quarter and year-to-date columns share end dates, so the cap below bounds it without a filter. An explicit period_type still wins.
• cap_periods: a new periods argument caps the distinct period end dates kept, newest first — default 2 for the balance sheet, 3 for the flow statements, max 100. The result carries periods (the keys the facts span, newest first) and, when older end dates were cut, periods_omitted with a tip to raise the cap.
• compact_fact: rows drop name (always the local part of qname — verified on the live output) and null fields.
• limit applies within the kept periods (dedup → cap → limit).
• Description and inputSchema state all of the above.

Docs — middleware/mcp/README.md: the tool table cell and the paragraph beneath it describe the cadence default, the cap, and the row shape.

Measured on the local MMM FY2024 10-K (before = every period, old row shape):

Statement	Before	After
Income statement	50,043 chars / 174 facts / 9 end dates	18,531 / 78 / 3
Balance sheet	25,617 / 99 / 4	15,837 / 94 / 2 (2 omitted, said so)
Cash flow	30,938 / 106 / 4	23,952 / 102 / 3

Not changed: the REST view op at routers/extensions/roboledger/views.py still returns every period and the full row model. This is a tool-layer change for the reader with the budget, as in #1376.

Breaking Changes

None. MCP response body and tool definition only; no REST or SDK surface moves. Under the ChatGPT plugin's rule the new periods parameter is a definition change that rides the next plugin version's review, alongside #1376's parameters.

Testing

• just test-code — passed (ruff, format, basedpyright, cf-lint).
• tests/middleware/mcp/tools/test_financial_statement_tools.py plus the description contract, manager helpers, MCP execute, and roboledger views suites — 335 passed. New tests cover the cadence default per form and statement, the cap (newest end dates, keys sharing an end date counting once, empty input), row compaction, and the tool end to end (annual default sent to the query, explicit period_type winning, balance sheet not forced annual, the 3-period and 2-period defaults with periods_omitted, the periods argument and its bound, limit within the kept periods).
• Live check against the local sec graph produced the table above.
• Full just test-all not run in-session.

Certification

• I have the right to submit this work under the Apache 2.0 license, and do so. Where any part of it is owned by my employer, I have their permission.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQNt2hdNJqbJwfoSRshaXo

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 3m 0s —— View job

---

Reviewing PR #1377

• Gather PR metadata, diff, and existing review feedback
• Read changed files and understand the request
• Review financial_statement_tools.py changes (correctness, layering, tests)
• Review test coverage
• Review README doc changes
• Post final review

PR Summary

Title: feat(mcp): cap financial-statement-analysis at the columns a filing presents
Author: jfrench9 | Branch: feature/statement-period-cap → main
Changes: +381 / -18 across 3 files

Caps financial-statement-analysis to the period columns a filing actually presents: defaults period_type to annual for 10-K/20-F/40-F when the caller doesn't specify one, caps the number of distinct period end dates kept (2 for balance sheet, 3 for flow statements, overridable via a new periods arg, max 100), and trims each fact row (name and null fields dropped). Follows the #1376 MCP response-economy pattern.

Existing Review Feedback

Only a change-classification: normal bot comment is present — no human or AI review feedback yet, and no CI status is visible from this environment (see note below).

My Review

Issues (should fix before merge)

1. limit can silently starve period coverage, with no signal that it happened. In financial_statement_tools.py:410-416, the same limit value is passed into query_financial_statement(..., limit=limit) and used later to trim kept[:limit]. The query itself (operations/roboledger/views/financial_statement_query.py:118) only fetches fetch_limit = min(limit * 3, 1000) raw rows, ordered by end_date DESC, before dedup and the new cap_periods step. With the default limit=1000 this is fine, but if a caller passes a smaller explicit limit (which the tool's own doc text invites: "a lower cap cuts rows while subtotals still reflect the full set"), the raw fetch can be dominated by rows from just the newest period (e.g. limit=5 → fetch_limit=15, but a single period's line items can easily exceed 15 rows). cap_periods then only sees whatever period(s) survived that truncation and reports periods/periods_omitted based on that already-incomplete set — so a request for 3 periods can silently come back with 1, with no periods_omitted hint that anything was cut, because from cap_periods's point of view nothing was cut. Given the entire point of this PR is trustworthy period accounting, this seems worth decoupling: the row budget that feeds cap_periods shouldn't be tied to the caller's final limit.
   • Note: this exact interaction isn't exercised by the new tests — _shared_repo_patches (tests/middleware/mcp/tools/test_financial_statement_tools.py:411-427) mocks query_financial_statement to return the full rows list regardless of the limit passed to it, so test_limit_applies_within_the_kept_periods (line 628) never touches the real fetch_limit = limit * 3 truncation in the query layer.
   • Fix this →

Suggestions (non-blocking)

2. compact_fact (financial_statement_tools.py:98-108) drops name on the assumption it's always the local part of qname. The PR description says this was "verified on the live output," which is reasonable evidence but is an empirical observation about one data set, not an invariant enforced anywhere in the schema/query layer. Not blocking, but worth a one-line comment noting the assumption (and what breaks if a future taxonomy extension violates it) so a future reader doesn't have to rediscover it.
3. The periods argument follows the arguments.get("periods") or periods_default pattern (financial_statement_tools.py:349), so an explicit periods: 0 silently falls back to the default rather than being floored to 1 like a negative value would be. Almost certainly harmless since 0 periods isn't a meaningful request, but it's a slightly different code path than limit uses (arguments.get("limit", 1000)), worth a glance if it's not intentional.

Questions

• None — the design rationale (why limit and periods are separate knobs, why balance sheet isn't forced to annual) is well explained in the docstrings and PR description.

Verdict

NEEDS_DISCUSSION — the implementation is clean, well-documented, and the described before/after measurements are convincing, but issue #1 (small explicit limit can silently truncate period coverage below what periods was asked to keep, with no periods_omitted signal) is a real correctness gap in the exact area this PR is meant to harden, and isn't caught by the current tests because the query layer's own truncation is mocked away. Worth a decision from the author on whether to fix now or file as a fast-follow.

Note on environment: gh and uv/pytest commands required interactive approval that wasn't available in this run, so I couldn't independently verify CI status or re-run the test suite — this review is based on reading the diff, the touched source files, and the test file directly (plain git commands were available).

[jfrench9]

Decision: fix now, in c682d4c. All three points addressed.

1. limit starving the cap — real. The query now always takes its 1,000-row ceiling (QUERY_ROW_CEILING) regardless of the caller's limit, and limit applies after dedup and the cap. A fetch that hits the ceiling is flagged rows_truncated with a tip, since the oldest kept period may then be incomplete. Tests: the query receives the ceiling when limit=5; a 1,000-row fetch is flagged; a smaller one is not.
2. name == local part of qname — the assumption was wrong, not just undocumented. It held on every SEC row but a tenant graph's rs-gaap elements carry a readable label there (rs-gaap:NonoperatingIncomeExpense → "Nonoperating Income (Expense)"). compact_fact now keeps name when it differs from the local part and drops it only when redundant; docstring says which graph has which shape. Test added for the tenant row.
3. periods: 0 — now floored to 1 like any other out-of-range value; only an absent argument takes the default. Test added.

just test-code clean, 249 tests in the tool's suites green.

@claude please re-review on the new commit.

[claude]

Claude finished @jfrench9's task in 1m 11s —— View job

---

Re-reviewing PR #1377 (commit c682d4c)

• Read trigger comment and understand the three fixes claimed
• Review the diff for commit c682d4c
• Verify fix 1: limit no longer starves cap_periods
• Verify fix 2: name retained when it differs from local part of qname
• Verify fix 3: periods: 0 floored to 1
• Check new tests cover the fixes
• Post final verdict

Re-review of c682d4c

All three points from the prior review are fixed correctly.

1. limit starving the cap (the blocking issue) — fixed. financial_statement_tools.py:430 now always passes limit=QUERY_ROW_CEILING (1000) into query_financial_statement, independent of the caller's limit. Confirmed against the query layer (operations/roboledger/views/financial_statement_query.py:118): fetch_limit = min(limit * 3, 1000) — with limit=1000 that's always min(3000, 1000) == 1000, so the ceiling is truly caller-independent now. cap_periods runs on the full 1000-row fetch, and the caller's limit is applied afterward via kept[:limit] (financial_statement_tools.py:437). The new rows_truncated/rows_tip flag (:453-460) correctly reads len(rows) — the raw pre-dedup fetch — so it reflects whether the graph fetch itself hit the ceiling, not some post-cap count. Only wrinkle: len(rows) >= QUERY_ROW_CEILING is a "maybe truncated" heuristic (a filing that happens to have exactly 1000 raw fact rows total would also trip it) rather than a certainty, but that's the same class of imprecision as fetch_limit's LIMIT $limit clause itself and is clearly caveated in the tip text ("may be incomplete"). Not worth a follow-up.
Test test_query_gets_the_full_row_budget_whatever_limit_is asserts the mock receives QUERY_ROW_CEILING even when the tool call passes limit=5 — this directly exercises the previously-missing interaction. test_a_fetch_at_the_ceiling_is_flagged / test_a_fetch_under_the_ceiling_is_not_flagged cover both sides of the new flag.

2. name/qname redundancy assumption — fixed. compact_fact (financial_statement_tools.py:103-126) now only nulls out name when it equals qname.rsplit(":", 1)[-1], keeping it otherwise. The docstring states the SEC-vs-tenant-graph rationale plainly. test_keeps_a_label_name_on_a_tenant_row covers the rs-gaap case; the existing test_drops_name_and_nulls still covers the redundant case. Null-stripping still happens correctly at the end via the dict comprehension, so a genuinely absent name is still omitted from the row.

3. periods: 0 — fixed. arguments.get("periods") is now checked for is None before applying the default, and any explicit value (including 0) goes through max(1, min(..., PERIODS_MAX)) (financial_statement_tools.py:371-376). test_periods_zero_is_floored_not_defaulted confirms periods=0 yields 1 kept period, not the 3-period default.

Other callers unaffected: query_financial_statement is also called directly by the REST view op (routers/extensions/roboledger/views.py) with its own limit; that path is untouched since only the MCP tool now hardcodes QUERY_ROW_CEILING.

Minor, non-blocking: the periods field's inputSchema description (financial_statement_tools.py around the "periods" schema block) still just says "how many period end dates to keep" — it doesn't mention that out-of-range values (including 0) are floored/capped rather than rejected. Purely cosmetic; the docstring-level explanation elsewhere is sufficient for a human reader and the behavior itself is correct.

Environment note: uv run pytest / uv run ruff required interactive approval unavailable in this run, so I read the diff and reasoned about the query-layer math directly rather than re-executing the suite. The new tests visibly target the exact three gaps from the prior review, and the logic checks out by inspection.

Verdict

LGTM — the fix directly addresses the correctness gap (the query no longer lets a small limit hide periods from the cap, and now flags when the raw fetch itself is truncated), the name assumption is now conditional instead of blind, and the periods: 0 edge case is handled consistently with other out-of-range inputs. Nothing further to block on.