fix(tools): bound every unbounded MCP response - #887
Merged
Conversation
#849 found get_affected_flows returning ~247k tokens inside a workflow documented as "5 tool calls, 800 tokens total". PR #853 capped that one tool. Measuring all 30 registered tools against a real 5.6k-node graph found the same class of bug in ten more places, several of them on the default path: list_communities 206,858 tokens with DEFAULT arguments get_community 134,781 default / 535,618 with members get_architecture_overview 625,012 in standard mode refactor dead_code 47,312 / suggest 38,246 detect_changes 46,089 for a ONE-file diff get_surprising_connections 1,287,174 at top_n=10**6 get_hub_nodes 555,848 / get_bridge_nodes 317,202 get_review_context 4,720,622 on a whole-repo diff PR #853's own fix was also only half a fix: standard mode carries a full steps list per flow (~980 tokens each), so its 50-flow cap still produced ~49k tokens, and max_flows=0 disabled the limit entirely. Every list-returning tool now follows one contract, the one #853 established: `total` (or a per-list `*_total`) always reports the untruncated count, `truncated` marks the cut, and the summary line says how many of how many are shown. Bounds are validated the way query.py validates max_results - booleans rejected, values below 1 rejected. detail_level="minimal" was added to the analysis and refactor tools, projecting to the same compact field sets their siblings use. Where a count alone cannot bound a response, a shared budget does: get_flow and get_affected_flows spend a step budget, get_review_context and detect_changes spend a source-line budget. Ceilings that depend on payload size depend on detail_level, mirroring query.py capping minimal-mode results at five. Two behaviour changes are deliberate and their #853 tests are updated in this commit: get_affected_flows standard mode now caps at 25 flows (minimal at 500), and max_flows=0 keeps its "no caller limit" meaning while still obeying the ceiling - an escape hatch that returns a quarter of a million tokens is the bug, not a feature. Default behaviour stays backward compatible in shape: an existing caller passing nothing still gets a valid response, just bounded. tests/test_token_budget.py records the measured budget table as reviewable data and pins it three ways: per-tool default and worst-case token ceilings, exact truncated list lengths against the imported ceiling constants, and the ceiling constants themselves. Removing a cap, raising a ceiling, or adding an unbounded field fails it. The fixture graph builds once per module and the whole file runs in ~7s, offline. Reported, not fixed: code_review_graph/tools/query.py is owned elsewhere and four of its tools remain unbounded - get_impact_radius (3.4M tokens; changed_nodes and edges ignore max_results, which is not even exposed on the MCP signature), find_large_functions (737k), traverse_graph (385k) and semantic_search_nodes, whose limit/token_budget are neither validated nor capped. Their default budgets are still asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3
code-review-graph reviewOverall risk: 0.75 (HIGH) — 70 changed function(s)/class(es), 0 affected flow(s), 48 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~45,602 fewer tokens (~80%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #849 reported one tool returning 247k tokens inside a workflow documented as "5 tool calls, 800 tokens total". PR #853 capped that one tool. Nobody had checked the other 29.
Eleven were unbounded, several on the default path, which is worse than #849: that one at least needed a large diff to trigger. These fire on a bare call with no arguments.
Independently verified, same graph before and after
Measured on a fresh full build of this repository (284 files, 5,764 nodes, 47,638 edges), serialized exactly as FastMCP does and counted with tiktoken cl100k_base:
list_communitiesrefactor dead_coderefactor suggestA single default
list_communities_toolcall returned more tokens than most models can hold in context at all, against a documented budget of 800.Worst-case paths were worse still:
get_review_context4.7M,get_surprising_connections1.29M,get_hub_nodes556k,get_community536k,get_architecture_overview625k.Notable causes
detect_changeswas leaking get_affected_flows_tool has no detail_level or limit parameter and returns 247k+ tokens, breaking the documented token budget #849's exact payload through a second door: it embeddedaffected_flowswith fullstepslists, uncapped. Now metadata only, pointing atget_affected_flows_toolfor detail.get_communityreturned 134k tokens even withinclude_members=False. The community's ownmemberslist was the payload._extract_relevant_linesignoredmax_lines_per_filewhen many functions in one file changed, because merged ranges covered the whole file.apply_refactor(dry_run=True)returned one unrestricted unified diff per touched file.stepslist per flow at roughly 980 tokens each, so its 50-flow cap still produced about 49k, andmax_flows=0disabled the limit entirely. Its tests are updated here rather than worked around, because an escape hatch that returns a quarter of a million tokens is the bug get_affected_flows_tool has no detail_level or limit parameter and returns 247k+ tokens, breaking the documented token budget #849 reported.Approach
Three shared helpers in
tools/_common.pygive every tool the contract #853 established:totalalways reports the untruncated count,truncatedmarks the cut, and the summary says "showing N of M". Validation matchesquery.pyexactly, rejecting bools and values below 1 with the same message. Where a count alone cannot bound a response, a shared budget does:get_flowandget_affected_flowsspend a step budget,get_review_contextanddetect_changesspend a source-line budget. Ceilings that depend on payload size vary bydetail_level, mirroring howquery.pycaps minimal mode at five results.Regression guard
tests/test_token_budget.py, 103 tests, offline, one shared fixture graph. It pins the contract three ways, because any one alone can be gamed:test_hard_ceilings_bindasserts truncated list lengths equal the ceiling constants, so a cap that silently stops applying changes a length.test_ceiling_constants_are_not_raisedasserts the constants themselves, since a token budget can be masked by its own headroom.Plus
test_budget_table_covers_every_registered_tool, so a new@mcp.tool()cannot ship without a budget entry. Verified it fails on regression: raising_MAX_MEMBERSto 100000 produces 2 failures.Four holes deliberately left open
tools/query.pywas being modified concurrently by the uncertainty work (#884, #885), so these are reported rather than fixed here, each with a follow-up owed:get_impact_radius, 3.4M tokens.changed_nodesandedgesignoremax_results; onlyimpacted_nodesis capped. Worse,max_resultsis not exposed on the MCP signature inmain.pyat all, so no client can bound it.find_large_functions, 737k.limitneither validated nor capped.traverse_graph, 385k.token_budgetneither validated nor capped, and the check uses a len/4 heuristic applied after the entry is built.semantic_search_nodes, 145k.limitneither validated nor capped.The budget test asserts their default budgets and skips only their worst case, listing each in a
QUERY_OWNED_UNBOUNDEDset with the reason, so a regression in normal use is still caught.Docs updated:
LLM-OPTIMIZED-REFERENCE.md,docs/COMMANDS.md(new Result Bounds section; every signature there was stale, missing even #853's parameters) anddocs/FEATURES.md.Full suite 2872 passed, 9 skipped, 2 xpassed. ruff and mypy clean.
Relates to #849.