diff --git a/CHANGELOG.md b/CHANGELOG.md index 04e81a9a1..3e3bfac7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,33 @@ ## [Unreleased] +### Changed + +- The MCP server instructions now route by question type instead of introducing + the server as a code-review tool. With deferred tool loading an agent sees + only the tool names and these instructions, so "for code reviews" kept it on + Grep for callers, importers and covering tests everywhere else. The new text + maps each relationship question to its tool and leaves plain-text search to + Grep. Measured with Claude Code on one TypeScript monorepo: across 8 ordinary + tasks that never mention the graph (implement, review, brainstorm, plan), the + graph was used in 1 run before and 6 after; plain-text questions stayed on + Grep (0 of 4 moved). `tests/test_server_instructions.py` keeps every tool and + query pattern the text names registered. + +### Fixed + +- An explicit embedding refresh (`--embedding-provider` with + `--embedding-model`, or the `embedding_provider` and `embedding_model` + arguments of `build_or_update_graph_tool` and `run_postprocess_tool`) on a + graph with no embeddings yet now reports that it embedded nothing. `build`, + `update` and `postprocess` return `embeddings_refresh_skipped: true` and a + warning to run `code-review-graph embed` once, and so does an `update` + that finds no changes. `watch` logs the same warning after each update and + keeps running; a refresh that fails against an existing index still stops + it, as before. The refresh used to return `ok` with no embedding field, + the same result as a build that never asked for one. It still loads no + model and makes no network call in that case (#1070). + ## [2.3.9] - 2026-09-18 ### Added diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d8f8021a2..7ac41fbbe 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,6 +19,11 @@ uv run pytest tests/test_parser.py -v CI runs the suite on Python 3.10, 3.11, 3.12 and 3.13 and fails below 65% coverage. +`tests/test_parser_grammar_contract.py` checks every built-in class, function, import, +and call dispatch name against its bundled Tree-sitter grammar. When updating a grammar +or adding dispatch entries, use the grammar's actual named node types. This contract +checks configured dispatch names; language fixtures still verify extraction behavior. + ## Linting and Type Checking ```bash diff --git a/code_review_graph/changes.py b/code_review_graph/changes.py index 9e0d5ce24..17c562992 100644 --- a/code_review_graph/changes.py +++ b/code_review_graph/changes.py @@ -787,10 +787,13 @@ def analyze_changes( # still gives the right answer: those rows carry ``is_test = 0`` and used # to be reported back to the author as their own tests needing tests # (issue #1014). + # TypeScript interfaces and type aliases have no runtime behavior to test, + # so they are never gap candidates (issue #1109). gap_candidates = [ node for node in changed_funcs if not (node.is_test or is_test_file(node.file_path, gap_root)) and node.name not in _TEST_GAP_EXEMPT_NAMES + and not node.extra.get("typescript_kind") ] # TESTED_BY edges are stored as source=production, target=test by the # parser, so a changed production function finds its tests by source. diff --git a/code_review_graph/embeddings.py b/code_review_graph/embeddings.py index 9e60645e6..c8d203c32 100644 --- a/code_review_graph/embeddings.py +++ b/code_review_graph/embeddings.py @@ -1315,6 +1315,33 @@ def embed_all_nodes(graph_store: GraphStore, embedding_store: EmbeddingStore) -> return embedding_store.embed_nodes(all_nodes) +# Reported by build and post-process paths when an explicit refresh finds no +# vectors, so a caller who asked for a refresh learns that nothing was embedded. +REFRESH_SKIPPED_WARNING = ( + "Embedding refresh skipped: this graph has no embeddings yet. Run " + "`code-review-graph embed` (MCP: embed_graph_tool) once with the same " + "provider and model to create them." +) + + +def has_embeddings(graph_store: GraphStore) -> bool: + """True when the graph database holds at least one stored vector. + + Reads the schema first, so the check never creates the ``embeddings`` + table and never resolves a provider. + """ + has_table = graph_store._conn.execute( + "SELECT 1 FROM sqlite_master " + "WHERE type = 'table' AND name = 'embeddings'", + ).fetchone() + if has_table is None: + return False + has_rows = graph_store._conn.execute( + "SELECT 1 FROM embeddings LIMIT 1", + ).fetchone() + return has_rows is not None + + def refresh_embeddings( graph_store: GraphStore, *, @@ -1325,8 +1352,9 @@ def refresh_embeddings( This function is deliberately not called by default build paths. Callers must supply both provider and model explicitly. A graph with no existing - vectors returns before provider resolution, so routine builds cannot load - a local model, contact a cloud service, or incur API cost. + vectors returns ``None`` before provider resolution, so routine builds + cannot load a local model, contact a cloud service, or incur API cost; + the build paths report that case with ``REFRESH_SKIPPED_WARNING``. Existing vectors must all use the identity resolved from the requested provider/model (including the endpoint for OpenAI-compatible providers). @@ -1339,16 +1367,7 @@ def refresh_embeddings( "Embedding refresh requires an explicit provider and model.", ) - has_table = graph_store._conn.execute( - "SELECT 1 FROM sqlite_master " - "WHERE type = 'table' AND name = 'embeddings'", - ).fetchone() - if has_table is None: - return None - has_rows = graph_store._conn.execute( - "SELECT 1 FROM embeddings LIMIT 1", - ).fetchone() - if has_rows is None: + if not has_embeddings(graph_store): return None try: rows = graph_store._conn.execute( diff --git a/code_review_graph/incremental.py b/code_review_graph/incremental.py index 7ad0c0315..8388da249 100644 --- a/code_review_graph/incremental.py +++ b/code_review_graph/incremental.py @@ -2185,10 +2185,22 @@ def _raise_watch_update_errors(result: dict, context: str) -> None: def _raise_watch_postprocess_warnings(result: object) -> None: - """Treat structured post-processing warnings as a failed watch update.""" + """Treat structured post-processing warnings as a failed watch update. + + One warning is exempt: an explicit embedding refresh that found no index + to refresh. The graph update itself succeeded and the post-processing step + has already logged the notice, so the watch keeps running. A refresh that + fails against an existing index still fails the update. + """ if not isinstance(result, dict): return - warnings = result.get("warnings") or [] + from .embeddings import REFRESH_SKIPPED_WARNING + + warnings = [ + warning + for warning in result.get("warnings") or [] + if warning != REFRESH_SKIPPED_WARNING + ] if warnings: details = "; ".join(str(warning) for warning in warnings) raise RuntimeError(f"post-processing reported warnings: {details}") diff --git a/code_review_graph/jsonc.py b/code_review_graph/jsonc.py index ca97e1e81..f94c8c4a6 100644 --- a/code_review_graph/jsonc.py +++ b/code_review_graph/jsonc.py @@ -301,6 +301,28 @@ def remove_paths(raw: str, paths: Iterable[Sequence[str | int]]) -> str: merged.append((start, end)) for start, end in reversed(merged): raw = raw[:start] + raw[end:] + # Adjacent removals use one snapshot: removing the last two siblings can + # leave the separator after the surviving sibling. Drop only newly exposed + # trailing commas; keep pre-existing JSONC trailing commas and all comments. + original_trailing = { + token.start + for index, token in enumerate(tokens[:-1]) + if token.kind == "," and tokens[index + 1].kind in ("}", "]") + } + surviving = [ + token for token in tokens + if not any(start <= token.start < end for start, end in merged) + ] + exposed = [ + token.start + for index, token in enumerate(surviving[:-1]) + if token.kind == "," and surviving[index + 1].kind in ("}", "]") + and token.start not in original_trailing + ] + # Offsets above refer to the original document, before the removed spans. + for offset in reversed(exposed): + rewritten_offset = offset - sum(end - start for start, end in merged if end <= offset) + raw = raw[:rewritten_offset] + raw[rewritten_offset + 1:] return raw diff --git a/code_review_graph/main.py b/code_review_graph/main.py index be58e8d91..f5fefbf35 100644 --- a/code_review_graph/main.py +++ b/code_review_graph/main.py @@ -210,14 +210,38 @@ def _run() -> dict: ) +# What an agent reads before it loads any of these tools: with deferred tool +# loading (Claude Code's default once many tools are installed) it sees the +# tool names and this text, nothing else, while Grep's description is always +# in context. So this text decides when the graph gets picked over text +# search. It routes by question type rather than introducing the server as a +# code-review tool, which kept agents on Grep for callers, importers and +# covering tests everywhere outside a review. Claude Code truncates server +# instructions at 2,048 characters. +SERVER_INSTRUCTIONS = ( + "Code knowledge graph of this repo (Tree-sitter AST + call / import / test edges). " + "Use it whenever the question is about RELATIONSHIPS between pieces of code, in any " + "phase of work - coding, debugging, planning, brainstorming, refactoring, review - " + "not only code review:\n" + "- who calls / references symbol X -> query_graph_tool pattern=callers_of / " + "references_to\n" + "- which files import module or file X -> query_graph_tool pattern=importers_of\n" + "- which tests cover X -> query_graph_tool pattern=tests_for\n" + "- what breaks if X changes (blast radius) -> get_impact_radius_tool\n" + "- what a diff or branch touches -> detect_changes_tool, get_review_context_tool\n" + "- how modules depend on each other -> get_architecture_overview_tool, " + "list_communities_tool\n" + "- follow an execution flow -> list_flows_tool, get_flow_tool\n" + "Graph answers include dependents reached through re-exports and barrel files that " + "never spell the name, which text search silently misses.\n" + "For plain text - string literals, log messages, env var names, comments, config " + "keys - use Grep; the graph does not index text." +) + mcp = FastMCP( "code-review-graph", version=__version__, - instructions=( - "Persistent incremental knowledge graph for token-efficient, " - "context-aware code reviews. Parses your codebase with Tree-sitter, " - "builds a structural graph, and provides smart impact analysis." - ), + instructions=SERVER_INSTRUCTIONS, ) @@ -257,6 +281,8 @@ async def build_or_update_graph_tool( When None (default), falls back to CRG_RECURSE_SUBMODULES env var. embedding_provider: Exact provider for an explicit post-build embedding refresh. Must be supplied with embedding_model. Default: disabled. + Refreshes an existing index only: on a graph with no embeddings + the result sets embeddings_refresh_skipped; run embed_graph_tool first. embedding_model: Exact model for an explicit post-build embedding refresh. Must be supplied with embedding_provider. Default: disabled. """ @@ -300,6 +326,8 @@ async def run_postprocess_tool( repo_root: Repository root path. Auto-detected if omitted. embedding_provider: Exact provider for an explicit embedding refresh. Must be supplied with embedding_model. Default: disabled. + Refreshes an existing index only: on a graph with no embeddings + the result sets embeddings_refresh_skipped; run embed_graph_tool first. embedding_model: Exact model for an explicit embedding refresh. Must be supplied with embedding_provider. Default: disabled. """ diff --git a/code_review_graph/parser.py b/code_review_graph/parser.py index 1a390c763..cb8c4f645 100644 --- a/code_review_graph/parser.py +++ b/code_review_graph/parser.py @@ -430,13 +430,44 @@ def _is_statically_false_condition(cond) -> bool: return False +def _is_static_dead_guard_child(parent, child) -> bool: + """Return whether *child* enters a statically-dead branch of *parent*.""" + if parent.type == "if_statement": + condition = parent.child_by_field_name("condition") + consequence = parent.child_by_field_name("consequence") + return ( + condition is not None + and consequence is not None + and _is_statically_false_condition(condition) + and child.start_byte == consequence.start_byte + and child.end_byte == consequence.end_byte + ) + + if parent.type in ("preproc_if", "preproc_elif"): + condition = parent.child_by_field_name("condition") + if ( + condition is None + or condition.type != "number_literal" + or condition.text != b"0" + ): + return False + alternative = parent.child_by_field_name("alternative") + return not ( + alternative is not None + and child.start_byte == alternative.start_byte + and child.end_byte == alternative.end_byte + ) + + return False + + def _is_in_static_dead_guard(node) -> bool: """Return True if *node* sits in a statically-dead branch (non-Python). - Two independent ancestor walks: + Walk the ancestor chain once, checking each parent-child edge: - * A walk for Go / TS / JS ``if false`` / ``if (0)``. - * A walk for C/C++ ``#if 0`` / ``#elif 0``. + * Go / TS / JS ``if false`` / ``if (0)``. + * C/C++ ``#if 0`` / ``#elif 0``. Neither walk stops at a function or class boundary. A declaration nested inside a dead branch is never evaluated, so calls in its body @@ -445,40 +476,13 @@ def _is_in_static_dead_guard(node) -> bool: JS/TS class declarations are not hoisted, so there is no reachable symbol to preserve either. """ - # Go / TS / JS: ``if`` with a statically-false condition. - cursor = node.parent - while cursor is not None: - node_type = cursor.type - if node_type == "if_statement": - condition = cursor.child_by_field_name("condition") - consequence = cursor.child_by_field_name("consequence") - if ( - condition is not None - and consequence is not None - and _is_statically_false_condition(condition) - and _node_is_in_child(node, consequence) - ): - return True - cursor = cursor.parent - - # C / C++: ``#if 0`` / ``#elif 0`` preprocessor block. - preproc = node.parent - while preproc is not None: - if preproc.type in ("preproc_if", "preproc_elif"): - condition = preproc.child_by_field_name("condition") - if ( - condition is not None - and condition.type == "number_literal" - and condition.text == b"0" - ): - alternative = preproc.child_by_field_name("alternative") - if not ( - alternative is not None - and _node_is_in_child(node, alternative) - ): - return True - preproc = preproc.parent - + child = node + parent = child.parent + while parent is not None: + if _is_static_dead_guard_child(parent, child): + return True + child = parent + parent = parent.parent return False @@ -1173,13 +1177,13 @@ def __post_init__(self) -> None: "csharp": [ "class_declaration", "interface_declaration", "enum_declaration", "struct_declaration", - "record_declaration", "record_struct_declaration", + "record_declaration", ], "ruby": ["class", "module"], "r": [], # Classes detected via call pattern-matching, not AST node types "perl": ["package_statement", "class_statement", "role_statement"], "kotlin": ["class_declaration", "object_declaration"], - "swift": ["class_declaration", "struct_declaration", "protocol_declaration"], + "swift": ["class_declaration", "protocol_declaration"], "php": [ "class_declaration", "interface_declaration", "trait_declaration", "enum_declaration", @@ -1197,7 +1201,7 @@ def __post_init__(self) -> None: "luau": ["type_definition"], # Luau type aliases; table-based OOP via constructs handler "objc": [ "class_interface", "class_implementation", - "category_interface", "protocol_declaration", + "protocol_declaration", ], "bash": [], # Shell has no classes # Elixir: `defmodule Name do ... end` is a ``call`` node whose first @@ -1393,7 +1397,7 @@ def __post_init__(self) -> None: "c": ["call_expression"], "cpp": ["call_expression"], "csharp": ["invocation_expression", "object_creation_expression"], - "ruby": ["call", "method_call"], + "ruby": ["call"], "r": ["call"], "perl": [ "function_call_expression", "method_call_expression", @@ -1429,7 +1433,7 @@ def __post_init__(self) -> None: # SuffixExpr); calls are walked explicitly in # _extract_zig_calls_in_subtree from inside function bodies. "zig": [], - "powershell": ["command_expression"], + "powershell": ["command"], "julia": [ "call_expression", "broadcast_call_expression", @@ -1522,6 +1526,7 @@ def _builtin_language_names() -> frozenset[str]: # Filename conventions. Matched against the basename alone, so they hold # whatever the repository root is. +_STORYBOOK_FILE_RE = re.compile(r"\.stories\.[jt]sx?$") _TEST_NAME_PATTERNS = [ # Python: pytest/unittest stems plus conftest, which holds fixtures. re.compile(r"^test_.*\.py$"), @@ -1529,6 +1534,8 @@ def _builtin_language_names() -> frozenset[str]: re.compile(r"^conftest\.py$"), # JavaScript / TypeScript. re.compile(r"\.(?:test|spec)\.[cm]?[jt]sx?$"), + # Storybook CSF files demonstrate components; they are not production callers. + _STORYBOOK_FILE_RE, # Go. re.compile(r"_test\.go$"), # Ruby. @@ -3382,10 +3389,14 @@ def _extract_bytes( @staticmethod def _has_cpp_header_evidence(root) -> bool: """Return whether a parsed ``.h`` tree contains C++-only syntax.""" - pending = [root] + # Carry dead-guard state down the tree. Calling + # ``_is_in_static_dead_guard`` for every node repeatedly walks the + # nested ``preproc_elif`` chain in large generated headers, making + # this scan quadratic in the number of branches. + pending = [(root, False)] while pending: - node = pending.pop() - if node.type == "ERROR" or _is_in_static_dead_guard(node): + node, in_dead_guard = pending.pop() + if node.type == "ERROR" or in_dead_guard: continue previous = node.prev_named_sibling @@ -3409,7 +3420,11 @@ def _has_cpp_header_evidence(root) -> bool: and node.text in _CPP_HEADER_EVIDENCE_QUALIFIERS ): return True - pending.extend(node.named_children) + for child in node.named_children: + pending.append(( + child, + in_dead_guard or _is_static_dead_guard_child(node, child), + )) return False @staticmethod @@ -11658,6 +11673,12 @@ def _extract_classes( # Tree-sitter maps struct/enum/actor/extension all to class_declaration; # protocol uses its own protocol_declaration node type. extra: dict = {} + # TypeScript interfaces and type aliases share kind Class but have no + # runtime body, so changes.py must not report them as test gaps (#1109). + if language in ("typescript", "tsx") and child.type in ( + "interface_declaration", "type_alias_declaration", + ): + extra["typescript_kind"] = child.type if language == "swift": if child.type == "class_declaration": _swift_keywords = {"class", "struct", "enum", "actor", "extension"} diff --git a/code_review_graph/postprocessing.py b/code_review_graph/postprocessing.py index e78e85b9b..d449197d6 100644 --- a/code_review_graph/postprocessing.py +++ b/code_review_graph/postprocessing.py @@ -46,10 +46,15 @@ def run_post_processing( changed_files: Files this update re-parsed, when the caller knows them. The FTS5 step then rewrites only those files' index entries instead of dropping and repopulating the index. + embedding_provider: Exact provider for an explicit embedding + refresh. Must be supplied with ``embedding_model``. + embedding_model: Exact model for an explicit embedding refresh. + Must be supplied with ``embedding_provider``. Returns: Dict with keys for each step's result count and a ``warnings`` - list (only present when at least one step failed). + list, present when at least one step failed or when an explicit + refresh found no index (``embeddings_refresh_skipped``). """ result: dict[str, Any] = {} warnings: list[str] = [] @@ -210,10 +215,14 @@ def _refresh_embeddings( return try: - from .embeddings import refresh_embeddings + from .embeddings import REFRESH_SKIPPED_WARNING, refresh_embeddings refreshed = refresh_embeddings(store, provider=provider, model=model) - if refreshed is not None: + if refreshed is None: + logger.warning(REFRESH_SKIPPED_WARNING) + warnings.append(REFRESH_SKIPPED_WARNING) + result["embeddings_refresh_skipped"] = True + else: result["embeddings_refreshed"] = refreshed["embedded"] result["embeddings_purged"] = refreshed["purged"] except Exception as exc: diff --git a/code_review_graph/refactor.py b/code_review_graph/refactor.py index 0aad45d8b..d7b9e3af1 100644 --- a/code_review_graph/refactor.py +++ b/code_review_graph/refactor.py @@ -19,6 +19,7 @@ from .flows import _has_framework_decorator, _matches_entry_name from .graph import GraphStore, _sanitize_name +from .parser import _STORYBOOK_FILE_RE from .parser import is_test_file as _is_test_file from .parser import repo_relative_path as _repo_relative @@ -495,12 +496,18 @@ def _is_plausible_caller( if base_name is not None: continue - incoming = store.get_edges_by_target(node.qualified_name) + incoming = [ + e for e in store.get_edges_by_target(node.qualified_name) + if not _STORYBOOK_FILE_RE.search(e.file_path) + ] # Also check class-qualified edges (e.g. "ClassName::method") which # lack the file-path prefix used in node.qualified_name. if not any(e.kind == "CALLS" for e in incoming) and node.parent_name: class_qn = f"{node.parent_name}::{node.name}" - incoming = incoming + store.get_edges_by_target(class_qn) + incoming += [ + e for e in store.get_edges_by_target(class_qn) + if not _STORYBOOK_FILE_RE.search(e.file_path) + ] # Also check bare-name and partially-qualified edges. # CALLS targets may be bare ("funcName"), class-qualified # ("Class::method"), or workspace-qualified ("pkg/dir::funcName"). @@ -557,6 +564,10 @@ def _is_plausible_caller( language=node.language or None, ) incoming = incoming + bare_inh + # Stories demonstrate a component but do not prove application use. + # Check paths as well as flags so pre-existing graphs behave correctly. + incoming = [e for e in incoming if not _STORYBOOK_FILE_RE.search(e.file_path)] + outgoing_tb = [e for e in outgoing_tb if not _STORYBOOK_FILE_RE.search(e.file_path)] has_callers = any(e.kind == "CALLS" for e in incoming) has_test_refs = bool(outgoing_tb) has_importers = any(e.kind == "IMPORTS_FROM" for e in incoming) @@ -573,11 +584,11 @@ def _is_plausible_caller( # Also check bare class-name pattern (unresolved CALLS targets) bare_prefix = node.name + "." member_calls = conn.execute( - "SELECT COUNT(*) FROM edges WHERE kind = 'CALLS'" + "SELECT file_path FROM edges WHERE kind = 'CALLS'" " AND (target_qualified LIKE ? OR target_qualified LIKE ?)", (f"%{member_prefix}%", f"%{bare_prefix}%"), - ).fetchone()[0] - if member_calls > 0: + ) + if any(not _STORYBOOK_FILE_RE.search(row[0]) for row in member_calls): has_callers = True if not ( diff --git a/code_review_graph/tools/build.py b/code_review_graph/tools/build.py index 588a2ffd3..b5dd6d7c6 100644 --- a/code_review_graph/tools/build.py +++ b/code_review_graph/tools/build.py @@ -64,7 +64,9 @@ def _run_embedding_refresh( from code_review_graph.embeddings import refresh_embeddings refreshed = refresh_embeddings(store, provider=provider, model=model) - if refreshed is not None: + if refreshed is None: + _report_refresh_skipped(result, warnings) + else: result["embeddings_refreshed"] = refreshed["embedded"] result["embeddings_purged"] = refreshed["purged"] except Exception as exc: @@ -74,6 +76,15 @@ def _run_embedding_refresh( ) +def _report_refresh_skipped(result: dict[str, Any], warnings: list[str]) -> None: + """Tell a caller who asked for a refresh that the graph has no index to refresh.""" + from code_review_graph.embeddings import REFRESH_SKIPPED_WARNING + + logger.warning(REFRESH_SKIPPED_WARNING) + warnings.append(REFRESH_SKIPPED_WARNING) + result["embeddings_refresh_skipped"] = True + + # SQLITE_BUSY and SQLITE_LOCKED are the only two primary result codes that # mean "somebody else holds it, come back later". Extended result codes carry # their detail in the high bits, so the low byte is what identifies the family. @@ -709,7 +720,7 @@ def build_or_update_graph( # leaving the marker set would make every later update a full # rebuild. store.set_metadata(BUILD_STATE_KEY, BUILD_COMPLETE) - return { + noop_result = { **result, "status": "ok", "build_type": "incremental", @@ -717,6 +728,17 @@ def build_or_update_graph( "summary": summary, "postprocess_level": postprocess, } + # No node changed, so there is nothing to refresh. A caller who + # passes the pair on every update would still never learn that + # the graph has no index at all, so that case is reported here. + if embedding_provider and embedding_model: + from code_review_graph.embeddings import has_embeddings + + if not has_embeddings(store): + noop_warnings: list[str] = [] + _report_refresh_skipped(noop_result, noop_warnings) + noop_result["warnings"] = noop_warnings + return noop_result summary = ( f"Incremental update: {result['files_updated']} files re-parsed, " f"{result['total_nodes']} nodes and " diff --git a/docs/USAGE.md b/docs/USAGE.md index a381c4d30..f02cd25b1 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -111,7 +111,7 @@ code-review-graph build \ --embedding-model all-MiniLM-L6-v2 ``` -A refresh only updates a previously embedded graph. It refuses to migrate vectors to a different provider, model or endpoint, removes vectors for deleted nodes, and turns provider or transport failures into build warnings. +A refresh only updates a previously embedded graph. On a graph with no embeddings yet it embeds nothing: `build`, `update` (also when it finds no changes) and `postprocess` return `embeddings_refresh_skipped: true` with a warning, and `watch` logs that warning after each update and keeps running. Run `code-review-graph embed` once to create the index. A refresh refuses to migrate vectors to a different provider, model or endpoint, removes vectors for deleted nodes, and turns provider or transport failures into build warnings. ### 7. Detect changes with risk scoring Ask your MCP client: "Review my recent changes with risk scoring". This calls `detect_changes_tool`, which maps the diff to affected functions, flows, communities and test gaps. @@ -160,6 +160,10 @@ Languages not covered can be added through a `.code-review-graph/languages.toml` ## What Gets Indexed +Storybook CSF files (`*.stories.js`, `.jsx`, `.ts`, `.tsx`) are indexed as test/demo code. +Their imports, calls, and references alone do not keep application symbols alive in +dead-code reports. Normal application callers and ordinary test evidence still count. + - **Nodes**: Files, Classes, Functions/Methods, Types, Tests, plus Endpoints, Schedulers and ConfigProperties where framework enrichment applies - **Edges**: CALLS, IMPORTS_FROM, INHERITS, IMPLEMENTS, CONTAINS, TESTED_BY, DEPENDS_ON, REFERENCES, plus framework-specific kinds (INJECTS, HANDLES, TRIGGERS, PUBLISHES, CONSUMES/PRODUCES, DEPENDS_ON_CONFIG, TEMPORAL_STUB) diff --git a/scripts/render_pr_comment.py b/scripts/render_pr_comment.py index b326c08bd..1d2a36689 100644 --- a/scripts/render_pr_comment.py +++ b/scripts/render_pr_comment.py @@ -580,7 +580,7 @@ def main(argv: list[str] | None = None) -> int: sys.stdout.write(_artifact_text(body)) else: try: - Path(args.output).write_text(_artifact_text(body), encoding="utf-8") + Path(args.output).write_bytes(_artifact_text(body).encode("utf-8")) except OSError as exc: logger.error("Cannot write output file %s: %s", args.output, exc) return 2 diff --git a/tests/test_action_render.py b/tests/test_action_render.py index ab2de7fa2..f4ce05d71 100644 --- a/tests/test_action_render.py +++ b/tests/test_action_render.py @@ -2,8 +2,12 @@ from __future__ import annotations +import builtins import importlib.util +import io import json +import os +import pathlib import subprocess import sys from pathlib import Path @@ -553,6 +557,377 @@ def test_written_artifact_never_exceeds_the_consumer_cap(tmp_path, offset): assert "Powered by [code-review-graph]" in out.read_text(encoding="utf-8") +def test_output_file_preserves_the_bytes_measured_by_the_budget(tmp_path, report): + """Writing a report must not add platform-specific CRLF bytes.""" + source = tmp_path / "report.json" + source.write_text(json.dumps(report), encoding="utf-8") + out = tmp_path / "comment.md" + + expected = render.render_markdown(report) + assert render.main( + ["--input", str(source), "--output", str(out)] + ) == 0 + + assert out.read_bytes() == (expected + "\n").encode("utf-8") + + +# --------------------------------------------------------------------------- +# The artifact under Windows newline semantics (#1076, fixed in #1077) +# --------------------------------------------------------------------------- +# +# On Windows a file opened for writing in text mode with newline=None turns +# every "\n" into "\r\n", one extra byte per line the budget never counted. +# POSIX CPython never translates, so on Linux and macOS the artifact is the +# same whichever way main() writes it and the tests above cannot tell a text +# write from a byte write. ``windows_text_writes`` reproduces the translation +# in-process by handing newline="\r\n" -- os.linesep on Windows -- to every +# text-mode write that left newline unset. The windows-native CI job does not +# run this module, so this is where the Windows write path is exercised. + + +def _is_text_write(mode: str) -> bool: + return "b" not in mode and any(flag in mode for flag in "wax+") + + +@pytest.fixture() +def windows_text_writes(monkeypatch): + """Make text-mode writes in this process translate newlines as Windows does.""" + real_open = io.open + + def win_open(file, mode="r", buffering=-1, encoding=None, errors=None, + newline=None, closefd=True, opener=None): + if _is_text_write(mode) and newline is None: + newline = "\r\n" + return real_open(file, mode, buffering, encoding, errors, newline, closefd, opener) + + def win_path_open(self, mode="r", buffering=-1, encoding=None, errors=None, + newline=None): + return win_open(self, mode, buffering, encoding, errors, newline) + + monkeypatch.setattr(io, "open", win_open) + monkeypatch.setattr(builtins, "open", win_open) + # Path.open is what Path.write_text goes through on every supported + # Python; 3.10 bound io.open at import, so patching io alone misses it. + monkeypatch.setattr(pathlib.Path, "open", win_path_open) + + +def test_the_windows_simulation_really_translates(tmp_path, windows_text_writes): + """Teeth for the tests below: an inert simulation would pass them vacuously.""" + probe = tmp_path / "probe.md" + probe.write_text("x\n" * 1000, encoding="utf-8") + # The issue's own reproduction: 3000 bytes on Windows, 2000 on POSIX. + assert len(probe.read_bytes()) == 3000 + with open(probe, "w", encoding="utf-8") as fh: + fh.write("a\n") + assert probe.read_bytes() == b"a\r\n" + probe.write_bytes(b"a\n") + assert probe.read_bytes() == b"a\n" + assert probe.read_text(encoding="utf-8") == "a\n", "reads must stay untouched" + + +def _render_to_file(tmp_path: Path, report: dict, *extra: str) -> tuple[int, bytes]: + source = tmp_path / "report.json" + source.write_text(json.dumps(report), encoding="utf-8") + out = tmp_path / "comment.md" + code = render.main(["--input", str(source), "--output", str(out), *extra]) + return code, out.read_bytes() + + +@pytest.mark.parametrize("offset", [-1, 0, 1]) +def test_windows_artifact_never_exceeds_the_consumer_cap( + tmp_path, windows_text_writes, offset +): + """The boundary from #1076: 61,399 bytes on Windows before the fix.""" + code, data = _render_to_file( + tmp_path, + report_rendering_to_exactly(render._MAX_BODY + offset), + "--max-functions", + "100000", + ) + assert code == 0 + assert b"\r" not in data + assert len(data) <= render._MAX_BODY, len(data) + assert b"Powered by [code-review-graph]" in data + + +def test_windows_artifact_that_fits_is_written_whole(tmp_path, windows_text_writes): + """A 59,999-byte body is a 60,000-byte artifact on Windows too, not cut.""" + code, data = _render_to_file( + tmp_path, + report_rendering_to_exactly(render._MAX_BODY - 1), + "--max-functions", + "100000", + ) + assert code == 0 + assert len(data) == render._MAX_BODY + assert b"*Report truncated.*" not in data + + +def test_windows_truncated_multibyte_report_fits_the_cap(tmp_path, windows_text_writes): + """Non-ASCII names: the cap is UTF-8 bytes on disk, and the cut stays valid UTF-8.""" + huge = { + "risk_score": 0.5, + "review_priorities": [ + { + "qualified_name": f"src/café_日本.py::grüße_{i}_" + "é🚀" * 30, + "risk_score": 0.5, + "file_path": "src/café_日本.py", + "line_start": i, + } + for i in range(3000) + ], + } + code, data = _render_to_file(tmp_path, huge, "--max-functions", "3000") + assert code == 0 + assert len(data) <= render._MAX_BODY, len(data) + assert b"\r" not in data + text = data.decode("utf-8") # strict: a split code point would raise + assert "*Report truncated.*" in text + assert text.endswith(render.FOOTER + "\n") + + +_NON_ASCII_REPORT = { + "risk_score": 0.5, + "changed_functions": [ + {"qualified_name": "src/café.py::grüße", "file_path": "src/café.py", + "line_start": 3, "risk_score": 0.5}, + {"qualified_name": "src/日本.py::関数_🚀", "file_path": "src/日本.py", + "line_start": 9, "risk_score": 0.4}, + ], + "test_gaps": [ + {"qualified_name": "src/café.py::grüße", "file_path": "src/café.py", + "line_start": 3}, + ], +} + + +@pytest.mark.parametrize( + ("input_bytes", "expected_body", "expected_code"), + [ + pytest.param( + FIXTURE.read_bytes(), + lambda: render.render_markdown( + json.loads(FIXTURE.read_text(encoding="utf-8")) + ), + 0, + id="analysis", + ), + pytest.param( + json.dumps(_NON_ASCII_REPORT, ensure_ascii=False).encode("utf-8"), + lambda: render.render_markdown(_NON_ASCII_REPORT), + 0, + id="non-ascii-names", + ), + pytest.param( + # An older report shape: no review_priorities, no coverage counts. + json.dumps({"risk_score": 0.2, "changed_functions": [ + {"qualified_name": "a.py::f", "file_path": "a.py", "line_start": 1} + ]}).encode("utf-8"), + lambda: render.render_markdown({"risk_score": 0.2, "changed_functions": [ + {"qualified_name": "a.py::f", "file_path": "a.py", "line_start": 1} + ]}), + 0, + id="legacy-report-shape", + ), + pytest.param(b"{}", lambda: render.render_markdown({}), 0, id="empty-report"), + pytest.param( + # detect-changes' clean-tree line as a Windows shell would save it. + b"No changes detected.\r\n", + render.render_no_changes, + 0, + id="no-changes-crlf-input", + ), + pytest.param( + b"Error: could not determine the changes\r\n", + lambda: render.render_not_analyzed( + "Error: could not determine the changes\n" + ), + 4, + id="not-analyzed", + ), + ], +) +def test_windows_output_file_is_the_measured_utf8_bytes( + tmp_path, windows_text_writes, input_bytes, expected_body, expected_code +): + """Every body main() can write lands on disk as exactly the bytes it measured.""" + source = tmp_path / "report.json" + source.write_bytes(input_bytes) + out = tmp_path / "comment.md" + code = render.main(["--input", str(source), "--output", str(out)]) + assert code == expected_code + assert out.read_bytes() == (expected_body() + "\n").encode("utf-8") + + +def test_risk_gate_breach_still_writes_the_exact_artifact(tmp_path, windows_text_writes): + """Exit 3 is decided after the write; the file is complete and LF-only.""" + out = tmp_path / "comment.md" + code = render.main( + ["--input", str(FIXTURE), "--output", str(out), "--fail-on-risk", "high"] + ) + assert code == 3 + expected = render.render_markdown(json.loads(FIXTURE.read_text(encoding="utf-8"))) + assert out.read_bytes() == (expected + "\n").encode("utf-8") + + +def test_output_path_with_spaces_and_unicode(tmp_path, windows_text_writes): + out_dir = tmp_path / "runner temp é 日本" + out_dir.mkdir() + out = out_dir / "crg comment 🚀.md" + assert render.main(["--input", str(FIXTURE), "--output", str(out)]) == 0 + expected = render.render_markdown(json.loads(FIXTURE.read_text(encoding="utf-8"))) + assert out.read_bytes() == (expected + "\n").encode("utf-8") + + +def test_rendering_twice_replaces_a_larger_artifact_exactly(tmp_path): + """A rerun into the same file leaves no tail of the previous, larger one.""" + out = tmp_path / "comment.md" + out.write_bytes(b"stale\r\n" * 20_000) + args = ["--input", str(FIXTURE), "--output", str(out)] + assert render.main(args) == 0 + first = out.read_bytes() + assert render.main(args) == 0 + assert out.read_bytes() == first + expected = render.render_markdown(json.loads(FIXTURE.read_text(encoding="utf-8"))) + assert first == (expected + "\n").encode("utf-8") + + +@pytest.mark.parametrize("target", ["missing-dir", "is-a-directory"]) +def test_unwritable_output_returns_2(tmp_path, caplog, target): + """The byte write keeps the OSError path: exit 2 and a logged reason.""" + if target == "missing-dir": + out = tmp_path / "no" / "such" / "dir" / "comment.md" + else: + out = tmp_path / "comment.md" + out.mkdir() + with caplog.at_level("ERROR", logger="render_pr_comment"): + code = render.main(["--input", str(FIXTURE), "--output", str(out)]) + assert code == 2 + assert "Cannot write output file" in caplog.text + if target == "missing-dir": + assert not out.exists() + + +# The consumer itself: the validator step of the privileged workflow, read +# straight out of pr-review-comment.yml together with the cap it enforces, so +# a Windows-written artifact is judged by the real check rather than by a +# restatement of it, and the renderer's budget cannot drift from that cap. +COMMENT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "pr-review-comment.yml" + + +def _trusted_validator() -> tuple[str, dict[str, str]]: + """The validator step's Python source and its declared environment.""" + import yaml # a runtime dependency, but only these tests need it + + workflow = yaml.safe_load(COMMENT_WORKFLOW.read_text(encoding="utf-8")) + for job in workflow["jobs"].values(): + for step in job.get("steps", []): + run = step.get("run") or "" + if "Report artifact is empty or too large" not in run: + continue + lines = run.split("\n") + start = lines.index("python - <<'PY'") + 1 + end = next(i for i in range(start, len(lines)) if lines[i].strip() == "PY") + source = "\n".join(lines[start:end]) + "\n" + return source, {key: str(value) for key, value in step["env"].items()} + raise AssertionError("validator step not found in pr-review-comment.yml") + + +def _run_trusted_validator(tmp_path: Path, download: Path) -> subprocess.CompletedProcess: + source, declared = _trusted_validator() + script = tmp_path / "validator.py" + script.write_bytes(source.encode("utf-8")) + env = dict(os.environ) + env.update(declared) + env.update( + { + "DOWNLOAD_DIR": str(download), + "COMMENT_BODY": str(tmp_path / "crg-comment-body.md"), + "VALIDATED_PR_NUMBER": str(tmp_path / "crg-pr-number.txt"), + } + ) + return subprocess.run( + [sys.executable, str(script)], + env=env, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + timeout=60, + stdin=subprocess.DEVNULL, + ) + + +def test_renderer_budget_is_the_cap_the_trusted_workflow_enforces(): + _source, declared = _trusted_validator() + assert render._MAX_BODY == int(declared["MAX_REPORT_BYTES"]) + assert render.MARKER == declared["TRUSTED_MARKER"] + + +@pytest.mark.parametrize("offset", [-1, 0, 1]) +def test_windows_artifact_at_the_cap_is_accepted_by_the_trusted_validator( + tmp_path, windows_text_writes, offset +): + """#1076 end to end: the file main() writes on Windows passes the real check. + + Before the fix all three were rejected ("Report artifact is empty or too + large"); the 59,999-byte body became the 61,399-byte artifact in the issue. + """ + _source, declared = _trusted_validator() + cap = int(declared["MAX_REPORT_BYTES"]) + source = tmp_path / "crg-report.json" + source.write_bytes(json.dumps(report_rendering_to_exactly(cap + offset)).encode("utf-8")) + download = tmp_path / "crg-report-download" + download.mkdir() + artifact = download / "crg-comment.md" + code = render.main( + ["--input", str(source), "--output", str(artifact), "--max-functions", "100000"] + ) + assert code == 0 + # pr-review.yml writes this on its Linux runner; bytes keep it exact here. + (download / "pr-number.txt").write_bytes(b"42\n") + + completed = _run_trusted_validator(tmp_path, download) + + assert completed.returncode == 0, completed.stderr or completed.stdout + wrapped = (tmp_path / "crg-comment-body.md").read_bytes().decode("utf-8") + assert wrapped.startswith(declared["TRUSTED_MARKER"] + "\n\n## code-review-graph review\n") + assert wrapped.rstrip("\n").endswith(render.FOOTER) + + +@pytest.mark.parametrize( + "report_data", + [ + pytest.param(json.loads(FIXTURE.read_text(encoding="utf-8")), id="analysis"), + pytest.param(_NON_ASCII_REPORT, id="non-ascii-names"), + ], +) +def test_windows_crlf_detect_changes_output_renders_the_exact_artifact( + tmp_path, windows_text_writes, report_data +): + """The input the Action really gets on a Windows runner. + + ``detect-changes`` prints ``json.dumps(result, indent=2)`` and the Action + redirects it into crg-report.json; Windows stdout is text mode, so that + file arrives ASCII-escaped with CRLF on every line. The artifact must be + the same LF-only bytes a Linux runner produces from the LF file. + """ + printed = json.dumps(report_data, indent=2, default=str) + "\n" + lf_input = tmp_path / "lf.json" + lf_input.write_bytes(printed.encode("ascii")) + crlf_input = tmp_path / "crlf.json" + crlf_input.write_bytes(printed.replace("\n", "\r\n").encode("ascii")) + lf_out = tmp_path / "lf.md" + crlf_out = tmp_path / "crlf.md" + + assert render.main(["--input", str(lf_input), "--output", str(lf_out)]) == 0 + assert render.main(["--input", str(crlf_input), "--output", str(crlf_out)]) == 0 + + expected = (render.render_markdown(report_data) + "\n").encode("utf-8") + assert crlf_out.read_bytes() == expected + assert lf_out.read_bytes() == expected + + def test_a_body_that_still_fits_is_not_truncated(tmp_path): """The reservation costs one byte, not a whole report. diff --git a/tests/test_cli_surface.py b/tests/test_cli_surface.py index ea623452b..b2ba6cb9f 100644 --- a/tests/test_cli_surface.py +++ b/tests/test_cli_surface.py @@ -47,6 +47,7 @@ import argparse import ast +import json import os import re import shutil @@ -1156,6 +1157,10 @@ class NumericVar: NumericVar("CRG_MAX_IMPACT_DEPTH", ("status",), "3"), NumericVar("CRG_MAX_BFS_DEPTH", ("status",), "10"), NumericVar("CRG_MAX_SEARCH_RESULTS", ("status",), "15"), + # Parsed at import; the test-gap caller walk that ``detect-changes`` + # runs is what reads them, so drive that command rather than ``status``. + NumericVar("CRG_CALLER_TEST_ROUTE_DEPTH", ("detect-changes", "--brief"), "3"), + NumericVar("CRG_CALLER_TEST_ROUTE_MAX_CALLERS", ("detect-changes", "--brief"), "128"), NumericVar("CRG_IMPACT_DEPTH_DECAY", ("status",), "0.5"), NumericVar("CRG_IMPACT_SCORE_FLOOR", ("status",), "0.1"), NumericVar("CRG_PARSE_WORKERS", ("status",), "2"), @@ -1386,6 +1391,100 @@ def test_no_new_unguarded_numeric_env_parse(): ) +# The two caller-walk limits (#1047) are parsed once, at import, and then +# baked into ``GraphStore.get_caller_test_routes``. The table rows above only +# prove a value does not crash; these prove the value reaches the walk, so a +# setting that silently stops being read (or reads the wrong name) fails here. +_ROUTE_DEPTH = "CRG_CALLER_TEST_ROUTE_DEPTH" +_ROUTE_MAX_CALLERS = "CRG_CALLER_TEST_ROUTE_MAX_CALLERS" + + +@pytest.fixture(scope="module") +def helper_changed_repo(tmp_path_factory, cli_env) -> Path: + """HEAD changes ``helper``: no test names it, but tested ``entry`` calls it. + + ``helper`` has exactly two callers (``entry`` and ``double_all``), which + the max-callers boundary cases below depend on. + """ + repo = init_repo(tmp_path_factory.mktemp("cli-surface-route") / "repo") + write_sample_project(repo) + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "initial") + core = repo / "pkg" / "core.py" + core.write_text( + core.read_text(encoding="utf-8").replace("return value * 2", "return value * 2 + 0"), + encoding="utf-8", + ) + _git(repo, "commit", "-qam", "change helper") + built = run_cli("build", "-q", cwd=repo, env=cli_env) + assert built.returncode == 0, built.describe() + return repo + + +def _route_env(cli_env: dict[str, str], **overrides: str) -> dict[str, str]: + env = {k: v for k, v in cli_env.items() if k not in (_ROUTE_DEPTH, _ROUTE_MAX_CALLERS)} + env.update(overrides) + return env + + +def _helper_gap(repo: Path, env: dict[str, str]) -> tuple[CliResult, dict]: + """Run ``detect-changes`` and return the ``helper`` row of ``test_gaps``.""" + result = run_cli("detect-changes", cwd=repo, env=env) + assert not result.traceback, result.describe() + assert result.returncode == 0, result.describe() + # A warning line may precede the JSON in the merged stream. + start = re.search(r"^\{", result.output, re.MULTILINE) + assert start is not None, result.describe() + payload, _ = json.JSONDecoder().raw_decode(result.output[start.start():]) + rows = [gap for gap in payload.get("test_gaps", []) if gap.get("name") == "helper"] + # Never suppressed: whatever the limits, the changed symbol stays a gap. + assert len(rows) == 1, result.describe() + return result, rows[0] + + +def test_caller_route_is_reported_with_the_default_limits(helper_changed_repo, cli_env): + _, gap = _helper_gap(helper_changed_repo, _route_env(cli_env)) + assert gap["coverage"] == "indirect", gap + assert gap["covered_via"].endswith("::entry"), gap + assert gap["covered_depth"] == 1, gap + + +@pytest.mark.parametrize( + ("name", "value", "coverage"), + [ + pytest.param(_ROUTE_DEPTH, "0", "none", id="depth-0-disables"), + pytest.param(_ROUTE_DEPTH, "1", "indirect", id="depth-1-keeps-direct-caller"), + pytest.param(_ROUTE_DEPTH, " 1 ", "indirect", id="depth-padded"), + pytest.param(_ROUTE_DEPTH, "-1", "none", id="depth-negative"), + pytest.param(_ROUTE_MAX_CALLERS, "0", "none", id="max-0-disables"), + pytest.param(_ROUTE_MAX_CALLERS, "1", "none", id="max-below-in-degree"), + pytest.param(_ROUTE_MAX_CALLERS, "2", "indirect", id="max-equal-in-degree"), + pytest.param(_ROUTE_MAX_CALLERS, "-1", "none", id="max-negative"), + ], +) +def test_caller_route_env_var_reaches_the_walk( + name, value, coverage, helper_changed_repo, cli_env +): + """A valid override changes the answer; a limit can only shrink the walk.""" + result, gap = _helper_gap(helper_changed_repo, _route_env(cli_env, **{name: value})) + assert gap["coverage"] == coverage, result.describe() + assert "Ignoring invalid" not in result.output, result.describe() + + +@pytest.mark.parametrize( + "bad", ["", "abc", "1.5.2", "2.5"], ids=["empty", "word", "version", "float"] +) +@pytest.mark.parametrize("name", [_ROUTE_DEPTH, _ROUTE_MAX_CALLERS]) +def test_invalid_caller_route_env_var_warns_by_name_and_keeps_the_default( + name, bad, helper_changed_repo, cli_env +): + """Unparseable means the documented default, not zero, and says so by name.""" + result, gap = _helper_gap(helper_changed_repo, _route_env(cli_env, **{name: bad})) + assert name in result.output, result.describe() + assert gap["coverage"] == "indirect", result.describe() + assert gap["covered_depth"] == 1, result.describe() + + def test_crg_data_dir_override_is_honoured(tmp_path, cli_env): """The documented global override sends the graph somewhere else.""" repo = init_repo(tmp_path / "repo") diff --git a/tests/test_cpp_qt_headers.py b/tests/test_cpp_qt_headers.py index 2fc4e7a50..ff421d220 100644 --- a/tests/test_cpp_qt_headers.py +++ b/tests/test_cpp_qt_headers.py @@ -3,6 +3,7 @@ import shutil from pathlib import Path +from code_review_graph import parser as parser_module from code_review_graph.graph import GraphStore from code_review_graph.incremental import full_build from code_review_graph.parser import CodeParser @@ -133,6 +134,31 @@ class Disabled {}; assert _file_language(nodes) == "c", name +def test_long_elif_chain_does_not_walk_ancestors_per_node(monkeypatch) -> None: + lines = ["#if defined(START_WITH_IF)"] + for index in range(256): + lines.extend([ + f"#elif defined(START_{index})", + f"#undef START_{index}", + "#pragma section", + ]) + lines.extend(["#endif", "typedef int value;"]) + + tree_parser = CodeParser()._get_parser("c") + assert tree_parser is not None + tree = tree_parser.parse("\n".join(lines).encode()) + + def unexpected_ancestor_walk(node) -> bool: + raise AssertionError("header evidence scan walked each node's ancestors") + + monkeypatch.setattr( + parser_module, + "_is_in_static_dead_guard", + unexpected_ancestor_walk, + ) + assert CodeParser._has_cpp_header_evidence(tree.root_node) is False + + def test_c_auto_and_c23_constexpr_are_not_cpp_evidence(tmp_path: Path) -> None: sources = { "auto_storage.h": "auto int value;\n", diff --git a/tests/test_embedding_refresh.py b/tests/test_embedding_refresh.py index 67f14e778..633f9b8b4 100644 --- a/tests/test_embedding_refresh.py +++ b/tests/test_embedding_refresh.py @@ -1,17 +1,24 @@ """Explicit, provider-scoped embedding refresh and orphan cleanup.""" import asyncio +import subprocess import sys from pathlib import Path from unittest.mock import MagicMock, patch import pytest -from code_review_graph.embeddings import EmbeddingStore, embed_all_nodes, refresh_embeddings +from code_review_graph.embeddings import ( + REFRESH_SKIPPED_WARNING, + EmbeddingStore, + embed_all_nodes, + refresh_embeddings, +) from code_review_graph.graph import GraphStore +from code_review_graph.incremental import get_db_path from code_review_graph.parser import NodeInfo from code_review_graph.postprocessing import run_post_processing -from code_review_graph.tools.build import _run_postprocess +from code_review_graph.tools.build import _run_postprocess, build_or_update_graph, run_postprocess from code_review_graph.tools.docs import embed_graph @@ -312,6 +319,162 @@ def test_legacy_rows_without_provider_identity_are_refused_precisely(self, tmp_p graph.close() +def _graph_without_vectors(tmp_path, *, empty_table: bool): + """A graph with nodes and no vectors: no embeddings table, or an empty one.""" + graph, _ = _graph_with_function(tmp_path) + if empty_table: + # Any EmbeddingStore creates the table; list_graph_stats does this too. + with patch("code_review_graph.embeddings.get_provider", return_value=None): + EmbeddingStore(graph.db_path).close() + return graph + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", "-c", "user.email=t@example.invalid", "-c", "user.name=T", *args], + cwd=repo, + check=True, + capture_output=True, + ) + + +def _git_repo(tmp_path: Path, monkeypatch) -> Path: + """A committed one-file git repository, parsed serially.""" + monkeypatch.setenv("CRG_SERIAL_PARSE", "1") + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-q") + (repo / "app.py").write_text("def hello():\n return 1\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "init") + return repo + + +class TestRefreshWithoutVectors: + """An explicit refresh on a never-embedded graph reports that it embedded nothing.""" + + @pytest.mark.parametrize("empty_table", [False, True]) + @pytest.mark.parametrize("level", ["none", "minimal", "full"]) + def test_build_postprocess_reports_skip_at_every_level(self, tmp_path, level, empty_table): + graph = _graph_without_vectors(tmp_path, empty_table=empty_table) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result: dict = {} + warnings = _run_postprocess( + graph, + result, + level, + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert result["embeddings_refresh_skipped"] is True + assert "embeddings_refreshed" not in result + assert REFRESH_SKIPPED_WARNING in warnings + finally: + graph.close() + + @pytest.mark.parametrize("empty_table", [False, True]) + def test_shared_postprocessing_reports_skip(self, tmp_path, empty_table): + graph = _graph_without_vectors(tmp_path, empty_table=empty_table) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = run_post_processing( + graph, + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert result["embeddings_refresh_skipped"] is True + assert "embeddings_refreshed" not in result + assert REFRESH_SKIPPED_WARNING in result["warnings"] + finally: + graph.close() + + def test_run_postprocess_reports_skip(self, tmp_path): + graph = _graph_without_vectors(tmp_path, empty_table=False) + db_path = graph.db_path + graph.close() + with ( + patch("code_review_graph.embeddings.get_provider") as get_provider, + patch( + "code_review_graph.tools.build._get_store", + side_effect=lambda root: (GraphStore(db_path), tmp_path), + ), + ): + result = run_postprocess( + repo_root=str(tmp_path), + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert result["embeddings_refresh_skipped"] is True + assert REFRESH_SKIPPED_WARNING in result["warnings"] + + def test_build_result_tells_the_caller_nothing_was_embedded(self, tmp_path, monkeypatch): + repo = _git_repo(tmp_path, monkeypatch) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph( + full_rebuild=True, + repo_root=str(repo), + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert result["status"] == "ok" + assert result["embeddings_refresh_skipped"] is True + assert "embeddings_refreshed" not in result + assert REFRESH_SKIPPED_WARNING in result["warnings"] + + def test_update_with_no_changes_still_reports_the_missing_index(self, tmp_path, monkeypatch): + repo = _git_repo(tmp_path, monkeypatch) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + default = build_or_update_graph(repo_root=str(repo)) + explicit = build_or_update_graph( + repo_root=str(repo), + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert default["build_type"] == explicit["build_type"] == "incremental" + assert default["files_updated"] == explicit["files_updated"] == 0 + assert "embeddings_refresh_skipped" not in default + assert "warnings" not in default + assert explicit["status"] == "ok" + assert explicit["embeddings_refresh_skipped"] is True + assert explicit["warnings"] == [REFRESH_SKIPPED_WARNING] + + def test_update_with_no_changes_on_an_embedded_graph_reports_nothing( + self, tmp_path, monkeypatch, + ): + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + graph = GraphStore(get_db_path(repo)) + try: + with patch( + "code_review_graph.embeddings.get_provider", + return_value=_StubProvider(), + ): + with EmbeddingStore(graph.db_path, provider="local", model="test-model") as store: + assert embed_all_nodes(graph, store) > 0 + finally: + graph.close() + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph( + repo_root=str(repo), + embedding_provider="local", + embedding_model="test-model", + ) + get_provider.assert_not_called() + assert result["files_updated"] == 0 + assert "embeddings_refresh_skipped" not in result + assert "warnings" not in result + + class TestRefreshWiring: def test_shared_postprocessing_is_default_off(self, tmp_path): graph, _ = _graph_with_function(tmp_path) @@ -319,8 +482,10 @@ def test_shared_postprocessing_is_default_off(self, tmp_path): with patch( "code_review_graph.embeddings.refresh_embeddings", ) as refresh: - run_post_processing(graph) + result = run_post_processing(graph) refresh.assert_not_called() + assert "embeddings_refresh_skipped" not in result + assert "warnings" not in result finally: graph.close() @@ -343,6 +508,7 @@ def test_shared_postprocessing_refresh_is_explicit_and_fail_soft(self, tmp_path) ) assert result["embeddings_refreshed"] == 3 assert result["embeddings_purged"] == 2 + assert "embeddings_refresh_skipped" not in result with patch( "code_review_graph.embeddings.refresh_embeddings", @@ -365,8 +531,9 @@ def test_build_postprocess_is_default_off_and_explicit_at_every_level(self, tmp_ return_value={"embedded": 1, "purged": 1}, ) as refresh: default_result: dict = {} - _run_postprocess(graph, default_result, "none") + assert _run_postprocess(graph, default_result, "none") == [] refresh.assert_not_called() + assert "embeddings_refresh_skipped" not in default_result explicit_result: dict = {} _run_postprocess( @@ -383,6 +550,7 @@ def test_build_postprocess_is_default_off_and_explicit_at_every_level(self, tmp_ ) assert explicit_result["embeddings_refreshed"] == 1 assert explicit_result["embeddings_purged"] == 1 + assert "embeddings_refresh_skipped" not in explicit_result finally: graph.close() diff --git a/tests/test_embedding_refresh_skip_notice.py b/tests/test_embedding_refresh_skip_notice.py new file mode 100644 index 000000000..0b0f4757c --- /dev/null +++ b/tests/test_embedding_refresh_skip_notice.py @@ -0,0 +1,559 @@ +"""An explicit refresh on a graph with no index says so, on every path (#1071). + +#1071 made an explicit embedding refresh (provider and model both passed) on a +never-embedded graph report ``embeddings_refresh_skipped`` and +``REFRESH_SKIPPED_WARNING`` instead of a silent ``ok``. These cases sit next +to the PR's own tests in ``test_embedding_refresh.py`` and +``test_watch_refresh_without_index.py``: an incremental update that does +re-parse files, a repository path with spaces and non-ASCII characters, +repeated requests, the interrupted-build repair, the MCP tools and the CLI +driven for real, an index written by an earlier release, and the full cycle +from skip to embed to refresh. Each one also checks that no provider is +resolved, which is the #653 guarantee the skip exists to keep. +""" + +from __future__ import annotations + +import asyncio +import logging +import sqlite3 +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +from code_review_graph.build_state import BUILD_IN_PROGRESS, BUILD_STATE_KEY +from code_review_graph.embeddings import ( + REFRESH_SKIPPED_WARNING, + EmbeddingStore, + embed_all_nodes, + has_embeddings, +) +from code_review_graph.graph import GraphStore +from code_review_graph.incremental import _raise_watch_postprocess_warnings, get_db_path +from code_review_graph.postprocessing import run_post_processing +from code_review_graph.tools.build import build_or_update_graph, run_postprocess + +PAIR = {"embedding_provider": "local", "embedding_model": "test-model"} + + +class _StubProvider: + dimension = 2 + + def __init__(self, name: str = "local:test-model") -> None: + self.name = name + + def embed(self, texts): + return [[float(len(text)), 1.0] for text in texts] + + def embed_query(self, text): + return [1.0, 0.0] + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + ["git", "-c", "user.email=t@example.invalid", "-c", "user.name=T", *args], + cwd=repo, + check=True, + capture_output=True, + ) + + +def _git_repo(tmp_path: Path, monkeypatch, name: str = "repo") -> Path: + """A committed one-file git repository, parsed serially.""" + monkeypatch.setenv("CRG_SERIAL_PARSE", "1") + repo = tmp_path / name + repo.mkdir() + _git(repo, "init", "-q") + (repo / "app.py").write_text("def hello():\n return 1\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "init") + return repo + + +def _commit_change(repo: Path) -> None: + (repo / "app.py").write_text( + "def hello():\n return 2\n\n\ndef bye():\n return hello()\n", + ) + _git(repo, "commit", "-qam", "change") + + +def _embeddings_table_exists(repo: Path) -> bool: + conn = sqlite3.connect(get_db_path(repo)) + try: + row = conn.execute( + "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'embeddings'", + ).fetchone() + finally: + conn.close() + return row is not None + + +def _embed(repo: Path) -> int: + """Create a real index with a stub provider, the way ``embed`` would.""" + graph = GraphStore(get_db_path(repo)) + try: + with patch( + "code_review_graph.embeddings.get_provider", + return_value=_StubProvider(), + ): + with EmbeddingStore(graph.db_path, provider="local", model="test-model") as store: + return embed_all_nodes(graph, store) + finally: + graph.close() + + +def _assert_skip_reported(result: dict) -> None: + assert result["status"] == "ok" + assert result["embeddings_refresh_skipped"] is True + assert "embeddings_refreshed" not in result + assert result["warnings"] == [REFRESH_SKIPPED_WARNING] + + +class TestBuildPaths: + @pytest.mark.parametrize("repo_name", ["repo", "my repo üñî"]) + def test_incremental_update_that_reparses_files_reports_the_skip( + self, tmp_path, monkeypatch, repo_name, + ): + repo = _git_repo(tmp_path, monkeypatch, repo_name) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + _commit_change(repo) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph(repo_root=str(repo), **PAIR) + get_provider.assert_not_called() + assert result["build_type"] == "incremental" + assert result["files_updated"] >= 1 + _assert_skip_reported(result) + + def test_repeated_requests_report_every_time_and_never_create_the_index( + self, tmp_path, monkeypatch, + ): + repo = _git_repo(tmp_path, monkeypatch) + with patch("code_review_graph.embeddings.get_provider") as get_provider: + results = [ + build_or_update_graph(full_rebuild=True, repo_root=str(repo), **PAIR), + build_or_update_graph(full_rebuild=True, repo_root=str(repo), **PAIR), + build_or_update_graph(repo_root=str(repo), **PAIR), + build_or_update_graph(repo_root=str(repo), **PAIR), + ] + get_provider.assert_not_called() + for result in results: + _assert_skip_reported(result) + # The check reads the schema; it must not leave an empty table behind + # that a later reader would mistake for an initialised index. + assert not _embeddings_table_exists(repo) + + @pytest.mark.parametrize("postprocess", ["none", "minimal", "full"]) + def test_no_change_update_reports_the_skip_at_every_level( + self, tmp_path, monkeypatch, postprocess, + ): + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph( + repo_root=str(repo), postprocess=postprocess, **PAIR, + ) + get_provider.assert_not_called() + assert result["files_updated"] == 0 + _assert_skip_reported(result) + + def test_interrupted_build_repair_reports_the_skip(self, tmp_path, monkeypatch): + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + graph = GraphStore(get_db_path(repo)) + try: + graph.set_metadata(BUILD_STATE_KEY, BUILD_IN_PROGRESS) + finally: + graph.close() + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph(repo_root=str(repo), **PAIR) + get_provider.assert_not_called() + assert result["build_type"] == "full" + assert result["repaired_interrupted_build"] is True + _assert_skip_reported(result) + + def test_postprocess_command_keeps_ok_status_with_an_empty_table( + self, tmp_path, monkeypatch, + ): + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + with patch("code_review_graph.embeddings.get_provider", return_value=None): + EmbeddingStore(get_db_path(repo)).close() + assert _embeddings_table_exists(repo) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = run_postprocess(repo_root=str(repo), **PAIR) + get_provider.assert_not_called() + assert result["summary"] == "Post-processing complete." + _assert_skip_reported(result) + + +class TestFromSkipToRefresh: + def test_skip_then_embed_then_refresh(self, tmp_path, monkeypatch): + repo = _git_repo(tmp_path, monkeypatch) + with patch("code_review_graph.embeddings.get_provider") as get_provider: + first = build_or_update_graph(full_rebuild=True, repo_root=str(repo), **PAIR) + get_provider.assert_not_called() + _assert_skip_reported(first) + + assert _embed(repo) > 0 + + _commit_change(repo) + with patch( + "code_review_graph.embeddings.get_provider", + return_value=_StubProvider(), + ): + refreshed = build_or_update_graph(repo_root=str(repo), **PAIR) + noop = build_or_update_graph(repo_root=str(repo), **PAIR) + assert refreshed["status"] == "ok" + assert refreshed["embeddings_refreshed"] >= 1 + assert "embeddings_refresh_skipped" not in refreshed + assert "warnings" not in refreshed + assert noop["files_updated"] == 0 + assert "embeddings_refresh_skipped" not in noop + assert "warnings" not in noop + + +class TestHasEmbeddings: + def _graph(self, tmp_path: Path) -> GraphStore: + return GraphStore(tmp_path / "graph.db") + + def test_no_table_is_false_and_creates_nothing(self, tmp_path): + graph = self._graph(tmp_path) + try: + assert has_embeddings(graph) is False + assert has_embeddings(graph) is False + assert graph._conn.execute( + "SELECT 1 FROM sqlite_master WHERE name = 'embeddings'", + ).fetchone() is None + finally: + graph.close() + + def test_empty_table_is_false(self, tmp_path): + graph = self._graph(tmp_path) + try: + with patch("code_review_graph.embeddings.get_provider", return_value=None): + EmbeddingStore(graph.db_path).close() + assert has_embeddings(graph) is False + finally: + graph.close() + + @pytest.mark.parametrize("legacy", [False, True]) + def test_any_stored_row_is_true(self, tmp_path, legacy): + graph = self._graph(tmp_path) + try: + if legacy: + # The shape earlier releases wrote: no provider column. + graph._conn.executescript( + "CREATE TABLE embeddings (qualified_name TEXT PRIMARY KEY, " + "vector BLOB NOT NULL, text_hash TEXT NOT NULL);", + ) + graph._conn.execute( + "INSERT INTO embeddings VALUES (?, ?, ?)", ("m.py::f", b"\x00" * 8, "h"), + ) + graph.commit() + else: + with patch("code_review_graph.embeddings.get_provider", return_value=None): + store = EmbeddingStore(graph.db_path) + store._conn.execute( + "INSERT INTO embeddings (qualified_name, vector, text_hash, provider) " + "VALUES (?, ?, ?, ?)", + ("m.py::f", b"\x00" * 8, "h", "local:test-model"), + ) + store._conn.commit() + store.close() + assert has_embeddings(graph) is True + finally: + graph.close() + + +class TestIndexFromAnEarlierRelease: + """An index written before provider identity existed is not a skip.""" + + def _legacy_graph(self, tmp_path: Path, *, rows: bool) -> GraphStore: + graph = GraphStore(tmp_path / "graph.db") + graph._conn.executescript( + "CREATE TABLE embeddings (qualified_name TEXT PRIMARY KEY, " + "vector BLOB NOT NULL, text_hash TEXT NOT NULL);", + ) + if rows: + graph._conn.execute( + "INSERT INTO embeddings VALUES (?, ?, ?)", ("m.py::f", b"\x00" * 8, "h"), + ) + graph.commit() + return graph + + def test_legacy_rows_fail_the_refresh_and_still_stop_watch(self, tmp_path): + graph = self._legacy_graph(tmp_path, rows=True) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = run_post_processing(graph, **PAIR) + get_provider.assert_not_called() + assert "embeddings_refresh_skipped" not in result + assert REFRESH_SKIPPED_WARNING not in result["warnings"] + assert any("provider identity" in w for w in result["warnings"]) + with pytest.raises(RuntimeError, match="provider identity"): + _raise_watch_postprocess_warnings(result) + finally: + graph.close() + + def test_empty_legacy_table_is_a_skip(self, tmp_path): + graph = self._legacy_graph(tmp_path, rows=False) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = run_post_processing(graph, **PAIR) + get_provider.assert_not_called() + assert result["embeddings_refresh_skipped"] is True + assert result["warnings"] == [REFRESH_SKIPPED_WARNING] + _raise_watch_postprocess_warnings(result) + finally: + graph.close() + + +class TestWatchExemption: + @pytest.mark.parametrize( + "result", + [ + {"warnings": [REFRESH_SKIPPED_WARNING, REFRESH_SKIPPED_WARNING]}, + {"warnings": []}, + {}, + None, + "not a dict", + ], + ids=["notice-twice", "empty-list", "no-key", "none", "non-dict"], + ) + def test_results_without_a_real_failure_keep_watch_running(self, result): + _raise_watch_postprocess_warnings(result) + + @pytest.mark.parametrize( + "warning", + [ + "Embedding refresh requires both an explicit provider and model.", + REFRESH_SKIPPED_WARNING + " ", + REFRESH_SKIPPED_WARNING.upper(), + ], + ids=["half-pair", "trailing-space", "different-case"], + ) + def test_only_the_exact_notice_is_exempt(self, warning): + with pytest.raises(RuntimeError, match="post-processing reported warnings"): + _raise_watch_postprocess_warnings({"warnings": [warning]}) + + +class TestRealEntryPoints: + def test_mcp_tools_return_the_skip(self, tmp_path, monkeypatch): + from code_review_graph import main as crg_main + + build_tool = getattr( + crg_main.build_or_update_graph_tool, "fn", crg_main.build_or_update_graph_tool, + ) + postprocess_tool = getattr( + crg_main.run_postprocess_tool, "fn", crg_main.run_postprocess_tool, + ) + repo = _git_repo(tmp_path, monkeypatch) + with patch("code_review_graph.embeddings.get_provider") as get_provider: + built = asyncio.run(build_tool(full_rebuild=True, repo_root=str(repo), **PAIR)) + noop = asyncio.run(build_tool(repo_root=str(repo), **PAIR)) + post = asyncio.run(postprocess_tool(repo_root=str(repo), **PAIR)) + default = asyncio.run(build_tool(repo_root=str(repo))) + get_provider.assert_not_called() + for result in (built, noop, post): + _assert_skip_reported(result) + assert "embeddings_refresh_skipped" not in default + assert "warnings" not in default + + @pytest.mark.parametrize( + "command", + [["build"], ["update"], ["postprocess"], ["build", "--quiet"]], + ids=["build", "noop-update", "postprocess", "quiet-build"], + ) + def test_cli_exits_cleanly_and_logs_the_notice( + self, tmp_path, monkeypatch, caplog, command, + ): + from code_review_graph import cli + + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + argv = [ + "code-review-graph", + *command, + "--repo", + str(repo), + "--embedding-provider", + "local", + "--embedding-model", + "test-model", + ] + caplog.set_level(logging.WARNING) + with ( + patch.object(sys, "argv", argv), + patch("code_review_graph.embeddings.get_provider") as get_provider, + ): + cli.main() + get_provider.assert_not_called() + notices = [r for r in caplog.records if r.getMessage() == REFRESH_SKIPPED_WARNING] + assert len(notices) == 1 + + def test_notice_names_a_real_command_and_tool(self): + from code_review_graph import cli + from code_review_graph import main as crg_main + + assert "`code-review-graph embed`" in REFRESH_SKIPPED_WARNING + assert "embed_graph_tool" in REFRESH_SKIPPED_WARNING + assert hasattr(crg_main, "embed_graph_tool") + args = cli.build_parser().parse_args(["embed"]) + assert args.command == "embed" + + +def _notices(caplog) -> list[logging.LogRecord]: + return [r for r in caplog.records if r.getMessage() == REFRESH_SKIPPED_WARNING] + + +class TestWatchTellsTheUser: + """Watch drops the notice from its failure check, so the log is all the user sees.""" + + def test_every_watch_update_logs_the_notice_once_and_keeps_running( + self, tmp_path, caplog, + ): + from functools import partial + + from watchdog.events import FileCreatedEvent + + from code_review_graph.incremental import _create_watch_handler + + repo = tmp_path / "repo" + repo.mkdir() + store = GraphStore(tmp_path / "graph.db") + # The exact callback ``code-review-graph watch`` builds for the pair. + handler = _create_watch_handler(repo, store, partial(run_post_processing, **PAIR)) + caplog.set_level(logging.WARNING) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + for name in ("first.py", "second.py"): + source = repo / name + source.write_text(f"def {source.stem}():\n pass\n") + caplog.clear() + handler.process([FileCreatedEvent(str(source))]) + handler.raise_if_failed() + assert len(_notices(caplog)) == 1, name + get_provider.assert_not_called() + assert len(store.get_all_files()) == 2 + assert not has_embeddings(store) + finally: + store.close() + + def test_cli_watch_with_the_pair_keeps_running_on_a_never_embedded_graph( + self, tmp_path, monkeypatch, caplog, + ): + from code_review_graph import cli + + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + # Changed while no watcher ran: startup reconciliation re-parses it and + # hands the store to the CLI's post-processing callback. + (repo / "app.py").unlink() + argv = [ + "code-review-graph", "watch", "--repo", str(repo), + "--embedding-provider", "local", "--embedding-model", "test-model", + ] + caplog.set_level(logging.WARNING) + with ( + patch.object(sys, "argv", argv), + patch("code_review_graph.embeddings.get_provider") as get_provider, + patch("watchdog.observers.Observer") as observer, + patch("time.sleep", side_effect=KeyboardInterrupt), + ): + cli.main() # a fatal post-processing warning would sys.exit(1) + get_provider.assert_not_called() + observer.return_value.start.assert_called_once() + assert len(_notices(caplog)) == 1 + assert not _embeddings_table_exists(repo) + + +class TestNoChangeUpdateEdgeCases: + def test_an_empty_table_left_by_graph_stats_is_still_a_skip(self, tmp_path, monkeypatch): + """``list_graph_stats`` opens an EmbeddingStore, which creates an empty table.""" + from code_review_graph.tools.query import list_graph_stats + + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + with patch("code_review_graph.embeddings.get_provider", return_value=None): + stats = list_graph_stats(repo_root=str(repo)) + assert stats["status"] == "ok" + assert _embeddings_table_exists(repo) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph(repo_root=str(repo), **PAIR) + get_provider.assert_not_called() + assert result["files_updated"] == 0 + _assert_skip_reported(result) + + @pytest.mark.parametrize("changed", [False, True], ids=["no-op", "reparse"]) + @pytest.mark.parametrize( + "half", + [{"embedding_provider": "local"}, {"embedding_model": "test-model"}], + ids=["provider-only", "model-only"], + ) + def test_half_a_pair_is_never_reported_as_a_skip( + self, tmp_path, monkeypatch, changed, half, + ): + """MCP callers can send half a pair; the CLI rejects it before this point.""" + repo = _git_repo(tmp_path, monkeypatch) + build_or_update_graph(full_rebuild=True, repo_root=str(repo)) + if changed: + _commit_change(repo) + + with patch("code_review_graph.embeddings.get_provider") as get_provider: + result = build_or_update_graph(repo_root=str(repo), **half) + get_provider.assert_not_called() + assert result["status"] == "ok" + assert "embeddings_refresh_skipped" not in result + assert REFRESH_SKIPPED_WARNING not in result.get("warnings", []) + + +class TestCliProcess: + def test_build_prints_the_notice_once_and_exits_zero(self, tmp_path, monkeypatch): + """The user-facing stream, from a real process, with no provider mocked.""" + import os + + repo = _git_repo(tmp_path, monkeypatch) + home = tmp_path / "home" + home.mkdir() + env = { + **os.environ, + "HOME": str(home), + "USERPROFILE": str(home), + "XDG_CONFIG_HOME": str(home / ".config"), + "XDG_DATA_HOME": str(home / ".local" / "share"), + "XDG_CACHE_HOME": str(home / ".cache"), + "APPDATA": str(home / "AppData"), + "CRG_HOME": str(home / ".code-review-graph"), + "HF_HUB_OFFLINE": "1", + "TRANSFORMERS_OFFLINE": "1", + "CRG_SERIAL_PARSE": "1", + } + script = ( + "import sys\n" + "from code_review_graph.cli import main\n" + "try:\n" + " main()\n" + "finally:\n" + " print('LOADED', 'sentence_transformers' in sys.modules, file=sys.stderr)\n" + ) + proc = subprocess.run( + [ + sys.executable, "-c", script, "build", "--repo", str(repo), + "--embedding-provider", "local", "--embedding-model", "all-MiniLM-L6-v2", + ], + capture_output=True, text=True, encoding="utf-8", env=env, timeout=120, + check=False, + ) + assert proc.returncode == 0, proc.stderr + assert proc.stderr.count(REFRESH_SKIPPED_WARNING) == 1, proc.stderr + assert "LOADED False" in proc.stderr + assert "Full build:" in proc.stdout + assert not _embeddings_table_exists(repo) diff --git a/tests/test_header_dead_branch_scan.py b/tests/test_header_dead_branch_scan.py new file mode 100644 index 000000000..f8488ed29 --- /dev/null +++ b/tests/test_header_dead_branch_scan.py @@ -0,0 +1,686 @@ +"""Header classification and dead-guard checks over long preprocessor chains. + +Regression coverage for PR #1099 (issue #977). A ``.h`` file is classified as +C or C++ by ``CodeParser._has_cpp_header_evidence``. It used to call +``_is_in_static_dead_guard`` for every syntax node, and each call walked the +whole ancestor chain. tree-sitter nests an ``#if`` / ``#elif`` chain as a +right-leaning spine, so on generated AUTOSAR MemMap headers the scan was +quadratic in the number of branches and a single file took minutes. + +The scan now carries the dead-branch state down the tree and checks each +parent-child edge once, and ``_is_in_static_dead_guard`` checks each edge of +the ancestor chain once. These tests pin four things: the scan does no +per-node ancestor walk, it makes one Python-level edge check per node, the +#977 header stays inside a wall-clock budget, and every ``#if 0`` / +``#elif 0`` / ``#else`` / ``if false`` decision is the same as before the +change. + +The scan still reads ``node.prev_named_sibling`` once per node, which +tree-sitter resolves from the tree root, so total cost is not yet linear in +the chain length. The budget below is set to catch a Python-level per-node +ancestor walk (seconds at this size), not that remaining native cost. +""" + +import time +from pathlib import Path + +import pytest + +from code_review_graph import parser as parser_module +from code_review_graph.graph import GraphStore +from code_review_graph.incremental import full_build, incremental_update +from code_review_graph.parser import ( + CodeParser, + _is_in_static_dead_guard, + _is_statically_false_condition, + _node_is_in_child, +) + +FIXTURES = Path(__file__).parent / "fixtures" + + +def _issue_977_memmap_header(sections: int, newline: str = "\n") -> bytes: + """Return the synthetic MemMap header produced by the generator in #977.""" + module = "SynMod" + lines = [ + "/* synthetic MemMap-style section-mapping header */", + "#if defined (START_WITH_IF)", + ] + for index in range(sections): + for kind, pragma in ( + ("START", f'#pragma section bss=".module_section_{index}"'), + ("STOP", "#pragma section"), + ): + macro = f"{module}_{kind}_SEC_M{index}" + lines.append(f"#elif defined {macro}") + lines.append(f" #undef {macro}") + lines.append(f" {pragma}") + lines.append("") + lines.append("#endif") + return newline.join(lines).encode() + + +def _file_language(nodes) -> str: + return next(node.language for node in nodes if node.kind == "File") + + +def _forbid_ancestor_walk(monkeypatch) -> None: + def unexpected_ancestor_walk(node) -> bool: + raise AssertionError("header evidence scan walked a node's ancestors") + + monkeypatch.setattr( + parser_module, "_is_in_static_dead_guard", unexpected_ancestor_walk, + ) + + +def _named_nodes(root) -> list: + found = [] + pending = [root] + while pending: + node = pending.pop() + found.append(node) + pending.extend(node.named_children) + return found + + +@pytest.mark.parametrize("newline", ["\n", "\r\n"], ids=["lf", "crlf"]) +def test_issue_977_memmap_header_parses_without_per_node_ancestor_walk( + tmp_path: Path, monkeypatch, newline: str, +) -> None: + source = _issue_977_memmap_header(200, newline) + _forbid_ancestor_walk(monkeypatch) + + nodes, edges = CodeParser().parse_bytes(tmp_path / "memmap.h", source) + + assert [node.kind for node in nodes] == ["File"] + assert _file_language(nodes) == "c" + assert edges == [] + + +def test_header_scan_checks_each_parent_child_edge_once(monkeypatch) -> None: + tree_parser = CodeParser()._get_parser("cpp") + assert tree_parser is not None + tree = tree_parser.parse(_issue_977_memmap_header(150)) + named_node_count = len(_named_nodes(tree.root_node)) + + calls = [] + edge_check = parser_module._is_static_dead_guard_child + + def counting_edge_check(parent, child) -> bool: + calls.append((parent.type, child.type)) + return edge_check(parent, child) + + monkeypatch.setattr( + parser_module, "_is_static_dead_guard_child", counting_edge_check, + ) + assert CodeParser._has_cpp_header_evidence(tree.root_node) is False + # One Python-level check per child edge of a visited node: never more than + # the tree has nodes, however deep the #elif spine is. This counts only the + # edge checks; the wall-clock budget test below covers total cost. + assert 0 < len(calls) < named_node_count + + +def test_issue_977_header_scan_stays_inside_wall_clock_budget() -> None: + """The 400-branch #977 header must classify in well under a second. + + With a per-node ancestor walk this scan took several seconds at this size + (about 14 seconds for a full parse before #1099); without it, it takes a + few hundredths of a second. The budget leaves wide room for slow CI. + """ + tree_parser = CodeParser()._get_parser("cpp") + assert tree_parser is not None + root = tree_parser.parse(_issue_977_memmap_header(200)).root_node + + best = float("inf") + for _ in range(3): + start = time.perf_counter() + assert CodeParser._has_cpp_header_evidence(root) is False + best = min(best, time.perf_counter() - start) + + assert best < 2.0, f"header evidence scan took {best:.3f}s for 400 branches" + + +@pytest.mark.parametrize( + ("source", "language"), + [ + pytest.param( + "#if defined(X)\ntypedef int a;\n#elif 0\nclass Dead {};\n#endif\n", + "c", + id="elif-0-branch-is-dead", + ), + pytest.param( + "#if 0\ntypedef int a;\n#else\nclass Live {};\n#endif\n", + "cpp", + id="else-of-if-0-is-live", + ), + pytest.param( + "#if 0\ntypedef int a;\n#elif 1\nclass Live {};\n#endif\n", + "cpp", + id="elif-after-if-0-is-live", + ), + pytest.param( + "#if defined(X)\ntypedef int a;\n#elif 0\ntypedef int b;\n" + "#else\nnamespace live {}\n#endif\n", + "cpp", + id="else-of-elif-0-is-live", + ), + pytest.param( + "#if 0\n#if 1\nclass Dead {};\n#endif\n#endif\ntypedef int v;\n", + "c", + id="live-guard-nested-in-if-0-is-dead", + ), + pytest.param( + "#if 0\n#if 0\n#else\nclass Dead {};\n#endif\n#endif\ntypedef int v;\n", + "c", + id="else-nested-in-if-0-is-dead", + ), + pytest.param( + "#if 1\n#if 0\nclass Dead {};\n#endif\ntypedef int v;\n#endif\n", + "c", + id="if-0-nested-in-live-guard-is-dead", + ), + pytest.param( + "#if 0\ntypedef int dead;\n#endif\nclass Live {};\n", + "cpp", + id="dead-state-does-not-leak-to-next-sibling", + ), + pytest.param( + "class Live {};\n#if 0\ntypedef int dead;\n#endif\n", + "cpp", + id="evidence-before-dead-block", + ), + pytest.param( + "/* ünïçødé 日本 */\n#if 0 /* ünï */\n" + "class Dead {};\n#endif\ntypedef int v; /* 日本 */\n", + "c", + id="unicode-comments-dead", + ), + pytest.param( + "/* ünïçødé 日本 */\n#if 0 /* ünï */\n" + "class Dead {};\n#else /* 日本 */\nclass Live {};\n#endif\n", + "cpp", + id="unicode-comments-live-else", + ), + pytest.param( + "#if 0\r\nclass Dead {};\r\n#endif\r\ntypedef int v;\r\n", + "c", + id="crlf-dead", + ), + pytest.param( + "#if 0\r\ntypedef int a;\r\n#else\r\nclass Live {};\r\n#endif\r\n", + "cpp", + id="crlf-live-else", + ), + pytest.param( + "void f(void) {\n#if 0\nauto g = [](){};\n#endif\n}\n", + "c", + id="dead-lambda-inside-function-body", + ), + pytest.param("", "c", id="empty"), + pytest.param("\n\n \n", "c", id="whitespace-only"), + pytest.param("/* only a comment */\n", "c", id="comment-only"), + ], +) +def test_header_language_honours_dead_preprocessor_branches( + tmp_path: Path, source: str, language: str, +) -> None: + nodes, _ = CodeParser().parse_bytes(tmp_path / "guarded.h", source.encode()) + + assert _file_language(nodes) == language + + +def _deep_chain(head: str, branch: str, tail: str, branches: int) -> bytes: + body = "".join(branch.format(index=index) for index in range(branches)) + return (head + body + tail).encode() + + +@pytest.mark.parametrize( + ("source", "language"), + [ + pytest.param( + _deep_chain( + "#if 0\n", + "#elif 0\ntypedef int dead_{index};\n", + "#else\nclass Live {};\n#endif\n", + 150, + ), + "cpp", + id="else-under-long-dead-elif-chain-is-live", + ), + pytest.param( + _deep_chain( + "#if defined(A)\n", + "#elif defined(X{index})\n#undef X{index}\n", + "#elif 0\nclass Dead {};\n#endif\n", + 150, + ), + "c", + id="elif-0-at-end-of-long-chain-is-dead", + ), + pytest.param( + _deep_chain( + "#if defined(A)\n", + "#elif defined(X{index})\n#undef X{index}\n", + "#elif defined(LAST)\nclass Live {};\n#endif\n", + 150, + ), + "cpp", + id="live-branch-at-end-of-long-chain", + ), + ], +) +def test_long_chain_classification_reaches_the_deepest_branch( + tmp_path: Path, monkeypatch, source: bytes, language: str, +) -> None: + _forbid_ancestor_walk(monkeypatch) + + nodes, _ = CodeParser().parse_bytes(tmp_path / "chain.h", source) + + assert _file_language(nodes) == language + + +def _reference_is_in_static_dead_guard(node) -> bool: + """The two-walk algorithm ``_is_in_static_dead_guard`` used before #1099.""" + cursor = node.parent + while cursor is not None: + if cursor.type == "if_statement": + condition = cursor.child_by_field_name("condition") + consequence = cursor.child_by_field_name("consequence") + if ( + condition is not None + and consequence is not None + and _is_statically_false_condition(condition) + and _node_is_in_child(node, consequence) + ): + return True + cursor = cursor.parent + + preproc = node.parent + while preproc is not None: + if preproc.type in ("preproc_if", "preproc_elif"): + condition = preproc.child_by_field_name("condition") + if ( + condition is not None + and condition.type == "number_literal" + and condition.text == b"0" + ): + alternative = preproc.child_by_field_name("alternative") + if not ( + alternative is not None + and _node_is_in_child(node, alternative) + ): + return True + preproc = preproc.parent + return False + + +_EQUIVALENCE_SOURCES = [ + ("go", (FIXTURES / "sample_dead_guard.go").read_bytes(), True), + ("typescript", (FIXTURES / "sample_dead_guard.ts").read_bytes(), True), + ("c", (FIXTURES / "sample_dead_guard.c").read_bytes(), True), + ( + "go", + b"func f() {\n if false { a() } else if false { b() } else { c() }\n" + b" if true { if false { d() } else { e() } }\n}\n", + True, + ), + ( + "typescript", + b"function f() {\n if (false) g(); else h();\n" + b" if (0) { if (true) { x(); } }\n if ((false)) { y(); }\n" + b" if (1) { z(); }\n}\n", + True, + ), + ("javascript", b"if (0) { a(); } else { b(); }\nif (false) c();\n", True), + ( + "c", + b"void f(void) {\n#if 0\n a();\n#elif 0\n b();\n#elif 1\n c();\n" + b"#else\n d();\n#endif\n#if 1\n#if 0\n e();\n#else\n g();\n#endif\n" + b"#endif\n}\n", + True, + ), + ( + "cpp", + _deep_chain( + "#if 0\nclass A {};\n", + "#elif 0\nvoid dead_{index}();\n", + "#else\nclass Live { void m(); };\n#endif\n", + 60, + ), + True, + ), + ("cpp", _issue_977_memmap_header(30), False), +] + + +@pytest.mark.parametrize( + ("language", "source", "has_dead_branch"), + _EQUIVALENCE_SOURCES, + ids=[f"{entry[0]}-{index}" for index, entry in enumerate(_EQUIVALENCE_SOURCES)], +) +def test_single_walk_dead_guard_matches_previous_two_walk_result( + language: str, source: bytes, has_dead_branch: bool, +) -> None: + tree_parser = CodeParser()._get_parser(language) + assert tree_parser is not None + tree = tree_parser.parse(source) + + nodes = _named_nodes(tree.root_node) + dead = [node for node in nodes if _reference_is_in_static_dead_guard(node)] + mismatches = [ + (node.type, node.start_point, node.text[:40]) + for node in nodes + if _is_in_static_dead_guard(node) != _reference_is_in_static_dead_guard(node) + ] + + assert mismatches == [] + assert bool(dead) is has_dead_branch + + +def test_cpp_callable_declarations_under_if_0_stay_unindexed(tmp_path: Path) -> None: + source = b"""#if 0 +void dead_decl(int); +#else +void live_decl(int); +#endif +class Widget { +#if 0 + void dead_member(); +#endif + void live_member(); +}; +""" + nodes, _ = CodeParser().parse_bytes(tmp_path / "Widget.hpp", source) + + functions = sorted(node.name for node in nodes if node.kind == "Function") + assert functions == ["live_decl", "live_member"] + + +def test_c_calls_in_dead_elif_chain_are_dropped(tmp_path: Path) -> None: + source = b"""void live(void); +void dead0(void); +void dead1(void); +void caller(void) { +#if 0 + dead0(); +#elif 0 + dead1(); +#else + live(); +#endif +} +""" + _, edges = CodeParser().parse_bytes(tmp_path / "caller.c", source) + + targets = sorted(edge.target.split("::")[-1] for edge in edges if edge.kind == "CALLS") + assert targets == ["live"] + + +def test_memmap_header_build_and_update_flip_language_with_live_evidence( + tmp_path: Path, +) -> None: + header = tmp_path / "memmap.h" + header.write_bytes(_issue_977_memmap_header(60)) + (tmp_path / "main.c").write_text( + "int helper(void);\nint use(void) { return helper(); }\n", + encoding="utf-8", + ) + + with GraphStore(tmp_path / "graph.db") as store: + result = full_build(tmp_path, store) + assert result["errors"] == [] + assert result["files_parsed"] == 2 + stored = store.get_nodes_by_file(str(header)) + assert [node.kind for node in stored] == ["File"] + assert stored[0].language == "c" + + header.write_bytes( + _issue_977_memmap_header(60).replace( + b"#endif", b"#else\nclass Live {};\n#endif", + ) + ) + updated = incremental_update(tmp_path, store, changed_files=["memmap.h"]) + assert updated["errors"] == [] + stored = store.get_nodes_by_file(str(header)) + assert {node.language for node in stored} == {"cpp"} + assert any(node.kind == "Class" and node.name == "Live" for node in stored) + + # Back to the plain header, twice: the second update must be a no-op. + header.write_bytes(_issue_977_memmap_header(60)) + for _ in range(2): + reverted = incremental_update(tmp_path, store, changed_files=["memmap.h"]) + assert reverted["errors"] == [] + stored = store.get_nodes_by_file(str(header)) + assert [node.kind for node in stored] == ["File"] + assert stored[0].language == "c" + + +# --------------------------------------------------------------------------- +# Audit additions for PR #1099. +# --------------------------------------------------------------------------- + + +def _reference_has_cpp_header_evidence(root) -> bool: + """``_has_cpp_header_evidence`` as it was before #1099: per-node walk.""" + pending = [root] + while pending: + node = pending.pop() + if node.type == "ERROR" or _reference_is_in_static_dead_guard(node): + continue + previous = node.prev_named_sibling + if ( + previous is not None + and previous.type == "ERROR" + and previous.end_byte == node.start_byte + ): + continue + if node.type in parser_module._CPP_HEADER_EVIDENCE_TYPES: + return True + if node.type == "enum_specifier" and any( + child.type in ("class", "struct") for child in node.children + ): + return True + if ( + node.type == "type_qualifier" + and node.text in parser_module._CPP_HEADER_EVIDENCE_QUALIFIERS + ): + return True + pending.extend(node.named_children) + return False + + +_RANDOM_HEADER_OPENERS = ( + "#if 0", "#if 0", "#if 1", "#if defined(X{n})", "#ifdef X{n}", "#ifndef X{n}", +) +_RANDOM_HEADER_BRANCHES = ( + "#elif 0", "#elif 0", "#elif 1", "#elif defined(Y{n})", "#elifdef Y{n}", "#else", +) +_RANDOM_HEADER_LEAVES = ( + "typedef int t{n};", + "#pragma section", + "#undef X{n}", + "int f{n}(void);", + "class C{n} {{}};", + "namespace n{n} {{}}", + "template struct S{n};", + "enum class E{n} {{ A }};", + "consteval int k{n}();", + "struct P{n} {{\n#if 0\npublic:\n#endif\n int x;\n}};", + "void g{n}(void) {{\n#if 0\n auto l = [](){{}};\n#endif\n if (0) {{ h(); }}\n}}", + "@class Forward{n};", +) + + +def _random_header(rng, depth: int = 0) -> str: + lines = [] + for _ in range(rng.randint(1, 3)): + if depth < 3 and rng.random() < 0.55: + n = rng.randint(0, 99) + lines.append(rng.choice(_RANDOM_HEADER_OPENERS).format(n=n)) + lines.append(_random_header(rng, depth + 1)) + seen_else = False + for _ in range(rng.randint(0, 3)): + branch = rng.choice(_RANDOM_HEADER_BRANCHES).format(n=n) + if seen_else: + break + seen_else = branch == "#else" + lines.append(branch) + lines.append(_random_header(rng, depth + 1)) + lines.append("#endif") + else: + lines.append(rng.choice(_RANDOM_HEADER_LEAVES).format(n=rng.randint(0, 99))) + return "\n".join(lines) + + +@pytest.mark.parametrize("grammar", ["cpp", "c"]) +def test_top_down_scan_matches_per_node_scan_on_random_guard_nesting( + grammar: str, +) -> None: + """The flag carried down the tree must give the old per-node answer. + + ``test_single_walk_dead_guard_matches_previous_two_walk_result`` checks the + ancestor-walk helper, but ``_has_cpp_header_evidence`` no longer calls it: + it carries its own flag. This compares that scan, and the helper on every + node, with the pre-#1099 algorithm over seeded random nestings of + ``#if 0`` / ``#if 1`` / ``#ifdef`` / ``#elif 0`` / ``#elifdef`` / ``#else`` + around C++-only syntax, C syntax, error recovery and ``if (0)``. Production + classifies ``.h`` files with the C++ grammar; the C grammar run covers the + helper on the trees ``.c`` files produce. + """ + import random + + rng = random.Random(1099) + tree_parser = CodeParser()._get_parser(grammar) + assert tree_parser is not None + outcomes = set() + dead_nodes = 0 + for _ in range(200): + source = _random_header(rng) + if rng.random() < 0.3: + source = source.replace("\n", "\r\n") + tree = tree_parser.parse(source.encode()) + expected = _reference_has_cpp_header_evidence(tree.root_node) + assert CodeParser._has_cpp_header_evidence(tree.root_node) is expected, source + outcomes.add(expected) + for node in _named_nodes(tree.root_node): + reference = _reference_is_in_static_dead_guard(node) + dead_nodes += reference + assert _is_in_static_dead_guard(node) is reference, ( + source, node.type, node.start_point, + ) + # The corpus must exercise dead branches and, under the C++ grammar, + # both verdicts, or it proves nothing. + assert dead_nodes > 0 + if grammar == "cpp": + assert outcomes == {True, False} + + +@pytest.mark.parametrize( + ("source", "language"), + [ + pytest.param( + "#ifndef MEMMAP_H\n#define MEMMAP_H\n#if 0\nclass Dead {};\n#endif\n" + "typedef int v;\n#endif\n", + "c", + id="if-0-inside-include-guard-is-dead", + ), + pytest.param( + "#ifndef MEMMAP_H\n#define MEMMAP_H\n#if 0\ntypedef int a;\n#else\n" + "class Live {};\n#endif\n#endif\n", + "cpp", + id="else-of-if-0-inside-include-guard-is-live", + ), + pytest.param( + "#pragma once\n#if 0\nnamespace dead {}\n#endif\nint f(void);\n", + "c", + id="if-0-after-pragma-once-is-dead", + ), + pytest.param( + "#ifdef X\ntypedef int a;\n#elif 0\nclass Dead {};\n#endif\n", + "c", + id="elif-0-under-ifdef-is-dead", + ), + pytest.param( + "#ifndef X\ntypedef int a;\n#elif 0\ntypedef int b;\n#else\n" + "class Live {};\n#endif\n", + "cpp", + id="else-after-elif-0-under-ifndef-is-live", + ), + pytest.param( + "#if 0\n#ifdef X\nclass Dead {};\n#else\nnamespace dead {}\n#endif\n#endif\n" + "typedef int v;\n", + "c", + id="ifdef-and-else-nested-in-if-0-are-dead", + ), + pytest.param( + "#if 0\ntypedef int a;\n#elifdef X\nclass Live {};\n#endif\n", + "cpp", + id="elifdef-after-if-0-is-live", + ), + pytest.param( + "struct S {\n#if 0\npublic:\n#endif\n int x;\n};\n", + "c", + id="dead-access-specifier-inside-struct-body", + ), + pytest.param( + "struct S {\n#if 0\n int y;\n#else\npublic:\n#endif\n int x;\n};\n", + "cpp", + id="live-access-specifier-inside-struct-body", + ), + pytest.param( + "#if 0\nenum class Dead { A };\n#endif\ntypedef int v;\n", + "c", + id="dead-scoped-enum", + ), + pytest.param( + "#if 0\nconsteval int dead();\n#endif\ntypedef int v;\n", + "c", + id="dead-consteval-qualifier", + ), + pytest.param( + "#if defined(A)\ntypedef int a;\n#elif 0\ntemplate struct D;\n" + "#endif\n", + "c", + id="dead-template-under-elif-0", + ), + pytest.param( + "#if 0\nclass Dead {};\n", + "c", + id="unterminated-if-0-is-dead", + ), + pytest.param( + "#if 0\nclass Dead {};\n#else\nclass Live {};\n", + "cpp", + id="unterminated-if-0-else-is-live", + ), + ], +) +def test_header_language_with_include_guards_and_every_evidence_kind( + tmp_path: Path, source: str, language: str, +) -> None: + nodes, _ = CodeParser().parse_bytes(tmp_path / "guarded.h", source.encode()) + + assert _file_language(nodes) == language + + +@pytest.mark.parametrize( + ("tail", "language"), + [ + pytest.param("#endif\n#endif\n", "c", id="dead-free"), + pytest.param("#else\nclass Live {};\n#endif\n#endif\n", "cpp", id="live-at-end"), + pytest.param( + "#elif 0\nclass Dead {};\n#endif\n#endif\n", "c", id="dead-at-end", + ), + ], +) +def test_include_guarded_memmap_chain_is_classified_without_ancestor_walk( + tmp_path: Path, monkeypatch, tail: str, language: str, +) -> None: + """Real MemMap headers sit inside an include guard or ``#pragma once``.""" + chain = _issue_977_memmap_header(150).decode().rsplit("#endif", 1)[0] + source = "#ifndef SYNMOD_MEMMAP_H\n#define SYNMOD_MEMMAP_H\n" + chain + tail + _forbid_ancestor_walk(monkeypatch) + + nodes, _ = CodeParser().parse_bytes(tmp_path / "SynMod_MemMap.h", source.encode()) + + assert _file_language(nodes) == language diff --git a/tests/test_jsonc_removals.py b/tests/test_jsonc_removals.py new file mode 100644 index 000000000..1395dcd91 --- /dev/null +++ b/tests/test_jsonc_removals.py @@ -0,0 +1,374 @@ +"""Strict JSON and comment preservation regressions for issue #1068.""" + +import itertools +import json +from pathlib import Path + +import pytest + +from code_review_graph import jsonc +from code_review_graph.jsonc import remove_paths +from code_review_graph.skills import _strip_jsonc + + +@pytest.mark.parametrize("raw,paths,expected", [ + ('{"keep": 1, "a": 2, "b": 3}', [("a",), ("b",)], {"keep": 1}), + ('{"a": 1, "b": 2, "keep": 3}', [("a",), ("b",)], {"keep": 3}), + ('{"items": [1, 2, 3]}', [("items", 1), ("items", 2)], {"items": [1]}), + ('{"items": [1, 2, 3]}', [("items", 0), ("items", 1)], {"items": [3]}), + ('{"a": 1, "b": 2}', [("a",), ("b",)], {}), +]) +def test_multi_removal_preserves_strict_json(raw, paths, expected): + assert json.loads(remove_paths(raw, paths)) == expected + + +def test_removal_preserves_comments_and_string_commas(): + raw = '{"keep": "literal,}", /* user comment */ "a": 2, "b": 3}' + rewritten = remove_paths(raw, [("a",), ("b",)]) + assert '"literal,}"' in rewritten + assert "/* user comment */" in rewritten + assert json.loads(rewritten.replace("/* user comment */", "")) == {"keep": "literal,}"} + + +def test_existing_jsonc_trailing_comma_is_preserved(): + raw = '{"keep": {"value": 1, /* keep comma */}, "a": 2, "b": 3}' + rewritten = remove_paths(raw, [("a",), ("b",)]) + assert '{"value": 1, /* keep comma */}' in rewritten + + +def test_install_uninstall_preserves_strict_claude_settings(tmp_path, monkeypatch): + from code_review_graph import skills, uninstall + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setattr(Path, "home", lambda: home) + repo = tmp_path / "repo" + (repo / ".git" / "hooks").mkdir(parents=True) + settings = repo / ".claude" / "settings.json" + settings.parent.mkdir() + original = { + "permissions": {"allow": ["Bash(npm test)"], "deny": ["Bash(git push:*)"]}, + "hooks": {"PreToolUse": [{ + "matcher": "Bash", "hooks": [{"type": "command", "command": "echo MY-OWN-HOOK"}], + }]}, + } + settings.write_text(json.dumps(original, indent=2), encoding="utf-8") + skills.install_hooks(repo) + json.loads(settings.read_text(encoding="utf-8")) + report = uninstall.run(repo=repo, keep_data=True) + assert not report.errors + assert json.loads(settings.read_text(encoding="utf-8")) == original + + +# --- Edge cases added while hardening PR #1078 --------------------------------- + +def _exposed_trailing_commas(text: str) -> list[int]: + """Offsets of commas that sit directly before a closing bracket.""" + tokens = jsonc.tokenize(text) + return [ + token.start + for token, following in zip(tokens, tokens[1:]) + if token.kind == "," and following.kind in ("}", "]") + ] + + +def _subset_cases(): + for size in range(1, 6): + for removed in itertools.chain.from_iterable( + itertools.combinations(range(size), count) for count in range(size + 1) + ): + yield size, removed + + +@pytest.mark.parametrize("indent", [None, 2]) +def test_every_removal_subset_leaves_strict_json(indent): + """Any subset of object members or array elements, adjacent or not.""" + for size, removed in _subset_cases(): + obj = {f"k{index}": index for index in range(size)} + raw = json.dumps({"obj": obj, "tail": 0}, indent=indent) + rewritten = remove_paths(raw, [("obj", f"k{index}") for index in removed]) + expected = {key: value for key, value in obj.items() if int(key[1:]) not in removed} + assert json.loads(rewritten) == {"obj": expected, "tail": 0}, (size, removed) + + arr = list(range(size)) + raw = json.dumps({"arr": arr}, indent=indent) + rewritten = remove_paths(raw, [("arr", index) for index in removed]) + expected_arr = [value for value in arr if value not in removed] + assert json.loads(rewritten) == {"arr": expected_arr}, (size, removed) + + +def test_every_removal_subset_keeps_comments_and_exposes_no_comma(): + """Commented JSONC: comments survive and no new trailing comma appears.""" + for size, removed in _subset_cases(): + lines = ["{"] + for index in range(size): + comma = "," if index < size - 1 else "" + lines.append(f' "k{index}": {index}{comma} // note-{index}') + lines.append(" /* closing note */") + lines.append("}") + raw = "\n".join(lines) + "\n" + rewritten = remove_paths(raw, [(f"k{index}",) for index in removed]) + for index in range(size): + assert f"// note-{index}" in rewritten, (size, removed) + assert "/* closing note */" in rewritten + assert _exposed_trailing_commas(rewritten) == [], (size, removed) + assert json.loads(_strip_jsonc(rewritten)) == { + f"k{index}": index for index in range(size) if index not in removed + } + + +def test_no_paths_returns_the_document_unchanged(): + raw = '{\n "a": [1, 2,],\n // note\n "b": {"c": 3,},\n}\n' + assert remove_paths(raw, []) == raw + + +def test_surviving_member_keeps_its_own_jsonc_trailing_comma(): + raw = '{\n "a": 1,\n "keep": 2,\n}\n' + rewritten = remove_paths(raw, [("a",)]) + assert '"keep": 2,\n}' in rewritten + assert json.loads(_strip_jsonc(rewritten)) == {"keep": 2} + + +def test_removed_tail_with_jsonc_trailing_comma_leaves_strict_json(): + raw = '{"keep": 1, "a": 2, "b": 3,}' + rewritten = remove_paths(raw, [("a",), ("b",)]) + assert json.loads(rewritten) == {"keep": 1} + + raw = '{"keep": 1, "b": 3,}' + assert json.loads(remove_paths(raw, [("b",)])) == {"keep": 1} + + +@pytest.mark.parametrize("paths", [ + [("a",), ("a",), ("b",)], + [("b",), ("a",), ("b",)], + [("a",), ("b",), ("a",), ("b",)], +]) +def test_duplicate_paths_are_removed_once(paths): + raw = '{"keep": 1, "a": 2, "b": 3}' + assert json.loads(remove_paths(raw, paths)) == {"keep": 1} + + +def test_parent_and_child_paths_overlap_cleanly(): + raw = json.dumps( + {"keep": 1, "hooks": {"PostToolUse": [1], "SessionStart": [2]}}, indent=2 + ) + rewritten = remove_paths( + raw, [("hooks", "PostToolUse"), ("hooks",), ("hooks", "SessionStart", 0)] + ) + assert json.loads(rewritten) == {"keep": 1} + + +def test_removals_in_several_containers_at_once(): + raw = json.dumps( + {"x": {"k": 1, "a": 2, "b": 3}, "y": [1, 2, 3], "z": {"n": [4, 5, 6]}, "w": 0}, + indent=2, + ) + rewritten = remove_paths(raw, [ + ("x", "a"), ("x", "b"), ("y", 1), ("y", 2), ("z", "n", 1), ("z", "n", 2), ("w",), + ]) + assert json.loads(rewritten) == {"x": {"k": 1}, "y": [1], "z": {"n": [4]}} + + +def test_nested_hook_entries_removed_at_the_end_of_a_group(): + raw = json.dumps({"hooks": {"PostToolUse": [{ + "matcher": "Edit|Write", + "hooks": [{"command": "user"}, {"command": "crg-1"}, {"command": "crg-2"}], + }]}}, indent=2) + rewritten = remove_paths(raw, [ + ("hooks", "PostToolUse", 0, "hooks", 1), + ("hooks", "PostToolUse", 0, "hooks", 2), + ]) + assert json.loads(rewritten) == {"hooks": {"PostToolUse": [{ + "matcher": "Edit|Write", "hooks": [{"command": "user"}], + }]}} + + +def test_unicode_escapes_and_windows_paths_before_the_edit_keep_offsets_right(): + original = { + "ключ": "値,}", + "emoji": "😀 \",] \\", + "path": "C:\\Users\\Ünïcode dir\\settings.json,}", + "a": 1, + "b": [2], + } + for ensure_ascii in (True, False): + raw = json.dumps(original, indent=2, ensure_ascii=ensure_ascii) + rewritten = remove_paths(raw, [("a",), ("b",)]) + expected = {key: value for key, value in original.items() if key not in ("a", "b")} + assert json.loads(rewritten) == expected + + +def test_crlf_line_endings_survive_and_stay_strict(): + raw = json.dumps({"keep": 1, "a": 2, "b": 3}, indent=2).replace("\n", "\r\n") + rewritten = remove_paths(raw, [("a",), ("b",)]) + assert json.loads(rewritten) == {"keep": 1} + assert "\n" not in rewritten.replace("\r\n", "") + + +def test_large_document_tail_and_interleaved_removals(): + size = 300 + raw = json.dumps({f"k{index}": index for index in range(size)}, indent=2) + rewritten = remove_paths(raw, [(f"k{index}",) for index in range(size // 2, size)]) + assert json.loads(rewritten) == {f"k{index}": index for index in range(size // 2)} + + raw = json.dumps({"a": list(range(size))}) + rewritten = remove_paths(raw, [("a", index) for index in range(1, size, 2)]) + assert json.loads(rewritten) == {"a": list(range(0, size, 2))} + + +@pytest.mark.parametrize("raw,paths,error", [ + ('{"a": 1}', [("missing",)], KeyError), + ('{"a": [1]}', [("a", 3)], IndexError), + ('{"a": [1]}', [("a", -1)], IndexError), + ('{"a": 1}', [()], ValueError), + ("", [("a",)], ValueError), + ('{"a": 1, "b": 2}', [("b",), ("missing",)], KeyError), +]) +def test_unresolvable_paths_still_raise(raw, paths, error): + with pytest.raises(error): + remove_paths(raw, paths) + + +# --- Audit additions for PR #1078 ---------------------------------------------- + +_FUZZ_SCALARS = (1, 2.5, True, None, "s,}", "x//y /* z */", 'q"],', "ü😀\\") + + +def _random_value(rng, depth): + roll = rng.random() + if depth <= 0 or roll < 0.3: + return rng.choice(_FUZZ_SCALARS) + size = rng.randint(0, 4) + if roll < 0.65: + return {f"k{index}": _random_value(rng, depth - 1) for index in range(size)} + return [_random_value(rng, depth - 1) for _ in range(size)] + + +def _render_jsonc(value, rng, path, trailing_at, *, depth, compact, trailing, comments): + """Render ``value`` as JSONC and record the containers given a trailing comma.""" + if not isinstance(value, (dict, list)) or not value: + return json.dumps(value, ensure_ascii=False) + is_object = isinstance(value, dict) + items = list(value.items()) if is_object else list(enumerate(value)) + newline = "" if compact else "\n" + pad = "" if compact else " " * (depth + 1) + parts = [] + for position, (key, child) in enumerate(items): + comma = "," + if position == len(items) - 1: + comma = "," if trailing and rng.random() < 0.5 else "" + if comma: + trailing_at.add(path) + note = "" + if comments and rng.random() < 0.35: + note = " /* note ] , */" if compact or rng.random() < 0.5 else " // note ,}" + body = _render_jsonc( + child, rng, path + (key,), trailing_at, + depth=depth + 1, compact=compact, trailing=trailing, comments=comments, + ) + head = f"{json.dumps(key, ensure_ascii=False)}: " if is_object else "" + parts.append(f"{pad}{head}{body}{comma}{note}") + opening, closing = ("{", "}") if is_object else ("[", "]") + close_pad = "" if compact else " " * depth + return opening + newline + newline.join(parts) + newline + close_pad + closing + + +def _all_paths(value, prefix=()): + children = ( + value.items() if isinstance(value, dict) + else enumerate(value) if isinstance(value, list) else () + ) + for key, child in children: + yield prefix + (key,) + yield from _all_paths(child, prefix + (key,)) + + +def _without(value, removed, prefix=()): + if isinstance(value, dict): + return { + key: _without(child, removed, prefix + (key,)) + for key, child in value.items() if prefix + (key,) not in removed + } + if isinstance(value, list): + return [ + _without(child, removed, prefix + (index,)) + for index, child in enumerate(value) if prefix + (index,) not in removed + ] + return value + + +def _walk(value, path): + for key in path: + value = value[key] + return value + + +def _is_deletion_of(original: str, rewritten: str) -> bool: + """Whether ``rewritten`` is ``original`` with characters only deleted.""" + remaining = iter(original) + return all(char in remaining for char in rewritten) + + +def _comment_free(text: str) -> str: + """Drop comments but keep every comma, as strip-json-comments would.""" + return " ".join(text[token.start:token.end] for token in jsonc.tokenize(text)) + + +def test_random_nested_jsonc_removals_match_the_parsed_edit(): + """Seeded sweep: nested containers, comments, JSONC trailing commas, overlaps. + + For every case the splice must equal the same edit made on the parsed + value, may only delete characters, must keep exactly the original trailing + commas whose container and last element survive, and, for a file that had + none, must leave strict JSON once its comments are stripped. + """ + import random + + rng = random.Random(1078) + for case in range(3000): + value = {"root": _random_value(rng, 3), "tail": _random_value(rng, 2)} + trailing = rng.random() < 0.4 + trailing_at: set = set() + raw = _render_jsonc( + value, rng, (), trailing_at, + depth=0, compact=rng.random() < 0.25, trailing=trailing, + comments=rng.random() < 0.5, + ) + paths = list(_all_paths(value)) + chosen = rng.sample(paths, rng.randint(0, min(len(paths), 6))) + if chosen and rng.random() < 0.1: + chosen.append(rng.choice(chosen)) + removed = set(chosen) + context = (case, raw, chosen) + + rewritten = remove_paths(raw, chosen) + + assert json.loads(_strip_jsonc(rewritten)) == _without(value, removed), context + assert _is_deletion_of(raw, rewritten), context + kept_trailing = 0 + for path in trailing_at: + container = _walk(value, path) + last = list(container)[-1] if isinstance(container, dict) else len(container) - 1 + gone = any(path[:size] in removed for size in range(1, len(path) + 1)) + if not gone and path + (last,) not in removed: + kept_trailing += 1 + assert len(_exposed_trailing_commas(rewritten)) == kept_trailing, context + if not trailing: + assert json.loads(_comment_free(rewritten)) == _without(value, removed), context + + +@pytest.mark.parametrize("raw,paths,expected", [ + ("[1, 2, 3]", [(1,), (2,)], [1]), + ("[1, 2, 3]", [(0,), (2,)], [2]), + ('[{"a": 1, "b": 2, "c": 3}, 4, 5]', [(0, "b"), (0, "c"), (1,), (2,)], [{"a": 1}]), +]) +def test_top_level_array_removals_leave_strict_json(raw, paths, expected): + assert json.loads(remove_paths(raw, paths)) == expected + + +def test_comma_after_a_value_comment_is_dropped_and_the_comment_kept(): + raw = '{\n "keep": 1 /* why */, // after\n "a": 2,\n "b": 3\n}\n' + rewritten = remove_paths(raw, [("a",), ("b",)]) + assert "/* why */" in rewritten and "// after" in rewritten + assert _exposed_trailing_commas(rewritten) == [] + assert json.loads(_comment_free(rewritten)) == {"keep": 1} diff --git a/tests/test_parser_grammar_contract.py b/tests/test_parser_grammar_contract.py new file mode 100644 index 000000000..397012742 --- /dev/null +++ b/tests/test_parser_grammar_contract.py @@ -0,0 +1,320 @@ +"""Grammar dispatch names must exist in the bundled grammar (issue #986). + +PR #1086 adds the contract and removes the names it found to be unreal +(C# ``record_struct_declaration``, Swift ``struct_declaration``, Objective-C +``category_interface``, Ruby ``method_call``) and renames PowerShell's call +dispatch from ``command_expression`` to ``command``. + +The extra cases below pin what the bare ``id_for_node_kind`` check cannot see: +a supertype or hidden rule has an id yet never appears in a parse tree; a +table keyed by a language the parser never detects matches nothing; the +PowerShell rename must actually route real commands into call extraction; +and the constructs the removed names were meant for must still be extracted. +""" + +import pytest +from tree_sitter_language_pack import get_language + +from code_review_graph.parser import ( + _CALL_TYPES, + _CLASS_TYPES, + _FUNCTION_TYPES, + _IMPORT_TYPES, + EXTENSION_TO_LANGUAGE, + SHEBANG_INTERPRETER_TO_LANGUAGE, + CodeParser, +) + +_TABLES = ( + ("classes", _CLASS_TYPES), ("functions", _FUNCTION_TYPES), + ("imports", _IMPORT_TYPES), ("calls", _CALL_TYPES), +) + +_DISPATCH_ENTRIES = [ + (table, language, node_type) + for table, mapping in _TABLES + for language, names in mapping.items() + for node_type in names +] + + +def _concrete_named_kind(grammar, node_type: str) -> bool: + """True when *node_type* is a named kind a parse tree can contain. + + ``id_for_node_kind`` also answers for supertypes (``expression`` in + Python), which are hidden and never appear as a node's ``type``, and + maps the empty string to the built-in ``ERROR`` symbol, so the id must + round-trip to the same name. + """ + kind_id = grammar.id_for_node_kind(node_type, True) + if kind_id is None or grammar.node_kind_for_id(kind_id) != node_type: + return False + if kind_id in getattr(grammar, "supertypes", ()): + return False + return bool(grammar.node_kind_is_visible(kind_id)) + + +@pytest.mark.parametrize(("table", "language", "node_type"), _DISPATCH_ENTRIES) +def test_dispatch_type_exists_in_grammar(table, language, node_type): + grammar = get_language(language) + assert grammar.id_for_node_kind(node_type, True) is not None, ( + f"{table}[{language!r}] contains nonexistent named node {node_type!r}" + ) + + +@pytest.mark.parametrize(("table", "language", "node_type"), _DISPATCH_ENTRIES) +def test_dispatch_type_is_a_concrete_visible_node(table, language, node_type): + grammar = get_language(language) + assert _concrete_named_kind(grammar, node_type), ( + f"{table}[{language!r}] names {node_type!r}, which never appears in a " + "parse tree (missing, hidden or a supertype)" + ) + + +@pytest.mark.parametrize( + ("language", "node_type", "expected"), + [ + ("python", "call", True), + ("python", "function_definition", True), + ("powershell", "command", True), + # Supertype: has an id, never a node type in a tree. + ("python", "expression", False), + ("python", "primary_expression", False), + # Hidden rule and anonymous keyword. + ("python", "_simple_statement", False), + ("python", "def", False), + # The five names issue #986 reported. + ("csharp", "record_struct_declaration", False), + ("swift", "struct_declaration", False), + ("objc", "category_interface", False), + ("ruby", "method_call", False), + ("powershell", "command_expression", False), + # Empty name resolves to ERROR's id; it must not count as real. + ("python", "", False), + ("powershell", "", False), + ], +) +def test_contract_checker_rejects_unreal_kinds(language, node_type, expected): + assert _concrete_named_kind(get_language(language), node_type) is expected + + +def test_dispatch_tables_are_keyed_by_detected_loadable_languages(): + detectable = set(EXTENSION_TO_LANGUAGE.values()) + detectable |= set(SHEBANG_INTERPRETER_TO_LANGUAGE.values()) + for table, mapping in _TABLES: + for language, names in mapping.items(): + assert language in detectable, ( + f"{table} has an entry for {language!r}, which the parser never detects" + ) + get_language(language) # raises if the grammar is not bundled + duplicates = sorted({n for n in names if names.count(n) > 1}) + assert not duplicates, f"{table}[{language!r}] repeats {duplicates}" + + +def _spy_call_dispatch(monkeypatch) -> list[tuple[str, str]]: + seen: list[tuple[str, str]] = [] + original = CodeParser._extract_calls + + def spy(self, child, source, language, *args, **kwargs): + if language == "powershell": + seen.append((child.type, child.text.decode("utf-8", errors="replace"))) + return original(self, child, source, language, *args, **kwargs) + + monkeypatch.setattr(CodeParser, "_extract_calls", spy) + return seen + + +_PS_SOURCE = """\ +function Get-Foo { + param($x) + Invoke-Bar -Name $x +} + +Invoke-Command -ScriptBlock { Get-Inner 1 } +Get-Foo 2 +""" + + +def test_powershell_commands_reach_call_extraction(tmp_path, monkeypatch): + """Every PowerShell command, nested or not, is handed to call extraction. + + On staging the dispatch named ``command_expression``, so no PowerShell + node ever reached ``_extract_calls``. + """ + seen = _spy_call_dispatch(monkeypatch) + path = tmp_path / "scripts dir" / "dépl oy.ps1" + path.parent.mkdir() + path.write_text(_PS_SOURCE, encoding="utf-8") + + nodes, _edges = CodeParser().parse_file(path) + + assert {kind for kind, _ in seen} == {"command"} + heads = sorted(text.split()[0] for _, text in seen) + assert heads == ["Get-Foo", "Get-Inner", "Invoke-Bar", "Invoke-Command"] + assert any(n.kind == "File" for n in nodes) + + +def test_powershell_long_script_dispatches_every_command(tmp_path, monkeypatch): + """A flat 2,000-command script: none is skipped by call dispatch.""" + seen = _spy_call_dispatch(monkeypatch) + count = 2000 + path = tmp_path / "many.ps1" + path.write_text( + "".join(f"Invoke-Step{i} -Index {i}\n" for i in range(count)), + encoding="utf-8", + ) + + CodeParser().parse_file(path) + + assert len(seen) == count + assert seen[-1][1].startswith(f"Invoke-Step{count - 1} ") + + +def test_csharp_record_struct_still_extracted_as_class(tmp_path): + """``record struct`` parses as ``record_declaration``; dropping the + unreal ``record_struct_declaration`` must not lose it.""" + path = tmp_path / "Money.cs" + path.write_text( + "namespace App {\n" + " public record struct Money(decimal Amount);\n" + " public readonly record struct Point(int X) {\n" + " public int Twice() => X * 2;\n" + " }\n" + " public record Person(string Name);\n" + "}\n", + encoding="utf-8", + ) + + nodes, edges = CodeParser().parse_file(path) + + classes = {n.name for n in nodes if n.kind == "Class"} + assert {"Money", "Point", "Person"} <= classes + twice = [n for n in nodes if n.kind == "Function" and n.name == "Twice"] + assert len(twice) == 1 and twice[0].parent_name == "Point" + + +def test_objc_category_still_extracted_as_class(tmp_path): + """A category parses as ``class_interface``/``class_implementation``; + dropping the unreal ``category_interface`` must not lose it.""" + path = tmp_path / "NSString+Shout.m" + path.write_text( + "#import \n" + "@interface NSString (Shout)\n" + "- (NSString *)shout;\n" + "@end\n" + "@implementation NSString (Shout)\n" + "- (NSString *)shout { return [self uppercaseString]; }\n" + "@end\n", + encoding="utf-8", + ) + + nodes, edges = CodeParser().parse_file(path) + + assert any(n.kind == "Class" and n.name == "NSString" for n in nodes) + shout = [n for n in nodes if n.kind == "Function" and n.name == "shout"] + assert shout and all(n.parent_name == "NSString" for n in shout) + calls = {e.target for e in edges if e.kind == "CALLS"} + assert "uppercaseString" in calls + + +def test_swift_struct_and_ruby_receiver_calls_still_extracted(tmp_path): + """Swift structs come from ``class_declaration`` and Ruby receiver calls + from ``call``; the removed names were never doing that work.""" + swift = tmp_path / "Point.swift" + swift.write_text( + "struct Point {\n func norm() -> Int { return 1 }\n}\n", + encoding="utf-8", + ) + ruby = tmp_path / "greeter.rb" + ruby.write_text( + "class Greeter\n def hello\n Foo.new.run\n end\nend\n", + encoding="utf-8", + ) + parser = CodeParser() + + swift_nodes, _ = parser.parse_file(swift) + point = [n for n in swift_nodes if n.kind == "Class" and n.name == "Point"] + assert len(point) == 1 and point[0].extra.get("swift_kind") == "struct" + + _, ruby_edges = parser.parse_file(ruby) + targets = {e.target.split("::")[-1].split(".")[-1] for e in ruby_edges if e.kind == "CALLS"} + assert {"new", "run"} <= targets + + +_PS_PIPELINE_SOURCE = """\ +class Widget { + [void] Run() { Write-Host "hi" } +} +Get-ChildItem -Path . | Where-Object { Test-Path $_ } | Sort-Object Name +""" + + +def test_powershell_pipeline_stages_and_class_methods_reach_call_extraction( + tmp_path, monkeypatch, +): + """Every stage of a pipeline is its own ``command`` node, as is a command + in a class method body; each one must reach call extraction.""" + seen = _spy_call_dispatch(monkeypatch) + path = tmp_path / "pipeline.ps1" + path.write_text(_PS_PIPELINE_SOURCE, encoding="utf-8") + + CodeParser().parse_file(path) + + heads = sorted(text.split()[0] for _, text in seen) + assert heads == [ + "Get-ChildItem", "Sort-Object", "Test-Path", "Where-Object", "Write-Host", + ] + + +def test_powershell_module_file_dispatches_calls(tmp_path, monkeypatch): + """``.psm1`` modules are PowerShell too and take the same call dispatch.""" + seen = _spy_call_dispatch(monkeypatch) + path = tmp_path / "Tools.psm1" + path.write_text( + "function Get-Foo { Write-Verbose 'x' }\n" + "Export-ModuleMember -Function Get-Foo\n", + encoding="utf-8", + ) + + CodeParser().parse_file(path) + + heads = sorted(text.split()[0] for _, text in seen) + assert heads == ["Export-ModuleMember", "Write-Verbose"] + + +def test_powershell_half_written_script_still_parses(tmp_path, monkeypatch): + """An unfinished script (unclosed function, dangling pipe) sends its + recovered commands through call extraction without raising.""" + seen = _spy_call_dispatch(monkeypatch) + path = tmp_path / "wip.ps1" + path.write_text( + 'Write-Output "before"\nfunction Broken {\n Get-Item $x |\n', + encoding="utf-8", + ) + + nodes, _edges = CodeParser().parse_file(path) + + assert any(n.kind == "File" for n in nodes) + assert {kind for kind, _ in seen} == {"command"} + assert "Write-Output" in {text.split()[0] for _, text in seen} + + +def test_objc_interface_only_category_is_a_class(tmp_path): + """The construct ``category_interface`` was meant for: a category declared + with ``@interface`` and no ``@implementation`` in the file. It parses as + ``class_interface``, which alone must still yield the Class node.""" + path = tmp_path / "Widget+Private.m" + path.write_text( + "@interface Widget (Private)\n" + "- (void)reset;\n" + "@end\n", + encoding="utf-8", + ) + + nodes, edges = CodeParser().parse_file(path) + + widget = [n for n in nodes if n.kind == "Class" and n.name == "Widget"] + assert len(widget) == 1 and widget[0].line_start == 1 + assert any( + e.kind == "CONTAINS" and e.target.endswith("::Widget") for e in edges + ) diff --git a/tests/test_platform_lifecycle.py b/tests/test_platform_lifecycle.py index afec3d962..123b56c4f 100644 --- a/tests/test_platform_lifecycle.py +++ b/tests/test_platform_lifecycle.py @@ -775,12 +775,6 @@ def test_step3_preexisting_user_config_survives(sandbox: Sandbox, key: str) -> N CONTENT_LEFTOVER_XFAIL = { - "qoder": ( - "uninstall._process_repo removes .qoder/skills//SKILL.md only for " - "names found in the TARGET repo's own skills/ directory, while install " - "copies them out of the installed package, so every Qoder skill file " - "survives uninstall in a repo that has no skills/ directory of its own" - ), "gemini-cli": ( "install_gemini_cli_hooks copies .gemini/settings.json to " "settings.json.bak, and uninstall never removes the backup, so a full " @@ -882,6 +876,37 @@ def test_step4b_uninstall_leaves_no_file_still_holding_our_content( _record("step4b_content_leftovers", key, len(created)) +def test_step4b_qoder_uninstall_removes_every_skill_file_install_wrote( + sandbox: Sandbox, +) -> None: + """Every ``.qoder/skills`` file install wrote is gone after uninstall. + + Step 4b only flags a leftover that still contains the string + ``code-review-graph``, and most bundled ``SKILL.md`` files never say it, + so an uninstall that dropped some of the bundled names would still pass + there. This compares by path against what install actually wrote, in a + repository with no ``skills/`` directory of its own (#909). + """ + skills_root = sandbox.repo / ".qoder" / "skills" + assert not (sandbox.repo / "skills").exists() + before = snapshot(sandbox) + run_install(sandbox, "qoder") + written = sorted( + path for path in set(snapshot(sandbox)) - set(before) if skills_root in path.parents + ) + installed = {path.relative_to(skills_root).parts[0] for path in written} + assert installed == set(skills.bundled_skill_names()), ( + f"qoder: install wrote {sorted(installed)}, bundled are {skills.bundled_skill_names()}" + ) + + report = run_uninstall(sandbox) + after = snapshot(sandbox) + + left = [str(path) for path in written if path in after] + assert not left, f"qoder: uninstall left skill files behind: {left}" + assert not report.errors, f"qoder: uninstall reported errors {report.errors}" + + @pytest.mark.parametrize( "key", [pytest.param(k, marks=[pytest.mark.xfail(strict=True, reason=HUSK_XFAIL)]) diff --git a/tests/test_server_instructions.py b/tests/test_server_instructions.py new file mode 100644 index 000000000..10dcd51ce --- /dev/null +++ b/tests/test_server_instructions.py @@ -0,0 +1,295 @@ +"""The MCP server instructions only name tools and patterns that exist. + +With deferred tool loading an agent sees the server instructions and the tool +names, not the tool descriptions, so the instructions are its routing table. A +renamed tool or query pattern would leave that table pointing at something the +server no longer has, with nothing else failing. + +Introduced by PR #1096. The later tests pin the instructions on the wire (the +real stdio handshake), under the ``serve --tools`` / ``CRG_TOOLS`` allow-list, +the argument names the routing lines spell, and the re-export claim the text +makes about graph answers. +""" + +from __future__ import annotations + +import asyncio +import os +import re +import sys +from datetime import timedelta +from pathlib import Path +from unittest.mock import patch + +import pytest + +from code_review_graph import main +from code_review_graph.tools.query import _QUERY_PATTERNS + +try: + from mcp import ClientSession + from mcp.client.stdio import StdioServerParameters, stdio_client +except ImportError: # pragma: no cover - ``mcp`` is a hard runtime dependency + ClientSession = None # type: ignore[assignment,misc] + StdioServerParameters = None # type: ignore[assignment,misc] + stdio_client = None # type: ignore[assignment] + +# Claude Code truncates server instructions past this many characters. +CLAUDE_CODE_INSTRUCTIONS_LIMIT = 2048 + + +def test_server_is_built_with_the_routing_instructions() -> None: + assert main.mcp.instructions == main.SERVER_INSTRUCTIONS + + +def test_instructions_fit_before_truncation() -> None: + assert len(main.SERVER_INSTRUCTIONS) <= CLAUDE_CODE_INSTRUCTIONS_LIMIT + + +def test_every_tool_named_is_registered() -> None: + named = set(re.findall(r"\b\w+_tool\b", main.SERVER_INSTRUCTIONS)) + assert named, "the instructions no longer name any tool" + registered = {tool.name for tool in asyncio.run(main.mcp.list_tools())} + assert named <= registered, f"named but not registered: {sorted(named - registered)}" + + +def test_every_query_pattern_named_exists() -> None: + named = set(re.findall(r"pattern=(\w+)", main.SERVER_INSTRUCTIONS)) + named |= set(re.findall(r"pattern=\w+ / (\w+)", main.SERVER_INSTRUCTIONS)) + assert named, "the instructions no longer name any query pattern" + assert named <= set(_QUERY_PATTERNS), ( + f"named but not a query pattern: {sorted(named - set(_QUERY_PATTERNS))}" + ) + + +def test_plain_text_search_is_left_to_grep() -> None: + # The graph indexes structure, not text. Without this line the routing table + # reads as "use the graph for everything", and an agent looking for a log + # message or an env var name gets an empty graph answer instead of a grep hit. + assert "Grep" in main.SERVER_INSTRUCTIONS + assert "does not index text" in main.SERVER_INSTRUCTIONS + + +# --------------------------------------------------------------------------- +# Edge cases +# --------------------------------------------------------------------------- + +REPO_SOURCE_ROOT = Path(__file__).resolve().parents[1] +HANDSHAKE_TIMEOUT = 120.0 +_TOOL_NAME = re.compile(r"\b\w+_tool\b") + +# The allow-list README.md shows for ``serve --tools`` (plus the review context +# tool its MCP client config example adds). +README_ALLOW_LIST = ( + "query_graph_tool,semantic_search_nodes_tool,detect_changes_tool,get_review_context_tool" +) + +requires_mcp_client = pytest.mark.skipif( + ClientSession is None, reason="mcp client library not installed" +) + + +def _server_env(home: Path, crg_tools: str | None) -> dict[str, str]: + """Child environment: isolated user state, the checkout under test importable.""" + env = os.environ.copy() + for key in ( + "HOME", "USERPROFILE", "APPDATA", "XDG_CONFIG_HOME", "XDG_DATA_HOME", + "XDG_CACHE_HOME", "CRG_HOME", + ): + env[key] = str(home) + for key in ("CRG_TOOLS", "CRG_DATA_DIR", "CRG_REPO_ROOT"): + env.pop(key, None) + if crg_tools is not None: + env["CRG_TOOLS"] = crg_tools + env["PYTHONIOENCODING"] = "utf-8" + env["PYTHONPATH"] = os.pathsep.join( + value for value in (str(REPO_SOURCE_ROOT), env.get("PYTHONPATH")) if value + ) + return env + + +async def _handshake( + tmp_path: Path, *, tools_arg: str | None = None, crg_tools: str | None = None +) -> tuple[str, set[str]]: + """Start ``serve`` over stdio as a client config would; return what the client sees.""" + repo = tmp_path / "repo with spaces" + (repo / ".code-review-graph").mkdir(parents=True) + home = tmp_path / "home" + home.mkdir() + args = ["-m", "code_review_graph", "serve", "--repo", str(repo)] + if tools_arg is not None: + args += ["--tools", tools_arg] + params = StdioServerParameters( + command=sys.executable, args=args, env=_server_env(home, crg_tools), cwd=str(repo), + ) + async with stdio_client(params) as (read, write): + async with ClientSession( + read, write, read_timeout_seconds=timedelta(seconds=HANDSHAKE_TIMEOUT), + ) as session: + init = await asyncio.wait_for(session.initialize(), timeout=HANDSHAKE_TIMEOUT) + listed = await asyncio.wait_for(session.list_tools(), timeout=HANDSHAKE_TIMEOUT) + return init.instructions or "", {tool.name for tool in listed.tools} + + +@pytest.mark.e2e +@requires_mcp_client +async def test_initialize_handshake_delivers_the_routing_instructions( + tmp_path: Path, +) -> None: + # The attribute on the server object is not what an agent reads: the client + # gets the text in the ``initialize`` result. Drive the real stdio server. + instructions, listed = await _handshake(tmp_path) + assert instructions == main.SERVER_INSTRUCTIONS + named = set(_TOOL_NAME.findall(instructions)) + assert named and named <= listed, f"named but not served: {sorted(named - listed)}" + + +@pytest.mark.e2e +@requires_mcp_client +@pytest.mark.parametrize("via", ["--tools", "CRG_TOOLS"]) +async def test_tool_allow_list_keeps_the_routing_lines_for_allowed_tools( + tmp_path: Path, via: str, +) -> None: + # ``serve --tools`` / ``CRG_TOOLS`` (README "Limiting tools") removes tools + # after the server is built. The handshake must still succeed, list exactly + # the allowed tools, and keep the routing lines that point at them. (The + # text may still name tools the allow-list removed, as the prompts and + # next_tool_suggestions already do; that is a separate follow-up.) + if via == "--tools": + instructions, listed = await _handshake(tmp_path, tools_arg=README_ALLOW_LIST) + else: + instructions, listed = await _handshake(tmp_path, crg_tools=README_ALLOW_LIST) + assert listed == set(README_ALLOW_LIST.split(",")) + for line in ( + "query_graph_tool pattern=callers_of", + "query_graph_tool pattern=importers_of", + "query_graph_tool pattern=tests_for", + "detect_changes_tool, get_review_context_tool", + ): + assert line in instructions, line + named = set(_TOOL_NAME.findall(instructions)) + assert {"query_graph_tool", "detect_changes_tool", "get_review_context_tool"} <= ( + named & listed + ) + + +def test_every_argument_named_belongs_to_a_tool_on_its_line() -> None: + # "query_graph_tool pattern=callers_of" only works if query_graph_tool still + # takes an argument called ``pattern``; a renamed parameter would turn every + # call the routing table suggests into a validation error. + schemas = { + tool.name: set((tool.parameters or {}).get("properties", {})) + for tool in asyncio.run(main.mcp.list_tools()) + } + checked = 0 + for line in main.SERVER_INSTRUCTIONS.splitlines(): + arguments = set(re.findall(r"\b(\w+)=", line)) + if not arguments: + continue + tools = _TOOL_NAME.findall(line) + assert tools, f"argument without a tool: {line!r}" + accepted = set().union(*(schemas.get(name, set()) for name in tools)) + assert arguments <= accepted, ( + f"{line!r}: {sorted(arguments - accepted)} is not a parameter of {tools}" + ) + checked += 1 + assert checked, "no routing line names an argument any more" + + +def test_every_alternative_after_pattern_is_a_query_pattern() -> None: + # The PR's own check reads at most two alternatives (``pattern=a / b``); a + # third (``pattern=a / b / c``) or a comma-separated list would go unchecked. + named: set[str] = set() + for line in main.SERVER_INSTRUCTIONS.splitlines(): + if "pattern=" not in line: + continue + tail = line.split("pattern=", 1)[1] + named |= { + token for token in re.findall(r"\b[a-z]+(?:_[a-z]+)+\b", tail) + if not token.endswith("_tool") + } + assert {"callers_of", "references_to", "importers_of", "tests_for"} <= named + assert named <= set(_QUERY_PATTERNS), ( + f"named but not a query pattern: {sorted(named - set(_QUERY_PATTERNS))}" + ) + + +_BARREL_FILES = { + "src/lib.ts": ( + "export function computeTotal(xs: number[]): number {\n" + " return xs.reduce((a, b) => a + b, 0);\n}\n" + ), + "src/index.ts": "export * from './lib';\n", + "src/consumer.ts": ( + "import { computeTotal } from './index';\n\n" + "export function checkout(xs: number[]): number {\n" + " return computeTotal(xs) * 2;\n}\n" + ), +} + + +@pytest.fixture +def barrel_repo(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A TypeScript package whose consumer reaches ``lib.ts`` only through a barrel.""" + from code_review_graph.tools.build import build_or_update_graph + + monkeypatch.delenv("CRG_DATA_DIR", raising=False) + monkeypatch.delenv("CRG_REPO_ROOT", raising=False) + repo = tmp_path / "barrel repo" + (repo / ".git").mkdir(parents=True) + for rel, text in _BARREL_FILES.items(): + path = repo / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + with patch( + "code_review_graph.incremental.get_all_tracked_files", + return_value=sorted(_BARREL_FILES), + ): + result = build_or_update_graph( + full_rebuild=True, repo_root=str(repo), postprocess="none", + ) + assert result["status"] == "ok" + return repo + + +def test_graph_reaches_dependents_through_a_barrel_file(barrel_repo: Path) -> None: + # The instructions promise "dependents reached through re-exports and barrel + # files that never spell the name". consumer.ts never spells "lib", so a text + # search for the module misses it; callers_of and the blast radius must not. + from code_review_graph.tools.query import get_impact_radius, query_graph + + assert "lib" not in _BARREL_FILES["src/consumer.ts"] + assert "pattern=callers_of" in main.SERVER_INSTRUCTIONS + assert "get_impact_radius_tool" in main.SERVER_INSTRUCTIONS + + callers = query_graph( + pattern="callers_of", target="computeTotal", repo_root=str(barrel_repo), + ) + assert callers["status"] == "ok" + assert {r["name"] for r in callers["results"]} == {"checkout"} + + impact = get_impact_radius(changed_files=["src/lib.ts"], repo_root=str(barrel_repo)) + assert impact["status"] == "ok" + impacted = {Path(f).as_posix() for f in impact["impacted_files"]} + assert any(f.endswith("src/consumer.ts") for f in impacted), impacted + + +@pytest.mark.xfail( + strict=True, + reason=( + "`export * from './lib'` creates no IMPORTS_FROM edge, so importers_of a " + "file reached only through a barrel is empty although the instructions " + "route 'which files import file X' to it; a text search for './lib' finds " + "the barrel. Graph limitation that predates #1096." + ), +) +def test_importers_of_includes_a_barrel_that_re_exports_the_file(barrel_repo: Path) -> None: + from code_review_graph.tools.query import query_graph + + assert "pattern=importers_of" in main.SERVER_INSTRUCTIONS + importers = query_graph( + pattern="importers_of", target="src/lib.ts", repo_root=str(barrel_repo), + ) + assert importers["status"] == "ok" + files = {Path(r["file"]).as_posix() for r in importers["results"]} + assert any(f.endswith("src/index.ts") for f in files), files diff --git a/tests/test_storybook_dead_code.py b/tests/test_storybook_dead_code.py new file mode 100644 index 000000000..e1bcb416c --- /dev/null +++ b/tests/test_storybook_dead_code.py @@ -0,0 +1,273 @@ +"""Storybook consumers do not prove production use (issue #902, PR #1085).""" + +from pathlib import Path + +import pytest + +from code_review_graph.changes import analyze_changes +from code_review_graph.graph import GraphStore +from code_review_graph.incremental import full_build, incremental_update +from code_review_graph.parser import CodeParser, EdgeInfo, NodeInfo, is_test_file +from code_review_graph.refactor import find_dead_code + + +@pytest.mark.parametrize('extension', ['js', 'jsx', 'ts', 'tsx']) +def test_story_filename_is_nonproduction(extension): + path = Path(f'components/Foo.stories.{extension}') + assert is_test_file(path) + nodes, _ = CodeParser().parse_bytes(path, b'export function Story() { return 1; }') + assert nodes and all(node.is_test for node in nodes) + + +@pytest.mark.parametrize('path', ['stories/Foo.tsx', 'Foo.stories.py', 'Foo.stories.tsx.bak']) +def test_story_like_production_paths_remain_production(path): + assert not is_test_file(path) + + +@pytest.mark.parametrize('production_caller', [False, True]) +def test_only_story_callers_do_not_keep_component_alive(tmp_path, production_caller): + with GraphStore(tmp_path / 'graph.db') as store: + component = 'components/Foo.tsx' + story = 'components/Foo.stories.tsx' + store.upsert_node(NodeInfo('Function', 'Foo', component, 1, 3, 'tsx')) + # False metadata models a graph built before stories were classified. + store.upsert_node(NodeInfo('File', story, story, 1, 5, 'tsx', is_test=False)) + store.upsert_edge(EdgeInfo('IMPORTS_FROM', story, component, story, 1)) + store.upsert_edge(EdgeInfo('CALLS', story, f'{component}::Foo', story, 3)) + if production_caller: + store.upsert_edge(EdgeInfo('CALLS', 'app.tsx::App', f'{component}::Foo', + 'app.tsx', 3)) + store.commit() + dead = {node['name'] for node in find_dead_code(store, root=tmp_path)} + assert ('Foo' in dead) is not production_caller + + +def test_story_call_does_not_hide_bare_production_caller_fallback(tmp_path): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Function', 'Foo', 'Foo.tsx', 1, 3, 'tsx')) + store.upsert_node(NodeInfo('File', 'app.tsx', 'app.tsx', 1, 3, 'tsx')) + store.upsert_node(NodeInfo('Function', 'App', 'app.tsx', 1, 3, 'tsx')) + store.upsert_edge(EdgeInfo('CALLS', 'Foo.stories.tsx', 'Foo.tsx::Foo', + 'Foo.stories.tsx', 2)) + store.upsert_edge(EdgeInfo('CALLS', 'app.tsx::App', 'Foo', 'app.tsx', 2)) + store.commit() + assert not any(n['name'] == 'Foo' for n in find_dead_code(store, root=tmp_path)) + + +def test_story_member_calls_do_not_keep_class_alive(tmp_path): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'Widget', 'Widget.tsx', 1, 5, 'tsx')) + store.upsert_edge(EdgeInfo('CALLS', 'Widget.stories.tsx', 'Widget.tsx::Widget.render', + 'Widget.stories.tsx', 3)) + store.commit() + assert any(n['name'] == 'Widget' for n in find_dead_code(store, root=tmp_path)) + + +# --- Edge cases added while hardening PR #1085 --------------------------------- + + +def _dead_names(store, root=None): + return {node['name'] for node in find_dead_code(store, root=root)} + + +def _dead_qualified(store, root=None): + return {node['qualified_name'] for node in find_dead_code(store, root=root)} + + +def test_prebuilt_graph_story_symbols_are_not_reported_dead(tmp_path): + # A graph built before #1085 stored story symbols with is_test = 0. They + # are demo code, not production symbols waiting for a caller. + story = 'components/Foo.stories.tsx' + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Function', 'Primary', story, 5, 5, 'tsx', is_test=False)) + store.upsert_node(NodeInfo('Class', 'StoryHarness', story, 7, 9, 'tsx', is_test=False)) + store.commit() + assert find_dead_code(store, root=tmp_path) == [] + + +@pytest.mark.parametrize('kind', ['CALLS', 'IMPORTS_FROM', 'REFERENCES']) +@pytest.mark.parametrize('caller_file, alive', [ + ('components/Foo.stories.tsx', False), + ('components/Foo.stories.js', False), + ('pages/Home.tsx', True), +]) +def test_each_story_evidence_kind_alone_does_not_keep_component_alive( + tmp_path, kind, caller_file, alive, +): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Function', 'Foo', 'components/Foo.tsx', 1, 3, 'tsx')) + store.upsert_edge(EdgeInfo(kind, f'{caller_file}::Use', 'components/Foo.tsx::Foo', + caller_file, 2)) + store.commit() + assert ('Foo' not in _dead_names(store, tmp_path)) is alive + + +@pytest.mark.parametrize('source', ['components/Foo.tsx::Foo', 'Foo']) +@pytest.mark.parametrize('test_file, alive', [ + ('components/Foo.stories.tsx', False), + ('components/Foo.test.tsx', True), +]) +def test_story_tested_by_is_not_test_evidence_but_real_tests_are( + tmp_path, source, test_file, alive, +): + # ``Foo`` covers the unresolved bare-name TESTED_BY fallback. + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Function', 'Foo', 'components/Foo.tsx', 1, 3, 'tsx')) + store.upsert_edge(EdgeInfo('TESTED_BY', source, f'{test_file}::check', test_file, 4)) + store.commit() + assert ('Foo' not in _dead_names(store, tmp_path)) is alive + + +@pytest.mark.parametrize('caller_file, alive', [ + ('Widget.stories.tsx', False), + ('app.tsx', True), +]) +def test_class_qualified_story_call_does_not_keep_method_alive(tmp_path, caller_file, alive): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'Widget', 'Widget.tsx', 1, 9, 'tsx')) + store.upsert_node(NodeInfo('Function', 'label', 'Widget.tsx', 2, 4, 'tsx', + parent_name='Widget')) + store.upsert_edge(EdgeInfo('CALLS', f'{caller_file}::Use', 'Widget::label', + caller_file, 3)) + store.commit() + assert ('label' not in _dead_names(store, tmp_path)) is alive + + +@pytest.mark.parametrize('subclass_file, alive', [ + ('Widget.stories.tsx', False), + ('FancyWidget.tsx', True), +]) +def test_story_subclass_does_not_keep_base_class_alive(tmp_path, subclass_file, alive): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'Widget', 'Widget.tsx', 1, 9, 'tsx')) + # The bare-name INHERITS lookup joins on the subclass node's language. + store.upsert_node(NodeInfo('Class', 'Demo', subclass_file, 1, 3, 'tsx', + is_test=is_test_file(subclass_file))) + store.upsert_edge(EdgeInfo('INHERITS', f'{subclass_file}::Demo', 'Widget', + subclass_file, 1)) + store.commit() + assert ('Widget' not in _dead_names(store, tmp_path)) is alive + + +def test_windows_style_story_paths_are_excluded(tmp_path): + assert is_test_file('C:\\repo\\components\\Foo.stories.tsx') + assert not is_test_file('C:\\repo\\components\\Foo.tsx') + component = 'C:/repo/components/Foo.tsx' + story = 'C:/repo/components/Foo.stories.tsx' + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Function', 'Foo', component, 1, 3, 'tsx')) + store.upsert_node(NodeInfo('Function', 'Primary', story, 5, 5, 'tsx', is_test=False)) + store.upsert_edge(EdgeInfo('CALLS', f'{story}::Primary', f'{component}::Foo', story, 5)) + store.upsert_edge(EdgeInfo('IMPORTS_FROM', story, f'{component}::Foo', story, 1)) + store.commit() + # Rows written by a Windows build before path normalisation (#774) + # kept native separators; the story check must still recognise them. + for table, columns in ( + ('nodes', ('file_path', 'qualified_name')), + ('edges', ('file_path', 'source_qualified', 'target_qualified')), + ): + assignments = ', '.join(f"{c} = replace({c}, '/', char(92))" for c in columns) + store._conn.execute(f'UPDATE {table} SET {assignments}') # nosec B608 + store.commit() + assert _dead_names(store) == {'Foo'} + + +def test_member_call_scan_sees_production_caller_among_many_story_rows(tmp_path): + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'Widget', 'Widget.tsx', 1, 9, 'tsx')) + for i in range(2000): + store.upsert_edge(EdgeInfo('CALLS', f'Widget.stories.tsx::Story{i}', + 'Widget.tsx::Widget.render', 'Widget.stories.tsx', i + 1)) + store.commit() + assert 'Widget' in _dead_names(store, tmp_path) + store.upsert_edge(EdgeInfo('CALLS', 'app.tsx::App', 'Widget.tsx::Widget.render', + 'app.tsx', 3)) + store.commit() + assert 'Widget' not in _dead_names(store, tmp_path) + + +def test_production_call_to_base_method_still_keeps_override_alive(tmp_path): + # Polymorphic dispatch: an application call to Animal.speak keeps + # Dog.speak alive. The story filter must not drop that path. + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'Animal', 'animal.ts', 1, 5, 'typescript')) + store.upsert_node(NodeInfo('Function', 'speak', 'animal.ts', 2, 4, 'typescript', + parent_name='Animal')) + store.upsert_node(NodeInfo('Class', 'Dog', 'dog.ts', 1, 5, 'typescript')) + store.upsert_node(NodeInfo('Function', 'speak', 'dog.ts', 2, 4, 'typescript', + parent_name='Dog')) + store.upsert_edge(EdgeInfo('INHERITS', 'dog.ts::Dog', 'Animal', 'dog.ts', 1)) + store.upsert_edge(EdgeInfo('CALLS', 'Animal.stories.ts::Use', 'animal.ts::Animal.speak', + 'Animal.stories.ts', 3)) + store.upsert_edge(EdgeInfo('CALLS', 'app.ts::main', 'animal.ts::Animal.speak', + 'app.ts', 3)) + store.commit() + dead = _dead_qualified(store, tmp_path) + assert 'animal.ts::Animal.speak' not in dead + assert 'dog.ts::Dog.speak' not in dead + + +def test_production_type_annotation_still_keeps_type_alive(tmp_path): + # Story files are now test code; an application function's annotation + # must still count as a use of the type. + with GraphStore(tmp_path / 'graph.db') as store: + store.upsert_node(NodeInfo('Class', 'UserShape', 'user.ts', 1, 3, 'typescript')) + store.upsert_node(NodeInfo('Function', 'loadUser', 'api.ts', 1, 3, 'typescript', + params='()', return_type=': UserShape')) + store.commit() + assert 'UserShape' not in _dead_names(store, tmp_path) + + +def _write(path, text): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding='utf-8') + + +def test_full_build_and_incremental_update_on_real_files(tmp_path): + root = tmp_path / 'rëpo with spaces' + comp = root / 'src' / 'compönents' + _write(comp / 'Foo.tsx', 'export function Foo() {\n return 1;\n}\n') + _write(comp / 'Foo.stories.tsx', + "import { Foo } from './Foo';\n\nexport default { component: Foo };\n" + 'export const Primary = () => Foo();\n') + _write(comp / 'Bar.tsx', 'export function Bar() {\n return 2;\n}\n') + _write(comp / 'Bar.stories.tsx', + "import { Bar } from './Bar';\n\nexport const Primary = () => Bar();\n") + _write(comp / 'Baz.tsx', 'export function Baz() {\n return 3;\n}\n') + _write(comp / 'Baz.test.tsx', + "import { Baz } from './Baz';\n\ntest('baz', () => {\n Baz();\n});\n") + app = root / 'src' / 'app.tsx' + _write(app, "import { Bar } from './compönents/Bar';\n\n" + 'export function main() {\n return Bar();\n}\n') + with GraphStore(tmp_path / 'graph.db') as store: + assert not full_build(root, store)['errors'] + flags = store._conn.execute( + "SELECT is_test FROM nodes WHERE file_path LIKE '%.stories.tsx'" + ).fetchall() + assert flags and all(row[0] for row in flags) + # Only the story-only component; Bar has an application caller and + # Baz has an ordinary test. Story exports are not candidates. + assert _dead_names(store, root) == {'Foo'} + assert _dead_names(store, root) == {'Foo'} # idempotent + + _write(app, "import { Bar } from './compönents/Bar';\n" + "import { Foo } from './compönents/Foo';\n\n" + 'export function main() {\n return Bar() + Foo();\n}\n') + assert not incremental_update(root, store, changed_files=['src/app.tsx'])['errors'] + assert _dead_names(store, root) == set() + + +def test_story_exports_are_not_reported_as_test_gaps(tmp_path): + # Classifying stories as test/demo code also keeps their exports out of + # the untested-production-code list in change analysis. + root = tmp_path / 'repo' + story = root / 'Foo.stories.tsx' + _write(root / 'Foo.tsx', 'export function Foo() {\n return 1;\n}\n') + _write(story, "import { Foo } from './Foo';\n\nexport const Primary = () => Foo();\n" + 'export function Secondary() {\n return Foo();\n}\n') + with GraphStore(tmp_path / 'graph.db') as store: + assert not full_build(root, store)['errors'] + story_path = story.as_posix() + result = analyze_changes(store, [story_path], changed_ranges={story_path: [(1, 6)]}, + repo_root=str(root)) + assert not [gap for gap in result['test_gaps'] if gap['file'].endswith('.stories.tsx')] diff --git a/tests/test_typescript_declaration_gaps.py b/tests/test_typescript_declaration_gaps.py new file mode 100644 index 000000000..bba75adfe --- /dev/null +++ b/tests/test_typescript_declaration_gaps.py @@ -0,0 +1,670 @@ +"""TypeScript interfaces and type aliases are not test gaps (issue #1109, PR #1112). + +They are stored as Class nodes but have no runtime behavior, so a changed +interface must not appear in test_gaps. + +The later tests pin the edges of that rule: where the declarations can sit +(TSX, ``.mts``/``.cts``/``.d.ts``, namespaces, ambient blocks, Vue and Svelte +``\n", + "vue", + ), + ( + "Comp.svelte", + "\n" + "
\n", + "svelte", + ), + ], +) +def test_single_file_component_typescript_declarations_are_tagged(path, source, language): + """Vue and Svelte relabel script nodes with their own language; the tag + is set while the block is parsed as TypeScript and must survive that.""" + classes = _classes(path, source) + assert classes["VProps"].language == language + assert classes["VProps"].extra["typescript_kind"] == "interface_declaration" + assert classes["VSize"].extra["typescript_kind"] == "type_alias_declaration" + assert "typescript_kind" not in classes["VC"].extra + + +# --------------------------------------------------------------------------- +# Real build and update path +# --------------------------------------------------------------------------- + +_WIDGET = ( + "export interface WidgetProps { title: string }\n" + "export type Size = { w: number };\n" + "export enum Tone { Light, Dark }\n" + "export function Widget({ title }: WidgetProps) {\n" + " return

{title}

;\n" + "}\n" + "export class Panel { render() { return 1; } }\n" +) +_WIDGET_EDITED = _WIDGET.replace("title: string }", "title: string; sub?: string }") +_RUNTIME_GAPS = {"Tone", "Widget", "Panel", "render"} +_DECLARATIONS = {"WidgetProps", "Size"} + + +class TestBuildAndUpdatePath: + """Parse -> SQLite -> analyze_changes, through full_build and + incremental_update, in a repository path with spaces and non-ASCII.""" + + @pytest.fixture + def repo(self, tmp_path): + root = tmp_path / "my repo \u00fc" + (root / "src dir").mkdir(parents=True) + (root / ".git").mkdir() + (root / "src dir" / "widget.tsx").write_text(_WIDGET, encoding="utf-8") + store = GraphStore(root / "graph.db") + try: + yield root, store + finally: + store.close() + + @staticmethod + def _build(root: Path, store: GraphStore) -> None: + with patch( + "code_review_graph.incremental.get_all_tracked_files", + return_value=["src dir/widget.tsx"], + ): + result = full_build(root, store) + assert result["errors"] == [] + + @staticmethod + def _widget_path(root: Path, store: GraphStore) -> str: + (path,) = [ + f for f in store.get_all_files() if f.endswith("widget.tsx") + ] + return path + + def _gaps(self, root: Path, store: GraphStore, ranges: bool = True) -> dict: + path = self._widget_path(root, store) + return analyze_changes( + store, + changed_files=[path], + changed_ranges={path: [(1, 7)]} if ranges else None, + ) + + def test_full_build_drops_declarations_but_keeps_runtime_gaps(self, repo): + root, store = repo + self._build(root, store) + result = self._gaps(root, store) + + assert {g["name"] for g in result["test_gaps"]} == _RUNTIME_GAPS + # Other analyses still see the declarations: they are changed nodes + # with a risk score, only the gap report leaves them out. + changed = {f["name"]: f for f in result["changed_functions"]} + assert _DECLARATIONS <= set(changed) + assert all("risk_score" in changed[n] for n in _DECLARATIONS) + assert f"{len(_RUNTIME_GAPS)} test gap(s)" in result["summary"] + untested_line = next( + line for line in result["summary"].splitlines() if "Untested:" in line + ) + assert not any(name in untested_line for name in _DECLARATIONS) + + def test_whole_file_fallback_also_drops_declarations(self, repo): + """No line ranges: every node of the changed file is a candidate.""" + root, store = repo + self._build(root, store) + result = self._gaps(root, store, ranges=False) + assert {g["name"] for g in result["test_gaps"]} == _RUNTIME_GAPS + + def test_change_touching_only_an_interface_reports_no_gap(self, repo): + root, store = repo + self._build(root, store) + path = self._widget_path(root, store) + result = analyze_changes( + store, changed_files=[path], changed_ranges={path: [(1, 2)]}, + ) + assert {f["name"] for f in result["changed_functions"]} == _DECLARATIONS + assert result["test_gaps"] == [] + + def test_graph_from_an_earlier_release_heals_when_the_file_is_reparsed(self, repo): + """A graph built before this change has no ``typescript_kind``. + + Until the file is parsed again its declarations are still reported + (the documented limitation). The normal edit -> update path reparses + the file, and from then on they drop out; running the update or a + full rebuild again changes nothing. + """ + root, store = repo + self._build(root, store) + path = self._widget_path(root, store) + for node in store.get_nodes_by_file(path): + if node.extra.get("typescript_kind"): + legacy = {k: v for k, v in node.extra.items() if k != "typescript_kind"} + store._conn.execute( + "UPDATE nodes SET extra = ? WHERE id = ?", + (json.dumps(legacy), node.id), + ) + store.commit() + + stale = {g["name"] for g in self._gaps(root, store)["test_gaps"]} + assert stale == _RUNTIME_GAPS | _DECLARATIONS + + (root / "src dir" / "widget.tsx").write_text(_WIDGET_EDITED, encoding="utf-8") + incremental_update(root, store, changed_files=["src dir/widget.tsx"]) + assert {g["name"] for g in self._gaps(root, store)["test_gaps"]} == _RUNTIME_GAPS + + incremental_update(root, store, changed_files=["src dir/widget.tsx"]) + assert {g["name"] for g in self._gaps(root, store)["test_gaps"]} == _RUNTIME_GAPS + + self._build(root, store) + assert {g["name"] for g in self._gaps(root, store)["test_gaps"]} == _RUNTIME_GAPS + + +# --------------------------------------------------------------------------- +# Name collisions, missing metadata and scale +# --------------------------------------------------------------------------- + + +class TestCollisionsAndScale: + def setup_method(self): + self.tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False) + self.tmp.close() + self.store = GraphStore(self.tmp.name) + + def teardown_method(self): + self.store.close() + Path(self.tmp.name).unlink(missing_ok=True) + + def _store_parsed(self, path: str, source: str) -> None: + nodes, _ = CodeParser().parse_bytes(Path(path), source.encode("utf-8")) + for node in nodes: + self.store.upsert_node(node, file_hash="abc") + self.store.commit() + + def _gap_names(self, path: str, start: int, end: int) -> set[str]: + result = analyze_changes( + self.store, + changed_files=[path], + changed_ranges={path: [(start, end)]}, + ) + return {g["name"] for g in result["test_gaps"]} + + def test_class_of_the_same_name_in_another_file_is_still_a_gap(self): + self._store_parsed("/r/types.ts", "export interface Widget { a: string }\n") + self._store_parsed("/r/widget.ts", "export class Widget { render() { return 1; } }\n") + + assert self._gap_names("/r/types.ts", 1, 1) == set() + assert self._gap_names("/r/widget.ts", 1, 1) == {"Widget", "render"} + + def test_interface_merged_into_a_class_keeps_the_class_methods_as_gaps(self): + """Declaration merging gives the class and the interface one + qualified name, so whichever comes last owns the stored node. The + class's methods are separate nodes and must stay gaps either way.""" + self._store_parsed( + "/r/after.ts", + "export class Emitter {\n" + " emit() { return 1; }\n" + "}\n" + "export interface Emitter {\n" + " on(e: string): this;\n" + "}\n", + ) + assert "emit" in self._gap_names("/r/after.ts", 1, 6) + + self._store_parsed( + "/r/before.ts", + "export interface Mixed {\n" + " on(e: string): this;\n" + "}\n" + "export class Mixed {\n" + " emit() { return 1; }\n" + "}\n", + ) + # The class is stored last here, so it keeps its own class-level gap. + assert self._gap_names("/r/before.ts", 1, 6) == {"Mixed", "emit"} + + def test_null_extra_from_an_old_row_is_still_a_gap(self): + self._store_parsed("/r/widget.ts", "export class Widget { render() { return 1; } }\n") + self.store._conn.execute( + "UPDATE nodes SET extra = NULL WHERE name = 'Widget'" + ) + self.store.commit() + assert self._gap_names("/r/widget.ts", 1, 1) == {"Widget", "render"} + + def test_many_declarations_in_one_file_leave_only_runtime_gaps(self): + # 402 changed nodes: under the default CRG_MAX_CHANGED_FUNCS cap of + # 500, which is applied before the gap filter. + lines = [] + for i in range(200): + lines.append(f"export interface I{i} {{ v{i}: number }}") + lines.append(f"export type T{i} = I{i} | null;") + lines.append("export class Big { run() { return 1; } }") + self._store_parsed("/r/big.ts", "\n".join(lines) + "\n") + + result = analyze_changes( + self.store, + changed_files=["/r/big.ts"], + changed_ranges={"/r/big.ts": [(1, len(lines))]}, + ) + assert {g["name"] for g in result["test_gaps"]} == {"Big", "run"} + assert len(result["changed_functions"]) == 402 + + +# --------------------------------------------------------------------------- +# A real Git diff through the MCP tool, a declaration changing kind across +# updates, declarations nested in other scopes, and types-only files +# --------------------------------------------------------------------------- + +_MULTILINE_WIDGET = ( + "export interface WidgetProps {\n" + " title: string;\n" + "}\n" + "export type Size = {\n" + " w: number;\n" + "};\n" + "export class Panel {\n" + " render() {\n" + " return 1;\n" + " }\n" + "}\n" +) + + +def _git(repo: Path, *args: str) -> None: + subprocess.run( + [ + "git", "-c", "user.email=test@example.com", "-c", "user.name=Test", + "-c", "commit.gpgsign=false", *args, + ], + cwd=repo, + check=True, + capture_output=True, + stdin=subprocess.DEVNULL, + text=True, + timeout=30, + ) + + +class TestRealDiffThroughTheTool: + """Build -> edit -> update -> ``detect_changes_func`` against a real Git + diff. The diff's line ranges fall inside multi-line declaration bodies, + so this pins the range-to-node mapping the reporter's MCP calls use, in + both detail levels.""" + + @pytest.fixture + def repo(self, tmp_path, monkeypatch): + monkeypatch.delenv("CRG_DATA_DIR", raising=False) + root = tmp_path / "ts repo \u00e9" + (root / "src").mkdir(parents=True) + _git(root, "init", "-q") + (root / "src" / "widget.tsx").write_text(_MULTILINE_WIDGET, encoding="utf-8") + _git(root, "add", "-A") + _git(root, "commit", "-q", "-m", "init") + store = GraphStore(get_db_path(root)) + try: + assert full_build(root, store)["errors"] == [] + finally: + store.close() + return root + + @staticmethod + def _edit_and_update(root: Path, old: str, new: str) -> None: + path = root / "src" / "widget.tsx" + text = path.read_text(encoding="utf-8") + assert old in text + path.write_text(text.replace(old, new), encoding="utf-8") + store = GraphStore(get_db_path(root)) + try: + incremental_update(root, store, changed_files=["src/widget.tsx"]) + finally: + store.close() + + def test_edits_inside_declaration_bodies_report_no_gap(self, repo): + self._edit_and_update(repo, "title: string;", "title: number;") + self._edit_and_update(repo, "w: number;", "w: string;") + + result = detect_changes_func(base="HEAD", repo_root=str(repo)) + assert result["status"] == "ok" + assert {f["name"] for f in result["changed_functions"]} == {"WidgetProps", "Size"} + assert result["test_gaps"] == [] + assert result["test_gaps_total"] == 0 + + minimal = detect_changes_func( + base="HEAD", repo_root=str(repo), detail_level="minimal", + ) + assert minimal["status"] == "ok" + assert minimal["test_gap_count"] == 0 + + def test_edit_inside_the_class_body_still_reports_its_gaps(self, repo): + self._edit_and_update(repo, "return 1;", "return 2;") + + result = detect_changes_func(base="HEAD", repo_root=str(repo)) + assert result["status"] == "ok" + assert {g["name"] for g in result["test_gaps"]} == {"Panel", "render"} + + minimal = detect_changes_func( + base="HEAD", repo_root=str(repo), detail_level="minimal", + ) + assert minimal["test_gap_count"] == 2 + + +def test_tag_follows_a_declaration_that_changes_kind_across_updates(tmp_path): + """An interface refactored into a class must become a gap on the next + update, and turning it back into a type must drop it again: the tag is + rewritten with the node, never left behind on it.""" + root = tmp_path / "repo" + root.mkdir() + (root / ".git").mkdir() + src = root / "shape.ts" + src.write_text("export interface Shape {\n area(): number;\n}\n", encoding="utf-8") + store = GraphStore(root / "graph.db") + try: + with patch( + "code_review_graph.incremental.get_all_tracked_files", + return_value=["shape.ts"], + ): + assert full_build(root, store)["errors"] == [] + + def gaps() -> set[str]: + (path,) = [f for f in store.get_all_files() if f.endswith("shape.ts")] + result = analyze_changes(store, changed_files=[path]) + return {g["name"] for g in result["test_gaps"]} + + assert gaps() == set() + + src.write_text( + "export class Shape {\n area() { return 1; }\n}\n", encoding="utf-8", + ) + incremental_update(root, store, changed_files=["shape.ts"]) + assert gaps() == {"Shape", "area"} + + src.write_text("export type Shape = {\n area(): number;\n};\n", encoding="utf-8") + incremental_update(root, store, changed_files=["shape.ts"]) + assert gaps() == set() + finally: + store.close() + + +class TestNestedAndTypesOnlyFiles: + def setup_method(self): + self.tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False) + self.tmp.close() + self.store = GraphStore(self.tmp.name) + + def teardown_method(self): + self.store.close() + Path(self.tmp.name).unlink(missing_ok=True) + + def _store_parsed(self, path: str, source: str) -> None: + nodes, _ = CodeParser().parse_bytes(Path(path), source.encode("utf-8")) + for node in nodes: + self.store.upsert_node(node, file_hash="abc") + self.store.commit() + + def test_declarations_local_to_a_function_drop_out_but_the_function_stays(self): + self._store_parsed( + "/r/outer.ts", + "export function outer() {\n" + " interface Local { a: number }\n" + " type Alias = Local | null;\n" + " return 1;\n" + "}\n", + ) + result = analyze_changes( + self.store, + changed_files=["/r/outer.ts"], + changed_ranges={"/r/outer.ts": [(2, 3)]}, + ) + assert {"Local", "Alias"} <= {f["name"] for f in result["changed_functions"]} + assert {g["name"] for g in result["test_gaps"]} == {"outer"} + + @pytest.mark.parametrize( + ("path", "source", "expected"), + [ + ( + "/r/global.d.ts", + "declare global {\n" + " interface Window { crg: string }\n" + "}\n" + "export type Id = string;\n" + "export interface Box { v: T }\n", + set(), + ), + ( + "/r/Setup.vue", + "\n" + "\n", + {"f"}, + ), + ( + "/r/Module.svelte", + "\n" + "
\n", + {"g"}, + ), + ], + ids=["d.ts", "vue-script-setup", "svelte-module-script"], + ) + def test_whole_file_change_reports_only_runtime_code(self, path, source, expected): + """Whole-file fallback over a types-only ``.d.ts`` and over Vue and + Svelte script variants: declarations are changed nodes but never + gaps, and the runtime function next to them still is.""" + self._store_parsed(path, source) + result = analyze_changes(self.store, changed_files=[path]) + assert result["changed_functions"], "the file produced no changed nodes" + assert {g["name"] for g in result["test_gaps"]} == expected diff --git a/tests/test_uninstall_strict_json.py b/tests/test_uninstall_strict_json.py new file mode 100644 index 000000000..a7b6f7dfe --- /dev/null +++ b/tests/test_uninstall_strict_json.py @@ -0,0 +1,385 @@ +"""Uninstall must leave strict JSON behind in every hook and MCP config it edits. + +Regression for issue #1068, fixed by PR #1078. Clients such as Claude Code read +``settings.json`` as strict JSON and skip the whole file on a trailing comma, +which silently disables the user's own hooks and permission rules. Uninstall +removes several sibling entries in one splice, so these tests seed the user's +own settings, run the real installer and uninstaller, and read the result back +with plain ``json.loads`` rather than the tolerant JSONC reader. +""" + +from __future__ import annotations + +import copy +import json +import sys +from pathlib import Path +from typing import Any, Callable +from unittest.mock import patch + +import pytest + +from code_review_graph import skills, uninstall + +_USER_PERMISSIONS = {"allow": ["Bash(npm test)"], "deny": ["Bash(git push:*)"]} +_USER_PRE_TOOL = [{ + "matcher": "Bash", + "hooks": [{"type": "command", "command": "echo MY-OWN-HOOK"}], +}] + + +@pytest.fixture +def fake_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + home = tmp_path / "home dir ü" + home.mkdir() + monkeypatch.setattr(Path, "home", lambda: home) + monkeypatch.setenv("HERMES_HOME", str(home / ".hermes")) + return home + + +@pytest.fixture +def fake_repo(tmp_path: Path) -> Path: + repo = tmp_path / "repo with spaces ü" + (repo / ".git" / "hooks").mkdir(parents=True) + return repo + + +def _strict(path: Path) -> Any: + """Parse exactly as a strict-JSON client would.""" + return json.loads(path.read_text(encoding="utf-8")) + + +def _write_json(path: Path, value: object) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(value, indent=2) + "\n", encoding="utf-8") + + +def _claude(repo: Path, home: Path) -> tuple[Path, Callable[[], object]]: + return repo / ".claude" / "settings.json", lambda: skills.install_hooks(repo) + + +def _qoder(repo: Path, home: Path) -> tuple[Path, Callable[[], object]]: + return ( + repo / ".qoder" / "settings.json", + lambda: skills.install_hooks(repo, platform="qoder"), + ) + + +def _codebuddy(repo: Path, home: Path) -> tuple[Path, Callable[[], object]]: + return ( + repo / ".codebuddy" / "settings.json", + lambda: skills.install_codebuddy_hooks(repo), + ) + + +def _codex(repo: Path, home: Path) -> tuple[Path, Callable[[], object]]: + return home / ".codex" / "hooks.json", lambda: skills.install_codex_hooks(repo) + + +def _gemini(repo: Path, home: Path) -> tuple[Path, Callable[[], object]]: + return ( + repo / ".gemini" / "settings.json", + lambda: skills.install_gemini_cli_hooks(repo), + ) + + +_NESTED_TARGETS = { + "claude": _claude, + "qoder": _qoder, + "codebuddy": _codebuddy, + "codex": _codex, + "gemini": _gemini, +} + + +def _nested_original(same_events: bool) -> dict[str, Any]: + hooks: dict[str, Any] = {"PreToolUse": copy.deepcopy(_USER_PRE_TOOL)} + if same_events: + # The user already listens on the events the installer also uses, so + # removal happens inside existing arrays rather than whole members. + for event in ("PostToolUse", "SessionStart", "AfterTool"): + hooks[event] = [{ + "matcher": "Edit", + "hooks": [{"type": "command", "command": f"echo user-{event}"}], + }] + return {"permissions": copy.deepcopy(_USER_PERMISSIONS), "hooks": hooks} + + +@pytest.mark.parametrize("same_events", [False, True], ids=["own-events", "shared-events"]) +@pytest.mark.parametrize("target", sorted(_NESTED_TARGETS)) +def test_install_uninstall_leaves_strict_json_with_user_hooks( + target: str, same_events: bool, fake_repo: Path, fake_home: Path +) -> None: + settings, install = _NESTED_TARGETS[target](fake_repo, fake_home) + original = _nested_original(same_events) + _write_json(settings, original) + + install() + installed = _strict(settings) + assert installed != original, "installer did not add its hooks" + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert not report.errors + assert _strict(settings) == original + + +def test_cursor_flat_hooks_uninstall_leaves_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + hooks_json = fake_home / ".cursor" / "hooks.json" + original = { + "version": 1, + "hooks": {"beforeSubmitPrompt": [{"command": "./my-own-hook.sh"}]}, + } + _write_json(hooks_json, original) + + skills.install_cursor_hooks() + assert set(_strict(hooks_json)["hooks"]) > {"beforeSubmitPrompt"} + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert not report.errors + assert _strict(hooks_json) == original + + +def test_continue_duplicate_registrations_at_the_end_leave_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + config = fake_home / ".continue" / "config.json" + other = {"name": "other", "command": "other-server"} + # Earlier releases could stack several registrations side by side. + _write_json(config, { + "models": [{"title": "mine"}], + "mcpServers": [ + other, + {"name": "code-review-graph", "command": "code-review-graph"}, + {"name": "code-review-graph", "command": "uvx"}, + ], + }) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert not report.errors + assert _strict(config) == {"models": [{"title": "mine"}], "mcpServers": [other]} + + +def test_object_mcp_entry_registered_last_leaves_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + config = fake_repo / ".mcp.json" + _write_json(config, {"mcpServers": { + "other": {"command": "other"}, + "code-review-graph": {"command": "code-review-graph"}, + }}) + + uninstall.run(repo=fake_repo, keep_data=True) + + assert _strict(config) == {"mcpServers": {"other": {"command": "other"}}} + + +def test_hooks_written_by_an_earlier_release_leave_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + settings = fake_repo / ".claude" / "settings.json" + legacy = sorted(uninstall._legacy_repo_hook_commands(fake_repo)) + original = {"permissions": _USER_PERMISSIONS, "hooks": {"PreToolUse": _USER_PRE_TOOL}} + seeded = copy.deepcopy(original) + seeded["hooks"]["PostToolUse"] = [ + {"matcher": "Edit|Write", "hooks": [{"type": "command", "command": legacy[1]}]}, + ] + seeded["hooks"]["SessionStart"] = [ + {"matcher": "", "hooks": [{"type": "command", "command": legacy[0]}]}, + ] + _write_json(settings, seeded) + + uninstall.run(repo=fake_repo, keep_data=True) + + assert _strict(settings) == original + + +def test_owned_hooks_appended_inside_a_user_group_leave_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + settings = fake_repo / ".claude" / "settings.json" + owned = [ + hook["command"] + for entries in skills.generate_hooks_config(fake_repo)["hooks"].values() + for entry in entries + for hook in entry["hooks"] + ] + user_hook = {"type": "command", "command": "echo mine"} + _write_json(settings, { + "permissions": _USER_PERMISSIONS, + "hooks": {"PostToolUse": [{ + "matcher": "Edit|Write", + "hooks": [user_hook] + [{"type": "command", "command": cmd} for cmd in owned], + }]}, + }) + + uninstall.run(repo=fake_repo, keep_data=True) + + assert _strict(settings) == { + "permissions": _USER_PERMISSIONS, + "hooks": {"PostToolUse": [{"matcher": "Edit|Write", "hooks": [user_hook]}]}, + } + + +def test_second_uninstall_and_reinstall_cycles_stay_strict( + fake_repo: Path, fake_home: Path +) -> None: + settings = fake_repo / ".claude" / "settings.json" + original = _nested_original(same_events=False) + _write_json(settings, original) + + for _ in range(2): + skills.install_hooks(fake_repo) + uninstall.run(repo=fake_repo, keep_data=True) + assert _strict(settings) == original + settled = settings.read_bytes() + uninstall.run(repo=fake_repo, keep_data=True) + assert settings.read_bytes() == settled + + +def test_dry_run_leaves_the_installed_settings_byte_for_byte( + fake_repo: Path, fake_home: Path +) -> None: + settings = fake_repo / ".claude" / "settings.json" + _write_json(settings, _nested_original(same_events=False)) + skills.install_hooks(fake_repo) + before = settings.read_bytes() + + uninstall.run(repo=fake_repo, keep_data=True, dry_run=True) + + assert settings.read_bytes() == before + + +def test_cli_install_then_uninstall_reproduction_from_issue_1068( + fake_repo: Path, fake_home: Path, capsys: pytest.CaptureFixture[str] +) -> None: + from code_review_graph import cli + + settings = fake_repo / ".claude" / "settings.json" + settings.parent.mkdir(parents=True) + # Byte-for-byte the file from the issue's reproduction steps. + settings.write_text( + '{ "permissions": { "allow": ["Bash(npm test)"], "deny": ["Bash(git push:*)"] },\n' + ' "hooks": { "PreToolUse": [ { "matcher": "Bash", "hooks": [ { "type": ' + '"command", "command": "echo MY-OWN-HOOK" } ] } ] } }\n', + encoding="utf-8", + ) + original = _strict(settings) + + argv = ["code-review-graph", "install", "--repo", str(fake_repo), "-y", + "--platform", "claude-code"] + with patch.object(sys, "argv", argv): + cli.main() + assert {"PostToolUse", "SessionStart"} <= set(_strict(settings)["hooks"]) + + argv = ["code-review-graph", "uninstall", "--repo", str(fake_repo), "-y"] + with patch.object(sys, "argv", argv): + cli.main() + capsys.readouterr() + + assert _strict(settings) == original + + +# --- Audit additions for PR #1078 ---------------------------------------------- + +_GEMINI_USER_SETTINGS = { + "theme": "GitHub", + "mcpServers": {"other": {"command": "other-server"}}, + "hooks": {"BeforeTool": [{ + "matcher": "run_shell_command", + "hooks": [{"type": "command", "command": "echo MY-OWN-HOOK"}], + }]}, +} + + +def _comment_free(text: str) -> str: + """Drop comments but keep every comma, as Gemini CLI's settings loader does.""" + from code_review_graph import jsonc + + return " ".join(text[token.start:token.end] for token in jsonc.tokenize(text)) + + +def test_cli_gemini_install_then_uninstall_keeps_shared_settings_strict( + fake_repo: Path, fake_home: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Gemini keeps the MCP entry and the hooks in one file; both edits must stay strict.""" + from code_review_graph import cli + + settings = fake_repo / ".gemini" / "settings.json" + _write_json(settings, _GEMINI_USER_SETTINGS) + + argv = ["code-review-graph", "install", "--repo", str(fake_repo), "-y", + "--platform", "gemini-cli"] + with patch.object(sys, "argv", argv): + cli.main() + installed = _strict(settings) + assert "code-review-graph" in installed["mcpServers"] + assert {"SessionStart", "AfterTool"} <= set(installed["hooks"]) + + argv = ["code-review-graph", "uninstall", "--repo", str(fake_repo), "-y"] + with patch.object(sys, "argv", argv): + cli.main() + capsys.readouterr() + + assert _strict(settings) == _GEMINI_USER_SETTINGS + + +def test_gemini_settings_commented_after_install_stay_strict_without_comments( + fake_repo: Path, fake_home: Path +) -> None: + """Gemini CLI strips comments and then parses strictly, so commas still matter.""" + settings = fake_repo / ".gemini" / "settings.json" + _write_json(settings, _GEMINI_USER_SETTINGS) + skills.install_gemini_cli_hooks(fake_repo) + + # The user annotates the installed file, including right after the comma + # that uninstall exposes when it drops the trailing crg events. + text = settings.read_text(encoding="utf-8") + text = text.replace("{\n", "{\n // my Gemini settings\n", 1) + text = text.replace(' "hooks": {', ' /* hooks below */\n "hooks": {', 1) + assert text.count(' ],\n "SessionStart"') == 1 + text = text.replace(' ],\n "SessionStart"', ' ], // mine\n "SessionStart"') + settings.write_text(text, encoding="utf-8") + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert not report.errors + raw = settings.read_text(encoding="utf-8") + for comment in ("// my Gemini settings", "/* hooks below */", "// mine"): + assert comment in raw + assert json.loads(_comment_free(raw)) == _GEMINI_USER_SETTINGS + + +def test_owned_group_after_a_user_group_holding_owned_hooks_leaves_strict_json( + fake_repo: Path, fake_home: Path +) -> None: + """Nested removals inside one group plus removal of the next group, in one splice.""" + settings = fake_repo / ".claude" / "settings.json" + owned = sorted({ + hook["command"] + for entries in skills.generate_hooks_config(fake_repo)["hooks"].values() + for entry in entries + for hook in entry["hooks"] + }) + user_hook = {"type": "command", "command": "echo mine"} + original = { + "hooks": {"PostToolUse": [{"matcher": "Edit|Write", "hooks": [user_hook]}]}, + "permissions": _USER_PERMISSIONS, + } + seeded = copy.deepcopy(original) + seeded["hooks"]["PostToolUse"][0]["hooks"].extend( + {"type": "command", "command": command} for command in owned + ) + seeded["hooks"]["PostToolUse"].append( + {"matcher": "", "hooks": [{"type": "command", "command": owned[0]}]} + ) + _write_json(settings, seeded) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert not report.errors + assert _strict(settings) == original diff --git a/tests/test_watch_refresh_without_index.py b/tests/test_watch_refresh_without_index.py new file mode 100644 index 000000000..59b464d5a --- /dev/null +++ b/tests/test_watch_refresh_without_index.py @@ -0,0 +1,102 @@ +"""Watch keeps running when an explicit embedding refresh finds no index (#1070). + +Watch fails closed on post-processing warnings. The one exception is the +notice that a requested refresh had no index to refresh: the graph update +succeeded, so stopping the watch over it would only stop graph updates. +""" + +from unittest.mock import patch + +import pytest +from watchdog.events import FileCreatedEvent + +from code_review_graph.embeddings import REFRESH_SKIPPED_WARNING +from code_review_graph.graph import GraphStore +from code_review_graph.incremental import ( + _create_watch_handler, + _raise_watch_postprocess_warnings, + incremental_update, + watch, +) +from code_review_graph.postprocessing import run_post_processing + + +def _recording_refresh(results: list[dict]): + """The CLI's watch callback for an explicit refresh pair, keeping each result.""" + + def refresh(store: GraphStore) -> dict: + result = run_post_processing( + store, + embedding_provider="local", + embedding_model="test-model", + ) + results.append(result) + return result + + return refresh + + +def test_startup_reconciliation_keeps_the_watch_running(tmp_path): + deleted = tmp_path / "offline.py" + deleted.write_text("def offline():\n pass\n") + store = GraphStore(tmp_path / "graph.db") + incremental_update(tmp_path, store, changed_files=["offline.py"]) + deleted.unlink() + results: list[dict] = [] + try: + with ( + patch("code_review_graph.embeddings.get_provider") as get_provider, + patch("watchdog.observers.Observer") as observer, + patch("time.sleep", side_effect=KeyboardInterrupt), + ): + watch(tmp_path, store, on_files_updated=_recording_refresh(results)) + get_provider.assert_not_called() + assert [result.get("embeddings_refresh_skipped") for result in results] == [True] + observer.return_value.start.assert_called_once() + finally: + store.close() + + +def test_watch_update_keeps_the_watch_running(tmp_path): + source = tmp_path / "source.py" + source.write_text("def source():\n pass\n") + store = GraphStore(tmp_path / "graph.db") + results: list[dict] = [] + handler = _create_watch_handler(tmp_path, store, _recording_refresh(results)) + try: + with patch("code_review_graph.embeddings.get_provider") as get_provider: + handler.process([FileCreatedEvent(str(source))]) + handler.raise_if_failed() + get_provider.assert_not_called() + assert [result.get("embeddings_refresh_skipped") for result in results] == [True] + assert len(store.get_all_files()) == 1 + finally: + store.close() + + +def test_the_refresh_notice_alone_is_not_a_failure(): + _raise_watch_postprocess_warnings( + {"embeddings_refresh_skipped": True, "warnings": [REFRESH_SKIPPED_WARNING]}, + ) + + +@pytest.mark.parametrize( + ("warnings", "failure"), + [ + # A step failure in the same batch as the notice. + ( + [REFRESH_SKIPPED_WARNING, "FTS index rebuild failed: OperationalError: forced"], + "FTS index rebuild failed", + ), + # A refresh against an existing index that the provider refuses. + ( + ["Embedding refresh failed: ValueError: Embedding refresh refused"], + "Embedding refresh failed", + ), + ], +) +def test_any_other_warning_still_fails_the_update(warnings, failure): + with pytest.raises(RuntimeError, match="post-processing reported warnings") as exc_info: + _raise_watch_postprocess_warnings({"warnings": warnings}) + assert failure in str(exc_info.value) + assert REFRESH_SKIPPED_WARNING not in str(exc_info.value)