diff --git a/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md b/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md index 2d4a4e10..6d40862c 100644 --- a/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md +++ b/code_review_graph/docs/LLM-OPTIMIZED-REFERENCE.md @@ -30,6 +30,10 @@ MCP prompts (5): review_changes, architecture_map, debug_issue, onboard_develope Skills: build-graph, debug-issue, explore-codebase, refactor-safely, review-changes, review-delta, review-pr CLI: code-review-graph [install|init|build|update|status|watch|visualize|serve|mcp|wiki|detect-changes|postprocess|embed|register|unregister|repos|eval|daemon] Token efficiency: Prefer detail_level="minimal" where available. Always call get_minimal_context_tool first. Some review/context tools return compact estimated context_savings metadata. + +Result bounds: every tool that returns a list is bounded. Defaults are small; pass the tool's cap parameter to widen up to its hard ceiling, or a smaller value to narrow. Truncation is never silent — the response reports the untruncated count (`total`, or a `*_total` field per list), sets `truncated: true`, and the summary line says how many of how many are shown. +Cap parameters by tool: max_results (query_graph, get_review_context, detect_changes, list_communities, get_architecture_overview, refactor, cross_repo_search), max_flows (get_affected_flows, detect_changes), max_members (list_communities, get_community, get_architecture_overview), max_steps + max_source_lines (get_flow), max_per_category (get_knowledge_gaps), top_n (get_hub_nodes, get_bridge_nodes, get_surprising_connections), limit (list_flows, semantic_search_nodes, find_large_functions), max_files (get_review_context), max_chars (get_wiki_page), max_diff_files (apply_refactor). +Bounds reject values below 1 and reject booleans. get_affected_flows keeps max_flows=0 as "no caller limit", still subject to its ceiling (25 flows in standard mode, 500 in minimal, plus a shared 400-step budget) — see #849.
diff --git a/code_review_graph/main.py b/code_review_graph/main.py index 346d4f68..0fbdadf3 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -294,6 +294,8 @@ def get_review_context_tool( repo_root: Optional[str] = None, base: str = "HEAD~1", detail_level: str = "standard", + max_results: int = 100, + max_files: int = 25, ) -> dict: """Generate a focused, token-efficient review context for code changes. @@ -309,12 +311,17 @@ def get_review_context_tool( base: Git ref for change detection. Default: HEAD~1. detail_level: "standard" for full output, "minimal" for token-efficient summary. Default: standard. + max_results: Maximum graph nodes per list and edges to return. + Default: 50. Each list reports its untruncated ``*_total``. + max_files: Maximum files listed and given source snippets. + Default: 25. Snippets share an 800-line budget. """ root = _resolve_repo_root(repo_root) return with_provenance(get_review_context( changed_files=changed_files, max_depth=max_depth, include_source=include_source, max_lines_per_file=max_lines_per_file, repo_root=root, base=base, detail_level=detail_level, + max_results=max_results, max_files=max_files, ), root) @@ -504,6 +511,8 @@ def get_flow_tool( flow_name: Optional[str] = None, include_source: bool = False, repo_root: Optional[str] = None, + max_steps: int = 50, + max_source_lines: int = 400, ) -> dict: """Get detailed information about a single execution flow. @@ -517,11 +526,16 @@ def get_flow_tool( flow_name: Name to search for (partial match). Ignored if flow_id given. include_source: Include source code snippets for each step. Default: False. repo_root: Repository root path. Auto-detected if omitted. + max_steps: Maximum steps to return; flow.total_steps reports the + full count. Default: 50. + max_source_lines: Total source lines across all steps when + include_source is set. Default: 400. """ root = _resolve_repo_root(repo_root) return with_provenance(get_flow( flow_id=flow_id, flow_name=flow_name, include_source=include_source, repo_root=root, + max_steps=max_steps, max_source_lines=max_source_lines, ), root) @@ -546,7 +560,9 @@ def get_affected_flows_tool( detail_level: "standard" for full step details, "minimal" for per-flow metadata only. Default: standard. max_flows: Maximum flows to return; total reports the full count. - Default: 50. Pass 0 to disable the limit. + Default: 50. Pass 0 for no caller limit. Standard mode + additionally caps visible flows at 25 and minimal mode at 500, + because a standard flow costs ~980 tokens against ~18 minimal. """ root = _resolve_repo_root(repo_root) return with_provenance(get_affected_flows_func( @@ -561,6 +577,8 @@ def list_communities_tool( min_size: int = 0, detail_level: str = "standard", repo_root: Optional[str] = None, + max_results: int = 50, + max_members: int = 10, ) -> dict: """List detected code communities in the codebase. @@ -575,11 +593,17 @@ def list_communities_tool( "minimal" returns only name, size, and cohesion per community. repo_root: Repository root path. Auto-detected if omitted. + max_results: Maximum communities to return; total reports the full + count. Default: 50. + max_members: Maximum member names listed per community in standard + mode. Each community's size still reports its true member + count. Default: 10. """ root = _resolve_repo_root(repo_root) return with_provenance(list_communities_func( repo_root=root, sort_by=sort_by, min_size=min_size, - detail_level=detail_level, + detail_level=detail_level, max_results=max_results, + max_members=max_members, ), root) @@ -589,6 +613,7 @@ def get_community_tool( community_id: Optional[int] = None, include_members: bool = False, repo_root: Optional[str] = None, + max_members: int = 25, ) -> dict: """Get detailed information about a single code community. @@ -603,11 +628,15 @@ def get_community_tool( community_id: Database ID of the community. include_members: Include full member node details. Default: False. repo_root: Repository root path. Auto-detected if omitted. + max_members: Maximum member entries to include; the community's + size still reports its true member count and + members_truncated marks the cut. Default: 25. """ root = _resolve_repo_root(repo_root) return with_provenance(get_community_func( community_name=community_name, community_id=community_id, include_members=include_members, repo_root=root, + max_members=max_members, ), root) @@ -615,6 +644,8 @@ def get_community_tool( def get_architecture_overview_tool( repo_root: Optional[str] = None, detail_level: str = "minimal", + max_results: int = 100, + max_members: int = 10, ) -> dict: """Generate an architecture overview based on community structure. @@ -628,11 +659,17 @@ def get_architecture_overview_tool( and aggregates cross-community edges to one row per community pair (typical reduction: 600KB -> <5KB); "standard" returns full per-edge detail. + max_results: Maximum cross-community rows and warnings to return; + cross_community_edges_total reports the full count. Default: 100. + max_members: Maximum member names per community in standard mode. + Default: 10. """ root = _resolve_repo_root(repo_root) return with_provenance(get_architecture_overview_func( repo_root=root, detail_level=detail_level, + max_results=max_results, + max_members=max_members, ), root) @@ -644,6 +681,8 @@ async def detect_changes_tool( max_depth: int = 2, repo_root: Optional[str] = None, detail_level: str = "standard", + max_results: int = 25, + max_flows: int = 20, ) -> dict: """Detect changes and produce risk-scored, priority-ordered review guidance. @@ -663,6 +702,12 @@ async def detect_changes_tool( repo_root: Repository root path. Auto-detected if omitted. detail_level: "standard" for full output, "minimal" for token-efficient summary. Default: standard. + max_results: Maximum changed functions, test gaps, and changed files + to return; the matching *_total fields report the full counts. + Default: 25. + max_flows: Maximum affected flows to embed. Embedded flows carry + per-flow metadata only — use get_affected_flows_tool for step + detail. Default: 20. """ root = _resolve_repo_root(repo_root) @@ -671,6 +716,7 @@ def _run() -> dict: base=base, changed_files=changed_files, include_source=include_source, max_depth=max_depth, repo_root=root, detail_level=detail_level, + max_results=max_results, max_flows=max_flows, ), root) coro = asyncio.to_thread(_run) @@ -701,6 +747,8 @@ def refactor_tool( kind: Optional[str] = None, file_pattern: Optional[str] = None, repo_root: Optional[str] = None, + max_results: int = 50, + detail_level: str = "standard", ) -> dict: """Graph-powered refactoring operations. @@ -722,11 +770,18 @@ def refactor_tool( kind: (dead_code) Optional filter: Function or Class. file_pattern: (dead_code) Filter by file path substring. repo_root: Repository root path. Auto-detected if omitted. + max_results: Maximum edits/symbols/suggestions in the response; + total reports the full count. The stored rename preview keeps + every edit, so apply_refactor_tool still applies them all. + Default: 50. + detail_level: "standard" for full records, "minimal" for + identifying fields only. Default: standard. """ root = _resolve_repo_root(repo_root) return with_provenance(refactor_func( mode=mode, old_name=old_name, new_name=new_name, kind=kind, file_pattern=file_pattern, repo_root=root, + max_results=max_results, detail_level=detail_level, ), root) @@ -735,6 +790,7 @@ def apply_refactor_tool( refactor_id: str, repo_root: Optional[str] = None, dry_run: bool = False, + max_diff_files: int = 25, ) -> dict: """Apply a previously previewed refactoring to source files. @@ -753,11 +809,13 @@ def apply_refactor_tool( the same preview can be applied in a follow-up call without dry_run. Use this for a human-in-the-loop review before committing changes to disk. See: #176 + max_diff_files: Maximum per-file diffs to include in a dry run. + would_modify still lists every file. Default: 25. """ root = _resolve_repo_root(repo_root) return with_provenance(apply_refactor_func( refactor_id=refactor_id, repo_root=root, - dry_run=dry_run, + dry_run=dry_run, max_diff_files=max_diff_files, ), root) @@ -794,6 +852,7 @@ def _run() -> dict: def get_wiki_page_tool( community_name: str, repo_root: Optional[str] = None, + max_chars: int = 20000, ) -> dict: """Retrieve a specific wiki page by community name. @@ -803,10 +862,13 @@ def get_wiki_page_tool( Args: community_name: Community name to look up. repo_root: Repository root path. Auto-detected if omitted. + max_chars: Maximum characters of page content to return; + total_chars reports the real length. Default: 20000. """ root = _resolve_repo_root(repo_root) return with_provenance(get_wiki_page_func( community_name=community_name, repo_root=root, + max_chars=max_chars, ), root) @@ -814,6 +876,7 @@ def get_wiki_page_tool( def get_hub_nodes_tool( top_n: int = 10, repo_root: Optional[str] = None, + detail_level: str = "standard", ) -> dict: """Find the most connected nodes in the codebase (architectural hotspots). @@ -821,12 +884,14 @@ def get_hub_nodes_tool( them have disproportionate blast radius. Excludes File nodes. Args: - top_n: Number of top hubs to return. Default: 10. + top_n: Number of top hubs to return (capped at 100). Default: 10. repo_root: Repository root path. Auto-detected if omitted. + detail_level: "standard" for full node data, "minimal" for name, + kind, and total_degree only. Default: standard. """ root = _resolve_repo_root(repo_root) return with_provenance(get_hub_nodes_func( - repo_root=root, top_n=top_n, + repo_root=root, top_n=top_n, detail_level=detail_level, ), root) @@ -834,6 +899,7 @@ def get_hub_nodes_tool( def get_bridge_nodes_tool( top_n: int = 10, repo_root: Optional[str] = None, + detail_level: str = "standard", ) -> dict: """Find architectural chokepoints via betweenness centrality. @@ -842,18 +908,22 @@ def get_bridge_nodes_tool( Uses sampling approximation for graphs > 5000 nodes. Args: - top_n: Number of top bridges to return. Default: 10. + top_n: Number of top bridges to return (capped at 100). Default: 10. repo_root: Repository root path. Auto-detected if omitted. + detail_level: "standard" for full node data, "minimal" for name, + kind, and betweenness only. Default: standard. """ root = _resolve_repo_root(repo_root) return with_provenance(get_bridge_nodes_func( - repo_root=root, top_n=top_n, + repo_root=root, top_n=top_n, detail_level=detail_level, ), root) @mcp.tool() def get_knowledge_gaps_tool( repo_root: Optional[str] = None, + max_per_category: int = 15, + detail_level: str = "standard", ) -> dict: """Identify structural weaknesses in the codebase graph. @@ -863,10 +933,15 @@ def get_knowledge_gaps_tool( Args: repo_root: Repository root path. Auto-detected if omitted. + max_per_category: Maximum entries per gap category; summary and + total_gaps still report the untruncated counts. Default: 15. + detail_level: "standard" for full gap records, "minimal" to drop + file paths. Default: standard. """ root = _resolve_repo_root(repo_root) return with_provenance(get_knowledge_gaps_func( - repo_root=root, + repo_root=root, max_per_category=max_per_category, + detail_level=detail_level, ), root) @@ -874,6 +949,7 @@ def get_knowledge_gaps_tool( def get_surprising_connections_tool( top_n: int = 15, repo_root: Optional[str] = None, + detail_level: str = "standard", ) -> dict: """Find unexpected architectural coupling via composite surprise scoring. @@ -882,12 +958,14 @@ def get_surprising_connections_tool( unusual edge kinds (+0.15). Args: - top_n: Number of top surprises to return. Default: 15. + top_n: Number of top surprises to return (capped at 100). Default: 15. repo_root: Repository root path. Auto-detected if omitted. + detail_level: "standard" for full edge records, "minimal" for + source, target, kind, and score only. Default: standard. """ root = _resolve_repo_root(repo_root) return with_provenance(get_surprising_connections_func( - repo_root=root, top_n=top_n, + repo_root=root, top_n=top_n, detail_level=detail_level, ), root) @@ -956,6 +1034,7 @@ def cross_repo_search_tool( query: str, kind: Optional[str] = None, limit: int = 20, + max_results: int = 50, ) -> dict: """Search for code entities across all registered repositories. @@ -968,8 +1047,12 @@ def cross_repo_search_tool( query: Search string to match against node names. kind: Optional filter: File, Class, Function, Type, or Test. limit: Maximum results per repo. Default: 20. + max_results: Maximum merged results across all repos; total reports + the untruncated merged count. Default: 50. """ - return cross_repo_search_func(query=query, kind=kind, limit=limit) + return cross_repo_search_func( + query=query, kind=kind, limit=limit, max_results=max_results, + ) @mcp.prompt() diff --git a/code_review_graph/tools/_common.py b/code_review_graph/tools/_common.py index e2cf7796..ce7fa25a 100644 --- a/code_review_graph/tools/_common.py +++ b/code_review_graph/tools/_common.py @@ -262,6 +262,53 @@ def add(path: str) -> None: return resolved +# --------------------------------------------------------------------------- +# Result bounding (#849 follow-up) +# --------------------------------------------------------------------------- +# +# Every MCP tool response has to survive a client-side context window. #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. These +# helpers give the remaining tools the same contract: +# +# * ``total`` always reports the untruncated count, +# * ``truncated`` marks that the list was cut, +# * the summary line says how many of how many are shown. +# +# Each tool pairs a caller-facing default with a hard ceiling. The ceiling +# exists so a caller passing ``max_results=1_000_000`` still gets a response +# that fits the ~25k-token budget most MCP clients allow for one tool result. + + +def _validate_positive_int(value: int, name: str) -> int: + """Validate a caller-supplied result bound. + + Mirrors the check ``query.py`` applies to ``max_results``: ``bool`` is + rejected explicitly because ``True`` would otherwise silently mean 1. + """ + if isinstance(value, bool) or value < 1: + raise ValueError(f"{name} must be an integer greater than or equal to 1") + return value + + +def _bounded( + items: "list[Any]", max_results: int, hard_cap: int, +) -> tuple[list[Any], int, bool]: + """Cap *items* at ``min(max_results, hard_cap)``. + + Returns ``(visible, total, truncated)`` where ``total`` is the + untruncated length, so callers can always report the real count. + """ + total = len(items) + limit = min(max_results, hard_cap) + return list(items[:limit]), total, total > limit + + +def _shown_of(shown: int, total: int) -> str: + """Return the ``", showing N of M"`` fragment used by capped summaries.""" + return f", showing {shown} of {total}" if shown < total else "" + + def compact_response( summary: str, key_entities: list[str] | None = None, diff --git a/code_review_graph/tools/analysis_tools.py b/code_review_graph/tools/analysis_tools.py index b67e6340..7e521c45 100644 --- a/code_review_graph/tools/analysis_tools.py +++ b/code_review_graph/tools/analysis_tools.py @@ -11,12 +11,34 @@ find_surprising_connections, generate_suggested_questions, ) -from ._common import _get_store +from ._common import _bounded, _get_store, _shown_of, _validate_positive_int + +# The ranking helpers already score every candidate before slicing, so asking +# for "all" costs nothing extra and lets the tool report an honest ``total``. +_FETCH_ALL = 10**9 + +# Hard ceilings. A caller may raise ``top_n`` above the default, but never +# past these: an unbounded top_n returned >500k tokens before #849's sweep. +_MAX_HUB_NODES = 100 +_MAX_BRIDGE_NODES = 100 +_MAX_SURPRISING = 100 +_MAX_GAPS_PER_CATEGORY = 50 + +_MINIMAL_HUB_FIELDS = ("name", "kind", "total_degree") +_MINIMAL_BRIDGE_FIELDS = ("name", "kind", "betweenness") +_MINIMAL_SURPRISE_FIELDS = ("source", "target", "edge_kind", "surprise_score") +_MINIMAL_GAP_FIELDS = ("name", "qualified_name", "community_id", "size", "degree") + + +def _project(rows: list[dict[str, Any]], fields: tuple[str, ...]) -> list[dict[str, Any]]: + """Keep only *fields* on each row, dropping keys the row does not have.""" + return [{k: r[k] for k in fields if k in r} for r in rows] def get_hub_nodes_func( repo_root: str | None = None, top_n: int = 10, + detail_level: str = "standard", ) -> dict[str, Any]: """Find the most connected nodes in the codebase graph. @@ -26,14 +48,33 @@ def get_hub_nodes_func( Args: repo_root: Repository root (auto-detected if omitted). - top_n: Number of top hubs to return (default 10). + top_n: Number of top hubs to return (default 10, capped at 100). + detail_level: "standard" (default) returns full node data; + "minimal" returns only name, kind, and total_degree. + + Returns: + Ranked hubs. ``total`` reports the untruncated candidate count and + ``truncated`` marks that ``top_n`` or the ceiling cut the list. """ + _validate_positive_int(top_n, "top_n") + store, _root = _get_store(repo_root or None) try: - hubs = find_hub_nodes(store, top_n=top_n) + hubs, total, truncated = _bounded( + find_hub_nodes(store, top_n=_FETCH_ALL), top_n, _MAX_HUB_NODES, + ) + if detail_level == "minimal": + hubs = _project(hubs, _MINIMAL_HUB_FIELDS) return { + "status": "ok", + "summary": ( + f"{total} hub node(s) ranked by degree" + + _shown_of(len(hubs), total) + ), "hub_nodes": hubs, "count": len(hubs), + "total": total, + "truncated": truncated, "next_tool_suggestions": [ "get_impact_radius -- check blast radius of a hub", "query_graph callers_of -- see what calls a hub", @@ -47,6 +88,7 @@ def get_hub_nodes_func( def get_bridge_nodes_func( repo_root: str | None = None, top_n: int = 10, + detail_level: str = "standard", ) -> dict[str, Any]: """Find architectural chokepoints via betweenness centrality. @@ -56,14 +98,33 @@ def get_bridge_nodes_func( Args: repo_root: Repository root (auto-detected if omitted). - top_n: Number of top bridges to return (default 10). + top_n: Number of top bridges to return (default 10, capped at 100). + detail_level: "standard" (default) returns full node data; + "minimal" returns only name, kind, and betweenness. + + Returns: + Ranked bridges. ``total`` reports the untruncated candidate count and + ``truncated`` marks that ``top_n`` or the ceiling cut the list. """ + _validate_positive_int(top_n, "top_n") + store, _root = _get_store(repo_root or None) try: - bridges = find_bridge_nodes(store, top_n=top_n) + bridges, total, truncated = _bounded( + find_bridge_nodes(store, top_n=_FETCH_ALL), top_n, _MAX_BRIDGE_NODES, + ) + if detail_level == "minimal": + bridges = _project(bridges, _MINIMAL_BRIDGE_FIELDS) return { + "status": "ok", + "summary": ( + f"{total} bridge node(s) ranked by betweenness" + + _shown_of(len(bridges), total) + ), "bridge_nodes": bridges, "count": len(bridges), + "total": total, + "truncated": truncated, "next_tool_suggestions": [ "get_hub_nodes -- find most connected nodes", "get_impact_radius -- check blast radius", @@ -76,6 +137,8 @@ def get_bridge_nodes_func( def get_knowledge_gaps_func( repo_root: str | None = None, + max_per_category: int = 15, + detail_level: str = "standard", ) -> dict[str, Any]: """Identify structural weaknesses in the codebase. @@ -85,26 +148,43 @@ def get_knowledge_gaps_func( Args: repo_root: Repository root (auto-detected if omitted). + max_per_category: Maximum entries per gap category (default 15, + capped at 50). ``summary`` and ``total_gaps`` always report the + untruncated counts. + detail_level: "standard" (default) returns full gap records; + "minimal" drops file paths and keeps identifying fields only. + + Returns: + Gaps by category. ``summary`` maps each category to its untruncated + count and ``truncated`` marks that at least one list was cut. """ + _validate_positive_int(max_per_category, "max_per_category") + store, _root = _get_store(repo_root or None) try: - gaps = find_knowledge_gaps(store) - total = sum(len(v) for v in gaps.values()) + raw = find_knowledge_gaps(store) + # Totals must come from the untruncated lists: the summary counts are + # the whole point of the tool, and capping them would hide the gap. + totals = {category: len(rows) for category, rows in raw.items()} + + gaps: dict[str, list[dict[str, Any]]] = {} + truncated = False + for category, rows in raw.items(): + visible, _total, cut = _bounded( + rows, max_per_category, _MAX_GAPS_PER_CATEGORY, + ) + if detail_level == "minimal": + visible = _project(visible, _MINIMAL_GAP_FIELDS) + gaps[category] = visible + truncated = truncated or cut + + total = sum(totals.values()) return { + "status": "ok", + "summary": totals, "gaps": gaps, "total_gaps": total, - "summary": { - "isolated_nodes": len(gaps["isolated_nodes"]), - "thin_communities": len( - gaps["thin_communities"] - ), - "untested_hotspots": len( - gaps["untested_hotspots"] - ), - "single_file_communities": len( - gaps["single_file_communities"] - ), - }, + "truncated": truncated, "next_tool_suggestions": [ "refactor dead_code -- find unused symbols", "get_hub_nodes -- find high-impact nodes", @@ -118,6 +198,7 @@ def get_knowledge_gaps_func( def get_surprising_connections_func( repo_root: str | None = None, top_n: int = 15, + detail_level: str = "standard", ) -> dict[str, Any]: """Find unexpected architectural coupling in the codebase. @@ -126,16 +207,35 @@ def get_surprising_connections_func( Args: repo_root: Repository root (auto-detected if omitted). - top_n: Number of top surprises to return (default 15). + top_n: Number of top surprises to return (default 15, capped at 100). + detail_level: "standard" (default) returns full edge records; + "minimal" returns only source, target, edge_kind, and score. + + Returns: + Ranked surprising edges. ``total`` reports the untruncated count and + ``truncated`` marks that ``top_n`` or the ceiling cut the list. """ + _validate_positive_int(top_n, "top_n") + store, _root = _get_store(repo_root or None) try: - surprises = find_surprising_connections( - store, top_n=top_n + surprises, total, truncated = _bounded( + find_surprising_connections(store, top_n=_FETCH_ALL), + top_n, + _MAX_SURPRISING, ) + if detail_level == "minimal": + surprises = _project(surprises, _MINIMAL_SURPRISE_FIELDS) return { + "status": "ok", + "summary": ( + f"{total} surprising connection(s) ranked by surprise score" + + _shown_of(len(surprises), total) + ), "surprising_connections": surprises, "count": len(surprises), + "total": total, + "truncated": truncated, "next_tool_suggestions": [ "get_architecture_overview -- community structure", "query_graph callers_of -- trace the coupling", @@ -155,6 +255,10 @@ def get_suggested_questions_func( surprising connections, thin communities, and untested hotspots. + The output is bounded by construction: ``generate_suggested_questions`` + draws at most 3 bridges, 3 hubs, 3 surprises, 2 thin communities and + 2 untested hotspots, so no result cap is needed here. + Args: repo_root: Repository root (auto-detected if omitted). """ diff --git a/code_review_graph/tools/community_tools.py b/code_review_graph/tools/community_tools.py index 4c6647c6..3647175f 100644 --- a/code_review_graph/tools/community_tools.py +++ b/code_review_graph/tools/community_tools.py @@ -9,18 +9,52 @@ from ..context_savings import attach_context_savings from ..graph import node_to_dict from ..hints import generate_hints, get_session -from ._common import _get_store +from ._common import _bounded, _get_store, _shown_of, _validate_positive_int # --------------------------------------------------------------------------- # Tool 13: list_communities [EXPLORE] # --------------------------------------------------------------------------- +# Hard ceilings. ``get_communities`` embeds every member's qualified name in +# each community, so a repo with one 3000-member community produced a 200k+ +# token response before this cap (the same class of bug as #849). +_MAX_COMMUNITIES = 200 +# A member entry is one absolute qualified name, ~36 tokens. 25 per +# community keeps a 200-community overview inside a single response. +_MAX_MEMBERS = 25 +# A per-edge cross-community row carries two absolute qualified names, +# ~100 tokens. 200 rows is the most a standard overview can afford. +_MAX_CROSS_EDGES = 200 + + +def _cap_members( + community: dict[str, Any], max_members: int, +) -> dict[str, Any]: + """Return a copy of *community* whose member lists are bounded. + + ``size`` already carries the true member count, so the caller never + loses the total; ``members_truncated`` marks that the list was cut. + """ + capped = dict(community) + for key in ("members", "member_details"): + rows = capped.get(key) + if not isinstance(rows, list): + continue + visible, total, truncated = _bounded(rows, max_members, _MAX_MEMBERS) + capped[key] = visible + if truncated: + capped[f"{key}_total"] = total + capped["members_truncated"] = True + return capped + def list_communities_func( repo_root: str | None = None, sort_by: str = "size", min_size: int = 0, detail_level: str = "standard", + max_results: int = 50, + max_members: int = 10, ) -> dict[str, Any]: """List detected code communities in the codebase. @@ -36,24 +70,42 @@ def list_communities_func( detail_level: "standard" (default) returns full community data; "minimal" returns only name, size, and cohesion per community. + max_results: Maximum communities to return (default 50, capped at + 200). ``total`` reports the untruncated count. + max_members: Maximum member names listed per community in standard + mode (default 10, capped at 100). Each community's + ``size`` still reports its true member count. Returns: - List of communities with size and cohesion scores. + Communities with size and cohesion scores, plus ``total`` and + ``truncated``. """ + _validate_positive_int(max_results, "max_results") + _validate_positive_int(max_members, "max_members") + store, root = _get_store(repo_root) try: - communities = get_communities( - store, sort_by=sort_by, min_size=min_size + communities, total, truncated = _bounded( + get_communities(store, sort_by=sort_by, min_size=min_size), + max_results, + _MAX_COMMUNITIES, ) if detail_level == "minimal": communities = [ {"name": c["name"], "size": c["size"], "cohesion": c["cohesion"]} for c in communities ] + else: + communities = [_cap_members(c, max_members) for c in communities] result: dict[str, object] = { "status": "ok", - "summary": f"Found {len(communities)} communities", + "summary": ( + f"Found {total} communities" + + _shown_of(len(communities), total) + ), "communities": communities, + "total": total, + "truncated": truncated, } result["_hints"] = generate_hints( "list_communities", result, get_session() @@ -75,6 +127,7 @@ def get_community_func( community_id: int | None = None, include_members: bool = False, repo_root: str | None = None, + max_members: int = 25, ) -> dict[str, Any]: """Get details of a single code community. @@ -87,10 +140,17 @@ def get_community_func( community_id: Database ID of the community. include_members: If True, include full member node details. repo_root: Repository root path. Auto-detected if omitted. + max_members: Maximum member entries to include (default 25, capped + at 100). The community's ``size`` still reports its + true member count and ``members_truncated`` marks the + cut. Without this bound a single 3000-member community + serialized to >130k tokens. Returns: Community details, or not_found status. """ + _validate_positive_int(max_members, "max_members") + store, root = _get_store(repo_root) try: community: dict | None = None @@ -122,6 +182,8 @@ def get_community_func( members = [node_to_dict(n) for n in member_nodes] community["member_details"] = members + community = _cap_members(community, max_members) + result = { "status": "ok", "summary": ( @@ -192,6 +254,8 @@ def _minimal_overview(overview: dict[str, Any]) -> dict[str, Any]: def get_architecture_overview_func( repo_root: str | None = None, detail_level: str = "minimal", + max_results: int = 100, + max_members: int = 10, ) -> dict[str, Any]: """Generate an architecture overview based on community structure. @@ -206,20 +270,47 @@ def get_architecture_overview_func( (typical reduction: 600KB -> <5KB); "standard" returns the full overview including per-edge cross-community detail. + max_results: Maximum cross-community rows and warnings to return + (default 100, capped at 200). + ``cross_community_edges_total`` reports the + untruncated count. + max_members: (standard only) Maximum member names per community + (default 10, capped at 25). Standard mode returned + >600k tokens on this repo without these bounds. Returns: Architecture overview with communities, cross-community edges, - and warnings. + warnings, and a ``truncated`` flag. """ + _validate_positive_int(max_results, "max_results") + _validate_positive_int(max_members, "max_members") + store, root = _get_store(repo_root) try: full_overview = get_architecture_overview(store) overview = full_overview if detail_level == "minimal": overview = _minimal_overview(full_overview) + else: + overview = dict(full_overview) + overview["communities"] = [ + _cap_members(c, max_members) + for c in overview.get("communities", []) + ] + cross, cross_total, truncated = _bounded( + overview["cross_community_edges"], max_results, _MAX_CROSS_EDGES, + ) + overview["cross_community_edges"] = cross + # One warning per highly-coupled community pair grows quadratically + # with the community count, so it needs the same bound. + warnings, warn_total, warn_cut = _bounded( + overview["warnings"], max_results, _MAX_CROSS_EDGES, + ) + overview["warnings"] = warnings + truncated = truncated or warn_cut n_communities = len(overview["communities"]) - n_cross = len(overview["cross_community_edges"]) - n_warnings = len(overview["warnings"]) + n_cross = len(cross) + n_warnings = warn_total cross_label = ( "community pairs" if detail_level == "minimal" @@ -229,10 +320,13 @@ def get_architecture_overview_func( "status": "ok", "summary": ( f"Architecture: {n_communities} communities, " - f"{n_cross} {cross_label}, " + f"{cross_total} {cross_label}" + + _shown_of(n_cross, cross_total) + ", " f"{n_warnings} warning(s)" ), **overview, + "cross_community_edges_total": cross_total, + "truncated": truncated, } result["_hints"] = generate_hints( "get_architecture_overview", result, get_session() diff --git a/code_review_graph/tools/docs.py b/code_review_graph/tools/docs.py index 88c68a30..d9464772 100644 --- a/code_review_graph/tools/docs.py +++ b/code_review_graph/tools/docs.py @@ -8,10 +8,18 @@ from ..embeddings import EmbeddingStore, embed_all_nodes from ..incremental import find_project_root, get_db_path -from ._common import _get_store, _resolve_root, _validate_repo_root +from ._common import ( + _get_store, + _resolve_root, + _validate_positive_int, + _validate_repo_root, +) logger = logging.getLogger(__name__) +# Hard ceiling for a single wiki page in one MCP response (~20k tokens). +_MAX_WIKI_CHARS = 80000 + # --------------------------------------------------------------------------- # Tool 7: embed_graph # --------------------------------------------------------------------------- @@ -249,6 +257,7 @@ def generate_wiki_func( def get_wiki_page_func( community_name: str, repo_root: str | None = None, + max_chars: int = 20000, ) -> dict[str, Any]: """Retrieve a specific wiki page by community name. @@ -258,13 +267,19 @@ def get_wiki_page_func( Args: community_name: Community name to look up (slugified for filename). repo_root: Repository root path. Auto-detected if omitted. + max_chars: Maximum characters of page content to return (default + 20000, capped at 80000). A wiki page grows with its community, + so a 3000-member community produces a page no context window + wants in one call. ``total_chars`` reports the real length. Returns: - Page content or not_found status. + Page content or not_found status, with ``truncated`` when cut. """ from ..incremental import get_data_dir from ..wiki import get_wiki_page + _validate_positive_int(max_chars, "max_chars") + root = _resolve_root(repo_root) wiki_dir = get_data_dir(root) / "wiki" content = get_wiki_page(wiki_dir, community_name) @@ -273,10 +288,16 @@ def get_wiki_page_func( "status": "not_found", "summary": f"No wiki page found for '{community_name}'.", } + total_chars = len(content) + limit = min(max_chars, _MAX_WIKI_CHARS) + truncated = total_chars > limit return { "status": "ok", "summary": ( - f"Wiki page for '{community_name}' ({len(content)} chars)" + f"Wiki page for '{community_name}' ({total_chars} chars)" + + (f", showing first {limit}" if truncated else "") ), - "content": content, + "content": content[:limit], + "total_chars": total_chars, + "truncated": truncated, } diff --git a/code_review_graph/tools/flows_tools.py b/code_review_graph/tools/flows_tools.py index 4df1f713..b0e4903d 100644 --- a/code_review_graph/tools/flows_tools.py +++ b/code_review_graph/tools/flows_tools.py @@ -7,12 +7,22 @@ from ..flows import get_flow_by_id, get_flows from ..hints import generate_hints, get_session -from ._common import _get_store +from ._common import _bounded, _get_store, _shown_of, _validate_positive_int # --------------------------------------------------------------------------- # Tool 10: list_flows [EXPLORE] # --------------------------------------------------------------------------- +# ``get_flows`` slices in SQL, so asking for "all" is how the tool counts the +# untruncated total before keeping a bounded prefix. +_FETCH_ALL = 10**9 + +# Hard ceilings. Flow counts scale with entry points, and ``get_flow`` +# embeds one record per step (plus optional source), so both need bounds. +_MAX_FLOWS = 200 +_MAX_FLOW_STEPS = 200 +_MAX_FLOW_SOURCE_LINES = 2000 + def list_flows( repo_root: str | None = None, @@ -31,21 +41,24 @@ def list_flows( repo_root: Repository root path. Auto-detected if omitted. sort_by: Sort column: criticality, depth, node_count, file_count, or name. - limit: Maximum flows to return (default: 50). + limit: Maximum flows to return (default: 50, capped at 200). kind: Optional filter by entry point kind (e.g. "Test", "Function"). detail_level: "standard" (default) returns full flow data; "minimal" returns only name, criticality, and node_count per flow. Returns: - List of flows with criticality scores. + Flows with criticality scores, plus ``total`` and ``truncated``. """ + _validate_positive_int(limit, "limit") + store, root = _get_store(repo_root) try: - fetch_limit = ( - limit if not kind else limit * 10 - ) # fetch more when filtering - flows = get_flows(store, sort_by=sort_by, limit=fetch_limit) + # Count every matching flow, then keep the bounded prefix — the same + # "count all, return a prefix" contract query.py uses for max_results. + # The flows table has one row per entry point, so a full read is cheap + # relative to serializing them all back to the client. + flows = get_flows(store, sort_by=sort_by, limit=_FETCH_ALL) if kind: filtered = [] @@ -55,7 +68,9 @@ def list_flows( node_kind = store.get_node_kind_by_id(ep_id) if node_kind == kind: filtered.append(f) - flows = filtered[:limit] + flows = filtered + + flows, total, truncated = _bounded(flows, limit, _MAX_FLOWS) if detail_level == "minimal": flows = [ @@ -69,8 +84,13 @@ def list_flows( result: dict[str, object] = { "status": "ok", - "summary": f"Found {len(flows)} execution flow(s)", + "summary": ( + f"Found {total} execution flow(s)" + + _shown_of(len(flows), total) + ), "flows": flows, + "total": total, + "truncated": truncated, } result["_hints"] = generate_hints( "list_flows", result, get_session() @@ -92,6 +112,8 @@ def get_flow( flow_name: str | None = None, include_source: bool = False, repo_root: str | None = None, + max_steps: int = 50, + max_source_lines: int = 400, ) -> dict[str, Any]: """Get details of a single execution flow. @@ -105,10 +127,19 @@ def get_flow( given. include_source: If True, include source code snippets for each step. repo_root: Repository root path. Auto-detected if omitted. + max_steps: Maximum steps to return (default 50, capped at 200). + ``flow["total_steps"]`` reports the untruncated count. + max_source_lines: Total source lines emitted across all steps when + ``include_source`` is set (default 400, capped at 2000). Without + this, one deep flow inlines every function body it touches. Returns: - Flow details with steps, or not_found status. + Flow details with steps, or not_found status. ``flow["truncated"]`` + marks that ``max_steps`` or the source budget cut the response. """ + _validate_positive_int(max_steps, "max_steps") + _validate_positive_int(max_source_lines, "max_source_lines") + store, root = _get_store(repo_root) try: flow: dict | None = None @@ -131,9 +162,22 @@ def get_flow( "summary": "No flow found matching the given criteria.", } - # Optionally include source snippets for each step - if include_source and "steps" in flow: - for step in flow["steps"]: + steps, total_steps, truncated = _bounded( + flow.get("steps") or [], max_steps, _MAX_FLOW_STEPS, + ) + flow["steps"] = steps + flow["total_steps"] = total_steps + flow["truncated"] = truncated + + # Optionally include source snippets for each step, spending a shared + # line budget so a deep flow cannot inline the whole call chain. + if include_source: + budget = min(max_source_lines, _MAX_FLOW_SOURCE_LINES) + for step in steps: + if budget <= 0: + flow["truncated"] = True + flow["source_truncated"] = True + break fp = Path(step["file"]) if step.get("file") else None if fp is not None and not fp.is_absolute(): fp = root / fp @@ -149,11 +193,13 @@ def get_flow( end = min( len(lines), step.get("line_end") or len(lines), + start + budget, ) step["source"] = "\n".join( f"{i + 1}: {lines[i]}" for i in range(start, end) ) + budget -= max(0, end - start) except (OSError, UnicodeDecodeError): step["source"] = "(could not read file)" @@ -163,6 +209,7 @@ def get_flow( f"Flow '{flow['name']}': {flow['node_count']} nodes, " f"depth {flow['depth']}, " f"criticality {flow['criticality']:.4f}" + + _shown_of(len(steps), total_steps) ), "flow": flow, } diff --git a/code_review_graph/tools/refactor_tools.py b/code_review_graph/tools/refactor_tools.py index c92891b3..bd775b5a 100644 --- a/code_review_graph/tools/refactor_tools.py +++ b/code_review_graph/tools/refactor_tools.py @@ -13,12 +13,31 @@ rename_preview, suggest_refactorings, ) -from ._common import _get_store, _validate_repo_root +from ._common import ( + _bounded, + _get_store, + _shown_of, + _validate_positive_int, + _validate_repo_root, +) # --------------------------------------------------------------------------- # Tool 17: refactor_tool [REFACTOR] # --------------------------------------------------------------------------- +# Hard ceiling. Dead-code and suggestion lists grow with the repository: +# an uncapped ``dead_code`` sweep returned ~47k tokens on this repo alone. +_MAX_REFACTOR_RESULTS = 150 + +_MINIMAL_DEAD_FIELDS = ("name", "kind", "relative_path", "line") +_MINIMAL_SUGGESTION_FIELDS = ("type", "description", "symbols") +_MINIMAL_EDIT_FIELDS = ("file", "line", "confidence") + + +def _project(rows: list[dict[str, Any]], fields: tuple[str, ...]) -> list[dict[str, Any]]: + """Keep only *fields* on each row, dropping keys the row does not have.""" + return [{k: r[k] for k in fields if k in r} for r in rows] + def refactor_func( mode: str = "rename", @@ -27,6 +46,8 @@ def refactor_func( kind: str | None = None, file_pattern: str | None = None, repo_root: str | None = None, + max_results: int = 50, + detail_level: str = "standard", ) -> dict[str, Any]: """Unified refactoring entry point. @@ -43,9 +64,15 @@ def refactor_func( kind: (dead_code mode) Optional node kind filter. file_pattern: (dead_code mode) Optional file path substring filter. repo_root: Repository root path. Auto-detected if omitted. + max_results: Maximum edits/symbols/suggestions to include in the + response (default 50, capped at 150). ``total`` always reports + the untruncated count. The stored rename preview keeps every + edit, so ``apply_refactor_tool`` still applies the full set. + detail_level: "standard" (default) returns full records; "minimal" + keeps only the identifying fields per record. Returns: - Mode-specific results dict. + Mode-specific results dict with ``total`` and ``truncated``. """ valid_modes = {"rename", "dead_code", "suggest"} if mode not in valid_modes: @@ -56,6 +83,7 @@ def refactor_func( f"Must be one of: {', '.join(sorted(valid_modes))}" ), } + _validate_positive_int(max_results, "max_results") store, root = _get_store(repo_root) try: @@ -73,15 +101,27 @@ def refactor_func( "status": "not_found", "summary": f"No node found matching '{old_name}'.", } + # Bound only the response copy. ``preview`` is the object held in + # the pending-refactor registry, so apply_refactor still writes + # every edit; slicing it here would silently drop real edits. + edits, total, truncated = _bounded( + preview["edits"], max_results, _MAX_REFACTOR_RESULTS, + ) + if detail_level == "minimal": + edits = _project(edits, _MINIMAL_EDIT_FIELDS) result = { "status": "ok", "summary": ( f"Rename preview: {old_name} -> {new_name}, " - f"{len(preview['edits'])} edit(s). " + f"{total} edit(s)" + + _shown_of(len(edits), total) + ". " f"Use apply_refactor_tool(refactor_id=" f"'{preview['refactor_id']}') to apply." ), **preview, + "edits": edits, + "total": total, + "truncated": truncated, } result["_hints"] = generate_hints( "refactor", result, get_session() @@ -89,14 +129,24 @@ def refactor_func( return result elif mode == "dead_code": - dead = find_dead_code( - store, kind=kind, file_pattern=file_pattern, root=root + dead, total, truncated = _bounded( + find_dead_code( + store, kind=kind, file_pattern=file_pattern, root=root + ), + max_results, + _MAX_REFACTOR_RESULTS, ) + if detail_level == "minimal": + dead = _project(dead, _MINIMAL_DEAD_FIELDS) result = { "status": "ok", - "summary": f"Found {len(dead)} dead code symbol(s).", + "summary": ( + f"Found {total} dead code symbol(s)" + + _shown_of(len(dead), total) + "." + ), "dead_code": dead, - "total": len(dead), + "total": total, + "truncated": truncated, } result["_hints"] = generate_hints( "refactor", result, get_session() @@ -104,15 +154,20 @@ def refactor_func( return result else: # suggest - suggestions = suggest_refactorings(store) + suggestions, total, truncated = _bounded( + suggest_refactorings(store), max_results, _MAX_REFACTOR_RESULTS, + ) + if detail_level == "minimal": + suggestions = _project(suggestions, _MINIMAL_SUGGESTION_FIELDS) result = { "status": "ok", "summary": ( - f"Generated {len(suggestions)} " - "refactoring suggestion(s)." + f"Generated {total} refactoring suggestion(s)" + + _shown_of(len(suggestions), total) + "." ), "suggestions": suggestions, - "total": len(suggestions), + "total": total, + "truncated": truncated, } result["_hints"] = generate_hints( "refactor", result, get_session() @@ -134,6 +189,7 @@ def apply_refactor_func( refactor_id: str, repo_root: str | None = None, dry_run: bool = False, + max_diff_files: int = 25, ) -> dict[str, Any]: """Apply a previously previewed refactoring to source files. @@ -148,13 +204,20 @@ def apply_refactor_func( without touching disk. The refactor_id remains valid so the user can review the diff, then call again with ``dry_run=False`` to actually write the changes. See: #176 + max_diff_files: Maximum per-file diffs to include in a dry run + (default 25, capped at 150). Renaming a widely-used symbol + produces one diff per touched file; ``would_modify`` still + lists every file that would change, and the write path is + never affected. Returns: Status with count of applied edits and modified files. When ``dry_run=True`` the response additionally contains ``would_modify`` - (list of file paths) and ``diffs`` (map of file -> unified-diff - string). + (list of file paths), ``diffs`` (map of file -> unified-diff + string), and ``diffs_truncated``. """ + _validate_positive_int(max_diff_files, "max_diff_files") + try: root = ( _validate_repo_root(Path(repo_root)) @@ -165,4 +228,13 @@ def apply_refactor_func( return {"status": "error", "error": str(exc)} result = apply_refactor(refactor_id, root, dry_run=dry_run) + + diffs = result.get("diffs") + if isinstance(diffs, dict) and diffs: + limit = min(max_diff_files, _MAX_REFACTOR_RESULTS) + if len(diffs) > limit: + kept = sorted(diffs)[:limit] + result["diffs"] = {path: diffs[path] for path in kept} + result["diffs_total"] = len(diffs) + result["diffs_truncated"] = True return result diff --git a/code_review_graph/tools/registry_tools.py b/code_review_graph/tools/registry_tools.py index d0b11d28..69dda2be 100644 --- a/code_review_graph/tools/registry_tools.py +++ b/code_review_graph/tools/registry_tools.py @@ -9,9 +9,14 @@ from ..graph import GraphStore from ..incremental import get_db_path from ..search import hybrid_search +from ._common import _bounded, _shown_of, _validate_positive_int logger = logging.getLogger(__name__) +# Hard ceiling on the merged result set. ``limit`` is per repo, so a +# registry with 40 repos returned 40x the caller's expectation. +_MAX_CROSS_REPO_RESULTS = 100 + # --------------------------------------------------------------------------- # Tool 21: list_repos [REGISTRY] @@ -50,6 +55,7 @@ def cross_repo_search_func( query: str, kind: str | None = None, limit: int = 20, + max_results: int = 50, ) -> dict[str, Any]: """Search across all registered repositories. @@ -60,12 +66,20 @@ def cross_repo_search_func( query: Search query string. kind: Optional node kind filter (e.g. "Function", "Class"). limit: Maximum results per repo (default: 20). + max_results: Maximum merged results to return across all repos + (default 50, capped at 100). ``total`` reports the untruncated + merged count; without it the response grew with the number of + registered repos rather than with the caller's ``limit``. Returns: - Combined search results from all registered repos. + Combined search results from all registered repos, plus ``total`` + and ``truncated``. """ from ..registry import Registry + _validate_positive_int(limit, "limit") + _validate_positive_int(max_results, "max_results") + try: registry = Registry() repos = registry.list_repos() @@ -110,15 +124,22 @@ def cross_repo_search_func( # Scores from different search paths are not comparable across repos. # Merge by each repo's local rank and use registry order as a stable tie-breaker. ranked_results.sort(key=lambda item: (item[0], item[1])) - all_results = [result for _, _, result in ranked_results] + all_results, total, truncated = _bounded( + [result for _, _, result in ranked_results], + max_results, + _MAX_CROSS_REPO_RESULTS, + ) return { "status": "ok", "summary": ( - f"Found {len(all_results)} result(s) across " + f"Found {total} result(s) across " f"{len(searched_repos)} repo(s) for '{query}'" + + _shown_of(len(all_results), total) ), "results": all_results, + "total": total, + "truncated": truncated, "repos_searched": searched_repos, } except Exception as exc: diff --git a/code_review_graph/tools/review.py b/code_review_graph/tools/review.py index b211857d..835e902b 100644 --- a/code_review_graph/tools/review.py +++ b/code_review_graph/tools/review.py @@ -13,10 +13,83 @@ from ..hints import generate_hints, get_session from ..incremental import get_changed_files, get_staged_and_unstaged from ..parser import normalize_file_path -from ._common import _get_store, _resolve_graph_file_paths +from ._common import ( + _bounded, + _get_store, + _resolve_graph_file_paths, + _shown_of, + _validate_positive_int, +) logger = logging.getLogger(__name__) +# Hard ceilings shared by the review tools. All three walk the full impact +# radius of a change set, so on a whole-repo diff every list below is +# proportional to the repository, not to the change. The numbers are set +# from measured cost per row against a 5.6k-node graph: +# node dict ~60 tok, node+risk_score ~118 tok, test gap ~85 tok, +# source line ~10 tok, flow with full steps ~980 tok, flow metadata ~18 tok. +_MAX_REVIEW_NODES = 100 +_MAX_REVIEW_EDGES = 150 +_MAX_REVIEW_FILES = 200 +_MAX_REVIEW_SOURCE_LINES = 800 +_MAX_LINES_PER_FILE = 500 +_MAX_CHANGED_FUNCTIONS = 100 +_MAX_DETECT_SOURCE_LINES = 600 + +# ``get_affected_flows`` in standard mode carries a full ``steps`` list per +# flow (~980 tokens each), so 50 flows is still ~49k tokens — #849 was only +# half-closed by capping the count. The ceiling therefore depends on +# detail_level, the same way query.py caps minimal mode at five results. +_MAX_AFFECTED_FLOWS_STANDARD = 25 +_MAX_AFFECTED_FLOWS_MINIMAL = 500 +# Flow depth varies hugely between codebases, so a flow *count* alone does +# not bound the response. Steps are filled from the most critical flow +# down until this shared budget runs out; the rest keep their metadata and +# are marked ``steps_omitted``. +_MAX_AFFECTED_FLOW_STEPS = 400 +_MAX_DETECT_FLOWS = 200 + +# ``detect_changes`` embeds affected flows for context, not for flow +# spelunking: every flow carries a full ``steps`` list, which is exactly +# what made get_affected_flows return 247k tokens in #849. Callers who want +# step detail should use get_affected_flows_tool, so the embedded copy keeps +# per-flow metadata only. +_DETECT_FLOW_FIELDS = ( + "id", "name", "criticality", "depth", "node_count", "file_count", +) + + +def _project(rows: list[dict[str, Any]], fields: tuple[str, ...]) -> list[dict[str, Any]]: + """Keep only *fields* on each row, dropping keys the row does not have.""" + return [{k: r[k] for k in fields if k in r} for r in rows] + + +def _bound_flow_steps( + flows: list[dict[str, Any]], +) -> tuple[list[dict[str, Any]], bool]: + """Spend a shared step budget across *flows*, most critical first. + + Returns ``(flows, truncated)``. Flows are already sorted by criticality, + so the ones a reviewer cares about keep their full step list; the tail + keeps metadata and is marked ``steps_omitted``. Every flow reports + ``total_steps`` so the untruncated depth is never lost. + """ + budget = _MAX_AFFECTED_FLOW_STEPS + truncated = False + bounded: list[dict[str, Any]] = [] + for flow in flows: + out = dict(flow) + steps = out.get("steps") or [] + out["total_steps"] = len(steps) + if len(steps) > budget: + out["steps"] = steps[:budget] + out["steps_omitted"] = True + truncated = True + budget -= min(len(steps), budget) + bounded.append(out) + return bounded, truncated + # --------------------------------------------------------------------------- # Tool 4: get_review_context @@ -31,6 +104,8 @@ def get_review_context( repo_root: str | None = None, base: str = "HEAD~1", detail_level: str = "standard", + max_results: int = 50, + max_files: int = 25, ) -> dict[str, Any]: """Generate a focused review context from changed files. @@ -47,11 +122,22 @@ def get_review_context( "minimal" returns summary, risk level, changed/impacted file counts, top 5 key entity names, test gap count, and next tool suggestions. Default: "standard". + max_results: Maximum graph nodes per list and edges to return + (default 50; nodes capped at 200, edges at 300). Each list + carries its untruncated ``*_total`` count. + max_files: Maximum files to list and to emit source snippets for + (default 25, capped at 200). A whole-repo diff otherwise inlined + every tracked file's source. Snippets additionally share an + 800-line budget and ``max_lines_per_file`` is capped at 500. Returns: Structured review context with subgraph, source snippets, and - review guidance. + review guidance, plus a ``truncated`` flag. """ + _validate_positive_int(max_results, "max_results") + _validate_positive_int(max_files, "max_files") + _validate_positive_int(max_lines_per_file, "max_lines_per_file") + store, root = _get_store(repo_root) try: # Get impact radius first @@ -122,44 +208,79 @@ def get_review_context( attach_context_savings(result, original_tokens=original_tokens) return result - # Build review context + # Build review context. Every list below scales with the change set, + # so each is bounded and reports its untruncated total. + shown_files, files_total, files_cut = _bounded( + changed_files, max_files, _MAX_REVIEW_FILES, + ) + impacted_files, impacted_total, impacted_cut = _bounded( + impact["impacted_files"], max_files, _MAX_REVIEW_FILES, + ) + changed_nodes, changed_nodes_total, cn_cut = _bounded( + impact["changed_nodes"], max_results, _MAX_REVIEW_NODES, + ) + impacted_nodes, impacted_nodes_total, in_cut = _bounded( + impact["impacted_nodes"], max_results, _MAX_REVIEW_NODES, + ) + edges, edges_total, edges_cut = _bounded( + impact["edges"], max_results, _MAX_REVIEW_EDGES, + ) + truncated = ( + files_cut or impacted_cut or cn_cut or in_cut or edges_cut + ) + context: dict[str, Any] = { - "changed_files": changed_files, - "impacted_files": impact["impacted_files"], + "changed_files": shown_files, + "changed_files_total": files_total, + "impacted_files": impacted_files, + "impacted_files_total": impacted_total, "graph": { - "changed_nodes": [ - node_to_dict(n) for n in impact["changed_nodes"] - ], - "impacted_nodes": [ - node_to_dict(n) for n in impact["impacted_nodes"] - ], - "edges": [edge_to_dict(e) for e in impact["edges"]], + "changed_nodes": [node_to_dict(n) for n in changed_nodes], + "changed_nodes_total": changed_nodes_total, + "impacted_nodes": [node_to_dict(n) for n in impacted_nodes], + "impacted_nodes_total": impacted_nodes_total, + "edges": [edge_to_dict(e) for e in edges], + "edges_total": edges_total, }, + "truncated": truncated, } - # Add source snippets for changed files + # Add source snippets for the bounded file list, spending a shared + # line budget. Snippets were 109k of a 134k-token worst case: without + # a total budget, ``max_lines_per_file`` alone lets N files each + # contribute a whole file. if include_source: snippets = {} - for rel_path in changed_files: + per_file = min(max_lines_per_file, _MAX_LINES_PER_FILE) + budget = _MAX_REVIEW_SOURCE_LINES + for rel_path in shown_files: + if budget <= 0: + context["source_truncated"] = True + context["truncated"] = True + break full_path = root / rel_path if full_path.is_file(): try: lines = full_path.read_text( errors="replace" ).splitlines() - if len(lines) > max_lines_per_file: + allowed = min(per_file, budget) + if len(lines) > allowed: # Include only the relevant functions/classes relevant_lines = _extract_relevant_lines( lines, impact["changed_nodes"], str(full_path), + allowed, ) snippets[rel_path] = relevant_lines + budget -= allowed else: snippets[rel_path] = "\n".join( f"{i+1}: {line}" for i, line in enumerate(lines) ) + budget -= len(lines) except (OSError, UnicodeDecodeError): snippets[rel_path] = "(could not read file)" context["source_snippets"] = snippets @@ -169,10 +290,13 @@ def get_review_context( context["review_guidance"] = guidance summary_parts = [ - f"Review context for {len(changed_files)} changed file(s):", - f" - {len(impact['changed_nodes'])} directly changed nodes", - f" - {len(impact['impacted_nodes'])} impacted nodes" - f" in {len(impact['impacted_files'])} files", + f"Review context for {files_total} changed file(s)" + + _shown_of(len(shown_files), files_total) + ":", + f" - {changed_nodes_total} directly changed nodes" + + _shown_of(len(changed_nodes), changed_nodes_total), + f" - {impacted_nodes_total} impacted nodes" + f" in {impacted_total} files" + + _shown_of(len(impacted_nodes), impacted_nodes_total), "", "Review guidance:", guidance, @@ -190,9 +314,14 @@ def get_review_context( def _extract_relevant_lines( - lines: list[str], nodes: list, file_path: str + lines: list[str], nodes: list, file_path: str, max_lines: int = 200, ) -> str: - """Extract only the lines relevant to changed nodes.""" + """Extract only the lines relevant to changed nodes. + + Bounded by *max_lines*: a file where every function changed merges into + one range covering the whole file, which would defeat the caller's + ``max_lines_per_file`` budget entirely. + """ ranges = [] for n in nodes: if n.file_path == file_path: @@ -203,7 +332,7 @@ def _extract_relevant_lines( if not ranges: # Show first N lines as fallback return "\n".join( - f"{i+1}: {line}" for i, line in enumerate(lines[:50]) + f"{i+1}: {line}" for i, line in enumerate(lines[:min(50, max_lines)]) ) # Merge overlapping ranges @@ -216,11 +345,16 @@ def _extract_relevant_lines( merged.append((start, end)) parts: list[str] = [] + emitted = 0 for start, end in merged: + if emitted >= max_lines: + parts.append("... (truncated)") + break if parts: parts.append("...") - for i in range(start, end): + for i in range(start, min(end, start + max_lines - emitted)): parts.append(f"{i+1}: {lines[i]}") + emitted += 1 return "\n".join(parts) @@ -312,11 +446,19 @@ def get_affected_flows_func( Every flow carries a full ``steps`` list in standard mode, so large change sets can exceed 200k tokens without a bound (#849). max_flows: Maximum flows to return (default: 50). ``total`` always - reports the untruncated count; 0 disables the limit. + reports the untruncated count; 0 means "no caller limit". + Standard mode additionally caps the visible flows at 25 and + minimal mode at 500, because one standard flow costs ~980 + tokens against ~18 for a minimal one. This mirrors the way + query.py caps minimal-mode results at five. Standard mode also + spends a shared 400-step budget across the returned flows, so + a codebase with very deep call chains cannot blow the budget + with a legal flow count. Returns: Affected flows sorted by criticality; ``truncated`` is set when - ``max_flows`` cut the list. + ``max_flows``, the per-detail-level ceiling, or the step budget cut + the response. """ store, root = _get_store(repo_root) try: @@ -340,27 +482,27 @@ def get_affected_flows_func( total = result["total"] flows = result["affected_flows"] - truncated = bool(max_flows) and max_flows > 0 and total > max_flows - if truncated: - flows = flows[:max_flows] + ceiling = ( + _MAX_AFFECTED_FLOWS_MINIMAL if detail_level == "minimal" + else _MAX_AFFECTED_FLOWS_STANDARD + ) + # ``max_flows=0`` keeps its documented "no caller limit" meaning, but + # the ceiling still applies -- an escape hatch that can return 250k + # tokens is the bug #849 reported, not a feature. + limit = ceiling if max_flows <= 0 else min(max_flows, ceiling) + truncated = total > limit + flows = flows[:limit] if detail_level == "minimal": - flows = [ - { - "id": f.get("id"), - "name": f.get("name"), - "criticality": f.get("criticality"), - "depth": f.get("depth"), - "node_count": f.get("node_count"), - "file_count": f.get("file_count"), - } - for f in flows - ] + flows = _project(flows, _DETECT_FLOW_FIELDS) + else: + flows, steps_cut = _bound_flow_steps(flows) + truncated = truncated or steps_cut out = { "status": "ok", "summary": ( f"{total} flow(s) affected by changes " f"in {len(changed_files)} file(s)" - + (f", showing {len(flows)}" if truncated else "") + + _shown_of(len(flows), total) ), "changed_files": changed_files, "affected_flows": flows, @@ -389,6 +531,8 @@ def detect_changes_func( max_depth: int = 2, repo_root: str | None = None, detail_level: str = "standard", + max_results: int = 25, + max_flows: int = 20, ) -> dict[str, Any]: """Detect changes and produce risk-scored review guidance. @@ -408,11 +552,20 @@ def detect_changes_func( "minimal" returns only summary, risk_score, changed_file_count, test_gap_count, and top 3 review priorities (text only). Default: "standard". + max_results: Maximum changed functions and test gaps to return + (default 25, capped at 200). ``changed_functions_total`` and + ``test_gaps_total`` report the untruncated counts. + max_flows: Maximum affected flows to embed (default 20, capped at + 200). The embedded flows carry per-flow metadata only; use + get_affected_flows_tool for step detail. See #849. Returns: Risk-scored analysis with changed functions, affected flows, - test gaps, and review priorities. + test gaps, and review priorities, plus ``truncated``. """ + _validate_positive_int(max_results, "max_results") + _validate_positive_int(max_flows, "max_flows") + store, root = _get_store(repo_root) try: # Detect changed files if not provided. @@ -454,9 +607,14 @@ def detect_changes_func( base=base, ) - # Optionally include source snippets for changed functions. + # Optionally include source snippets for changed functions, spending a + # shared line budget. Inlining every changed function body turned a + # whole-repo diff into a 30k-token ``changed_functions`` list. if include_source: + budget = _MAX_DETECT_SOURCE_LINES for func in analysis.get("changed_functions", []): + if budget <= 0: + break fp = func.get("file_path") ls = func.get("line_start") le = func.get("line_end") @@ -468,11 +626,12 @@ def detect_changes_func( errors="replace" ).splitlines() start = max(0, ls - 1) - end = min(len(lines), le) + end = min(len(lines), le, start + budget) func["source"] = "\n".join( f"{i + 1}: {lines[i]}" for i in range(start, end) ) + budget -= max(0, end - start) except (OSError, UnicodeDecodeError): func["source"] = "(could not read file)" @@ -491,10 +650,43 @@ def detect_changes_func( "review_priorities": top_priorities, } else: + funcs, funcs_total, funcs_cut = _bounded( + analysis.get("changed_functions", []), + max_results, _MAX_CHANGED_FUNCTIONS, + ) + gaps, gaps_total, gaps_cut = _bounded( + analysis.get("test_gaps", []), + max_results, _MAX_CHANGED_FUNCTIONS, + ) + flows, flows_total, flows_cut = _bounded( + analysis.get("affected_flows", []), + max_flows, _MAX_DETECT_FLOWS, + ) + files, files_total, files_cut = _bounded( + changed_files, max_results, _MAX_REVIEW_FILES, + ) + any_cut = funcs_cut or gaps_cut or flows_cut or files_cut + summary = analysis.get("summary", "") + if any_cut: + summary += ( + "\n - Response bounded: " + f"{len(funcs)} of {funcs_total} changed function(s), " + f"{len(gaps)} of {gaps_total} test gap(s), " + f"{len(flows)} of {flows_total} flow(s) shown" + ) result = { "status": "ok", - "changed_files": changed_files, **analysis, + "summary": summary, + "changed_files": files, + "changed_file_count": files_total, + "changed_functions": funcs, + "changed_functions_total": funcs_total, + "test_gaps": gaps, + "test_gaps_total": gaps_total, + "affected_flows": _project(flows, _DETECT_FLOW_FIELDS), + "affected_flows_total": flows_total, + "truncated": any_cut, } result["_hints"] = generate_hints( "detect_changes", result, get_session() diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 59b7fefc..704bdc23 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -67,6 +67,7 @@ pattern: str # callers_of, references_to, callees_of, imports_of, importers_o target: str # Node name, qualified name, or file path repo_root: str | None detail_level: str = "standard" # "standard" or "minimal" +max_results: int = 100 # Minimal mode additionally caps visible results at 5 ``` #### `get_review_context_tool` @@ -74,11 +75,15 @@ detail_level: str = "standard" # "standard" or "minimal" changed_files: list[str] | None max_depth: int = 2 include_source: bool = True -max_lines_per_file: int = 200 +max_lines_per_file: int = 200 # Capped at 500 repo_root: str | None base: str = "HEAD~1" detail_level: str = "standard" # "standard" or "minimal" +max_results: int = 50 # Graph nodes per list (max 100) and edges (max 150) +max_files: int = 25 # Files listed and given snippets (max 200) ``` +Snippets share an 800-line budget across the whole response. Each list reports +its untruncated `*_total`, and `context.truncated` marks any cut. Relevant responses may include compact estimated `context_savings` metadata. #### `traverse_graph_tool` @@ -145,6 +150,8 @@ flow_id: int | None # Database ID from list_flows_tool flow_name: str | None # Name to search (partial match) include_source: bool = False # Include source snippets for each step repo_root: str | None +max_steps: int = 50 # Capped at 200; flow.total_steps reports the full count +max_source_lines: int = 400 # Shared across all steps; capped at 2000 ``` #### `get_affected_flows_tool` @@ -152,7 +159,12 @@ repo_root: str | None changed_files: list[str] | None # Auto-detected from VCS base: str = "HEAD~1" repo_root: str | None +detail_level: str = "standard" # "standard" full step details, "minimal" metadata only +max_flows: int = 50 # 0 means "no caller limit" ``` +Standard mode carries a full `steps` list per flow, so it additionally caps +visible flows at 25 and spends a shared 400-step budget across them; minimal +mode caps at 500. `total` always reports the untruncated flow count. See #849. ### Community Tools @@ -162,7 +174,11 @@ sort_by: str = "size" # size, cohesion, name min_size: int = 0 repo_root: str | None detail_level: str = "standard" +max_results: int = 50 # Communities returned (max 200) +max_members: int = 10 # Member names per community in standard mode (max 25) ``` +Each community's `size` still reports its true member count; +`members_truncated` marks a cut member list. #### `get_community_tool` ``` @@ -170,38 +186,48 @@ community_name: str | None # Name to search (partial match) community_id: int | None # Database ID include_members: bool = False repo_root: str | None +max_members: int = 25 # Member entries returned (max 25) ``` #### `get_architecture_overview_tool` ``` repo_root: str | None detail_level: str = "minimal" # "minimal" compact default, "standard" full detail +max_results: int = 100 # Cross-community rows and warnings (max 200) +max_members: int = 10 # Member names per community in standard mode (max 25) ``` +`cross_community_edges_total` reports the untruncated row count. Minimal responses may include compact estimated `context_savings` metadata. ### Graph Health and Architecture Tools #### `get_hub_nodes_tool` ``` -top_n: int = 10 +top_n: int = 10 # Capped at 100 repo_root: str | None +detail_level: str = "standard" # "minimal" returns name, kind, total_degree ``` #### `get_bridge_nodes_tool` ``` -top_n: int = 10 +top_n: int = 10 # Capped at 100 repo_root: str | None +detail_level: str = "standard" # "minimal" returns name, kind, betweenness ``` #### `get_knowledge_gaps_tool` ``` repo_root: str | None +max_per_category: int = 15 # Entries per gap category (max 50) +detail_level: str = "standard" # "minimal" drops file paths ``` +`summary` maps each category to its untruncated count. #### `get_surprising_connections_tool` ``` -top_n: int = 15 +top_n: int = 15 # Capped at 100 repo_root: str | None +detail_level: str = "standard" # "minimal" returns source, target, kind, score ``` #### `get_suggested_questions_tool` @@ -219,8 +245,13 @@ include_source: bool = False max_depth: int = 2 repo_root: str | None detail_level: str = "standard" +max_results: int = 25 # Changed functions, test gaps, changed files (max 100) +max_flows: int = 20 # Affected flows embedded (max 200) ``` Primary tool for code review. Maps changed files to affected functions, flows, communities, and test coverage gaps. Returns risk scores and prioritized review items. +Embedded flows carry per-flow metadata only — use `get_affected_flows_tool` for +step detail. `changed_functions_total`, `test_gaps_total`, and +`affected_flows_total` report the untruncated counts. Relevant responses may include compact estimated `context_savings` metadata. #### `refactor_tool` @@ -231,13 +262,18 @@ new_name: str | None # (rename) New name kind: str | None # (dead_code) Function or Class file_pattern: str | None # (dead_code) Filter by file path substring repo_root: str | None +max_results: int = 50 # Edits/symbols/suggestions returned (max 150) +detail_level: str = "standard" # "minimal" keeps identifying fields only ``` +Truncating a rename preview truncates only the response: the stored preview +keeps every edit, so `apply_refactor_tool` still applies the full set. #### `apply_refactor_tool` ``` refactor_id: str # ID from prior refactor_tool call repo_root: str | None dry_run: bool = False # Return diff without writing files +max_diff_files: int = 25 # Per-file diffs in a dry run (max 150) ``` ### Wiki Tools @@ -252,7 +288,9 @@ force: bool = False # Regenerate all pages even if unchanged ``` community_name: str # Community name to look up repo_root: str | None +max_chars: int = 20000 # Page content returned (max 80000) ``` +`total_chars` reports the real page length; `truncated` marks a cut. ### Multi-Repo Tools @@ -265,9 +303,25 @@ repo_root: str | None ``` query: str kind: str | None -limit: int = 20 +limit: int = 20 # Results per repo +max_results: int = 50 # Merged results across all repos (max 100) ``` +## Result Bounds + +Every tool that returns a list is bounded, so no single MCP response can blow +a context window (see #849). The contract is uniform: + +- Defaults are small. Pass the tool's cap parameter to widen up to its hard + ceiling, or a smaller value to narrow. +- Truncation is never silent: the response reports the untruncated count + (`total`, or a `*_total` field per list), sets `truncated: true`, and the + summary line says how many of how many are shown. +- Cap parameters reject values below 1 and reject booleans, the same way + `query_graph_tool`'s `max_results` does. +- Hard ceilings are enforced in code and pinned by `tests/test_token_budget.py`, + which records the measured per-tool budget table as reviewable data. + ## MCP Prompts (5 workflow templates) ### `review_changes` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 87a383b0..b775fc5f 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -1,6 +1,7 @@ # Features ## v2.3.6 (Current) +- **Every MCP tool response is bounded**: #849 found `get_affected_flows` returning ~247k tokens inside a workflow documented as "5 tool calls, 800 tokens total"; a sweep of the other 29 tools found the same class of bug in ten more places — `list_communities` returned 206k tokens with *default* arguments, `get_community` 134k, `get_architecture_overview` 625k in standard mode. All are now capped with a uniform contract: `total` (or a per-list `*_total`) always reports the untruncated count, `truncated` marks the cut, and the summary says how many of how many are shown. Cap parameters reject values below 1 and reject booleans. `detail_level="minimal"` was added to the analysis and refactor tools. `tests/test_token_budget.py` pins the per-tool budget table and the hard ceilings so a removed cap fails CI. See [COMMANDS.md](COMMANDS.md#result-bounds). - **Framework-aware PHP parsing**: traits, enums, object creation, and base clauses are indexed; Composer PSR-4 resolution is longest-prefix, multi-directory, cached, and repository-bounded; Blade references ignore comments/escaped directives; Laravel Route and Eloquent edges require explicit framework/import/receiver evidence. - **Custom languages without forking**: drop a `.code-review-graph/languages.toml` into your repo to index any grammar shipped by tree-sitter-language-pack — extension map plus node-type lists, validated and capped, with built-in languages always winning. See [CUSTOM_LANGUAGES.md](CUSTOM_LANGUAGES.md). - **GitHub Action for risk-scored PR reviews**: composite `action.yml` builds/restores the graph from CI cache, runs `detect-changes` against the PR base, and upserts a sticky comment with risk table, affected flows, test gaps, and the Token Savings line. Optional `fail-on-risk` merge gate. Dogfooded on this repo via `.github/workflows/pr-review.yml`. See [GITHUB_ACTION.md](GITHUB_ACTION.md). diff --git a/tests/test_pr853_edges.py b/tests/test_pr853_edges.py index 78c18504..416e57e3 100644 --- a/tests/test_pr853_edges.py +++ b/tests/test_pr853_edges.py @@ -122,21 +122,40 @@ def test_truncated_key_present_when_nothing_matches(self): class TestDefaultCapAtScale(_FlowFixture): - """The default max_flows=50 must bound a 60-flow response (#849).""" + """The default max_flows must bound a 60-flow response (#849). + + PR #853 capped the *count* at 50. That was not enough: in standard mode + each flow carries a full ``steps`` list (~980 tokens on a real graph), so + 50 flows still serialized to ~49k tokens. Standard mode now caps at 25 + and minimal mode at 500 -- a per-detail-level ceiling, the same shape + query.py uses when it caps minimal-mode results at five. + """ def setup_method(self): self.setup_flows(60) - def test_default_truncates_sixty_flows_to_fifty(self): + def test_default_truncates_sixty_flows_to_the_standard_ceiling(self): result = self.affected() assert result["status"] == "ok" assert result["total"] == 60 - assert len(result["affected_flows"]) == 50 + assert len(result["affected_flows"]) == 25 assert result["truncated"] is True - assert "showing 50" in result["summary"] + assert "showing 25 of 60" in result["summary"] + + def test_zero_means_no_caller_limit_but_still_hits_the_ceiling(self): + """``max_flows=0`` keeps its documented meaning, bounded. - def test_zero_disables_limit_at_scale(self): + An escape hatch that can return a quarter of a million tokens is the + bug #849 reported, not a feature, so the ceiling still applies. + """ result = self.affected(max_flows=0) + assert result["total"] == 60 + assert len(result["affected_flows"]) == 25 + assert result["truncated"] is True + + def test_zero_in_minimal_mode_returns_everything_under_its_ceiling(self): + """Minimal rows cost ~18 tokens, so their ceiling is 500.""" + result = self.affected(max_flows=0, detail_level="minimal") assert result["truncated"] is False assert len(result["affected_flows"]) == result["total"] == 60 diff --git a/tests/test_token_budget.py b/tests/test_token_budget.py new file mode 100644 index 00000000..20936e02 --- /dev/null +++ b/tests/test_token_budget.py @@ -0,0 +1,847 @@ +"""Per-tool token budgets for every registered MCP tool. + +Why this file exists +-------------------- +The project's core promise is that a graph tool call is cheap: CLAUDE.md +documents "5 tool calls, 800 tokens total" for a task. Issue #849 found +``get_affected_flows`` returning ~247k tokens inside that workflow, and +PR #853 capped that one tool. A sweep of the other 29 found the same class +of bug in ten more places -- ``list_communities`` returned 206k tokens with +*default* arguments, ``get_community`` 134k, ``get_architecture_overview`` +625k in standard mode. + +This module is the regression guard. It calls every registered tool against +one real, module-scoped fixture graph and asserts the serialized response +stays under a documented per-tool ceiling. Removing a cap, or adding an +unbounded field to a response, makes the matching case fail loudly. + +How the numbers are produced +---------------------------- +* Responses are serialized the way FastMCP 3.x serializes them: + ``pydantic_core.to_json(value, fallback=str)`` -- compact JSON, no indent. +* Tokens are counted with tiktoken ``cl100k_base`` when tiktoken is + importable. tiktoken is not a project dependency, so the documented + fallback is ``len(serialized) / 4``. The two agree within ~5% on this + payload shape (measured: 206,858 tiktoken vs 203,131 len/4 on the same + 812KB response), and every ceiling below carries far more headroom than + that, so the assertions hold under either counter. + +Reading the budget table +------------------------ +``DEFAULT_BUDGET`` is what the tool costs with only its required arguments +-- the number an agent actually pays in a normal workflow. +``WORST_BUDGET`` is what it costs with every verbosity knob pushed past its +hard ceiling; it proves the ceiling exists and binds. + +The fixture graph is deliberately small, so these ceilings are not the +absolute worst case on a large repository. What they pin down is the +*shape* of each response: a cap that gets removed lets the list grow with +the fixture's node count and blows the ceiling immediately. +""" + +from __future__ import annotations + +import asyncio +import inspect +import json +import os +from pathlib import Path +from typing import Any + +import pytest + +from code_review_graph import main as crg_main +from code_review_graph.graph import GraphStore +from code_review_graph.incremental import full_build +from code_review_graph.tools import analysis_tools, community_tools, review +from code_review_graph.tools import refactor_tools as refactor_mod + +try: # pragma: no cover - exercised only when tiktoken is installed + import tiktoken + + _ENCODING = tiktoken.get_encoding("cl100k_base") +except Exception: # pragma: no cover - the documented default path + _ENCODING = None + + +# --------------------------------------------------------------------------- +# Fixture repository +# --------------------------------------------------------------------------- + +# Sized so the hard ceilings actually bind: 6 packages x 12 modules x 12 +# functions = 864 functions across 78 files, giving communities of ~150 +# members, >100 hub candidates and >25 affected flows. That is what lets +# ``test_hard_ceilings_bind`` assert exact truncated lengths rather than +# hoping a cap was applied. +_PACKAGES = 6 +_MODULES_PER_PACKAGE = 12 +_FUNCS_PER_MODULE = 12 + + +def _write_fixture_repo(root: Path) -> list[str]: + """Generate a deterministic multi-package Python repo. Returns rel paths.""" + (root / ".code-review-graph").mkdir(parents=True, exist_ok=True) + rel_paths: list[str] = [] + + for pkg in range(_PACKAGES): + pkg_dir = root / f"pkg{pkg}" + pkg_dir.mkdir(exist_ok=True) + (pkg_dir / "__init__.py").write_text("", encoding="utf-8") + rel_paths.append(f"pkg{pkg}/__init__.py") + + for mod in range(_MODULES_PER_PACKAGE): + lines = [f'"""Module {pkg}.{mod}."""', ""] + # Import from the neighbouring package so cross-community edges, + # hub nodes and bridge nodes all have something to find. + neighbour = (pkg + 1) % _PACKAGES + lines.append(f"from pkg{neighbour}.mod0 import helper_{neighbour}_0_0") + lines.append("") + for fn in range(_FUNCS_PER_MODULE): + name = f"helper_{pkg}_{mod}_{fn}" + lines.append(f"def {name}(value):") + lines.append(f' """Helper {pkg}.{mod}.{fn}."""') + # A body long enough for find_large_functions and for the + # source-snippet budgets to have something to trim. + for step in range(12): + lines.append(f" value = value + {step} # step {step}") + if fn > 0: + lines.append(f" value = helper_{pkg}_{mod}_{fn - 1}(value)") + lines.append(f" return helper_{neighbour}_0_0(value)") + lines.append("") + path = pkg_dir / f"mod{mod}.py" + path.write_text("\n".join(lines), encoding="utf-8") + rel_paths.append(f"pkg{pkg}/mod{mod}.py") + + # One test module per package so flows have entry points and + # tests_for / test-gap analysis has real data. + test_lines = [f"from pkg{pkg}.mod0 import *", ""] + for fn in range(_FUNCS_PER_MODULE): + test_lines.append(f"def test_helper_{pkg}_0_{fn}():") + test_lines.append(f" assert helper_{pkg}_0_{fn}(1) is not None") + test_lines.append("") + test_path = pkg_dir / f"test_pkg{pkg}.py" + test_path.write_text("\n".join(test_lines), encoding="utf-8") + rel_paths.append(f"pkg{pkg}/test_pkg{pkg}.py") + + return rel_paths + + +@pytest.fixture(scope="module") +def graph_repo(tmp_path_factory) -> dict[str, Any]: + """Build the fixture graph exactly once for the whole module.""" + root = tmp_path_factory.mktemp("token-budget-repo") + rel_paths = _write_fixture_repo(root) + + db_path = root / ".code-review-graph" / "graph.db" + # Serial parsing keeps the build deterministic and avoids spawning a + # ProcessPoolExecutor inside the test session. + os.environ["CRG_SERIAL_PARSE"] = "1" + with GraphStore(db_path) as store: + full_build(root, store) + + # Populate communities and flows so the tools that read them have data. + asyncio.run(crg_main.run_postprocess_tool(repo_root=str(root))) + asyncio.run(crg_main.generate_wiki_tool(repo_root=str(root))) + + return { + "root": str(root), + "files": rel_paths, + # The last module of the last package: imported by nothing, so a + # change there is an ordinary change rather than a repo-wide one. + "leaf_file": f"pkg{_PACKAGES - 1}/mod{_MODULES_PER_PACKAGE - 1}.py", + } + + +# --------------------------------------------------------------------------- +# Token accounting +# --------------------------------------------------------------------------- + + +def _serialize(value: Any) -> str: + """Serialize a tool result the way the FastMCP layer does.""" + try: + import pydantic_core + + return pydantic_core.to_json(value, fallback=str).decode() + except Exception: # pragma: no cover - pydantic_core ships with fastmcp + return json.dumps(value, default=str, separators=(",", ":")) + + +def count_tokens(value: Any) -> int: + """Token cost of a serialized tool response. + + Uses tiktoken cl100k_base when available, otherwise the documented + ``len / 4`` estimate. See the module docstring for why both are safe. + """ + text = _serialize(value) + if _ENCODING is not None: + return len(_ENCODING.encode(text, disallowed_special=())) + return len(text) // 4 + + +# --------------------------------------------------------------------------- +# The budget table +# --------------------------------------------------------------------------- + +# A sentinel that pushes any result cap past its hard ceiling. +HUGE = 10**6 + +# Tools whose result lists live in code_review_graph/tools/query.py. That +# module is owned elsewhere and its unbounded worst cases are reported, not +# fixed, by this change: +# * get_impact_radius -- changed_nodes and edges ignore max_results +# (3.4M tokens on a whole-repo diff), and max_results is not even +# exposed on the MCP tool signature. +# * find_large_functions -- limit is neither validated nor capped +# (737k tokens at limit=10**6). +# * traverse_graph -- token_budget is neither validated nor capped +# (385k tokens at token_budget=10**6). +# * semantic_search_nodes -- limit is neither validated nor capped. +# Their *default* budgets are still asserted below; only the worst case is +# skipped, so a regression in normal use is still caught here. +QUERY_OWNED_UNBOUNDED = { + "get_impact_radius_tool", + "find_large_functions_tool", + "traverse_graph_tool", + "semantic_search_nodes_tool", +} + +# No tool this change owns may exceed this even with every knob maxed out. +# Before the sweep, six tools blew past it by one to two orders of +# magnitude. It is deliberately generous: it is a catastrophe backstop, not +# the workflow budget. The workflow budget is DEFAULT_BUDGET. +ABSOLUTE_MAX_TOKENS = 50_000 + +# tool name -> (default kwargs, worst-case kwargs, DEFAULT_BUDGET, WORST_BUDGET) +# +# Budgets carry roughly 2x headroom over the measured fixture cost so that +# ordinary parser or fixture drift does not turn this into a flaky test, +# while removing a cap (which multiplies a list by 5-50x) still fails. +BUDGETS: dict[str, dict[str, Any]] = { + "build_or_update_graph_tool": { + "default": {"postprocess": "none"}, + "worst": {"postprocess": "none"}, + "default_max": 1_500, + "worst_max": 1_500, + }, + "run_postprocess_tool": { + "default": {}, + "worst": {}, + "default_max": 1_500, + "worst_max": 1_500, + }, + "get_minimal_context_tool": { + "default": {}, + "worst": {"task": "review the pull request", "changed_files": "ALL"}, + "default_max": 800, + "worst_max": 800, + }, + "get_impact_radius_tool": { + "default": {"changed_files": "LEAF"}, + "worst": {"changed_files": "ALL", "max_depth": 5}, + # Higher than it should be: changed_nodes and edges ignore + # max_results in query.py, so even a single-file default grows with + # the graph. Reported, not fixed here. + "default_max": 12_000, + "worst_max": None, # see QUERY_OWNED_UNBOUNDED + }, + "query_graph_tool": { + "default": {"pattern": "callers_of", "target": "helper_0_0_0"}, + "worst": { + "pattern": "file_summary", "target": "pkg0/mod0.py", + "max_results": HUGE, + }, + "default_max": 4_000, + # query.py caps this one via max_results; the ceiling is the caller's + # own value, so a whole-file summary is the realistic worst case. + "worst_max": 40_000, + }, + "get_review_context_tool": { + "default": {"changed_files": "LEAF"}, + "worst": { + "changed_files": "ALL", "include_source": True, + "max_lines_per_file": HUGE, "max_results": HUGE, + "max_files": HUGE, + }, + # Larger budgets by design: this is the "give me everything needed to + # review" tool, and it inlines source. Bounded by max_results + # (100 nodes / 150 edges), an 800-line shared source budget, and + # max_lines_per_file capped at 500. + "default_max": 20_000, + "worst_max": 50_000, + }, + "semantic_search_nodes_tool": { + "default": {"query": "helper"}, + "worst": {"query": "helper", "limit": HUGE}, + "default_max": 8_000, + "worst_max": None, # see QUERY_OWNED_UNBOUNDED + }, + "embed_graph_tool": { + # sentence-transformers is not a test dependency, so this exercises + # the structured "provider unavailable" error response. + "default": {}, + "worst": {}, + "default_max": 800, + "worst_max": 800, + }, + "list_graph_stats_tool": { + "default": {}, + "worst": {}, + "default_max": 1_500, + "worst_max": 1_500, + }, + "get_docs_section_tool": { + "default": {"section_name": "usage"}, + "worst": {"section_name": "commands"}, + "default_max": 4_000, + "worst_max": 4_000, + }, + "find_large_functions_tool": { + "default": {}, + "worst": {"min_lines": 1, "limit": HUGE}, + "default_max": 20_000, + "worst_max": None, # see QUERY_OWNED_UNBOUNDED + }, + "list_flows_tool": { + "default": {}, + "worst": {"limit": HUGE}, + "default_max": 12_000, + "worst_max": 40_000, + }, + "get_flow_tool": { + "default": {"flow_id": "FLOW_ID"}, + "worst": { + "flow_id": "FLOW_ID", "include_source": True, + "max_steps": HUGE, "max_source_lines": HUGE, + }, + "default_max": 8_000, + "worst_max": 40_000, + }, + "get_affected_flows_tool": { + "default": {"changed_files": "LEAF"}, + # max_flows=0 is PR #853's documented "no caller limit" escape; it is + # now still subject to the per-detail-level ceiling. + "worst": {"changed_files": "ALL", "max_flows": 0}, + "default_max": 6_000, + # Standard mode carries a full steps list per flow (~980 tokens each + # on a real repo), so the ceiling is 25 flows rather than PR #853's 50. + "worst_max": 40_000, + }, + "list_communities_tool": { + "default": {}, + "worst": {"max_results": HUGE, "max_members": HUGE}, + "default_max": 12_000, + "worst_max": 40_000, + }, + "get_community_tool": { + "default": {"community_id": "COMMUNITY_ID"}, + "worst": { + "community_id": "COMMUNITY_ID", "include_members": True, + "max_members": HUGE, + }, + "default_max": 3_000, + "worst_max": 20_000, + }, + "get_architecture_overview_tool": { + "default": {}, + "worst": { + "detail_level": "standard", "max_results": HUGE, + "max_members": HUGE, + }, + "default_max": 4_000, + # Standard mode is the explicit "full per-edge detail" mode: 200 + # cross-community rows plus 25 members per community. + "worst_max": 50_000, + }, + "detect_changes_tool": { + "default": {"changed_files": "LEAF"}, + "worst": { + "changed_files": "ALL", "include_source": True, "max_depth": 5, + "max_results": HUGE, "max_flows": HUGE, + }, + "default_max": 12_000, + "worst_max": 50_000, + }, + "refactor_tool:dead_code": { + "tool": "refactor_tool", + "default": {"mode": "dead_code"}, + "worst": {"mode": "dead_code", "max_results": HUGE}, + "default_max": 12_000, + "worst_max": 40_000, + }, + "refactor_tool:suggest": { + "tool": "refactor_tool", + "default": {"mode": "suggest"}, + "worst": {"mode": "suggest", "max_results": HUGE}, + "default_max": 12_000, + "worst_max": 40_000, + }, + "refactor_tool:rename": { + "tool": "refactor_tool", + "default": { + "mode": "rename", "old_name": "helper_0_0_0", + "new_name": "renamed_helper", + }, + "worst": { + "mode": "rename", "old_name": "helper_0_0_0", + "new_name": "renamed_helper", "max_results": HUGE, + }, + "default_max": 8_000, + "worst_max": 20_000, + }, + "apply_refactor_tool": { + "default": {"refactor_id": "REFACTOR_ID", "dry_run": True}, + "worst": { + "refactor_id": "REFACTOR_ID", "dry_run": True, + "max_diff_files": HUGE, + }, + # A dry-run diff is a bulk artifact by nature -- one unified diff per + # touched file. It is bounded by max_diff_files (25 by default), + # never by trimming the individual diffs, so a reviewer always sees + # a complete diff for each file shown. + "default_max": 25_000, + "worst_max": 40_000, + }, + "generate_wiki_tool": { + "default": {}, + "worst": {"force": True}, + "default_max": 1_000, + "worst_max": 1_000, + }, + "get_wiki_page_tool": { + "default": {"community_name": "COMMUNITY_NAME"}, + "worst": {"community_name": "COMMUNITY_NAME", "max_chars": HUGE}, + "default_max": 8_000, + "worst_max": 25_000, + }, + "get_hub_nodes_tool": { + "default": {}, + "worst": {"top_n": HUGE}, + "default_max": 4_000, + "worst_max": 25_000, + }, + "get_bridge_nodes_tool": { + "default": {}, + "worst": {"top_n": HUGE}, + "default_max": 4_000, + "worst_max": 25_000, + }, + "get_knowledge_gaps_tool": { + "default": {}, + "worst": {"max_per_category": HUGE}, + "default_max": 8_000, + "worst_max": 20_000, + }, + "get_surprising_connections_tool": { + "default": {}, + "worst": {"top_n": HUGE}, + "default_max": 4_000, + "worst_max": 30_000, + }, + "get_suggested_questions_tool": { + # Bounded by construction: generate_suggested_questions draws at most + # 3 bridges + 3 hubs + 3 surprises + 2 thin communities + 2 untested + # hotspots, so it takes no result cap. + "default": {}, + "worst": {}, + "default_max": 2_000, + "worst_max": 2_000, + }, + "traverse_graph_tool": { + "default": {"query": "helper_0_0_0"}, + "worst": {"query": "helper_0_0_0", "depth": 6, "token_budget": HUGE}, + "default_max": 8_000, + "worst_max": None, # see QUERY_OWNED_UNBOUNDED + }, + "list_repos_tool": { + "no_repo_root": True, + "default": {}, + "worst": {}, + "default_max": 2_000, + "worst_max": 2_000, + }, + "cross_repo_search_tool": { + "no_repo_root": True, + "default": {"query": "helper"}, + "worst": {"query": "helper", "limit": HUGE, "max_results": HUGE}, + "default_max": 4_000, + "worst_max": 30_000, + }, +} + + +def _pick_row(repo: dict[str, Any], sql: str, column: int) -> Any: + """Read one id straight from the graph, at call time. + + ``run_postprocess_tool`` is itself under test and rebuilds the flows and + communities tables with fresh ids, so ids captured once in the fixture + go stale mid-module. + """ + with GraphStore(Path(repo["root"]) / ".code-review-graph" / "graph.db") as store: + rows = store._conn.execute(sql).fetchall() + return rows[0][column] if rows else None + + +_FLOW_SQL = "SELECT id FROM flows ORDER BY node_count DESC LIMIT 1" +_COMMUNITY_SQL = "SELECT id, name FROM communities ORDER BY size DESC LIMIT 1" + + +def _resolve_kwargs(kwargs: dict[str, Any], repo: dict[str, Any]) -> dict[str, Any]: + """Substitute the fixture-dependent placeholders in a budget entry.""" + resolved: dict[str, Any] = {} + for key, value in kwargs.items(): + if value == "ALL": + resolved[key] = repo["files"] + elif value == "LEAF": + # A module nothing else imports, so "default" arguments measure a + # typical change rather than a whole-repo blast radius. + resolved[key] = [repo["leaf_file"]] + elif value == "FLOW_ID": + resolved[key] = _pick_row(repo, _FLOW_SQL, 0) + elif value == "COMMUNITY_ID": + resolved[key] = _pick_row(repo, _COMMUNITY_SQL, 0) + elif value == "COMMUNITY_NAME": + resolved[key] = _pick_row(repo, _COMMUNITY_SQL, 1) + elif value == "REFACTOR_ID": + resolved[key] = repo["refactor_id"] + else: + resolved[key] = value + return resolved + + +def _call(name: str, spec: dict[str, Any], kwargs: dict[str, Any], + repo: dict[str, Any]) -> Any: + """Invoke one registered tool with fixture-resolved arguments.""" + tool = getattr(crg_main, spec.get("tool", name)) + func = getattr(tool, "fn", tool) + call_kwargs = _resolve_kwargs(kwargs, repo) + if not spec.get("no_repo_root"): + call_kwargs["repo_root"] = repo["root"] + if inspect.iscoroutinefunction(func): + return asyncio.run(func(**call_kwargs)) + return func(**call_kwargs) + + +@pytest.fixture(scope="module") +def repo(graph_repo) -> dict[str, Any]: + """Fixture graph plus a live refactor_id for apply_refactor_tool.""" + preview = crg_main.refactor_tool( + mode="rename", old_name="helper_0_0_0", new_name="renamed_helper", + repo_root=graph_repo["root"], + ) + return {**graph_repo, "refactor_id": preview.get("refactor_id", "missing")} + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +def test_budget_table_covers_every_registered_tool(): + """Every ``@mcp.tool()`` in main.py must have a budget entry. + + Without this, a new tool could ship unbounded and no case would notice. + """ + registered = { + name for name in dir(crg_main) + if name.endswith("_tool") and callable(getattr(crg_main, name)) + and not name.startswith("_") + } + covered = {spec.get("tool", name) for name, spec in BUDGETS.items()} + assert registered - covered == set(), ( + "MCP tools missing a token budget entry: " + f"{sorted(registered - covered)}" + ) + + +@pytest.mark.parametrize("name", sorted(BUDGETS)) +def test_default_args_stay_in_budget(name, repo): + """A tool called the way an agent calls it must be cheap.""" + spec = BUDGETS[name] + result = _call(name, spec, spec["default"], repo) + tokens = count_tokens(result) + assert tokens <= spec["default_max"], ( + f"{name} returned {tokens} tokens with default arguments, over its " + f"{spec['default_max']} budget. A result cap was probably removed or " + f"an unbounded field added." + ) + + +@pytest.mark.parametrize("name", sorted(BUDGETS)) +def test_worst_case_args_stay_in_budget(name, repo): + """Maxing every verbosity knob must still hit a hard ceiling.""" + spec = BUDGETS[name] + if spec["worst_max"] is None: + pytest.skip( + f"{name} is bounded in code_review_graph/tools/query.py, which " + "this change does not own; its unbounded worst case is reported " + "rather than fixed (see QUERY_OWNED_UNBOUNDED)." + ) + result = _call(name, spec, spec["worst"], repo) + tokens = count_tokens(result) + assert tokens <= spec["worst_max"], ( + f"{name} returned {tokens} tokens with maxed arguments, over its " + f"{spec['worst_max']} ceiling. The hard cap was probably raised or " + f"removed." + ) + assert tokens <= ABSOLUTE_MAX_TOKENS, ( + f"{name} returned {tokens} tokens, over the {ABSOLUTE_MAX_TOKENS} " + "absolute ceiling that no MCP tool response may cross." + ) + + +def test_caps_actually_bind_on_the_fixture_graph(repo): + """Guard against a budget that passes only because the fixture is small. + + Each tool below must report ``truncated`` on this fixture at its own + default arguments. If a cap is removed, the flag disappears and this + fails even where the raw token count would still squeak under budget. + """ + root = repo["root"] + all_files = repo["files"] + cases = { + "list_communities": crg_main.list_communities_tool( + repo_root=root, max_results=1, + ), + "list_flows": crg_main.list_flows_tool(repo_root=root, limit=1), + "get_affected_flows": crg_main.get_affected_flows_tool( + repo_root=root, changed_files=all_files, max_flows=1, + ), + "refactor_dead_code": crg_main.refactor_tool( + repo_root=root, mode="dead_code", max_results=1, + ), + "get_hub_nodes": crg_main.get_hub_nodes_tool(repo_root=root, top_n=1), + "get_bridge_nodes": crg_main.get_bridge_nodes_tool( + repo_root=root, top_n=1, + ), + "get_surprising_connections": crg_main.get_surprising_connections_tool( + repo_root=root, top_n=1, + ), + "get_architecture_overview": crg_main.get_architecture_overview_tool( + repo_root=root, max_results=1, + ), + } + for label, result in cases.items(): + assert result.get("truncated") is True, ( + f"{label} did not report truncation at a cap of 1 -- its result " + "cap is no longer applied" + ) + + +# The ceiling constants are themselves part of the contract. Asserting them +# directly is what catches "someone raised the cap" -- a token budget alone +# can be masked by its own headroom, and a length assertion silently passes +# once the ceiling exceeds what the fixture can produce. +MAX_CEILINGS = { + "community_tools._MAX_MEMBERS": (community_tools._MAX_MEMBERS, 25), + "community_tools._MAX_COMMUNITIES": (community_tools._MAX_COMMUNITIES, 200), + "community_tools._MAX_CROSS_EDGES": (community_tools._MAX_CROSS_EDGES, 200), + "analysis_tools._MAX_HUB_NODES": (analysis_tools._MAX_HUB_NODES, 100), + "analysis_tools._MAX_BRIDGE_NODES": (analysis_tools._MAX_BRIDGE_NODES, 100), + "analysis_tools._MAX_SURPRISING": (analysis_tools._MAX_SURPRISING, 100), + "analysis_tools._MAX_GAPS_PER_CATEGORY": ( + analysis_tools._MAX_GAPS_PER_CATEGORY, 50, + ), + "review._MAX_REVIEW_NODES": (review._MAX_REVIEW_NODES, 100), + "review._MAX_REVIEW_EDGES": (review._MAX_REVIEW_EDGES, 150), + "review._MAX_REVIEW_SOURCE_LINES": (review._MAX_REVIEW_SOURCE_LINES, 800), + "review._MAX_LINES_PER_FILE": (review._MAX_LINES_PER_FILE, 500), + "review._MAX_CHANGED_FUNCTIONS": (review._MAX_CHANGED_FUNCTIONS, 100), + "review._MAX_AFFECTED_FLOWS_STANDARD": ( + review._MAX_AFFECTED_FLOWS_STANDARD, 25, + ), + "review._MAX_AFFECTED_FLOWS_MINIMAL": ( + review._MAX_AFFECTED_FLOWS_MINIMAL, 500, + ), + "review._MAX_AFFECTED_FLOW_STEPS": (review._MAX_AFFECTED_FLOW_STEPS, 400), + "refactor_tools._MAX_REFACTOR_RESULTS": ( + refactor_mod._MAX_REFACTOR_RESULTS, 150, + ), +} + + +@pytest.mark.parametrize("name", sorted(MAX_CEILINGS)) +def test_ceiling_constants_are_not_raised(name): + """Raising a hard ceiling is a budget change and must be deliberate. + + If a ceiling genuinely needs to grow, update the number here in the same + commit and say why -- that is the review conversation this test forces. + """ + actual, allowed = MAX_CEILINGS[name] + assert actual <= allowed, ( + f"{name} was raised to {actual}, over the {allowed} this budget " + "table was measured against" + ) + + +def test_hard_ceilings_bind(repo): + """A caller asking for everything gets the ceiling, not everything. + + Complements the constant check above: this proves the ceilings are + actually applied to the response, not merely declared. + """ + root = repo["root"] + all_files = repo["files"] + + communities = crg_main.list_communities_tool( + repo_root=root, max_members=HUGE, + )["communities"] + oversized = [c for c in communities if c["size"] > 25] + assert oversized, "fixture no longer has a community big enough to cap" + for community in oversized: + assert len(community["members"]) == community_tools._MAX_MEMBERS + assert community["members_truncated"] is True + + hubs = crg_main.get_hub_nodes_tool(repo_root=root, top_n=HUGE) + assert hubs["total"] > analysis_tools._MAX_HUB_NODES + assert len(hubs["hub_nodes"]) == analysis_tools._MAX_HUB_NODES + + surprises = crg_main.get_surprising_connections_tool( + repo_root=root, top_n=HUGE, + ) + assert len(surprises["surprising_connections"]) == ( + min(surprises["total"], analysis_tools._MAX_SURPRISING) + ) + + flows = crg_main.get_affected_flows_tool( + repo_root=root, changed_files=all_files, max_flows=HUGE, + ) + assert flows["total"] > review._MAX_AFFECTED_FLOWS_STANDARD + assert len(flows["affected_flows"]) == review._MAX_AFFECTED_FLOWS_STANDARD + # The shared step budget must also hold, whatever the flow depth. + emitted = sum(len(f.get("steps") or []) for f in flows["affected_flows"]) + assert emitted <= review._MAX_AFFECTED_FLOW_STEPS + + changes = asyncio.run(crg_main.detect_changes_tool( + repo_root=root, changed_files=all_files, max_results=HUGE, + )) + assert changes["changed_functions_total"] > review._MAX_CHANGED_FUNCTIONS + assert len(changes["changed_functions"]) == review._MAX_CHANGED_FUNCTIONS + + context = crg_main.get_review_context_tool( + repo_root=root, changed_files=all_files, max_results=HUGE, + max_files=HUGE, include_source=True, max_lines_per_file=HUGE, + )["context"] + assert len(context["graph"]["impacted_nodes"]) <= review._MAX_REVIEW_NODES + assert len(context["graph"]["edges"]) <= review._MAX_REVIEW_EDGES + emitted_lines = sum( + len(snippet.splitlines()) + for snippet in context["source_snippets"].values() + ) + # Each snippet can overshoot by the "..." separators it inserts, so allow + # a small margin over the raw line budget. + assert emitted_lines <= review._MAX_REVIEW_SOURCE_LINES * 1.5 + + dead = crg_main.refactor_tool( + repo_root=root, mode="dead_code", max_results=HUGE, + ) + assert len(dead["dead_code"]) == min( + dead["total"], refactor_mod._MAX_REFACTOR_RESULTS, + ) + + +class TestTruncationContract: + """The contract PR #853 established, applied to the newly capped tools.""" + + def test_list_communities_reports_untruncated_total(self, repo): + result = crg_main.list_communities_tool( + repo_root=repo["root"], max_results=1, + ) + assert result["status"] == "ok" + assert result["truncated"] is True + assert result["total"] > len(result["communities"]) + assert f"showing {len(result['communities'])} of" in result["summary"] + + def test_get_community_keeps_true_size_when_members_cut(self, repo): + result = crg_main.get_community_tool( + repo_root=repo["root"], + community_id=_pick_row(repo, _COMMUNITY_SQL, 0), + include_members=True, max_members=1, + ) + community = result["community"] + assert community["members_truncated"] is True + # ``size`` is the real member count, not the truncated list length. + assert community["size"] > len(community["members"]) + + def test_detect_changes_flows_carry_no_step_lists(self, repo): + """#849's payload must not leak back in through detect_changes.""" + result = asyncio.run(crg_main.detect_changes_tool( + repo_root=repo["root"], changed_files=repo["files"], + )) + for flow in result["affected_flows"]: + assert "steps" not in flow, ( + "detect_changes embeds per-flow metadata only; full step " + "lists are what made get_affected_flows return 247k tokens" + ) + + def test_affected_flows_zero_still_hits_the_ceiling(self, repo): + """``max_flows=0`` means 'no caller limit', not 'no limit'.""" + result = crg_main.get_affected_flows_tool( + repo_root=repo["root"], changed_files=repo["files"], max_flows=0, + ) + assert len(result["affected_flows"]) <= 25 + assert result["total"] >= len(result["affected_flows"]) + + def test_rename_preview_response_is_cut_but_apply_is_not(self, repo): + """Truncating the response must not truncate the stored refactor.""" + preview = crg_main.refactor_tool( + repo_root=repo["root"], mode="rename", old_name="helper_0_0_0", + new_name="renamed_helper", max_results=1, + ) + assert preview["truncated"] is True + assert len(preview["edits"]) == 1 + assert preview["total"] > 1 + # The stored preview still holds every edit, so a dry run reports the + # full set of files rather than the one shown edit. + applied = crg_main.apply_refactor_tool( + repo_root=repo["root"], refactor_id=preview["refactor_id"], + dry_run=True, + ) + assert applied["status"] == "ok" + assert applied["edits_applied"] >= preview["total"] + + +class TestBoundValidation: + """Result bounds are validated the way query.py validates max_results.""" + + @pytest.mark.parametrize( + ("tool", "kwargs"), + [ + ("get_hub_nodes_tool", {"top_n": 0}), + ("get_hub_nodes_tool", {"top_n": True}), + ("get_bridge_nodes_tool", {"top_n": -1}), + ("get_surprising_connections_tool", {"top_n": 0}), + ("get_knowledge_gaps_tool", {"max_per_category": 0}), + ("list_communities_tool", {"max_results": 0}), + ("list_communities_tool", {"max_members": True}), + ("get_community_tool", {"max_members": 0}), + ("get_architecture_overview_tool", {"max_results": 0}), + ("list_flows_tool", {"limit": 0}), + ("get_flow_tool", {"max_steps": 0}), + ("get_flow_tool", {"max_source_lines": -5}), + ("get_review_context_tool", {"max_results": 0}), + ("get_review_context_tool", {"max_files": True}), + ("get_wiki_page_tool", {"community_name": "x", "max_chars": 0}), + ("apply_refactor_tool", {"refactor_id": "x", "max_diff_files": 0}), + ], + ) + def test_rejects_non_positive_bounds(self, tool, kwargs, repo): + func = getattr(crg_main, tool) + with pytest.raises(ValueError, match="greater than or equal to 1"): + func(repo_root=repo["root"], **kwargs) + + def test_refactor_rejects_non_positive_max_results(self, repo): + with pytest.raises(ValueError, match="greater than or equal to 1"): + crg_main.refactor_tool( + repo_root=repo["root"], mode="dead_code", max_results=0, + ) + + def test_detect_changes_rejects_non_positive_bounds(self, repo): + with pytest.raises(ValueError, match="greater than or equal to 1"): + asyncio.run(crg_main.detect_changes_tool( + repo_root=repo["root"], max_results=0, + )) + + def test_cross_repo_search_rejects_non_positive_bounds(self): + with pytest.raises(ValueError, match="greater than or equal to 1"): + crg_main.cross_repo_search_tool(query="x", max_results=0)