Skip to content

fix(query): bound remaining response lists - #895

Open
yzxcj797 wants to merge 1 commit into
tirth8205:mainfrom
yzxcj797:fix/888-bound-query-responses
Open

fix(query): bound remaining response lists#895
yzxcj797 wants to merge 1 commit into
tirth8205:mainfrom
yzxcj797:fix/888-bound-query-responses

Conversation

@yzxcj797

Copy link
Copy Markdown
Contributor

Summary

  • Bound every response list from the four remaining query tools, including impact-radius changed files, changed nodes, impacted nodes, impacted files, and edges.
  • Expose max_results on the impact-radius MCP tool and validate all caller-supplied bounds, rejecting booleans and values below 1.
  • Add detail-aware hard ceilings, preserve untruncated totals, mark truncation, and report "showing N of M" in summaries.
  • Cap traversal at a hard approximate-token ceiling while still computing and reporting the complete bounded-depth node count.

Regression coverage

  • Removed all four tools from the deferred unbounded set and enabled their worst-case token assertions.
  • Added fixture checks for per-list truncation, exact omitted counts, detail-level ceilings, bound validation, and a one-token traversal that emits no entry beyond its budget.

Validation

  • pytest tests/test_token_budget.py tests/test_tools.py tests/test_main.py tests/test_documentation.py -q — 298 passed.
  • pytest tests/test_token_budget.py -q — 127 passed.
  • ruff check code_review_graph/tools/query.py code_review_graph/main.py tests/test_token_budget.py — passed.
  • python -m py_compile for all changed Python files and git diff --check — passed.

Fixes #888

@github-actions

Copy link
Copy Markdown

code-review-graph review

Overall risk: 0.54 (MEDIUM) — 16 changed function(s)/class(es), 0 affected flow(s), 6 test gap(s)

Risk-scored changes

Risk Level Symbol Location Tested
0.54 medium code_review_graph/main.py::get_impact_radius_tool code_review_graph/main.py:220 no
0.54 medium code_review_graph/main.py::find_large_functions_tool code_review_graph/main.py:457 no
0.50 medium code_review_graph/tools/query.py::get_impact_radius code_review_graph/tools/query.py:133 yes
0.50 medium code_review_graph/tools/query.py::semantic_search_nodes code_review_graph/tools/query.py:796 yes
0.50 medium code_review_graph/tools/query.py::find_large_functions code_review_graph/tools/query.py:971 no
0.50 medium code_review_graph/tools/query.py::traverse_graph_func code_review_graph/tools/query.py:1080 no
0.50 medium tests/test_token_budget.py::test_hard_ceilings_bind tests/test_token_budget.py:676 (test)
0.50 medium tests/test_token_budget.py::TestTruncationContract.test_impact_radius_caps_every_response_list tests/test_token_budget.py:795 (test)
0.50 medium tests/test_token_budget.py::TestTruncationContract.test_impact_radius_caps_reachable_response_lists tests/test_token_budget.py:806 (test)
0.50 medium tests/test_token_budget.py::TestTruncationContract.test_semantic_search_reports_untruncated_total tests/test_token_budget.py:816 (test)

Test gaps

  • code_review_graph/main.py::get_impact_radius_tool (code_review_graph/main.py:220)
  • code_review_graph/main.py::find_large_functions_tool (code_review_graph/main.py:457)
  • code_review_graph/tools/query.py::find_large_functions (code_review_graph/tools/query.py:971)
  • code_review_graph/tools/query.py::traverse_graph_func (code_review_graph/tools/query.py:1080)
  • tests/test_token_budget.py::TestTruncationContract (tests/test_token_budget.py:772)
  • ...and 1 more without direct tests

Token savings: this graph-backed report used ~36,715 fewer tokens (~92%) 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.

@tirth8205

Copy link
Copy Markdown
Owner

The net win here is real and I measured it: on main, get_impact_radius(changed_files=["code_review_graph/graph.py"], max_depth=5) and find_large_functions(min_lines=1, limit=HUGE) cost roughly 71k and 469k tokens; on this branch they land at ~45k and ~14k. The mechanism also reuses _bounded/_shown_of/_validate_positive_int from #887 rather than inventing a second one, and the totals really are the untruncated counts. Two things need to change before it can merge, though, and both are about which rows survive rather than how many.

1. The edge cap slices an unranked list, so the visible edges misrepresent the graph. _bounded(edge_dicts, max_results, _MAX_IMPACT_EDGES) at query.py:214-216 takes a prefix of get_edges_among()'s raw row order. Nothing sorts it first. On this repo's own graph:

full edges : 4285  {'CALLS': 2187, 'TESTED_BY': 1624, 'CONTAINS': 317, 'IMPORTS_FROM': 83, 'REFERENCES': 74}
shown      :  150  {'TESTED_BY': 139, 'CALLS': 11}
edges cost : ~20k tokens of a ~45k response

CALLS is 51% of the real set and 7% of what the caller sees; IMPORTS_FROM and REFERENCES disappear entirely. Before this PR the list was unbounded — too big, but complete. Now it is small and skewed, and an agent reading it draws the wrong conclusion about the blast radius while still paying 20k tokens for the privilege. Please rank before bounding (endpoint impact score, or an edge-kind priority that puts CALLS/IMPORTS_FROM ahead of TESTED_BY/CONTAINS). The same concern applies to impacted_files and changed_nodes, which are built from a set in graph.py and so are hash-ordered — this repo never exceeds those caps, but a bigger one would.

2. detail_level="minimal" is now the most expensive way to call find_large_functions. _MAX_LARGE_FUNCTIONS_MINIMAL = 500 is 5× the standard ceiling while a minimal row is only ~2.6× smaller:

find_large_functions(min_lines=1, limit=10**6)                        -> 100 rows, ~14.3k tokens
find_large_functions(min_lines=1, limit=10**6, detail_level="minimal") -> 500 rows, ~42.6k tokens

That is over the worst_max: 30_000 this PR declares for the tool, and it inverts the advice in CLAUDE.md that tells agents to use minimal unless they need more. The budget table can't catch it because the entry's worst kwargs are {"min_lines": 1, "limit": HUGE} with no detail_levelget_architecture_overview_tool right above it does pin one, so this looks like an oversight rather than a convention. Please lower the minimal ceiling so minimal actually costs less than standard, and pin detail_level in the budget entry so the test can see it.

Worth fixing in the same pass:

  • semantic_search_nodes totals are now the whole graph. hybrid_search(..., limit=_FETCH_ALL) defeats the limit * 3 fetch cap in search.py, so FTS returns everything: query="graph" gives total=5794 — that is every node in the graph — with results_omitted=5774 and a summary reading showing 20 of 5794. It also materializes 5,794 result dicts to return 20. Bound the fetch to a multiple of the requested limit instead of _FETCH_ALL, and let total mean "matches worth ranking".
  • traverse_graph's token budget stops bounding work. It now walks the whole bounded-depth component and slices afterwards: depth=6 on GraphStore takes ~0.33s and issues the same ~13k point queries whether the budget is 1 or 1,000,000, emitting 0 entries in the first case. graph.py documents 500k nodes / 3M edges as the design scale; there this is a lot of work to produce nothing. Short-circuit the walk at the budget again and keep the separate full count.
  • result_count now means two different things in one file. fix(tools): bound every unbounded MCP response #887 set it to the untruncated total for query_graph; here it is the shown count for semantic_search_nodes (a change from its previous meaning) and find_large_functions. Nothing downstream reads it, but it is an LLM-facing contract, so please pick one.
  • The ceilings are count-based, so the default impact call is still ~45k tokens — 3.6× the tool's own default_max of 12k — because rows cost 126–135 tokens each. Not a regression (it inherits fix(tools): bound every unbounded MCP response #887's 100/150 shape) and it stays under ABSOLUTE_MAX_TOKENS, but a token-aware cap would actually deliver the documented budget.
  • docs/COMMANDS.md:59 says each list reports its untruncated *_total; there is no impacted_nodes_total — that list reports total_impacted. Also, three of the four new ceilings aren't mentioned in the MCP docstrings agents read (main.py limit/token_budget), and one docstring leaks the private constant name _MAX_TRAVERSAL_TOKEN_BUDGET instead of its value.

Verified good, for the record: bounds reject 0, -1, True and False on all four params; impacted_nodes keeps the true top-100 by impact score in order; traversal at a one-token budget still reports the correct complete count; the new tests genuinely fail without the source change; and the parameter names and defaults in the docs match the code.

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.

get_impact_radius, find_large_functions, traverse_graph and semantic_search_nodes return unbounded responses

2 participants