Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion code_review_graph/tools/_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,21 @@ def _get_store(repo_root: str | None = None) -> tuple[GraphStore, Path]:
def _resolve_graph_file_paths(
store: GraphStore, root: Path, file_paths: list[str],
) -> list[str]:
"""Compatibility wrapper returning only matched graph paths."""
return _resolve_graph_file_paths_with_unmatched(store, root, file_paths)[0]


def _resolve_graph_file_paths_with_unmatched(
store: GraphStore, root: Path, file_paths: list[str],
) -> tuple[list[str], list[str]]:
"""Resolve user-facing file paths to the paths stored in the graph.

Graphs may contain absolute paths, repo-relative paths, or cwd-relative
paths depending on how they were built. Tool inputs are usually relative to
repo root, so exact matching alone can miss existing graph nodes.
"""
resolved: list[str] = []
unmatched: list[str] = []
seen: set[str] = set()

def add(path: str) -> None:
Expand All @@ -247,6 +255,7 @@ def add(path: str) -> None:
seen.add(path)

for file_path in file_paths:
matched = False
raw = file_path.replace("\\", "/")
candidates = [raw]
path = Path(file_path)
Expand All @@ -260,6 +269,7 @@ def add(path: str) -> None:

for candidate in candidates:
if store.get_nodes_by_file(candidate):
matched = True
add(candidate)

suffixes = []
Expand All @@ -270,9 +280,22 @@ def add(path: str) -> None:

for suffix in suffixes:
for matched_path in store.get_files_matching(suffix):
matched = True
add(matched_path)
if not matched and file_path not in unmatched:
unmatched.append(file_path)

return resolved
return resolved, unmatched


def _unmatched_file_warning(unmatched: list[str]) -> str:
"""Explain incomplete path coverage without growing an unbounded summary."""
if not unmatched:
return ""
paths = ", ".join(unmatched[:5])
if len(unmatched) > 5:
paths += f" (+{len(unmatched) - 5} more)"
return f" - {len(unmatched)} requested file(s) are not indexed: {paths}"


# ---------------------------------------------------------------------------
Expand Down
23 changes: 15 additions & 8 deletions code_review_graph/tools/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
_error_response,
_get_store,
_resolve_graph_file_paths,
_resolve_graph_file_paths_with_unmatched,
_unmatched_file_warning,
)

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -256,7 +258,9 @@ def get_impact_radius(

# Resolve user-facing paths to the file paths stored in the graph.
original_tokens = estimate_file_tokens(root, changed_files)
abs_files = _resolve_graph_file_paths(store, root, changed_files)
abs_files, unmatched_files = _resolve_graph_file_paths_with_unmatched(
store, root, changed_files,
)
result = store.get_impact_radius(
abs_files, max_depth=max_depth, max_nodes=max_results,
resolution=resolution,
Expand Down Expand Up @@ -334,13 +338,20 @@ def get_impact_radius(
or files_omitted > 0
)
total_impacted = result["total_impacted"]
risk = (
"unknown" if not abs_files else
"high" if total_impacted > 20 else
"medium" if total_impacted > 5 else "low"
)

summary_parts = [
f"Blast radius for {len(changed_files)} changed file(s):",
f" - {len(result['changed_nodes'])} nodes directly changed",
f" - {total_impacted} nodes impacted (within {max_depth} hops)",
f" - {files_total} additional files affected",
]
if unmatched_files:
summary_parts.append(_unmatched_file_warning(unmatched_files))
if len(impacted_dicts) < total_impacted:
summary_parts.append(
f" - Results truncated: showing {len(impacted_dicts)}"
Expand All @@ -367,20 +378,14 @@ def get_impact_radius(
if detail_level == "minimal":
# The full count, not the displayed one: the risk band must not
# change because a display cap trimmed the list.
impacted_count = total_impacted
if impacted_count > 20:
risk = "high"
elif impacted_count > 5:
risk = "medium"
else:
risk = "low"
key_entities = [
n["name"] for n in impacted_dicts[:5]
]
minimal_response = {
"status": "ok",
"summary": "\n".join(summary_parts),
"risk": risk,
"unmatched_files": unmatched_files,
"impacted_file_count": len(result["impacted_files"]),
"key_entities": key_entities,
"truncated": truncated,
Expand All @@ -396,6 +401,8 @@ def get_impact_radius(

response: dict[str, Any] = {
"status": "ok",
"risk": risk,
"unmatched_files": unmatched_files,
"summary": "\n".join(summary_parts),
"changed_files": changed_files,
"changed_nodes": changed_dicts,
Expand Down
27 changes: 18 additions & 9 deletions code_review_graph/tools/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@
_bounded,
_error_response,
_get_store,
_resolve_graph_file_paths,
_resolve_graph_file_paths_with_unmatched,
_shown_of,
_unmatched_file_warning,
_validate_positive_int,
)

Expand Down Expand Up @@ -490,18 +491,19 @@ def get_review_context(
"context": {},
}

graph_files = _resolve_graph_file_paths(store, root, changed_files)
graph_files, unmatched_files = _resolve_graph_file_paths_with_unmatched(
store, root, changed_files,
)
original_tokens = estimate_file_tokens(root, changed_files)
impact = store.get_impact_radius(graph_files, max_depth=max_depth)
impacted_count = impact.get("total_impacted", len(impact["impacted_nodes"]))
risk = (
"unknown" if not graph_files else
"high" if impacted_count > 20 else
"medium" if impacted_count > 5 else "low"
)

if detail_level == "minimal":
impacted_count = len(impact["impacted_nodes"])
if impacted_count > 20:
risk = "high"
elif impacted_count > 5:
risk = "medium"
else:
risk = "low"

key_entities = [
n.name for n in impact["changed_nodes"][:5]
Expand Down Expand Up @@ -535,11 +537,14 @@ def get_review_context(
f" - {len(impact['impacted_nodes'])} impacted nodes"
f" in {len(impact['impacted_files'])} files",
]
if unmatched_files:
summary_parts.append(_unmatched_file_warning(unmatched_files))

result = {
"status": "ok",
"summary": "\n".join(summary_parts),
"risk": risk,
"unmatched_files": unmatched_files,
"changed_file_count": len(changed_files),
"impacted_file_count": len(impact["impacted_files"]),
"key_entities": key_entities,
Expand Down Expand Up @@ -706,9 +711,13 @@ def get_review_context(
"Review guidance:",
guidance,
]
if unmatched_files:
summary_parts.append(_unmatched_file_warning(unmatched_files))

result = {
"status": "ok",
"risk": risk,
"unmatched_files": unmatched_files,
"summary": "\n".join(summary_parts),
"context": context,
}
Expand Down
5 changes: 5 additions & 0 deletions docs/USAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ Then use `cross_repo_search_tool` to search every registered repository, or pass

## Context Savings

Impact and review-context tools return `risk: "unknown"` when none of the requested file
paths match the graph. Their `unmatched_files` field and summary identify missing paths.
For a mix of indexed and missing files, risk describes the indexed subset; check the
missing paths or rebuild the graph before treating that result as complete.

Review and impact responses include compact `context_savings` metadata (`estimated`, `saved_tokens`, `saved_percent`). The CLI shows the same figures as a boxed `Token Savings` panel on `detect-changes --brief` and `update --brief`, with a breakdown (Functions / Tests / Risk / Other) that sums to the graph response size. Add `--verify` to compare against OpenAI's `cl100k_base` tokenizer (needs `pip install tiktoken`). The figures are labelled estimated because they use a `chars / 4` approximation; the calibration in [REPRODUCING.md](REPRODUCING.md#calibration-table) puts the aggregate estimate within about 1% of real tokens. A small single-file change can use more context than the raw file, because the graph metadata has a fixed overhead.

The evaluation runner produces the benchmark numbers quoted in the README:
Expand Down
51 changes: 51 additions & 0 deletions tests/test_unmatched_risk.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
"""Unknown graph paths must not receive a low-risk all-clear (#983)."""

import pytest

from code_review_graph.graph import GraphStore
from code_review_graph.parser import EdgeInfo, NodeInfo
from code_review_graph.tools import query, review
from code_review_graph.tools._common import _resolve_graph_file_paths


@pytest.fixture
def graph(tmp_path, monkeypatch):
store = GraphStore(tmp_path / 'graph.db')
path = (tmp_path / 'src' / 'app.py').as_posix()
store.upsert_node(NodeInfo('File', path, path, 1, 3, 'python'))
store.upsert_node(NodeInfo('Function', 'handle', path, 1, 3, 'python'))
for index in range(30):
caller = (tmp_path / f'caller{index}.py').as_posix()
store.upsert_node(NodeInfo('Function', f'caller{index}', caller, 1, 2, 'python'))
store.upsert_edge(EdgeInfo('CALLS', f'{caller}::caller{index}',
f'{path}::handle', caller, 2))
store.commit()
monkeypatch.setattr(query, '_get_store', lambda _: (store, tmp_path))
monkeypatch.setattr(review, '_get_store', lambda _: (store, tmp_path))
monkeypatch.setattr(review, 'resolve_review_base', lambda root, base: base)
yield store, tmp_path
store.close()


@pytest.mark.parametrize('tool', [query.get_impact_radius, review.get_review_context])
@pytest.mark.parametrize('detail', ['minimal', 'standard'])
@pytest.mark.parametrize('path', ['src/ap.py', 'src/ghost.py', 'src/app.py::handle'])
def test_unmatched_path_is_unknown(graph, tool, detail, path):
result = tool(changed_files=[path], detail_level=detail)
assert result['risk'] == 'unknown'
assert result['unmatched_files'] == [path]
assert 'not indexed' in result['summary'].lower()


@pytest.mark.parametrize('tool', [query.get_impact_radius, review.get_review_context])
@pytest.mark.parametrize('detail', ['minimal', 'standard'])
def test_partial_match_keeps_measured_risk_and_names_miss(graph, tool, detail):
result = tool(changed_files=['src/app.py', 'missing.py'], detail_level=detail)
assert result['risk'] == 'high'
assert result['unmatched_files'] == ['missing.py']


def test_compatibility_wrapper_resolves_duplicate_relative_and_absolute_paths(graph):
store, root = graph
absolute = (root / 'src' / 'app.py').as_posix()
assert _resolve_graph_file_paths(store, root, ['src/app.py', absolute]) == [absolute]
Loading